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.

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

pkg/redirectpolicy: Make code robust against incorrect policy configu… #16216

Merged
merged 1 commit into from
Aug 13, 2021
Merged

pkg/redirectpolicy: Make code robust against incorrect policy configu… #16216

merged 1 commit into from
Aug 13, 2021

Conversation

aditighag
Copy link
Member

@aditighag aditighag commented May 19, 2021 •
edited
Loading

See commit message.

Fixes: #16204

Release note

Fix a crash where user specifies incorrect service name in a local redirect policy config, or policy selected service is added after the policy is added.

@aditighag aditighag requested review from a team and christarazi May 19, 2021 07:41
@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 May 19, 2021
@aditighag aditighag requested a review from joamaki May 19, 2021 07:41
@aditighag aditighag marked this pull request as draft May 19, 2021 07:41
@aditighag aditighag added the release-note/bug This PR fixes an issue in a previous release of Cilium. label May 19, 2021
@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 May 19, 2021
@joamaki
Copy link
Contributor

joamaki commented Jun 14, 2021

@aditighag hey, what's the status of this PR? I've been waiting for it to be marked ready for review before having a look.

@aditighag
Copy link
Member Author

test-only --focus="K8sServicesTest.* LRP" --kernel_version="net-next"

@stale
Copy link

stale bot commented Jul 20, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. and removed stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. labels Jul 20, 2021
@aditighag aditighag force-pushed the pr/aditighag/lrp-fix-incorrect-config branch from 4cbaf5f to 70d510a Compare July 20, 2021 23:55
@aditighag
Copy link
Member Author

aditighag commented Jul 20, 2021 •
edited
Loading

test-only --focus="K8sLRPTests" --kernel_version="net-next"

Rebased the PR.

@aditighag aditighag marked this pull request as ready for review July 20, 2021 23:57
@aditighag
Copy link
Member Author

Focused tests relevant to the code changes (LRP tests) have passed.

christarazi
christarazi requested changes Jul 21, 2021
View reviewed changes
pkg/k8s/service_cache.go Outdated Show resolved Hide resolved
pkg/redirectpolicy/manager.go Outdated Show resolved Hide resolved
pkg/redirectpolicy/manager.go Outdated Show resolved Hide resolved
@christarazi christarazi marked this pull request as draft July 29, 2021 16:47
@joamaki joamaki removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Aug 12, 2021
@aditighag
Copy link
Member Author

aditighag commented Aug 12, 2021 •
edited
Loading

Reviews are in and focused LRP tests have passed - https://jenkins.cilium.io/job/Cilium-PR-Tests-Kernel-Focus/293/.

Based on the out-of-band comment from Jussi, I've added a comment in the commit description and the code. No need to re-run the test.

Edit : checkpath warning can be ignored -

ERROR:CUSTOM: Please avoid long commit subjects (max: 75, found: 76)

@aditighag
pkg/redirectpolicy: Make code robust against incorrect policy configu…
ce0d7af 
…rations

If an incorrect service name is specified in an LRP, we need to guard against
this case when service IP can't be retrieved.
Also, when a valid LRP is applied before the service it selects, service information
won't be available in the manager callback to add an LRP. The LRP will be
applied when the service callback is later received by the manager.

Signed-off-by: Aditi Ghag <aditi@cilium.io>
@aditighag aditighag force-pushed the pr/aditighag/lrp-fix-incorrect-config branch from 43ee72c to ce0d7af Compare August 12, 2021 15:53
@aditighag aditighag added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Aug 12, 2021
@ti-mo ti-mo merged commit fbeb5c8 into cilium:master Aug 13, 2021
@pchaigno pchaigno mentioned this pull request Aug 17, 2021
Merged
@ti-mo ti-mo mentioned this pull request Aug 18, 2021
Merged
This was referenced Sep 1, 2021
Merged
Merged
@pchaigno pchaigno mentioned this pull request Nov 1, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christarazi christarazi christarazi approved these changes

@joamaki joamaki joamaki approved these changes

Assignees
No one assigned
Labels
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.

localRedirectPolicy: pointing to a non-existent service crashes the agent
7 participants
@aditighag @joamaki @christarazi @tklauser @joestringer @pchaigno @ti-mo