-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
client: refactor service interpolation #16413
Conversation
Services may reference runtime variables that must be interpolated in the client when reading them, otherwise any equality check fails because something like `${NOMAD_NAMESPACE}` is compared with its rendered value stored in the service catalog. This change attempts to make this requirement easier to discover by providing methods in the `TaskGroup` and `Task` structs that return the interpolated services. These methods take a `ServiceInterpolator` interface to avoid a circular dependency between the `structs` and `taskenv` packages.
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 LGTM as is.
A thought on how we could push it further (and this sort of thing applies across a lot of the code base). Right now this is relying on the developer noticing there's both a ConsulServices()
and InterpolatedConsulServices()
method and using the right one for their environment, which improves things but not by a lot because both of them return the same type that the consuming code in the client takes. If the consuming code in the client could take an InterpolatedServices
type, that would make it so that it'd be impossible to use the uninterpolated services improperly. And it'd let us use ordinary functions instead of having to thread an interface back up to nomad/structs
.
This frankly may be painful to implement in Go, but it might be worth doing a spike on at least.
@@ -21,7 +21,7 @@ func BuildAllocServices( | |||
AllocID: alloc.ID, | |||
Group: alloc.TaskGroup, | |||
}, | |||
Services: taskenv.InterpolateServices(taskenv.NewBuilder(mock.Node(), alloc, nil, alloc.Job.Region).Build(), tg.Services), | |||
Services: taskenv.NewBuilder(mock.Node(), alloc, nil, alloc.Job.Region).Build().InterpolateServices(tg.Services), |
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.
We have the TaskGroup
in scope here, so shouldn't we be calling tg.InterpolatedServices(taskenv.NewBuilder(...).Build())
?
(Also, I realize this is take from the original code but why do we call mock.Node()
here rather than passing in the node
parameter?)
@@ -219,7 +219,7 @@ func (h *serviceHook) Stop(ctx context.Context, req *interfaces.TaskStopRequest, | |||
|
|||
func (h *serviceHook) getWorkloadServices() *serviceregistration.WorkloadServices { | |||
// Interpolate with the task's environment | |||
interpolatedServices := taskenv.InterpolateServices(h.taskEnv, h.services) | |||
interpolatedServices := h.taskEnv.InterpolateServices(h.services) |
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 shows a bit of the awkwardness of this approach -- unless the TaskGroup
is in scope, we can't call tg.InterpolatedServices
, so having the method indirection doesn't seem all that valuable.
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.
Wondering if it would make sense if we had something like
type Services []Service
(s Services) Interpolate(TaskEnv) Services
eliding the need for the TaskGroup
reference? Seems like in practice you'd only get the Services from a tg anyway
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.
Ah nice, I think I will go with this approach. I was trying to have a InterpoaltedService
type but it breaks quite a bit of things 😬
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.
It turns out that structs.Services
is already defined for something completely unrelated. I took this as a sign from the universe telling me to stop fiddling with this part of the code for now and just fix the bug 😅
I'm going to close this one without merge and have updated #16402 with the right fix.
Thanks for all the help and apologies for the noise!
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.
heh FWIW all those constants should probably be named like ServicesContext
instead of stealing the good names 🙂
Services may reference runtime variables that must be interpolated in the client when reading them, otherwise any equality check fails because something like
${NOMAD_NAMESPACE}
is compared with its rendered value stored in the service catalog.This change attempts to make this requirement easier to discover by providing methods in the
TaskGroup
andTask
structs that return the interpolated services.These methods take a
ServiceInterpolator
interface to avoid a circular dependency between thestructs
andtaskenv
packages.No CHANGELOG because there are no user-facing changes.
This was extracted from #16402 to help facilitate review and discussion. I think this change makes the need to interpolate services a little more clear, but not by much. I can easily be convinced that we don't need this 😅