Fix session affinity for canaries #7169

wasker · 2021-05-27T02:01:09Z

What this PR does / why we need it:

If canary ingress is configured, session affinity settings no longer apply, which results in requests within the same session being served by both canary and non-canary endpoints. This affects scenarios when backwards-incompatible changes were deployed to canary, and in inconsistent results being served in general.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

The change makes canaries respect session affinity settings by default, as this makes more sense than the current behavior. Nonetheless, there's a way to restore original behavior by adding a special annotation on a non-canary ingress definition, if someone prefers that behavior for some reason.

Which issue/s this PR fixes

Fixes #3717.

How Has This Been Tested?

Used repro steps from the bug to simulate the issue before and after changes.

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I've read the CONTRIBUTION guide
I have added tests to cover my changes.
All new and existing tests passed.

k8s-ci-robot · 2021-05-27T02:01:16Z

Welcome @wasker!

It looks like this is your first PR to kubernetes/ingress-nginx 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/ingress-nginx has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2021-05-27T02:01:17Z

Hi @wasker. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

wasker · 2021-06-02T21:33:33Z

@cmluciano @ElvinEfendi Can I get eyes on this PR, please?

wasker · 2021-06-07T18:17:15Z

@rikatz Could you help with unblocking this PR, please?

bowei · 2021-06-14T17:55:33Z

/ok-to-test

wasker · 2021-06-14T18:24:30Z

/retest

bowei · 2021-06-16T19:52:39Z

/assign @rikatz

rikatz · 2021-06-21T17:28:20Z

/kind bug
/priority important-soon

wasker · 2021-06-24T21:00:54Z

/retest

wasker · 2021-06-24T21:11:15Z

@rikatz FYI - after rebasing onto the latest from master, SSL test cases do not pass + there's an issue with a build. Is this known?

rikatz · 2021-06-27T19:10:48Z

Hi @wasker

From a Go perspective your PR looks good to me. I've asked for @ElvinEfendi and @moonming to do a review in Lua part.

To be honest I'm not sure we should change the default behavior (even broken). I think so, but we need to be really careful warning users about that, like "after this release, it will start working as desired but the desired may not be your desired anymore" :)

Let's re-test your tests and I'll keep an eye here, maybe you need to rebase so the latest commits/tests are present in yours as well :)

EDIT: Just saw that you already rebased, let me check here

rikatz · 2021-06-27T19:11:04Z

/retest

rikatz · 2021-06-27T19:12:15Z

/triage accepted
/cc @moonming @ElvinEfendi

k8s-ci-robot · 2021-06-27T19:12:16Z

@rikatz: The label(s) triage/approved cannot be applied, because the repository doesn't have them.

In response to this:

/triage approved
/cc @moonming @ElvinEfendi

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.

rikatz · 2021-06-27T19:12:44Z

/triage accepted

wasker · 2021-06-29T15:58:57Z

@rikatz I see this and this setup, and yet SSL tests are failing due to "unexpected error storing SSL certificate: could not create PEM certificate file /etc/ingress-controller/ssl/test-1624980835549457600.pem".

Am I missing something? This fails both in PR and locally.

* Drop v1beta1 from ingress nginx Signed-off-by: Ricardo Pchevuzinske Katz <[email protected]> * Fix intorstr logic in controller Signed-off-by: Ricardo Pchevuzinske Katz <[email protected]> * fixing admission Signed-off-by: Ricardo Pchevuzinske Katz <[email protected]> * more intorstr fixing * correct template rendering Signed-off-by: Ricardo Pchevuzinske Katz <[email protected]> * Fix e2e tests for v1 api Signed-off-by: Ricardo Pchevuzinske Katz <[email protected]> * Fix gofmt errors * This is finally working...almost there... Signed-off-by: Ricardo Pchevuzinske Katz <[email protected]> * Re-add removed validation of AdmissionReview

Update the DaemonSet namespace references to use the `POD_NAMESPACE` environment variable in the same way that the Deployment does.

Signed-off-by: Jintao Zhang <[email protected]>

) It should be 128 as documented in https://github.com/kubernetes/ingress-nginx/blob/master/internal/ingress/controller/config/config.go#L780

* Fix MaxWorkerOpenFiles calculation on high cores nodes * Add e2e test for rlimit_nofile * Fix doc for max-worker-open-files

Signed-off-by: RinkiyaKeDad <[email protected]>

Signed-off-by: Carlos Panato <[email protected]>

k8s-ci-robot · 2021-07-17T15:55:00Z

@wasker: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Rerun command
pull-ingress-nginx-test	`b5507fb`	link	`/test pull-ingress-nginx-test`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

wasker · 2021-07-18T18:49:59Z

@rikatz I restarted the change here: #7371

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 27, 2021

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 27, 2021

k8s-ci-robot requested review from cmluciano and ElvinEfendi May 27, 2021 02:01

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 14, 2021

wasker mentioned this pull request Jun 14, 2021

Fix chashsubset backend with canary #7235

Merged

8 tasks

k8s-ci-robot assigned rikatz Jun 16, 2021

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Jun 21, 2021

missedone mentioned this pull request Jun 22, 2021

Session affinity doesn't work for canaries #3717

Closed

k8s-ci-robot requested a review from moonming June 27, 2021 19:12

k8s-ci-robot added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Jun 27, 2021

wasker and others added 26 commits July 17, 2021 08:38

UTs for affinitized requests.

f6621a6

Follow same patterns as balancer UTs.

c1a006e

UTs for cookies

1e6bc30

Use same style across the board.

e5b9bcd

Documentation updates.

c89c060

Make existing UTs pass.

006c136

Added UTs for "sticky" canary behavior.

2f3e54d

Tests for annotations parser.

131b509

Fixed lua linter errors.

dcfafba

Revert unnecessary changes.

bea4768

add docs for syslog feature (#7219)

f2b73ce

Fix link to e2e-tests.md in developer-guide (#7201)

60afa7d

Use ENV expansion for namespace in args (#7146)

cfde9df

Update the DaemonSet namespace references to use the `POD_NAMESPACE` environment variable in the same way that the Deployment does.

chart: using Helm builtin capabilities check (#7190)

9898bae

Signed-off-by: Jintao Zhang <[email protected]>

Update proper default value for HTTP2MaxConcurrentStreams in Docs (#6944

4168ab9

) It should be 128 as documented in https://github.com/kubernetes/ingress-nginx/blob/master/internal/ingress/controller/config/config.go#L780

Fix MaxWorkerOpenFiles calculation on high cores nodes (#7107)

62609f2

* Fix MaxWorkerOpenFiles calculation on high cores nodes * Add e2e test for rlimit_nofile * Fix doc for max-worker-open-files

Use v1 API contract.

39e1d31

Fixed setup for SSL UTs.

67ec22a

ingress/tcp: add additional error logging on failed (#7208)

55ea378

replacing master with main for k/k8s.io links (#7307)

8ca9c12

Signed-off-by: RinkiyaKeDad <[email protected]>

Add file containing stable release (#7313)

2adde86

Handle named (non-numeric) ports correctly (#7311)

a26d8f8

Signed-off-by: Carlos Panato <[email protected]>

Updated v1beta1 to v1 as its deprecated (#7308)

bfc5a8a

use listen to ensure the port is free (#6990)

f152efe

remove mercurial from build (#7031)

b5507fb

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 17, 2021

wasker closed this Jul 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix session affinity for canaries #7169

Fix session affinity for canaries #7169

wasker commented May 27, 2021

k8s-ci-robot commented May 27, 2021

k8s-ci-robot commented May 27, 2021

wasker commented Jun 2, 2021

wasker commented Jun 7, 2021

bowei commented Jun 14, 2021

wasker commented Jun 14, 2021

bowei commented Jun 16, 2021

rikatz commented Jun 21, 2021

wasker commented Jun 24, 2021

wasker commented Jun 24, 2021 •

edited

Loading

rikatz commented Jun 27, 2021 •

edited

Loading

rikatz commented Jun 27, 2021

rikatz commented Jun 27, 2021 •

edited

Loading

k8s-ci-robot commented Jun 27, 2021

rikatz commented Jun 27, 2021

wasker commented Jun 29, 2021

k8s-ci-robot commented Jul 17, 2021 •

edited

Loading

wasker commented Jul 18, 2021

Fix session affinity for canaries #7169

Fix session affinity for canaries #7169

Conversation

wasker commented May 27, 2021

What this PR does / why we need it:

Types of changes

Which issue/s this PR fixes

How Has This Been Tested?

Checklist:

k8s-ci-robot commented May 27, 2021

k8s-ci-robot commented May 27, 2021

wasker commented Jun 2, 2021

wasker commented Jun 7, 2021

bowei commented Jun 14, 2021

wasker commented Jun 14, 2021

bowei commented Jun 16, 2021

rikatz commented Jun 21, 2021

wasker commented Jun 24, 2021

wasker commented Jun 24, 2021 • edited Loading

rikatz commented Jun 27, 2021 • edited Loading

rikatz commented Jun 27, 2021

rikatz commented Jun 27, 2021 • edited Loading

k8s-ci-robot commented Jun 27, 2021

rikatz commented Jun 27, 2021

wasker commented Jun 29, 2021

k8s-ci-robot commented Jul 17, 2021 • edited Loading

wasker commented Jul 18, 2021

wasker commented Jun 24, 2021 •

edited

Loading

rikatz commented Jun 27, 2021 •

edited

Loading

rikatz commented Jun 27, 2021 •

edited

Loading

k8s-ci-robot commented Jul 17, 2021 •

edited

Loading