From abcaecb044ffe3882c07cf2b83a74df9ff250f97 Mon Sep 17 00:00:00 2001 From: Guy Daich Date: Fri, 3 Jan 2025 06:00:26 -0600 Subject: [PATCH 1/3] fix: store one copy of HTTPRoute Extension Filters Signed-off-by: Guy Daich --- internal/provider/kubernetes/resource.go | 50 ++-- internal/provider/kubernetes/routes.go | 10 +- internal/provider/kubernetes/routes_test.go | 254 ++++++++++++++++++++ release-notes/current.yaml | 1 + 4 files changed, 290 insertions(+), 25 deletions(-) diff --git a/internal/provider/kubernetes/resource.go b/internal/provider/kubernetes/resource.go index b867d6319d3..13a06688855 100644 --- a/internal/provider/kubernetes/resource.go +++ b/internal/provider/kubernetes/resource.go @@ -64,32 +64,36 @@ type resourceMappings struct { // httpRouteFilters is a map of HTTPRouteFilters, where the key is the namespaced name, // group and kind of the HTTPFilter. httpRouteFilters map[utils.NamespacedNameWithGroupKind]*egv1a1.HTTPRouteFilter + // Set for storing HTTPRouteExtensions (Envoy Gateway or Custom) NamespacedNames referenced by various + // route rules objects. + allAssociatedHTTPRouteExtensionFilters sets.Set[utils.NamespacedNameWithGroupKind] } func newResourceMapping() *resourceMappings { return &resourceMappings{ - allAssociatedGateways: sets.New[string](), - allAssociatedReferenceGrants: sets.New[string](), - allAssociatedServiceImports: sets.New[string](), - allAssociatedEndpointSlices: sets.New[string](), - allAssociatedBackends: sets.New[string](), - allAssociatedSecrets: sets.New[string](), - allAssociatedConfigMaps: sets.New[string](), - allAssociatedNamespaces: sets.New[string](), - allAssociatedEnvoyProxies: sets.New[string](), - allAssociatedEnvoyPatchPolicies: sets.New[string](), - allAssociatedTLSRoutes: sets.New[string](), - allAssociatedHTTPRoutes: sets.New[string](), - allAssociatedGRPCRoutes: sets.New[string](), - allAssociatedTCPRoutes: sets.New[string](), - allAssociatedUDPRoutes: sets.New[string](), - allAssociatedBackendRefs: sets.New[gwapiv1.BackendObjectReference](), - allAssociatedClientTrafficPolicies: sets.New[string](), - allAssociatedBackendTrafficPolicies: sets.New[string](), - allAssociatedSecurityPolicies: sets.New[string](), - allAssociatedBackendTLSPolicies: sets.New[string](), - allAssociatedEnvoyExtensionPolicies: sets.New[string](), - extensionRefFilters: map[utils.NamespacedNameWithGroupKind]unstructured.Unstructured{}, - httpRouteFilters: map[utils.NamespacedNameWithGroupKind]*egv1a1.HTTPRouteFilter{}, + allAssociatedGateways: sets.New[string](), + allAssociatedReferenceGrants: sets.New[string](), + allAssociatedServiceImports: sets.New[string](), + allAssociatedEndpointSlices: sets.New[string](), + allAssociatedBackends: sets.New[string](), + allAssociatedSecrets: sets.New[string](), + allAssociatedConfigMaps: sets.New[string](), + allAssociatedNamespaces: sets.New[string](), + allAssociatedEnvoyProxies: sets.New[string](), + allAssociatedEnvoyPatchPolicies: sets.New[string](), + allAssociatedTLSRoutes: sets.New[string](), + allAssociatedHTTPRoutes: sets.New[string](), + allAssociatedGRPCRoutes: sets.New[string](), + allAssociatedTCPRoutes: sets.New[string](), + allAssociatedUDPRoutes: sets.New[string](), + allAssociatedBackendRefs: sets.New[gwapiv1.BackendObjectReference](), + allAssociatedClientTrafficPolicies: sets.New[string](), + allAssociatedBackendTrafficPolicies: sets.New[string](), + allAssociatedSecurityPolicies: sets.New[string](), + allAssociatedBackendTLSPolicies: sets.New[string](), + allAssociatedEnvoyExtensionPolicies: sets.New[string](), + extensionRefFilters: map[utils.NamespacedNameWithGroupKind]unstructured.Unstructured{}, + httpRouteFilters: map[utils.NamespacedNameWithGroupKind]*egv1a1.HTTPRouteFilter{}, + allAssociatedHTTPRouteExtensionFilters: sets.New[utils.NamespacedNameWithGroupKind](), } } diff --git a/internal/provider/kubernetes/routes.go b/internal/provider/kubernetes/routes.go index fa148ffd441..e0ecb7c9d03 100644 --- a/internal/provider/kubernetes/routes.go +++ b/internal/provider/kubernetes/routes.go @@ -421,7 +421,10 @@ func (r *gatewayAPIReconciler) processHTTPRoutes(ctx context.Context, gatewayNam r.log.Error(err, "HTTPRouteFilters not found; bypassing rule", "index", i) continue } - resourceTree.HTTPRouteFilters = append(resourceTree.HTTPRouteFilters, httpFilter) + if !resourceMap.allAssociatedHTTPRouteExtensionFilters.Has(key) { + resourceMap.allAssociatedHTTPRouteExtensionFilters.Insert(key) + resourceTree.HTTPRouteFilters = append(resourceTree.HTTPRouteFilters, httpFilter) + } default: extRefFilter, ok := resourceMap.extensionRefFilters[key] if !ok { @@ -433,7 +436,10 @@ func (r *gatewayAPIReconciler) processHTTPRoutes(ctx context.Context, gatewayNam continue } - resourceTree.ExtensionRefFilters = append(resourceTree.ExtensionRefFilters, extRefFilter) + if !resourceMap.allAssociatedHTTPRouteExtensionFilters.Has(key) { + resourceMap.allAssociatedHTTPRouteExtensionFilters.Insert(key) + resourceTree.ExtensionRefFilters = append(resourceTree.ExtensionRefFilters, extRefFilter) + } } } } diff --git a/internal/provider/kubernetes/routes_test.go b/internal/provider/kubernetes/routes_test.go index 5fc3654657b..d1850b9bfc1 100644 --- a/internal/provider/kubernetes/routes_test.go +++ b/internal/provider/kubernetes/routes_test.go @@ -75,6 +75,7 @@ func TestProcessHTTPRoutes(t *testing.T) { routes []*gwapiv1.HTTPRoute extensionFilters []*unstructured.Unstructured extensionAPIGroups []schema.GroupVersionKind + httpRouteFilters []*egv1a1.HTTPRouteFilter expected bool }{ { @@ -338,6 +339,253 @@ func TestProcessHTTPRoutes(t *testing.T) { }, expected: true, }, + { + name: "multiple httproute with same extension filter", + routes: []*gwapiv1.HTTPRoute{ + { + ObjectMeta: metav1.ObjectMeta{ + Namespace: httpRouteNS, + Name: "test", + }, + Spec: gwapiv1.HTTPRouteSpec{ + CommonRouteSpec: gwapiv1.CommonRouteSpec{ + ParentRefs: []gwapiv1.ParentReference{ + { + Name: "test", + }, + }, + }, + Rules: []gwapiv1.HTTPRouteRule{ + { + Matches: []gwapiv1.HTTPRouteMatch{ + { + Path: &gwapiv1.HTTPPathMatch{ + Type: ptr.To(gwapiv1.PathMatchPathPrefix), + Value: ptr.To("/1"), + }, + }, + }, + Filters: []gwapiv1.HTTPRouteFilter{ + { + Type: gwapiv1.HTTPRouteFilterExtensionRef, + ExtensionRef: &gwapiv1.LocalObjectReference{ + Group: gwapiv1.Group("gateway.example.io"), + Kind: gwapiv1.Kind("Bar"), + Name: gwapiv1.ObjectName("test"), + }, + }, + }, + BackendRefs: []gwapiv1.HTTPBackendRef{ + { + BackendRef: gwapiv1.BackendRef{ + BackendObjectReference: gwapiv1.BackendObjectReference{ + Group: gatewayapi.GroupPtr(corev1.GroupName), + Kind: gatewayapi.KindPtr(resource.KindService), + Name: "test", + }, + }, + }, + }, + }, + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Namespace: httpRouteNS, + Name: "test-2", + }, + Spec: gwapiv1.HTTPRouteSpec{ + CommonRouteSpec: gwapiv1.CommonRouteSpec{ + ParentRefs: []gwapiv1.ParentReference{ + { + Name: "test", + }, + }, + }, + Rules: []gwapiv1.HTTPRouteRule{ + { + Matches: []gwapiv1.HTTPRouteMatch{ + { + Path: &gwapiv1.HTTPPathMatch{ + Type: ptr.To(gwapiv1.PathMatchPathPrefix), + Value: ptr.To("/2"), + }, + }, + }, + Filters: []gwapiv1.HTTPRouteFilter{ + { + Type: gwapiv1.HTTPRouteFilterExtensionRef, + ExtensionRef: &gwapiv1.LocalObjectReference{ + Group: gwapiv1.Group("gateway.example.io"), + Kind: gwapiv1.Kind("Bar"), + Name: gwapiv1.ObjectName("test"), + }, + }, + }, + BackendRefs: []gwapiv1.HTTPBackendRef{ + { + BackendRef: gwapiv1.BackendRef{ + BackendObjectReference: gwapiv1.BackendObjectReference{ + Group: gatewayapi.GroupPtr(corev1.GroupName), + Kind: gatewayapi.KindPtr(resource.KindService), + Name: "test", + }, + }, + }, + }, + }, + }, + }, + }, + }, + extensionFilters: []*unstructured.Unstructured{ + { + Object: map[string]interface{}{ + "apiVersion": "gateway.example.io/v1alpha1", + "kind": "Bar", + "metadata": map[string]interface{}{ + "name": "test", + "namespace": httpRouteNS, + }, + }, + }, + }, + extensionAPIGroups: []schema.GroupVersionKind{ + { + Group: "gateway.example.io", + Version: "v1alpha1", + Kind: "Bar", + }, + { + Group: "gateway.example.io", + Version: "v1alpha1", + Kind: "Foo", + }, + }, + expected: true, + }, + { + name: "multiple httproute with same extension filter: Envoy Gateway HTTPRouteFilter", + routes: []*gwapiv1.HTTPRoute{ + { + ObjectMeta: metav1.ObjectMeta{ + Namespace: httpRouteNS, + Name: "test", + }, + Spec: gwapiv1.HTTPRouteSpec{ + CommonRouteSpec: gwapiv1.CommonRouteSpec{ + ParentRefs: []gwapiv1.ParentReference{ + { + Name: "test", + }, + }, + }, + Rules: []gwapiv1.HTTPRouteRule{ + { + Matches: []gwapiv1.HTTPRouteMatch{ + { + Path: &gwapiv1.HTTPPathMatch{ + Type: ptr.To(gwapiv1.PathMatchPathPrefix), + Value: ptr.To("/1"), + }, + }, + }, + Filters: []gwapiv1.HTTPRouteFilter{ + { + Type: gwapiv1.HTTPRouteFilterExtensionRef, + ExtensionRef: &gwapiv1.LocalObjectReference{ + Group: gwapiv1.Group(egv1a1.GroupName), + Kind: gwapiv1.Kind(egv1a1.KindHTTPRouteFilter), + Name: gwapiv1.ObjectName("test"), + }, + }, + }, + BackendRefs: []gwapiv1.HTTPBackendRef{ + { + BackendRef: gwapiv1.BackendRef{ + BackendObjectReference: gwapiv1.BackendObjectReference{ + Group: gatewayapi.GroupPtr(corev1.GroupName), + Kind: gatewayapi.KindPtr(resource.KindService), + Name: "test", + }, + }, + }, + }, + }, + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Namespace: httpRouteNS, + Name: "test-2", + }, + Spec: gwapiv1.HTTPRouteSpec{ + CommonRouteSpec: gwapiv1.CommonRouteSpec{ + ParentRefs: []gwapiv1.ParentReference{ + { + Name: "test", + }, + }, + }, + Rules: []gwapiv1.HTTPRouteRule{ + { + Matches: []gwapiv1.HTTPRouteMatch{ + { + Path: &gwapiv1.HTTPPathMatch{ + Type: ptr.To(gwapiv1.PathMatchPathPrefix), + Value: ptr.To("/2"), + }, + }, + }, + Filters: []gwapiv1.HTTPRouteFilter{ + { + Type: gwapiv1.HTTPRouteFilterExtensionRef, + ExtensionRef: &gwapiv1.LocalObjectReference{ + Group: gwapiv1.Group(egv1a1.GroupName), + Kind: gwapiv1.Kind(egv1a1.KindHTTPRouteFilter), + Name: gwapiv1.ObjectName("test"), + }, + }, + }, + BackendRefs: []gwapiv1.HTTPBackendRef{ + { + BackendRef: gwapiv1.BackendRef{ + BackendObjectReference: gwapiv1.BackendObjectReference{ + Group: gatewayapi.GroupPtr(corev1.GroupName), + Kind: gatewayapi.KindPtr(resource.KindService), + Name: "test", + }, + }, + }, + }, + }, + }, + }, + }, + }, + httpRouteFilters: []*egv1a1.HTTPRouteFilter{ + { + TypeMeta: metav1.TypeMeta{ + Kind: egv1a1.KindHTTPRouteFilter, + APIVersion: egv1a1.GroupVersion.String(), + }, + ObjectMeta: metav1.ObjectMeta{ + Namespace: httpRouteNS, + Name: "test", + }, + Spec: egv1a1.HTTPRouteFilterSpec{ + URLRewrite: &egv1a1.HTTPURLRewriteFilter{ + Hostname: &egv1a1.HTTPHostnameModifier{ + Type: egv1a1.BackendHTTPHostnameModifier, + }, + }, + }, + }, + }, + expected: true, + }, } for i := range testCases { @@ -355,6 +603,7 @@ func TestProcessHTTPRoutes(t *testing.T) { r := &gatewayAPIReconciler{ log: logger, classController: gcCtrlName, + hrfCRDExists: true, } // Add the test case objects to the reconciler client. @@ -367,6 +616,9 @@ func TestProcessHTTPRoutes(t *testing.T) { if len(tc.extensionAPIGroups) > 0 { r.extGVKs = append(r.extGVKs, tc.extensionAPIGroups...) } + for _, filter := range tc.httpRouteFilters { + objs = append(objs, filter) + } r.client = fakeclient.NewClientBuilder(). WithScheme(envoygateway.GetScheme()). WithObjects(objs...). @@ -390,6 +642,8 @@ func TestProcessHTTPRoutes(t *testing.T) { require.NoError(t, err) // Ensure the resource tree and map are as expected. require.Equal(t, tc.routes, resourceTree.HTTPRoutes) + require.Equal(t, len(tc.extensionFilters), len(resourceTree.ExtensionRefFilters)) + require.Equal(t, len(tc.httpRouteFilters), len(resourceTree.HTTPRouteFilters)) if tc.extensionFilters != nil { for _, filter := range tc.extensionFilters { key := utils.NamespacedNameWithGroupKind{ diff --git a/release-notes/current.yaml b/release-notes/current.yaml index 9616c0ff02c..083f27c791f 100644 --- a/release-notes/current.yaml +++ b/release-notes/current.yaml @@ -22,6 +22,7 @@ new features: | # Fixes for bugs identified in previous versions. bug fixes: | Fixed a nil pointer error that occurs when a SecurityPolicy refers to a UDS backend + Fixed validation failure when multiple HTTPRoutes refer to the same extension filter # Enhancements that improve performance. performance improvements: | From 2be279e48a97c6d4110382474164dedd868ea020 Mon Sep 17 00:00:00 2001 From: Guy Daich Date: Mon, 6 Jan 2025 13:05:08 -0600 Subject: [PATCH 2/3] fix code review comments Signed-off-by: Guy Daich --- internal/provider/kubernetes/filters.go | 11 +++++------ internal/provider/kubernetes/resource.go | 5 ----- internal/provider/kubernetes/routes.go | 17 +++-------------- release-notes/current.yaml | 2 +- 4 files changed, 9 insertions(+), 26 deletions(-) diff --git a/internal/provider/kubernetes/filters.go b/internal/provider/kubernetes/filters.go index cffcf214f6b..065ddb97d16 100644 --- a/internal/provider/kubernetes/filters.go +++ b/internal/provider/kubernetes/filters.go @@ -50,13 +50,12 @@ func (r *gatewayAPIReconciler) getExtensionRefFilters(ctx context.Context) ([]un return resourceItems, nil } -func (r *gatewayAPIReconciler) getHTTPRouteFilters(ctx context.Context) ([]egv1a1.HTTPRouteFilter, error) { - httpFilterList := new(egv1a1.HTTPRouteFilterList) - if err := r.client.List(ctx, httpFilterList); err != nil { - return nil, fmt.Errorf("failed to list HTTPRouteFilters: %w", err) +func (r *gatewayAPIReconciler) getHTTPRouteFilter(ctx context.Context, name, namespace string) (*egv1a1.HTTPRouteFilter, error) { + hrf := new(egv1a1.HTTPRouteFilter) + if err := r.client.Get(ctx, types.NamespacedName{Namespace: namespace, Name: name}, hrf); err != nil { + return nil, fmt.Errorf("failed to get HTTPRouteFilter: %w", err) } - - return httpFilterList.Items, nil + return hrf, nil } // processRouteFilterConfigMapRef adds the referenced ConfigMap in a HTTPRouteFilter diff --git a/internal/provider/kubernetes/resource.go b/internal/provider/kubernetes/resource.go index 13a06688855..1744722559b 100644 --- a/internal/provider/kubernetes/resource.go +++ b/internal/provider/kubernetes/resource.go @@ -10,7 +10,6 @@ import ( "k8s.io/apimachinery/pkg/util/sets" gwapiv1 "sigs.k8s.io/gateway-api/apis/v1" - egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1" "github.com/envoyproxy/gateway/internal/utils" ) @@ -61,9 +60,6 @@ type resourceMappings struct { // The key is the namespaced name, group and kind of the filter and the value is the // unstructured form of the resource. extensionRefFilters map[utils.NamespacedNameWithGroupKind]unstructured.Unstructured - // httpRouteFilters is a map of HTTPRouteFilters, where the key is the namespaced name, - // group and kind of the HTTPFilter. - httpRouteFilters map[utils.NamespacedNameWithGroupKind]*egv1a1.HTTPRouteFilter // Set for storing HTTPRouteExtensions (Envoy Gateway or Custom) NamespacedNames referenced by various // route rules objects. allAssociatedHTTPRouteExtensionFilters sets.Set[utils.NamespacedNameWithGroupKind] @@ -93,7 +89,6 @@ func newResourceMapping() *resourceMappings { allAssociatedBackendTLSPolicies: sets.New[string](), allAssociatedEnvoyExtensionPolicies: sets.New[string](), extensionRefFilters: map[utils.NamespacedNameWithGroupKind]unstructured.Unstructured{}, - httpRouteFilters: map[utils.NamespacedNameWithGroupKind]*egv1a1.HTTPRouteFilter{}, allAssociatedHTTPRouteExtensionFilters: sets.New[utils.NamespacedNameWithGroupKind](), } } diff --git a/internal/provider/kubernetes/routes.go b/internal/provider/kubernetes/routes.go index e0ecb7c9d03..8395e0eb573 100644 --- a/internal/provider/kubernetes/routes.go +++ b/internal/provider/kubernetes/routes.go @@ -238,18 +238,6 @@ func (r *gatewayAPIReconciler) processHTTPRoutes(ctx context.Context, gatewayNam resourceMap *resourceMappings, resourceTree *resource.Resources, ) error { httpRouteList := &gwapiv1.HTTPRouteList{} - if r.hrfCRDExists { - httpFilters, err := r.getHTTPRouteFilters(ctx) - if err != nil { - return err - } - - for i := range httpFilters { - filter := httpFilters[i] - resourceMap.httpRouteFilters[utils.GetNamespacedNameWithGroupKind(&filter)] = &filter - r.processRouteFilterConfigMapRef(ctx, &filter, resourceMap, resourceTree) - } - } extensionRefFilters, err := r.getExtensionRefFilters(ctx) if err != nil { @@ -416,12 +404,13 @@ func (r *gatewayAPIReconciler) processHTTPRoutes(ctx context.Context, gatewayNam switch string(filter.ExtensionRef.Kind) { case egv1a1.KindHTTPRouteFilter: - httpFilter, ok := resourceMap.httpRouteFilters[key] - if !ok { + httpFilter, err := r.getHTTPRouteFilter(ctx, key.Name, key.Namespace) + if err != nil { r.log.Error(err, "HTTPRouteFilters not found; bypassing rule", "index", i) continue } if !resourceMap.allAssociatedHTTPRouteExtensionFilters.Has(key) { + r.processRouteFilterConfigMapRef(ctx, httpFilter, resourceMap, resourceTree) resourceMap.allAssociatedHTTPRouteExtensionFilters.Insert(key) resourceTree.HTTPRouteFilters = append(resourceTree.HTTPRouteFilters, httpFilter) } diff --git a/release-notes/current.yaml b/release-notes/current.yaml index 083f27c791f..ab4db4360fa 100644 --- a/release-notes/current.yaml +++ b/release-notes/current.yaml @@ -22,7 +22,7 @@ new features: | # Fixes for bugs identified in previous versions. bug fixes: | Fixed a nil pointer error that occurs when a SecurityPolicy refers to a UDS backend - Fixed validation failure when multiple HTTPRoutes refer to the same extension filter + Fixed a validation failure when multiple HTTPRoutes refer to the same extension filter # Enhancements that improve performance. performance improvements: | From eb9b2dbbe5b2514abd3b66ea1c53a94fcd08eb71 Mon Sep 17 00:00:00 2001 From: Guy Daich Date: Mon, 6 Jan 2025 13:58:04 -0600 Subject: [PATCH 3/3] check if httproutefilter crd exists Signed-off-by: Guy Daich --- internal/provider/kubernetes/routes.go | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/internal/provider/kubernetes/routes.go b/internal/provider/kubernetes/routes.go index 8395e0eb573..1dd2afe5ea5 100644 --- a/internal/provider/kubernetes/routes.go +++ b/internal/provider/kubernetes/routes.go @@ -404,15 +404,17 @@ func (r *gatewayAPIReconciler) processHTTPRoutes(ctx context.Context, gatewayNam switch string(filter.ExtensionRef.Kind) { case egv1a1.KindHTTPRouteFilter: - httpFilter, err := r.getHTTPRouteFilter(ctx, key.Name, key.Namespace) - if err != nil { - r.log.Error(err, "HTTPRouteFilters not found; bypassing rule", "index", i) - continue - } - if !resourceMap.allAssociatedHTTPRouteExtensionFilters.Has(key) { - r.processRouteFilterConfigMapRef(ctx, httpFilter, resourceMap, resourceTree) - resourceMap.allAssociatedHTTPRouteExtensionFilters.Insert(key) - resourceTree.HTTPRouteFilters = append(resourceTree.HTTPRouteFilters, httpFilter) + if r.hrfCRDExists { + httpFilter, err := r.getHTTPRouteFilter(ctx, key.Name, key.Namespace) + if err != nil { + r.log.Error(err, "HTTPRouteFilters not found; bypassing rule", "index", i) + continue + } + if !resourceMap.allAssociatedHTTPRouteExtensionFilters.Has(key) { + r.processRouteFilterConfigMapRef(ctx, httpFilter, resourceMap, resourceTree) + resourceMap.allAssociatedHTTPRouteExtensionFilters.Insert(key) + resourceTree.HTTPRouteFilters = append(resourceTree.HTTPRouteFilters, httpFilter) + } } default: extRefFilter, ok := resourceMap.extensionRefFilters[key]