From 6341183a84fe5b87f5402bb07f0053ae7470433b Mon Sep 17 00:00:00 2001 From: "Chris S. Kim" Date: Mon, 26 Jul 2021 17:12:29 -0400 Subject: [PATCH] agent: update proxy upstreams to inherit namespace from service (#10688) --- .changelog/10688.txt | 3 ++ agent/agent_endpoint_test.go | 40 ++++++++++++++----- agent/config/runtime_test.go | 7 ++-- agent/sidecar_service.go | 8 ++-- agent/sidecar_service_test.go | 5 ++- agent/structs/service_definition.go | 8 +++- agent/structs/testing_connect_proxy_config.go | 7 +++- 7 files changed, 56 insertions(+), 22 deletions(-) create mode 100644 .changelog/10688.txt diff --git a/.changelog/10688.txt b/.changelog/10688.txt new file mode 100644 index 000000000..1e1b1b0a3 --- /dev/null +++ b/.changelog/10688.txt @@ -0,0 +1,3 @@ +```release-note:bug +connect: proxy upstreams inherit namespace from service if none are defined. +``` \ No newline at end of file diff --git a/agent/agent_endpoint_test.go b/agent/agent_endpoint_test.go index 77dbb9489..c17bec9da 100644 --- a/agent/agent_endpoint_test.go +++ b/agent/agent_endpoint_test.go @@ -20,6 +20,7 @@ import ( "github.com/hashicorp/go-uuid" "github.com/hashicorp/serf/serf" + "github.com/mitchellh/hashstructure" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/time/rate" @@ -391,15 +392,14 @@ func TestAgent_Service(t *testing.T) { // API struct types. expectProxy := proxy expectProxy.Upstreams = - structs.TestAddDefaultsToUpstreams(t, sidecarProxy.Proxy.Upstreams) + structs.TestAddDefaultsToUpstreams(t, sidecarProxy.Proxy.Upstreams, *structs.DefaultEnterpriseMetaInDefaultPartition()) expectedResponse := &api.AgentService{ - Kind: api.ServiceKindConnectProxy, - ID: "web-sidecar-proxy", - Service: "web-sidecar-proxy", - Port: 8000, - Proxy: expectProxy.ToAPI(), - ContentHash: "854327a458fe02a6", + Kind: api.ServiceKindConnectProxy, + ID: "web-sidecar-proxy", + Service: "web-sidecar-proxy", + Port: 8000, + Proxy: expectProxy.ToAPI(), Weights: api.AgentWeights{ Passing: 1, Warning: 1, @@ -409,11 +409,21 @@ func TestAgent_Service(t *testing.T) { Datacenter: "dc1", } fillAgentServiceEnterpriseMeta(expectedResponse, structs.DefaultEnterpriseMetaInDefaultPartition()) + hash1, err := hashstructure.Hash(expectedResponse, nil) + if err != nil { + t.Fatalf("error generating hash: %v", err) + } + expectedResponse.ContentHash = fmt.Sprintf("%x", hash1) // Copy and modify updatedResponse := *expectedResponse updatedResponse.Port = 9999 - updatedResponse.ContentHash = "b80a4d9370ed1104" + updatedResponse.ContentHash = "" // clear field before generating a new hash + hash2, err := hashstructure.Hash(updatedResponse, nil) + if err != nil { + t.Fatalf("error generating hash: %v", err) + } + updatedResponse.ContentHash = fmt.Sprintf("%x", hash2) // Simple response for non-proxy service registered in TestAgent config expectWebResponse := &api.AgentService{ @@ -3537,8 +3547,18 @@ func testAgent_RegisterService_UnmanagedConnectProxy(t *testing.T, extraHCL stri svc := a.State.Service(sid) require.NotNil(t, svc, "has service") require.Equal(t, structs.ServiceKindConnectProxy, svc.Kind) - // Registration must set that default type - args.Proxy.Upstreams[0].DestinationType = api.UpstreamDestTypeService + + // Registration sets default types and namespaces + for i := range args.Proxy.Upstreams { + if args.Proxy.Upstreams[i].DestinationType == "" { + args.Proxy.Upstreams[i].DestinationType = api.UpstreamDestTypeService + } + if args.Proxy.Upstreams[i].DestinationNamespace == "" { + args.Proxy.Upstreams[i].DestinationNamespace = + structs.DefaultEnterpriseMetaInDefaultPartition().NamespaceOrEmpty() + } + } + require.Equal(t, args.Proxy, svc.Proxy.ToAPI()) // Ensure the token was configured diff --git a/agent/config/runtime_test.go b/agent/config/runtime_test.go index dad9cf723..c5976adb8 100644 --- a/agent/config/runtime_test.go +++ b/agent/config/runtime_test.go @@ -5647,9 +5647,10 @@ func TestLoad_FullConfig(t *testing.T) { }, Upstreams: structs.Upstreams{ { - DestinationType: "service", // Default should be explicitly filled - DestinationName: "KPtAj2cb", - LocalBindPort: 4051, + DestinationType: "service", // Default should be explicitly filled + DestinationName: "KPtAj2cb", + DestinationNamespace: defaultEntMeta.NamespaceOrEmpty(), + LocalBindPort: 4051, Config: map[string]interface{}{ "kzRnZOyd": "nUNKoL8H", }, diff --git a/agent/sidecar_service.go b/agent/sidecar_service.go index aa9b897c7..da87b6fcc 100644 --- a/agent/sidecar_service.go +++ b/agent/sidecar_service.go @@ -18,7 +18,7 @@ func sidecarServiceID(serviceID string) string { // config. // // It assumes the ns has been validated already which means the nested -// SidecarService is also already validated.It also assumes that any check +// SidecarService is also already validated. It also assumes that any check // definitions within the sidecar service definition have been validated if // necessary. If no sidecar service is defined in ns, then nil is returned with // nil error. @@ -35,6 +35,9 @@ func (a *Agent) sidecarServiceFromNodeService(ns *structs.NodeService, token str return nil, nil, "", nil } + // for now at least these must be identical + ns.Connect.SidecarService.EnterpriseMeta = ns.EnterpriseMeta + // Start with normal conversion from service definition sidecar := ns.Connect.SidecarService.NodeService() @@ -42,9 +45,6 @@ func (a *Agent) sidecarServiceFromNodeService(ns *structs.NodeService, token str // ID. We rely on this for lifecycle management of the nested definition. sidecar.ID = sidecarServiceID(ns.ID) - // for now at least these must be identical - sidecar.EnterpriseMeta = ns.EnterpriseMeta - // Set some meta we can use to disambiguate between service instances we added // later and are responsible for deregistering. if sidecar.Meta != nil { diff --git a/agent/sidecar_service_test.go b/agent/sidecar_service_test.go index 6b17599da..e6d1666f3 100644 --- a/agent/sidecar_service_test.go +++ b/agent/sidecar_service_test.go @@ -129,7 +129,8 @@ func TestAgent_sidecarServiceFromNodeService(t *testing.T) { LocalServiceAddress: "127.0.127.0", LocalServicePort: 9999, Config: map[string]interface{}{"baz": "qux"}, - Upstreams: structs.TestAddDefaultsToUpstreams(t, structs.TestUpstreams(t)), + Upstreams: structs.TestAddDefaultsToUpstreams(t, structs.TestUpstreams(t), + *structs.DefaultEnterpriseMetaInDefaultPartition()), }, }, wantChecks: []*structs.CheckType{ @@ -308,7 +309,7 @@ func TestAgent_sidecarServiceFromNodeService(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - // Set port range to be tiny (one availabl) to test consuming all of it. + // Set port range to be tiny (one available) to test consuming all of it. // This allows a single assigned port at 2222 thanks to being inclusive at // both ends. if tt.maxPort == 0 { diff --git a/agent/structs/service_definition.go b/agent/structs/service_definition.go index 96c19112c..552d5df5b 100644 --- a/agent/structs/service_definition.go +++ b/agent/structs/service_definition.go @@ -80,11 +80,17 @@ func (s *ServiceDefinition) NodeService() *NodeService { } if s.Proxy != nil { ns.Proxy = *s.Proxy - // Ensure the Upstream type is defaulted for i := range ns.Proxy.Upstreams { + // Ensure the Upstream type is defaulted if ns.Proxy.Upstreams[i].DestinationType == "" { ns.Proxy.Upstreams[i].DestinationType = UpstreamDestTypeService } + + // If a proxy's namespace is not defined, inherit the server's namespace. + // Applicable only to Consul Enterprise. + if ns.Proxy.Upstreams[i].DestinationNamespace == "" { + ns.Proxy.Upstreams[i].DestinationNamespace = ns.EnterpriseMeta.NamespaceOrEmpty() + } } ns.Proxy.Expose = s.Proxy.Expose } diff --git a/agent/structs/testing_connect_proxy_config.go b/agent/structs/testing_connect_proxy_config.go index 694c5e663..527ffea58 100644 --- a/agent/structs/testing_connect_proxy_config.go +++ b/agent/structs/testing_connect_proxy_config.go @@ -42,9 +42,9 @@ func TestUpstreams(t testing.T) Upstreams { // TestAddDefaultsToUpstreams takes an array of upstreams (such as that from // TestUpstreams) and adds default values that are populated during -// refigistration. Use this for generating the expected Upstreams value after +// registration. Use this for generating the expected Upstreams value after // registration. -func TestAddDefaultsToUpstreams(t testing.T, upstreams []Upstream) Upstreams { +func TestAddDefaultsToUpstreams(t testing.T, upstreams []Upstream, entMeta EnterpriseMeta) Upstreams { ups := make([]Upstream, len(upstreams)) for i := range upstreams { ups[i] = upstreams[i] @@ -52,6 +52,9 @@ func TestAddDefaultsToUpstreams(t testing.T, upstreams []Upstream) Upstreams { if ups[i].DestinationType == "" { ups[i].DestinationType = UpstreamDestTypeService } + if ups[i].DestinationNamespace == "" { + ups[i].DestinationNamespace = entMeta.NamespaceOrEmpty() + } } return ups }