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

feat: add finalizer for UDS Package CRs #953

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

feat: add finalizer for UDS Package CRs #953

wants to merge 13 commits into from

Conversation

mjnagel
Copy link
Contributor

@mjnagel mjnagel commented Oct 23, 2024

Description

This PR moves our onDelete code into a finalizer, using Pepr's Finalize functionality. Due to an open Pepr feature request this does not update the Package CR status during deletion to avoid triggering the finalization multiple times. In order to provide visibility into deletion status and failure this adds eventing around the specific cleanup operations as well as specific failure events in catch blocks.

This PR does also add a new status to the Package CR of Removing, which was stubbed out for a future state where we may be able to use it (pending addition of the Pepr feature to not delete the finalizer).

3 additional related QoL changes:

  • await removal of SSO tokens from the store
  • modify log message around istio injection to be more accurate based on state
  • ensure addlicense is run on generated CRD files post-generate

Related Issue

Fixes #523

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Other (security config, docs update, etc)

Checklist before merging

@mjnagel mjnagel self-assigned this Oct 23, 2024
@mjnagel mjnagel requested a review from a team as a code owner October 23, 2024 17:14
@rjferguson21
Copy link
Contributor

Going to try to figure out exactly why but FYI it seems like if I Create and then Delete many (5 or 6) packages at once the authservice secret does not get cleaned up properly. I see lingering chains for Packages that no longer exist.

I checked back on main to make sure it was not a regression introduced by the authservice batch update PR and it seemed to work as expected there.

@mjnagel mjnagel marked this pull request as draft October 24, 2024 20:35
@mjnagel
Copy link
Contributor Author

mjnagel commented Oct 24, 2024

I was able to reproduce the same issue identified ^, digging into it.

@rjferguson21 rjferguson21 marked this pull request as ready for review October 25, 2024 13:02
@rjferguson21
Copy link
Contributor

Discussed offline with Micah but wanted to follow up here. Part of this change included moving from Store.removeItem to Store.removeItemAndWait when cleaning up SSO clients.

This was causing resource contention for the in memory copy of the authservice configuration in circumstances where many packages are deleted simultaneously due to the fact that the underlying Pepr store implementation would resolve the removeItemAndWait calls for every package at the same time.

I introduced a basic resource lock to avoid the contention on the in the memory update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Leverage finalizer for Package deletions
2 participants