[backport release/2.12.x] fix: annotation konghq.com/rewrite for mult…

…iple Ingresses routing to the same Service (#5171) (#5215)

internal/dataplane/parser/translate_ingress_test.go

@@ -223,16 +223,18 @@ func TestRewriteURIAnnotation(t *testing.T) {
 		require.Len(t, rules, 1)
 
 		for _, svc := range rules {
-			require.Equal(t, []kong.Plugin{
-				{
-					Name: kong.String("request-transformer"),
-					Config: kong.Configuration{
-						"replace": map[string]string{
-							"uri": "/rewrite/$(uri_captures[1])/xx",
+			for _, route := range svc.Routes {
+				require.Equal(t, []kong.Plugin{
+					{
+						Name: kong.String("request-transformer"),
+						Config: kong.Configuration{
+							"replace": map[string]string{
+								"uri": "/rewrite/$(uri_captures[1])/xx",
+							},
 						},
 					},
-				},
-			}, svc.Plugins)
+				}, route.Plugins)
+			}
 		}
 
 		errs := p.failuresCollector.PopResourceFailures()

internal/dataplane/parser/translators/ingress.go

@@ -470,34 +470,37 @@ func generateRewriteURIConfig(uri string) (string, error) {
 	return out.String(), nil
 }
 
-// MaybeRewriteURI appends a request-transformer plugin if the value of konghq.com/rewrite annotation is valid.
+// MaybeRewriteURI appends a request-transformer plugin to Kong routes based on
+// the value of konghq.com/rewrite annotation configured on related K8s Ingresses.
 func MaybeRewriteURI(service *kongstate.Service, rewriteURIEnable bool) error {
-	rewriteURI, exists := annotations.ExtractRewriteURI(service.Parent.GetAnnotations())
-	if !exists {
-		return nil
-	}
-
-	if !rewriteURIEnable {
-		return fmt.Errorf("konghq.com/rewrite annotation not supported when rewrite uris disabled")
-	}
+	for i := range service.Routes {
+		route := &service.Routes[i]
 
-	if rewriteURI == "" {
-		rewriteURI = "/"
-	}
+		rewriteURI, exists := annotations.ExtractRewriteURI(route.Ingress.Annotations)
+		if !exists {
+			return nil
+		}
+		if !rewriteURIEnable {
+			return fmt.Errorf("konghq.com/rewrite annotation not supported when rewrite uris disabled")
+		}
 
-	config, err := generateRewriteURIConfig(rewriteURI)
-	if err != nil {
-		return err
-	}
+		if rewriteURI == "" {
+			rewriteURI = "/"
+		}
 
-	service.Plugins = append(service.Plugins, kong.Plugin{
-		Name: kong.String("request-transformer"),
-		Config: kong.Configuration{
-			"replace": map[string]string{
-				"uri": config,
+		config, err := generateRewriteURIConfig(rewriteURI)
+		if err != nil {
+			return err
+		}
+		route.Plugins = append(route.Plugins, kong.Plugin{
+			Name: kong.String("request-transformer"),
+			Config: kong.Configuration{
+				"replace": map[string]string{
+					"uri": config,
+				},
 			},
-		},
-	})
+		})
 
+	}
 	return nil
 }

internal/dataplane/parser/translators/ingress_test.go

@@ -1755,12 +1755,14 @@ func TestMaybeRewriteURI(t *testing.T) {
 		},
 	}
 
-	for i := range testCases {
-		tc := testCases[i]
+	for _, tc := range testCases {
+		tc := tc
 		t.Run(tc.name, func(t *testing.T) {
 			err := MaybeRewriteURI(&tc.service, true)
 			require.Equal(t, tc.expectedError, err)
-			require.Equal(t, tc.expectedPlugins, tc.service.Plugins)
+			for _, route := range tc.service.Routes {
+				require.Equal(t, tc.expectedPlugins, route.Plugins)
+			}
 		})
 	}
 }

test/integration/ingress_test.go

@@ -24,6 +24,7 @@ import (
 	"k8s.io/client-go/util/retry"
 
 	"github.com/kong/kubernetes-ingress-controller/v2/internal/annotations"
+	"github.com/kong/kubernetes-ingress-controller/v2/internal/manager/featuregates"
 	"github.com/kong/kubernetes-ingress-controller/v2/internal/versions"
 	kongv1alpha1 "github.com/kong/kubernetes-ingress-controller/v2/pkg/apis/configuration/v1alpha1"
 	"github.com/kong/kubernetes-ingress-controller/v2/pkg/clientset"
@@ -1163,13 +1164,16 @@ func TestIngressWorksWithServiceBackendsSpecifyingOnlyPortNames(t *testing.T) {
 }
 
 func TestIngressRewriteURI(t *testing.T) {
-	pathImplementationSpecific := netv1.PathTypeImplementationSpecific
+	const (
+		jpegMagicNumber = "\xff\xd8\xff\xe0\x00\x10JFIF"
+		pngMagicNumber  = "\x89PNG\r\n\x1a\n"
+	)
 
 	ctx := context.Background()
 
 	ns, cleaner := helpers.Setup(ctx, t, env)
 
-	t.Log("deploying a minimal HTTP container deployment to test Ingress routes")
+	t.Log("deploying a minimal HTTP Bin container deployment to test Ingress routes")
 	container := generators.NewContainer("httpbin", test.HTTPBinImage, test.HTTPBinPort)
 	deployment := generators.NewDeploymentForContainer(container)
 	deployment, err := env.Cluster().Client().AppsV1().Deployments(ns.Name).Create(ctx, deployment, metav1.CreateOptions{})
@@ -1182,102 +1186,140 @@ func TestIngressRewriteURI(t *testing.T) {
 	require.NoError(t, err)
 	cleaner.Add(service)
 
-	t.Logf("creating an ingress for service %s with rewrite annotation", service.Name)
-	ingress := generators.NewIngressForService("/~/foo/(.*)", map[string]string{
-		"konghq.com/strip-path": "true",
-		"konghq.com/rewrite":    "/image/$1",
+	t.Logf("creating an Ingress for service %s without rewrite annotation", service.Name)
+	const serviceDomainDirect = "direct.example"
+	ingressDirect := generators.NewIngressForService("/", map[string]string{
+		annotations.AnnotationPrefix + annotations.StripPathKey: "true",
 	}, service)
-	ingress.Spec.IngressClassName = kong.String(consts.IngressClass)
-	for i := range ingress.Spec.Rules {
-		ingress.Spec.Rules[i].Host = "test.example"
-		ingress.Spec.Rules[i].HTTP.Paths[0].PathType = &pathImplementationSpecific
+	ingressDirect.Name += "-direct"
+	ingressDirect.Spec.IngressClassName = kong.String(consts.IngressClass)
+	for i := range ingressDirect.Spec.Rules {
+		ingressDirect.Spec.Rules[i].Host = serviceDomainDirect
+		ingressDirect.Spec.Rules[i].HTTP.Paths[0].PathType = lo.ToPtr(netv1.PathTypePrefix)
 	}
-	ingress, err = env.Cluster().Client().NetworkingV1().Ingresses(ns.Name).Create(ctx, ingress, metav1.CreateOptions{})
+	ingressDirect, err = env.Cluster().Client().NetworkingV1().Ingresses(ns.Name).Create(ctx, ingressDirect, metav1.CreateOptions{})
 	require.NoError(t, err)
-	cleaner.Add(ingress)
+	cleaner.Add(ingressDirect)
+
+	// There is no programmed condition for Ingress to check. Hence to avoid 503 and 404 that are result of Ingress not being configured yet,
+	// wait for first successful response. After it all subsequent must be successful too.
+	t.Log("wait for the Ingress direct to become available")
+	const path = "image/jpeg"
+	helpers.EventuallyGETPath(t, proxyURL, path, http.StatusOK, jpegMagicNumber, setHostHeader(serviceDomainDirect), ingressWait, waitTick)
+
+	waitForMainTestToFinish, cancelBackgroundTest := context.WithCancel(ctx)
+	backgroundTestError := make(chan error)
+	go func() {
+		t.Log("check constantly in the background that Ingress without rewrite annotation is not affected by Ingress with rewrite annotation")
+		var cntOK, cntAttempts int
+		unexpectedErrs := make(map[string]int)
+		for {
+			select {
+			case <-waitForMainTestToFinish.Done():
+				if cntAttempts == 0 {
+					backgroundTestError <- fmt.Errorf("no requests were made to Ingress without rewrite")
+					return
+				}
+				if ratio := float64(cntOK) / float64(cntAttempts); ratio >= 0.99 {
+					backgroundTestError <- nil
+				} else {
+					backgroundTestError <- fmt.Errorf(
+						"expected >= 99%% of requests to be successful, current rate is %f %%, unexpected status codes: %v",
+						ratio*100, unexpectedErrs,
+					)
+				}
+				return
+			case <-time.After(50 * time.Millisecond):
+			}
+			cntAttempts++
+			resp, err := helpers.DefaultHTTPClient().Do(helpers.MustHTTPRequest(t, http.MethodGet, proxyURL, path, setHostHeader(serviceDomainDirect)))
+			if err != nil {
+				t.Logf("WARNING: Ingress without rewrite - http request failed for GET %s/%s to %s: %v", serviceDomainDirect, path, proxyURL, err)
+				continue
+			}
+			if resp.StatusCode == http.StatusOK {
+				var b bytes.Buffer
+				if _, err := b.ReadFrom(resp.Body); err == nil {
+					if strings.HasPrefix(b.String(), jpegMagicNumber) {
+						cntOK++
+					} else {
+						t.Log("WARNING: Ingress without rewrite - response body is not a JPEG image")
+						unexpectedErrs["response no JPEG"]++
+					}
+				} else {
+					t.Log("WARNING: Ingress without rewrite - failed to read response body:", err)
+					unexpectedErrs["cannot read response body"]++
+				}
+			} else {
+				t.Log("WARNING: Ingress without rewrite - response status code is:", resp.StatusCode)
+				unexpectedErrs[fmt.Sprintf("status code: %d", resp.StatusCode)]++
+			}
+			resp.Body.Close()
+		}
+	}()
 
-	featureGates := testenv.ControllerFeatureGates()
-	if !strings.Contains(featureGates, "RewriteURIsFeature=true") {
-		t.Log("try to access the ingress with rewrite uri disabled")
-		req := helpers.MustHTTPRequest(t, http.MethodGet, proxyURL, "/foo/jpeg", nil)
-		req.Host = "test.example"
-		resp, err := helpers.DefaultHTTPClient().Do(req)
-		require.NoError(t, err)
-		defer resp.Body.Close()
-		require.Equal(t, resp.StatusCode, http.StatusNotFound)
+	t.Logf("creating an Ingress for service %s with rewrite annotation", service.Name)
+	const serviceDomainRewrite = "rewrite.example"
+	ingressRewrite := generators.NewIngressForService("/~/foo/(.*)", map[string]string{
+		annotations.AnnotationPrefix + annotations.StripPathKey:  "true",
+		annotations.AnnotationPrefix + annotations.RewriteURIKey: "/image/$1",
+	}, service)
+	ingressRewrite.Name += "-rewrite"
+	ingressRewrite.Spec.IngressClassName = kong.String(consts.IngressClass)
+	for i := range ingressRewrite.Spec.Rules {
+		ingressRewrite.Spec.Rules[i].Host = serviceDomainRewrite
+		ingressRewrite.Spec.Rules[i].HTTP.Paths[0].PathType = lo.ToPtr(netv1.PathTypeImplementationSpecific)
+	}
+	ingressRewrite, err = env.Cluster().Client().NetworkingV1().Ingresses(ns.Name).Create(ctx, ingressRewrite, metav1.CreateOptions{})
+	require.NoError(t, err)
+	cleaner.Add(ingressRewrite)
 
+	if !strings.Contains(testenv.ControllerFeatureGates(), featuregates.RewriteURIsFeature) {
+		t.Log("rewrite uri feature is disabled")
+		t.Log("try to access the ingress with rewrite uri disabled")
+		helpers.EventuallyGETPath(t, proxyURL, "/foo/jpeg", http.StatusNotFound, "", setHostHeader(serviceDomainRewrite), ingressWait, waitTick)
+		cancelBackgroundTest()
+		require.NoError(t, <-backgroundTestError, "for Ingress without rewrite run in background")
 		return
 	}
+	t.Log("rewrite uri feature is enabled")
 
 	t.Log("try to access the ingress with valid capture group")
-	req := helpers.MustHTTPRequest(t, http.MethodGet, proxyURL, "/foo/jpeg", nil)
-	req.Host = "test.example"
-	require.Eventually(t, func() bool {
-		resp, err := helpers.DefaultHTTPClient().Do(req)
-		if err != nil {
-			t.Logf("WARNING: error while waiting for %s: %v", proxyURL, err)
-			return false
-		}
-		defer resp.Body.Close()
-		if resp.StatusCode == http.StatusOK {
-			return resp.Header.Get("Content-Type") == "image/jpeg"
-		}
-		return false
-	}, ingressWait, waitTick)
+	helpers.EventuallyGETPath(t, proxyURL, "/foo/jpeg", http.StatusOK, jpegMagicNumber, setHostHeader(serviceDomainRewrite), ingressWait, waitTick)
 
 	t.Log("try to access the ingress with invalid capture group, should return 404")
-	req = helpers.MustHTTPRequest(t, http.MethodGet, proxyURL, "/", nil)
-	req.Host = "test.example"
-	resp, err := helpers.DefaultHTTPClient().Do(req)
-	require.NoError(t, err)
-	defer resp.Body.Close()
-	require.Equal(t, resp.StatusCode, http.StatusNotFound)
+	helpers.EventuallyGETPath(t, proxyURL, "/", http.StatusNotFound, "", setHostHeader(serviceDomainRewrite), ingressWait, waitTick)
 
-	ingress, err = env.Cluster().Client().NetworkingV1().Ingresses(ns.Name).Get(ctx, ingress.Name, metav1.GetOptions{})
+	ingressRewrite, err = env.Cluster().Client().NetworkingV1().Ingresses(ns.Name).Get(ctx, ingressRewrite.Name, metav1.GetOptions{})
 	require.NoError(t, err)
 	t.Log("update the ingress capture group")
-	for i := range ingress.Spec.Rules {
-		ingress.Spec.Rules[i].HTTP.Paths[0].Path = "/~/foo/(\\w+)/(.*)"
+	for i := range ingressRewrite.Spec.Rules {
+		ingressRewrite.Spec.Rules[i].HTTP.Paths[0].Path = "/~/foo/(\\w+)/(.*)"
 	}
 
-	_, err = env.Cluster().Client().NetworkingV1().Ingresses(ns.Name).Update(ctx, ingress, metav1.UpdateOptions{})
+	_, err = env.Cluster().Client().NetworkingV1().Ingresses(ns.Name).Update(ctx, ingressRewrite, metav1.UpdateOptions{})
 	require.NoError(t, err)
 
 	t.Log("try to access the ingress with new valid capture group")
-	req = helpers.MustHTTPRequest(t, http.MethodGet, proxyURL, "/foo/jpeg/png", nil)
-	req.Host = "test.example"
-	require.Eventually(t, func() bool {
-		resp, err := helpers.DefaultHTTPClient().Do(req)
-		if err != nil {
-			t.Logf("WARNING: error while waiting for %s: %v", proxyURL, err)
-			return false
-		}
-		defer resp.Body.Close()
-		if resp.StatusCode == http.StatusOK {
-			return resp.Header.Get("Content-Type") == "image/jpeg"
-		}
-		return false
-	}, ingressWait, waitTick)
+	helpers.EventuallyGETPath(t, proxyURL, "/foo/jpeg", http.StatusOK, jpegMagicNumber, setHostHeader(serviceDomainRewrite), ingressWait, waitTick)
 
-	ingress, err = env.Cluster().Client().NetworkingV1().Ingresses(ns.Name).Get(ctx, ingress.Name, metav1.GetOptions{})
+	ingressRewrite, err = env.Cluster().Client().NetworkingV1().Ingresses(ns.Name).Get(ctx, ingressRewrite.Name, metav1.GetOptions{})
 	require.NoError(t, err)
 	t.Log("update the ingress rewrite annotation")
-	ingress.Annotations["konghq.com/rewrite"] = "/image/$2"
+	ingressRewrite.Annotations[annotations.AnnotationPrefix+annotations.RewriteURIKey] = "/image/$2"
 
-	_, err = env.Cluster().Client().NetworkingV1().Ingresses(ns.Name).Update(ctx, ingress, metav1.UpdateOptions{})
+	_, err = env.Cluster().Client().NetworkingV1().Ingresses(ns.Name).Update(ctx, ingressRewrite, metav1.UpdateOptions{})
 	require.NoError(t, err)
 
 	t.Log("try to access the ingress with new rewrite annotation")
-	require.Eventually(t, func() bool {
-		resp, err := helpers.DefaultHTTPClient().Do(req)
-		if err != nil {
-			t.Logf("WARNING: error while waiting for %s: %v", proxyURL, err)
-			return false
-		}
-		defer resp.Body.Close()
-		if resp.StatusCode == http.StatusOK {
-			return resp.Header.Get("Content-Type") == "image/png"
-		}
-		return false
-	}, ingressWait, waitTick)
+	helpers.EventuallyGETPath(t, proxyURL, "/foo/test/png", http.StatusOK, pngMagicNumber, setHostHeader(serviceDomainRewrite), ingressWait, waitTick)
+
+	cancelBackgroundTest()
+	require.NoError(t, <-backgroundTestError, "for Ingress without rewrite run in background")
+}
+
+func setHostHeader(hostname string) map[string]string {
+	return map[string]string{
+		"Host": hostname,
+	}
 }

test/internal/helpers/http.go

@@ -43,6 +43,10 @@ func MustHTTPRequest(t *testing.T, method string, proxyURL *url.URL, path string
 	req, err := http.NewRequest(method, fmt.Sprintf("%s/%s", host, path), nil)
 	require.NoError(t, err)
 	for header, value := range headers {
+		if header == "Host" {
+			req.Host = value
+			continue
+		}
 		req.Header.Set(header, value)
 	}
 	return req