-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add hash on cookie #3472
Add hash on cookie #3472
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you ok with this comment to address the need to refactor the balancer structure? cc: @thibaultcha @Tieske |
||
ctx.balancer_hash_cookie = { | ||
key = upstream.hash_on_cookie, | ||
value = identifier, | ||
path = upstream.hash_on_cookie_path | ||
} | ||
end | ||
|
||
end | ||
|
||
if identifier then | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. perhaps also add a "cookie" entry to the "fallback" section below at https://github.com/Kong/kong/pull/3472/files#diff-354d3b9f422325fb4a8342988b3a500cR539 ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch! Didn't see that :) |
||
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() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like this block is the code same as the block above, but it should exercise There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Woops! Fixed |
||
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) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there some automated way contributors are generating these migrations? I just manually created the timestamp/ name and it seemed to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no migrations have to be created manual (for now). Also datastore specific. And indeed the name is date+timestamp, just to make it unique (within a migrations file).