Merge pull request #4166 from Shopify/use-correct-type

Session Affinity ChangeOnFailure should be boolean
kubernetes · Jun 6, 2019 · d460147 · d460147
2 parents c6c6f51 + 83f2acb
commit d460147
Show file tree

Hide file tree

Showing 5 changed files with 15 additions and 16 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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