Repository: Kong/kubernetes-ingress-controller
Pull request #6786 (2024-12-05 15:41:24)

ci: fix assigning duplicated instance names to plugins bound to different entity types (e.g. route and consumer group)
**What this PR does / why we need it**:

#4588 introduced assigning instance names to plugins and handling collisions in generates names.

This missed the fact that plugins can be bound to multiple entities, e.g. routes **and** consumer groups.

This PR fixes it.

**Which issue this PR fixes**:

<!--
Here you can add any links to issues that this PR is relevant for.
You can use Github keywords (like: closes, fixes or resolves) to auto-resolve
the linked issue(s) when this PR gets merged.

For example: fixes #<issue number>
-->

**Special notes for your reviewer**:

<!-- Here you can add any open questions or notes that you might have for reviewers -->

**PR Readiness Checklist**:

Complete these before marking the PR as `ready to review`:

- [x] the `CHANGELOG.md` release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR



Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index f42cb9fd0..7c4dfb5f4 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -168,6 +168,11 @@ Adding a new version? You'll need three changes:
 - Fixed an issue where `KongPlugin` used as `HTTPRoute`'s `ExtensionRef` filter
   would produce an invalid configuration.
   [#6762](https://github.com/Kong/kubernetes-ingress-controller/pull/6762)
+- Fixed an issue of assigning duplicated instance names to plugins where
+  `KongPlugin`s would be bound to multiple entities, e.g.:
+  - route R1 and consumer group CG
+  - route R2 and consumer group CG
+  [#6786](https://github.com/Kong/kubernetes-ingress-controller/pull/6786)
 
 ### Added
 
diff --git a/internal/dataplane/kongstate/kongstate.go b/internal/dataplane/kongstate/kongstate.go
index 5f227b542..dfc3ab61c 100644
--- a/internal/dataplane/kongstate/kongstate.go
+++ b/internal/dataplane/kongstate/kongstate.go
@@ -1,6 +1,7 @@
 package kongstate
 
 import (
+	"bytes"
 	"crypto/sha256"
 	"fmt"
 	"strconv"
@@ -574,28 +575,32 @@ func buildPlugins(
 		usedInstanceNames := sets.New[string]()
 		for _, rel := range relations.GetCombinations() {
 			plugin := plugin.DeepCopy()
-			var sha [32]byte
-			// ID is populated because that is read by decK and in_memory
-			// translator too
+
+			var toHash bytes.Buffer
+
+			// ID is populated because that is read by decK and in_memory translator too
 			if rel.Service != "" {
 				plugin.Service = &kong.Service{ID: kong.String(rel.Service)}
-				sha = sha256.Sum256([]byte("service-" + rel.Service))
+				toHash.WriteString("_service-" + rel.Service)
 			}
 			if rel.Route != "" {
 				plugin.Route = &kong.Route{ID: kong.String(rel.Route)}
-				sha = sha256.Sum256([]byte("route-" + rel.Route))
+				toHash.WriteString("_route-" + rel.Route)
 			}
 			if rel.Consumer != "" {
 				plugin.Consumer = &kong.Consumer{ID: kong.String(rel.Consumer)}
-				sha = sha256.Sum256([]byte("consumer-" + rel.Consumer))
+				toHash.WriteString("_consumer-" + rel.Consumer)
 			}
 			if rel.ConsumerGroup != "" {
 				plugin.ConsumerGroup = &kong.ConsumerGroup{ID: kong.String(rel.ConsumerGroup)}
-				sha = sha256.Sum256([]byte("group-" + rel.ConsumerGroup))
+				toHash.WriteString("_group-" + rel.ConsumerGroup)
 			}
+
 			// instance_name must be unique. Using the same KongPlugin on multiple resources will result in duplicates
 			// unless we add some sort of suffix.
 			if plugin.InstanceName != nil {
+
+				sha := sha256.Sum256(toHash.Bytes())
 				suffix := fmt.Sprintf("%x", sha)
 				short := suffix[:9]
 				suffixed := fmt.Sprintf("%s-%s", *plugin.InstanceName, short)


Test patch:
diff --git a/internal/dataplane/kongstate/kongstate_test.go b/internal/dataplane/kongstate/kongstate_test.go
index 95a557c5e..2d501bafc 100644
--- a/internal/dataplane/kongstate/kongstate_test.go
+++ b/internal/dataplane/kongstate/kongstate_test.go
@@ -1348,7 +1348,79 @@ func TestKongState_BuildPluginsCollisions(t *testing.T) {
 					Route: []string{"collision", "collision"},
 				},
 			},
-			want: []string{"test-bae3267aa", "test-bae3267aafead3adb6031bc1c732516336e7f7b324baf61bb68a39cc89112741"},
+			want: []string{
+				"test-b1b50e2fe",
+				"test-b1b50e2fea3955bfc13ffed52f9074e50f5056c2173fc05b8ff04388ecd25ffe",
+			},
+		},
+		{
+			name: "binding to route and consumer group",
+			in: []*kongv1.KongPlugin{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "foo-plugin",
+						Namespace: "default",
+					},
+					InstanceName: "test",
+				},
+			},
+			pluginRels: map[string]util.ForeignRelations{
+				"default:foo-plugin": {
+					Route:         []string{"route1", "route2", "route3"},
+					ConsumerGroup: []string{"group1"},
+				},
+			},
+			want: []string{
+				"test-512a42cb1",
+				"test-944f0bd89",
+				"test-c091fbb10",
+			},
+		},
+		{
+			name: "binding to routes and consumer",
+			in: []*kongv1.KongPlugin{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "foo-plugin",
+						Namespace: "default",
+					},
+					InstanceName: "test",
+				},
+			},
+			pluginRels: map[string]util.ForeignRelations{
+				"default:foo-plugin": {
+					Route:    []string{"route1", "route2", "route3"},
+					Consumer: []string{"consumer1"},
+				},
+			},
+			want: []string{
+				"test-ebd486ffe",
+				"test-3aaa2f367",
+				"test-5420f37ef",
+			},
+		},
+		{
+			name: "binding to service and consumer group",
+			in: []*kongv1.KongPlugin{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "foo-plugin",
+						Namespace: "default",
+					},
+					InstanceName: "test",
+				},
+			},
+			pluginRels: map[string]util.ForeignRelations{
+				"default:foo-plugin": {
+					Service:  []string{"service1", "service2", "service3"},
+					Consumer: []string{"consumer1"},
+				},
+			},
+			want: []string{
+				"test-42768cbda",
+				"test-6e9279c81",
+				"test-9bbcb305f",
+			},
 		},
 	} {
 		t.Run(tt.name, func(t *testing.T) {
@@ -1358,8 +1430,12 @@ func TestKongState_BuildPluginsCollisions(t *testing.T) {
 			})
 			// this is not testing the kongPluginFromK8SPlugin failure cases, so there is no failures collector
 			got := buildPlugins(log, store, nil, tt.pluginRels)
-			require.Len(t, got, 2)
-			require.Equal(t, tt.want, []string{*got[0].InstanceName, *got[1].InstanceName})
+			require.Len(t, got, len(tt.want))
+			gotInstanceNames := lo.Map(got, func(p Plugin, _ int) string { return *p.InstanceName })
+			require.Equal(t, tt.want, gotInstanceNames)
+
+			gotUniqueInstanceNames := lo.Uniq(gotInstanceNames)
+			require.Len(t, gotUniqueInstanceNames, len(tt.want), "should only return unique instance names")
 		})
 	}
 }
diff --git a/internal/dataplane/testdata/golden/kongplugin-instance-name/default_golden.yaml b/internal/dataplane/testdata/golden/kongplugin-instance-name/default_golden.yaml
index 06947144f..c4d5160ec 100644
--- a/internal/dataplane/testdata/golden/kongplugin-instance-name/default_golden.yaml
+++ b/internal/dataplane/testdata/golden/kongplugin-instance-name/default_golden.yaml
@@ -2,7 +2,7 @@ _format_version: "3.0"
 plugins:
 - config:
     header_name: kong-id
-  instance_name: example-7d9f28edc
+  instance_name: example-21a1f5b5f
   name: correlation-id
   route: default.httpbin.httpbin..80
   tags:
@@ -13,7 +13,7 @@ plugins:
   - k8s-version:v1
 - config:
     header_name: kong-id
-  instance_name: example-76ac9ccd1
+  instance_name: example-2bc7f3de3
   name: correlation-id
   route: default.httpbin-other.httpbin..80
   tags:
@@ -24,7 +24,7 @@ plugins:
   - k8s-version:v1
 - config:
     header_name: kong-id
-  instance_name: example-b8b9400c0
+  instance_name: example-6c322b5f5
   name: correlation-id
   route: default.httpbin-other.httpbin-other..80
   tags: