From ec2db4415329742fbb211463881061212d0a0ed5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20Ma=C5=82ek?= Date: Thu, 26 Sep 2024 17:56:41 +0200 Subject: [PATCH] fix(konnect): do not create Service+Route combinations for konghq.com/plugins annotated entities --- .../reconciler_kongplugin_combinations.go | 137 +++++------------- ...reconciler_kongplugin_combinations_test.go | 41 +++++- .../envtest/kongpluginbinding_managed_test.go | 92 +++++++----- 3 files changed, 133 insertions(+), 137 deletions(-) diff --git a/controller/konnect/reconciler_kongplugin_combinations.go b/controller/konnect/reconciler_kongplugin_combinations.go index 20aa12b52..ac690e503 100644 --- a/controller/konnect/reconciler_kongplugin_combinations.go +++ b/controller/konnect/reconciler_kongplugin_combinations.go @@ -4,7 +4,6 @@ import ( "context" "k8s.io/apimachinery/pkg/types" - "k8s.io/apimachinery/pkg/util/sets" "sigs.k8s.io/controller-runtime/pkg/client" configurationv1alpha1 "github.com/kong/kubernetes-configuration/api/configuration/v1alpha1" @@ -96,6 +95,15 @@ type Rel struct { } // GetCombinations returns all possible combinations of relations. +// +// NOTE: This is heavily based on the implementation in the Kong Ingress Controller: +// https://github.com/Kong/kubernetes-ingress-controller/blob/ee797b4e84bd176526af32ab6db54f16ee9c245b/internal/util/relations_test.go +// +// TODO: https://github.com/Kong/gateway-operator/pull/659 +// The combinations created here should be reconsidered. +// Specifically the Service + Route combination which currently creates 2 separate relations targeting +// Service and Route independently. +// This most likely should create 2 relations targeting Service and Service+Route. func (relations *ForeignRelations) GetCombinations() []Rel { var ( lConsumer = len(relations.Consumer) @@ -110,46 +118,21 @@ func (relations *ForeignRelations) GetCombinations() []Rel { if lConsumer > 0 { //nolint:gocritic if l > 0 { cartesianProduct = make([]Rel, 0, l*lConsumer) - servicesForRoutes := sets.NewString() for _, consumer := range relations.Consumer { for _, route := range relations.Route { - - var serviceForRouteFound bool - for _, service := range relations.Service { - if route.Spec.ServiceRef.NamespacedRef.Name == service.Name { - serviceForRouteFound = true - servicesForRoutes.Insert(service.Name) - cartesianProduct = append(cartesianProduct, Rel{ - Service: service.Name, - Route: route.Name, - Consumer: consumer, - }) - } else { - cartesianProduct = append(cartesianProduct, Rel{ - Service: service.Name, - Consumer: consumer, - }) - } - } - if !serviceForRouteFound { - cartesianProduct = append(cartesianProduct, Rel{ - Route: route.Name, - Consumer: consumer, - }) - } + cartesianProduct = append(cartesianProduct, Rel{ + Route: route.Name, + Consumer: consumer, + }) } - for _, service := range relations.Service { - if !servicesForRoutes.Has(service.Name) { - cartesianProduct = append(cartesianProduct, Rel{ - Service: service.Name, - Consumer: consumer, - }) - } + cartesianProduct = append(cartesianProduct, Rel{ + Service: service.Name, + Consumer: consumer, + }) } } - } else { cartesianProduct = make([]Rel, 0, len(relations.Consumer)) for _, consumer := range relations.Consumer { @@ -159,93 +142,41 @@ func (relations *ForeignRelations) GetCombinations() []Rel { } else if lConsumerGroup > 0 { if l > 0 { cartesianProduct = make([]Rel, 0, l*lConsumerGroup) - servicesForRoutes := sets.NewString() for _, group := range relations.ConsumerGroup { for _, route := range relations.Route { - - var serviceForRouteFound bool - for _, service := range relations.Service { - serviceRef := route.Spec.ServiceRef - if serviceRef.Type != configurationv1alpha1.ServiceRefNamespacedRef { - continue - } - - if serviceRef.NamespacedRef.Name == service.Name { - serviceForRouteFound = true - servicesForRoutes.Insert(service.Name) - cartesianProduct = append(cartesianProduct, Rel{ - Service: service.Name, - Route: route.Name, - ConsumerGroup: group, - }) - } else { - cartesianProduct = append(cartesianProduct, Rel{ - Service: service.Name, - ConsumerGroup: group, - }) - } - } - if !serviceForRouteFound { - cartesianProduct = append(cartesianProduct, Rel{ - Route: route.Name, - ConsumerGroup: group, - }) - } + cartesianProduct = append(cartesianProduct, Rel{ + Route: route.Name, + ConsumerGroup: group, + }) } - for _, service := range relations.Service { - if !servicesForRoutes.Has(service.Name) { - cartesianProduct = append(cartesianProduct, Rel{ - Service: service.Name, - ConsumerGroup: group, - }) - } + cartesianProduct = append(cartesianProduct, Rel{ + Service: service.Name, + ConsumerGroup: group, + }) } } } else { cartesianProduct = make([]Rel, 0, lConsumerGroup) for _, group := range relations.ConsumerGroup { - cartesianProduct = append(cartesianProduct, Rel{ConsumerGroup: group}) + cartesianProduct = append(cartesianProduct, Rel{ + ConsumerGroup: group, + }) } } } else if l > 0 { cartesianProduct = make([]Rel, 0, l) - servicesForRoutes := sets.NewString() for _, route := range relations.Route { - var serviceForRouteFound bool - for _, service := range relations.Service { - serviceRef := route.Spec.ServiceRef - if serviceRef.Type != configurationv1alpha1.ServiceRefNamespacedRef { - continue - } - - if serviceRef.NamespacedRef.Name == service.Name { - serviceForRouteFound = true - servicesForRoutes.Insert(service.Name) - cartesianProduct = append(cartesianProduct, Rel{ - Service: service.Name, - Route: route.Name, - }) - } else { - cartesianProduct = append(cartesianProduct, Rel{ - Service: service.Name, - }) - } - } - if !serviceForRouteFound { - cartesianProduct = append(cartesianProduct, Rel{ - Route: route.Name, - }) - } + cartesianProduct = append(cartesianProduct, Rel{ + Route: route.Name, + }) } for _, service := range relations.Service { - if !servicesForRoutes.Has(service.Name) { - cartesianProduct = append(cartesianProduct, Rel{ - Service: service.Name, - }) - } + cartesianProduct = append(cartesianProduct, Rel{ + Service: service.Name, + }) } } diff --git a/controller/konnect/reconciler_kongplugin_combinations_test.go b/controller/konnect/reconciler_kongplugin_combinations_test.go index be7f9cc10..5c37dbb40 100644 --- a/controller/konnect/reconciler_kongplugin_combinations_test.go +++ b/controller/konnect/reconciler_kongplugin_combinations_test.go @@ -133,7 +133,9 @@ func TestGetCombinations(t *testing.T) { }, want: []Rel{ { - Route: "r1", + Route: "r1", + }, + { Service: "s1", }, }, @@ -217,8 +219,39 @@ func TestGetCombinations(t *testing.T) { { Consumer: "c1", Route: "r1", + }, + { + Consumer: "c1", + Service: "s1", + }, + }, + }, + { + name: "plugins on combination of service and consumers", + args: args{ + relations: ForeignRelations{ + Service: []configurationv1alpha1.KongService{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "s1", + }, + }, + }, + Consumer: []string{"c1", "c2"}, + }, + }, + want: []Rel{ + { + Consumer: "c1", + Service: "s1", + }, + { + Consumer: "c2", Service: "s1", }, + // { + // Service: "s1", + // }, }, }, { @@ -254,11 +287,17 @@ func TestGetCombinations(t *testing.T) { { Consumer: "c1", Route: "r1", + }, + { + Consumer: "c1", Service: "s1", }, { Consumer: "c2", Route: "r1", + }, + { + Consumer: "c2", Service: "s1", }, }, diff --git a/test/envtest/kongpluginbinding_managed_test.go b/test/envtest/kongpluginbinding_managed_test.go index e09c9229f..e27641c83 100644 --- a/test/envtest/kongpluginbinding_managed_test.go +++ b/test/envtest/kongpluginbinding_managed_test.go @@ -315,16 +315,27 @@ func TestKongPluginBindingManaged(t *testing.T) { }) updateKongRouteStatusWithProgrammed(t, ctx, clientNamespaced, kongRoute, routeID, cp.GetKonnectStatus().GetKonnectID(), serviceID) - t.Logf("waiting for KongPluginBinding to be created") - kongPluginBinding := watchFor(t, ctx, wKongPluginBinding, watch.Added, + t.Logf("waiting for 2 KongPluginBindings to be created") + kpbRouteFound := false + kpbServiceFound := false + watchFor(t, ctx, wKongPluginBinding, watch.Added, func(kpb *configurationv1alpha1.KongPluginBinding) bool { - return kpb.Spec.Targets.RouteReference != nil && + if kpb.Spec.PluginReference.Name != rateLimitingkongPlugin.Name { + return false + } + + if kpb.Spec.Targets.RouteReference != nil && kpb.Spec.Targets.RouteReference.Name == kongRoute.Name && + kpb.Spec.Targets.ServiceReference == nil { + kpbRouteFound = true + } else if kpb.Spec.Targets.RouteReference == nil && kpb.Spec.Targets.ServiceReference != nil && - kpb.Spec.Targets.ServiceReference.Name == kongService.Name && - kpb.Spec.PluginReference.Name == rateLimitingkongPlugin.Name + kpb.Spec.Targets.ServiceReference.Name == kongService.Name { + kpbServiceFound = true + } + return kpbRouteFound && kpbServiceFound }, - "KongPluginBinding wasn't created", + "2 KongPluginBindings were not created", ) t.Logf( "checking that managed KongPlugin %s gets plugin-in-use finalizer added", @@ -338,60 +349,75 @@ func TestKongPluginBindingManaged(t *testing.T) { "KongPlugin wasn't updated to get plugin-in-use finalizer added", ) - t.Logf("delete managed kongPluginBinding %s, then check it gets recreated", client.ObjectKeyFromObject(kongPluginBinding)) - require.NoError(t, clientNamespaced.Delete(ctx, kongPluginBinding)) - _ = watchFor(t, ctx, wKongPluginBinding, watch.Added, + var l configurationv1alpha1.KongPluginBindingList + require.NoError(t, clientNamespaced.List(ctx, &l)) + for _, kpb := range l.Items { + t.Logf("delete managed kongPluginBinding %s, then check it gets recreated", client.ObjectKeyFromObject(&kpb)) + require.NoError(t, clientNamespaced.Delete(ctx, &kpb)) + } + + var kpbRoute, kpbService *configurationv1alpha1.KongPluginBinding + watchFor(t, ctx, wKongPluginBinding, watch.Added, func(kpb *configurationv1alpha1.KongPluginBinding) bool { - return kpb.Spec.Targets.RouteReference != nil && + if kpb.Spec.PluginReference.Name != rateLimitingkongPlugin.Name { + return false + } + + if kpb.Spec.Targets.RouteReference != nil && kpb.Spec.Targets.RouteReference.Name == kongRoute.Name && + kpb.Spec.Targets.ServiceReference == nil { + kpbRoute = kpb + } else if kpb.Spec.Targets.RouteReference == nil && kpb.Spec.Targets.ServiceReference != nil && - kpb.Spec.Targets.ServiceReference.Name == kongService.Name && - kpb.Spec.PluginReference.Name == rateLimitingkongPlugin.Name + kpb.Spec.Targets.ServiceReference.Name == kongService.Name { + kpbService = kpb + } + return kpbRoute != nil && kpbService != nil }, - "KongPluginBinding wasn't recreated", + "2 KongPluginBindings were not recreated", ) t.Logf( - "remove annotation from KongRoute %s and check that there exists (is created) "+ + "remove annotation from KongRoute %s and check that there exists "+ "a managed KongPluginBinding with only KongService in its targets", client.ObjectKeyFromObject(kongRoute), ) + sdk.PluginSDK.EXPECT(). + DeletePlugin(mock.Anything, cp.GetKonnectStatus().GetKonnectID(), mock.Anything). + Return( + &sdkkonnectops.DeletePluginResponse{ + StatusCode: 200, + }, + nil, + ) delete(kongRoute.Annotations, consts.PluginsAnnotationKey) require.NoError(t, clientNamespaced.Update(ctx, kongRoute)) - kongPluginBinding = watchFor(t, ctx, wKongPluginBinding, watch.Added, - func(kpb *configurationv1alpha1.KongPluginBinding) bool { - return kpb.Spec.Targets.RouteReference == nil && - kpb.Spec.Targets.ServiceReference != nil && - kpb.Spec.Targets.ServiceReference.Name == kongService.Name && - kpb.Spec.PluginReference.Name == rateLimitingkongPlugin.Name - }, - "KongPluginBinding wasn't created", + assert.EventuallyWithT(t, + func(c *assert.CollectT) { + assert.True(c, k8serrors.IsNotFound( + clientNamespaced.Get(ctx, client.ObjectKeyFromObject(kpbRoute), kpbRoute), + )) + }, waitTime, tickTime, + "KongPluginBinding bound to Route wasn't deleted after removing annotation from KongRoute", ) t.Logf( "remove annotation from KongService %s and check a managed KongPluginBinding (%s) gets deleted", client.ObjectKeyFromObject(kongService), - client.ObjectKeyFromObject(kongPluginBinding), + client.ObjectKeyFromObject(kpbService), ) - sdk.PluginSDK.EXPECT(). - DeletePlugin(mock.Anything, cp.GetKonnectStatus().GetKonnectID(), mock.Anything). - Return( - &sdkkonnectops.DeletePluginResponse{ - StatusCode: 200, - }, - nil, - ) + delete(kongService.Annotations, consts.PluginsAnnotationKey) require.NoError(t, clientNamespaced.Update(ctx, kongService)) assert.EventuallyWithT(t, func(c *assert.CollectT) { assert.True(c, k8serrors.IsNotFound( - clientNamespaced.Get(ctx, client.ObjectKeyFromObject(kongPluginBinding), kongPluginBinding), + clientNamespaced.Get(ctx, client.ObjectKeyFromObject(kpbService), kpbService), )) }, waitTime, tickTime, - "KongPluginBinding wasn't deleted after removing annotation from KongRoute", + "KongPluginBinding bound to Service wasn't deleted after removing annotation from KongService", ) assert.EventuallyWithT(t, func(c *assert.CollectT) {