From 25569282b9b0ed6aeafdfeedc7469ad092db94f5 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Tue, 5 Dec 2017 11:39:42 -0800 Subject: [PATCH] Allow custom ports for services and checks Fixes #3380 Adds address_mode to checks (but no auto) and allows services and checks to set literal port numbers when using address_mode=driver. This allows SDNs, overlays, etc to advertise internal and host addresses as well as do checks against either. --- api/tasks.go | 1 + command/agent/consul/client.go | 91 +++++++++--- command/agent/consul/unit_test.go | 139 ++++++++++++++++++ command/agent/job_endpoint.go | 1 + command/agent/job_endpoint_test.go | 2 + jobspec/parse.go | 1 + jobspec/parse_test.go | 48 ++++++ .../service-check-driver-address.hcl | 38 +++++ nomad/structs/structs.go | 27 +++- 9 files changed, 323 insertions(+), 25 deletions(-) create mode 100644 jobspec/test-fixtures/service-check-driver-address.hcl diff --git a/api/tasks.go b/api/tasks.go index 462d9f549..7dc2950b1 100644 --- a/api/tasks.go +++ b/api/tasks.go @@ -154,6 +154,7 @@ type ServiceCheck struct { Path string Protocol string PortLabel string `mapstructure:"port"` + AddressMode string `mapstructure:"address_mode"` Interval time.Duration Timeout time.Duration InitialStatus string `mapstructure:"initial_status"` diff --git a/command/agent/consul/client.go b/command/agent/consul/client.go index 57912b35b..574e89dd9 100644 --- a/command/agent/consul/client.go +++ b/command/agent/consul/client.go @@ -569,23 +569,16 @@ func (c *ServiceClient) serviceRegs(ops *operations, allocID string, service *st checkIDs: make(map[string]struct{}, len(service.Checks)), } - // Determine the address to advertise + // Service address modes default to auto addrMode := service.AddressMode - if addrMode == structs.AddressModeAuto { - if net.Advertise() { - addrMode = structs.AddressModeDriver - } else { - // No driver network or shouldn't default to driver's network - addrMode = structs.AddressModeHost - } + if addrMode == "" { + addrMode = structs.AddressModeAuto } - ip, port := task.Resources.Networks.Port(service.PortLabel) - if addrMode == structs.AddressModeDriver { - if net == nil { - return nil, fmt.Errorf("service %s cannot use driver's IP because driver didn't set one", service.Name) - } - ip = net.IP - port = net.PortMap[service.PortLabel] + + // Determine the address to advertise based on the mode + ip, port, err := getAddress(addrMode, service.PortLabel, task.Resources.Networks, net) + if err != nil { + return nil, fmt.Errorf("unable to get address for service %q: %v", service.Name, err) } // Build the Consul Service registration request @@ -641,13 +634,24 @@ func (c *ServiceClient) checkRegs(ops *operations, allocID, serviceID string, se } - // Checks should always use the host ip:port + // Default to the service's port but allow check to override portLabel := check.PortLabel if portLabel == "" { // Default to the service's port label portLabel = service.PortLabel } - ip, port := task.Resources.Networks.Port(portLabel) + + // Checks address mode defaults to host for pre-#3380 backward compat + addrMode := check.AddressMode + if addrMode == "" { + addrMode = structs.AddressModeHost + } + + 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) + } + checkReg, err := createCheckReg(serviceID, checkID, check, ip, port) if err != nil { return nil, fmt.Errorf("failed to add check %q: %v", check.Name, err) @@ -709,8 +713,8 @@ func (c *ServiceClient) RegisterTask(allocID string, task *structs.Task, restart func (c *ServiceClient) UpdateTask(allocID string, existing, newTask *structs.Task, restarter TaskRestarter, exec driver.ScriptExecutor, net *cstructs.DriverNetwork) error { ops := &operations{} - t := new(TaskRegistration) - t.Services = make(map[string]*ServiceRegistration, len(newTask.Services)) + taskReg := new(TaskRegistration) + taskReg.Services = make(map[string]*ServiceRegistration, len(newTask.Services)) existingIDs := make(map[string]*structs.Service, len(existing.Services)) for _, s := range existing.Services { @@ -745,7 +749,7 @@ func (c *ServiceClient) UpdateTask(allocID string, existing, newTask *structs.Ta serviceID: existingID, checkIDs: make(map[string]struct{}, len(newSvc.Checks)), } - t.Services[existingID] = sreg + taskReg.Services[existingID] = sreg // PortLabel and AddressMode aren't included in the ID, so we // have to compare manually. @@ -755,7 +759,7 @@ func (c *ServiceClient) UpdateTask(allocID string, existing, newTask *structs.Ta delete(newIDs, existingID) } - // Check to see what checks were updated + // See what checks were updated existingChecks := make(map[string]*structs.ServiceCheck, len(existingSvc.Checks)) for _, check := range existingSvc.Checks { existingChecks[makeCheckID(existingID, check)] = check @@ -806,11 +810,11 @@ func (c *ServiceClient) UpdateTask(allocID string, existing, newTask *structs.Ta return err } - t.Services[sreg.serviceID] = sreg + taskReg.Services[sreg.serviceID] = sreg } // Add the task to the allocation's registration - c.addTaskRegistration(allocID, newTask.Name, t) + c.addTaskRegistration(allocID, newTask.Name, taskReg) c.commit(ops) @@ -1079,3 +1083,44 @@ func isNomadService(id string) bool { const prefix = nomadServicePrefix + "-executor" 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. +func getAddress(addrMode, portLabel string, networks structs.Networks, driverNet *cstructs.DriverNetwork) (string, int, error) { + switch addrMode { + case structs.AddressModeAuto: + if driverNet.Advertise() { + addrMode = structs.AddressModeDriver + } else { + addrMode = structs.AddressModeHost + } + return getAddress(addrMode, portLabel, networks, driverNet) + case structs.AddressModeHost: + // Default path: use host ip:port + ip, port := networks.Port(portLabel) + return ip, port, nil + + case structs.AddressModeDriver: + // Require a driver network if driver address mode is used + if driverNet == nil { + return "", 0, fmt.Errorf(`cannot use address_mode="driver": no driver network exists`) + } + + // If the port is a label, use the driver's port (not the host's) + if port, ok := driverNet.PortMap[portLabel]; ok { + return driverNet.IP, port, nil + } + + // If port isn't a label, try to parse it as a literal port number + port, err := strconv.Atoi(portLabel) + if err != nil { + return "", 0, fmt.Errorf("invalid port %q: %v", portLabel, err) + } + + return driverNet.IP, port, nil + + default: + // Shouldn't happen due to validation, but enforce invariants + return "", 0, fmt.Errorf("invalid address mode %q", addrMode) + } +} diff --git a/command/agent/consul/unit_test.go b/command/agent/consul/unit_test.go index 8bd3e08a5..73d82bd12 100644 --- a/command/agent/consul/unit_test.go +++ b/command/agent/consul/unit_test.go @@ -16,6 +16,7 @@ import ( cstructs "github.com/hashicorp/nomad/client/structs" "github.com/hashicorp/nomad/nomad/structs" "github.com/kr/pretty" + "github.com/stretchr/testify/assert" ) const ( @@ -1440,3 +1441,141 @@ func TestCreateCheckReg(t *testing.T) { t.Fatalf("diff:\n%s\n", strings.Join(diff, "\n")) } } + +// TestGetAddress asserts Nomad uses the correct ip and port for services and +// checks depending on port labels, driver networks, and address mode. +func TestGetAddress(t *testing.T) { + const HostIP = "127.0.0.1" + + cases := []struct { + Name string + + // Parameters + Mode string + PortLabel string + Host map[string]int // will be converted to structs.Networks + Driver *cstructs.DriverNetwork + + // Results + IP string + Port int + ErrContains string + }{ + { + Name: "ExampleService", + Mode: structs.AddressModeAuto, + PortLabel: "db", + Host: map[string]int{"db": 12435}, + Driver: &cstructs.DriverNetwork{ + PortMap: map[string]int{"db": 6379}, + IP: "10.1.2.3", + }, + IP: HostIP, + Port: 12435, + }, + { + Name: "Host", + Mode: structs.AddressModeHost, + PortLabel: "db", + Host: map[string]int{"db": 12345}, + Driver: &cstructs.DriverNetwork{ + PortMap: map[string]int{"db": 6379}, + IP: "10.1.2.3", + }, + IP: HostIP, + Port: 12345, + }, + { + Name: "Driver", + Mode: structs.AddressModeDriver, + PortLabel: "db", + Host: map[string]int{"db": 12345}, + Driver: &cstructs.DriverNetwork{ + PortMap: map[string]int{"db": 6379}, + IP: "10.1.2.3", + }, + IP: "10.1.2.3", + Port: 6379, + }, + { + Name: "AutoDriver", + Mode: structs.AddressModeAuto, + PortLabel: "db", + Host: map[string]int{"db": 12345}, + Driver: &cstructs.DriverNetwork{ + PortMap: map[string]int{"db": 6379}, + IP: "10.1.2.3", + AutoAdvertise: true, + }, + IP: "10.1.2.3", + Port: 6379, + }, + { + Name: "DriverCustomPort", + Mode: structs.AddressModeDriver, + PortLabel: "7890", + Host: map[string]int{"db": 12345}, + Driver: &cstructs.DriverNetwork{ + PortMap: map[string]int{"db": 6379}, + IP: "10.1.2.3", + }, + IP: "10.1.2.3", + Port: 7890, + }, + { + Name: "DriverWithoutNetwork", + Mode: structs.AddressModeDriver, + PortLabel: "db", + Host: map[string]int{"db": 12345}, + Driver: nil, + ErrContains: "no driver network exists", + }, + { + Name: "DriverBadPort", + Mode: structs.AddressModeDriver, + PortLabel: "bad-port-label", + Host: map[string]int{"db": 12345}, + Driver: &cstructs.DriverNetwork{ + PortMap: map[string]int{"db": 6379}, + IP: "10.1.2.3", + }, + ErrContains: "invalid port", + }, + { + Name: "InvalidMode", + Mode: "invalid-mode", + ErrContains: "invalid address mode", + }, + } + + for _, tc := range cases { + t.Run(tc.Name, func(t *testing.T) { + // convert host port map into a structs.Networks + networks := []*structs.NetworkResource{ + { + IP: HostIP, + ReservedPorts: make([]structs.Port, len(tc.Host)), + }, + } + + i := 0 + for label, port := range tc.Host { + networks[0].ReservedPorts[i].Label = label + networks[0].ReservedPorts[i].Value = port + i++ + } + + // Run getAddress + ip, port, err := getAddress(tc.Mode, tc.PortLabel, networks, tc.Driver) + + // Assert the results + assert.Equal(t, tc.IP, ip, "IP mismatch") + assert.Equal(t, tc.Port, port, "Port mismatch") + if tc.ErrContains == "" { + assert.Nil(t, err) + } else { + assert.Contains(t, err.Error(), tc.ErrContains) + } + }) + } +} diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index 9a154679e..6b0e3a565 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -710,6 +710,7 @@ func ApiTaskToStructsTask(apiTask *api.Task, structsTask *structs.Task) { Path: check.Path, Protocol: check.Protocol, PortLabel: check.PortLabel, + AddressMode: check.AddressMode, Interval: check.Interval, Timeout: check.Timeout, InitialStatus: check.InitialStatus, diff --git a/command/agent/job_endpoint_test.go b/command/agent/job_endpoint_test.go index 8eb74a9b9..019a82ae0 100644 --- a/command/agent/job_endpoint_test.go +++ b/command/agent/job_endpoint_test.go @@ -1222,6 +1222,7 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) { Path: "/check", Protocol: "http", PortLabel: "foo", + AddressMode: "driver", Interval: 4 * time.Second, Timeout: 2 * time.Second, InitialStatus: "ok", @@ -1418,6 +1419,7 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) { Path: "/check", Protocol: "http", PortLabel: "foo", + AddressMode: "driver", Interval: 4 * time.Second, Timeout: 2 * time.Second, InitialStatus: "ok", diff --git a/jobspec/parse.go b/jobspec/parse.go index f44cb7540..d25f38bd0 100644 --- a/jobspec/parse.go +++ b/jobspec/parse.go @@ -981,6 +981,7 @@ func parseChecks(service *api.Service, checkObjs *ast.ObjectList) error { "header", "method", "check_restart", + "address_mode", } if err := helper.CheckHCLKeys(co.Val, valid); err != nil { return multierror.Prefix(err, "check ->") diff --git a/jobspec/parse_test.go b/jobspec/parse_test.go index 5922edeff..2dfc890d4 100644 --- a/jobspec/parse_test.go +++ b/jobspec/parse_test.go @@ -583,6 +583,54 @@ func TestParse(t *testing.T) { }, false, }, + { + "service-check-driver-address.hcl", + &api.Job{ + ID: helper.StringToPtr("address_mode_driver"), + Name: helper.StringToPtr("address_mode_driver"), + Type: helper.StringToPtr("service"), + TaskGroups: []*api.TaskGroup{ + { + Name: helper.StringToPtr("group"), + Tasks: []*api.Task{ + { + Name: "task", + Services: []*api.Service{ + { + Name: "http-service", + PortLabel: "http", + AddressMode: "auto", + Checks: []api.ServiceCheck{ + { + Name: "http-check", + Type: "http", + Path: "/", + PortLabel: "http", + AddressMode: "driver", + }, + }, + }, + { + Name: "random-service", + PortLabel: "9000", + AddressMode: "driver", + Checks: []api.ServiceCheck{ + { + Name: "random-check", + Type: "tcp", + PortLabel: "9001", + AddressMode: "driver", + }, + }, + }, + }, + }, + }, + }, + }, + }, + false, + }, } for _, tc := range cases { diff --git a/jobspec/test-fixtures/service-check-driver-address.hcl b/jobspec/test-fixtures/service-check-driver-address.hcl new file mode 100644 index 000000000..a9fa88d42 --- /dev/null +++ b/jobspec/test-fixtures/service-check-driver-address.hcl @@ -0,0 +1,38 @@ +job "address_mode_driver" { + type = "service" + group "group" { + task "task" { + service { + name = "http-service" + port = "http" + + address_mode = "auto" + + check { + name = "http-check" + type = "http" + path = "/" + port = "http" + + address_mode = "driver" + } + } + + service { + name = "random-service" + port = "9000" + + address_mode = "driver" + + check { + name = "random-check" + type = "tcp" + port = "9001" + + address_mode = "driver" + } + } + } + } +} + diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 1836aae82..fa29071aa 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -2866,6 +2866,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 + 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 InitialStatus string // Initial status of the check @@ -2911,6 +2912,7 @@ func (sc *ServiceCheck) Canonicalize(serviceName string) { // validate a Service's ServiceCheck func (sc *ServiceCheck) validate() error { + // Validate Type switch strings.ToLower(sc.Type) { case ServiceCheckTCP: case ServiceCheckHTTP: @@ -2926,6 +2928,7 @@ func (sc *ServiceCheck) validate() error { return fmt.Errorf(`invalid type (%+q), must be one of "http", "tcp", or "script" type`, sc.Type) } + // Validate interval and timeout if sc.Interval == 0 { return fmt.Errorf("missing required value interval. Interval cannot be less than %v", minCheckInterval) } else if sc.Interval < minCheckInterval { @@ -2938,9 +2941,9 @@ func (sc *ServiceCheck) validate() error { return fmt.Errorf("timeout (%v) is lower than required minimum timeout %v", sc.Timeout, minCheckInterval) } + // Validate InitialStatus switch sc.InitialStatus { case "": - // case api.HealthUnknown: TODO: Add when Consul releases 0.7.1 case api.HealthPassing: case api.HealthWarning: case api.HealthCritical: @@ -2949,6 +2952,16 @@ func (sc *ServiceCheck) validate() error { } + // Validate AddressMode + switch sc.AddressMode { + case "", AddressModeHost, AddressModeDriver: + // Ok + case AddressModeAuto: + return fmt.Errorf("invalid address_mode %q - %s only valid for services", sc.AddressMode, AddressModeAuto) + default: + return fmt.Errorf("invalid address_mode %q", sc.AddressMode) + } + return sc.CheckRestart.Validate() } @@ -3001,6 +3014,11 @@ func (sc *ServiceCheck) Hash(serviceID string) string { io.WriteString(h, strings.Join(headers, "")) } + // Only include AddressMode if set to maintain ID stability with Nomad <0.7.1 + if len(sc.AddressMode) > 0 { + io.WriteString(h, sc.AddressMode) + } + return fmt.Sprintf("%x", h.Sum(nil)) } @@ -3455,7 +3473,12 @@ func validateServices(t *Task) error { knownServices[service.Name+service.PortLabel] = struct{}{} if service.PortLabel != "" { - servicePorts[service.PortLabel] = append(servicePorts[service.PortLabel], service.Name) + if _, err := strconv.Atoi(service.PortLabel); service.AddressMode == "driver" && err == nil { + // Numeric ports are valid when AddressMode=driver + } else { + + servicePorts[service.PortLabel] = append(servicePorts[service.PortLabel], service.Name) + } } // Ensure that check names are unique.