diff --git a/api/services.go b/api/services.go index 823634ac09ab..68e93ea99659 100644 --- a/api/services.go +++ b/api/services.go @@ -81,6 +81,7 @@ type ServiceCheck struct { Path string Protocol string PortLabel string `mapstructure:"port"` + Expose bool AddressMode string `mapstructure:"address_mode"` Interval time.Duration Timeout time.Duration @@ -183,7 +184,6 @@ type ConsulUpstream struct { type ConsulExposeConfig struct { Path []*ConsulExposePath `mapstructure:"path"` - // todo(shoenig): add magic for 'checks' option } type ConsulExposePath struct { diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index e0bc544684a9..f99ca0b41340 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -1142,6 +1142,7 @@ func ApiServicesToStructs(in []*api.Service) []*structs.Service { Path: check.Path, Protocol: check.Protocol, PortLabel: check.PortLabel, + Expose: check.Expose, AddressMode: check.AddressMode, Interval: check.Interval, Timeout: check.Timeout, diff --git a/jobspec/parse_service.go b/jobspec/parse_service.go index f89b7d0ab5e8..00963b327f90 100644 --- a/jobspec/parse_service.go +++ b/jobspec/parse_service.go @@ -405,7 +405,6 @@ func parseProxy(o *ast.ObjectItem) (*api.ConsulProxy, error) { func parseExpose(eo *ast.ObjectItem) (*api.ConsulExposeConfig, error) { valid := []string{ "path", // an array of path blocks - // todo(shoenig) checks boolean } if err := helper.CheckHCLKeys(eo.Val, valid); err != nil { @@ -514,6 +513,7 @@ func parseChecks(service *api.Service, checkObjs *ast.ObjectList) error { "path", "protocol", "port", + "expose", "command", "args", "initial_status", diff --git a/jobspec/parse_test.go b/jobspec/parse_test.go index 91e3ca17e93e..1d1be051eda5 100644 --- a/jobspec/parse_test.go +++ b/jobspec/parse_test.go @@ -1147,6 +1147,32 @@ func TestParse(t *testing.T) { }, false, }, + { + "tg-service-check-expose.hcl", + &api.Job{ + ID: helper.StringToPtr("group_service_proxy_expose"), + Name: helper.StringToPtr("group_service_proxy_expose"), + TaskGroups: []*api.TaskGroup{{ + Name: helper.StringToPtr("group"), + Services: []*api.Service{{ + Name: "example", + Connect: &api.ConsulConnect{ + SidecarService: &api.ConsulSidecarService{ + Proxy: &api.ConsulProxy{}, + }, + }, + Checks: []api.ServiceCheck{{ + Name: "example-check1", + Expose: true, + }, { + Name: "example-check2", + Expose: false, + }}, + }}, + }}, + }, + false, + }, { "tg-service-enable-tag-override.hcl", &api.Job{ diff --git a/jobspec/test-fixtures/tg-service-check-expose.hcl b/jobspec/test-fixtures/tg-service-check-expose.hcl new file mode 100644 index 000000000000..694086ed237d --- /dev/null +++ b/jobspec/test-fixtures/tg-service-check-expose.hcl @@ -0,0 +1,23 @@ +job "group_service_proxy_expose" { + group "group" { + service { + name = "example" + connect { + sidecar_service { + proxy { + } + } + } + + check { + name = "example-check1" + expose = true + } + + check { + name = "example-check2" + expose = false + } + } + } +} diff --git a/nomad/job_endpoint.go b/nomad/job_endpoint.go index 86ad10bcccbf..571bfc9cc039 100644 --- a/nomad/job_endpoint.go +++ b/nomad/job_endpoint.go @@ -61,10 +61,12 @@ func NewJobEndpoints(s *Server) *Job { mutators: []jobMutator{ jobCanonicalizer{}, jobConnectHook{}, + jobExposeCheckHook{}, jobImpliedConstraints{}, }, validators: []jobValidator{ jobConnectHook{}, + jobExposeCheckHook{}, jobValidate{}, }, } diff --git a/nomad/job_endpoint_hook_expose_check.go b/nomad/job_endpoint_hook_expose_check.go new file mode 100644 index 000000000000..eea2da41ebb2 --- /dev/null +++ b/nomad/job_endpoint_hook_expose_check.go @@ -0,0 +1,228 @@ +package nomad + +import ( + "strconv" + "strings" + + "github.com/hashicorp/nomad/nomad/structs" + "github.com/pkg/errors" +) + +type jobExposeCheckHook struct{} + +func (jobExposeCheckHook) Name() string { + return "expose-check" +} + +// Mutate will scan every task group for group-services which have checks defined +// that have the Expose field configured, and generate expose path configurations +// extrapolated from those check definitions. +func (jobExposeCheckHook) Mutate(job *structs.Job) (_ *structs.Job, warnings []error, err error) { + for _, tg := range job.TaskGroups { + for _, s := range tg.Services { + for _, c := range s.Checks { + if c.Expose { + if exposePath, err := exposePathForCheck(tg, s, c); err != nil { + return nil, nil, err + } else if exposePath != nil { + serviceExposeConfig := serviceExposeConfig(s) + // insert only if not already present - required for job + // updates which would otherwise create duplicates + if !containsExposePath(serviceExposeConfig.Paths, *exposePath) { + serviceExposeConfig.Paths = append( + serviceExposeConfig.Paths, *exposePath, + ) + } + } + } + } + } + } + return job, nil, nil +} + +// Validate will ensure: +// - The job contains valid network configuration for each task group in which +// an expose path is configured. The network must be of type bridge mode. +// - The check Expose field is configured only for connect-enabled group-services. +func (jobExposeCheckHook) Validate(job *structs.Job) (warnings []error, err error) { + for _, tg := range job.TaskGroups { + // Make sure any group that contains a group-service that enables expose + // is configured with one network that is in "bridge" mode. This check + // is being done independently of the preceding Connect task injection + // hook, because at some point in the future Connect will not require the + // use of network namespaces, whereas the use of "expose" does not make + // sense without the use of network namespace. + if err := tgValidateUseOfBridgeMode(tg); err != nil { + return nil, err + } + // Make sure any group-service that contains a check that enables expose + // is connect-enabled and does not specify a custom sidecar task. We only + // support the expose feature when using the built-in Envoy integration. + if err := tgValidateUseOfCheckExpose(tg); err != nil { + return nil, err + } + } + return nil, nil +} + +// serviceExposeConfig digs through s to extract the connect sidecar service proxy +// expose configuration. It is not required of the user to provide this, so it +// is created on demand here as needed in the case where any service check exposes +// itself. +// +// The service, connect, and sidecar_service are assumed not to be nil, as they +// are enforced in previous hooks / validation. +func serviceExposeConfig(s *structs.Service) *structs.ConsulExposeConfig { + if s.Connect.SidecarService.Proxy == nil { + s.Connect.SidecarService.Proxy = new(structs.ConsulProxy) + } + if s.Connect.SidecarService.Proxy.Expose == nil { + s.Connect.SidecarService.Proxy.Expose = new(structs.ConsulExposeConfig) + } + return s.Connect.SidecarService.Proxy.Expose +} + +// containsExposePath returns true if path is contained in paths. +func containsExposePath(paths []structs.ConsulExposePath, path structs.ConsulExposePath) bool { + for _, p := range paths { + if p == path { + return true + } + } + return false +} + +// tgValidateUseOfCheckExpose ensures that any service check in tg making use +// of the expose field is within an appropriate context to do so. The check must +// be a group level check, and must use the builtin envoy proxy. +func tgValidateUseOfCheckExpose(tg *structs.TaskGroup) error { + // validation for group services (which must use built-in connect proxy) + for _, s := range tg.Services { + for _, check := range s.Checks { + if check.Expose && !serviceUsesConnectEnvoy(s) { + return errors.Errorf( + "exposed service check %s->%s->%s requires use of Nomad's builtin Connect proxy", + tg.Name, s.Name, check.Name, + ) + } + } + } + + // validation for task services (which must not be configured to use Expose) + for _, t := range tg.Tasks { + for _, s := range t.Services { + for _, check := range s.Checks { + if check.Expose { + return errors.Errorf( + "exposed service check %s[%s]->%s->%s is not a task-group service", + tg.Name, t.Name, s.Name, check.Name, + ) + } + } + } + } + return nil +} + +// tgValidateUseOfBridgeMode ensures there is exactly 1 network configured for +// the task group, and that it makes use of "bridge" mode (i.e. enables network +// namespaces). +func tgValidateUseOfBridgeMode(tg *structs.TaskGroup) error { + if tgUsesExposeCheck(tg) { + if len(tg.Networks) != 1 { + return errors.Errorf("group %q must specify one bridge network for exposing service check(s)", tg.Name) + } + if tg.Networks[0].Mode != "bridge" { + return errors.Errorf("group %q must use bridge network for exposing service check(s)", tg.Name) + } + } + return nil +} + +// tgUsesExposeCheck returns true if any group service in the task group makes +// use of the expose field. +func tgUsesExposeCheck(tg *structs.TaskGroup) bool { + for _, s := range tg.Services { + for _, check := range s.Checks { + if check.Expose { + return true + } + } + } + return false +} + +// serviceUsesConnectEnvoy returns true if the service is going to end up using +// the built-in envoy proxy. +// +// This implementation is kind of reading tea leaves - firstly Connect +// must be enabled, and second the sidecar_task must not be overridden. If these +// conditions are met, the preceding connect hook will have injected a Connect +// sidecar task, the configuration of which is interpolated at runtime. +func serviceUsesConnectEnvoy(s *structs.Service) bool { + // A non-nil connect stanza implies this service isn't connect enabled in + // the first place. + if s.Connect == nil { + return false + } + + // A non-nil connect.sidecar_task stanza implies the sidecar task is being + // overridden (i.e. the default Envoy is not being uesd). + if s.Connect.SidecarTask != nil { + return false + } + + return true +} + +// checkIsExposable returns true if check is qualified for automatic generation +// of connect proxy expose path configuration based on configured consul checks. +// To qualify, the check must be of type "http" or "grpc", and must have a Path +// configured. +func checkIsExposable(check *structs.ServiceCheck) bool { + switch strings.ToLower(check.Type) { + case "grpc", "http": + return strings.HasPrefix(check.Path, "/") + default: + return false + } +} + +// exposePathForCheck extrapolates the necessary expose path configuration for +// the given consul service check. If the check is not compatible, nil is +// returned. +func exposePathForCheck(tg *structs.TaskGroup, s *structs.Service, check *structs.ServiceCheck) (*structs.ConsulExposePath, error) { + if !checkIsExposable(check) { + return nil, nil + } + + // Determine the local service port (i.e. what port the service is actually + // listening to inside the network namespace). + // + // Similar logic exists in getAddress of client.go which is used for + // creating check & service registration objects. + // + // The difference here is the address is predestined to be localhost since + // it is binding inside the namespace. + var port int + if _, port = tg.Networks.Port(s.PortLabel); port <= 0 { // try looking up by port label + if port, _ = strconv.Atoi(s.PortLabel); port <= 0 { // then try direct port value + return nil, errors.Errorf( + "unable to determine local service port for service check %s->%s->%s", + tg.Name, s.Name, check.Name, + ) + } + } + + // The Path, Protocol, and PortLabel are just copied over from the service + // check definition. It is required that the user configure their own port + // mapping for each check, including setting the 'to = -1' sentinel value + // enabling the network namespace pass-through. + return &structs.ConsulExposePath{ + Path: check.Path, + Protocol: check.Protocol, + LocalPathPort: port, + ListenerPort: check.PortLabel, + }, nil +} diff --git a/nomad/job_endpoint_hook_expose_check_test.go b/nomad/job_endpoint_hook_expose_check_test.go new file mode 100644 index 000000000000..8b00b5884311 --- /dev/null +++ b/nomad/job_endpoint_hook_expose_check_test.go @@ -0,0 +1,598 @@ +package nomad + +import ( + "testing" + + "github.com/hashicorp/nomad/nomad/structs" + "github.com/stretchr/testify/require" +) + +func TestJobExposeCheckHook_Name(t *testing.T) { + t.Parallel() + + require.Equal(t, "expose-check", new(jobExposeCheckHook).Name()) +} + +func TestJobExposeCheckHook_serviceUsesConnectEnvoy(t *testing.T) { + t.Parallel() + + t.Run("connect is nil", func(t *testing.T) { + require.False(t, serviceUsesConnectEnvoy(&structs.Service{ + Connect: nil, + })) + }) + + t.Run("sidecar-task is overridden", func(t *testing.T) { + require.False(t, serviceUsesConnectEnvoy(&structs.Service{ + Connect: &structs.ConsulConnect{ + SidecarTask: &structs.SidecarTask{ + Name: "my-sidecar", + }, + }, + })) + }) + + t.Run("sidecar-task is nil", func(t *testing.T) { + require.True(t, serviceUsesConnectEnvoy(&structs.Service{ + Connect: &structs.ConsulConnect{ + SidecarTask: nil, + }, + })) + }) +} + +func TestJobExposeCheckHook_tgUsesExposeCheck(t *testing.T) { + t.Parallel() + + t.Run("no check.expose", func(t *testing.T) { + require.False(t, tgUsesExposeCheck(&structs.TaskGroup{ + Services: []*structs.Service{{ + Checks: []*structs.ServiceCheck{{ + Expose: false, + }}, + }}, + })) + }) + + t.Run("with check.expose", func(t *testing.T) { + require.True(t, tgUsesExposeCheck(&structs.TaskGroup{ + Services: []*structs.Service{{ + Checks: []*structs.ServiceCheck{{ + Expose: false, + }, { + Expose: true, + }}, + }}, + })) + }) +} + +func TestJobExposeCheckHook_tgValidateUseOfBridgeMode(t *testing.T) { + t.Parallel() + + s1 := &structs.Service{ + Name: "s1", + Checks: []*structs.ServiceCheck{{ + Name: "s1-check1", + Type: "http", + PortLabel: "health", + Expose: true, + }}, + } + + t.Run("no networks but no use of expose", func(t *testing.T) { + require.Nil(t, tgValidateUseOfBridgeMode(&structs.TaskGroup{ + Networks: make(structs.Networks, 0), + })) + }) + + t.Run("no networks and uses expose", func(t *testing.T) { + require.EqualError(t, tgValidateUseOfBridgeMode(&structs.TaskGroup{ + Name: "g1", + Networks: make(structs.Networks, 0), + Services: []*structs.Service{s1}, + }), `group "g1" must specify one bridge network for exposing service check(s)`) + }) + + t.Run("non-bridge network and uses expose", func(t *testing.T) { + require.EqualError(t, tgValidateUseOfBridgeMode(&structs.TaskGroup{ + Name: "g1", + Networks: structs.Networks{{ + Mode: "host", + }}, + Services: []*structs.Service{s1}, + }), `group "g1" must use bridge network for exposing service check(s)`) + }) + + t.Run("bridge network uses expose", func(t *testing.T) { + require.Nil(t, tgValidateUseOfBridgeMode(&structs.TaskGroup{ + Name: "g1", + Networks: structs.Networks{{ + Mode: "bridge", + }}, + Services: []*structs.Service{s1}, + })) + }) +} + +func TestJobExposeCheckHook_tgValidateUseOfCheckExpose(t *testing.T) { + t.Parallel() + + withCustomProxyTask := &structs.Service{ + Name: "s1", + Connect: &structs.ConsulConnect{ + SidecarTask: &structs.SidecarTask{Name: "custom"}, + }, + Checks: []*structs.ServiceCheck{{ + Name: "s1-check1", + Type: "http", + PortLabel: "health", + Expose: true, + }}, + } + + t.Run("group-service uses custom proxy", func(t *testing.T) { + require.EqualError(t, tgValidateUseOfCheckExpose(&structs.TaskGroup{ + Name: "g1", + Services: []*structs.Service{withCustomProxyTask}, + }), `exposed service check g1->s1->s1-check1 requires use of Nomad's builtin Connect proxy`) + }) + + t.Run("group-service uses custom proxy but no expose", func(t *testing.T) { + withCustomProxyTaskNoExpose := &(*withCustomProxyTask) + withCustomProxyTask.Checks[0].Expose = false + require.Nil(t, tgValidateUseOfCheckExpose(&structs.TaskGroup{ + Name: "g1", + Services: []*structs.Service{withCustomProxyTaskNoExpose}, + })) + }) + + t.Run("task-service sets expose", func(t *testing.T) { + require.EqualError(t, tgValidateUseOfCheckExpose(&structs.TaskGroup{ + Name: "g1", + Tasks: []*structs.Task{{ + Name: "t1", + Services: []*structs.Service{{ + Name: "s2", + Checks: []*structs.ServiceCheck{{ + Name: "check1", + Type: "http", + Expose: true, + }}, + }}, + }}, + }), `exposed service check g1[t1]->s2->check1 is not a task-group service`) + }) +} + +func TestJobExposeCheckHook_Validate(t *testing.T) { + s1 := &structs.Service{ + Name: "s1", + Checks: []*structs.ServiceCheck{{ + Name: "s1-check1", + Type: "http", + Expose: true, + }}, + } + + t.Run("double network", func(t *testing.T) { + warnings, err := new(jobExposeCheckHook).Validate(&structs.Job{ + TaskGroups: []*structs.TaskGroup{{ + Name: "g1", + Networks: structs.Networks{{ + Mode: "bridge", + }, { + Mode: "bridge", + }}, + Services: []*structs.Service{s1}, + }}, + }) + require.Empty(t, warnings) + require.EqualError(t, err, `group "g1" must specify one bridge network for exposing service check(s)`) + }) + + t.Run("expose in service check", func(t *testing.T) { + warnings, err := new(jobExposeCheckHook).Validate(&structs.Job{ + TaskGroups: []*structs.TaskGroup{{ + Name: "g1", + Networks: structs.Networks{{ + Mode: "bridge", + }}, + Tasks: []*structs.Task{{ + Name: "t1", + Services: []*structs.Service{{ + Name: "s2", + Checks: []*structs.ServiceCheck{{ + Name: "s2-check1", + Type: "http", + Expose: true, + }}, + }}, + }}, + }}, + }) + require.Empty(t, warnings) + require.EqualError(t, err, `exposed service check g1[t1]->s2->s2-check1 is not a task-group service`) + }) + + t.Run("ok", func(t *testing.T) { + warnings, err := new(jobExposeCheckHook).Validate(&structs.Job{ + TaskGroups: []*structs.TaskGroup{{ + Name: "g1", + Networks: structs.Networks{{ + Mode: "bridge", + }}, + Services: []*structs.Service{{ + Name: "s1", + Connect: &structs.ConsulConnect{}, + Checks: []*structs.ServiceCheck{{ + Name: "check1", + Type: "http", + Expose: true, + }}, + }}, + Tasks: []*structs.Task{{ + Name: "t1", + Services: []*structs.Service{{ + Name: "s2", + Checks: []*structs.ServiceCheck{{ + Name: "s2-check1", + Type: "http", + Expose: false, + }}, + }}, + }}, + }}, + }) + require.Empty(t, warnings) + require.Nil(t, err) + }) +} + +func TestJobExposeCheckHook_exposePathForCheck(t *testing.T) { + t.Parallel() + + t.Run("not expose compatible", func(t *testing.T) { + c := &structs.ServiceCheck{ + Type: "tcp", // not expose compatible + } + s := &structs.Service{ + Checks: []*structs.ServiceCheck{c}, + } + ePath, err := exposePathForCheck(&structs.TaskGroup{ + Services: []*structs.Service{s}, + }, s, c) + require.NoError(t, err) + require.Nil(t, ePath) + }) + + t.Run("direct port", func(t *testing.T) { + c := &structs.ServiceCheck{ + Name: "check1", + Type: "http", + Path: "/health", + PortLabel: "hcPort", + } + s := &structs.Service{ + Name: "service1", + PortLabel: "4000", + Checks: []*structs.ServiceCheck{c}, + } + ePath, err := exposePathForCheck(&structs.TaskGroup{ + Name: "group1", + Services: []*structs.Service{s}, + }, s, c) + require.NoError(t, err) + require.Equal(t, &structs.ConsulExposePath{ + Path: "/health", + Protocol: "", // often blank, consul does the Right Thing + LocalPathPort: 4000, + ListenerPort: "hcPort", + }, ePath) + }) + + t.Run("labeled port", func(t *testing.T) { + c := &structs.ServiceCheck{ + Name: "check1", + Type: "http", + Path: "/health", + PortLabel: "hcPort", + } + s := &structs.Service{ + Name: "service1", + PortLabel: "sPort", // port label indirection + Checks: []*structs.ServiceCheck{c}, + } + ePath, err := exposePathForCheck(&structs.TaskGroup{ + Name: "group1", + Services: []*structs.Service{s}, + Networks: structs.Networks{{ + Mode: "bridge", + DynamicPorts: []structs.Port{ + {Label: "sPort", Value: 4000}, + }, + }}, + }, s, c) + require.NoError(t, err) + require.Equal(t, &structs.ConsulExposePath{ + Path: "/health", + Protocol: "", + LocalPathPort: 4000, + ListenerPort: "hcPort", + }, ePath) + }) + + t.Run("missing port", func(t *testing.T) { + c := &structs.ServiceCheck{ + Name: "check1", + Type: "http", + Path: "/health", + PortLabel: "hcPort", + } + s := &structs.Service{ + Name: "service1", + PortLabel: "sPort", // port label indirection + Checks: []*structs.ServiceCheck{c}, + } + _, err := exposePathForCheck(&structs.TaskGroup{ + Name: "group1", + Services: []*structs.Service{s}, + Networks: structs.Networks{{ + Mode: "bridge", + DynamicPorts: []structs.Port{ + // service declares "sPort", but does not exist + }, + }}, + }, s, c) + require.EqualError(t, err, `unable to determine local service port for service check group1->service1->check1`) + }) +} + +func TestJobExposeCheckHook_containsExposePath(t *testing.T) { + t.Parallel() + + t.Run("contains path", func(t *testing.T) { + require.True(t, containsExposePath([]structs.ConsulExposePath{{ + Path: "/v2/health", + Protocol: "grpc", + LocalPathPort: 8080, + ListenerPort: "v2Port", + }, { + Path: "/health", + Protocol: "http", + LocalPathPort: 8080, + ListenerPort: "hcPort", + }}, structs.ConsulExposePath{ + Path: "/health", + Protocol: "http", + LocalPathPort: 8080, + ListenerPort: "hcPort", + })) + }) + + t.Run("no such path", func(t *testing.T) { + require.False(t, containsExposePath([]structs.ConsulExposePath{{ + Path: "/v2/health", + Protocol: "grpc", + LocalPathPort: 8080, + ListenerPort: "v2Port", + }, { + Path: "/health", + Protocol: "http", + LocalPathPort: 8080, + ListenerPort: "hcPort", + }}, structs.ConsulExposePath{ + Path: "/v3/health", + Protocol: "http", + LocalPathPort: 8080, + ListenerPort: "hcPort", + })) + }) +} + +func TestJobExposeCheckHook_serviceExposeConfig(t *testing.T) { + t.Parallel() + + t.Run("proxy is nil", func(t *testing.T) { + require.NotNil(t, serviceExposeConfig(&structs.Service{ + Connect: &structs.ConsulConnect{ + SidecarService: &structs.ConsulSidecarService{}, + }, + })) + }) + + t.Run("expose is nil", func(t *testing.T) { + require.NotNil(t, serviceExposeConfig(&structs.Service{ + Connect: &structs.ConsulConnect{ + SidecarService: &structs.ConsulSidecarService{ + Proxy: &structs.ConsulProxy{}, + }, + }, + })) + }) + + t.Run("expose pre-existing", func(t *testing.T) { + exposeConfig := serviceExposeConfig(&structs.Service{ + Connect: &structs.ConsulConnect{ + SidecarService: &structs.ConsulSidecarService{ + Proxy: &structs.ConsulProxy{ + Expose: &structs.ConsulExposeConfig{ + Paths: []structs.ConsulExposePath{{ + Path: "/health", + }}, + }, + }, + }, + }, + }) + require.NotNil(t, exposeConfig) + require.Equal(t, []structs.ConsulExposePath{{ + Path: "/health", + }}, exposeConfig.Paths) + }) + + t.Run("append to paths is safe", func(t *testing.T) { + // double check that serviceExposeConfig(s).Paths can be appended to + // from a derived pointer without fear of the original underlying array + // pointer being lost + + s := &structs.Service{ + Connect: &structs.ConsulConnect{ + SidecarService: &structs.ConsulSidecarService{ + Proxy: &structs.ConsulProxy{ + Expose: &structs.ConsulExposeConfig{ + Paths: []structs.ConsulExposePath{{ + Path: "/one", + }}, + }, + }, + }, + }, + } + + exposeConfig := serviceExposeConfig(s) + exposeConfig.Paths = append(exposeConfig.Paths, + structs.ConsulExposePath{Path: "/two"}, + structs.ConsulExposePath{Path: "/three"}, + structs.ConsulExposePath{Path: "/four"}, + structs.ConsulExposePath{Path: "/five"}, + structs.ConsulExposePath{Path: "/six"}, + structs.ConsulExposePath{Path: "/seven"}, + structs.ConsulExposePath{Path: "/eight"}, + structs.ConsulExposePath{Path: "/nine"}, + ) + + // works, because exposeConfig.Paths gets re-assigned into exposeConfig + // which is a pointer, meaning the field is modified also from the + // service struct's perspective + require.Equal(t, 9, len(s.Connect.SidecarService.Proxy.Expose.Paths)) + }) +} + +func TestJobExposeCheckHook_checkIsExposable(t *testing.T) { + t.Parallel() + + t.Run("grpc", func(t *testing.T) { + require.True(t, checkIsExposable(&structs.ServiceCheck{ + Type: "grpc", + Path: "/health", + })) + require.True(t, checkIsExposable(&structs.ServiceCheck{ + Type: "gRPC", + Path: "/health", + })) + }) + + t.Run("http", func(t *testing.T) { + require.True(t, checkIsExposable(&structs.ServiceCheck{ + Type: "http", + Path: "/health", + })) + require.True(t, checkIsExposable(&structs.ServiceCheck{ + Type: "HTTP", + Path: "/health", + })) + }) + + t.Run("tcp", func(t *testing.T) { + require.False(t, checkIsExposable(&structs.ServiceCheck{ + Type: "tcp", + Path: "/health", + })) + }) + + t.Run("no path slash prefix", func(t *testing.T) { + require.False(t, checkIsExposable(&structs.ServiceCheck{ + Type: "http", + Path: "health", + })) + }) +} + +func TestJobExposeCheckHook_Mutate(t *testing.T) { + t.Parallel() + + t.Run("typical", func(t *testing.T) { + result, warnings, err := new(jobExposeCheckHook).Mutate(&structs.Job{ + TaskGroups: []*structs.TaskGroup{{ + Name: "group0", + Networks: structs.Networks{{ + Mode: "host", + }}, + }, { + Name: "group1", + Networks: structs.Networks{{ + Mode: "bridge", + }}, + Services: []*structs.Service{{ + Name: "service1", + PortLabel: "8000", + Checks: []*structs.ServiceCheck{{ + Name: "check1", + Type: "tcp", + PortLabel: "8100", + }, { + Name: "check2", + Type: "http", + PortLabel: "health", + Path: "/health", + Expose: true, + }, { + Name: "check3", + Type: "grpc", + PortLabel: "health", + Path: "/v2/health", + Expose: true, + }}, + Connect: &structs.ConsulConnect{ + SidecarService: &structs.ConsulSidecarService{ + Proxy: &structs.ConsulProxy{ + Expose: &structs.ConsulExposeConfig{ + Paths: []structs.ConsulExposePath{{ + Path: "/pre-existing", + Protocol: "http", + LocalPathPort: 9000, + ListenerPort: "otherPort", + }}}}}}}, { + Name: "service2", + PortLabel: "3000", + Checks: []*structs.ServiceCheck{{ + Name: "check1", + Type: "grpc", + Protocol: "http2", + Path: "/ok", + PortLabel: "health", + Expose: true, + }}, + Connect: &structs.ConsulConnect{ + SidecarService: &structs.ConsulSidecarService{ + Proxy: &structs.ConsulProxy{}, + }, + }, + }}}}, + }) + + require.NoError(t, err) + require.Empty(t, warnings) + require.Equal(t, []structs.ConsulExposePath{{ + Path: "/pre-existing", + LocalPathPort: 9000, + Protocol: "http", + ListenerPort: "otherPort", + }, { + Path: "/health", + LocalPathPort: 8000, + ListenerPort: "health", + }, { + Path: "/v2/health", + LocalPathPort: 8000, + ListenerPort: "health", + }}, result.TaskGroups[1].Services[0].Connect.SidecarService.Proxy.Expose.Paths) + require.Equal(t, []structs.ConsulExposePath{{ + Path: "/ok", + LocalPathPort: 3000, + Protocol: "http2", + ListenerPort: "health", + }}, result.TaskGroups[1].Services[1].Connect.SidecarService.Proxy.Expose.Paths) + }) +} diff --git a/nomad/job_endpoint_test.go b/nomad/job_endpoint_test.go index 62d0e4b75da7..188bf764a8e3 100644 --- a/nomad/job_endpoint_test.go +++ b/nomad/job_endpoint_test.go @@ -121,19 +121,15 @@ func TestJobEndpoint_Register_Connect(t *testing.T) { // Create the register request job := mock.Job() - job.TaskGroups[0].Networks = structs.Networks{ - { - Mode: "bridge", - }, - } - job.TaskGroups[0].Services = []*structs.Service{ - { - Name: "backend", - PortLabel: "8080", - Connect: &structs.ConsulConnect{ - SidecarService: &structs.ConsulSidecarService{}, - }, - }, + job.TaskGroups[0].Networks = structs.Networks{{ + Mode: "bridge", + }} + job.TaskGroups[0].Services = []*structs.Service{{ + Name: "backend", + PortLabel: "8080", + Connect: &structs.ConsulConnect{ + SidecarService: &structs.ConsulSidecarService{}, + }}, } req := &structs.JobRegisterRequest{ Job: job, @@ -178,7 +174,118 @@ func TestJobEndpoint_Register_Connect(t *testing.T) { require.Len(out.TaskGroups[0].Tasks, 2) require.Exactly(sidecarTask, out.TaskGroups[0].Tasks[1]) +} + +func TestJobEndpoint_Register_ConnectExposeCheck(t *testing.T) { + t.Parallel() + r := require.New(t) + + s1, cleanupS1 := TestServer(t, func(c *Config) { + c.NumSchedulers = 0 // Prevent automatic dequeue + }) + defer cleanupS1() + codec := rpcClient(t, s1) + testutil.WaitForLeader(t, s1.RPC) + + // Setup the job we are going to register + job := mock.Job() + job.TaskGroups[0].Networks = structs.Networks{{ + Mode: "bridge", + DynamicPorts: []structs.Port{{ + Label: "hcPort", + To: -1, + }, { + Label: "v2Port", + To: -1, + }}, + }} + job.TaskGroups[0].Services = []*structs.Service{{ + Name: "backend", + PortLabel: "8080", + Checks: []*structs.ServiceCheck{{ + Name: "check1", + Type: "http", + Protocol: "http", + Path: "/health", + Expose: true, + PortLabel: "hcPort", + Interval: 1 * time.Second, + Timeout: 1 * time.Second, + }, { + Name: "check2", + Type: "script", + Command: "/bin/true", + Interval: 1 * time.Second, + Timeout: 1 * time.Second, + }, { + Name: "check3", + Type: "grpc", + Protocol: "grpc", + Path: "/v2/health", + Expose: true, + PortLabel: "v2Port", + Interval: 1 * time.Second, + Timeout: 1 * time.Second, + }}, + Connect: &structs.ConsulConnect{ + SidecarService: &structs.ConsulSidecarService{}}, + }} + + // Create the register request + req := &structs.JobRegisterRequest{ + Job: job, + WriteRequest: structs.WriteRequest{ + Region: "global", + Namespace: job.Namespace, + }, + } + + // Fetch the response + var resp structs.JobRegisterResponse + r.NoError(msgpackrpc.CallWithCodec(codec, "Job.Register", req, &resp)) + r.NotZero(resp.Index) + + // Check for the node in the FSM + state := s1.fsm.State() + ws := memdb.NewWatchSet() + out, err := state.JobByID(ws, job.Namespace, job.ID) + r.NoError(err) + r.NotNil(out) + r.Equal(resp.JobModifyIndex, out.CreateIndex) + + // Check that the new expose paths got created + r.Len(out.TaskGroups[0].Services[0].Connect.SidecarService.Proxy.Expose.Paths, 2) + httpPath := out.TaskGroups[0].Services[0].Connect.SidecarService.Proxy.Expose.Paths[0] + r.Equal(structs.ConsulExposePath{ + Path: "/health", + Protocol: "http", + LocalPathPort: 8080, + ListenerPort: "hcPort", + }, httpPath) + grpcPath := out.TaskGroups[0].Services[0].Connect.SidecarService.Proxy.Expose.Paths[1] + r.Equal(structs.ConsulExposePath{ + Path: "/v2/health", + Protocol: "grpc", + LocalPathPort: 8080, + ListenerPort: "v2Port", + }, grpcPath) + + // make sure round tripping does not create duplicate expose paths + out.Meta["test"] = "abc" + req.Job = out + r.NoError(msgpackrpc.CallWithCodec(codec, "Job.Register", req, &resp)) + r.NotZero(resp.Index) + + // Check for the new node in the FSM + state = s1.fsm.State() + ws = memdb.NewWatchSet() + out, err = state.JobByID(ws, job.Namespace, job.ID) + r.NoError(err) + r.NotNil(out) + r.Equal(resp.JobModifyIndex, out.CreateIndex) + // make sure we are not re-adding what has already been added + r.Len(out.TaskGroups[0].Services[0].Connect.SidecarService.Proxy.Expose.Paths, 2) } func TestJobEndpoint_Register_ConnectWithSidecarTask(t *testing.T) { diff --git a/nomad/structs/diff_test.go b/nomad/structs/diff_test.go index 708e72ead823..c51f38d0f80c 100644 --- a/nomad/structs/diff_test.go +++ b/nomad/structs/diff_test.go @@ -2454,6 +2454,7 @@ func TestTaskGroupDiff(t *testing.T) { Args: []string{"foo"}, Path: "foo", Protocol: "http", + Expose: true, Interval: 1 * time.Second, Timeout: 1 * time.Second, }, @@ -2486,6 +2487,7 @@ func TestTaskGroupDiff(t *testing.T) { Command: "bar", Path: "bar", Protocol: "tcp", + Expose: false, Interval: 2 * time.Second, Timeout: 2 * time.Second, Header: map[string][]string{ @@ -2548,7 +2550,6 @@ func TestTaskGroupDiff(t *testing.T) { }, }, Objects: []*ObjectDiff{ - { Type: DiffTypeEdited, Name: "Check", @@ -2565,6 +2566,12 @@ func TestTaskGroupDiff(t *testing.T) { Old: "foo", New: "bar", }, + { + Type: DiffTypeEdited, + Name: "Expose", + Old: "true", + New: "false", + }, { Type: DiffTypeNone, Name: "GRPCService", @@ -2619,6 +2626,7 @@ func TestTaskGroupDiff(t *testing.T) { Old: "http", New: "tcp", }, + { Type: DiffTypeNone, Name: "TLSSkipVerify", @@ -4864,6 +4872,12 @@ func TestTaskDiff(t *testing.T) { Old: "", New: "foo", }, + { + Type: DiffTypeAdded, + Name: "Expose", + Old: "", + New: "false", + }, { Type: DiffTypeAdded, Name: "GRPCUseTLS", @@ -4924,6 +4938,12 @@ func TestTaskDiff(t *testing.T) { Old: "foo", New: "", }, + { + Type: DiffTypeDeleted, + Name: "Expose", + Old: "false", + New: "", + }, { Type: DiffTypeDeleted, Name: "GRPCUseTLS", @@ -5092,6 +5112,12 @@ func TestTaskDiff(t *testing.T) { Old: "foo", New: "foo", }, + { + Type: DiffTypeNone, + Name: "Expose", + Old: "false", + New: "false", + }, { Type: DiffTypeNone, Name: "GRPCService", diff --git a/nomad/structs/services.go b/nomad/structs/services.go index 5b9f38f2fa66..04a34d0e450f 100644 --- a/nomad/structs/services.go +++ b/nomad/structs/services.go @@ -47,6 +47,7 @@ type ServiceCheck struct { Path string // path of the health check url for http type check Protocol string // Protocol to use if check is http, defaults to http PortLabel string // The port to use for tcp/http checks + Expose bool // Whether to have Envoy expose the check path (connect-enabled group-services only) AddressMode string // 'host' to use host ip:port or 'driver' to use driver's Interval time.Duration // Interval of the check Timeout time.Duration // Timeout of the response from the check before consul fails the check @@ -136,6 +137,10 @@ func (sc *ServiceCheck) Equals(o *ServiceCheck) bool { return false } + if sc.Expose != o.Expose { + return false + } + if sc.Protocol != o.Protocol { return false } @@ -180,18 +185,19 @@ func (sc *ServiceCheck) Canonicalize(serviceName string) { // validate a Service's ServiceCheck func (sc *ServiceCheck) validate() error { // Validate Type - switch strings.ToLower(sc.Type) { + checkType := strings.ToLower(sc.Type) + switch checkType { case ServiceCheckGRPC: case ServiceCheckTCP: case ServiceCheckHTTP: if sc.Path == "" { return fmt.Errorf("http type must have a valid http path") } - url, err := url.Parse(sc.Path) + checkPath, err := url.Parse(sc.Path) if err != nil { return fmt.Errorf("http type must have a valid http path") } - if url.IsAbs() { + if checkPath.IsAbs() { return fmt.Errorf("http type must have a relative http path") } @@ -238,6 +244,19 @@ func (sc *ServiceCheck) validate() error { return fmt.Errorf("invalid address_mode %q", sc.AddressMode) } + // Note that we cannot completely validate the Expose field yet - we do not + // know whether this ServiceCheck belongs to a connect-enabled group-service. + // Instead, such validation will happen in a job admission controller. + if sc.Expose { + // We can however immediately ensure expose is configured only for HTTP + // and gRPC checks. + switch checkType { + case ServiceCheckGRPC, ServiceCheckHTTP: // ok + default: + return fmt.Errorf("expose may only be set on HTTP or gRPC checks") + } + } + return sc.CheckRestart.Validate() } @@ -263,49 +282,58 @@ func (sc *ServiceCheck) TriggersRestarts() bool { // called. func (sc *ServiceCheck) Hash(serviceID string) string { h := sha1.New() - io.WriteString(h, serviceID) - io.WriteString(h, sc.Name) - io.WriteString(h, sc.Type) - io.WriteString(h, sc.Command) - io.WriteString(h, strings.Join(sc.Args, "")) - io.WriteString(h, sc.Path) - io.WriteString(h, sc.Protocol) - io.WriteString(h, sc.PortLabel) - io.WriteString(h, sc.Interval.String()) - io.WriteString(h, sc.Timeout.String()) - io.WriteString(h, sc.Method) - // Only include TLSSkipVerify if set to maintain ID stability with Nomad <0.6 - if sc.TLSSkipVerify { - io.WriteString(h, "true") - } - - // Since map iteration order isn't stable we need to write k/v pairs to - // a slice and sort it before hashing. - if len(sc.Header) > 0 { - headers := make([]string, 0, len(sc.Header)) - for k, v := range sc.Header { - headers = append(headers, k+strings.Join(v, "")) - } - sort.Strings(headers) - io.WriteString(h, strings.Join(headers, "")) - } + hashString(h, serviceID) + hashString(h, sc.Name) + hashString(h, sc.Type) + hashString(h, sc.Command) + hashString(h, strings.Join(sc.Args, "")) + hashString(h, sc.Path) + hashString(h, sc.Protocol) + hashString(h, sc.PortLabel) + hashBool(h, sc.Expose, "Expose") + hashString(h, sc.Interval.String()) + hashString(h, sc.Timeout.String()) + hashString(h, sc.Method) + + // use name "true" to maintain ID stability + hashBool(h, sc.TLSSkipVerify, "true") + + // maintain artisanal map hashing to maintain ID stability + hashHeader(h, sc.Header) // Only include AddressMode if set to maintain ID stability with Nomad <0.7.1 - if len(sc.AddressMode) > 0 { - io.WriteString(h, sc.AddressMode) - } + hashStringIfNonEmpty(h, sc.AddressMode) - // Only include GRPC if set to maintain ID stability with Nomad <0.8.4 - if sc.GRPCService != "" { - io.WriteString(h, sc.GRPCService) - } - if sc.GRPCUseTLS { - io.WriteString(h, "true") - } + // Only include gRPC if set to maintain ID stability with Nomad <0.8.4 + hashStringIfNonEmpty(h, sc.GRPCService) + + // use name "true" to maintain ID stability + hashBool(h, sc.GRPCUseTLS, "true") + // maintain use of hex (i.e. not b32) to maintain ID stability return fmt.Sprintf("%x", h.Sum(nil)) } +func hashStringIfNonEmpty(h hash.Hash, s string) { + if len(s) > 0 { + hashString(h, s) + } +} + +func hashHeader(h hash.Hash, m map[string][]string) { + // maintain backwards compatibility for ID stability + // using the %v formatter on a map with string keys produces consistent + // output, but our existing format here is incompatible + if len(m) > 0 { + headers := make([]string, 0, len(m)) + for k, v := range m { + headers = append(headers, k+strings.Join(v, "")) + } + sort.Strings(headers) + hashString(h, strings.Join(headers, "")) + } +} + const ( AddressModeAuto = "auto" AddressModeHost = "host" diff --git a/nomad/structs/structs_test.go b/nomad/structs/structs_test.go index a1c999d5e787..b34269d4c814 100644 --- a/nomad/structs/structs_test.go +++ b/nomad/structs/structs_test.go @@ -1487,6 +1487,26 @@ func TestTask_Validate_Service_Check(t *testing.T) { if !strings.Contains(err.Error(), "relative http path") { t.Fatalf("err: %v", err) } + + t.Run("check expose", func(t *testing.T) { + t.Run("type http", func(t *testing.T) { + require.NoError(t, (&ServiceCheck{ + Type: ServiceCheckHTTP, + Interval: 1 * time.Second, + Timeout: 1 * time.Second, + Path: "/health", + Expose: true, + }).validate()) + }) + t.Run("type tcp", func(t *testing.T) { + require.EqualError(t, (&ServiceCheck{ + Type: ServiceCheckTCP, + Interval: 1 * time.Second, + Timeout: 1 * time.Second, + Expose: true, + }).validate(), "expose may only be set on HTTP or gRPC checks") + }) + }) } // TestTask_Validate_Service_Check_AddressMode asserts that checks do not