[1759] Ingress affinity session cookie with Secure flag for HTTPS #3509
Conversation
Signed-off-by: Fabian Topfstedt <topfstedt@schneevonmorgen.com>
k8s-ci-robot
added
cncf-cla: yes
|
/assign @bowei Thanks a lot! |
|
@fabiant7t please don't assign PRs manually |
|
@aledbf I'm sorry, I followed k8s-ci-robot's hint above, which must have been addressed to the reviewers then. |
| @@ -40,7 +40,7 @@ end | |||
|
|
|||
| describe("Sticky", function() | |||
| before_each(function() | |||
| mock_ngx({ var = { location_path = "/", host = "test.com" } }) | |||
| mock_ngx({ var = { location_path = "/", host = "test.com", https = "on" } }) | |||
There was a problem hiding this comment.
instead of changing this globally please introduce another test just like https://github.com/kubernetes/ingress-nginx/pull/3509/files#diff-f62b36fb1d17870f3fbc347d5624fc1fR112 but for when ngx.var.https is "on".
Then inside that specific test case (it) you can mock $https using ngx.var.https = "on"
|
Thanks for fixing this @fabiant7t ! Just one comment about the unit test otherwise LGTM. |
Signed-off-by: Fabian Topfstedt <topfstedt@schneevonmorgen.com>
Signed-off-by: Fabian Topfstedt <topfstedt@schneevonmorgen.com>
k8s-ci-robot
added
size/M
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ElvinEfendi, fabiant7t The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
The annotation `ingress.k8s.collaborne.com/session-cookie-flags` can be set to "secure" to configure the sticky affinity cookie to have the 'secure' flag. This is a complete reimplementation of kubernetes#3509
Signed-off-by: Fabian Topfstedt topfstedt@schneevonmorgen.com
What this PR does / why we need it:
Ingress affinity session cookies did not set the
Secureflag when the Ingress is called through HTTPS. The affinity cookie is therefore ignored by the client and every recurring request to the ingress will result in a newSet-Cookieheader being send, preventing stickyness to work.Since ingresses may be called both through both HTTP and HTTPS, setting the
Secureflag depends on the context.My proposal to solve it:
(See http://nginx.org/en/docs/http/ngx_http_core_module.html#var_https).
secure(See https://github.com/cloudflare/lua-resty-cookie/blob/master/lib/resty/cookie.lua#L156)
cookie_datamap that has no set asecureattribute yet. Adding the keysecure = ngx.var.https == "on"should be sufficiant to set theSecurewhen (and only when) being in SSL mode.(See https://github.com/kubernetes/ingress-nginx/blob/master/rootfs/etc/nginx/lua/balancer/sticky.lua#L56)
Which issue this PR fixes: fixes #1759
Special notes for your reviewer:
I managed to run and edit the Lua testsuite to unit test my assumptions. Being my first PR, I need someone experienced to review the integration though.