Fix ingress updating for session-cookie-* annotation changes #3740

alexkursell · 2019-02-07T20:55:17Z

What this PR does / why we need it: The sticky load balancer was ignoring changes in

nginx.ingress.kubernetes.io/session-cookie-name
nginx.ingress.kubernetes.io/session-cookie-hash
nginx.ingress.kubernetes.io/session-cookie-expires
nginx.ingress.kubernetes.io/session-cookie-max-age

requiring the creation of a new pod before the annotations were applied.

Which issue this PR fixes : fixes #3725

Special notes for your reviewer: @ElvinEfendi let me know if this change is big enough to add an e2e test.

alexkursell · 2019-02-07T21:15:39Z

Side note: why does nginx.ingress.kubernetes.io/session-cookie-hash exist? The only thing I can tell that the hash function does is to make randomly generated cookie value look more random, so why does the hash function you use for that need to be swappable or user-configurable?

aledbf · 2019-02-07T22:45:43Z

Side note: why does nginx.ingress.kubernetes.io/session-cookie-hash exist?

Because before the lua implementation we used bitbucket.org/nginx-goodies/nginx-sticky-module-ng where you can choose between index|md5|sha1 as implementation

aledbf · 2019-02-07T22:46:12Z

@alexkursell @ElvinEfendi if this makes no sense now we could deprecate the annotation.

aledbf · 2019-02-12T13:52:37Z

@alexkursell please squash the commits

rootfs/etc/nginx/lua/balancer/sticky.lua

ElvinEfendi · 2019-02-12T14:19:33Z

@alexkursell you can remove all individual attributes at https://github.com/kubernetes/ingress-nginx/pull/3740/files#diff-8f83606fdd5dc8d2406cda71b9609762R21 now, since you're storing the whole config object now.

ElvinEfendi · 2019-02-12T14:27:09Z

This is a great find @alexkursell ! I'm sure chashsubset suffers from the similar issue since it's also skipping sync when endpoints don't change. cc @diegows

alexkursell · 2019-02-13T15:41:54Z

@alexkursell you can remove all individual attributes at https://github.com/kubernetes/ingress-nginx/pull/3740/files#diff-8f83606fdd5dc8d2406cda71b9609762R21 now, since you're storing the whole config object now.

Fixed

diegows · 2019-02-13T15:57:40Z

ack, I'll check chashsubset this week.

ElvinEfendi · 2019-02-18T20:20:10Z

rootfs/etc/nginx/lua/balancer/sticky.lua

@@ -15,13 +15,10 @@ function _M.new(self, backend)
  local o = {
    instance = self.factory:new(nodes),
    cookie_name = backend["sessionAffinityConfig"]["cookieSessionAffinity"]["name"] or "route",


Why are you keeping this?

Thought it looked cleaner than oring it with "route" in 2 different places. But this is probably more confusing, because everything else is in cookie_sesstion_affinity. fixed.

ElvinEfendi · 2019-02-18T20:25:13Z

rootfs/etc/nginx/lua/balancer/sticky.lua

+    return
+  end
+
+  for key, val in pairs(self:new(backend)) do


This is not good - what you wanna do here is to update only the things that might change when self.cookie_session_affinity changes. But right now you are also creating a new resty balancer instance and updating the instance of self with new one unnecessarily.

All you need to do here is to update cookie_session_affinity and digest_func.

ElvinEfendi · 2019-02-19T19:05:31Z

rootfs/etc/nginx/lua/test/balancer/sticky_test.lua

-      end)
-    end)
-
-    context("when backend does not specify cookie name", function()


Why is this test not necessary anymore?

I removed it because the actual cookie name wasn't being stored on the balancer anymore. I've added the test back and made it use a method on the balancer to check the cookie name.

ElvinEfendi · 2019-02-19T19:06:06Z

rootfs/etc/nginx/lua/balancer/sticky.lua

@@ -4,24 +4,26 @@ local util = require("util")
 local ck = require("resty.cookie")

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


The convention is to use uppercase for constants.

ElvinEfendi · 2019-02-19T19:07:29Z

rootfs/etc/nginx/lua/balancer/sticky.lua

  if not cookie_path then
    cookie_path = ngx.var.location_path
  end

+  local cookie_name = self.cookie_session_affinity.name or default_cookie_name


You could define a local function cookie_name to avoid duplicate oring

fixed. I defined a method so thatsticky_test.lua can also use it to check that the logic is correct.

Usually we define local function in these cases and then conditionally export it for testing if _TEST global variable is defined. But it's OK.

ElvinEfendi · 2019-02-19T21:37:49Z

/lgtm
/approve

alexkursell · 2019-02-19T22:28:01Z

@ElvinEfendi , sorry, needs another lgtm after I squashed the commits.

ElvinEfendi · 2019-02-19T23:09:56Z

/lgtm
/approve

k8s-ci-robot · 2019-02-19T23:10:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexkursell, ElvinEfendi

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:

~~OWNERS~~ [ElvinEfendi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 7, 2019

k8s-ci-robot requested review from aledbf and ElvinEfendi February 7, 2019 20:55

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 7, 2019

alexkursell force-pushed the session-annotation-reload branch from 9d63dfa to c1c98d6 Compare February 7, 2019 21:05

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 8, 2019

alexkursell mentioned this pull request Feb 8, 2019

Remove session-cookie-hash annotation #3743

Merged

ElvinEfendi reviewed Feb 12, 2019

View reviewed changes

rootfs/etc/nginx/lua/balancer/sticky.lua Outdated Show resolved Hide resolved

alexkursell force-pushed the session-annotation-reload branch 2 times, most recently from be79d13 to 6e41383 Compare February 13, 2019 15:41

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 13, 2019

ElvinEfendi reviewed Feb 18, 2019

View reviewed changes

alexkursell force-pushed the session-annotation-reload branch from 20bb1f9 to 279bd0e Compare February 19, 2019 18:40

ElvinEfendi reviewed Feb 19, 2019

View reviewed changes

k8s-ci-robot assigned ElvinEfendi Feb 19, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 19, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 19, 2019

Fix session-cookie-* annotation reloading

c180a09

alexkursell force-pushed the session-annotation-reload branch from 90d3516 to c180a09 Compare February 19, 2019 22:27

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 19, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 19, 2019

k8s-ci-robot merged commit 15d5ef9 into kubernetes:master Feb 19, 2019

alexkursell deleted the session-annotation-reload branch February 20, 2019 14:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ingress updating for session-cookie-* annotation changes #3740

Fix ingress updating for session-cookie-* annotation changes #3740

alexkursell commented Feb 7, 2019

alexkursell commented Feb 7, 2019 •

edited

Loading

aledbf commented Feb 7, 2019

aledbf commented Feb 7, 2019

aledbf commented Feb 12, 2019

ElvinEfendi commented Feb 12, 2019

ElvinEfendi commented Feb 12, 2019

alexkursell commented Feb 13, 2019

diegows commented Feb 13, 2019

ElvinEfendi Feb 18, 2019

alexkursell Feb 19, 2019

ElvinEfendi Feb 18, 2019

alexkursell Feb 19, 2019

ElvinEfendi Feb 19, 2019

alexkursell Feb 19, 2019

ElvinEfendi Feb 19, 2019

alexkursell Feb 19, 2019

ElvinEfendi Feb 19, 2019

alexkursell Feb 19, 2019

ElvinEfendi Feb 19, 2019

ElvinEfendi commented Feb 19, 2019

alexkursell commented Feb 19, 2019

ElvinEfendi commented Feb 19, 2019

k8s-ci-robot commented Feb 19, 2019

Fix ingress updating for session-cookie-* annotation changes #3740

Fix ingress updating for session-cookie-* annotation changes #3740

Conversation

alexkursell commented Feb 7, 2019

alexkursell commented Feb 7, 2019 • edited Loading

aledbf commented Feb 7, 2019

aledbf commented Feb 7, 2019

aledbf commented Feb 12, 2019

ElvinEfendi commented Feb 12, 2019

ElvinEfendi commented Feb 12, 2019

alexkursell commented Feb 13, 2019

diegows commented Feb 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElvinEfendi commented Feb 19, 2019

alexkursell commented Feb 19, 2019

ElvinEfendi commented Feb 19, 2019

k8s-ci-robot commented Feb 19, 2019

alexkursell commented Feb 7, 2019 •

edited

Loading