diff --git a/Tiltfile b/Tiltfile index 3d8e39b76..10b3f39ec 100644 --- a/Tiltfile +++ b/Tiltfile @@ -39,6 +39,7 @@ k8s_resource( "linodeclustertemplates.infrastructure.cluster.x-k8s.io:customresourcedefinition", "linodemachinetemplates.infrastructure.cluster.x-k8s.io:customresourcedefinition", "linodevpcs.infrastructure.cluster.x-k8s.io:customresourcedefinition", + "linodeobjectstoragebuckets.infrastructure.cluster.x-k8s.io:customresourcedefinition", "capl-controller-manager:serviceaccount", "capl-leader-election-role:role", "capl-manager-role:clusterrole", diff --git a/cloud/scope/client.go b/cloud/scope/client.go index a45f909db..7f4de2e11 100644 --- a/cloud/scope/client.go +++ b/cloud/scope/client.go @@ -11,6 +11,7 @@ import ( type LinodeObjectStorageClient interface { GetObjectStorageBucket(ctx context.Context, cluster, label string) (*linodego.ObjectStorageBucket, error) CreateObjectStorageBucket(ctx context.Context, opts linodego.ObjectStorageBucketCreateOptions) (*linodego.ObjectStorageBucket, 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 93e6be7d6..c11c0677a 100644 --- a/cloud/scope/object_storage_bucket.go +++ b/cloud/scope/object_storage_bucket.go @@ -8,8 +8,10 @@ import ( "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" infrav1alpha1 "github.com/linode/cluster-api-provider-linode/api/v1alpha1" @@ -23,11 +25,11 @@ type ObjectStorageBucketScopeParams struct { } type ObjectStorageBucketScope struct { - client k8sClient - Bucket *infrav1alpha1.LinodeObjectStorageBucket - Logger logr.Logger - LinodeClient LinodeObjectStorageClient - BucketPatchHelper *patch.Helper + Client k8sClient + Bucket *infrav1alpha1.LinodeObjectStorageBucket + Logger logr.Logger + LinodeClient LinodeObjectStorageClient + PatchHelper *patch.Helper } const AccessKeyNameTemplate = "%s-access-keys" @@ -65,23 +67,23 @@ func NewObjectStorageBucketScope(ctx context.Context, apiKey string, params Obje return nil, fmt.Errorf("failed to create linode client: %w", err) } - bucketPatchHelper, err := patch.NewHelper(params.Bucket, params.Client) + patchHelper, err := patch.NewHelper(params.Bucket, params.Client) if err != nil { return nil, fmt.Errorf("failed to init patch helper: %w", err) } return &ObjectStorageBucketScope{ - client: params.Client, - Bucket: params.Bucket, - Logger: *params.Logger, - LinodeClient: linodeClient, - BucketPatchHelper: bucketPatchHelper, + Client: params.Client, + Bucket: params.Bucket, + Logger: *params.Logger, + LinodeClient: linodeClient, + PatchHelper: patchHelper, }, nil } // PatchObject persists the object storage bucket configuration and status. func (s *ObjectStorageBucketScope) PatchObject(ctx context.Context) error { - return s.BucketPatchHelper.Patch(ctx, s.Bucket) + return s.PatchHelper.Patch(ctx, s.Bucket) } // Close closes the current scope persisting the object storage bucket configuration and status. @@ -99,8 +101,17 @@ func (s *ObjectStorageBucketScope) AddFinalizer(ctx context.Context) error { return nil } -// ApplyAccessKeySecret applies a Secret containing keys created for accessing the bucket. -func (s *ObjectStorageBucketScope) ApplyAccessKeySecret(ctx context.Context, keys [NumAccessKeys]linodego.ObjectStorageKey, secretName string) error { +// 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) (*corev1.Secret, error) { + for _, key := range keys { + if key == nil { + return nil, errors.New("expected two non-nil object storage keys") + } + } + + secretName := fmt.Sprintf(AccessKeyNameTemplate, s.Bucket.Name) + secret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: secretName, @@ -112,20 +123,34 @@ func (s *ObjectStorageBucketScope) ApplyAccessKeySecret(ctx context.Context, key }, } - if err := controllerutil.SetOwnerReference(s.Bucket, secret, s.client.Scheme()); err != nil { - return fmt.Errorf("could not set owner ref on access key secret %s: %w", secretName, err) + 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) } - - result, err := controllerutil.CreateOrPatch(ctx, s.client, secret, func() error { return nil }) - if err != nil { - return fmt.Errorf("could not create/patch 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) } - s.Logger.Info(fmt.Sprintf("Secret %s was %s with new access keys", secret.Name, result)) + return secret, nil +} - return nil +func (s *ObjectStorageBucketScope) ShouldInitKeys() bool { + return s.Bucket.Status.LastKeyGeneration == nil } func (s *ObjectStorageBucketScope) ShouldRotateKeys() bool { - return *s.Bucket.Spec.KeyGeneration != *s.Bucket.Status.LastKeyGeneration + 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 23c04b11c..771b40441 100644 --- a/cloud/scope/object_storage_bucket_test.go +++ b/cloud/scope/object_storage_bucket_test.go @@ -2,16 +2,20 @@ 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" @@ -245,7 +249,7 @@ func TestObjectStorageBucketScopeMethods(t *testing.T) { expects func(mock *mock.Mockk8sClient) }{ { - name: "Success - finalizer should be added to the Linode Machine object", + name: "Success - finalizer should be added to the Linode Object Storage Bucket object", Bucket: &infrav1alpha1.LinodeObjectStorageBucket{}, expects: func(mock *mock.Mockk8sClient) { mock.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme { @@ -308,133 +312,131 @@ func TestObjectStorageBucketScopeMethods(t *testing.T) { } } -func TestApplyAccessKeySecretUpdate(t *testing.T) { +func TestGenerateKeySecret(t *testing.T) { t.Parallel() - type args struct { - keys [NumAccessKeys]linodego.ObjectStorageKey - secretName string - } tests := []struct { name string Bucket *infrav1alpha1.LinodeObjectStorageBucket - args args + keys [NumAccessKeys]*linodego.ObjectStorageKey expectedErr error expects func(mock *mock.Mockk8sClient) }{ { - name: "Success - Create/Patch access key secret. Return no error", + name: "happy path", Bucket: &infrav1alpha1.LinodeObjectStorageBucket{ ObjectMeta: metav1.ObjectMeta{ Name: "test-bucket", Namespace: "test-namespace", }, Status: infrav1alpha1.LinodeObjectStorageBucketStatus{ - KeySecretName: ptr.To("test-secret"), + KeySecretName: ptr.To("test-bucket-access-keys"), }, }, - args: args{ - 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", - Cluster: "test-bucket", - Permissions: "read_write", - }, + 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", + Cluster: "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", - Cluster: "test-bucket", - Permissions: "read_only", - }, + }, + { + ID: 2, + Label: "read_only", + SecretKey: "read_only_key", + AccessKey: "read_only_access_key", + Limited: true, + BucketAccess: &[]linodego.ObjectStorageKeyBucketAccess{ + { + BucketName: "bucket", + Cluster: "test-bucket", + Permissions: "read_only", }, }, }, - secretName: "test-secret", }, expects: func(mock *mock.Mockk8sClient) { mock.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme { s := runtime.NewScheme() infrav1alpha1.AddToScheme(s) return s - }) - mock.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) + }).Times(1) }, expectedErr: nil, }, { - name: "Error - could not create/patch access key secret", + name: "missing one or more keys", Bucket: &infrav1alpha1.LinodeObjectStorageBucket{ ObjectMeta: metav1.ObjectMeta{ Name: "test-bucket", Namespace: "test-namespace", }, Status: infrav1alpha1.LinodeObjectStorageBucketStatus{ - KeySecretName: ptr.To("test-secret"), + KeySecretName: ptr.To("test-bucket-access-keys"), }, }, - args: args{ - 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", - Cluster: "test-bucket", - Permissions: "read_write", - }, + 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", + Cluster: "test-bucket", + Permissions: "read_write", }, }, }, - secretName: "test-secret", - }, - expects: func(mock *mock.Mockk8sClient) { - mock.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme { - s := runtime.NewScheme() - infrav1alpha1.AddToScheme(s) - return s - }) - mock.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Return(fmt.Errorf("could not update secret")).Times(1) }, - expectedErr: fmt.Errorf("could not create/patch access key secret"), + expectedErr: errors.New("expected two non-nil object storage keys"), }, { - name: "Error - controllerutil.SetOwnerReference() return an error", + name: "client scheme does not have infrav1alpha1", Bucket: &infrav1alpha1.LinodeObjectStorageBucket{ ObjectMeta: metav1.ObjectMeta{ Name: "test-bucket", Namespace: "test-namespace", }, }, - args: args{ - keys: [NumAccessKeys]linodego.ObjectStorageKey{ - { - ID: 1, - Label: "read_write", - SecretKey: "read_write_key", - AccessKey: "read_write_access_key", - Limited: false, - BucketAccess: nil, + 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", + Cluster: "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", + Cluster: "test-bucket", + Permissions: "read_only", + }, }, }, - secretName: "test-secret", }, expects: func(mock *mock.Mockk8sClient) { mock.EXPECT().Scheme().Return(runtime.NewScheme()) @@ -447,23 +449,69 @@ func TestApplyAccessKeySecretUpdate(t *testing.T) { t.Run(testcase.name, func(t *testing.T) { t.Parallel() - ctrl := gomock.NewController(t) - defer ctrl.Finish() + var mockClient *mock.Mockk8sClient + if testcase.expects != nil { + ctrl := gomock.NewController(t) + defer ctrl.Finish() - mockK8sClient := mock.NewMockk8sClient(ctrl) - testcase.expects(mockK8sClient) + mockClient = mock.NewMockk8sClient(ctrl) + testcase.expects(mockClient) + } objScope := &ObjectStorageBucketScope{ - client: mockK8sClient, - Bucket: testcase.Bucket, - Logger: logr.Logger{}, - LinodeClient: nil, - BucketPatchHelper: nil, + Client: mockClient, + Bucket: testcase.Bucket, } - err := objScope.ApplyAccessKeySecret(context.Background(), testcase.args.keys, testcase.args.secretName) + secret, err := objScope.GenerateKeySecret(context.Background(), testcase.keys) if testcase.expectedErr != nil { - assert.ErrorContains(t, err, testcase.expectedErr.Error()) + 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 *infrav1alpha1.LinodeObjectStorageBucket + }{ + { + name: "should init keys", + want: true, + Bucket: &infrav1alpha1.LinodeObjectStorageBucket{ + Spec: infrav1alpha1.LinodeObjectStorageBucketSpec{ + KeyGeneration: ptr.To(1), + }, + Status: infrav1alpha1.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) } }) } @@ -495,11 +543,11 @@ func TestShouldRotateKeys(t *testing.T) { t.Parallel() objScope := &ObjectStorageBucketScope{ - client: nil, - Bucket: testcase.Bucket, - Logger: logr.Logger{}, - LinodeClient: &linodego.Client{}, - BucketPatchHelper: &patch.Helper{}, + Client: nil, + Bucket: testcase.Bucket, + Logger: logr.Logger{}, + LinodeClient: &linodego.Client{}, + PatchHelper: &patch.Helper{}, } rotate := objScope.ShouldRotateKeys() @@ -510,3 +558,105 @@ func TestShouldRotateKeys(t *testing.T) { }) } } + +func TestShouldRestoreKeySecret(t *testing.T) { + t.Parallel() + tests := []struct { + name string + bucket *infrav1alpha1.LinodeObjectStorageBucket + expects func(k8s *mock.Mockk8sClient) + want bool + expectedErr error + }{ + { + name: "status has no secret name", + bucket: &infrav1alpha1.LinodeObjectStorageBucket{ + Status: infrav1alpha1.LinodeObjectStorageBucketStatus{ + KeySecretName: nil, + }, + }, + want: false, + }, + { + name: "status has secret name and secret exists", + bucket: &infrav1alpha1.LinodeObjectStorageBucket{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bucket", + Namespace: "ns", + }, + Status: infrav1alpha1.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: &infrav1alpha1.LinodeObjectStorageBucket{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bucket", + Namespace: "ns", + }, + Status: infrav1alpha1.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: &infrav1alpha1.LinodeObjectStorageBucket{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bucket", + Namespace: "ns", + }, + Status: infrav1alpha1.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 8240ad232..e3a84dd4e 100644 --- a/cloud/services/object_storage_buckets.go +++ b/cloud/services/object_storage_buckets.go @@ -2,6 +2,7 @@ package services import ( "context" + "errors" "fmt" "net/http" @@ -42,8 +43,8 @@ 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 +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 @@ -58,11 +59,11 @@ func RotateObjectStorageKeys(ctx context.Context, bScope *scope.ObjectStorageBuc return newKeys, err } - newKeys[idx] = *key + newKeys[idx] = key } // If key revocation fails here, just log the errors since new keys have been created - if bScope.Bucket.Status.LastKeyGeneration != nil && bScope.ShouldRotateKeys() { + 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") } @@ -117,3 +118,25 @@ func revokeObjectStorageKey(ctx context.Context, bScope *scope.ObjectStorageBuck 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 new file mode 100644 index 000000000..5ac0084a9 --- /dev/null +++ b/cloud/services/object_storage_buckets_test.go @@ -0,0 +1,198 @@ +package services + +import ( + "context" + "errors" + "reflect" + "strings" + "testing" + + "github.com/linode/linodego" + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" + + infrav1alpha1 "github.com/linode/cluster-api-provider-linode/api/v1alpha1" + "github.com/linode/cluster-api-provider-linode/cloud/scope" + "github.com/linode/cluster-api-provider-linode/mock" +) + +func TestRotateObjectStorageKeysRevocation(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + bucket *infrav1alpha1.LinodeObjectStorageBucket + expects func(*mock.MockLinodeObjectStorageClient) + }{ + { + name: "should revoke existing keys", + bucket: &infrav1alpha1.LinodeObjectStorageBucket{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bucket", + }, + Spec: infrav1alpha1.LinodeObjectStorageBucketSpec{ + KeyGeneration: ptr.To(1), + }, + Status: infrav1alpha1.LinodeObjectStorageBucketStatus{ + LastKeyGeneration: ptr.To(0), + AccessKeyRefs: []int{0, 1}, + }, + }, + expects: func(mockClient *mock.MockLinodeObjectStorageClient) { + for keyID := range 2 { + mockClient.EXPECT(). + DeleteObjectStorageKey(gomock.Any(), keyID). + Return(nil). + Times(1) + } + }, + }, + { + name: "shouldInitKeys", + bucket: &infrav1alpha1.LinodeObjectStorageBucket{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bucket", + }, + Status: infrav1alpha1.LinodeObjectStorageBucketStatus{ + LastKeyGeneration: nil, + }, + }, + }, + { + name: "not shouldRotateKeys", + bucket: &infrav1alpha1.LinodeObjectStorageBucket{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bucket", + }, + Spec: infrav1alpha1.LinodeObjectStorageBucketSpec{ + KeyGeneration: ptr.To(1), + }, + Status: infrav1alpha1.LinodeObjectStorageBucketStatus{ + LastKeyGeneration: ptr.To(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.NewMockLinodeObjectStorageClient(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.MockLinodeObjectStorageClient) + want [2]*linodego.ObjectStorageKey + wantErr string + }{ + { + name: "happy path", + bScope: &scope.ObjectStorageBucketScope{ + Bucket: &infrav1alpha1.LinodeObjectStorageBucket{ + Status: infrav1alpha1.LinodeObjectStorageBucketStatus{ + AccessKeyRefs: []int{0, 1}, + }, + }, + }, + expects: func(mockClient *mock.MockLinodeObjectStorageClient) { + 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: &infrav1alpha1.LinodeObjectStorageBucket{}, + }, + wantErr: "expected two object storage key IDs in .status.accessKeyRefs", + }, + { + name: "one client error", + bScope: &scope.ObjectStorageBucketScope{ + Bucket: &infrav1alpha1.LinodeObjectStorageBucket{ + Status: infrav1alpha1.LinodeObjectStorageBucketStatus{ + AccessKeyRefs: []int{0, 1}, + }, + }, + }, + expects: func(mockClient *mock.MockLinodeObjectStorageClient) { + 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.MockLinodeObjectStorageClient + if testcase.expects != nil { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockClient = mock.NewMockLinodeObjectStorageClient(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/cmd/main.go b/cmd/main.go index 0c2fd063f..ceab65f86 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -142,7 +142,6 @@ func main() { } if err = (&controller2.LinodeObjectStorageBucketReconciler{ Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), Logger: ctrl.Log.WithName("LinodeObjectStorageBucketReconciler"), Recorder: mgr.GetEventRecorderFor("LinodeObjectStorageBucketReconciler"), WatchFilterValue: objectStorageBucketWatchFilter, diff --git a/controller/linodeobjectstoragebucket_controller.go b/controller/linodeobjectstoragebucket_controller.go index 4679c1c8e..37a82102f 100644 --- a/controller/linodeobjectstoragebucket_controller.go +++ b/controller/linodeobjectstoragebucket_controller.go @@ -23,10 +23,10 @@ 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" - "k8s.io/apimachinery/pkg/runtime" utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/client-go/tools/record" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" @@ -47,7 +47,6 @@ import ( // LinodeObjectStorageBucketReconciler reconciles a LinodeObjectStorageBucket object type LinodeObjectStorageBucketReconciler struct { client.Client - Scheme *runtime.Scheme Logger logr.Logger Recorder record.EventRecorder LinodeApiKey string @@ -116,12 +115,14 @@ func (r *LinodeObjectStorageBucketReconciler) reconcile(ctx context.Context, bSc } }() - // Delete if !bScope.Bucket.DeletionTimestamp.IsZero() { return res, r.reconcileDelete(ctx, bScope) } - // Apply + if err := bScope.AddFinalizer(ctx); err != nil { + return res, err + } + if err := r.reconcileApply(ctx, bScope); err != nil { return res, err } @@ -140,10 +141,6 @@ func (r *LinodeObjectStorageBucketReconciler) reconcileApply(ctx context.Context bScope.Bucket.Status.Ready = false - if err := bScope.AddFinalizer(ctx); err != nil { - return err - } - bucket, err := services.EnsureObjectStorageBucket(ctx, bScope) if err != nil { bScope.Logger.Error(err, "Failed to ensure bucket exists") @@ -152,38 +149,78 @@ func (r *LinodeObjectStorageBucketReconciler) reconcileApply(ctx context.Context return err } - if bucket == nil { - err = fmt.Errorf("bucket created is nil") - bScope.Logger.Error(err, "Failed to ensure bucket exists") - r.setFailure(bScope, err) - return err - } - bScope.Bucket.Status.Hostname = util.Pointer(bucket.Hostname) bScope.Bucket.Status.CreationTime = &metav1.Time{Time: *bucket.Created} - if bScope.Bucket.Status.LastKeyGeneration == nil || bScope.ShouldRotateKeys() { - keys, err := services.RotateObjectStorageKeys(ctx, bScope) + 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{keys[0].ID, keys[1].ID} + 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) + if err != nil { + bScope.Logger.Error(err, "Failed to generate key secret") + r.setFailure(bScope, err) + + return err + } - secretName := fmt.Sprintf(scope.AccessKeyNameTemplate, bScope.Bucket.Name) - if err := bScope.ApplyAccessKeySecret(ctx, keys, secretName); err != nil { - bScope.Logger.Error(err, "Failed to apply access key secret") + 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.Bucket.Status.KeySecretName = util.Pointer(secretName) + 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, "Ready", "Object storage bucket applied") + r.Recorder.Event(bScope.Bucket, corev1.EventTypeNormal, "Synced", "Object storage bucket synced") bScope.Bucket.Status.Ready = true conditions.MarkTrue(bScope.Bucket, clusterv1.ReadyCondition) @@ -209,7 +246,7 @@ func (r *LinodeObjectStorageBucketReconciler) reconcileDelete(ctx context.Contex return err } - r.Recorder.Event(bScope.Bucket, clusterv1.DeletedReason, "Ready", "Object storage bucket deleted") + r.Recorder.Event(bScope.Bucket, clusterv1.DeletedReason, "Revoked", "Object storage keys revoked") return nil } @@ -218,6 +255,7 @@ func (r *LinodeObjectStorageBucketReconciler) reconcileDelete(ctx context.Contex func (r *LinodeObjectStorageBucketReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). For(&infrav1alpha1.LinodeObjectStorageBucket{}). + Owns(&corev1.Secret{}). WithEventFilter(predicate.And( predicates.ResourceHasFilterLabel(mgr.GetLogger(), r.WatchFilterValue), predicate.GenerationChangedPredicate{}, diff --git a/controller/linodeobjectstoragebucket_controller_test.go b/controller/linodeobjectstoragebucket_controller_test.go index c5b746af7..40a4a6552 100644 --- a/controller/linodeobjectstoragebucket_controller_test.go +++ b/controller/linodeobjectstoragebucket_controller_test.go @@ -17,7 +17,8 @@ package controller import ( - "context" + "bytes" + "errors" "fmt" "time" @@ -26,9 +27,15 @@ import ( 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/client-go/tools/record" - ctrl "sigs.k8s.io/controller-runtime" + "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/log/zap" "sigs.k8s.io/controller-runtime/pkg/reconcile" infrav1 "github.com/linode/cluster-api-provider-linode/api/v1alpha1" @@ -40,18 +47,20 @@ import ( . "github.com/onsi/gomega" ) -func mockClientBuilder(m *mock.MockLinodeObjectStorageClient) scope.LinodeObjectStorageClientBuilder { +func mockLinodeClientBuilder(m *mock.MockLinodeObjectStorageClient) scope.LinodeObjectStorageClientBuilder { return func(_ string) (scope.LinodeObjectStorageClient, error) { return m, nil } } -var _ = Describe("LinodeObjectStorageBucket controller", func() { - ctx := context.Background() +var _ = Describe("lifecycle", Label("lifecycle"), func() { + var mockCtrl *gomock.Controller + var reconciler *LinodeObjectStorageBucketReconciler + var testLogs *bytes.Buffer - obj := &infrav1.LinodeObjectStorageBucket{ + obj := infrav1.LinodeObjectStorageBucket{ ObjectMeta: metav1.ObjectMeta{ - Name: "sample-bucket", + Name: "lifecycle", Namespace: "default", }, Spec: infrav1.LinodeObjectStorageBucketSpec{ @@ -59,33 +68,51 @@ var _ = Describe("LinodeObjectStorageBucket controller", func() { }, } - recorder := record.NewFakeRecorder(3) + secret := corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf(scope.AccessKeyNameTemplate, obj.Name), + Namespace: "default", + }, + } - secretName := fmt.Sprintf(scope.AccessKeyNameTemplate, obj.Name) - - var secret corev1.Secret - var mockCtrl *gomock.Controller + // Create a recorder with a buffered channel for consuming event strings. + recorder := record.NewFakeRecorder(10) BeforeEach(func() { // Create a new gomock controller for each test run mockCtrl = gomock.NewController(GinkgoT()) + // Inject io.Writer as log sink for consuming logs + testLogs = &bytes.Buffer{} + reconciler = &LinodeObjectStorageBucketReconciler{ + Client: k8sClient, + Recorder: recorder, + Logger: zap.New( + zap.WriteTo(GinkgoWriter), + zap.WriteTo(testLogs), + zap.UseDevMode(true), + ), + } }) AfterEach(func() { // At the end of each test run, tell the gomock controller it's done // so it can check configured expectations and validate the methods called mockCtrl.Finish() + // Flush the channel if any events were not consumed. + for len(recorder.Events) > 0 { + <-recorder.Events + } }) - It("should reconcile an object apply", func() { - mockClient := mock.NewMockLinodeObjectStorageClient(mockCtrl) + It("should provision a bucket and keys", func(ctx SpecContext) { + mockLinodeClient := mock.NewMockLinodeObjectStorageClient(mockCtrl) - getCall := mockClient.EXPECT(). + getCall := mockLinodeClient.EXPECT(). GetObjectStorageBucket(gomock.Any(), obj.Spec.Cluster, gomock.Any()). Return(nil, nil). Times(1) - createBucketCall := mockClient.EXPECT(). + createBucketCall := mockLinodeClient.EXPECT(). CreateObjectStorageBucket(gomock.Any(), gomock.Any()). Return(&linodego.ObjectStorageBucket{ Label: obj.Name, @@ -96,88 +123,522 @@ var _ = Describe("LinodeObjectStorageBucket controller", func() { Times(1). After(getCall) - for idx, permission := range []string{"rw", "ro"} { - mockClient.EXPECT(). - CreateObjectStorageKey( - gomock.Any(), - gomock.Cond(func(opt any) bool { - createOpt, ok := opt.(linodego.ObjectStorageKeyCreateOptions) - if !ok { - return false - } - - return createOpt.Label == fmt.Sprintf("%s-%s", obj.Name, permission) - }), - ). - Return(&linodego.ObjectStorageKey{ID: idx}, nil). + for idx := range 2 { + mockLinodeClient.EXPECT(). + CreateObjectStorageKey(gomock.Any(), gomock.Any()). + DoAndReturn( + func(_ any, opts linodego.ObjectStorageKeyCreateOptions) (*linodego.ObjectStorageKey, error) { + return &linodego.ObjectStorageKey{ID: idx, AccessKey: fmt.Sprintf("key-%d", idx)}, nil + }). Times(1). After(createBucketCall) } - reconciler := &LinodeObjectStorageBucketReconciler{ - Client: k8sClient, - Scheme: k8sClient.Scheme(), - Logger: ctrl.Log.WithName("LinodeObjectStorageBucketReconciler"), - Recorder: recorder, - LinodeClientBuilder: mockClientBuilder(mockClient), - } + objectKey := client.ObjectKeyFromObject(&obj) + Expect(k8sClient.Create(ctx, &obj)).To(Succeed()) - objectKey := client.ObjectKeyFromObject(obj) - Expect(k8sClient.Create(ctx, obj)).To(Succeed()) + reconciler.LinodeClientBuilder = mockLinodeClientBuilder(mockLinodeClient) _, err := reconciler.Reconcile(ctx, reconcile.Request{ NamespacedName: objectKey, }) Expect(err).NotTo(HaveOccurred()) - By("updating its status fields") - Expect(k8sClient.Get(ctx, objectKey, obj)).To(Succeed()) + By("updating the bucket resource's status fields") + 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.KeySecretName).To(Equal(secretName)) + 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.Ready).To(BeTrue()) + Expect(*obj.Status.KeySecretName).To(Equal(secret.Name)) + Expect(obj.Status.AccessKeyRefs).To(HaveLen(scope.NumAccessKeys)) + + By("creating a secret with access keys") + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(&secret), &secret)).To(Succeed()) + Expect(secret.Data).To(HaveLen(2)) + Expect(string(secret.Data["read_write"])).To(Equal(string("key-0"))) + Expect(string(secret.Data["read_only"])).To(Equal(string("key-1"))) + + By("recording the expected events") + Expect(<-recorder.Events).To(ContainSubstring("Object storage keys assigned")) + Expect(<-recorder.Events).To(ContainSubstring("Object storage keys stored in secret")) + Expect(<-recorder.Events).To(ContainSubstring("Object storage bucket synced")) + + By("logging the expected messages") + logOutput := testLogs.String() + Expect(logOutput).To(ContainSubstring("Reconciling apply")) + Expect(logOutput).To(ContainSubstring("Secret lifecycle-access-keys was applied with new access keys")) + }) + + It("should ensure the bucket's secret exists", func(ctx SpecContext) { + mockLinodeClient := mock.NewMockLinodeObjectStorageClient(mockCtrl) + + getCall := mockLinodeClient.EXPECT(). + GetObjectStorageBucket(gomock.Any(), obj.Spec.Cluster, gomock.Any()). + Return(&linodego.ObjectStorageBucket{ + Label: obj.Name, + Cluster: obj.Spec.Cluster, + Created: util.Pointer(time.Now()), + Hostname: "hostname", + }, nil). + Times(1) + + for idx := range 2 { + mockLinodeClient.EXPECT(). + GetObjectStorageKey(gomock.Any(), idx). + Return(&linodego.ObjectStorageKey{ + ID: idx, + AccessKey: fmt.Sprintf("key-%d", idx), + }, nil). + Times(1). + After(getCall) + } + + objectKey := client.ObjectKeyFromObject(&obj) + Expect(k8sClient.Delete(ctx, &secret)).To(Succeed()) + + reconciler.LinodeClientBuilder = mockLinodeClientBuilder(mockLinodeClient) + _, err := reconciler.Reconcile(ctx, reconcile.Request{ + NamespacedName: objectKey, + }) + Expect(err).NotTo(HaveOccurred()) + + By("re-creating it when it is deleted") + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(&secret), &secret)).To(Succeed()) + Expect(secret.Data).To(HaveLen(2)) + Expect(string(secret.Data["read_write"])).To(Equal("key-0")) + Expect(string(secret.Data["read_only"])).To(Equal("key-1")) + + By("recording the expected events") + Expect(<-recorder.Events).To(ContainSubstring("Object storage keys retrieved")) + Expect(<-recorder.Events).To(ContainSubstring("Object storage keys stored in secret")) + Expect(<-recorder.Events).To(ContainSubstring("Object storage bucket synced")) + + By("logging the expected messages") + logOutput := testLogs.String() + Expect(logOutput).To(ContainSubstring("Reconciling apply")) + Expect(logOutput).To(ContainSubstring("Secret lifecycle-access-keys was applied with new access keys")) + }) + + It("should rotate the bucket's keys", func(ctx SpecContext) { + mockLinodeClient := mock.NewMockLinodeObjectStorageClient(mockCtrl) + + getCall := mockLinodeClient.EXPECT(). + GetObjectStorageBucket(gomock.Any(), obj.Spec.Cluster, gomock.Any()). + Return(&linodego.ObjectStorageBucket{ + Label: obj.Name, + Cluster: obj.Spec.Cluster, + Created: util.Pointer(time.Now()), + Hostname: "hostname", + }, nil). + Times(1) - By("creating a Secret with access keys") - Expect(k8sClient.Get(ctx, client.ObjectKey{ - Name: secretName, - Namespace: obj.Namespace, - }, &secret)).To(Succeed()) - Expect(secret.Data["read_write"]).To(Not(BeNil())) - Expect(secret.Data["read_only"]).To(Not(BeNil())) + for idx := range 2 { + createCall := mockLinodeClient.EXPECT(). + CreateObjectStorageKey(gomock.Any(), gomock.Any()). + After(getCall). + DoAndReturn( + func(_ any, opts linodego.ObjectStorageKeyCreateOptions) (*linodego.ObjectStorageKey, error) { + return &linodego.ObjectStorageKey{ID: idx + 2, AccessKey: fmt.Sprintf("key-%d", idx+2)}, nil + }). + Times(1) + mockLinodeClient.EXPECT(). + DeleteObjectStorageKey(gomock.Any(), idx). + After(createCall). + Return(nil). + Times(1) + } + + objectKey := client.ObjectKeyFromObject(&obj) + Expect(k8sClient.Get(ctx, objectKey, &obj)).To(Succeed()) + obj.Spec.KeyGeneration = ptr.To(1) + Expect(k8sClient.Update(ctx, &obj)).To(Succeed()) + + reconciler.LinodeClientBuilder = mockLinodeClientBuilder(mockLinodeClient) + _, err := reconciler.Reconcile(ctx, reconcile.Request{ + NamespacedName: objectKey, + }) + Expect(err).NotTo(HaveOccurred()) + + By("updating the bucket resource's status fields") + Expect(k8sClient.Get(ctx, objectKey, &obj)).To(Succeed()) + Expect(*obj.Status.LastKeyGeneration).To(Equal(1)) By("recording the expected event") - Expect(<-recorder.Events).To(ContainSubstring("Object storage bucket applied")) + Expect(<-recorder.Events).To(ContainSubstring("Object storage keys assigned")) + + By("logging the expected messages") + logOutput := testLogs.String() + Expect(logOutput).To(ContainSubstring("Reconciling apply")) + Expect(logOutput).To(ContainSubstring("Secret lifecycle-access-keys was applied with new access keys")) }) - It("should reconcile an object delete", func() { - mockClient := mock.NewMockLinodeObjectStorageClient(mockCtrl) + It("should revoke the bucket's keys", func(ctx SpecContext) { + mockLinodeClient := mock.NewMockLinodeObjectStorageClient(mockCtrl) for i := range 2 { - mockClient.EXPECT(). - DeleteObjectStorageKey(gomock.Any(), i). + mockLinodeClient.EXPECT(). + DeleteObjectStorageKey(gomock.Any(), i+2). Return(nil). Times(1) } - reconciler := &LinodeObjectStorageBucketReconciler{ - Client: k8sClient, - Scheme: k8sClient.Scheme(), - Logger: ctrl.Log.WithName("LinodeObjectStorageBucketReconciler"), - Recorder: recorder, - LinodeClientBuilder: mockClientBuilder(mockClient), - } + objectKey := client.ObjectKeyFromObject(&obj) + Expect(k8sClient.Delete(ctx, &obj)).To(Succeed()) - objectKey := client.ObjectKeyFromObject(obj) - Expect(k8sClient.Delete(ctx, obj)).To(Succeed()) + reconciler.LinodeClientBuilder = mockLinodeClientBuilder(mockLinodeClient) _, err := reconciler.Reconcile(ctx, reconcile.Request{ NamespacedName: objectKey, }) Expect(err).NotTo(HaveOccurred()) - By("removing the finalizer so it is deleted") - Expect(apierrors.IsNotFound(k8sClient.Get(ctx, objectKey, obj))).To(BeTrue()) + By("removing the bucket's finalizer so it is deleted") + Expect(apierrors.IsNotFound(k8sClient.Get(ctx, objectKey, &obj))).To(BeTrue()) By("recording the expected event") - Expect(<-recorder.Events).To(ContainSubstring("Object storage bucket deleted")) + Expect(<-recorder.Events).To(ContainSubstring("Object storage keys revoked")) + + By("logging the expected messages") + logOutput := testLogs.String() + Expect(logOutput).To(ContainSubstring("Reconciling delete")) + }) +}) + +var _ = Describe("pre-reconcile", Label("pre-reconcile"), func() { + var obj infrav1.LinodeObjectStorageBucket + var mockCtrl *gomock.Controller + var reconciler *LinodeObjectStorageBucketReconciler + var testLogs *bytes.Buffer + + recorder := record.NewFakeRecorder(10) + + BeforeEach(func() { + // Use a generated name to isolate objects per spec. + obj = infrav1.LinodeObjectStorageBucket{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "pre-reconcile-", + Namespace: "default", + }, + Spec: infrav1.LinodeObjectStorageBucketSpec{ + Cluster: "cluster", + }, + } + mockCtrl = gomock.NewController(GinkgoT()) + testLogs = &bytes.Buffer{} + reconciler = &LinodeObjectStorageBucketReconciler{ + Client: k8sClient, + Recorder: recorder, + Logger: zap.New( + zap.WriteTo(GinkgoWriter), + zap.WriteTo(testLogs), + zap.UseDevMode(true), + ), + } + }) + + AfterEach(func() { + mockCtrl.Finish() + for len(recorder.Events) > 0 { + <-recorder.Events + } + }) + + It("returns a nil error when the resource does not exist", func(ctx SpecContext) { + obj.Name = "empty" + _, err := reconciler.Reconcile(ctx, reconcile.Request{ + NamespacedName: client.ObjectKeyFromObject(&obj), + }) + Expect(err).To(BeNil()) + }) + + It("fails when the resource cannot be fetched", func(ctx SpecContext) { + mockK8sClient := mock.NewMockk8sClient(mockCtrl) + mockK8sClient.EXPECT(). + Get(gomock.Any(), gomock.Any(), gomock.Any()). + Return(errors.New("non-404 error")). + Times(1) + + reconciler.Client = mockK8sClient + _, err := reconciler.Reconcile(ctx, reconcile.Request{ + NamespacedName: client.ObjectKeyFromObject(&obj), + }) + Expect(err.Error()).To(ContainSubstring("non-404 error")) + Expect(testLogs.String()).To(ContainSubstring("Failed to fetch LinodeObjectStorageBucket")) + }) + + It("fails when a scope cannot be created due to missing arguments", func(ctx SpecContext) { + Expect(k8sClient.Create(ctx, &obj)).To(Succeed()) + _, err := reconciler.Reconcile(ctx, reconcile.Request{ + NamespacedName: client.ObjectKeyFromObject(&obj), + }) + Expect(err.Error()).To(ContainSubstring("failed to create object storage bucket scope")) + Expect(testLogs.String()).To(ContainSubstring("Failed to create object storage bucket scope")) + }) +}) + +var _ = Describe("apply", Label("apply"), func() { + var obj infrav1.LinodeObjectStorageBucket + var mockCtrl *gomock.Controller + var testLogs *bytes.Buffer + + recorder := record.NewFakeRecorder(10) + reconciler := &LinodeObjectStorageBucketReconciler{ + Logger: zap.New( + zap.WriteTo(GinkgoWriter), + zap.WriteTo(testLogs), + zap.UseDevMode(true), + ), + Recorder: recorder, + } + + BeforeEach(func() { + // We can use a consistent name since these tests are stateless. + obj = infrav1.LinodeObjectStorageBucket{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mock", + Namespace: "default", + UID: "12345", + }, + Spec: infrav1.LinodeObjectStorageBucketSpec{ + Cluster: "cluster", + }, + } + mockCtrl = gomock.NewController(GinkgoT()) + testLogs = &bytes.Buffer{} + reconciler.Logger = zap.New( + zap.WriteTo(GinkgoWriter), + zap.WriteTo(testLogs), + zap.UseDevMode(true), + ) + }) + + AfterEach(func() { + mockCtrl.Finish() + for len(recorder.Events) > 0 { + <-recorder.Events + } + }) + + It("fails when a finalizer cannot be added", Label("current"), func(ctx SpecContext) { + mockK8sClient := mock.NewMockk8sClient(mockCtrl) + prev := mockK8sClient.EXPECT(). + Scheme(). + Return(scheme.Scheme). + Times(1) + mockK8sClient.EXPECT(). + Scheme(). + After(prev). + Return(runtime.NewScheme()). + Times(2) + + patchHelper, err := patch.NewHelper(&obj, mockK8sClient) + Expect(err).NotTo(HaveOccurred()) + + // Create a scope directly since only a subset of fields are needed. + bScope := scope.ObjectStorageBucketScope{ + Client: mockK8sClient, + Bucket: &obj, + PatchHelper: patchHelper, + } + + _, err = reconciler.reconcile(ctx, &bScope) + Expect(err.Error()).To(ContainSubstring("no kind is registered")) + }) + + It("fails when it can't ensure a bucket exists", func(ctx SpecContext) { + mockLinodeClient := mock.NewMockLinodeObjectStorageClient(mockCtrl) + mockLinodeClient.EXPECT(). + GetObjectStorageBucket(gomock.Any(), gomock.Any(), gomock.Any()). + Return(nil, errors.New("non-404 error")). + Times(1) + + bScope := scope.ObjectStorageBucketScope{ + LinodeClient: mockLinodeClient, + Bucket: &obj, + Logger: reconciler.Logger, + } + + err := reconciler.reconcileApply(ctx, &bScope) + Expect(err.Error()).To(ContainSubstring("non-404 error")) + Expect(<-recorder.Events).To(ContainSubstring("non-404 error")) + Expect(testLogs.String()).To(ContainSubstring("Failed to ensure bucket exists")) + }) + + It("fails when it can't provision new access keys", func(ctx SpecContext) { + mockLinodeClient := mock.NewMockLinodeObjectStorageClient(mockCtrl) + mockLinodeClient.EXPECT(). + GetObjectStorageBucket(gomock.Any(), gomock.Any(), gomock.Any()). + Return(&linodego.ObjectStorageBucket{Created: ptr.To(time.Now())}, nil). + Times(1) + mockLinodeClient.EXPECT(). + CreateObjectStorageKey(gomock.Any(), gomock.Any()). + Return(nil, errors.New("api error")). + Times(1) + + bScope := scope.ObjectStorageBucketScope{ + LinodeClient: mockLinodeClient, + Bucket: &obj, + Logger: reconciler.Logger, + } + + err := reconciler.reconcileApply(ctx, &bScope) + Expect(err.Error()).To(ContainSubstring("api error")) + Expect(<-recorder.Events).To(ContainSubstring("api error")) + Expect(testLogs.String()).To(ContainSubstring("Failed to provision new access keys")) + }) + + It("fails when it can't evaluate whether to restore a key secret", func(ctx SpecContext) { + mockLinodeClient := mock.NewMockLinodeObjectStorageClient(mockCtrl) + mockLinodeClient.EXPECT(). + GetObjectStorageBucket(gomock.Any(), gomock.Any(), gomock.Any()). + Return(&linodego.ObjectStorageBucket{Created: ptr.To(time.Now())}, nil). + Times(1) + + mockK8sClient := mock.NewMockk8sClient(mockCtrl) + mockK8sClient.EXPECT(). + Get(gomock.Any(), gomock.Any(), gomock.Any()). + Return(errors.New("api error")). + Times(1) + + obj.Spec.KeyGeneration = ptr.To(1) + obj.Status.LastKeyGeneration = obj.Spec.KeyGeneration + obj.Status.KeySecretName = ptr.To("mock-access-keys") + obj.Status.AccessKeyRefs = []int{0, 1} + + bScope := scope.ObjectStorageBucketScope{ + Client: mockK8sClient, + LinodeClient: mockLinodeClient, + Bucket: &obj, + Logger: reconciler.Logger, + } + + err := reconciler.reconcileApply(ctx, &bScope) + Expect(err.Error()).To(ContainSubstring("api error")) + Expect(<-recorder.Events).To(ContainSubstring("api error")) + Expect(testLogs.String()).To(ContainSubstring("Failed to ensure access key secret exists")) + }) + + It("fails when it can't retrieve access keys for a deleted secret", func(ctx SpecContext) { + mockLinodeClient := mock.NewMockLinodeObjectStorageClient(mockCtrl) + mockLinodeClient.EXPECT(). + GetObjectStorageBucket(gomock.Any(), gomock.Any(), gomock.Any()). + Return(&linodego.ObjectStorageBucket{Created: ptr.To(time.Now())}, nil). + Times(1) + mockLinodeClient.EXPECT(). + GetObjectStorageKey(gomock.Any(), gomock.Any()). + Return(nil, errors.New("key creation error")). + Times(2) + + mockK8sClient := mock.NewMockk8sClient(mockCtrl) + mockK8sClient.EXPECT(). + Get(gomock.Any(), gomock.Any(), gomock.Any()). + Return(apierrors.NewNotFound(schema.GroupResource{Resource: "Secret"}, "mock-access-keys")). + Times(1) + + obj.Spec.KeyGeneration = ptr.To(1) + obj.Status.LastKeyGeneration = obj.Spec.KeyGeneration + obj.Status.KeySecretName = ptr.To("mock-access-keys") + obj.Status.AccessKeyRefs = []int{0, 1} + + bScope := scope.ObjectStorageBucketScope{ + Client: mockK8sClient, + LinodeClient: mockLinodeClient, + Bucket: &obj, + Logger: reconciler.Logger, + } + + err := reconciler.reconcileApply(ctx, &bScope) + Expect(err.Error()).To(ContainSubstring("key creation error")) + Expect(<-recorder.Events).To(ContainSubstring("key creation error")) + Expect(testLogs.String()).To(ContainSubstring("Failed to restore access keys for deleted secret")) + }) + + It("fails when it can't generate a secret", func(ctx SpecContext) { + mockLinodeClient := mock.NewMockLinodeObjectStorageClient(mockCtrl) + mockLinodeClient.EXPECT(). + GetObjectStorageBucket(gomock.Any(), gomock.Any(), gomock.Any()). + Return(&linodego.ObjectStorageBucket{Created: ptr.To(time.Now())}, nil). + Times(1) + for idx := range 2 { + mockLinodeClient.EXPECT(). + GetObjectStorageKey(gomock.Any(), idx). + Return(&linodego.ObjectStorageKey{ID: idx}, nil). + Times(1) + } + + mockK8sClient := mock.NewMockk8sClient(mockCtrl) + mockK8sClient.EXPECT(). + Get(gomock.Any(), gomock.Any(), gomock.Any()). + Return(apierrors.NewNotFound(schema.GroupResource{Resource: "Secret"}, "mock-access-keys")). + Times(1) + mockK8sClient.EXPECT(). + Scheme(). + Return(runtime.NewScheme()). + Times(1) + + obj.Spec.KeyGeneration = ptr.To(1) + obj.Status.LastKeyGeneration = obj.Spec.KeyGeneration + obj.Status.KeySecretName = ptr.To("mock-access-keys") + obj.Status.AccessKeyRefs = []int{0, 1} + + bScope := scope.ObjectStorageBucketScope{ + Client: mockK8sClient, + LinodeClient: mockLinodeClient, + Bucket: &obj, + Logger: reconciler.Logger, + } + + err := reconciler.reconcileApply(ctx, &bScope) + Expect(err.Error()).To(ContainSubstring("no kind is registered")) + Expect(<-recorder.Events).To(ContainSubstring("keys retrieved")) + Expect(<-recorder.Events).To(ContainSubstring("no kind is registered")) + Expect(testLogs.String()).To(ContainSubstring("Failed to generate key secret")) + }) + + It("fails when it can't restore a deleted secret", func(ctx SpecContext) { + mockLinodeClient := mock.NewMockLinodeObjectStorageClient(mockCtrl) + mockLinodeClient.EXPECT(). + GetObjectStorageBucket(gomock.Any(), gomock.Any(), gomock.Any()). + Return(&linodego.ObjectStorageBucket{Created: ptr.To(time.Now())}, nil). + Times(1) + for idx := range 2 { + mockLinodeClient.EXPECT(). + GetObjectStorageKey(gomock.Any(), idx). + Return(&linodego.ObjectStorageKey{ID: idx}, nil). + Times(1) + } + + mockK8sClient := mock.NewMockk8sClient(mockCtrl) + mockK8sClient.EXPECT(). + Scheme(). + Return(scheme.Scheme). + Times(1) + mockK8sClient.EXPECT(). + Get(gomock.Any(), gomock.Any(), gomock.Any()). + Return(apierrors.NewNotFound(schema.GroupResource{Resource: "Secret"}, "mock-access-keys")). + Times(1) + mockK8sClient.EXPECT(). + Create(gomock.Any(), gomock.Any(), gomock.Any()). + Return(errors.New("secret creation error")) + + obj.Spec.KeyGeneration = ptr.To(1) + obj.Status.LastKeyGeneration = obj.Spec.KeyGeneration + obj.Status.KeySecretName = ptr.To("mock-access-keys") + obj.Status.AccessKeyRefs = []int{0, 1} + + bScope := scope.ObjectStorageBucketScope{ + Client: mockK8sClient, + LinodeClient: mockLinodeClient, + Bucket: &obj, + Logger: reconciler.Logger, + } + + err := reconciler.reconcileApply(ctx, &bScope) + Expect(err.Error()).To(ContainSubstring("secret creation error")) + Expect(<-recorder.Events).To(ContainSubstring("keys retrieved")) + Expect(<-recorder.Events).To(ContainSubstring("secret creation error")) + Expect(testLogs.String()).To(ContainSubstring("Failed to apply key secret")) }) }) diff --git a/e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/05-assert.yaml b/e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/05-assert.yaml new file mode 100644 index 000000000..4693637bb --- /dev/null +++ b/e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/05-assert.yaml @@ -0,0 +1,4 @@ +apiVersion: v1 +kind: Secret +metadata: + name: ($access_keys_secret) diff --git a/e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/05-errors.yaml b/e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/07-errors.yaml similarity index 100% rename from e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/05-errors.yaml rename to e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/07-errors.yaml diff --git a/e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/chainsaw-test.yaml b/e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/chainsaw-test.yaml index 2d065df75..1c6854896 100755 --- a/e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/chainsaw-test.yaml +++ b/e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/chainsaw-test.yaml @@ -49,13 +49,24 @@ spec: - assert: file: 03-assert.yaml - name: step-04 + try: + - delete: + ref: + apiVersion: v1 + kind: Secret + name: (join('-', [($namespace), 'backups-access-keys'])) + - name: step-05 + try: + - assert: + file: 05-assert.yaml + - name: step-06 try: - delete: ref: apiVersion: infrastructure.cluster.x-k8s.io/v1alpha1 kind: LinodeObjectStorageBucket name: ($bucket) - - name: step-05 + - name: step-07 try: - error: - file: 05-errors.yaml + file: 07-errors.yaml diff --git a/mock/client.go b/mock/client.go index b67a6e022..9034945fa 100644 --- a/mock/client.go +++ b/mock/client.go @@ -103,6 +103,21 @@ func (mr *MockLinodeObjectStorageClientMockRecorder) GetObjectStorageBucket(ctx, return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetObjectStorageBucket", reflect.TypeOf((*MockLinodeObjectStorageClient)(nil).GetObjectStorageBucket), ctx, cluster, label) } +// GetObjectStorageKey mocks base method. +func (m *MockLinodeObjectStorageClient) GetObjectStorageKey(ctx context.Context, keyID int) (*linodego.ObjectStorageKey, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetObjectStorageKey", ctx, keyID) + ret0, _ := ret[0].(*linodego.ObjectStorageKey) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetObjectStorageKey indicates an expected call of GetObjectStorageKey. +func (mr *MockLinodeObjectStorageClientMockRecorder) GetObjectStorageKey(ctx, keyID any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetObjectStorageKey", reflect.TypeOf((*MockLinodeObjectStorageClient)(nil).GetObjectStorageKey), ctx, keyID) +} + // Mockk8sClient is a mock of k8sClient interface. type Mockk8sClient struct { ctrl *gomock.Controller