From 8e5516dbd6637a892e52e2b4643218aadf0b61e4 Mon Sep 17 00:00:00 2001 From: Christian Ang Date: Fri, 16 Feb 2024 00:29:18 +0000 Subject: [PATCH 1/2] Add test validating cluster name with upstream tls - add a test to ensure that two clusters with different tls settings due to one being created with HTTPRoute and BackendTLSPolicy vs HTTPProxy have different cluster names Signed-off-by: Christian Ang --- .../featuretests/v3/backendclientauth_test.go | 3 + internal/featuretests/v3/upstreamtls_test.go | 153 ++++++++++++++++++ 2 files changed, 156 insertions(+) diff --git a/internal/featuretests/v3/backendclientauth_test.go b/internal/featuretests/v3/backendclientauth_test.go index 0ee16ea7173..cac489eb8cc 100644 --- a/internal/featuretests/v3/backendclientauth_test.go +++ b/internal/featuretests/v3/backendclientauth_test.go @@ -55,6 +55,9 @@ func proxyClientCertificateOpt(t *testing.T) func(*dag.Builder) { ClientCertificate: &secret, FieldLogger: log.WithField("context", "ExtensionServiceProcessor"), }, + &dag.GatewayAPIProcessor{ + FieldLogger: log.WithField("context", "GatewayAPIProcessor"), + }, } b.Source.ConfiguredSecretRefs = []*types.NamespacedName{ diff --git a/internal/featuretests/v3/upstreamtls_test.go b/internal/featuretests/v3/upstreamtls_test.go index 88c84c2c710..9416430be34 100644 --- a/internal/featuretests/v3/upstreamtls_test.go +++ b/internal/featuretests/v3/upstreamtls_test.go @@ -454,3 +454,156 @@ func TestBackendTLSPolicyPrecedenceOverUpstreamProtocolAnnotationWithHTTPRoute(t TypeUrl: clusterType, }) } + +func TestUpstreamTLSWithHTTPRouteANDHTTPProxy(t *testing.T) { + rh, c, done := setup(t, func(b *dag.Builder) { + for _, processor := range b.Processors { + if httpProxyProcessor, ok := processor.(*dag.HTTPProxyProcessor); ok { + httpProxyProcessor.UpstreamTLS = &dag.UpstreamTLS{ + MinimumProtocolVersion: "1.2", + MaximumProtocolVersion: "1.2", + } + } + if gatewayAPIProcessor, ok := processor.(*dag.GatewayAPIProcessor); ok { + gatewayAPIProcessor.UpstreamTLS = &dag.UpstreamTLS{ + MinimumProtocolVersion: "1.2", + MaximumProtocolVersion: "1.2", + } + } + } + }) + defer done() + + caSecret := featuretests.CASecret(t, "backendcacert", &featuretests.CACertificate) + rh.OnAdd(caSecret) + + sec1 := featuretests.TLSSecret(t, "sec1", &featuretests.ClientCertificate) + sec2 := featuretests.CASecret(t, "sec2", &featuretests.CACertificate) + rh.OnAdd(sec1) + rh.OnAdd(sec2) + + svc := fixture.NewService("backend"). + WithPorts(core_v1.ServicePort{Name: "http", Port: 443}) + rh.OnAdd(svc) + + proxy := fixture.NewProxy("authenticated").WithSpec( + contour_v1.HTTPProxySpec{ + VirtualHost: &contour_v1.VirtualHost{ + Fqdn: "www.example.com", + }, + Routes: []contour_v1.Route{{ + Services: []contour_v1.Service{{ + Name: svc.Name, + Port: 443, + Protocol: ptr.To("tls"), + UpstreamValidation: &contour_v1.UpstreamValidation{ + CACertificate: caSecret.Name, + SubjectName: "subjname", + }, + }}, + }}, + }) + rh.OnAdd(proxy) + + rh.OnAdd(&gatewayapi_v1.GatewayClass{ + TypeMeta: meta_v1.TypeMeta{}, + ObjectMeta: fixture.ObjectMeta("test-gc"), + Spec: gatewayapi_v1.GatewayClassSpec{ + ControllerName: "projectcontour.io/contour", + }, + Status: gatewayapi_v1.GatewayClassStatus{ + Conditions: []meta_v1.Condition{ + { + Type: string(gatewayapi_v1.GatewayClassConditionStatusAccepted), + Status: meta_v1.ConditionTrue, + }, + }, + }, + }) + + gateway := &gatewayapi_v1.Gateway{ + ObjectMeta: fixture.ObjectMeta("projectcontour/contour"), + Spec: gatewayapi_v1.GatewaySpec{ + Listeners: []gatewayapi_v1.Listener{{ + Name: "http", + Port: 80, + Protocol: gatewayapi_v1.HTTPProtocolType, + AllowedRoutes: &gatewayapi_v1.AllowedRoutes{ + Namespaces: &gatewayapi_v1.RouteNamespaces{ + From: ptr.To(gatewayapi_v1.NamespacesFromAll), + }, + }, + }}, + }, + } + rh.OnAdd(gateway) + + rh.OnAdd(&gatewayapi_v1.HTTPRoute{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "authenticated", + Namespace: "default", + }, + Spec: gatewayapi_v1.HTTPRouteSpec{ + CommonRouteSpec: gatewayapi_v1.CommonRouteSpec{ + ParentRefs: []gatewayapi_v1.ParentReference{ + gatewayapi.GatewayParentRef("projectcontour", "contour"), + }, + }, + Hostnames: []gatewayapi_v1.Hostname{ + "test.projectcontour.io", + }, + Rules: []gatewayapi_v1.HTTPRouteRule{{ + Matches: gatewayapi.HTTPRouteMatch(gatewayapi_v1.PathMatchPathPrefix, "/"), + BackendRefs: gatewayapi.HTTPBackendRef("backend", 443, 1), + }}, + }, + }) + + rh.OnAdd(&gatewayapi_v1alpha2.BackendTLSPolicy{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "authenticated", + Namespace: "default", + }, + Spec: gatewayapi_v1alpha2.BackendTLSPolicySpec{ + TargetRef: gatewayapi_v1alpha2.PolicyTargetReferenceWithSectionName{ + PolicyTargetReference: gatewayapi_v1alpha2.PolicyTargetReference{ + Kind: "Service", + Name: "backend", + }, + }, + TLS: gatewayapi_v1alpha2.BackendTLSPolicyConfig{ + CACertRefs: []gatewayapi_v1alpha2.LocalObjectReference{{ + Kind: "Secret", + Name: gatewayapi_v1.ObjectName(sec2.Name), + }}, + Hostname: "subjname", + }, + }, + }) + + c.Request(clusterType).Equals(&envoy_service_discovery_v3.DiscoveryResponse{ + Resources: resources(t, + tlsCluster( + cluster("default/backend/443/867941ed65", "default/backend/http", "default_backend_443"), + sec2, + "subjname", + "", + nil, + &dag.UpstreamTLS{ + MinimumProtocolVersion: "1.2", + MaximumProtocolVersion: "1.2", + }), + tlsCluster( + cluster("default/backend/443/950c17581f", "default/backend/http", "default_backend_443"), + caSecret, + "subjname", + "", + nil, + &dag.UpstreamTLS{ + MinimumProtocolVersion: "1.2", + MaximumProtocolVersion: "1.2", + }), + ), + TypeUrl: clusterType, + }) +} From e506bcbe18742b76476217be9e8a17cfaf529f7f Mon Sep 17 00:00:00 2001 From: Christian Ang Date: Wed, 21 Feb 2024 00:12:35 +0000 Subject: [PATCH 2/2] Add type to statusupdatercache - to differentiate resources with the same name Signed-off-by: Christian Ang --- internal/featuretests/v3/upstreamtls_test.go | 12 ++++++------ internal/k8s/statusaddress_test.go | 9 +++++---- internal/k8s/statuscache.go | 18 +++++++++--------- 3 files changed, 20 insertions(+), 19 deletions(-) diff --git a/internal/featuretests/v3/upstreamtls_test.go b/internal/featuretests/v3/upstreamtls_test.go index 9416430be34..76ca2408ce6 100644 --- a/internal/featuretests/v3/upstreamtls_test.go +++ b/internal/featuretests/v3/upstreamtls_test.go @@ -455,6 +455,8 @@ func TestBackendTLSPolicyPrecedenceOverUpstreamProtocolAnnotationWithHTTPRoute(t }) } +// Test that a unique cluster name is generated when there is an HTTPProxy with upstream TLS settings +// and an HTTPRoute with a BackendTLSPolicy, configured with unique TLS settings, targeting the same service. func TestUpstreamTLSWithHTTPRouteANDHTTPProxy(t *testing.T) { rh, c, done := setup(t, func(b *dag.Builder) { for _, processor := range b.Processors { @@ -477,10 +479,8 @@ func TestUpstreamTLSWithHTTPRouteANDHTTPProxy(t *testing.T) { caSecret := featuretests.CASecret(t, "backendcacert", &featuretests.CACertificate) rh.OnAdd(caSecret) - sec1 := featuretests.TLSSecret(t, "sec1", &featuretests.ClientCertificate) - sec2 := featuretests.CASecret(t, "sec2", &featuretests.CACertificate) + sec1 := featuretests.CASecret(t, "sec1", &featuretests.CACertificate) rh.OnAdd(sec1) - rh.OnAdd(sec2) svc := fixture.NewService("backend"). WithPorts(core_v1.ServicePort{Name: "http", Port: 443}) @@ -574,7 +574,7 @@ func TestUpstreamTLSWithHTTPRouteANDHTTPProxy(t *testing.T) { TLS: gatewayapi_v1alpha2.BackendTLSPolicyConfig{ CACertRefs: []gatewayapi_v1alpha2.LocalObjectReference{{ Kind: "Secret", - Name: gatewayapi_v1.ObjectName(sec2.Name), + Name: gatewayapi_v1.ObjectName(sec1.Name), }}, Hostname: "subjname", }, @@ -584,8 +584,8 @@ func TestUpstreamTLSWithHTTPRouteANDHTTPProxy(t *testing.T) { c.Request(clusterType).Equals(&envoy_service_discovery_v3.DiscoveryResponse{ Resources: resources(t, tlsCluster( - cluster("default/backend/443/867941ed65", "default/backend/http", "default_backend_443"), - sec2, + cluster("default/backend/443/242c9163af", "default/backend/http", "default_backend_443"), + sec1, "subjname", "", nil, diff --git a/internal/k8s/statusaddress_test.go b/internal/k8s/statusaddress_test.go index db2553407cb..e8bbf88cf18 100644 --- a/internal/k8s/statusaddress_test.go +++ b/internal/k8s/statusaddress_test.go @@ -14,6 +14,7 @@ package k8s import ( + "fmt" "testing" "github.com/sirupsen/logrus" @@ -325,7 +326,7 @@ func TestStatusAddressUpdater(t *testing.T) { isu.OnAdd(tc.preop, false) - newObj := suc.Get(objName, objName) + newObj := suc.Get(fmt.Sprintf("%T", tc.preop), objName, objName) assert.Equal(t, tc.postop, newObj) }) @@ -344,7 +345,7 @@ func TestStatusAddressUpdater(t *testing.T) { isu.OnUpdate(tc.preop, tc.preop) - newObj := suc.Get(objName, objName) + newObj := suc.Get(fmt.Sprintf("%T", tc.preop), objName, objName) assert.Equal(t, tc.postop, newObj) }) } @@ -531,7 +532,7 @@ func TestStatusAddressUpdater_Gateway(t *testing.T) { isu.OnAdd(tc.preop, false) - newObj := suc.Get(tc.preop.Name, tc.preop.Namespace) + newObj := suc.Get(fmt.Sprintf("%T", tc.preop), tc.preop.Name, tc.preop.Namespace) assert.Equal(t, tc.postop, newObj) }) @@ -554,7 +555,7 @@ func TestStatusAddressUpdater_Gateway(t *testing.T) { isu.OnUpdate(tc.preop, tc.preop) - newObj := suc.Get(tc.preop.Name, tc.preop.Namespace) + newObj := suc.Get(fmt.Sprintf("%T", tc.preop), tc.preop.Name, tc.preop.Namespace) assert.Equal(t, tc.postop, newObj) }) } diff --git a/internal/k8s/statuscache.go b/internal/k8s/statuscache.go index d51a7158d47..7be3a9d0ad5 100644 --- a/internal/k8s/statuscache.go +++ b/internal/k8s/statuscache.go @@ -42,7 +42,7 @@ func (suc *StatusUpdateCacher) OnDelete(obj any) { if suc.objectCache != nil { switch o := obj.(type) { case *contour_v1.HTTPProxy: - delete(suc.objectCache, suc.objKey(o.Name, o.Namespace)) + delete(suc.objectCache, suc.objKey(fmt.Sprintf("%T", o), o.Name, o.Namespace)) default: panic(fmt.Sprintf("status caching not supported for object type %T", obj)) } @@ -57,19 +57,19 @@ func (suc *StatusUpdateCacher) OnAdd(obj any) { switch o := obj.(type) { case *contour_v1.HTTPProxy: - suc.objectCache[suc.objKey(o.Name, o.Namespace)] = o + suc.objectCache[suc.objKey(fmt.Sprintf("%T", o), o.Name, o.Namespace)] = o default: panic(fmt.Sprintf("status caching not supported for object type %T", obj)) } } // Get allows retrieval of objects from the cache. -func (suc *StatusUpdateCacher) Get(name, namespace string) any { +func (suc *StatusUpdateCacher) Get(objType, name, namespace string) any { if suc.objectCache == nil { suc.objectCache = make(map[string]client.Object) } - obj, ok := suc.objectCache[suc.objKey(name, namespace)] + obj, ok := suc.objectCache[suc.objKey(objType, name, namespace)] if ok { return obj } @@ -81,7 +81,7 @@ func (suc *StatusUpdateCacher) Add(name, namespace string, obj client.Object) bo suc.objectCache = make(map[string]client.Object) } - prefix := suc.objKey(name, namespace) + prefix := suc.objKey(fmt.Sprintf("%T", obj), name, namespace) _, ok := suc.objectCache[prefix] if ok { return false @@ -95,7 +95,7 @@ func (suc *StatusUpdateCacher) Add(name, namespace string, obj client.Object) bo func (suc *StatusUpdateCacher) GetStatus(obj any) (*contour_v1.HTTPProxyStatus, error) { switch o := obj.(type) { case *contour_v1.HTTPProxy: - objectKey := suc.objKey(o.Name, o.Namespace) + objectKey := suc.objKey(fmt.Sprintf("%T", o), o.Name, o.Namespace) cachedObj, ok := suc.objectCache[objectKey] if ok { if c, ok := cachedObj.(*contour_v1.HTTPProxy); ok { @@ -108,8 +108,8 @@ func (suc *StatusUpdateCacher) GetStatus(obj any) (*contour_v1.HTTPProxyStatus, } } -func (suc *StatusUpdateCacher) objKey(name, namespace string) string { - return fmt.Sprintf("%s/%s", namespace, name) +func (suc *StatusUpdateCacher) objKey(objType, name, namespace string) string { + return fmt.Sprintf("%s/%s/%s", objType, namespace, name) } func (suc *StatusUpdateCacher) Send(su StatusUpdate) { @@ -117,7 +117,7 @@ func (suc *StatusUpdateCacher) Send(su StatusUpdate) { suc.objectCache = make(map[string]client.Object) } - objKey := suc.objKey(su.NamespacedName.Name, su.NamespacedName.Namespace) + objKey := suc.objKey(fmt.Sprintf("%T", su.Resource), su.NamespacedName.Name, su.NamespacedName.Namespace) obj, ok := suc.objectCache[objKey] if ok { suc.objectCache[objKey] = su.Mutator.Mutate(obj)