fix: issue where canary overwrites default backend #10072

Spazzy757 · 2023-06-12T07:25:44Z

What this PR does / why we need it:

We have a bug in the case of canary deployments where if the canary deployment returns an error code it will ignore the custom-error codes and custom default backend setup.

This is due to how we overwrite the balancer in the function route_to_alternative_balancer as it will take precedence over any balancer that is set

NOTE: Majority of the code added here is around E2E testing for this specific scenario. This code will be simplified with the work in #9948

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
CVE Report (Scanner found CVE and adding report)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation only

Which issue/s this PR fixes

fixes: #9944

How Has This Been Tested?

Checklist:

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

/kind bug

rootfs/etc/nginx/template/nginx.tmpl

Spazzy757 · 2023-06-14T12:04:57Z

/kind bug

Spazzy757 · 2023-06-14T12:07:09Z

/triage accepted

Spazzy757 · 2023-06-14T12:07:32Z

/assign @tao12345666333

tao12345666333 · 2023-06-19T02:01:58Z

I am reading the full context of this PR and issue, it will take some time. Thanks

k8s-ci-robot · 2023-07-04T12:40:16Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Spazzy757
Once this PR has been reviewed and has the lgtm label, please ask for approval from tao12345666333. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

strongjz · 2023-07-20T15:43:49Z

/priority backlog
/ok-to-test

strongjz · 2023-07-20T15:44:19Z

Possible candidate for 1.9.0

dfl-aeb · 2024-01-30T12:19:42Z

@tao12345666333 @Spazzy757 @strongjz any news on this PR?

dfl-aeb · 2024-02-09T07:39:58Z

@tao12345666333 @Spazzy757
Sorry to bother you again, this PR looks pretty ready to me. Can we help push it forward?

Spazzy757 · 2024-02-09T08:13:24Z

I will rebase the PR, but I am waiting on @tao12345666333 or @strongjz to approve this. As I believe the concern is that it's a breaking change

tao12345666333 · 2024-02-10T07:46:56Z

Thanks 🙏 let me do a review today

dfl-aeb · 2024-02-19T06:08:24Z

@tao12345666333 Friendly reminder 😊👌

Spazzy757 · 2024-02-29T06:28:10Z

I'll try rebase again tomorrow @tao12345666333 @strongjz

Signed-off-by: Spazzy <brendankamp757@gmail.com>

tao12345666333 · 2024-03-03T15:52:18Z

test/e2e/annotations/canary.go

+				f.HTTPTestClient().
+					GET("/status/401").
+					WithHeader("Host", host).
+					Expect().
+					Status(http.StatusUnauthorized).
+					Body().
+					Contains("401")


Due to the passage of time, I need to reload some memories.

Can this case cover the actual scenarios in that issue?

Yes, the test case was written specifically to reproduce the issue, before fixing it

Means we are good to go? ;)

I have been trying to find time to verify what would happen with this test case before the Lua part modification.

This is the only issue that I am currently worried about.

@dfl-aeb @Spazzy757 Sorry for the delay, very busy with daily work.

Can you help me verify the result of this test case when there are no modifications to the Lua part? Other than that, I have no other concerns about this PR.

Sorry to bother you again @Spazzy757
Do you think you can find some time in the next few weeks?

I can try do this next week and paste the results, I wrote the test before making changes to the Lua to verify the issue.

I also tested this, manually using the same method I used in #9944

Do you have any updates @Spazzy757 ?

@Spazzy757 @tao12345666333 friendly reminder :)

@Spazzy757 could you please share your test results?

dfl-aeb · 2024-03-27T09:17:48Z

@Spazzy757 @tao12345666333 are we good to go?

Spazzy757 · 2024-03-27T11:35:57Z

@Spazzy757 @tao12345666333 are we good to go?

I assume so but not sure when it will be merged

k8s-ci-robot · 2024-09-07T00:43:01Z

PR needs rebase.

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-sigs/prow repository.

dfl-aeb · 2024-09-24T08:36:07Z

@Spazzy757 @tao12345666333 @longwuyuan
Is there any update on this? It's been a while, and this issue is still affecting us. Would appreciate any feedback or progress.
Thanks!

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 12, 2023

Spazzy757 marked this pull request as draft June 12, 2023 07:25

k8s-ci-robot requested review from strongjz and tao12345666333 June 12, 2023 07:25

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 12, 2023

Spazzy757 commented Jun 12, 2023

View reviewed changes

rootfs/etc/nginx/template/nginx.tmpl Outdated Show resolved Hide resolved

Spazzy757 force-pushed the fix-canary-annotations branch from 0bec4f4 to 868943d Compare June 14, 2023 11:52

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 14, 2023

Spazzy757 force-pushed the fix-canary-annotations branch 2 times, most recently from 4f0ef5a to c4d4afa Compare June 14, 2023 11:55

Spazzy757 marked this pull request as ready for review June 14, 2023 11:56

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 Jun 14, 2023

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Jun 14, 2023

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jun 14, 2023

k8s-ci-robot assigned tao12345666333 Jun 14, 2023

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

Spazzy757 force-pushed the fix-canary-annotations branch from c4d4afa to 84d2d02 Compare July 4, 2023 12:40

Spazzy757 force-pushed the fix-canary-annotations branch 3 times, most recently from 5480293 to 3615993 Compare July 11, 2023 12:22

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-priority labels Jul 20, 2023

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 31, 2023

Spazzy757 force-pushed the fix-canary-annotations branch from 3615993 to bb89ea0 Compare February 9, 2024 08:23

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

Spazzy757 force-pushed the fix-canary-annotations branch from bb89ea0 to c0ae368 Compare February 9, 2024 08:32

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 28, 2024

Spazzy757 force-pushed the fix-canary-annotations branch from c0ae368 to 68a77ab Compare March 1, 2024 08:14

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

fix: issue where canary overwrites default backend

64229d2

Signed-off-by: Spazzy <brendankamp757@gmail.com>

Spazzy757 force-pushed the fix-canary-annotations branch from 68a77ab to 64229d2 Compare March 1, 2024 08:16

tao12345666333 reviewed Mar 3, 2024

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 7, 2024

dfl-aeb mentioned this pull request Sep 24, 2024

Default-backend is ignored by canary ingress #9944

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: issue where canary overwrites default backend #10072

fix: issue where canary overwrites default backend #10072

Spazzy757 commented Jun 12, 2023 •

edited

Loading

Spazzy757 commented Jun 14, 2023

Spazzy757 commented Jun 14, 2023

Spazzy757 commented Jun 14, 2023

tao12345666333 commented Jun 19, 2023

k8s-ci-robot commented Jul 4, 2023

strongjz commented Jul 20, 2023

strongjz commented Jul 20, 2023

dfl-aeb commented Jan 30, 2024

dfl-aeb commented Feb 9, 2024

Spazzy757 commented Feb 9, 2024

tao12345666333 commented Feb 10, 2024

dfl-aeb commented Feb 19, 2024

Spazzy757 commented Feb 29, 2024

tao12345666333 Mar 3, 2024

Spazzy757 Mar 8, 2024

dfl-aeb Mar 19, 2024

tao12345666333 Mar 27, 2024

tao12345666333 Mar 27, 2024

dfl-aeb May 15, 2024

Spazzy757 May 16, 2024

dfl-aeb Jun 12, 2024

dfl-aeb Jul 2, 2024

dfl-aeb Aug 29, 2024

dfl-aeb commented Mar 27, 2024

Spazzy757 commented Mar 27, 2024

k8s-ci-robot commented Sep 7, 2024

dfl-aeb commented Sep 24, 2024

fix: issue where canary overwrites default backend #10072

Are you sure you want to change the base?

fix: issue where canary overwrites default backend #10072

Conversation

Spazzy757 commented Jun 12, 2023 • edited Loading

What this PR does / why we need it:

Types of changes

Which issue/s this PR fixes

How Has This Been Tested?

Checklist:

Spazzy757 commented Jun 14, 2023

Spazzy757 commented Jun 14, 2023

Spazzy757 commented Jun 14, 2023

tao12345666333 commented Jun 19, 2023

k8s-ci-robot commented Jul 4, 2023

strongjz commented Jul 20, 2023

strongjz commented Jul 20, 2023

dfl-aeb commented Jan 30, 2024

dfl-aeb commented Feb 9, 2024

Spazzy757 commented Feb 9, 2024

tao12345666333 commented Feb 10, 2024

dfl-aeb commented Feb 19, 2024

Spazzy757 commented Feb 29, 2024

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

dfl-aeb commented Mar 27, 2024

Spazzy757 commented Mar 27, 2024

k8s-ci-robot commented Sep 7, 2024

dfl-aeb commented Sep 24, 2024

Spazzy757 commented Jun 12, 2023 •

edited

Loading