Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(konnect): do not create Service+Route combinations for konghq.com/plugins annotated entities #659

Merged
merged 2 commits into from
Sep 27, 2024
Merged

fix(konnect): do not create Service+Route combinations for konghq.com/plugins annotated entities #659

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
50849ec
fix(konnect): do not create Service+Route combinations for konghq.com…
pmalek Sep 26, 2024
2d9cd71
chore: add comments about plugin binding combinations generated from …
pmalek Sep 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
137 changes: 34 additions & 103 deletions controller/konnect/reconciler_kongplugin_combinations.go
View file
Open in desktop
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
59 changes: 58 additions & 1 deletion controller/konnect/reconciler_kongplugin_combinations_test.go
View file
Open in desktop
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 All @@ -157,6 +159,13 @@ func TestGetCombinations(t *testing.T) {
Consumer: "c1",
Service: "s1",
},
// NOTE: https://github.com/Kong/gateway-operator/issues/660
// is related to the following combination not being present.
// Currently we do not generate combination for Service only
// when Service **and** Consumers have the annotation present.
// {
// Service: "s1",
// },
},
},
{
Expand All @@ -182,6 +191,45 @@ func TestGetCombinations(t *testing.T) {
Consumer: "c2",
Service: "s1",
},
// NOTE: https://github.com/Kong/gateway-operator/issues/660
// is related to the following combination not being present.
// Currently we do not generate combination for Service only
// when Service **and** Consumers have the annotation present.
// {
// Service: "s1",
// },
},
},
{
name: "plugins on combination of service and consumer groups",
args: args{
relations: ForeignRelations{
Service: []configurationv1alpha1.KongService{
{
ObjectMeta: metav1.ObjectMeta{
Name: "s1",
},
},
},
ConsumerGroup: []string{"cg1", "cg2"},
},
},
want: []Rel{
{
ConsumerGroup: "cg1",
Service: "s1",
},
{
ConsumerGroup: "cg2",
Service: "s1",
},
// NOTE: https://github.com/Kong/gateway-operator/issues/660
// is related to the following combination not being present.
// Currently we do not generate combination for Service only
// when Service **and** ConsumerGroups have the annotation present.
// {
// Service: "s1",
// },
},
},
{
Expand Down Expand Up @@ -217,6 +265,9 @@ func TestGetCombinations(t *testing.T) {
{
Consumer: "c1",
Route: "r1",
},
{
Consumer: "c1",
Service: "s1",
},
},
Expand Down Expand Up @@ -254,11 +305,17 @@ func TestGetCombinations(t *testing.T) {
{
Consumer: "c1",
Route: "r1",
},
{
Consumer: "c1",
Service: "s1",
},
{
Consumer: "c2",
Route: "r1",
},
{
Consumer: "c2",
Service: "s1",
},
},
Expand Down
Loading
Loading