diff --git a/api/services.go b/api/services.go index 50932ccf043..e561189aa16 100644 --- a/api/services.go +++ b/api/services.go @@ -189,9 +189,10 @@ func (cc *ConsulConnect) Canonicalize() { // ConsulSidecarService represents a Consul Connect SidecarService jobspec // stanza. type ConsulSidecarService struct { - Tags []string `hcl:"tags,optional"` - Port string `hcl:"port,optional"` - Proxy *ConsulProxy `hcl:"proxy,block"` + Tags []string `hcl:"tags,optional"` + Port string `hcl:"port,optional"` + Proxy *ConsulProxy `hcl:"proxy,block"` + Checks []ServiceCheck `hcl:"check,block"` } func (css *ConsulSidecarService) Canonicalize() { diff --git a/command/agent/consul/connect.go b/command/agent/consul/connect.go index 4a52d05bb01..f94d192282f 100644 --- a/command/agent/consul/connect.go +++ b/command/agent/consul/connect.go @@ -105,12 +105,9 @@ func connectSidecarRegistration(serviceId string, css *structs.ConsulSidecarServ return nil, err } - return &api.AgentServiceRegistration{ - Tags: helper.CopySliceString(css.Tags), - Port: cMapping.Value, - Address: cMapping.HostIP, - Proxy: proxy, - Checks: api.AgentServiceChecks{ + var checks []*api.AgentServiceCheck + if len(css.Checks) == 0 { + checks = api.AgentServiceChecks{ { Name: "Connect Sidecar Listening", TCP: net.JoinHostPort(cMapping.HostIP, strconv.Itoa(cMapping.Value)), @@ -120,7 +117,25 @@ func connectSidecarRegistration(serviceId string, css *structs.ConsulSidecarServ Name: "Connect Sidecar Aliasing " + serviceId, AliasService: serviceId, }, - }, + } + } else { + checks = make([]*api.AgentServiceCheck, len(css.Checks)) + for i, c := range css.Checks { + check, err := createCheckReg(serviceId, "", c, cMapping.HostIP, cMapping.Value, "") + if err != nil { + return nil, fmt.Errorf("failed to register check %v: %w", c.Name, err) + } + + checks[i] = &check.AgentServiceCheck + } + } + + return &api.AgentServiceRegistration{ + Tags: helper.CopySliceString(css.Tags), + Port: cMapping.Value, + Address: cMapping.HostIP, + Proxy: proxy, + Checks: checks, }, nil } diff --git a/command/agent/consul/connect_test.go b/command/agent/consul/connect_test.go index 68eb908e67c..8c58ec699bc 100644 --- a/command/agent/consul/connect_test.go +++ b/command/agent/consul/connect_test.go @@ -79,6 +79,42 @@ func TestConnect_newConnect(t *testing.T) { }, }, asr.SidecarService) }) + + t.Run("with sidecar and custom check", func(t *testing.T) { + asr, err := newConnect("redis-service-id", "redis", &structs.ConsulConnect{ + Native: false, + SidecarService: &structs.ConsulSidecarService{ + Tags: []string{"foo", "bar"}, + Port: "connect-proxy-redis", + Checks: []*structs.ServiceCheck{ + { + Type: "tcp", + Interval: 5 * time.Second, + Timeout: 2 * time.Second, + }, + }, + }, + }, testConnectNetwork, testConnectPorts) + require.NoError(t, err) + require.Equal(t, &api.AgentServiceRegistration{ + Tags: []string{"foo", "bar"}, + Port: 3000, + Address: "192.168.30.1", + Proxy: &api.AgentServiceConnectProxyConfig{ + Config: map[string]interface{}{ + "bind_address": "0.0.0.0", + "bind_port": 3000, + }, + }, + Checks: api.AgentServiceChecks{ + { + TCP: "192.168.30.1:3000", + Interval: "5s", + Timeout: "2s", + }, + }, + }, asr.SidecarService) + }) } func TestConnect_connectSidecarRegistration(t *testing.T) { diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index f218d159f2c..4bca60aeda2 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -1291,51 +1291,57 @@ func ApiServicesToStructs(in []*api.Service) []*structs.Service { Meta: helper.CopyMapStringString(s.Meta), CanaryMeta: helper.CopyMapStringString(s.CanaryMeta), OnUpdate: s.OnUpdate, + Checks: apiServiceChecksToStructs(s.Checks, s.OnUpdate), // Inherit OnUpdate from service by default + Connect: ApiConsulConnectToStructs(s.Connect), } - if l := len(s.Checks); l != 0 { - out[i].Checks = make([]*structs.ServiceCheck, l) - for j, check := range s.Checks { - onUpdate := s.OnUpdate // Inherit from service as default - if check.OnUpdate != "" { - onUpdate = check.OnUpdate - } - out[i].Checks[j] = &structs.ServiceCheck{ - Name: check.Name, - Type: check.Type, - Command: check.Command, - Args: check.Args, - Path: check.Path, - Protocol: check.Protocol, - PortLabel: check.PortLabel, - Expose: check.Expose, - AddressMode: check.AddressMode, - Interval: check.Interval, - Timeout: check.Timeout, - InitialStatus: check.InitialStatus, - TLSSkipVerify: check.TLSSkipVerify, - Header: check.Header, - Method: check.Method, - Body: check.Body, - GRPCService: check.GRPCService, - GRPCUseTLS: check.GRPCUseTLS, - TaskName: check.TaskName, - OnUpdate: onUpdate, - } - if check.CheckRestart != nil { - out[i].Checks[j].CheckRestart = &structs.CheckRestart{ - Limit: check.CheckRestart.Limit, - Grace: *check.CheckRestart.Grace, - IgnoreWarnings: check.CheckRestart.IgnoreWarnings, - } - } - } - } + } - if s.Connect != nil { - out[i].Connect = ApiConsulConnectToStructs(s.Connect) + return out +} + +func apiServiceChecksToStructs(in []api.ServiceCheck, defaultOnUpdate string) []*structs.ServiceCheck { + if len(in) == 0 { + return nil + } + + out := make([]*structs.ServiceCheck, len(in)) + for i, inc := range in { + onUpdate := defaultOnUpdate + if inc.OnUpdate != "" { + onUpdate = inc.OnUpdate + } + check := &structs.ServiceCheck{ + Name: inc.Name, + Type: inc.Type, + Command: inc.Command, + Args: inc.Args, + Path: inc.Path, + Protocol: inc.Protocol, + PortLabel: inc.PortLabel, + Expose: inc.Expose, + AddressMode: inc.AddressMode, + Interval: inc.Interval, + Timeout: inc.Timeout, + InitialStatus: inc.InitialStatus, + TLSSkipVerify: inc.TLSSkipVerify, + Header: inc.Header, + Method: inc.Method, + Body: inc.Body, + GRPCService: inc.GRPCService, + GRPCUseTLS: inc.GRPCUseTLS, + TaskName: inc.TaskName, + OnUpdate: onUpdate, + } + if inc.CheckRestart != nil { + check.CheckRestart = &structs.CheckRestart{ + Limit: inc.CheckRestart.Limit, + Grace: *inc.CheckRestart.Grace, + IgnoreWarnings: inc.CheckRestart.IgnoreWarnings, + } } + out[i] = check } return out @@ -1499,9 +1505,10 @@ func apiConnectSidecarServiceToStructs(in *api.ConsulSidecarService) *structs.Co return nil } return &structs.ConsulSidecarService{ - Port: in.Port, - Tags: helper.CopySliceString(in.Tags), - Proxy: apiConnectSidecarServiceProxyToStructs(in.Proxy), + Port: in.Port, + Tags: helper.CopySliceString(in.Tags), + Proxy: apiConnectSidecarServiceProxyToStructs(in.Proxy), + Checks: apiServiceChecksToStructs(in.Checks, ""), } } diff --git a/command/agent/job_endpoint_test.go b/command/agent/job_endpoint_test.go index f7ed9ff0c36..b5d60043b43 100644 --- a/command/agent/job_endpoint_test.go +++ b/command/agent/job_endpoint_test.go @@ -4,7 +4,6 @@ import ( "net/http" "net/http/httptest" "reflect" - "strings" "testing" "time" @@ -13,7 +12,6 @@ import ( "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/nomad/mock" "github.com/hashicorp/nomad/nomad/structs" - "github.com/kr/pretty" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -2037,6 +2035,14 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) { SidecarService: &api.ConsulSidecarService{ Tags: []string{"f", "g"}, Port: "9000", + + Checks: []api.ServiceCheck{ + { + Type: "TCP", + Interval: 10 * time.Second, + Timeout: 5 * time.Second, + }, + }, }, }, }, @@ -2416,6 +2422,13 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) { SidecarService: &structs.ConsulSidecarService{ Tags: []string{"f", "g"}, Port: "9000", + Checks: []*structs.ServiceCheck{ + { + Type: "TCP", + Interval: 10 * time.Second, + Timeout: 5 * time.Second, + }, + }, }, }, }, @@ -2605,9 +2618,7 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) { structsJob := ApiJobToStructJob(apiJob) - if diff := pretty.Diff(expected, structsJob); len(diff) > 0 { - t.Fatalf("bad:\n%s", strings.Join(diff, "\n")) - } + require.Equal(t, expected, structsJob) systemAPIJob := &api.Job{ Stop: helper.BoolToPtr(true), @@ -2850,9 +2861,7 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) { systemStructsJob := ApiJobToStructJob(systemAPIJob) - if diff := pretty.Diff(expectedSystemJob, systemStructsJob); len(diff) > 0 { - t.Fatalf("bad:\n%s", strings.Join(diff, "\n")) - } + require.Equal(t, expectedSystemJob, systemStructsJob) } func TestJobs_ApiJobToStructsJobUpdate(t *testing.T) { diff --git a/jobspec/parse_service.go b/jobspec/parse_service.go index c8778cbb09a..40ddf5be047 100644 --- a/jobspec/parse_service.go +++ b/jobspec/parse_service.go @@ -80,9 +80,11 @@ func parseService(o *ast.ObjectItem) (*api.Service, error) { } if co := listVal.Filter("check"); len(co.Items) > 0 { - if err := parseChecks(&service, co); err != nil { + checks, err := parseChecks(co) + if err != nil { return nil, multierror.Prefix(err, fmt.Sprintf("'%s',", service.Name)) } + service.Checks = checks } // Filter check_restart @@ -550,6 +552,7 @@ func parseSidecarService(o *ast.ObjectItem) (*api.ConsulSidecarService, error) { "port", "proxy", "tags", + "check", } if err := checkHCLKeys(o.Val, valid); err != nil { @@ -562,6 +565,7 @@ func parseSidecarService(o *ast.ObjectItem) (*api.ConsulSidecarService, error) { return nil, err } + delete(m, "check") delete(m, "proxy") dec, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{ @@ -576,27 +580,31 @@ func parseSidecarService(o *ast.ObjectItem) (*api.ConsulSidecarService, error) { return nil, fmt.Errorf("sidecar_service: %v", err) } - var proxyList *ast.ObjectList + var listVal *ast.ObjectList if ot, ok := o.Val.(*ast.ObjectType); ok { - proxyList = ot.List + listVal = ot.List } else { return nil, fmt.Errorf("sidecar_service: should be an object") } // Parse the proxy - po := proxyList.Filter("proxy") - if len(po.Items) == 0 { - return &sidecar, nil - } - if len(po.Items) > 1 { + if po := listVal.Filter("proxy"); len(po.Items) > 1 { return nil, fmt.Errorf("only one 'proxy' block allowed per task") + } else if len(po.Items) == 1 { + r, err := parseProxy(po.Items[0]) + if err != nil { + return nil, fmt.Errorf("proxy, %v", err) + } + sidecar.Proxy = r } - r, err := parseProxy(po.Items[0]) - if err != nil { - return nil, fmt.Errorf("proxy, %v", err) + if co := listVal.Filter("check"); len(co.Items) > 0 { + checks, err := parseChecks(co) + if err != nil { + return nil, multierror.Prefix(err, "'service_sidecar',") + } + sidecar.Checks = checks } - sidecar.Proxy = r return &sidecar, nil } @@ -833,8 +841,8 @@ func parseUpstream(uo *ast.ObjectItem) (*api.ConsulUpstream, error) { return &upstream, nil } -func parseChecks(service *api.Service, checkObjs *ast.ObjectList) error { - service.Checks = make([]api.ServiceCheck, len(checkObjs.Items)) +func parseChecks(checkObjs *ast.ObjectList) ([]api.ServiceCheck, error) { + checks := make([]api.ServiceCheck, len(checkObjs.Items)) for idx, co := range checkObjs.Items { // Check for invalid keys valid := []string{ @@ -861,13 +869,13 @@ func parseChecks(service *api.Service, checkObjs *ast.ObjectList) error { "failures_before_critical", } if err := checkHCLKeys(co.Val, valid); err != nil { - return multierror.Prefix(err, "check ->") + return nil, multierror.Prefix(err, "check ->") } var check api.ServiceCheck var cm map[string]interface{} if err := hcl.DecodeObject(&cm, co.Val); err != nil { - return err + return nil, err } // HCL allows repeating stanzas so merge 'header' into a single @@ -875,19 +883,19 @@ func parseChecks(service *api.Service, checkObjs *ast.ObjectList) error { if headerI, ok := cm["header"]; ok { headerRaw, ok := headerI.([]map[string]interface{}) if !ok { - return fmt.Errorf("check -> header -> expected a []map[string][]string but found %T", headerI) + return nil, fmt.Errorf("check -> header -> expected a []map[string][]string but found %T", headerI) } m := map[string][]string{} for _, rawm := range headerRaw { for k, vI := range rawm { vs, ok := vI.([]interface{}) if !ok { - return fmt.Errorf("check -> header -> %q expected a []string but found %T", k, vI) + return nil, fmt.Errorf("check -> header -> %q expected a []string but found %T", k, vI) } for _, vI := range vs { v, ok := vI.(string) if !ok { - return fmt.Errorf("check -> header -> %q expected a string but found %T", k, vI) + return nil, fmt.Errorf("check -> header -> %q expected a string but found %T", k, vI) } m[k] = append(m[k], v) } @@ -908,10 +916,10 @@ func parseChecks(service *api.Service, checkObjs *ast.ObjectList) error { Result: &check, }) if err != nil { - return err + return nil, err } if err := dec.Decode(cm); err != nil { - return err + return nil, err } // Filter check_restart @@ -919,24 +927,24 @@ func parseChecks(service *api.Service, checkObjs *ast.ObjectList) error { if ot, ok := co.Val.(*ast.ObjectType); ok { checkRestartList = ot.List } else { - return fmt.Errorf("check_restart '%s': should be an object", check.Name) + return nil, fmt.Errorf("check_restart '%s': should be an object", check.Name) } if cro := checkRestartList.Filter("check_restart"); len(cro.Items) > 0 { if len(cro.Items) > 1 { - return fmt.Errorf("check_restart '%s': cannot have more than 1 check_restart", check.Name) + return nil, fmt.Errorf("check_restart '%s': cannot have more than 1 check_restart", check.Name) } cr, err := parseCheckRestart(cro.Items[0]) if err != nil { - return multierror.Prefix(err, fmt.Sprintf("check: '%s',", check.Name)) + return nil, multierror.Prefix(err, fmt.Sprintf("check: '%s',", check.Name)) } check.CheckRestart = cr } - service.Checks[idx] = check + checks[idx] = check } - return nil + return checks, nil } func parseCheckRestart(cro *ast.ObjectItem) (*api.CheckRestart, error) { diff --git a/jobspec/parse_test.go b/jobspec/parse_test.go index 9e11388608b..5a9cba97e72 100644 --- a/jobspec/parse_test.go +++ b/jobspec/parse_test.go @@ -2,14 +2,13 @@ package jobspec import ( "path/filepath" - "reflect" "strings" "testing" "time" capi "github.com/hashicorp/consul/api" "github.com/hashicorp/nomad/api" - "github.com/kr/pretty" + "github.com/stretchr/testify/require" ) // consts copied from nomad/structs package to keep jobspec isolated from rest of nomad @@ -1253,6 +1252,33 @@ func TestParse(t *testing.T) { }, false, }, + { + "tg-service-connect-sidecar_check.hcl", + &api.Job{ + ID: stringToPtr("sidecar_task_name"), + Name: stringToPtr("sidecar_task_name"), + Type: stringToPtr("service"), + TaskGroups: []*api.TaskGroup{{ + Name: stringToPtr("group"), + Services: []*api.Service{{ + Name: "example", + Connect: &api.ConsulConnect{ + Native: false, + SidecarService: &api.ConsulSidecarService{ + Checks: []api.ServiceCheck{ + { + Type: "tcp", + Interval: 10 * time.Second, + Timeout: 2 * time.Second, + }, + }, + }, + }, + }}, + }}, + }, + false, + }, { "tg-service-connect-proxy.hcl", &api.Job{ @@ -1608,26 +1634,20 @@ func TestParse(t *testing.T) { } for _, tc := range cases { - t.Logf("Testing parse: %s", tc.File) - - path, err := filepath.Abs(filepath.Join("./test-fixtures", tc.File)) - if err != nil { - t.Fatalf("file: %s\n\n%s", tc.File, err) - continue - } + t.Run(tc.File, func(t *testing.T) { + t.Logf("Testing parse: %s", tc.File) - actual, err := ParseFile(path) - if (err != nil) != tc.Err { - t.Fatalf("file: %s\n\n%s", tc.File, err) - continue - } + path, err := filepath.Abs(filepath.Join("./test-fixtures", tc.File)) + require.NoError(t, err) - if !reflect.DeepEqual(actual, tc.Result) { - for _, d := range pretty.Diff(actual, tc.Result) { - t.Logf(d) + actual, err := ParseFile(path) + if tc.Err { + require.Error(t, err) + } else { + require.NoError(t, err) + require.Equal(t, tc.Result, actual) } - t.Fatalf("file: %s", tc.File) - } + }) } } diff --git a/jobspec/test-fixtures/tg-service-connect-sidecar_check.hcl b/jobspec/test-fixtures/tg-service-connect-sidecar_check.hcl new file mode 100644 index 00000000000..6f7109174b6 --- /dev/null +++ b/jobspec/test-fixtures/tg-service-connect-sidecar_check.hcl @@ -0,0 +1,19 @@ +job "sidecar_task_name" { + type = "service" + + group "group" { + service { + name = "example" + + connect { + sidecar_service { + check { + type = "tcp" + interval = "10s" + timeout = "2s" + } + } + } + } + } +} diff --git a/nomad/structs/services.go b/nomad/structs/services.go index 7fcf363d1b7..4d0e006b9f5 100644 --- a/nomad/structs/services.go +++ b/nomad/structs/services.go @@ -516,6 +516,9 @@ func (s *Service) Canonicalize(job string, taskGroup string, task string) { if len(s.Checks) == 0 { s.Checks = nil } + if s.Connect != nil && s.Connect.SidecarService != nil { + s.Connect.SidecarService.Canonicalize() + } s.Name = args.ReplaceEnv(s.Name, map[string]string{ "JOB": job, @@ -680,6 +683,27 @@ func hashConfig(h hash.Hash, c map[string]interface{}) { _, _ = fmt.Fprintf(h, "%v", c) } +func checksEqual(c1, c2 []*ServiceCheck) bool { + if len(c1) != len(c2) { + return false + } + +OUTER: + for i := range c1 { + for j := range c2 { + if c1[i].Equals(c2[j]) { + // Found match; continue with next check + continue OUTER + } + } + + // No match + return false + } + + return true +} + // Equals returns true if the structs are recursively equal. func (s *Service) Equals(o *Service) bool { if s == nil || o == nil { @@ -702,20 +726,7 @@ func (s *Service) Equals(o *Service) bool { return false } - if len(s.Checks) != len(o.Checks) { - return false - } - -OUTER: - for i := range s.Checks { - for ii := range o.Checks { - if s.Checks[i].Equals(o.Checks[ii]) { - // Found match; continue with next check - continue OUTER - } - } - - // No match + if !checksEqual(s.Checks, o.Checks) { return false } @@ -866,6 +877,11 @@ func (c *ConsulConnect) Validate() error { } } + if c.SidecarService != nil { + if err := c.SidecarService.Validate(); err != nil { + return err + } + } // The Native and Sidecar cases are validated up at the service level. return nil @@ -884,6 +900,9 @@ type ConsulSidecarService struct { // Proxy stanza defining the sidecar proxy configuration. Proxy *ConsulProxy + + // Checks define the checks for the sidecar service, overriding the default TCP check + Checks []*ServiceCheck } // HasUpstreams checks if the sidecar service has any upstreams configured @@ -896,11 +915,48 @@ func (s *ConsulSidecarService) Copy() *ConsulSidecarService { if s == nil { return nil } + + var checks []*ServiceCheck + if s.Checks != nil { + checks = make([]*ServiceCheck, len(s.Checks)) + for i, c := range s.Checks { + checks[i] = c.Copy() + } + } + return &ConsulSidecarService{ - Tags: helper.CopySliceString(s.Tags), - Port: s.Port, - Proxy: s.Proxy.Copy(), + Tags: helper.CopySliceString(s.Tags), + Port: s.Port, + Proxy: s.Proxy.Copy(), + Checks: checks, + } + +} + +func (s *ConsulSidecarService) Canonicalize() { + if len(s.Checks) == 0 { + s.Checks = nil + } + + for _, check := range s.Checks { + check.Canonicalize("sidecar_service") + } +} + +func (s *ConsulSidecarService) Validate() error { + var mErr multierror.Error + for _, c := range s.Checks { + + //if c.PortLabel != "" { + // mErr.Errors = append(mErr.Errors, fmt.Errorf("Check %s invalid: port is automatically assigned", c.Name)) + // continue + //} + + if err := c.validate(); err != nil { + mErr.Errors = append(mErr.Errors, fmt.Errorf("Check %s invalid: %v", c.Name, err)) + } } + return mErr.ErrorOrNil() } // Equals returns true if the structs are recursively equal. @@ -917,7 +973,15 @@ func (s *ConsulSidecarService) Equals(o *ConsulSidecarService) bool { return false } - return s.Proxy.Equals(o.Proxy) + if !s.Proxy.Equals(o.Proxy) { + return false + } + + if !checksEqual(s.Checks, o.Checks) { + return false + } + + return true } // SidecarTask represents a subset of Task fields that are able to be overridden diff --git a/vendor/github.com/hashicorp/nomad/api/services.go b/vendor/github.com/hashicorp/nomad/api/services.go index 50932ccf043..e561189aa16 100644 --- a/vendor/github.com/hashicorp/nomad/api/services.go +++ b/vendor/github.com/hashicorp/nomad/api/services.go @@ -189,9 +189,10 @@ func (cc *ConsulConnect) Canonicalize() { // ConsulSidecarService represents a Consul Connect SidecarService jobspec // stanza. type ConsulSidecarService struct { - Tags []string `hcl:"tags,optional"` - Port string `hcl:"port,optional"` - Proxy *ConsulProxy `hcl:"proxy,block"` + Tags []string `hcl:"tags,optional"` + Port string `hcl:"port,optional"` + Proxy *ConsulProxy `hcl:"proxy,block"` + Checks []ServiceCheck `hcl:"check,block"` } func (css *ConsulSidecarService) Canonicalize() {