auth: Switch to observing identity changes #26375
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mhofstetter
added
kind/bug
This was referenced Jun 20, 2023
mhofstetter
force-pushed
the
pr/mhofstetter/auth-observable-id-allocator
branch
5 times, most recently
from
20766fc
to
2bdb5ab
Compare
joestringer
added
release-note/bug
mhofstetter
force-pushed
the
pr/mhofstetter/auth-observable-id-allocator
branch
5 times, most recently
from
bbfc60c
to
eb5eb6f
Compare
|
dependent PR #26373 (making the identity allocator observable) has been merged -> will remove |
mhofstetter
removed
the
dont-merge/blocked
|
/test |
Observe the identity changes via the CachingIdentityAllocator instead of using CiliumIdentity CRD directly. This both fixes the issue of having two informers (and thus double the bandwidth), but it also allows auth to work with the kvstore identity allocation backend. Co-authored-by: Marco Hofstetter <marco.hofstetter@isovalent.com> Signed-off-by: Jussi Maki <jussi@isovalent.com>
This commit renames the helm value `authentication.expiredGCInterval` to `authentication.gcInterval` as it will be used for multiple types of auth related GC's. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
Latest changes reflect deleted identities and nodes only in the internal state of the garbage collector without deleting the related entries immediately. Therefore, this commit changes the auth map gc interval from `15m` to `5m` which reflects the changes faster in the map itself. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
Currently, auth map entries related to a deleted cilium identity are immediately deleted when the event has been received. The actual deletion might result in errors, which no longer can be reported back to the IdentityAllocator which emits the events. To prevent events result in errors, the events should no longer delete auth map entries. Therefore, this commit refactors that the deletion information is stored within the garbage collector, and the actual garbage collection run uses these information to cleanup the map. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
This commit reorders the functions within the garbage collector Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
Currently, auth map entries related to a deleted node are immediately deleted when the event has been received. To prevent events result in errors, the events should no longer delete auth map entries. Therefore, this commit refactors that the deletion information is stored within the garbage collector, and the actual garbage collection run uses these information to cleanup the map. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
This commit combines the different timer based auth map gc jobs into a single job. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
This commit separates the existing auth map gc tests into multiple tests per "type" * identities * nodes * policies * expiration Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
This commit cleans up the auth module. * improve comments * renamed newAuthManager -> registerAuthManager * grouped params in authManagerParams * rename gc job names * split registration into instantiation & job/lifecycle registration sections Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
sayboras
force-pushed
the
pr/mhofstetter/auth-observable-id-allocator
branch
from
eb5eb6f
to
734794b
Compare
|
/test |
|
/ci-multicluster |
maintainer-s-little-helper
bot
added
the
ready-to-merge
|
This PR was marked as release-blocker for 1.14, so nominate it for backport. Feel free to let us know if you think otherwise. Thanks. |
sayboras
added
the
needs-backport/1.14
joamaki
added
backport-pending/1.14
jibi
added
backport-done/1.14
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Observe the identity changes via the CachingIdentityAllocator instead of using CiliumIdentity CRD directly. This both fixes the issue of having two informers (and thus double the bandwidth), but it also allows auth to work with the kvstore identity allocation
backend.
In addition, the the auth map garbage collection has been refactored to use the events (deleted nodes, deleted
identities) only to update the internal state - whereas the actual GC is combined to a timer based job. This way we prevent events resulting in errors - that wouldn't be retried in case of the observable identity allocator (therefore this refactoring has become mandatory).
It's recommended to review the individual commits for more clarity & more context.
Depends on: #26373
Fixes: #25898