From 83f2acbe38bccad4952d27d8738335edfed7ca43 Mon Sep 17 00:00:00 2001 From: Elvin Efendi Date: Thu, 6 Jun 2019 11:02:51 -0400 Subject: [PATCH] Session Affinity ChangeOnFailure should be boolean --- .../ingress/annotations/sessionaffinity/main.go | 9 ++++----- .../annotations/sessionaffinity/main_test.go | 2 +- internal/ingress/types.go | 2 +- rootfs/etc/nginx/lua/balancer/sticky.lua | 4 ++-- rootfs/etc/nginx/lua/test/balancer/sticky_test.lua | 14 +++++++------- 5 files changed, 15 insertions(+), 16 deletions(-) diff --git a/internal/ingress/annotations/sessionaffinity/main.go b/internal/ingress/annotations/sessionaffinity/main.go index 3f5db661f..5bea0421e 100644 --- a/internal/ingress/annotations/sessionaffinity/main.go +++ b/internal/ingress/annotations/sessionaffinity/main.go @@ -50,8 +50,7 @@ const ( ) var ( - affinityCookieExpiresRegex = regexp.MustCompile(`(^0|-?[1-9]\d*$)`) - affinityCookieChangeOnFailureRegex = regexp.MustCompile(`^(true|false)$`) + affinityCookieExpiresRegex = regexp.MustCompile(`(^0|-?[1-9]\d*$)`) ) // Config describes the per ingress session affinity config @@ -72,7 +71,7 @@ type Cookie struct { // The path that a cookie will be set on Path string `json:"path"` // Flag that allows cookie regeneration on request failure - ChangeOnFailure string `json:"changeonfailure"` + ChangeOnFailure bool `json:"changeonfailure"` } // cookieAffinityParse gets the annotation values related to Cookie Affinity @@ -105,8 +104,8 @@ func (a affinity) cookieAffinityParse(ing *extensions.Ingress) *Cookie { klog.V(3).Infof("Invalid or no annotation value found in Ingress %v: %v. Ignoring it", ing.Name, annotationAffinityCookieMaxAge) } - cookie.ChangeOnFailure, err = parser.GetStringAnnotation(annotationAffinityCookieChangeOnFailure, ing) - if err != nil || !affinityCookieChangeOnFailureRegex.MatchString(cookie.ChangeOnFailure) { + cookie.ChangeOnFailure, err = parser.GetBoolAnnotation(annotationAffinityCookieChangeOnFailure, ing) + if err != nil { klog.V(3).Infof("Invalid or no annotation value found in Ingress %v: %v. Ignoring it", ing.Name, annotationAffinityCookieChangeOnFailure) } diff --git a/internal/ingress/annotations/sessionaffinity/main_test.go b/internal/ingress/annotations/sessionaffinity/main_test.go index cf7bbcb31..9875e7580 100644 --- a/internal/ingress/annotations/sessionaffinity/main_test.go +++ b/internal/ingress/annotations/sessionaffinity/main_test.go @@ -100,7 +100,7 @@ func TestIngressAffinityCookieConfig(t *testing.T) { t.Errorf("expected /foo as session-cookie-path but returned %v", nginxAffinity.Cookie.Path) } - if nginxAffinity.Cookie.ChangeOnFailure != "true" { + if !nginxAffinity.Cookie.ChangeOnFailure { t.Errorf("expected change of failure parameter set to true but returned %v", nginxAffinity.Cookie.ChangeOnFailure) } } diff --git a/internal/ingress/types.go b/internal/ingress/types.go index ec4128479..ab13ae892 100644 --- a/internal/ingress/types.go +++ b/internal/ingress/types.go @@ -149,7 +149,7 @@ type CookieSessionAffinity struct { MaxAge string `json:"maxage,omitempty"` Locations map[string][]string `json:"locations,omitempty"` Path string `json:"path,omitempty"` - ChangeOnFailure string `json:"changeonfailure"` + ChangeOnFailure bool `json:"change_on_failure,omitempty"` } // UpstreamHashByConfig described setting from the upstream-hash-by* annotations. diff --git a/rootfs/etc/nginx/lua/balancer/sticky.lua b/rootfs/etc/nginx/lua/balancer/sticky.lua index 798112eaf..17acd4077 100644 --- a/rootfs/etc/nginx/lua/balancer/sticky.lua +++ b/rootfs/etc/nginx/lua/balancer/sticky.lua @@ -96,7 +96,7 @@ function _M.balance(self) if upstream_from_cookie ~= nil then -- use previous upstream if this is the first attempt or previous attempt succeeded -- or ingress is configured to ignore previous request result - if state_name == nil or self.cookie_session_affinity.changeonfailure == "false" then + if state_name == nil or not self.cookie_session_affinity.change_on_failure then return upstream_from_cookie end end @@ -106,7 +106,7 @@ function _M.balance(self) -- If previous attempt failed recent upstream can be obtained from ngx.var.upstream_addr. -- Do nothing if ingress is configured to ignore previous request result. - if state_name ~= nil and self.cookie_session_affinity.changeonfailure == "true" then + if state_name ~= nil and self.cookie_session_affinity.change_on_failure then local upstream_addr = ngx.var.upstream_addr failed_upstream = split.get_last_value(upstream_addr) diff --git a/rootfs/etc/nginx/lua/test/balancer/sticky_test.lua b/rootfs/etc/nginx/lua/test/balancer/sticky_test.lua index 3e18a8274..b0e0cc30e 100644 --- a/rootfs/etc/nginx/lua/test/balancer/sticky_test.lua +++ b/rootfs/etc/nginx/lua/test/balancer/sticky_test.lua @@ -194,7 +194,7 @@ describe("Sticky", function() end) end) - local function get_several_test_backends(changeOnFailure) + local function get_several_test_backends(change_on_failure) return { name = "access-router-production-web-80", endpoints = { @@ -203,7 +203,7 @@ describe("Sticky", function() }, sessionAffinityConfig = { name = "cookie", - cookieSessionAffinity = { name = "test_name", hash = "sha1", changeonfailure = changeOnFailure } + cookieSessionAffinity = { name = "test_name", hash = "sha1", change_on_failure = change_on_failure } }, } end @@ -221,7 +221,7 @@ describe("Sticky", function() end) context("when request to upstream fails", function() - it("changes upstream when changeOnFailure option is true", function() + it("changes upstream when change_on_failure option is true", function() -- create sticky cookie cookie.new = function(self) local return_obj = { @@ -231,7 +231,7 @@ describe("Sticky", function() return return_obj, false end - local options = {'false', 'true'} + local options = {false, true} for _, option in ipairs(options) do local sticky_balancer_instance = sticky:new(get_several_test_backends(option)) @@ -250,11 +250,11 @@ describe("Sticky", function() for _ = 1, 100 do local new_upstream = sticky_balancer_instance:balance() - if option == 'false' then - -- upstream should be the same inspite of error, if changeOnFailure option is false + if option == false then + -- upstream should be the same inspite of error, if change_on_failure option is false assert.equal(new_upstream, old_upstream) else - -- upstream should change after error, if changeOnFailure option is true + -- upstream should change after error, if change_on_failure option is true assert.not_equal(new_upstream, old_upstream) end end