From 7b6c00b1d14aaf2a8d00b0f8266b77d544b9039c Mon Sep 17 00:00:00 2001 From: Unnati Aggarwal <113070152+unnatiagg@users.noreply.github.com> Date: Fri, 16 Aug 2024 12:53:38 -0400 Subject: [PATCH] [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 | 3 +- ...nodeobjectstoragebucket_conversion_test.go | 6 +- .../linodeobjectstoragebucket_types.go | 3 +- api/v1alpha1/zz_generated.conversion.go | 3 +- .../linodeobjectstoragebucket_types.go | 20 ++-- api/v1alpha2/zz_generated.deepcopy.go | 5 - clients/clients.go | 2 + cloud/scope/object_storage_bucket.go | 11 --- cloud/scope/object_storage_bucket_test.go | 92 ++++--------------- cloud/services/object_storage_buckets.go | 43 ++++++--- cloud/services/object_storage_buckets_test.go | 82 ++++++++++++++++- ...r.x-k8s.io_linodeobjectstoragebuckets.yaml | 7 +- .../linodeobjectstoragebucket_controller.go | 33 +------ ...nodeobjectstoragebucket_controller_test.go | 86 ++++++++++------- mock/client.go | 58 ++++++++++++ .../wrappers/linodeclient/linodeclient.gen.go | 52 +++++++++++ 16 files changed, 308 insertions(+), 198 deletions(-) diff --git a/api/v1alpha1/conversion.go b/api/v1alpha1/conversion.go index ba45ceac2..f2abb7a48 100644 --- a/api/v1alpha1/conversion.go +++ b/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 } diff --git a/api/v1alpha1/linodeobjectstoragebucket_conversion_test.go b/api/v1alpha1/linodeobjectstoragebucket_conversion_test.go index 83f6276be..78c53e960 100644 --- a/api/v1alpha1/linodeobjectstoragebucket_conversion_test.go +++ b/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,7 +94,6 @@ func TestLinodeObjectStorageBucketFrom(t *testing.T) { Namespace: "default", Name: "cred-secret", }, - SecretType: "Opaque", }, Status: infrav1alpha2.LinodeObjectStorageBucketStatus{}, } @@ -103,7 +101,7 @@ func TestLinodeObjectStorageBucketFrom(t *testing.T) { 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{}, } diff --git a/api/v1alpha1/linodeobjectstoragebucket_types.go b/api/v1alpha1/linodeobjectstoragebucket_types.go index 16e01f1b2..cfbc42de1 100644 --- a/api/v1alpha1/linodeobjectstoragebucket_types.go +++ b/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 diff --git a/api/v1alpha1/zz_generated.conversion.go b/api/v1alpha1/zz_generated.conversion.go index 0c0672efd..f9f79e333 100644 --- a/api/v1alpha1/zz_generated.conversion.go +++ b/api/v1alpha1/zz_generated.conversion.go @@ -1014,7 +1014,7 @@ func autoConvert_v1alpha1_LinodeObjectStorageBucketSpec_To_v1alpha2_LinodeObject // WARNING: in.Cluster requires manual conversion: does not exist in peer-type out.CredentialsRef = (*v1.SecretReference)(unsafe.Pointer(in.CredentialsRef)) // WARNING: in.KeyGeneration requires manual conversion: does not exist in peer-type - out.SecretType = in.SecretType + // WARNING: in.SecretType requires manual conversion: does not exist in peer-type return nil } @@ -1023,7 +1023,6 @@ func autoConvert_v1alpha2_LinodeObjectStorageBucketSpec_To_v1alpha1_LinodeObject // WARNING: in.ACL requires manual conversion: does not exist in peer-type // WARNING: in.CorsEnabled requires manual conversion: does not exist in peer-type out.CredentialsRef = (*v1.SecretReference)(unsafe.Pointer(in.CredentialsRef)) - out.SecretType = in.SecretType return nil } diff --git a/api/v1alpha2/linodeobjectstoragebucket_types.go b/api/v1alpha2/linodeobjectstoragebucket_types.go index 4a3371444..9b74067ea 100644 --- a/api/v1alpha2/linodeobjectstoragebucket_types.go +++ b/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 diff --git a/api/v1alpha2/zz_generated.deepcopy.go b/api/v1alpha2/zz_generated.deepcopy.go index cbf30fdb6..437b1d388 100644 --- a/api/v1alpha2/zz_generated.deepcopy.go +++ b/api/v1alpha2/zz_generated.deepcopy.go @@ -838,11 +838,6 @@ func (in *LinodeObjectStorageBucketList) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *LinodeObjectStorageBucketSpec) DeepCopyInto(out *LinodeObjectStorageBucketSpec) { *out = *in - if in.CorsEnabled != nil { - in, out := &in.CorsEnabled, &out.CorsEnabled - *out = new(bool) - **out = **in - } if in.CredentialsRef != nil { in, out := &in.CredentialsRef, &out.CredentialsRef *out = new(v1.SecretReference) diff --git a/clients/clients.go b/clients/clients.go index 533b8fab0..93ff0d2a9 100644 --- a/clients/clients.go +++ b/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 diff --git a/cloud/scope/object_storage_bucket.go b/cloud/scope/object_storage_bucket.go index 5a77928db..4e3dd7f4e 100644 --- a/cloud/scope/object_storage_bucket.go +++ b/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 -} diff --git a/cloud/scope/object_storage_bucket_test.go b/cloud/scope/object_storage_bucket_test.go index e0c0a8064..2d204305a 100644 --- a/cloud/scope/object_storage_bucket_test.go +++ b/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") - } - }) - } -} diff --git a/cloud/services/object_storage_buckets.go b/cloud/services/object_storage_buckets.go index cd912dc57..528970da7 100644 --- a/cloud/services/object_storage_buckets.go +++ b/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 } diff --git a/cloud/services/object_storage_buckets_test.go b/cloud/services/object_storage_buckets_test.go index da29152de..601d32340 100644 --- a/cloud/services/object_storage_buckets_test.go +++ b/cloud/services/object_storage_buckets_test.go @@ -33,7 +33,9 @@ func TestEnsureObjectStorageBucket(t *testing.T) { Name: "test-bucket", }, Spec: infrav1alpha2.LinodeObjectStorageBucketSpec{ - Region: "test-region", + Region: "test-region", + ACL: infrav1alpha2.ACLPrivate, + CorsEnabled: true, }, }, }, @@ -44,6 +46,10 @@ func TestEnsureObjectStorageBucket(t *testing.T) { mockClient.EXPECT().GetObjectStorageBucket(gomock.Any(), gomock.Any(), gomock.Any()).Return(&linodego.ObjectStorageBucket{ Label: "test-bucket", }, nil) + mockClient.EXPECT().GetObjectStorageBucketAccess(gomock.Any(), gomock.Any(), gomock.Any()).Return(&linodego.ObjectStorageBucketAccess{ + ACL: linodego.ACLPrivate, + CorsEnabled: true, + }, nil) }, }, { @@ -103,6 +109,78 @@ func TestEnsureObjectStorageBucket(t *testing.T) { }, expectedError: fmt.Errorf("failed to create bucket:"), }, + { + name: "Success - Successfully update the OBJ bucket", + bScope: &scope.ObjectStorageBucketScope{ + Bucket: &infrav1alpha2.LinodeObjectStorageBucket{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-bucket", + }, + Spec: infrav1alpha2.LinodeObjectStorageBucketSpec{ + Region: "test-region", + ACL: infrav1alpha2.ACLPublicRead, + CorsEnabled: true, + }, + }, + }, + want: &linodego.ObjectStorageBucket{ + Label: "test-bucket", + }, + expects: func(mockClient *mock.MockLinodeClient) { + mockClient.EXPECT().GetObjectStorageBucket(gomock.Any(), gomock.Any(), gomock.Any()).Return(&linodego.ObjectStorageBucket{ + Label: "test-bucket", + }, nil) + mockClient.EXPECT().GetObjectStorageBucketAccess(gomock.Any(), gomock.Any(), gomock.Any()).Return(&linodego.ObjectStorageBucketAccess{ + ACL: linodego.ACLPrivate, + CorsEnabled: true, + }, nil) + mockClient.EXPECT().UpdateObjectStorageBucketAccess(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) + }, + }, + { + name: "Error - unable to update the OBJ bucket", + bScope: &scope.ObjectStorageBucketScope{ + Bucket: &infrav1alpha2.LinodeObjectStorageBucket{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-bucket", + }, + Spec: infrav1alpha2.LinodeObjectStorageBucketSpec{ + Region: "test-region", + }, + }, + }, + expects: func(mockClient *mock.MockLinodeClient) { + mockClient.EXPECT().GetObjectStorageBucket(gomock.Any(), gomock.Any(), gomock.Any()).Return(&linodego.ObjectStorageBucket{ + Label: "test-bucket", + }, nil) + mockClient.EXPECT().GetObjectStorageBucketAccess(gomock.Any(), gomock.Any(), gomock.Any()).Return(&linodego.ObjectStorageBucketAccess{ + ACL: linodego.ACLPrivate, + CorsEnabled: true, + }, nil) + mockClient.EXPECT().UpdateObjectStorageBucketAccess(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(fmt.Errorf("error in updating object storage bucket")) + }, + expectedError: fmt.Errorf("failed to update the bucket access options"), + }, + { + name: "Error - unable to fetch the OBJ bucket Access", + bScope: &scope.ObjectStorageBucketScope{ + Bucket: &infrav1alpha2.LinodeObjectStorageBucket{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-bucket", + }, + Spec: infrav1alpha2.LinodeObjectStorageBucketSpec{ + Region: "test-region", + }, + }, + }, + expects: func(mockClient *mock.MockLinodeClient) { + mockClient.EXPECT().GetObjectStorageBucket(gomock.Any(), gomock.Any(), gomock.Any()).Return(&linodego.ObjectStorageBucket{ + Label: "test-bucket", + }, nil) + mockClient.EXPECT().GetObjectStorageBucketAccess(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, fmt.Errorf("error in fetching object storage bucket access details")) + }, + expectedError: fmt.Errorf("failed to get bucket access details"), + }, } for _, tt := range tests { testcase := tt @@ -118,7 +196,7 @@ func TestEnsureObjectStorageBucket(t *testing.T) { testcase.expects(mockClient) - got, err := EnsureObjectStorageBucket(context.Background(), testcase.bScope) + got, err := EnsureAndUpdateObjectStorageBucket(context.Background(), testcase.bScope) if testcase.expectedError != nil { assert.ErrorContains(t, err, testcase.expectedError.Error()) } else { diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_linodeobjectstoragebuckets.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_linodeobjectstoragebuckets.yaml index ac9a20bd4..5eadb8d28 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_linodeobjectstoragebuckets.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_linodeobjectstoragebuckets.yaml @@ -224,7 +224,7 @@ spec: properties: acl: default: private - description: ACL sets The Access Control Level of the bucket using + description: Acl sets the Access Control Level of the bucket using a canned ACL string enum: - private @@ -259,11 +259,6 @@ spec: x-kubernetes-validations: - message: Value is immutable rule: self == oldSelf - secretType: - default: addons.cluster.x-k8s.io/resource-set - description: SecretType sets the type for the bucket-details secret - that will be generated by the controller. - type: string required: - region type: object diff --git a/controller/linodeobjectstoragebucket_controller.go b/controller/linodeobjectstoragebucket_controller.go index 1e3274d60..40ea8efc6 100644 --- a/controller/linodeobjectstoragebucket_controller.go +++ b/controller/linodeobjectstoragebucket_controller.go @@ -18,7 +18,6 @@ package controller import ( "context" - "errors" "fmt" "time" @@ -36,7 +35,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" crcontroller "sigs.k8s.io/controller-runtime/pkg/controller" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/predicate" @@ -117,15 +115,6 @@ func (r *LinodeObjectStorageBucketReconciler) reconcile(ctx context.Context, bSc reterr = err } }() - - if !bScope.Bucket.DeletionTimestamp.IsZero() { - return res, r.reconcileDelete(bScope) - } - - if err := bScope.AddFinalizer(ctx); err != nil { - return res, err - } - if err := r.reconcileApply(ctx, bScope); err != nil { return res, err } @@ -145,9 +134,9 @@ func (r *LinodeObjectStorageBucketReconciler) reconcileApply(ctx context.Context bScope.Bucket.Status.Ready = false bScope.Bucket.Status.FailureMessage = nil - bucket, err := services.EnsureObjectStorageBucket(ctx, bScope) + bucket, err := services.EnsureAndUpdateObjectStorageBucket(ctx, bScope) if err != nil { - bScope.Logger.Error(err, "Failed to ensure bucket exists") + bScope.Logger.Error(err, "Failed to ensure bucket or update bucket") r.setFailure(bScope, err) return err @@ -163,24 +152,6 @@ func (r *LinodeObjectStorageBucketReconciler) reconcileApply(ctx context.Context return nil } -func (r *LinodeObjectStorageBucketReconciler) reconcileDelete(bScope *scope.ObjectStorageBucketScope) error { - bScope.Logger.Info("Reconciling delete") - - if !controllerutil.RemoveFinalizer(bScope.Bucket, infrav1alpha2.ObjectStorageBucketFinalizer) { - err := errors.New("failed to remove finalizer from bucket; unable to delete") - bScope.Logger.Error(err, "controllerutil.RemoveFinalizer") - r.setFailure(bScope, err) - - return err - } - // TODO: remove this check and removal later - if controllerutil.ContainsFinalizer(bScope.Bucket, infrav1alpha2.GroupVersion.String()) { - controllerutil.RemoveFinalizer(bScope.Bucket, infrav1alpha2.GroupVersion.String()) - } - - return nil -} - // SetupWithManager sets up the controller with the Manager. // //nolint:dupl // This follows the pattern used for the LinodeObjectStorageBucket controller. diff --git a/controller/linodeobjectstoragebucket_controller_test.go b/controller/linodeobjectstoragebucket_controller_test.go index 5e05035ab..fef9464f0 100644 --- a/controller/linodeobjectstoragebucket_controller_test.go +++ b/controller/linodeobjectstoragebucket_controller_test.go @@ -25,9 +25,7 @@ import ( "go.uber.org/mock/gomock" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/client-go/kubernetes/scheme" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/cluster-api/util/patch" "sigs.k8s.io/controller-runtime/pkg/client" @@ -145,7 +143,7 @@ var _ = Describe("lifecycle", Ordered, Label("bucket", "lifecycle"), func() { }), ), Path( - Call("bucket is retrieved on update", func(ctx context.Context, mck Mock) { + Call("bucket access options are not retrieved on update", func(ctx context.Context, mck Mock) { mck.LinodeClient.EXPECT().GetObjectStorageBucket(gomock.Any(), obj.Spec.Region, gomock.Any()). Return(&linodego.ObjectStorageBucket{ Label: "bucket", @@ -153,6 +151,54 @@ var _ = Describe("lifecycle", Ordered, Label("bucket", "lifecycle"), func() { Hostname: "hostname", Created: util.Pointer(time.Now()), }, nil) + mck.LinodeClient.EXPECT().GetObjectStorageBucketAccess(gomock.Any(), gomock.Any(), gomock.Any()). + Return(nil, errors.New("bucket access options fetch error")) + }), + Result("error", func(ctx context.Context, mck Mock) { + bScope.LinodeClient = mck.LinodeClient + _, err := reconciler.reconcile(ctx, &bScope) + Expect(err.Error()).To(ContainSubstring("failed to get bucket access details")) + }), + ), + Path( + Call("bucket access options are not successfully updated", func(ctx context.Context, mck Mock) { + mck.LinodeClient.EXPECT().GetObjectStorageBucket(gomock.Any(), obj.Spec.Region, gomock.Any()). + Return(&linodego.ObjectStorageBucket{ + Label: "bucket", + Region: obj.Spec.Region, + Hostname: "hostname", + Created: util.Pointer(time.Now()), + }, nil) + mck.LinodeClient.EXPECT().GetObjectStorageBucketAccess(gomock.Any(), gomock.Any(), gomock.Any()). + Return(&linodego.ObjectStorageBucketAccess{ + ACL: linodego.ACLPrivate, + CorsEnabled: true, + }, nil) + mck.LinodeClient.EXPECT().UpdateObjectStorageBucketAccess(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(errors.New("bucket access options update error")) + }), + Result("error", func(ctx context.Context, mck Mock) { + bScope.LinodeClient = mck.LinodeClient + _, err := reconciler.reconcile(ctx, &bScope) + Expect(err.Error()).To(ContainSubstring("failed to update the bucket access options")) + }), + ), + Path( + Call("bucket is retrieved and updated", func(ctx context.Context, mck Mock) { + mck.LinodeClient.EXPECT().GetObjectStorageBucket(gomock.Any(), obj.Spec.Region, gomock.Any()). + Return(&linodego.ObjectStorageBucket{ + Label: "bucket", + Region: obj.Spec.Region, + Hostname: "hostname", + Created: util.Pointer(time.Now()), + }, nil) + mck.LinodeClient.EXPECT().GetObjectStorageBucketAccess(gomock.Any(), gomock.Any(), gomock.Any()). + Return(&linodego.ObjectStorageBucketAccess{ + ACL: linodego.ACLPrivate, + CorsEnabled: true, + }, nil) + mck.LinodeClient.EXPECT().UpdateObjectStorageBucketAccess(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(nil) }), Result("success", func(ctx context.Context, mck Mock) { bScope.LinodeClient = mck.LinodeClient @@ -161,20 +207,14 @@ var _ = Describe("lifecycle", Ordered, Label("bucket", "lifecycle"), func() { }), ), ), - Once("resource is deleted", func(ctx context.Context, _ Mock) { - // nb: client.Delete does not set DeletionTimestamp on the object, so re-fetch from the apiserver. - objectKey := client.ObjectKeyFromObject(&obj) + Call("resource is deleted", func(ctx context.Context, _ Mock) { Expect(k8sClient.Delete(ctx, &obj)).To(Succeed()) - Expect(k8sClient.Get(ctx, objectKey, &obj)).To(Succeed()) }), - Result("finalizer is removed", func(ctx context.Context, mck Mock) { + Result("success", func(ctx context.Context, mck Mock) { objectKey := client.ObjectKeyFromObject(&obj) k8sClient.Get(ctx, objectKey, &obj) bScope.LinodeClient = mck.LinodeClient - _, err := reconciler.reconcile(ctx, &bScope) - Expect(err).NotTo(HaveOccurred()) Expect(apierrors.IsNotFound(k8sClient.Get(ctx, objectKey, &obj))).To(BeTrue()) - Expect(mck.Logs()).To(ContainSubstring("Reconciling delete")) }), ) }) @@ -248,29 +288,5 @@ var _ = Describe("errors", Label("bucket", "errors"), func() { Expect(err.Error()).To(ContainSubstring("failed to create object storage bucket scope")) Expect(mck.Logs()).To(ContainSubstring("Failed to create object storage bucket scope")) }), - Call("scheme with no infrav1alpha2", func(ctx context.Context, mck Mock) { - prev := mck.K8sClient.EXPECT().Scheme().Return(scheme.Scheme) - mck.K8sClient.EXPECT().Scheme().After(prev).Return(runtime.NewScheme()).Times(2) - }), - Result("error", func(ctx context.Context, mck Mock) { - bScope.Client = mck.K8sClient - - patchHelper, err := patch.NewHelper(bScope.Bucket, mck.K8sClient) - Expect(err).NotTo(HaveOccurred()) - bScope.PatchHelper = patchHelper - - _, err = reconciler.reconcile(ctx, &bScope) - Expect(err.Error()).To(ContainSubstring("no kind is registered")) - }), - Once("finalizer is missing", func(ctx context.Context, _ Mock) { - bScope.Bucket.ObjectMeta.Finalizers = []string{} - }), - Result("error", func(ctx context.Context, mck Mock) { - bScope.LinodeClient = mck.LinodeClient - bScope.Client = mck.K8sClient - err := reconciler.reconcileDelete(&bScope) - Expect(err.Error()).To(ContainSubstring("failed to remove finalizer from bucket")) - Expect(mck.Events()).To(ContainSubstring("failed to remove finalizer from bucket")) - }), ) }) diff --git a/mock/client.go b/mock/client.go index f0bb9d523..3e20c26e5 100644 --- a/mock/client.go +++ b/mock/client.go @@ -530,6 +530,21 @@ func (mr *MockLinodeClientMockRecorder) GetObjectStorageBucket(ctx, regionID, la return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetObjectStorageBucket", reflect.TypeOf((*MockLinodeClient)(nil).GetObjectStorageBucket), ctx, regionID, label) } +// GetObjectStorageBucketAccess mocks base method. +func (m *MockLinodeClient) GetObjectStorageBucketAccess(ctx context.Context, clusterOrRegionID, label string) (*linodego.ObjectStorageBucketAccess, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetObjectStorageBucketAccess", ctx, clusterOrRegionID, label) + ret0, _ := ret[0].(*linodego.ObjectStorageBucketAccess) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetObjectStorageBucketAccess indicates an expected call of GetObjectStorageBucketAccess. +func (mr *MockLinodeClientMockRecorder) GetObjectStorageBucketAccess(ctx, clusterOrRegionID, label any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetObjectStorageBucketAccess", reflect.TypeOf((*MockLinodeClient)(nil).GetObjectStorageBucketAccess), ctx, clusterOrRegionID, label) +} + // GetObjectStorageKey mocks base method. func (m *MockLinodeClient) GetObjectStorageKey(ctx context.Context, keyID int) (*linodego.ObjectStorageKey, error) { m.ctrl.T.Helper() @@ -799,6 +814,20 @@ func (mr *MockLinodeClientMockRecorder) UpdateInstanceConfig(ctx, linodeID, conf return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateInstanceConfig", reflect.TypeOf((*MockLinodeClient)(nil).UpdateInstanceConfig), ctx, linodeID, configID, opts) } +// UpdateObjectStorageBucketAccess mocks base method. +func (m *MockLinodeClient) UpdateObjectStorageBucketAccess(ctx context.Context, clusterOrRegionID, label string, opts linodego.ObjectStorageBucketUpdateAccessOptions) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "UpdateObjectStorageBucketAccess", ctx, clusterOrRegionID, label, opts) + ret0, _ := ret[0].(error) + return ret0 +} + +// UpdateObjectStorageBucketAccess indicates an expected call of UpdateObjectStorageBucketAccess. +func (mr *MockLinodeClientMockRecorder) UpdateObjectStorageBucketAccess(ctx, clusterOrRegionID, label, opts any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateObjectStorageBucketAccess", reflect.TypeOf((*MockLinodeClient)(nil).UpdateObjectStorageBucketAccess), ctx, clusterOrRegionID, label, opts) +} + // UpdatePlacementGroup mocks base method. func (m *MockLinodeClient) UpdatePlacementGroup(ctx context.Context, id int, options linodego.PlacementGroupUpdateOptions) (*linodego.PlacementGroup, error) { m.ctrl.T.Helper() @@ -1554,6 +1583,21 @@ func (mr *MockLinodeObjectStorageClientMockRecorder) GetObjectStorageBucket(ctx, return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetObjectStorageBucket", reflect.TypeOf((*MockLinodeObjectStorageClient)(nil).GetObjectStorageBucket), ctx, regionID, label) } +// GetObjectStorageBucketAccess mocks base method. +func (m *MockLinodeObjectStorageClient) GetObjectStorageBucketAccess(ctx context.Context, clusterOrRegionID, label string) (*linodego.ObjectStorageBucketAccess, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetObjectStorageBucketAccess", ctx, clusterOrRegionID, label) + ret0, _ := ret[0].(*linodego.ObjectStorageBucketAccess) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetObjectStorageBucketAccess indicates an expected call of GetObjectStorageBucketAccess. +func (mr *MockLinodeObjectStorageClientMockRecorder) GetObjectStorageBucketAccess(ctx, clusterOrRegionID, label any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetObjectStorageBucketAccess", reflect.TypeOf((*MockLinodeObjectStorageClient)(nil).GetObjectStorageBucketAccess), ctx, clusterOrRegionID, label) +} + // GetObjectStorageKey mocks base method. func (m *MockLinodeObjectStorageClient) GetObjectStorageKey(ctx context.Context, keyID int) (*linodego.ObjectStorageKey, error) { m.ctrl.T.Helper() @@ -1569,6 +1613,20 @@ func (mr *MockLinodeObjectStorageClientMockRecorder) GetObjectStorageKey(ctx, ke return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetObjectStorageKey", reflect.TypeOf((*MockLinodeObjectStorageClient)(nil).GetObjectStorageKey), ctx, keyID) } +// UpdateObjectStorageBucketAccess mocks base method. +func (m *MockLinodeObjectStorageClient) UpdateObjectStorageBucketAccess(ctx context.Context, clusterOrRegionID, label string, opts linodego.ObjectStorageBucketUpdateAccessOptions) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "UpdateObjectStorageBucketAccess", ctx, clusterOrRegionID, label, opts) + ret0, _ := ret[0].(error) + return ret0 +} + +// UpdateObjectStorageBucketAccess indicates an expected call of UpdateObjectStorageBucketAccess. +func (mr *MockLinodeObjectStorageClientMockRecorder) UpdateObjectStorageBucketAccess(ctx, clusterOrRegionID, label, opts any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateObjectStorageBucketAccess", reflect.TypeOf((*MockLinodeObjectStorageClient)(nil).UpdateObjectStorageBucketAccess), ctx, clusterOrRegionID, label, opts) +} + // MockLinodeDNSClient is a mock of LinodeDNSClient interface. type MockLinodeDNSClient struct { ctrl *gomock.Controller diff --git a/observability/wrappers/linodeclient/linodeclient.gen.go b/observability/wrappers/linodeclient/linodeclient.gen.go index 8b9bb72ba..05a4ed485 100644 --- a/observability/wrappers/linodeclient/linodeclient.gen.go +++ b/observability/wrappers/linodeclient/linodeclient.gen.go @@ -865,6 +865,32 @@ func (_d LinodeClientWithTracing) GetObjectStorageBucket(ctx context.Context, re return _d.LinodeClient.GetObjectStorageBucket(ctx, regionID, label) } +// GetObjectStorageBucketAccess implements clients.LinodeClient +func (_d LinodeClientWithTracing) GetObjectStorageBucketAccess(ctx context.Context, clusterOrRegionID string, label string) (op1 *linodego.ObjectStorageBucketAccess, err error) { + ctx, _span := tracing.Start(ctx, "clients.LinodeClient.GetObjectStorageBucketAccess") + defer func() { + if _d._spanDecorator != nil { + _d._spanDecorator(_span, map[string]interface{}{ + "ctx": ctx, + "clusterOrRegionID": clusterOrRegionID, + "label": label}, map[string]interface{}{ + "op1": op1, + "err": err}) + } + + if err != nil { + _span.RecordError(err) + _span.SetAttributes( + attribute.String("event", "error"), + attribute.String("message", err.Error()), + ) + } + + _span.End() + }() + return _d.LinodeClient.GetObjectStorageBucketAccess(ctx, clusterOrRegionID, label) +} + // GetObjectStorageKey implements clients.LinodeClient func (_d LinodeClientWithTracing) GetObjectStorageKey(ctx context.Context, keyID int) (op1 *linodego.ObjectStorageKey, err error) { ctx, _span := tracing.Start(ctx, "clients.LinodeClient.GetObjectStorageKey") @@ -1325,6 +1351,32 @@ func (_d LinodeClientWithTracing) UpdateInstanceConfig(ctx context.Context, lino return _d.LinodeClient.UpdateInstanceConfig(ctx, linodeID, configID, opts) } +// UpdateObjectStorageBucketAccess implements clients.LinodeClient +func (_d LinodeClientWithTracing) UpdateObjectStorageBucketAccess(ctx context.Context, clusterOrRegionID string, label string, opts linodego.ObjectStorageBucketUpdateAccessOptions) (err error) { + ctx, _span := tracing.Start(ctx, "clients.LinodeClient.UpdateObjectStorageBucketAccess") + defer func() { + if _d._spanDecorator != nil { + _d._spanDecorator(_span, map[string]interface{}{ + "ctx": ctx, + "clusterOrRegionID": clusterOrRegionID, + "label": label, + "opts": opts}, map[string]interface{}{ + "err": err}) + } + + if err != nil { + _span.RecordError(err) + _span.SetAttributes( + attribute.String("event", "error"), + attribute.String("message", err.Error()), + ) + } + + _span.End() + }() + return _d.LinodeClient.UpdateObjectStorageBucketAccess(ctx, clusterOrRegionID, label, opts) +} + // UpdatePlacementGroup implements clients.LinodeClient func (_d LinodeClientWithTracing) UpdatePlacementGroup(ctx context.Context, id int, options linodego.PlacementGroupUpdateOptions) (pp1 *linodego.PlacementGroup, err error) { ctx, _span := tracing.Start(ctx, "clients.LinodeClient.UpdatePlacementGroup")