From 63a4ebb484e48dea324c77ec61407407f24345b7 Mon Sep 17 00:00:00 2001 From: OlegErshov Date: Wed, 17 Dec 2025 11:52:39 +0100 Subject: [PATCH 01/22] feat: added idp resource creation On-behalf-of: SAP aleh.yarshou@sap.com --- internal/subroutine/idp.go | 123 +++++++++++++ internal/subroutine/idp_test.go | 299 ++++++++++++++++++++++++++++++++ 2 files changed, 422 insertions(+) create mode 100644 internal/subroutine/idp.go create mode 100644 internal/subroutine/idp_test.go diff --git a/internal/subroutine/idp.go b/internal/subroutine/idp.go new file mode 100644 index 0000000..ddb41ac --- /dev/null +++ b/internal/subroutine/idp.go @@ -0,0 +1,123 @@ +package subroutine + +import ( + "context" + "fmt" + + kcpv1alpha1 "github.com/kcp-dev/kcp/sdk/apis/core/v1alpha1" + accountv1alpha1 "github.com/platform-mesh/account-operator/api/v1alpha1" + "github.com/platform-mesh/golang-commons/controller/lifecycle/runtimeobject" + lifecyclesubroutine "github.com/platform-mesh/golang-commons/controller/lifecycle/subroutine" + "github.com/rs/zerolog/log" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + + "github.com/platform-mesh/golang-commons/errors" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + mcmanager "sigs.k8s.io/multicluster-runtime/pkg/manager" + + "github.com/platform-mesh/security-operator/api/v1alpha1" + "github.com/platform-mesh/security-operator/internal/config" +) + +func NewIDPSubroutine(orgsClient client.Client, mgr mcmanager.Manager, cfg *config.Config, baseDomain string) *IDPSubroutine { + return &IDPSubroutine{ + orgsClient: orgsClient, + mgr: mgr, + cfg: cfg, + baseDomain: baseDomain, + } +} + +var _ lifecyclesubroutine.Subroutine = &IDPSubroutine{} + +type IDPSubroutine struct { + orgsClient client.Client + mgr mcmanager.Manager + cfg *config.Config + baseDomain string +} + +func (w *IDPSubroutine) Finalize(ctx context.Context, instance runtimeobject.RuntimeObject) (ctrl.Result, errors.OperatorError) { + return ctrl.Result{}, nil +} + +func (w *IDPSubroutine) Finalizers(_ runtimeobject.RuntimeObject) []string { + return nil +} + +func (w *IDPSubroutine) GetName() string { return "IDPSubroutine" } + +func (w *IDPSubroutine) Process(ctx context.Context, instance runtimeobject.RuntimeObject) (ctrl.Result, errors.OperatorError) { + lc := instance.(*kcpv1alpha1.LogicalCluster) + + workspaceName := getWorkspaceName(lc) + if workspaceName == "" { + return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to get workspace name"), true, false) + } + + cl, err := w.mgr.ClusterFromContext(ctx) + if err != nil { + return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to get cluster from context %w", err), true, true) + } + + var account accountv1alpha1.Account + err = w.orgsClient.Get(ctx, types.NamespacedName{Name: workspaceName}, &account) + if err != nil { + return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to get account resource %w", err), true, true) + } + + if account.Spec.Type != accountv1alpha1.AccountTypeOrg { + log.Info().Str("workspace", workspaceName).Msg("account is not of type organization, skipping idp creation") + return ctrl.Result{}, nil + } + + clientConfig := v1alpha1.IdentityProviderClientConfig{ + ClientName: workspaceName, + ClientType: v1alpha1.IdentityProviderClientTypeConfidential, + RedirectURIs: append(w.cfg.IDP.AdditionalRedirectURLs, fmt.Sprintf("https://%s.%s/*", workspaceName, w.baseDomain)), + PostLogoutRedirectURIs: []string{fmt.Sprintf("http://%s.%s/logout*", workspaceName, w.baseDomain)}, + SecretRef: corev1.SecretReference{ + Name: fmt.Sprintf("portal-client-secret-%s-%s", workspaceName, workspaceName), + Namespace: "default", + }, + } + + idp := &v1alpha1.IdentityProviderConfiguration{ObjectMeta: metav1.ObjectMeta{Name: workspaceName}} + _, err = controllerutil.CreateOrUpdate(ctx, cl.GetClient(), idp, func() error { + for i := range idp.Spec.Clients { + if idp.Spec.Clients[i].ClientName == clientConfig.ClientName { + idp.Spec.Clients[i].RedirectURIs = clientConfig.RedirectURIs + idp.Spec.Clients[i].ClientType = clientConfig.ClientType + idp.Spec.Clients[i].SecretRef = clientConfig.SecretRef + idp.Spec.Clients[i].PostLogoutRedirectURIs = clientConfig.PostLogoutRedirectURIs + + return nil + } + } + + idp.Spec.Clients = []v1alpha1.IdentityProviderClientConfig{clientConfig} + + return nil + }) + if err != nil { + return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to create idp resource %w", err), true, true) + } + + log.Info().Str("workspace", workspaceName).Msg("idp configuration resource is created") + + if err := cl.GetClient().Get(ctx, types.NamespacedName{Name: workspaceName}, idp); err != nil { + return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to get idp resource %w", err), true, true) + } + if !meta.IsStatusConditionTrue(idp.GetConditions(), "Ready") { + return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("idp resource is not ready yet"), true, false) + } + + log.Info().Str("workspace", workspaceName).Msg("idp resource is ready") + return ctrl.Result{}, nil +} diff --git a/internal/subroutine/idp_test.go b/internal/subroutine/idp_test.go new file mode 100644 index 0000000..cfd715a --- /dev/null +++ b/internal/subroutine/idp_test.go @@ -0,0 +1,299 @@ +package subroutine + +import ( + "context" + "testing" + + kcpv1alpha1 "github.com/kcp-dev/kcp/sdk/apis/core/v1alpha1" + accountv1alpha1 "github.com/platform-mesh/account-operator/api/v1alpha1" + "github.com/platform-mesh/golang-commons/logger/testlogger" + secopv1alpha1 "github.com/platform-mesh/security-operator/api/v1alpha1" + "github.com/platform-mesh/security-operator/internal/config" + "github.com/platform-mesh/security-operator/internal/subroutine/mocks" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + apierrors "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" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +func TestNewIDPSubroutine(t *testing.T) { + orgsClient := mocks.NewMockClient(t) + mgr := mocks.NewMockManager(t) + cfg := &config.Config{ + BaseDomain: "example.com", + IDP: struct { + SMTPServer string `mapstructure:"idp-smtp-server"` + SMTPPort int `mapstructure:"idp-smtp-port"` + FromAddress string `mapstructure:"idp-from-address"` + SSL bool `mapstructure:"idp-smtp-ssl" default:"false"` + StartTLS bool `mapstructure:"idp-smtp-starttls" default:"false"` + SMTPUser string `mapstructure:"idp-smtp-user"` + SMTPPassword string `mapstructure:"idp-smtp-password"` + AdditionalRedirectURLs []string `mapstructure:"idp-additional-redirect-urls"` + }{ + AdditionalRedirectURLs: []string{"https://example.com/callback"}, + }, + } + baseDomain := "example.com" + + subroutine := NewIDPSubroutine(orgsClient, mgr, cfg, baseDomain) + + assert.NotNil(t, subroutine) + assert.Equal(t, orgsClient, subroutine.orgsClient) + assert.Equal(t, mgr, subroutine.mgr) + assert.Equal(t, cfg, subroutine.cfg) + assert.Equal(t, baseDomain, subroutine.baseDomain) +} + +func TestIDPSubroutine_GetName(t *testing.T) { + orgsClient := mocks.NewMockClient(t) + mgr := mocks.NewMockManager(t) + cfg := &config.Config{} + subroutine := NewIDPSubroutine(orgsClient, mgr, cfg, "example.com") + + name := subroutine.GetName() + assert.Equal(t, "IDPSubroutine", name) +} + +func TestIDPSubroutine_Finalizers(t *testing.T) { + orgsClient := mocks.NewMockClient(t) + mgr := mocks.NewMockManager(t) + cfg := &config.Config{} + subroutine := NewIDPSubroutine(orgsClient, mgr, cfg, "example.com") + + finalizers := subroutine.Finalizers(nil) + assert.Nil(t, finalizers) +} + +func TestIDPSubroutine_Finalize(t *testing.T) { + orgsClient := mocks.NewMockClient(t) + mgr := mocks.NewMockManager(t) + cfg := &config.Config{} + subroutine := NewIDPSubroutine(orgsClient, mgr, cfg, "example.com") + + ctx := context.Background() + instance := &kcpv1alpha1.LogicalCluster{} + + result, opErr := subroutine.Finalize(ctx, instance) + + assert.Nil(t, opErr) + assert.Equal(t, ctrl.Result{}, result) +} + +func TestIDPSubroutine_Process(t *testing.T) { + tests := []struct { + name string + setupMocks func(*mocks.MockClient, *mocks.MockManager, *mocks.MockCluster, *config.Config) + lc *kcpv1alpha1.LogicalCluster + expectedErr bool + expectedResult ctrl.Result + }{ + { + name: "Empty workspace name - early return", + setupMocks: func(orgsClient *mocks.MockClient, mgr *mocks.MockManager, cluster *mocks.MockCluster, cfg *config.Config) {}, + lc: &kcpv1alpha1.LogicalCluster{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{}, + }, + }, + expectedErr: true, + expectedResult: ctrl.Result{}, + }, + { + name: "ClusterFromContext error", + setupMocks: func(orgsClient *mocks.MockClient, mgr *mocks.MockManager, cluster *mocks.MockCluster, cfg *config.Config) { + mgr.EXPECT().ClusterFromContext(mock.Anything).Return(nil, assert.AnError).Once() + }, + lc: &kcpv1alpha1.LogicalCluster{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "kcp.io/path": "root:orgs:test", + }, + }, + }, + expectedErr: true, + expectedResult: ctrl.Result{}, + }, + { + name: "Account Get error", + setupMocks: func(orgsClient *mocks.MockClient, mgr *mocks.MockManager, cluster *mocks.MockCluster, cfg *config.Config) { + mgr.EXPECT().ClusterFromContext(mock.Anything).Return(cluster, nil).Once() + orgsClient.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "test"}, mock.AnythingOfType("*v1alpha1.Account")). + Return(assert.AnError).Once() + }, + lc: &kcpv1alpha1.LogicalCluster{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "kcp.io/path": "root:orgs:test", + }, + }, + }, + expectedErr: true, + expectedResult: ctrl.Result{}, + }, + { + name: "Account not of type organization - skip idp creation", + setupMocks: func(orgsClient *mocks.MockClient, mgr *mocks.MockManager, cluster *mocks.MockCluster, cfg *config.Config) { + mgr.EXPECT().ClusterFromContext(mock.Anything).Return(cluster, nil).Once() + orgsClient.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "test"}, mock.AnythingOfType("*v1alpha1.Account")). + RunAndReturn(func(_ context.Context, _ types.NamespacedName, obj client.Object, _ ...client.GetOption) error { + acc := obj.(*accountv1alpha1.Account) + acc.Spec.Type = accountv1alpha1.AccountTypeAccount // Not organization type + return nil + }).Once() + }, + lc: &kcpv1alpha1.LogicalCluster{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "kcp.io/path": "root:orgs:test", + }, + }, + }, + expectedErr: false, + expectedResult: ctrl.Result{}, + }, + { + name: "CreateOrUpdate and Ready", + setupMocks: func(orgsClient *mocks.MockClient, mgr *mocks.MockManager, cluster *mocks.MockCluster, cfg *config.Config) { + mgr.EXPECT().ClusterFromContext(mock.Anything).Return(cluster, nil).Once() + orgsClient.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "acme"}, mock.AnythingOfType("*v1alpha1.Account")). + RunAndReturn(func(_ context.Context, _ types.NamespacedName, obj client.Object, _ ...client.GetOption) error { + acc := obj.(*accountv1alpha1.Account) + acc.Spec.Type = accountv1alpha1.AccountTypeOrg + return nil + }).Once() + cluster.EXPECT().GetClient().Return(orgsClient).Maybe() + orgsClient.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "acme"}, mock.AnythingOfType("*v1alpha1.IdentityProviderConfiguration")). + Return(apierrors.NewNotFound(schema.GroupResource{Group: "core.platform-mesh.io", Resource: "identityproviderconfigurations"}, "acme")).Once() + orgsClient.EXPECT().Create(mock.Anything, mock.AnythingOfType("*v1alpha1.IdentityProviderConfiguration")). + RunAndReturn(func(_ context.Context, obj client.Object, _ ...client.CreateOption) error { + idp := obj.(*secopv1alpha1.IdentityProviderConfiguration) + idp.Status.Conditions = []metav1.Condition{{Type: "Ready", Status: metav1.ConditionTrue}} + return nil + }).Once() + orgsClient.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "acme"}, mock.AnythingOfType("*v1alpha1.IdentityProviderConfiguration")). + RunAndReturn(func(_ context.Context, _ types.NamespacedName, obj client.Object, _ ...client.GetOption) error { + idp := obj.(*secopv1alpha1.IdentityProviderConfiguration) + idp.Status.Conditions = []metav1.Condition{{Type: "Ready", Status: metav1.ConditionTrue}} + return nil + }).Once() + }, + lc: &kcpv1alpha1.LogicalCluster{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "kcp.io/path": "root:orgs:acme", + }, + }, + }, + expectedErr: false, + expectedResult: ctrl.Result{}, + }, + { + name: "CreateOrUpdate NotReady", + setupMocks: func(orgsClient *mocks.MockClient, mgr *mocks.MockManager, cluster *mocks.MockCluster, cfg *config.Config) { + mgr.EXPECT().ClusterFromContext(mock.Anything).Return(cluster, nil).Once() + orgsClient.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "beta"}, mock.AnythingOfType("*v1alpha1.Account")). + RunAndReturn(func(_ context.Context, _ types.NamespacedName, obj client.Object, _ ...client.GetOption) error { + acc := obj.(*accountv1alpha1.Account) + acc.Spec.Type = accountv1alpha1.AccountTypeOrg + return nil + }).Once() + cluster.EXPECT().GetClient().Return(orgsClient).Maybe() + orgsClient.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "beta"}, mock.AnythingOfType("*v1alpha1.IdentityProviderConfiguration")). + Return(apierrors.NewNotFound(schema.GroupResource{Group: "core.platform-mesh.io", Resource: "identityproviderconfigurations"}, "beta")).Once() + orgsClient.EXPECT().Create(mock.Anything, mock.AnythingOfType("*v1alpha1.IdentityProviderConfiguration")). + RunAndReturn(func(_ context.Context, obj client.Object, _ ...client.CreateOption) error { + idp := obj.(*secopv1alpha1.IdentityProviderConfiguration) + idp.Status.Conditions = []metav1.Condition{{Type: "Ready", Status: metav1.ConditionFalse}} + return nil + }).Once() + orgsClient.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "beta"}, mock.AnythingOfType("*v1alpha1.IdentityProviderConfiguration")). + RunAndReturn(func(_ context.Context, _ types.NamespacedName, obj client.Object, _ ...client.GetOption) error { + idp := obj.(*secopv1alpha1.IdentityProviderConfiguration) + idp.Status.Conditions = []metav1.Condition{{Type: "Ready", Status: metav1.ConditionFalse}} + return nil + }).Once() + }, + lc: &kcpv1alpha1.LogicalCluster{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "kcp.io/path": "root:orgs:beta", + }, + }, + }, + expectedErr: true, + expectedResult: ctrl.Result{}, + }, + { + name: "Get IDP resource error", + setupMocks: func(orgsClient *mocks.MockClient, mgr *mocks.MockManager, cluster *mocks.MockCluster, cfg *config.Config) { + mgr.EXPECT().ClusterFromContext(mock.Anything).Return(cluster, nil).Once() + orgsClient.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "gamma"}, mock.AnythingOfType("*v1alpha1.Account")). + RunAndReturn(func(_ context.Context, _ types.NamespacedName, obj client.Object, _ ...client.GetOption) error { + acc := obj.(*accountv1alpha1.Account) + acc.Spec.Type = accountv1alpha1.AccountTypeOrg + return nil + }).Once() + cluster.EXPECT().GetClient().Return(orgsClient).Maybe() + orgsClient.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "gamma"}, mock.AnythingOfType("*v1alpha1.IdentityProviderConfiguration")). + Return(apierrors.NewNotFound(schema.GroupResource{Group: "core.platform-mesh.io", Resource: "identityproviderconfigurations"}, "gamma")).Once() + orgsClient.EXPECT().Create(mock.Anything, mock.AnythingOfType("*v1alpha1.IdentityProviderConfiguration")). + Return(nil).Once() + orgsClient.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "gamma"}, mock.AnythingOfType("*v1alpha1.IdentityProviderConfiguration")). + Return(assert.AnError).Once() + }, + lc: &kcpv1alpha1.LogicalCluster{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "kcp.io/path": "root:orgs:gamma", + }, + }, + }, + expectedErr: true, + expectedResult: ctrl.Result{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + orgsClient := mocks.NewMockClient(t) + mgr := mocks.NewMockManager(t) + cluster := mocks.NewMockCluster(t) + cfg := &config.Config{ + BaseDomain: "example.com", + IDP: struct { + SMTPServer string `mapstructure:"idp-smtp-server"` + SMTPPort int `mapstructure:"idp-smtp-port"` + FromAddress string `mapstructure:"idp-from-address"` + SSL bool `mapstructure:"idp-smtp-ssl" default:"false"` + StartTLS bool `mapstructure:"idp-smtp-starttls" default:"false"` + SMTPUser string `mapstructure:"idp-smtp-user"` + SMTPPassword string `mapstructure:"idp-smtp-password"` + AdditionalRedirectURLs []string `mapstructure:"idp-additional-redirect-urls"` + }{ + AdditionalRedirectURLs: []string{}, + }, + } + subroutine := NewIDPSubroutine(orgsClient, mgr, cfg, "example.com") + + tt.setupMocks(orgsClient, mgr, cluster, cfg) + + l := testlogger.New() + ctx := l.WithContext(context.Background()) + + result, opErr := subroutine.Process(ctx, tt.lc) + + if tt.expectedErr { + assert.NotNil(t, opErr) + } else { + assert.Nil(t, opErr) + } + assert.Equal(t, tt.expectedResult, result) + }) + } +} + From efb720ecfedd26f2b1cfb0c6a201512f0fc8a928 Mon Sep 17 00:00:00 2001 From: OlegErshov Date: Wed, 17 Dec 2025 12:16:32 +0100 Subject: [PATCH 02/22] fixed tests On-behalf-of: SAP aleh.yarshou@sap.com --- internal/subroutine/idp_test.go | 38 ++++++--------------------------- 1 file changed, 6 insertions(+), 32 deletions(-) diff --git a/internal/subroutine/idp_test.go b/internal/subroutine/idp_test.go index cfd715a..d2d6e64 100644 --- a/internal/subroutine/idp_test.go +++ b/internal/subroutine/idp_test.go @@ -23,21 +23,8 @@ import ( func TestNewIDPSubroutine(t *testing.T) { orgsClient := mocks.NewMockClient(t) mgr := mocks.NewMockManager(t) - cfg := &config.Config{ - BaseDomain: "example.com", - IDP: struct { - SMTPServer string `mapstructure:"idp-smtp-server"` - SMTPPort int `mapstructure:"idp-smtp-port"` - FromAddress string `mapstructure:"idp-from-address"` - SSL bool `mapstructure:"idp-smtp-ssl" default:"false"` - StartTLS bool `mapstructure:"idp-smtp-starttls" default:"false"` - SMTPUser string `mapstructure:"idp-smtp-user"` - SMTPPassword string `mapstructure:"idp-smtp-password"` - AdditionalRedirectURLs []string `mapstructure:"idp-additional-redirect-urls"` - }{ - AdditionalRedirectURLs: []string{"https://example.com/callback"}, - }, - } + cfg := &config.Config{} + cfg.IDP.AdditionalRedirectURLs = []string{"https://example.com/callback"} baseDomain := "example.com" subroutine := NewIDPSubroutine(orgsClient, mgr, cfg, baseDomain) @@ -94,7 +81,8 @@ func TestIDPSubroutine_Process(t *testing.T) { }{ { name: "Empty workspace name - early return", - setupMocks: func(orgsClient *mocks.MockClient, mgr *mocks.MockManager, cluster *mocks.MockCluster, cfg *config.Config) {}, + setupMocks: func(orgsClient *mocks.MockClient, mgr *mocks.MockManager, cluster *mocks.MockCluster, cfg *config.Config) { + }, lc: &kcpv1alpha1.LogicalCluster{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{}, @@ -263,21 +251,8 @@ func TestIDPSubroutine_Process(t *testing.T) { orgsClient := mocks.NewMockClient(t) mgr := mocks.NewMockManager(t) cluster := mocks.NewMockCluster(t) - cfg := &config.Config{ - BaseDomain: "example.com", - IDP: struct { - SMTPServer string `mapstructure:"idp-smtp-server"` - SMTPPort int `mapstructure:"idp-smtp-port"` - FromAddress string `mapstructure:"idp-from-address"` - SSL bool `mapstructure:"idp-smtp-ssl" default:"false"` - StartTLS bool `mapstructure:"idp-smtp-starttls" default:"false"` - SMTPUser string `mapstructure:"idp-smtp-user"` - SMTPPassword string `mapstructure:"idp-smtp-password"` - AdditionalRedirectURLs []string `mapstructure:"idp-additional-redirect-urls"` - }{ - AdditionalRedirectURLs: []string{}, - }, - } + cfg := &config.Config{} + cfg.IDP.AdditionalRedirectURLs = []string{} subroutine := NewIDPSubroutine(orgsClient, mgr, cfg, "example.com") tt.setupMocks(orgsClient, mgr, cluster, cfg) @@ -296,4 +271,3 @@ func TestIDPSubroutine_Process(t *testing.T) { }) } } - From 255dc4fb10bf89a7c8661d511b266719bdbfa70d Mon Sep 17 00:00:00 2001 From: OlegErshov Date: Wed, 17 Dec 2025 12:29:01 +0100 Subject: [PATCH 03/22] feat: added workspace type patching On-behalf-of: SAP aleh.yarshou@sap.com --- .../subroutine/worksapce_authorization.go | 33 +++- .../workspace_authorization_test.go | 159 +++++++++++++++++- 2 files changed, 181 insertions(+), 11 deletions(-) diff --git a/internal/subroutine/worksapce_authorization.go b/internal/subroutine/worksapce_authorization.go index 8194da7..6d67eed 100644 --- a/internal/subroutine/worksapce_authorization.go +++ b/internal/subroutine/worksapce_authorization.go @@ -20,14 +20,14 @@ import ( ) type workspaceAuthSubroutine struct { - client client.Client + orgClient client.Client runtimeClient client.Client cfg config.Config } -func NewWorkspaceAuthConfigurationSubroutine(client, runtimeClient client.Client, cfg config.Config) *workspaceAuthSubroutine { +func NewWorkspaceAuthConfigurationSubroutine(orgClient, runtimeClient client.Client, cfg config.Config) *workspaceAuthSubroutine { return &workspaceAuthSubroutine{ - client: client, + orgClient: orgClient, runtimeClient: runtimeClient, cfg: cfg, } @@ -62,7 +62,7 @@ func (r *workspaceAuthSubroutine) Process(ctx context.Context, instance runtimeo } obj := &kcptenancyv1alphav1.WorkspaceAuthenticationConfiguration{ObjectMeta: metav1.ObjectMeta{Name: workspaceName}} - _, err := controllerutil.CreateOrUpdate(ctx, r.client, obj, func() error { + _, err := controllerutil.CreateOrUpdate(ctx, r.orgClient, obj, func() error { obj.Spec = kcptenancyv1alphav1.WorkspaceAuthenticationConfigurationSpec{ JWT: []kcptenancyv1alphav1.JWTAuthenticator{ { @@ -95,5 +95,30 @@ func (r *workspaceAuthSubroutine) Process(ctx context.Context, instance runtimeo return reconcile.Result{}, errors.NewOperatorError(fmt.Errorf("failed to create WorkspaceAuthConfiguration resource: %w", err), true, true) } + err = r.patchWorkspaceType(ctx, r.orgClient, fmt.Sprintf("%s-org", workspaceName), workspaceName) + if err != nil { + return reconcile.Result{}, errors.NewOperatorError(fmt.Errorf("failed to create patch workspace type resource: %w", err), true, true) + } + + err = r.patchWorkspaceType(ctx, r.orgClient, fmt.Sprintf("%s-acc", workspaceName), workspaceName) + if err != nil { + return reconcile.Result{}, errors.NewOperatorError(fmt.Errorf("failed to create patch workspace type resource: %w", err), true, true) + } + return ctrl.Result{}, nil } + +func (r *workspaceAuthSubroutine) patchWorkspaceType(ctx context.Context, client client.Client, workspaceTypeName, authConfigurationRefName string) error { + wsType := kcptenancyv1alphav1.WorkspaceType{ + ObjectMeta: metav1.ObjectMeta{Name: workspaceTypeName}, + } + _, err := controllerutil.CreateOrUpdate(ctx, client, &wsType, func() error { + wsType.Spec.AuthenticationConfigurations = []kcptenancyv1alphav1.AuthenticationConfigurationReference{{Name: authConfigurationRefName}} + return nil + }) + if err != nil { + return err + } + + return nil +} diff --git a/internal/subroutine/workspace_authorization_test.go b/internal/subroutine/workspace_authorization_test.go index b32a664..48ff119 100644 --- a/internal/subroutine/workspace_authorization_test.go +++ b/internal/subroutine/workspace_authorization_test.go @@ -42,7 +42,6 @@ func TestWorkspaceAuthSubroutine_Process(t *testing.T) { setupMocks: func(m *mocks.MockClient) { m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "test-workspace"}, mock.AnythingOfType("*v1alpha1.WorkspaceAuthenticationConfiguration"), mock.Anything). Return(apierrors.NewNotFound(kcptenancyv1alphav1.Resource("workspaceauthenticationconfigurations"), "test-workspace")).Once() - m.EXPECT().Create(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceAuthenticationConfiguration"), mock.Anything). RunAndReturn(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { wac := obj.(*kcptenancyv1alphav1.WorkspaceAuthenticationConfiguration) @@ -53,6 +52,26 @@ func TestWorkspaceAuthSubroutine_Process(t *testing.T) { assert.Equal(t, "email", wac.Spec.JWT[0].ClaimMappings.Username.Claim) return nil }).Once() + + m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "test-workspace-org"}, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). + Return(apierrors.NewNotFound(kcptenancyv1alphav1.Resource("workspacetypes"), "test-workspace-org")).Once() + m.EXPECT().Create(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). + RunAndReturn(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + wt := obj.(*kcptenancyv1alphav1.WorkspaceType) + assert.Equal(t, "test-workspace-org", wt.Name) + assert.Equal(t, "test-workspace", wt.Spec.AuthenticationConfigurations[0].Name) + return nil + }).Once() + + m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "test-workspace-acc"}, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). + Return(apierrors.NewNotFound(kcptenancyv1alphav1.Resource("workspacetypes"), "test-workspace-acc")).Once() + m.EXPECT().Create(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). + RunAndReturn(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + wt := obj.(*kcptenancyv1alphav1.WorkspaceType) + assert.Equal(t, "test-workspace-acc", wt.Name) + assert.Equal(t, "test-workspace", wt.Spec.AuthenticationConfigurations[0].Name) + return nil + }).Once() }, expectError: false, expectedResult: ctrl.Result{}, @@ -83,7 +102,6 @@ func TestWorkspaceAuthSubroutine_Process(t *testing.T) { } return nil }).Once() - m.EXPECT().Update(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceAuthenticationConfiguration"), mock.Anything). RunAndReturn(func(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error { wac := obj.(*kcptenancyv1alphav1.WorkspaceAuthenticationConfiguration) @@ -91,6 +109,26 @@ func TestWorkspaceAuthSubroutine_Process(t *testing.T) { assert.Equal(t, "https://example.com/keycloak/realms/existing-workspace", wac.Spec.JWT[0].Issuer.URL) return nil }).Once() + + m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "existing-workspace-org"}, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). + Return(apierrors.NewNotFound(kcptenancyv1alphav1.Resource("workspacetypes"), "existing-workspace-org")).Once() + m.EXPECT().Create(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). + RunAndReturn(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + wt := obj.(*kcptenancyv1alphav1.WorkspaceType) + assert.Equal(t, "existing-workspace-org", wt.Name) + assert.Equal(t, "existing-workspace", wt.Spec.AuthenticationConfigurations[0].Name) + return nil + }).Once() + + m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "existing-workspace-acc"}, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). + Return(apierrors.NewNotFound(kcptenancyv1alphav1.Resource("workspacetypes"), "existing-workspace-acc")).Once() + m.EXPECT().Create(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). + RunAndReturn(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + wt := obj.(*kcptenancyv1alphav1.WorkspaceType) + assert.Equal(t, "existing-workspace-acc", wt.Name) + assert.Equal(t, "existing-workspace", wt.Spec.AuthenticationConfigurations[0].Name) + return nil + }).Once() }, expectError: false, expectedResult: ctrl.Result{}, @@ -134,7 +172,6 @@ func TestWorkspaceAuthSubroutine_Process(t *testing.T) { setupMocks: func(m *mocks.MockClient) { m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "test-workspace"}, mock.AnythingOfType("*v1alpha1.WorkspaceAuthenticationConfiguration"), mock.Anything). Return(apierrors.NewNotFound(kcptenancyv1alphav1.Resource("workspaceauthenticationconfigurations"), "test-workspace")).Once() - m.EXPECT().Create(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceAuthenticationConfiguration"), mock.Anything). Return(errors.New("create failed")).Once() }, @@ -158,7 +195,6 @@ func TestWorkspaceAuthSubroutine_Process(t *testing.T) { wac.Name = "test-workspace" return nil }).Once() - m.EXPECT().Update(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceAuthenticationConfiguration"), mock.Anything). Return(errors.New("update failed")).Once() }, @@ -195,7 +231,6 @@ func TestWorkspaceAuthSubroutine_Process(t *testing.T) { setupMocks: func(m *mocks.MockClient) { m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "single-workspace"}, mock.AnythingOfType("*v1alpha1.WorkspaceAuthenticationConfiguration"), mock.Anything). Return(apierrors.NewNotFound(kcptenancyv1alphav1.Resource("workspaceauthenticationconfigurations"), "single-workspace")).Once() - m.EXPECT().Create(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceAuthenticationConfiguration"), mock.Anything). RunAndReturn(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { wac := obj.(*kcptenancyv1alphav1.WorkspaceAuthenticationConfiguration) @@ -203,6 +238,26 @@ func TestWorkspaceAuthSubroutine_Process(t *testing.T) { assert.Equal(t, "https://test.domain/keycloak/realms/single-workspace", wac.Spec.JWT[0].Issuer.URL) return nil }).Once() + + m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "single-workspace-org"}, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). + Return(apierrors.NewNotFound(kcptenancyv1alphav1.Resource("workspacetypes"), "single-workspace-org")).Once() + m.EXPECT().Create(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). + RunAndReturn(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + wt := obj.(*kcptenancyv1alphav1.WorkspaceType) + assert.Equal(t, "single-workspace-org", wt.Name) + assert.Equal(t, "single-workspace", wt.Spec.AuthenticationConfigurations[0].Name) + return nil + }).Once() + + m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "single-workspace-acc"}, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). + Return(apierrors.NewNotFound(kcptenancyv1alphav1.Resource("workspacetypes"), "single-workspace-acc")).Once() + m.EXPECT().Create(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). + RunAndReturn(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + wt := obj.(*kcptenancyv1alphav1.WorkspaceType) + assert.Equal(t, "single-workspace-acc", wt.Name) + assert.Equal(t, "single-workspace", wt.Spec.AuthenticationConfigurations[0].Name) + return nil + }).Once() }, expectError: false, expectedResult: ctrl.Result{}, @@ -223,7 +278,6 @@ func TestWorkspaceAuthSubroutine_Process(t *testing.T) { DomainCALookup: true, }, setupMocks: func(m *mocks.MockClient) { - m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "domain-certificate-ca", Namespace: "platform-mesh-system"}, mock.Anything, mock.Anything). RunAndReturn(func(ctx context.Context, key types.NamespacedName, obj client.Object, opts ...client.GetOption) error { secret := obj.(*corev1.Secret) @@ -235,7 +289,6 @@ func TestWorkspaceAuthSubroutine_Process(t *testing.T) { m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "single-workspace"}, mock.AnythingOfType("*v1alpha1.WorkspaceAuthenticationConfiguration"), mock.Anything). Return(apierrors.NewNotFound(kcptenancyv1alphav1.Resource("workspaceauthenticationconfigurations"), "single-workspace")).Once() - m.EXPECT().Create(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceAuthenticationConfiguration"), mock.Anything). RunAndReturn(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { wac := obj.(*kcptenancyv1alphav1.WorkspaceAuthenticationConfiguration) @@ -243,10 +296,102 @@ func TestWorkspaceAuthSubroutine_Process(t *testing.T) { assert.Equal(t, "https://test.domain/keycloak/realms/single-workspace", wac.Spec.JWT[0].Issuer.URL) return nil }).Once() + + m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "single-workspace-org"}, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). + Return(apierrors.NewNotFound(kcptenancyv1alphav1.Resource("workspacetypes"), "single-workspace-org")).Once() + m.EXPECT().Create(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). + RunAndReturn(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + wt := obj.(*kcptenancyv1alphav1.WorkspaceType) + assert.Equal(t, "single-workspace-org", wt.Name) + assert.Equal(t, "single-workspace", wt.Spec.AuthenticationConfigurations[0].Name) + return nil + }).Once() + + m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "single-workspace-acc"}, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). + Return(apierrors.NewNotFound(kcptenancyv1alphav1.Resource("workspacetypes"), "single-workspace-acc")).Once() + m.EXPECT().Create(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). + RunAndReturn(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + wt := obj.(*kcptenancyv1alphav1.WorkspaceType) + assert.Equal(t, "single-workspace-acc", wt.Name) + assert.Equal(t, "single-workspace", wt.Spec.AuthenticationConfigurations[0].Name) + return nil + }).Once() }, expectError: false, expectedResult: ctrl.Result{}, }, + { + name: "error - patchWorkspaceType fails for -org", + logicalCluster: &kcpv1alpha1.LogicalCluster{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "kcp.io/path": "root:orgs:test-workspace", + }, + }, + }, + cfg: config.Config{BaseDomain: "test.domain", GroupClaim: "groups", UserClaim: "email"}, + setupMocks: func(m *mocks.MockClient) { + m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "test-workspace"}, mock.AnythingOfType("*v1alpha1.WorkspaceAuthenticationConfiguration"), mock.Anything). + Return(apierrors.NewNotFound(kcptenancyv1alphav1.Resource("workspaceauthenticationconfigurations"), "test-workspace")).Once() + m.EXPECT().Create(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceAuthenticationConfiguration"), mock.Anything).Return(nil).Once() + + m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "test-workspace-org"}, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). + Return(apierrors.NewNotFound(kcptenancyv1alphav1.Resource("workspacetypes"), "test-workspace-org")).Once() + m.EXPECT().Create(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). + Return(errors.New("failed to create workspace type")).Once() + }, + expectError: true, + expectedResult: ctrl.Result{}, + }, + { + name: "error - patchWorkspaceType fails for -acc", + logicalCluster: &kcpv1alpha1.LogicalCluster{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "kcp.io/path": "root:orgs:test-workspace", + }, + }, + }, + cfg: config.Config{BaseDomain: "test.domain", GroupClaim: "groups", UserClaim: "email"}, + setupMocks: func(m *mocks.MockClient) { + m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "test-workspace"}, mock.AnythingOfType("*v1alpha1.WorkspaceAuthenticationConfiguration"), mock.Anything). + Return(apierrors.NewNotFound(kcptenancyv1alphav1.Resource("workspaceauthenticationconfigurations"), "test-workspace")).Once() + m.EXPECT().Create(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceAuthenticationConfiguration"), mock.Anything).Return(nil).Once() + + m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "test-workspace-org"}, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). + Return(apierrors.NewNotFound(kcptenancyv1alphav1.Resource("workspacetypes"), "test-workspace-org")).Once() + m.EXPECT().Create(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything).Return(nil).Once() + + m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "test-workspace-acc"}, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). + Return(apierrors.NewNotFound(kcptenancyv1alphav1.Resource("workspacetypes"), "test-workspace-acc")).Once() + m.EXPECT().Create(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). + Return(errors.New("failed to create workspace type")).Once() + }, + expectError: true, + expectedResult: ctrl.Result{}, + }, + { + name: "error - domain CA secret Get fails", + logicalCluster: &kcpv1alpha1.LogicalCluster{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "kcp.io/path": "root:orgs:test-workspace", + }, + }, + }, + cfg: config.Config{ + BaseDomain: "test.domain", + GroupClaim: "groups", + UserClaim: "email", + DomainCALookup: true, + }, + setupMocks: func(m *mocks.MockClient) { + m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "domain-certificate-ca", Namespace: "platform-mesh-system"}, mock.Anything, mock.Anything). + Return(errors.New("failed to get domain CA secret")).Once() + }, + expectError: true, + expectedResult: ctrl.Result{}, + }, } for _, tt := range tests { From a2f013a71e1d09eb85f1c2b2d5a585fd71351303 Mon Sep 17 00:00:00 2001 From: OlegErshov Date: Wed, 17 Dec 2025 12:33:29 +0100 Subject: [PATCH 04/22] feat: removed secret waiting functionality On-behalf-of: SAP aleh.yarshou@sap.com --- internal/subroutine/remove_initializer.go | 40 ++--------- .../subroutine/remove_initializer_test.go | 70 ++----------------- 2 files changed, 10 insertions(+), 100 deletions(-) diff --git a/internal/subroutine/remove_initializer.go b/internal/subroutine/remove_initializer.go index 1810a19..833921b 100644 --- a/internal/subroutine/remove_initializer.go +++ b/internal/subroutine/remove_initializer.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "slices" - "time" "github.com/kcp-dev/kcp/sdk/apis/cache/initialization" kcpv1alpha1 "github.com/kcp-dev/kcp/sdk/apis/core/v1alpha1" @@ -13,9 +12,6 @@ import ( "github.com/platform-mesh/golang-commons/errors" "github.com/platform-mesh/security-operator/internal/config" "github.com/rs/zerolog/log" - corev1 "k8s.io/api/core/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" mcmanager "sigs.k8s.io/multicluster-runtime/pkg/manager" @@ -26,10 +22,8 @@ const ( ) type removeInitializer struct { - initializerName string - mgr mcmanager.Manager - runtimeClient client.Client - secretWaitTimeout time.Duration + initializerName string + mgr mcmanager.Manager } // Finalize implements subroutine.Subroutine. @@ -59,28 +53,6 @@ func (r *removeInitializer) Process(ctx context.Context, instance runtimeobject. return ctrl.Result{}, nil } - // we need to wait until keycloak crossplane provider creates a portal secret - workspaceName := getWorkspaceName(lc) - if workspaceName == "" { - return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to get workspace path"), true, false) - } - - secretName := fmt.Sprintf("portal-client-secret-%s", workspaceName) - key := types.NamespacedName{Name: secretName, Namespace: PortalClientSecretNamespace} - - var secret corev1.Secret - if err := r.runtimeClient.Get(ctx, key, &secret); err != nil { - if apierrors.IsNotFound(err) { - age := time.Since(lc.CreationTimestamp.Time) - if age <= r.secretWaitTimeout { - log.Info().Str("secret", secretName).Msg("portal secret not ready yet, requeueing") - return ctrl.Result{RequeueAfter: 5 * time.Second}, nil - } - return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("portal client secret %s was not created within %s", secretName, r.secretWaitTimeout.String()), true, true) - } - return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to get secret %s: %w", secretName, err), true, true) - } - patch := client.MergeFrom(lc.DeepCopy()) lc.Status.Initializers = initialization.EnsureInitializerAbsent(initializer, lc.Status.Initializers) @@ -93,12 +65,10 @@ func (r *removeInitializer) Process(ctx context.Context, instance runtimeobject. return ctrl.Result{}, nil } -func NewRemoveInitializer(mgr mcmanager.Manager, cfg config.Config, runtimeClient client.Client) *removeInitializer { +func NewRemoveInitializer(mgr mcmanager.Manager, cfg config.Config) *removeInitializer { return &removeInitializer{ - initializerName: cfg.InitializerName, - mgr: mgr, - runtimeClient: runtimeClient, - secretWaitTimeout: time.Duration(cfg.SecretWaitingTimeoutInSeconds) * time.Second, + initializerName: cfg.InitializerName, + mgr: mgr, } } diff --git a/internal/subroutine/remove_initializer_test.go b/internal/subroutine/remove_initializer_test.go index 8bdca84..b525bfd 100644 --- a/internal/subroutine/remove_initializer_test.go +++ b/internal/subroutine/remove_initializer_test.go @@ -3,7 +3,6 @@ package subroutine_test import ( "context" "testing" - "time" kcpv1alpha1 "github.com/kcp-dev/kcp/sdk/apis/core/v1alpha1" "github.com/platform-mesh/security-operator/internal/config" @@ -11,9 +10,6 @@ import ( "github.com/platform-mesh/security-operator/internal/subroutine/mocks" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" - apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -48,14 +44,13 @@ func TestRemoveInitializer_Process(t *testing.T) { t.Run("skips when initializer is absent", func(t *testing.T) { mgr := mocks.NewMockManager(t) - runtimeClient := mocks.NewMockClient(t) cluster := mocks.NewMockCluster(t) mgr.EXPECT().ClusterFromContext(mock.Anything).Return(cluster, nil) lc := &kcpv1alpha1.LogicalCluster{} lc.Status.Initializers = []kcpv1alpha1.LogicalClusterInitializer{"other.initializer"} - r := subroutine.NewRemoveInitializer(mgr, config.Config{InitializerName: initializerName, SecretWaitingTimeoutInSeconds: 60}, runtimeClient) + r := subroutine.NewRemoveInitializer(mgr, config.Config{InitializerName: initializerName}) _, err := r.Process(context.Background(), lc) assert.Nil(t, err) }) @@ -63,12 +58,9 @@ func TestRemoveInitializer_Process(t *testing.T) { t.Run("removes initializer and patches status", func(t *testing.T) { mgr := mocks.NewMockManager(t) cluster := mocks.NewMockCluster(t) - runtimeClient := mocks.NewMockClient(t) k8s := mocks.NewMockClient(t) mgr.EXPECT().ClusterFromContext(mock.Anything).Return(cluster, nil) - // Secret must exist for the flow to proceed - runtimeClient.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "portal-client-secret-test", Namespace: subroutine.PortalClientSecretNamespace}, mock.Anything).Return(nil) cluster.EXPECT().GetClient().Return(k8s) k8s.EXPECT().Status().Return(&fakeStatusWriter{t: t, expectClear: kcpv1alpha1.LogicalClusterInitializer(initializerName), err: nil}) @@ -77,12 +69,10 @@ func TestRemoveInitializer_Process(t *testing.T) { kcpv1alpha1.LogicalClusterInitializer(initializerName), "another.initializer", } - lc.Annotations = map[string]string{"kcp.io/path": "root:org:test"} - r := subroutine.NewRemoveInitializer(mgr, config.Config{InitializerName: initializerName, SecretWaitingTimeoutInSeconds: 60}, runtimeClient) + r := subroutine.NewRemoveInitializer(mgr, config.Config{InitializerName: initializerName}) _, err := r.Process(context.Background(), lc) assert.Nil(t, err) - // ensure it's removed in in-memory object as well for _, init := range lc.Status.Initializers { assert.NotEqual(t, initializerName, string(init)) } @@ -91,12 +81,9 @@ func TestRemoveInitializer_Process(t *testing.T) { t.Run("returns error when status patch fails", func(t *testing.T) { mgr := mocks.NewMockManager(t) cluster := mocks.NewMockCluster(t) - runtimeClient := mocks.NewMockClient(t) k8s := mocks.NewMockClient(t) mgr.EXPECT().ClusterFromContext(mock.Anything).Return(cluster, nil) - // Secret exists so we hit the patch failure path - runtimeClient.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "portal-client-secret-test", Namespace: subroutine.PortalClientSecretNamespace}, mock.Anything).Return(nil) cluster.EXPECT().GetClient().Return(k8s) k8s.EXPECT().Status().Return(&fakeStatusWriter{t: t, expectClear: kcpv1alpha1.LogicalClusterInitializer(initializerName), err: assert.AnError}) @@ -104,52 +91,8 @@ func TestRemoveInitializer_Process(t *testing.T) { lc.Status.Initializers = []kcpv1alpha1.LogicalClusterInitializer{ kcpv1alpha1.LogicalClusterInitializer(initializerName), } - lc.Annotations = map[string]string{"kcp.io/path": "root:org:test"} - r := subroutine.NewRemoveInitializer(mgr, config.Config{InitializerName: initializerName, SecretWaitingTimeoutInSeconds: 60}, runtimeClient) - _, err := r.Process(context.Background(), lc) - assert.NotNil(t, err) - }) - - t.Run("requeues when secret not found under 1 minute", func(t *testing.T) { - mgr := mocks.NewMockManager(t) - cluster := mocks.NewMockCluster(t) - runtimeClient := mocks.NewMockClient(t) - - mgr.EXPECT().ClusterFromContext(mock.Anything).Return(cluster, nil) - // Simulate NotFound error - runtimeClient.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "portal-client-secret-test", Namespace: subroutine.PortalClientSecretNamespace}, mock.Anything).Return(apierrors.NewNotFound(schema.GroupResource{Group: "", Resource: "secrets"}, "portal-client-secret-test")) - - lc := &kcpv1alpha1.LogicalCluster{} - lc.Status.Initializers = []kcpv1alpha1.LogicalClusterInitializer{ - kcpv1alpha1.LogicalClusterInitializer(initializerName), - } - lc.Annotations = map[string]string{"kcp.io/path": "root:org:test"} - lc.CreationTimestamp.Time = time.Now().Add(-30 * time.Second) - - r := subroutine.NewRemoveInitializer(mgr, config.Config{InitializerName: initializerName, SecretWaitingTimeoutInSeconds: 60}, runtimeClient) - res, err := r.Process(context.Background(), lc) - assert.Nil(t, err) - assert.Equal(t, 5*time.Second, res.RequeueAfter) - }) - - t.Run("errors when secret not found after 1 minute", func(t *testing.T) { - mgr := mocks.NewMockManager(t) - cluster := mocks.NewMockCluster(t) - runtimeClient := mocks.NewMockClient(t) - - mgr.EXPECT().ClusterFromContext(mock.Anything).Return(cluster, nil) - // Simulate NotFound error - runtimeClient.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "portal-client-secret-test", Namespace: subroutine.PortalClientSecretNamespace}, mock.Anything).Return(apierrors.NewNotFound(schema.GroupResource{Group: "", Resource: "secrets"}, "portal-client-secret-test")) - - lc := &kcpv1alpha1.LogicalCluster{} - lc.Status.Initializers = []kcpv1alpha1.LogicalClusterInitializer{ - kcpv1alpha1.LogicalClusterInitializer(initializerName), - } - lc.Annotations = map[string]string{"kcp.io/path": "root:org:test"} - lc.CreationTimestamp.Time = time.Now().Add(-2 * time.Minute) - - r := subroutine.NewRemoveInitializer(mgr, config.Config{InitializerName: initializerName, SecretWaitingTimeoutInSeconds: 60}, runtimeClient) + r := subroutine.NewRemoveInitializer(mgr, config.Config{InitializerName: initializerName}) _, err := r.Process(context.Background(), lc) assert.NotNil(t, err) }) @@ -157,8 +100,7 @@ func TestRemoveInitializer_Process(t *testing.T) { func TestRemoveInitializer_Misc(t *testing.T) { mgr := mocks.NewMockManager(t) - runtimeClient := mocks.NewMockClient(t) - r := subroutine.NewRemoveInitializer(mgr, config.Config{InitializerName: "foo.initializer.kcp.dev", SecretWaitingTimeoutInSeconds: 60}, runtimeClient) + r := subroutine.NewRemoveInitializer(mgr, config.Config{InitializerName: "foo.initializer.kcp.dev"}) assert.Equal(t, "RemoveInitializer", r.GetName()) assert.Equal(t, []string{}, r.Finalizers(nil)) @@ -169,11 +111,9 @@ func TestRemoveInitializer_Misc(t *testing.T) { func TestRemoveInitializer_ManagerError(t *testing.T) { mgr := mocks.NewMockManager(t) - // Simulate error fetching cluster from context mgr.EXPECT().ClusterFromContext(mock.Anything).Return(nil, assert.AnError) - runtimeClient := mocks.NewMockClient(t) - r := subroutine.NewRemoveInitializer(mgr, config.Config{InitializerName: "foo.initializer.kcp.dev", SecretWaitingTimeoutInSeconds: 60}, runtimeClient) + r := subroutine.NewRemoveInitializer(mgr, config.Config{InitializerName: "foo.initializer.kcp.dev"}) _, err := r.Process(context.Background(), &kcpv1alpha1.LogicalCluster{}) assert.NotNil(t, err) } From 17ca9c547b0f5506e9f5c6a2ef2c5dbbc8cab867 Mon Sep 17 00:00:00 2001 From: OlegErshov Date: Wed, 17 Dec 2025 12:40:46 +0100 Subject: [PATCH 05/22] feat: removed realm subroutine On-behalf-of: SAP aleh.yarshou@sap.com --- internal/controller/initializer_controller.go | 5 +- internal/subroutine/idp.go | 9 + internal/subroutine/realm.go | 221 ------------- internal/subroutine/realm_test.go | 303 ------------------ 4 files changed, 12 insertions(+), 526 deletions(-) delete mode 100644 internal/subroutine/realm.go delete mode 100644 internal/subroutine/realm_test.go diff --git a/internal/controller/initializer_controller.go b/internal/controller/initializer_controller.go index 8aa3d5a..6a00ac0 100644 --- a/internal/controller/initializer_controller.go +++ b/internal/controller/initializer_controller.go @@ -32,11 +32,12 @@ func NewLogicalClusterReconciler(log *logger.Logger, orgClient client.Client, cf mclifecycle: builder.NewBuilder("logicalcluster", "LogicalClusterReconciler", []lifecyclesubroutine.Subroutine{ subroutine.NewWorkspaceInitializer(orgClient, cfg, mgr), subroutine.NewWorkspaceAuthConfigurationSubroutine(orgClient, inClusterClient, cfg), - subroutine.NewRealmSubroutine(inClusterClient, &cfg, cfg.BaseDomain), + subroutine.NewIDPSubroutine(orgClient, mgr, &cfg, cfg.BaseDomain), subroutine.NewInviteSubroutine(orgClient, mgr), - subroutine.NewRemoveInitializer(mgr, cfg, inClusterClient), + subroutine.NewRemoveInitializer(mgr, cfg), }, log). WithReadOnly(). + WithStaticThenExponentialRateLimiter(). BuildMultiCluster(mgr), } } diff --git a/internal/subroutine/idp.go b/internal/subroutine/idp.go index ddb41ac..c95cabf 100644 --- a/internal/subroutine/idp.go +++ b/internal/subroutine/idp.go @@ -3,6 +3,7 @@ package subroutine import ( "context" "fmt" + "strings" kcpv1alpha1 "github.com/kcp-dev/kcp/sdk/apis/core/v1alpha1" accountv1alpha1 "github.com/platform-mesh/account-operator/api/v1alpha1" @@ -121,3 +122,11 @@ func (w *IDPSubroutine) Process(ctx context.Context, instance runtimeobject.Runt log.Info().Str("workspace", workspaceName).Msg("idp resource is ready") return ctrl.Result{}, nil } + +func getWorkspaceName(lc *kcpv1alpha1.LogicalCluster) string { + if path, ok := lc.Annotations["kcp.io/path"]; ok { + pathElements := strings.Split(path, ":") + return pathElements[len(pathElements)-1] + } + return "" +} \ No newline at end of file diff --git a/internal/subroutine/realm.go b/internal/subroutine/realm.go deleted file mode 100644 index 6a32aab..0000000 --- a/internal/subroutine/realm.go +++ /dev/null @@ -1,221 +0,0 @@ -package subroutine - -import ( - "bytes" - "context" - _ "embed" - "encoding/json" - "fmt" - "strings" - "text/template" - - kcpv1alpha1 "github.com/kcp-dev/kcp/sdk/apis/core/v1alpha1" - "github.com/platform-mesh/golang-commons/controller/lifecycle/runtimeobject" - lifecyclesubroutine "github.com/platform-mesh/golang-commons/controller/lifecycle/subroutine" - "github.com/platform-mesh/golang-commons/errors" - "github.com/platform-mesh/golang-commons/logger" - "github.com/platform-mesh/security-operator/internal/config" - apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/reconcile" - "sigs.k8s.io/yaml" -) - -var ( - - //go:embed manifests/organizationIdp/helmrelease.yaml - helmRelease string -) - -type realmSubroutine struct { - k8s client.Client - baseDomain string - cfg *config.Config -} - -func NewRealmSubroutine(k8s client.Client, cfg *config.Config, baseDomain string) *realmSubroutine { - return &realmSubroutine{ - k8s, - baseDomain, - cfg, - } -} - -var _ lifecyclesubroutine.Subroutine = &realmSubroutine{} - -func (r *realmSubroutine) GetName() string { return "Realm" } - -func (r *realmSubroutine) Finalizers(_ runtimeobject.RuntimeObject) []string { - return []string{} -} - -func (r *realmSubroutine) Finalize(ctx context.Context, instance runtimeobject.RuntimeObject) (reconcile.Result, errors.OperatorError) { - log := logger.LoadLoggerFromContext(ctx) - - lc := instance.(*kcpv1alpha1.LogicalCluster) - workspaceName := getWorkspaceName(lc) - if workspaceName == "" { - return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to get workspace path"), true, false) - } - - helmObj, err := unstructuredFromString(helmRelease, nil, log) - if err != nil { - return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to load HelmRelease manifest: %w", err), true, true) - } - helmObj.SetName(workspaceName) - if err := r.k8s.Delete(ctx, &helmObj); err != nil { - return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to delete HelmRelease: %w", err), true, true) - } - - log.Info().Str("realm", workspaceName).Msg("Successfully finalized resources") - return ctrl.Result{}, nil -} - -func (r *realmSubroutine) Process(ctx context.Context, instance runtimeobject.RuntimeObject) (reconcile.Result, errors.OperatorError) { - lc := instance.(*kcpv1alpha1.LogicalCluster) - - workspaceName := getWorkspaceName(lc) - if workspaceName == "" { - return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to get workspace path"), true, false) - } - - patch := map[string]any{ - "crossplane": map[string]any{ - "realm": map[string]any{ - "name": workspaceName, - "displayName": workspaceName, - }, - "client": map[string]any{ - "name": workspaceName, - "displayName": workspaceName, - "validRedirectUris": append(r.cfg.IDP.AdditionalRedirectURLs, fmt.Sprintf("https://%s.%s/*", workspaceName, r.baseDomain)), - }, - "organization": map[string]any{ - "domain": "example.com", // TODO: change - }, - }, - "keycloakConfig": map[string]any{ - "client": map[string]any{ - "name": workspaceName, - "targetSecret": map[string]any{ - "name": fmt.Sprintf("portal-client-secret-%s", workspaceName), - }, - }, - }, - } - - if r.cfg.IDP.SMTPServer != "" { - - smtpConfig := map[string]any{ - "host": r.cfg.IDP.SMTPServer, - "port": fmt.Sprintf("%d", r.cfg.IDP.SMTPPort), - "from": r.cfg.IDP.FromAddress, - "ssl": r.cfg.IDP.SSL, - "starttls": r.cfg.IDP.StartTLS, - } - - if r.cfg.IDP.SMTPUser != "" { - smtpConfig["auth"] = map[string]any{ - "username": r.cfg.IDP.SMTPUser, - "passwordSecretRef": map[string]any{ - "namespace": "platform-mesh-system", - "name": r.cfg.IDP.SMTPPasswordSecretName, - "key": r.cfg.IDP.SMTPPasswordSecretKey, - }, - } - } - - err := unstructured.SetNestedField(patch, []any{smtpConfig}, "crossplane", "realm", "smtpConfig") - if err != nil { - return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to set SMTP server config: %w", err), true, true) - } - } - - marshalledPatch, err := json.Marshal(patch) - if err != nil { - return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to marshall patch map: %w", err), true, true) - } - - values := apiextensionsv1.JSON{Raw: marshalledPatch} - releaseName := fmt.Sprintf("%s-idp", workspaceName) - - err = applyReleaseWithValues(ctx, helmRelease, r.k8s, values, releaseName) - if err != nil { - return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to create HelmRelease: %w", err), true, true) - } - - return ctrl.Result{}, nil -} - -func getWorkspaceName(lc *kcpv1alpha1.LogicalCluster) string { - if path, ok := lc.Annotations["kcp.io/path"]; ok { - pathElements := strings.Split(path, ":") - return pathElements[len(pathElements)-1] - } - return "" -} - -func applyReleaseWithValues(ctx context.Context, release string, k8sClient client.Client, values apiextensionsv1.JSON, releaseName string) error { - log := logger.LoadLoggerFromContext(ctx) - - obj, err := unstructuredFromString(release, map[string]string{}, log) - if err != nil { - return errors.Wrap(err, "Failed to get unstructuredFromFile") - } - obj.SetName(releaseName) - - if err := unstructured.SetNestedField(obj.Object, releaseName, "spec", "releaseName"); err != nil { - return errors.Wrap(err, "failed to set spec.releaseName") - } - - obj.Object["spec"].(map[string]interface{})["values"] = values - - err = k8sClient.Patch(ctx, &obj, client.Apply, client.FieldOwner("security-operator")) - if err != nil { - return errors.Wrap(err, "Failed to apply manifest: (%s/%s)", obj.GetKind(), obj.GetName()) - } - return nil -} - -func unstructuredFromString(manifest string, templateData map[string]string, log *logger.Logger) (unstructured.Unstructured, error) { - manifestBytes := []byte(manifest) - - res, err := ReplaceTemplate(templateData, manifestBytes) - if err != nil { - return unstructured.Unstructured{}, errors.Wrap(err, "Failed to replace template") - } - - var objMap map[string]interface{} - if err := yaml.Unmarshal(res, &objMap); err != nil { - return unstructured.Unstructured{}, errors.Wrap(err, "Failed to unmarshal YAML from template. Output:\n%s", string(res)) - } - - log.Debug().Str("obj", fmt.Sprintf("%+v", objMap)).Msg("Unmarshalled object") - - obj := unstructured.Unstructured{Object: objMap} - - log.Debug().Str("kind", obj.GetKind()).Str("name", obj.GetName()).Str("namespace", obj.GetNamespace()).Msg("Applying manifest") - return obj, err -} - -func ReplaceTemplate(templateData map[string]string, templateBytes []byte) ([]byte, error) { - tmpl, err := template.New("manifest").Parse(string(templateBytes)) - if err != nil { - return []byte{}, errors.Wrap(err, "Failed to parse template") - } - var result bytes.Buffer - err = tmpl.Execute(&result, templateData) - if err != nil { - keys := make([]string, 0, len(templateData)) - for k := range templateData { - keys = append(keys, k) - } - return []byte{}, errors.Wrap(err, "Failed to execute template with keys %v", keys) - } - if result.Len() == 0 { - return []byte{}, nil - } - return result.Bytes(), nil -} diff --git a/internal/subroutine/realm_test.go b/internal/subroutine/realm_test.go deleted file mode 100644 index c659566..0000000 --- a/internal/subroutine/realm_test.go +++ /dev/null @@ -1,303 +0,0 @@ -package subroutine - -import ( - "context" - "strings" - "testing" - - kcpv1alpha1 "github.com/kcp-dev/kcp/sdk/apis/core/v1alpha1" - "github.com/platform-mesh/golang-commons/errors" - "github.com/platform-mesh/golang-commons/logger" - "github.com/platform-mesh/security-operator/internal/config" - "github.com/platform-mesh/security-operator/internal/subroutine/mocks" - "github.com/stretchr/testify/mock" - "github.com/stretchr/testify/require" - apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" -) - -const ( - helmReleaseYAML = ` -apiVersion: helm.toolkit.fluxcd.io/v2beta1 -kind: HelmRelease -metadata: - name: helm-min -spec: - releaseName: placeholder - values: {} -` - baseDomain = "portal.dev.local" -) - -func newClientMock(t *testing.T, setup func(m *mocks.MockClient)) *mocks.MockClient { - t.Helper() - m := new(mocks.MockClient) - if setup != nil { - setup(m) - } - t.Cleanup(func() { m.AssertExpectations(t) }) - return m -} - -func testLogger() *logger.Logger { - l, _ := logger.New(logger.DefaultConfig()) - return l -} - -func trim(s string) string { return strings.TrimSpace(s) } - -func TestApplyReleaseAndManifest(t *testing.T) { - cases := []struct { - name string - call string // "release" or "manifest" - content string - setupMocks func(m *mocks.MockClient) - expectErr bool - }{ - {"release: invalid YAML", "release", "not: : valid: yaml", nil, true}, - { - "release: spec scalar", - "release", - trim(` -apiVersion: helm.toolkit.fluxcd.io/v2beta1 -kind: HelmRelease -metadata: - name: test-release - namespace: default -spec: "test spec" -`), - nil, - true, - }, - { - "release: patch error wrapped", - "release", - trim(` -apiVersion: helm.toolkit.fluxcd.io/v2beta1 -kind: HelmRelease -metadata: - name: test-release - namespace: default -spec: - chart: - spec: - chart: mychart - version: 1.2.3 -`), - func(m *mocks.MockClient) { - m.EXPECT().Patch(mock.Anything, mock.Anything, mock.Anything, mock.Anything). - Return(errors.New("simulated patch fail")).Once() - }, - true, - }, - } - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - clientMock := newClientMock(t, tc.setupMocks) - ctx := context.Background() - - switch tc.call { - case "release": - err := applyReleaseWithValues(ctx, tc.content, clientMock, apiextensionsv1.JSON{}, "org-name") - if tc.expectErr { - require.Error(t, err) - } else { - require.NoError(t, err) - } - default: - t.Fatalf("unknown call type %q", tc.call) - } - }) - } -} - -func TestRealmSubroutine_ProcessAndFinalize(t *testing.T) { - origHR := helmRelease - defer func() { helmRelease = origHR }() - - t.Run("Process", func(t *testing.T) { - t.Run("success create repo then helmrelease", func(t *testing.T) { - t.Parallel() - clientMock := newClientMock(t, func(m *mocks.MockClient) { - m.EXPECT().Patch(mock.Anything, mock.Anything, mock.Anything, mock.Anything). - RunAndReturn(func(_ context.Context, obj client.Object, _ client.Patch, _ ...client.PatchOption) error { - hr := obj.(*unstructured.Unstructured) - spec, _, _ := unstructured.NestedFieldNoCopy(hr.Object, "spec") - specMap := spec.(map[string]interface{}) - specValues, _, _ := unstructured.NestedFieldNoCopy(specMap, "values") - _, ok := specValues.(apiextensionsv1.JSON) - require.True(t, ok) - return nil - }).Once() - }) - - helmRelease = trim(helmReleaseYAML) - - rs := NewRealmSubroutine(clientMock, &config.Config{}, baseDomain) - lc := &kcpv1alpha1.LogicalCluster{} - lc.Annotations = map[string]string{"kcp.io/path": "root:orgs:test"} - res, opErr := rs.Process(context.Background(), lc) - require.Nil(t, opErr) - require.Equal(t, ctrl.Result{}, res) - }) - - // New: success create with SMTP config - t.Run("success create with SMTP config", func(t *testing.T) { - t.Parallel() - clientMock := newClientMock(t, func(m *mocks.MockClient) { - m.EXPECT().Patch(mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Once() - }) - - cfg := &config.Config{} - cfg.IDP.SMTPServer = "smtp.example.com" - cfg.IDP.SMTPPort = 587 - cfg.IDP.FromAddress = "noreply@example.com" - cfg.IDP.SSL = false - cfg.IDP.StartTLS = true - rs := NewRealmSubroutine(clientMock, cfg, baseDomain) - lc := &kcpv1alpha1.LogicalCluster{} - lc.Annotations = map[string]string{"kcp.io/path": "root:orgs:test"} - res, opErr := rs.Process(context.Background(), lc) - require.Nil(t, opErr) - require.Equal(t, ctrl.Result{}, res) - }) - - t.Run("helmrelease apply fails", func(t *testing.T) { - t.Parallel() - clientMock := newClientMock(t, func(m *mocks.MockClient) { - m.EXPECT().Patch(mock.Anything, mock.Anything, mock.Anything, mock.Anything). - Return(errors.New("simulated patch failure for HelmRelease")).Once() - }) - - helmRelease = trim(helmReleaseYAML) - rs := NewRealmSubroutine(clientMock, &config.Config{}, baseDomain) - lc := &kcpv1alpha1.LogicalCluster{} - lc.Annotations = map[string]string{"kcp.io/path": "root:orgs:test"} - res, opErr := rs.Process(context.Background(), lc) - require.NotNil(t, opErr) - require.Equal(t, ctrl.Result{}, res) - }) - - // New: Finalize missing workspace annotation - t.Run("missing workspace annotation in Finalize", func(t *testing.T) { - clientMock := newClientMock(t, nil) - rs := NewRealmSubroutine(clientMock, &config.Config{}, baseDomain) - lc := &kcpv1alpha1.LogicalCluster{} - res, opErr := rs.Finalize(context.Background(), lc) - require.NotNil(t, opErr) - require.Equal(t, ctrl.Result{}, res) - }) - - t.Run("missing workspace annotation", func(t *testing.T) { - t.Parallel() - clientMock := newClientMock(t, nil) - rs := NewRealmSubroutine(clientMock, &config.Config{}, baseDomain) - lc := &kcpv1alpha1.LogicalCluster{} - res, opErr := rs.Process(context.Background(), lc) - require.NotNil(t, opErr) - require.Equal(t, ctrl.Result{}, res) - }) - }) - - t.Run("Finalize - delete scenarios", func(t *testing.T) { - t.Parallel() - cases := []struct { - name string - setupMocks func(m *mocks.MockClient) - expectErr bool - expectedResult ctrl.Result - }{ - { - "HelmRelease delete error", - func(m *mocks.MockClient) { - m.EXPECT().Delete(mock.Anything, mock.Anything).Return(errors.New("failed to delete helmRelease")).Once() - }, - true, - ctrl.Result{}, - }, - { - "Delete succeeds", - func(m *mocks.MockClient) { m.EXPECT().Delete(mock.Anything, mock.Anything).Return(nil).Once() }, - false, - ctrl.Result{}, - }, - } - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - clientMock := newClientMock(t, tc.setupMocks) - rs := NewRealmSubroutine(clientMock, &config.Config{}, baseDomain) - lc := &kcpv1alpha1.LogicalCluster{} - lc.Annotations = map[string]string{"kcp.io/path": "root:orgs:test"} - res, opErr := rs.Finalize(context.Background(), lc) - if tc.expectErr { - require.NotNil(t, opErr) - } else { - require.Nil(t, opErr) - } - require.Equal(t, tc.expectedResult, res) - }) - } - }) - -} - -func TestReplaceTemplateAndUnstructured(t *testing.T) { - cases := []struct { - name string - templateData map[string]string - template []byte - expectErr bool - expectOutput string - }{ - {"parse error invalid template", nil, []byte("{{"), true, ""}, - {"empty template yields empty result", map[string]string{}, []byte(""), false, ""}, - {"successful template rendering", map[string]string{"Name": "testing"}, []byte("hello {{ .Name }}"), false, "hello testing"}, - {"execute error indexing missing map", map[string]string{}, []byte(`{{ index .MissingMap "k" }}`), true, ""}, - {"nil template data with static content", nil, []byte("static content"), false, "static content"}, - } - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - out, err := ReplaceTemplate(tc.templateData, tc.template) - if tc.expectErr { - require.Error(t, err) - return - } - require.NoError(t, err) - if tc.expectOutput != "" { - require.Equal(t, tc.expectOutput, string(out)) - } - }) - } - - t.Run("unstructured invalid yaml", func(t *testing.T) { - l := testLogger() - _, err := unstructuredFromString("not: : valid: yaml", nil, l) - require.Error(t, err) - }) - - t.Run("unstructured template success", func(t *testing.T) { - l := testLogger() - templ := "kind: Test\nmetadata:\n name: {{ .Name }}\nspec:\n v: 1" - out, err := ReplaceTemplate(map[string]string{"Name": "templated"}, []byte(templ)) - require.NoError(t, err) - _, err2 := unstructuredFromString(string(out), nil, l) - require.NoError(t, err2) - }) -} - -func TestRealmSubroutine_GetName(t *testing.T) { - r := NewRealmSubroutine(nil, &config.Config{}, "") - require.Equal(t, "Realm", r.GetName()) -} - -func TestRealmSubroutine_Finalizers(t *testing.T) { - r := NewRealmSubroutine(nil, &config.Config{}, "") - require.Equal(t, []string{}, r.Finalizers(nil)) -} From 5c9a8dd84a2944b867575732773984b7d0a85a8f Mon Sep 17 00:00:00 2001 From: OlegErshov Date: Wed, 17 Dec 2025 13:15:15 +0100 Subject: [PATCH 06/22] feat: updated client id fetching On-behalf-of: SAP aleh.yarshou@sap.com --- internal/subroutine/invite/subroutine.go | 32 ++++- internal/subroutine/invite/subroutine_test.go | 109 ++++++++++++++---- 2 files changed, 110 insertions(+), 31 deletions(-) diff --git a/internal/subroutine/invite/subroutine.go b/internal/subroutine/invite/subroutine.go index 5af533e..db6030e 100644 --- a/internal/subroutine/invite/subroutine.go +++ b/internal/subroutine/invite/subroutine.go @@ -9,7 +9,7 @@ import ( "net/url" "github.com/coreos/go-oidc" - corev1alpha1 "github.com/platform-mesh/account-operator/api/v1alpha1" + accountsv1alpha1 "github.com/platform-mesh/account-operator/api/v1alpha1" "github.com/platform-mesh/golang-commons/controller/lifecycle/runtimeobject" lifecyclesubroutine "github.com/platform-mesh/golang-commons/controller/lifecycle/subroutine" "github.com/platform-mesh/golang-commons/errors" @@ -108,7 +108,7 @@ func (s *subroutine) Process(ctx context.Context, instance runtimeobject.Runtime cl := cluster.GetClient() - var accountInfo corev1alpha1.AccountInfo + var accountInfo accountsv1alpha1.AccountInfo if err := cl.Get(ctx, client.ObjectKey{Name: "account"}, &accountInfo); err != nil { log.Err(err).Msg("Failed to get AccountInfo") return ctrl.Result{}, errors.NewOperatorError(err, true, false) @@ -142,8 +142,14 @@ func (s *subroutine) Process(ctx context.Context, instance runtimeobject.Runtime log.Info().Str("email", invite.Spec.Email).Msg("User does not exist, creating user and sending invite") + clientID, err := s.getClientID(ctx, cl, realm) + if err != nil { + log.Err(err).Msg(fmt.Sprintf("failed to get client ID for client %s",realm)) + return ctrl.Result{}, errors.NewOperatorError(err, true, false) + } + clientQueryParams := url.Values{ - "clientId": {realm}, + "clientId": {clientID}, } res, err = s.keycloak.Get(fmt.Sprintf("%s/admin/realms/%s/clients?%s", s.keycloakBaseURL, realm, clientQueryParams.Encode())) @@ -163,11 +169,11 @@ func (s *subroutine) Process(ctx context.Context, instance runtimeobject.Runtime } if len(clients) == 0 { - log.Info().Str("clientId", realm).Msg("Client does not exist yet, requeuing") + log.Info().Str("clientId", clientID).Msg("Client does not exist yet, requeuing") return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("client %s does not exist yet", realm), true, false) } - log.Debug().Str("clientId", realm).Msg("Client verified") + log.Debug().Str("clientId", clientID).Msg("Client verified") // Create user newUser := keycloakUser{ @@ -213,7 +219,7 @@ func (s *subroutine) Process(ctx context.Context, instance runtimeobject.Runtime queryParams := url.Values{ "redirect_uri": {fmt.Sprintf("https://%s.%s/", realm, s.baseDomain)}, - "client_id": {realm}, + "client_id": {clientID}, } req, err := http.NewRequestWithContext(ctx, http.MethodPut, fmt.Sprintf("%s/admin/realms/%s/users/%s/execute-actions-email?%s", s.keycloakBaseURL, realm, newUser.ID, queryParams.Encode()), http.NoBody) @@ -238,3 +244,17 @@ func (s *subroutine) Process(ctx context.Context, instance runtimeobject.Runtime } var _ lifecyclesubroutine.Subroutine = &subroutine{} + +func (s *subroutine) getClientID(ctx context.Context, cl client.Client, clientName string) (string, error) { + idp := &v1alpha1.IdentityProviderConfiguration{} + if err := cl.Get(ctx, client.ObjectKey{Name: clientName}, idp); err != nil { + return "", err + } + + for _, v := range idp.Spec.Clients { + if v.ClientName == clientName { + return v.ClientID, nil + } + } + return "", fmt.Errorf("client not found") +} diff --git a/internal/subroutine/invite/subroutine_test.go b/internal/subroutine/invite/subroutine_test.go index 742f691..fa1630a 100644 --- a/internal/subroutine/invite/subroutine_test.go +++ b/internal/subroutine/invite/subroutine_test.go @@ -67,8 +67,8 @@ func TestSubroutineProcess(t *testing.T) { }, }, setupK8sMocks: func(m *mocks.MockClient) { - m.EXPECT().Get(mock.Anything, mock.Anything, mock.Anything, mock.Anything).RunAndReturn( - func(ctx context.Context, nn types.NamespacedName, o client.Object, opts ...client.GetOption) error { + m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "account"}, mock.AnythingOfType("*v1alpha1.AccountInfo"), mock.Anything). + RunAndReturn(func(ctx context.Context, nn types.NamespacedName, o client.Object, opts ...client.GetOption) error { accountInfo := &accountsv1alpha1.AccountInfo{ Spec: accountsv1alpha1.AccountInfoSpec{ Organization: accountsv1alpha1.AccountLocation{ @@ -78,8 +78,23 @@ func TestSubroutineProcess(t *testing.T) { } *o.(*accountsv1alpha1.AccountInfo) = *accountInfo return nil - }, - ) + }).Once() + + m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "acme"}, mock.AnythingOfType("*v1alpha1.IdentityProviderConfiguration"), mock.Anything). + RunAndReturn(func(ctx context.Context, nn types.NamespacedName, o client.Object, opts ...client.GetOption) error { + idp := &v1alpha1.IdentityProviderConfiguration{ + Spec: v1alpha1.IdentityProviderConfigurationSpec{ + Clients: []v1alpha1.IdentityProviderClientConfig{ + { + ClientName: "acme", + ClientID: "acme", + }, + }, + }, + } + *o.(*v1alpha1.IdentityProviderConfiguration) = *idp + return nil + }).Once() }, setupKeycloakMocks: func(mux *http.ServeMux) { users := []map[string]any{} @@ -122,8 +137,8 @@ func TestSubroutineProcess(t *testing.T) { }, expectErr: true, setupK8sMocks: func(m *mocks.MockClient) { - m.EXPECT().Get(mock.Anything, mock.Anything, mock.Anything, mock.Anything).RunAndReturn( - func(ctx context.Context, nn types.NamespacedName, o client.Object, opts ...client.GetOption) error { + m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "account"}, mock.AnythingOfType("*v1alpha1.AccountInfo"), mock.Anything). + RunAndReturn(func(ctx context.Context, nn types.NamespacedName, o client.Object, opts ...client.GetOption) error { accountInfo := &accountsv1alpha1.AccountInfo{ Spec: accountsv1alpha1.AccountInfoSpec{ Organization: accountsv1alpha1.AccountLocation{ @@ -133,8 +148,7 @@ func TestSubroutineProcess(t *testing.T) { } *o.(*accountsv1alpha1.AccountInfo) = *accountInfo return nil - }, - ) + }).Once() }, setupKeycloakMocks: func(mux *http.ServeMux) { mux.HandleFunc("GET /admin/realms/acme/users", func(w http.ResponseWriter, r *http.Request) { @@ -152,8 +166,8 @@ func TestSubroutineProcess(t *testing.T) { }, expectErr: true, setupK8sMocks: func(m *mocks.MockClient) { - m.EXPECT().Get(mock.Anything, mock.Anything, mock.Anything, mock.Anything).RunAndReturn( - func(ctx context.Context, nn types.NamespacedName, o client.Object, opts ...client.GetOption) error { + m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "account"}, mock.AnythingOfType("*v1alpha1.AccountInfo"), mock.Anything). + RunAndReturn(func(ctx context.Context, nn types.NamespacedName, o client.Object, opts ...client.GetOption) error { accountInfo := &accountsv1alpha1.AccountInfo{ Spec: accountsv1alpha1.AccountInfoSpec{ Organization: accountsv1alpha1.AccountLocation{ @@ -163,8 +177,23 @@ func TestSubroutineProcess(t *testing.T) { } *o.(*accountsv1alpha1.AccountInfo) = *accountInfo return nil - }, - ) + }).Once() + + m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "acme"}, mock.AnythingOfType("*v1alpha1.IdentityProviderConfiguration"), mock.Anything). + RunAndReturn(func(ctx context.Context, nn types.NamespacedName, o client.Object, opts ...client.GetOption) error { + idp := &v1alpha1.IdentityProviderConfiguration{ + Spec: v1alpha1.IdentityProviderConfigurationSpec{ + Clients: []v1alpha1.IdentityProviderClientConfig{ + { + ClientName: "acme", + ClientID: "acme", + }, + }, + }, + } + *o.(*v1alpha1.IdentityProviderConfiguration) = *idp + return nil + }).Once() }, setupKeycloakMocks: func(mux *http.ServeMux) { first := true @@ -195,7 +224,8 @@ func TestSubroutineProcess(t *testing.T) { }, expectErr: true, setupK8sMocks: func(m *mocks.MockClient) { - m.EXPECT().Get(mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(fmt.Errorf("accountinfo not found")) + m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "account"}, mock.AnythingOfType("*v1alpha1.AccountInfo"), mock.Anything). + Return(fmt.Errorf("accountinfo not found")).Once() }, setupKeycloakMocks: func(mux *http.ServeMux) { }, @@ -209,8 +239,8 @@ func TestSubroutineProcess(t *testing.T) { }, expectErr: true, setupK8sMocks: func(m *mocks.MockClient) { - m.EXPECT().Get(mock.Anything, mock.Anything, mock.Anything, mock.Anything).RunAndReturn( - func(ctx context.Context, nn types.NamespacedName, o client.Object, opts ...client.GetOption) error { + m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "account"}, mock.AnythingOfType("*v1alpha1.AccountInfo"), mock.Anything). + RunAndReturn(func(ctx context.Context, nn types.NamespacedName, o client.Object, opts ...client.GetOption) error { accountInfo := &accountsv1alpha1.AccountInfo{ Spec: accountsv1alpha1.AccountInfoSpec{ Organization: accountsv1alpha1.AccountLocation{ @@ -220,8 +250,23 @@ func TestSubroutineProcess(t *testing.T) { } *o.(*accountsv1alpha1.AccountInfo) = *accountInfo return nil - }, - ) + }).Once() + + m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "acme"}, mock.AnythingOfType("*v1alpha1.IdentityProviderConfiguration"), mock.Anything). + RunAndReturn(func(ctx context.Context, nn types.NamespacedName, o client.Object, opts ...client.GetOption) error { + idp := &v1alpha1.IdentityProviderConfiguration{ + Spec: v1alpha1.IdentityProviderConfigurationSpec{ + Clients: []v1alpha1.IdentityProviderClientConfig{ + { + ClientName: "acme", + ClientID: "acme", + }, + }, + }, + } + *o.(*v1alpha1.IdentityProviderConfiguration) = *idp + return nil + }).Once() }, setupKeycloakMocks: func(mux *http.ServeMux) { users := []map[string]any{} @@ -255,8 +300,8 @@ func TestSubroutineProcess(t *testing.T) { }, expectErr: true, setupK8sMocks: func(m *mocks.MockClient) { - m.EXPECT().Get(mock.Anything, mock.Anything, mock.Anything, mock.Anything).RunAndReturn( - func(ctx context.Context, nn types.NamespacedName, o client.Object, opts ...client.GetOption) error { + m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "account"}, mock.AnythingOfType("*v1alpha1.AccountInfo"), mock.Anything). + RunAndReturn(func(ctx context.Context, nn types.NamespacedName, o client.Object, opts ...client.GetOption) error { accountInfo := &accountsv1alpha1.AccountInfo{ Spec: accountsv1alpha1.AccountInfoSpec{ Organization: accountsv1alpha1.AccountLocation{ @@ -266,8 +311,23 @@ func TestSubroutineProcess(t *testing.T) { } *o.(*accountsv1alpha1.AccountInfo) = *accountInfo return nil - }, - ) + }).Once() + + m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "acme"}, mock.AnythingOfType("*v1alpha1.IdentityProviderConfiguration"), mock.Anything). + RunAndReturn(func(ctx context.Context, nn types.NamespacedName, o client.Object, opts ...client.GetOption) error { + idp := &v1alpha1.IdentityProviderConfiguration{ + Spec: v1alpha1.IdentityProviderConfigurationSpec{ + Clients: []v1alpha1.IdentityProviderClientConfig{ + { + ClientName: "acme", + ClientID: "acme", + }, + }, + }, + } + *o.(*v1alpha1.IdentityProviderConfiguration) = *idp + return nil + }).Once() }, setupKeycloakMocks: func(mux *http.ServeMux) { users := []map[string]any{} @@ -304,8 +364,8 @@ func TestSubroutineProcess(t *testing.T) { }, expectErr: true, setupK8sMocks: func(m *mocks.MockClient) { - m.EXPECT().Get(mock.Anything, mock.Anything, mock.Anything, mock.Anything).RunAndReturn( - func(ctx context.Context, nn types.NamespacedName, o client.Object, opts ...client.GetOption) error { + m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "account"}, mock.AnythingOfType("*v1alpha1.AccountInfo"), mock.Anything). + RunAndReturn(func(ctx context.Context, nn types.NamespacedName, o client.Object, opts ...client.GetOption) error { accountInfo := &accountsv1alpha1.AccountInfo{ Spec: accountsv1alpha1.AccountInfoSpec{ Organization: accountsv1alpha1.AccountLocation{ @@ -315,8 +375,7 @@ func TestSubroutineProcess(t *testing.T) { } *o.(*accountsv1alpha1.AccountInfo) = *accountInfo return nil - }, - ) + }).Once() }, setupKeycloakMocks: func(mux *http.ServeMux) { }, From 6ac8c7bf46785f2096b47c628088fe3bae928e62 Mon Sep 17 00:00:00 2001 From: OlegErshov Date: Wed, 17 Dec 2025 15:28:42 +0100 Subject: [PATCH 07/22] fix: improved timout and secret error handling On-behalf-of: SAP aleh.yarshou@sap.com --- internal/config/config.go | 27 ++++++++++++--------------- internal/subroutine/idp/subroutine.go | 4 ++-- 2 files changed, 14 insertions(+), 17 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index dc5dd7c..0195e8f 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -14,16 +14,16 @@ type Config struct { KCP struct { Kubeconfig string `mapstructure:"kcp-kubeconfig" default:"/api-kubeconfig/kubeconfig"` } `mapstructure:",squash"` - APIExportEndpointSliceName string `mapstructure:"api-export-endpoint-slice-name"` - CoreModulePath string `mapstructure:"core-module-path"` - WorkspaceDir string `mapstructure:"workspace-dir" default:"/operator/"` - BaseDomain string `mapstructure:"base-domain" default:"portal.dev.local:8443"` - GroupClaim string `mapstructure:"group-claim" default:"groups"` - UserClaim string `mapstructure:"user-claim" default:"email"` - InitializerName string `mapstructure:"initializer-name" default:"root:security"` - DomainCALookup bool `mapstructure:"domain-ca-lookup" default:"false"` - SecretWaitingTimeoutInSeconds int `mapstructure:"secret-waiting-timeout-seconds" default:"60"` - IDP struct { + APIExportEndpointSliceName string `mapstructure:"api-export-endpoint-slice-name"` + CoreModulePath string `mapstructure:"core-module-path"` + WorkspaceDir string `mapstructure:"workspace-dir" default:"/operator/"` + BaseDomain string `mapstructure:"base-domain" default:"portal.dev.local:8443"` + GroupClaim string `mapstructure:"group-claim" default:"groups"` + UserClaim string `mapstructure:"user-claim" default:"email"` + InitializerName string `mapstructure:"initializer-name" default:"root:security"` + DomainCALookup bool `mapstructure:"domain-ca-lookup" default:"false"` + HttpClientTimeout int `mapstructure:"http-client-timeout" default:"30"` + IDP struct { // SMTP settings SMTPServer string `mapstructure:"idp-smtp-server"` SMTPPort int `mapstructure:"idp-smtp-port"` @@ -34,11 +34,8 @@ type Config struct { StartTLS bool `mapstructure:"idp-smtp-starttls" default:"false"` // Auth settings - SMTPUser string `mapstructure:"idp-smtp-user"` - // secret name and key will be removed with idp creation subroutine - SMTPPasswordSecretName string `mapstructure:"idp-smtp-password-secret-name"` - SMTPPasswordSecretKey string `mapstructure:"idp-smtp-password-secret-key" default:"password"` - SMTPPassword string `mapstructure:"idp-smtp-password"` + SMTPUser string `mapstructure:"idp-smtp-user"` + SMTPPassword string `mapstructure:"idp-smtp-password"` AdditionalRedirectURLs []string `mapstructure:"idp-additional-redirect-urls"` } `mapstructure:",squash"` diff --git a/internal/subroutine/idp/subroutine.go b/internal/subroutine/idp/subroutine.go index 81972cf..f7dd0ec 100644 --- a/internal/subroutine/idp/subroutine.go +++ b/internal/subroutine/idp/subroutine.go @@ -48,7 +48,7 @@ func New(ctx context.Context, cfg *config.Config, orgsClient client.Client, mgr httpClient := cCfg.Client(ctx) oidcClient := &http.Client{ - Timeout: 30 * time.Second, + Timeout: time.Duration(cfg.HttpClientTimeout) * time.Second, } return &subroutine{ @@ -194,7 +194,7 @@ func (s *subroutine) Process(ctx context.Context, instance runtimeobject.Runtime var clientInfo clientInfo if clientID != "" { registrationAccessToken, err := s.readRegistrationAccessTokenFromSecret(ctx, clientConfig.SecretRef) - if err != nil { + if err != nil && !kerrors.IsNotFound(err) { return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to get registration access token from secret: %w", err), true, true) } From 48d1840158702396bbe17e7615852e874be7f558 Mon Sep 17 00:00:00 2001 From: OlegErshov Date: Wed, 17 Dec 2025 16:07:23 +0100 Subject: [PATCH 08/22] fix: added waiting for finalizers functionality in integration tests On-behalf-of: SAP aleh.yarshou@sap.com --- .../authorization_model_generation_test.go | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/internal/test/integration/authorization_model_generation_test.go b/internal/test/integration/authorization_model_generation_test.go index 9b517ad..62de6c0 100644 --- a/internal/test/integration/authorization_model_generation_test.go +++ b/internal/test/integration/authorization_model_generation_test.go @@ -111,9 +111,20 @@ func (suite *IntegrationSuite) TestAuthorizationModelGeneration_Finalize() { var testApiBinding1, testApiBinding2 kcpapiv1alpha1.APIBinding suite.Require().NoError(testAccount1Client.Get(ctx, client.ObjectKey{Name: apiBinding1.Name}, &testApiBinding1)) suite.Require().NoError(testAccount2Client.Get(ctx, client.ObjectKey{Name: apiBinding2.Name}, &testApiBinding2)) + expectedFinalizers := []string{"apis.kcp.io/apibinding-finalizer", "core.platform-mesh.io/apibinding-finalizer"} - suite.Require().Equal(expectedFinalizers, testApiBinding1.Finalizers, "APIBinding %s should have the expected finalizers", testApiBinding1.Name) - suite.Require().Equal(expectedFinalizers, testApiBinding2.Finalizers, "APIBinding %s should have the expected finalizers", testApiBinding2.Name) + + suite.Assert().Eventually(func() bool { + var testApiBinding1, testApiBinding2 kcpapiv1alpha1.APIBinding + if err := testAccount1Client.Get(ctx, client.ObjectKey{Name: apiBinding1.Name}, &testApiBinding1); err != nil { + return false + } + if err := testAccount2Client.Get(ctx, client.ObjectKey{Name: apiBinding2.Name}, &testApiBinding2); err != nil { + return false + } + return suite.Equal(expectedFinalizers, testApiBinding1.Finalizers) && + suite.Equal(expectedFinalizers, testApiBinding2.Finalizers) + }, 1*time.Second, 200*time.Millisecond, "APIBindings should have the expected finalizers") err = testAccount1Client.Delete(ctx, apiBinding1) suite.Require().NoError(err) From 9e7ee4937d8d3863439f3863e7865ec59ca63635 Mon Sep 17 00:00:00 2001 From: OlegErshov Date: Wed, 17 Dec 2025 17:19:53 +0100 Subject: [PATCH 09/22] chore: refactored idp resource creation On-behalf-of: SAP aleh.yarshou@sap.com --- internal/subroutine/idp.go | 24 ++++++++++++------------ internal/subroutine/invite/subroutine.go | 4 ++-- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/internal/subroutine/idp.go b/internal/subroutine/idp.go index c95cabf..8edafc8 100644 --- a/internal/subroutine/idp.go +++ b/internal/subroutine/idp.go @@ -3,6 +3,7 @@ package subroutine import ( "context" "fmt" + "slices" "strings" kcpv1alpha1 "github.com/kcp-dev/kcp/sdk/apis/core/v1alpha1" @@ -91,19 +92,18 @@ func (w *IDPSubroutine) Process(ctx context.Context, instance runtimeobject.Runt idp := &v1alpha1.IdentityProviderConfiguration{ObjectMeta: metav1.ObjectMeta{Name: workspaceName}} _, err = controllerutil.CreateOrUpdate(ctx, cl.GetClient(), idp, func() error { - for i := range idp.Spec.Clients { - if idp.Spec.Clients[i].ClientName == clientConfig.ClientName { - idp.Spec.Clients[i].RedirectURIs = clientConfig.RedirectURIs - idp.Spec.Clients[i].ClientType = clientConfig.ClientType - idp.Spec.Clients[i].SecretRef = clientConfig.SecretRef - idp.Spec.Clients[i].PostLogoutRedirectURIs = clientConfig.PostLogoutRedirectURIs - - return nil - } + clientIdx := slices.IndexFunc(idp.Spec.Clients, func(c v1alpha1.IdentityProviderClientConfig) bool { + return c.ClientName == clientConfig.ClientName + }) + if clientIdx != -1 { + idp.Spec.Clients[clientIdx].RedirectURIs = clientConfig.RedirectURIs + idp.Spec.Clients[clientIdx].ClientType = clientConfig.ClientType + idp.Spec.Clients[clientIdx].SecretRef = clientConfig.SecretRef + idp.Spec.Clients[clientIdx].PostLogoutRedirectURIs = clientConfig.PostLogoutRedirectURIs + return nil } - idp.Spec.Clients = []v1alpha1.IdentityProviderClientConfig{clientConfig} - + idp.Spec.Clients = append(idp.Spec.Clients, clientConfig) return nil }) if err != nil { @@ -129,4 +129,4 @@ func getWorkspaceName(lc *kcpv1alpha1.LogicalCluster) string { return pathElements[len(pathElements)-1] } return "" -} \ No newline at end of file +} diff --git a/internal/subroutine/invite/subroutine.go b/internal/subroutine/invite/subroutine.go index db6030e..fcaf693 100644 --- a/internal/subroutine/invite/subroutine.go +++ b/internal/subroutine/invite/subroutine.go @@ -144,7 +144,7 @@ func (s *subroutine) Process(ctx context.Context, instance runtimeobject.Runtime clientID, err := s.getClientID(ctx, cl, realm) if err != nil { - log.Err(err).Msg(fmt.Sprintf("failed to get client ID for client %s",realm)) + log.Err(err).Msg("failed to get client ID") return ctrl.Result{}, errors.NewOperatorError(err, true, false) } @@ -256,5 +256,5 @@ func (s *subroutine) getClientID(ctx context.Context, cl client.Client, clientNa return v.ClientID, nil } } - return "", fmt.Errorf("client not found") + return "", fmt.Errorf("client %s not found",clientName) } From 38baad0f41c628c449f599b9080695e317710717 Mon Sep 17 00:00:00 2001 From: OlegErshov Date: Wed, 17 Dec 2025 17:29:39 +0100 Subject: [PATCH 10/22] chore: refactored client id fetching On-behalf-of: SAP aleh.yarshou@sap.com --- internal/subroutine/invite/subroutine.go | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/internal/subroutine/invite/subroutine.go b/internal/subroutine/invite/subroutine.go index fcaf693..58fd19c 100644 --- a/internal/subroutine/invite/subroutine.go +++ b/internal/subroutine/invite/subroutine.go @@ -7,6 +7,7 @@ import ( "fmt" "net/http" "net/url" + "slices" "github.com/coreos/go-oidc" accountsv1alpha1 "github.com/platform-mesh/account-operator/api/v1alpha1" @@ -251,10 +252,13 @@ func (s *subroutine) getClientID(ctx context.Context, cl client.Client, clientNa return "", err } - for _, v := range idp.Spec.Clients { - if v.ClientName == clientName { - return v.ClientID, nil - } + clientIdx := slices.IndexFunc(idp.Spec.Clients, func(c v1alpha1.IdentityProviderClientConfig) bool { + return c.ClientName == clientName + }) + + if clientIdx == -1 { + return "", fmt.Errorf("client %s not found", clientName) } - return "", fmt.Errorf("client %s not found",clientName) + + return idp.Spec.Clients[clientIdx].ClientID, nil } From d94f9bea2a95e82b6eddfa40f6ad022b02ef7130 Mon Sep 17 00:00:00 2001 From: OlegErshov Date: Wed, 17 Dec 2025 17:41:59 +0100 Subject: [PATCH 11/22] fix: replaces http schema with https for logout uris On-behalf-of: SAP aleh.yarshou@sap.com --- internal/subroutine/idp.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/subroutine/idp.go b/internal/subroutine/idp.go index 8edafc8..1dad6ca 100644 --- a/internal/subroutine/idp.go +++ b/internal/subroutine/idp.go @@ -83,7 +83,7 @@ func (w *IDPSubroutine) Process(ctx context.Context, instance runtimeobject.Runt ClientName: workspaceName, ClientType: v1alpha1.IdentityProviderClientTypeConfidential, RedirectURIs: append(w.cfg.IDP.AdditionalRedirectURLs, fmt.Sprintf("https://%s.%s/*", workspaceName, w.baseDomain)), - PostLogoutRedirectURIs: []string{fmt.Sprintf("http://%s.%s/logout*", workspaceName, w.baseDomain)}, + PostLogoutRedirectURIs: []string{fmt.Sprintf("https://%s.%s/logout*", workspaceName, w.baseDomain)}, SecretRef: corev1.SecretReference{ Name: fmt.Sprintf("portal-client-secret-%s-%s", workspaceName, workspaceName), Namespace: "default", From 4e3e7572f2afa7d97f325f4b740e69f1c736cbb9 Mon Sep 17 00:00:00 2001 From: OlegErshov Date: Thu, 18 Dec 2025 08:40:03 +0100 Subject: [PATCH 12/22] chore: refactored idp subroutine initialization On-behalf-of: SAP aleh.yarshou@sap.com --- internal/controller/initializer_controller.go | 2 +- internal/subroutine/idp.go | 20 ++++---- internal/subroutine/idp_test.go | 46 ++++++++++--------- internal/subroutine/invite/subroutine.go | 2 +- 4 files changed, 37 insertions(+), 33 deletions(-) diff --git a/internal/controller/initializer_controller.go b/internal/controller/initializer_controller.go index 6a00ac0..fbcb3be 100644 --- a/internal/controller/initializer_controller.go +++ b/internal/controller/initializer_controller.go @@ -32,7 +32,7 @@ func NewLogicalClusterReconciler(log *logger.Logger, orgClient client.Client, cf mclifecycle: builder.NewBuilder("logicalcluster", "LogicalClusterReconciler", []lifecyclesubroutine.Subroutine{ subroutine.NewWorkspaceInitializer(orgClient, cfg, mgr), subroutine.NewWorkspaceAuthConfigurationSubroutine(orgClient, inClusterClient, cfg), - subroutine.NewIDPSubroutine(orgClient, mgr, &cfg, cfg.BaseDomain), + subroutine.NewIDPSubroutine(orgClient, mgr, cfg), subroutine.NewInviteSubroutine(orgClient, mgr), subroutine.NewRemoveInitializer(mgr, cfg), }, log). diff --git a/internal/subroutine/idp.go b/internal/subroutine/idp.go index 1dad6ca..c94e17f 100644 --- a/internal/subroutine/idp.go +++ b/internal/subroutine/idp.go @@ -27,22 +27,22 @@ import ( "github.com/platform-mesh/security-operator/internal/config" ) -func NewIDPSubroutine(orgsClient client.Client, mgr mcmanager.Manager, cfg *config.Config, baseDomain string) *IDPSubroutine { +func NewIDPSubroutine(orgsClient client.Client, mgr mcmanager.Manager, cfg config.Config) *IDPSubroutine { return &IDPSubroutine{ - orgsClient: orgsClient, - mgr: mgr, - cfg: cfg, - baseDomain: baseDomain, + orgsClient: orgsClient, + mgr: mgr, + additionalRedirectURLs: cfg.IDP.AdditionalRedirectURLs, + baseDomain: cfg.BaseDomain, } } var _ lifecyclesubroutine.Subroutine = &IDPSubroutine{} type IDPSubroutine struct { - orgsClient client.Client - mgr mcmanager.Manager - cfg *config.Config - baseDomain string + orgsClient client.Client + mgr mcmanager.Manager + additionalRedirectURLs []string + baseDomain string } func (w *IDPSubroutine) Finalize(ctx context.Context, instance runtimeobject.RuntimeObject) (ctrl.Result, errors.OperatorError) { @@ -82,7 +82,7 @@ func (w *IDPSubroutine) Process(ctx context.Context, instance runtimeobject.Runt clientConfig := v1alpha1.IdentityProviderClientConfig{ ClientName: workspaceName, ClientType: v1alpha1.IdentityProviderClientTypeConfidential, - RedirectURIs: append(w.cfg.IDP.AdditionalRedirectURLs, fmt.Sprintf("https://%s.%s/*", workspaceName, w.baseDomain)), + RedirectURIs: append(w.additionalRedirectURLs, fmt.Sprintf("https://%s.%s/*", workspaceName, w.baseDomain)), PostLogoutRedirectURIs: []string{fmt.Sprintf("https://%s.%s/logout*", workspaceName, w.baseDomain)}, SecretRef: corev1.SecretReference{ Name: fmt.Sprintf("portal-client-secret-%s-%s", workspaceName, workspaceName), diff --git a/internal/subroutine/idp_test.go b/internal/subroutine/idp_test.go index d2d6e64..81bdfdd 100644 --- a/internal/subroutine/idp_test.go +++ b/internal/subroutine/idp_test.go @@ -23,24 +23,25 @@ import ( func TestNewIDPSubroutine(t *testing.T) { orgsClient := mocks.NewMockClient(t) mgr := mocks.NewMockManager(t) - cfg := &config.Config{} + cfg := config.Config{} cfg.IDP.AdditionalRedirectURLs = []string{"https://example.com/callback"} - baseDomain := "example.com" + cfg.BaseDomain = "example.com" - subroutine := NewIDPSubroutine(orgsClient, mgr, cfg, baseDomain) + subroutine := NewIDPSubroutine(orgsClient, mgr, cfg) assert.NotNil(t, subroutine) assert.Equal(t, orgsClient, subroutine.orgsClient) assert.Equal(t, mgr, subroutine.mgr) - assert.Equal(t, cfg, subroutine.cfg) - assert.Equal(t, baseDomain, subroutine.baseDomain) + assert.Equal(t, []string{"https://example.com/callback"}, subroutine.additionalRedirectURLs) + assert.Equal(t, "example.com", subroutine.baseDomain) } func TestIDPSubroutine_GetName(t *testing.T) { orgsClient := mocks.NewMockClient(t) mgr := mocks.NewMockManager(t) - cfg := &config.Config{} - subroutine := NewIDPSubroutine(orgsClient, mgr, cfg, "example.com") + cfg := config.Config{} + cfg.BaseDomain = "example.com" + subroutine := NewIDPSubroutine(orgsClient, mgr, cfg) name := subroutine.GetName() assert.Equal(t, "IDPSubroutine", name) @@ -49,8 +50,9 @@ func TestIDPSubroutine_GetName(t *testing.T) { func TestIDPSubroutine_Finalizers(t *testing.T) { orgsClient := mocks.NewMockClient(t) mgr := mocks.NewMockManager(t) - cfg := &config.Config{} - subroutine := NewIDPSubroutine(orgsClient, mgr, cfg, "example.com") + cfg := config.Config{} + cfg.BaseDomain = "example.com" + subroutine := NewIDPSubroutine(orgsClient, mgr, cfg) finalizers := subroutine.Finalizers(nil) assert.Nil(t, finalizers) @@ -59,8 +61,9 @@ func TestIDPSubroutine_Finalizers(t *testing.T) { func TestIDPSubroutine_Finalize(t *testing.T) { orgsClient := mocks.NewMockClient(t) mgr := mocks.NewMockManager(t) - cfg := &config.Config{} - subroutine := NewIDPSubroutine(orgsClient, mgr, cfg, "example.com") + cfg := config.Config{} + cfg.BaseDomain = "example.com" + subroutine := NewIDPSubroutine(orgsClient, mgr, cfg) ctx := context.Background() instance := &kcpv1alpha1.LogicalCluster{} @@ -74,14 +77,14 @@ func TestIDPSubroutine_Finalize(t *testing.T) { func TestIDPSubroutine_Process(t *testing.T) { tests := []struct { name string - setupMocks func(*mocks.MockClient, *mocks.MockManager, *mocks.MockCluster, *config.Config) + setupMocks func(*mocks.MockClient, *mocks.MockManager, *mocks.MockCluster, config.Config) lc *kcpv1alpha1.LogicalCluster expectedErr bool expectedResult ctrl.Result }{ { name: "Empty workspace name - early return", - setupMocks: func(orgsClient *mocks.MockClient, mgr *mocks.MockManager, cluster *mocks.MockCluster, cfg *config.Config) { + setupMocks: func(orgsClient *mocks.MockClient, mgr *mocks.MockManager, cluster *mocks.MockCluster, cfg config.Config) { }, lc: &kcpv1alpha1.LogicalCluster{ ObjectMeta: metav1.ObjectMeta{ @@ -93,7 +96,7 @@ func TestIDPSubroutine_Process(t *testing.T) { }, { name: "ClusterFromContext error", - setupMocks: func(orgsClient *mocks.MockClient, mgr *mocks.MockManager, cluster *mocks.MockCluster, cfg *config.Config) { + setupMocks: func(orgsClient *mocks.MockClient, mgr *mocks.MockManager, cluster *mocks.MockCluster, cfg config.Config) { mgr.EXPECT().ClusterFromContext(mock.Anything).Return(nil, assert.AnError).Once() }, lc: &kcpv1alpha1.LogicalCluster{ @@ -108,7 +111,7 @@ func TestIDPSubroutine_Process(t *testing.T) { }, { name: "Account Get error", - setupMocks: func(orgsClient *mocks.MockClient, mgr *mocks.MockManager, cluster *mocks.MockCluster, cfg *config.Config) { + setupMocks: func(orgsClient *mocks.MockClient, mgr *mocks.MockManager, cluster *mocks.MockCluster, cfg config.Config) { mgr.EXPECT().ClusterFromContext(mock.Anything).Return(cluster, nil).Once() orgsClient.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "test"}, mock.AnythingOfType("*v1alpha1.Account")). Return(assert.AnError).Once() @@ -125,7 +128,7 @@ func TestIDPSubroutine_Process(t *testing.T) { }, { name: "Account not of type organization - skip idp creation", - setupMocks: func(orgsClient *mocks.MockClient, mgr *mocks.MockManager, cluster *mocks.MockCluster, cfg *config.Config) { + setupMocks: func(orgsClient *mocks.MockClient, mgr *mocks.MockManager, cluster *mocks.MockCluster, cfg config.Config) { mgr.EXPECT().ClusterFromContext(mock.Anything).Return(cluster, nil).Once() orgsClient.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "test"}, mock.AnythingOfType("*v1alpha1.Account")). RunAndReturn(func(_ context.Context, _ types.NamespacedName, obj client.Object, _ ...client.GetOption) error { @@ -146,7 +149,7 @@ func TestIDPSubroutine_Process(t *testing.T) { }, { name: "CreateOrUpdate and Ready", - setupMocks: func(orgsClient *mocks.MockClient, mgr *mocks.MockManager, cluster *mocks.MockCluster, cfg *config.Config) { + setupMocks: func(orgsClient *mocks.MockClient, mgr *mocks.MockManager, cluster *mocks.MockCluster, cfg config.Config) { mgr.EXPECT().ClusterFromContext(mock.Anything).Return(cluster, nil).Once() orgsClient.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "acme"}, mock.AnythingOfType("*v1alpha1.Account")). RunAndReturn(func(_ context.Context, _ types.NamespacedName, obj client.Object, _ ...client.GetOption) error { @@ -182,7 +185,7 @@ func TestIDPSubroutine_Process(t *testing.T) { }, { name: "CreateOrUpdate NotReady", - setupMocks: func(orgsClient *mocks.MockClient, mgr *mocks.MockManager, cluster *mocks.MockCluster, cfg *config.Config) { + setupMocks: func(orgsClient *mocks.MockClient, mgr *mocks.MockManager, cluster *mocks.MockCluster, cfg config.Config) { mgr.EXPECT().ClusterFromContext(mock.Anything).Return(cluster, nil).Once() orgsClient.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "beta"}, mock.AnythingOfType("*v1alpha1.Account")). RunAndReturn(func(_ context.Context, _ types.NamespacedName, obj client.Object, _ ...client.GetOption) error { @@ -218,7 +221,7 @@ func TestIDPSubroutine_Process(t *testing.T) { }, { name: "Get IDP resource error", - setupMocks: func(orgsClient *mocks.MockClient, mgr *mocks.MockManager, cluster *mocks.MockCluster, cfg *config.Config) { + setupMocks: func(orgsClient *mocks.MockClient, mgr *mocks.MockManager, cluster *mocks.MockCluster, cfg config.Config) { mgr.EXPECT().ClusterFromContext(mock.Anything).Return(cluster, nil).Once() orgsClient.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "gamma"}, mock.AnythingOfType("*v1alpha1.Account")). RunAndReturn(func(_ context.Context, _ types.NamespacedName, obj client.Object, _ ...client.GetOption) error { @@ -251,9 +254,10 @@ func TestIDPSubroutine_Process(t *testing.T) { orgsClient := mocks.NewMockClient(t) mgr := mocks.NewMockManager(t) cluster := mocks.NewMockCluster(t) - cfg := &config.Config{} + cfg := config.Config{} cfg.IDP.AdditionalRedirectURLs = []string{} - subroutine := NewIDPSubroutine(orgsClient, mgr, cfg, "example.com") + cfg.BaseDomain = "example.com" + subroutine := NewIDPSubroutine(orgsClient, mgr, cfg) tt.setupMocks(orgsClient, mgr, cluster, cfg) diff --git a/internal/subroutine/invite/subroutine.go b/internal/subroutine/invite/subroutine.go index 58fd19c..a23e6ae 100644 --- a/internal/subroutine/invite/subroutine.go +++ b/internal/subroutine/invite/subroutine.go @@ -255,7 +255,7 @@ func (s *subroutine) getClientID(ctx context.Context, cl client.Client, clientNa clientIdx := slices.IndexFunc(idp.Spec.Clients, func(c v1alpha1.IdentityProviderClientConfig) bool { return c.ClientName == clientName }) - + if clientIdx == -1 { return "", fmt.Errorf("client %s not found", clientName) } From 11d478c5740ee92c1aa465f46352dbb005c83e61 Mon Sep 17 00:00:00 2001 From: OlegErshov Date: Thu, 18 Dec 2025 14:38:57 +0100 Subject: [PATCH 13/22] chore: removed unused fields in config On-behalf-of: SAP aleh.yarshou@sap.com --- internal/config/config.go | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 0195e8f..08ef1db 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -14,16 +14,14 @@ type Config struct { KCP struct { Kubeconfig string `mapstructure:"kcp-kubeconfig" default:"/api-kubeconfig/kubeconfig"` } `mapstructure:",squash"` - APIExportEndpointSliceName string `mapstructure:"api-export-endpoint-slice-name"` - CoreModulePath string `mapstructure:"core-module-path"` - WorkspaceDir string `mapstructure:"workspace-dir" default:"/operator/"` - BaseDomain string `mapstructure:"base-domain" default:"portal.dev.local:8443"` - GroupClaim string `mapstructure:"group-claim" default:"groups"` - UserClaim string `mapstructure:"user-claim" default:"email"` - InitializerName string `mapstructure:"initializer-name" default:"root:security"` - DomainCALookup bool `mapstructure:"domain-ca-lookup" default:"false"` - HttpClientTimeout int `mapstructure:"http-client-timeout" default:"30"` - IDP struct { + CoreModulePath string `mapstructure:"core-module-path"` + BaseDomain string `mapstructure:"base-domain" default:"portal.dev.local:8443"` + GroupClaim string `mapstructure:"group-claim" default:"groups"` + UserClaim string `mapstructure:"user-claim" default:"email"` + InitializerName string `mapstructure:"initializer-name" default:"root:security"` + DomainCALookup bool `mapstructure:"domain-ca-lookup" default:"false"` + HttpClientTimeout int `mapstructure:"http-client-timeout" default:"30"` + IDP struct { // SMTP settings SMTPServer string `mapstructure:"idp-smtp-server"` SMTPPort int `mapstructure:"idp-smtp-port"` From e82eb279609420f81888f125edc41f2d8b30409c Mon Sep 17 00:00:00 2001 From: OlegErshov Date: Thu, 18 Dec 2025 14:44:30 +0100 Subject: [PATCH 14/22] chore: updated secret's naming in tests On-behalf-of: SAP aleh.yarshou@sap.com --- internal/subroutine/idp_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/internal/subroutine/idp_test.go b/internal/subroutine/idp_test.go index 81bdfdd..e4573c9 100644 --- a/internal/subroutine/idp_test.go +++ b/internal/subroutine/idp_test.go @@ -163,6 +163,10 @@ func TestIDPSubroutine_Process(t *testing.T) { orgsClient.EXPECT().Create(mock.Anything, mock.AnythingOfType("*v1alpha1.IdentityProviderConfiguration")). RunAndReturn(func(_ context.Context, obj client.Object, _ ...client.CreateOption) error { idp := obj.(*secopv1alpha1.IdentityProviderConfiguration) + assert.Len(t, idp.Spec.Clients, 1) + assert.Equal(t, "acme", idp.Spec.Clients[0].ClientName) + assert.Equal(t, "portal-client-secret-acme-acme", idp.Spec.Clients[0].SecretRef.Name) + assert.Equal(t, "default", idp.Spec.Clients[0].SecretRef.Namespace) idp.Status.Conditions = []metav1.Condition{{Type: "Ready", Status: metav1.ConditionTrue}} return nil }).Once() @@ -199,6 +203,10 @@ func TestIDPSubroutine_Process(t *testing.T) { orgsClient.EXPECT().Create(mock.Anything, mock.AnythingOfType("*v1alpha1.IdentityProviderConfiguration")). RunAndReturn(func(_ context.Context, obj client.Object, _ ...client.CreateOption) error { idp := obj.(*secopv1alpha1.IdentityProviderConfiguration) + assert.Len(t, idp.Spec.Clients, 1) + assert.Equal(t, "beta", idp.Spec.Clients[0].ClientName) + assert.Equal(t, "portal-client-secret-beta-beta", idp.Spec.Clients[0].SecretRef.Name) + assert.Equal(t, "default", idp.Spec.Clients[0].SecretRef.Namespace) idp.Status.Conditions = []metav1.Condition{{Type: "Ready", Status: metav1.ConditionFalse}} return nil }).Once() From 00c609616a3e3545056b5b369696072c7d222db0 Mon Sep 17 00:00:00 2001 From: OlegErshov Date: Thu, 18 Dec 2025 15:13:45 +0100 Subject: [PATCH 15/22] chore: addressed other coderabbit comments On-behalf-of: SAP aleh.yarshou@sap.com --- internal/config/config.go | 16 ++++++++-------- internal/subroutine/idp/subroutine.go | 3 ++- internal/subroutine/remove_initializer.go | 4 ---- ...thorization.go => workspace_authorization.go} | 4 ++-- 4 files changed, 12 insertions(+), 15 deletions(-) rename internal/subroutine/{worksapce_authorization.go => workspace_authorization.go} (96%) diff --git a/internal/config/config.go b/internal/config/config.go index 08ef1db..0367616 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -14,14 +14,14 @@ type Config struct { KCP struct { Kubeconfig string `mapstructure:"kcp-kubeconfig" default:"/api-kubeconfig/kubeconfig"` } `mapstructure:",squash"` - CoreModulePath string `mapstructure:"core-module-path"` - BaseDomain string `mapstructure:"base-domain" default:"portal.dev.local:8443"` - GroupClaim string `mapstructure:"group-claim" default:"groups"` - UserClaim string `mapstructure:"user-claim" default:"email"` - InitializerName string `mapstructure:"initializer-name" default:"root:security"` - DomainCALookup bool `mapstructure:"domain-ca-lookup" default:"false"` - HttpClientTimeout int `mapstructure:"http-client-timeout" default:"30"` - IDP struct { + CoreModulePath string `mapstructure:"core-module-path"` + BaseDomain string `mapstructure:"base-domain" default:"portal.dev.local:8443"` + GroupClaim string `mapstructure:"group-claim" default:"groups"` + UserClaim string `mapstructure:"user-claim" default:"email"` + InitializerName string `mapstructure:"initializer-name" default:"root:security"` + DomainCALookup bool `mapstructure:"domain-ca-lookup" default:"false"` + HttpClientTimeoutSeconds int `mapstructure:"http-client-timeout-seconds" default:"30"` + IDP struct { // SMTP settings SMTPServer string `mapstructure:"idp-smtp-server"` SMTPPort int `mapstructure:"idp-smtp-port"` diff --git a/internal/subroutine/idp/subroutine.go b/internal/subroutine/idp/subroutine.go index f7dd0ec..99df1f3 100644 --- a/internal/subroutine/idp/subroutine.go +++ b/internal/subroutine/idp/subroutine.go @@ -48,7 +48,7 @@ func New(ctx context.Context, cfg *config.Config, orgsClient client.Client, mgr httpClient := cCfg.Client(ctx) oidcClient := &http.Client{ - Timeout: time.Duration(cfg.HttpClientTimeout) * time.Second, + Timeout: time.Duration(cfg.HttpClientTimeoutSeconds) * time.Second, } return &subroutine{ @@ -165,6 +165,7 @@ func (s *subroutine) Process(ctx context.Context, instance runtimeobject.Runtime log.Info().Str("secretName", managedClient.SecretRef.Name).Msg("Secret not found, client was likely deleted") continue } + return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to get registration access token from secret: %w", err), true, true) } if err := s.deleteClient(ctx, realmName, managedClient.ClientID, managedClient.RegistrationClientURI, registrationAccessToken); err != nil { diff --git a/internal/subroutine/remove_initializer.go b/internal/subroutine/remove_initializer.go index 833921b..4ae1b83 100644 --- a/internal/subroutine/remove_initializer.go +++ b/internal/subroutine/remove_initializer.go @@ -17,10 +17,6 @@ import ( mcmanager "sigs.k8s.io/multicluster-runtime/pkg/manager" ) -const ( - PortalClientSecretNamespace = "platform-mesh-system" -) - type removeInitializer struct { initializerName string mgr mcmanager.Manager diff --git a/internal/subroutine/worksapce_authorization.go b/internal/subroutine/workspace_authorization.go similarity index 96% rename from internal/subroutine/worksapce_authorization.go rename to internal/subroutine/workspace_authorization.go index 6d67eed..3f66232 100644 --- a/internal/subroutine/worksapce_authorization.go +++ b/internal/subroutine/workspace_authorization.go @@ -97,12 +97,12 @@ func (r *workspaceAuthSubroutine) Process(ctx context.Context, instance runtimeo err = r.patchWorkspaceType(ctx, r.orgClient, fmt.Sprintf("%s-org", workspaceName), workspaceName) if err != nil { - return reconcile.Result{}, errors.NewOperatorError(fmt.Errorf("failed to create patch workspace type resource: %w", err), true, true) + return reconcile.Result{}, errors.NewOperatorError(fmt.Errorf("failed to patch workspace type: %w", err), true, true) } err = r.patchWorkspaceType(ctx, r.orgClient, fmt.Sprintf("%s-acc", workspaceName), workspaceName) if err != nil { - return reconcile.Result{}, errors.NewOperatorError(fmt.Errorf("failed to create patch workspace type resource: %w", err), true, true) + return reconcile.Result{}, errors.NewOperatorError(fmt.Errorf("failed to patch workspace type: %w", err), true, true) } return ctrl.Result{}, nil From a4e02ddeca43b2a6f6595f20b4b94188d37e2f80 Mon Sep 17 00:00:00 2001 From: OlegErshov Date: Thu, 18 Dec 2025 15:26:20 +0100 Subject: [PATCH 16/22] chore: increased timeout On-behalf-of: SAP aleh.yarshou@sap.com --- .../test/integration/authorization_model_generation_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/test/integration/authorization_model_generation_test.go b/internal/test/integration/authorization_model_generation_test.go index 62de6c0..0df0be3 100644 --- a/internal/test/integration/authorization_model_generation_test.go +++ b/internal/test/integration/authorization_model_generation_test.go @@ -124,7 +124,7 @@ func (suite *IntegrationSuite) TestAuthorizationModelGeneration_Finalize() { } return suite.Equal(expectedFinalizers, testApiBinding1.Finalizers) && suite.Equal(expectedFinalizers, testApiBinding2.Finalizers) - }, 1*time.Second, 200*time.Millisecond, "APIBindings should have the expected finalizers") + }, 5*time.Second, 200*time.Millisecond, "APIBindings should have the expected finalizers") err = testAccount1Client.Delete(ctx, apiBinding1) suite.Require().NoError(err) From 8690c3bc3d358316fac29f06a6113c3ea90dfb19 Mon Sep 17 00:00:00 2001 From: Oleg Ershov <97069709+OlegErshov@users.noreply.github.com> Date: Thu, 18 Dec 2025 15:56:57 +0100 Subject: [PATCH 17/22] Update internal/subroutine/idp.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Echterhölter --- internal/subroutine/idp.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/subroutine/idp.go b/internal/subroutine/idp.go index c94e17f..25cd77f 100644 --- a/internal/subroutine/idp.go +++ b/internal/subroutine/idp.go @@ -75,7 +75,7 @@ func (w *IDPSubroutine) Process(ctx context.Context, instance runtimeobject.Runt } if account.Spec.Type != accountv1alpha1.AccountTypeOrg { - log.Info().Str("workspace", workspaceName).Msg("account is not of type organization, skipping idp creation") + log.Debug().Str("workspace", workspaceName).Msg("account is not of type organization, skipping idp creation") return ctrl.Result{}, nil } From 5755207eb5a8619b6e29f41a6b8fc76af1fcb495 Mon Sep 17 00:00:00 2001 From: OlegErshov Date: Thu, 18 Dec 2025 16:50:22 +0100 Subject: [PATCH 18/22] fix: used patch call instead of createOrUpdate On-behalf-of: SAP aleh.yarshou@sap.com --- internal/subroutine/invite.go | 4 +- .../subroutine/workspace_authorization.go | 21 ++-- .../workspace_authorization_test.go | 108 ++++++++++++------ 3 files changed, 91 insertions(+), 42 deletions(-) diff --git a/internal/subroutine/invite.go b/internal/subroutine/invite.go index 7202a0a..2d544c5 100644 --- a/internal/subroutine/invite.go +++ b/internal/subroutine/invite.go @@ -90,7 +90,7 @@ func (w *inviteSubroutine) Process(ctx context.Context, instance runtimeobject.R return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to create invite resource %w", err), true, true) } - log.Info().Str("workspace", wsName).Msg("invite resource created") + log.Info().Str("workspace", wsName).Msg("invite resource is created") err = wait.ExponentialBackoffWithContext(ctx, retry.DefaultBackoff, func(ctx context.Context) (bool, error) { @@ -105,6 +105,6 @@ func (w *inviteSubroutine) Process(ctx context.Context, instance runtimeobject.R return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("invite resource is not ready yet"), true, false) } - log.Info().Str("workspace", wsName).Msg("invite resource ready") + log.Info().Str("workspace", wsName).Msg("invite resource is ready") return ctrl.Result{}, nil } diff --git a/internal/subroutine/workspace_authorization.go b/internal/subroutine/workspace_authorization.go index 3f66232..56d4b79 100644 --- a/internal/subroutine/workspace_authorization.go +++ b/internal/subroutine/workspace_authorization.go @@ -108,17 +108,22 @@ func (r *workspaceAuthSubroutine) Process(ctx context.Context, instance runtimeo return ctrl.Result{}, nil } -func (r *workspaceAuthSubroutine) patchWorkspaceType(ctx context.Context, client client.Client, workspaceTypeName, authConfigurationRefName string) error { - wsType := kcptenancyv1alphav1.WorkspaceType{ +func (r *workspaceAuthSubroutine) patchWorkspaceType(ctx context.Context, cl client.Client, workspaceTypeName, authConfigurationRefName string) error { + wsType := &kcptenancyv1alphav1.WorkspaceType{ ObjectMeta: metav1.ObjectMeta{Name: workspaceTypeName}, } - _, err := controllerutil.CreateOrUpdate(ctx, client, &wsType, func() error { - wsType.Spec.AuthenticationConfigurations = []kcptenancyv1alphav1.AuthenticationConfigurationReference{{Name: authConfigurationRefName}} - return nil - }) - if err != nil { - return err + + if err := cl.Get(ctx, client.ObjectKey{Name: workspaceTypeName}, wsType); err != nil { + return fmt.Errorf("failed to get WorkspaceType: %w", err) } + original := wsType.DeepCopy() + wsType.Spec.AuthenticationConfigurations = []kcptenancyv1alphav1.AuthenticationConfigurationReference{ + {Name: authConfigurationRefName}, + } + + if err := cl.Patch(ctx, wsType, client.MergeFrom(original)); err != nil { + return fmt.Errorf("failed to patch WorkspaceType: %w", err) + } return nil } diff --git a/internal/subroutine/workspace_authorization_test.go b/internal/subroutine/workspace_authorization_test.go index 48ff119..1d54875 100644 --- a/internal/subroutine/workspace_authorization_test.go +++ b/internal/subroutine/workspace_authorization_test.go @@ -54,9 +54,13 @@ func TestWorkspaceAuthSubroutine_Process(t *testing.T) { }).Once() m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "test-workspace-org"}, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). - Return(apierrors.NewNotFound(kcptenancyv1alphav1.Resource("workspacetypes"), "test-workspace-org")).Once() - m.EXPECT().Create(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). - RunAndReturn(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + RunAndReturn(func(ctx context.Context, key types.NamespacedName, obj client.Object, opts ...client.GetOption) error { + wt := obj.(*kcptenancyv1alphav1.WorkspaceType) + wt.Name = "test-workspace-org" + return nil + }).Once() + m.EXPECT().Patch(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). + RunAndReturn(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { wt := obj.(*kcptenancyv1alphav1.WorkspaceType) assert.Equal(t, "test-workspace-org", wt.Name) assert.Equal(t, "test-workspace", wt.Spec.AuthenticationConfigurations[0].Name) @@ -64,9 +68,13 @@ func TestWorkspaceAuthSubroutine_Process(t *testing.T) { }).Once() m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "test-workspace-acc"}, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). - Return(apierrors.NewNotFound(kcptenancyv1alphav1.Resource("workspacetypes"), "test-workspace-acc")).Once() - m.EXPECT().Create(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). - RunAndReturn(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + RunAndReturn(func(ctx context.Context, key types.NamespacedName, obj client.Object, opts ...client.GetOption) error { + wt := obj.(*kcptenancyv1alphav1.WorkspaceType) + wt.Name = "test-workspace-acc" + return nil + }).Once() + m.EXPECT().Patch(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). + RunAndReturn(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { wt := obj.(*kcptenancyv1alphav1.WorkspaceType) assert.Equal(t, "test-workspace-acc", wt.Name) assert.Equal(t, "test-workspace", wt.Spec.AuthenticationConfigurations[0].Name) @@ -111,9 +119,13 @@ func TestWorkspaceAuthSubroutine_Process(t *testing.T) { }).Once() m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "existing-workspace-org"}, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). - Return(apierrors.NewNotFound(kcptenancyv1alphav1.Resource("workspacetypes"), "existing-workspace-org")).Once() - m.EXPECT().Create(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). - RunAndReturn(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + RunAndReturn(func(ctx context.Context, key types.NamespacedName, obj client.Object, opts ...client.GetOption) error { + wt := obj.(*kcptenancyv1alphav1.WorkspaceType) + wt.Name = "existing-workspace-org" + return nil + }).Once() + m.EXPECT().Patch(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). + RunAndReturn(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { wt := obj.(*kcptenancyv1alphav1.WorkspaceType) assert.Equal(t, "existing-workspace-org", wt.Name) assert.Equal(t, "existing-workspace", wt.Spec.AuthenticationConfigurations[0].Name) @@ -121,9 +133,13 @@ func TestWorkspaceAuthSubroutine_Process(t *testing.T) { }).Once() m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "existing-workspace-acc"}, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). - Return(apierrors.NewNotFound(kcptenancyv1alphav1.Resource("workspacetypes"), "existing-workspace-acc")).Once() - m.EXPECT().Create(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). - RunAndReturn(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + RunAndReturn(func(ctx context.Context, key types.NamespacedName, obj client.Object, opts ...client.GetOption) error { + wt := obj.(*kcptenancyv1alphav1.WorkspaceType) + wt.Name = "existing-workspace-acc" + return nil + }).Once() + m.EXPECT().Patch(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). + RunAndReturn(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { wt := obj.(*kcptenancyv1alphav1.WorkspaceType) assert.Equal(t, "existing-workspace-acc", wt.Name) assert.Equal(t, "existing-workspace", wt.Spec.AuthenticationConfigurations[0].Name) @@ -240,9 +256,13 @@ func TestWorkspaceAuthSubroutine_Process(t *testing.T) { }).Once() m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "single-workspace-org"}, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). - Return(apierrors.NewNotFound(kcptenancyv1alphav1.Resource("workspacetypes"), "single-workspace-org")).Once() - m.EXPECT().Create(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). - RunAndReturn(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + RunAndReturn(func(ctx context.Context, key types.NamespacedName, obj client.Object, opts ...client.GetOption) error { + wt := obj.(*kcptenancyv1alphav1.WorkspaceType) + wt.Name = "single-workspace-org" + return nil + }).Once() + m.EXPECT().Patch(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). + RunAndReturn(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { wt := obj.(*kcptenancyv1alphav1.WorkspaceType) assert.Equal(t, "single-workspace-org", wt.Name) assert.Equal(t, "single-workspace", wt.Spec.AuthenticationConfigurations[0].Name) @@ -250,9 +270,13 @@ func TestWorkspaceAuthSubroutine_Process(t *testing.T) { }).Once() m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "single-workspace-acc"}, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). - Return(apierrors.NewNotFound(kcptenancyv1alphav1.Resource("workspacetypes"), "single-workspace-acc")).Once() - m.EXPECT().Create(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). - RunAndReturn(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + RunAndReturn(func(ctx context.Context, key types.NamespacedName, obj client.Object, opts ...client.GetOption) error { + wt := obj.(*kcptenancyv1alphav1.WorkspaceType) + wt.Name = "single-workspace-acc" + return nil + }).Once() + m.EXPECT().Patch(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). + RunAndReturn(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { wt := obj.(*kcptenancyv1alphav1.WorkspaceType) assert.Equal(t, "single-workspace-acc", wt.Name) assert.Equal(t, "single-workspace", wt.Spec.AuthenticationConfigurations[0].Name) @@ -298,9 +322,13 @@ func TestWorkspaceAuthSubroutine_Process(t *testing.T) { }).Once() m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "single-workspace-org"}, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). - Return(apierrors.NewNotFound(kcptenancyv1alphav1.Resource("workspacetypes"), "single-workspace-org")).Once() - m.EXPECT().Create(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). - RunAndReturn(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + RunAndReturn(func(ctx context.Context, key types.NamespacedName, obj client.Object, opts ...client.GetOption) error { + wt := obj.(*kcptenancyv1alphav1.WorkspaceType) + wt.Name = "single-workspace-org" + return nil + }).Once() + m.EXPECT().Patch(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). + RunAndReturn(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { wt := obj.(*kcptenancyv1alphav1.WorkspaceType) assert.Equal(t, "single-workspace-org", wt.Name) assert.Equal(t, "single-workspace", wt.Spec.AuthenticationConfigurations[0].Name) @@ -308,9 +336,13 @@ func TestWorkspaceAuthSubroutine_Process(t *testing.T) { }).Once() m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "single-workspace-acc"}, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). - Return(apierrors.NewNotFound(kcptenancyv1alphav1.Resource("workspacetypes"), "single-workspace-acc")).Once() - m.EXPECT().Create(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). - RunAndReturn(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + RunAndReturn(func(ctx context.Context, key types.NamespacedName, obj client.Object, opts ...client.GetOption) error { + wt := obj.(*kcptenancyv1alphav1.WorkspaceType) + wt.Name = "single-workspace-acc" + return nil + }).Once() + m.EXPECT().Patch(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). + RunAndReturn(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { wt := obj.(*kcptenancyv1alphav1.WorkspaceType) assert.Equal(t, "single-workspace-acc", wt.Name) assert.Equal(t, "single-workspace", wt.Spec.AuthenticationConfigurations[0].Name) @@ -336,9 +368,13 @@ func TestWorkspaceAuthSubroutine_Process(t *testing.T) { m.EXPECT().Create(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceAuthenticationConfiguration"), mock.Anything).Return(nil).Once() m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "test-workspace-org"}, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). - Return(apierrors.NewNotFound(kcptenancyv1alphav1.Resource("workspacetypes"), "test-workspace-org")).Once() - m.EXPECT().Create(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). - Return(errors.New("failed to create workspace type")).Once() + RunAndReturn(func(ctx context.Context, key types.NamespacedName, obj client.Object, opts ...client.GetOption) error { + wt := obj.(*kcptenancyv1alphav1.WorkspaceType) + wt.Name = "test-workspace-org" + return nil + }).Once() + m.EXPECT().Patch(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). + Return(errors.New("failed to patch workspace type")).Once() }, expectError: true, expectedResult: ctrl.Result{}, @@ -359,13 +395,21 @@ func TestWorkspaceAuthSubroutine_Process(t *testing.T) { m.EXPECT().Create(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceAuthenticationConfiguration"), mock.Anything).Return(nil).Once() m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "test-workspace-org"}, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). - Return(apierrors.NewNotFound(kcptenancyv1alphav1.Resource("workspacetypes"), "test-workspace-org")).Once() - m.EXPECT().Create(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything).Return(nil).Once() + RunAndReturn(func(ctx context.Context, key types.NamespacedName, obj client.Object, opts ...client.GetOption) error { + wt := obj.(*kcptenancyv1alphav1.WorkspaceType) + wt.Name = "test-workspace-org" + return nil + }).Once() + m.EXPECT().Patch(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything).Return(nil).Once() m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "test-workspace-acc"}, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). - Return(apierrors.NewNotFound(kcptenancyv1alphav1.Resource("workspacetypes"), "test-workspace-acc")).Once() - m.EXPECT().Create(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). - Return(errors.New("failed to create workspace type")).Once() + RunAndReturn(func(ctx context.Context, key types.NamespacedName, obj client.Object, opts ...client.GetOption) error { + wt := obj.(*kcptenancyv1alphav1.WorkspaceType) + wt.Name = "test-workspace-acc" + return nil + }).Once() + m.EXPECT().Patch(mock.Anything, mock.AnythingOfType("*v1alpha1.WorkspaceType"), mock.Anything). + Return(errors.New("failed to patch workspace type")).Once() }, expectError: true, expectedResult: ctrl.Result{}, From 908dd146ea0d60f48abd9a6b0a32486fe23da8a8 Mon Sep 17 00:00:00 2001 From: OlegErshov Date: Fri, 19 Dec 2025 11:34:54 +0100 Subject: [PATCH 19/22] fix: added equality check before patching workspace type On-behalf-of: SAP aleh.yarshou@sap.com --- internal/controller/initializer_controller.go | 2 +- internal/subroutine/workspace_authorization.go | 13 +++++++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/internal/controller/initializer_controller.go b/internal/controller/initializer_controller.go index fbcb3be..5711d7b 100644 --- a/internal/controller/initializer_controller.go +++ b/internal/controller/initializer_controller.go @@ -31,9 +31,9 @@ func NewLogicalClusterReconciler(log *logger.Logger, orgClient client.Client, cf log: log, mclifecycle: builder.NewBuilder("logicalcluster", "LogicalClusterReconciler", []lifecyclesubroutine.Subroutine{ subroutine.NewWorkspaceInitializer(orgClient, cfg, mgr), - subroutine.NewWorkspaceAuthConfigurationSubroutine(orgClient, inClusterClient, cfg), subroutine.NewIDPSubroutine(orgClient, mgr, cfg), subroutine.NewInviteSubroutine(orgClient, mgr), + subroutine.NewWorkspaceAuthConfigurationSubroutine(orgClient, inClusterClient, cfg), subroutine.NewRemoveInitializer(mgr, cfg), }, log). WithReadOnly(). diff --git a/internal/subroutine/workspace_authorization.go b/internal/subroutine/workspace_authorization.go index 56d4b79..738169f 100644 --- a/internal/subroutine/workspace_authorization.go +++ b/internal/subroutine/workspace_authorization.go @@ -10,7 +10,9 @@ import ( lifecyclesubroutine "github.com/platform-mesh/golang-commons/controller/lifecycle/subroutine" "github.com/platform-mesh/golang-commons/errors" "github.com/platform-mesh/security-operator/internal/config" + "github.com/rs/zerolog/log" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" @@ -117,11 +119,18 @@ func (r *workspaceAuthSubroutine) patchWorkspaceType(ctx context.Context, cl cli return fmt.Errorf("failed to get WorkspaceType: %w", err) } - original := wsType.DeepCopy() - wsType.Spec.AuthenticationConfigurations = []kcptenancyv1alphav1.AuthenticationConfigurationReference{ + desiredAuthConfig := []kcptenancyv1alphav1.AuthenticationConfigurationReference{ {Name: authConfigurationRefName}, } + if equality.Semantic.DeepEqual(wsType.Spec.AuthenticationConfigurations, desiredAuthConfig) { + log.Debug().Msg(fmt.Sprintf("workspaceType %s already has authentication configuration, skip patching", workspaceTypeName)) + return nil + } + + original := wsType.DeepCopy() + wsType.Spec.AuthenticationConfigurations = desiredAuthConfig + if err := cl.Patch(ctx, wsType, client.MergeFrom(original)); err != nil { return fmt.Errorf("failed to patch WorkspaceType: %w", err) } From da02ebdb34d9674237d1183176697dcc37ad3ddb Mon Sep 17 00:00:00 2001 From: OlegErshov Date: Mon, 22 Dec 2025 11:50:38 +0100 Subject: [PATCH 20/22] feat: simplified client id look up On-behalf-of: SAP aleh.yarshou@sap.com --- internal/subroutine/invite/subroutine.go | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/internal/subroutine/invite/subroutine.go b/internal/subroutine/invite/subroutine.go index a23e6ae..c61d21e 100644 --- a/internal/subroutine/invite/subroutine.go +++ b/internal/subroutine/invite/subroutine.go @@ -7,7 +7,6 @@ import ( "fmt" "net/http" "net/url" - "slices" "github.com/coreos/go-oidc" accountsv1alpha1 "github.com/platform-mesh/account-operator/api/v1alpha1" @@ -252,13 +251,9 @@ func (s *subroutine) getClientID(ctx context.Context, cl client.Client, clientNa return "", err } - clientIdx := slices.IndexFunc(idp.Spec.Clients, func(c v1alpha1.IdentityProviderClientConfig) bool { - return c.ClientName == clientName - }) - - if clientIdx == -1 { - return "", fmt.Errorf("client %s not found", clientName) + client, ok := idp.Status.ManagedClients[clientName] + if !ok { + return "", fmt.Errorf("client %s not found in idp %s", clientName, idp.Name) } - - return idp.Spec.Clients[clientIdx].ClientID, nil + return client.ClientID, nil } From 2318d0896d15f76225ce3fc5d383934a91618459 Mon Sep 17 00:00:00 2001 From: OlegErshov Date: Mon, 22 Dec 2025 11:56:53 +0100 Subject: [PATCH 21/22] fix: updated tests On-behalf-of: SAP aleh.yarshou@sap.com --- internal/subroutine/invite/subroutine_test.go | 49 +++++++++++-------- 1 file changed, 29 insertions(+), 20 deletions(-) diff --git a/internal/subroutine/invite/subroutine_test.go b/internal/subroutine/invite/subroutine_test.go index fa1630a..6b44f0c 100644 --- a/internal/subroutine/invite/subroutine_test.go +++ b/internal/subroutine/invite/subroutine_test.go @@ -18,6 +18,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "golang.org/x/oauth2" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -83,11 +84,13 @@ func TestSubroutineProcess(t *testing.T) { m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "acme"}, mock.AnythingOfType("*v1alpha1.IdentityProviderConfiguration"), mock.Anything). RunAndReturn(func(ctx context.Context, nn types.NamespacedName, o client.Object, opts ...client.GetOption) error { idp := &v1alpha1.IdentityProviderConfiguration{ - Spec: v1alpha1.IdentityProviderConfigurationSpec{ - Clients: []v1alpha1.IdentityProviderClientConfig{ - { - ClientName: "acme", - ClientID: "acme", + ObjectMeta: metav1.ObjectMeta{ + Name: "acme", + }, + Status: v1alpha1.IdentityProviderConfigurationStatus{ + ManagedClients: map[string]v1alpha1.ManagedClient{ + "acme": { + ClientID: "acme", }, }, }, @@ -182,11 +185,13 @@ func TestSubroutineProcess(t *testing.T) { m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "acme"}, mock.AnythingOfType("*v1alpha1.IdentityProviderConfiguration"), mock.Anything). RunAndReturn(func(ctx context.Context, nn types.NamespacedName, o client.Object, opts ...client.GetOption) error { idp := &v1alpha1.IdentityProviderConfiguration{ - Spec: v1alpha1.IdentityProviderConfigurationSpec{ - Clients: []v1alpha1.IdentityProviderClientConfig{ - { - ClientName: "acme", - ClientID: "acme", + ObjectMeta: metav1.ObjectMeta{ + Name: "acme", + }, + Status: v1alpha1.IdentityProviderConfigurationStatus{ + ManagedClients: map[string]v1alpha1.ManagedClient{ + "acme": { + ClientID: "acme", }, }, }, @@ -255,11 +260,13 @@ func TestSubroutineProcess(t *testing.T) { m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "acme"}, mock.AnythingOfType("*v1alpha1.IdentityProviderConfiguration"), mock.Anything). RunAndReturn(func(ctx context.Context, nn types.NamespacedName, o client.Object, opts ...client.GetOption) error { idp := &v1alpha1.IdentityProviderConfiguration{ - Spec: v1alpha1.IdentityProviderConfigurationSpec{ - Clients: []v1alpha1.IdentityProviderClientConfig{ - { - ClientName: "acme", - ClientID: "acme", + ObjectMeta: metav1.ObjectMeta{ + Name: "acme", + }, + Status: v1alpha1.IdentityProviderConfigurationStatus{ + ManagedClients: map[string]v1alpha1.ManagedClient{ + "acme": { + ClientID: "acme", }, }, }, @@ -316,11 +323,13 @@ func TestSubroutineProcess(t *testing.T) { m.EXPECT().Get(mock.Anything, types.NamespacedName{Name: "acme"}, mock.AnythingOfType("*v1alpha1.IdentityProviderConfiguration"), mock.Anything). RunAndReturn(func(ctx context.Context, nn types.NamespacedName, o client.Object, opts ...client.GetOption) error { idp := &v1alpha1.IdentityProviderConfiguration{ - Spec: v1alpha1.IdentityProviderConfigurationSpec{ - Clients: []v1alpha1.IdentityProviderClientConfig{ - { - ClientName: "acme", - ClientID: "acme", + ObjectMeta: metav1.ObjectMeta{ + Name: "acme", + }, + Status: v1alpha1.IdentityProviderConfigurationStatus{ + ManagedClients: map[string]v1alpha1.ManagedClient{ + "acme": { + ClientID: "acme", }, }, }, From a8c52912da38304580f07d1d9aff5521a71d4e8e Mon Sep 17 00:00:00 2001 From: OlegErshov Date: Mon, 22 Dec 2025 12:00:37 +0100 Subject: [PATCH 22/22] chore: added apiexport endpoint slice name config variable back On-behalf-of: SAP aleh.yarshou@sap.com --- internal/config/config.go | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 0367616..d6ea7c7 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -14,14 +14,15 @@ type Config struct { KCP struct { Kubeconfig string `mapstructure:"kcp-kubeconfig" default:"/api-kubeconfig/kubeconfig"` } `mapstructure:",squash"` - CoreModulePath string `mapstructure:"core-module-path"` - BaseDomain string `mapstructure:"base-domain" default:"portal.dev.local:8443"` - GroupClaim string `mapstructure:"group-claim" default:"groups"` - UserClaim string `mapstructure:"user-claim" default:"email"` - InitializerName string `mapstructure:"initializer-name" default:"root:security"` - DomainCALookup bool `mapstructure:"domain-ca-lookup" default:"false"` - HttpClientTimeoutSeconds int `mapstructure:"http-client-timeout-seconds" default:"30"` - IDP struct { + APIExportEndpointSliceName string `mapstructure:"api-export-endpoint-slice-name"` + CoreModulePath string `mapstructure:"core-module-path"` + BaseDomain string `mapstructure:"base-domain" default:"portal.dev.local:8443"` + GroupClaim string `mapstructure:"group-claim" default:"groups"` + UserClaim string `mapstructure:"user-claim" default:"email"` + InitializerName string `mapstructure:"initializer-name" default:"root:security"` + DomainCALookup bool `mapstructure:"domain-ca-lookup" default:"false"` + HttpClientTimeoutSeconds int `mapstructure:"http-client-timeout-seconds" default:"30"` + IDP struct { // SMTP settings SMTPServer string `mapstructure:"idp-smtp-server"` SMTPPort int `mapstructure:"idp-smtp-port"`