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

Auth Map: Initial Garbage Collection #25754

Merged
merged 3 commits into from Jun 6, 2023
Merged

Auth Map: Initial Garbage Collection #25754

merged 3 commits into from Jun 6, 2023

Conversation

mhofstetter
Copy link
Member

@mhofstetter mhofstetter commented May 30, 2023 •
edited

This PR introduces garbage collection for the auth map based on the following events:

  • deleted cilium node: all auth map entries which belong to the deleted node will be deleted
  • deleted cilium identity: all auth map entries which belong to the deleted identity will be deleted
  • timer (configurable - defaults to every 15m): expired auth map entries will be deleted

The implementation uses the hive job framework.

Known Limitations:

  • Garbage Collection based on deleted Cilium Identities is currently only supported if Cilium Identities are backed by the CRD storage. Eventually the expired entries will be cleaned up by the periodic timer based run.
  • Currently no GC based on deleted policies

Fixes: #25213

@mhofstetter mhofstetter added release-note/misc This PR makes changes that have no direct user impact. area/servicemesh GH issues or PRs regarding servicemesh labels May 30, 2023
@mhofstetter mhofstetter marked this pull request as ready for review May 30, 2023 09:44
@mhofstetter mhofstetter requested review from a team as code owners May 30, 2023 09:44
@mhofstetter mhofstetter mentioned this pull request May 30, 2023
Closed
@mhofstetter mhofstetter added the kind/feature This introduces new functionality. label May 30, 2023
jrajahalme
jrajahalme reviewed May 30, 2023
View reviewed changes
Copy link
Member

@jrajahalme jrajahalme left a comment

Choose a reason for hiding this comment

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

Looking good :-)

You may need to tap into func (d *Daemon) UpdateIdentities(added, deleted cache.IdentityCache) to get identity deletes regardless of identity allocation mode.

jrajahalme
jrajahalme reviewed May 30, 2023
View reviewed changes
pkg/auth/authmap_gc.go Outdated Show resolved Hide resolved
jrajahalme
jrajahalme reviewed May 30, 2023
View reviewed changes
pkg/auth/authmap_gc.go Outdated Show resolved Hide resolved
@mhofstetter
Copy link
Member Author

rebased to main and resolved conflicts

@mhofstetter
Copy link
Member Author

You may need to tap into func (d *Daemon) UpdateIdentities(added, deleted cache.IdentityCache) to get identity deletes regardless of identity allocation mode.

@jrajahalme thanks for your review! as mentioned offline - i think it's ok to start by supporting CRD backed only because the expired entries will be deleted eventually by the timer based GC anyway. an abstraction which should remove this need to handle both cases on the usage side is in discussion anyway AFAIK

@mhofstetter
Copy link
Member Author

Added additional GC unit tests (delete entries where identity & nodes no longer exist at startup (via event type upsert & sync))

@mhofstetter mhofstetter changed the title Auth Map: Garbage Collection Auth Map: Initial Garbage Collection Jun 1, 2023
viktor-kurchenko
viktor-kurchenko approved these changes Jun 1, 2023
View reviewed changes
Copy link
Contributor

@viktor-kurchenko viktor-kurchenko left a comment

Choose a reason for hiding this comment

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

LGTM.

jrajahalme
jrajahalme approved these changes Jun 1, 2023
View reviewed changes
Copy link
Member

@jrajahalme jrajahalme left a comment

Choose a reason for hiding this comment

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

authMapCache.All() is still returning the internal map to be used with the internal lock released. This function does not seem to be used however, so maybe split the interface requiring All() to be implemented into two, one with it and one without? Or make it to return a copy of the map instead?

@mhofstetter
Copy link
Member Author

authMapCache.All() is still returning the internal map to be used with the internal lock released. This function does not seem to be used however, so maybe split the interface requiring All() to be implemented into two, one with it and one without? Or make it to return a copy of the map instead?

@jrajahalme 🙈 fixed by returning a copy of the map in All() - thanks! This method will be used in upcoming Re-Authentication - therefore keeping it exposed!

@youngnick youngnick added the release-blocker/1.14 This issue will prevent the release of the next version of Cilium. label Jun 1, 2023
dylandreimerink
dylandreimerink requested changes Jun 2, 2023
View reviewed changes
Copy link
Member

@dylandreimerink dylandreimerink left a comment

Choose a reason for hiding this comment

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

Nice work! Just the one thing

pkg/auth/authmap_gc.go Show resolved Hide resolved
joamaki
joamaki reviewed Jun 2, 2023
View reviewed changes
pkg/auth/authmap_gc.go Outdated Show resolved Hide resolved
joamaki
joamaki reviewed Jun 2, 2023
View reviewed changes
pkg/auth/authmap_gc.go Outdated Show resolved Hide resolved
@mhofstetter
Copy link
Member Author

rebased to main to resolve conflicts

@mhofstetter mhofstetter removed the request for review from rgo3 June 5, 2023 10:46
@mhofstetter
Copy link
Member Author

/test

joamaki
joamaki requested changes Jun 5, 2023
View reviewed changes
pkg/auth/authmap_cache.go Outdated Show resolved Hide resolved
pkg/auth/authmap_gc.go Outdated Show resolved Hide resolved
pkg/auth/authmap_writer.go Outdated Show resolved Hide resolved
pkg/auth/cell.go Show resolved Hide resolved
@mhofstetter mhofstetter requested a review from joamaki June 5, 2023 12:30
@mhofstetter
Copy link
Member Author

/test

@mhofstetter
Copy link
Member Author

ci-l4lb more or less constantly fails due to #25892

joamaki
joamaki approved these changes Jun 5, 2023
View reviewed changes
pkg/auth/authmap_cache.go Show resolved Hide resolved
pkg/auth/authmap_gc.go Outdated Show resolved Hide resolved
pkg/auth/cell.go Show resolved Hide resolved
pkg/auth/manager_test.go Outdated Show resolved Hide resolved
@mhofstetter
Copy link
Member Author

fixed optimizations recommended by @joamaki - thanks!

@mhofstetter
auth: provide (all) CiliumNodes and identity privately
bc5a20d 
Currently, only the local CiliumNode is available in the shared
resources.

This commit introduces the possibility to watch all CiliumNodes
and CiliumIdentities within the auth cell by providing them privately
for the cell only.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter
auth: implementation of auth map garbage collection
b93e435 
This commit introduces garbage collection for the auth map based on the
following events:

* deleted node: all auth map entries which belong to the deleted node
  will be deleted
* deleted identity: all auth map entries which belong to the deleted
  identity will be deleted
* timer (configurable - defaults to every 15m): expired auth map entries
  will be deleted

Garbage Collection based on deleted Cilium Identities is currently only
supported if Cilium Identities are backed by the CRD storage.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter
auth: authmapCache.All() returns copy of map
a9aab40 
This commit lets authmapCache.All() returning a copy of the map of
cached entries instead of the map instead. This is necessary to prevent
issues when using the map without acquiring the lock.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter
Copy link
Member Author

mhofstetter commented Jun 5, 2023 •
edited

rebased to main - to fetch potential ci-l4lb fix 🙏

(edit: It seems that #25826 resolved the issue, the test is passing on my PRs after rebasing)

@mhofstetter
Copy link
Member Author

/test

@mhofstetter
Copy link
Member Author

mhofstetter commented Jun 6, 2023 •
edited

pending check installation-and-connectivity (from a deleted workflow) has been removed from the required checks by @aanm .

-> marking this as ready-to-merge

@mhofstetter mhofstetter added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 6, 2023
@dylandreimerink dylandreimerink merged commit a9faaf1 into cilium:main Jun 6, 2023
61 checks passed
@mhofstetter mhofstetter deleted the pr/mhofstetter/authmap-gc branch June 6, 2023 07:53
@aanm aanm mentioned this pull request Jun 8, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joamaki joamaki joamaki approved these changes

@dylandreimerink dylandreimerink dylandreimerink approved these changes

@jrajahalme jrajahalme jrajahalme approved these changes

@youngnick youngnick youngnick approved these changes

@viktor-kurchenko viktor-kurchenko viktor-kurchenko approved these changes

Assignees
No one assigned
Labels
area/servicemesh GH issues or PRs regarding servicemesh kind/feature This introduces new functionality. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-blocker/1.14 This issue will prevent the release of the next version of Cilium. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Garbage Collection for auth map
6 participants
@mhofstetter @joamaki @dylandreimerink @jrajahalme @youngnick @viktor-kurchenko