From 6ae123bb0dca3c7bb5cf3cf7ca99c189f0d18690 Mon Sep 17 00:00:00 2001 From: Unnati Aggarwal <113070152+unnatiagg@users.noreply.github.com> Date: Thu, 8 Aug 2024 14:08:42 -0400 Subject: [PATCH] [feat] Adding new fields in LinodeObjectStorageBucket and decoupling ObjKey Functionality (#443) * Adding fields and decoupling ObjBucket and ObjKey Functionality * Modifying e2e test case for minimal-linodeobjectstoragebucket * Adding cors in create opts and lint suggestions * lint suggestions * lint suggestions * key generation pointer changes --- api/v1alpha1/conversion.go | 15 +- ...nodeobjectstoragebucket_conversion_test.go | 10 +- api/v1alpha1/zz_generated.conversion.go | 29 +- .../linodeobjectstoragebucket_types.go | 37 +- api/v1alpha2/zz_generated.deepcopy.go | 25 +- cloud/scope/object_storage_bucket.go | 98 +---- cloud/scope/object_storage_bucket_test.go | 356 ----------------- cloud/services/object_storage_buckets.go | 107 +----- cloud/services/object_storage_buckets_test.go | 301 --------------- ...r.x-k8s.io_linodeobjectstoragebuckets.yaml | 33 +- .../linodeobjectstoragebucket_controller.go | 84 +--- ...nodeobjectstoragebucket_controller_test.go | 358 +++--------------- .../linodeobjectstoragekey_controller.go | 1 - .../assert-key-secret.yaml | 4 - .../assert-keygen.yaml | 11 - .../assert-obj-and-secret.yaml | 17 - .../{patch-obj.yaml => assert-obj.yaml} | 4 +- .../chainsaw-test.yaml | 36 +- ...-deletion.yaml => check-obj-deletion.yaml} | 5 - 19 files changed, 132 insertions(+), 1399 deletions(-) delete mode 100644 e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/assert-key-secret.yaml delete mode 100644 e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/assert-keygen.yaml delete mode 100644 e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/assert-obj-and-secret.yaml rename e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/{patch-obj.yaml => assert-obj.yaml} (83%) rename e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/{check-obj-and-key-deletion.yaml => check-obj-deletion.yaml} (61%) diff --git a/api/v1alpha1/conversion.go b/api/v1alpha1/conversion.go index 35f0a593e..ba45ceac2 100644 --- a/api/v1alpha1/conversion.go +++ b/api/v1alpha1/conversion.go @@ -20,6 +20,7 @@ import ( "strings" "k8s.io/apimachinery/pkg/conversion" + "k8s.io/utils/ptr" infrastructurev1alpha2 "github.com/linode/cluster-api-provider-linode/api/v1alpha2" ) @@ -50,16 +51,26 @@ 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.KeyGeneration = in.KeyGeneration out.SecretType = in.SecretType return nil } +func Convert_v1alpha1_LinodeObjectStorageBucketStatus_To_v1alpha2_LinodeObjectStorageBucketStatus(in *LinodeObjectStorageBucketStatus, out *infrastructurev1alpha2.LinodeObjectStorageBucketStatus, s conversion.Scope) error { + out.Ready = in.Ready + out.FailureMessage = in.FailureMessage + out.Conditions = in.Conditions + out.Hostname = in.Hostname + out.CreationTime = in.CreationTime + // WARNING: in.LastKeyGeneration requires manual conversion: does not exist in peer-type + // WARNING: in.KeySecretName requires manual conversion: does not exist in peer-type + // WARNING: in.AccessKeyRefs requires manual conversion: does not exist in peer-type + return nil +} func Convert_v1alpha2_LinodeObjectStorageBucketSpec_To_v1alpha1_LinodeObjectStorageBucketSpec(in *infrastructurev1alpha2.LinodeObjectStorageBucketSpec, out *LinodeObjectStorageBucketSpec, s conversion.Scope) error { // WARNING: in.Region requires manual conversion: does not exist in peer-type out.Cluster = in.Region out.CredentialsRef = in.CredentialsRef - out.KeyGeneration = in.KeyGeneration + out.KeyGeneration = ptr.To(0) out.SecretType = in.SecretType return nil } diff --git a/api/v1alpha1/linodeobjectstoragebucket_conversion_test.go b/api/v1alpha1/linodeobjectstoragebucket_conversion_test.go index 5863efc22..83f6276be 100644 --- a/api/v1alpha1/linodeobjectstoragebucket_conversion_test.go +++ b/api/v1alpha1/linodeobjectstoragebucket_conversion_test.go @@ -59,8 +59,7 @@ func TestLinodeObjectStorageBucketConvertTo(t *testing.T) { Namespace: "default", Name: "cred-secret", }, - KeyGeneration: ptr.To(1), - SecretType: "Opaque", + SecretType: "Opaque", }, Status: infrav1alpha2.LinodeObjectStorageBucketStatus{}, } @@ -96,8 +95,7 @@ func TestLinodeObjectStorageBucketFrom(t *testing.T) { Namespace: "default", Name: "cred-secret", }, - KeyGeneration: ptr.To(1), - SecretType: "Opaque", + SecretType: "Opaque", }, Status: infrav1alpha2.LinodeObjectStorageBucketStatus{}, } @@ -105,7 +103,7 @@ func TestLinodeObjectStorageBucketFrom(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "test-bucket", Annotations: map[string]string{ - ConversionDataAnnotation: `{"spec":{"credentialsRef":{"name":"cred-secret","namespace":"default"},"keyGeneration":1,"region":"us-mia-1","secretType":"Opaque"},"status":{"ready":false}}`, + ConversionDataAnnotation: `{"spec":{"credentialsRef":{"name":"cred-secret","namespace":"default"},"region":"us-mia-1","secretType":"Opaque"},"status":{"ready":false}}`, }, }, Spec: LinodeObjectStorageBucketSpec{ @@ -114,7 +112,7 @@ func TestLinodeObjectStorageBucketFrom(t *testing.T) { Namespace: "default", Name: "cred-secret", }, - KeyGeneration: ptr.To(1), + KeyGeneration: ptr.To(0), SecretType: "Opaque", }, Status: LinodeObjectStorageBucketStatus{}, diff --git a/api/v1alpha1/zz_generated.conversion.go b/api/v1alpha1/zz_generated.conversion.go index c0e65c508..0c0672efd 100644 --- a/api/v1alpha1/zz_generated.conversion.go +++ b/api/v1alpha1/zz_generated.conversion.go @@ -240,11 +240,6 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } - if err := s.AddGeneratedConversionFunc((*LinodeObjectStorageBucketStatus)(nil), (*v1alpha2.LinodeObjectStorageBucketStatus)(nil), func(a, b interface{}, scope conversion.Scope) error { - return Convert_v1alpha1_LinodeObjectStorageBucketStatus_To_v1alpha2_LinodeObjectStorageBucketStatus(a.(*LinodeObjectStorageBucketStatus), b.(*v1alpha2.LinodeObjectStorageBucketStatus), scope) - }); err != nil { - return err - } if err := s.AddGeneratedConversionFunc((*v1alpha2.LinodeObjectStorageBucketStatus)(nil), (*LinodeObjectStorageBucketStatus)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1alpha2_LinodeObjectStorageBucketStatus_To_v1alpha1_LinodeObjectStorageBucketStatus(a.(*v1alpha2.LinodeObjectStorageBucketStatus), b.(*LinodeObjectStorageBucketStatus), scope) }); err != nil { @@ -315,6 +310,11 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } + if err := s.AddConversionFunc((*LinodeObjectStorageBucketStatus)(nil), (*v1alpha2.LinodeObjectStorageBucketStatus)(nil), func(a, b interface{}, scope conversion.Scope) error { + return Convert_v1alpha1_LinodeObjectStorageBucketStatus_To_v1alpha2_LinodeObjectStorageBucketStatus(a.(*LinodeObjectStorageBucketStatus), b.(*v1alpha2.LinodeObjectStorageBucketStatus), scope) + }); err != nil { + return err + } if err := s.AddConversionFunc((*NetworkSpec)(nil), (*v1alpha2.NetworkSpec)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1alpha1_NetworkSpec_To_v1alpha2_NetworkSpec(a.(*NetworkSpec), b.(*v1alpha2.NetworkSpec), scope) }); err != nil { @@ -1013,15 +1013,16 @@ func Convert_v1alpha2_LinodeObjectStorageBucketList_To_v1alpha1_LinodeObjectStor func autoConvert_v1alpha1_LinodeObjectStorageBucketSpec_To_v1alpha2_LinodeObjectStorageBucketSpec(in *LinodeObjectStorageBucketSpec, out *v1alpha2.LinodeObjectStorageBucketSpec, s conversion.Scope) error { // WARNING: in.Cluster requires manual conversion: does not exist in peer-type out.CredentialsRef = (*v1.SecretReference)(unsafe.Pointer(in.CredentialsRef)) - out.KeyGeneration = (*int)(unsafe.Pointer(in.KeyGeneration)) + // WARNING: in.KeyGeneration requires manual conversion: does not exist in peer-type out.SecretType = in.SecretType return nil } func autoConvert_v1alpha2_LinodeObjectStorageBucketSpec_To_v1alpha1_LinodeObjectStorageBucketSpec(in *v1alpha2.LinodeObjectStorageBucketSpec, out *LinodeObjectStorageBucketSpec, s conversion.Scope) error { // WARNING: in.Region requires manual conversion: does not exist in peer-type + // 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.KeyGeneration = (*int)(unsafe.Pointer(in.KeyGeneration)) out.SecretType = in.SecretType return nil } @@ -1032,26 +1033,18 @@ func autoConvert_v1alpha1_LinodeObjectStorageBucketStatus_To_v1alpha2_LinodeObje out.Conditions = *(*v1beta1.Conditions)(unsafe.Pointer(&in.Conditions)) out.Hostname = (*string)(unsafe.Pointer(in.Hostname)) out.CreationTime = (*metav1.Time)(unsafe.Pointer(in.CreationTime)) - out.LastKeyGeneration = (*int)(unsafe.Pointer(in.LastKeyGeneration)) - out.KeySecretName = (*string)(unsafe.Pointer(in.KeySecretName)) - out.AccessKeyRefs = *(*[]int)(unsafe.Pointer(&in.AccessKeyRefs)) + // WARNING: in.LastKeyGeneration requires manual conversion: does not exist in peer-type + // WARNING: in.KeySecretName requires manual conversion: does not exist in peer-type + // WARNING: in.AccessKeyRefs requires manual conversion: does not exist in peer-type return nil } -// Convert_v1alpha1_LinodeObjectStorageBucketStatus_To_v1alpha2_LinodeObjectStorageBucketStatus is an autogenerated conversion function. -func Convert_v1alpha1_LinodeObjectStorageBucketStatus_To_v1alpha2_LinodeObjectStorageBucketStatus(in *LinodeObjectStorageBucketStatus, out *v1alpha2.LinodeObjectStorageBucketStatus, s conversion.Scope) error { - return autoConvert_v1alpha1_LinodeObjectStorageBucketStatus_To_v1alpha2_LinodeObjectStorageBucketStatus(in, out, s) -} - func autoConvert_v1alpha2_LinodeObjectStorageBucketStatus_To_v1alpha1_LinodeObjectStorageBucketStatus(in *v1alpha2.LinodeObjectStorageBucketStatus, out *LinodeObjectStorageBucketStatus, s conversion.Scope) error { out.Ready = in.Ready out.FailureMessage = (*string)(unsafe.Pointer(in.FailureMessage)) out.Conditions = *(*v1beta1.Conditions)(unsafe.Pointer(&in.Conditions)) out.Hostname = (*string)(unsafe.Pointer(in.Hostname)) out.CreationTime = (*metav1.Time)(unsafe.Pointer(in.CreationTime)) - out.LastKeyGeneration = (*int)(unsafe.Pointer(in.LastKeyGeneration)) - out.KeySecretName = (*string)(unsafe.Pointer(in.KeySecretName)) - out.AccessKeyRefs = *(*[]int)(unsafe.Pointer(&in.AccessKeyRefs)) return nil } diff --git a/api/v1alpha2/linodeobjectstoragebucket_types.go b/api/v1alpha2/linodeobjectstoragebucket_types.go index 91e59957f..4a3371444 100644 --- a/api/v1alpha2/linodeobjectstoragebucket_types.go +++ b/api/v1alpha2/linodeobjectstoragebucket_types.go @@ -22,10 +22,17 @@ import ( clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" ) +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" + ObjectStorageBucketFinalizer = "linodeobjectstoragebucket.infrastructure.cluster.x-k8s.io" + ACLPrivate ObjectStorageACL = "private" + ACLPublicRead ObjectStorageACL = "public-read" + ACLAuthenticatedRead ObjectStorageACL = "authenticated-read" + ACLPublicReadWrite ObjectStorageACL = "public-read-write" ) // LinodeObjectStorageBucketSpec defines the desired state of LinodeObjectStorageBucket @@ -37,16 +44,22 @@ 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 + // +optional + // +kubebuilder:default=private + // +kubebuilder:validation:Enum=private;public-read;authenticated-read;public-read-write + ACL ObjectStorageACL `json:"acl,omitempty"` + + // 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"` + // 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"` - // KeyGeneration may be modified to trigger rotations of access keys created for the bucket. - // +optional - // +kubebuilder:default=0 - KeyGeneration *int `json:"keyGeneration,omitempty"` - // 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 @@ -80,18 +93,6 @@ type LinodeObjectStorageBucketStatus struct { // CreationTime specifies the creation timestamp for the bucket. // +optional CreationTime *metav1.Time `json:"creationTime,omitempty"` - - // LastKeyGeneration tracks the last known value of .spec.keyGeneration. - // +optional - LastKeyGeneration *int `json:"lastKeyGeneration,omitempty"` - - // KeySecretName specifies the name of the Secret containing access keys for the bucket. - // +optional - KeySecretName *string `json:"keySecretName,omitempty"` - - // AccessKeyRefs stores IDs for Object Storage keys provisioned along with the bucket. - // +optional - AccessKeyRefs []int `json:"accessKeyRefs,omitempty"` } // +kubebuilder:object:root=true diff --git a/api/v1alpha2/zz_generated.deepcopy.go b/api/v1alpha2/zz_generated.deepcopy.go index 5b2c01ed7..8fd5895c8 100644 --- a/api/v1alpha2/zz_generated.deepcopy.go +++ b/api/v1alpha2/zz_generated.deepcopy.go @@ -688,16 +688,16 @@ 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) **out = **in } - if in.KeyGeneration != nil { - in, out := &in.KeyGeneration, &out.KeyGeneration - *out = new(int) - **out = **in - } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new LinodeObjectStorageBucketSpec. @@ -734,21 +734,6 @@ func (in *LinodeObjectStorageBucketStatus) DeepCopyInto(out *LinodeObjectStorage in, out := &in.CreationTime, &out.CreationTime *out = (*in).DeepCopy() } - if in.LastKeyGeneration != nil { - in, out := &in.LastKeyGeneration, &out.LastKeyGeneration - *out = new(int) - **out = **in - } - if in.KeySecretName != nil { - in, out := &in.KeySecretName, &out.KeySecretName - *out = new(string) - **out = **in - } - if in.AccessKeyRefs != nil { - in, out := &in.AccessKeyRefs, &out.AccessKeyRefs - *out = make([]int, len(*in)) - copy(*out, *in) - } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new LinodeObjectStorageBucketStatus. diff --git a/cloud/scope/object_storage_bucket.go b/cloud/scope/object_storage_bucket.go index 0ba2517d7..5a77928db 100644 --- a/cloud/scope/object_storage_bucket.go +++ b/cloud/scope/object_storage_bucket.go @@ -7,12 +7,7 @@ import ( "time" "github.com/go-logr/logr" - "github.com/linode/linodego" - corev1 "k8s.io/api/core/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/cluster-api/util/patch" - "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" infrav1alpha2 "github.com/linode/cluster-api-provider-linode/api/v1alpha2" @@ -20,20 +15,6 @@ import ( . "github.com/linode/cluster-api-provider-linode/clients" ) -const bucketDataSecret = `kind: Secret -apiVersion: v1 -metadata: - name: etcd-backup - namespace: kube-system -stringData: - bucket_name: %s - bucket_region: %s - bucket_endpoint: %s - access_key_rw: %s - secret_key_rw: %s - access_key_ro: %s - secret_key_ro: %s` - type ObjectStorageBucketScopeParams struct { Client K8sClient Bucket *infrav1alpha2.LinodeObjectStorageBucket @@ -49,9 +30,7 @@ type ObjectStorageBucketScope struct { } const ( - AccessKeyNameTemplate = "%s-bucket-details" - NumAccessKeys = 2 - clientTimeout = 20 * time.Second + clientTimeout = 20 * time.Second ) func validateObjectStorageBucketScopeParams(params ObjectStorageBucketScopeParams) error { @@ -119,78 +98,3 @@ func (s *ObjectStorageBucketScope) AddFinalizer(ctx context.Context) error { return nil } - -// GenerateKeySecret returns a secret suitable for submission to the Kubernetes API. -// The secret is expected to contain keys for accessing the bucket, as well as owner and controller references. -func (s *ObjectStorageBucketScope) GenerateKeySecret(ctx context.Context, keys [NumAccessKeys]*linodego.ObjectStorageKey, bucket *linodego.ObjectStorageBucket) (*corev1.Secret, error) { - for _, key := range keys { - if key == nil { - return nil, errors.New("expected two non-nil object storage keys") - } - } - var secretStringData map[string]string - secretName := fmt.Sprintf(AccessKeyNameTemplate, s.Bucket.Name) - // If the secret is of ClusterResourceSet type, encapsulate real data in the outer data - if s.Bucket.Spec.SecretType == "addons.cluster.x-k8s.io/resource-set" { - secretStringData = map[string]string{ - "bucket-details-secret.yaml": fmt.Sprintf(bucketDataSecret, - bucket.Label, - bucket.Region, - bucket.Hostname, - keys[0].AccessKey, - keys[0].SecretKey, - keys[1].AccessKey, - keys[1].SecretKey, - ), - } - } else { - secretStringData = map[string]string{ - "bucket_name": bucket.Label, - "bucket_region": bucket.Region, - "bucket_endpoint": bucket.Hostname, - "access_key_rw": keys[0].AccessKey, - "secret_key_rw": keys[0].SecretKey, - "access_key_ro": keys[1].AccessKey, - "secret_key_ro": keys[1].SecretKey, - } - } - secret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: secretName, - Namespace: s.Bucket.Namespace, - }, - Type: corev1.SecretType(s.Bucket.Spec.SecretType), - StringData: secretStringData, - } - - scheme := s.Client.Scheme() - if err := controllerutil.SetOwnerReference(s.Bucket, secret, scheme); err != nil { - return nil, fmt.Errorf("could not set owner ref on access key secret %s: %w", secretName, err) - } - if err := controllerutil.SetControllerReference(s.Bucket, secret, scheme); err != nil { - return nil, fmt.Errorf("could not set controller ref on access key secret %s: %w", secretName, err) - } - - return secret, nil -} - -func (s *ObjectStorageBucketScope) ShouldInitKeys() bool { - return s.Bucket.Status.LastKeyGeneration == nil -} - -func (s *ObjectStorageBucketScope) ShouldRotateKeys() bool { - return s.Bucket.Status.LastKeyGeneration != nil && - *s.Bucket.Spec.KeyGeneration != *s.Bucket.Status.LastKeyGeneration -} - -func (s *ObjectStorageBucketScope) ShouldRestoreKeySecret(ctx context.Context) (bool, error) { - if s.Bucket.Status.KeySecretName == nil { - return false, nil - } - - secret := &corev1.Secret{} - key := client.ObjectKey{Namespace: s.Bucket.Namespace, Name: *s.Bucket.Status.KeySecretName} - err := s.Client.Get(ctx, key, secret) - - return apierrors.IsNotFound(err), client.IgnoreNotFound(err) -} diff --git a/cloud/scope/object_storage_bucket_test.go b/cloud/scope/object_storage_bucket_test.go index e235a28a3..e0c0a8064 100644 --- a/cloud/scope/object_storage_bucket_test.go +++ b/cloud/scope/object_storage_bucket_test.go @@ -2,23 +2,16 @@ package scope import ( "context" - "errors" "fmt" "testing" "github.com/go-logr/logr" - "github.com/linode/linodego" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" corev1 "k8s.io/api/core/v1" - 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/apimachinery/pkg/types" - "k8s.io/utils/ptr" - "sigs.k8s.io/cluster-api/util/patch" "sigs.k8s.io/controller-runtime/pkg/client" infrav1alpha2 "github.com/linode/cluster-api-provider-linode/api/v1alpha2" @@ -295,352 +288,3 @@ func TestObjectStorageBucketScopeMethods(t *testing.T) { }) } } - -func TestGenerateKeySecretBucket(t *testing.T) { - t.Parallel() - tests := []struct { - name string - Bucket *infrav1alpha2.LinodeObjectStorageBucket - keys [NumAccessKeys]*linodego.ObjectStorageKey - expectedErr error - expects func(mock *mock.MockK8sClient) - }{ - { - name: "happy path", - Bucket: &infrav1alpha2.LinodeObjectStorageBucket{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-bucket", - Namespace: "test-namespace", - }, - Status: infrav1alpha2.LinodeObjectStorageBucketStatus{ - KeySecretName: ptr.To("test-bucket-bucket-details"), - }, - }, - keys: [NumAccessKeys]*linodego.ObjectStorageKey{ - { - ID: 1, - Label: "read_write", - SecretKey: "read_write_key", - AccessKey: "read_write_access_key", - Limited: false, - BucketAccess: &[]linodego.ObjectStorageKeyBucketAccess{ - { - BucketName: "bucket", - Region: "test-bucket", - Permissions: "read_write", - }, - }, - }, - { - ID: 2, - Label: "read_only", - SecretKey: "read_only_key", - AccessKey: "read_only_access_key", - Limited: true, - BucketAccess: &[]linodego.ObjectStorageKeyBucketAccess{ - { - BucketName: "bucket", - Region: "test-bucket", - Permissions: "read_only", - }, - }, - }, - }, - expects: func(mock *mock.MockK8sClient) { - mock.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme { - s := runtime.NewScheme() - infrav1alpha2.AddToScheme(s) - return s - }).Times(1) - }, - expectedErr: nil, - }, - { - name: "missing one or more keys", - Bucket: &infrav1alpha2.LinodeObjectStorageBucket{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-bucket", - Namespace: "test-namespace", - }, - Status: infrav1alpha2.LinodeObjectStorageBucketStatus{ - KeySecretName: ptr.To("test-bucket-bucket-details"), - }, - }, - keys: [NumAccessKeys]*linodego.ObjectStorageKey{ - { - ID: 1, - Label: "read_write", - SecretKey: "read_write_key", - AccessKey: "read_write_access_key", - Limited: false, - BucketAccess: &[]linodego.ObjectStorageKeyBucketAccess{ - { - BucketName: "bucket", - Region: "test-bucket", - Permissions: "read_write", - }, - }, - }, - }, - expectedErr: errors.New("expected two non-nil object storage keys"), - }, - { - name: "client scheme does not have infrav1alpha2", - Bucket: &infrav1alpha2.LinodeObjectStorageBucket{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-bucket", - Namespace: "test-namespace", - }, - }, - keys: [NumAccessKeys]*linodego.ObjectStorageKey{ - { - ID: 1, - Label: "read_write", - SecretKey: "read_write_key", - AccessKey: "read_write_access_key", - Limited: false, - BucketAccess: &[]linodego.ObjectStorageKeyBucketAccess{ - { - BucketName: "bucket", - Region: "test-bucket", - Permissions: "read_write", - }, - }, - }, - { - ID: 2, - Label: "read_only", - SecretKey: "read_only_key", - AccessKey: "read_only_access_key", - Limited: true, - BucketAccess: &[]linodego.ObjectStorageKeyBucketAccess{ - { - BucketName: "bucket", - Region: "test-bucket", - Permissions: "read_only", - }, - }, - }, - }, - expects: func(mock *mock.MockK8sClient) { - mock.EXPECT().Scheme().Return(runtime.NewScheme()) - }, - expectedErr: fmt.Errorf("could not set owner ref on access key secret"), - }, - } - for _, tt := range tests { - testcase := tt - t.Run(testcase.name, func(t *testing.T) { - t.Parallel() - - var mockClient *mock.MockK8sClient - if testcase.expects != nil { - ctrl := gomock.NewController(t) - defer ctrl.Finish() - - mockClient = mock.NewMockK8sClient(ctrl) - testcase.expects(mockClient) - } - - objScope := &ObjectStorageBucketScope{ - Client: mockClient, - Bucket: testcase.Bucket, - } - - secret, err := objScope.GenerateKeySecret(context.Background(), testcase.keys, &linodego.ObjectStorageBucket{}) - if testcase.expectedErr != nil { - require.ErrorContains(t, err, testcase.expectedErr.Error()) - return - } - - assert.Equal(t, "LinodeObjectStorageBucket", secret.OwnerReferences[0].Kind) - assert.True(t, *secret.OwnerReferences[0].Controller) - }) - } -} - -func TestShouldInitKeys(t *testing.T) { - t.Parallel() - tests := []struct { - name string - want bool - Bucket *infrav1alpha2.LinodeObjectStorageBucket - }{ - { - name: "should init keys", - want: true, - Bucket: &infrav1alpha2.LinodeObjectStorageBucket{ - Spec: infrav1alpha2.LinodeObjectStorageBucketSpec{ - KeyGeneration: ptr.To(1), - }, - Status: infrav1alpha2.LinodeObjectStorageBucketStatus{ - LastKeyGeneration: nil, - }, - }, - }, - } - for _, tt := range tests { - testcase := tt - t.Run(testcase.name, func(t *testing.T) { - t.Parallel() - - objScope := &ObjectStorageBucketScope{ - Client: nil, - Bucket: testcase.Bucket, - Logger: logr.Logger{}, - LinodeClient: &linodego.Client{}, - PatchHelper: &patch.Helper{}, - } - - shouldInit := objScope.ShouldInitKeys() - - if shouldInit != testcase.want { - t.Errorf("ObjectStorageBucketScope.ShouldInitKeys() = %v, want %v", shouldInit, testcase.want) - } - }) - } -} - -func TestShouldRotateKeys(t *testing.T) { - t.Parallel() - tests := []struct { - name string - want bool - Bucket *infrav1alpha2.LinodeObjectStorageBucket - }{ - { - name: "should rotate keys", - want: true, - Bucket: &infrav1alpha2.LinodeObjectStorageBucket{ - Spec: infrav1alpha2.LinodeObjectStorageBucketSpec{ - KeyGeneration: ptr.To(1), - }, - Status: infrav1alpha2.LinodeObjectStorageBucketStatus{ - LastKeyGeneration: ptr.To(0), - }, - }, - }, - } - for _, tt := range tests { - testcase := tt - t.Run(testcase.name, func(t *testing.T) { - t.Parallel() - - objScope := &ObjectStorageBucketScope{ - Client: nil, - Bucket: testcase.Bucket, - Logger: logr.Logger{}, - LinodeClient: &linodego.Client{}, - PatchHelper: &patch.Helper{}, - } - - rotate := objScope.ShouldRotateKeys() - - if rotate != testcase.want { - t.Errorf("ObjectStorageBucketScope.ShouldRotateKeys() = %v, want %v", rotate, testcase.want) - } - }) - } -} - -func TestShouldRestoreKeySecret(t *testing.T) { - t.Parallel() - tests := []struct { - name string - bucket *infrav1alpha2.LinodeObjectStorageBucket - expects func(k8s *mock.MockK8sClient) - want bool - expectedErr error - }{ - { - name: "status has no secret name", - bucket: &infrav1alpha2.LinodeObjectStorageBucket{ - Status: infrav1alpha2.LinodeObjectStorageBucketStatus{ - KeySecretName: nil, - }, - }, - want: false, - }, - { - name: "status has secret name and secret exists", - bucket: &infrav1alpha2.LinodeObjectStorageBucket{ - ObjectMeta: metav1.ObjectMeta{ - Name: "bucket", - Namespace: "ns", - }, - Status: infrav1alpha2.LinodeObjectStorageBucketStatus{ - KeySecretName: ptr.To("secret"), - }, - }, - expects: func(k8s *mock.MockK8sClient) { - k8s.EXPECT(). - Get(gomock.Any(), client.ObjectKey{Namespace: "ns", Name: "secret"}, gomock.Any()). - Return(nil) - }, - want: false, - }, - { - name: "status has secret name and secret is missing", - bucket: &infrav1alpha2.LinodeObjectStorageBucket{ - ObjectMeta: metav1.ObjectMeta{ - Name: "bucket", - Namespace: "ns", - }, - Status: infrav1alpha2.LinodeObjectStorageBucketStatus{ - KeySecretName: ptr.To("secret"), - }, - }, - expects: func(k8s *mock.MockK8sClient) { - k8s.EXPECT(). - Get(gomock.Any(), client.ObjectKey{Namespace: "ns", Name: "secret"}, gomock.Any()). - Return(apierrors.NewNotFound(schema.GroupResource{Resource: "Secret"}, "secret")) - }, - want: true, - }, - { - name: "non-404 api error", - bucket: &infrav1alpha2.LinodeObjectStorageBucket{ - ObjectMeta: metav1.ObjectMeta{ - Name: "bucket", - Namespace: "ns", - }, - Status: infrav1alpha2.LinodeObjectStorageBucketStatus{ - KeySecretName: ptr.To("secret"), - }, - }, - expects: func(k8s *mock.MockK8sClient) { - k8s.EXPECT(). - Get(gomock.Any(), client.ObjectKey{Namespace: "ns", Name: "secret"}, gomock.Any()). - Return(errors.New("unexpected error")) - }, - want: false, - }, - } - for _, tt := range tests { - testcase := tt - t.Run(testcase.name, func(t *testing.T) { - t.Parallel() - - var mockClient *mock.MockK8sClient - if testcase.expects != nil { - ctrl := gomock.NewController(t) - defer ctrl.Finish() - - mockClient = mock.NewMockK8sClient(ctrl) - testcase.expects(mockClient) - } - - objScope := &ObjectStorageBucketScope{ - Client: mockClient, - Bucket: testcase.bucket, - } - - restore, err := objScope.ShouldRestoreKeySecret(context.TODO()) - if testcase.expectedErr != nil { - require.ErrorContains(t, err, testcase.expectedErr.Error()) - } - - assert.Equal(t, testcase.want, restore) - }) - } -} diff --git a/cloud/services/object_storage_buckets.go b/cloud/services/object_storage_buckets.go index dd86344d3..cd912dc57 100644 --- a/cloud/services/object_storage_buckets.go +++ b/cloud/services/object_storage_buckets.go @@ -2,12 +2,10 @@ package services import ( "context" - "errors" "fmt" "net/http" "github.com/linode/linodego" - utilerrors "k8s.io/apimachinery/pkg/util/errors" "github.com/linode/cluster-api-provider-linode/cloud/scope" "github.com/linode/cluster-api-provider-linode/util" @@ -29,9 +27,10 @@ func EnsureObjectStorageBucket(ctx context.Context, bScope *scope.ObjectStorageB } opts := linodego.ObjectStorageBucketCreateOptions{ - Region: bScope.Bucket.Spec.Region, - Label: bScope.Bucket.Name, - ACL: linodego.ACLPrivate, + 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 { @@ -42,101 +41,3 @@ func EnsureObjectStorageBucket(ctx context.Context, bScope *scope.ObjectStorageB return bucket, nil } - -func RotateObjectStorageKeys(ctx context.Context, bScope *scope.ObjectStorageBucketScope) ([scope.NumAccessKeys]*linodego.ObjectStorageKey, error) { - var newKeys [scope.NumAccessKeys]*linodego.ObjectStorageKey - - for idx, permission := range []struct { - name string - suffix string - }{ - {"read_write", "rw"}, - {"read_only", "ro"}, - } { - keyLabel := fmt.Sprintf("%s-%s", bScope.Bucket.Name, permission.suffix) - key, err := createObjectStorageKeyForBucket(ctx, bScope, keyLabel, permission.name) - if err != nil { - return newKeys, err - } - - newKeys[idx] = key - } - - // If key revocation fails here, just log the errors since new keys have been created - if !bScope.ShouldInitKeys() && bScope.ShouldRotateKeys() { - if err := RevokeObjectStorageKeys(ctx, bScope); err != nil { - bScope.Logger.Error(err, "Failed to revoke access keys; keys must be manually revoked") - } - } - - return newKeys, nil -} - -func createObjectStorageKeyForBucket(ctx context.Context, bScope *scope.ObjectStorageBucketScope, label, permission string) (*linodego.ObjectStorageKey, error) { - opts := linodego.ObjectStorageKeyCreateOptions{ - Label: label, - BucketAccess: &[]linodego.ObjectStorageKeyBucketAccess{ - { - BucketName: bScope.Bucket.Name, - Region: bScope.Bucket.Spec.Region, - Permissions: permission, - }, - }, - } - - key, err := bScope.LinodeClient.CreateObjectStorageKey(ctx, opts) - if err != nil { - bScope.Logger.Error(err, "Failed to create access key", "label", label) - - return nil, fmt.Errorf("failed to create access key: %w", err) - } - - bScope.Logger.Info("Created access key", "id", key.ID) - - return key, nil -} - -func RevokeObjectStorageKeys(ctx context.Context, bScope *scope.ObjectStorageBucketScope) error { - var errs []error - for _, keyID := range bScope.Bucket.Status.AccessKeyRefs { - if err := revokeObjectStorageKey(ctx, bScope, keyID); err != nil { - errs = append(errs, err) - } - } - - return utilerrors.NewAggregate(errs) -} - -func revokeObjectStorageKey(ctx context.Context, bScope *scope.ObjectStorageBucketScope, keyID int) error { - err := bScope.LinodeClient.DeleteObjectStorageKey(ctx, keyID) - if util.IgnoreLinodeAPIError(err, http.StatusNotFound) != nil { - bScope.Logger.Error(err, "Failed to revoke access key", "id", keyID) - return fmt.Errorf("failed to revoke access key: %w", err) - } - - bScope.Logger.Info("Revoked access key", "id", keyID) - - return nil -} - -func GetObjectStorageKeys(ctx context.Context, bScope *scope.ObjectStorageBucketScope) ([2]*linodego.ObjectStorageKey, error) { - var keys [2]*linodego.ObjectStorageKey - - if len(bScope.Bucket.Status.AccessKeyRefs) != scope.NumAccessKeys { - return keys, errors.New("expected two object storage key IDs in .status.accessKeyRefs") - } - - var errs []error - for idx, keyID := range bScope.Bucket.Status.AccessKeyRefs { - key, err := bScope.LinodeClient.GetObjectStorageKey(ctx, keyID) - if err != nil { - errs = append(errs, err) - - continue - } - - keys[idx] = key - } - - return keys, utilerrors.NewAggregate(errs) -} diff --git a/cloud/services/object_storage_buckets_test.go b/cloud/services/object_storage_buckets_test.go index 554c50a86..da29152de 100644 --- a/cloud/services/object_storage_buckets_test.go +++ b/cloud/services/object_storage_buckets_test.go @@ -2,18 +2,13 @@ package services import ( "context" - "errors" "fmt" - "reflect" - "strings" "testing" "github.com/linode/linodego" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/utils/ptr" infrav1alpha2 "github.com/linode/cluster-api-provider-linode/api/v1alpha2" "github.com/linode/cluster-api-provider-linode/cloud/scope" @@ -132,299 +127,3 @@ func TestEnsureObjectStorageBucket(t *testing.T) { }) } } - -// TestRotateObjectStorageKeysCreation tests the creation flow of object storage keys -func TestRotateObjectStorageKeysCreation(t *testing.T) { - t.Parallel() - tests := []struct { - name string - bScope *scope.ObjectStorageBucketScope - want [scope.NumAccessKeys]*linodego.ObjectStorageKey - expectedError error - expects func(*mock.MockLinodeClient) - }{ - { - name: "Creates new access keys", - bScope: &scope.ObjectStorageBucketScope{ - Bucket: &infrav1alpha2.LinodeObjectStorageBucket{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-bucket", - }, - Spec: infrav1alpha2.LinodeObjectStorageBucketSpec{ - Region: "test-region", - KeyGeneration: ptr.To(1), - }, - Status: infrav1alpha2.LinodeObjectStorageBucketStatus{ - LastKeyGeneration: ptr.To(0), - AccessKeyRefs: []int{ - 11, - 22, - }, - }, - }, - }, - want: [scope.NumAccessKeys]*linodego.ObjectStorageKey{ - { - ID: 1234, - Label: "test-bucket-rw", - }, - { - ID: 5678, - Label: "test-bucket-ro", - }, - }, - expects: func(mockClient *mock.MockLinodeClient) { - mockClient.EXPECT().CreateObjectStorageKey(gomock.Any(), gomock.Any()).Return(&linodego.ObjectStorageKey{ - Label: "test-bucket-rw", - ID: 1234, - }, nil) - mockClient.EXPECT().CreateObjectStorageKey(gomock.Any(), gomock.Any()).Return(&linodego.ObjectStorageKey{ - Label: "test-bucket-ro", - ID: 5678, - }, nil) - mockClient.EXPECT().DeleteObjectStorageKey(gomock.Any(), gomock.Any()).Return(nil).Times(2) - }, - }, - { - name: "Error creating keys", - bScope: &scope.ObjectStorageBucketScope{ - Bucket: &infrav1alpha2.LinodeObjectStorageBucket{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-bucket", - }, - Spec: infrav1alpha2.LinodeObjectStorageBucketSpec{ - Region: "test-region", - }, - }, - }, - want: [scope.NumAccessKeys]*linodego.ObjectStorageKey{}, - expectedError: fmt.Errorf("failed to create access key:"), - expects: func(c *mock.MockLinodeClient) { - c.EXPECT().CreateObjectStorageKey(gomock.Any(), gomock.Any()).Return(nil, fmt.Errorf("error in creating access key")) - }, - }, - } - for _, tt := range tests { - testcase := tt - t.Run(testcase.name, func(t *testing.T) { - t.Parallel() - - ctrl := gomock.NewController(t) - defer ctrl.Finish() - - mockClient := mock.NewMockLinodeClient(ctrl) - - testcase.bScope.LinodeClient = mockClient - - testcase.expects(mockClient) - - got, err := RotateObjectStorageKeys(context.Background(), testcase.bScope) - if testcase.expectedError != nil { - assert.ErrorContains(t, err, testcase.expectedError.Error()) - } else { - assert.Equal(t, testcase.want, got) - } - }) - } -} - -func TestRotateObjectStorageKeysRevocation(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - bucket *infrav1alpha2.LinodeObjectStorageBucket - expects func(*mock.MockLinodeClient) - }{ - { - name: "should revoke existing keys", - bucket: &infrav1alpha2.LinodeObjectStorageBucket{ - ObjectMeta: metav1.ObjectMeta{ - Name: "bucket", - }, - Spec: infrav1alpha2.LinodeObjectStorageBucketSpec{ - KeyGeneration: ptr.To(1), - }, - Status: infrav1alpha2.LinodeObjectStorageBucketStatus{ - LastKeyGeneration: ptr.To(0), - AccessKeyRefs: []int{0, 1}, - }, - }, - expects: func(mockClient *mock.MockLinodeClient) { - for keyID := range 2 { - mockClient.EXPECT(). - DeleteObjectStorageKey(gomock.Any(), keyID). - Return(nil). - Times(1) - } - }, - }, - { - name: "shouldInitKeys", - bucket: &infrav1alpha2.LinodeObjectStorageBucket{ - ObjectMeta: metav1.ObjectMeta{ - Name: "bucket", - }, - Status: infrav1alpha2.LinodeObjectStorageBucketStatus{ - LastKeyGeneration: nil, - }, - }, - }, - { - name: "not shouldRotateKeys", - bucket: &infrav1alpha2.LinodeObjectStorageBucket{ - ObjectMeta: metav1.ObjectMeta{ - Name: "bucket", - }, - Spec: infrav1alpha2.LinodeObjectStorageBucketSpec{ - KeyGeneration: ptr.To(1), - }, - Status: infrav1alpha2.LinodeObjectStorageBucketStatus{ - LastKeyGeneration: ptr.To(1), - }, - }, - }, - { - name: "unable to revoke keys", - bucket: &infrav1alpha2.LinodeObjectStorageBucket{ - ObjectMeta: metav1.ObjectMeta{ - Name: "bucket", - }, - Spec: infrav1alpha2.LinodeObjectStorageBucketSpec{ - KeyGeneration: ptr.To(1), - }, - Status: infrav1alpha2.LinodeObjectStorageBucketStatus{ - LastKeyGeneration: ptr.To(0), - AccessKeyRefs: []int{0, 1}, - }, - }, - expects: func(mockClient *mock.MockLinodeClient) { - for keyID := range 2 { - mockClient.EXPECT(). - DeleteObjectStorageKey(gomock.Any(), keyID). - Return(fmt.Errorf("error in deleting access key")). - 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() - - mockClient := mock.NewMockLinodeClient(ctrl) - mockClient.EXPECT(). - CreateObjectStorageKey(gomock.Any(), gomock.Any()). - Return(&linodego.ObjectStorageKey{ID: 3}, nil). - Times(2) - if testcase.expects != nil { - testcase.expects(mockClient) - } - - bScope := &scope.ObjectStorageBucketScope{ - LinodeClient: mockClient, - Bucket: testcase.bucket, - } - - _, err := RotateObjectStorageKeys(context.TODO(), bScope) - require.NoError(t, err) - }) - } -} - -func TestGetObjectStorageKeys(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - bScope *scope.ObjectStorageBucketScope - expects func(*mock.MockLinodeClient) - want [2]*linodego.ObjectStorageKey - wantErr string - }{ - { - name: "happy path", - bScope: &scope.ObjectStorageBucketScope{ - Bucket: &infrav1alpha2.LinodeObjectStorageBucket{ - Status: infrav1alpha2.LinodeObjectStorageBucketStatus{ - AccessKeyRefs: []int{0, 1}, - }, - }, - }, - expects: func(mockClient *mock.MockLinodeClient) { - mockClient.EXPECT(). - GetObjectStorageKey(gomock.Any(), gomock.Any()). - DoAndReturn(func(_ any, keyID int) (*linodego.ObjectStorageKey, error) { - return &linodego.ObjectStorageKey{ID: keyID}, nil - }). - Times(2) - }, - want: [2]*linodego.ObjectStorageKey{ - {ID: 0}, - {ID: 1}, - }, - }, - { - name: "not two key refs in status", - bScope: &scope.ObjectStorageBucketScope{ - Bucket: &infrav1alpha2.LinodeObjectStorageBucket{}, - }, - wantErr: "expected two object storage key IDs in .status.accessKeyRefs", - }, - { - name: "one client error", - bScope: &scope.ObjectStorageBucketScope{ - Bucket: &infrav1alpha2.LinodeObjectStorageBucket{ - Status: infrav1alpha2.LinodeObjectStorageBucketStatus{ - AccessKeyRefs: []int{0, 1}, - }, - }, - }, - expects: func(mockClient *mock.MockLinodeClient) { - mockClient.EXPECT(). - GetObjectStorageKey(gomock.Any(), 0). - DoAndReturn(func(_ any, keyID int) (*linodego.ObjectStorageKey, error) { - return &linodego.ObjectStorageKey{ID: keyID}, nil - }) - mockClient.EXPECT(). - GetObjectStorageKey(gomock.Any(), 1). - DoAndReturn(func(_ any, keyID int) (*linodego.ObjectStorageKey, error) { - return nil, errors.New("some error") - }) - }, - want: [2]*linodego.ObjectStorageKey{ - {ID: 0}, - nil, - }, - wantErr: "some error", - }, - } - for _, tt := range tests { - testcase := tt - t.Run(testcase.name, func(t *testing.T) { - t.Parallel() - - var mockClient *mock.MockLinodeClient - if testcase.expects != nil { - ctrl := gomock.NewController(t) - defer ctrl.Finish() - - mockClient = mock.NewMockLinodeClient(ctrl) - testcase.expects(mockClient) - testcase.bScope.LinodeClient = mockClient - } - - got, err := GetObjectStorageKeys(context.TODO(), testcase.bScope) - if testcase.wantErr != "" && (err == nil || !strings.Contains(err.Error(), testcase.wantErr)) { - t.Errorf("GetObjectStorageKeys() error = %v, should contain %v", err, testcase.wantErr) - } - if !reflect.DeepEqual(got, testcase.want) { - t.Errorf("GetObjectStorageKeys() = %v, want %v", got, testcase.want) - } - }) - } -} 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 475cc4cee..ac9a20bd4 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_linodeobjectstoragebuckets.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_linodeobjectstoragebuckets.yaml @@ -222,6 +222,21 @@ spec: description: LinodeObjectStorageBucketSpec defines the desired state of LinodeObjectStorageBucket properties: + acl: + default: private + description: ACL sets The Access Control Level of the bucket using + a canned ACL string + enum: + - private + - public-read + - authenticated-read + - public-read-write + type: string + corsEnabled: + default: true + description: corsEnabled enables for all origins in the bucket .If + set to false, CORS is disabled for all origins in the bucket + type: boolean credentialsRef: description: |- CredentialsRef is a reference to a Secret that contains the credentials to use for provisioning the bucket. @@ -237,11 +252,6 @@ spec: type: string type: object x-kubernetes-map-type: atomic - keyGeneration: - default: 0 - description: KeyGeneration may be modified to trigger rotations of - access keys created for the bucket. - type: integer region: description: Region is the ID of the Object Storage region for the bucket. @@ -261,12 +271,6 @@ spec: description: LinodeObjectStorageBucketStatus defines the observed state of LinodeObjectStorageBucket properties: - accessKeyRefs: - description: AccessKeyRefs stores IDs for Object Storage keys provisioned - along with the bucket. - items: - type: integer - type: array conditions: description: Conditions specify the service state of the LinodeObjectStorageBucket. items: @@ -326,13 +330,6 @@ spec: hostname: description: Hostname is the address assigned to the bucket. type: string - keySecretName: - description: KeySecretName specifies the name of the Secret containing - access keys for the bucket. - type: string - lastKeyGeneration: - description: LastKeyGeneration tracks the last known value of .spec.keyGeneration. - type: integer ready: default: false description: Ready denotes that the bucket has been provisioned along diff --git a/controller/linodeobjectstoragebucket_controller.go b/controller/linodeobjectstoragebucket_controller.go index 573871162..a8ce79069 100644 --- a/controller/linodeobjectstoragebucket_controller.go +++ b/controller/linodeobjectstoragebucket_controller.go @@ -23,7 +23,6 @@ import ( "time" "github.com/go-logr/logr" - "github.com/linode/linodego" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -109,7 +108,6 @@ func (r *LinodeObjectStorageBucketReconciler) Reconcile(ctx context.Context, req return r.reconcile(ctx, bScope) } -//nolint:dupl // This follows the pattern used for the LinodeObjectStorageKey controller. func (r *LinodeObjectStorageBucketReconciler) reconcile(ctx context.Context, bScope *scope.ObjectStorageBucketScope) (res ctrl.Result, reterr error) { // Always close the scope when exiting this function so we can persist any LinodeObjectStorageBucket changes. defer func() { @@ -121,7 +119,7 @@ func (r *LinodeObjectStorageBucketReconciler) reconcile(ctx context.Context, bSc }() if !bScope.Bucket.DeletionTimestamp.IsZero() { - return res, r.reconcileDelete(ctx, bScope) + return res, r.reconcileDelete(bScope) } if err := bScope.AddFinalizer(ctx); err != nil { @@ -156,75 +154,6 @@ func (r *LinodeObjectStorageBucketReconciler) reconcileApply(ctx context.Context bScope.Bucket.Status.Hostname = util.Pointer(bucket.Hostname) bScope.Bucket.Status.CreationTime = &metav1.Time{Time: *bucket.Created} - - var keys [2]*linodego.ObjectStorageKey - var secretDeleted bool - - switch { - case bScope.ShouldInitKeys(), bScope.ShouldRotateKeys(): - newKeys, err := services.RotateObjectStorageKeys(ctx, bScope) - if err != nil { - bScope.Logger.Error(err, "Failed to provision new access keys") - r.setFailure(bScope, err) - - return err - } - bScope.Bucket.Status.AccessKeyRefs = []int{newKeys[0].ID, newKeys[1].ID} - keys = newKeys - - r.Recorder.Event(bScope.Bucket, corev1.EventTypeNormal, "KeysAssigned", "Object storage keys assigned") - - case bScope.Bucket.Status.AccessKeyRefs != nil: - secretDeleted, err = bScope.ShouldRestoreKeySecret(ctx) - if err != nil { - bScope.Logger.Error(err, "Failed to ensure access key secret exists") - r.setFailure(bScope, err) - - return err - } - - if secretDeleted { - sameKeys, err := services.GetObjectStorageKeys(ctx, bScope) - if err != nil { - bScope.Logger.Error(err, "Failed to restore access keys for deleted secret") - r.setFailure(bScope, err) - - return err - } - keys = sameKeys - } - - r.Recorder.Event(bScope.Bucket, corev1.EventTypeNormal, "KeysRetrieved", "Object storage keys retrieved") - } - - if keys[0] != nil && keys[1] != nil { - secret, err := bScope.GenerateKeySecret(ctx, keys, bucket) - if err != nil { - bScope.Logger.Error(err, "Failed to generate key secret") - r.setFailure(bScope, err) - - return err - } - - if secretDeleted { - err = bScope.Client.Create(ctx, secret) - } else { - _, err = controllerutil.CreateOrUpdate(ctx, bScope.Client, secret, func() error { return nil }) - } - if err != nil { - bScope.Logger.Error(err, "Failed to apply key secret") - r.setFailure(bScope, err) - - return err - } - bScope.Logger.Info(fmt.Sprintf("Secret %s was applied with new access keys", secret.Name)) - - bScope.Bucket.Status.KeySecretName = util.Pointer(secret.Name) - bScope.Bucket.Status.LastKeyGeneration = bScope.Bucket.Spec.KeyGeneration - - r.Recorder.Event(bScope.Bucket, corev1.EventTypeNormal, "KeysStored", "Object storage keys stored in secret") - } - r.Recorder.Event(bScope.Bucket, corev1.EventTypeNormal, "Synced", "Object storage bucket synced") bScope.Bucket.Status.Ready = true @@ -233,16 +162,9 @@ func (r *LinodeObjectStorageBucketReconciler) reconcileApply(ctx context.Context return nil } -func (r *LinodeObjectStorageBucketReconciler) reconcileDelete(ctx context.Context, bScope *scope.ObjectStorageBucketScope) error { +func (r *LinodeObjectStorageBucketReconciler) reconcileDelete(bScope *scope.ObjectStorageBucketScope) error { bScope.Logger.Info("Reconciling delete") - if err := services.RevokeObjectStorageKeys(ctx, bScope); err != nil { - bScope.Logger.Error(err, "failed to revoke access keys; keys must be manually revoked") - r.setFailure(bScope, err) - - return err - } - 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") @@ -255,8 +177,6 @@ func (r *LinodeObjectStorageBucketReconciler) reconcileDelete(ctx context.Contex controllerutil.RemoveFinalizer(bScope.Bucket, infrav1alpha2.GroupVersion.String()) } - r.Recorder.Event(bScope.Bucket, clusterv1.DeletedReason, "Revoked", "Object storage keys revoked") - return nil } diff --git a/controller/linodeobjectstoragebucket_controller_test.go b/controller/linodeobjectstoragebucket_controller_test.go index 7a649c40c..d60cce598 100644 --- a/controller/linodeobjectstoragebucket_controller_test.go +++ b/controller/linodeobjectstoragebucket_controller_test.go @@ -19,23 +19,19 @@ package controller import ( "context" "errors" - "fmt" "time" "github.com/linode/linodego" "go.uber.org/mock/gomock" - corev1 "k8s.io/api/core/v1" 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" - "k8s.io/utils/ptr" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/cluster-api/util/patch" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/reconcile" - "sigs.k8s.io/yaml" infrav1alpha2 "github.com/linode/cluster-api-provider-linode/api/v1alpha2" "github.com/linode/cluster-api-provider-linode/cloud/scope" @@ -47,24 +43,6 @@ import ( . "github.com/onsi/gomega" ) -type accessKeySecret struct { - APIVersion string `json:"apiVersion"` - Kind string `json:"kind"` - Metadata struct { - Name string `json:"name"` - Namespace string `json:"namespace"` - } `json:"metadata"` - StringData struct { - BucketName string `json:"bucket_name"` - BucketRegion string `json:"bucket_region"` - BucketEndpoint string `json:"bucket_endpoint"` - AccessKeyRW string `json:"access_key_rw"` - SecretKeyRW string `json:"secret_key_rw"` - AccessKeyRO string `json:"access_key_ro"` - SecretKeyRO string `json:"secret_key_ro"` - } `json:"stringData"` -} - var _ = Describe("lifecycle", Ordered, Label("bucket", "lifecycle"), func() { suite := NewControllerSuite(GinkgoT(), mock.MockLinodeClient{}) @@ -105,108 +83,56 @@ var _ = Describe("lifecycle", Ordered, Label("bucket", "lifecycle"), func() { suite.Run( OneOf( - Path(Call("bucket is created", func(ctx context.Context, mck Mock) { - getBucket := mck.LinodeClient.EXPECT().GetObjectStorageBucket(gomock.Any(), obj.Spec.Region, gomock.Any()).Return(nil, nil) - mck.LinodeClient.EXPECT().CreateObjectStorageBucket(gomock.Any(), gomock.Any()). - After(getBucket). - Return(&linodego.ObjectStorageBucket{ - Label: obj.Name, - Region: obj.Spec.Region, - Created: util.Pointer(time.Now()), - Hostname: "hostname", - }, nil) - })), Path( - Call("bucket is not created", func(ctx context.Context, mck Mock) { + Call("bucket is created", func(ctx context.Context, mck Mock) { getBucket := mck.LinodeClient.EXPECT().GetObjectStorageBucket(gomock.Any(), obj.Spec.Region, gomock.Any()).Return(nil, nil) - mck.LinodeClient.EXPECT().CreateObjectStorageBucket(gomock.Any(), gomock.Any()).After(getBucket).Return(nil, errors.New("create bucket error")) + mck.LinodeClient.EXPECT().CreateObjectStorageBucket(gomock.Any(), gomock.Any()). + After(getBucket). + Return(&linodego.ObjectStorageBucket{ + Label: "bucket", + Region: obj.Spec.Region, + Created: util.Pointer(time.Now()), + Hostname: "hostname", + }, nil) }), - Result("error", func(ctx context.Context, mck Mock) { + Result("resource status is updated", func(ctx context.Context, mck Mock) { + objectKey := client.ObjectKeyFromObject(&obj) bScope.LinodeClient = mck.LinodeClient _, err := reconciler.reconcile(ctx, &bScope) - Expect(err.Error()).To(ContainSubstring("create bucket error")) + Expect(err).NotTo(HaveOccurred()) + + By("status") + Expect(k8sClient.Get(ctx, objectKey, &obj)).To(Succeed()) + Expect(obj.Status.Ready).To(BeTrue()) + Expect(obj.Status.Conditions).To(HaveLen(1)) + Expect(obj.Status.Conditions[0].Type).To(Equal(clusterv1.ReadyCondition)) + Expect(*obj.Status.Hostname).To(Equal("hostname")) + Expect(obj.Status.CreationTime).NotTo(BeNil()) + + events := mck.Events() + Expect(events).To(ContainSubstring("Object storage bucket synced")) + + logOutput := mck.Logs() + Expect(logOutput).To(ContainSubstring("Reconciling apply")) }), ), - ), - OneOf( - Path(Call("keys are created", func(ctx context.Context, mck Mock) { - for idx := range 2 { - mck.LinodeClient.EXPECT().CreateObjectStorageKey(gomock.Any(), gomock.Any()). - Return(&linodego.ObjectStorageKey{ - ID: idx, - AccessKey: fmt.Sprintf("access-key-%d", idx), - SecretKey: fmt.Sprintf("secret-key-%d", idx), - }, nil) - } - })), Path( - Call("keys are not created", func(ctx context.Context, mck Mock) { - mck.LinodeClient.EXPECT().CreateObjectStorageKey(gomock.Any(), gomock.Any()).Return(nil, errors.New("create key error")) + Call("bucket is not created", func(ctx context.Context, mck Mock) { + getBucket := mck.LinodeClient.EXPECT().GetObjectStorageBucket(gomock.Any(), obj.Spec.Region, gomock.Any()).Return(nil, nil) + mck.LinodeClient.EXPECT().CreateObjectStorageBucket(gomock.Any(), gomock.Any()).After(getBucket).Return(nil, errors.New("create bucket error")) }), Result("error", func(ctx context.Context, mck Mock) { bScope.LinodeClient = mck.LinodeClient _, err := reconciler.reconcile(ctx, &bScope) - Expect(err.Error()).To(ContainSubstring("create key error")) + Expect(err.Error()).To(ContainSubstring("create bucket error")) }), ), ), - Result("resource status is updated and key secret is created", func(ctx context.Context, mck Mock) { - objectKey := client.ObjectKeyFromObject(&obj) - bScope.LinodeClient = mck.LinodeClient - _, err := reconciler.reconcile(ctx, &bScope) - Expect(err).NotTo(HaveOccurred()) - - By("status") - Expect(k8sClient.Get(ctx, objectKey, &obj)).To(Succeed()) - Expect(obj.Status.Ready).To(BeTrue()) - Expect(obj.Status.Conditions).To(HaveLen(1)) - Expect(obj.Status.Conditions[0].Type).To(Equal(clusterv1.ReadyCondition)) - Expect(*obj.Status.Hostname).To(Equal("hostname")) - Expect(obj.Status.CreationTime).NotTo(BeNil()) - Expect(*obj.Status.LastKeyGeneration).To(Equal(*obj.Spec.KeyGeneration)) - Expect(*obj.Status.LastKeyGeneration).To(Equal(0)) - Expect(*obj.Status.KeySecretName).To(Equal(fmt.Sprintf(scope.AccessKeyNameTemplate, "lifecycle"))) - Expect(obj.Status.AccessKeyRefs).To(HaveLen(scope.NumAccessKeys)) - - By("secret") - var secret corev1.Secret - secretKey := client.ObjectKey{Namespace: "default", Name: *obj.Status.KeySecretName} - Expect(k8sClient.Get(ctx, secretKey, &secret)).To(Succeed()) - Expect(secret.Data).To(HaveLen(1)) - - var key accessKeySecret - Expect(yaml.Unmarshal(secret.Data["bucket-details-secret.yaml"], &key)).NotTo(HaveOccurred()) - Expect(key.StringData.BucketName).To(Equal("lifecycle")) - Expect(key.StringData.BucketRegion).To(Equal("region")) - Expect(key.StringData.BucketEndpoint).To(Equal("hostname")) - Expect(key.StringData.AccessKeyRW).To(Equal("access-key-0")) - Expect(key.StringData.SecretKeyRW).To(Equal("secret-key-0")) - Expect(key.StringData.AccessKeyRO).To(Equal("access-key-1")) - Expect(key.StringData.SecretKeyRO).To(Equal("secret-key-1")) - - events := mck.Events() - Expect(events).To(ContainSubstring("Object storage keys assigned")) - Expect(events).To(ContainSubstring("Object storage keys stored in secret")) - Expect(events).To(ContainSubstring("Object storage bucket synced")) - - logOutput := mck.Logs() - Expect(logOutput).To(ContainSubstring("Reconciling apply")) - Expect(logOutput).To(ContainSubstring("Secret lifecycle-bucket-details was applied with new access keys")) - }), - Once("resource keyGeneration is modified", func(ctx context.Context, _ Mock) { - obj.Spec.KeyGeneration = ptr.To(1) + Once("resource ACL is modified", func(ctx context.Context, _ Mock) { + obj.Spec.ACL = infrav1alpha2.ACLPublicRead Expect(k8sClient.Update(ctx, &obj)).To(Succeed()) }), OneOf( - Path(Call("bucket is retrieved on update", func(ctx context.Context, mck Mock) { - mck.LinodeClient.EXPECT().GetObjectStorageBucket(gomock.Any(), obj.Spec.Region, gomock.Any()). - Return(&linodego.ObjectStorageBucket{ - Label: obj.Name, - Region: obj.Spec.Region, - Created: util.Pointer(time.Now()), - Hostname: "hostname", - }, nil) - })), Path( Call("bucket is not retrieved on update", func(ctx context.Context, mck Mock) { mck.LinodeClient.EXPECT().GetObjectStorageBucket(gomock.Any(), obj.Spec.Region, gomock.Any()).Return(nil, errors.New("get bucket error")) @@ -217,104 +143,20 @@ var _ = Describe("lifecycle", Ordered, Label("bucket", "lifecycle"), func() { Expect(err.Error()).To(ContainSubstring("get bucket error")) }), ), - ), - OneOf( - // nb: Order matters for paths of the same length. The leftmost path is evaluated first. - // If we evaluate the happy path first, the bucket resource is mutated so the error path won't occur. - Path( - Call("keys are not rotated", func(ctx context.Context, mck Mock) { - mck.LinodeClient.EXPECT().CreateObjectStorageKey(gomock.Any(), gomock.Any()).Return(nil, errors.New("create key error")) - }), - Result("error", func(ctx context.Context, mck Mock) { - bScope.LinodeClient = mck.LinodeClient - _, err := reconciler.reconcile(ctx, &bScope) - Expect(err.Error()).To(ContainSubstring("create key error")) - }), - ), - Path( - Call("keys are rotated", func(ctx context.Context, mck Mock) { - for idx := range 2 { - createCall := mck.LinodeClient.EXPECT().CreateObjectStorageKey(gomock.Any(), gomock.Any()). - Return(&linodego.ObjectStorageKey{ - ID: idx + 2, - AccessKey: fmt.Sprintf("access-key-%d", idx+2), - SecretKey: fmt.Sprintf("secret-key-%d", idx+2), - }, nil) - mck.LinodeClient.EXPECT().DeleteObjectStorageKey(gomock.Any(), idx).After(createCall).Return(nil) - } - }), - Result("resource lastKeyGeneration is updated", func(ctx context.Context, mck Mock) { - objectKey := client.ObjectKeyFromObject(&obj) - bScope.LinodeClient = mck.LinodeClient - _, err := reconciler.reconcile(ctx, &bScope) - Expect(err).NotTo(HaveOccurred()) - Expect(k8sClient.Get(ctx, objectKey, &obj)).To(Succeed()) - Expect(*obj.Status.LastKeyGeneration).To(Equal(1)) - - Expect(mck.Events()).To(ContainSubstring("Object storage keys assigned")) - - logOutput := mck.Logs() - Expect(logOutput).To(ContainSubstring("Reconciling apply")) - Expect(logOutput).To(ContainSubstring("Secret lifecycle-bucket-details was applied with new access keys")) - }), - ), - Path(Once("secret is deleted", func(ctx context.Context, _ Mock) { - var secret corev1.Secret - secretKey := client.ObjectKey{Namespace: "default", Name: *obj.Status.KeySecretName} - Expect(k8sClient.Get(ctx, secretKey, &secret)).To(Succeed()) - Expect(k8sClient.Delete(ctx, &secret)).To(Succeed()) - })), - ), - OneOf( - Path( - Call("keys are not retrieved", func(ctx context.Context, mck Mock) { - mck.LinodeClient.EXPECT().GetObjectStorageKey(gomock.Any(), gomock.Any()).Times(2).Return(nil, errors.New("get key error")) - }), - Result("error", func(ctx context.Context, mck Mock) { - bScope.LinodeClient = mck.LinodeClient - _, err := reconciler.reconcile(ctx, &bScope) - Expect(err.Error()).To(ContainSubstring("get key error")) - }), - ), Path( - Call("keys are retrieved", func(ctx context.Context, mck Mock) { - for idx := range 2 { - mck.LinodeClient.EXPECT().GetObjectStorageKey(gomock.Any(), idx+2). - Return(&linodego.ObjectStorageKey{ - ID: idx + 2, - AccessKey: fmt.Sprintf("access-key-%d", idx+2), - SecretKey: fmt.Sprintf("secret-key-%d", idx+2), - }, nil) - } + Call("bucket is 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", + Region: obj.Spec.Region, + Hostname: "hostname", + Created: util.Pointer(time.Now()), + }, nil) }), - Result("secret is restored", func(ctx context.Context, mck Mock) { + Result("success", func(ctx context.Context, mck Mock) { bScope.LinodeClient = mck.LinodeClient _, err := reconciler.reconcile(ctx, &bScope) - Expect(err).NotTo(HaveOccurred()) - - var secret corev1.Secret - secretKey := client.ObjectKey{Namespace: "default", Name: *obj.Status.KeySecretName} - Expect(k8sClient.Get(ctx, secretKey, &secret)).To(Succeed()) - Expect(secret.Data).To(HaveLen(1)) - - var key accessKeySecret - Expect(yaml.Unmarshal(secret.Data["bucket-details-secret.yaml"], &key)).NotTo(HaveOccurred()) - Expect(key.StringData.BucketName).To(Equal("lifecycle")) - Expect(key.StringData.BucketRegion).To(Equal("region")) - Expect(key.StringData.BucketEndpoint).To(Equal("hostname")) - Expect(key.StringData.AccessKeyRW).To(Equal("access-key-2")) - Expect(key.StringData.SecretKeyRW).To(Equal("secret-key-2")) - Expect(key.StringData.AccessKeyRO).To(Equal("access-key-3")) - Expect(key.StringData.SecretKeyRO).To(Equal("secret-key-3")) - - events := mck.Events() - Expect(events).To(ContainSubstring("Object storage keys retrieved")) - Expect(events).To(ContainSubstring("Object storage keys stored in secret")) - Expect(events).To(ContainSubstring("Object storage bucket synced")) - - logOutput := mck.Logs() - Expect(logOutput).To(ContainSubstring("Reconciling apply")) - Expect(logOutput).To(ContainSubstring("Secret lifecycle-bucket-details was applied with new access keys")) + Expect(err).ToNot(HaveOccurred()) }), ), ), @@ -324,35 +166,15 @@ var _ = Describe("lifecycle", Ordered, Label("bucket", "lifecycle"), func() { Expect(k8sClient.Delete(ctx, &obj)).To(Succeed()) Expect(k8sClient.Get(ctx, objectKey, &obj)).To(Succeed()) }), - OneOf( - Path( - Call("keys are not revoked", func(ctx context.Context, mck Mock) { - mck.LinodeClient.EXPECT().DeleteObjectStorageKey(gomock.Any(), gomock.Any()).Times(2).Return(errors.New("revoke error")) - }), - Result("error", func(ctx context.Context, mck Mock) { - bScope.LinodeClient = mck.LinodeClient - _, err := reconciler.reconcile(ctx, &bScope) - Expect(err.Error()).To(ContainSubstring("revoke error")) - }), - ), - Path( - Call("keys are revoked", func(ctx context.Context, mck Mock) { - mck.LinodeClient.EXPECT().DeleteObjectStorageKey(gomock.Any(), 2).Return(nil) - mck.LinodeClient.EXPECT().DeleteObjectStorageKey(gomock.Any(), 3).Return(nil) - }), - Result("finalizer is removed", 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.Events()).To(ContainSubstring("Object storage keys revoked")) - Expect(mck.Logs()).To(ContainSubstring("Reconciling delete")) - }), - ), - ), + Result("finalizer is removed", 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")) + }), ) }) @@ -439,89 +261,13 @@ var _ = Describe("errors", Label("bucket", "errors"), func() { _, err = reconciler.reconcile(ctx, &bScope) Expect(err.Error()).To(ContainSubstring("no kind is registered")) }), - Call("get bucket", func(ctx context.Context, mck Mock) { - mck.LinodeClient.EXPECT().GetObjectStorageBucket(gomock.Any(), gomock.Any(), gomock.Any()).Return(&linodego.ObjectStorageBucket{Created: ptr.To(time.Now())}, nil) - }), - OneOf( - Path( - Call("failed check for deleted secret", func(ctx context.Context, mck Mock) { - mck.K8sClient.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Return(errors.New("api error")) - }), - Result("error", func(ctx context.Context, mck Mock) { - bScope.Bucket.Spec.KeyGeneration = ptr.To(1) - bScope.Bucket.Status.LastKeyGeneration = bScope.Bucket.Spec.KeyGeneration - bScope.Bucket.Status.KeySecretName = ptr.To("mock-bucket-details") - bScope.Bucket.Status.AccessKeyRefs = []int{0, 1} - - bScope.LinodeClient = mck.LinodeClient - bScope.Client = mck.K8sClient - err := reconciler.reconcileApply(ctx, &bScope) - Expect(err.Error()).To(ContainSubstring("api error")) - Expect(mck.Events()).To(ContainSubstring("api error")) - Expect(mck.Logs()).To(ContainSubstring("Failed to ensure access key secret exists")) - }), - ), - Path(Call("secret deleted", func(ctx context.Context, mck Mock) { - mck.K8sClient.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Return(apierrors.NewNotFound(schema.GroupResource{Resource: "Secret"}, "mock-bucket-details")) - })), - ), - Call("get keys", func(ctx context.Context, mck Mock) { - for idx := range 2 { - mck.LinodeClient.EXPECT().GetObjectStorageKey(gomock.Any(), idx).Return(&linodego.ObjectStorageKey{ID: idx}, nil) - } - }), - OneOf( - Path( - Call("secret resource creation fails", func(ctx context.Context, mck Mock) { - mck.K8sClient.EXPECT().Scheme().Return(scheme.Scheme).AnyTimes() - mck.K8sClient.EXPECT().Create(gomock.Any(), gomock.Any(), gomock.Any()).Return(errors.New("secret creation error")) - }), - Result("creation error", func(ctx context.Context, mck Mock) { - bScope.Bucket.Spec.KeyGeneration = ptr.To(1) - bScope.Bucket.Status.LastKeyGeneration = bScope.Bucket.Spec.KeyGeneration - bScope.Bucket.Status.KeySecretName = ptr.To("mock-bucket-details") - bScope.Bucket.Status.AccessKeyRefs = []int{0, 1} - - bScope.LinodeClient = mck.LinodeClient - bScope.Client = mck.K8sClient - err := reconciler.reconcileApply(ctx, &bScope) - Expect(err.Error()).To(ContainSubstring("secret creation error")) - Expect(mck.Events()).To(ContainSubstring("keys retrieved")) - Expect(mck.Events()).To(ContainSubstring("secret creation error")) - Expect(mck.Logs()).To(ContainSubstring("Failed to apply key secret")) - }), - ), - Path( - Call("secret generation fails", func(ctx context.Context, mck Mock) { - mck.K8sClient.EXPECT().Scheme().Return(runtime.NewScheme()) - }), - Result("error", func(ctx context.Context, mck Mock) { - bScope.Bucket.Spec.KeyGeneration = ptr.To(1) - bScope.Bucket.Status.LastKeyGeneration = bScope.Bucket.Spec.KeyGeneration - bScope.Bucket.Status.KeySecretName = ptr.To("mock-bucket-details") - bScope.Bucket.Status.AccessKeyRefs = []int{0, 1} - - bScope.LinodeClient = mck.LinodeClient - bScope.Client = mck.K8sClient - err := reconciler.reconcileApply(ctx, &bScope) - Expect(err.Error()).To(ContainSubstring("no kind is registered")) - Expect(mck.Events()).To(ContainSubstring("keys retrieved")) - Expect(mck.Events()).To(ContainSubstring("no kind is registered")) - Expect(mck.Logs()).To(ContainSubstring("Failed to generate key secret")) - }), - ), - ), Once("finalizer is missing", func(ctx context.Context, _ Mock) { - bScope.Bucket.Status.AccessKeyRefs = []int{0, 1} bScope.Bucket.ObjectMeta.Finalizers = []string{} }), - Call("revoke keys", func(ctx context.Context, mck Mock) { - mck.LinodeClient.EXPECT().DeleteObjectStorageKey(gomock.Any(), gomock.Any()).Times(2).Return(nil) - }), Result("error", func(ctx context.Context, mck Mock) { bScope.LinodeClient = mck.LinodeClient bScope.Client = mck.K8sClient - err := reconciler.reconcileDelete(ctx, &bScope) + 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/controller/linodeobjectstoragekey_controller.go b/controller/linodeobjectstoragekey_controller.go index 6fbee7c3d..33e6bc2cf 100644 --- a/controller/linodeobjectstoragekey_controller.go +++ b/controller/linodeobjectstoragekey_controller.go @@ -113,7 +113,6 @@ func (r *LinodeObjectStorageKeyReconciler) Reconcile(ctx context.Context, req ct return r.reconcile(ctx, keyScope) } -//nolint:dupl // This follows the pattern used for the LinodeObjectStorageBucket controller. func (r *LinodeObjectStorageKeyReconciler) reconcile(ctx context.Context, keyScope *scope.ObjectStorageKeyScope) (res ctrl.Result, reterr error) { // Always close the scope when exiting this function so we can persist any LinodeObjectStorageKey changes. defer func() { diff --git a/e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/assert-key-secret.yaml b/e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/assert-key-secret.yaml deleted file mode 100644 index 4693637bb..000000000 --- a/e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/assert-key-secret.yaml +++ /dev/null @@ -1,4 +0,0 @@ -apiVersion: v1 -kind: Secret -metadata: - name: ($access_keys_secret) diff --git a/e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/assert-keygen.yaml b/e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/assert-keygen.yaml deleted file mode 100644 index 002d2aee1..000000000 --- a/e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/assert-keygen.yaml +++ /dev/null @@ -1,11 +0,0 @@ -apiVersion: infrastructure.cluster.x-k8s.io/v1alpha2 -kind: LinodeObjectStorageBucket -metadata: - name: ($bucket) -spec: - region: us-sea - keyGeneration: 1 -status: - ready: true - keySecretName: ($access_keys_secret) - lastKeyGeneration: 1 diff --git a/e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/assert-obj-and-secret.yaml b/e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/assert-obj-and-secret.yaml deleted file mode 100644 index c6532c454..000000000 --- a/e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/assert-obj-and-secret.yaml +++ /dev/null @@ -1,17 +0,0 @@ ---- -apiVersion: infrastructure.cluster.x-k8s.io/v1alpha2 -kind: LinodeObjectStorageBucket -metadata: - name: ($bucket) -spec: - region: us-sea - keyGeneration: 0 -status: - ready: true - keySecretName: ($access_keys_secret) - lastKeyGeneration: 0 ---- -apiVersion: v1 -kind: Secret -metadata: - name: ($access_keys_secret) diff --git a/e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/patch-obj.yaml b/e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/assert-obj.yaml similarity index 83% rename from e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/patch-obj.yaml rename to e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/assert-obj.yaml index 67f07c126..01aefed1d 100644 --- a/e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/patch-obj.yaml +++ b/e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/assert-obj.yaml @@ -1,7 +1,9 @@ +--- apiVersion: infrastructure.cluster.x-k8s.io/v1alpha2 kind: LinodeObjectStorageBucket metadata: name: ($bucket) spec: region: us-sea - keyGeneration: 1 +status: + ready: true diff --git a/e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/chainsaw-test.yaml b/e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/chainsaw-test.yaml index 0b5a3e271..f658c9918 100755 --- a/e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/chainsaw-test.yaml +++ b/e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/chainsaw-test.yaml @@ -17,8 +17,6 @@ spec: # Format the bucket name into a valid Kubernetes object name # TODO: This is over-truncated to account for the Kubernetes access key Secret value: (trim((truncate(($run), `52`)), '-')) - - name: access_keys_secret - value: (join('-', [($bucket), 'bucket-details'])) template: true steps: - name: Check if CAPI provider resources exist @@ -30,14 +28,11 @@ spec: - apply: file: create-linodeobjectstoragebucket.yaml - assert: - file: assert-obj-and-secret.yaml + file: assert-obj.yaml catch: - describe: apiVersion: infrastructure.cluster.x-k8s.io/v1alpha2 kind: LinodeObjectStorageBucket - - describe: - apiVersion: v1 - kind: Secret - name: Check if the bucket was created try: - script: @@ -54,31 +49,6 @@ spec: ($error): ~ (json_parse($stdout)): label: ($bucket) - - name: Test Key Generation - try: - - apply: - file: patch-obj.yaml - - assert: - file: assert-keygen.yaml - catch: - - describe: - apiVersion: infrastructure.cluster.x-k8s.io/v1alpha2 - kind: LinodeObjectStorageBucket - - name: Delete LinodeObjectStorageBucket details Secret - try: - - delete: - ref: - apiVersion: v1 - kind: Secret - name: (join('-', [($namespace), 'backups-bucket-details'])) - - name: Assert LinodeObjectStorageBucket key secret exists - try: - - assert: - file: assert-key-secret.yaml - catch: - - describe: - apiVersion: v1 - kind: Secret - name: Delete LinodeObjectStorageBucket try: - delete: @@ -95,10 +65,10 @@ spec: curl -s \ -H "Authorization: Bearer $LINODE_TOKEN" \ -X DELETE \ - "https://api.linode.com/v4/object-storage/buckets/us-sea-1/$BUCKET" + "https://api.linode.com/v4/object-storage/buckets/us-sea/$BUCKET" check: ($error): ~ - name: Check if the LinodeObjectStorageBucket was deleted try: - error: - file: check-obj-and-key-deletion.yaml + file: check-obj-deletion.yaml diff --git a/e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/check-obj-and-key-deletion.yaml b/e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/check-obj-deletion.yaml similarity index 61% rename from e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/check-obj-and-key-deletion.yaml rename to e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/check-obj-deletion.yaml index a7a731828..d5c53e336 100644 --- a/e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/check-obj-and-key-deletion.yaml +++ b/e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/check-obj-deletion.yaml @@ -2,8 +2,3 @@ apiVersion: infrastructure.cluster.x-k8s.io/v1alpha2 kind: LinodeObjectStorageBucket metadata: name: ($bucket) ---- -apiVersion: v1 -kind: Secret -metadata: - name: ($access_keys_secret)