Skip to content

Commit

Permalink
fix(konnect): do not create Service+Route combinations for konghq.com…
Browse files Browse the repository at this point in the history 
…/plugins annotated entities
  • Loading branch information
@pmalek
pmalek committed Sep 26, 2024
1 parent a8465d5 commit ec2db44
Show file tree
Hide file tree
Showing 3 changed files with 133 additions and 137 deletions.
137 changes: 34 additions & 103 deletions controller/konnect/reconciler_kongplugin_combinations.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand All @@ -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 {
Expand All @@ -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,
})
}
}

Expand Down
41 changes: 40 additions & 1 deletion controller/konnect/reconciler_kongplugin_combinations_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,9 @@ func TestGetCombinations(t *testing.T) {
},
want: []Rel{
{
Route: "r1",
Route: "r1",
},
{
Service: "s1",
},
},
Expand Down Expand Up @@ -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",
// },
},
},
{
Expand Down Expand Up @@ -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",
},
},
Expand Down
92 changes: 59 additions & 33 deletions test/envtest/kongpluginbinding_managed_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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) {
Expand Down

0 comments on commit ec2db44

Please sign in to comment.