From d4912a6a6d2fb1471910fa7f482a331a9c9be344 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20Echterh=C3=B6lter?= Date: Fri, 12 Apr 2024 13:59:08 +0200 Subject: [PATCH] feat: using createOrUpdate and incorporating PR feedback --- api/v1alpha1/account_types.go | 16 ++--- api/v1alpha1/zz_generated.deepcopy.go | 9 ++- chart/crds/core.openmfp.io_accounts.yaml | 20 +++--- .../controller/account_controller_test.go | 8 +-- internal/subroutines/namespace.go | 64 +++++++------------ internal/subroutines/namespace_test.go | 22 +++---- 6 files changed, 60 insertions(+), 79 deletions(-) diff --git a/api/v1alpha1/account_types.go b/api/v1alpha1/account_types.go index 4abd131..52c9065 100644 --- a/api/v1alpha1/account_types.go +++ b/api/v1alpha1/account_types.go @@ -20,28 +20,28 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -type AccountRole string +type AccountType string const ( - AccountRoleFolder AccountRole = "Folder" - AccountRoleAccount AccountRole = "Account" + AccountTypeFolder AccountType = "folder" + AccountTypeAccount AccountType = "account" ) // AccountSpec defines the desired state of Account type AccountSpec struct { - // AccountRole specifies the intended role for this Account object. + // Type specifies the intended type for this Account object. // +kubebuilder:validation:Enum=Folder;Account - AccountRole AccountRole `json:"accountRole"` + Type AccountType `json:"type"` - // Existing Namespace is the account should take ownership of - ExistingNamespace *string `json:"existingNamespace,omitempty"` + // Namespace is the account should take ownership of + Namespace *string `json:"namespace,omitempty"` // The display name for this account // +kubebuilder:validation:MaxLength=255 DisplayName string `json:"displayName"` // An optional description for this account - Description string `json:"description,omitempty"` + Description *string `json:"description,omitempty"` // The initial creator of this account Creator string `json:"creator"` diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index f6e0066..83210a4 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -87,8 +87,13 @@ func (in *AccountList) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *AccountSpec) DeepCopyInto(out *AccountSpec) { *out = *in - if in.ExistingNamespace != nil { - in, out := &in.ExistingNamespace, &out.ExistingNamespace + if in.Namespace != nil { + in, out := &in.Namespace, &out.Namespace + *out = new(string) + **out = **in + } + if in.Description != nil { + in, out := &in.Description, &out.Description *out = new(string) **out = **in } diff --git a/chart/crds/core.openmfp.io_accounts.yaml b/chart/crds/core.openmfp.io_accounts.yaml index e9985b3..607592d 100644 --- a/chart/crds/core.openmfp.io_accounts.yaml +++ b/chart/crds/core.openmfp.io_accounts.yaml @@ -52,13 +52,6 @@ spec: spec: description: AccountSpec defines the desired state of Account properties: - accountRole: - description: AccountRole specifies the intended role for this Account - object. - enum: - - Folder - - Account - type: string creator: description: The initial creator of this account type: string @@ -69,14 +62,19 @@ spec: description: The display name for this account maxLength: 255 type: string - existingNamespace: - description: Existing Namespace is the account should take ownership - of + namespace: + description: Namespace is the account should take ownership of + type: string + type: + description: Type specifies the intended type for this Account object. + enum: + - Folder + - Account type: string required: - - accountRole - creator - displayName + - type type: object status: description: AccountStatus defines the observed state of Account diff --git a/internal/controller/account_controller_test.go b/internal/controller/account_controller_test.go index 6d4c66e..64837e5 100644 --- a/internal/controller/account_controller_test.go +++ b/internal/controller/account_controller_test.go @@ -112,7 +112,7 @@ func (suite *AccountTestSuite) TestAddingFinalizer() { Namespace: defaultNamespace, }, Spec: corev1alpha1.AccountSpec{ - AccountRole: corev1alpha1.AccountRoleFolder, + Type: corev1alpha1.AccountTypeFolder, }} // When @@ -142,7 +142,7 @@ func (suite *AccountTestSuite) TestNamespaceCreation() { Namespace: defaultNamespace, }, Spec: corev1alpha1.AccountSpec{ - AccountRole: corev1alpha1.AccountRoleFolder, + Type: corev1alpha1.AccountTypeFolder, }} // When @@ -174,8 +174,8 @@ func (suite *AccountTestSuite) TestNamespaceUsingExisitingNamespace() { Namespace: defaultNamespace, }, Spec: corev1alpha1.AccountSpec{ - AccountRole: corev1alpha1.AccountRoleFolder, - ExistingNamespace: &existingNamespaceName, + Type: corev1alpha1.AccountTypeFolder, + Namespace: &existingNamespaceName, }} nsToCreate := &v1.Namespace{ObjectMeta: metaV1.ObjectMeta{Name: existingNamespaceName}} diff --git a/internal/subroutines/namespace.go b/internal/subroutines/namespace.go index b5e7108..fe27cd0 100644 --- a/internal/subroutines/namespace.go +++ b/internal/subroutines/namespace.go @@ -4,16 +4,14 @@ import ( "context" v1 "k8s.io/api/core/v1" - kerrors "k8s.io/apimachinery/pkg/api/errors" 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" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" corev1alpha1 "github.com/openmfp/account-operator/api/v1alpha1" "github.com/openmfp/golang-commons/controller/lifecycle" "github.com/openmfp/golang-commons/errors" - "github.com/openmfp/golang-commons/logger" ) const ( @@ -50,43 +48,20 @@ func (r *NamespaceSubroutine) Process(ctx context.Context, runtimeObj lifecycle. // Test if namespace was already created based on status createdNamespace := &v1.Namespace{} if instance.Status.Namespace != nil { - - // Test if namespace exists - err := r.client.Get(ctx, types.NamespacedName{Name: *instance.Status.Namespace}, createdNamespace) - if err != nil { - if kerrors.IsNotFound(err) { - - // Namespace does not exist, create it - createdNamespace = generateNamespace(instance) - err = r.client.Create(ctx, createdNamespace) - if err != nil { - return ctrl.Result{}, errors.NewOperatorError(err, true, true) - } - // Processing completed - return ctrl.Result{}, nil - } - return ctrl.Result{}, errors.NewOperatorError(err, true, true) - } - - // Namespace exists, verify labels - err = r.ensureNamespaceLabels(ctx, createdNamespace, instance) + createdNamespace = generateNamespace(instance) + _, err := controllerutil.CreateOrUpdate(ctx, r.client, createdNamespace, func() error { + setNamespaceLabels(createdNamespace, instance) + return nil + }) if err != nil { return ctrl.Result{}, errors.NewOperatorError(err, true, true) } } else { - if instance.Spec.ExistingNamespace != nil { - // Verify if namespace exists - err := r.client.Get(ctx, types.NamespacedName{Name: *instance.Spec.ExistingNamespace}, createdNamespace) - if err != nil { - if kerrors.IsNotFound(err) { - // Provided existing namespace does not exist - return ctrl.Result{}, errors.NewOperatorError(err, false, false) - } - return ctrl.Result{}, errors.NewOperatorError(err, true, true) - } - - // Namespace exists, ensure labels - err = r.ensureNamespaceLabels(ctx, createdNamespace, instance) + if instance.Spec.Namespace != nil { + createdNamespace = &v1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: *instance.Spec.Namespace}} + _, err := controllerutil.CreateOrUpdate(ctx, r.client, createdNamespace, func() error { + return setNamespaceLabels(createdNamespace, instance) + }) if err != nil { return ctrl.Result{}, errors.NewOperatorError(err, true, true) } @@ -104,21 +79,26 @@ func (r *NamespaceSubroutine) Process(ctx context.Context, runtimeObj lifecycle. return ctrl.Result{}, nil } -func (r *NamespaceSubroutine) ensureNamespaceLabels(ctx context.Context, ns *v1.Namespace, instance *corev1alpha1.Account) error { +var NamespaceOwnedByAnotherAccountErr = errors.New("Namespace already owned by another account") +var NamespaceOwnedByAnAccountInAnotherNamespaceErr = errors.New("Namespace already owned by another account in another namespace") + +func setNamespaceLabels(ns *v1.Namespace, instance *corev1alpha1.Account) error { hasOwnerLabel := verifyLabel(NamespaceAccountOwnerLabel, instance.GetName(), ns.Labels) hasOwnerNamespaceLabel := verifyLabel(NamespaceAccountOwnerNamespaceLabel, instance.GetNamespace(), ns.Labels) + if hasOwnerLabel && instance.Labels[NamespaceAccountOwnerLabel] != instance.GetName() { + return NamespaceOwnedByAnotherAccountErr + } + if hasOwnerNamespaceLabel && instance.Labels[NamespaceAccountOwnerNamespaceLabel] != instance.GetNamespace() { + return NamespaceOwnedByAnAccountInAnotherNamespaceErr + } + if !hasOwnerLabel || !hasOwnerNamespaceLabel { if ns.Labels == nil { ns.Labels = make(map[string]string) } ns.Labels[NamespaceAccountOwnerLabel] = instance.GetName() ns.Labels[NamespaceAccountOwnerNamespaceLabel] = instance.GetNamespace() - err := r.client.Update(ctx, ns) - if err != nil { - logger.LoadLoggerFromContext(ctx).Error().Err(err).Msg("Failed to update namespace labels") - return err - } } return nil } diff --git a/internal/subroutines/namespace_test.go b/internal/subroutines/namespace_test.go index 801d4fb..299e411 100644 --- a/internal/subroutines/namespace_test.go +++ b/internal/subroutines/namespace_test.go @@ -218,12 +218,12 @@ func (suite *NamespaceSubroutineTestSuite) TestProcessingWithNamespaceInStatusAn suite.Nil(err) } -func (suite *NamespaceSubroutineTestSuite) TestProcessingWithExistingNamespace_OK() { +func (suite *NamespaceSubroutineTestSuite) TestProcessingWithDeclaredNamespace_OK() { // Given namespaceName := "a-names-space" testAccount := &corev1alpha1.Account{ Spec: corev1alpha1.AccountSpec{ - ExistingNamespace: &namespaceName, + Namespace: &namespaceName, }, } mockGetNamespaceCallWithLabels(suite, namespaceName, map[string]string{ @@ -242,34 +242,32 @@ func (suite *NamespaceSubroutineTestSuite) TestProcessingWithExistingNamespace_O } -// Test processing with an account specifying an existing namespace that does not exist -func (suite *NamespaceSubroutineTestSuite) TestProcessingWithExistingNamespaceNotFound() { +func (suite *NamespaceSubroutineTestSuite) TestProcessingWithDeclaredNamespaceNotFound() { // Given namespaceName := "a-names-space" testAccount := &corev1alpha1.Account{ Spec: corev1alpha1.AccountSpec{ - ExistingNamespace: &namespaceName, + Namespace: &namespaceName, }, } mockGetNamespaceCallNotFound(suite) + mockNewNamespaceCreateCall(suite, namespaceName) + // When _, err := suite.testObj.Process(context.Background(), testAccount) // Then - suite.Nil(testAccount.Status.Namespace) - suite.NotNil(err) - suite.False(err.Retry()) - suite.False(err.Sentry()) + suite.Equal(namespaceName, *testAccount.Status.Namespace) + suite.Nil(err) } -// Test like TestProcessingWithExistingNamespaceNotFound but namespace lookup fails unexpectedly -func (suite *NamespaceSubroutineTestSuite) TestProcessingWithExistingNamespaceLookupError() { +func (suite *NamespaceSubroutineTestSuite) TestProcessingWithDeclaredNamespaceLookupError() { // Given namespaceName := "a-names-space" testAccount := &corev1alpha1.Account{ Spec: corev1alpha1.AccountSpec{ - ExistingNamespace: &namespaceName, + Namespace: &namespaceName, }, } suite.clientMock.EXPECT().