more compatiable with nginx's rewrite (alibaba#636)

CH3CHO · Nov 14, 2023 · 88c0386 · 88c0386
1 parent 5174397
commit 88c0386
Show file tree

Hide file tree

Showing 5 changed files with 63 additions and 23 deletions.
diff --git a/pkg/ingress/kube/annotations/annotations.go b/pkg/ingress/kube/annotations/annotations.go
@@ -15,6 +15,8 @@
 package annotations
 
 import (
+	"strings"
+
 	networking "istio.io/api/networking/v1alpha3"
 	"istio.io/istio/pilot/pkg/util/sets"
 	listersv1 "k8s.io/client-go/listers/core/v1"
@@ -73,8 +75,17 @@ type Ingress struct {
 	Http2Rpc *Http2RpcConfig
 }
 
-func (i *Ingress) NeedRegexMatch() bool {
-	return i.Rewrite != nil && (i.IsPrefixRegexMatch() || i.IsFullPathRegexMatch())
+func (i *Ingress) NeedRegexMatch(path string) bool {
+	if i.Rewrite == nil {
+		return false
+	}
+	if strings.ContainsAny(path, `\.+*?()|[]{}^$`) {
+		return true
+	}
+	if strings.ContainsAny(i.Rewrite.RewriteTarget, `$\`) {
+		return true
+	}
+	return i.IsPrefixRegexMatch() || i.IsFullPathRegexMatch()
 }
 
 func (i *Ingress) IsPrefixRegexMatch() bool {

diff --git a/pkg/ingress/kube/annotations/annotations_test.go b/pkg/ingress/kube/annotations/annotations_test.go
@@ -18,8 +18,9 @@ import "testing"
 
 func TestNeedRegexMatch(t *testing.T) {
 	testCases := []struct {
-		input  *Ingress
-		expect bool
+		input     *Ingress
+		inputPath string
+		expect    bool
 	}{
 		{
 			input:  &Ingress{},
@@ -47,12 +48,41 @@ func TestNeedRegexMatch(t *testing.T) {
 			},
 			expect: false,
 		},
+		{
+			input: &Ingress{
+				Rewrite: &RewriteConfig{
+					UseRegex:      false,
+					RewriteTarget: "/$1",
+				},
+			},
+			expect: true,
+		},
+		{
+			input: &Ingress{
+				Rewrite: &RewriteConfig{
+					UseRegex:      false,
+					RewriteTarget: "/",
+				},
+			},
+			inputPath: "/.*",
+			expect:    true,
+		},
+		{
+			input: &Ingress{
+				Rewrite: &RewriteConfig{
+					UseRegex:      false,
+					RewriteTarget: "/",
+				},
+			},
+			inputPath: "/",
+			expect:    false,
+		},
 	}
 
 	for _, testCase := range testCases {
 		t.Run("", func(t *testing.T) {
-			if testCase.input.NeedRegexMatch() != testCase.expect {
-				t.Fatalf("Should be %t, but actual is %t", testCase.expect, testCase.input.NeedRegexMatch())
+			if testCase.input.NeedRegexMatch(testCase.inputPath) != testCase.expect {
+				t.Fatalf("Should be %t, but actual is %t", testCase.expect, !testCase.expect)
 			}
 		})
 	}

diff --git a/pkg/ingress/kube/ingress/controller.go b/pkg/ingress/kube/ingress/controller.go
@@ -535,11 +535,11 @@ func (c *controller) ConvertHTTPRoute(convertOptions *common.ConvertOptions, wra
 
 			var pathType common.PathType
 			originPath := httpPath.Path
-			if annotationsConfig := wrapper.AnnotationsConfig; annotationsConfig.NeedRegexMatch() {
-				if annotationsConfig.IsPrefixRegexMatch() {
-					pathType = common.PrefixRegex
-				} else if annotationsConfig.IsFullPathRegexMatch() {
+			if annotationsConfig := wrapper.AnnotationsConfig; annotationsConfig.NeedRegexMatch(originPath) {
+				if annotationsConfig.IsFullPathRegexMatch() {
 					pathType = common.FullPathRegex
+				} else {
+					pathType = common.PrefixRegex
 				}
 			} else {
 				switch *httpPath.PathType {
@@ -746,11 +746,11 @@ func (c *controller) ApplyCanaryIngress(convertOptions *common.ConvertOptions, w
 
 			var pathType common.PathType
 			originPath := httpPath.Path
-			if annotationsConfig := wrapper.AnnotationsConfig; annotationsConfig.NeedRegexMatch() {
-				if annotationsConfig.IsPrefixRegexMatch() {
-					pathType = common.PrefixRegex
-				} else if annotationsConfig.IsFullPathRegexMatch() {
+			if annotationsConfig := wrapper.AnnotationsConfig; annotationsConfig.NeedRegexMatch(originPath) {
+				if annotationsConfig.IsFullPathRegexMatch() {
 					pathType = common.FullPathRegex
+				} else {
+					pathType = common.PrefixRegex
 				}
 			} else {
 				switch *httpPath.PathType {

diff --git a/pkg/ingress/kube/ingressv1/controller.go b/pkg/ingress/kube/ingressv1/controller.go
@@ -517,11 +517,11 @@ func (c *controller) ConvertHTTPRoute(convertOptions *common.ConvertOptions, wra
 
 			var pathType common.PathType
 			originPath := httpPath.Path
-			if annotationsConfig := wrapper.AnnotationsConfig; annotationsConfig.NeedRegexMatch() {
-				if annotationsConfig.IsPrefixRegexMatch() {
-					pathType = common.PrefixRegex
-				} else if annotationsConfig.IsFullPathRegexMatch() {
+			if annotationsConfig := wrapper.AnnotationsConfig; annotationsConfig.NeedRegexMatch(originPath) {
+				if annotationsConfig.IsFullPathRegexMatch() {
 					pathType = common.FullPathRegex
+				} else {
+					pathType = common.PrefixRegex
 				}
 			} else {
 				switch *httpPath.PathType {
@@ -750,11 +750,11 @@ func (c *controller) ApplyCanaryIngress(convertOptions *common.ConvertOptions, w
 
 			var pathType common.PathType
 			originPath := httpPath.Path
-			if annotationsConfig := wrapper.AnnotationsConfig; annotationsConfig.NeedRegexMatch() {
-				if annotationsConfig.IsPrefixRegexMatch() {
-					pathType = common.PrefixRegex
-				} else if annotationsConfig.IsFullPathRegexMatch() {
+			if annotationsConfig := wrapper.AnnotationsConfig; annotationsConfig.NeedRegexMatch(originPath) {
+				if annotationsConfig.IsFullPathRegexMatch() {
 					pathType = common.FullPathRegex
+				} else {
+					pathType = common.PrefixRegex
 				}
 			} else {
 				switch *httpPath.PathType {

diff --git a/test/e2e/conformance/tests/httproute-rewrite-path.yaml b/test/e2e/conformance/tests/httproute-rewrite-path.yaml
@@ -17,7 +17,6 @@ kind: Ingress
 metadata:
   annotations:
     nginx.ingress.kubernetes.io/rewrite-target: "/$1"
-    nginx.ingress.kubernetes.io/use-regex: "true"
   name: httproute-rewrite-path
   namespace: higress-conformance-infra
 spec: