Skip to content

feat: only remove finalizer in case no child accounts exist #48

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

Draft
kekcleader wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

feat: only remove finalizer in case no child accounts exist #48

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
842fde9
fix: removed duplicated function
kekcleader Oct 9, 2025
d06dfd0
feat: only remove finalizer in case no child accounts exist
kekcleader Oct 9, 2025
427b3dd
chore: go update 1.24.8
kekcleader Oct 9, 2025
b96fabb
chore: Dockerfile update
kekcleader Oct 9, 2025
8059850
chore: Dockerfile update (2)
kekcleader Oct 9, 2025
374485b
fix: scope Account list in finalizer to child Accounts via label; add…
kekcleader Oct 9, 2025
06c0925
Merge remote-tracking branch 'origin/main' into feat/delete-account-i…
kekcleader Oct 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# Build the manager binary
FROM golang:1.24.6-bullseye AS builder
FROM golang:1.24.8 AS builder

WORKDIR /workspace

Expand Down
6 changes: 4 additions & 2 deletions api/v1alpha1/account_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,10 @@ import (
type AccountType string

const (
AccountTypeOrg AccountType = "org"
AccountTypeAccount AccountType = "account"
AccountTypeOrg AccountType = "org"
AccountTypeAccount AccountType = "account"
NamespaceAccountOwnerLabel AccountType = "account.core.platform-mesh.io/owner"
NamespaceAccountOwnerNamespaceLabel AccountType = "account.core.platform-mesh.io/owner-namespace"
)

// AccountSpec defines the desired state of Account
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
module github.com/platform-mesh/account-operator

go 1.24.5
go 1.24.8

replace (
k8s.io/api => k8s.io/api v0.34.1
Expand Down
35 changes: 33 additions & 2 deletions pkg/subroutines/accountinfo/accountinfo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,40 @@ func (r *AccountInfoSubroutine) Finalizers(_ runtimeobject.RuntimeObject) []stri
func (r *AccountInfoSubroutine) Finalize(ctx context.Context, ro runtimeobject.RuntimeObject) (ctrl.Result, errors.OperatorError) {
cn := clusteredname.MustGetClusteredName(ctx, ro)

// The account info object is relevant input for other finalizers, removing the accountinfo finalizer at last
requeue := true
if ts := ro.GetDeletionTimestamp(); ts != nil {
oneMinAgo := v1.Now().Add(-1 * time.Minute)
if ts.Time.Before(oneMinAgo) {
requeue = false
}
}

if len(ro.GetFinalizers()) > 1 {
return ctrl.Result{RequeueAfter: r.limiter.When(cn)}, nil
if requeue {
return ctrl.Result{RequeueAfter: r.limiter.When(cn)}, nil
}
return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("finalizer not removed yet"), true, false)
}

clusterRef, err := r.mgr.GetCluster(ctx, string(cn.ClusterID))
if err != nil {
return ctrl.Result{}, errors.NewOperatorError(err, true, true)
}
clusterClient := clusterRef.GetClient()

accountList := &v1alpha1.AccountList{}
if err := clusterClient.List(
ctx,
accountList,
client.MatchingLabels(map[string]string{string(v1alpha1.NamespaceAccountOwnerLabel): ro.GetName()}),
); err != nil {
return ctrl.Result{}, errors.NewOperatorError(err, true, true)
}
if len(accountList.Items) > 0 {
if requeue {
return ctrl.Result{RequeueAfter: r.limiter.When(cn)}, nil
}
return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("finalizer not removed yet"), true, false)
}

r.limiter.Forget(cn)
Expand Down
155 changes: 26 additions & 129 deletions pkg/subroutines/fga.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,11 @@ import (
"strings"
"time"

kcpcorev1alpha "github.com/kcp-dev/kcp/sdk/apis/core/v1alpha1"
kcptenancyv1alpha "github.com/kcp-dev/kcp/sdk/apis/tenancy/v1alpha1"
openfgav1 "github.com/openfga/api/proto/openfga/v1"
"github.com/platform-mesh/golang-commons/controller/lifecycle/runtimeobject"
"github.com/platform-mesh/golang-commons/errors"
"github.com/platform-mesh/golang-commons/fga/helpers"
"github.com/platform-mesh/golang-commons/logger"
"k8s.io/apimachinery/pkg/api/meta"
"k8s.io/client-go/util/workqueue"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand All @@ -32,27 +29,31 @@ type FGASubroutine struct {
objectType string
parentRelation string
creatorRelation string

limiter workqueue.TypedRateLimiter[clusteredname.ClusteredName]
limiter workqueue.TypedRateLimiter[clusteredname.ClusteredName]
}

func NewFGASubroutine(mgr mcmanager.Manager, fgaClient openfgav1.OpenFGAServiceClient, creatorRelation, parentRelation, objectType string) *FGASubroutine {
exp := workqueue.NewTypedItemExponentialFailureRateLimiter[clusteredname.ClusteredName](1*time.Second, 120*time.Second)
return &FGASubroutine{
mgr: mgr,
fgaClient: fgaClient,
creatorRelation: creatorRelation,
parentRelation: parentRelation,
objectType: objectType,
limiter: workqueue.NewTypedItemExponentialFailureRateLimiter[clusteredname.ClusteredName](1*time.Second, 120*time.Second),
limiter: exp,
}
}

func (e *FGASubroutine) Process(ctx context.Context, ro runtimeobject.RuntimeObject) (ctrl.Result, errors.OperatorError) {
account := ro.(*v1alpha1.Account)
cn := clusteredname.MustGetClusteredName(ctx, ro)
log := logger.LoadLoggerFromContext(ctx)
log.Debug().Msg("Skipping FGASubroutine.Process during initialization; handled by workspace initializer")
return ctrl.Result{}, nil
}

func (e *FGASubroutine) Finalize(ctx context.Context, runtimeObj runtimeobject.RuntimeObject) (ctrl.Result, errors.OperatorError) {
account := runtimeObj.(*v1alpha1.Account)
log := logger.LoadLoggerFromContext(ctx)
log.Debug().Msg("Starting creator subroutine process() function")
cn := clusteredname.MustGetClusteredName(ctx, runtimeObj)

clusterName, ok := mccontext.ClusterFrom(ctx)
if !ok {
Expand All @@ -65,119 +66,20 @@ func (e *FGASubroutine) Process(ctx context.Context, ro runtimeobject.RuntimeObj
}
clusterClient := clusterRef.GetClient()

accountWorkspace := &kcptenancyv1alpha.Workspace{}
if err := clusterClient.Get(ctx, client.ObjectKey{Name: account.Name}, accountWorkspace); err != nil {
return ctrl.Result{}, errors.NewOperatorError(err, true, true)
}

if accountWorkspace.Status.Phase != kcpcorev1alpha.LogicalClusterPhaseReady {
log.Info().Msg("workspace is not ready yet, retry")
return ctrl.Result{RequeueAfter: e.limiter.When(cn)}, nil
}

accountCluster, err := e.mgr.GetCluster(ctx, accountWorkspace.Spec.Cluster)
if err != nil {
childAccounts := &v1alpha1.AccountList{}
if err := clusterClient.List(
ctx,
childAccounts,
client.MatchingLabels(map[string]string{string(v1alpha1.NamespaceAccountOwnerLabel): account.Name}),
); err != nil {
return ctrl.Result{}, errors.NewOperatorError(err, true, true)
}
accountClusterClient := accountCluster.GetClient()

accountInfo, err := e.getAccountInfo(ctx, accountClusterClient)
if err != nil {
log.Error().Err(err).Msg("Couldn't get Store Id")
return ctrl.Result{}, errors.NewOperatorError(err, true, true)
}

if accountInfo.Spec.FGA.Store.Id == "" {
log.Error().Msg("FGA Store Id is empty")
return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("FGA Store Id is empty"), true, true)
}

if accountInfo.Spec.Account.GeneratedClusterId == "" {
log.Error().Msg("account cluster id is empty")
return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("account cluster id is empty"), true, true)
}

if account.Spec.Type != v1alpha1.AccountTypeOrg && accountInfo.Spec.ParentAccount.GeneratedClusterId == "" {
log.Error().Msg("parent account cluster id is empty")
return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("parent account cluster id is empty"), true, true)
}

writes := []*openfgav1.TupleKey{}

// Parent Name
if account.Spec.Type != v1alpha1.AccountTypeOrg {
parentAccountName := accountInfo.Spec.ParentAccount.Name

// Determine parent account to create parent relation
writes = append(writes, &openfgav1.TupleKey{
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()),
})
}

// Assign creator to the account
creatorTuplesWritten := meta.IsStatusConditionTrue(account.Status.Conditions, fmt.Sprintf("%s_Ready", e.GetName()))
if account.Spec.Creator != nil && !creatorTuplesWritten {
if valid := validateCreator(*account.Spec.Creator); !valid {
log.Error().Str("creator", *account.Spec.Creator).Msg("creator string is in the protected service account prefix range")
return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("creator in protected service account range"), false, false)
}
creator := formatUser(*account.Spec.Creator)

writes = append(writes, &openfgav1.TupleKey{
User: fmt.Sprintf("user:%s", creator),
Relation: "assignee",
Object: fmt.Sprintf("role:%s/%s/%s/owner", e.objectType, accountInfo.Spec.Account.OriginClusterId, account.Name),
})

writes = append(writes, &openfgav1.TupleKey{
User: fmt.Sprintf("role:%s/%s/%s/owner#assignee", e.objectType, accountInfo.Spec.Account.OriginClusterId, account.Name),
Relation: e.creatorRelation,
Object: fmt.Sprintf("%s:%s/%s", e.objectType, accountInfo.Spec.Account.OriginClusterId, account.Name),
})
}

for _, writeTuple := range writes {
_, err = e.fgaClient.Write(ctx, &openfgav1.WriteRequest{
StoreId: accountInfo.Spec.FGA.Store.Id,
Writes: &openfgav1.WriteRequestWrites{
TupleKeys: []*openfgav1.TupleKey{writeTuple},
},
})

if helpers.IsDuplicateWriteError(err) {
log.Info().Err(err).Msg("Open FGA writeTuple failed due to invalid input (possible duplicate)")
err = nil
}

if err != nil {
log.Error().Err(err).Msg("Open FGA writeTuple failed")
return ctrl.Result{}, errors.NewOperatorError(err, true, true)
}
if len(childAccounts.Items) > 0 {
delay := e.limiter.When(cn)
return ctrl.Result{RequeueAfter: delay}, nil
}

e.limiter.Forget(cn)
return ctrl.Result{}, nil
}

func (e *FGASubroutine) Finalize(ctx context.Context, runtimeObj runtimeobject.RuntimeObject) (ctrl.Result, errors.OperatorError) {
account := runtimeObj.(*v1alpha1.Account)
log := logger.LoadLoggerFromContext(ctx)

// Skip fga account finalization for organizations because the store is removed completely
if account.Spec.Type != v1alpha1.AccountTypeOrg {
clusterName, ok := mccontext.ClusterFrom(ctx)
if !ok {
return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("cluster client not available: ensure context carries cluster information"), true, true)
}

clusterRef, err := e.mgr.GetCluster(ctx, clusterName)
if err != nil {
return ctrl.Result{}, errors.NewOperatorError(err, true, true)
}
clusterClient := clusterRef.GetClient()

accountInfo, err := e.getAccountInfo(ctx, clusterClient)
if err != nil {
log.Error().Err(err).Msg("Couldn't get Store Id")
Expand All @@ -190,15 +92,12 @@ func (e *FGASubroutine) Finalize(ctx context.Context, runtimeObj runtimeobject.R
}

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()),
})
Comment on lines 94 to +100
Copy link

@coderabbitai coderabbitai bot Oct 13, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 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.go

Length 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.go

Length of output: 318

🏁 Script executed:

#!/bin/bash
rg -n 'type AccountLocation' -C3 api/v1alpha1/account_info_types.go

Length 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.


if account.Spec.Creator != nil {
creator := formatUser(*account.Spec.Creator)
Expand All @@ -216,7 +115,6 @@ func (e *FGASubroutine) Finalize(ctx context.Context, runtimeObj runtimeobject.R
}

for _, deleteTuple := range deletes {

_, err = e.fgaClient.Write(ctx, &openfgav1.WriteRequest{
StoreId: accountInfo.Spec.FGA.Store.Id,
Deletes: &openfgav1.WriteRequestDeletes{
Expand All @@ -228,15 +126,14 @@ func (e *FGASubroutine) Finalize(ctx context.Context, runtimeObj runtimeobject.R
log.Info().Err(err).Msg("Open FGA write failed due to invalid input (possibly trying to deleteTuple nonexisting entry)")
err = nil
}

if err != nil {
log.Error().Err(err).Msg("Open FGA write failed")
return ctrl.Result{}, errors.NewOperatorError(err, true, true)
}

}
}

e.limiter.Forget(cn)
return ctrl.Result{}, nil
}

Expand Down
Loading
Loading