From 30fa29a4a7b960a1b82582778bd604a18a047427 Mon Sep 17 00:00:00 2001 From: OlegErshov Date: Thu, 2 Apr 2026 11:34:53 +0200 Subject: [PATCH] fix: add check to prevent authorization model duplication On-behalf-of: SAP aleh.yarshou@sap.com --- .../authorization_model_generation.go | 17 +++- .../authorization_model_generation_test.go | 88 +++++++++++++++++++ 2 files changed, 104 insertions(+), 1 deletion(-) diff --git a/internal/subroutine/authorization_model_generation.go b/internal/subroutine/authorization_model_generation.go index ce73ed23..c1dfbf84 100644 --- a/internal/subroutine/authorization_model_generation.go +++ b/internal/subroutine/authorization_model_generation.go @@ -217,6 +217,7 @@ func (a *AuthorizationModelGenerationSubroutine) GetName() string { // Process implements lifecycle.Subroutine. func (a *AuthorizationModelGenerationSubroutine) Process(ctx context.Context, instance lifecyclecontrollerruntime.RuntimeObject) (ctrl.Result, errors.OperatorError) { binding := instance.(*kcpapisv1alpha2.APIBinding) + log := logger.LoadLoggerFromContext(ctx) internalAPIBindings := []string{"core.platform-mesh.io", "system.platform-mesh.io"} @@ -247,6 +248,11 @@ func (a *AuthorizationModelGenerationSubroutine) Process(ctx context.Context, in var apiExport kcpapisv1alpha2.APIExport err = apiExportCluster.GetClient().Get(ctx, types.NamespacedName{Name: binding.Spec.Reference.Export.Name}, &apiExport) if err != nil { + return ctrl.Result{},errors.NewOperatorError(err, true, true) + } + + allAuthorizationModels := securityv1alpha1.AuthorizationModelList{} + if err := a.allClient.List(ctx, &allAuthorizationModels); err != nil { return ctrl.Result{}, errors.NewOperatorError(err, true, true) } @@ -257,6 +263,15 @@ func (a *AuthorizationModelGenerationSubroutine) Process(ctx context.Context, in return ctrl.Result{}, errors.NewOperatorError(err, true, true) } + modelName := toK8sName(resourceSchema.Spec.Group, resourceSchema.Spec.Names.Plural, accountInfo.Spec.Organization.Name) + + if slices.ContainsFunc(allAuthorizationModels.Items, func(m securityv1alpha1.AuthorizationModel) bool { + return m.Name == modelName + }) { + log.Warn().Msg(fmt.Sprintf("authorization model: %s already exist", modelName)) + continue + } + longestRelationName := fmt.Sprintf("create_%s_%s", resourceSchema.Spec.Group, resourceSchema.Spec.Names.Plural) group := resourceSchema.Spec.Group @@ -278,7 +293,7 @@ func (a *AuthorizationModelGenerationSubroutine) Process(ctx context.Context, in model := securityv1alpha1.AuthorizationModel{ ObjectMeta: metav1.ObjectMeta{ - Name: toK8sName(resourceSchema.Spec.Group, resourceSchema.Spec.Names.Plural, accountInfo.Spec.Organization.Name), + Name: modelName, }, } diff --git a/internal/subroutine/authorization_model_generation_test.go b/internal/subroutine/authorization_model_generation_test.go index 1df2fff4..b0004d6e 100644 --- a/internal/subroutine/authorization_model_generation_test.go +++ b/internal/subroutine/authorization_model_generation_test.go @@ -5,6 +5,7 @@ import ( "testing" accountv1alpha1 "github.com/platform-mesh/account-operator/api/v1alpha1" + securityv1alpha1 "github.com/platform-mesh/security-operator/api/v1alpha1" "github.com/platform-mesh/security-operator/internal/subroutine" "github.com/platform-mesh/security-operator/internal/subroutine/mocks" "github.com/stretchr/testify/assert" @@ -13,6 +14,7 @@ import ( apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" kerrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" @@ -51,6 +53,7 @@ func mockAccountInfo(cl *mocks.MockClient, orgName, originCluster string) { }).Once() } + func TestAuthorizationModelGeneration_Process(t *testing.T) { tests := []struct { name string @@ -115,6 +118,12 @@ func TestAuthorizationModelGeneration_Process(t *testing.T) { } return nil }).Once() + allClient.EXPECT().List(mock.Anything, mock.Anything).RunAndReturn(func(ctx context.Context, ol client.ObjectList, lo ...client.ListOption) error { + if list, ok := ol.(*securityv1alpha1.AuthorizationModelList); ok { + list.Items = []securityv1alpha1.AuthorizationModel{} + } + return nil + }).Once() kcpClient.EXPECT().Get(mock.Anything, mock.Anything, mock.Anything).RunAndReturn(func(ctx context.Context, nn types.NamespacedName, o client.Object, opts ...client.GetOption) error { if rs, ok := o.(*kcpapisv1alpha1.APIResourceSchema); ok { rs.Spec.Group = "group" @@ -154,6 +163,12 @@ func TestAuthorizationModelGeneration_Process(t *testing.T) { } return nil }).Once() + allClient.EXPECT().List(mock.Anything, mock.Anything).RunAndReturn(func(ctx context.Context, ol client.ObjectList, lo ...client.ListOption) error { + if list, ok := ol.(*securityv1alpha1.AuthorizationModelList); ok { + list.Items = []securityv1alpha1.AuthorizationModel{} + } + return nil + }).Once() kcpClient.EXPECT().Get(mock.Anything, mock.Anything, mock.Anything).RunAndReturn(func(ctx context.Context, nn types.NamespacedName, o client.Object, opts ...client.GetOption) error { if rs, ok := o.(*kcpapisv1alpha1.APIResourceSchema); ok { rs.Spec.Group = "group" @@ -184,6 +199,12 @@ func TestAuthorizationModelGeneration_Process(t *testing.T) { } return nil }).Once() + allClient.EXPECT().List(mock.Anything, mock.Anything).RunAndReturn(func(ctx context.Context, ol client.ObjectList, lo ...client.ListOption) error { + if list, ok := ol.(*securityv1alpha1.AuthorizationModelList); ok { + list.Items = []securityv1alpha1.AuthorizationModel{} + } + return nil + }).Once() kcpClient.EXPECT().Get(mock.Anything, mock.Anything, mock.Anything).RunAndReturn(func(ctx context.Context, nn types.NamespacedName, o client.Object, opts ...client.GetOption) error { if rs, ok := o.(*kcpapisv1alpha1.APIResourceSchema); ok { rs.Spec.Group = "group" @@ -227,6 +248,12 @@ func TestAuthorizationModelGeneration_Process(t *testing.T) { } return nil }).Once() + allClient.EXPECT().List(mock.Anything, mock.Anything).RunAndReturn(func(ctx context.Context, ol client.ObjectList, lo ...client.ListOption) error { + if list, ok := ol.(*securityv1alpha1.AuthorizationModelList); ok { + list.Items = []securityv1alpha1.AuthorizationModel{} + } + return nil + }).Once() kcpClient.EXPECT().Get(mock.Anything, mock.Anything, mock.Anything).Return(assert.AnError) }, }, @@ -245,6 +272,12 @@ func TestAuthorizationModelGeneration_Process(t *testing.T) { } return nil }).Once() + allClient.EXPECT().List(mock.Anything, mock.Anything).RunAndReturn(func(ctx context.Context, ol client.ObjectList, lo ...client.ListOption) error { + if list, ok := ol.(*securityv1alpha1.AuthorizationModelList); ok { + list.Items = []securityv1alpha1.AuthorizationModel{} + } + return nil + }).Once() kcpClient.EXPECT().Get(mock.Anything, mock.Anything, mock.Anything).RunAndReturn(func(ctx context.Context, nn types.NamespacedName, o client.Object, opts ...client.GetOption) error { if rs, ok := o.(*kcpapisv1alpha1.APIResourceSchema); ok { rs.Spec.Group = "veryverylonggroup.platform-mesh.org" @@ -281,6 +314,61 @@ func TestAuthorizationModelGeneration_Process(t *testing.T) { manager.EXPECT().GetCluster(mock.Anything, "export-cluster").Return(nil, assert.AnError) }, }, + { + name: "error on List AuthorizationModels in Process", + binding: newApiBinding("foo", "bar"), + expectError: true, + mockSetup: func(manager *mocks.MockManager, allClient *mocks.MockClient, cluster *mocks.MockCluster, kcpClient *mocks.MockClient) { + manager.EXPECT().ClusterFromContext(mock.Anything).Return(cluster, nil) + cluster.EXPECT().GetClient().Return(kcpClient) + mockAccountInfo(kcpClient, "org", "origin") + manager.EXPECT().GetCluster(mock.Anything, mock.Anything).Return(cluster, nil) + kcpClient.EXPECT().Get(mock.Anything, mock.Anything, mock.Anything).RunAndReturn(func(ctx context.Context, nn types.NamespacedName, o client.Object, opts ...client.GetOption) error { + if ae, ok := o.(*kcpapisv1alpha2.APIExport); ok { + ae.Spec.Resources = []kcpapisv1alpha2.ResourceSchema{{Schema: "schema1"}} + return nil + } + return nil + }).Once() + allClient.EXPECT().List(mock.Anything, mock.Anything).Return(assert.AnError).Once() + }, + }, + { + name: "skip model creation when model already exists in Process", + binding: newApiBinding("foo", "bar"), + mockSetup: func(manager *mocks.MockManager, allClient *mocks.MockClient, cluster *mocks.MockCluster, kcpClient *mocks.MockClient) { + manager.EXPECT().ClusterFromContext(mock.Anything).Return(cluster, nil) + cluster.EXPECT().GetClient().Return(kcpClient) + mockAccountInfo(kcpClient, "org", "origin") + manager.EXPECT().GetCluster(mock.Anything, mock.Anything).Return(cluster, nil) + kcpClient.EXPECT().Get(mock.Anything, mock.Anything, mock.Anything).RunAndReturn(func(ctx context.Context, nn types.NamespacedName, o client.Object, opts ...client.GetOption) error { + if ae, ok := o.(*kcpapisv1alpha2.APIExport); ok { + ae.Spec.Resources = []kcpapisv1alpha2.ResourceSchema{{Schema: "schema1"}} + return nil + } + return nil + }).Once() + // model "group-foos-org" already exists — creation must be skipped + allClient.EXPECT().List(mock.Anything, mock.Anything).RunAndReturn(func(ctx context.Context, ol client.ObjectList, lo ...client.ListOption) error { + if list, ok := ol.(*securityv1alpha1.AuthorizationModelList); ok { + list.Items = []securityv1alpha1.AuthorizationModel{ + {ObjectMeta: metav1.ObjectMeta{Name: "group-foos-org"}}, + } + } + return nil + }).Once() + kcpClient.EXPECT().Get(mock.Anything, mock.Anything, mock.Anything).RunAndReturn(func(ctx context.Context, nn types.NamespacedName, o client.Object, opts ...client.GetOption) error { + if rs, ok := o.(*kcpapisv1alpha1.APIResourceSchema); ok { + rs.Spec.Group = "group" + rs.Spec.Names.Plural = "foos" + rs.Spec.Names.Singular = "foo" + rs.Spec.Scope = apiextensionsv1.ClusterScoped + return nil + } + return nil + }).Once() + }, + }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) {