From 842fde9cd5ba801634dea9d10b41e54e47d9c632 Mon Sep 17 00:00:00 2001 From: Arturs Jefimovs Date: Thu, 9 Oct 2025 16:37:28 +0300 Subject: [PATCH 1/6] fix: removed duplicated function --- .gitignore | 3 ++- pkg/subroutines/fga_test.go | 5 ----- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/.gitignore b/.gitignore index a941c4f..54584cd 100644 --- a/.gitignore +++ b/.gitignore @@ -32,4 +32,5 @@ go.work *~ .DS_Store kcp.log -coverage.html \ No newline at end of file +coverage.html +.gocache \ No newline at end of file diff --git a/pkg/subroutines/fga_test.go b/pkg/subroutines/fga_test.go index aba4d96..8f588ba 100644 --- a/pkg/subroutines/fga_test.go +++ b/pkg/subroutines/fga_test.go @@ -40,11 +40,6 @@ func newFgaError(c openfgav1.ErrorCode, m string) *fgaError { } } -func TestFGASubroutine_GetName(t *testing.T) { - routine := NewFGASubroutine(nil, nil, "", "", "") - assert.Equal(t, "FGASubroutine", routine.GetName()) -} - func TestFGASubroutine_Finalizers(t *testing.T) { routine := NewFGASubroutine(nil, nil, "", "", "") assert.Equal(t, []string{"account.core.platform-mesh.io/fga"}, routine.Finalizers()) From d06dfd0460b8930b26204df248469a22b0a1543c Mon Sep 17 00:00:00 2001 From: Arturs Jefimovs Date: Thu, 9 Oct 2025 16:54:28 +0300 Subject: [PATCH 2/6] feat: only remove finalizer in case no child accounts exist --- pkg/subroutines/accountinfo.go | 29 +++++++++++++- pkg/subroutines/accountinfo_test.go | 59 +++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 2 deletions(-) diff --git a/pkg/subroutines/accountinfo.go b/pkg/subroutines/accountinfo.go index 836572d..80bc77c 100644 --- a/pkg/subroutines/accountinfo.go +++ b/pkg/subroutines/accountinfo.go @@ -49,12 +49,37 @@ func (r *AccountInfoSubroutine) GetName() string { 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) + if ts.Time.Before(oneMinAgo) { + requeue = false + } + } + // The account info object is relevant input for other finalizers, removing the accountinfo finalizer at last if len(ro.GetFinalizers()) > 1 { - delay := r.limiter.When(cn) - return ctrl.Result{RequeueAfter: delay}, nil + 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) + } + + 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) } + // All clear: no other finalizers and no child accounts exist. r.limiter.Forget(cn) return ctrl.Result{}, nil } diff --git a/pkg/subroutines/accountinfo_test.go b/pkg/subroutines/accountinfo_test.go index 646a4a0..8b3bae5 100644 --- a/pkg/subroutines/accountinfo_test.go +++ b/pkg/subroutines/accountinfo_test.go @@ -485,6 +485,65 @@ func (suite *AccountInfoSubroutineTestSuite) TestFinalizeNoContext() { }) } +func (suite *AccountInfoSubroutineTestSuite) TestFinalize_Blocks_WhenChildAccountsExist() { + // Given: only our finalizer remains and there are child accounts + ctx := context.Background() + ctx = kontext.WithCluster(ctx, "some-cluster-id") + acc := &v1alpha1.Account{ + ObjectMeta: v1.ObjectMeta{ + Name: "example-account", + Finalizers: []string{"account.core.platform-mesh.io/info"}, + DeletionTimestamp: &v1.Time{Time: time.Now()}, + }, + } + // Mock: List returns one child account + suite.clientMock.EXPECT(). + List(mock.Anything, mock.AnythingOfType("*v1alpha1.AccountList")). + Run(func(ctx context.Context, list client.ObjectList, opts ...client.ListOption) { + al := list.(*v1alpha1.AccountList) + al.Items = []v1alpha1.Account{{}} + }). + Return(nil) + + // When + res, err := suite.testObj.Finalize(ctx, acc) + + // Then + suite.Nil(err) + suite.Assert().NotZero(res.RequeueAfter) +} + +func (suite *AccountInfoSubroutineTestSuite) TestFinalize_ReturnsError_AfterOneMinute_WhenChildAccountsExist() { + // Given: only our finalizer remains, deletion older than 1 minute, and there are child accounts + ctx := context.Background() + ctx = kontext.WithCluster(ctx, "some-cluster-id") + acc := &v1alpha1.Account{ + ObjectMeta: v1.ObjectMeta{ + Name: "example-account", + Finalizers: []string{"account.core.platform-mesh.io/info"}, + DeletionTimestamp: &v1.Time{Time: time.Now().Add(-2 * time.Minute)}, + }, + } + // Mock: List returns one child account + suite.clientMock.EXPECT(). + List(mock.Anything, mock.AnythingOfType("*v1alpha1.AccountList")). + Run(func(ctx context.Context, list client.ObjectList, opts ...client.ListOption) { + al := list.(*v1alpha1.AccountList) + al.Items = []v1alpha1.Account{{}} + }). + Return(nil) + + // When + res, err := suite.testObj.Finalize(ctx, acc) + + // Then + suite.NotNil(err) + suite.Contains(err.Err().Error(), "finalizer not removed yet") + suite.True(err.Retry()) + suite.False(err.Sentry()) + suite.Assert().Zero(res.RequeueAfter) +} + func (suite *AccountInfoSubroutineTestSuite) mockGetAccountInfoCallNotFound() *mocks.Client_Get_Call { return suite.clientMock.EXPECT(). Get(mock.Anything, mock.Anything, mock.AnythingOfType("*v1alpha1.AccountInfo")). From 427b3dd9d91e5506c356c8862ef403e92e90274c Mon Sep 17 00:00:00 2001 From: Arturs Jefimovs Date: Thu, 9 Oct 2025 17:08:02 +0300 Subject: [PATCH 3/6] chore: go update 1.24.8 --- go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go.mod b/go.mod index 3e4485f..c13962d 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/platform-mesh/account-operator -go 1.24.5 +go 1.24.8 replace sigs.k8s.io/controller-runtime => github.com/kcp-dev/controller-runtime v0.19.0-kcp.1 From b96fabbcaf82f8a5a2cb391f56b90589074056d5 Mon Sep 17 00:00:00 2001 From: Arturs Jefimovs Date: Thu, 9 Oct 2025 17:10:48 +0300 Subject: [PATCH 4/6] chore: Dockerfile update --- Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Dockerfile b/Dockerfile index be950ab..9984b12 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,5 +1,5 @@ # Build the manager binary -FROM golang:1.24.6-bullseye AS builder +FROM golang:1.24.8-bullseye AS builder WORKDIR /workspace From 80598502934c9ab42e3aa2ade56af18b786198cf Mon Sep 17 00:00:00 2001 From: Arturs Jefimovs Date: Thu, 9 Oct 2025 17:14:00 +0300 Subject: [PATCH 5/6] chore: Dockerfile update (2) --- Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Dockerfile b/Dockerfile index 9984b12..55c4c2d 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,5 +1,5 @@ # Build the manager binary -FROM golang:1.24.8-bullseye AS builder +FROM golang:1.24.8 AS builder WORKDIR /workspace From 374485b557baa33e2883f721cf2df6ce0951f93d Mon Sep 17 00:00:00 2001 From: Arturs Jefimovs Date: Thu, 9 Oct 2025 18:20:59 +0300 Subject: [PATCH 6/6] fix: scope Account list in finalizer to child Accounts via label; add selector assertions in tests --- pkg/subroutines/accountinfo.go | 8 +++++++- pkg/subroutines/accountinfo_test.go | 28 ++++++++++++++++++++++++---- 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/pkg/subroutines/accountinfo.go b/pkg/subroutines/accountinfo.go index 80bc77c..82df885 100644 --- a/pkg/subroutines/accountinfo.go +++ b/pkg/subroutines/accountinfo.go @@ -68,7 +68,13 @@ func (r *AccountInfoSubroutine) Finalize(ctx context.Context, ro runtimeobject.R } accountList := &v1alpha1.AccountList{} - if err := r.client.List(ctx, accountList); err != nil { + // Only consider child Accounts of the current Account to decide on finalizer removal. + // We use the parent-identifying label set on child Accounts to filter the list. + if err := r.client.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 { diff --git a/pkg/subroutines/accountinfo_test.go b/pkg/subroutines/accountinfo_test.go index 8b3bae5..5395d84 100644 --- a/pkg/subroutines/accountinfo_test.go +++ b/pkg/subroutines/accountinfo_test.go @@ -27,6 +27,8 @@ import ( "github.com/platform-mesh/account-operator/internal/config" "github.com/platform-mesh/account-operator/pkg/subroutines" "github.com/platform-mesh/account-operator/pkg/subroutines/mocks" + + "k8s.io/apimachinery/pkg/labels" ) type AccountInfoSubroutineTestSuite struct { @@ -496,10 +498,19 @@ func (suite *AccountInfoSubroutineTestSuite) TestFinalize_Blocks_WhenChildAccoun DeletionTimestamp: &v1.Time{Time: time.Now()}, }, } - // Mock: List returns one child account + // Mock: List returns one child account and assert proper scoping via label selector suite.clientMock.EXPECT(). - List(mock.Anything, mock.AnythingOfType("*v1alpha1.AccountList")). + List(mock.Anything, mock.AnythingOfType("*v1alpha1.AccountList"), mock.Anything). Run(func(ctx context.Context, list client.ObjectList, opts ...client.ListOption) { + // Verify ListOptions include a label selector that matches the expected owner label + var lo client.ListOptions + for _, o := range opts { + o.ApplyToList(&lo) + } + suite.Require().NotNil(lo.LabelSelector, "expected a label selector for filtering child accounts") + expected := labels.Set{string(v1alpha1.NamespaceAccountOwnerLabel): "example-account"} + suite.True(lo.LabelSelector.Matches(expected), "label selector should match owner label for current Account") + al := list.(*v1alpha1.AccountList) al.Items = []v1alpha1.Account{{}} }). @@ -524,10 +535,19 @@ func (suite *AccountInfoSubroutineTestSuite) TestFinalize_ReturnsError_AfterOneM DeletionTimestamp: &v1.Time{Time: time.Now().Add(-2 * time.Minute)}, }, } - // Mock: List returns one child account + // Mock: List returns one child account and assert proper scoping via label selector suite.clientMock.EXPECT(). - List(mock.Anything, mock.AnythingOfType("*v1alpha1.AccountList")). + List(mock.Anything, mock.AnythingOfType("*v1alpha1.AccountList"), mock.Anything). Run(func(ctx context.Context, list client.ObjectList, opts ...client.ListOption) { + // Verify ListOptions include a label selector that matches the expected owner label + var lo client.ListOptions + for _, o := range opts { + o.ApplyToList(&lo) + } + suite.Require().NotNil(lo.LabelSelector, "expected a label selector for filtering child accounts") + expected := labels.Set{string(v1alpha1.NamespaceAccountOwnerLabel): "example-account"} + suite.True(lo.LabelSelector.Matches(expected), "label selector should match owner label for current Account") + al := list.(*v1alpha1.AccountList) al.Items = []v1alpha1.Account{{}} }).