Skip to content

Commit

Permalink
fix(plugins) handle CG plugins like consumers (#6132)
Browse files Browse the repository at this point in the history
Use logic similar to consumers for consumer groups. This fixes an issue
where plugins assigned to a consumer group and a route (or service)
would not actually be associated with the consumer group, only the route
(or service).
  • Loading branch information
rainest authored Jun 7, 2024
1 parent 28567d7 commit ee797b4
Show file tree
Hide file tree
Showing 5 changed files with 229 additions and 41 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,11 @@ Adding a new version? You'll need three changes:
- Fixed KIC non leaders correctly getting up to date Admin API addresses by not
requiring leader election for the related controller.
[#6126](https://github.com/Kong/kubernetes-ingress-controller/pull/6126)
- Plugins attached to both a KongConsumerGroup and a route-like resource or
Service now properly generate a plugin attached to both a Kong consumer group
and route or service. Previously, these incorrectly generated plugins
attached to the route or service only.
[#6132](https://github.com/Kong/kubernetes-ingress-controller/pull/6132)
- KongPlugin's `config` field is no longer incorrectly sanitized.
[#6138](https://github.com/Kong/kubernetes-ingress-controller/pull/6138)

Expand Down
53 changes: 42 additions & 11 deletions internal/util/relations.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,36 +9,67 @@ type Rel struct {
}

func (relations *ForeignRelations) GetCombinations() []Rel {
var (
lConsumer = len(relations.Consumer)
lConsumerGroup = len(relations.ConsumerGroup)
lRoutes = len(relations.Route)
lServices = len(relations.Service)
l = lRoutes + lServices
)

var cartesianProduct []Rel

if len(relations.Consumer) > 0 {
consumers := relations.Consumer
if len(relations.Route)+len(relations.Service) > 0 {
for _, service := range relations.Service {
for _, consumer := range consumers {
// gocritic I don't care that you think switch statements are the one true god of readability, the language offers
// multiple options for a reason. go away, gocritic.
if lConsumer > 0 { //nolint:gocritic
if l > 0 {
cartesianProduct = make([]Rel, 0, l*lConsumer)
for _, consumer := range relations.Consumer {
for _, service := range relations.Service {
cartesianProduct = append(cartesianProduct, Rel{
Service: service,
Consumer: consumer,
})
}
}
for _, route := range relations.Route {
for _, consumer := range consumers {
for _, route := range relations.Route {
cartesianProduct = append(cartesianProduct, Rel{
Route: route,
Consumer: consumer,
})
}
}

} else {
cartesianProduct = make([]Rel, 0, len(relations.Consumer))
for _, consumer := range relations.Consumer {
cartesianProduct = append(cartesianProduct, Rel{Consumer: consumer})
}
}
} else {
for _, consumerGroup := range relations.ConsumerGroup {
cartesianProduct = append(cartesianProduct, Rel{ConsumerGroup: consumerGroup})
} else if lConsumerGroup > 0 {
if l > 0 {
cartesianProduct = make([]Rel, 0, l*lConsumerGroup)
for _, group := range relations.ConsumerGroup {
for _, service := range relations.Service {
cartesianProduct = append(cartesianProduct, Rel{
Service: service,
ConsumerGroup: group,
})
}
for _, route := range relations.Route {
cartesianProduct = append(cartesianProduct, Rel{
Route: route,
ConsumerGroup: group,
})
}
}
} else {
cartesianProduct = make([]Rel, 0, lConsumerGroup)
for _, group := range relations.ConsumerGroup {
cartesianProduct = append(cartesianProduct, Rel{ConsumerGroup: group})
}
}
} else if l > 0 {
cartesianProduct = make([]Rel, 0, l)
for _, service := range relations.Service {
cartesianProduct = append(cartesianProduct, Rel{Service: service})
}
Expand Down
112 changes: 91 additions & 21 deletions internal/util/relations_test.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
package util

import (
"reflect"
"testing"

"github.com/stretchr/testify/require"
)

func TestGetCombinations(t *testing.T) {
Expand Down Expand Up @@ -121,14 +122,14 @@ func TestGetCombinations(t *testing.T) {
Consumer: "foo",
Route: "foo",
},
{
Consumer: "bar",
Route: "foo",
},
{
Consumer: "foo",
Route: "bar",
},
{
Consumer: "bar",
Route: "foo",
},
{
Consumer: "bar",
Route: "bar",
Expand All @@ -148,14 +149,14 @@ func TestGetCombinations(t *testing.T) {
Consumer: "foo",
Service: "foo",
},
{
Consumer: "bar",
Service: "foo",
},
{
Consumer: "foo",
Service: "bar",
},
{
Consumer: "bar",
Service: "foo",
},
{
Consumer: "bar",
Service: "bar",
Expand All @@ -177,41 +178,110 @@ func TestGetCombinations(t *testing.T) {
Service: "s1",
},
{
Consumer: "c2",
Service: "s1",
Consumer: "c1",
Service: "s2",
},
{
Consumer: "c1",
Service: "s2",
Route: "r1",
},
{
Consumer: "c1",
Route: "r2",
},
{
Consumer: "c2",
Service: "s2",
Service: "s1",
},
{
Consumer: "c1",
Route: "r1",
Consumer: "c2",
Service: "s2",
},
{
Consumer: "c2",
Route: "r1",
},
{
Consumer: "c1",
Consumer: "c2",
Route: "r2",
},
},
},
{
name: "plugins on combination of service,route and consumer group",
args: args{
relations: ForeignRelations{
Route: []string{"r1", "r2"},
Service: []string{"s1", "s2"},
ConsumerGroup: []string{"cg1", "cg2"},
},
},
want: []Rel{
{
Consumer: "c2",
Route: "r2",
ConsumerGroup: "cg1",
Service: "s1",
},
{
ConsumerGroup: "cg1",
Service: "s2",
},
{
ConsumerGroup: "cg1",
Route: "r1",
},
{
ConsumerGroup: "cg1",
Route: "r2",
},
{
ConsumerGroup: "cg2",
Service: "s1",
},
{
ConsumerGroup: "cg2",
Service: "s2",
},
{
ConsumerGroup: "cg2",
Route: "r1",
},
{
ConsumerGroup: "cg2",
Route: "r2",
},
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := tt.args.relations.GetCombinations(); !reflect.DeepEqual(got, tt.want) {
t.Errorf("GetCombinations() = %v, want %v", got, tt.want)
}
require.Equal(t, tt.want, tt.args.relations.GetCombinations())
})
}
}

func BenchmarkGetCombinations(b *testing.B) {
b.Run("consumer groups", func(b *testing.B) {
for i := 0; i < b.N; i++ {
relations := ForeignRelations{
Route: []string{"r1", "r2"},
Service: []string{"s1", "s2"},
ConsumerGroup: []string{"cg1", "cg2"},
}

rels := relations.GetCombinations()
_ = rels
}
})
b.Run("consumers", func(b *testing.B) {
for i := 0; i < b.N; i++ {
relations := ForeignRelations{
Route: []string{"r1", "r2"},
Service: []string{"s1", "s2"},
Consumer: []string{"c1", "c2", "c3"},
}

rels := relations.GetCombinations()
_ = rels
}
})
}
2 changes: 1 addition & 1 deletion test/conformance/gateway_conformance_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ var skippedTestsForTraditionalRoutes = []string{
// tests.GRPCRouteHeaderMatching.ShortName and tests.GRPCExactMethodMatching.ShortName may
// have some conflicts, skipping either one will still pass normally.
// TODO: https://github.com/Kong/kubernetes-ingress-controller/issues/6144
tests.GRPCExactMethodMatching.ShortName,
tests.GRPCRouteHeaderMatching.ShortName,
}

var skippedTestsForExpressionRoutes = []string{
Expand Down
Loading

0 comments on commit ee797b4

Please sign in to comment.