fix rule: fixed issue with alertmanager URL containing +

[FUSAKLA]

fixes #1564

• CHANGELOG entry if change is relevant to the end user.

Changes

• fixed issue when Alertmanager URL contains + which can be for example in credentials

Verification

• added tests which are passing
• tested to startup the rule with various URLs

[GiedriusS]

The code looks very good to me and well tested 💪 however a philosophical question/idea: maybe it would be worth to error out if more than one + has been found in the parsedUrl.Scheme variable considering that none of the dns.QType variables have a +?

[FUSAKLA]

Thanks! Yes, good point. The function handles just parsing not validation. The validation is done in the resolver package so it eventually errors out afterwards (below example). This way the rule does not necessarily know all the resolvers types and who knows, maybe there will be some QType with + 😄
But yeah, I'm ok with erroring out if you think it will be safer this way.

Example:

$ ./thanos rule --alertmanagers.url=foo+bar+http:user:pass+word@test.haha --query=http://test.dev
...
level=info ts=2019-09-28T13:12:36.055664442Z caller=rule.go:591 msg="starting rule node"
...
level=error ts=2019-09-28T13:12:36.055936546Z caller=rule.go:363 msg="refreshing alertmanagers failed" err="alertmanager resolve: invalid lookup scheme \"foo+bar\""

[GiedriusS]

Thanks! Yes, good point. The function handles just parsing not validation. The validation is done in the resolver package so it eventually errors out afterwards (below example). This way the rule does not necessarily know all the resolvers types and who knows, maybe there will be some QType with + smile
But yeah, I'm ok with erroring out if you think it will be safer this way.

Example:

$ ./thanos rule --alertmanagers.url=foo+bar+http:user:pass+word@test.haha --query=http://test.dev
...
level=info ts=2019-09-28T13:12:36.055664442Z caller=rule.go:591 msg="starting rule node"
...
level=error ts=2019-09-28T13:12:36.055936546Z caller=rule.go:363 msg="refreshing alertmanagers failed" err="alertmanager resolve: invalid lookup scheme \"foo+bar\""

Ah, I haven't checked. Then it should be all good, thanks for the example output.

[bwplotka]

Nice! Extremely small nit only =D then LGTM

[FUSAKLA]

@bwplotka fixed and rebased, thanks for noticing :))