feat: move fga subroutine to security operator #52
Conversation
WalkthroughRemoves the OpenFGA integration (subroutine, mocks, config, gRPC wiring), updates AccountInfo handling to preserve existing FGA fields and requeue when AccountInfo is missing, adds creator fields to API types/schemas and deepcopy, updates operator wiring/tests, adjusts test manifests, and updates module dependencies. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
…ine-to-security-operator
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/subroutines/fga.go (1)
23-45: Consider documenting the asymmetric migration design.The
Processmethod is now a no-op (lines 41-45), butFGASubroutinestill exists with an activeFinalizemethod (lines 47-124) containing FGA cleanup logic. This creates an asymmetric design where:
- FGA resource creation is handled elsewhere (presumably the Security Operator or workspace initializer)
- FGA resource cleanup remains in the Account Operator
If this split responsibility is the intended final design, document it clearly (e.g., in code comments or ADRs) to help future maintainers understand why FGA processing and cleanup are handled separately. If this is a transitional state during migration, consider:
- Opening a follow-up issue to track moving
Finalizelogic to the Security Operator- Adding a TODO comment indicating the temporary nature of this design
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/subroutines/fga.go(1 hunks)
⏰ 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). (2)
- GitHub Check: pipe / dockerBuild / docker
- GitHub Check: pipe / testSource / test
🔇 Additional comments (1)
pkg/subroutines/fga.go (1)
41-45: Clarify workspace initializer implementation and no-opProcessdesign
- The log claims “handled by workspace initializer,” but no initializer code is present. Confirm or add the initializer that takes over FGA processing.
- Ensure it’s intentional for
Processto be stubbed whileFinalizecontinues cleanup.
aaronschweig
left a comment
aaronschweig
left a comment
There was a problem hiding this comment.
You can just remove the whole file as well as the corresponding tests. We do not need any remainders of FGA inside the account-operator
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/controller/account_controller.go (1)
38-63: Consider warning users when FGA configuration is present but unused.Since FGA functionality is being moved to the security-operator, users with
cfg.Subroutines.FGA.Enabled = truewill have their configuration silently ignored. Consider adding a warning log to aid migration:subs := []lifecyclesubroutine.Subroutine{} + if cfg.Subroutines.FGA.Enabled { + log.Warn().Msg("FGA subroutine configuration is present but FGA functionality has been moved to security-operator") + } + if cfg.Subroutines.WorkspaceType.Enabled {This helps operators understand why FGA-related functionality may not be working as expected during the migration period.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/operator.go(1 hunks)internal/controller/account_controller.go(1 hunks)internal/controller/account_controller_test.go(1 hunks)pkg/subroutines/fga.go(0 hunks)pkg/subroutines/fga_helpers_test.go(0 hunks)
💤 Files with no reviewable changes (2)
- pkg/subroutines/fga.go
- pkg/subroutines/fga_helpers_test.go
🧰 Additional context used
🧬 Code graph analysis (3)
internal/controller/account_controller.go (1)
internal/config/config.go (1)
OperatorConfig(4-34)
internal/controller/account_controller_test.go (1)
internal/controller/account_controller.go (1)
NewAccountReconciler(38-63)
cmd/operator.go (1)
internal/controller/account_controller.go (1)
NewAccountReconciler(38-63)
⏰ 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). (2)
- GitHub Check: pipe / dockerBuild / docker
- GitHub Check: pipe / testSource / test
🔇 Additional comments (3)
internal/controller/account_controller_test.go (1)
142-142: LGTM! Constructor call correctly updated.The removal of the FGA client parameter aligns with the broader removal of OpenFGA integration. The test configuration explicitly disables FGA (line 89), and the finalizer expectations (lines 198-201) correctly reflect only the three non-FGA subroutines.
cmd/operator.go (1)
131-131: LGTM! Operator wiring correctly updated.The constructor call correctly reflects the updated
NewAccountReconcilersignature without the FGA client parameter. This aligns with the removal of OpenFGA dependencies throughout the operator startup path.internal/controller/account_controller.go (1)
38-38: LGTM! Function signature correctly updated.The removal of the
fgaClientparameter is consistent across all call sites and aligns with the broader removal of OpenFGA integration from this operator.
aaronschweig
left a comment
aaronschweig
left a comment
There was a problem hiding this comment.
Now you removed a bit too much.
test/setup/01-platform-mesh-system/apiresourceschema-accountinfos.core.platform-mesh.io.yaml
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/subroutines/accountinfo/accountinfo.go (1)
146-157: Critical: Accessing removed FGA field causes compilation error.Line 151 attempts to access
parentAccountInfo.Spec.FGA.Store.Id, but according to the PR summary and AI-generated summary, FGA fields were removed from the AccountInfo schema. This will cause a compilation error.Additionally, there's an inconsistency: the Org account flow (lines 109-135) explicitly avoids touching FGA fields to preserve initializer-provided values, while this non-Org account flow tries to copy FGA.Store.Id from parent.
For non-Org accounts (Team/Project), you should follow the same pattern as Org accounts: use a patch-based approach that preserves FGA fields rather than attempting to copy them. Apply this diff:
- accountInfo := &v1alpha1.AccountInfo{ObjectMeta: v1.ObjectMeta{Name: DefaultAccountInfoName}} - _, err = controllerutil.CreateOrUpdate(ctx, accountClusterClient, accountInfo, func() error { + accountInfo := &v1alpha1.AccountInfo{} + if err := accountClusterClient.Get(ctx, client.ObjectKey{Name: DefaultAccountInfoName}, accountInfo); err != nil { + if kerrors.IsNotFound(err) { + log.Info().Msg("AccountInfo not yet created by initializer; requeue") + return ctrl.Result{RequeueAfter: r.limiter.When(cn)}, nil + } + return ctrl.Result{}, errors.NewOperatorError(err, true, true) + } + + _, err = controllerutil.CreateOrPatch(ctx, accountClusterClient, accountInfo, func() error { accountInfo.Spec.Account = selfAccountLocation accountInfo.Spec.ParentAccount = &parentAccountInfo.Spec.Account accountInfo.Spec.Organization = parentAccountInfo.Spec.Organization - accountInfo.Spec.FGA.Store.Id = parentAccountInfo.Spec.FGA.Store.Id accountInfo.Spec.ClusterInfo.CA = r.serverCA + // Do not touch accountInfo.Spec.FGA to preserve initializer-provided values. return nil })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/subroutines/accountinfo/accountinfo.go(1 hunks)test/setup/01-platform-mesh-system/apiexport-core.platform-mesh.io.yaml(1 hunks)test/setup/01-platform-mesh-system/apiexport-core.platform-mesh.org.yaml(1 hunks)test/setup/01-platform-mesh-system/apiresourceschema-accountinfos.core.platform-mesh.io.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/setup/01-platform-mesh-system/apiresourceschema-accountinfos.core.platform-mesh.io.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/subroutines/accountinfo/accountinfo.go (1)
api/v1alpha1/account_info_types.go (2)
AccountInfo(65-71)ClusterInfo(32-34)
⏰ 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). (2)
- GitHub Check: pipe / testSource / test
- GitHub Check: pipe / dockerBuild / docker
🔇 Additional comments (3)
pkg/subroutines/accountinfo/accountinfo.go (3)
110-111: LGTM! Clear explanation of the new approach.The comments effectively communicate the architectural change: the security-operator initializer now owns FGA population, and this code waits for that initialization before patching non-FGA fields.
112-119: LGTM! Proper error handling with requeue.The logic correctly distinguishes between not-found (requeue to wait for initializer) and other errors (propagate as operator error). This aligns well with the new initialization flow.
121-131: LGTM! Correct patch-based approach preserving FGA.The
CreateOrPatchupdates only non-FGA fields (Account, ParentAccount, Organization, CA) while explicitly preserving the initializer-provided FGA values. This matches the architectural intent.
test/setup/01-platform-mesh-system/apiexport-core.platform-mesh.org.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/subroutines/accountinfo/accountinfo_test.go (1)
434-470: Consider renaming test for clarity.The test name
TestProcess_Org_RequeuesUntilAccountInfoExistsimplies iterative requeuing until a condition is met, but the test only verifies a single requeue when AccountInfo doesn't exist. Consider renaming toTestProcess_Org_RequeuesWhenAccountInfoDoesNotExistfor accuracy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/clusteredname/clusteredname_test.go(2 hunks)pkg/subroutines/accountinfo/accountinfo_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/subroutines/accountinfo/accountinfo_test.go (5)
pkg/subroutines/mocks/mock_K8Cluster.go (2)
NewCluster(22-32)Cluster(35-37)pkg/subroutines/mocks/mock_K8Service.go (1)
NewClient(505-515)pkg/subroutines/accountinfo/accountinfo.go (1)
New(41-44)api/v1alpha1/account_types.go (4)
Account(79-85)AccountSpec(32-51)AccountTypeOrg(27-27)AccountTypeAccount(28-28)api/v1alpha1/account_info_types.go (1)
AccountInfo(65-71)
pkg/clusteredname/clusteredname_test.go (2)
api/v1alpha1/account_types.go (1)
Account(79-85)pkg/clusteredname/clusteredname.go (2)
GetClusteredName(17-29)MustGetClusteredName(31-36)
⏰ 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). (2)
- GitHub Check: pipe / testSource / test
- GitHub Check: pipe / dockerBuild / docker
🔇 Additional comments (4)
pkg/clusteredname/clusteredname_test.go (3)
4-4: LGTM! Imports are appropriate for the new tests.The
contextpackage andmccontextalias are correctly added to support the new cluster-context test cases.Also applies to: 12-12
28-38: LGTM! Test correctly verifies cluster context handling.The test properly validates that
GetClusteredNameextracts the cluster ID from context and populates theClusteredNamestruct when a cluster is present.
40-48: LGTM! Panic test is correctly implemented.The test properly validates that
MustGetClusteredNamepanics when no cluster is present in the context, using the standard Go defer/recover pattern.pkg/subroutines/accountinfo/accountinfo_test.go (1)
108-432: LGTM: FGA fields correctly removed from test data.The test correctly reflects the removal of FGA-related fields from AccountInfo.Spec (lines 152-160), leaving only AccountLocation and Organization fields.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/v1alpha1/account_info_types.go (1)
30-30: Add documentation for the Creator field.The Creator field lacks a comment explaining what it represents (e.g., user ID, email, subject identifier). Adding documentation will help API consumers understand its purpose and expected format.
Apply this diff to add documentation:
ClusterInfo ClusterInfo `json:"clusterInfo"` + // Creator is the identifier of the user who created this account Creator *string `json:"creator,omitempty"` }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
api/v1alpha1/account_info_types.go(2 hunks)api/v1alpha1/zz_generated.deepcopy.go(1 hunks)
⏰ 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). (2)
- GitHub Check: pipe / dockerBuild / docker
- GitHub Check: pipe / testSource / test
🔇 Additional comments (2)
api/v1alpha1/zz_generated.deepcopy.go (1)
142-146: LGTM! Auto-generated deepcopy logic is correct.The nil-safe deepcopy implementation for the Creator pointer field follows the standard pattern used throughout this file and is correctly generated by controller-gen.
api/v1alpha1/account_info_types.go (1)
59-60: LGTM! Status field appropriately tracks FGA tuple write status.The CreatorTupleWritten field is well-documented and appropriately placed in the status struct to track whether the security operator has written the initial creator FGA tuple. This aligns with the PR's objective of moving FGA handling to the security operator.
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 (3)
config/resources/apiexport-core.platform-mesh.io.yaml(1 hunks)config/resources/apiresourceschema-accountinfos.core.platform-mesh.io.yaml(3 hunks)test/setup/01-platform-mesh-system/apiresourceschema-accountinfos.core.platform-mesh.io.yaml(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/setup/01-platform-mesh-system/apiresourceschema-accountinfos.core.platform-mesh.io.yaml
⏰ 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). (2)
- GitHub Check: pipe / testSource / test
- GitHub Check: pipe / dockerBuild / docker
🔇 Additional comments (5)
config/resources/apiexport-core.platform-mesh.io.yaml (1)
11-20: Confirm permissionClaims restructuring aligns with security intent.The permissionClaims now use a flat list structure with three entries granting broad access to namespaces, workspaces, and workspacetypes. Verify that this restructuring maintains the intended security posture and doesn't inadvertently expand or restrict access compared to the previous version.
To confirm the restructuring is intentional, please share context on whether this permission set change is:
- Purely a structural refactoring with identical semantics
- An intentional security adjustment for the FGA migration
- Necessary for the Security Operator integration
config/resources/apiresourceschema-accountinfos.core.platform-mesh.io.yaml (4)
5-5: Schema name correctly updated and versioned.The metadata.name aligns with the new schema version referenced in the APIExport. The date-based versioning scheme (v251022-fga-fix) is consistent with codebase patterns and clearly identifies this as an FGA-related update from October 22, 2025.
127-144: New creator and fga fields properly structured; verify operator support.The creator (optional string) and fga (required object with store.id) fields are correctly added at the spec root level. Creator is optional to allow backward compatibility; fga is required to enforce authorization metadata. Verify that:
- The Account Operator correctly populates these fields when creating AccountInfo resources
- The operator validates and rejects AccountInfo without fga (since it's required)
- Existing AccountInfo instances are migrated or requeued if fga is missing (as mentioned in the PR objectives)
153-157: Status field properly tracks FGA tuple write completion.The creatorTupleWritten boolean correctly tracks operational state separate from the spec-level creator field. This design allows the operator to signal whether the FGA system has successfully processed the creator tuple, supporting the Security Operator's responsibility for FGA management.
145-150: Required fields appropriately enforce FGA while maintaining backward compatibility.FGA is required (enforcing authorization metadata), while creator is optional (supporting legacy AccountInfo instances). This balanced approach aligns with the PR objective to preserve existing FGA fields while adding new creator tracking.
Move the FGA subroutine functionality from the Account Operator to the Security Operator
Testing
Deploy WorkspaceTypes
Apply in order
Verify Configuration
Check security has initializer
kubectl get workspacetype security -o jsonpath='{.spec.initializer}'Expected: true
Check org extends security
kubectl get workspacetype org -o jsonpath='{.spec.extend.with[*].name}'Expected: security
Check account extends security
kubectl get workspacetype account -o jsonpath='{.spec.extend.with[*].name}'Expected: security
Test Initializer
Create orgs workspace
kubectl apply -f workspace-orgs.yaml --validate=falseSwitch to orgs and create test org
kubectl ws orgsVerify initializer added
kubectl get workspace test-org -o jsonpath='{.status.initializers}'Expected: ["root:security","system:apibindings"]
kubectl get workspace test-org -o jsonpath='{.status.phase}'Expected: Initializing
Success Criteria
securityhasinitializer: trueorgandaccountextendsecurityroot:securityto workspace initializersInitializingphaseSummary by CodeRabbit
Refactor
New Features
Tests
Bug Fixes
Chore