Document for cookie expires annotation #3363

skeeey · 2018-11-05T09:25:11Z

What this PR does / why we need it:
Document for cookie expires annotation

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:
/assign @ElvinEfendi

ElvinEfendi · 2018-11-05T09:36:33Z

docs/examples/affinity/cookie/README.md

@@ -50,13 +54,13 @@ Date: Fri, 10 Feb 2017 14:11:12 GMT
 Content-Type: text/html
 Content-Length: 612
 Connection: keep-alive
-Set-Cookie: INGRESSCOOKIE=a9907b79b248140b56bb13723f72b67697baac3d; Path=/; HttpOnly
+Set-Cookie: INGRESSCOOKIE=a9907b79b248140b56bb13723f72b67697baac3d; Expires=Wed, 07-Nov-18 09:05:06 GMT; Max-Age=172800; Path=/; HttpOnly


Are you sure this is the correct expiry given the annotation is set to 172800? Isn’t the annotation value a Unix timestamp ?

Sure, there is a screenshot from my test enviroment

@ElvinEfendi Any more comments?

@skeeey as you can see from your screenshot it is set to 03 January 1970 which is expected given the way you implemented the feature. However in the docs you say it will be 07 Nov 2018.

@ElvinEfendi, for my screenshot, it is dev environment, I do not adjust the system time (the initial system time is 01 January 1970), so the result is 03 January 1970, but for document, I think, if user adjust his system time, it is more close to real world, Do you think we should keep the result as dev environment? if it is the expected, I can change this

01 January 1970 is Unix epoch time, same in every Unix based system.

aledbf · 2019-01-04T22:57:13Z

@ElvinEfendi ping

ElvinEfendi · 2019-01-05T00:11:48Z

docs/examples/affinity/cookie/README.md

@@ -11,6 +11,8 @@ 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|Number of seconds until the cookie expires that will correspond to cookie `Expires` directive|number of seconds|


This is not an accurate description. This value is rather a date as UNIX timestamp that the cookie will expire on.

change this to

|nginx.ingress.kubernetes.io/session-cookie-expires|The value is rather a date as UNIX timestamp that the cookie will expire on, it will correspond to cookie `Expires` directive|UNIX timestamp|

Do you think this is OK or not?

Looks good with a slight adjustment to the description as following:

The value is a date as UNIX timestamp that the cookie will expire on, it corresponds to cookie Expires directive

ElvinEfendi · 2019-01-07T14:35:46Z

@skeeey I think there's a flaw in the implementation. Let's say as in your example we set session-cookie-expires to 172800. With that whenever you send a request it will always set Expires to 03 January 1970, whereas expected behaviour should be it sets the Expires to <UNIX timestamp at the time of request> + 172800 so that the cookie lasts for 2 days.

skeeey · 2019-01-08T02:10:07Z

@skeeey I think there's a flaw in the implementation. Let's say as in your example we set session-cookie-expires to 172800. With that whenever you send a request it will always set Expires to 03 January 1970, whereas expected behaviour should be it sets the Expires to <UNIX timestamp at the time of request> + 172800 so that the cookie lasts for 2 days.

@ElvinEfendi, you are right, I examine the current implementation, I think the implementation is right, I use ngx.cookie_time to format the session-cookie-expires,

if self.cookie_expires and self.cookie_expires ~= "" then
      cookie_data.expires = ngx.cookie_time(tonumber(self.cookie_expires))
end

so the behavior should be our expected, the problem is the document is wrong, so I plan to update the example to

nginx.ingress.kubernetes.io/session-cookie-expires: "1546912535"
nginx.ingress.kubernetes.io/session-cookie-max-age: "172800"

to distinguish the session-cookie-max-age

and this example will yield

Set-Cookie: INGRESSCOOKIE=a9907b79b248140b56bb13723f72b67697baac3d; Expires=Tue, 08-Jan-19 01:55:35 GMT; Max-Age=172800; Path=/; HttpOnly

Do you think this is OK or not?

ElvinEfendi · 2019-01-08T16:23:12Z

  cookie_data.expires = ngx.cookie_time(tonumber(self.cookie_expires))

@skeeey the problem is here, it should look like this:

      cookie_data.expires = ngx.cookie_time(ngx.time() + tonumber(self.cookie_expires))

otherwise as I explained at #3363 (comment) everyone is going to get same Expires value in their cookie whenver (today, tomorrow, after week etc.) they get the cookie.

skeeey · 2019-01-09T05:24:37Z

@ElvinEfendi, I am confused by timestamp :), yes, I should add it, I correct the code and document, please see the new code diff, thanks

BTW, the current example date is Date: Fri, 10 Feb 2017 14:11:12 GMT, so if the session-cookie-expires is 172800, it should be Sun, 12-Feb-17 14:11:12 GMT

skeeey · 2019-01-10T09:38:32Z

@ElvinEfendi, the CI test failed, would you have a look? because I can run the test in my environment successfully

ElvinEfendi · 2019-01-10T14:28:37Z

test/e2e/annotations/affinity.go

@@ -219,7 +219,7 @@ var _ = framework.IngressNginxDescribe("Annotations - Affinity/Sticky Sessions",
 		Expect(resp.StatusCode).Should(Equal(http.StatusOK))
 		local, _ := time.LoadLocation("GMT")
 		duration, _ := time.ParseDuration("48h")
-		expected := time.Date(1970, time.January, 1, 0, 0, 0, 0, local).Add(duration).Format("Mon, 02-Jan-06 15:04:05 MST")
+		expected := time.Now().In(local).Add(duration).Format("Mon, 02-Jan-06 15:04:05 MST")


This is going to be flaky. Imagine you send the request at the very end of a second and by the time this code is being evaluated next second starts. Then the assertion below won't succeed because actual datetime will have second n, whereas expected will have n+1

I suggest you drop seconds from the format and leave only minute granularity.

ElvinEfendi · 2019-01-10T14:29:34Z

@skeeey this is looking good now. Once you address https://github.com/kubernetes/ingress-nginx/pull/3363/files#r246778598 please squash your commits.

aledbf · 2019-01-12T01:30:03Z

/check-cla

aledbf · 2019-01-12T01:32:03Z

@ElvinEfendi ping (last PR to be included in 0.22)

ElvinEfendi · 2019-01-14T15:49:36Z

docs/examples/affinity/cookie/README.md

@@ -11,6 +11,8 @@ 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|


This is not accurate now that we made this annotation to accept number of seconds as well (it gets appended to current time to generate date for Expires attribute)

ElvinEfendi · 2019-01-14T15:49:44Z

/lgtm
/approve

k8s-ci-robot · 2019-01-14T15:50:05Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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 assigned ElvinEfendi Nov 5, 2018

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 5, 2018

k8s-ci-robot requested review from antoineco and ElvinEfendi November 5, 2018 09:25

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 5, 2018

ElvinEfendi reviewed Nov 5, 2018

View reviewed changes

ElvinEfendi reviewed Jan 5, 2019

View reviewed changes

ElvinEfendi reviewed Jan 10, 2019

View reviewed changes

add cookie expires document and fix a flaw for session-cookie-expires

7aa5834

ElvinEfendi reviewed Jan 14, 2019

View reviewed changes

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

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

k8s-ci-robot merged commit 1db9c91 into kubernetes:master Jan 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document for cookie expires annotation #3363

Document for cookie expires annotation #3363

skeeey commented Nov 5, 2018

ElvinEfendi Nov 5, 2018

skeeey Nov 6, 2018

skeeey Nov 8, 2018

ElvinEfendi Jan 5, 2019

skeeey Jan 7, 2019

ElvinEfendi Jan 7, 2019

aledbf commented Jan 4, 2019

ElvinEfendi Jan 5, 2019

skeeey Jan 7, 2019

ElvinEfendi Jan 7, 2019

ElvinEfendi commented Jan 7, 2019

skeeey commented Jan 8, 2019 •

edited

Loading

ElvinEfendi commented Jan 8, 2019

skeeey commented Jan 9, 2019

skeeey commented Jan 10, 2019

ElvinEfendi Jan 10, 2019 •

edited

Loading

ElvinEfendi commented Jan 10, 2019

aledbf commented Jan 12, 2019

aledbf commented Jan 12, 2019

ElvinEfendi Jan 14, 2019

ElvinEfendi commented Jan 14, 2019

k8s-ci-robot commented Jan 14, 2019

Document for cookie expires annotation #3363

Document for cookie expires annotation #3363

Conversation

skeeey commented Nov 5, 2018

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

aledbf commented Jan 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElvinEfendi commented Jan 7, 2019

skeeey commented Jan 8, 2019 • edited Loading

ElvinEfendi commented Jan 8, 2019

skeeey commented Jan 9, 2019

skeeey commented Jan 10, 2019

ElvinEfendi Jan 10, 2019 • edited Loading

Choose a reason for hiding this comment

ElvinEfendi commented Jan 10, 2019

aledbf commented Jan 12, 2019

aledbf commented Jan 12, 2019

Choose a reason for hiding this comment

ElvinEfendi commented Jan 14, 2019

k8s-ci-robot commented Jan 14, 2019

skeeey commented Jan 8, 2019 •

edited

Loading

ElvinEfendi Jan 10, 2019 •

edited

Loading