-
Notifications
You must be signed in to change notification settings - Fork 2k
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
consul/connect: enable setting local_bind_address in upstream #10071
consul/connect: enable setting local_bind_address in upstream #10071
Conversation
@tgross can we have this reviewed :) thank you! |
@jorgensoares we're in the middle of prepping the Nomad 1.0.4 release but it'll get reviewed soon. |
Hi regarding the changelog I added it there, but might not make sense, since I noticed other PR's were adding it 1.0.4 incorrectly as well. |
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.
This is looking really great @Ilhicas, thanks for the PR!
There's a block of code where we interpolate fields, and we'll want to add support for interpolating the local_bind_address also
nomad/client/taskenv/services.go
Line 131 in d164824
func interpolateConnectSidecarService(taskEnv *TaskEnv, sidecar *structs.ConsulSidecarService) { |
Just that and the CHANGELOG shuffle, I think this will be good to go.
@@ -1,4 +1,5 @@ | |||
## 1.0.4 (Unreleased) | |||
* consul/connect: Enable setting `local_bind_address` field on connect upstreams [[GH-6248](https://github.com/hashicorp/nomad/issues/6248)] |
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.
Like you already mentioned, we'll want this under the 1.1.0
header now, specifically under the IMPROVEMENTS
sub header
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.
Updated changelog
Also added LocalBindAddress to interpolation function, and updated interpolation tests
22bd91f
to
6ff9bd6
Compare
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.
LGTM; thanks @Ilhicas !
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Upstreams configuration in service->proxy->upstream stanza currently doesn't allow for the upstream local_bind_address definition
This allows to define different local_bind_addresses allowing envoy to read values from consul xds for example to bind the upstream to 127.0.0.X or others, allowing it to effectively avoid port binding in upstreams.
As an example
127.0.0.1:5432
127.0.0.2:5432
Could be used in multiple upstream stanzas without port binding issues.
This field is optional and defaults to the local_bind_address.
Closes #6248
More info on the motivations can be found at : #10057