Ensure OBJ key secrets are re-created on delete

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
 }

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,16 +118,37 @@ 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))
 
 	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
 }

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)
+}

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,28 +155,60 @@ 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)
 
 			return err
 		}
 		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
 }

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,27 +135,93 @@ 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))
 		Expect(*obj.Status.LastKeyGeneration).To(Equal(*obj.Spec.KeyGeneration))
 		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"))
 	})
 })

e2e/linodeobjectstoragebucket-controller/minimal-linodeobjectstoragebucket/05-assert.yaml

@@ -0,0 +1,4 @@
+apiVersion: v1
+kind: Secret
+metadata:
+  name: (join('-', [$namespace, 'backups-access-keys']))

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