[release/2.12] fix: provide descriptive error for known bug with conf…

…licting Kong Routes naming (#5205) Co-authored-by: Travis Raines <[email protected]> Co-authored-by: Grzegorz Burzyński <[email protected]>
Kong · Nov 21, 2023 · c29db3e · c29db3e
1 parent c44af94
commit c29db3e
Show file tree

Hide file tree

Showing 3 changed files with 112 additions and 2 deletions.
diff --git a/internal/dataplane/parser/translate_ingress.go b/internal/dataplane/parser/translate_ingress.go
@@ -7,6 +7,7 @@ import (
 
 	"github.com/kong/go-kong/kong"
 	netv1 "k8s.io/api/networking/v1"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/kong/kubernetes-ingress-controller/v2/internal/dataplane/failures"
 	"github.com/kong/kubernetes-ingress-controller/v2/internal/dataplane/kongstate"
@@ -137,6 +138,7 @@ func ingressV1ToKongServiceLegacy(
 	for _, ingress := range ingresses {
 		ingressSpec := ingress.Spec
 		maybePrependRegexPrefixFn := translators.MaybePrependRegexPrefixForIngressV1Fn(ingress, icp.EnableLegacyRegexDetection && featureFlags.RegexPathPrefix)
+		routeName := routeNamer(failuresCollector, ingress)
 		for i, rule := range ingressSpec.Rules {
 			if rule.HTTP == nil {
 				continue
@@ -162,7 +164,7 @@ func ingressV1ToKongServiceLegacy(
 				r := kongstate.Route{
 					Ingress: util.FromK8sObject(ingress),
 					Route: kong.Route{
-						Name:              kong.String(fmt.Sprintf("%s.%s.%d%d", ingress.Namespace, ingress.Name, i, j)),
+						Name:              routeName(i, j),
 						Paths:             paths,
 						StripPath:         kong.Bool(false),
 						PreserveHost:      kong.Bool(true),
@@ -222,6 +224,28 @@ func ingressV1ToKongServiceLegacy(
 	return servicesCache
 }
 
+// routeNamer returns function for generating a name for a Kong Route based on the Ingress name, namespace, rule index and path index.
+// If the name won't be unique, it registers a failure.
+func routeNamer(failuresCollector *failures.ResourceFailuresCollector, objIngress client.Object) func(ruleIndex, pathIndex int) *string {
+	uniqueRouteNames := make(map[string]struct{})
+	return func(ruleIndex, pathIndex int) *string {
+		routeName := kong.String(fmt.Sprintf("%s.%s.%d%d", objIngress.GetNamespace(), objIngress.GetName(), ruleIndex, pathIndex))
+		if _, conflicting := uniqueRouteNames[*routeName]; conflicting {
+			failuresCollector.PushResourceFailure(
+				fmt.Sprint(
+					"Kong route with conflicting name: ", *routeName, " ",
+					"use feature gate CombinedRoutes=true ",
+					"or update Kong Kubernetes Ingress Controller version to 3.0.0 or above ",
+					"(both remediation changes naming schema of Kong routes)",
+				),
+				objIngress,
+			)
+		}
+		uniqueRouteNames[*routeName] = struct{}{}
+		return routeName
+	}
+}
+
 // getDefaultBackendService picks the oldest Ingress with a DefaultBackend defined and returns a Kong Service for it.
 func getDefaultBackendService(allDefaultBackends []netv1.Ingress, expressionRoutes bool) (kongstate.Service, bool) {
 	sort.SliceStable(allDefaultBackends, func(i, j int) bool {

diff --git a/internal/dataplane/parser/translate_ingress_test.go b/internal/dataplane/parser/translate_ingress_test.go
@@ -1,6 +1,7 @@
 package parser
 
 import (
+	"fmt"
 	"testing"
 	"time"
 
@@ -13,6 +14,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/kong/kubernetes-ingress-controller/v2/internal/annotations"
+	"github.com/kong/kubernetes-ingress-controller/v2/internal/dataplane/failures"
 	"github.com/kong/kubernetes-ingress-controller/v2/internal/dataplane/kongstate"
 	"github.com/kong/kubernetes-ingress-controller/v2/internal/dataplane/parser/translators"
 	"github.com/kong/kubernetes-ingress-controller/v2/internal/store"
@@ -272,3 +274,86 @@ func TestRewriteURIAnnotation(t *testing.T) {
 		}
 	})
 }
+
+func TestIngressGeneratingKongRoutesWithConflictingNames(t *testing.T) {
+	createIngress := func(noRules, noPaths int) *netv1.Ingress {
+		var ingresRules []netv1.IngressRule
+		for i := 0; i <= noRules; i++ {
+			var paths []netv1.HTTPIngressPath
+			for j := 0; j <= noPaths; j++ {
+				paths = append(paths, netv1.HTTPIngressPath{
+					Path:     "/~/api/(.*)",
+					PathType: lo.ToPtr(netv1.PathTypePrefix),
+					Backend: netv1.IngressBackend{
+						Service: &netv1.IngressServiceBackend{
+							Name: "foo-svc",
+							Port: netv1.ServiceBackendPort{Number: 80},
+						},
+					},
+				},
+				)
+			}
+			ingresRules = append(ingresRules, netv1.IngressRule{
+				Host: "example.com",
+				IngressRuleValue: netv1.IngressRuleValue{
+					HTTP: &netv1.HTTPIngressRuleValue{
+						Paths: paths,
+					},
+				},
+			})
+		}
+		return &netv1.Ingress{
+			TypeMeta: metav1.TypeMeta{Kind: "Ingress"},
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      fmt.Sprintf("conflicting-one-%d-%d", noRules, noPaths),
+				Namespace: "foo-namespace",
+				Annotations: map[string]string{
+					annotations.IngressClassKey: annotations.DefaultIngressClass,
+				},
+			},
+			Spec: netv1.IngressSpec{
+				Rules: ingresRules,
+			},
+		}
+	}
+	s, err := store.NewFakeStore(store.FakeObjects{
+		IngressesV1: []*netv1.Ingress{
+			createIngress(1, 1),
+			createIngress(7, 11),
+			createIngress(11, 5),
+			// The below generates Kong Routes that are conflicting with each other.
+			createIngress(11, 11),
+			createIngress(22, 23),
+		},
+	})
+	require.NoError(t, err)
+	p := mustNewParser(t, s)
+
+	t.Run("more than 11 rules and paths in Ingress and CombinedRoutes=false warns about conflicts", func(t *testing.T) {
+		p.featureFlags.CombinedServiceRoutes = false
+		_ = p.ingressRulesFromIngressV1()
+		errs := p.failuresCollector.PopResourceFailures()
+		require.Len(t, errs, 30)
+		errMsgs := lo.Map(errs, func(err failures.ResourceFailure, _ int) string {
+			return err.Message()
+		})
+
+		// Just check couple of them, other follows the same pattern.
+		const expectedErrMsg = "Kong route with conflicting name: %s use feature gate CombinedRoutes=true or update Kong Kubernetes Ingress Controller version to 3.0.0 or above (both remediation changes naming schema of Kong routes)"
+		for _, conflictingName := range []string{
+			"foo-namespace.conflicting-one-11-11.110",
+			"foo-namespace.conflicting-one-11-11.111",
+			"foo-namespace.conflicting-one-22-23.114",
+			"foo-namespace.conflicting-one-22-23.222",
+		} {
+			require.Contains(t, errMsgs, fmt.Sprintf(expectedErrMsg, conflictingName))
+		}
+	})
+
+	t.Run("more than 11 rules and paths in Ingress and CombinedRoutes=true gives no conflicts and no warnings", func(t *testing.T) {
+		p.featureFlags.CombinedServiceRoutes = true
+		_ = p.ingressRulesFromIngressV1()
+		errs := p.failuresCollector.PopResourceFailures()
+		require.Len(t, errs, 0)
+	})
+}
diff --git a/internal/dataplane/parser/translate_knative.go b/internal/dataplane/parser/translate_knative.go
@@ -57,6 +57,7 @@ func (p *Parser) ingressRulesFromKnativeIngress() ingressRules {
 
 		secretToSNIs.addFromIngressV1TLS(knativeIngressToNetworkingTLS(ingress.Spec.TLS), ingress)
 
+		routeName := routeNamer(p.failuresCollector, ingress)
 		var objectSuccessfullyParsed bool
 		for i, rule := range ingressSpec.Rules {
 			hosts := rule.Hosts
@@ -73,7 +74,7 @@ func (p *Parser) ingressRulesFromKnativeIngress() ingressRules {
 				r := kongstate.Route{
 					Ingress: util.FromK8sObject(ingress),
 					Route: kong.Route{
-						Name:              kong.String(fmt.Sprintf("%s.%s.%d%d", ingress.Namespace, ingress.Name, i, j)),
+						Name:              routeName(i, j),
 						Paths:             kong.StringSlice(path),
 						StripPath:         kong.Bool(false),
 						PreserveHost:      kong.Bool(true),