Skip to content

Commit

Permalink
Remove session-cookie-hash annotation
Browse files Browse the repository at this point in the history
  • Loading branch information
Alex Kursell committed Mar 4, 2019
1 parent 79c52cf commit d3ac73b
Show file tree
Hide file tree
Showing 18 changed files with 22 additions and 189 deletions.
2 changes: 0 additions & 2 deletions docs/examples/affinity/cookie/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ Session stickiness is achieved through 3 annotations on the Ingress, as shown in
| --- | --- | --- |
|nginx.ingress.kubernetes.io/affinity|Sets the affinity type|string (in NGINX only ``cookie`` is possible|
|nginx.ingress.kubernetes.io/session-cookie-name|Name of the cookie that will be used|string (default to INGRESSCOOKIE)|
|nginx.ingress.kubernetes.io/session-cookie-hash|Type of hash that will be used in cookie value|sha1/md5/index|
|nginx.ingress.kubernetes.io/session-cookie-expires|The value is a date as UNIX timestamp that the cookie will expire on, it corresponds to cookie Expires directive|number of seconds|
|nginx.ingress.kubernetes.io/session-cookie-max-age|Number of seconds until the cookie expires that will correspond to cookie `Max-Age` directive|number of seconds|

Expand All @@ -37,7 +36,6 @@ Rules:
/ nginx-service:80 (<none>)
Annotations:
affinity: cookie
session-cookie-hash: sha1
session-cookie-name: INGRESSCOOKIE
session-cookie-expires: 172800
session-cookie-max-age: 172800
Expand Down
1 change: 0 additions & 1 deletion docs/examples/affinity/cookie/ingress.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ metadata:
annotations:
nginx.ingress.kubernetes.io/affinity: "cookie"
nginx.ingress.kubernetes.io/session-cookie-name: "route"
nginx.ingress.kubernetes.io/session-cookie-hash: "sha1"
nginx.ingress.kubernetes.io/session-cookie-expires: "172800"
nginx.ingress.kubernetes.io/session-cookie-max-age: "172800"

Expand Down
11 changes: 0 additions & 11 deletions docs/user-guide/nginx-configuration/annotations.md
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,6 @@ You can add these Kubernetes annotations to specific Ingress objects to customiz
|[nginx.ingress.kubernetes.io/server-snippet](#server-snippet)|string|
|[nginx.ingress.kubernetes.io/service-upstream](#service-upstream)|"true" or "false"|
|[nginx.ingress.kubernetes.io/session-cookie-name](#cookie-affinity)|string|
|[nginx.ingress.kubernetes.io/session-cookie-hash](#cookie-affinity)|string|
|[nginx.ingress.kubernetes.io/session-cookie-path](#cookie-affinity)|string|
|[nginx.ingress.kubernetes.io/ssl-redirect](#server-side-https-enforcement-through-redirect)|"true" or "false"|
|[nginx.ingress.kubernetes.io/ssl-passthrough](#ssl-passthrough)|"true" or "false"|
Expand Down Expand Up @@ -149,18 +148,8 @@ The only affinity type available for NGINX is `cookie`.

If you use the ``cookie`` affinity type you can also specify the name of the cookie that will be used to route the requests with the annotation `nginx.ingress.kubernetes.io/session-cookie-name`. The default is to create a cookie named 'INGRESSCOOKIE'.

In case of NGINX the annotation `nginx.ingress.kubernetes.io/session-cookie-hash` defines which algorithm will be used to hash the used upstream. Default value is `md5` and possible values are `md5`, `sha1` and `index`.

The NGINX annotation `nginx.ingress.kubernetes.io/session-cookie-path` defines the path that will be set on the cookie. This is optional unless the annotation `nginx.ingress.kubernetes.io/use-regex` is set to true; Session cookie paths do not support regex.

!!! attention
The `index` option is not an actual hash; an in-memory index is used instead, which has less overhead.
However, with `index`, matching against a changing upstream server list is inconsistent.
So, at reload, if upstream servers have changed, index values are not guaranteed to correspond to the same server as before!
**Use `index` with caution** and only if you need to!

In NGINX this feature is implemented by the third party module [nginx-sticky-module-ng](https://bitbucket.org/nginx-goodies/nginx-sticky-module-ng). The workflow used to define which upstream server will be used is explained [here](https://bitbucket.org/nginx-goodies/nginx-sticky-module-ng/raw/08a395c66e425540982c00482f55034e1fee67b6/docs/sticky.pdf)


### Authentication

Expand Down
18 changes: 6 additions & 12 deletions internal/ingress/annotations/annotations_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ var (
defaultCorsMethods = "GET, PUT, POST, DELETE, PATCH, OPTIONS"
defaultCorsHeaders = "DNT,X-CustomHeader,Keep-Alive,User-Agent,X-Requested-With,If-Modified-Since,Cache-Control,Content-Type,Authorization"
annotationAffinityCookieName = parser.GetAnnotationWithPrefix("session-cookie-name")
annotationAffinityCookieHash = parser.GetAnnotationWithPrefix("session-cookie-hash")
annotationUpstreamHashBy = parser.GetAnnotationWithPrefix("upstream-hash-by")
annotationCustomHTTPErrors = parser.GetAnnotationWithPrefix("custom-http-errors")
)
Expand Down Expand Up @@ -200,24 +199,19 @@ func TestAffinitySession(t *testing.T) {
fooAnns := []struct {
annotations map[string]string
affinitytype string
hash string
name string
}{
{map[string]string{annotationAffinityType: "cookie", annotationAffinityCookieHash: "md5", annotationAffinityCookieName: "route"}, "cookie", "md5", "route"},
{map[string]string{annotationAffinityType: "cookie", annotationAffinityCookieHash: "xpto", annotationAffinityCookieName: "route1"}, "cookie", "md5", "route1"},
{map[string]string{annotationAffinityType: "cookie", annotationAffinityCookieHash: "", annotationAffinityCookieName: ""}, "cookie", "md5", "INGRESSCOOKIE"},
{map[string]string{}, "", "", ""},
{nil, "", "", ""},
{map[string]string{annotationAffinityType: "cookie", annotationAffinityCookieName: "route"}, "cookie", "route"},
{map[string]string{annotationAffinityType: "cookie", annotationAffinityCookieName: "route1"}, "cookie", "route1"},
{map[string]string{annotationAffinityType: "cookie", annotationAffinityCookieName: ""}, "cookie", "INGRESSCOOKIE"},
{map[string]string{}, "", ""},
{nil, "", ""},
}

for _, foo := range fooAnns {
ing.SetAnnotations(foo.annotations)
r := ec.Extract(ing).SessionAffinity
t.Logf("Testing pass %v %v %v", foo.affinitytype, foo.hash, foo.name)

if r.Cookie.Hash != foo.hash {
t.Errorf("Returned %v but expected %v for Hash", r.Cookie.Hash, foo.hash)
}
t.Logf("Testing pass %v %v", foo.affinitytype, foo.name)

if r.Cookie.Name != foo.name {
t.Errorf("Returned %v but expected %v for Name", r.Cookie.Name, foo.name)
Expand Down
14 changes: 0 additions & 14 deletions internal/ingress/annotations/sessionaffinity/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,6 @@ const (

defaultAffinityCookieName = "INGRESSCOOKIE"

// This is the algorithm used by nginx to generate a value for the session cookie, if
// 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 number of seconds until the
// cookie expires
annotationAffinityCookieExpires = "session-cookie-expires"
Expand All @@ -52,7 +47,6 @@ const (
)

var (
affinityCookieHashRegex = regexp.MustCompile(`^(index|md5|sha1)$`)
affinityCookieExpiresRegex = regexp.MustCompile(`(^0|-?[1-9]\d*$)`)
)

Expand All @@ -67,8 +61,6 @@ type Config struct {
type Cookie struct {
// The name of the cookie that will be used in case of cookie affinity type.
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 duration to control cookie expires
Expires string `json:"expires"`
// The number of seconds until the cookie expires
Expand All @@ -90,12 +82,6 @@ func (a affinity) cookieAffinityParse(ing *extensions.Ingress) *Cookie {
cookie.Name = defaultAffinityCookieName
}

cookie.Hash, err = parser.GetStringAnnotation(annotationAffinityCookieHash, ing)
if err != nil || !affinityCookieHashRegex.MatchString(cookie.Hash) {
klog.V(3).Infof("Invalid or no annotation value found in Ingress %v: %v. Setting it to default %v", ing.Name, annotationAffinityCookieHash, defaultAffinityCookieHash)
cookie.Hash = defaultAffinityCookieHash
}

cookie.Expires, err = parser.GetStringAnnotation(annotationAffinityCookieExpires, ing)
if err != nil || !affinityCookieExpiresRegex.MatchString(cookie.Expires) {
klog.V(3).Infof("Invalid or no annotation value found in Ingress %v: %v. Ignoring it", ing.Name, annotationAffinityCookieExpires)
Expand Down
5 changes: 0 additions & 5 deletions internal/ingress/annotations/sessionaffinity/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ func TestIngressAffinityCookieConfig(t *testing.T) {

data := map[string]string{}
data[parser.GetAnnotationWithPrefix(annotationAffinityType)] = "cookie"
data[parser.GetAnnotationWithPrefix(annotationAffinityCookieHash)] = "sha123"
data[parser.GetAnnotationWithPrefix(annotationAffinityCookieName)] = "INGRESSCOOKIE"
data[parser.GetAnnotationWithPrefix(annotationAffinityCookieExpires)] = "4500"
data[parser.GetAnnotationWithPrefix(annotationAffinityCookieMaxAge)] = "3000"
Expand All @@ -84,10 +83,6 @@ func TestIngressAffinityCookieConfig(t *testing.T) {
t.Errorf("expected cookie as affinity but returned %v", nginxAffinity.Type)
}

if nginxAffinity.Cookie.Hash != "md5" {
t.Errorf("expected md5 as session-cookie-hash but returned %v", nginxAffinity.Cookie.Hash)
}

if nginxAffinity.Cookie.Name != "INGRESSCOOKIE" {
t.Errorf("expected route as session-cookie-name but returned %v", nginxAffinity.Cookie.Name)
}
Expand Down
1 change: 0 additions & 1 deletion internal/ingress/controller/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -548,7 +548,6 @@ func (n *NGINXController) getBackendServers(ingresses []*ingress.Ingress) ([]*in
}

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
ups.SessionAffinity.CookieSessionAffinity.Path = cookiePath
Expand Down
1 change: 0 additions & 1 deletion internal/ingress/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,6 @@ type SessionAffinityConfig struct {
// +k8s:deepcopy-gen=true
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"`
Expand Down
3 changes: 0 additions & 3 deletions internal/ingress/types_equals.go
Original file line number Diff line number Diff line change
Expand Up @@ -224,9 +224,6 @@ func (csa1 *CookieSessionAffinity) Equal(csa2 *CookieSessionAffinity) bool {
if csa1.Name != csa2.Name {
return false
}
if csa1.Hash != csa2.Hash {
return false
}
if csa1.Path != csa2.Path {
return false
}
Expand Down
24 changes: 2 additions & 22 deletions rootfs/etc/nginx/lua/balancer/sticky.lua
Original file line number Diff line number Diff line change
Expand Up @@ -2,29 +2,20 @@ local balancer_resty = require("balancer.resty")
local resty_chash = require("resty.chash")
local util = require("util")
local ck = require("resty.cookie")
local math = require("math")

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

local function get_digest_func(hash)
local digest_func = util.md5_digest
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),
digest_func = digest_func,
traffic_shaping_policy = backend.trafficShapingPolicy,
alternative_backends = backend.alternativeBackends,
cookie_session_affinity = backend["sessionAffinityConfig"]["cookieSessionAffinity"]
Expand All @@ -34,15 +25,6 @@ function _M.new(self, backend)
return o
end

local function encrypted_endpoint_string(self, endpoint_string)
local encrypted, err = self.digest_func(endpoint_string)
if err ~= nil then
ngx.log(ngx.ERR, err)
end

return encrypted
end

local function set_cookie(self, value)
local cookie, err = ck:new()
if not cookie then
Expand Down Expand Up @@ -86,8 +68,7 @@ function _M.balance(self)

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)
key = string.format("%s.%s.%s", ngx.now(), ngx.worker.pid(), math.random(999999))

if self.cookie_session_affinity.locations then
local locs = self.cookie_session_affinity.locations[ngx.var.host]
Expand Down Expand Up @@ -118,7 +99,6 @@ function _M.sync(self, backend)
end

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

return _M
29 changes: 0 additions & 29 deletions rootfs/etc/nginx/lua/test/balancer/sticky_test.lua
Original file line number Diff line number Diff line change
Expand Up @@ -68,26 +68,6 @@ describe("Sticky", function()
assert.equal(sticky_balancer_instance:cookie_name(), default_cookie_name)
end)
end)

context("when backend specifies hash function", function()
it("returns an instance with the corresponding hash implementation", function()
local sticky_balancer_instance = sticky:new(test_backend)
local test_backend_hash_fn = test_backend.sessionAffinityConfig.cookieSessionAffinity.hash
local test_backend_hash_implementation = util[test_backend_hash_fn .. "_digest"]
assert.equal(sticky_balancer_instance.digest_func, test_backend_hash_implementation)
end)
end)

context("when backend does not specify hash function", function()
it("returns an instance with the default implementation (md5)", function()
local temp_backend = util.deepcopy(test_backend)
temp_backend.sessionAffinityConfig.cookieSessionAffinity.hash = nil
local sticky_balancer_instance = sticky:new(temp_backend)
local default_hash_fn = "md5"
local default_hash_implementation = util[default_hash_fn .. "_digest"]
assert.equal(sticky_balancer_instance.digest_func, default_hash_implementation)
end)
end)
end)

describe("balance()", function()
Expand All @@ -112,12 +92,9 @@ describe("Sticky", function()
it("sets a cookie on the client", function()
local s = {}
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)
local expected_len = #util[test_backend_hash_fn .. "_digest"]("anything")
assert.equal(#payload.value, expected_len)
assert.equal(payload.path, ngx.var.location_path)
assert.equal(payload.domain, nil)
assert.equal(payload.httponly, true)
Expand All @@ -141,12 +118,9 @@ describe("Sticky", function()
ngx.var.https = "on"
local s = {}
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)
local expected_len = #util[test_backend_hash_fn .. "_digest"]("anything")
assert.equal(#payload.value, expected_len)
assert.equal(payload.path, ngx.var.location_path)
assert.equal(payload.domain, nil)
assert.equal(payload.httponly, true)
Expand Down Expand Up @@ -177,12 +151,9 @@ describe("Sticky", function()
it("does not set a cookie on the client", function()
local s = {}
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)
local expected_len = #util[test_backend_hash_fn .. "_digest"]("anything")
assert.equal(#payload.value, expected_len)
assert.equal(payload.path, ngx.var.location_path)
assert.equal(payload.domain, ngx.var.host)
assert.equal(payload.httponly, true)
Expand Down
6 changes: 3 additions & 3 deletions rootfs/etc/nginx/lua/test/balancer_test.lua
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ local function reset_backends()
{ address = "10.184.97.100", port = "8080", maxFails = 0, failTimeout = 0 },
{ address = "10.184.98.239", port = "8080", maxFails = 0, failTimeout = 0 },
},
sessionAffinityConfig = { name = "", cookieSessionAffinity = { name = "", hash = "" } },
sessionAffinityConfig = { name = "", cookieSessionAffinity = { name = "" } },
},
{ name = "my-dummy-app-1", ["load-balance"] = "round_robin", },
{
Expand All @@ -38,12 +38,12 @@ local function reset_backends()
},
{
name = "my-dummy-app-3", ["load-balance"] = "ewma",
sessionAffinityConfig = { name = "cookie", cookieSessionAffinity = { name = "route", hash = "sha1" } }
sessionAffinityConfig = { name = "cookie", cookieSessionAffinity = { name = "route" } }
},
{ name = "my-dummy-app-4", ["load-balance"] = "ewma", },
{
name = "my-dummy-app-5", ["load-balance"] = "ewma", ["upstream-hash-by"] = "$request_uri",
sessionAffinityConfig = { name = "cookie", cookieSessionAffinity = { name = "route", hash = "sha1" } }
sessionAffinityConfig = { name = "cookie", cookieSessionAffinity = { name = "route" } }
},
}
end
Expand Down
2 changes: 1 addition & 1 deletion rootfs/etc/nginx/lua/test/configuration_test.lua
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ function get_backends()
{
name = "my-dummy-backend-1", ["load-balance"] = "sticky",
endpoints = { { address = "10.183.7.40", port = "8080", maxFails = 0, failTimeout = 0 } },
sessionAffinityConfig = { name = "cookie", cookieSessionAffinity = { name = "route", hash = "sha1" } },
sessionAffinityConfig = { name = "cookie", cookieSessionAffinity = { name = "route" } },
},
{
name = "my-dummy-backend-2", ["load-balance"] = "ewma",
Expand Down
27 changes: 0 additions & 27 deletions rootfs/etc/nginx/lua/util.lua
Original file line number Diff line number Diff line change
@@ -1,8 +1,5 @@
local string_len = string.len
local string_sub = string.sub
local resty_str = require("resty.string")
local resty_sha1 = require("resty.sha1")
local resty_md5 = require("resty.md5")

local _M = {}

Expand All @@ -18,30 +15,6 @@ function _M.get_nodes(endpoints)
return nodes
end

local function hash_digest(hash_factory, message)
local hash = hash_factory:new()
if not hash then
return nil, "failed to create object"
end
local ok = hash:update(message)
if not ok then
return nil, "failed to add data"
end
local binary_digest = hash:final()
if binary_digest == nil then
return nil, "failed to create digest"
end
return resty_str.to_hex(binary_digest), nil
end

function _M.sha1_digest(message)
return hash_digest(resty_sha1, message)
end

function _M.md5_digest(message)
return hash_digest(resty_md5, message)
end

-- given an Nginx variable i.e $request_uri
-- it returns value of ngx.var[request_uri]
function _M.lua_ngx_var(ngx_var)
Expand Down
Loading

0 comments on commit d3ac73b

Please sign in to comment.