[feat] LOBJ controller updated for mutable fields LinodeObjectStorage…

… Finalizer removed (#456)

* lobj-controller-update

* refactoring update logic

* adding more testcases

* adding testcases

api/v1alpha1/conversion.go

@@ -51,7 +51,6 @@ func Convert_v1alpha1_LinodeObjectStorageBucketSpec_To_v1alpha2_LinodeObjectStor
 	// WARNING: in.Cluster requires manual conversion: does not exist in peer-type
 	out.Region = in.Cluster
 	out.CredentialsRef = in.CredentialsRef
-	out.SecretType = in.SecretType
 	return nil
 }
 func Convert_v1alpha1_LinodeObjectStorageBucketStatus_To_v1alpha2_LinodeObjectStorageBucketStatus(in *LinodeObjectStorageBucketStatus, out *infrastructurev1alpha2.LinodeObjectStorageBucketStatus, s conversion.Scope) error {
@@ -71,7 +70,7 @@ func Convert_v1alpha2_LinodeObjectStorageBucketSpec_To_v1alpha1_LinodeObjectStor
 	out.Cluster = in.Region
 	out.CredentialsRef = in.CredentialsRef
 	out.KeyGeneration = ptr.To(0)
-	out.SecretType = in.SecretType
+	out.SecretType = DefaultSecretTypeObjectStorageBucket
 	return nil
 }
 

api/v1alpha1/linodeobjectstoragebucket_conversion_test.go

@@ -59,7 +59,6 @@ func TestLinodeObjectStorageBucketConvertTo(t *testing.T) {
 				Namespace: "default",
 				Name:      "cred-secret",
 			},
-			SecretType: "Opaque",
 		},
 		Status: infrav1alpha2.LinodeObjectStorageBucketStatus{},
 	}
@@ -95,15 +94,14 @@ func TestLinodeObjectStorageBucketFrom(t *testing.T) {
 				Namespace: "default",
 				Name:      "cred-secret",
 			},
-			SecretType: "Opaque",
 		},
 		Status: infrav1alpha2.LinodeObjectStorageBucketStatus{},
 	}
 	expectedDst := &LinodeObjectStorageBucket{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: "test-bucket",
 			Annotations: map[string]string{
-				ConversionDataAnnotation: `{"spec":{"credentialsRef":{"name":"cred-secret","namespace":"default"},"region":"us-mia-1","secretType":"Opaque"},"status":{"ready":false}}`,
+				ConversionDataAnnotation: `{"spec":{"credentialsRef":{"name":"cred-secret","namespace":"default"},"region":"us-mia-1"},"status":{"ready":false}}`,
 			},
 		},
 		Spec: LinodeObjectStorageBucketSpec{
@@ -113,7 +111,7 @@ func TestLinodeObjectStorageBucketFrom(t *testing.T) {
 				Name:      "cred-secret",
 			},
 			KeyGeneration: ptr.To(0),
-			SecretType:    "Opaque",
+			SecretType:    DefaultSecretTypeObjectStorageBucket,
 		},
 		Status: LinodeObjectStorageBucketStatus{},
 	}

api/v1alpha1/linodeobjectstoragebucket_types.go

@@ -25,7 +25,8 @@ import (
 const (
 	// ObjectStorageBucketFinalizer allows ReconcileLinodeObjectStorageBucket to clean up Linode resources associated
 	// with LinodeObjectStorageBucket before removing it from the apiserver.
-	ObjectStorageBucketFinalizer = "linodeobjectstoragebucket.infrastructure.cluster.x-k8s.io"
+	ObjectStorageBucketFinalizer         = "linodeobjectstoragebucket.infrastructure.cluster.x-k8s.io"
+	DefaultSecretTypeObjectStorageBucket = "addons.cluster.x-k8s.io/resource-set"
 )
 
 // LinodeObjectStorageBucketSpec defines the desired state of LinodeObjectStorageBucket

api/v1alpha2/linodeobjectstoragebucket_types.go

@@ -26,13 +26,10 @@ type ObjectStorageACL string
 
 // ObjectStorageACL options represent the access control level of a bucket.
 const (
-	// ObjectStorageBucketFinalizer allows ReconcileLinodeObjectStorageBucket to clean up Linode resources associated
-	// with LinodeObjectStorageBucket before removing it from the apiserver.
-	ObjectStorageBucketFinalizer                  = "linodeobjectstoragebucket.infrastructure.cluster.x-k8s.io"
-	ACLPrivate                   ObjectStorageACL = "private"
-	ACLPublicRead                ObjectStorageACL = "public-read"
-	ACLAuthenticatedRead         ObjectStorageACL = "authenticated-read"
-	ACLPublicReadWrite           ObjectStorageACL = "public-read-write"
+	ACLPrivate           ObjectStorageACL = "private"
+	ACLPublicRead        ObjectStorageACL = "public-read"
+	ACLAuthenticatedRead ObjectStorageACL = "authenticated-read"
+	ACLPublicReadWrite   ObjectStorageACL = "public-read-write"
 )
 
 // LinodeObjectStorageBucketSpec defines the desired state of LinodeObjectStorageBucket
@@ -44,7 +41,7 @@ type LinodeObjectStorageBucketSpec struct {
 	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="Value is immutable"
 	Region string `json:"region"`
 
-	// ACL sets The Access Control Level of the bucket using a canned ACL string
+	// Acl sets the Access Control Level of the bucket using a canned ACL string
 	// +optional
 	// +kubebuilder:default=private
 	// +kubebuilder:validation:Enum=private;public-read;authenticated-read;public-read-write
@@ -53,17 +50,12 @@ type LinodeObjectStorageBucketSpec struct {
 	// corsEnabled enables for all origins in the bucket .If set to false, CORS is disabled for all origins in the bucket
 	// +optional
 	// +kubebuilder:default=true
-	CorsEnabled *bool `json:"corsEnabled,omitempty"`
+	CorsEnabled bool `json:"corsEnabled,omitempty"`
 
 	// CredentialsRef is a reference to a Secret that contains the credentials to use for provisioning the bucket.
 	// If not supplied then the credentials of the controller will be used.
 	// +optional
 	CredentialsRef *corev1.SecretReference `json:"credentialsRef"`
-
-	// SecretType sets the type for the bucket-details secret that will be generated by the controller.
-	// +optional
-	// +kubebuilder:default=addons.cluster.x-k8s.io/resource-set
-	SecretType string `json:"secretType,omitempty"`
 }
 
 // LinodeObjectStorageBucketStatus defines the observed state of LinodeObjectStorageBucket

clients/clients.go

@@ -75,6 +75,8 @@ type LinodeNodeBalancerClient interface {
 type LinodeObjectStorageClient interface {
 	GetObjectStorageBucket(ctx context.Context, regionID, label string) (*linodego.ObjectStorageBucket, error)
 	CreateObjectStorageBucket(ctx context.Context, opts linodego.ObjectStorageBucketCreateOptions) (*linodego.ObjectStorageBucket, error)
+	GetObjectStorageBucketAccess(ctx context.Context, clusterOrRegionID, label string) (*linodego.ObjectStorageBucketAccess, error)
+	UpdateObjectStorageBucketAccess(ctx context.Context, clusterOrRegionID, label string, opts linodego.ObjectStorageBucketUpdateAccessOptions) error
 	GetObjectStorageKey(ctx context.Context, keyID int) (*linodego.ObjectStorageKey, error)
 	CreateObjectStorageKey(ctx context.Context, opts linodego.ObjectStorageKeyCreateOptions) (*linodego.ObjectStorageKey, error)
 	DeleteObjectStorageKey(ctx context.Context, keyID int) error

cloud/scope/object_storage_bucket.go

@@ -8,7 +8,6 @@ import (
 
 	"github.com/go-logr/logr"
 	"sigs.k8s.io/cluster-api/util/patch"
-	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	infrav1alpha2 "github.com/linode/cluster-api-provider-linode/api/v1alpha2"
 
@@ -88,13 +87,3 @@ func (s *ObjectStorageBucketScope) PatchObject(ctx context.Context) error {
 func (s *ObjectStorageBucketScope) Close(ctx context.Context) error {
 	return s.PatchObject(ctx)
 }
-
-// AddFinalizer adds a finalizer if not present and immediately patches the
-// object to avoid any race conditions.
-func (s *ObjectStorageBucketScope) AddFinalizer(ctx context.Context) error {
-	if controllerutil.AddFinalizer(s.Bucket, infrav1alpha2.ObjectStorageBucketFinalizer) {
-		return s.Close(ctx)
-	}
-
-	return nil
-}

cloud/scope/object_storage_bucket_test.go

@@ -9,7 +9,6 @@ import (
 	"github.com/stretchr/testify/assert"
 	"go.uber.org/mock/gomock"
 	corev1 "k8s.io/api/core/v1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -135,7 +134,7 @@ func TestNewObjectStorageBucketScope(t *testing.T) {
 			},
 		},
 		{
-			name: "Error - ValidateClusterScopeParams triggers error because ClusterScopeParams is empty",
+			name: "Error - ValidateObjectStorageBucketScopeParams triggers error because ObjectStorageBucketScopeParams is empty",
 			args: args{
 				apiKey: "apikey",
 				params: ObjectStorageBucketScopeParams{},
@@ -159,7 +158,7 @@ func TestNewObjectStorageBucketScope(t *testing.T) {
 			},
 		},
 		{
-			name: "Error - Using getCredentialDataFromRef(), func returns an error. Unable to create a valid ClusterScope",
+			name: "Error - Using getCredentialDataFromRef(), func returns an error. Unable to create a valid ObjectStorageBucketScope",
 			args: args{
 				apiKey: "test-key",
 				params: ObjectStorageBucketScopeParams{
@@ -181,7 +180,7 @@ func TestNewObjectStorageBucketScope(t *testing.T) {
 			},
 		},
 		{
-			name: "Error - createLinodeCluster throws an error for passing empty apiKey. Unable to create a valid ClusterScope",
+			name: "Error - createObjectStorageBucket throws an error for passing empty apiKey. Unable to create a valid ObjectStorageBucketScope",
 			args: args{
 				apiKey: "",
 				params: ObjectStorageBucketScopeParams{
@@ -193,6 +192,21 @@ func TestNewObjectStorageBucketScope(t *testing.T) {
 			expectedErr: fmt.Errorf("failed to create linode client: token cannot be empty"),
 			expects:     func(mock *mock.MockK8sClient) {},
 		},
+		{
+			name: "Error - kind is not registered",
+			args: args{
+				apiKey: "apikey",
+				params: ObjectStorageBucketScopeParams{
+					Client: nil,
+					Bucket: &infrav1alpha2.LinodeObjectStorageBucket{},
+					Logger: &logr.Logger{},
+				},
+			},
+			expectedErr: fmt.Errorf("no kind is registered for the type v1alpha2.LinodeObjectStorageBucket"),
+			expects: func(k8s *mock.MockK8sClient) {
+				k8s.EXPECT().Scheme().Return(runtime.NewScheme())
+			},
+		},
 	}
 	for _, tt := range tests {
 		testcase := tt
@@ -218,73 +232,3 @@ func TestNewObjectStorageBucketScope(t *testing.T) {
 		})
 	}
 }
-
-func TestObjectStorageBucketScopeMethods(t *testing.T) {
-	t.Parallel()
-	tests := []struct {
-		name    string
-		Bucket  *infrav1alpha2.LinodeObjectStorageBucket
-		expects func(mock *mock.MockK8sClient)
-	}{
-		{
-			name:   "Success - finalizer should be added to the Linode Object Storage Bucket object",
-			Bucket: &infrav1alpha2.LinodeObjectStorageBucket{},
-			expects: func(mock *mock.MockK8sClient) {
-				mock.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme {
-					s := runtime.NewScheme()
-					infrav1alpha2.AddToScheme(s)
-					return s
-				}).Times(2)
-				mock.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
-			},
-		},
-		{
-			name: "Failure - finalizer should not be added to the Bucket object. Function returns nil since it was already present",
-			Bucket: &infrav1alpha2.LinodeObjectStorageBucket{
-				ObjectMeta: metav1.ObjectMeta{
-					Finalizers: []string{infrav1alpha2.ObjectStorageBucketFinalizer},
-				},
-			},
-			expects: func(mock *mock.MockK8sClient) {
-				mock.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme {
-					s := runtime.NewScheme()
-					infrav1alpha2.AddToScheme(s)
-					return s
-				}).Times(1)
-			},
-		},
-	}
-	for _, tt := range tests {
-		testcase := tt
-		t.Run(testcase.name, func(t *testing.T) {
-			t.Parallel()
-
-			ctrl := gomock.NewController(t)
-			defer ctrl.Finish()
-
-			mockK8sClient := mock.NewMockK8sClient(ctrl)
-
-			testcase.expects(mockK8sClient)
-
-			objScope, err := NewObjectStorageBucketScope(
-				context.Background(),
-				ClientConfig{Token: "test-key"},
-				ObjectStorageBucketScopeParams{
-					Client: mockK8sClient,
-					Bucket: testcase.Bucket,
-					Logger: &logr.Logger{},
-				})
-			if err != nil {
-				t.Errorf("NewObjectStorageBucketScope() error = %v", err)
-			}
-
-			if err := objScope.AddFinalizer(context.Background()); err != nil {
-				t.Errorf("ClusterScope.AddFinalizer() error = %v", err)
-			}
-
-			if objScope.Bucket.Finalizers[0] != infrav1alpha2.ObjectStorageBucketFinalizer {
-				t.Errorf("Finalizer was not added")
-			}
-		})
-	}
-}

cloud/services/object_storage_buckets.go

@@ -11,7 +11,7 @@ import (
 	"github.com/linode/cluster-api-provider-linode/util"
 )
 
-func EnsureObjectStorageBucket(ctx context.Context, bScope *scope.ObjectStorageBucketScope) (*linodego.ObjectStorageBucket, error) {
+func EnsureAndUpdateObjectStorageBucket(ctx context.Context, bScope *scope.ObjectStorageBucketScope) (*linodego.ObjectStorageBucket, error) {
 	bucket, err := bScope.LinodeClient.GetObjectStorageBucket(
 		ctx,
 		bScope.Bucket.Spec.Region,
@@ -20,24 +20,45 @@ func EnsureObjectStorageBucket(ctx context.Context, bScope *scope.ObjectStorageB
 	if util.IgnoreLinodeAPIError(err, http.StatusNotFound) != nil {
 		return nil, fmt.Errorf("failed to get bucket from region %s: %w", bScope.Bucket.Spec.Region, err)
 	}
-	if bucket != nil {
-		bScope.Logger.Info("Bucket exists")
+	if bucket == nil {
+		opts := linodego.ObjectStorageBucketCreateOptions{
+			Region:      bScope.Bucket.Spec.Region,
+			Label:       bScope.Bucket.Name,
+			ACL:         linodego.ObjectStorageACL(bScope.Bucket.Spec.ACL),
+			CorsEnabled: &bScope.Bucket.Spec.CorsEnabled,
+		}
+
+		if bucket, err = bScope.LinodeClient.CreateObjectStorageBucket(ctx, opts); err != nil {
+			return nil, fmt.Errorf("failed to create bucket: %w", err)
+		}
+
+		bScope.Logger.Info("Created bucket")
 
 		return bucket, nil
 	}
 
-	opts := linodego.ObjectStorageBucketCreateOptions{
-		Region:      bScope.Bucket.Spec.Region,
-		Label:       bScope.Bucket.Name,
-		ACL:         linodego.ObjectStorageACL(bScope.Bucket.Spec.ACL),
-		CorsEnabled: bScope.Bucket.Spec.CorsEnabled,
+	bucketAccess, err := bScope.LinodeClient.GetObjectStorageBucketAccess(
+		ctx,
+		bucket.Region,
+		bucket.Label,
+	)
+	if err != nil {
+		return nil, fmt.Errorf("failed to get bucket access details for %s: %w", bScope.Bucket.Name, err)
 	}
 
-	if bucket, err = bScope.LinodeClient.CreateObjectStorageBucket(ctx, opts); err != nil {
-		return nil, fmt.Errorf("failed to create bucket: %w", err)
+	if bucketAccess.ACL == linodego.ObjectStorageACL(bScope.Bucket.Spec.ACL) && bucketAccess.CorsEnabled == bScope.Bucket.Spec.CorsEnabled {
+		return bucket, nil
+	}
+
+	opts := linodego.ObjectStorageBucketUpdateAccessOptions{
+		ACL:         linodego.ObjectStorageACL(bScope.Bucket.Spec.ACL),
+		CorsEnabled: &bScope.Bucket.Spec.CorsEnabled,
+	}
+	if err = bScope.LinodeClient.UpdateObjectStorageBucketAccess(ctx, bucket.Region, bucket.Label, opts); err != nil {
+		return nil, fmt.Errorf("failed to update the bucket access options for %s: %w", bScope.Bucket.Name, err)
 	}
 
-	bScope.Logger.Info("Created bucket")
+	bScope.Logger.Info("Updated Bucket")
 
 	return bucket, nil
 }