feat: using createOrUpdate and incorporating PR feedback

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"`

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

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}}

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
 }

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().