From d915a23463e1b4a712d45cf3dcfdca9fea6f2e11 Mon Sep 17 00:00:00 2001 From: Brian Mendoza Date: Wed, 13 Mar 2024 17:36:45 -0400 Subject: [PATCH] Ensure OBJ key secrets are re-created on delete --- cloud/scope/client.go | 1 + cloud/scope/object_storage_bucket.go | 32 ++++++- cloud/services/object_storage_buckets.go | 20 +++- .../linodeobjectstoragebucket_controller.go | 45 +++++++-- ...nodeobjectstoragebucket_controller_test.go | 94 ++++++++++++++++--- .../05-assert.yaml | 4 + .../{05-errors.yaml => 07-errors.yaml} | 0 .../chainsaw-test.yaml | 15 ++- mock/client.go | 15 +++ 9 files changed, 201 insertions(+), 25 deletions(-) create mode 100644 e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/05-assert.yaml rename e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/{05-errors.yaml => 07-errors.yaml} (100%) diff --git a/cloud/scope/client.go b/cloud/scope/client.go index 4ac98c377..78d75690e 100644 --- a/cloud/scope/client.go +++ b/cloud/scope/client.go @@ -12,6 +12,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 63c9bb4ed..1c61abb58 100644 --- a/cloud/scope/object_storage_bucket.go +++ b/cloud/scope/object_storage_bucket.go @@ -8,6 +8,7 @@ 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" @@ -100,8 +101,8 @@ 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 { +// ApplyKeySecret applies a Secret containing keys created for accessing the bucket. +func (s *ObjectStorageBucketScope) ApplyKeySecret(ctx context.Context, keys [NumAccessKeys]linodego.ObjectStorageKey, secretName string) error { secret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: secretName, @@ -117,9 +118,13 @@ func (s *ObjectStorageBucketScope) ApplyAccessKeySecret(ctx context.Context, key return 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 := controllerutil.SetControllerReference(s.Bucket, secret, s.client.Scheme()); err != nil { + return fmt.Errorf("could not set controller ref on access key secret %s: %w", secretName, err) + } + + result, err := controllerutil.CreateOrUpdate(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) + return fmt.Errorf("could not create/update access key secret %s: %w", secretName, err) } s.Logger.Info(fmt.Sprintf("Secret %s was %s with new access keys", secret.Name, result)) @@ -127,6 +132,23 @@ func (s *ObjectStorageBucketScope) ApplyAccessKeySecret(ctx context.Context, key 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 { + secret := &corev1.Secret{} + key := client.ObjectKey{Namespace: s.Bucket.Namespace, Name: *s.Bucket.Status.KeySecretName} + if err := s.client.Get(ctx, key, secret); err != nil { + return apierrors.IsNotFound(err), client.IgnoreNotFound(err) + } + } + + return false, nil } diff --git a/cloud/services/object_storage_buckets.go b/cloud/services/object_storage_buckets.go index cd2c327b9..86ecf1464 100644 --- a/cloud/services/object_storage_buckets.go +++ b/cloud/services/object_storage_buckets.go @@ -63,7 +63,7 @@ func RotateObjectStorageKeys(ctx context.Context, bScope *scope.ObjectStorageBuc } // 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") } @@ -121,3 +121,21 @@ 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 + 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/controller/linodeobjectstoragebucket_controller.go b/controller/linodeobjectstoragebucket_controller.go index a781a6d44..83a59cf78 100644 --- a/controller/linodeobjectstoragebucket_controller.go +++ b/controller/linodeobjectstoragebucket_controller.go @@ -23,6 +23,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" @@ -154,18 +155,48 @@ func (r *LinodeObjectStorageBucketReconciler) reconcileApply(ctx context.Context 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 + + 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: + ok, 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 ok { + 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 != nil { secretName := fmt.Sprintf(scope.AccessKeyNameTemplate, bScope.Bucket.Name) - if err := bScope.ApplyAccessKeySecret(ctx, keys, secretName); err != nil { + if err := bScope.ApplyKeySecret(ctx, *keys, secretName); err != nil { bScope.Logger.Error(err, "Failed to apply access key secret") r.setFailure(bScope, err) @@ -173,9 +204,11 @@ func (r *LinodeObjectStorageBucketReconciler) reconcileApply(ctx context.Context } bScope.Bucket.Status.KeySecretName = util.Pointer(secretName) 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, clusterv1.DeletedReason, "Synced", "Object storage bucket synced") bScope.Bucket.Status.Ready = true conditions.MarkTrue(bScope.Bucket, clusterv1.ReadyCondition) @@ -201,7 +234,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 } diff --git a/controller/linodeobjectstoragebucket_controller_test.go b/controller/linodeobjectstoragebucket_controller_test.go index c5b746af7..6a624b2fb 100644 --- a/controller/linodeobjectstoragebucket_controller_test.go +++ b/controller/linodeobjectstoragebucket_controller_test.go @@ -77,7 +77,7 @@ var _ = Describe("LinodeObjectStorageBucket controller", func() { mockCtrl.Finish() }) - It("should reconcile an object apply", func() { + It("should provision a bucket and keys", func() { mockClient := mock.NewMockLinodeObjectStorageClient(mockCtrl) getCall := mockClient.EXPECT(). @@ -109,7 +109,13 @@ var _ = Describe("LinodeObjectStorageBucket controller", func() { return createOpt.Label == fmt.Sprintf("%s-%s", obj.Name, permission) }), ). - Return(&linodego.ObjectStorageKey{ID: idx}, nil). + Return( + &linodego.ObjectStorageKey{ + ID: idx, + AccessKey: fmt.Sprintf("key-%d", idx), + }, + nil, + ). Times(1). After(createBucketCall) } @@ -129,7 +135,7 @@ var _ = Describe("LinodeObjectStorageBucket controller", func() { }) Expect(err).NotTo(HaveOccurred()) - By("updating its status fields") + By("updating the bucket resource's status fields") Expect(k8sClient.Get(ctx, objectKey, obj)).To(Succeed()) Expect(*obj.Status.Hostname).To(Equal("hostname")) Expect(*obj.Status.KeySecretName).To(Equal(secretName)) @@ -137,19 +143,85 @@ var _ = Describe("LinodeObjectStorageBucket controller", func() { Expect(*obj.Status.LastKeyGeneration).To(Equal(0)) Expect(obj.Status.Ready).To(BeTrue()) - By("creating a Secret with access keys") + 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())) + 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("recording the expected event") - Expect(<-recorder.Events).To(ContainSubstring("Object storage bucket applied")) + It("should ensure the bucket's secret exists", func() { + mockClient := mock.NewMockLinodeObjectStorageClient(mockCtrl) + + getCall := mockClient.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 i := range 2 { + mockClient.EXPECT(). + GetObjectStorageKey(gomock.Any(), i). + Return( + &linodego.ObjectStorageKey{ + ID: i, + AccessKey: fmt.Sprintf("key-%d", i), + }, + nil, + ). + Times(1). + After(getCall) + } + + reconciler := &LinodeObjectStorageBucketReconciler{ + Client: k8sClient, + Scheme: k8sClient.Scheme(), + Logger: ctrl.Log.WithName("LinodeObjectStorageBucketReconciler"), + Recorder: recorder, + LinodeClientBuilder: mockClientBuilder(mockClient), + } + + objectKey := client.ObjectKeyFromObject(obj) + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: secretName, + Namespace: obj.Namespace, + }, + } + Expect(k8sClient.Delete(ctx, secret)).To(Succeed()) + _, 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")) }) - It("should reconcile an object delete", func() { + It("should revoke the bucket's keys", func() { mockClient := mock.NewMockLinodeObjectStorageClient(mockCtrl) for i := range 2 { @@ -174,10 +246,10 @@ var _ = Describe("LinodeObjectStorageBucket controller", func() { }) Expect(err).NotTo(HaveOccurred()) - By("removing the finalizer so it is deleted") + 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")) }) }) 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..1792aabf8 --- /dev/null +++ b/e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/05-assert.yaml @@ -0,0 +1,4 @@ +apiVersion: v1 +kind: Secret +metadata: + name: (join('-', [$namespace, 'backups-access-keys'])) 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 0331924ae..b23a7e846 100755 --- a/e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/chainsaw-test.yaml +++ b/e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/chainsaw-test.yaml @@ -39,13 +39,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: (join('-', [($namespace), 'backups'])) - - 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 6895442fe..b2d6c3882 100644 --- a/mock/client.go +++ b/mock/client.go @@ -104,6 +104,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