From 1e4263ac5d3255f5e2ef84bac7a470d300dfc038 Mon Sep 17 00:00:00 2001 From: johnlanni Date: Mon, 13 Nov 2023 11:22:19 +0800 Subject: [PATCH 1/7] fix bug of header control --- .../kube/annotations/header_control.go | 5 ++- .../kube/annotations/header_control_test.go | 40 ++++++++++++------- 2 files changed, 30 insertions(+), 15 deletions(-) diff --git a/pkg/ingress/kube/annotations/header_control.go b/pkg/ingress/kube/annotations/header_control.go index b3dc08f161..e1e9aacba8 100644 --- a/pkg/ingress/kube/annotations/header_control.go +++ b/pkg/ingress/kube/annotations/header_control.go @@ -15,6 +15,7 @@ package annotations import ( + "regexp" "strings" networking "istio.io/api/networking/v1alpha3" @@ -37,6 +38,8 @@ const ( var ( _ Parser = headerControl{} _ RouteHandler = headerControl{} + + pattern = regexp.MustCompile(`\s+`) ) type HeaderOperation struct { @@ -147,7 +150,7 @@ func convertAddOrUpdate(headers string) map[string]string { continue } - keyValue := strings.Fields(part) + keyValue := pattern.Split(part, 2) if len(keyValue) != 2 { IngressLog.Errorf("Header format %s is invalid.", keyValue) continue diff --git a/pkg/ingress/kube/annotations/header_control_test.go b/pkg/ingress/kube/annotations/header_control_test.go index 9f022d0626..9ef1922661 100644 --- a/pkg/ingress/kube/annotations/header_control_test.go +++ b/pkg/ingress/kube/annotations/header_control_test.go @@ -48,8 +48,8 @@ func TestHeaderControlParse(t *testing.T) { }, { input: map[string]string{ - buildHigressAnnotationKey(requestHeaderAdd): "one 1\n two 2\nthree 3 \n", - buildHigressAnnotationKey(requestHeaderUpdate): "two 2", + buildHigressAnnotationKey(requestHeaderAdd): "one 1\n two 2\nthree 3 \nx-test mse; test=true\nx-pro mse; pro=true\n", + buildHigressAnnotationKey(requestHeaderUpdate): "two 2\n set-cookie name=test; sameage=111\nset-stage name=stage; stage=true\n", buildHigressAnnotationKey(requestHeaderRemove): "one, two,three\n", buildHigressAnnotationKey(responseHeaderAdd): "A a\nB b\n", buildHigressAnnotationKey(responseHeaderUpdate): "X x\nY y\n", @@ -58,12 +58,16 @@ func TestHeaderControlParse(t *testing.T) { expect: &HeaderControlConfig{ Request: &HeaderOperation{ Add: map[string]string{ - "one": "1", - "two": "2", - "three": "3", + "one": "1", + "two": "2", + "three": "3", + "x-test": "mse; test=true", + "x-pro": "mse; pro=true", }, Update: map[string]string{ - "two": "2", + "two": "2", + "set-cookie": "name=test; sameage=111", + "set-stage": "name=stage; stage=true", }, Remove: []string{"one", "two", "three"}, }, @@ -122,12 +126,16 @@ func TestHeaderControlApplyRoute(t *testing.T) { HeaderControl: &HeaderControlConfig{ Request: &HeaderOperation{ Add: map[string]string{ - "one": "1", - "two": "2", - "three": "3", + "one": "1", + "two": "2", + "three": "3", + "x-test": "mse; test=true", + "x-pro": "mse; pro=true", }, Update: map[string]string{ - "two": "2", + "two": "2", + "set-cookie": "name=test; sameage=111", + "set-stage": "name=stage; sameage=111", }, Remove: []string{"one", "two", "three"}, }, @@ -138,12 +146,16 @@ func TestHeaderControlApplyRoute(t *testing.T) { Headers: &networking.Headers{ Request: &networking.Headers_HeaderOperations{ Add: map[string]string{ - "one": "1", - "two": "2", - "three": "3", + "one": "1", + "two": "2", + "three": "3", + "x-test": "mse; test=true", + "x-pro": "mse; pro=true", }, Set: map[string]string{ - "two": "2", + "two": "2", + "set-cookie": "name=test; sameage=111", + "set-stage": "name=stage; sameage=111", }, Remove: []string{"one", "two", "three"}, }, From 803c0ec31e2d6c981301673ac9aa7872437fe458 Mon Sep 17 00:00:00 2001 From: johnlanni Date: Mon, 13 Nov 2023 14:22:26 +0800 Subject: [PATCH 2/7] add e2e test --- test/e2e/conformance/tests/httproute-request-header-control.go | 1 + test/e2e/conformance/tests/httproute-request-header-control.yaml | 1 + 2 files changed, 2 insertions(+) diff --git a/test/e2e/conformance/tests/httproute-request-header-control.go b/test/e2e/conformance/tests/httproute-request-header-control.go index d3f5788a80..1e714a4581 100644 --- a/test/e2e/conformance/tests/httproute-request-header-control.go +++ b/test/e2e/conformance/tests/httproute-request-header-control.go @@ -80,6 +80,7 @@ var HTTPRouteRequestHeaderControl = suite.ConformanceTest{ Headers: map[string]string{ "stage": "test", "canary": "true", + "x-test": "higress; test=true", }, }, }, diff --git a/test/e2e/conformance/tests/httproute-request-header-control.yaml b/test/e2e/conformance/tests/httproute-request-header-control.yaml index 8126b0f2c4..911d6fed0a 100644 --- a/test/e2e/conformance/tests/httproute-request-header-control.yaml +++ b/test/e2e/conformance/tests/httproute-request-header-control.yaml @@ -40,6 +40,7 @@ metadata: higress.io/request-header-control-add: | stage test canary true + x-test "higress; test=true" name: httproute-request-header-control-add-more namespace: higress-conformance-infra spec: From 7aaa2c5c0d8cc0d22f7af08f25a0434470274323 Mon Sep 17 00:00:00 2001 From: johnlanni Date: Mon, 13 Nov 2023 15:31:50 +0800 Subject: [PATCH 3/7] update e2e --- .../conformance/tests/httproute-request-header-control.go | 7 ++++--- .../tests/httproute-request-header-control.yaml | 1 + 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/test/e2e/conformance/tests/httproute-request-header-control.go b/test/e2e/conformance/tests/httproute-request-header-control.go index 1e714a4581..f8f8f24e7b 100644 --- a/test/e2e/conformance/tests/httproute-request-header-control.go +++ b/test/e2e/conformance/tests/httproute-request-header-control.go @@ -78,9 +78,10 @@ var HTTPRouteRequestHeaderControl = suite.ConformanceTest{ Path: "/foo2", Host: "foo.com", Headers: map[string]string{ - "stage": "test", - "canary": "true", - "x-test": "higress; test=true", + "stage": "test", + "canary": "true", + "x-test": "higress; test=true", + "x-test2": "higress; test=false", }, }, }, diff --git a/test/e2e/conformance/tests/httproute-request-header-control.yaml b/test/e2e/conformance/tests/httproute-request-header-control.yaml index 911d6fed0a..57cdcc36c6 100644 --- a/test/e2e/conformance/tests/httproute-request-header-control.yaml +++ b/test/e2e/conformance/tests/httproute-request-header-control.yaml @@ -41,6 +41,7 @@ metadata: stage test canary true x-test "higress; test=true" + 'x-test2' "higress; test=false" name: httproute-request-header-control-add-more namespace: higress-conformance-infra spec: From e493e767ae5a47839513803fc51c8e1daecf2483 Mon Sep 17 00:00:00 2001 From: johnlanni Date: Mon, 13 Nov 2023 16:02:15 +0800 Subject: [PATCH 4/7] fix e2e --- pkg/ingress/kube/annotations/header_control.go | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/pkg/ingress/kube/annotations/header_control.go b/pkg/ingress/kube/annotations/header_control.go index e1e9aacba8..4ab4974983 100644 --- a/pkg/ingress/kube/annotations/header_control.go +++ b/pkg/ingress/kube/annotations/header_control.go @@ -141,6 +141,18 @@ func needHeaderControlConfig(annotations Annotations) bool { annotations.HasHigress(responseHeaderRemove) } +func trimQuotes(s string) string { + if len(s) >= 2 { + if s[0] == '"' && s[len(s)-1] == '"' { + return s[1 : len(s)-1] + } + if s[0] == '\'' && s[len(s)-1] == '\'' { + return s[1 : len(s)-1] + } + } + return s +} + func convertAddOrUpdate(headers string) map[string]string { result := map[string]string{} parts := strings.Split(headers, "\n") @@ -155,8 +167,8 @@ func convertAddOrUpdate(headers string) map[string]string { IngressLog.Errorf("Header format %s is invalid.", keyValue) continue } - key := strings.TrimSpace(keyValue[0]) - value := strings.TrimSpace(keyValue[1]) + key := trimQuotes(strings.TrimSpace(keyValue[0])) + value := trimQuotes(strings.TrimSpace(keyValue[1])) result[key] = value } return result From 56ff084b2367542497eab5fb7d8500e4ac37163f Mon Sep 17 00:00:00 2001 From: johnlanni Date: Mon, 13 Nov 2023 17:25:22 +0800 Subject: [PATCH 5/7] rework rewrite --- pkg/ingress/kube/annotations/annotations.go | 6 +- .../kube/annotations/annotations_test.go | 6 +- pkg/ingress/kube/annotations/canary.go | 4 +- pkg/ingress/kube/annotations/rewrite.go | 25 ++++-- pkg/ingress/kube/annotations/rewrite_test.go | 87 +++++++++++++++++-- pkg/ingress/kube/ingress/controller.go | 43 ++++----- pkg/ingress/kube/ingressv1/controller.go | 40 ++++----- 7 files changed, 140 insertions(+), 71 deletions(-) diff --git a/pkg/ingress/kube/annotations/annotations.go b/pkg/ingress/kube/annotations/annotations.go index e78135cbfb..d6bd42e09b 100644 --- a/pkg/ingress/kube/annotations/annotations.go +++ b/pkg/ingress/kube/annotations/annotations.go @@ -74,11 +74,7 @@ type Ingress struct { } func (i *Ingress) NeedRegexMatch() bool { - if i.Rewrite == nil { - return false - } - - return i.Rewrite.RewriteTarget != "" || i.IsPrefixRegexMatch() || i.IsFullPathRegexMatch() + return i.Rewrite != nil && (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 9195e918b4..fadb6dc355 100644 --- a/pkg/ingress/kube/annotations/annotations_test.go +++ b/pkg/ingress/kube/annotations/annotations_test.go @@ -34,7 +34,7 @@ func TestNeedRegexMatch(t *testing.T) { { input: &Ingress{ Rewrite: &RewriteConfig{ - RewriteTarget: "/test", + UseRegex: true, }, }, expect: true, @@ -42,10 +42,10 @@ func TestNeedRegexMatch(t *testing.T) { { input: &Ingress{ Rewrite: &RewriteConfig{ - UseRegex: true, + UseRegex: false, }, }, - expect: true, + expect: false, }, } diff --git a/pkg/ingress/kube/annotations/canary.go b/pkg/ingress/kube/annotations/canary.go index ea39004abb..99216061d4 100644 --- a/pkg/ingress/kube/annotations/canary.go +++ b/pkg/ingress/kube/annotations/canary.go @@ -108,6 +108,8 @@ func ApplyByWeight(canary, route *networking.HTTPRoute, canaryIngress *Ingress) // canary route use the header control applied on itself. headerControl{}.ApplyRoute(canary, canaryIngress) + // reset + canary.Route[0].FallbackClusters = nil // Move route level to destination level canary.Route[0].Headers = canary.Headers @@ -127,8 +129,6 @@ func ApplyByHeader(canary, route *networking.HTTPRoute, canaryIngress *Ingress) // Inherit configuration from non-canary rule route.DeepCopyInto(canary) - // Assign temp copied canary route match - canary.Match = temp.Match // Assign temp copied canary route destination canary.Route = temp.Route diff --git a/pkg/ingress/kube/annotations/rewrite.go b/pkg/ingress/kube/annotations/rewrite.go index 84420aafc3..1bc5280f43 100644 --- a/pkg/ingress/kube/annotations/rewrite.go +++ b/pkg/ingress/kube/annotations/rewrite.go @@ -59,12 +59,6 @@ func (r rewrite) Parse(annotations Annotations, config *Ingress, _ *GlobalContex rewriteConfig.RewritePath, _ = annotations.ParseStringForHigress(rewritePath) if rewriteConfig.RewritePath == "" && rewriteConfig.RewriteTarget != "" { - // When rewrite target is present and not empty, - // we will enforce regex match on all rules in this ingress. - if !rewriteConfig.UseRegex && !rewriteConfig.FullPathRegex { - rewriteConfig.UseRegex = true - } - // We should convert nginx regex rule to envoy regex rule. rewriteConfig.RewriteTarget = convertToRE2(rewriteConfig.RewriteTarget) } @@ -92,9 +86,22 @@ func (r rewrite) ApplyRoute(route *networking.HTTPRoute, config *Ingress) { } } } else if rewriteConfig.RewriteTarget != "" { - route.Rewrite.UriRegex = &networking.RegexMatchAndSubstitute{ - Pattern: route.Match[0].Uri.GetRegex(), - Substitution: rewriteConfig.RewriteTarget, + uri := route.Match[0].Uri + if uri.GetExact() != "" { + route.Rewrite.UriRegex = &networking.RegexMatchAndSubstitute{ + Pattern: uri.GetExact(), + Substitution: rewriteConfig.RewriteTarget, + } + } else if uri.GetPrefix() != "" { + route.Rewrite.UriRegex = &networking.RegexMatchAndSubstitute{ + Pattern: uri.GetPrefix(), + Substitution: rewriteConfig.RewriteTarget, + } + } else { + route.Rewrite.UriRegex = &networking.RegexMatchAndSubstitute{ + Pattern: uri.GetRegex(), + Substitution: rewriteConfig.RewriteTarget, + } } } diff --git a/pkg/ingress/kube/annotations/rewrite_test.go b/pkg/ingress/kube/annotations/rewrite_test.go index a3f7690a0b..356dec769c 100644 --- a/pkg/ingress/kube/annotations/rewrite_test.go +++ b/pkg/ingress/kube/annotations/rewrite_test.go @@ -77,16 +77,13 @@ func TestRewriteParse(t *testing.T) { }, expect: &RewriteConfig{ RewriteTarget: "/test", - UseRegex: true, }, }, { input: Annotations{ buildNginxAnnotationKey(rewriteTarget): "", }, - expect: &RewriteConfig{ - UseRegex: false, - }, + expect: &RewriteConfig{}, }, { input: Annotations{ @@ -94,7 +91,6 @@ func TestRewriteParse(t *testing.T) { }, expect: &RewriteConfig{ RewriteTarget: "/\\2/\\1", - UseRegex: true, }, }, { @@ -113,6 +109,16 @@ func TestRewriteParse(t *testing.T) { RewriteHost: "test.com", }, }, + { + input: Annotations{ + buildNginxAnnotationKey(useRegex): "true", + buildNginxAnnotationKey(rewriteTarget): "/$1", + }, + expect: &RewriteConfig{ + UseRegex: true, + RewriteTarget: "/\\1", + }, + }, { input: Annotations{ buildNginxAnnotationKey(rewriteTarget): "/$2/$1", @@ -120,7 +126,6 @@ func TestRewriteParse(t *testing.T) { }, expect: &RewriteConfig{ RewriteTarget: "/\\2/\\1", - UseRegex: true, RewriteHost: "test.com", }, }, @@ -330,6 +335,76 @@ func TestRewriteApplyRoute(t *testing.T) { }, }, }, + { + config: &Ingress{ + Rewrite: &RewriteConfig{ + RewriteTarget: "/test", + }, + }, + input: &networking.HTTPRoute{ + Match: []*networking.HTTPMatchRequest{ + { + Uri: &networking.StringMatch{ + MatchType: &networking.StringMatch_Exact{ + Exact: "/exact", + }, + }, + }, + }, + }, + expect: &networking.HTTPRoute{ + Match: []*networking.HTTPMatchRequest{ + { + Uri: &networking.StringMatch{ + MatchType: &networking.StringMatch_Exact{ + Exact: "/exact", + }, + }, + }, + }, + Rewrite: &networking.HTTPRewrite{ + UriRegex: &networking.RegexMatchAndSubstitute{ + Pattern: "/exact", + Substitution: "/test", + }, + }, + }, + }, + { + config: &Ingress{ + Rewrite: &RewriteConfig{ + RewriteTarget: "/test", + }, + }, + input: &networking.HTTPRoute{ + Match: []*networking.HTTPMatchRequest{ + { + Uri: &networking.StringMatch{ + MatchType: &networking.StringMatch_Prefix{ + Prefix: "/prefix", + }, + }, + }, + }, + }, + expect: &networking.HTTPRoute{ + Match: []*networking.HTTPMatchRequest{ + { + Uri: &networking.StringMatch{ + MatchType: &networking.StringMatch_Prefix{ + Prefix: "/prefix", + }, + }, + }, + }, + Rewrite: &networking.HTTPRewrite{ + UriRegex: &networking.RegexMatchAndSubstitute{ + Pattern: "/prefix", + Substitution: "/test", + }, + }, + }, + }, } for _, inputCase := range inputCases { diff --git a/pkg/ingress/kube/ingress/controller.go b/pkg/ingress/kube/ingress/controller.go index 1b2c4eec95..083e36de02 100644 --- a/pkg/ingress/kube/ingress/controller.go +++ b/pkg/ingress/kube/ingress/controller.go @@ -712,7 +712,7 @@ func (c *controller) ApplyCanaryIngress(convertOptions *common.ConvertOptions, w return fmt.Errorf("wrapperConfig is nil") } - byHeader, byWeight := wrapper.AnnotationsConfig.CanaryKind() + byHeader, _ := wrapper.AnnotationsConfig.CanaryKind() cfg := wrapper.Config ingressV1Beta, ok := cfg.Spec.(ingress.IngressSpec) @@ -765,9 +765,6 @@ func (c *controller) ApplyCanaryIngress(convertOptions *common.ConvertOptions, w } canary.OriginPath = originPath canary.OriginPathType = pathType - canary.HTTPRoute.Match = c.generateHttpMatches(pathType, httpPath.Path, nil) - canary.HTTPRoute.Name = common.GenerateUniqueRouteName(c.options.SystemNamespace, canary) - ingressRouteBuilder := convertOptions.IngressRouteCache.New(canary) // backend service check var event common.Event @@ -781,39 +778,37 @@ func (c *controller) ApplyCanaryIngress(convertOptions *common.ConvertOptions, w } canary.RuleKey = createRuleKey(canary.WrapperConfig.Config.Annotations, canary.PathFormat()) - canaryConfig := wrapper.AnnotationsConfig.Canary - if byWeight { - canary.HTTPRoute.Route[0].Weight = int32(canaryConfig.Weight) - } - + // find the base ingress pos := 0 var targetRoute *common.WrapperHTTPRoute for _, route := range routes { if isCanaryRoute(canary, route) { targetRoute = route - // Header, Cookie - if byHeader { - IngressLog.Debug("Insert canary route by header") - annotations.ApplyByHeader(canary.HTTPRoute, route.HTTPRoute, canary.WrapperConfig.AnnotationsConfig) - canary.HTTPRoute.Name = common.GenerateUniqueRouteName(c.options.SystemNamespace, canary) - } else { - IngressLog.Debug("Merge canary route by weight") - if route.WeightTotal == 0 { - route.WeightTotal = int32(canaryConfig.WeightTotal) - } - annotations.ApplyByWeight(canary.HTTPRoute, route.HTTPRoute, canary.WrapperConfig.AnnotationsConfig) - } - break } pos += 1 } - - IngressLog.Debugf("Canary route is %v", canary) if targetRoute == nil { continue } + canaryConfig := wrapper.AnnotationsConfig.Canary + + // Header, Cookie + if byHeader { + IngressLog.Debug("Insert canary route by header") + annotations.ApplyByHeader(canary.HTTPRoute, targetRoute.HTTPRoute, canary.WrapperConfig.AnnotationsConfig) + canary.HTTPRoute.Name = common.GenerateUniqueRouteName(c.options.SystemNamespace, canary) + } else { + IngressLog.Debug("Merge canary route by weight") + if targetRoute.WeightTotal == 0 { + targetRoute.WeightTotal = int32(canaryConfig.WeightTotal) + } + annotations.ApplyByWeight(canary.HTTPRoute, targetRoute.HTTPRoute, canary.WrapperConfig.AnnotationsConfig) + } + + IngressLog.Debugf("Canary route is %v", canary) + if byHeader { // Inherit policy from normal route canary.WrapperConfig.AnnotationsConfig.Auth = targetRoute.WrapperConfig.AnnotationsConfig.Auth diff --git a/pkg/ingress/kube/ingressv1/controller.go b/pkg/ingress/kube/ingressv1/controller.go index dfe319c396..5c86a2cd76 100644 --- a/pkg/ingress/kube/ingressv1/controller.go +++ b/pkg/ingress/kube/ingressv1/controller.go @@ -716,7 +716,7 @@ func (c *controller) ApplyDefaultBackend(convertOptions *common.ConvertOptions, } func (c *controller) ApplyCanaryIngress(convertOptions *common.ConvertOptions, wrapper *common.WrapperConfig) error { - byHeader, byWeight := wrapper.AnnotationsConfig.CanaryKind() + byHeader, _ := wrapper.AnnotationsConfig.CanaryKind() cfg := wrapper.Config ingressV1, ok := cfg.Spec.(ingress.IngressSpec) @@ -769,8 +769,6 @@ func (c *controller) ApplyCanaryIngress(convertOptions *common.ConvertOptions, w } canary.OriginPath = originPath canary.OriginPathType = pathType - canary.HTTPRoute.Match = c.generateHttpMatches(pathType, httpPath.Path, nil) - canary.HTTPRoute.Name = common.GenerateUniqueRouteName(c.options.SystemNamespace, canary) ingressRouteBuilder := convertOptions.IngressRouteCache.New(canary) // backend service check @@ -785,39 +783,37 @@ func (c *controller) ApplyCanaryIngress(convertOptions *common.ConvertOptions, w } canary.RuleKey = createRuleKey(canary.WrapperConfig.Config.Annotations, canary.PathFormat()) - canaryConfig := wrapper.AnnotationsConfig.Canary - if byWeight { - canary.HTTPRoute.Route[0].Weight = int32(canaryConfig.Weight) - } - + // find the base ingress pos := 0 var targetRoute *common.WrapperHTTPRoute for _, route := range routes { if isCanaryRoute(canary, route) { targetRoute = route - // Header, Cookie - if byHeader { - IngressLog.Debug("Insert canary route by header") - annotations.ApplyByHeader(canary.HTTPRoute, route.HTTPRoute, canary.WrapperConfig.AnnotationsConfig) - canary.HTTPRoute.Name = common.GenerateUniqueRouteName(c.options.SystemNamespace, canary) - } else { - IngressLog.Debug("Merge canary route by weight") - if route.WeightTotal == 0 { - route.WeightTotal = int32(canaryConfig.WeightTotal) - } - annotations.ApplyByWeight(canary.HTTPRoute, route.HTTPRoute, canary.WrapperConfig.AnnotationsConfig) - } - break } pos += 1 } - IngressLog.Debugf("Canary route is %v", canary) if targetRoute == nil { continue } + canaryConfig := wrapper.AnnotationsConfig.Canary + + // Header, Cookie + if byHeader { + IngressLog.Debug("Insert canary route by header") + annotations.ApplyByHeader(canary.HTTPRoute, targetRoute.HTTPRoute, canary.WrapperConfig.AnnotationsConfig) + canary.HTTPRoute.Name = common.GenerateUniqueRouteName(c.options.SystemNamespace, canary) + } else { + IngressLog.Debug("Merge canary route by weight") + if targetRoute.WeightTotal == 0 { + targetRoute.WeightTotal = int32(canaryConfig.WeightTotal) + } + annotations.ApplyByWeight(canary.HTTPRoute, targetRoute.HTTPRoute, canary.WrapperConfig.AnnotationsConfig) + } + IngressLog.Debugf("Canary route is %v", canary) + if byHeader { // Inherit policy from normal route canary.WrapperConfig.AnnotationsConfig.Auth = targetRoute.WrapperConfig.AnnotationsConfig.Auth From 008c81a37d1e6f9bee8ea9265fe7379864569964 Mon Sep 17 00:00:00 2001 From: johnlanni Date: Mon, 13 Nov 2023 19:28:09 +0800 Subject: [PATCH 6/7] fix e2e test --- pkg/ingress/kube/annotations/retry.go | 11 +++++-- pkg/ingress/kube/annotations/retry_test.go | 31 ++++++++++++++++--- .../tests/httproute-rewrite-path.yaml | 1 + 3 files changed, 35 insertions(+), 8 deletions(-) diff --git a/pkg/ingress/kube/annotations/retry.go b/pkg/ingress/kube/annotations/retry.go index 341e61b8b1..37110a79de 100644 --- a/pkg/ingress/kube/annotations/retry.go +++ b/pkg/ingress/kube/annotations/retry.go @@ -70,8 +70,13 @@ func (r retry) Parse(annotations Annotations, config *Ingress, _ *GlobalContext) } if retryOn, err := annotations.ParseStringASAP(retryOn); err == nil { - extraConfigs := splitBySeparator(retryOn, ",") - conditions := toSet(extraConfigs) + var retryOnConditions []string + if strings.Contains(retryOn, ",") { + retryOnConditions = splitBySeparator(retryOn, ",") + } else { + retryOnConditions = strings.Fields(retryOn) + } + conditions := toSet(retryOnConditions) if len(conditions) > 0 { if conditions.Contains("off") { retryConfig.retryCount = 0 @@ -88,7 +93,7 @@ func (r retry) Parse(annotations Annotations, config *Ingress, _ *GlobalContext) stringBuilder.WriteString("non_idempotent,") } // Append the status codes. - statusCodes := convertStatusCodes(extraConfigs) + statusCodes := convertStatusCodes(retryOnConditions) if len(statusCodes) > 0 { stringBuilder.WriteString(retryStatusCode + ",") for _, code := range statusCodes { diff --git a/pkg/ingress/kube/annotations/retry_test.go b/pkg/ingress/kube/annotations/retry_test.go index 376489bec3..dbfec32c1f 100644 --- a/pkg/ingress/kube/annotations/retry_test.go +++ b/pkg/ingress/kube/annotations/retry_test.go @@ -37,8 +37,8 @@ func TestRetryParse(t *testing.T) { }, expect: &RetryConfig{ retryCount: 1, - perRetryTimeout: &types.Duration{}, retryOn: "5xx", + perRetryTimeout: &types.Duration{}, }, }, { @@ -60,8 +60,8 @@ func TestRetryParse(t *testing.T) { }, expect: &RetryConfig{ retryCount: 0, - perRetryTimeout: &types.Duration{}, retryOn: "5xx", + perRetryTimeout: &types.Duration{}, }, }, { @@ -71,8 +71,19 @@ func TestRetryParse(t *testing.T) { }, expect: &RetryConfig{ retryCount: 2, + retryOn: "5xx", perRetryTimeout: &types.Duration{}, + }, + }, + { + input: map[string]string{ + buildNginxAnnotationKey(retryCount): "2", + buildNginxAnnotationKey(retryOn): "error timeout", + }, + expect: &RetryConfig{ + retryCount: 2, retryOn: "5xx", + perRetryTimeout: &types.Duration{}, }, }, { @@ -81,8 +92,18 @@ func TestRetryParse(t *testing.T) { }, expect: &RetryConfig{ retryCount: 3, + retryOn: "5xx,non_idempotent", perRetryTimeout: &types.Duration{}, + }, + }, + { + input: map[string]string{ + buildNginxAnnotationKey(retryOn): "timeout non_idempotent", + }, + expect: &RetryConfig{ + retryCount: 3, retryOn: "5xx,non_idempotent", + perRetryTimeout: &types.Duration{}, }, }, { @@ -91,18 +112,18 @@ func TestRetryParse(t *testing.T) { }, expect: &RetryConfig{ retryCount: 3, - perRetryTimeout: &types.Duration{}, retryOn: "5xx,retriable-status-codes,503,502,404", + perRetryTimeout: &types.Duration{}, }, }, { input: map[string]string{ - buildNginxAnnotationKey(retryOn): "timeout,http_505,http_503,http_502,http_404,http_403", + buildNginxAnnotationKey(retryOn): "timeout http_503 http_502 http_404", }, expect: &RetryConfig{ retryCount: 3, + retryOn: "5xx,retriable-status-codes,503,502,404", perRetryTimeout: &types.Duration{}, - retryOn: "5xx,retriable-status-codes,505,503,502,404,403", }, }, } diff --git a/test/e2e/conformance/tests/httproute-rewrite-path.yaml b/test/e2e/conformance/tests/httproute-rewrite-path.yaml index ad60709d84..5b9cc3c7ed 100644 --- a/test/e2e/conformance/tests/httproute-rewrite-path.yaml +++ b/test/e2e/conformance/tests/httproute-rewrite-path.yaml @@ -17,6 +17,7 @@ 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 ba114565ed0a0886d742ea638571721099d61419 Mon Sep 17 00:00:00 2001 From: johnlanni Date: Mon, 13 Nov 2023 19:31:16 +0800 Subject: [PATCH 7/7] fix cookie canary match --- pkg/ingress/kube/annotations/canary.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/ingress/kube/annotations/canary.go b/pkg/ingress/kube/annotations/canary.go index 99216061d4..8f9173228b 100644 --- a/pkg/ingress/kube/annotations/canary.go +++ b/pkg/ingress/kube/annotations/canary.go @@ -165,7 +165,7 @@ func ApplyByHeader(canary, route *networking.HTTPRoute, canaryIngress *Ingress) match.Headers = map[string]*networking.StringMatch{ "cookie": { MatchType: &networking.StringMatch_Regex{ - Regex: "^(.\\*?;)?(" + canaryConfig.Cookie + "=always)(;.\\*)?$", + Regex: "^(.*?;\\s*)?(" + canaryConfig.Cookie + "=always)(;.*)?$", }, }, }