-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
config: warn the user if client_addr is empty #11461
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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