Skip to content
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

Bind the readiness service to the wildcard address #91329

Merged
merged 12 commits into from
Nov 16, 2022

Conversation

grcevski
Copy link
Contributor

@grcevski grcevski commented Nov 4, 2022

This change makes the readiness service bind itself to the wildcard host address, e.g 0.

Fixes #90997

This change allows for configuring the host binding
on the readiness service. If the host isn't
specified we bind on localhost, same as before.
@grcevski grcevski added >enhancement :Core/Infra/Core Core issues without another label Team:Core/Infra Meta label for core/infra team v8.6.0 labels Nov 4, 2022
@github-actions
Copy link

github-actions bot commented Nov 4, 2022

Documentation preview:

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@elasticsearchmachine
Copy link
Collaborator

Hi @grcevski, I've created a changelog YAML for you.

@rjernst
Copy link
Member

rjernst commented Nov 5, 2022

Since the readiness probe is already opt in, and only for cloud, could we just change the existing binding instead of making it configurable?

@grcevski
Copy link
Contributor Author

grcevski commented Nov 7, 2022

Yes you are right, I was originally concerned that we'll break folks because anyone outside of cloud might've used the readiness feature, but you reminded me now that we marked this as experimental, so I think it's fine to change the default behaviour without an option. I'll make the service bind on all addresses if enabled.

@grcevski
Copy link
Contributor Author

grcevski commented Nov 7, 2022

@elasticsearchmachine run elasticsearch-ci/packaging-tests-unix-sample

@grcevski grcevski changed the title Allow configuring readiness host Bind readiness service to the defined bind hosts Nov 8, 2022
assert portNumber >= 0;

List<String> httpBindHost = SETTING_HTTP_BIND_HOST.get(settings);
var bindHosts = (httpBindHost.isEmpty() ? NetworkService.GLOBAL_NETWORK_BIND_HOST_SETTING.get(settings) : httpBindHost).toArray(
Strings.EMPTY_ARRAY
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be two levels of configuration before the fallback to the loopback. Do we need the configuration checks? And should the fallback be to the wildcard address which implies all (rather than to the loopback) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I struggled with this a lot, what's the right address to pick, 0, publish, bound? I borrowed this code from how we setup the bound addresses in AbstractHttpServerTransport, I don't mind binding to 0 (all) as fallback, but if a user doesn't configure any of those above for Elasticsearch, they will get ES bound to localhost for HTTP, unless they configure SETTING_HTTP_PUBLISH_HOST. I don't know if binding to localhost and publishing another host address is a valid configuration?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I discussed it with @rjernst just now, we decided that perhaps the easiest is to bind for 0 and if there are any concerns firewall rules should be used to limit access to the port.

@grcevski grcevski changed the title Bind readiness service to the defined bind hosts Bind the readiness service to the wildcard address Nov 15, 2022
Copy link
Contributor

@ChrisHegarty ChrisHegarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kingherc kingherc added v8.7.0 and removed v8.6.0 labels Nov 16, 2022
@grcevski grcevski merged commit 3fd338f into elastic:main Nov 16, 2022
@grcevski
Copy link
Contributor Author

Thanks Chris!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Core Core issues without another label >enhancement Team:Core/Infra Meta label for core/infra team v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Readiness port should not only be available on the loopback device
5 participants