Fix up EC2 go-discover docs and possibly change escaping behavior

[slackpad]

Capturing the conversation from https://gitter.im/hashicorp-consul/Lobby?at=599f7b80ee5c9a4c5ff81fe4:

Hello, trying to upgrade Consul which requires a differently formatted "retry-join" syntax for ec2 auto-join. Documentation is inconsistent (secret_access_id vs secret_access_key). Using secret_access_id I get NoCredentialProviders and using secret_access_key I get AuthFailure. I've double-checked that the keys haven't changed and are the same in the working older agents, I've tried specifying the region with no effect.

Looks like a documentation issue. We should also make the URL unescaping behavior more prominent/clear.

[featheredtoast]

I just hit this as well when attempting to bring up a consul server cluster during testing, and thought my eyes were bugging out on me re: id vs key. URL escaping behavior was quite the gotcha as well - is there a way to quote the value directly in the array itself so poor end users wouldn't need to worry about escaping in the config?

[magiconair]

@featheredtoast I'm really sorry for the eye strain. In all fairness, the example in the next sentence was using the correct name. :)

Authentication & Precedence

Static credentials acesss_key_id=xxx secret_access_key=xxx

I'm not sure what you mean with your escaping question. The format of the retry-join config is key=val key=val ... and the values are URL decoded when being parsed. During my testing the only thing I had to do was to replace a space with a +, e.g. provider=aws tag_key=consul+server ... Can you explain the problem you had a bit more, please?

[magiconair]

Merged and updated the website.

[featheredtoast]

Sure thing - I had the same issue as was recorded in that gitter link. AWS keys may occasionally have slashes or plus characters. Naively copy/pasting these into a consul config doesn't work, as these characters need to be replaced with the URL encoded equivalents, which isn't super clear in the docs.

Eg a key like: kWcrlUX5JEDGM/LtmEENI/aVmYvHNif5zB+d9+ct will need to be typed in the consul config as secret_access_key=kWcrlUX5JEDGM%2FLtmEENI%2FaVmYvHNif5zB%2Bd9%2Bct in order to be read correctly.

I see glancing back at the docs, a single reference:

The values need to be URL encoded but for most practical purposes you need to replace spaces with + signs.

but I seemed to have missed this when parsing through. Perhaps an example making the URL encoded bit a more explicit would help?

[magiconair]

@featheredtoast I've added that reference today so you did not miss that.

The intended syntax is key=val key=val ... splitting k/v pairs on space. Therefore the only value that needs to be escaped is a space and we can just use the +. Using url.QueryUnescape looks like gold plating with no additional gain.

If nobody sees an edge case then I'd change the documentation to only replace space with +. I can also add examples for each provider so that it is more prominent.

[odysseus654]

Note that the password I was using contained a "+" which absolutely had to be %-escaped in the current model.

Saying that spaces are the only issue here is incomplete, at minimum all "+"s and "%"s need to be changed as well.

[odysseus654]

More specific:

space -> +

• -> %2B
  % -> %25

[magiconair]

@odysseus654 of course you're right. Sleep deprivation doesn't help.

However, the issue that have to encode this at all is bugging me a bit since you can't just copy and paste the values you got from the cloud providers and make it work.

I could write a custom parser which could handle this without encoding since the keys are known tokens and can become the separators. However, that would be a backwards incompatible change since + would change its meaning. Since this feature is new I'd rather fix this now before this becomes widespread.

Crude BNF attempt:

ws  ::= " " <ws> | " "
key ::= provider | region | tag_key | tag_value | access_key_id | secret_access_key
kv  ::= <key> "=" <text>
kvs ::= <kv> <ws> <kv> | <kv>

[odysseus654]

Is there a reason why we can't just use an object instead of an encoded string here?

[magiconair]

The main motivation was to have a single format that works for all providers so that we could move that code into a separate library and use it across multiple projects. (https://github.com/hashicorp/go-discover) Otherwise, each project would have to implement the argument parsing. Since -retry-join also allows for addresses and hostnames to be combined with auto-join you get a single []string.

Having said that, I could also provide a parser that can handle both strings and objects/maps. You could then write

retry_join : [
  "1.2.3.4",
  {"provider":"aws", "tag_key":"consul", "tag_value":"server"}
]

I'll chew on this a bit more.

[magiconair]

@odysseus654 The other argument for a string value is that you can then use it on the command line without introducing flags for all possible parameters. This allows us to have a unified interface for all current and future cloud providers we want to support without adding lots of flags.

[odysseus654]

FWIW I am grateful that retry_join takes an array now (and that retry_join_wan exists now, if I recognize what it does). I was coming close to having this need to scan multiple EC2 regions and I wasn't certain how to do it with the previous implementation.

[slackpad]

Opening this up again to capture potentially changing the escaping.

[magiconair]

I've come to the conclusion that using URL encoding for the values was a mistake. It was easy to implement and I only thought of escaping spaces with + signs. However, the more important part is usability which is now broken since the values need to be encoded before they can be used. This introduces another step, another source for mistakes and it isn't obvious. Users have to read the documentation to find this.

Therefore, I think we should replace this with a better solution which will be a breaking change for 1.0.

I see two options:

HCL

HCL would have the benefit of already being used in other places but it would require to quote all string values. provider=aws would become provider="aws". This would be ugly in a JSON or HCL config file since HCL does not support single quoted strings.

JSON:  {"retry_join": ["provider=\"aws\" tag_name=\"consul server\""]}
HCL:   retry_join = "provider=\"aws\" tag_name=\"consul server\""
Shell: -retry-join 'provider="aws" tag_name="consul server"'

Keep custom DSL format but change escaping rules

With a custom parser we can keep the current format and only fix the quoting and escaping rules. All keys and values are literals and cannot contain spaces. To use spaces you have to double-quote either key and/or value and standard escaping rules with \ apply.

JSON:  {"retry_join": ["provider=aws tag_name=\"consul server\""]}
HCL:   retry_join = "provider=aws tag_name=\"consul server\""
Shell: -retry-join 'provider=aws tag_name="consul server"'

Unquoting of keys and values would follow the same rules as strconv.Unquote for double-quoted strings. Single quoted and backquoted strings are not supported.

Since almost all keys and values will not contain spaces I suggest to stick with the custom DSL option with the changed escaping rules.