From 057ede21d70adf79f9583dc9fe7666ef45dc0b2a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20Ma=C5=82ek?= Date: Fri, 7 Jun 2024 14:37:15 +0200 Subject: [PATCH] fix(plugins) handle CG plugins like consumers (#6132) (#6160) 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). Co-authored-by: Travis Raines <571832+rainest@users.noreply.github.com> --- internal/util/relations.go | 53 ++++++++--- internal/util/relations_test.go | 112 +++++++++++++++++++----- test/integration/consumer_group_test.go | 98 +++++++++++++++++++-- 3 files changed, 223 insertions(+), 40 deletions(-) diff --git a/internal/util/relations.go b/internal/util/relations.go index a0f9fc0cd5..bc923d8c10 100644 --- a/internal/util/relations.go +++ b/internal/util/relations.go @@ -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}) } diff --git a/internal/util/relations_test.go b/internal/util/relations_test.go index 1f9dbff416..1e76640d19 100644 --- a/internal/util/relations_test.go +++ b/internal/util/relations_test.go @@ -1,8 +1,9 @@ package util import ( - "reflect" "testing" + + "github.com/stretchr/testify/require" ) func TestGetCombinations(t *testing.T) { @@ -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", @@ -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", @@ -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 + } + }) +} diff --git a/test/integration/consumer_group_test.go b/test/integration/consumer_group_test.go index 2749971112..940f3ed247 100644 --- a/test/integration/consumer_group_test.go +++ b/test/integration/consumer_group_test.go @@ -12,6 +12,7 @@ import ( "github.com/kong/go-kong/kong" "github.com/kong/kubernetes-testing-framework/pkg/clusters" "github.com/kong/kubernetes-testing-framework/pkg/utils/kubernetes/generators" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" @@ -36,11 +37,16 @@ func TestConsumerGroup(t *testing.T) { ctx := context.Background() ns, cleaner := helpers.Setup(ctx, t, env) - d, s, i, p := deployMinimalSvcWithKeyAuth(ctx, t, ns.Name) - cleaner.Add(d) - cleaner.Add(s) - cleaner.Add(i) - cleaner.Add(p) + // path is the basic path used for most of the test + path := "/test-consumer-group/basic" + // multiPath is the path used to test consumer group + route plugins + multiPath := "/test-consumer-group/multi" + + deployment, service, ingress, keyauthPlugin := deployMinimalSvcWithKeyAuth(ctx, t, ns.Name, path) + cleaner.Add(deployment) + cleaner.Add(service) + cleaner.Add(ingress) + cleaner.Add(keyauthPlugin) addedHeader := header{ K: "X-Test-Header", @@ -89,6 +95,15 @@ func TestConsumerGroup(t *testing.T) { ctx, t, ns.Name, "test-consumer-group-2", pluginRateLimit.Name, ) cleaner.Add(rateLimitGroup) + // 3 has consumers but no plugins + nothingGroup := configureConsumerGroupWithPlugins( + ctx, t, ns.Name, "test-consumer-group-3", + ) + cleaner.Add(nothingGroup) + addHeaderRouteGroup := configureConsumerGroupWithPlugins( + ctx, t, ns.Name, "test-consumer-group-4", pluginRespTrans.Name, + ) + cleaner.Add(addHeaderRouteGroup) rateLimitHeader := header{ K: "RateLimit-Limit", @@ -130,7 +145,7 @@ func TestConsumerGroup(t *testing.T) { t.Log("checking if consumer has plugin configured correctly based on consumer group membership") for _, consumer := range consumers { require.Eventually(t, func() bool { - req := helpers.MustHTTPRequest(t, http.MethodGet, proxyURL.Host, "/", map[string]string{ + req := helpers.MustHTTPRequest(t, http.MethodGet, proxyURL.Host, path, map[string]string{ "apikey": consumer.Name, }) resp, err := helpers.DefaultHTTPClientWithProxy(proxyURL).Do(req) @@ -158,10 +173,77 @@ func TestConsumerGroup(t *testing.T) { return true }, ingressWait, waitTick) } + + t.Log("checking plugins attached to a consumer group and route only apply when request matches both") + four, fourSecret := configureConsumerWithAPIKey(ctx, t, ns.Name, "test-consumer-4", "test-consumer-group-4") + cleaner.Add(four) + cleaner.Add(fourSecret) + + multiIngress := generators.NewIngressForService(multiPath, map[string]string{ + annotations.AnnotationPrefix + annotations.StripPathKey: "true", + annotations.AnnotationPrefix + annotations.PluginsKey: strings.Join([]string{keyauthPlugin.Name, pluginRespTrans.Name}, ","), + }, service) + multiIngress.Spec.IngressClassName = kong.String(consts.IngressClass) + multiIngress.Name = "multi" + require.NoError(t, clusters.DeployIngress(ctx, env.Cluster(), ns.Name, multiIngress)) + cleaner.Add(multiIngress) + + require.EventuallyWithT(t, func(c *assert.CollectT) { + // this should see the header, it uses a consumer in the group on the associated route + req := helpers.MustHTTPRequest(t, http.MethodGet, proxyURL.Host, multiPath, map[string]string{ + "apikey": four.Name, + }) + resp, err := helpers.DefaultHTTPClientWithProxy(proxyURL).Do(req) + if !assert.NoError(c, err) { + return + } + defer resp.Body.Close() + if !assert.Equal(c, resp.StatusCode, http.StatusOK) { + return + } + hv := resp.Header.Get(addedHeader.K) + if !assert.Equal(c, addedHeader.V, hv) { + return + } + + // this should not see the header, it uses a consumer in the group on another route + clear := helpers.MustHTTPRequest(t, http.MethodGet, proxyURL.Host, path, map[string]string{ + "apikey": four.Name, + }) + clearResp, err := helpers.DefaultHTTPClientWithProxy(proxyURL).Do(clear) + if !assert.NoError(c, err) { + return + } + defer clearResp.Body.Close() + if !assert.Equal(c, clearResp.StatusCode, http.StatusOK) { + return + } + hv = clearResp.Header.Get(addedHeader.K) + if !assert.NotEqual(c, addedHeader.V, hv) { + return + } + + // this should not see the header, it uses a consumer outside the group on the associated route + empty := helpers.MustHTTPRequest(t, http.MethodGet, proxyURL.Host, multiPath, map[string]string{ + "apikey": "test-consumer-3", + }) + emptyResp, err := helpers.DefaultHTTPClientWithProxy(proxyURL).Do(empty) + if !assert.NoError(c, err) { + return + } + defer emptyResp.Body.Close() + if !assert.Equal(c, emptyResp.StatusCode, http.StatusOK) { + return + } + hv = emptyResp.Header.Get(addedHeader.K) + if !assert.NotEqual(c, addedHeader.V, hv) { + return + } + }, ingressWait, waitTick) } func deployMinimalSvcWithKeyAuth( - ctx context.Context, t *testing.T, namespace string, + ctx context.Context, t *testing.T, namespace, path string, ) (*appsv1.Deployment, *corev1.Service, *netv1.Ingress, *kongv1.KongPlugin) { const pluginKeyAuthName = "key-auth" t.Logf("configuring plugin %q (to give consumers an identity)", pluginKeyAuthName) @@ -194,7 +276,7 @@ func deployMinimalSvcWithKeyAuth( require.NoError(t, err) t.Logf("creating an ingress for service %q with plugin %q attached", service.Name, pluginKeyAuthName) - ingress := generators.NewIngressForService("/", map[string]string{ + ingress := generators.NewIngressForService(path, map[string]string{ annotations.AnnotationPrefix + annotations.StripPathKey: "true", annotations.AnnotationPrefix + annotations.PluginsKey: pluginKeyAuthName, }, service)