feat: only remove finalizer in case no child accounts exist #48
Conversation
|
@coderabbitai review |
WalkthroughUpdates Go versions in Dockerfile and go.mod. Refactors FGASubroutine to shift work from Process to a new Finalize method with requeue logic based on child accounts and limiter management. Enhances AccountInfo Finalize with deletion-timestamp-aware requeue handling and cross-cluster account listing. Adds unit tests for FGASubroutine finalizers and finalize behavior. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/subroutines/accountinfo.go (2)
52-59: Magic number: extract the 1-minute threshold as a constant.The hardcoded 1-minute duration at line 55 should be extracted as a named constant for better maintainability and documentation of this business rule.
Apply this diff to extract the constant:
const ( AccountInfoSubroutineName = "AccountInfoSubroutine" DefaultAccountInfoName = "account" + FinalizerGracePeriod = 1 * time.Minute )Then update the usage:
func (r *AccountInfoSubroutine) Finalize(ctx context.Context, ro runtimeobject.RuntimeObject) (ctrl.Result, errors.OperatorError) { cn := MustGetClusteredName(ctx, ro) // Determine whether we should keep requeuing or surface an error based on deletion age. requeue := true if ts := ro.GetDeletionTimestamp(); ts != nil { - oneMinAgo := v1.Now().Add(-1 * time.Minute) + oneMinAgo := v1.Now().Add(-FinalizerGracePeriod) if ts.Time.Before(oneMinAgo) { requeue = false } }
63-68: Duplicate requeue logic: extract to a helper function.The requeue-or-error decision at lines 63-68 and 75-79 is identical. Extract this pattern into a helper function to improve maintainability.
Add a helper method:
func (r *AccountInfoSubroutine) requeueOrError(cn ClusteredName, requeue bool, reason string) (ctrl.Result, errors.OperatorError) { if requeue { delay := r.limiter.When(cn) return ctrl.Result{RequeueAfter: delay}, nil } return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("finalizer not removed yet: %s", reason), true, false) }Then refactor the Finalize method:
// The account info object is relevant input for other finalizers, removing the accountinfo finalizer at last if len(ro.GetFinalizers()) > 1 { - if requeue { - delay := r.limiter.When(cn) - return ctrl.Result{RequeueAfter: delay}, nil - } - return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("finalizer not removed yet"), true, false) + return r.requeueOrError(cn, requeue, "other finalizers still present") } accountList := &v1alpha1.AccountList{} if err := r.client.List(ctx, accountList); err != nil { return ctrl.Result{}, errors.NewOperatorError(err, true, true) } if len(accountList.Items) > 0 { - if requeue { - delay := r.limiter.When(cn) - return ctrl.Result{RequeueAfter: delay}, nil - } - return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("finalizer not removed yet"), true, false) + return r.requeueOrError(cn, requeue, "child accounts still exist") }Also applies to: 75-79
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitignore(1 hunks)Dockerfile(1 hunks)go.mod(1 hunks)pkg/subroutines/accountinfo.go(1 hunks)pkg/subroutines/accountinfo_test.go(1 hunks)pkg/subroutines/fga_test.go(0 hunks)
💤 Files with no reviewable changes (1)
- pkg/subroutines/fga_test.go
🔇 Additional comments (1)
pkg/subroutines/accountinfo.go (1)
70-80: Critical: List operation lacks proper scoping and may fetch accounts from all clusters.Line 71 calls
r.client.List(ctx, accountList)without any list options to filter by workspace, namespace, or parent account. This will likely list ALL accounts across ALL clusters/workspaces, not just the child accounts of the resource being finalized.Based on the PR objectives ("only remove finalizer in case no child accounts exist"), the intent is to check for child accounts of the current AccountInfo's workspace, not all accounts system-wide.
Run the following script to verify how List is used elsewhere in the codebase and check if there's a pattern for scoping lists to specific workspaces:
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitignore(1 hunks)Dockerfile(1 hunks)go.mod(1 hunks)pkg/subroutines/accountinfo.go(1 hunks)pkg/subroutines/accountinfo_test.go(1 hunks)pkg/subroutines/fga_test.go(0 hunks)
💤 Files with no reviewable changes (1)
- pkg/subroutines/fga_test.go
🔇 Additional comments (8)
go.mod (1)
3-3: Version bump looks good.Updating the module’s Go toolchain to 1.24.8 keeps it aligned with the Docker builder image. No issues spotted.
Dockerfile (1)
2-2: Double-check the Debian base change.Switching from
golang:1.24.x-bullseyeto the plaingolang:1.24.8tag also moves the underlying distro from Bullseye to the default (currently Bookworm). Please confirm we’re not depending on Bullseye-specific packages or behavior before merging..gitignore (1)
35-36: Nice addition.Ignoring
.gocachekeeps local Go build artifacts out of the repo. Looks good.pkg/subroutines/accountinfo.go (3)
52-59: LGTM! Deletion-age aware requeue logic is correctly implemented.The requeue flag logic properly checks if the deletion timestamp is older than one minute and disables requeue accordingly. This aligns with the PR objective of using exponential backoff for up to 1 minute before returning a non-Sentry error.
62-68: LGTM! Multiple finalizers handling is correctly implemented.The logic properly applies the requeue-or-error behavior based on the deletion age: requeues with exponential backoff before 1 minute, then returns a non-Sentry retryable error after 1 minute. This matches the PR objectives.
82-84: LGTM! Early exit path is correctly implemented.The finalizer removal logic properly cleans up the rate limiter state and returns with no requeue when all conditions are met (assuming the child accounts check is fixed).
pkg/subroutines/accountinfo_test.go (2)
488-514: LGTM! Test correctly verifies requeue behavior when child accounts exist.The test properly validates that Finalize returns no error and requeues with exponential backoff when child accounts exist and deletion is recent (less than 1 minute old).
516-545: LGTM! Test correctly verifies error behavior after the 1-minute threshold.The test properly validates that Finalize returns a retryable, non-Sentry error with zero requeue when deletion is older than 1 minute and child accounts still exist. The assertions match the expected behavior per the PR objectives.
… selector assertions in tests
…nfo-only-after-accounts
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
go.mod(1 hunks)pkg/subroutines/accountinfo/accountinfo.go(1 hunks)pkg/subroutines/fga.go(4 hunks)pkg/subroutines/fga_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/subroutines/fga.go (2)
pkg/clusteredname/clusteredname.go (2)
ClusteredName(12-15)MustGetClusteredName(31-36)api/v1alpha1/account_types.go (3)
Account(81-87)AccountList(92-96)NamespaceAccountOwnerLabel(29-29)
pkg/subroutines/fga_test.go (8)
pkg/subroutines/fga.go (1)
NewFGASubroutine(35-45)api/v1alpha1/account_types.go (4)
Account(81-87)AccountList(92-96)AccountSpec(34-53)AccountTypeAccount(28-28)pkg/subroutines/mocks/mock_MCManager.go (1)
NewManager(25-35)pkg/subroutines/mocks/mock_K8Cluster.go (1)
NewCluster(22-32)pkg/subroutines/mocks/mock_K8Service.go (1)
NewClient(505-515)pkg/subroutines/mocks/mock_OpenFGAServiceClient.go (1)
NewOpenFGAServiceClient(17-27)pkg/subroutines/accountinfo/accountinfo.go (1)
DefaultAccountInfoName(31-31)api/v1alpha1/account_info_types.go (5)
AccountInfo(65-71)AccountInfoSpec(24-30)AccountLocation(36-46)FGAInfo(48-50)StoreInfo(52-54)
pkg/subroutines/accountinfo/accountinfo.go (1)
api/v1alpha1/account_types.go (2)
AccountList(92-96)NamespaceAccountOwnerLabel(29-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: pipe / dockerBuild / docker
- GitHub Check: pipe / testSource / test
- GitHub Check: pipe / lint / lint
🔇 Additional comments (10)
pkg/subroutines/accountinfo/accountinfo.go (3)
57-63: LGTM! Deletion timestamp logic is correct.The requeue flag correctly disables requeue when the deletion timestamp is older than one minute, implementing the backoff behavior described in the PR objectives.
65-70: LGTM! Multiple finalizers handling is correct.The code properly implements the backoff behavior: requeue with exponential backoff when allowed, then return a non-Sentry retryable error after one minute. The error parameters (retryable=true, sentry=false) are appropriate for this expected blocking scenario.
72-91: LGTM! Child account checking logic is correct.The code properly implements the requirement to remove the finalizer only when no child accounts exist. The logic correctly:
- Retrieves the cluster client using the clustered name
- Lists child accounts filtered by the owner label
- Requeues if child accounts exist (with the same backoff behavior)
- Proceeds with finalization and clears the limiter when no child accounts exist
The flow ensures this check only runs when this is the last finalizer, which is the correct behavior.
pkg/subroutines/fga.go (4)
47-51: LGTM! Process method correctly converted to no-op.The architectural shift to handle FGA setup via workspace initializer (with cleanup moved to Finalize) is clear and well-documented. The debug message provides useful context for developers.
102-133: LGTM! Creator relation deletion and FGA write loop are correct.The code properly:
- Constructs delete tuples for both assignee-to-role and role-to-account relations
- Uses
formatUserto handle service account naming conventions- Writes deletes individually with appropriate error handling
- Gracefully handles duplicate write errors (expected for idempotent deletes)
The error handling strategy is sound for cleanup operations.
136-136: LGTM! Proper limiter cleanup.Forgetting the limiter state after successful finalization prevents stale rate-limit data from affecting future reconciliations.
36-36: Align maximum backoff delay with PR objectives.All instances of
NewTypedItemExponentialFailureRateLimiterin pkg/subroutines/{fga.go:36, workspace/workspace.go:41, accountinfo/accountinfo.go:42} are set to a 120 s max delay, but the PR calls for up to 1 minute. Adjust to 60 s or confirm that 120 s is intentional.pkg/subroutines/fga_test.go (3)
26-29: LGTM! Finalizer test is clear and correct.The test properly verifies that the
Finalizersmethod returns the expected FGA finalizer.
31-36: LGTM! Process no-op test is correct.The test validates that
Processreturns an empty result with no error, confirming the expected no-op behavior after the refactoring.
38-68: LGTM! Child account requeue test is comprehensive.The test properly verifies the blocking behavior when child accounts exist:
- Mocks are set up correctly with appropriate expectations
- Context includes cluster information required by the code
- Verification confirms requeue is scheduled with a positive delay
Good test coverage for the PR's key requirement.
| ) | ||
|
|
||
| fgaMock := mocks.NewOpenFGAServiceClient(t) | ||
| fgaMock.EXPECT().Write(mock.Anything, mock.Anything).Return(&openfgav1.WriteResponse{}, nil).Times(2) |
There was a problem hiding this comment.
Fix the expected Write call count.
The test expects Write to be called twice (Times(2)), but the code in fga.go constructs three delete tuples when a creator is present:
- Parent relation tuple (lines 96-100)
- Assignee-to-role tuple (lines 104-108)
- Role-to-account tuple (lines 110-114)
Each tuple is written individually in the loop (lines 117-133), resulting in three Write calls.
Apply this diff to fix the test:
- fgaMock.EXPECT().Write(mock.Anything, mock.Anything).Return(&openfgav1.WriteResponse{}, nil).Times(2)
+ fgaMock.EXPECT().Write(mock.Anything, mock.Anything).Return(&openfgav1.WriteResponse{}, nil).Times(3)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fgaMock.EXPECT().Write(mock.Anything, mock.Anything).Return(&openfgav1.WriteResponse{}, nil).Times(2) | |
| fgaMock.EXPECT().Write(mock.Anything, mock.Anything).Return(&openfgav1.WriteResponse{}, nil).Times(3) |
🤖 Prompt for AI Agents
In pkg/subroutines/fga_test.go around line 104 the mock expectation currently
uses Times(2) but fga.go creates and writes three delete tuples when a creator
is present, so update the expectation to Times(3) to match the three individual
Write calls; change the call from Times(2) to Times(3) so the test reflects the
actual number of Write invocations.
| deletes := []*openfgav1.TupleKeyWithoutCondition{} | ||
| if account.Spec.Type != v1alpha1.AccountTypeOrg { | ||
| parentAccountName := accountInfo.Spec.Account.Name | ||
|
|
||
| deletes = append(deletes, &openfgav1.TupleKeyWithoutCondition{ | ||
| User: fmt.Sprintf("%s:%s/%s", e.objectType, accountInfo.Spec.ParentAccount.OriginClusterId, parentAccountName), | ||
| Relation: e.parentRelation, | ||
| Object: fmt.Sprintf("%s:%s/%s", e.objectType, accountInfo.Spec.Account.OriginClusterId, account.GetName()), | ||
| }) | ||
| } | ||
| parentAccountName := accountInfo.Spec.Account.Name | ||
| deletes = append(deletes, &openfgav1.TupleKeyWithoutCondition{ | ||
| User: fmt.Sprintf("%s:%s/%s", e.objectType, accountInfo.Spec.ParentAccount.OriginClusterId, parentAccountName), | ||
| Relation: e.parentRelation, | ||
| Object: fmt.Sprintf("%s:%s/%s", e.objectType, accountInfo.Spec.Account.OriginClusterId, account.GetName()), | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify parent account name in delete tuple.
Line 95 assigns accountInfo.Spec.Account.Name to parentAccountName, but line 97 uses it as the parent in the User field. This appears incorrect—the parent's name should come from accountInfo.Spec.ParentAccount.Name instead.
Apply this diff if the parent account name should be sourced from ParentAccount:
- parentAccountName := accountInfo.Spec.Account.Name
+ parentAccountName := accountInfo.Spec.ParentAccount.Name
deletes = append(deletes, &openfgav1.TupleKeyWithoutCondition{
User: fmt.Sprintf("%s:%s/%s", e.objectType, accountInfo.Spec.ParentAccount.OriginClusterId, parentAccountName),
Relation: e.parentRelation,Verify the intended tuple structure by checking the corresponding write logic that created this tuple, or review the FGA model definition.
🏁 Script executed:
#!/bin/bash
sed -n '80,140p' pkg/subroutines/fga.goLength of output: 2356
🏁 Script executed:
#!/bin/bash
rg -n 'type AccountInfo' -n .Length of output: 361
🏁 Script executed:
#!/bin/bash
rg -n 'ParentAccount' -C3 api/v1alpha1/account_info_types.goLength of output: 318
🏁 Script executed:
#!/bin/bash
rg -n 'type AccountLocation' -C3 api/v1alpha1/account_info_types.goLength of output: 267
Use the parent’s Name when building the delete tuple
parentAccountName is incorrectly sourced from Spec.Account.Name; it must use Spec.ParentAccount.Name so the User field truly references the parent.
- parentAccountName := accountInfo.Spec.Account.Name
+ parentAccountName := accountInfo.Spec.ParentAccount.Name🤖 Prompt for AI Agents
In pkg/subroutines/fga.go around lines 94 to 100, the code builds a delete tuple
for the parent User but sources parentAccountName from Spec.Account.Name; change
it to use accountInfo.Spec.ParentAccount.Name (or assign parentAccountName :=
accountInfo.Spec.ParentAccount.Name) so the User field correctly references the
parent account, then use that variable when formatting the User string.
#24
Finalizer: Remove AccountInfo finalizer only when no other finalizers remain and no child Account CRs exist.
Backoff: Use existing rate limiter to requeue with exponential backoff for up to 1 minute; after that, return a non-sentry retryable error (“finalizer not removed yet”).
Tests: Added
Summary by CodeRabbit