From 783d7fdc31494bd0731b845758c3b38fd664be09 Mon Sep 17 00:00:00 2001 From: James Rasell Date: Mon, 14 Mar 2022 09:21:20 +0100 Subject: [PATCH 1/3] jobspec: add service block provider parameter and validation. --- command/agent/job_endpoint.go | 1 + command/agent/job_endpoint_test.go | 2 + nomad/structs/consul.go | 8 +- nomad/structs/diff_test.go | 103 ++++++++++ nomad/structs/services.go | 82 +++++++- nomad/structs/services_test.go | 80 ++++++++ nomad/structs/structs.go | 30 ++- nomad/structs/structs_test.go | 319 ++++++++++++++++++++++++----- 8 files changed, 560 insertions(+), 65 deletions(-) diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index 46a0a60d6..e29fe751d 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -1372,6 +1372,7 @@ func ApiServicesToStructs(in []*api.Service, group bool) []*structs.Service { Meta: helper.CopyMapStringString(s.Meta), CanaryMeta: helper.CopyMapStringString(s.CanaryMeta), OnUpdate: s.OnUpdate, + Provider: s.Provider, } if l := len(s.Checks); l != 0 { diff --git a/command/agent/job_endpoint_test.go b/command/agent/job_endpoint_test.go index bbdd6479f..dfe1c8253 100644 --- a/command/agent/job_endpoint_test.go +++ b/command/agent/job_endpoint_test.go @@ -2902,6 +2902,7 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) { Services: []*structs.Service{ { Name: "groupserviceA", + Provider: "consul", Tags: []string{"a", "b"}, CanaryTags: []string{"d", "e"}, EnableTagOverride: true, @@ -2993,6 +2994,7 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) { Services: []*structs.Service{ { Name: "serviceA", + Provider: "consul", Tags: []string{"1", "2"}, CanaryTags: []string{"3", "4"}, EnableTagOverride: true, diff --git a/nomad/structs/consul.go b/nomad/structs/consul.go index aa934a1a2..2c988c30d 100644 --- a/nomad/structs/consul.go +++ b/nomad/structs/consul.go @@ -70,13 +70,17 @@ func (j *Job) ConsulUsages() map[string]*ConsulUsage { // Gather group services for _, service := range tg.Services { - m[namespace].Services = append(m[namespace].Services, service.Name) + if service.Provider == ServiceProviderConsul { + m[namespace].Services = append(m[namespace].Services, service.Name) + } } // Gather task services and KV usage for _, task := range tg.Tasks { for _, service := range task.Services { - m[namespace].Services = append(m[namespace].Services, service.Name) + if service.Provider == ServiceProviderConsul { + m[namespace].Services = append(m[namespace].Services, service.Name) + } } if len(task.Templates) > 0 { m[namespace].KV = true diff --git a/nomad/structs/diff_test.go b/nomad/structs/diff_test.go index ef950b90d..53b158262 100644 --- a/nomad/structs/diff_test.go +++ b/nomad/structs/diff_test.go @@ -2872,6 +2872,12 @@ func TestTaskGroupDiff(t *testing.T) { Old: "", New: "", }, + { + Type: DiffTypeNone, + Name: "Provider", + Old: "", + New: "", + }, { Type: DiffTypeEdited, Name: "TaskName", @@ -5601,6 +5607,10 @@ func TestTaskDiff(t *testing.T) { Old: "foo", New: "bar", }, + { + Type: DiffTypeNone, + Name: "Provider", + }, { Type: DiffTypeAdded, Name: "TaskName", @@ -5745,6 +5755,10 @@ func TestTaskDiff(t *testing.T) { Type: DiffTypeNone, Name: "PortLabel", }, + { + Type: DiffTypeNone, + Name: "Provider", + }, { Type: DiffTypeNone, Name: "TaskName", @@ -6264,6 +6278,10 @@ func TestTaskDiff(t *testing.T) { Old: "", New: "", }, + { + Type: DiffTypeNone, + Name: "Provider", + }, { Type: DiffTypeNone, Name: "TaskName", @@ -7347,6 +7365,10 @@ func TestServicesDiff(t *testing.T) { Old: "http", New: "https", }, + { + Type: DiffTypeNone, + Name: "Provider", + }, { Type: DiffTypeNone, Name: "TaskName", @@ -7432,6 +7454,10 @@ func TestServicesDiff(t *testing.T) { Name: "PortLabel", New: "http", }, + { + Type: DiffTypeNone, + Name: "Provider", + }, { Type: DiffTypeNone, Name: "TaskName", @@ -7491,6 +7517,10 @@ func TestServicesDiff(t *testing.T) { Name: "PortLabel", New: "https", }, + { + Type: DiffTypeNone, + Name: "Provider", + }, { Type: DiffTypeNone, Name: "TaskName", @@ -7556,6 +7586,9 @@ func TestServicesDiff(t *testing.T) { Name: "PortLabel", Old: "http", New: "https-redirect", + }, { + Type: DiffTypeNone, + Name: "Provider", }, { Type: DiffTypeNone, @@ -7627,6 +7660,10 @@ func TestServicesDiff(t *testing.T) { Old: "http", New: "http", }, + { + Type: DiffTypeNone, + Name: "Provider", + }, { Type: DiffTypeNone, Name: "TaskName", @@ -7654,6 +7691,72 @@ func TestServicesDiff(t *testing.T) { }, }, }, + { + Name: "Service with different provider", + Contextual: true, + Old: []*Service{ + { + Name: "webapp", + Provider: "nomad", + PortLabel: "http", + }, + }, + New: []*Service{ + { + Name: "webapp", + Provider: "consul", + PortLabel: "http", + }, + }, + Expected: []*ObjectDiff{ + { + Type: DiffTypeEdited, + Name: "Service", + Fields: []*FieldDiff{ + { + Type: DiffTypeNone, + Name: "AddressMode", + }, + { + Type: DiffTypeNone, + Name: "EnableTagOverride", + Old: "false", + New: "false", + }, + { + Type: DiffTypeNone, + Name: "Name", + Old: "webapp", + New: "webapp", + }, + { + Type: DiffTypeNone, + Name: "Namespace", + }, + { + Type: DiffTypeNone, + Name: "OnUpdate", + }, + { + Type: DiffTypeNone, + Name: "PortLabel", + Old: "http", + New: "http", + }, + { + Type: DiffTypeEdited, + Name: "Provider", + Old: "nomad", + New: "consul", + }, + { + Type: DiffTypeNone, + Name: "TaskName", + }, + }, + }, + }, + }, } for _, c := range cases { diff --git a/nomad/structs/services.go b/nomad/structs/services.go index 7c6a1c4e5..dc9209220 100644 --- a/nomad/structs/services.go +++ b/nomad/structs/services.go @@ -420,6 +420,15 @@ const ( AddressModeHost = "host" AddressModeDriver = "driver" AddressModeAlloc = "alloc" + + // ServiceProviderConsul is the default service provider and the way Nomad + // worked before native service discovery. + ServiceProviderConsul = "consul" + + // ServiceProviderNomad is the native service discovery provider. At the + // time of writing, there are a number of restrictions around its + // functionality and use. + ServiceProviderNomad = "nomad" ) // Service represents a Consul service definition @@ -468,6 +477,11 @@ type Service struct { // OnUpdate Specifies how the service and its checks should be evaluated // during an update OnUpdate string + + // Provider dictates which service discovery provider to use. This can be + // either ServiceProviderConsul or ServiceProviderNomad and defaults to the former when + // left empty by the operator. + Provider string } const ( @@ -504,7 +518,7 @@ func (s *Service) Copy() *Service { // Canonicalize interpolates values of Job, Task Group and Task in the Service // Name. This also generates check names, service id and check ids. -func (s *Service) Canonicalize(job string, taskGroup string, task string) { +func (s *Service) Canonicalize(job, taskGroup, task, jobNamespace string) { // Ensure empty lists are treated as null to avoid scheduler issues when // using DeepEquals if len(s.Tags) == 0 { @@ -528,10 +542,23 @@ func (s *Service) Canonicalize(job string, taskGroup string, task string) { check.Canonicalize(s.Name) } + // Set the provider to its default value. The value of consul ensures this + // new feature and parameter behaves in the same manner a previous versions + // which did not include this. + if s.Provider == "" { + s.Provider = ServiceProviderConsul + } + // Consul API returns "default" whether the namespace is empty or set as - // such, so we coerce our copy of the service to be the same. - if s.Namespace == "" { + // such, so we coerce our copy of the service to be the same if using the + // consul provider. + // + // When using ServiceProviderNomad, set the namespace to that of the job. This + // makes modifications and diffs on the service correct. + if s.Namespace == "" && s.Provider == ServiceProviderConsul { s.Namespace = "default" + } else if s.Provider == ServiceProviderNomad { + s.Namespace = jobNamespace } } @@ -563,6 +590,26 @@ func (s *Service) Validate() error { mErr.Errors = append(mErr.Errors, fmt.Errorf("Service on_update must be %q, %q, or %q; not %q", OnUpdateRequireHealthy, OnUpdateIgnoreWarn, OnUpdateIgnore, s.OnUpdate)) } + // Up until this point, all service validation has been independent of the + // provider. From this point on, we have different validation paths. We can + // also catch an incorrect provider parameter. + switch s.Provider { + case ServiceProviderConsul: + s.validateConsulService(&mErr) + case ServiceProviderNomad: + s.validateNomadService(&mErr) + default: + mErr.Errors = append(mErr.Errors, fmt.Errorf("Service provider must be %q, or %q; not %q", + ServiceProviderConsul, ServiceProviderNomad, s.Provider)) + } + + return mErr.ErrorOrNil() +} + +// validateConsulService performs validation on a service which is using the +// consul provider. +func (s *Service) validateConsulService(mErr *multierror.Error) { + // check checks for _, c := range s.Checks { if s.PortLabel == "" && c.PortLabel == "" && c.RequiresPort() { @@ -595,8 +642,23 @@ func (s *Service) Validate() error { mErr.Errors = append(mErr.Errors, fmt.Errorf("Service %s is Connect Native and requires setting the task", s.Name)) } } +} - return mErr.ErrorOrNil() +// validateNomadService performs validation on a service which is using the +// nomad provider. +func (s *Service) validateNomadService(mErr *multierror.Error) { + + // Service blocks for the Nomad provider do not support checks. We perform + // a nil check, as an empty check list is nilled within the service + // canonicalize function. + if s.Checks != nil { + mErr.Errors = append(mErr.Errors, errors.New("Service with provider nomad cannot include Check blocks")) + } + + // Services using the Nomad provider do not support Consul connect. + if s.Connect != nil { + mErr.Errors = append(mErr.Errors, errors.New("Service with provider nomad cannot include Connect blocks")) + } } // ValidateName checks if the service Name is valid and should be called after @@ -614,7 +676,8 @@ func (s *Service) ValidateName(name string) error { } // Hash returns a base32 encoded hash of a Service's contents excluding checks -// as they're hashed independently. +// as they're hashed independently and the provider in order to not cause churn +// during cluster upgrades. func (s *Service) Hash(allocID, taskName string, canary bool) string { h := sha1.New() hashString(h, allocID) @@ -632,6 +695,11 @@ func (s *Service) Hash(allocID, taskName string, canary bool) string { hashString(h, s.OnUpdate) hashString(h, s.Namespace) + // Don't hash the provider parameter, so we don't cause churn of all + // registered services when upgrading Nomad versions. The provider is not + // used at the level the hash is and therefore is not needed to tell + // whether the service has changed. + // Base32 is used for encoding the hash as sha1 hashes can always be // encoded without padding, only 4 bytes larger than base64, and saves // 8 bytes vs hex. Since these hashes are used in Consul URLs it's nice @@ -687,6 +755,10 @@ func (s *Service) Equals(o *Service) bool { return s == o } + if s.Provider != o.Provider { + return false + } + if s.Namespace != o.Namespace { return false } diff --git a/nomad/structs/services_test.go b/nomad/structs/services_test.go index 9375366d7..12c98ecaa 100644 --- a/nomad/structs/services_test.go +++ b/nomad/structs/services_test.go @@ -1,6 +1,8 @@ package structs import ( + "errors" + "github.com/hashicorp/go-multierror" "testing" "time" @@ -1472,3 +1474,81 @@ func TestConsulMeshGateway_Validate(t *testing.T) { require.NoError(t, err) }) } + +func TestService_validateNomadService(t *testing.T) { + t.Parallel() + + testCases := []struct { + inputService *Service + inputErr *multierror.Error + expectedOutputErrors []error + name string + }{ + { + inputService: &Service{ + Name: "webapp", + PortLabel: "http", + Namespace: "default", + Provider: "nomad", + }, + inputErr: &multierror.Error{}, + expectedOutputErrors: []error{}, + name: "valid service", + }, + { + inputService: &Service{ + Name: "webapp", + PortLabel: "http", + Namespace: "default", + Provider: "nomad", + Checks: []*ServiceCheck{ + {Name: "some-check"}, + }, + }, + inputErr: &multierror.Error{}, + expectedOutputErrors: []error{errors.New("Service with provider nomad cannot include Check blocks")}, + name: "invalid service due to checks", + }, + { + inputService: &Service{ + Name: "webapp", + PortLabel: "http", + Namespace: "default", + Provider: "nomad", + Connect: &ConsulConnect{ + Native: true, + }, + }, + inputErr: &multierror.Error{}, + expectedOutputErrors: []error{errors.New("Service with provider nomad cannot include Connect blocks")}, + name: "invalid service due to connect", + }, + { + inputService: &Service{ + Name: "webapp", + PortLabel: "http", + Namespace: "default", + Provider: "nomad", + Connect: &ConsulConnect{ + Native: true, + }, + Checks: []*ServiceCheck{ + {Name: "some-check"}, + }, + }, + inputErr: &multierror.Error{}, + expectedOutputErrors: []error{ + errors.New("Service with provider nomad cannot include Check blocks"), + errors.New("Service with provider nomad cannot include Connect blocks"), + }, + name: "invalid service due to checks and connect", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + tc.inputService.validateNomadService(tc.inputErr) + require.ElementsMatch(t, tc.expectedOutputErrors, tc.inputErr.Errors) + }) + } +} diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 9188c8204..797a5c511 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -6195,7 +6195,7 @@ func (tg *TaskGroup) Canonicalize(job *Job) { } for _, service := range tg.Services { - service.Canonicalize(job.Name, tg.Name, "group") + service.Canonicalize(job.Name, tg.Name, "group", job.Namespace) } for _, network := range tg.Networks { @@ -6491,6 +6491,10 @@ func (tg *TaskGroup) validateServices() error { var mErr multierror.Error knownTasks := make(map[string]struct{}) + // Track the providers used for this task group. Currently, Nomad only + // allows the use of a single service provider within a task group. + configuredProviders := make(map[string]struct{}) + // Create a map of known tasks and their services so we can compare // vs the group-level services and checks for _, task := range tg.Tasks { @@ -6504,9 +6508,22 @@ func (tg *TaskGroup) validateServices() error { mErr.Errors = append(mErr.Errors, fmt.Errorf("Check %s is invalid: only task group service checks can be assigned tasks", check.Name)) } } + + // Add the service provider to the tracking, if it has not already + // been seen. + if _, ok := configuredProviders[service.Provider]; !ok { + configuredProviders[service.Provider] = struct{}{} + } } } for i, service := range tg.Services { + + // Add the service provider to the tracking, if it has not already been + // seen. + if _, ok := configuredProviders[service.Provider]; !ok { + configuredProviders[service.Provider] = struct{}{} + } + if err := service.Validate(); err != nil { outer := fmt.Errorf("Service[%d] %s validation failed: %s", i, service.Name, err) mErr.Errors = append(mErr.Errors, outer) @@ -6535,6 +6552,15 @@ func (tg *TaskGroup) validateServices() error { } } } + + // The initial feature release of native service discovery only allows for + // a single service provider to be used across all services in a task + // group. + if len(configuredProviders) > 1 { + mErr.Errors = append(mErr.Errors, + errors.New("Multiple service providers used: task group services must use the same provider")) + } + return mErr.ErrorOrNil() } @@ -6946,7 +6972,7 @@ func (t *Task) Canonicalize(job *Job, tg *TaskGroup) { } for _, service := range t.Services { - service.Canonicalize(job.Name, tg.Name, t.Name) + service.Canonicalize(job.Name, tg.Name, t.Name, job.Namespace) } // If Resources are nil initialize them to defaults, otherwise canonicalize diff --git a/nomad/structs/structs_test.go b/nomad/structs/structs_test.go index 40b343910..ddfc242fe 100644 --- a/nomad/structs/structs_test.go +++ b/nomad/structs/structs_test.go @@ -371,6 +371,7 @@ func testJob() *Job { { Name: "${TASK}-frontend", PortLabel: "http", + Provider: "consul", }, }, Tasks: []*Task{ @@ -1203,7 +1204,8 @@ func TestTaskGroup_Validate(t *testing.T) { Name: "group-a", Services: []*Service{ { - Name: "service-a", + Name: "service-a", + Provider: "consul", Checks: []*ServiceCheck{ { Name: "check-a", @@ -1225,6 +1227,47 @@ func TestTaskGroup_Validate(t *testing.T) { expected = `Check check-a invalid: only script and gRPC checks should have tasks` require.Contains(t, err.Error(), expected) + tg = &TaskGroup{ + Name: "group-a", + Services: []*Service{ + { + Name: "service-a", + Provider: "nomad", + }, + { + Name: "service-b", + Provider: "consul", + }, + }, + Tasks: []*Task{{Name: "task-a"}}, + } + err = tg.Validate(&Job{}) + expected = "Multiple service providers used: task group services must use the same provider" + require.Contains(t, err.Error(), expected) + + tg = &TaskGroup{ + Name: "group-a", + Services: []*Service{ + { + Name: "service-a", + Provider: "nomad", + }, + }, + Tasks: []*Task{ + { + Name: "task-a", + Services: []*Service{ + { + Name: "service-b", + Provider: "consul", + }, + }, + }, + }, + } + err = tg.Validate(&Job{}) + expected = "Multiple service providers used: task group services must use the same provider" + require.Contains(t, err.Error(), expected) } func TestTaskGroupNetwork_Validate(t *testing.T) { @@ -1689,6 +1732,7 @@ func TestNetworkResource_Copy(t *testing.T) { func TestTask_Validate_Services(t *testing.T) { s1 := &Service{ Name: "service-name", + Provider: "consul", PortLabel: "bar", Checks: []*ServiceCheck{ { @@ -1711,15 +1755,18 @@ func TestTask_Validate_Services(t *testing.T) { s2 := &Service{ Name: "service-name", + Provider: "consul", PortLabel: "bar", } s3 := &Service{ Name: "service-A", + Provider: "consul", PortLabel: "a", } s4 := &Service{ Name: "service-A", + Provider: "consul", PortLabel: "b", } @@ -1812,25 +1859,30 @@ func TestTask_Validate_Service_AddressMode_Ok(t *testing.T) { { // https://github.com/hashicorp/nomad/issues/3681#issuecomment-357274177 Name: "DriverModeWithLabel", + Provider: "consul", PortLabel: "http", AddressMode: AddressModeDriver, }, { Name: "DriverModeWithPort", + Provider: "consul", PortLabel: "80", AddressMode: AddressModeDriver, }, { Name: "HostModeWithLabel", + Provider: "consul", PortLabel: "http", AddressMode: AddressModeHost, }, { Name: "HostModeWithoutLabel", + Provider: "consul", AddressMode: AddressModeHost, }, { Name: "DriverModeWithoutLabel", + Provider: "consul", AddressMode: AddressModeDriver, }, } @@ -2030,6 +2082,7 @@ func TestTask_Validate_Service_Check_AddressMode(t *testing.T) { { Service: &Service{ Name: "invalid-driver", + Provider: "consul", PortLabel: "80", AddressMode: "host", }, @@ -2054,6 +2107,7 @@ func TestTask_Validate_Service_Check_AddressMode(t *testing.T) { { Service: &Service{ Name: "http-driver-fail-2", + Provider: "consul", PortLabel: "80", AddressMode: "driver", Checks: []*ServiceCheck{ @@ -2071,6 +2125,7 @@ func TestTask_Validate_Service_Check_AddressMode(t *testing.T) { { Service: &Service{ Name: "http-driver-fail-3", + Provider: "consul", PortLabel: "80", AddressMode: "driver", Checks: []*ServiceCheck{ @@ -2088,6 +2143,7 @@ func TestTask_Validate_Service_Check_AddressMode(t *testing.T) { { Service: &Service{ Name: "http-driver-passes", + Provider: "consul", PortLabel: "80", AddressMode: "driver", Checks: []*ServiceCheck{ @@ -2117,7 +2173,8 @@ func TestTask_Validate_Service_Check_AddressMode(t *testing.T) { }, { Service: &Service{ - Name: "empty-address-3673-passes-1", + Name: "empty-address-3673-passes-1", + Provider: "consul", Checks: []*ServiceCheck{ { Name: "valid-port-label", @@ -2143,7 +2200,8 @@ func TestTask_Validate_Service_Check_AddressMode(t *testing.T) { }, { Service: &Service{ - Name: "empty-address-3673-fails", + Name: "empty-address-3673-fails", + Provider: "consul", Checks: []*ServiceCheck{ { Name: "empty-is-not-ok", @@ -2192,8 +2250,9 @@ func TestTask_Validate_Service_Check_GRPC(t *testing.T) { Timeout: time.Second, } service := &Service{ - Name: "test", - Checks: []*ServiceCheck{invalidGRPC}, + Name: "test", + Provider: "consul", + Checks: []*ServiceCheck{invalidGRPC}, } assert.Error(t, service.Validate()) @@ -3119,6 +3178,7 @@ func BenchmarkEncodeDecode(b *testing.B) { func TestInvalidServiceCheck(t *testing.T) { s := Service{ Name: "service-name", + Provider: "consul", PortLabel: "bar", Checks: []*ServiceCheck{ { @@ -3133,6 +3193,7 @@ func TestInvalidServiceCheck(t *testing.T) { s = Service{ Name: "service.name", + Provider: "consul", PortLabel: "bar", } if err := s.ValidateName(s.Name); err == nil { @@ -3141,6 +3202,7 @@ func TestInvalidServiceCheck(t *testing.T) { s = Service{ Name: "-my-service", + Provider: "consul", PortLabel: "bar", } if err := s.Validate(); err == nil { @@ -3149,6 +3211,7 @@ func TestInvalidServiceCheck(t *testing.T) { s = Service{ Name: "my-service-${NOMAD_META_FOO}", + Provider: "consul", PortLabel: "bar", } if err := s.Validate(); err != nil { @@ -3157,6 +3220,7 @@ func TestInvalidServiceCheck(t *testing.T) { s = Service{ Name: "my_service-${NOMAD_META_FOO}", + Provider: "consul", PortLabel: "bar", } if err := s.Validate(); err == nil { @@ -3165,6 +3229,7 @@ func TestInvalidServiceCheck(t *testing.T) { s = Service{ Name: "abcdef0123456789-abcdef0123456789-abcdef0123456789-abcdef0123456", + Provider: "consul", PortLabel: "bar", } if err := s.ValidateName(s.Name); err == nil { @@ -3172,7 +3237,8 @@ func TestInvalidServiceCheck(t *testing.T) { } s = Service{ - Name: "service-name", + Name: "service-name", + Provider: "consul", Checks: []*ServiceCheck{ { Name: "check-tcp", @@ -3194,7 +3260,8 @@ func TestInvalidServiceCheck(t *testing.T) { } s = Service{ - Name: "service-name", + Name: "service-name", + Provider: "consul", Checks: []*ServiceCheck{ { Name: "check-script", @@ -3210,7 +3277,8 @@ func TestInvalidServiceCheck(t *testing.T) { } s = Service{ - Name: "service-name", + Name: "service-name", + Provider: "consul", Checks: []*ServiceCheck{ { Name: "tcp-check", @@ -3261,62 +3329,198 @@ func TestDistinctCheckID(t *testing.T) { } func TestService_Canonicalize(t *testing.T) { - job := "example" - taskGroup := "cache" - task := "redis" - - s := Service{ - Name: "${TASK}-db", + testCases := []struct { + inputService *Service + inputJob string + inputTaskGroup string + inputTask string + inputJobNamespace string + expectedOutputService *Service + name string + }{ + { + inputService: &Service{ + Name: "${TASK}-db", + }, + inputJob: "example", + inputTaskGroup: "cache", + inputTask: "redis", + inputJobNamespace: "platform", + expectedOutputService: &Service{ + Name: "redis-db", + Provider: "consul", + Namespace: "default", + }, + name: "interpolate task in name", + }, + { + inputService: &Service{ + Name: "db", + }, + inputJob: "example", + inputTaskGroup: "cache", + inputTask: "redis", + inputJobNamespace: "platform", + expectedOutputService: &Service{ + Name: "db", + Provider: "consul", + Namespace: "default", + }, + name: "no interpolation in name", + }, + { + inputService: &Service{ + Name: "${JOB}-${TASKGROUP}-${TASK}-db", + }, + inputJob: "example", + inputTaskGroup: "cache", + inputTask: "redis", + inputJobNamespace: "platform", + expectedOutputService: &Service{ + Name: "example-cache-redis-db", + Provider: "consul", + Namespace: "default", + }, + name: "interpolate job, taskgroup and task in name", + }, + { + inputService: &Service{ + Name: "${BASE}-db", + }, + inputJob: "example", + inputTaskGroup: "cache", + inputTask: "redis", + inputJobNamespace: "platform", + expectedOutputService: &Service{ + Name: "example-cache-redis-db", + Provider: "consul", + Namespace: "default", + }, + name: "interpolate base in name", + }, + { + inputService: &Service{ + Name: "db", + Provider: "nomad", + }, + inputJob: "example", + inputTaskGroup: "cache", + inputTask: "redis", + inputJobNamespace: "platform", + expectedOutputService: &Service{ + Name: "db", + Provider: "nomad", + Namespace: "platform", + }, + name: "nomad provider", + }, } - s.Canonicalize(job, taskGroup, task) - if s.Name != "redis-db" { - t.Fatalf("Expected name: %v, Actual: %v", "redis-db", s.Name) + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + tc.inputService.Canonicalize(tc.inputJob, tc.inputTaskGroup, tc.inputTask, tc.inputJobNamespace) + assert.Equal(t, tc.expectedOutputService, tc.inputService) + }) } - - s.Name = "db" - s.Canonicalize(job, taskGroup, task) - if s.Name != "db" { - t.Fatalf("Expected name: %v, Actual: %v", "redis-db", s.Name) - } - - s.Name = "${JOB}-${TASKGROUP}-${TASK}-db" - s.Canonicalize(job, taskGroup, task) - if s.Name != "example-cache-redis-db" { - t.Fatalf("Expected name: %v, Actual: %v", "example-cache-redis-db", s.Name) - } - - s.Name = "${BASE}-db" - s.Canonicalize(job, taskGroup, task) - if s.Name != "example-cache-redis-db" { - t.Fatalf("Expected name: %v, Actual: %v", "example-cache-redis-db", s.Name) - } - } func TestService_Validate(t *testing.T) { - s := Service{ - Name: "testservice", + testCases := []struct { + inputService *Service + expectedError bool + expectedErrorContains string + name string + }{ + { + inputService: &Service{ + Name: "testservice", + }, + expectedError: false, + name: "base service", + }, + { + inputService: &Service{ + Name: "testservice", + Connect: &ConsulConnect{ + Native: true, + }, + }, + expectedError: true, + expectedErrorContains: "Connect Native and requires setting the task", + name: "Native Connect without task name", + }, + { + inputService: &Service{ + Name: "testservice", + TaskName: "testtask", + Connect: &ConsulConnect{ + Native: true, + }, + }, + expectedError: false, + name: "Native Connect with task name", + }, + { + inputService: &Service{ + Name: "testservice", + TaskName: "testtask", + Connect: &ConsulConnect{ + Native: true, + SidecarService: &ConsulSidecarService{}, + }, + }, + expectedError: true, + expectedErrorContains: "Consul Connect must be exclusively native", + name: "Native Connect with Sidecar", + }, + { + inputService: &Service{ + Name: "testservice", + Provider: "nomad", + Checks: []*ServiceCheck{ + { + Name: "servicecheck", + }, + }, + }, + expectedError: true, + expectedErrorContains: "Service with provider nomad cannot include Check blocks", + name: "provider nomad with checks", + }, + { + inputService: &Service{ + Name: "testservice", + Provider: "nomad", + Connect: &ConsulConnect{ + Native: true, + }, + }, + expectedError: true, + expectedErrorContains: "Service with provider nomad cannot include Connect blocks", + name: "provider nomad with connect", + }, + { + inputService: &Service{ + Name: "testservice", + Provider: "nomad", + }, + expectedError: false, + name: "provider nomad valid", + }, } - s.Canonicalize("testjob", "testgroup", "testtask") - - // Base service should be valid - require.NoError(t, s.Validate()) - - // Native Connect requires task name on service - s.Connect = &ConsulConnect{ - Native: true, + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + tc.inputService.Canonicalize("testjob", "testgroup", "testtask", "testnamespace") + err := tc.inputService.Validate() + if tc.expectedError { + assert.Error(t, err) + assert.Contains(t, err.Error(), tc.expectedErrorContains) + } else { + assert.NoError(t, err) + } + }) } - require.Error(t, s.Validate()) - - // Native Connect should work with task name on service set - s.TaskName = "testtask" - require.NoError(t, s.Validate()) - - // Native Connect + Sidecar should be invalid - s.Connect.SidecarService = &ConsulSidecarService{} - require.Error(t, s.Validate()) } func TestService_Equals(t *testing.T) { @@ -3324,7 +3528,7 @@ func TestService_Equals(t *testing.T) { Name: "testservice", } - s.Canonicalize("testjob", "testgroup", "testtask") + s.Canonicalize("testjob", "testgroup", "testtask", "default") o := s.Copy() @@ -3362,6 +3566,9 @@ func TestService_Equals(t *testing.T) { o.EnableTagOverride = true assertDiff() + + o.Provider = "nomad" + assertDiff() } func TestJob_ExpandServiceNames(t *testing.T) { From 4a334c1721bcad842db1a10a8cfc94223288793a Mon Sep 17 00:00:00 2001 From: James Rasell Date: Mon, 14 Mar 2022 10:00:53 +0100 Subject: [PATCH 2/3] hcl1: add service block provider parameter. --- jobspec/parse_service.go | 1 + jobspec/parse_test.go | 26 ++++++++++++++++++++++ jobspec/test-fixtures/service-provider.hcl | 14 ++++++++++++ 3 files changed, 41 insertions(+) create mode 100644 jobspec/test-fixtures/service-provider.hcl diff --git a/jobspec/parse_service.go b/jobspec/parse_service.go index fe679805b..9ca32e352 100644 --- a/jobspec/parse_service.go +++ b/jobspec/parse_service.go @@ -51,6 +51,7 @@ func parseService(o *ast.ObjectItem) (*api.Service, error) { "meta", "canary_meta", "on_update", + "provider", } if err := checkHCLKeys(o.Val, valid); err != nil { return nil, err diff --git a/jobspec/parse_test.go b/jobspec/parse_test.go index aa219c117..c774cb335 100644 --- a/jobspec/parse_test.go +++ b/jobspec/parse_test.go @@ -1763,6 +1763,32 @@ func TestParse(t *testing.T) { }, false, }, + { + "service-provider.hcl", + &api.Job{ + ID: stringToPtr("service-provider"), + Name: stringToPtr("service-provider"), + TaskGroups: []*api.TaskGroup{ + { + Count: intToPtr(5), + Name: stringToPtr("group"), + Tasks: []*api.Task{ + { + Name: "task", + Driver: "docker", + Services: []*api.Service{ + { + Name: "service-provider", + Provider: "nomad", + }, + }, + }, + }, + }, + }, + }, + false, + }, } for _, tc := range cases { diff --git a/jobspec/test-fixtures/service-provider.hcl b/jobspec/test-fixtures/service-provider.hcl new file mode 100644 index 000000000..6a31599d1 --- /dev/null +++ b/jobspec/test-fixtures/service-provider.hcl @@ -0,0 +1,14 @@ +job "service-provider" { + group "group" { + count = 5 + + task "task" { + driver = "docker" + + service { + name = "service-provider" + provider = "nomad" + } + } + } +} From d18f861530905255ab82118884719d8e3b36fd23 Mon Sep 17 00:00:00 2001 From: James Rasell Date: Mon, 14 Mar 2022 10:01:20 +0100 Subject: [PATCH 3/3] api: add service block provider parameter. --- api/jobs_test.go | 1 + api/services.go | 14 ++++++++++++++ api/services_test.go | 1 + 3 files changed, 16 insertions(+) diff --git a/api/jobs_test.go b/api/jobs_test.go index ed5909c28..0f9ba8855 100644 --- a/api/jobs_test.go +++ b/api/jobs_test.go @@ -742,6 +742,7 @@ func TestJobs_Canonicalize(t *testing.T) { PortLabel: "db", AddressMode: "auto", OnUpdate: "require_healthy", + Provider: "consul", Checks: []ServiceCheck{ { Name: "alive", diff --git a/api/services.go b/api/services.go index 9017b1396..d927c514e 100644 --- a/api/services.go +++ b/api/services.go @@ -116,12 +116,21 @@ type Service struct { CanaryMeta map[string]string `hcl:"canary_meta,block"` TaskName string `mapstructure:"task" hcl:"task,optional"` OnUpdate string `mapstructure:"on_update" hcl:"on_update,optional"` + + // Provider defines which backend system provides the service registration + // mechanism for this service. This supports either structs.ProviderConsul + // or structs.ProviderNomad and defaults for the former. + Provider string `hcl:"provider,optional"` } const ( OnUpdateRequireHealthy = "require_healthy" OnUpdateIgnoreWarn = "ignore_warnings" OnUpdateIgnore = "ignore" + + // ServiceProviderConsul is the default provider for services when no + // parameter is set. + ServiceProviderConsul = "consul" ) // Canonicalize the Service by ensuring its name and address mode are set. Task @@ -145,6 +154,11 @@ func (s *Service) Canonicalize(t *Task, tg *TaskGroup, job *Job) { s.OnUpdate = OnUpdateRequireHealthy } + // Default the service provider. + if s.Provider == "" { + s.Provider = ServiceProviderConsul + } + s.Connect.Canonicalize() // Canonicalize CheckRestart on Checks and merge Service.CheckRestart diff --git a/api/services_test.go b/api/services_test.go index 648c975d8..28dd5d963 100644 --- a/api/services_test.go +++ b/api/services_test.go @@ -21,6 +21,7 @@ func TestService_Canonicalize(t *testing.T) { require.Equal(t, fmt.Sprintf("%s-%s-%s", *j.Name, *tg.Name, task.Name), s.Name) require.Equal(t, "auto", s.AddressMode) require.Equal(t, OnUpdateRequireHealthy, s.OnUpdate) + require.Equal(t, ServiceProviderConsul, s.Provider) } func TestServiceCheck_Canonicalize(t *testing.T) {