From 7466496608ef0b334aade02ad9521ed6ffbc3d13 Mon Sep 17 00:00:00 2001 From: Luiz Aoqui Date: Thu, 31 Aug 2023 10:22:48 -0400 Subject: [PATCH] config: fix identity config for Consul service (#18363) Rename the agent configuraion for workload identity to `WorkloadIdentityConfig` to make its use more explicit and remove the `ServiceName` field since it is never expected to be defined in a configuration file. Also update the job mutation to inject a service identity following these rules: 1. Don't inject identity if `consul.use_identity` is false. 2. Don't inject identity if `consul.service_identity` is not specified. 3. Don't inject identity if service provider is not `consul`. 4. Set name and service name if the service specifies an identity. 5. Inject `consul.service_identity` if service does not specify an identity. --- api/services.go | 2 +- command/agent/config_parse.go | 8 +- command/agent/config_parse_test.go | 24 +- nomad/config.go | 49 ++++ nomad/job_endpoint.go | 2 +- nomad/job_endpoint_hook_connect_test.go | 2 +- .../job_endpoint_hook_implicit_identities.go | 76 ++++++ ..._endpoint_hook_implicit_identities_test.go | 240 +++++++++++++++++ nomad/job_endpoint_hooks.go | 75 +++--- nomad/job_endpoint_hooks_test.go | 209 ++++++++------- nomad/structs/config/consul.go | 86 ++----- nomad/structs/config/consul_test.go | 23 +- nomad/structs/config/workload_id.go | 99 +++++++ nomad/structs/config/workload_id_test.go | 242 ++++++++++++++++++ nomad/structs/workload_id.go | 6 +- 15 files changed, 904 insertions(+), 239 deletions(-) create mode 100644 nomad/job_endpoint_hook_implicit_identities.go create mode 100644 nomad/job_endpoint_hook_implicit_identities_test.go create mode 100644 nomad/structs/config/workload_id.go create mode 100644 nomad/structs/config/workload_id_test.go diff --git a/api/services.go b/api/services.go index ff66a7593..43759ac00 100644 --- a/api/services.go +++ b/api/services.go @@ -243,7 +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"` + Identity *WorkloadIdentity `hcl:"identity,block"` // Provider defines which backend system provides the service registration, // either "consul" (default) or "nomad". diff --git a/command/agent/config_parse.go b/command/agent/config_parse.go index 392effae9..cf74386f4 100644 --- a/command/agent/config_parse.go +++ b/command/agent/config_parse.go @@ -57,8 +57,8 @@ func ParseConfigFile(path string) (*Config, error) { ACL: &ACLConfig{}, Audit: &config.AuditConfig{}, Consul: &config.ConsulConfig{ - ServiceIdentity: &config.WorkloadIdentity{}, - TemplateIdentity: &config.WorkloadIdentity{}, + ServiceIdentity: &config.WorkloadIdentityConfig{}, + TemplateIdentity: &config.WorkloadIdentityConfig{}, }, Consuls: map[string]*config.ConsulConfig{}, Autopilot: &config.AutopilotConfig{}, @@ -418,7 +418,7 @@ func parseConsuls(c *Config, list *ast.ObjectList) error { return err } - var serviceIdentity config.WorkloadIdentity + var serviceIdentity config.WorkloadIdentityConfig if err := mapstructure.WeakDecode(m, &serviceIdentity); err != nil { return err } @@ -432,7 +432,7 @@ func parseConsuls(c *Config, list *ast.ObjectList) error { return err } - var templateIdentity config.WorkloadIdentity + var templateIdentity config.WorkloadIdentityConfig if err := mapstructure.WeakDecode(m, &templateIdentity); err != nil { return err } diff --git a/command/agent/config_parse_test.go b/command/agent/config_parse_test.go index 950ea4d2e..0fdd05919 100644 --- a/command/agent/config_parse_test.go +++ b/command/agent/config_parse_test.go @@ -236,15 +236,15 @@ var basicConfig = &Config{ Timeout: 5 * time.Second, TimeoutHCL: "5s", UseIdentity: &trueValue, - ServiceIdentity: &config.WorkloadIdentity{ + ServiceIdentity: &config.WorkloadIdentityConfig{ Audience: []string{"consul.io", "nomad.dev"}, - Env: false, - File: true, + Env: pointer.Of(false), + File: pointer.Of(true), }, - TemplateIdentity: &config.WorkloadIdentity{ + TemplateIdentity: &config.WorkloadIdentityConfig{ Audience: []string{"consul.io"}, - Env: true, - File: false, + Env: pointer.Of(true), + File: pointer.Of(false), }, }, Consuls: map[string]*config.ConsulConfig{ @@ -272,15 +272,15 @@ var basicConfig = &Config{ Timeout: 5 * time.Second, TimeoutHCL: "5s", UseIdentity: &trueValue, - ServiceIdentity: &config.WorkloadIdentity{ + ServiceIdentity: &config.WorkloadIdentityConfig{ Audience: []string{"consul.io", "nomad.dev"}, - Env: false, - File: true, + Env: pointer.Of(false), + File: pointer.Of(true), }, - TemplateIdentity: &config.WorkloadIdentity{ + TemplateIdentity: &config.WorkloadIdentityConfig{ Audience: []string{"consul.io"}, - Env: true, - File: false, + Env: pointer.Of(true), + File: pointer.Of(false), }, }, }, diff --git a/nomad/config.go b/nomad/config.go index 315dde5a1..68a45e67e 100644 --- a/nomad/config.go +++ b/nomad/config.go @@ -463,6 +463,55 @@ func (c *Config) Copy() *Config { return &nc } +// ConsulServiceIdentity returns the workload identity to be used for accessing +// the Consul API to register and manage Consul services. +func (c *Config) ConsulServiceIdentity() *structs.WorkloadIdentity { + if c.ConsulConfig == nil { + return nil + } + + return workloadIdentityFromConfig(c.ConsulConfig.ServiceIdentity) +} + +// ConsulTemplateIdentity returns the workload identity to be used for +// accessing the Consul API from templates. +func (c *Config) ConsulTemplateIdentity() *structs.WorkloadIdentity { + if c.ConsulConfig == nil { + return nil + } + + return workloadIdentityFromConfig(c.ConsulConfig.TemplateIdentity) +} + +// UseConsulIdentity returns true when Consul workload identity is enabled. +func (c *Config) UseConsulIdentity() bool { + return c.ConsulConfig != nil && + c.ConsulConfig.UseIdentity != nil && + *c.ConsulConfig.UseIdentity +} + +// workloadIdentityFromConfig returns a structs.WorkloadIdentity to be used in +// a job from a config.WorkloadIdentityConfig parsed from an agent config file. +func workloadIdentityFromConfig(widConfig *config.WorkloadIdentityConfig) *structs.WorkloadIdentity { + if widConfig == nil { + return nil + } + + wid := &structs.WorkloadIdentity{} + + if len(widConfig.Audience) > 0 { + wid.Audience = widConfig.Audience + } + if widConfig.Env != nil { + wid.Env = *widConfig.Env + } + if widConfig.File != nil { + wid.File = *widConfig.File + } + + return wid +} + // DefaultConfig returns the default configuration. Only used as the basis for // merging agent or test parameters. func DefaultConfig() *Config { diff --git a/nomad/job_endpoint.go b/nomad/job_endpoint.go index 8ca314af2..ba2171c03 100644 --- a/nomad/job_endpoint.go +++ b/nomad/job_endpoint.go @@ -73,7 +73,7 @@ func NewJobEndpoints(s *Server, ctx *RPCContext) *Job { jobExposeCheckHook{}, jobImpliedConstraints{}, jobNodePoolMutatingHook{srv: s}, - jobIdentityCreator{srv: s}, + jobImplicitIdentitiesHook{srv: s}, }, validators: []jobValidator{ jobConnectHook{}, diff --git a/nomad/job_endpoint_hook_connect_test.go b/nomad/job_endpoint_hook_connect_test.go index 2e0fc88bf..88977b7f7 100644 --- a/nomad/job_endpoint_hook_connect_test.go +++ b/nomad/job_endpoint_hook_connect_test.go @@ -407,7 +407,7 @@ func TestJobEndpointConnect_groupConnectHook_MeshGateway(t *testing.T) { func TestJobEndpointConnect_ConnectInterpolation(t *testing.T) { ci.Parallel(t) - server := &Server{logger: testlog.HCLogger(t)} + server := &Server{logger: testlog.HCLogger(t), config: DefaultConfig()} jobEndpoint := NewJobEndpoints(server, nil) j := mock.ConnectJob() diff --git a/nomad/job_endpoint_hook_implicit_identities.go b/nomad/job_endpoint_hook_implicit_identities.go new file mode 100644 index 000000000..82cc9d060 --- /dev/null +++ b/nomad/job_endpoint_hook_implicit_identities.go @@ -0,0 +1,76 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: BUSL-1.1 + +package nomad + +import ( + "fmt" + + "github.com/hashicorp/nomad/nomad/structs" +) + +var ( + consulServiceIdentityNamePrefix = "consul-service" +) + +// jobImplicitIdentitiesHook adds implicit `identity` blocks for external +// services, like Consul and Vault. +type jobImplicitIdentitiesHook struct { + srv *Server +} + +func (jobImplicitIdentitiesHook) Name() string { + return "implicit-identities" +} + +func (h jobImplicitIdentitiesHook) Mutate(job *structs.Job) (*structs.Job, []error, error) { + for _, tg := range job.TaskGroups { + for _, s := range tg.Services { + h.handleConsulService(s) + } + + for _, t := range tg.Tasks { + for _, s := range t.Services { + h.handleConsulService(s) + } + } + } + + return job, nil, nil +} + +// handleConsulService injects a workload identity to the service if: +// 1. The service uses the Consul provider. +// 2. The server is configured with `consul.use_identity = true` and a +// `consul.service_identity` is provided. +// +// If the service already has an identity it sets the identity name and service +// name values. +func (h jobImplicitIdentitiesHook) handleConsulService(s *structs.Service) { + if !h.srv.config.UseConsulIdentity() { + return + } + + if s.Provider != "" && s.Provider != "consul" { + return + } + + // Use the identity specified in the service. + serviceWID := s.Identity + if serviceWID == nil { + // If the service doesn't specify an identity, fallback to the service + // identity defined in the server configuration. + serviceWID = h.srv.config.ConsulServiceIdentity() + if serviceWID == nil { + // If no identity is found, skip injecting the implicit identity + // and fallback to the legacy flow. + return + } + } + + // Set the expected identity name and service name. + serviceWID.Name = fmt.Sprintf("%s/%s", consulServiceIdentityNamePrefix, s.Name) + serviceWID.ServiceName = s.Name + + s.Identity = serviceWID +} diff --git a/nomad/job_endpoint_hook_implicit_identities_test.go b/nomad/job_endpoint_hook_implicit_identities_test.go new file mode 100644 index 000000000..809dba121 --- /dev/null +++ b/nomad/job_endpoint_hook_implicit_identities_test.go @@ -0,0 +1,240 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: BUSL-1.1 + +package nomad + +import ( + "testing" + + "github.com/hashicorp/nomad/ci" + "github.com/hashicorp/nomad/helper/pointer" + "github.com/hashicorp/nomad/nomad/structs" + "github.com/hashicorp/nomad/nomad/structs/config" + "github.com/shoenig/test/must" +) + +func Test_jobImplicitIndentitiesHook_Mutate_consul_service(t *testing.T) { + ci.Parallel(t) + + testCases := []struct { + name string + inputJob *structs.Job + inputConfig *Config + expectedOutputJob *structs.Job + }{ + { + name: "no mutation when identity is disabled", + inputJob: &structs.Job{ + TaskGroups: []*structs.TaskGroup{{ + Services: []*structs.Service{{ + Provider: "consul", + }}, + }}, + }, + inputConfig: &Config{ + ConsulConfig: &config.ConsulConfig{ + UseIdentity: pointer.Of(false), + }, + }, + expectedOutputJob: &structs.Job{ + TaskGroups: []*structs.TaskGroup{{ + Services: []*structs.Service{{ + Provider: "consul", + }}, + }}, + }, + }, + { + name: "no mutation when identity is enabled but no service identity is configured", + inputJob: &structs.Job{ + TaskGroups: []*structs.TaskGroup{{ + Services: []*structs.Service{{ + Provider: "consul", + }}, + }}, + }, + inputConfig: &Config{ + ConsulConfig: &config.ConsulConfig{ + UseIdentity: pointer.Of(true), + }, + }, + expectedOutputJob: &structs.Job{ + TaskGroups: []*structs.TaskGroup{{ + Services: []*structs.Service{{ + Provider: "consul", + }}, + }}, + }, + }, + { + name: "no mutation when nomad service", + inputJob: &structs.Job{ + TaskGroups: []*structs.TaskGroup{{ + Services: []*structs.Service{{ + Provider: "nomad", + }}, + }}, + }, + inputConfig: &Config{ + ConsulConfig: &config.ConsulConfig{ + UseIdentity: pointer.Of(true), + ServiceIdentity: &config.WorkloadIdentityConfig{ + Audience: []string{"consul.io"}, + }, + }, + }, + expectedOutputJob: &structs.Job{ + TaskGroups: []*structs.TaskGroup{{ + Services: []*structs.Service{{ + Provider: "nomad", + }}, + }}, + }, + }, + { + name: "mutate identity name and service name when custom identity is provided", + inputJob: &structs.Job{ + TaskGroups: []*structs.TaskGroup{{ + Services: []*structs.Service{ + { + Provider: "consul", + Name: "web", + Identity: &structs.WorkloadIdentity{ + Audience: []string{"consul.io", "nomad.dev"}, + File: true, + Env: false, + }, + }, + { + Name: "web", + Identity: &structs.WorkloadIdentity{ + Audience: []string{"consul.io", "nomad.dev"}, + File: true, + Env: false, + }, + }, + }, + Tasks: []*structs.Task{{ + Services: []*structs.Service{{ + Provider: "consul", + Name: "web-task", + Identity: &structs.WorkloadIdentity{ + Audience: []string{"consul.io", "nomad.dev"}, + File: true, + Env: false, + }, + }}, + }}, + }}, + }, + inputConfig: &Config{ + ConsulConfig: &config.ConsulConfig{ + UseIdentity: pointer.Of(true), + ServiceIdentity: &config.WorkloadIdentityConfig{ + Audience: []string{"consul.io"}, + }, + }, + }, + expectedOutputJob: &structs.Job{ + TaskGroups: []*structs.TaskGroup{{ + Services: []*structs.Service{ + { + Provider: "consul", + Name: "web", + Identity: &structs.WorkloadIdentity{ + Name: "consul-service/web", + Audience: []string{"consul.io", "nomad.dev"}, + File: true, + Env: false, + ServiceName: "web", + }, + }, + { + Name: "web", + Identity: &structs.WorkloadIdentity{ + Name: "consul-service/web", + Audience: []string{"consul.io", "nomad.dev"}, + File: true, + Env: false, + ServiceName: "web", + }, + }, + }, + Tasks: []*structs.Task{{ + Services: []*structs.Service{{ + Provider: "consul", + Name: "web-task", + Identity: &structs.WorkloadIdentity{ + Name: "consul-service/web-task", + Audience: []string{"consul.io", "nomad.dev"}, + File: true, + Env: false, + ServiceName: "web-task", + }, + }}, + }}, + }}, + }, + }, + { + name: "mutate service to inject identity", + inputJob: &structs.Job{ + TaskGroups: []*structs.TaskGroup{{ + Services: []*structs.Service{{ + Provider: "consul", + Name: "web", + }}, + Tasks: []*structs.Task{{ + Services: []*structs.Service{{ + Provider: "consul", + Name: "web-task", + }}, + }}, + }}, + }, + inputConfig: &Config{ + ConsulConfig: &config.ConsulConfig{ + UseIdentity: pointer.Of(true), + ServiceIdentity: &config.WorkloadIdentityConfig{ + Audience: []string{"consul.io"}, + }, + }, + }, + expectedOutputJob: &structs.Job{ + TaskGroups: []*structs.TaskGroup{{ + Services: []*structs.Service{{ + Provider: "consul", + Name: "web", + Identity: &structs.WorkloadIdentity{ + Name: "consul-service/web", + Audience: []string{"consul.io"}, + ServiceName: "web", + }, + }}, + Tasks: []*structs.Task{{ + Services: []*structs.Service{{ + Provider: "consul", + Name: "web-task", + Identity: &structs.WorkloadIdentity{ + Name: "consul-service/web-task", + Audience: []string{"consul.io"}, + ServiceName: "web-task", + }, + }}, + }}, + }}, + }, + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + impl := jobImplicitIdentitiesHook{srv: &Server{ + config: tc.inputConfig, + }} + actualJob, actualWarnings, actualError := impl.Mutate(tc.inputJob) + must.Eq(t, tc.expectedOutputJob, actualJob) + must.NoError(t, actualError) + must.Nil(t, actualWarnings) + }) + } +} diff --git a/nomad/job_endpoint_hooks.go b/nomad/job_endpoint_hooks.go index d28ca487c..0a4c7d050 100644 --- a/nomad/job_endpoint_hooks.go +++ b/nomad/job_endpoint_hooks.go @@ -307,8 +307,24 @@ func (v *jobValidate) Validate(job *structs.Job) (warnings []error, err error) { 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")) + serviceWarn, serviceErr := v.validateServiceIdentity(s) + if serviceErr != nil { + multierror.Append(validationErrors, serviceErr) + } + if len(serviceWarn) > 0 { + warnings = append(warnings, serviceWarn...) + } + } + + for _, t := range tg.Tasks { + for _, s := range t.Services { + serviceWarn, serviceErr := v.validateServiceIdentity(s) + if serviceErr != nil { + multierror.Append(validationErrors, serviceErr) + } + if len(serviceWarn) > 0 { + warnings = append(warnings, serviceWarn...) + } } } } @@ -316,6 +332,22 @@ func (v *jobValidate) Validate(job *structs.Job) (warnings []error, err error) { return warnings, validationErrors.ErrorOrNil() } +func (v *jobValidate) validateServiceIdentity(s *structs.Service) (warnings []error, err error) { + if s.Identity != nil { + if !v.srv.config.UseConsulIdentity() { + return nil, fmt.Errorf("service %s defines an identity but server configuration for consul.use_identity is not true", s.Name) + } + + if s.Identity.Name == "" { + return nil, fmt.Errorf("identity for service %s has an empty name", s.Name) + } + } else if v.srv.config.UseConsulIdentity() && v.srv.config.ConsulServiceIdentity() == nil { + return nil, fmt.Errorf("service %s does not have an identity and no default service identity is provided", s.Name) + } + + return nil, nil +} + type memoryOversubscriptionValidate struct { srv *Server } @@ -377,42 +409,3 @@ func (j *Job) submissionController(args *structs.JobRegisterRequest) error { } return nil } - -// jobIdentityCreator adds implicit `identity` blocks for external services, -// like Consul and Vault. -type jobIdentityCreator struct { - srv *Server -} - -func (jobIdentityCreator) Name() string { - return "identityBlockCreator" -} - -func (jobIdentityCreator) Mutate(job *structs.Job) (*structs.Job, []error, error) { - for _, tg := range job.TaskGroups { - for _, s := range tg.Services { - if s.Provider != "consul" { - continue - } - - identityName := fmt.Sprintf("consul-service/%s", s.Name) - - // if there's an identity block present, overwrite its name and ServiceName, but - // keep the rest. Users can set custom aud, file and env properties to account - // for the specifics of their environments, but Name and ServiceName must always - // conform to a convention. - if s.Identity != nil { - s.Identity.Name = identityName - s.Identity.ServiceName = s.Name - } else { - s.Identity = &structs.WorkloadIdentity{ - Name: identityName, - Audience: []string{"consul.io"}, - ServiceName: s.Name, - } - } - } - } - - return job, nil, nil -} diff --git a/nomad/job_endpoint_hooks_test.go b/nomad/job_endpoint_hooks_test.go index c00aa8179..3653d6a99 100644 --- a/nomad/job_endpoint_hooks_test.go +++ b/nomad/job_endpoint_hooks_test.go @@ -7,11 +7,127 @@ import ( "testing" "github.com/hashicorp/nomad/ci" + "github.com/hashicorp/nomad/helper/pointer" + "github.com/hashicorp/nomad/nomad/mock" "github.com/hashicorp/nomad/nomad/structs" + "github.com/hashicorp/nomad/nomad/structs/config" "github.com/shoenig/test/must" "github.com/stretchr/testify/require" ) +func Test_jobValidate_Validate_consul_service(t *testing.T) { + ci.Parallel(t) + + testCases := []struct { + name string + inputService *structs.Service + inputConfig *Config + expectedWarn []error + expectedErr string + }{ + { + name: "no error when consul identity not enabled and services does not have an identity", + inputService: &structs.Service{ + Provider: "consul", + Name: "web", + }, + inputConfig: &Config{ + ConsulConfig: &config.ConsulConfig{ + UseIdentity: pointer.Of(false), + }, + }, + }, + { + name: "no error when consul identity is enabled and default service identity is provided", + inputService: &structs.Service{ + Provider: "consul", + Name: "web", + }, + inputConfig: &Config{ + ConsulConfig: &config.ConsulConfig{ + UseIdentity: pointer.Of(true), + ServiceIdentity: &config.WorkloadIdentityConfig{ + Audience: []string{"consul.io"}, + }, + }, + }, + }, + { + name: "no error when consul identity is enabled and service has a proper identity", + inputService: &structs.Service{ + Provider: "consul", + Name: "web", + Identity: &structs.WorkloadIdentity{ + Name: "consul-service/web", + Audience: []string{"consul.io", "nomad.dev"}, + File: true, + Env: false, + ServiceName: "web", + }, + }, + inputConfig: &Config{ + ConsulConfig: &config.ConsulConfig{ + UseIdentity: pointer.Of(true), + }, + }, + }, + { + name: "error when service defines identity but consul identity is disabled", + inputService: &structs.Service{ + Provider: "consul", + Name: "web", + Identity: &structs.WorkloadIdentity{ + Audience: []string{"consul.io", "nomad.dev"}, + File: true, + Env: false, + }, + }, + inputConfig: &Config{ + ConsulConfig: &config.ConsulConfig{ + UseIdentity: pointer.Of(false), + }, + }, + expectedErr: "server configuration for consul.use_identity is not true", + }, + { + name: "error when service does not define identity and consul identity is enabled but no default is provided", + inputService: &structs.Service{ + Provider: "consul", + Name: "web", + }, + inputConfig: &Config{ + ConsulConfig: &config.ConsulConfig{ + UseIdentity: pointer.Of(true), + }, + }, + expectedErr: "no default service identity is provided", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + tc.inputConfig.JobMaxPriority = 100 + impl := jobValidate{srv: &Server{ + config: tc.inputConfig, + }} + + job := mock.Job() + job.TaskGroups[0].Services = []*structs.Service{tc.inputService} + job.TaskGroups[0].Tasks[0].Services = []*structs.Service{tc.inputService} + + warn, err := impl.Validate(job) + must.Eq(t, tc.expectedWarn, warn) + + if len(tc.expectedErr) == 0 { + must.NoError(t, err) + } else { + must.Error(t, err) + must.ErrorContains(t, err, tc.expectedErr) + } + }) + } +} + func Test_jobImpliedConstraints_Mutate(t *testing.T) { ci.Parallel(t) @@ -806,96 +922,3 @@ func TestJob_submissionController(t *testing.T) { must.Nil(t, args.Submission) }) } - -func Test_jobIdentityCreator_Mutate(t *testing.T) { - ci.Parallel(t) - - testCases := []struct { - name string - inputJob *structs.Job - expectedOutputJob *structs.Job - }{ - { - name: "no mutation", - inputJob: &structs.Job{ - TaskGroups: []*structs.TaskGroup{{ - Services: []*structs.Service{{ - Provider: "nomad", - }}, - }}, - }, - expectedOutputJob: &structs.Job{ - TaskGroups: []*structs.TaskGroup{{ - Services: []*structs.Service{{ - Provider: "nomad", - }}, - }}, - }, - }, - { - name: "custom set identity, merge", - inputJob: &structs.Job{ - TaskGroups: []*structs.TaskGroup{{ - Services: []*structs.Service{{ - Provider: "consul", - Name: "web", - Identity: &structs.WorkloadIdentity{ - Name: "test", - Audience: []string{"consul.io", "nomad.dev"}, - File: true, - Env: false, - }, - }}, - }}, - }, - expectedOutputJob: &structs.Job{ - TaskGroups: []*structs.TaskGroup{{ - Services: []*structs.Service{{ - Provider: "consul", - Name: "web", - Identity: &structs.WorkloadIdentity{ - Name: "consul-service/web", - Audience: []string{"consul.io", "nomad.dev"}, - ServiceName: "web", - File: true, - Env: false, - }, - }}, - }}, - }, - }, - { - name: "mutate", - inputJob: &structs.Job{ - TaskGroups: []*structs.TaskGroup{{ - Services: []*structs.Service{{ - Provider: "consul", - Name: "web", - }}, - }}, - }, - expectedOutputJob: &structs.Job{ - TaskGroups: []*structs.TaskGroup{{ - Services: []*structs.Service{{ - Provider: "consul", - Name: "web", - Identity: &structs.WorkloadIdentity{ - Name: "consul-service/web", - Audience: []string{"consul.io"}, - ServiceName: "web", - }, - }}, - }}, - }, - }, - } - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - impl := jobIdentityCreator{} - actualJob, actualWarnings, actualError := impl.Mutate(tc.inputJob) - must.Eq(t, tc.expectedOutputJob, actualJob) - must.NoError(t, actualError) - must.Nil(t, actualWarnings) - }) - } -} diff --git a/nomad/structs/config/consul.go b/nomad/structs/config/consul.go index 1e0c26e42..5c338329a 100644 --- a/nomad/structs/config/consul.go +++ b/nomad/structs/config/consul.go @@ -154,7 +154,7 @@ type ConsulConfig struct { // "consul-service/${service_name}-${service_port}". // // ServiceIdentity is set on the server. - ServiceIdentity *WorkloadIdentity `mapstructure:"service_identity"` + ServiceIdentity *WorkloadIdentityConfig `mapstructure:"service_identity"` // TemplateIdentity is intended to reduce overhead for jobspec authors and make // for graceful upgrades without forcing rewrite of all jobspecs. If set, when a @@ -165,7 +165,7 @@ type ConsulConfig struct { // The name field of the identity is always set to "consul". // // TemplateIdentity is set on the server. - TemplateIdentity *WorkloadIdentity `mapstructure:"template_identity"` + TemplateIdentity *WorkloadIdentityConfig `mapstructure:"template_identity"` // ExtraKeysHCL is used by hcl to surface unexpected keys ExtraKeysHCL []string `mapstructure:",unusedKeys" json:"-"` @@ -296,12 +296,21 @@ func (c *ConsulConfig) Merge(b *ConsulConfig) *ConsulConfig { if b.UseIdentity != nil { result.UseIdentity = pointer.Of(*b.UseIdentity) } - if b.ServiceIdentity != nil { - result.ServiceIdentity = pointer.Of(*b.ServiceIdentity) + + if result.ServiceIdentity == nil && b.ServiceIdentity != nil { + sID := *b.ServiceIdentity + result.ServiceIdentity = &sID + } else if b.ServiceIdentity != nil { + result.ServiceIdentity = result.ServiceIdentity.Merge(b.ServiceIdentity) } - if b.TemplateIdentity != nil { - result.TemplateIdentity = pointer.Of(*b.TemplateIdentity) + + if result.TemplateIdentity == nil && b.TemplateIdentity != nil { + tID := *b.TemplateIdentity + result.TemplateIdentity = &tID + } else if b.TemplateIdentity != nil { + result.TemplateIdentity = result.TemplateIdentity.Merge(b.TemplateIdentity) } + return result } @@ -407,68 +416,3 @@ func (c *ConsulConfig) Copy() *ConsulConfig { ExtraKeysHCL: slices.Clone(c.ExtraKeysHCL), } } - -// WorkloadIdentity is the jobspec block which determines if and how a workload -// identity is exposed to tasks similar to the Vault block. -// -// This is a copy of WorkloadIdentity from nomad/structs package in order to -// avoid import cycles. -type WorkloadIdentity struct { - Name string `mapstructure:"name"` - - // Audience is the valid recipients for this identity (the "aud" JWT claim) - // and defaults to the identity's name. - Audience []string `mapstructure:"aud"` - - // Env injects the Workload Identity into the Task's environment if - // set. - Env bool `mapstructure:"env"` - - // File writes the Workload Identity into the Task's secrets directory - // if set. - File bool `mapstructure:"file"` - - // ServiceName is used to bind the identity to a correct Consul service. - ServiceName string `mapstructure:"-" json:"-"` -} - -func (wi *WorkloadIdentity) Copy() *WorkloadIdentity { - if wi == nil { - return nil - } - return &WorkloadIdentity{ - Name: wi.Name, - Audience: slices.Clone(wi.Audience), - Env: wi.Env, - File: wi.File, - ServiceName: wi.ServiceName, - } -} - -func (wi *WorkloadIdentity) Equal(other *WorkloadIdentity) bool { - if wi == nil || other == nil { - return wi == other - } - - if wi.Name != other.Name { - return false - } - - if !slices.Equal(wi.Audience, other.Audience) { - return false - } - - if wi.Env != other.Env { - return false - } - - if wi.File != other.File { - return false - } - - if wi.ServiceName != other.ServiceName { - return false - } - - return true -} diff --git a/nomad/structs/config/consul_test.go b/nomad/structs/config/consul_test.go index 5a717a660..2a9f451da 100644 --- a/nomad/structs/config/consul_test.go +++ b/nomad/structs/config/consul_test.go @@ -17,6 +17,7 @@ import ( "github.com/stretchr/testify/require" "github.com/hashicorp/nomad/ci" + "github.com/hashicorp/nomad/helper/pointer" ) func TestMain(m *testing.M) { @@ -93,12 +94,11 @@ func TestConsulConfig_Merge(t *testing.T) { ServerAutoJoin: &yes, ClientAutoJoin: &yes, UseIdentity: &yes, - ServiceIdentity: &WorkloadIdentity{ - Name: "test", - Audience: []string{"consul.io", "nomad.dev"}, - Env: false, - File: true, - ServiceName: "test", + ServiceIdentity: &WorkloadIdentityConfig{ + Name: "test", + Audience: []string{"consul.io", "nomad.dev"}, + Env: pointer.Of(false), + File: pointer.Of(true), }, ExtraKeysHCL: []string{"b", "2"}, } @@ -129,12 +129,11 @@ func TestConsulConfig_Merge(t *testing.T) { ServerAutoJoin: &yes, ClientAutoJoin: &yes, UseIdentity: &yes, - ServiceIdentity: &WorkloadIdentity{ - Name: "test", - Audience: []string{"consul.io", "nomad.dev"}, - Env: false, - File: true, - ServiceName: "test", + ServiceIdentity: &WorkloadIdentityConfig{ + Name: "test", + Audience: []string{"consul.io", "nomad.dev"}, + Env: pointer.Of(false), + File: pointer.Of(true), }, ExtraKeysHCL: []string{"a", "1"}, // not merged } diff --git a/nomad/structs/config/workload_id.go b/nomad/structs/config/workload_id.go new file mode 100644 index 000000000..664209c98 --- /dev/null +++ b/nomad/structs/config/workload_id.go @@ -0,0 +1,99 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: BUSL-1.1 + +package config + +import ( + "slices" + + "github.com/hashicorp/go-set" + "github.com/hashicorp/nomad/helper/pointer" +) + +// WorkloadIdentityConfig is the agent configuraion block used to define +// default workload identities. +// +// This based on the WorkloadIdentity struct from nomad/structs/workload_id.go +// and may need to be kept in sync. +type WorkloadIdentityConfig struct { + // Name is used to identity the workload identity. It is not expected to be + // set by users, but may have a default value. + Name string `mapstructure:"-" json:"-"` + + // Audience is the valid recipients for this identity (the "aud" JWT claim) + // and defaults to the identity's name. + Audience []string `mapstructure:"aud"` + + // Env injects the Workload Identity into the Task's environment if + // set. + Env *bool `mapstructure:"env"` + + // File writes the Workload Identity into the Task's secrets directory + // if set. + File *bool `mapstructure:"file"` +} + +func (wi *WorkloadIdentityConfig) Copy() *WorkloadIdentityConfig { + if wi == nil { + return nil + } + nwi := new(WorkloadIdentityConfig) + *nwi = *wi + nwi.Audience = slices.Clone(wi.Audience) + + if wi.Env != nil { + nwi.Env = pointer.Of(*wi.Env) + } + if wi.File != nil { + nwi.File = pointer.Of(*wi.File) + } + + return nwi +} + +func (wi *WorkloadIdentityConfig) Equal(other *WorkloadIdentityConfig) bool { + if wi == nil || other == nil { + return wi == other + } + + if wi.Name != other.Name { + return false + } + if !slices.Equal(wi.Audience, other.Audience) { + return false + } + if !pointer.Eq(wi.Env, other.Env) { + return false + } + if !pointer.Eq(wi.File, other.File) { + return false + } + + return true +} + +func (wi *WorkloadIdentityConfig) Merge(other *WorkloadIdentityConfig) *WorkloadIdentityConfig { + result := wi.Copy() + + if other.Name != "" { + result.Name = other.Name + } + + if len(result.Audience) == 0 { + result.Audience = slices.Clone(other.Audience) + } else if len(other.Audience) > 0 { + // Append incoming audiences avoiding duplicates. + audSet := set.From(result.Audience) + for _, aud := range other.Audience { + if !audSet.Contains(aud) { + audSet.Insert(aud) + result.Audience = append(result.Audience, aud) + } + } + } + + result.Env = pointer.Merge(result.Env, other.Env) + result.File = pointer.Merge(result.File, other.File) + + return result +} diff --git a/nomad/structs/config/workload_id_test.go b/nomad/structs/config/workload_id_test.go new file mode 100644 index 000000000..d3cbc12c3 --- /dev/null +++ b/nomad/structs/config/workload_id_test.go @@ -0,0 +1,242 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: BUSL-1.1 + +package config + +import ( + "testing" + + "github.com/hashicorp/nomad/ci" + "github.com/hashicorp/nomad/helper/pointer" + "github.com/shoenig/test/must" +) + +func TestWorkloadIdentityConfig_Copy(t *testing.T) { + ci.Parallel(t) + + original := &WorkloadIdentityConfig{ + Name: "test", + Audience: []string{"aud"}, + Env: pointer.Of(true), + File: pointer.Of(false), + } + + // Verify Copy() returns the same values but different pointer. + clone := original.Copy() + must.Eq(t, original, clone) + must.NotEqOp(t, original, clone) + + // Verify returned struct does not mutate original. + clone.Name = "clone" + clone.Audience = []string{"aud", "clone"} + clone.Env = pointer.Of(false) + clone.File = pointer.Of(true) + + must.NotEq(t, original, clone) + must.NotEqOp(t, original, clone) +} + +func TestWorkloadIdentityConfig_Equal(t *testing.T) { + ci.Parallel(t) + + testCases := []struct { + name string + a *WorkloadIdentityConfig + b *WorkloadIdentityConfig + expectEq bool + }{ + { + name: "equal", + a: &WorkloadIdentityConfig{ + Name: "test", + Audience: []string{"aud"}, + Env: pointer.Of(true), + File: pointer.Of(false), + }, + b: &WorkloadIdentityConfig{ + Name: "test", + Audience: []string{"aud"}, + Env: pointer.Of(true), + File: pointer.Of(false), + }, + expectEq: true, + }, + { + name: "different name", + a: &WorkloadIdentityConfig{ + Name: "a", + }, + b: &WorkloadIdentityConfig{ + Name: "b", + }, + expectEq: false, + }, + { + name: "different audience", + a: &WorkloadIdentityConfig{ + Audience: []string{"a"}, + }, + b: &WorkloadIdentityConfig{ + Audience: []string{"b"}, + }, + expectEq: false, + }, + { + name: "different env", + a: &WorkloadIdentityConfig{ + Env: pointer.Of(true), + }, + b: &WorkloadIdentityConfig{ + Env: pointer.Of(false), + }, + expectEq: false, + }, + { + name: "different env nil", + a: &WorkloadIdentityConfig{ + Env: pointer.Of(true), + }, + b: &WorkloadIdentityConfig{ + Env: nil, + }, + expectEq: false, + }, + { + name: "different file", + a: &WorkloadIdentityConfig{ + File: pointer.Of(true), + }, + b: &WorkloadIdentityConfig{ + File: pointer.Of(false), + }, + expectEq: false, + }, + { + name: "different file nil", + a: &WorkloadIdentityConfig{ + File: pointer.Of(true), + }, + b: &WorkloadIdentityConfig{ + File: nil, + }, + expectEq: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + if tc.expectEq { + must.True(t, tc.a.Equal(tc.b)) + } else { + must.False(t, tc.a.Equal(tc.b)) + } + }) + } +} + +func TestWorkloadIdentityConfig_Merge(t *testing.T) { + ci.Parallel(t) + + testCases := []struct { + name string + other *WorkloadIdentityConfig + expected *WorkloadIdentityConfig + }{ + { + name: "merge name", + other: &WorkloadIdentityConfig{ + Name: "other", + }, + expected: &WorkloadIdentityConfig{ + Name: "other", + Audience: []string{"aud"}, + Env: pointer.Of(true), + File: pointer.Of(false), + }, + }, + { + name: "merge audience", + other: &WorkloadIdentityConfig{ + Audience: []string{"aud", "aud", "other"}, + }, + expected: &WorkloadIdentityConfig{ + Name: "test", + Audience: []string{"aud", "other"}, + Env: pointer.Of(true), + File: pointer.Of(false), + }, + }, + { + name: "merge env", + other: &WorkloadIdentityConfig{ + Env: pointer.Of(false), + }, + expected: &WorkloadIdentityConfig{ + Name: "test", + Audience: []string{"aud"}, + Env: pointer.Of(false), + File: pointer.Of(false), + }, + }, + { + name: "merge file", + other: &WorkloadIdentityConfig{ + File: pointer.Of(true), + }, + expected: &WorkloadIdentityConfig{ + Name: "test", + Audience: []string{"aud"}, + Env: pointer.Of(true), + File: pointer.Of(true), + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + original := &WorkloadIdentityConfig{ + Name: "test", + Audience: []string{"aud"}, + Env: pointer.Of(true), + File: pointer.Of(false), + } + got := original.Merge(tc.other) + must.Eq(t, tc.expected, got) + }) + } +} + +func TestWorkloadIdentityConfig_Merge_multiple(t *testing.T) { + widConfig1 := &WorkloadIdentityConfig{ + Name: "wid1", + Audience: []string{"aud1"}, + Env: pointer.Of(true), + } + + widConfig2 := &WorkloadIdentityConfig{ + Name: "wid2", + Audience: []string{"aud2"}, + Env: pointer.Of(false), + } + + got12 := widConfig1.Merge(widConfig2) + must.Eq(t, &WorkloadIdentityConfig{ + Name: "wid2", + Audience: []string{"aud1", "aud2"}, + Env: pointer.Of(false), + }, got12) + + widConfig3 := &WorkloadIdentityConfig{ + Name: "wid3", + Audience: []string{"aud1", "aud2", "aud3"}, + File: pointer.Of(false), + } + + got123 := got12.Merge(widConfig3) + must.Eq(t, &WorkloadIdentityConfig{ + Name: "wid3", + Audience: []string{"aud1", "aud2", "aud3"}, + Env: pointer.Of(false), + File: pointer.Of(false), + }, got123) +} diff --git a/nomad/structs/workload_id.go b/nomad/structs/workload_id.go index e255ea47c..c15a51070 100644 --- a/nomad/structs/workload_id.go +++ b/nomad/structs/workload_id.go @@ -43,9 +43,9 @@ var ( // WorkloadIdentity is the jobspec block which determines if and how a workload // identity is exposed to tasks similar to the Vault block. // -// CAUTION: a copy of this struct definition lives in config/consul.go in order -// to avoid import cycles. If updating WorkloadIdentity, please remember to update -// its copy as well. +// CAUTION: a similar struct called WorkloadIdentityConfig lives in +// nomad/structs/config/workload_id.go and is used for agent configuration. +// Updates here may need to be applied there as well. type WorkloadIdentity struct { Name string