config: warn the user if client_addr is empty #11461
Conversation
|
Linked to #5371. As a part of the review, we should determine whether this partially or fully addresses 5371. |
.changelog/11446.txt
Outdated
| @@ -0,0 +1,3 @@ | |||
| ```release-note:improvement | |||
There was a problem hiding this comment.
Thanks for adding the changelog!
Something to address later (once you get a review back from engineering): this changelog file should match the PR number (.changelog/11461). This currently overwrites a different (existing) changelog entry.
There was a problem hiding this comment.
Ops! I wanted to be compliant with the style/process and I think I overdid it!
OK, I am gonna remove it for now then and I'll wait for further instructions.
Thanks @jkirschner-hashicorp!
There was a problem hiding this comment.
You did the right thing! You just need to change the filename :) Should be .changelog/11461.txt, not .changelog/11446.txt.
There was a problem hiding this comment.
I currently have a PR open to update the contributing docs to make the changelog entry creation process clearer: #11391
I hope to get it merged soon!
There was a problem hiding this comment.
Mhh, not sure if config is the right code area 🤔, it's not a new config option even if the change affects config options. Thoughts?
There was a problem hiding this comment.
I think type feature and code area config would be used for a new config option. Type improvement area config feels appropriate here to me!
There was a problem hiding this comment.
Awesome, thanks @jkirschner-hashicorp
|
hey @deblasis thanks for putting this out! Chiming in to point out that the recent test failures are part of an expired test cert. We actually "fixed" this in the upstream. Would you mind rebasing on |
if the provided value is empty string then the client services (DNS, HTTP, HTTPS, GRPC) are not listening and the user is not notified in any way about what's happening. Also, since a not provided client_addr defaults to 127.0.0.1, we make sure we are not getting unwanted warnings Signed-off-by: Alessandro De Blasis <alex@deblasis.net>
deblasis
force-pushed
the
feature/empty_client_addr_warning
branch
from
a058f69
to
2238df6
Compare
|
Hi @FFMMM! No worries, my pleasure. I use consul everywhere, the minimum I can do is contribute a little! 😃 Anything else I can do please let me know |
There was a problem hiding this comment.
Looks good to merge - thanks for the PR, @deblasis !
In the future we could consider erroring outright in this case in order to make this more visible to the user. The only method we officially document for disabling an interface is by setting -1 for the port - passing an empty address like this is less explicit and could potentially happen by accident with config management, so it might be better not to allow it and make sure the user is intentionally disabling the interface via port = -1.
|
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/497846. |
if the provided value is an empty string then the client services
(DNS, HTTP, HTTPS, GRPC) are not listening and the user is not notified
in any way about what's happening.
Also, since a not provided client_addr defaults to 127.0.0.1, we make sure
we are not getting unwanted warnings
Signed-off-by: Alessandro De Blasis alex@deblasis.net