From 660dd14f34b961174a8d74fd185887d4cd93e8de Mon Sep 17 00:00:00 2001 From: johnlanni Date: Tue, 14 Nov 2023 18:02:47 +0800 Subject: [PATCH 1/2] more compatiable with nginx's rewrite --- pkg/ingress/kube/annotations/annotations.go | 15 +++++++- .../kube/annotations/annotations_test.go | 38 +++++++++++++++++-- pkg/ingress/kube/ingress/controller.go | 16 ++++---- pkg/ingress/kube/ingressv1/controller.go | 16 ++++---- .../tests/httproute-rewrite-path.yaml | 1 - 5 files changed, 63 insertions(+), 23 deletions(-) diff --git a/pkg/ingress/kube/annotations/annotations.go b/pkg/ingress/kube/annotations/annotations.go index d6bd42e09b..82e8948bcb 100644 --- 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 strings.ContainsAny(path, `\.+*?()|[]{}^$`) { + return true + } + if i.Rewrite == nil { + return false + } + 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 index fadb6dc355..993d4c441d 100644 --- 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 index 083e36de02..0dca636a4f 100644 --- 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 index 5c86a2cd76..5e7789c69c 100644 --- 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 index 5b9cc3c7ed..ad60709d84 100644 --- 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: From 4fa07eff304359f89192b5f75bd5d011c2a21bd6 Mon Sep 17 00:00:00 2001 From: johnlanni Date: Tue, 14 Nov 2023 18:06:07 +0800 Subject: [PATCH 2/2] fix --- pkg/ingress/kube/annotations/annotations.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/ingress/kube/annotations/annotations.go b/pkg/ingress/kube/annotations/annotations.go index 82e8948bcb..76d97a07a3 100644 --- a/pkg/ingress/kube/annotations/annotations.go +++ b/pkg/ingress/kube/annotations/annotations.go @@ -76,12 +76,12 @@ type Ingress struct { } func (i *Ingress) NeedRegexMatch(path string) bool { - if strings.ContainsAny(path, `\.+*?()|[]{}^$`) { - return true - } if i.Rewrite == nil { return false } + if strings.ContainsAny(path, `\.+*?()|[]{}^$`) { + return true + } if strings.ContainsAny(i.Rewrite.RewriteTarget, `$\`) { return true }