Skip to content

Add tests to cover sticky cookie and rewrite-target annotations #1231

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

Merged
merged 2 commits into from
Nov 23, 2017
Merged

Add tests to cover sticky cookie and rewrite-target annotations #1231

merged 2 commits into from
Nov 23, 2017

Conversation

canhnt
Copy link
Contributor

@canhnt canhnt commented Aug 23, 2017

When an ingress rule enables sticky cookie (ingress.kubernetes.io/affinity: cookie combining with the ingress.kubernetes.io/rewrite-target, the sticky cookie feature does not work anymore.

Affect versions: from nginx-ingress-controller:0.9.0-beta.11

Reproduction:

  • Deploy http-svc:
kubectl create -f https://raw.githubusercontent.com/kubernetes/ingress-nginx/master/docs/examples/http-svc.yaml
  • Scale http-svc to 2 pods:
kubectl scale deploy http-svc --replicas=2
  • Create an ingress rule
echo "
apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  annotations:
    ingress.kubernetes.io/affinity: cookie
    ingress.kubernetes.io/session-cookie-hash: sha1
    ingress.kubernetes.io/session-cookie-name: SERVERID
    ingress.kubernetes.io/rewrite-target: /foo
    kubernetes.io/ingress.class: "nginx"
  name: example.com
spec:
  rules:
  - host: example.com
    http:
      paths:
      - backend:
          serviceName: http-svc
          servicePort: 80
        path: /
" | kubectl create -f - 
  • Create a request
curl -v http://<ingress-svc-address> -H 'Host: example.com'

Verify that the response header does not container a sticky cookie like

Set-Cookie: SERVERID=cbe88003c2aefcf43d9103dc5ce614ed733d0891; Path=/; HttpOnly

Remove ingress.kubernetes.io/rewrite-target annotation from the example.com ingress rule, the response header contains the sticky cookie.

Update: this bug has been fixed in NGINX: 0.9.0-beta.17.

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
  • If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: [email protected]

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-reviewable
Copy link

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Aug 23, 2017
@coveralls
Copy link

Coverage Status

Coverage remained the same at 44.407% when pulling 53cbb06b4ed7e2483ad550e7831aea0191e66fa3 on canhnt:sticky-path-rewriter into f0144a1 on kubernetes:master.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Aug 23, 2017
@coveralls
Copy link

Coverage Status

Coverage remained the same at 44.313% when pulling 3c2d5f1f3c1ec6e586c6b92cbb4921466f3344f4 on canhnt:sticky-path-rewriter into 338df02 on kubernetes:master.

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
  • If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: [email protected]

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Aug 24, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 44.378% when pulling 62797b90cb0d76b1676b7fff5f88376ba5ca1361 on canhnt:sticky-path-rewriter into e7d2ff6 on kubernetes:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 44.391% when pulling 62797b90cb0d76b1676b7fff5f88376ba5ca1361 on canhnt:sticky-path-rewriter into e7d2ff6 on kubernetes:master.

@canhnt canhnt changed the title WIP: Fixed sticky cookie annotation to work with rewrite-target annotation Fixed sticky cookie annotation to work with rewrite-target annotation Aug 24, 2017
@canhnt canhnt changed the title Fixed sticky cookie annotation to work with rewrite-target annotation [nginx] rewrite-target annotation conflicts with sticky cookie setting Aug 27, 2017
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 20, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 43.764% when pulling 2be1309ce5d42c97c5ee7e7de4ad88ca6b70bbc4 on canhnt:sticky-path-rewriter into 37bd14d on kubernetes:master.

@canhnt
Copy link
Contributor Author

canhnt commented Sep 21, 2017

Hi all,
I have rebased to get latest changes from master branch. What should I so this request can be merged?

Thanks,
Canh.

@canhnt canhnt changed the title [nginx] rewrite-target annotation conflicts with sticky cookie setting Rewrite-target annotation conflicts with sticky cookie setting Nov 13, 2017
@canhnt canhnt changed the title Rewrite-target annotation conflicts with sticky cookie setting Sticky cookie does not work with rewrite-target annotation Nov 13, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 36.278% when pulling 5625c6267124ca2e37d1dca25909ca6fc48c2662 on canhnt:sticky-path-rewriter into e4c8488 on kubernetes:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 36.241% when pulling 5625c6267124ca2e37d1dca25909ca6fc48c2662 on canhnt:sticky-path-rewriter into e4c8488 on kubernetes:master.

@bilalyasar
Copy link

@aledbf any plan for merging this pull request? we need this bugfix.

@aledbf
Copy link
Member

aledbf commented Nov 21, 2017

@bilalyasar no until we have e2e test to cover this scenario

@k8s-ci-robot 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 Nov 21, 2017
@canhnt canhnt changed the title Sticky cookie does not work with rewrite-target annotation WIP - Sticky cookie does not work with rewrite-target annotation Nov 21, 2017
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 21, 2017
@canhnt canhnt changed the title WIP - Sticky cookie does not work with rewrite-target annotation WIP: Sticky cookie does not work with rewrite-target annotation Nov 21, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 36.154% when pulling 3bc9e823366b6afab16a3759913ae9131d8c33db on canhnt:sticky-path-rewriter into 419fe52 on kubernetes:master.

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
  • If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: [email protected]

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 22, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 36.191% when pulling 77449f32489b5ca3aa4f8866565a5c8e64fc10d6 on canhnt:sticky-path-rewriter into 51da945 on kubernetes:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 36.154% when pulling 1d1a3975755e71a14e46bf5449086bcbb1a370e6 on canhnt:sticky-path-rewriter into 51da945 on kubernetes:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 36.191% when pulling 0b252f6398bb160fe32be9ab9a29e57f55f1c57c on canhnt:sticky-path-rewriter into 51da945 on kubernetes:master.

@aledbf
Copy link
Member

aledbf commented Nov 22, 2017

@canhnt please sign the CLA

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Nov 22, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 36.191% when pulling 89e8360 on canhnt:sticky-path-rewriter into 6657845 on kubernetes:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 36.154% when pulling 89e8360 on canhnt:sticky-path-rewriter into 6657845 on kubernetes:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 36.191% when pulling 76a01a9 on canhnt:sticky-path-rewriter into 6657845 on kubernetes:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 36.191% when pulling 7c81bc8 on canhnt:sticky-path-rewriter into 6657845 on kubernetes:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 36.191% when pulling d14054cb0ced41f58d0bfa16823644afcbb356ce on canhnt:sticky-path-rewriter into 6657845 on kubernetes:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 36.191% when pulling c7946dc on canhnt:sticky-path-rewriter into 6657845 on kubernetes:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 36.154% when pulling f039572 on canhnt:sticky-path-rewriter into 6657845 on kubernetes:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 37.322% when pulling d37722b on canhnt:sticky-path-rewriter into a5e8b81 on kubernetes:master.

@canhnt
Copy link
Contributor Author

canhnt commented Nov 23, 2017

@aledbf: this bug was fixed in master. I added a unit-test and e2e tests to cover this case. Could you please review it?
PS: I signed CLA.

@canhnt canhnt changed the title WIP: Sticky cookie does not work with rewrite-target annotation Add tests to cover sticky cookie and rewrite-target annotations Nov 23, 2017
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 23, 2017
@aledbf
Copy link
Member

aledbf commented Nov 23, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 23, 2017
@aledbf
Copy link
Member

aledbf commented Nov 23, 2017

@canhnt thanks!

@aledbf aledbf merged commit 82b4d2a into kubernetes:master Nov 23, 2017
@canhnt canhnt deleted the sticky-path-rewriter branch November 24, 2017 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants