Consul: Using vertical bar in node name messes up consul members pretty output

[lawrence-c]

Description of the Issue

Created consul agent with node name: test | 2D33AE (everything works fine), but when running consul members, the 2D33AE is pushed into the Address Column, and everything else is shifted one a long.

EG:

Node                 Address           Status            Type    Build   Protocol  DC            Segment
extra-mac            <redacted>        alive             client  1.0.6   2         dc1           <default>
test-win7            <redacted>        left              client  1.0.6   2         dc1           <default>
test-win7            2D33AE            <redacted>        alive   client  1.0.6     2             dc1           <default>

consul version for both Client and Server

Client: [1.0.6]
Server: [1.0.6]

Operating system and Environment details

Tested on macOS 10.12 and Ubuntu 16.04

[pearkes]

Interesting! We may need to filter or validate from the input or output here to avoid this. I'm going to tag this as a bug, and as a "good first issue" as we would happily accept a PR. I am also adding the security tag as there could be a way to craft output here that is related.

[shireeshaBongarala]

For pretty-printing, the column values are first appended with | Link. The string split and formatted with the columnize library, which uses strings.split function. I tried to escape the character | with \| and \| in strings.split but it doesn't seem to work.
To fix this,

1. Use another delimeter for doing pretty-printing.
2. Replace the character | from the name with empty/hyphen character before splitting.
   Any ideas or suggestions?
   I would be happy to raise a PR.

[freddygv]

@shireeshaBongarala It would be great if you could submit a PR here.

There a two options we would suggest:

1. Using another delimiter. Rather than using a | we could insert a more complex (and rare) delimiter such as |#_#| and then pass that in a config struct to columnize.Format().

1. Replacing ryanuber/columnize so that we handle the columnizing internally. This way instead of creating strings delimited by | in standardOutput and detailedOutput we could create a slice of strings and then measure the length of each field ourselves as the columnize library does.

The second option is preferable to the first since it would avoid using delimiters altogether. However, it is a larger commitment so feel free to start wherever you feel comfortable.

Looking forward to your contributions to Consul and let me know if you have any other questions!

[freddygv]

@SurferL We suggest using the characters - and . instead of | in node names.

As specified in RFC-952 and RFC-1123, hostnames should only include ASCII letters A-Z, digits 0-9 and symbols - and ..

Using invalid characters and whitespaces prevents that node from being discovered via DNS (though it can still be discovered via the HTTP API).

[shireeshaBongarala]

@freddygv Thanks for the response. I want to add some tests to the existing code before making any changes. I raised this PR https://github.com/hashicorp/consul/pull/4621 with a sample test case. Please let me know the feedback on it. There are many paths that can be covered. I can work on them as well.