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

Checks k8s metadata for pod before removing IP from ipcache #17161

Merged
merged 1 commit into from Aug 18, 2021

Conversation

Weil0ng
Copy link
Contributor

@Weil0ng Weil0ng commented Aug 14, 2021

This is to prevent removing ip cache when a Pod's IP is reused. We will
only remove ip cache entry if the k8s metadata associated with the IP is
"current", meaning tha the last time we update this entry, it is from
the same pod or CEP.

Fixes: #16565

@Weil0ng Weil0ng requested a review from a team August 14, 2021 00:17
@maintainer-s-little-helper
Copy link

Commit fa90d82a6ad4e14c17f8c03b8492ef27bcd215a0 does not contain "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 dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Aug 14, 2021
@maintainer-s-little-helper
Copy link

Commit fa90d82a6ad4e14c17f8c03b8492ef27bcd215a0 does not contain "Signed-off-by".

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

@Weil0ng
Copy link
Contributor Author

Weil0ng commented Aug 14, 2021

Odd, commit has "Signed-off-by"...not sure why bot thinks otherwise...

@Weil0ng Weil0ng requested a review from aanm August 14, 2021 00:20
@Weil0ng Weil0ng added release-note/misc This PR makes changes that have no direct user impact. and removed dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Aug 14, 2021
@aanm
Copy link
Member

aanm commented Aug 14, 2021

Odd, commit has "Signed-off-by"...not sure why bot thinks otherwise...

The commit fa90d82 didn't have the Sign-off 😅

@Weil0ng
Copy link
Contributor Author

Weil0ng commented Aug 14, 2021

The commit fa90d82 didn't have the Sign-off 😅

huh, still confused, yes when I created this PR, I forgot to add a signoff, so bot freaked out first time (and it should :) ), but then I amended the commmit with signoff and pushed, now if I click on "Commits" I can see the signoff in commit, but somehow bot is still unhappy...am I missing something?

Also curious, how did you find the previous commit? I somehow couldn't find it...

Copy link
Contributor

@errordeveloper errordeveloper left a comment

Choose a reason for hiding this comment

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

Looks like there is a typo, as reported by the CI... Overall the change LGTM.

@aanm
Copy link
Member

aanm commented Aug 16, 2021

@Weil0ng if you click here or in the commit the bot had left:

image

BTW the CI failures are related with the changes made in this PR.

This is to prevent removing ip cache when a Pod's IP is reused. We will
only remove ip cache entry if the k8s metadata associated with the IP is
"current", meaning tha the last time we update this entry, it is from
the same pod or CEP.

Signed-off-by: Weilong Cui <cuiwl@google.com>
@Weil0ng
Copy link
Contributor Author

Weil0ng commented Aug 16, 2021

@Weil0ng if you click here or in the commit the bot had left:

Thanks! I guess I was confused why bot keeps complaining an "old" commit :)

Fixed typo in code, PTAL.

@aanm
Copy link
Member

aanm commented Aug 16, 2021

test-me-please

@rolinh rolinh changed the title Checks k8s metadata for pod before removing IP from ipccahe Checks k8s metadata for pod before removing IP from ipcache Aug 17, 2021
@Weil0ng
Copy link
Contributor Author

Weil0ng commented Aug 17, 2021

ci-awscni

@Weil0ng
Copy link
Contributor Author

Weil0ng commented Aug 17, 2021

ci-aks

@Weil0ng
Copy link
Contributor Author

Weil0ng commented Aug 17, 2021

ci-eks

@Weil0ng
Copy link
Contributor Author

Weil0ng commented Aug 17, 2021

ci-gke

@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 Aug 17, 2021
@ti-mo ti-mo merged commit 63c0b29 into cilium:master Aug 18, 2021
Comment on lines +844 to +846
k8sMeta := ipcache.IPIdentityCache.GetK8sMetadata(podIP)
if k8sMeta.Namespace == pod.Namespace && k8sMeta.PodName == pod.Name {
ipcache.IPIdentityCache.Delete(podIP, source.Kubernetes)
Copy link
Member

Choose a reason for hiding this comment

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

This should occur within a single critical section, otherwise other parts of the system could delete, modify or update the IPCache in between the lookup and the delete here.

Copy link
Contributor Author

@Weil0ng Weil0ng Nov 16, 2021

Choose a reason for hiding this comment

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

Sent a fix in #17909, ptal.

Weil0ng added a commit to Weil0ng/cilium that referenced this pull request Nov 16, 2021
Fixes potential racing condition introduced in PR cilium#17161.

Suggested-by: Joe Stringer <joe@cilium.io>
Signed-off-by: Weilong Cui <cuiwl@google.com>
aanm pushed a commit that referenced this pull request Nov 26, 2021
Fixes potential racing condition introduced in PR #17161.

Suggested-by: Joe Stringer <joe@cilium.io>
Signed-off-by: Weilong Cui <cuiwl@google.com>
qmonnet pushed a commit to qmonnet/cilium that referenced this pull request Dec 1, 2021
[ upstream commit 3650544 ]

Fixes potential racing condition introduced in PR cilium#17161.

Suggested-by: Joe Stringer <joe@cilium.io>
Signed-off-by: Weilong Cui <cuiwl@google.com>
Signed-off-by: Quentin Monnet <quentin@isovalent.com>
joestringer pushed a commit that referenced this pull request Dec 2, 2021
[ upstream commit 3650544 ]

Fixes potential racing condition introduced in PR #17161.

Suggested-by: Joe Stringer <joe@cilium.io>
Signed-off-by: Weilong Cui <cuiwl@google.com>
Signed-off-by: Quentin Monnet <quentin@isovalent.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reusing IP in the event of CEP propagation delay might result in inconsistent ipcache
5 participants