From 7cab209cce6a3af90855ee2e4a342be684808c0f Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Wed, 20 Mar 2024 10:07:13 -0400 Subject: [PATCH] jobspec: add support for destination partition to `upstream` block Adds support for specifying a destination Consul admin partition in the `upstream` block. Fixes: https://github.com/hashicorp/nomad/issues/19785 --- .changelog/20167.txt | 3 ++ api/consul.go | 19 ++++------- api/consul_test.go | 1 + command/agent/consul/connect.go | 1 + command/agent/consul/connect_test.go | 2 ++ command/agent/consul/service_client.go | 2 ++ command/agent/consul/service_client_test.go | 9 +++++ command/agent/job_endpoint.go | 1 + command/agent/job_endpoint_test.go | 2 ++ jobspec/parse_service.go | 1 + jobspec/parse_test.go | 17 +++++----- jobspec/test-fixtures/tg-network.hcl | 1 + nomad/structs/diff_test.go | 30 +++++++++++----- nomad/structs/services.go | 6 ++++ nomad/structs/services_test.go | 34 +++++++++++++------ .../docs/job-specification/upstreams.mdx | 1 + 16 files changed, 90 insertions(+), 40 deletions(-) create mode 100644 .changelog/20167.txt diff --git a/.changelog/20167.txt b/.changelog/20167.txt new file mode 100644 index 00000000000..375017307d7 --- /dev/null +++ b/.changelog/20167.txt @@ -0,0 +1,3 @@ +```release-note:improvement +consul/connect: Added support for destination partition in `upstream` block +``` diff --git a/api/consul.go b/api/consul.go index 1e1b74b9444..c62db1f6eaa 100644 --- a/api/consul.go +++ b/api/consul.go @@ -224,6 +224,7 @@ type ConsulUpstream struct { DestinationName string `mapstructure:"destination_name" hcl:"destination_name,optional"` DestinationNamespace string `mapstructure:"destination_namespace" hcl:"destination_namespace,optional"` DestinationPeer string `mapstructure:"destination_peer" hcl:"destination_peer,optional"` + DestinationPartition string `mapstructure:"destination_partition" hcl:"destination_partition,optional"` DestinationType string `mapstructure:"destination_type" hcl:"destination_type,optional"` LocalBindPort int `mapstructure:"local_bind_port" hcl:"local_bind_port,optional"` Datacenter string `mapstructure:"datacenter" hcl:"datacenter,optional"` @@ -238,19 +239,11 @@ func (cu *ConsulUpstream) Copy() *ConsulUpstream { if cu == nil { return nil } - return &ConsulUpstream{ - DestinationName: cu.DestinationName, - DestinationNamespace: cu.DestinationNamespace, - DestinationPeer: cu.DestinationPeer, - DestinationType: cu.DestinationType, - LocalBindPort: cu.LocalBindPort, - Datacenter: cu.Datacenter, - LocalBindAddress: cu.LocalBindAddress, - LocalBindSocketPath: cu.LocalBindSocketPath, - LocalBindSocketMode: cu.LocalBindSocketMode, - MeshGateway: cu.MeshGateway.Copy(), - Config: maps.Clone(cu.Config), - } + up := new(ConsulUpstream) + *up = *cu + up.MeshGateway = cu.MeshGateway.Copy() + up.Config = maps.Clone(cu.Config) + return up } func (cu *ConsulUpstream) Canonicalize() { diff --git a/api/consul_test.go b/api/consul_test.go index 38db52de8f6..37d31e70d44 100644 --- a/api/consul_test.go +++ b/api/consul_test.go @@ -178,6 +178,7 @@ func TestConsulUpstream_Copy(t *testing.T) { DestinationName: "dest1", DestinationNamespace: "ns2", DestinationPeer: "10.0.0.1:6379", + DestinationPartition: "infra", DestinationType: "tcp", Datacenter: "dc2", LocalBindPort: 2000, diff --git a/command/agent/consul/connect.go b/command/agent/consul/connect.go index fc91817ff18..3185838be0a 100644 --- a/command/agent/consul/connect.go +++ b/command/agent/consul/connect.go @@ -204,6 +204,7 @@ func connectUpstreams(in []structs.ConsulUpstream) []api.Upstream { DestinationName: upstream.DestinationName, DestinationNamespace: upstream.DestinationNamespace, DestinationType: api.UpstreamDestType(upstream.DestinationType), + DestinationPartition: upstream.DestinationPartition, DestinationPeer: upstream.DestinationPeer, LocalBindPort: upstream.LocalBindPort, LocalBindSocketPath: upstream.LocalBindSocketPath, diff --git a/command/agent/consul/connect_test.go b/command/agent/consul/connect_test.go index a7e606ff814..49b096bd093 100644 --- a/command/agent/consul/connect_test.go +++ b/command/agent/consul/connect_test.go @@ -375,6 +375,7 @@ func TestConnect_connectUpstreams(t *testing.T) { }, { DestinationName: "bar", DestinationPeer: "10.0.0.1:6379", + DestinationPartition: "infra", DestinationType: "tcp", DestinationNamespace: "ns2", LocalBindPort: 9000, @@ -391,6 +392,7 @@ func TestConnect_connectUpstreams(t *testing.T) { DestinationName: "bar", DestinationNamespace: "ns2", DestinationPeer: "10.0.0.1:6379", + DestinationPartition: "infra", DestinationType: "tcp", LocalBindPort: 9000, LocalBindSocketPath: "/var/run/testsocket.sock", diff --git a/command/agent/consul/service_client.go b/command/agent/consul/service_client.go index fb4f759c971..266de37ad1d 100644 --- a/command/agent/consul/service_client.go +++ b/command/agent/consul/service_client.go @@ -349,6 +349,8 @@ func proxyUpstreamsDifferent(wanted *api.AgentServiceConnect, sidecar *api.Agent return true case A.DestinationPeer != B.DestinationPeer: return true + case A.DestinationPartition != B.DestinationPartition: + return true case A.DestinationType != B.DestinationType: return true case A.LocalBindSocketPath != B.LocalBindSocketPath: diff --git a/command/agent/consul/service_client_test.go b/command/agent/consul/service_client_test.go index 9c61230665b..7d7aaf985c1 100644 --- a/command/agent/consul/service_client_test.go +++ b/command/agent/consul/service_client_test.go @@ -714,6 +714,15 @@ func TestSyncLogic_proxyUpstreamsDifferent(t *testing.T) { } }) + try(t, "different destination partition", func(p proxy) { + diff := upstream1() + diff.DestinationPartition = "foo" + p.Upstreams = []api.Upstream{ + diff, + upstream2(), + } + }) + try(t, "different destination type", func(p proxy) { diff := upstream1() diff.DestinationType = "service" diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index 33770122695..a1e58b22c02 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -1881,6 +1881,7 @@ func apiUpstreamsToStructs(in []*api.ConsulUpstream) []structs.ConsulUpstream { DestinationName: upstream.DestinationName, DestinationNamespace: upstream.DestinationNamespace, DestinationPeer: upstream.DestinationPeer, + DestinationPartition: upstream.DestinationPartition, DestinationType: upstream.DestinationType, LocalBindPort: upstream.LocalBindPort, LocalBindSocketPath: upstream.LocalBindSocketPath, diff --git a/command/agent/job_endpoint_test.go b/command/agent/job_endpoint_test.go index 7925740b0f5..8c5914d6535 100644 --- a/command/agent/job_endpoint_test.go +++ b/command/agent/job_endpoint_test.go @@ -4027,6 +4027,7 @@ func TestConversion_apiUpstreamsToStructs(t *testing.T) { DestinationName: "upstream", DestinationNamespace: "ns2", DestinationPeer: "10.0.0.1:6379", + DestinationPartition: "infra", DestinationType: "tcp", LocalBindPort: 8000, LocalBindSocketPath: "/var/run/testsocket.sock", @@ -4038,6 +4039,7 @@ func TestConversion_apiUpstreamsToStructs(t *testing.T) { DestinationName: "upstream", DestinationNamespace: "ns2", DestinationPeer: "10.0.0.1:6379", + DestinationPartition: "infra", DestinationType: "tcp", LocalBindPort: 8000, LocalBindSocketPath: "/var/run/testsocket.sock", diff --git a/jobspec/parse_service.go b/jobspec/parse_service.go index 2e1491fabea..38e27571d9e 100644 --- a/jobspec/parse_service.go +++ b/jobspec/parse_service.go @@ -923,6 +923,7 @@ func parseUpstream(uo *ast.ObjectItem) (*api.ConsulUpstream, error) { valid := []string{ "destination_name", "destination_peer", + "destination_partition", "destination_type", "local_bind_port", "local_bind_address", diff --git a/jobspec/parse_test.go b/jobspec/parse_test.go index c08c25da2b1..5f27d28f9df 100644 --- a/jobspec/parse_test.go +++ b/jobspec/parse_test.go @@ -1209,14 +1209,15 @@ func TestParse(t *testing.T) { LocalServicePort: 8080, Upstreams: []*api.ConsulUpstream{ { - DestinationName: "other-service", - DestinationPeer: "10.0.0.1:6379", - DestinationType: "tcp", - LocalBindPort: 4567, - LocalBindAddress: "0.0.0.0", - LocalBindSocketPath: "/var/run/testsocket.sock", - LocalBindSocketMode: "0666", - Datacenter: "dc1", + DestinationName: "other-service", + DestinationPeer: "10.0.0.1:6379", + DestinationPartition: "infra", + DestinationType: "tcp", + LocalBindPort: 4567, + LocalBindAddress: "0.0.0.0", + LocalBindSocketPath: "/var/run/testsocket.sock", + LocalBindSocketMode: "0666", + Datacenter: "dc1", MeshGateway: &api.ConsulMeshGateway{ Mode: "local", diff --git a/jobspec/test-fixtures/tg-network.hcl b/jobspec/test-fixtures/tg-network.hcl index 77c19e44c1d..c3cf36edea5 100644 --- a/jobspec/test-fixtures/tg-network.hcl +++ b/jobspec/test-fixtures/tg-network.hcl @@ -39,6 +39,7 @@ job "foo" { upstreams { destination_name = "other-service" destination_peer = "10.0.0.1:6379" + destination_partition = "infra" destination_type = "tcp" local_bind_port = 4567 local_bind_address = "0.0.0.0" diff --git a/nomad/structs/diff_test.go b/nomad/structs/diff_test.go index 71c598731c7..cef7736881e 100644 --- a/nomad/structs/diff_test.go +++ b/nomad/structs/diff_test.go @@ -3830,6 +3830,12 @@ func TestTaskGroupDiff(t *testing.T) { Old: "", New: "ns2", }, + { + Type: DiffTypeNone, + Name: "DestinationPartition", + Old: "", + New: "", + }, { Type: DiffTypeNone, Name: "DestinationPeer", @@ -9940,14 +9946,15 @@ func TestServicesDiff(t *testing.T) { LocalServicePort: 8080, Upstreams: []ConsulUpstream{ { - DestinationName: "count-api", - LocalBindPort: 8080, - Datacenter: "dc2", - LocalBindAddress: "127.0.0.1", - LocalBindSocketMode: "0700", - LocalBindSocketPath: "/tmp/redis_5678.sock", - DestinationPeer: "cloud-services", - DestinationType: "service", + DestinationName: "count-api", + LocalBindPort: 8080, + Datacenter: "dc2", + LocalBindAddress: "127.0.0.1", + LocalBindSocketMode: "0700", + LocalBindSocketPath: "/tmp/redis_5678.sock", + DestinationPeer: "cloud-services", + DestinationPartition: "infra", + DestinationType: "service", MeshGateway: ConsulMeshGateway{ Mode: "remote", }, @@ -9979,6 +9986,13 @@ func TestServicesDiff(t *testing.T) { Type: DiffTypeEdited, Name: "ConsulUpstreams", Fields: []*FieldDiff{ + { + Type: DiffTypeAdded, + Name: "DestinationPartition", + Old: "", + New: "infra", + Annotations: nil, + }, { Type: DiffTypeAdded, Name: "DestinationPeer", diff --git a/nomad/structs/services.go b/nomad/structs/services.go index fc9f017a771..1cb9a008afd 100644 --- a/nomad/structs/services.go +++ b/nomad/structs/services.go @@ -965,6 +965,7 @@ func hashConnect(h hash.Hash, connect *ConsulConnect) { hashStringIfNonEmpty(h, upstream.Datacenter) hashStringIfNonEmpty(h, upstream.LocalBindAddress) hashString(h, upstream.DestinationPeer) + hashString(h, upstream.DestinationPartition) hashString(h, upstream.DestinationType) hashString(h, upstream.LocalBindSocketPath) hashString(h, upstream.LocalBindSocketMode) @@ -1608,6 +1609,9 @@ type ConsulUpstream struct { // DestinationNamespace is the namespace of the upstream service. DestinationNamespace string + // DestinationNamespace is the admin partition of the upstream service. + DestinationPartition string + // DestinationPeer the destination service address DestinationPeer string @@ -1654,6 +1658,8 @@ func (u *ConsulUpstream) Equal(o *ConsulUpstream) bool { return false case u.DestinationPeer != o.DestinationPeer: return false + case u.DestinationPartition != o.DestinationPartition: + return false case u.DestinationType != o.DestinationType: return false case u.LocalBindPort != o.LocalBindPort: diff --git a/nomad/structs/services_test.go b/nomad/structs/services_test.go index 17d8369bbd4..312b7296802 100644 --- a/nomad/structs/services_test.go +++ b/nomad/structs/services_test.go @@ -162,13 +162,13 @@ func TestServiceCheck_validate_FailingTypes(t *testing.T) { t.Run("invalid", func(t *testing.T) { err := (&ServiceCheck{ - Name: "check", - Type: "script", - Command: "/nothing", - Interval: 1 * time.Second, - Timeout: 2 * time.Second, - SuccessBeforePassing: 0, - FailuresBeforeWarning: 3, + Name: "check", + Type: "script", + Command: "/nothing", + Interval: 1 * time.Second, + Timeout: 2 * time.Second, + SuccessBeforePassing: 0, + FailuresBeforeWarning: 3, }).validateConsul() require.EqualError(t, err, `failures_before_warning not supported for check of type "script"`) }) @@ -298,10 +298,10 @@ func TestServiceCheck_validateNomad(t *testing.T) { { name: "failures_before_warning", sc: &ServiceCheck{ - Type: ServiceCheckTCP, - FailuresBeforeWarning: 3, // consul only - Interval: 3 * time.Second, - Timeout: 1 * time.Second, + Type: ServiceCheckTCP, + FailuresBeforeWarning: 3, // consul only + Interval: 3 * time.Second, + Timeout: 1 * time.Second, }, exp: `failures_before_warning may only be set for Consul service checks`, }, @@ -786,6 +786,16 @@ func TestConsulUpstream_upstreamEqual(t *testing.T) { must.False(t, upstreamsEquals(a, b)) }) + t.Run("different dest partition", func(t *testing.T) { + a := []ConsulUpstream{up("foo", 8000)} + a[0].DestinationPeer = "infra" + + b := []ConsulUpstream{up("foo", 8000)} + b[0].DestinationPeer = "dev" + + must.False(t, upstreamsEquals(a, b)) + }) + t.Run("different dest type", func(t *testing.T) { a := []ConsulUpstream{up("foo", 8000)} a[0].DestinationType = "tcp" @@ -832,10 +842,12 @@ func TestConsulUpstream_upstreamEqual(t *testing.T) { a := []ConsulUpstream{up("foo", 8000), up("bar", 9000)} b := []ConsulUpstream{up("foo", 8000), up("bar", 9000)} a[0].DestinationPeer = "10.0.0.1:6379" + a[0].DestinationPartition = "infra" a[0].DestinationType = "tcp" a[0].LocalBindSocketPath = "/var/run/mysocket.sock" a[0].LocalBindSocketMode = "0666" b[0].DestinationPeer = "10.0.0.1:6379" + b[0].DestinationPartition = "infra" b[0].DestinationType = "tcp" b[0].LocalBindSocketPath = "/var/run/mysocket.sock" b[0].LocalBindSocketMode = "0666" diff --git a/website/content/docs/job-specification/upstreams.mdx b/website/content/docs/job-specification/upstreams.mdx index ef39911d354..102cefb6d9b 100644 --- a/website/content/docs/job-specification/upstreams.mdx +++ b/website/content/docs/job-specification/upstreams.mdx @@ -86,6 +86,7 @@ job "countdash" { for details. Keys and values support [runtime variable interpolation][interpolation]. - `destination_name` `(string: )` - Name of the upstream service. - `destination_namespace` `(string: )` - Name of the upstream Consul namespace. +- `destination_partition` `(string: "")` - Name of the Cluster admin partition containing the upstream service. - `destination_peer` `(string: "")` - Name of the peer cluster containing the upstream service. - `destination_type` - `(string: "service")` - The type of discovery query the proxy should use for finding service mesh instances. - `local_bind_port` - `(int: )` - The port the proxy will receive