From 9fa39eb82962f6bedf01f7bdb9e09c0dbac31dfe Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak <470696+pkazmierczak@users.noreply.github.com> Date: Mon, 21 Aug 2023 20:07:47 +0200 Subject: [PATCH] jobspec: add nomad_service field and identity block (#18239) This PR introduces updates to the jobspec required for workload identity support for services. --------- Co-authored-by: Luiz Aoqui --- api/services.go | 1 + api/tasks.go | 9 ++-- command/agent/job_endpoint.go | 17 +++++++ command/agent/job_endpoint_test.go | 18 +++++++ nomad/job_endpoint_hooks.go | 8 +++ nomad/structs/diff.go | 12 +++++ nomad/structs/diff_test.go | 78 ++++++++++++++++++++++++++++++ nomad/structs/services.go | 46 ++++++++++++++++++ nomad/structs/workload_id.go | 16 ++++-- 9 files changed, 197 insertions(+), 8 deletions(-) diff --git a/api/services.go b/api/services.go index 95f027810..ff66a7593 100644 --- a/api/services.go +++ b/api/services.go @@ -243,6 +243,7 @@ type Service struct { TaggedAddresses map[string]string `hcl:"tagged_addresses,block"` TaskName string `mapstructure:"task" hcl:"task,optional"` OnUpdate string `mapstructure:"on_update" hcl:"on_update,optional"` + Identity *WorkloadIdentity `hcl:"identity,optional"` // Provider defines which backend system provides the service registration, // either "consul" (default) or "nomad". diff --git a/api/tasks.go b/api/tasks.go index 8b94e488c..b582a7059 100644 --- a/api/tasks.go +++ b/api/tasks.go @@ -1152,8 +1152,9 @@ func (t *TaskCSIPluginConfig) Canonicalize() { // WorkloadIdentity is the jobspec block which determines if and how a workload // identity is exposed to tasks. type WorkloadIdentity struct { - Name string `hcl:"name,optional"` - Audience []string `mapstructure:"aud" hcl:"aud,optional"` - Env bool `hcl:"env,optional"` - File bool `hcl:"file,optional"` + Name string `hcl:"name,optional"` + Audience []string `mapstructure:"aud" hcl:"aud,optional"` + Env bool `hcl:"env,optional"` + File bool `hcl:"file,optional"` + ServiceName string `hcl:"service_name,optional"` } diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index 2df06df26..6e8ee7e98 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -1540,11 +1540,28 @@ func ApiServicesToStructs(in []*api.Service, group bool) []*structs.Service { out[i].Connect = ApiConsulConnectToStructs(s.Connect) } + if s.Identity != nil { + out[i].Identity = apiWorkloadIdentityToStructs(s.Identity) + } + } return out } +func apiWorkloadIdentityToStructs(in *api.WorkloadIdentity) *structs.WorkloadIdentity { + if in == nil { + return nil + } + return &structs.WorkloadIdentity{ + Name: in.Name, + Audience: in.Audience, + Env: in.Env, + File: in.File, + ServiceName: in.ServiceName, + } +} + func ApiConsulConnectToStructs(in *api.ConsulConnect) *structs.ConsulConnect { if in == nil { return nil diff --git a/command/agent/job_endpoint_test.go b/command/agent/job_endpoint_test.go index b66c67abf..bf85f04f9 100644 --- a/command/agent/job_endpoint_test.go +++ b/command/agent/job_endpoint_test.go @@ -4070,3 +4070,21 @@ func TestConversion_ApiConsulConnectToStructs(t *testing.T) { })) }) } + +func Test_apiWorkloadIdentityToStructs(t *testing.T) { + ci.Parallel(t) + must.Nil(t, apiWorkloadIdentityToStructs(nil)) + must.Eq(t, &structs.WorkloadIdentity{ + Name: "consul/test", + Audience: []string{"consul.io"}, + Env: false, + File: false, + ServiceName: "web", + }, apiWorkloadIdentityToStructs(&api.WorkloadIdentity{ + Name: "consul/test", + Audience: []string{"consul.io"}, + Env: false, + File: false, + ServiceName: "web", + })) +} diff --git a/nomad/job_endpoint_hooks.go b/nomad/job_endpoint_hooks.go index a7b7a4ad2..0ff15af8f 100644 --- a/nomad/job_endpoint_hooks.go +++ b/nomad/job_endpoint_hooks.go @@ -305,6 +305,14 @@ func (v *jobValidate) Validate(job *structs.Job) (warnings []error, err error) { multierror.Append(validationErrors, fmt.Errorf("job priority must be between [%d, %d]", structs.JobMinPriority, v.srv.config.JobMaxPriority)) } + for _, tg := range job.TaskGroups { + for _, s := range tg.Services { + if s.Identity != nil && s.Identity.Name == "" { + multierror.Append(validationErrors, fmt.Errorf("service identity name cannot be empty")) + } + } + } + return warnings, validationErrors.ErrorOrNil() } diff --git a/nomad/structs/diff.go b/nomad/structs/diff.go index 8e649568a..6328504bf 100644 --- a/nomad/structs/diff.go +++ b/nomad/structs/diff.go @@ -665,6 +665,11 @@ func serviceDiff(old, new *Service, contextual bool) *ObjectDiff { diff.Objects = append(diff.Objects, conDiffs) } + // Workload Identity diffs + if wiDiffs := idDiff(old.Identity, new.Identity, contextual); wiDiffs != nil { + diff.Objects = append(diff.Objects, wiDiffs) + } + return diff } @@ -2390,9 +2395,11 @@ func idDiff(oldWI, newWI *WorkloadIdentity, contextual bool) *ObjectDiff { if reflect.DeepEqual(oldWI, newWI) { return nil } else if oldWI == nil { + oldWI = &WorkloadIdentity{} diff.Type = DiffTypeAdded newPrimitiveFlat = flatmap.Flatten(newWI, nil, true) } else if newWI == nil { + newWI = &WorkloadIdentity{} diff.Type = DiffTypeDeleted oldPrimitiveFlat = flatmap.Flatten(oldWI, nil, true) } else { @@ -2401,6 +2408,11 @@ func idDiff(oldWI, newWI *WorkloadIdentity, contextual bool) *ObjectDiff { newPrimitiveFlat = flatmap.Flatten(newWI, nil, true) } + audDiff := stringSetDiff(oldWI.Audience, newWI.Audience, "Audience", contextual) + if audDiff != nil { + diff.Objects = append(diff.Objects, audDiff) + } + // Diff the primitive fields. diff.Fields = fieldDiffs(oldPrimitiveFlat, newPrimitiveFlat, contextual) diff --git a/nomad/structs/diff_test.go b/nomad/structs/diff_test.go index 1244bc731..24c55c211 100644 --- a/nomad/structs/diff_test.go +++ b/nomad/structs/diff_test.go @@ -8534,6 +8534,84 @@ func TestServicesDiff(t *testing.T) { }, }, }, + { + Name: "Service with different identity name and aud", + Contextual: false, + Old: []*Service{ + { + Name: "test", + Provider: "consul", + PortLabel: "http", + Identity: &WorkloadIdentity{ + Name: "test", + Audience: []string{"consul.io"}, + File: true, + }, + }, + }, + New: []*Service{ + { + Name: "test2", + Provider: "consul", + PortLabel: "http", + Identity: &WorkloadIdentity{ + Name: "test2", + Audience: []string{"consul.io", "nomad.dev"}, + File: false, + }, + }, + }, + Expected: []*ObjectDiff{ + { + Type: DiffTypeEdited, + Name: "Service", + Fields: []*FieldDiff{ + { + Type: DiffTypeEdited, + Name: "Name", + Old: "test", + New: "test2", + Annotations: nil, + }, + }, + Objects: []*ObjectDiff{ + { + Type: DiffTypeEdited, + Name: "Identity", + Fields: []*FieldDiff{ + { + Type: DiffTypeEdited, + Name: "File", + Old: "true", + New: "false", + Annotations: nil, + }, + { + Type: DiffTypeEdited, + Name: "Name", + Old: "test", + New: "test2", + Annotations: nil, + }, + }, + Objects: []*ObjectDiff{ + { + Type: DiffTypeAdded, + Name: "Audience", + Fields: []*FieldDiff{ + { + Type: DiffTypeAdded, + Name: "Audience", + New: "nomad.dev", + }, + }, + }, + }, + }, + }, + }, + }, + }, } for _, c := range cases { diff --git a/nomad/structs/services.go b/nomad/structs/services.go index 1dee85910..49bdf2dee 100644 --- a/nomad/structs/services.go +++ b/nomad/structs/services.go @@ -609,6 +609,11 @@ type Service struct { // either ServiceProviderConsul or ServiceProviderNomad and defaults to the former when // left empty by the operator. Provider string + + // Identity is a field populated automatically by the job mutating hook. + // Its name will be `consul-service/${service_name}`, and its contents will + // match the server's `consul.service_identity` configuration block. + Identity *WorkloadIdentity } // Copy the block recursively. Returns nil if nil. @@ -635,6 +640,8 @@ func (s *Service) Copy() *Service { ns.CanaryMeta = maps.Clone(s.CanaryMeta) ns.TaggedAddresses = maps.Clone(s.TaggedAddresses) + ns.Identity = s.Identity.Copy() + return ns } @@ -736,6 +743,10 @@ func (s *Service) Validate() error { ServiceProviderConsul, ServiceProviderNomad, s.Provider)) } + if err := s.validateIdentity(); err != nil { + mErr.Errors = append(mErr.Errors, err) + } + return mErr.ErrorOrNil() } @@ -808,6 +819,20 @@ func (s *Service) validateNomadService(mErr *multierror.Error) { } } +// validateIdentity performs validation on workload identity field populated by +// the job mutating hook +func (s *Service) validateIdentity() error { + if s.Identity == nil { + return nil + } + + if len(s.Identity.Audience) == 0 { + return fmt.Errorf("Service identity must provide at least one target aud value") + } + + return nil +} + // ValidateName checks if the service Name is valid and should be called after // the name has been interpolated func (s *Service) ValidateName(name string) error { @@ -846,6 +871,7 @@ func (s *Service) Hash(allocID, taskName string, canary bool) string { hashConnect(h, s.Connect) hashString(h, s.OnUpdate) hashString(h, s.Namespace) + hashIdentity(h, s.Identity) // Don't hash the provider parameter, so we don't cause churn of all // registered services when upgrading Nomad versions. The provider is not @@ -879,6 +905,22 @@ func hashConnect(h hash.Hash, connect *ConsulConnect) { } } +func hashIdentity(h hash.Hash, identity *WorkloadIdentity) { + if identity != nil { + hashString(h, identity.Name) + hashAud(h, identity.Audience) + hashBool(h, identity.Env, "Env") + hashBool(h, identity.File, "File") + hashString(h, identity.ServiceName) + } +} + +func hashAud(h hash.Hash, aud []string) { + for _, a := range aud { + hashString(h, a) + } +} + func hashString(h hash.Hash, s string) { _, _ = io.WriteString(h, s) } @@ -969,6 +1011,10 @@ func (s *Service) Equal(o *Service) bool { return false } + if !s.Identity.Equal(o.Identity) { + return false + } + return true } diff --git a/nomad/structs/workload_id.go b/nomad/structs/workload_id.go index b023f0041..cb5055c25 100644 --- a/nomad/structs/workload_id.go +++ b/nomad/structs/workload_id.go @@ -56,6 +56,9 @@ type WorkloadIdentity struct { // File writes the Workload Identity into the Task's secrets directory // if set. File bool + + // ServiceName is used to bind the identity to a correct Consul service. + ServiceName string } func (wi *WorkloadIdentity) Copy() *WorkloadIdentity { @@ -63,10 +66,11 @@ func (wi *WorkloadIdentity) Copy() *WorkloadIdentity { return nil } return &WorkloadIdentity{ - Name: wi.Name, - Audience: slices.Clone(wi.Audience), - Env: wi.Env, - File: wi.File, + Name: wi.Name, + Audience: slices.Clone(wi.Audience), + Env: wi.Env, + File: wi.File, + ServiceName: wi.ServiceName, } } @@ -91,6 +95,10 @@ func (wi *WorkloadIdentity) Equal(other *WorkloadIdentity) bool { return false } + if wi.ServiceName != other.ServiceName { + return false + } + return true }