-
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
services: enable setting arbitrary address value in service registrations #12720
Conversation
f5c9d86
to
ccc29d1
Compare
ed41878
to
d54d4ec
Compare
d54d4ec
to
84d059e
Compare
84d059e
to
9ae5f1a
Compare
9ae5f1a
to
96dacf2
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.
Looks great! Some options for docs updates and an ask for an interpolation test, but nothing that needs to block merging.
//FIXME Id is unused. Remove? | ||
Id string `hcl:"id,optional"` |
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 answered that question! Thanks for getting rid of deadcode 👍
// service or check registration. If no port label is specified (an empty value), | ||
// zero values are returned because no address could be resolved. |
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.
If no port label is specified (an empty value), zero values are returned because no address could be resolved.
I don't think this is quite accurate anymore? If no port label is specified address
is returned which may or may not be empty.
Perhaps:
If no port label or an explicit address are specified, zero values are returned because no address could be resolved.
// A custom advertise address can be used with a port map; using the | ||
// Value and ignoring the IP. The routing from your custom address to | ||
// the group network address is DIY. | ||
if mapping, exists := ports.Get(portLabel); exists { | ||
return address, mapping.Value, nil | ||
} |
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.
Took me a second to realize this is the "EC2" use case: advertise the public IP address with whatever port is defined.
Not sure it's worth expanding an already ample comment to include that though.
@@ -118,7 +118,7 @@ Connect][connect] integration. | |||
If a `to` value is not set, the port falls back to using the allocated host port. The `port` | |||
field may be a numeric port or a port label specified in the same group's network stanza. | |||
|
|||
- `driver` - Advertise the port determined by the driver (e.g. Docker or rkt). | |||
- `driver` - Advertise the port determined by the driver (e.g. Docker). |
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.
🪦
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.
🪦
- `address` `(string: <optional>)` - Specifies a custom address to advertise in | ||
Consul or Nomad service registration. Can be an IP address or domain name. If | ||
set, `address_mode` must be in `auto` mode. |
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.
Let's note that interpolation is supported or perhaps even suggest using it. Perhaps:
address
(string: <optional>)
- Manually specifies a custom address to advertise.address_mode
must be left on its defaultauto
mode. This is most useful in combination with interpolation. For example to advertise the public IP address of an AWS EC2 node set this to${unique.platform.aws.public-ipv4}
@@ -36,6 +36,7 @@ func InterpolateServices(taskEnv *TaskEnv, services []*structs.Service) []*struc | |||
|
|||
service.Name = taskEnv.ReplaceEnv(service.Name) | |||
service.PortLabel = taskEnv.ReplaceEnv(service.PortLabel) | |||
service.Address = taskEnv.ReplaceEnv(service.Address) |
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.
Let's add a test to assert this gets interpolated. We rarely touch this, so the risk of it getting accidentally broken is low, but given how critical interpolation is to this feature I think it's worth covering at least once.
// address could be resolved. | ||
// GetAddress returns the IP (or custom advertise address) and port to use for a | ||
// service or check registration. If no port label is specified (an empty value), | ||
// zero values are returned because no address could be resolved. | ||
func GetAddress( |
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.
Nothing for this PR, but noting for the future that I wonder if it's worth changing this function signature to take a struct rather than a growing list of types.
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, and also I think the port mapping lookup and numeric interpolation fallback can be consolidated from now 4 places
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 along with some of the comments already present. I really appreciate the updates to the comments particularly in the API.
A nice followup might be to relax the service name validation #11097.
…ions This PR introduces the `address` field in the `service` block so that Nomad or Consul services can be registered with a custom `.Address.` to advertise. The address can be an IP address or domain name. If the `address` field is set, the `service.address_mode` must be set in `auto` mode.
Co-authored-by: Michael Schurter <[email protected]>
470ed8a
to
2443174
Compare
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. |
This PR introduces the
address
field in theservice
block so that Nomador Consul services can be registered with a custom
.Address.
to advertise.The address can be an IP address or domain name. If the
address
field isset, the
service.address_mode
must be set inauto
mode.Note this slightly changes the output of
nomad service info
, such that:0
-portsare no longer included in the CLI output.
Closes #12722
Closes #2770
Closes #4815
Closes #3629
Basic Example
➜ curl -s localhost:8500/v1/catalog/service/basic | jq -r .[].ServiceAddress basic.example.com
Interpolation Example
Example with Consul Checks
Bonus EC2 Public IPv4 Example
ubuntu@ip-172-31-15-70:~$ ./nomad service info interp Job ID Address Tags Node ID Alloc ID interp 3.80.11.106:9911 [] 03b507af 403af357