diff --git a/kong-0.13.1-0.rockspec b/kong-0.13.1-0.rockspec index a37152887ea7..0b5b9ebabc36 100644 --- a/kong-0.13.1-0.rockspec +++ b/kong-0.13.1-0.rockspec @@ -31,6 +31,7 @@ dependencies = { "lua-resty-worker-events == 0.3.3", "lua-resty-mediador == 0.1.2", "lua-resty-healthcheck == 0.4.0", + "lua-resty-cookie == 0.1.0", "lua-resty-mlcache == 2.0.2", -- external Kong plugins "kong-plugin-azure-functions == 0.1.0", diff --git a/kong/dao/migrations/cassandra.lua b/kong/dao/migrations/cassandra.lua index f282f373ae8f..c4271301af4e 100644 --- a/kong/dao/migrations/cassandra.lua +++ b/kong/dao/migrations/cassandra.lua @@ -748,5 +748,16 @@ return { CREATE INDEX IF NOT EXISTS ON consumers(custom_id); CREATE INDEX IF NOT EXISTS ON consumers(username); ]] + }, + { + name = "2018-05-17-173100_hash_on_cookie", + up = [[ + ALTER TABLE upstreams ADD hash_on_cookie text; + ALTER TABLE upstreams ADD hash_on_cookie_path text; + ]], + down = [[ + ALTER TABLE upstreams DROP hash_on_cookie; + ALTER TABLE upstreams DROP hash_on_cookie_path; + ]] } } diff --git a/kong/dao/migrations/postgres.lua b/kong/dao/migrations/postgres.lua index 97c34a80245a..1f5755452f4a 100644 --- a/kong/dao/migrations/postgres.lua +++ b/kong/dao/migrations/postgres.lua @@ -785,4 +785,15 @@ return { ]], down = nil }, + { + name = "2018-05-17-173100_hash_on_cookie", + up = [[ + ALTER TABLE upstreams ADD hash_on_cookie text; + ALTER TABLE upstreams ADD hash_on_cookie_path text; + ]], + down = [[ + ALTER TABLE upstreams DROP hash_on_cookie; + ALTER TABLE upstreams DROP hash_on_cookie_path; + ]] + } } diff --git a/kong/dao/schemas/upstreams.lua b/kong/dao/schemas/upstreams.lua index b71d4e73358f..224f2227c7d7 100644 --- a/kong/dao/schemas/upstreams.lua +++ b/kong/dao/schemas/upstreams.lua @@ -168,6 +168,7 @@ return { "consumer", "ip", "header", + "cookie", }, }, hash_fallback = { @@ -179,6 +180,7 @@ return { "consumer", "ip", "header", + "cookie", }, }, hash_on_header = { @@ -189,6 +191,15 @@ return { -- header name, if `hash_fallback == "header"` type = "string", }, + hash_on_cookie = { + -- cookie name, if `hash_on` or `hash_fallback` == `"cookie"` + type = "string", + }, + hash_on_cookie_path = { + -- cookie path, if `hash_on` or `hash_fallback` == `"cookie"` + type = "string", + default = "/", + }, slots = { -- the number of slots in the loadbalancer algorithm type = "number", @@ -230,6 +241,20 @@ return { end end + if config.hash_on_cookie then + local ok, err = utils.validate_cookie_name(config.hash_on_cookie) + if not ok then + return false, Errors.schema("Cookie name: " .. err) + end + end + + if config.hash_on_cookie_path then + local ok, err = check_http_path(config.hash_on_cookie_path) + if not ok then + return false, Errors.schema("Cookie path: " .. err) + end + end + if (config.hash_on == "header" and not config.hash_on_header) or (config.hash_fallback == "header" @@ -238,6 +263,12 @@ return { "but no header name provided") end + if (config.hash_on == "cookie" or config.hash_fallback == "cookie") + and not config.hash_on_cookie then + return false, Errors.schema("Hashing on 'cookie', " .. + "but no cookie name provided") + end + if config.hash_on and config.hash_fallback then if config.hash_on == "none" then if config.hash_fallback ~= "none" then @@ -245,6 +276,14 @@ return { "'hash_on' is not set") end + elseif config.hash_on == "cookie" then + if config.hash_fallback ~= "none" then + -- Cookie value will be set with the server response if not present, + -- so the fallback value would never be evaluated. + return false, Errors.schema("Cannot set `hash_fallback` if primary " .. + "`hash_on` is set to cookie") + end + else if config.hash_on == config.hash_fallback then if config.hash_on ~= "header" then diff --git a/kong/runloop/balancer.lua b/kong/runloop/balancer.lua index 83ccd7b9dba7..d134f8e8d529 100644 --- a/kong/runloop/balancer.lua +++ b/kong/runloop/balancer.lua @@ -1,5 +1,6 @@ local pl_tablex = require "pl.tablex" local singletons = require "kong.singletons" +local utils = require "kong.tools.utils" -- due to startup/require order, cannot use the ones from 'singletons' here local dns_client = require "resty.dns.client" @@ -656,6 +657,25 @@ local create_hash = function(upstream) if type(identifier) == "table" then identifier = table_concat(identifier) end + + elseif hash_on == "cookie" then + identifier = ngx.var["cookie_" .. upstream.hash_on_cookie] + + -- If the cookie doesn't exist, create one and store in `ctx` + -- to be added to the "Set-Cookie" header in the response + if not identifier then + identifier = utils.uuid() + + -- TODO: This should be added the `ngx.ctx.balancer_address` + -- structure, where other balancer-related values are stored, + -- when that is renamed/ refactored. + ctx.balancer_hash_cookie = { + key = upstream.hash_on_cookie, + value = identifier, + path = upstream.hash_on_cookie_path + } + end + end if identifier then diff --git a/kong/runloop/handler.lua b/kong/runloop/handler.lua index 9b48649bfcfe..59a22fe9f10a 100644 --- a/kong/runloop/handler.lua +++ b/kong/runloop/handler.lua @@ -7,6 +7,7 @@ -- -- In the `access_by_lua` phase, it is responsible for retrieving the route being proxied by -- a consumer. Then it is responsible for loading the plugins to execute on this request. +local ck = require "resty.cookie" local utils = require "kong.tools.utils" local Router = require "kong.router" local ApiRouter = require "kong.api_router" @@ -707,6 +708,18 @@ return { header[upstream_status_header] = matches[0] end end + + local cookie_hash_data = ctx.balancer_hash_cookie + if cookie_hash_data then + local cookie = ck:new() + local ok, err = cookie:set(cookie_hash_data) + + if not ok then + log(ngx.WARN, "failed to set the cookie for hash-based load balancing: ", err, + " (key=", cookie_hash_data.key, + ", path=", cookie_hash_data.path, ")") + end + end end end, after = function(ctx) diff --git a/kong/tools/utils.lua b/kong/tools/utils.lua index 58b8df8b303c..f52097356db1 100644 --- a/kong/tools/utils.lua +++ b/kong/tools/utils.lua @@ -810,4 +810,22 @@ _M.validate_header_name = function(name) "', allowed characters are A-Z, a-z, 0-9, '_', and '-'" end +--- Validates a cookie name. +-- Checks characters used in a cookie name to be valid +-- a-z, A-Z, 0-9, '_' and '-' are allowed. +-- @param name (string) the cookie name to verify +-- @return the valid cookie name, or `nil+error` +_M.validate_cookie_name = function(name) + if name == nil or name == "" then + return nil, "no cookie name provided" + end + + if re_match(name, "^[a-zA-Z0-9-_]+$", "jo") then + return name + end + + return nil, "bad cookie name '" .. name .. + "', allowed characters are A-Z, a-z, 0-9, '_', and '-'" +end + return _M diff --git a/spec/01-unit/004-utils_spec.lua b/spec/01-unit/004-utils_spec.lua index 1190584381e3..15097a010728 100644 --- a/spec/01-unit/004-utils_spec.lua +++ b/spec/01-unit/004-utils_spec.lua @@ -525,6 +525,21 @@ describe("Utils", function() end end end) + it("validate_cookie_name() validates cookie names", function() + local header_chars = [[_-0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz]] + + for i = 1, 255 do + local c = string.char(i) + + if string.find(header_chars, c, nil, true) then + assert(utils.validate_cookie_name(c) == c, + "ascii character '" .. c .. "' (" .. i .. ") should have been allowed") + else + assert(utils.validate_cookie_name(c) == nil, + "ascii character " .. i .. " should not have been allowed") + end + end + end) it("pack() stores results, including nils, properly", function() assert.same({ n = 0 }, utils.pack()) assert.same({ n = 1 }, utils.pack(nil)) diff --git a/spec/01-unit/011-balancer_spec.lua b/spec/01-unit/011-balancer_spec.lua index 6df27fb49dc5..f024ba0982af 100644 --- a/spec/01-unit/011-balancer_spec.lua +++ b/spec/01-unit/011-balancer_spec.lua @@ -506,6 +506,27 @@ describe("Balancer", function() }) assert.are.same(crc32(value), hash) end) + describe("cookie", function() + it("uses the cookie when present in the request", function() + local value = "some cookie value" + ngx.var.cookie_Foo = value + local hash = balancer._create_hash({ + hash_on = "cookie", + hash_on_cookie = "Foo", + }) + assert.are.same(crc32(value), hash) + assert.is_nil(ngx.ctx.balancer_hash_cookie) + end) + it("creates the cookie when not present in the request", function() + balancer._create_hash({ + hash_on = "cookie", + hash_on_cookie = "Foo", + hash_on_cookie_path = "/", + }) + assert.are.same(ngx.ctx.balancer_hash_cookie.key, "Foo") + assert.are.same(ngx.ctx.balancer_hash_cookie.path, "/") + end) + end) it("multi-header", function() local value = { "some header value", "another value" } headers.HeaderName = value @@ -562,6 +583,29 @@ describe("Balancer", function() }) assert.are.same(crc32(table.concat(value)), hash) end) + describe("cookie", function() + it("uses the cookie when present in the request", function() + local value = "some cookie value" + ngx.var.cookie_Foo = value + local hash = balancer._create_hash({ + hash_on = "consumer", + hash_fallback = "cookie", + hash_on_cookie = "Foo", + }) + assert.are.same(crc32(value), hash) + assert.is_nil(ngx.ctx.balancer_hash_cookie) + end) + it("creates the cookie when not present in the request", function() + balancer._create_hash({ + hash_on = "consumer", + hash_fallback = "cookie", + hash_on_cookie = "Foo", + hash_on_cookie_path = "/", + }) + assert.are.same(ngx.ctx.balancer_hash_cookie.key, "Foo") + assert.are.same(ngx.ctx.balancer_hash_cookie.path, "/") + end) + end) end) end) diff --git a/spec/02-integration/04-admin_api/07-upstreams_routes_spec.lua b/spec/02-integration/04-admin_api/07-upstreams_routes_spec.lua index 0d3e87c827ae..91f8487cc0f3 100644 --- a/spec/02-integration/04-admin_api/07-upstreams_routes_spec.lua +++ b/spec/02-integration/04-admin_api/07-upstreams_routes_spec.lua @@ -87,6 +87,29 @@ describe("Admin API: #" .. kong_config.database, function() assert.are.equal("HeaderFallback", json.hash_fallback_header) end end) + it_content_types("creates an upstream with hash_on cookie parameters", function(content_type) + return function() + local res = assert(client:send { + method = "POST", + path = "/upstreams", + body = { + name = "my.upstream", + hash_on = "cookie", + hash_on_cookie = "CookieName1", + hash_on_cookie_path = "/foo", + }, + headers = {["Content-Type"] = content_type} + }) + assert.response(res).has.status(201) + local json = assert.response(res).has.jsonbody() + assert.equal("my.upstream", json.name) + assert.is_number(json.created_at) + assert.is_string(json.id) + assert.are.equal("cookie", json.hash_on) + assert.are.equal("CookieName1", json.hash_on_cookie) + assert.are.equal("/foo", json.hash_on_cookie_path) + end + end) it_content_types("creates an upstream with 2 header hashes", function(content_type) return function() local res = assert(client:send { @@ -198,7 +221,7 @@ describe("Admin API: #" .. kong_config.database, function() }) body = assert.res_status(400, res) local json = cjson.decode(body) - assert.same({ hash_on = '"something that is invalid" is not allowed. Allowed values are: "none", "consumer", "ip", "header"' }, json) + assert.same({ hash_on = '"something that is invalid" is not allowed. Allowed values are: "none", "consumer", "ip", "header", "cookie"' }, json) -- Invalid hash_fallback entries res = assert(client:send { @@ -213,7 +236,7 @@ describe("Admin API: #" .. kong_config.database, function() }) body = assert.res_status(400, res) local json = cjson.decode(body) - assert.same({ hash_fallback = '"something that is invalid" is not allowed. Allowed values are: "none", "consumer", "ip", "header"' }, json) + assert.same({ hash_fallback = '"something that is invalid" is not allowed. Allowed values are: "none", "consumer", "ip", "header", "cookie"' }, json) -- same hash entries res = assert(client:send { @@ -246,7 +269,7 @@ describe("Admin API: #" .. kong_config.database, function() local json = cjson.decode(body) assert.same({ message = "Header: bad header name 'not a <> valid <> header name', allowed characters are A-Z, a-z, 0-9, '_', and '-'" }, json) - -- Invalid header + -- Invalid fallback header res = assert(client:send { method = "POST", path = "/upstreams", @@ -279,6 +302,23 @@ describe("Admin API: #" .. kong_config.database, function() local json = cjson.decode(body) assert.same({ message = "Cannot set fallback and primary hashes to the same value" }, json) + -- Cookie with hash_fallback + res = assert(client:send { + method = "POST", + path = "/upstreams", + body = { + name = "my.upstream", + hash_on = "cookie", + hash_on_cookie = "cookiename", + hash_fallback = "header", + hash_fallback_header = "Cool-Header", + }, + headers = {["Content-Type"] = content_type} + }) + body = assert.res_status(400, res) + local json = cjson.decode(body) + assert.same({ message = "Cannot set `hash_fallback` if primary `hash_on` is set to cookie" }, json) + -- No headername provided res = assert(client:send { method = "POST", @@ -311,6 +351,70 @@ describe("Admin API: #" .. kong_config.database, function() local json = cjson.decode(body) assert.same({ message = "Hashing on 'header', but no header name provided" }, json) + -- Invalid cookie + res = assert(client:send { + method = "POST", + path = "/upstreams", + body = { + name = "my.upstream", + hash_on = "cookie", + hash_on_cookie = "not a <> valid <> cookie name", + }, + headers = {["Content-Type"] = content_type} + }) + body = assert.res_status(400, res) + local json = cjson.decode(body) + assert.same({ message = "Cookie name: bad cookie name 'not a <> valid <> cookie name', allowed characters are A-Z, a-z, 0-9, '_', and '-'" }, json) + + -- Invalid cookie path + res = assert(client:send { + method = "POST", + path = "/upstreams", + body = { + name = "my.upstream", + hash_on = "cookie", + hash_on_cookie = "hashme", + hash_on_cookie_path = "not a path", + }, + headers = {["Content-Type"] = content_type} + }) + body = assert.res_status(400, res) + local json = cjson.decode(body) + assert.same({ message = "Cookie path: must be prefixed with slash" }, json) + + -- Invalid cookie in hash fallback + res = assert(client:send { + method = "POST", + path = "/upstreams", + body = { + name = "my.upstream", + hash_on = "consumer", + hash_fallback = "cookie", + hash_on_cookie = "not a <> valid <> cookie name", + }, + headers = {["Content-Type"] = content_type} + }) + body = assert.res_status(400, res) + local json = cjson.decode(body) + assert.same({ message = "Cookie name: bad cookie name 'not a <> valid <> cookie name', allowed characters are A-Z, a-z, 0-9, '_', and '-'" }, json) + + -- Invalid cookie path in hash fallback + res = assert(client:send { + method = "POST", + path = "/upstreams", + body = { + name = "my.upstream", + hash_on = "consumer", + hash_fallback = "cookie", + hash_on_cookie = "my-cookie", + hash_on_cookie_path = "not a path", + }, + headers = {["Content-Type"] = content_type} + }) + body = assert.res_status(400, res) + local json = cjson.decode(body) + assert.same({ message = "Cookie path: must be prefixed with slash" }, json) + end end) it_content_types("returns 409 on conflict", function(content_type) diff --git a/spec/02-integration/05-proxy/09-balancer_spec.lua b/spec/02-integration/05-proxy/09-balancer_spec.lua index 31414b3d536a..8ae7db6f1ce1 100644 --- a/spec/02-integration/05-proxy/09-balancer_spec.lua +++ b/spec/02-integration/05-proxy/09-balancer_spec.lua @@ -1331,37 +1331,123 @@ for _, strategy in helpers.each_strategy() do describe("with consistent hashing", function() - it("over multiple targets", function() - local requests = SLOTS * 2 -- go round the balancer twice + describe("over multiple targets", function() + + it("hashing on header", function() + local requests = SLOTS * 2 -- go round the balancer twice + + local upstream_name = add_upstream({ + hash_on = "header", + hash_on_header = "hashme", + }) + local port1 = add_target(upstream_name, localhost) + local port2 = add_target(upstream_name, localhost) + local api_host = add_api(upstream_name) + + -- setup target servers + local server1 = http_server(localhost, port1, { requests }) + local server2 = http_server(localhost, port2, { requests }) + + -- Go hit them with our test requests + local oks = client_requests(requests, { + ["Host"] = api_host, + ["hashme"] = "just a value", + }) + assert.are.equal(requests, oks) + + -- collect server results; hitcount + -- one should get all the hits, the other 0 + local _, count1 = server1:done() + local _, count2 = server2:done() + + -- verify + assert(count1 == 0 or count1 == requests, "counts should either get 0 or ALL hits") + assert(count2 == 0 or count2 == requests, "counts should either get 0 or ALL hits") + assert(count1 + count2 == requests) + end) + + describe("hashing on cookie", function() + it("does not reply with Set-Cookie if cookie is already set", function() + local upstream_name = add_upstream({ + hash_on = "cookie", + hash_on_cookie = "hashme", + }) + local port = add_target(upstream_name, localhost) + local api_host = add_api(upstream_name) + + -- setup target server + local server = http_server(localhost, port, { 1 }) + + -- send request + local client = helpers.proxy_client() + local res = client:send { + method = "GET", + path = "/", + headers = { + ["Host"] = api_host, + ["Cookie"] = "hashme=some-cookie-value", + } + } + local set_cookie = res.headers["Set-Cookie"] + + client:close() + server:done() + + -- verify + assert.is_nil(set_cookie) + end) + + it("replies with Set-Cookie if cookie is not set", function() + local requests = SLOTS * 2 -- go round the balancer twice + + local upstream_name = add_upstream({ + hash_on = "cookie", + hash_on_cookie = "hashme", + }) + local port1 = add_target(upstream_name, localhost) + local port2 = add_target(upstream_name, localhost) + local api_host = add_api(upstream_name) + + -- setup target servers + local server1 = http_server(localhost, port1, { requests }) + local server2 = http_server(localhost, port2, { requests }) + + -- initial request without the `hash_on` cookie + local client = helpers.proxy_client() + local res = client:send { + method = "GET", + path = "/", + headers = { + ["Host"] = api_host, + ["Cookie"] = "some-other-cooke=some-other-value", + } + } + local cookie = res.headers["Set-Cookie"]:match("hashme%=(.*)%;") - local upstream_name = add_upstream({ - hash_on = "header", - hash_on_header = "hashme", - }) - local port1 = add_target(upstream_name, localhost) - local port2 = add_target(upstream_name, localhost) - local api_host = add_api(upstream_name) + client:close() - -- setup target servers - local server1 = http_server(localhost, port1, { requests }) - local server2 = http_server(localhost, port2, { requests }) + -- subsequent requests add the cookie that was set by the first response + local oks = 1 + client_requests(requests - 1, { + ["Host"] = api_host, + ["Cookie"] = "hashme=" .. cookie, + }) + assert.are.equal(requests, oks) - -- Go hit them with our test requests - local oks = client_requests(requests, { - ["Host"] = api_host, - ["hashme"] = "just a value", - }) - assert.are.equal(requests, oks) + -- collect server results; hitcount + -- one should get all the hits, the other 0 + local _, count1 = server1:done() + local _, count2 = server2:done() - -- collect server results; hitcount - -- one should get all the hits, the other 0 - local _, count1 = server1:done() - local _, count2 = server2:done() + -- verify + assert(count1 == 0 or count1 == requests, + "counts should either get 0 or ALL hits, but got " .. count1 .. " of " .. requests) + assert(count2 == 0 or count2 == requests, + "counts should either get 0 or ALL hits, but got " .. count2 .. " of " .. requests) + assert(count1 + count2 == requests) + end) + + end) - -- verify - assert(count1 == 0 or count1 == requests, "counts should either get 0 or ALL hits") - assert(count2 == 0 or count2 == requests, "counts should either get 0 or ALL hits") - assert(count1 + count2 == requests) end) end)