Skip to content

pkg/redirectpolicy: Fix backend slices in processConfig #35496

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 5, 2024
Merged

pkg/redirectpolicy: Fix backend slices in processConfig #35496

merged 1 commit into from
Nov 5, 2024

Conversation

Sm0ckingBird
Copy link
Contributor

@Sm0ckingBird Sm0ckingBird commented Oct 23, 2024 •
edited by julianwiedmann
Loading

In each iteration of pod in function processConfigWithSinglePort and processConfigWithNamedPorts bes4 and bes6 need to be cleared. Otherwise, when size of pods is larger than one, aka when the iteration time is more than one, bes4 and bes6 will aggregate all of the backends.

For example, in the first iteration backend 10.0.2.250:80 is added, then in the second iteration [10.0.2.250:80, 10.0.2.199:80] are added.

10.108.13.48:80  LocalRedirect  1 => 10.0.2.199:80
                                2 => 10.0.2.250:80
                                3 => 10.0.2.250:80

Fixes: e7bb8a7 ("k8s/cilium Event handlers and processing logic for LRPs")

@Sm0ckingBird Sm0ckingBird requested a review from a team as a code owner October 23, 2024 02:52
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 23, 2024
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Oct 23, 2024
@julianwiedmann julianwiedmann added release-note/bug This PR fixes an issue in a previous release of Cilium. area/lrp Impacts Local Redirect Policy. labels Oct 24, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Oct 24, 2024
@julianwiedmann julianwiedmann added the kind/bug This is a bug in the Cilium logic. label Oct 24, 2024
@Sm0ckingBird Sm0ckingBird force-pushed the main branch 2 times, most recently from 252e9d9 to 3f45507 Compare October 24, 2024 17:31
@julianwiedmann
Copy link
Member

/test

@maintainer-s-little-helper
Copy link

Commit 35c4bc4 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Oct 29, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Oct 29, 2024
@Sm0ckingBird
Copy link
Contributor Author

/test

Cilium E2E Upgrade (ci-e2e-upgrade) test failed, and I did a rebasing, shall we redo the testing?

@julianwiedmann
Copy link
Member

hi @Sm0ckingBird , thank you for the contribution! That does look like a bug indeed :). Could you please add some tests in manager_test.go to reproduce the problem?

Besides that, just one comment to polish the patch:

For example, in the first iteration backend 10.0.2.250:80 is added, then in the second iteration [10.0.2.199:80 10.0.2.250:80, 10.0.2.199:80] are added.

I believe you meant to say the above, correct?

@Sm0ckingBird
Copy link
Contributor Author

hi @Sm0ckingBird , thank you for the contribution! That does look like a bug indeed :). Could you please add some tests in manager_test.go to reproduce the problem?

No problem :)

Besides that, just one comment to polish the patch:

For example, in the first iteration backend 10.0.2.250:80 is added, then in the second iteration [10.0.2.199:80 10.0.2.250:80, 10.0.2.199:80] are added.

I believe you meant to say the above, correct?

That's correct, thanks for pointing this out!

@Sm0ckingBird
Copy link
Contributor Author

Sm0ckingBird commented Oct 30, 2024 •
edited
Loading

@julianwiedmann tests added, without the fix, the following errors will be reported

=== RUN   TestManager_AddrMatcherConfigSinglePortMulPods
    manager_test.go:536: 
                Error Trace:    /data01/zijian/cilium/pkg/redirectpolicy/manager_test.go:536
                Error:          "[{{1.2.3.4 {TCP 8080} 0} { foo-be ns1} 0} {{5.6.7.8 {TCP 8080} 0} { foo-be ns1} 0} {{1.2.3.4 {TCP 8080} 0} { foo-be ns1} 0} {{5.6.7.8 {TCP 8080} 0} { foo-be ns1} 0} {{5.6.7.10 {TCP 8080} 0} { foo-be2 ns1} 0} {{5.6.7.9 {TCP 8080} 0} { foo-be2 ns1} 0}]" should have 4 item(s), but has 6
                Test:           TestManager_AddrMatcherConfigSinglePortMulPods
--- FAIL: TestManager_AddrMatcherConfigSinglePortMulPods (0.00s)
=== RUN   TestManager_AddrMatcherConfigMultiplePorts
--- PASS: TestManager_AddrMatcherConfigMultiplePorts (0.00s)
=== RUN   TestManager_AddrMatcherConfigMultiplePortsMulPods
    manager_test.go:685: 
                Error Trace:    /data01/zijian/cilium/pkg/redirectpolicy/manager_test.go:685
                Error:          "[{{1.2.3.4 {TCP 8080} 0} { foo-be ns1} 0} {{5.6.7.8 {TCP 8080} 0} { foo-be ns1} 0} {{1.2.3.4 {TCP 8080} 0} { foo-be ns1} 0} {{5.6.7.8 {TCP 8080} 0} { foo-be ns1} 0} {{5.6.7.10 {TCP 8080} 0} { foo-be2 ns1} 0} {{5.6.7.9 {TCP 8080} 0} { foo-be2 ns1} 0}]" should have 4 item(s), but has 6
                Test:           TestManager_AddrMatcherConfigMultiplePortsMulPods
--- FAIL: TestManager_AddrMatcherConfigMultiplePortsMulPods (0.00s)

Currently, I have the fix and the tests in one commit, do you prefer them in two separate commit?

@Sm0ckingBird
pkg/redirectpolicy: Fix backend slices in processConfig
30d77d9 
In each iteration of pod in function processConfigWithSinglePort and
processConfigWithNamedPorts bes4 and bes6 need to be cleared. Otherwise,
when size of pods is larger than one, aka when the iteration time is
more than one, bes4 and bes6 will aggregate all of the backends.

For example, in the first iteration backend 10.0.2.250:80 is added, then
in the second iteration [10.0.2.250:80, 10.0.2.199:80] are added.
10.108.13.48:80  LocalRedirect  1 => 10.0.2.199:80
                                2 => 10.0.2.250:80
                                3 => 10.0.2.250:80

Fixes: e7bb8a7 ("k8s/cilium Event handlers and processing logic for LRPs")

Signed-off-by: Zijian Zhang <zijianzhang@bytedance.com>
@aanm
Copy link
Member

aanm commented Nov 4, 2024

/test

@aanm aanm enabled auto-merge November 4, 2024 10:34
julianwiedmann
julianwiedmann approved these changes Nov 5, 2024
View reviewed changes
Copy link
Member

@julianwiedmann julianwiedmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you, lgtm! Having it as a single commit is fine.

@aanm aanm added this pull request to the merge queue Nov 5, 2024
Merged via the queue into cilium:main with commit 055b7a3 Nov 5, 2024
64 checks passed
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 5, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 5, 2024
@julianwiedmann julianwiedmann mentioned this pull request Dec 19, 2024
Closed
3 tasks
@julianwiedmann julianwiedmann added the needs-backport/1.16 This PR / issue needs backporting to the v1.16 branch label Dec 20, 2024
@viktor-kurchenko viktor-kurchenko mentioned this pull request Jan 7, 2025
Merged
7 tasks
@viktor-kurchenko viktor-kurchenko added backport-pending/1.16 The backport for Cilium 1.16.x for this PR is in progress. and removed needs-backport/1.16 This PR / issue needs backporting to the v1.16 branch labels Jan 7, 2025
@github-actions github-actions bot added backport-done/1.16 The backport for Cilium 1.16.x for this PR is done. and removed backport-pending/1.16 The backport for Cilium 1.16.x for this PR is in progress. labels Jan 15, 2025
This was referenced Jan 15, 2025
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julianwiedmann julianwiedmann julianwiedmann approved these changes

Assignees
No one assigned
Labels
area/lrp Impacts Local Redirect Policy. backport-done/1.16 The backport for Cilium 1.16.x for this PR is done. kind/bug This is a bug in the Cilium logic. kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Sm0ckingBird @julianwiedmann @aanm @viktor-kurchenko