From 54a22cef8ea360db92870cba39b32180cf37134b Mon Sep 17 00:00:00 2001 From: Nick Ethier Date: Wed, 6 Jan 2021 13:52:48 -0500 Subject: [PATCH 1/3] command/agent/consul: use port's to value when building service address under 'alloc' addr_mode --- CHANGELOG.md | 1 + command/agent/consul/service_client.go | 6 +- command/agent/consul/service_client_test.go | 77 +++++++++++++++++++ .../docs/job-specification/service.mdx | 6 +- 4 files changed, 88 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 90e396c68..f8776288d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ IMPROVEMENTS: BUG FIXES: * consul: Fixed a bug where updating a task to include services would not work [[GH-9707](https://github.com/hashicorp/nomad/issues/9707)] + * consul: Fixed alloc address mode port advertisement to use the mapped `to` port value [[GH-9730](https://github.com/hashicorp/nomad/issues/9730)] * consul/connect: Fixed a bug where absent ingress envoy proxy configuration could panic client [[GH-9669](https://github.com/hashicorp/nomad/issues/9669)] * template: Fixed multiple issues in template src/dest and artifact dest interpolation [[GH-9671](https://github.com/hashicorp/nomad/issues/9671)] * template: Fixed a bug where dynamic secrets did not trigger the template `change_mode` after a client restart. [[GH-9636](https://github.com/hashicorp/nomad/issues/9636)] diff --git a/command/agent/consul/service_client.go b/command/agent/consul/service_client.go index 532547124..a96fd78c3 100644 --- a/command/agent/consul/service_client.go +++ b/command/agent/consul/service_client.go @@ -1560,7 +1560,7 @@ func getAddress(addrMode, portLabel string, networks structs.Networks, driverNet return driverNet.IP, port, nil - case "alloc": + case structs.AddressModeAlloc: if netStatus == nil { return "", 0, fmt.Errorf(`cannot use address_mode="alloc": no allocation network status reported`) } @@ -1572,6 +1572,10 @@ func getAddress(addrMode, portLabel string, networks structs.Networks, driverNet // If port is a label and is found then return it if port, ok := ports.Get(portLabel); ok { + // Use port.To value unless not set + if port.To > 0 { + return netStatus.Address, port.To, nil + } return netStatus.Address, port.Value, nil } diff --git a/command/agent/consul/service_client_test.go b/command/agent/consul/service_client_test.go index bb90313c2..0dcc5c49f 100644 --- a/command/agent/consul/service_client_test.go +++ b/command/agent/consul/service_client_test.go @@ -4,6 +4,9 @@ import ( "reflect" "testing" + "github.com/hashicorp/nomad/nomad/structs" + "github.com/hashicorp/nomad/plugins/drivers" + "github.com/hashicorp/consul/api" "github.com/stretchr/testify/require" ) @@ -300,3 +303,77 @@ func TestSyncLogic_maybeTweakTags_emptySC(t *testing.T) { }, }) } + +func TestServiceClient_getAddress(t *testing.T) { + cases := []struct { + name string + addrMode string + portLabel string + networks structs.Networks + driverNet *drivers.DriverNetwork + ports structs.AllocatedPorts + netStatus *structs.AllocNetworkStatus + resultAddr string + resultPort int + resultHasErr bool + }{ + { + name: "nil case", + resultHasErr: true, + }, + { + name: "auto happy path", + addrMode: "auto", + portLabel: "www", + ports: []structs.AllocatedPortMapping{ + { + "www", + 80, + 80, + "10.0.0.1", + }, + }, + resultAddr: "10.0.0.1", + resultPort: 80, + }, + { + name: "alloc mode with to value", + addrMode: "alloc", + portLabel: "www", + ports: []structs.AllocatedPortMapping{{"www", 22222, 80, "10.0.0.1"}}, + netStatus: &structs.AllocNetworkStatus{ + + Address: "172.16.0.1", + }, + resultAddr: "172.16.0.1", + resultPort: 80, + }, + { + name: "alloc mode without to value", + addrMode: "alloc", + portLabel: "www", + ports: []structs.AllocatedPortMapping{{"www", 22222, 0, "10.0.0.1"}}, + netStatus: &structs.AllocNetworkStatus{ + + Address: "172.16.0.1", + }, + resultAddr: "172.16.0.1", + resultPort: 22222, + }, + } + require := require.New(t) + for _, c := range cases { + tc := c + t.Run(c.name, func(t *testing.T) { + + addr, port, err := getAddress(tc.addrMode, tc.portLabel, tc.networks, tc.driverNet, tc.ports, tc.netStatus) + if tc.resultHasErr { + require.Error(err) + return + } + require.Equal(addr, tc.resultAddr) + require.Equal(port, tc.resultPort) + require.NoError(err) + }) + } +} diff --git a/website/content/docs/job-specification/service.mdx b/website/content/docs/job-specification/service.mdx index c8af46898..070757a05 100644 --- a/website/content/docs/job-specification/service.mdx +++ b/website/content/docs/job-specification/service.mdx @@ -106,9 +106,13 @@ Connect][connect] integration. service. The value of `port` depends on which [`address_mode`](#address_mode) is being used: + - `alloc` - Advertises the mapped to value of the labeled port and the allocation address. + If a `to` value is not set, the port fallsback to using the allocated host port. The `port` + field may be a numeric port or a port label specified in the same group's network stanza. + - `driver` - Advertise the port determined by the driver (e.g. Docker or rkt). The `port` may be a numeric port or a port label specified in the driver's - `port_map`. + `ports` field. - `host` - Advertise the host port for this service. `port` must match a port _label_ specified in the [`network`][network] stanza. From da1bf449c1600c45dc72abb0a60a8cca26b163fd Mon Sep 17 00:00:00 2001 From: Nick Ethier Date: Wed, 6 Jan 2021 14:11:31 -0500 Subject: [PATCH 2/3] command/agent/consul: remove duplicated tests --- command/agent/consul/service_client_test.go | 77 --------------------- command/agent/consul/unit_test.go | 18 +++++ 2 files changed, 18 insertions(+), 77 deletions(-) diff --git a/command/agent/consul/service_client_test.go b/command/agent/consul/service_client_test.go index 0dcc5c49f..bb90313c2 100644 --- a/command/agent/consul/service_client_test.go +++ b/command/agent/consul/service_client_test.go @@ -4,9 +4,6 @@ import ( "reflect" "testing" - "github.com/hashicorp/nomad/nomad/structs" - "github.com/hashicorp/nomad/plugins/drivers" - "github.com/hashicorp/consul/api" "github.com/stretchr/testify/require" ) @@ -303,77 +300,3 @@ func TestSyncLogic_maybeTweakTags_emptySC(t *testing.T) { }, }) } - -func TestServiceClient_getAddress(t *testing.T) { - cases := []struct { - name string - addrMode string - portLabel string - networks structs.Networks - driverNet *drivers.DriverNetwork - ports structs.AllocatedPorts - netStatus *structs.AllocNetworkStatus - resultAddr string - resultPort int - resultHasErr bool - }{ - { - name: "nil case", - resultHasErr: true, - }, - { - name: "auto happy path", - addrMode: "auto", - portLabel: "www", - ports: []structs.AllocatedPortMapping{ - { - "www", - 80, - 80, - "10.0.0.1", - }, - }, - resultAddr: "10.0.0.1", - resultPort: 80, - }, - { - name: "alloc mode with to value", - addrMode: "alloc", - portLabel: "www", - ports: []structs.AllocatedPortMapping{{"www", 22222, 80, "10.0.0.1"}}, - netStatus: &structs.AllocNetworkStatus{ - - Address: "172.16.0.1", - }, - resultAddr: "172.16.0.1", - resultPort: 80, - }, - { - name: "alloc mode without to value", - addrMode: "alloc", - portLabel: "www", - ports: []structs.AllocatedPortMapping{{"www", 22222, 0, "10.0.0.1"}}, - netStatus: &structs.AllocNetworkStatus{ - - Address: "172.16.0.1", - }, - resultAddr: "172.16.0.1", - resultPort: 22222, - }, - } - require := require.New(t) - for _, c := range cases { - tc := c - t.Run(c.name, func(t *testing.T) { - - addr, port, err := getAddress(tc.addrMode, tc.portLabel, tc.networks, tc.driverNet, tc.ports, tc.netStatus) - if tc.resultHasErr { - require.Error(err) - return - } - require.Equal(addr, tc.resultAddr) - require.Equal(port, tc.resultPort) - require.NoError(err) - }) - } -} diff --git a/command/agent/consul/unit_test.go b/command/agent/consul/unit_test.go index 29f1a1c81..01f2c003d 100644 --- a/command/agent/consul/unit_test.go +++ b/command/agent/consul/unit_test.go @@ -1632,6 +1632,24 @@ func TestGetAddress(t *testing.T) { Address: "172.26.0.1", }, ExpectedIP: "172.26.0.1", + ExpectedPort: 6379, + }, + { + Name: "Alloc no to value", + Mode: structs.AddressModeAlloc, + PortLabel: "db", + Ports: []structs.AllocatedPortMapping{ + { + Label: "db", + Value: 12345, + HostIP: HostIP, + }, + }, + Status: &structs.AllocNetworkStatus{ + InterfaceName: "eth0", + Address: "172.26.0.1", + }, + ExpectedIP: "172.26.0.1", ExpectedPort: 12345, }, { From 90aab3fdc5a3a6b175a471626dbc5f16302a8fe0 Mon Sep 17 00:00:00 2001 From: Nick Ethier Date: Thu, 7 Jan 2021 08:53:54 -0500 Subject: [PATCH 3/3] Apply suggestions from code review Co-authored-by: Tim Gross --- website/content/docs/job-specification/service.mdx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/website/content/docs/job-specification/service.mdx b/website/content/docs/job-specification/service.mdx index 070757a05..b55752b63 100644 --- a/website/content/docs/job-specification/service.mdx +++ b/website/content/docs/job-specification/service.mdx @@ -106,8 +106,8 @@ Connect][connect] integration. service. The value of `port` depends on which [`address_mode`](#address_mode) is being used: - - `alloc` - Advertises the mapped to value of the labeled port and the allocation address. - If a `to` value is not set, the port fallsback to using the allocated host port. The `port` + - `alloc` - Advertise the mapped `to` value of the labeled port and the allocation address. + If a `to` value is not set, the port falls back to using the allocated host port. The `port` field may be a numeric port or a port label specified in the same group's network stanza. - `driver` - Advertise the port determined by the driver (e.g. Docker or rkt).