Skip to content
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

Fix local redirect policies selecting host networked pods #18563

Merged
merged 5 commits into from
Jan 31, 2022

Conversation

aditighag
Copy link
Member

@aditighag aditighag commented Jan 20, 2022

See commit messages.

Fixes: da35c88 ("k8s/watchers: don't silently ignore (*K8sWatcher).updatePodHostData error")
Fixes: 0ab4fa1 ("pkg/k8s: ignore certain ipcache errors")
Related: #16920

Release note

Fix a bug with local redirect policies selecting host networked pods as local endpoints not taking effect.

@aditighag aditighag added kind/bug This is a bug in the Cilium logic. kind/regression This functionality worked fine before, but was broken in a newer release of Cilium. needs-backport/1.10 labels Jan 20, 2022
@aditighag aditighag requested review from a team as code owners January 20, 2022 21:45
@aditighag aditighag requested review from a team and joestringer January 20, 2022 21:45
@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 Jan 20, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.11.2 Jan 20, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.10.8 Jan 20, 2022
@aditighag aditighag added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Jan 20, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jan 20, 2022
@aditighag aditighag added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. and removed kind/bug This is a bug in the Cilium logic. labels Jan 20, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jan 20, 2022
@aditighag
Copy link
Member Author

aditighag commented Jan 20, 2022

/test

Job 'Cilium-PR-K8s-1.23-kernel-net-next' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sPolicyTest Multi-node policy test with L7 policy using connectivity-check to check datapath

Failure Output

FAIL: cannot install connectivity-check

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.23-kernel-net-next so I can create a new GitHub issue to track it.

Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

LGTM. One minor nit, the last commit refers to commit 3805bae724e7 but github doesn't recognize it. Is that meant to be 0ab4fa1 ?

@aditighag
Copy link
Member Author

aditighag commented Jan 20, 2022

LGTM. One minor nit, the last commit refers to commit 3805bae724e7 but github doesn't recognize it. Is that meant to be 0ab4fa1 ?

Yes, I've already fixed it in both PR and commit descriptions. Was waiting to push the latest commits until the ongoing CI run is complete. Check patch is failing for the same reason.

@aditighag
Copy link
Member Author

aditighag commented Jan 21, 2022

net-next hit #18520.

All other tests passed.

@aditighag
Copy link
Member Author

Only updated the commit message, no need to run the full CI again.

@aditighag
Copy link
Member Author

aditighag commented Jan 25, 2022

4.19 hit #18566

All other tests passed.

Screen Shot 2022-01-25 at 8 21 11 PM

Signed-off-by: Aditi Ghag <aditi@cilium.io>
The configuration option is moved from extra args -
https://artifacthub.io/packages/helm/uswitch/kiam/5.9.0.

Signed-off-by: Aditi Ghag <aditi@cilium.io>
@aditighag aditighag requested a review from aanm January 25, 2022 21:45
pkg/k8s/watchers/pod.go Outdated Show resolved Hide resolved
Commits da35c88 and 0ab4fa1 introduced a regression for
local-redirect use cases like kiam, whereby host networked pod
updates were skipped. As a result, node-local redirection for
cases where LRPs select host networked pods as backends broke.

Tested the fix on an EKS configured with kiam setup.

Fixes: da35c88 ("k8s/watchers: don't silently ignore (*K8sWatcher).updatePodHostData error")
Fixes: 0ab4fa1 (pkg/k8s: ignore certain ipcache errors)
Fixes: cilium#16920

Signed-off-by: Aditi Ghag <aditi@cilium.io>
Test kiam like use cases (LRP select host networked pods) with an address
matcher LRP -
https://docs.cilium.io/en/latest/gettingstarted/local-redirect-policy/#addressmatcher.

Signed-off-by: Aditi Ghag <aditi@cilium.io>
Signed-off-by: Aditi Ghag <aditi@cilium.io>
@aditighag aditighag force-pushed the pr/aditighag/lrp-watcher-regression-fix branch from 568cd70 to 6769ab0 Compare January 26, 2022 04:22
@aditighag aditighag requested a review from aanm January 26, 2022 04:22
@aditighag
Copy link
Member Author

@aanm Are you planning to review this PR further? Other reviews are in, and your review comments have been addressed.

@aanm aanm merged commit 9020fda into cilium:master Jan 31, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.10.8 Feb 7, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.11.2 Feb 7, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.10 in 1.10.8 Feb 8, 2022
@joamaki joamaki added backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. and removed backport-pending/1.11 labels Feb 8, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.11 in 1.11.2 Feb 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. kind/regression This functionality worked fine before, but was broken in a newer release of Cilium. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
No open projects
1.10.8
Backport done to v1.10
1.11.2
Backport done to v1.11
Development

Successfully merging this pull request may close these issues.

None yet

5 participants