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 <[email protected]>

Author: pmalek

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})
 		}

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
+		}
+	})
+}

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)