From 95e2e3569953f5f4be74f55d9b1e6777d6e9d241 Mon Sep 17 00:00:00 2001 From: Lior Okman Date: Mon, 20 May 2024 19:51:02 +0300 Subject: [PATCH] bug: Route extension filters with different types but the same name and namespace aren't correctly cached (#3388) * Route extension filters are unstructured.Unstructured instances, so caching them should be done with both the name and type as a key. Signed-off-by: Lior Okman * Moved NamespacedNameAndType to the Kubernetes helpers, and renamed it to be clearer about what it has. Signed-off-by: Lior Okman * Also renamed the helper function. Signed-off-by: Lior Okman * Moved to the 'utils' package to be beside NamespacedName. Signed-off-by: Lior Okman * Renamed structure according to review, and updated the comments Signed-off-by: Lior Okman --------- Signed-off-by: Lior Okman --- internal/provider/kubernetes/controller.go | 6 +- internal/provider/kubernetes/routes.go | 27 +++-- internal/provider/kubernetes/routes_test.go | 110 +++++++++++++++++++- internal/utils/misc.go | 20 ++++ 4 files changed, 149 insertions(+), 14 deletions(-) diff --git a/internal/provider/kubernetes/controller.go b/internal/provider/kubernetes/controller.go index 1852384de4d..0c33873c1df 100644 --- a/internal/provider/kubernetes/controller.go +++ b/internal/provider/kubernetes/controller.go @@ -129,9 +129,9 @@ type resourceMappings struct { // Map for storing backendRefs' NamespaceNames referred by various Route objects. allAssociatedBackendRefs map[gwapiv1.BackendObjectReference]struct{} // extensionRefFilters is a map of filters managed by an extension. - // The key is the namespaced name of the filter and the value is the + // The key is the namespaced name, group and kind of the filter and the value is the // unstructured form of the resource. - extensionRefFilters map[types.NamespacedName]unstructured.Unstructured + extensionRefFilters map[utils.NamespacedNameWithGroupKind]unstructured.Unstructured } func newResourceMapping() *resourceMappings { @@ -143,7 +143,7 @@ func newResourceMapping() *resourceMappings { allAssociatedTCPRoutes: map[string]struct{}{}, allAssociatedUDPRoutes: map[string]struct{}{}, allAssociatedBackendRefs: map[gwapiv1.BackendObjectReference]struct{}{}, - extensionRefFilters: map[types.NamespacedName]unstructured.Unstructured{}, + extensionRefFilters: map[utils.NamespacedNameWithGroupKind]unstructured.Unstructured{}, } } diff --git a/internal/provider/kubernetes/routes.go b/internal/provider/kubernetes/routes.go index f72d1b2cfa2..76991be9947 100644 --- a/internal/provider/kubernetes/routes.go +++ b/internal/provider/kubernetes/routes.go @@ -185,10 +185,17 @@ func (r *gatewayAPIReconciler) processGRPCRoutes(ctx context.Context, gatewayNam if filter.Type == gwapiv1.GRPCRouteFilterExtensionRef { // NOTE: filters must be in the same namespace as the GRPCRoute // Check if it's a Kind managed by an extension and add to resourceTree - key := types.NamespacedName{ - Namespace: grpcRoute.Namespace, - Name: string(filter.ExtensionRef.Name), + key := utils.NamespacedNameWithGroupKind{ + NamespacedName: types.NamespacedName{ + Namespace: grpcRoute.Namespace, + Name: string(filter.ExtensionRef.Name), + }, + GroupKind: schema.GroupKind{ + Group: string(filter.ExtensionRef.Group), + Kind: string(filter.ExtensionRef.Kind), + }, } + extRefFilter, ok := resourceMap.extensionRefFilters[key] if !ok { r.log.Error( @@ -229,7 +236,7 @@ func (r *gatewayAPIReconciler) processHTTPRoutes(ctx context.Context, gatewayNam } for i := range extensionRefFilters { filter := extensionRefFilters[i] - resourceMap.extensionRefFilters[utils.NamespacedName(&filter)] = filter + resourceMap.extensionRefFilters[utils.GetNamespacedNameWithGroupKind(&filter)] = filter } if err := r.client.List(ctx, httpRouteList, &client.ListOptions{ @@ -367,9 +374,15 @@ func (r *gatewayAPIReconciler) processHTTPRoutes(ctx context.Context, gatewayNam } else if filter.Type == gwapiv1.HTTPRouteFilterExtensionRef { // NOTE: filters must be in the same namespace as the HTTPRoute // Check if it's a Kind managed by an extension and add to resourceTree - key := types.NamespacedName{ - Namespace: httpRoute.Namespace, - Name: string(filter.ExtensionRef.Name), + key := utils.NamespacedNameWithGroupKind{ + NamespacedName: types.NamespacedName{ + Namespace: httpRoute.Namespace, + Name: string(filter.ExtensionRef.Name), + }, + GroupKind: schema.GroupKind{ + Group: string(filter.ExtensionRef.Group), + Kind: string(filter.ExtensionRef.Kind), + }, } extRefFilter, ok := resourceMap.extensionRefFilters[key] if !ok { diff --git a/internal/provider/kubernetes/routes_test.go b/internal/provider/kubernetes/routes_test.go index 9a230e6824b..be6769589a1 100644 --- a/internal/provider/kubernetes/routes_test.go +++ b/internal/provider/kubernetes/routes_test.go @@ -120,6 +120,102 @@ func TestProcessHTTPRoutes(t *testing.T) { }, expected: true, }, + { + name: "httproute with extension filter multiple types same name", + 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("/"), + }, + }, + }, + Filters: []gwapiv1.HTTPRouteFilter{ + { + Type: gwapiv1.HTTPRouteFilterExtensionRef, + ExtensionRef: &gwapiv1.LocalObjectReference{ + Group: gwapiv1.Group("gateway.example.io"), + Kind: gwapiv1.Kind("Bar"), + Name: gwapiv1.ObjectName("test"), + }, + }, + { + Type: gwapiv1.HTTPRouteFilterExtensionRef, + ExtensionRef: &gwapiv1.LocalObjectReference{ + Group: gwapiv1.Group("gateway.example.io"), + Kind: gwapiv1.Kind("Foo"), + Name: gwapiv1.ObjectName("test"), + }, + }, + }, + BackendRefs: []gwapiv1.HTTPBackendRef{ + { + BackendRef: gwapiv1.BackendRef{ + BackendObjectReference: gwapiv1.BackendObjectReference{ + Group: gatewayapi.GroupPtr(corev1.GroupName), + Kind: gatewayapi.KindPtr(gatewayapi.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, + }, + }, + }, + { + Object: map[string]interface{}{ + "apiVersion": "gateway.example.io/v1alpha1", + "kind": "Foo", + "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: "httproute with one filter_from_extension", routes: []*gwapiv1.HTTPRoute{ @@ -294,10 +390,16 @@ func TestProcessHTTPRoutes(t *testing.T) { // Ensure the resource tree and map are as expected. require.Equal(t, tc.routes, resourceTree.HTTPRoutes) if tc.extensionFilters != nil { - for i, filter := range tc.extensionFilters { - key := types.NamespacedName{ - Namespace: tc.routes[i].Namespace, - Name: filter.GetName(), + for _, filter := range tc.extensionFilters { + key := utils.NamespacedNameWithGroupKind{ + NamespacedName: types.NamespacedName{ + Namespace: tc.routes[0].Namespace, + Name: filter.GetName(), + }, + GroupKind: schema.GroupKind{ + Group: filter.GroupVersionKind().Group, + Kind: filter.GroupVersionKind().Kind, + }, } require.Equal(t, *filter, resourceMap.extensionRefFilters[key]) } diff --git a/internal/utils/misc.go b/internal/utils/misc.go index 6e434660de6..84043ecdcd2 100644 --- a/internal/utils/misc.go +++ b/internal/utils/misc.go @@ -11,10 +11,30 @@ import ( "hash/fnv" "strings" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" ) +type NamespacedNameWithGroupKind struct { + types.NamespacedName + schema.GroupKind +} + +// GetNamespacedNameWithGroupKind creates and returns object's NamespacedNameWithGroupKind. +func GetNamespacedNameWithGroupKind(obj client.Object) NamespacedNameWithGroupKind { + return NamespacedNameWithGroupKind{ + NamespacedName: types.NamespacedName{ + Namespace: obj.GetNamespace(), + Name: obj.GetName(), + }, + GroupKind: schema.GroupKind{ + Group: obj.GetObjectKind().GroupVersionKind().GroupKind().Group, + Kind: obj.GetObjectKind().GroupVersionKind().GroupKind().Kind, + }, + } +} + // NamespacedName creates and returns object's NamespacedName. func NamespacedName(obj client.Object) types.NamespacedName { return types.NamespacedName{