From 714eb0b26615e4dfd60c60322876118df10bf39f Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Mon, 18 Dec 2017 16:18:42 -0800 Subject: [PATCH] Services should not require a port Fixes #3673 --- command/agent/consul/client.go | 17 +- command/agent/consul/int_test.go | 24 ++- command/agent/consul/unit_test.go | 5 + nomad/mock/mock.go | 2 +- nomad/structs/structs.go | 14 +- nomad/structs/structs_test.go | 146 ++++++++++++++---- .../docs/job-specification/service.html.md | 9 +- 7 files changed, 164 insertions(+), 53 deletions(-) diff --git a/command/agent/consul/client.go b/command/agent/consul/client.go index c22cb8d74..b8db963ae 100644 --- a/command/agent/consul/client.go +++ b/command/agent/consul/client.go @@ -663,7 +663,7 @@ func (c *ServiceClient) checkRegs(ops *operations, allocID, serviceID string, se ip, port, err := getAddress(addrMode, portLabel, task.Resources.Networks, net) if err != nil { - return nil, fmt.Errorf("unable to get address for check %q: %v", check.Name, err) + return nil, fmt.Errorf("error getting address for check %q: %v", check.Name, err) } checkReg, err := createCheckReg(serviceID, checkID, check, ip, port) @@ -1036,6 +1036,11 @@ func createCheckReg(serviceID, checkID string, check *structs.ServiceCheck, host chkReg.Timeout = check.Timeout.String() chkReg.Interval = check.Interval.String() + // Require an address for http or tcp checks + if port == 0 && check.RequiresPort() { + return nil, fmt.Errorf("%s checks require an address", check.Type) + } + switch check.Type { case structs.ServiceCheckHTTP: proto := check.Protocol @@ -1089,9 +1094,15 @@ func isOldNomadService(id string) bool { return strings.HasPrefix(id, prefix) } -// getAddress returns the ip and port to use for a service or check. An error -// is returned if an ip and port cannot be determined. +// getAddress returns the IP and port to use for a service or check. If no port +// label is specified (an empty value), zero values are returned because no +// address could be resolved. func getAddress(addrMode, portLabel string, networks structs.Networks, driverNet *cstructs.DriverNetwork) (string, int, error) { + // No port label specified, no address can be assembled + if portLabel == "" { + return "", 0, nil + } + switch addrMode { case structs.AddressModeAuto: if driverNet.Advertise() { diff --git a/command/agent/consul/int_test.go b/command/agent/consul/int_test.go index ba9975532..ee0fff748 100644 --- a/command/agent/consul/int_test.go +++ b/command/agent/consul/int_test.go @@ -87,8 +87,17 @@ func TestConsul_Integration(t *testing.T) { task.Config = map[string]interface{}{ "run_for": "1h", } + // Choose a port that shouldn't be in use - task.Resources.Networks[0].ReservedPorts = []structs.Port{{Label: "http", Value: 3}} + netResource := &structs.NetworkResource{ + Device: "eth0", + IP: "127.0.0.1", + MBits: 50, + ReservedPorts: []structs.Port{{Label: "http", Value: 3}}, + } + alloc.Resources.Networks[0] = netResource + alloc.TaskResources["web"].Networks[0] = netResource + task.Resources.Networks[0] = netResource task.Services = []*structs.Service{ { Name: "httpd", @@ -96,13 +105,12 @@ func TestConsul_Integration(t *testing.T) { Tags: []string{"nomad", "test", "http"}, Checks: []*structs.ServiceCheck{ { - Name: "httpd-http-check", - Type: "http", - Path: "/", - Protocol: "http", - PortLabel: "http", - Interval: 9000 * time.Hour, - Timeout: 1, // fail as fast as possible + Name: "httpd-http-check", + Type: "http", + Path: "/", + Protocol: "http", + Interval: 9000 * time.Hour, + Timeout: 1, // fail as fast as possible }, { Name: "httpd-script-check", diff --git a/command/agent/consul/unit_test.go b/command/agent/consul/unit_test.go index 88acc4ee1..4d8123b88 100644 --- a/command/agent/consul/unit_test.go +++ b/command/agent/consul/unit_test.go @@ -1566,8 +1566,13 @@ func TestGetAddress(t *testing.T) { { Name: "InvalidMode", Mode: "invalid-mode", + PortLabel: "80", ErrContains: "invalid address mode", }, + { + Name: "EmptyIsOk", + Mode: structs.AddressModeHost, + }, } for _, tc := range cases { diff --git a/nomad/mock/mock.go b/nomad/mock/mock.go index d0c889b09..c4921a644 100644 --- a/nomad/mock/mock.go +++ b/nomad/mock/mock.go @@ -292,7 +292,7 @@ func Alloc() *structs.Allocation { IP: "192.168.0.100", ReservedPorts: []structs.Port{{Label: "admin", Value: 5000}}, MBits: 50, - DynamicPorts: []structs.Port{{Label: "http"}}, + DynamicPorts: []structs.Port{{Label: "http", Value: 9876}}, }, }, }, diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 439e6a858..fa994e194 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -3131,8 +3131,8 @@ func (s *Service) Validate() error { } for _, c := range s.Checks { - if s.PortLabel == "" && c.RequiresPort() { - mErr.Errors = append(mErr.Errors, fmt.Errorf("check %s invalid: check requires a port but the service %+q has no port", c.Name, s.Name)) + if s.PortLabel == "" && c.PortLabel == "" && c.RequiresPort() { + mErr.Errors = append(mErr.Errors, fmt.Errorf("check %s invalid: check requires a port but neither check nor service %+q have a port", c.Name, s.Name)) continue } @@ -3577,8 +3577,16 @@ func validateServices(t *Task) error { } } + // Iterate over a sorted list of keys to make error listings stable + keys := make([]string, 0, len(servicePorts)) + for p := range servicePorts { + keys = append(keys, p) + } + sort.Strings(keys) + // Ensure all ports referenced in services exist. - for servicePort, services := range servicePorts { + for _, servicePort := range keys { + services := servicePorts[servicePort] _, ok := portLabels[servicePort] if !ok { names := make([]string, 0, len(services)) diff --git a/nomad/structs/structs_test.go b/nomad/structs/structs_test.go index 26b2d2a02..4897e0422 100644 --- a/nomad/structs/structs_test.go +++ b/nomad/structs/structs_test.go @@ -1235,55 +1235,93 @@ func TestTask_Validate_Service_Check(t *testing.T) { // TestTask_Validate_Service_Check_AddressMode asserts that checks do not // inherit address mode but do inherit ports. func TestTask_Validate_Service_Check_AddressMode(t *testing.T) { - task := &Task{ - Resources: &Resources{ - Networks: []*NetworkResource{ - { - DynamicPorts: []Port{ - { - Label: "http", - Value: 9999, + getTask := func(s *Service) *Task { + return &Task{ + Resources: &Resources{ + Networks: []*NetworkResource{ + { + DynamicPorts: []Port{ + { + Label: "http", + Value: 9999, + }, }, }, }, }, - }, - Services: []*Service{ - { + Services: []*Service{s}, + } + } + + cases := []struct { + Service *Service + ErrContains string + }{ + { + Service: &Service{ Name: "invalid-driver", PortLabel: "80", AddressMode: "host", }, - { - Name: "http-driver", + ErrContains: `port label "80" referenced`, + }, + { + Service: &Service{ + Name: "http-driver-fail-1", PortLabel: "80", AddressMode: "driver", Checks: []*ServiceCheck{ { - // Should fail Name: "invalid-check-1", Type: "tcp", Interval: time.Second, Timeout: time.Second, }, + }, + }, + ErrContains: `check "invalid-check-1" cannot use a numeric port`, + }, + { + Service: &Service{ + Name: "http-driver-fail-2", + PortLabel: "80", + AddressMode: "driver", + Checks: []*ServiceCheck{ { - // Should fail Name: "invalid-check-2", Type: "tcp", PortLabel: "80", Interval: time.Second, Timeout: time.Second, }, + }, + }, + ErrContains: `check "invalid-check-2" cannot use a numeric port`, + }, + { + Service: &Service{ + Name: "http-driver-fail-3", + PortLabel: "80", + AddressMode: "driver", + Checks: []*ServiceCheck{ { - // Should fail Name: "invalid-check-3", Type: "tcp", PortLabel: "missing-port-label", Interval: time.Second, Timeout: time.Second, }, + }, + }, + ErrContains: `port label "missing-port-label" referenced`, + }, + { + Service: &Service{ + Name: "http-driver-passes", + PortLabel: "80", + AddressMode: "driver", + Checks: []*ServiceCheck{ { - // Should pass Name: "valid-script-check", Type: "script", Command: "ok", @@ -1291,7 +1329,6 @@ func TestTask_Validate_Service_Check_AddressMode(t *testing.T) { Timeout: time.Second, }, { - // Should pass Name: "valid-host-check", Type: "tcp", PortLabel: "http", @@ -1299,7 +1336,6 @@ func TestTask_Validate_Service_Check_AddressMode(t *testing.T) { Timeout: time.Second, }, { - // Should pass Name: "valid-driver-check", Type: "tcp", AddressMode: "driver", @@ -1309,23 +1345,65 @@ func TestTask_Validate_Service_Check_AddressMode(t *testing.T) { }, }, }, - } - err := validateServices(task) - if err == nil { - t.Fatalf("expected errors but task validated successfully") - } - errs := err.(*multierror.Error).Errors - if expected := 4; len(errs) != expected { - for i, err := range errs { - t.Logf("errs[%d] -> %s", i, err) - } - t.Fatalf("expected %d errors but found %d", expected, len(errs)) + { + Service: &Service{ + Name: "empty-address-3673-passes-1", + Checks: []*ServiceCheck{ + { + Name: "valid-port-label", + Type: "tcp", + PortLabel: "http", + Interval: time.Second, + Timeout: time.Second, + }, + { + Name: "empty-is-ok", + Type: "script", + Command: "ok", + Interval: time.Second, + Timeout: time.Second, + }, + }, + }, + }, + { + Service: &Service{ + Name: "empty-address-3673-passes-2", + }, + }, + { + Service: &Service{ + Name: "empty-address-3673-fails", + Checks: []*ServiceCheck{ + { + Name: "empty-is-not-ok", + Type: "tcp", + Interval: time.Second, + Timeout: time.Second, + }, + }, + }, + ErrContains: `invalid: check requires a port but neither check nor service`, + }, } - assert.Contains(t, errs[0].Error(), `check "invalid-check-1" cannot use a numeric port`) - assert.Contains(t, errs[1].Error(), `check "invalid-check-2" cannot use a numeric port`) - assert.Contains(t, errs[2].Error(), `port label "80" referenced`) - assert.Contains(t, errs[3].Error(), `port label "missing-port-label" referenced`) + for _, tc := range cases { + tc := tc + task := getTask(tc.Service) + t.Run(tc.Service.Name, func(t *testing.T) { + err := validateServices(task) + if err == nil && tc.ErrContains == "" { + // Ok! + return + } + if err == nil { + t.Fatalf("no error returned. expected: %s", tc.ErrContains) + } + if !strings.Contains(err.Error(), tc.ErrContains) { + t.Fatalf("expected %q but found: %v", tc.ErrContains, err) + } + }) + } } func TestTask_Validate_Service_Check_CheckRestart(t *testing.T) { diff --git a/website/source/docs/job-specification/service.html.md b/website/source/docs/job-specification/service.html.md index 9707d8ec4..578f0c1ed 100644 --- a/website/source/docs/job-specification/service.html.md +++ b/website/source/docs/job-specification/service.html.md @@ -96,7 +96,7 @@ does not automatically enable service discovery. interpolated and revalidated. This can cause certain service names to pass validation at submit time but fail at runtime. -- `port` `(string: )` - Specifies the label of the port on which this +- `port` `(string: )` - Specifies the label of the port on which this service is running. Note this is the _label_ of the port and not the port number unless `address_mode = driver`. The port label must match one defined in the [`network`][network] stanza unless you're also using @@ -174,14 +174,15 @@ scripts. add the IP of the service and the port, so this is just the relative URL to the health check endpoint. This is required for http-based health checks. -- `port` `(string: )` - Specifies the label of the port on which the +- `port` `(string: )` - Specifies the label of the port on which the check will be performed. Note this is the _label_ of the port and not the port number unless `address_mode = driver`. The port label must match one defined in the [`network`][network] stanza. If a port value was declared on the `service`, this will inherit from that value if not supplied. If supplied, this value takes precedence over the `service.port` value. This is useful for - services which operate on multiple ports. Checks will use the host IP and - ports by default. In Nomad 0.7.1 or later numeric ports may be used if + services which operate on multiple ports. `http` and `tcp` checks require a + port while `script` checks do not. Checks will use the host IP and ports by + default. In Nomad 0.7.1 or later numeric ports may be used if `address_mode="driver"` is set on the check. - `protocol` `(string: "http")` - Specifies the protocol for the http-based