From 827c5d9010163e9627b80af6df0e8e47e3cdf961 Mon Sep 17 00:00:00 2001 From: Alex Dzyoba Date: Mon, 9 Mar 2020 10:24:56 +0000 Subject: [PATCH] command: change delim in columnize to funny node names (#6652) When node name contains vertical bar symbol some commands output is garbled because `|` is used as a delimiter in `columnize.SimpleFormat`. This commit changes format string to use `\x1f` - ASCII unit separator[1] as a delimiter and also adds test to cover this case. Affected commands: * `consul catalog nodes` * `consul members` * `consul operator raft list-peers` * `consul intention get` Fixes #3951. [1]: https://en.wikipedia.org/wiki/Delimiter#Solutions --- .../catalog/list/nodes/catalog_list_nodes.go | 10 ++--- .../list/nodes/catalog_list_nodes_test.go | 26 +++++++++++++ command/intention/get/get.go | 16 ++++---- command/intention/get/get_test.go | 37 +++++++++++++++++++ command/members/members.go | 14 +++---- command/members/members_test.go | 26 +++++++++++++ .../raft/listpeers/operator_raft_list.go | 6 +-- .../raft/listpeers/operator_raft_list_test.go | 24 ++++++++++++ 8 files changed, 136 insertions(+), 23 deletions(-) diff --git a/command/catalog/list/nodes/catalog_list_nodes.go b/command/catalog/list/nodes/catalog_list_nodes.go index 8b67e4f7f..dbe36dbf9 100644 --- a/command/catalog/list/nodes/catalog_list_nodes.go +++ b/command/catalog/list/nodes/catalog_list_nodes.go @@ -147,16 +147,16 @@ func printNodes(nodes []*api.Node, detailed bool) (string, error) { result = simpleNodes(nodes) } - return columnize.SimpleFormat(result), nil + return columnize.Format(result, &columnize.Config{Delim: string([]byte{0x1f})}), nil } func detailedNodes(nodes []*api.Node) []string { result := make([]string, 0, len(nodes)+1) - header := "Node|ID|Address|DC|TaggedAddresses|Meta" + header := "Node\x1fID\x1fAddress\x1fDC\x1fTaggedAddresses\x1fMeta" result = append(result, header) for _, node := range nodes { - result = append(result, fmt.Sprintf("%s|%s|%s|%s|%s|%s", + result = append(result, fmt.Sprintf("%s\x1f%s\x1f%s\x1f%s\x1f%s\x1f%s", node.Node, node.ID, node.Address, node.Datacenter, mapToKV(node.TaggedAddresses, ", "), mapToKV(node.Meta, ", "))) } @@ -166,7 +166,7 @@ func detailedNodes(nodes []*api.Node) []string { func simpleNodes(nodes []*api.Node) []string { result := make([]string, 0, len(nodes)+1) - header := "Node|ID|Address|DC" + header := "Node\x1fID\x1fAddress\x1fDC" result = append(result, header) for _, node := range nodes { @@ -176,7 +176,7 @@ func simpleNodes(nodes []*api.Node) []string { if idx > 0 { id = id[0:idx] } - result = append(result, fmt.Sprintf("%s|%s|%s|%s", + result = append(result, fmt.Sprintf("%s\x1f%s\x1f%s\x1f%s", node.Node, id, node.Address, node.Datacenter)) } diff --git a/command/catalog/list/nodes/catalog_list_nodes_test.go b/command/catalog/list/nodes/catalog_list_nodes_test.go index b738c3495..bbb280d2f 100644 --- a/command/catalog/list/nodes/catalog_list_nodes_test.go +++ b/command/catalog/list/nodes/catalog_list_nodes_test.go @@ -163,3 +163,29 @@ func TestCatalogListNodesCommand(t *testing.T) { } }) } + +func TestCatalogListNodesCommand_verticalBar(t *testing.T) { + t.Parallel() + + nodeName := "name|with|bars" + a := agent.NewTestAgent(t, "", `node_name = "`+nodeName+`"`) + defer a.Shutdown() + + ui := cli.NewMockUi() + c := New(ui) + c.flags.SetOutput(ui.ErrorWriter) + + args := []string{ + "-http-addr=" + a.HTTPAddr(), + } + + code := c.Run(args) + if code != 0 { + t.Fatalf("bad exit code: %d. %q", code, ui.ErrorWriter.String()) + } + + // Check for nodeName presense because it should not be parsed by columnize + if !strings.Contains(ui.OutputWriter.String(), nodeName) { + t.Fatalf("expected %q to contain %q", ui.OutputWriter.String(), nodeName) + } +} diff --git a/command/intention/get/get.go b/command/intention/get/get.go index bfe4c754c..cfe717a02 100644 --- a/command/intention/get/get.go +++ b/command/intention/get/get.go @@ -66,13 +66,13 @@ func (c *cmd) Run(args []string) int { // Format the tabular data data := []string{ - fmt.Sprintf("Source:|%s", ixn.SourceString()), - fmt.Sprintf("Destination:|%s", ixn.DestinationString()), - fmt.Sprintf("Action:|%s", ixn.Action), - fmt.Sprintf("ID:|%s", ixn.ID), + fmt.Sprintf("Source:\x1f%s", ixn.SourceString()), + fmt.Sprintf("Destination:\x1f%s", ixn.DestinationString()), + fmt.Sprintf("Action:\x1f%s", ixn.Action), + fmt.Sprintf("ID:\x1f%s", ixn.ID), } if v := ixn.Description; v != "" { - data = append(data, fmt.Sprintf("Description:|%s", v)) + data = append(data, fmt.Sprintf("Description:\x1f%s", v)) } if len(ixn.Meta) > 0 { var keys []string @@ -81,14 +81,14 @@ func (c *cmd) Run(args []string) int { } sort.Strings(keys) for _, k := range keys { - data = append(data, fmt.Sprintf("Meta[%s]:|%s", k, ixn.Meta[k])) + data = append(data, fmt.Sprintf("Meta[%s]:\x1f%s", k, ixn.Meta[k])) } } data = append(data, - fmt.Sprintf("Created At:|%s", ixn.CreatedAt.Local().Format(time.RFC850)), + fmt.Sprintf("Created At:\x1f%s", ixn.CreatedAt.Local().Format(time.RFC850)), ) - c.UI.Output(columnize.SimpleFormat(data)) + c.UI.Output(columnize.Format(data, &columnize.Config{Delim: string([]byte{0x1f})})) return 0 } diff --git a/command/intention/get/get_test.go b/command/intention/get/get_test.go index 822d3a16e..96827acfc 100644 --- a/command/intention/get/get_test.go +++ b/command/intention/get/get_test.go @@ -122,3 +122,40 @@ func TestCommand_srcDst(t *testing.T) { require.Equal(0, c.Run(args), ui.ErrorWriter.String()) require.Contains(ui.OutputWriter.String(), id) } + +func TestCommand_verticalBar(t *testing.T) { + t.Parallel() + + require := require.New(t) + a := agent.NewTestAgent(t, t.Name(), ``) + defer a.Shutdown() + client := a.Client() + + sourceName := "source|name|with|bars" + + // Create the intention + var id string + { + var err error + id, _, err = client.Connect().IntentionCreate(&api.Intention{ + SourceName: sourceName, + DestinationName: "db", + Action: api.IntentionActionAllow, + }, nil) + require.NoError(err) + } + + // Get it + ui := cli.NewMockUi() + c := New(ui) + + args := []string{ + "-http-addr=" + a.HTTPAddr(), + id, + } + require.Equal(0, c.Run(args), ui.ErrorWriter.String()) + + // Check for sourceName presense because it should not be parsed by + // columnize + require.Contains(ui.OutputWriter.String(), sourceName) +} diff --git a/command/members/members.go b/command/members/members.go index 6def2231d..c76ae45d7 100644 --- a/command/members/members.go +++ b/command/members/members.go @@ -120,7 +120,7 @@ func (c *cmd) Run(args []string) int { } // Generate the columnized version - output := columnize.SimpleFormat(result) + output := columnize.Format(result, &columnize.Config{Delim: string([]byte{0x1f})}) c.UI.Output(output) return 0 @@ -146,7 +146,7 @@ func (m ByMemberNameAndSegment) Less(i, j int) bool { // in a more human-friendly format func (c *cmd) standardOutput(members []*consulapi.AgentMember) []string { result := make([]string, 0, len(members)) - header := "Node|Address|Status|Type|Build|Protocol|DC|Segment" + header := "Node\x1fAddress\x1fStatus\x1fType\x1fBuild\x1fProtocol\x1fDC\x1fSegment" result = append(result, header) for _, member := range members { addr := net.TCPAddr{IP: net.ParseIP(member.Addr), Port: int(member.Port)} @@ -163,15 +163,15 @@ func (c *cmd) standardOutput(members []*consulapi.AgentMember) []string { statusString := serf.MemberStatus(member.Status).String() switch member.Tags["role"] { case "node": - line := fmt.Sprintf("%s|%s|%s|client|%s|%s|%s|%s", + line := fmt.Sprintf("%s\x1f%s\x1f%s\x1fclient\x1f%s\x1f%s\x1f%s\x1f%s", member.Name, addr.String(), statusString, build, protocol, dc, segment) result = append(result, line) case "consul": - line := fmt.Sprintf("%s|%s|%s|server|%s|%s|%s|%s", + line := fmt.Sprintf("%s\x1f%s\x1f%s\x1fserver\x1f%s\x1f%s\x1f%s\x1f%s", member.Name, addr.String(), statusString, build, protocol, dc, segment) result = append(result, line) default: - line := fmt.Sprintf("%s|%s|%s|unknown||||", + line := fmt.Sprintf("%s\x1f%s\x1f%s\x1funknown\x1f\x1f\x1f\x1f", member.Name, addr.String(), statusString) result = append(result, line) } @@ -183,7 +183,7 @@ func (c *cmd) standardOutput(members []*consulapi.AgentMember) []string { // their raw format func (c *cmd) detailedOutput(members []*consulapi.AgentMember) []string { result := make([]string, 0, len(members)) - header := "Node|Address|Status|Tags" + header := "Node\x1fAddress\x1fStatus\x1fTags" result = append(result, header) for _, member := range members { // Get the tags sorted by key @@ -202,7 +202,7 @@ func (c *cmd) detailedOutput(members []*consulapi.AgentMember) []string { tags := strings.Join(tagPairs, ",") addr := net.TCPAddr{IP: net.ParseIP(member.Addr), Port: int(member.Port)} - line := fmt.Sprintf("%s|%s|%s|%s", + line := fmt.Sprintf("%s\x1f%s\x1f%s\x1f%s", member.Name, addr.String(), serf.MemberStatus(member.Status).String(), tags) result = append(result, line) } diff --git a/command/members/members_test.go b/command/members/members_test.go index 18712c63d..21ec8378c 100644 --- a/command/members/members_test.go +++ b/command/members/members_test.go @@ -120,3 +120,29 @@ func TestMembersCommand_statusFilter_failed(t *testing.T) { t.Fatalf("bad: %d", code) } } + +func TestMembersCommand_verticalBar(t *testing.T) { + t.Parallel() + + nodeName := "name|with|bars" + a := agent.NewTestAgent(t, "", `node_name = "`+nodeName+`"`) + defer a.Shutdown() + + ui := cli.NewMockUi() + c := New(ui) + c.flags.SetOutput(ui.ErrorWriter) + + args := []string{ + "-http-addr=" + a.HTTPAddr(), + } + + code := c.Run(args) + if code == 1 { + t.Fatalf("bad: %d. %#v", code, ui.ErrorWriter.String()) + } + + // Check for nodeName presense because it should not be parsed by columnize + if !strings.Contains(ui.OutputWriter.String(), nodeName) { + t.Fatalf("bad: %#v", ui.OutputWriter.String()) + } +} diff --git a/command/operator/raft/listpeers/operator_raft_list.go b/command/operator/raft/listpeers/operator_raft_list.go index ae5a39868..98934d8d0 100644 --- a/command/operator/raft/listpeers/operator_raft_list.go +++ b/command/operator/raft/listpeers/operator_raft_list.go @@ -68,7 +68,7 @@ func raftListPeers(client *api.Client, stale bool) (string, error) { } // Format it as a nice table. - result := []string{"Node|ID|Address|State|Voter|RaftProtocol"} + result := []string{"Node\x1fID\x1fAddress\x1fState\x1fVoter\x1fRaftProtocol"} for _, s := range reply.Servers { raftProtocol := s.ProtocolVersion @@ -79,11 +79,11 @@ func raftListPeers(client *api.Client, stale bool) (string, error) { if s.Leader { state = "leader" } - result = append(result, fmt.Sprintf("%s|%s|%s|%s|%v|%s", + result = append(result, fmt.Sprintf("%s\x1f%s\x1f%s\x1f%s\x1f%v\x1f%s", s.Node, s.ID, s.Address, state, s.Voter, raftProtocol)) } - return columnize.SimpleFormat(result), nil + return columnize.Format(result, &columnize.Config{Delim: string([]byte{0x1f})}), nil } func (c *cmd) Synopsis() string { diff --git a/command/operator/raft/listpeers/operator_raft_list_test.go b/command/operator/raft/listpeers/operator_raft_list_test.go index 336f2f2a6..414c776ea 100644 --- a/command/operator/raft/listpeers/operator_raft_list_test.go +++ b/command/operator/raft/listpeers/operator_raft_list_test.go @@ -38,3 +38,27 @@ func TestOperatorRaftListPeersCommand(t *testing.T) { t.Fatalf("bad: %q, %q", output, expected) } } + +func TestOperatorRaftListPeersCommand_verticalBar(t *testing.T) { + t.Parallel() + + nodeName := "name|with|bars" + a := agent.NewTestAgent(t, "", `node_name = "`+nodeName+`"`) + defer a.Shutdown() + + ui := cli.NewMockUi() + c := New(ui) + c.flags.SetOutput(ui.ErrorWriter) + + args := []string{"-http-addr=" + a.HTTPAddr()} + + code := c.Run(args) + if code != 0 { + t.Fatalf("bad exit code: %d. %q", code, ui.ErrorWriter.String()) + } + + // Check for nodeName presense because it should not be parsed by columnize + if !strings.Contains(ui.OutputWriter.String(), nodeName) { + t.Fatalf("expected %q to contain %q", ui.OutputWriter.String(), nodeName) + } +}