From a1c4f16cf12fa284e37b8b57dd0a660c6e78f8db Mon Sep 17 00:00:00 2001 From: Seth Hoenig Date: Thu, 14 Apr 2022 11:30:21 -0500 Subject: [PATCH] connect: prefix tag with nomad.; merge into envoy_stats_tags; update docs This PR expands on the work done in #12543 to - prefix the tag, so it is now "nomad.alloc_id" to be more consistent with Consul tags - merge into pre-existing envoy_stats_tags fields - update the upgrade guide docs - update changelog --- .changelog/12543.txt | 3 + command/agent/consul/connect.go | 50 ++++-- command/agent/consul/connect_test.go | 165 +++++++++++++----- command/agent/consul/group_test.go | 5 +- command/agent/consul/service_client.go | 4 +- command/agent/consul/unit_test.go | 1 - .../content/docs/upgrade/upgrade-specific.mdx | 19 ++ 7 files changed, 183 insertions(+), 64 deletions(-) create mode 100644 .changelog/12543.txt diff --git a/.changelog/12543.txt b/.changelog/12543.txt new file mode 100644 index 000000000..03c5eccfb --- /dev/null +++ b/.changelog/12543.txt @@ -0,0 +1,3 @@ +```release-note:improvement +connect: automatically set alloc_id in envoy_stats_tags configuration +``` diff --git a/command/agent/consul/connect.go b/command/agent/consul/connect.go index 35e28e503..e70b728d4 100644 --- a/command/agent/consul/connect.go +++ b/command/agent/consul/connect.go @@ -4,6 +4,7 @@ import ( "fmt" "net" "strconv" + "strings" "github.com/hashicorp/consul/api" "github.com/hashicorp/nomad/helper" @@ -13,7 +14,7 @@ import ( // newConnect creates a new Consul AgentServiceConnect struct based on a Nomad // Connect struct. If the nomad Connect struct is nil, nil will be returned to // disable Connect for this service. -func newConnect(serviceId string, serviceName string, nc *structs.ConsulConnect, networks structs.Networks, ports structs.AllocatedPorts, allocId string) (*api.AgentServiceConnect, error) { +func newConnect(serviceID, allocID string, serviceName string, nc *structs.ConsulConnect, networks structs.Networks, ports structs.AllocatedPorts) (*api.AgentServiceConnect, error) { switch { case nc == nil: // no connect stanza means there is no connect service to register @@ -32,7 +33,7 @@ func newConnect(serviceId string, serviceName string, nc *structs.ConsulConnect, if nc.SidecarService.Port == "" { nc.SidecarService.Port = fmt.Sprintf("%s-%s", structs.ConnectProxyPrefix, serviceName) } - sidecarReg, err := connectSidecarRegistration(serviceId, nc.SidecarService, networks, ports, allocId) + sidecarReg, err := connectSidecarRegistration(serviceID, allocID, nc.SidecarService, networks, ports) if err != nil { return nil, err } @@ -47,7 +48,7 @@ func newConnect(serviceId string, serviceName string, nc *structs.ConsulConnect, // newConnectGateway creates a new Consul AgentServiceConnectProxyConfig struct based on // a Nomad Connect struct. If the Nomad Connect struct does not contain a gateway, nil // will be returned as this service is not a gateway. -func newConnectGateway(serviceName string, connect *structs.ConsulConnect) *api.AgentServiceConnectProxyConfig { +func newConnectGateway(connect *structs.ConsulConnect) *api.AgentServiceConnectProxyConfig { if !connect.IsGateway() { return nil } @@ -89,7 +90,7 @@ func newConnectGateway(serviceName string, connect *structs.ConsulConnect) *api. return &api.AgentServiceConnectProxyConfig{Config: envoyConfig} } -func connectSidecarRegistration(serviceId string, css *structs.ConsulSidecarService, networks structs.Networks, ports structs.AllocatedPorts, allocId string) (*api.AgentServiceRegistration, error) { +func connectSidecarRegistration(serviceID, allocID string, css *structs.ConsulSidecarService, networks structs.Networks, ports structs.AllocatedPorts) (*api.AgentServiceRegistration, error) { if css == nil { // no sidecar stanza means there is no sidecar service to register return nil, nil @@ -100,7 +101,7 @@ func connectSidecarRegistration(serviceId string, css *structs.ConsulSidecarServ return nil, err } - proxy, err := connectSidecarProxy(css.Proxy, cMapping.To, networks, allocId) + proxy, err := connectSidecarProxy(allocID, css.Proxy, cMapping.To, networks) if err != nil { return nil, err } @@ -109,8 +110,8 @@ func connectSidecarRegistration(serviceId string, css *structs.ConsulSidecarServ // ensure service discovery excludes this sidecar from queries // (ex. in the case of Connect upstreams) checks := api.AgentServiceChecks{{ - Name: "Connect Sidecar Aliasing " + serviceId, - AliasService: serviceId, + Name: "Connect Sidecar Aliasing " + serviceID, + AliasService: serviceID, }} if !css.DisableDefaultTCPCheck { checks = append(checks, &api.AgentServiceCheck{ @@ -129,7 +130,7 @@ func connectSidecarRegistration(serviceId string, css *structs.ConsulSidecarServ }, nil } -func connectSidecarProxy(proxy *structs.ConsulProxy, cPort int, networks structs.Networks, allocId string) (*api.AgentServiceConnectProxyConfig, error) { +func connectSidecarProxy(allocID string, proxy *structs.ConsulProxy, cPort int, networks structs.Networks) (*api.AgentServiceConnectProxyConfig, error) { if proxy == nil { proxy = new(structs.ConsulProxy) } @@ -142,7 +143,7 @@ func connectSidecarProxy(proxy *structs.ConsulProxy, cPort int, networks structs return &api.AgentServiceConnectProxyConfig{ LocalServiceAddress: proxy.LocalServiceAddress, LocalServicePort: proxy.LocalServicePort, - Config: connectProxyConfig(proxy.Config, cPort, allocId), + Config: connectProxyConfig(proxy.Config, cPort, allocID), Upstreams: connectUpstreams(proxy.Upstreams), Expose: expose, }, nil @@ -228,17 +229,40 @@ func connectMeshGateway(in *structs.ConsulMeshGateway) api.MeshGatewayConfig { return gw } -func connectProxyConfig(cfg map[string]interface{}, port int, allocId string) map[string]interface{} { +func connectProxyConfig(cfg map[string]interface{}, port int, allocID string) map[string]interface{} { if cfg == nil { cfg = make(map[string]interface{}) } cfg["bind_address"] = "0.0.0.0" cfg["bind_port"] = port - if allocId != "" { - cfg["envoy_stats_tags"] = []string{"alloc_id=" + allocId} + injectAllocID(cfg, allocID) + return cfg +} + +// injectAllocID merges allocID into cfg=>envoy_stats_tags +// +// cfg must not be nil +func injectAllocID(cfg map[string]interface{}, allocID string) { + const key = "envoy_stats_tags" + const prefix = "nomad.alloc_id=" + pair := prefix + allocID + tags, exists := cfg[key] + if !exists { + cfg[key] = []string{pair} + return } - return cfg + switch v := tags.(type) { + case []string: + // scan the existing tags to see if alloc_id= is already set + for _, s := range v { + if strings.HasPrefix(s, prefix) { + return + } + } + v = append(v, pair) + cfg[key] = v + } } func connectNetworkInvariants(networks structs.Networks) error { diff --git a/command/agent/consul/connect_test.go b/command/agent/consul/connect_test.go index cacf13bb3..89bebd230 100644 --- a/command/agent/consul/connect_test.go +++ b/command/agent/consul/connect_test.go @@ -7,6 +7,7 @@ import ( "github.com/hashicorp/consul/api" "github.com/hashicorp/nomad/ci" "github.com/hashicorp/nomad/helper" + "github.com/hashicorp/nomad/helper/uuid" "github.com/hashicorp/nomad/nomad/structs" "github.com/stretchr/testify/require" ) @@ -33,29 +34,33 @@ var ( func TestConnect_newConnect(t *testing.T) { ci.Parallel(t) + service := "redis" + redisID := uuid.Generate() + allocID := uuid.Generate() + t.Run("nil", func(t *testing.T) { - asr, err := newConnect("", "", nil, nil, nil, "") + asr, err := newConnect("", "", "", nil, nil, nil) require.NoError(t, err) require.Nil(t, asr) }) t.Run("native", func(t *testing.T) { - asr, err := newConnect("", "", &structs.ConsulConnect{ + asr, err := newConnect(redisID, allocID, service, &structs.ConsulConnect{ Native: true, - }, nil, nil, "") + }, nil, nil) require.NoError(t, err) require.True(t, asr.Native) require.Nil(t, asr.SidecarService) }) t.Run("with sidecar", func(t *testing.T) { - asr, err := newConnect("redis-service-id", "redis", &structs.ConsulConnect{ + asr, err := newConnect(redisID, allocID, service, &structs.ConsulConnect{ Native: false, SidecarService: &structs.ConsulSidecarService{ Tags: []string{"foo", "bar"}, Port: "connect-proxy-redis", }, - }, testConnectNetwork, testConnectPorts, "redis-alloc-id") + }, testConnectNetwork, testConnectPorts) require.NoError(t, err) require.Equal(t, &api.AgentServiceRegistration{ Tags: []string{"foo", "bar"}, @@ -65,13 +70,13 @@ func TestConnect_newConnect(t *testing.T) { Config: map[string]interface{}{ "bind_address": "0.0.0.0", "bind_port": 3000, - "envoy_stats_tags": []string{"alloc_id=redis-alloc-id"}, + "envoy_stats_tags": []string{"nomad.alloc_id=" + allocID}, }, }, Checks: api.AgentServiceChecks{ { - Name: "Connect Sidecar Aliasing redis-service-id", - AliasService: "redis-service-id", + Name: "Connect Sidecar Aliasing " + redisID, + AliasService: redisID, }, { Name: "Connect Sidecar Listening", @@ -83,14 +88,14 @@ func TestConnect_newConnect(t *testing.T) { }) t.Run("with sidecar without TCP checks", func(t *testing.T) { - asr, err := newConnect("redis-service-id", "redis", &structs.ConsulConnect{ + asr, err := newConnect(redisID, allocID, service, &structs.ConsulConnect{ Native: false, SidecarService: &structs.ConsulSidecarService{ Tags: []string{"foo", "bar"}, Port: "connect-proxy-redis", DisableDefaultTCPCheck: true, }, - }, testConnectNetwork, testConnectPorts, "redis-alloc-id") + }, testConnectNetwork, testConnectPorts) require.NoError(t, err) require.Equal(t, &api.AgentServiceRegistration{ Tags: []string{"foo", "bar"}, @@ -100,13 +105,13 @@ func TestConnect_newConnect(t *testing.T) { Config: map[string]interface{}{ "bind_address": "0.0.0.0", "bind_port": 3000, - "envoy_stats_tags": []string{"alloc_id=redis-alloc-id"}, + "envoy_stats_tags": []string{"nomad.alloc_id=" + allocID}, }, }, Checks: api.AgentServiceChecks{ { - Name: "Connect Sidecar Aliasing redis-service-id", - AliasService: "redis-service-id", + Name: "Connect Sidecar Aliasing " + redisID, + AliasService: redisID, }, }, }, asr.SidecarService) @@ -116,21 +121,24 @@ func TestConnect_newConnect(t *testing.T) { func TestConnect_connectSidecarRegistration(t *testing.T) { ci.Parallel(t) + redisID := uuid.Generate() + allocID := uuid.Generate() + t.Run("nil", func(t *testing.T) { - sidecarReg, err := connectSidecarRegistration("", nil, testConnectNetwork, testConnectPorts, "") + sidecarReg, err := connectSidecarRegistration(redisID, allocID, nil, testConnectNetwork, testConnectPorts) require.NoError(t, err) require.Nil(t, sidecarReg) }) t.Run("no service port", func(t *testing.T) { - _, err := connectSidecarRegistration("unknown-id", &structs.ConsulSidecarService{ + _, err := connectSidecarRegistration("unknown-id", allocID, &structs.ConsulSidecarService{ Port: "unknown-label", - }, testConnectNetwork, testConnectPorts, "") + }, testConnectNetwork, testConnectPorts) require.EqualError(t, err, `No port of label "unknown-label" defined`) }) t.Run("bad proxy", func(t *testing.T) { - _, err := connectSidecarRegistration("redis-service-id", &structs.ConsulSidecarService{ + _, err := connectSidecarRegistration(redisID, allocID, &structs.ConsulSidecarService{ Port: "connect-proxy-redis", Proxy: &structs.ConsulProxy{ Expose: &structs.ConsulExposeConfig{ @@ -139,15 +147,15 @@ func TestConnect_connectSidecarRegistration(t *testing.T) { }}, }, }, - }, testConnectNetwork, testConnectPorts, "") + }, testConnectNetwork, testConnectPorts) require.EqualError(t, err, `No port of label "badPort" defined`) }) t.Run("normal", func(t *testing.T) { - proxy, err := connectSidecarRegistration("redis-service-id", &structs.ConsulSidecarService{ + proxy, err := connectSidecarRegistration(redisID, allocID, &structs.ConsulSidecarService{ Tags: []string{"foo", "bar"}, Port: "connect-proxy-redis", - }, testConnectNetwork, testConnectPorts, "") + }, testConnectNetwork, testConnectPorts) require.NoError(t, err) require.Equal(t, &api.AgentServiceRegistration{ Tags: []string{"foo", "bar"}, @@ -155,14 +163,15 @@ func TestConnect_connectSidecarRegistration(t *testing.T) { Address: "192.168.30.1", Proxy: &api.AgentServiceConnectProxyConfig{ Config: map[string]interface{}{ - "bind_address": "0.0.0.0", - "bind_port": 3000, + "bind_address": "0.0.0.0", + "bind_port": 3000, + "envoy_stats_tags": []string{"nomad.alloc_id=" + allocID}, }, }, Checks: api.AgentServiceChecks{ { - Name: "Connect Sidecar Aliasing redis-service-id", - AliasService: "redis-service-id", + Name: "Connect Sidecar Aliasing " + redisID, + AliasService: redisID, }, { Name: "Connect Sidecar Listening", @@ -177,10 +186,12 @@ func TestConnect_connectSidecarRegistration(t *testing.T) { func TestConnect_connectProxy(t *testing.T) { ci.Parallel(t) + allocID := uuid.Generate() + // If the input proxy is nil, we expect the output to be a proxy with its // config set to default values. t.Run("nil proxy", func(t *testing.T) { - proxy, err := connectSidecarProxy(nil, 2000, testConnectNetwork, "") + proxy, err := connectSidecarProxy(allocID, nil, 2000, testConnectNetwork) require.NoError(t, err) require.Equal(t, &api.AgentServiceConnectProxyConfig{ LocalServiceAddress: "", @@ -188,14 +199,15 @@ func TestConnect_connectProxy(t *testing.T) { Upstreams: nil, Expose: api.ExposeConfig{}, Config: map[string]interface{}{ - "bind_address": "0.0.0.0", - "bind_port": 2000, + "bind_address": "0.0.0.0", + "bind_port": 2000, + "envoy_stats_tags": []string{"nomad.alloc_id=" + allocID}, }, }, proxy) }) t.Run("bad proxy", func(t *testing.T) { - _, err := connectSidecarProxy(&structs.ConsulProxy{ + _, err := connectSidecarProxy(allocID, &structs.ConsulProxy{ LocalServiceAddress: "0.0.0.0", LocalServicePort: 2000, Upstreams: nil, @@ -205,12 +217,12 @@ func TestConnect_connectProxy(t *testing.T) { }}, }, Config: nil, - }, 2000, testConnectNetwork, "") + }, 2000, testConnectNetwork) require.EqualError(t, err, `No port of label "badPort" defined`) }) t.Run("normal", func(t *testing.T) { - proxy, err := connectSidecarProxy(&structs.ConsulProxy{ + proxy, err := connectSidecarProxy(allocID, &structs.ConsulProxy{ LocalServiceAddress: "0.0.0.0", LocalServicePort: 2000, Upstreams: nil, @@ -223,7 +235,7 @@ func TestConnect_connectProxy(t *testing.T) { }}, }, Config: nil, - }, 2000, testConnectNetwork, "") + }, 2000, testConnectNetwork) require.NoError(t, err) require.Equal(t, &api.AgentServiceConnectProxyConfig{ LocalServiceAddress: "0.0.0.0", @@ -238,8 +250,9 @@ func TestConnect_connectProxy(t *testing.T) { }}, }, Config: map[string]interface{}{ - "bind_address": "0.0.0.0", - "bind_port": 2000, + "bind_address": "0.0.0.0", + "bind_port": 2000, + "envoy_stats_tags": []string{"nomad.alloc_id=" + allocID}, }, }, proxy) }) @@ -372,7 +385,7 @@ func TestConnect_connectProxyConfig(t *testing.T) { require.Equal(t, map[string]interface{}{ "bind_address": "0.0.0.0", "bind_port": 42, - "envoy_stats_tags": []string{"alloc_id=test_alloc1"}, + "envoy_stats_tags": []string{"nomad.alloc_id=test_alloc1"}, }, connectProxyConfig(nil, 42, "test_alloc1")) }) @@ -381,18 +394,11 @@ func TestConnect_connectProxyConfig(t *testing.T) { "bind_address": "0.0.0.0", "bind_port": 42, "foo": "bar", - "envoy_stats_tags": []string{"alloc_id=test_alloc2"}, + "envoy_stats_tags": []string{"nomad.alloc_id=test_alloc2"}, }, connectProxyConfig(map[string]interface{}{ "foo": "bar", }, 42, "test_alloc2")) }) - - t.Run("emtpy alloc id", func(t *testing.T) { - require.Equal(t, map[string]interface{}{ - "bind_address": "0.0.0.0", - "bind_port": 42, - }, connectProxyConfig(nil, 42, "")) - }) } func TestConnect_getConnectPort(t *testing.T) { @@ -485,12 +491,12 @@ func TestConnect_newConnectGateway(t *testing.T) { ci.Parallel(t) t.Run("not a gateway", func(t *testing.T) { - result := newConnectGateway("s1", &structs.ConsulConnect{Native: true}) + result := newConnectGateway(&structs.ConsulConnect{Native: true}) require.Nil(t, result) }) t.Run("canonical empty", func(t *testing.T) { - result := newConnectGateway("s1", &structs.ConsulConnect{ + result := newConnectGateway(&structs.ConsulConnect{ Gateway: &structs.ConsulGateway{ Proxy: &structs.ConsulGatewayProxy{ ConnectTimeout: helper.TimeToPtr(1 * time.Second), @@ -509,7 +515,7 @@ func TestConnect_newConnectGateway(t *testing.T) { }) t.Run("proxy undefined", func(t *testing.T) { - result := newConnectGateway("s1", &structs.ConsulConnect{ + result := newConnectGateway(&structs.ConsulConnect{ Gateway: &structs.ConsulGateway{ Proxy: nil, }, @@ -520,7 +526,7 @@ func TestConnect_newConnectGateway(t *testing.T) { }) t.Run("full", func(t *testing.T) { - result := newConnectGateway("s1", &structs.ConsulConnect{ + result := newConnectGateway(&structs.ConsulConnect{ Gateway: &structs.ConsulGateway{ Proxy: &structs.ConsulGatewayProxy{ ConnectTimeout: helper.TimeToPtr(1 * time.Second), @@ -585,3 +591,70 @@ func Test_connectMeshGateway(t *testing.T) { require.Equal(t, api.MeshGatewayConfig{Mode: api.MeshGatewayModeDefault}, result) }) } + +func Test_injectAllocID(t *testing.T) { + ci.Parallel(t) + + id := "abc123" + + try := func(allocID string, cfg, exp map[string]interface{}) { + injectAllocID(cfg, allocID) + require.Equal(t, exp, cfg) + } + + // empty + try( + id, + make(map[string]interface{}), + map[string]interface{}{ + "envoy_stats_tags": []string{"nomad.alloc_id=abc123"}, + }, + ) + + // merge fresh + try( + id, + map[string]interface{}{"foo": "bar"}, + map[string]interface{}{ + "foo": "bar", + "envoy_stats_tags": []string{"nomad.alloc_id=abc123"}, + }, + ) + + // merge append + try( + id, + map[string]interface{}{ + "foo": "bar", + "envoy_stats_tags": []string{"k1=v1", "k2=v2"}, + }, + map[string]interface{}{ + "foo": "bar", + "envoy_stats_tags": []string{"k1=v1", "k2=v2", "nomad.alloc_id=abc123"}, + }, + ) + + // merge exists + try( + id, + map[string]interface{}{ + "foo": "bar", + "envoy_stats_tags": []string{"k1=v1", "k2=v2", "nomad.alloc_id=xyz789"}, + }, + map[string]interface{}{ + "foo": "bar", + "envoy_stats_tags": []string{"k1=v1", "k2=v2", "nomad.alloc_id=xyz789"}, + }, + ) + + // merge wrong type + try( + id, + map[string]interface{}{ + "envoy_stats_tags": "not a slice of string", + }, + map[string]interface{}{ + "envoy_stats_tags": "not a slice of string", + }, + ) +} diff --git a/command/agent/consul/group_test.go b/command/agent/consul/group_test.go index 754827ae4..1af9a113d 100644 --- a/command/agent/consul/group_test.go +++ b/command/agent/consul/group_test.go @@ -122,8 +122,9 @@ func TestConsul_Connect(t *testing.T) { require.Equal(t, connectService.Proxy.LocalServiceAddress, "127.0.0.1") require.Equal(t, connectService.Proxy.LocalServicePort, 9000) require.Equal(t, connectService.Proxy.Config, map[string]interface{}{ - "bind_address": "0.0.0.0", - "bind_port": float64(9998), + "bind_address": "0.0.0.0", + "bind_port": float64(9998), + "envoy_stats_tags": []interface{}{"nomad.alloc_id=" + alloc.ID}, }) require.Equal(t, alloc.ID, agentService.Meta["alloc_id"]) diff --git a/command/agent/consul/service_client.go b/command/agent/consul/service_client.go index 133303526..9c7f7cd4f 100644 --- a/command/agent/consul/service_client.go +++ b/command/agent/consul/service_client.go @@ -964,13 +964,13 @@ func (c *ServiceClient) serviceRegs( } // newConnect returns (nil, nil) if there's no Connect-enabled service. - connect, err := newConnect(id, service.Name, service.Connect, workload.Networks, workload.Ports, workload.AllocID) + connect, err := newConnect(id, workload.AllocID, service.Name, service.Connect, workload.Networks, workload.Ports) if err != nil { return nil, fmt.Errorf("invalid Consul Connect configuration for service %q: %v", service.Name, err) } // newConnectGateway returns nil if there's no Connect gateway. - gateway := newConnectGateway(service.Name, service.Connect) + gateway := newConnectGateway(service.Connect) // Determine whether to use meta or canary_meta var meta map[string]string diff --git a/command/agent/consul/unit_test.go b/command/agent/consul/unit_test.go index b1cbe81fe..f7328bfee 100644 --- a/command/agent/consul/unit_test.go +++ b/command/agent/consul/unit_test.go @@ -749,7 +749,6 @@ func TestConsul_ShutdownBlocked(t *testing.T) { ci.Parallel(t) require := require.New(t) - ci.Parallel(t) ctx := setupFake(t) // can be short because we're intentionally blocking, but needs to // be longer than the time we'll block Consul so we can be sure diff --git a/website/content/docs/upgrade/upgrade-specific.mdx b/website/content/docs/upgrade/upgrade-specific.mdx index 8c08af3db..d9f3ec527 100644 --- a/website/content/docs/upgrade/upgrade-specific.mdx +++ b/website/content/docs/upgrade/upgrade-specific.mdx @@ -107,6 +107,25 @@ However *if you have overridden the default `template.function_denylist` in your client configuration, you must add `writeToFile` to your denylist.* Failing to do so allows templates to write to arbitrary paths on the host. +#### Changes to Envoy metrics labels + +When using Envoy as a sidecar proxy for Connect enabled services, Nomad will now +automatically inject the unique allocation ID into Envoy's stats tags configuration. +Users who wish to set the tag values themselves may do so using the [`proxy.config`](/docs/job-specification/proxy#config) +block. + +```hcl +connect { + sidecar_service { + proxy { + config { + envoy_stats_tags = ["nomad.alloc_id="] + } + } + } +} +``` + #### Linux Control Groups Version 2 Starting with Nomad 1.3.0, Linux systems configured to use [cgroups v2][cgroups2]