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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

client: refactor service interpolation #16413

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion client/allocrunner/group_service_hook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ func (h *groupServiceHook) deregister() {

func (h *groupServiceHook) getWorkloadServices() *serviceregistration.WorkloadServices {
// Interpolate with the task's environment
interpolatedServices := taskenv.InterpolateServices(h.taskEnvBuilder.Build(), h.services)
interpolatedServices := h.taskEnvBuilder.Build().InterpolateServices(h.services)

var netStatus *structs.AllocNetworkStatus
if h.networkStatus != nil {
Expand Down
7 changes: 2 additions & 5 deletions client/allocrunner/taskrunner/envoy_bootstrap_hook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,10 +206,8 @@ func (_ *envoyBootstrapHook) extractNameAndKind(kind structs.TaskKind) (string,

func (h *envoyBootstrapHook) lookupService(svcKind, svcName string, taskEnv *taskenv.TaskEnv) (*structs.Service, error) {
tg := h.alloc.Job.LookupTaskGroup(h.alloc.TaskGroup)
interpolatedServices := taskenv.InterpolateServices(taskEnv, tg.Services)

var service *structs.Service
for _, s := range interpolatedServices {
for _, s := range tg.InterpolatedServices(taskEnv) {
if s.Name == svcName {
service = s
break
Expand Down Expand Up @@ -427,8 +425,7 @@ func buildEnvoyBind(alloc *structs.Allocation, ifce, service, task string, taskE
port := basePort
switch tg.Networks[0].Mode {
case "host":
interpolatedServices := taskenv.InterpolateServices(taskEnv, tg.Services)
for _, svc := range interpolatedServices {
for _, svc := range tg.InterpolatedServices(taskEnv) {
if svc.Name == service {
mapping := tg.Networks.Port(svc.PortLabel)
port = mapping.Value
Expand Down
6 changes: 2 additions & 4 deletions client/allocrunner/taskrunner/script_check_hook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,8 +176,7 @@ func (h *scriptCheckHook) Stop(ctx context.Context, req *interfaces.TaskStopRequ

func (h *scriptCheckHook) newScriptChecks() map[string]*scriptCheck {
scriptChecks := make(map[string]*scriptCheck)
interpolatedTaskServices := taskenv.InterpolateServices(h.taskEnv, h.task.Services)
for _, service := range interpolatedTaskServices {
for _, service := range h.task.InterpolatedServices(h.taskEnv) {
for _, check := range service.Checks {
if check.Type != structs.ServiceCheckScript {
continue
Expand Down Expand Up @@ -212,8 +211,7 @@ func (h *scriptCheckHook) newScriptChecks() map[string]*scriptCheck {
// service.check.task matches the task name. The service.check.task takes
// precedence.
tg := h.alloc.Job.LookupTaskGroup(h.alloc.TaskGroup)
interpolatedGroupServices := taskenv.InterpolateServices(h.taskEnv, tg.Services)
for _, service := range interpolatedGroupServices {
for _, service := range tg.InterpolatedServices(h.taskEnv) {
for _, check := range service.Checks {
if check.Type != structs.ServiceCheckScript {
continue
Expand Down
2 changes: 1 addition & 1 deletion client/allocrunner/taskrunner/service_hook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

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.

Copy link
Contributor

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

Copy link
Contributor Author

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 😬

Copy link
Contributor Author

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!

Copy link
Contributor

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 🙂


info := structs.AllocInfo{
AllocID: h.allocID,
Expand Down
44 changes: 22 additions & 22 deletions client/taskenv/services.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@ import (

// InterpolateServices returns an interpolated copy of services and checks with
// values from the task's environment.
func InterpolateServices(taskEnv *TaskEnv, services []*structs.Service) []*structs.Service {
func (t *TaskEnv) InterpolateServices(services []*structs.Service) []*structs.Service {
// Guard against not having a valid taskEnv. This can be the case if the
// PreKilling or Exited hook is run before Poststart.
if taskEnv == nil || len(services) == 0 {
if t == nil || len(services) == 0 {
return nil
}

Expand All @@ -21,28 +21,28 @@ func InterpolateServices(taskEnv *TaskEnv, services []*structs.Service) []*struc
service := origService.Copy()

for _, check := range service.Checks {
check.Name = taskEnv.ReplaceEnv(check.Name)
check.Type = taskEnv.ReplaceEnv(check.Type)
check.Command = taskEnv.ReplaceEnv(check.Command)
check.Args = taskEnv.ParseAndReplace(check.Args)
check.Path = taskEnv.ReplaceEnv(check.Path)
check.Protocol = taskEnv.ReplaceEnv(check.Protocol)
check.PortLabel = taskEnv.ReplaceEnv(check.PortLabel)
check.InitialStatus = taskEnv.ReplaceEnv(check.InitialStatus)
check.Method = taskEnv.ReplaceEnv(check.Method)
check.GRPCService = taskEnv.ReplaceEnv(check.GRPCService)
check.Header = interpolateMapStringSliceString(taskEnv, check.Header)
check.Name = t.ReplaceEnv(check.Name)
check.Type = t.ReplaceEnv(check.Type)
check.Command = t.ReplaceEnv(check.Command)
check.Args = t.ParseAndReplace(check.Args)
check.Path = t.ReplaceEnv(check.Path)
check.Protocol = t.ReplaceEnv(check.Protocol)
check.PortLabel = t.ReplaceEnv(check.PortLabel)
check.InitialStatus = t.ReplaceEnv(check.InitialStatus)
check.Method = t.ReplaceEnv(check.Method)
check.GRPCService = t.ReplaceEnv(check.GRPCService)
check.Header = interpolateMapStringSliceString(t, check.Header)
}

service.Name = taskEnv.ReplaceEnv(service.Name)
service.PortLabel = taskEnv.ReplaceEnv(service.PortLabel)
service.Address = taskEnv.ReplaceEnv(service.Address)
service.Tags = taskEnv.ParseAndReplace(service.Tags)
service.CanaryTags = taskEnv.ParseAndReplace(service.CanaryTags)
service.Meta = interpolateMapStringString(taskEnv, service.Meta)
service.CanaryMeta = interpolateMapStringString(taskEnv, service.CanaryMeta)
service.TaggedAddresses = interpolateMapStringString(taskEnv, service.TaggedAddresses)
interpolateConnect(taskEnv, service.Connect)
service.Name = t.ReplaceEnv(service.Name)
service.PortLabel = t.ReplaceEnv(service.PortLabel)
service.Address = t.ReplaceEnv(service.Address)
service.Tags = t.ParseAndReplace(service.Tags)
service.CanaryTags = t.ParseAndReplace(service.CanaryTags)
service.Meta = interpolateMapStringString(t, service.Meta)
service.CanaryMeta = interpolateMapStringString(t, service.CanaryMeta)
service.TaggedAddresses = interpolateMapStringString(t, service.TaggedAddresses)
interpolateConnect(t, service.Connect)

interpolated[i] = service
}
Expand Down
2 changes: 1 addition & 1 deletion client/taskenv/services_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ func TestInterpolateServices(t *testing.T) {
},
}

interpolated := InterpolateServices(env, services)
interpolated := env.InterpolateServices(services)

exp := []*structs.Service{
{
Expand Down
2 changes: 1 addition & 1 deletion command/agent/consul/structs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Copy link
Member

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?)

Networks: alloc.AllocatedResources.Shared.Networks,

//TODO(schmichael) there's probably a better way than hacking driver network
Expand Down
9 changes: 9 additions & 0 deletions nomad/structs/services.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,15 @@ const (
minCheckTimeout = 1 * time.Second
)

// ServiceInterpolator is the interface that wraps methods used to interpolate
// runtime values into services.
//
// Implementations must not mutate their input but rather act on and return
// copies.
type ServiceInterpolator interface {
InterpolateServices([]*Service) []*Service
}

// ServiceCheck represents a Nomad or Consul service health check.
//
// The fields available depend on the service provider the check is being
Expand Down
33 changes: 31 additions & 2 deletions nomad/structs/structs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6334,7 +6334,8 @@ type TaskGroup struct {
// Consul configuration specific to this task group
Consul *Consul

// Services this group provides
// Services this group provides.Service fields may reference runtime
// variables that must be interpolated when reading them from clients.
Services []*Service

// Volumes is a map of volumes that have been requested by the task group.
Expand Down Expand Up @@ -6472,6 +6473,26 @@ func (tg *TaskGroup) ConsulServices() []*Service {
})
}

// InterpolatedServices returns a copy of the all services in the task group
// and its tasks with runtime values interpolated.
func (tg *TaskGroup) InterpolatedServices(i ServiceInterpolator) []*Service {
return i.InterpolateServices(tg.Services)
}

// InterpolatedConsulServices returns a copy of the all services that use the
// "consul" service provider in the task group and its tasks with runtime
// values interpolated.
func (tg *TaskGroup) InterpolatedConsulServices(i ServiceInterpolator) []*Service {
return i.InterpolateServices(tg.ConsulServices())
}

// InterpolatedNomadServices returns a copy of the all services that use the
// "nomad" service provider in the task group and its tasks with runtime values
// interpolated.
func (tg *TaskGroup) InterpolatedNomadServices(i ServiceInterpolator) []*Service {
return i.InterpolateServices(tg.NomadServices())
}

func (tg *TaskGroup) filterServices(f func(s *Service) bool) []*Service {
var services []*Service
for _, service := range tg.Services {
Expand Down Expand Up @@ -7164,7 +7185,9 @@ type Task struct {
// Map of environment variables to be used by the driver
Env map[string]string

// List of service definitions exposed by the Task
// List of service definitions exposed by the Task. Service fields may
// reference runtime variables that must be interpolated when reading them
// from clients.
Services []*Service

// Vault is used to define the set of Vault policies that this task should
Expand Down Expand Up @@ -7371,6 +7394,12 @@ func (t *Task) GoString() string {
return fmt.Sprintf("*%#v", *t)
}

// InterpolatedServices returns a copy of the all services in the task with
// runtime values interpolated.
func (t *Task) InterpolatedServices(i ServiceInterpolator) []*Service {
return i.InterpolateServices(t.Services)
}

// Validate is used to check a task for reasonable configuration
func (t *Task) Validate(ephemeralDisk *EphemeralDisk, jobType string, tgServices []*Service, tgNetworks Networks) error {
var mErr multierror.Error
Expand Down