From 659321d0084a610612cf75c1c6b83732383a4da3 Mon Sep 17 00:00:00 2001 From: Paul Banks Date: Wed, 25 Aug 2021 13:40:47 +0100 Subject: [PATCH] Handle namespaces in route names correctly; add tests for enterprise --- agent/proxycfg/ingress_gateway.go | 4 +- agent/structs/structs_oss.go | 8 +++ agent/xds/listeners.go | 15 ++++-- agent/xds/routes_test.go | 83 +++++++++++++++++++++++-------- 4 files changed, 85 insertions(+), 25 deletions(-) diff --git a/agent/proxycfg/ingress_gateway.go b/agent/proxycfg/ingress_gateway.go index d8b5842864..ae791dfba0 100644 --- a/agent/proxycfg/ingress_gateway.go +++ b/agent/proxycfg/ingress_gateway.go @@ -154,8 +154,8 @@ func (s *handlerIngressGateway) handleUpdate(ctx context.Context, u cache.Update func makeUpstream(g *structs.GatewayService) structs.Upstream { upstream := structs.Upstream{ DestinationName: g.Service.Name, - DestinationNamespace: g.Service.NamespaceOrDefault(), - DestinationPartition: g.Gateway.PartitionOrDefault(), + DestinationNamespace: g.Service.NamespaceOrEmpty(), + DestinationPartition: g.Gateway.PartitionOrEmpty(), LocalBindPort: g.Port, IngressHosts: g.Hosts, // Pass the protocol that was configured on the ingress listener in order diff --git a/agent/structs/structs_oss.go b/agent/structs/structs_oss.go index 13d2872bc4..045617b503 100644 --- a/agent/structs/structs_oss.go +++ b/agent/structs/structs_oss.go @@ -79,6 +79,10 @@ func (m *EnterpriseMeta) NamespaceOrEmpty() string { return "" } +func (m *EnterpriseMeta) InDefaultNamespace() bool { + return true +} + func (m *EnterpriseMeta) PartitionOrDefault() string { return "default" } @@ -91,6 +95,10 @@ func (m *EnterpriseMeta) PartitionOrEmpty() string { return "" } +func (m *EnterpriseMeta) InDefaultPartition() bool { + return true +} + // ReplicationEnterpriseMeta stub func ReplicationEnterpriseMeta() *EnterpriseMeta { return &emptyEnterpriseMeta diff --git a/agent/xds/listeners.go b/agent/xds/listeners.go index 0d64cf08d1..bc8bdc68aa 100644 --- a/agent/xds/listeners.go +++ b/agent/xds/listeners.go @@ -664,7 +664,7 @@ func (s *ResourceGenerator) makeIngressGatewayListeners(address string, cfgSnap func routeNameForUpstream(l structs.IngressListener, s structs.IngressService) (string, error) { key := proxycfg.IngressListenerKeyFromListener(l) - // If the upsteam service doesn't have any TLS overrides then it can just use + // If the upstream service doesn't have any TLS overrides then it can just use // the combined filterchain with all the merged routes. if !ingressServiceHasSDSOverrides(s) { return key.RouteName(), nil @@ -672,8 +672,17 @@ func routeNameForUpstream(l structs.IngressListener, s structs.IngressService) ( // Return a specific route for this service as it needs a custom FilterChain // to serve it's custom cert so we should attach it's routes to a separate - // Route too. - return fmt.Sprintf("%s_%s", key.RouteName(), s.ToServiceName().ToServiceID().String()), nil + // Route too. We need this to be consistent between OSS and Enterprise to + // avoid xDS config golden files in tests conflicting so we can't use + // ServiceID.String() which normalizes to included all identifiers in + // Enterprise. + sn := s.ToServiceName() + svcIdentifier := sn.Name + if !sn.InDefaultPartition() || !sn.InDefaultNamespace() { + // Non-default partition/namespace, use a full identifier + svcIdentifier = sn.String() + } + return fmt.Sprintf("%s_%s", key.RouteName(), svcIdentifier), nil } func ingressServiceHasSDSOverrides(s structs.IngressService) bool { diff --git a/agent/xds/routes_test.go b/agent/xds/routes_test.go index 2dd5eb5a21..0f3c472364 100644 --- a/agent/xds/routes_test.go +++ b/agent/xds/routes_test.go @@ -387,6 +387,17 @@ func TestRoutesFromSnapshot(t *testing.T) { gName += ".v2compat" + // It's easy to miss a new type that encodes a version from just + // looking at the golden files so lets make it an error here. If + // there are ever false positives we can maybe include an allow list + // here as it seems safer to assume something was missed than to + // assume we'll notice the golden file being wrong. Note the first + // one matches both resourceApiVersion and transportApiVersion. I + // left it as a suffix in case there are other field names that + // follow that convention now or in the future. + require.NotContains(t, gotJSON, `ApiVersion": "V3"`) + require.NotContains(t, gotJSON, `type.googleapis.com/envoy.api.v3`) + require.JSONEq(t, goldenEnvoy(t, filepath.Join("routes", gName), envoyVersion, latestEnvoyVersion_v2, gotJSON), gotJSON) }) }) @@ -651,6 +662,7 @@ func TestEnvoyLBConfig_InjectToRouteAction(t *testing.T) { type ingressSDSOpts struct { listenerSDS, webSDS, fooSDS, wildcard bool + entMetas map[string]*structs.EnterpriseMeta } // setupIngressWithTwoHTTPServices can be used with @@ -662,24 +674,34 @@ func setupIngressWithTwoHTTPServices(t *testing.T, o ingressSDSOpts) func(snap * snap.IngressGateway.TLSConfig.SDS = nil + webUpstream := structs.Upstream{ + DestinationName: "web", + // We use empty not default here because of the way upstream identifiers + // vary between OSS and Enterprise currently causing test conflicts. In + // real life `proxycfg` always sets ingress upstream namespaces to + // `NamespaceOrDefault` which shouldn't matter because we should be + // consistent within a single binary it's just inconvenient if OSS and + // enterprise tests generate different output. + DestinationNamespace: o.entMetas["web"].NamespaceOrEmpty(), + DestinationPartition: o.entMetas["web"].PartitionOrEmpty(), + LocalBindPort: 9191, + IngressHosts: []string{ + "www.example.com", + }, + } + fooUpstream := structs.Upstream{ + DestinationName: "foo", + DestinationNamespace: o.entMetas["foo"].NamespaceOrEmpty(), + DestinationPartition: o.entMetas["foo"].PartitionOrEmpty(), + LocalBindPort: 9191, + IngressHosts: []string{ + "foo.example.com", + }, + } + // Setup additional HTTP service on same listener with default router snap.IngressGateway.Upstreams = map[proxycfg.IngressListenerKey]structs.Upstreams{ - {Protocol: "http", Port: 9191}: { - { - DestinationName: "web", - LocalBindPort: 9191, - IngressHosts: []string{ - "www.example.com", - }, - }, - { - DestinationName: "foo", - LocalBindPort: 9191, - IngressHosts: []string{ - "foo.example.com", - }, - }, - }, + {Protocol: "http", Port: 9191}: {webUpstream, fooUpstream}, } il := structs.IngressListener{ Port: 9191, @@ -694,6 +716,11 @@ func setupIngressWithTwoHTTPServices(t *testing.T, o ingressSDSOpts) func(snap * }, }, } + for i, svc := range il.Services { + if em, ok := o.entMetas[svc.Name]; ok && em != nil { + il.Services[i].EnterpriseMeta = *em + } + } // Now set the appropriate SDS configs if o.listenerSDS { @@ -760,9 +787,25 @@ func setupIngressWithTwoHTTPServices(t *testing.T, o ingressSDSOpts) func(snap * ConnectTimeout: 22 * time.Second, }, } - webChain := discoverychain.TestCompileConfigEntries(t, "web", "default", "dc1", connect.TestClusterID+".consul", "dc1", nil, entries...) - fooChain := discoverychain.TestCompileConfigEntries(t, "foo", "default", "dc1", connect.TestClusterID+".consul", "dc1", nil, entries...) - snap.IngressGateway.DiscoveryChain["web"] = webChain - snap.IngressGateway.DiscoveryChain["foo"] = fooChain + for i, e := range entries { + switch v := e.(type) { + // Add other Service types here if we ever need them above + case *structs.ServiceResolverConfigEntry: + if em, ok := o.entMetas[v.Name]; ok && em != nil { + v.EnterpriseMeta = *em + entries[i] = v + } + } + } + + webChain := discoverychain.TestCompileConfigEntries(t, "web", + o.entMetas["web"].NamespaceOrDefault(), "dc1", + connect.TestClusterID+".consul", "dc1", nil, entries...) + fooChain := discoverychain.TestCompileConfigEntries(t, "foo", + o.entMetas["foo"].NamespaceOrDefault(), "dc1", + connect.TestClusterID+".consul", "dc1", nil, entries...) + + snap.IngressGateway.DiscoveryChain[webUpstream.Identifier()] = webChain + snap.IngressGateway.DiscoveryChain[fooUpstream.Identifier()] = fooChain } }