From ad57c76b73bee1a5f2a7f820a18dc81dfab1c529 Mon Sep 17 00:00:00 2001 From: liuwei Date: Mon, 29 Oct 2018 15:21:10 +0800 Subject: [PATCH 01/10] Support cookie expires --- .../annotations/sessionaffinity/main.go | 36 +++++- .../annotations/sessionaffinity/main_test.go | 10 ++ internal/ingress/controller/controller.go | 2 + internal/ingress/types.go | 2 + rootfs/etc/nginx/lua/balancer/sticky.lua | 49 +++++++- .../nginx/lua/test/balancer/sticky_test.lua | 117 ++++++++++++++++++ 6 files changed, 209 insertions(+), 7 deletions(-) diff --git a/internal/ingress/annotations/sessionaffinity/main.go b/internal/ingress/annotations/sessionaffinity/main.go index 49de25d33..35ae09335 100644 --- a/internal/ingress/annotations/sessionaffinity/main.go +++ b/internal/ingress/annotations/sessionaffinity/main.go @@ -39,10 +39,21 @@ const ( // one isn't supplied and affinity is set to "cookie". annotationAffinityCookieHash = "session-cookie-hash" defaultAffinityCookieHash = "md5" + + // This is used to control the cookie expires, its value is a time period, the cookie will + // not expire during the time period. e.g. 1h (1 hour) the validated time unit includes: + // y, M, w, d, h, m and s + annotationAffinityCookieExpires = "session-cookie-expires" + + // This is used to control the cookie expires, its value is a number of seconds until the + // cookie expires + annotationAffinityCookieMaxAge = "session-cookie-max-age" ) var ( - affinityCookieHashRegex = regexp.MustCompile(`^(index|md5|sha1)$`) + affinityCookieHashRegex = regexp.MustCompile(`^(index|md5|sha1)$`) + affinityCookieExpiresRegex = regexp.MustCompile(`^(\d+(y|M|w|d|h|m|s))$`) + affinityCookieMaxAgeRegex = regexp.MustCompile(`(^0|-?[1-9]\d*$)`) ) // Config describes the per ingress session affinity config @@ -58,17 +69,23 @@ type Cookie struct { Name string `json:"name"` // The hash that will be used to encode the cookie in case of cookie affinity type Hash string `json:"hash"` + // The time period to control cookie expires + Expires string `json:"expires"` + // The number of seconds until the cookie expires + MaxAge string `json:"maxage"` } // cookieAffinityParse gets the annotation values related to Cookie Affinity // It also sets default values when no value or incorrect value is found func (a affinity) cookieAffinityParse(ing *extensions.Ingress) *Cookie { + cookie := &Cookie{} sn, err := parser.GetStringAnnotation(annotationAffinityCookieName, ing) if err != nil || sn == "" { glog.V(3).Infof("Ingress %v: No value found in annotation %v. Using the default %v", ing.Name, annotationAffinityCookieName, defaultAffinityCookieName) sn = defaultAffinityCookieName } + cookie.Name = sn sh, err := parser.GetStringAnnotation(annotationAffinityCookieHash, ing) @@ -76,11 +93,22 @@ func (a affinity) cookieAffinityParse(ing *extensions.Ingress) *Cookie { glog.V(3).Infof("Invalid or no annotation value found in Ingress %v: %v. Setting it to default %v", ing.Name, annotationAffinityCookieHash, defaultAffinityCookieHash) sh = defaultAffinityCookieHash } + cookie.Hash = sh - return &Cookie{ - Name: sn, - Hash: sh, + se, err := parser.GetStringAnnotation(annotationAffinityCookieExpires, ing) + if err != nil && !affinityCookieExpiresRegex.MatchString(se) { + glog.V(3).Infof("Invalid or no annotation value found in Ingress %v: %v. Ignoring it", ing.Name, annotationAffinityCookieExpires) + se = "" } + cookie.Expires = se + + sm, err := parser.GetStringAnnotation(annotationAffinityCookieMaxAge, ing) + if err != nil && !affinityCookieMaxAgeRegex.MatchString(sm) { + glog.V(3).Infof("Invalid or no annotation value found in Ingress %v: %v. Ignoring it", ing.Name, annotationAffinityCookieMaxAge) + sm = "" + } + cookie.MaxAge = sm + return cookie } // NewParser creates a new Affinity annotation parser diff --git a/internal/ingress/annotations/sessionaffinity/main_test.go b/internal/ingress/annotations/sessionaffinity/main_test.go index 3db53a2f7..d00e41c20 100644 --- a/internal/ingress/annotations/sessionaffinity/main_test.go +++ b/internal/ingress/annotations/sessionaffinity/main_test.go @@ -69,6 +69,8 @@ func TestIngressAffinityCookieConfig(t *testing.T) { data[parser.GetAnnotationWithPrefix(annotationAffinityType)] = "cookie" data[parser.GetAnnotationWithPrefix(annotationAffinityCookieHash)] = "sha123" data[parser.GetAnnotationWithPrefix(annotationAffinityCookieName)] = "INGRESSCOOKIE" + data[parser.GetAnnotationWithPrefix(annotationAffinityCookieExpires)] = "1h" + data[parser.GetAnnotationWithPrefix(annotationAffinityCookieMaxAge)] = "3000" ing.SetAnnotations(data) affin, _ := NewParser(&resolver.Mock{}).Parse(ing) @@ -88,4 +90,12 @@ func TestIngressAffinityCookieConfig(t *testing.T) { if nginxAffinity.Cookie.Name != "INGRESSCOOKIE" { t.Errorf("expected route as sticky-name but returned %v", nginxAffinity.Cookie.Name) } + + if nginxAffinity.Cookie.Expires != "1h" { + t.Errorf("expected 1h as sticky-expires but returned %v", nginxAffinity.Cookie.Expires) + } + + if nginxAffinity.Cookie.MaxAge != "3000" { + t.Errorf("expected 3000 as sticky-max-age but returned %v", nginxAffinity.Cookie.MaxAge) + } } diff --git a/internal/ingress/controller/controller.go b/internal/ingress/controller/controller.go index d070c5e12..e211db233 100644 --- a/internal/ingress/controller/controller.go +++ b/internal/ingress/controller/controller.go @@ -405,6 +405,8 @@ func (n *NGINXController) getBackendServers(ingresses []*extensions.Ingress) ([] if anns.SessionAffinity.Type == "cookie" { ups.SessionAffinity.CookieSessionAffinity.Name = anns.SessionAffinity.Cookie.Name ups.SessionAffinity.CookieSessionAffinity.Hash = anns.SessionAffinity.Cookie.Hash + ups.SessionAffinity.CookieSessionAffinity.Expires = anns.SessionAffinity.Cookie.Expires + ups.SessionAffinity.CookieSessionAffinity.MaxAge = anns.SessionAffinity.Cookie.MaxAge locs := ups.SessionAffinity.CookieSessionAffinity.Locations if _, ok := locs[host]; !ok { diff --git a/internal/ingress/types.go b/internal/ingress/types.go index fcf496108..fea5b0c28 100644 --- a/internal/ingress/types.go +++ b/internal/ingress/types.go @@ -109,6 +109,8 @@ type SessionAffinityConfig struct { type CookieSessionAffinity struct { Name string `json:"name"` Hash string `json:"hash"` + Expires string `json:"expires,omitempty"` + MaxAge string `json:"maxage,omitempty"` Locations map[string][]string `json:"locations,omitempty"` } diff --git a/rootfs/etc/nginx/lua/balancer/sticky.lua b/rootfs/etc/nginx/lua/balancer/sticky.lua index 4d4684146..fc9387dd5 100644 --- a/rootfs/etc/nginx/lua/balancer/sticky.lua +++ b/rootfs/etc/nginx/lua/balancer/sticky.lua @@ -15,6 +15,8 @@ function _M.new(self, backend) local o = { instance = self.factory:new(nodes), cookie_name = backend["sessionAffinityConfig"]["cookieSessionAffinity"]["name"] or "route", + cookie_expires = backend["sessionAffinityConfig"]["cookieSessionAffinity"]["expires"], + cookie_max_age = backend["sessionAffinityConfig"]["cookieSessionAffinity"]["maxage"], digest_func = digest_func, } setmetatable(o, self) @@ -31,20 +33,61 @@ local function encrypted_endpoint_string(self, endpoint_string) return encrypted end +local function parse_cookie_expires(expires) + local time = tonumber(string.sub(expires, 0, string.len(expires) - 1)) + if time == nil then + return nil, string.format("the time of expires (%s) is wrong", expires) + end + + local unit = string.sub(expires, -1) + if unit == "y" then + return time * 60 * 60 * 24 * 365 + elseif unit == "M" then + return time * 60 * 60 * 24 * 30 + elseif unit == "w" then + return time * 60 * 60 * 24 * 7 + elseif unit == "d" then + return time * 60 * 60 * 24 + elseif unit == "h" then + return time * 60 * 60 + elseif unit == "m" then + return time * 60 + elseif unit == "s" then + return time + else + return nil, string.format("the unit of expires (%s) is wrong, validated unit includes: y, M, w, d, h, m and s", expires) + end +end + local function set_cookie(self, value) local cookie, err = ck:new() if not cookie then ngx.log(ngx.ERR, err) end - local ok - ok, err = cookie:set({ + local cookie_data = { key = self.cookie_name, value = value, path = ngx.var.location_path, domain = ngx.var.host, httponly = true, - }) + } + + if self.cookie_expires then + local expires, err = parse_cookie_expires(self.cookie_expires) + if err then + ngx.log(ngx.WARN, string.format("error when parsing cookie expires: %s, ignoring it", tostring(err))) + else + cookie_data.expires = ngx.cookie_time(expires) + end + end + + if self.cookie_max_age then + cookie_data.max_age = tonumber(self.cookie_max_age) + end + + local ok + ok, err = cookie:set(cookie_data) if not ok then ngx.log(ngx.ERR, err) end diff --git a/rootfs/etc/nginx/lua/test/balancer/sticky_test.lua b/rootfs/etc/nginx/lua/test/balancer/sticky_test.lua index d5c402236..264acf39c 100644 --- a/rootfs/etc/nginx/lua/test/balancer/sticky_test.lua +++ b/rootfs/etc/nginx/lua/test/balancer/sticky_test.lua @@ -88,6 +88,38 @@ describe("Sticky", function() assert.equal(sticky_balancer_instance.digest_func, default_hash_implementation) end) end) + + context("when backend specifies cookie expires", function() + it("returns an instance containing the corresponding cookie expires", function() + local temp_backend = util.deepcopy(test_backend) + temp_backend.sessionAffinityConfig.cookieSessionAffinity.expires = "1y" + local sticky_balancer_instance = sticky:new(temp_backend) + assert.equal(sticky_balancer_instance.cookie_expires, "1y") + end) + end) + + context("when backend does not specify cookie expires", function() + it("returns an instance without cookie expires", function() + local sticky_balancer_instance = sticky:new(test_backend) + assert.equal(sticky_balancer_instance.cookie_expires, nil) + end) + end) + + context("when backend specifies cookie max-age", function() + it("returns an instance containing the corresponding cookie max-age", function() + local temp_backend = util.deepcopy(test_backend) + temp_backend.sessionAffinityConfig.cookieSessionAffinity.maxage = 1000 + local sticky_balancer_instance = sticky:new(temp_backend) + assert.equal(sticky_balancer_instance.cookie_max_age, 1000) + end) + end) + + context("when backend does not specify cookie max-age", function() + it("returns an instance without cookie max-age", function() + local sticky_balancer_instance = sticky:new(test_backend) + assert.equal(sticky_balancer_instance.cookie_max_age, nil) + end) + end) end) describe("balance()", function() @@ -131,6 +163,91 @@ describe("Sticky", function() assert.has_no.errors(function() sticky_balancer_instance:balance() end) assert.spy(s).was_called() end) + + it("sets an expires cookie (with expires) on the client", function() + local s = {} + local expected_expires + cookie.new = function(self) + local test_backend_hash_fn = test_backend.sessionAffinityConfig.cookieSessionAffinity.hash + local cookie_instance = { + set = function(self, payload) + assert.equal(payload.key, test_backend.sessionAffinityConfig.cookieSessionAffinity.name) + assert.equal(payload.value, util[test_backend_hash_fn .. "_digest"](test_backend_endpoint)) + assert.equal(payload.path, ngx.var.location_path) + assert.equal(payload.domain, nil) + assert.equal(payload.httponly, true) + expected_expires = payload.expires + return true, nil + end, + get = function(k) return false end, + } + s = spy.on(cookie_instance, "set") + return cookie_instance, false + end + local temp_backend = util.deepcopy(get_test_backend()) + temp_backend.sessionAffinityConfig.cookieSessionAffinity.expires = "1y" + local sticky_balancer_instance = sticky:new(temp_backend) + assert.has_no.errors(function() sticky_balancer_instance:balance() end) + assert.spy(s).was_called() + assert.equal(expected_expires, ngx.cookie_time(1 * 60 * 60 * 24 * 365)) + temp_backend.sessionAffinityConfig.cookieSessionAffinity.expires = "10M" + sticky_balancer_instance = sticky:new(temp_backend) + assert.has_no.errors(function() sticky_balancer_instance:balance() end) + assert.spy(s).was_called() + assert.equal(expected_expires, ngx.cookie_time(10 * 60 * 60 * 24 * 30)) + temp_backend.sessionAffinityConfig.cookieSessionAffinity.expires = "8w" + sticky_balancer_instance = sticky:new(temp_backend) + assert.has_no.errors(function() sticky_balancer_instance:balance() end) + assert.spy(s).was_called() + assert.equal(expected_expires, ngx.cookie_time(8 * 60 * 60 * 24 * 7)) + temp_backend.sessionAffinityConfig.cookieSessionAffinity.expires = "200d" + sticky_balancer_instance = sticky:new(temp_backend) + assert.has_no.errors(function() sticky_balancer_instance:balance() end) + assert.spy(s).was_called() + assert.equal(expected_expires, ngx.cookie_time(200 * 60 * 60 * 24)) + temp_backend.sessionAffinityConfig.cookieSessionAffinity.expires = "24h" + sticky_balancer_instance = sticky:new(temp_backend) + assert.has_no.errors(function() sticky_balancer_instance:balance() end) + assert.spy(s).was_called() + assert.equal(expected_expires, ngx.cookie_time(24 * 60 * 60)) + temp_backend.sessionAffinityConfig.cookieSessionAffinity.expires = "30m" + sticky_balancer_instance = sticky:new(temp_backend) + assert.has_no.errors(function() sticky_balancer_instance:balance() end) + assert.spy(s).was_called() + assert.equal(expected_expires, ngx.cookie_time(30 * 60)) + temp_backend.sessionAffinityConfig.cookieSessionAffinity.expires = "300s" + sticky_balancer_instance = sticky:new(temp_backend) + assert.has_no.errors(function() sticky_balancer_instance:balance() end) + assert.spy(s).was_called() + assert.equal(expected_expires, ngx.cookie_time(300)) + end) + + it("sets an expires cookie (with max-age) on the client", function() + local s = {} + local cookie_payload = {} + cookie.new = function(self) + local test_backend_hash_fn = test_backend.sessionAffinityConfig.cookieSessionAffinity.hash + local cookie_instance = { + set = function(self, payload) + assert.equal(payload.key, test_backend.sessionAffinityConfig.cookieSessionAffinity.name) + assert.equal(payload.value, util[test_backend_hash_fn .. "_digest"](test_backend_endpoint)) + assert.equal(payload.path, ngx.var.location_path) + assert.equal(payload.domain, nil) + assert.equal(payload.httponly, true) + assert.equal(payload.max_age, 1000) + return true, nil + end, + get = function(k) return false end, + } + s = spy.on(cookie_instance, "set") + return cookie_instance, false + end + local temp_backend = util.deepcopy(get_test_backend()) + temp_backend.sessionAffinityConfig.cookieSessionAffinity.maxage = "1000" + local sticky_balancer_instance = sticky:new(temp_backend) + assert.has_no.errors(function() sticky_balancer_instance:balance() end) + assert.spy(s).was_called() + end) end) context("when client has a cookie set", function() From 7de718f35935f3aa81b11f1167cbf5ec79338e8d Mon Sep 17 00:00:00 2001 From: liuwei Date: Mon, 29 Oct 2018 15:39:43 +0800 Subject: [PATCH 02/10] pass code static-check --- rootfs/etc/nginx/lua/balancer/sticky.lua | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/rootfs/etc/nginx/lua/balancer/sticky.lua b/rootfs/etc/nginx/lua/balancer/sticky.lua index fc9387dd5..08811b4c3 100644 --- a/rootfs/etc/nginx/lua/balancer/sticky.lua +++ b/rootfs/etc/nginx/lua/balancer/sticky.lua @@ -55,7 +55,7 @@ local function parse_cookie_expires(expires) elseif unit == "s" then return time else - return nil, string.format("the unit of expires (%s) is wrong, validated unit includes: y, M, w, d, h, m and s", expires) + return nil, string.format("the unit of expires (%s) is wrong", expires) end end @@ -73,8 +73,9 @@ local function set_cookie(self, value) httponly = true, } + local expires if self.cookie_expires then - local expires, err = parse_cookie_expires(self.cookie_expires) + expires, err = parse_cookie_expires(self.cookie_expires) if err then ngx.log(ngx.WARN, string.format("error when parsing cookie expires: %s, ignoring it", tostring(err))) else From 38279366a53ebc0a138e6597e8b303b4a86405bc Mon Sep 17 00:00:00 2001 From: liuwei Date: Tue, 30 Oct 2018 19:27:21 +0800 Subject: [PATCH 03/10] add e2e test for cookie annotations --- rootfs/etc/nginx/lua/balancer/sticky.lua | 4 +-- .../nginx/lua/test/balancer/sticky_test.lua | 32 ------------------- test/e2e/lua/dynamic_configuration.go | 23 ++++++++++++- 3 files changed, 24 insertions(+), 35 deletions(-) diff --git a/rootfs/etc/nginx/lua/balancer/sticky.lua b/rootfs/etc/nginx/lua/balancer/sticky.lua index 08811b4c3..1ba695258 100644 --- a/rootfs/etc/nginx/lua/balancer/sticky.lua +++ b/rootfs/etc/nginx/lua/balancer/sticky.lua @@ -74,7 +74,7 @@ local function set_cookie(self, value) } local expires - if self.cookie_expires then + if self.cookie_expires and self.cookie_expires ~= "" then expires, err = parse_cookie_expires(self.cookie_expires) if err then ngx.log(ngx.WARN, string.format("error when parsing cookie expires: %s, ignoring it", tostring(err))) @@ -83,7 +83,7 @@ local function set_cookie(self, value) end end - if self.cookie_max_age then + if self.cookie_max_age and self.cookie_max_age ~= "" then cookie_data.max_age = tonumber(self.cookie_max_age) end diff --git a/rootfs/etc/nginx/lua/test/balancer/sticky_test.lua b/rootfs/etc/nginx/lua/test/balancer/sticky_test.lua index 264acf39c..87f0fc186 100644 --- a/rootfs/etc/nginx/lua/test/balancer/sticky_test.lua +++ b/rootfs/etc/nginx/lua/test/balancer/sticky_test.lua @@ -88,38 +88,6 @@ describe("Sticky", function() assert.equal(sticky_balancer_instance.digest_func, default_hash_implementation) end) end) - - context("when backend specifies cookie expires", function() - it("returns an instance containing the corresponding cookie expires", function() - local temp_backend = util.deepcopy(test_backend) - temp_backend.sessionAffinityConfig.cookieSessionAffinity.expires = "1y" - local sticky_balancer_instance = sticky:new(temp_backend) - assert.equal(sticky_balancer_instance.cookie_expires, "1y") - end) - end) - - context("when backend does not specify cookie expires", function() - it("returns an instance without cookie expires", function() - local sticky_balancer_instance = sticky:new(test_backend) - assert.equal(sticky_balancer_instance.cookie_expires, nil) - end) - end) - - context("when backend specifies cookie max-age", function() - it("returns an instance containing the corresponding cookie max-age", function() - local temp_backend = util.deepcopy(test_backend) - temp_backend.sessionAffinityConfig.cookieSessionAffinity.maxage = 1000 - local sticky_balancer_instance = sticky:new(temp_backend) - assert.equal(sticky_balancer_instance.cookie_max_age, 1000) - end) - end) - - context("when backend does not specify cookie max-age", function() - it("returns an instance without cookie max-age", function() - local sticky_balancer_instance = sticky:new(test_backend) - assert.equal(sticky_balancer_instance.cookie_max_age, nil) - end) - end) end) describe("balance()", function() diff --git a/test/e2e/lua/dynamic_configuration.go b/test/e2e/lua/dynamic_configuration.go index a3f8c7037..2177824c1 100644 --- a/test/e2e/lua/dynamic_configuration.go +++ b/test/e2e/lua/dynamic_configuration.go @@ -156,11 +156,32 @@ var _ = framework.IngressNginxDescribe("Dynamic Configuration", func() { Expect(nginxConfig).ShouldNot(Equal(newNginxConfig)) }) + + It("should set sticky cookie correctly", func() { + resp, _, errs := gorequest.New(). + Get(f.IngressController.HTTPURL). + Set("Host", "foo.com"). + End() + + Expect(len(errs)).Should(BeNumerically("==", 0)) + Expect(resp.StatusCode).Should(Equal(http.StatusOK)) + //fmt.Println("--------->") + //fmt.Println(resp.Header.Get("Set-Cookie")) + //fmt.Println("--------->") + Expect(resp.Header.Get("Set-Cookie")).Should(ContainSubstring("SERVERID=")) + Expect(resp.Header.Get("Set-Cookie")).Should(ContainSubstring("Expires=")) + Expect(resp.Header.Get("Set-Cookie")).Should(ContainSubstring("Max-Age=172800")) + }) }) func ensureIngress(f *framework.Framework, host string) *extensions.Ingress { ing, err := f.EnsureIngress(framework.NewSingleIngress(host, "/", host, f.IngressController.Namespace, "http-svc", 80, - &map[string]string{"nginx.ingress.kubernetes.io/load-balance": "ewma"})) + &map[string]string{ + "nginx.ingress.kubernetes.io/load-balance": "ewma", + "nginx.ingress.kubernetes.io/affinity": "cookie", + "nginx.ingress.kubernetes.io/session-cookie-name": "SERVERID", + "nginx.ingress.kubernetes.io/session-cookie-expires": "2d", + "nginx.ingress.kubernetes.io/session-cookie-max-age": "172800"})) Expect(err).NotTo(HaveOccurred()) Expect(ing).NotTo(BeNil()) err = f.WaitForNginxServer(host, From 6d2d42ee0bdde381e7d148ee8c0ab79e1897e891 Mon Sep 17 00:00:00 2001 From: liuwei Date: Tue, 30 Oct 2018 19:31:17 +0800 Subject: [PATCH 04/10] remove some useless comments --- test/e2e/lua/dynamic_configuration.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/test/e2e/lua/dynamic_configuration.go b/test/e2e/lua/dynamic_configuration.go index 2177824c1..19c5e70a6 100644 --- a/test/e2e/lua/dynamic_configuration.go +++ b/test/e2e/lua/dynamic_configuration.go @@ -165,9 +165,6 @@ var _ = framework.IngressNginxDescribe("Dynamic Configuration", func() { Expect(len(errs)).Should(BeNumerically("==", 0)) Expect(resp.StatusCode).Should(Equal(http.StatusOK)) - //fmt.Println("--------->") - //fmt.Println(resp.Header.Get("Set-Cookie")) - //fmt.Println("--------->") Expect(resp.Header.Get("Set-Cookie")).Should(ContainSubstring("SERVERID=")) Expect(resp.Header.Get("Set-Cookie")).Should(ContainSubstring("Expires=")) Expect(resp.Header.Get("Set-Cookie")).Should(ContainSubstring("Max-Age=172800")) From c74e59fa4c001c7d75d6f1f307ccc8cf6817c1ea Mon Sep 17 00:00:00 2001 From: liuwei Date: Fri, 2 Nov 2018 17:05:38 +0800 Subject: [PATCH 05/10] Use second as cookie expires unit --- .../annotations/sessionaffinity/main.go | 14 ++- .../annotations/sessionaffinity/main_test.go | 4 +- rootfs/etc/nginx/lua/balancer/sticky.lua | 33 +------ .../nginx/lua/test/balancer/sticky_test.lua | 87 +------------------ test/e2e/annotations/affinity.go | 33 ++++++- test/e2e/lua/dynamic_configuration.go | 25 +----- 6 files changed, 45 insertions(+), 151 deletions(-) diff --git a/internal/ingress/annotations/sessionaffinity/main.go b/internal/ingress/annotations/sessionaffinity/main.go index 35ae09335..09599b6b5 100644 --- a/internal/ingress/annotations/sessionaffinity/main.go +++ b/internal/ingress/annotations/sessionaffinity/main.go @@ -40,9 +40,8 @@ const ( annotationAffinityCookieHash = "session-cookie-hash" defaultAffinityCookieHash = "md5" - // This is used to control the cookie expires, its value is a time period, the cookie will - // not expire during the time period. e.g. 1h (1 hour) the validated time unit includes: - // y, M, w, d, h, m and s + // This is used to control the cookie expires, its value is a number of seconds until the + // cookie expires annotationAffinityCookieExpires = "session-cookie-expires" // This is used to control the cookie expires, its value is a number of seconds until the @@ -52,8 +51,7 @@ const ( var ( affinityCookieHashRegex = regexp.MustCompile(`^(index|md5|sha1)$`) - affinityCookieExpiresRegex = regexp.MustCompile(`^(\d+(y|M|w|d|h|m|s))$`) - affinityCookieMaxAgeRegex = regexp.MustCompile(`(^0|-?[1-9]\d*$)`) + affinityCookieExpiresRegex = regexp.MustCompile(`(^0|-?[1-9]\d*$)`) ) // Config describes the per ingress session affinity config @@ -69,7 +67,7 @@ type Cookie struct { Name string `json:"name"` // The hash that will be used to encode the cookie in case of cookie affinity type Hash string `json:"hash"` - // The time period to control cookie expires + // The time duration to control cookie expires Expires string `json:"expires"` // The number of seconds until the cookie expires MaxAge string `json:"maxage"` @@ -96,14 +94,14 @@ func (a affinity) cookieAffinityParse(ing *extensions.Ingress) *Cookie { cookie.Hash = sh se, err := parser.GetStringAnnotation(annotationAffinityCookieExpires, ing) - if err != nil && !affinityCookieExpiresRegex.MatchString(se) { + if err != nil || !affinityCookieExpiresRegex.MatchString(se) { glog.V(3).Infof("Invalid or no annotation value found in Ingress %v: %v. Ignoring it", ing.Name, annotationAffinityCookieExpires) se = "" } cookie.Expires = se sm, err := parser.GetStringAnnotation(annotationAffinityCookieMaxAge, ing) - if err != nil && !affinityCookieMaxAgeRegex.MatchString(sm) { + if err != nil || !affinityCookieExpiresRegex.MatchString(sm) { glog.V(3).Infof("Invalid or no annotation value found in Ingress %v: %v. Ignoring it", ing.Name, annotationAffinityCookieMaxAge) sm = "" } diff --git a/internal/ingress/annotations/sessionaffinity/main_test.go b/internal/ingress/annotations/sessionaffinity/main_test.go index d00e41c20..a3e89d662 100644 --- a/internal/ingress/annotations/sessionaffinity/main_test.go +++ b/internal/ingress/annotations/sessionaffinity/main_test.go @@ -69,7 +69,7 @@ func TestIngressAffinityCookieConfig(t *testing.T) { data[parser.GetAnnotationWithPrefix(annotationAffinityType)] = "cookie" data[parser.GetAnnotationWithPrefix(annotationAffinityCookieHash)] = "sha123" data[parser.GetAnnotationWithPrefix(annotationAffinityCookieName)] = "INGRESSCOOKIE" - data[parser.GetAnnotationWithPrefix(annotationAffinityCookieExpires)] = "1h" + data[parser.GetAnnotationWithPrefix(annotationAffinityCookieExpires)] = "4500" data[parser.GetAnnotationWithPrefix(annotationAffinityCookieMaxAge)] = "3000" ing.SetAnnotations(data) @@ -91,7 +91,7 @@ func TestIngressAffinityCookieConfig(t *testing.T) { t.Errorf("expected route as sticky-name but returned %v", nginxAffinity.Cookie.Name) } - if nginxAffinity.Cookie.Expires != "1h" { + if nginxAffinity.Cookie.Expires != "4500" { t.Errorf("expected 1h as sticky-expires but returned %v", nginxAffinity.Cookie.Expires) } diff --git a/rootfs/etc/nginx/lua/balancer/sticky.lua b/rootfs/etc/nginx/lua/balancer/sticky.lua index fd1af8ed3..15f3f3971 100644 --- a/rootfs/etc/nginx/lua/balancer/sticky.lua +++ b/rootfs/etc/nginx/lua/balancer/sticky.lua @@ -33,32 +33,6 @@ local function encrypted_endpoint_string(self, endpoint_string) return encrypted end -local function parse_cookie_expires(expires) - local time = tonumber(string.sub(expires, 0, string.len(expires) - 1)) - if time == nil then - return nil, string.format("the time of expires (%s) is wrong", expires) - end - - local unit = string.sub(expires, -1) - if unit == "y" then - return time * 60 * 60 * 24 * 365 - elseif unit == "M" then - return time * 60 * 60 * 24 * 30 - elseif unit == "w" then - return time * 60 * 60 * 24 * 7 - elseif unit == "d" then - return time * 60 * 60 * 24 - elseif unit == "h" then - return time * 60 * 60 - elseif unit == "m" then - return time * 60 - elseif unit == "s" then - return time - else - return nil, string.format("the unit of expires (%s) is wrong", expires) - end -end - local function set_cookie(self, value) local cookie, err = ck:new() if not cookie then @@ -75,12 +49,7 @@ local function set_cookie(self, value) local expires if self.cookie_expires and self.cookie_expires ~= "" then - expires, err = parse_cookie_expires(self.cookie_expires) - if err then - ngx.log(ngx.WARN, string.format("error when parsing cookie expires: %s, ignoring it", tostring(err))) - else - cookie_data.expires = ngx.cookie_time(expires) - end + cookie_data.expires = ngx.cookie_time(tonumber(self.cookie_expires)) end if self.cookie_max_age and self.cookie_max_age ~= "" then diff --git a/rootfs/etc/nginx/lua/test/balancer/sticky_test.lua b/rootfs/etc/nginx/lua/test/balancer/sticky_test.lua index a6b4652ad..c38773885 100644 --- a/rootfs/etc/nginx/lua/test/balancer/sticky_test.lua +++ b/rootfs/etc/nginx/lua/test/balancer/sticky_test.lua @@ -132,91 +132,6 @@ describe("Sticky", function() assert.has_no.errors(function() sticky_balancer_instance:balance() end) assert.spy(s).was_called() end) - - it("sets an expires cookie (with expires) on the client", function() - local s = {} - local expected_expires - cookie.new = function(self) - local test_backend_hash_fn = test_backend.sessionAffinityConfig.cookieSessionAffinity.hash - local cookie_instance = { - set = function(self, payload) - assert.equal(payload.key, test_backend.sessionAffinityConfig.cookieSessionAffinity.name) - assert.equal(payload.value, util[test_backend_hash_fn .. "_digest"](test_backend_endpoint)) - assert.equal(payload.path, ngx.var.location_path) - assert.equal(payload.domain, nil) - assert.equal(payload.httponly, true) - expected_expires = payload.expires - return true, nil - end, - get = function(k) return false end, - } - s = spy.on(cookie_instance, "set") - return cookie_instance, false - end - local temp_backend = util.deepcopy(get_test_backend()) - temp_backend.sessionAffinityConfig.cookieSessionAffinity.expires = "1y" - local sticky_balancer_instance = sticky:new(temp_backend) - assert.has_no.errors(function() sticky_balancer_instance:balance() end) - assert.spy(s).was_called() - assert.equal(expected_expires, ngx.cookie_time(1 * 60 * 60 * 24 * 365)) - temp_backend.sessionAffinityConfig.cookieSessionAffinity.expires = "10M" - sticky_balancer_instance = sticky:new(temp_backend) - assert.has_no.errors(function() sticky_balancer_instance:balance() end) - assert.spy(s).was_called() - assert.equal(expected_expires, ngx.cookie_time(10 * 60 * 60 * 24 * 30)) - temp_backend.sessionAffinityConfig.cookieSessionAffinity.expires = "8w" - sticky_balancer_instance = sticky:new(temp_backend) - assert.has_no.errors(function() sticky_balancer_instance:balance() end) - assert.spy(s).was_called() - assert.equal(expected_expires, ngx.cookie_time(8 * 60 * 60 * 24 * 7)) - temp_backend.sessionAffinityConfig.cookieSessionAffinity.expires = "200d" - sticky_balancer_instance = sticky:new(temp_backend) - assert.has_no.errors(function() sticky_balancer_instance:balance() end) - assert.spy(s).was_called() - assert.equal(expected_expires, ngx.cookie_time(200 * 60 * 60 * 24)) - temp_backend.sessionAffinityConfig.cookieSessionAffinity.expires = "24h" - sticky_balancer_instance = sticky:new(temp_backend) - assert.has_no.errors(function() sticky_balancer_instance:balance() end) - assert.spy(s).was_called() - assert.equal(expected_expires, ngx.cookie_time(24 * 60 * 60)) - temp_backend.sessionAffinityConfig.cookieSessionAffinity.expires = "30m" - sticky_balancer_instance = sticky:new(temp_backend) - assert.has_no.errors(function() sticky_balancer_instance:balance() end) - assert.spy(s).was_called() - assert.equal(expected_expires, ngx.cookie_time(30 * 60)) - temp_backend.sessionAffinityConfig.cookieSessionAffinity.expires = "300s" - sticky_balancer_instance = sticky:new(temp_backend) - assert.has_no.errors(function() sticky_balancer_instance:balance() end) - assert.spy(s).was_called() - assert.equal(expected_expires, ngx.cookie_time(300)) - end) - - it("sets an expires cookie (with max-age) on the client", function() - local s = {} - local cookie_payload = {} - cookie.new = function(self) - local test_backend_hash_fn = test_backend.sessionAffinityConfig.cookieSessionAffinity.hash - local cookie_instance = { - set = function(self, payload) - assert.equal(payload.key, test_backend.sessionAffinityConfig.cookieSessionAffinity.name) - assert.equal(payload.value, util[test_backend_hash_fn .. "_digest"](test_backend_endpoint)) - assert.equal(payload.path, ngx.var.location_path) - assert.equal(payload.domain, nil) - assert.equal(payload.httponly, true) - assert.equal(payload.max_age, 1000) - return true, nil - end, - get = function(k) return false end, - } - s = spy.on(cookie_instance, "set") - return cookie_instance, false - end - local temp_backend = util.deepcopy(get_test_backend()) - temp_backend.sessionAffinityConfig.cookieSessionAffinity.maxage = "1000" - local sticky_balancer_instance = sticky:new(temp_backend) - assert.has_no.errors(function() sticky_balancer_instance:balance() end) - assert.spy(s).was_called() - end) end) context("when client has a cookie set", function() @@ -242,4 +157,4 @@ describe("Sticky", function() end) end) end) -end) +end) \ No newline at end of file diff --git a/test/e2e/annotations/affinity.go b/test/e2e/annotations/affinity.go index 5903f7e15..4b8c028db 100644 --- a/test/e2e/annotations/affinity.go +++ b/test/e2e/annotations/affinity.go @@ -20,6 +20,7 @@ import ( "fmt" "net/http" "strings" + "time" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" @@ -28,7 +29,6 @@ import ( "k8s.io/api/extensions/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" - "k8s.io/ingress-nginx/test/e2e/framework" ) @@ -157,4 +157,35 @@ var _ = framework.IngressNginxDescribe("Annotations - Affinity/Sticky Sessions", Expect(resp.StatusCode).Should(Equal(http.StatusOK)) Expect(resp.Header.Get("Set-Cookie")).Should(ContainSubstring("Path=/somewhereelese;")) }) + + It("should set sticky cookie with expires", func() { + host := "sticky.foo.com" + annotations := map[string]string{ + "nginx.ingress.kubernetes.io/affinity": "cookie", + "nginx.ingress.kubernetes.io/session-cookie-name": "SERVERID", + "nginx.ingress.kubernetes.io/session-cookie-expires": "172800", + "nginx.ingress.kubernetes.io/session-cookie-max-age": "259200", + } + + ing := framework.NewSingleIngress(host, "/", host, f.IngressController.Namespace, "http-svc", 80, &annotations) + f.EnsureIngress(ing) + + f.WaitForNginxServer(host, + func(server string) bool { + return strings.Contains(server, fmt.Sprintf("server_name %s ;", host)) + }) + + resp, _, errs := gorequest.New(). + Get(f.IngressController.HTTPURL). + Set("Host", host). + End() + + Expect(len(errs)).Should(BeNumerically("==", 0)) + Expect(resp.StatusCode).Should(Equal(http.StatusOK)) + local, _ := time.LoadLocation("GMT") + duration, _ := time.ParseDuration("48h") + want := time.Date(1970, time.January, 1, 0, 0, 0, 0, local).Add(duration).Format("Mon, 02-Jan-06 15:04:05 MST") + Expect(resp.Header.Get("Set-Cookie")).Should(ContainSubstring(fmt.Sprintf("Expires=%s", want))) + Expect(resp.Header.Get("Set-Cookie")).Should(ContainSubstring("Max-Age=259200")) + }) }) diff --git a/test/e2e/lua/dynamic_configuration.go b/test/e2e/lua/dynamic_configuration.go index 74ddf428b..38bb9ffd2 100644 --- a/test/e2e/lua/dynamic_configuration.go +++ b/test/e2e/lua/dynamic_configuration.go @@ -149,31 +149,12 @@ var _ = framework.IngressNginxDescribe("Dynamic Configuration", func() { }) Expect(nginxConfig).ShouldNot(Equal(newNginxConfig)) }) - - It("should set sticky cookie correctly", func() { - resp, _, errs := gorequest.New(). - Get(f.IngressController.HTTPURL). - Set("Host", "foo.com"). - End() - - Expect(len(errs)).Should(BeNumerically("==", 0)) - Expect(resp.StatusCode).Should(Equal(http.StatusOK)) - Expect(resp.Header.Get("Set-Cookie")).Should(ContainSubstring("SERVERID=")) - Expect(resp.Header.Get("Set-Cookie")).Should(ContainSubstring("Expires=")) - Expect(resp.Header.Get("Set-Cookie")).Should(ContainSubstring("Max-Age=172800")) - }) }) func ensureIngress(f *framework.Framework, host string) *extensions.Ingress { - ing, err := f.EnsureIngress(framework.NewSingleIngress(host, "/", host, f.IngressController.Namespace, "http-svc", 80, - &map[string]string{ - "nginx.ingress.kubernetes.io/load-balance": "ewma", - "nginx.ingress.kubernetes.io/affinity": "cookie", - "nginx.ingress.kubernetes.io/session-cookie-name": "SERVERID", - "nginx.ingress.kubernetes.io/session-cookie-expires": "2d", - "nginx.ingress.kubernetes.io/session-cookie-max-age": "172800"})) - Expect(err).NotTo(HaveOccurred()) - Expect(ing).NotTo(BeNil()) + ing := f.EnsureIngress(framework.NewSingleIngress(host, "/", host, f.IngressController.Namespace, "http-svc", 80, + &map[string]string{"nginx.ingress.kubernetes.io/load-balance": "ewma"})) + f.WaitForNginxServer(host, func(server string) bool { return strings.Contains(server, fmt.Sprintf("server_name %s ;", host)) && From 3477df4c12748b5156ce171635b6c9547ebecbf1 Mon Sep 17 00:00:00 2001 From: liuwei Date: Fri, 2 Nov 2018 17:17:29 +0800 Subject: [PATCH 06/10] pass static-check --- rootfs/etc/nginx/lua/balancer/sticky.lua | 1 - rootfs/etc/nginx/lua/test/balancer/sticky_test.lua | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/rootfs/etc/nginx/lua/balancer/sticky.lua b/rootfs/etc/nginx/lua/balancer/sticky.lua index 15f3f3971..c61cef446 100644 --- a/rootfs/etc/nginx/lua/balancer/sticky.lua +++ b/rootfs/etc/nginx/lua/balancer/sticky.lua @@ -47,7 +47,6 @@ local function set_cookie(self, value) httponly = true, } - local expires if self.cookie_expires and self.cookie_expires ~= "" then cookie_data.expires = ngx.cookie_time(tonumber(self.cookie_expires)) end diff --git a/rootfs/etc/nginx/lua/test/balancer/sticky_test.lua b/rootfs/etc/nginx/lua/test/balancer/sticky_test.lua index c38773885..9c022d9c1 100644 --- a/rootfs/etc/nginx/lua/test/balancer/sticky_test.lua +++ b/rootfs/etc/nginx/lua/test/balancer/sticky_test.lua @@ -157,4 +157,4 @@ describe("Sticky", function() end) end) end) -end) \ No newline at end of file +end) From f48e0162a323b702dd6b83b1d1641236e103ed04 Mon Sep 17 00:00:00 2001 From: liuwei Date: Fri, 2 Nov 2018 18:13:54 +0800 Subject: [PATCH 07/10] Use second as cookie expires unit --- Makefile | 2 +- test/e2e/annotations/affinity.go | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 37e38ed06..534251b38 100644 --- a/Makefile +++ b/Makefile @@ -24,7 +24,7 @@ GOHOSTOS ?= $(shell go env GOHOSTOS) # e2e settings # Allow limiting the scope of the e2e tests. By default run everything -FOCUS ?= .* +FOCUS ?= Annotations\\s-\\sAffinity/Sticky\\sSessions # number of parallel test E2E_NODES ?= 4 # slow test only if takes > 40s diff --git a/test/e2e/annotations/affinity.go b/test/e2e/annotations/affinity.go index 4b8c028db..b451f28ab 100644 --- a/test/e2e/annotations/affinity.go +++ b/test/e2e/annotations/affinity.go @@ -29,6 +29,7 @@ import ( "k8s.io/api/extensions/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/ingress-nginx/test/e2e/framework" ) From 25a839c9dffa606937b64b70282250f98bccd1e0 Mon Sep 17 00:00:00 2001 From: liuwei Date: Fri, 2 Nov 2018 19:02:32 +0800 Subject: [PATCH 08/10] Use second as cookie expires unit --- test/e2e/annotations/affinity.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/e2e/annotations/affinity.go b/test/e2e/annotations/affinity.go index 5225bc1cf..76f1f8a7f 100644 --- a/test/e2e/annotations/affinity.go +++ b/test/e2e/annotations/affinity.go @@ -193,11 +193,11 @@ var _ = framework.IngressNginxDescribe("Annotations - Affinity/Sticky Sessions", Expect(resp.Header.Get("Set-Cookie")).Should(ContainSubstring("Path=/somewhereelese;")) }) - It("should set sticky cookie with expires", func() { - host := "sticky.foo.com" + It("should set cookie with expires", func() { + host := "cookie.foo.com" annotations := map[string]string{ "nginx.ingress.kubernetes.io/affinity": "cookie", - "nginx.ingress.kubernetes.io/session-cookie-name": "SERVERID", + "nginx.ingress.kubernetes.io/session-cookie-name": "ExpiresCookie", "nginx.ingress.kubernetes.io/session-cookie-expires": "172800", "nginx.ingress.kubernetes.io/session-cookie-max-age": "259200", } From 41cc1121eac804a4b71026d7149b47889cc16356 Mon Sep 17 00:00:00 2001 From: liuwei Date: Fri, 2 Nov 2018 19:23:33 +0800 Subject: [PATCH 09/10] Use second as cookie expires unit --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 534251b38..37e38ed06 100644 --- a/Makefile +++ b/Makefile @@ -24,7 +24,7 @@ GOHOSTOS ?= $(shell go env GOHOSTOS) # e2e settings # Allow limiting the scope of the e2e tests. By default run everything -FOCUS ?= Annotations\\s-\\sAffinity/Sticky\\sSessions +FOCUS ?= .* # number of parallel test E2E_NODES ?= 4 # slow test only if takes > 40s From 5febef5e69a60cb30026de5ef2d07b3911c138cc Mon Sep 17 00:00:00 2001 From: liuwei Date: Fri, 2 Nov 2018 20:07:23 +0800 Subject: [PATCH 10/10] Use second as cookie expires unit --- test/e2e/annotations/affinity.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/e2e/annotations/affinity.go b/test/e2e/annotations/affinity.go index 76f1f8a7f..6e175b6c3 100644 --- a/test/e2e/annotations/affinity.go +++ b/test/e2e/annotations/affinity.go @@ -219,8 +219,8 @@ var _ = framework.IngressNginxDescribe("Annotations - Affinity/Sticky Sessions", Expect(resp.StatusCode).Should(Equal(http.StatusOK)) local, _ := time.LoadLocation("GMT") duration, _ := time.ParseDuration("48h") - want := time.Date(1970, time.January, 1, 0, 0, 0, 0, local).Add(duration).Format("Mon, 02-Jan-06 15:04:05 MST") - Expect(resp.Header.Get("Set-Cookie")).Should(ContainSubstring(fmt.Sprintf("Expires=%s", want))) + expected := time.Date(1970, time.January, 1, 0, 0, 0, 0, local).Add(duration).Format("Mon, 02-Jan-06 15:04:05 MST") + Expect(resp.Header.Get("Set-Cookie")).Should(ContainSubstring(fmt.Sprintf("Expires=%s", expected))) Expect(resp.Header.Get("Set-Cookie")).Should(ContainSubstring("Max-Age=259200")) }) })