Skip to content

Commit

Permalink
Merge pull request #3740 from Shopify/session-annotation-reload
Browse files Browse the repository at this point in the history 
Fix ingress updating for session-cookie-* annotation changes
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Feb 19, 2019
2 parents 43cfbb9 + c180a09 commit 15d5ef9
Show file tree
Hide file tree
Showing 4 changed files with 102 additions and 26 deletions.
6 changes: 6 additions & 0 deletions internal/ingress/types_equals.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,12 @@ func (csa1 *CookieSessionAffinity) Equal(csa2 *CookieSessionAffinity) bool {
if csa1.Path != csa2.Path {
return false
}
if csa1.Expires != csa2.Expires {
return false
}
if csa1.MaxAge != csa2.MaxAge {
return false
}

return true
}
Expand Down
56 changes: 39 additions & 17 deletions rootfs/etc/nginx/lua/balancer/sticky.lua
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,24 +4,30 @@ local util = require("util")
local ck = require("resty.cookie")

local _M = balancer_resty:new({ factory = resty_chash, name = "sticky" })
local DEFAULT_COOKIE_NAME = "route"

function _M.new(self, backend)
local nodes = util.get_nodes(backend.endpoints)
local function get_digest_func(hash)
local digest_func = util.md5_digest
if backend["sessionAffinityConfig"]["cookieSessionAffinity"]["hash"] == "sha1" then
if hash == "sha1" then
digest_func = util.sha1_digest
end
return digest_func
end

function _M.cookie_name(self)
return self.cookie_session_affinity.name or DEFAULT_COOKIE_NAME
end

function _M.new(self, backend)
local nodes = util.get_nodes(backend.endpoints)
local digest_func = get_digest_func(backend["sessionAffinityConfig"]["cookieSessionAffinity"]["hash"])

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"],
cookie_path = backend["sessionAffinityConfig"]["cookieSessionAffinity"]["path"],
cookie_locations = backend["sessionAffinityConfig"]["cookieSessionAffinity"]["locations"],
digest_func = digest_func,
traffic_shaping_policy = backend.trafficShapingPolicy,
alternative_backends = backend.alternativeBackends,
cookie_session_affinity = backend["sessionAffinityConfig"]["cookieSessionAffinity"]
}
setmetatable(o, self)
self.__index = self
Expand All @@ -43,25 +49,25 @@ local function set_cookie(self, value)
ngx.log(ngx.ERR, err)
end

local cookie_path = self.cookie_path
local cookie_path = self.cookie_session_affinity.path
if not cookie_path then
cookie_path = ngx.var.location_path
end

local cookie_data = {
key = self.cookie_name,
key = self:cookie_name(),
value = value,
path = cookie_path,
httponly = true,
secure = ngx.var.https == "on",
}

if self.cookie_expires and self.cookie_expires ~= "" then
cookie_data.expires = ngx.cookie_time(ngx.time() + tonumber(self.cookie_expires))
if self.cookie_session_affinity.expires and self.cookie_session_affinity.expires ~= "" then
cookie_data.expires = ngx.cookie_time(ngx.time() + tonumber(self.cookie_session_affinity.expires))
end

if self.cookie_max_age and self.cookie_max_age ~= "" then
cookie_data.max_age = tonumber(self.cookie_max_age)
if self.cookie_session_affinity.maxage and self.cookie_session_affinity.maxage ~= "" then
cookie_data.max_age = tonumber(self.cookie_session_affinity.maxage)
end

local ok
Expand All @@ -78,13 +84,13 @@ function _M.balance(self)
return
end

local key = cookie:get(self.cookie_name)
local key = cookie:get(self:cookie_name())
if not key then
local random_str = string.format("%s.%s", ngx.now(), ngx.worker.pid())
key = encrypted_endpoint_string(self, random_str)

if self.cookie_locations then
local locs = self.cookie_locations[ngx.var.host]
if self.cookie_session_affinity.locations then
local locs = self.cookie_session_affinity.locations[ngx.var.host]
if locs ~= nil then
for _, path in pairs(locs) do
if ngx.var.location_path == path then
Expand All @@ -99,4 +105,20 @@ function _M.balance(self)
return self.instance:find(key)
end

function _M.sync(self, backend)
balancer_resty.sync(self, backend)

-- Reload the balancer if any of the annotations have changed.
local changed = not util.deep_compare(
self.cookie_session_affinity,
backend.sessionAffinityConfig.cookieSessionAffinity
)
if not changed then
return
end

self.cookie_session_affinity = backend.sessionAffinityConfig.cookieSessionAffinity
self.digest_func = get_digest_func(backend.sessionAffinityConfig.cookieSessionAffinity.hash)
end

return _M
4 changes: 2 additions & 2 deletions rootfs/etc/nginx/lua/test/balancer/sticky_test.lua
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ describe("Sticky", function()
it("returns an instance containing the corresponding cookie name", function()
local sticky_balancer_instance = sticky:new(test_backend)
local test_backend_cookie_name = test_backend.sessionAffinityConfig.cookieSessionAffinity.name
assert.equal(sticky_balancer_instance.cookie_name, test_backend_cookie_name)
assert.equal(sticky_balancer_instance:cookie_name(), test_backend_cookie_name)
end)
end)

Expand All @@ -65,7 +65,7 @@ describe("Sticky", function()
temp_backend.sessionAffinityConfig.cookieSessionAffinity.name = nil
local sticky_balancer_instance = sticky:new(temp_backend)
local default_cookie_name = "route"
assert.equal(sticky_balancer_instance.cookie_name, default_cookie_name)
assert.equal(sticky_balancer_instance:cookie_name(), default_cookie_name)
end)
end)

Expand Down
62 changes: 55 additions & 7 deletions test/e2e/annotations/affinity.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ var _ = framework.IngressNginxDescribe("Annotations - Affinity/Sticky Sessions",
func(server string) bool {
return strings.Contains(server, fmt.Sprintf("server_name %s ;", host))
})
time.Sleep(waitForLuaSync)

resp, _, errs := gorequest.New().
Get(f.IngressController.HTTPURL).
Expand All @@ -73,8 +74,48 @@ var _ = framework.IngressNginxDescribe("Annotations - Affinity/Sticky Sessions",
Expect(resp.Header.Get("Set-Cookie")).Should(ContainSubstring(match[0]))
})

It("should change cookie name on ingress definition change", func() {
host := "change.foo.com"
annotations := map[string]string{
"nginx.ingress.kubernetes.io/affinity": "cookie",
"nginx.ingress.kubernetes.io/session-cookie-name": "SERVERID",
}

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))
})
time.Sleep(waitForLuaSync)

resp, _, errs := gorequest.New().
Get(f.IngressController.HTTPURL).
Set("Host", host).
End()

Expect(errs).Should(BeEmpty())
Expect(resp.StatusCode).Should(Equal(http.StatusOK))
Expect(resp.Header.Get("Set-Cookie")).Should(ContainSubstring("SERVERID"))

ing.ObjectMeta.Annotations["nginx.ingress.kubernetes.io/session-cookie-name"] = "OTHERCOOKIENAME"
f.EnsureIngress(ing)

time.Sleep(waitForLuaSync)

resp, _, errs = gorequest.New().
Get(f.IngressController.HTTPURL).
Set("Host", host).
End()

Expect(errs).Should(BeEmpty())
Expect(resp.StatusCode).Should(Equal(http.StatusOK))
Expect(resp.Header.Get("Set-Cookie")).Should(ContainSubstring("OTHERCOOKIENAME"))
})

It("should set sticky cookie with sha1 hash", func() {
host := "sticky.foo.com"
host := "sha1.foo.com"
annotations := map[string]string{
"nginx.ingress.kubernetes.io/affinity": "cookie",
"nginx.ingress.kubernetes.io/session-cookie-hash": "sha1",
Expand All @@ -87,6 +128,7 @@ var _ = framework.IngressNginxDescribe("Annotations - Affinity/Sticky Sessions",
func(server string) bool {
return strings.Contains(server, fmt.Sprintf("server_name %s ;", host))
})
time.Sleep(waitForLuaSync)

resp, _, errs := gorequest.New().
Get(f.IngressController.HTTPURL).
Expand All @@ -103,7 +145,7 @@ var _ = framework.IngressNginxDescribe("Annotations - Affinity/Sticky Sessions",
})

It("should set the path to /something on the generated cookie", func() {
host := "example.com"
host := "path.foo.com"
annotations := map[string]string{
"nginx.ingress.kubernetes.io/affinity": "cookie",
"nginx.ingress.kubernetes.io/session-cookie-name": "SERVERID",
Expand All @@ -116,6 +158,7 @@ var _ = framework.IngressNginxDescribe("Annotations - Affinity/Sticky Sessions",
func(server string) bool {
return strings.Contains(server, fmt.Sprintf("server_name %s ;", host))
})
time.Sleep(waitForLuaSync)

resp, _, errs := gorequest.New().
Get(f.IngressController.HTTPURL+"/something").
Expand All @@ -128,7 +171,7 @@ var _ = framework.IngressNginxDescribe("Annotations - Affinity/Sticky Sessions",
})

It("does not set the path to / on the generated cookie if there's more than one rule referring to the same backend", func() {
host := "example.com"
host := "morethanonerule.foo.com"
annotations := map[string]string{
"nginx.ingress.kubernetes.io/affinity": "cookie",
"nginx.ingress.kubernetes.io/session-cookie-name": "SERVERID",
Expand Down Expand Up @@ -173,6 +216,7 @@ var _ = framework.IngressNginxDescribe("Annotations - Affinity/Sticky Sessions",
func(server string) bool {
return strings.Contains(server, fmt.Sprintf("server_name %s ;", host))
})
time.Sleep(waitForLuaSync)

resp, _, errs := gorequest.New().
Get(f.IngressController.HTTPURL+"/something").
Expand All @@ -194,7 +238,7 @@ var _ = framework.IngressNginxDescribe("Annotations - Affinity/Sticky Sessions",
})

It("should set cookie with expires", func() {
host := "cookie.foo.com"
host := "cookieexpires.foo.com"
annotations := map[string]string{
"nginx.ingress.kubernetes.io/affinity": "cookie",
"nginx.ingress.kubernetes.io/session-cookie-name": "ExpiresCookie",
Expand All @@ -209,6 +253,7 @@ var _ = framework.IngressNginxDescribe("Annotations - Affinity/Sticky Sessions",
func(server string) bool {
return strings.Contains(server, fmt.Sprintf("server_name %s ;", host))
})
time.Sleep(waitForLuaSync)

resp, _, errs := gorequest.New().
Get(f.IngressController.HTTPURL).
Expand All @@ -225,7 +270,7 @@ var _ = framework.IngressNginxDescribe("Annotations - Affinity/Sticky Sessions",
})

It("should work with use-regex annotation and session-cookie-path", func() {
host := "cookie.foo.com"
host := "useregex.foo.com"
annotations := map[string]string{
"nginx.ingress.kubernetes.io/affinity": "cookie",
"nginx.ingress.kubernetes.io/session-cookie-name": "SERVERID",
Expand All @@ -240,6 +285,7 @@ var _ = framework.IngressNginxDescribe("Annotations - Affinity/Sticky Sessions",
func(server string) bool {
return strings.Contains(server, fmt.Sprintf("server_name %s ;", host))
})
time.Sleep(waitForLuaSync)

resp, _, errs := gorequest.New().
Get(f.IngressController.HTTPURL+"/foo/bar").
Expand All @@ -257,7 +303,7 @@ var _ = framework.IngressNginxDescribe("Annotations - Affinity/Sticky Sessions",
})

It("should warn user when use-regex is true and session-cookie-path is not set", func() {
host := "cookie.foo.com"
host := "useregexwarn.foo.com"
annotations := map[string]string{
"nginx.ingress.kubernetes.io/affinity": "cookie",
"nginx.ingress.kubernetes.io/session-cookie-name": "SERVERID",
Expand All @@ -271,6 +317,7 @@ var _ = framework.IngressNginxDescribe("Annotations - Affinity/Sticky Sessions",
func(server string) bool {
return strings.Contains(server, fmt.Sprintf("server_name %s ;", host))
})
time.Sleep(waitForLuaSync)

resp, _, errs := gorequest.New().
Get(f.IngressController.HTTPURL+"/foo/bar").
Expand All @@ -286,7 +333,7 @@ var _ = framework.IngressNginxDescribe("Annotations - Affinity/Sticky Sessions",
})

It("should not set affinity across all server locations when using separate ingresses", func() {
host := "cookie.foo.com"
host := "separate.foo.com"

annotations := map[string]string{
"nginx.ingress.kubernetes.io/affinity": "cookie",
Expand All @@ -301,6 +348,7 @@ var _ = framework.IngressNginxDescribe("Annotations - Affinity/Sticky Sessions",
func(server string) bool {
return strings.Contains(server, `location /foo/bar`) && strings.Contains(server, `location /foo`)
})
time.Sleep(waitForLuaSync)

resp, _, errs := gorequest.New().
Get(f.IngressController.HTTPURL+"/foo").
Expand Down

0 comments on commit 15d5ef9

Please sign in to comment.