From 9941843385ed85c81075fb8e49f4ec572b74dfb6 Mon Sep 17 00:00:00 2001 From: Mingjie Li Date: Fri, 18 Oct 2024 19:52:02 +0200 Subject: [PATCH 1/2] fix #1712 : sort gateway api header fileter to fix canary restart Signed-off-by: Mingjie Li --- pkg/router/gateway_api.go | 18 ++++++ pkg/router/gateway_api_test.go | 88 ++++++++------------------ pkg/router/gateway_api_v1beta1.go | 17 +++++ pkg/router/gateway_api_v1beta1_test.go | 40 ++++++++++++ 4 files changed, 100 insertions(+), 63 deletions(-) diff --git a/pkg/router/gateway_api.go b/pkg/router/gateway_api.go index dd4ecd49f..b16075e93 100644 --- a/pkg/router/gateway_api.go +++ b/pkg/router/gateway_api.go @@ -20,6 +20,7 @@ import ( "context" "fmt" "reflect" + "sort" "strings" flaggerv1 "github.com/fluxcd/flagger/pkg/apis/flagger/v1beta1" @@ -647,10 +648,23 @@ func (gwr *GatewayAPIRouter) mergeMatchConditions(analysis, service []v1.HTTPRou return merged } +func sortFiltersV1(headers []v1.HTTPHeader) { + + if headers != nil { + sort.Slice(headers, func(i, j int) bool { + if headers[i].Name == headers[j].Name { + return headers[i].Value < headers[j].Value + } + return headers[i].Name < headers[j].Name + }) + } +} + func (gwr *GatewayAPIRouter) makeFilters(canary *flaggerv1.Canary) []v1.HTTPRouteFilter { var filters []v1.HTTPRouteFilter if canary.Spec.Service.Headers != nil { + if canary.Spec.Service.Headers.Request != nil { requestHeaderFilter := v1.HTTPRouteFilter{ Type: v1.HTTPRouteFilterRequestHeaderModifier, @@ -663,6 +677,7 @@ func (gwr *GatewayAPIRouter) makeFilters(canary *flaggerv1.Canary) []v1.HTTPRout Value: val, }) } + sortFiltersV1(requestHeaderFilter.RequestHeaderModifier.Add) for name, val := range canary.Spec.Service.Headers.Request.Set { requestHeaderFilter.RequestHeaderModifier.Set = append(requestHeaderFilter.RequestHeaderModifier.Set, v1.HTTPHeader{ Name: v1.HTTPHeaderName(name), @@ -670,6 +685,7 @@ func (gwr *GatewayAPIRouter) makeFilters(canary *flaggerv1.Canary) []v1.HTTPRout }) } + sortFiltersV1(requestHeaderFilter.RequestHeaderModifier.Set) for _, name := range canary.Spec.Service.Headers.Request.Remove { requestHeaderFilter.RequestHeaderModifier.Remove = append(requestHeaderFilter.RequestHeaderModifier.Remove, name) } @@ -688,12 +704,14 @@ func (gwr *GatewayAPIRouter) makeFilters(canary *flaggerv1.Canary) []v1.HTTPRout Value: val, }) } + sortFiltersV1(responseHeaderFilter.ResponseHeaderModifier.Add) for name, val := range canary.Spec.Service.Headers.Response.Set { responseHeaderFilter.ResponseHeaderModifier.Set = append(responseHeaderFilter.ResponseHeaderModifier.Set, v1.HTTPHeader{ Name: v1.HTTPHeaderName(name), Value: val, }) } + sortFiltersV1(responseHeaderFilter.ResponseHeaderModifier.Set) for _, name := range canary.Spec.Service.Headers.Response.Remove { responseHeaderFilter.ResponseHeaderModifier.Remove = append(responseHeaderFilter.ResponseHeaderModifier.Remove, name) diff --git a/pkg/router/gateway_api_test.go b/pkg/router/gateway_api_test.go index b03b5ab8e..88b34fdda 100644 --- a/pkg/router/gateway_api_test.go +++ b/pkg/router/gateway_api_test.go @@ -24,7 +24,9 @@ import ( flaggerv1 "github.com/fluxcd/flagger/pkg/apis/flagger/v1beta1" v1 "github.com/fluxcd/flagger/pkg/apis/gatewayapi/v1" + istiov1beta1 "github.com/fluxcd/flagger/pkg/apis/istio/v1beta1" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -276,13 +278,18 @@ func TestGatewayAPIRouter_Routes(t *testing.T) { }) } -func TestGatewayAPIRouter_getSessionAffinityRouteRules(t *testing.T) { +func TestGatewayAPIRouter_makeFilters(t *testing.T) { canary := newTestGatewayAPICanary() mocks := newFixture(canary) - cookieKey := "flagger-cookie" - canary.Spec.Analysis.SessionAffinity = &flaggerv1.SessionAffinity{ - CookieName: cookieKey, - MaxAge: 300, + canary.Spec.Service.Headers = &istiov1beta1.Headers{ + Response: &istiov1beta1.HeaderOperations{ + Set: map[string]string{"h1": "v1", "h2": "v2", "h3": "v3"}, + Add: map[string]string{"h1": "v1", "h2": "v2", "h3": "v3"}, + }, + Request: &istiov1beta1.HeaderOperations{ + Set: map[string]string{"h1": "v1", "h2": "v2", "h3": "v3"}, + Add: map[string]string{"h1": "v1", "h2": "v2", "h3": "v3"}, + }, } router := &GatewayAPIRouter{ @@ -290,65 +297,20 @@ func TestGatewayAPIRouter_getSessionAffinityRouteRules(t *testing.T) { kubeClient: mocks.kubeClient, logger: mocks.logger, } - _, pSvcName, cSvcName := canary.GetServiceNames() - weightedRouteRule := &v1.HTTPRouteRule{ - BackendRefs: []v1.HTTPBackendRef{ - { - BackendRef: router.makeBackendRef(pSvcName, initialPrimaryWeight, canary.Spec.Service.Port), - }, - { - BackendRef: router.makeBackendRef(cSvcName, initialCanaryWeight, canary.Spec.Service.Port), - }, - }, - } - rules, err := router.getSessionAffinityRouteRules(canary, 10, weightedRouteRule) - require.NoError(t, err) - assert.Equal(t, len(rules), 2) - assert.True(t, strings.HasPrefix(canary.Status.SessionAffinityCookie, cookieKey)) - - stickyRule := rules[0] - cookieMatch := stickyRule.Matches[0].Headers[0] - assert.Equal(t, *cookieMatch.Type, v1.HeaderMatchRegularExpression) - assert.Equal(t, string(cookieMatch.Name), cookieHeader) - assert.Contains(t, cookieMatch.Value, cookieKey) - - assert.Equal(t, len(stickyRule.BackendRefs), 2) - for _, backendRef := range stickyRule.BackendRefs { - if string(backendRef.BackendRef.Name) == pSvcName { - assert.Equal(t, *backendRef.BackendRef.Weight, int32(0)) - } - if string(backendRef.BackendRef.Name) == cSvcName { - assert.Equal(t, *backendRef.BackendRef.Weight, int32(100)) - } + + ignoreCmpOptions := []cmp.Option{ + cmpopts.IgnoreFields(v1.BackendRef{}, "Weight"), + cmpopts.EquateEmpty(), } - weightedRule := rules[1] - var found bool - for _, backendRef := range weightedRule.BackendRefs { - if string(backendRef.Name) == cSvcName { - found = true - filter := backendRef.Filters[0] - assert.Equal(t, filter.Type, v1.HTTPRouteFilterResponseHeaderModifier) - assert.NotNil(t, filter.ResponseHeaderModifier) - assert.Equal(t, string(filter.ResponseHeaderModifier.Add[0].Name), setCookieHeader) - assert.Equal(t, filter.ResponseHeaderModifier.Add[0].Value, fmt.Sprintf("%s; %s=%d", canary.Status.SessionAffinityCookie, maxAgeAttr, 300)) - } + filters := router.makeFilters(canary) + + for i := 0; i < 10; i++ { + newFilters := router.makeFilters(canary) + filtersDiff := cmp.Diff( + filters, newFilters, + ignoreCmpOptions..., + ) + assert.Equal(t, "", filtersDiff) } - assert.True(t, found) - - rules, err = router.getSessionAffinityRouteRules(canary, 0, weightedRouteRule) - assert.Empty(t, canary.Status.SessionAffinityCookie) - assert.Contains(t, canary.Status.PreviousSessionAffinityCookie, cookieKey) - - stickyRule = rules[0] - cookieMatch = stickyRule.Matches[0].Headers[0] - assert.Equal(t, *cookieMatch.Type, v1.HeaderMatchRegularExpression) - assert.Equal(t, string(cookieMatch.Name), cookieHeader) - assert.Contains(t, cookieMatch.Value, cookieKey) - - assert.Equal(t, stickyRule.Filters[0].Type, v1.HTTPRouteFilterResponseHeaderModifier) - headerModifier := stickyRule.Filters[0].ResponseHeaderModifier - assert.NotNil(t, headerModifier) - assert.Equal(t, string(headerModifier.Add[0].Name), setCookieHeader) - assert.Equal(t, headerModifier.Add[0].Value, fmt.Sprintf("%s; %s=%d", canary.Status.PreviousSessionAffinityCookie, maxAgeAttr, -1)) } diff --git a/pkg/router/gateway_api_v1beta1.go b/pkg/router/gateway_api_v1beta1.go index 1d2468020..fd389fe77 100644 --- a/pkg/router/gateway_api_v1beta1.go +++ b/pkg/router/gateway_api_v1beta1.go @@ -20,6 +20,7 @@ import ( "context" "fmt" "reflect" + "sort" "strings" flaggerv1 "github.com/fluxcd/flagger/pkg/apis/flagger/v1beta1" @@ -608,6 +609,18 @@ func (gwr *GatewayAPIV1Beta1Router) mergeMatchConditions(analysis, service []v1b return merged } +func sortFiltersV1beta1(headers []v1beta1.HTTPHeader) { + + if headers != nil { + sort.Slice(headers, func(i, j int) bool { + if headers[i].Name == headers[j].Name { + return headers[i].Value < headers[j].Value + } + return headers[i].Name < headers[j].Name + }) + } +} + func (gwr *GatewayAPIV1Beta1Router) makeFilters(canary *flaggerv1.Canary) []v1beta1.HTTPRouteFilter { var filters []v1beta1.HTTPRouteFilter @@ -624,12 +637,14 @@ func (gwr *GatewayAPIV1Beta1Router) makeFilters(canary *flaggerv1.Canary) []v1be Value: val, }) } + sortFiltersV1beta1(requestHeaderFilter.RequestHeaderModifier.Add) for name, val := range canary.Spec.Service.Headers.Request.Set { requestHeaderFilter.RequestHeaderModifier.Set = append(requestHeaderFilter.RequestHeaderModifier.Set, v1beta1.HTTPHeader{ Name: v1beta1.HTTPHeaderName(name), Value: val, }) } + sortFiltersV1beta1(requestHeaderFilter.RequestHeaderModifier.Set) for _, name := range canary.Spec.Service.Headers.Request.Remove { requestHeaderFilter.RequestHeaderModifier.Remove = append(requestHeaderFilter.RequestHeaderModifier.Remove, name) @@ -649,12 +664,14 @@ func (gwr *GatewayAPIV1Beta1Router) makeFilters(canary *flaggerv1.Canary) []v1be Value: val, }) } + sortFiltersV1beta1(responseHeaderFilter.ResponseHeaderModifier.Add) for name, val := range canary.Spec.Service.Headers.Response.Set { responseHeaderFilter.ResponseHeaderModifier.Set = append(responseHeaderFilter.ResponseHeaderModifier.Set, v1beta1.HTTPHeader{ Name: v1beta1.HTTPHeaderName(name), Value: val, }) } + sortFiltersV1beta1(responseHeaderFilter.ResponseHeaderModifier.Set) for _, name := range canary.Spec.Service.Headers.Response.Remove { responseHeaderFilter.ResponseHeaderModifier.Remove = append(responseHeaderFilter.ResponseHeaderModifier.Remove, name) diff --git a/pkg/router/gateway_api_v1beta1_test.go b/pkg/router/gateway_api_v1beta1_test.go index 647061c32..8652aae7c 100644 --- a/pkg/router/gateway_api_v1beta1_test.go +++ b/pkg/router/gateway_api_v1beta1_test.go @@ -23,8 +23,11 @@ import ( "testing" flaggerv1 "github.com/fluxcd/flagger/pkg/apis/flagger/v1beta1" + v1 "github.com/fluxcd/flagger/pkg/apis/gatewayapi/v1" "github.com/fluxcd/flagger/pkg/apis/gatewayapi/v1beta1" + istiov1beta1 "github.com/fluxcd/flagger/pkg/apis/istio/v1beta1" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -349,3 +352,40 @@ func TestGatewayAPIV1Beta1Router_getSessionAffinityRouteRules(t *testing.T) { assert.Equal(t, string(headerModifier.Add[0].Name), setCookieHeader) assert.Equal(t, headerModifier.Add[0].Value, fmt.Sprintf("%s; %s=%d", canary.Status.PreviousSessionAffinityCookie, maxAgeAttr, -1)) } + +func TestGatewayAPIV1Beta1Router_makeFilters(t *testing.T) { + canary := newTestGatewayAPICanary() + mocks := newFixture(canary) + canary.Spec.Service.Headers = &istiov1beta1.Headers{ + Response: &istiov1beta1.HeaderOperations{ + Set: map[string]string{"h1": "v1", "h2": "v2", "h3": "v3"}, + Add: map[string]string{"h1": "v1", "h2": "v2", "h3": "v3"}, + }, + Request: &istiov1beta1.HeaderOperations{ + Set: map[string]string{"h1": "v1", "h2": "v2", "h3": "v3"}, + Add: map[string]string{"h1": "v1", "h2": "v2", "h3": "v3"}, + }, + } + + router := &GatewayAPIV1Beta1Router{ + gatewayAPIClient: mocks.meshClient, + kubeClient: mocks.kubeClient, + logger: mocks.logger, + } + + ignoreCmpOptions := []cmp.Option{ + cmpopts.IgnoreFields(v1.BackendRef{}, "Weight"), + cmpopts.EquateEmpty(), + } + + filters := router.makeFilters(canary) + + for i := 0; i < 10; i++ { + newFilters := router.makeFilters(canary) + filtersDiff := cmp.Diff( + filters, newFilters, + ignoreCmpOptions..., + ) + assert.Equal(t, "", filtersDiff) + } +} From b88e080a6616a9728309f6956bf9c82c38afb436 Mon Sep 17 00:00:00 2001 From: Mingjie Li Date: Sat, 26 Oct 2024 16:48:02 +0200 Subject: [PATCH 2/2] add test back and use slices.SortFunc Signed-off-by: Mingjie Li --- pkg/router/gateway_api.go | 10 ++-- pkg/router/gateway_api_test.go | 77 +++++++++++++++++++++++++++++++ pkg/router/gateway_api_v1beta1.go | 10 ++-- 3 files changed, 87 insertions(+), 10 deletions(-) diff --git a/pkg/router/gateway_api.go b/pkg/router/gateway_api.go index b16075e93..7d2ae79d1 100644 --- a/pkg/router/gateway_api.go +++ b/pkg/router/gateway_api.go @@ -20,7 +20,7 @@ import ( "context" "fmt" "reflect" - "sort" + "slices" "strings" flaggerv1 "github.com/fluxcd/flagger/pkg/apis/flagger/v1beta1" @@ -651,11 +651,11 @@ func (gwr *GatewayAPIRouter) mergeMatchConditions(analysis, service []v1.HTTPRou func sortFiltersV1(headers []v1.HTTPHeader) { if headers != nil { - sort.Slice(headers, func(i, j int) bool { - if headers[i].Name == headers[j].Name { - return headers[i].Value < headers[j].Value + slices.SortFunc(headers, func(a, b v1.HTTPHeader) int { + if a.Name == b.Name { + return strings.Compare(a.Value, b.Value) } - return headers[i].Name < headers[j].Name + return strings.Compare(string(a.Name), string(b.Name)) }) } } diff --git a/pkg/router/gateway_api_test.go b/pkg/router/gateway_api_test.go index 88b34fdda..a7d6b6d89 100644 --- a/pkg/router/gateway_api_test.go +++ b/pkg/router/gateway_api_test.go @@ -278,6 +278,83 @@ func TestGatewayAPIRouter_Routes(t *testing.T) { }) } +func TestGatewayAPIRouter_getSessionAffinityRouteRules(t *testing.T) { + canary := newTestGatewayAPICanary() + mocks := newFixture(canary) + cookieKey := "flagger-cookie" + canary.Spec.Analysis.SessionAffinity = &flaggerv1.SessionAffinity{ + CookieName: cookieKey, + MaxAge: 300, + } + + router := &GatewayAPIRouter{ + gatewayAPIClient: mocks.meshClient, + kubeClient: mocks.kubeClient, + logger: mocks.logger, + } + _, pSvcName, cSvcName := canary.GetServiceNames() + weightedRouteRule := &v1.HTTPRouteRule{ + BackendRefs: []v1.HTTPBackendRef{ + { + BackendRef: router.makeBackendRef(pSvcName, initialPrimaryWeight, canary.Spec.Service.Port), + }, + { + BackendRef: router.makeBackendRef(cSvcName, initialCanaryWeight, canary.Spec.Service.Port), + }, + }, + } + rules, err := router.getSessionAffinityRouteRules(canary, 10, weightedRouteRule) + require.NoError(t, err) + assert.Equal(t, len(rules), 2) + assert.True(t, strings.HasPrefix(canary.Status.SessionAffinityCookie, cookieKey)) + + stickyRule := rules[0] + cookieMatch := stickyRule.Matches[0].Headers[0] + assert.Equal(t, *cookieMatch.Type, v1.HeaderMatchRegularExpression) + assert.Equal(t, string(cookieMatch.Name), cookieHeader) + assert.Contains(t, cookieMatch.Value, cookieKey) + + assert.Equal(t, len(stickyRule.BackendRefs), 2) + for _, backendRef := range stickyRule.BackendRefs { + if string(backendRef.BackendRef.Name) == pSvcName { + assert.Equal(t, *backendRef.BackendRef.Weight, int32(0)) + } + if string(backendRef.BackendRef.Name) == cSvcName { + assert.Equal(t, *backendRef.BackendRef.Weight, int32(100)) + } + } + + weightedRule := rules[1] + var found bool + for _, backendRef := range weightedRule.BackendRefs { + if string(backendRef.Name) == cSvcName { + found = true + filter := backendRef.Filters[0] + assert.Equal(t, filter.Type, v1.HTTPRouteFilterResponseHeaderModifier) + assert.NotNil(t, filter.ResponseHeaderModifier) + assert.Equal(t, string(filter.ResponseHeaderModifier.Add[0].Name), setCookieHeader) + assert.Equal(t, filter.ResponseHeaderModifier.Add[0].Value, fmt.Sprintf("%s; %s=%d", canary.Status.SessionAffinityCookie, maxAgeAttr, 300)) + } + } + assert.True(t, found) + + rules, err = router.getSessionAffinityRouteRules(canary, 0, weightedRouteRule) + assert.Empty(t, canary.Status.SessionAffinityCookie) + assert.Contains(t, canary.Status.PreviousSessionAffinityCookie, cookieKey) + + stickyRule = rules[0] + cookieMatch = stickyRule.Matches[0].Headers[0] + assert.Equal(t, *cookieMatch.Type, v1.HeaderMatchRegularExpression) + assert.Equal(t, string(cookieMatch.Name), cookieHeader) + assert.Contains(t, cookieMatch.Value, cookieKey) + + assert.Equal(t, stickyRule.Filters[0].Type, v1.HTTPRouteFilterResponseHeaderModifier) + headerModifier := stickyRule.Filters[0].ResponseHeaderModifier + assert.NotNil(t, headerModifier) + assert.Equal(t, string(headerModifier.Add[0].Name), setCookieHeader) + assert.Equal(t, headerModifier.Add[0].Value, fmt.Sprintf("%s; %s=%d", canary.Status.PreviousSessionAffinityCookie, maxAgeAttr, -1)) +} + func TestGatewayAPIRouter_makeFilters(t *testing.T) { canary := newTestGatewayAPICanary() mocks := newFixture(canary) diff --git a/pkg/router/gateway_api_v1beta1.go b/pkg/router/gateway_api_v1beta1.go index fd389fe77..e9042ea28 100644 --- a/pkg/router/gateway_api_v1beta1.go +++ b/pkg/router/gateway_api_v1beta1.go @@ -20,7 +20,7 @@ import ( "context" "fmt" "reflect" - "sort" + "slices" "strings" flaggerv1 "github.com/fluxcd/flagger/pkg/apis/flagger/v1beta1" @@ -612,11 +612,11 @@ func (gwr *GatewayAPIV1Beta1Router) mergeMatchConditions(analysis, service []v1b func sortFiltersV1beta1(headers []v1beta1.HTTPHeader) { if headers != nil { - sort.Slice(headers, func(i, j int) bool { - if headers[i].Name == headers[j].Name { - return headers[i].Value < headers[j].Value + slices.SortFunc(headers, func(a, b v1beta1.HTTPHeader) int { + if a.Name == b.Name { + return strings.Compare(a.Value, b.Value) } - return headers[i].Name < headers[j].Name + return strings.Compare(string(a.Name), string(b.Name)) }) } }