[fix] Remove OBJ label field and add key ID status fields (#188)

Author: bcm820

api/v1alpha1/linodeobjectstoragebucket_types.go

@@ -34,14 +34,6 @@ type LinodeObjectStorageBucketSpec struct {
 	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="Value is immutable"
 	Cluster string `json:"cluster"`
 
-	// Label specifies the name of the Object Storage Bucket.
-	// If not supplied then the name of the LinodeObjectStorageBucket resource will be used.
-	// +kubebuilder:validation:MinLength=3
-	// +kubebuilder:validation:MaxLength=63
-	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="Value is immutable"
-	// +optional
-	Label *string `json:"label,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
@@ -88,6 +80,10 @@ type LinodeObjectStorageBucketStatus struct {
 	// 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

api/v1alpha1/zz_generated.deepcopy.go

@@ -2,7 +2,6 @@ package scope
 
 import (
 	"context"
-	"encoding/json"
 	"errors"
 	"fmt"
 
@@ -103,86 +102,31 @@ func (s *ObjectStorageBucketScope) AddFinalizer(ctx context.Context) error {
 
 // 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 {
-	var err error
-
-	accessKeys := make([]json.RawMessage, NumAccessKeys)
-	for i, key := range keys {
-		accessKeys[i], err = json.Marshal(key)
-		if err != nil {
-			return fmt.Errorf("could not unmarshal access key %s: %w", key.Label, err)
-		}
-	}
-
 	secret := &corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      secretName,
 			Namespace: s.Bucket.Namespace,
 		},
 		StringData: map[string]string{
-			"read_write": string(accessKeys[0]),
-			"read_only":  string(accessKeys[1]),
+			"read_write": keys[0].AccessKey,
+			"read_only":  keys[1].AccessKey,
 		},
 	}
 
 	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)
 	}
 
-	// Add finalizer to secret so it isn't deleted when bucket deletion is triggered
-	controllerutil.AddFinalizer(secret, infrav1alpha1.GroupVersion.String())
-
-	if s.Bucket.Status.KeySecretName == nil {
-		if err := s.client.Create(ctx, secret); err != nil {
-			return fmt.Errorf("could not create access key secret %s: %w", secretName, err)
-		}
-
-		return nil
+	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 := s.client.Update(ctx, secret); err != nil {
-		return fmt.Errorf("could not 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) GetAccessKeySecret(ctx context.Context) (*corev1.Secret, error) {
-	secretName := fmt.Sprintf(AccessKeyNameTemplate, *s.Bucket.Spec.Label)
-
-	objKey := client.ObjectKey{
-		Namespace: s.Bucket.Namespace,
-		Name:      secretName,
-	}
-	var secret corev1.Secret
-	if err := s.client.Get(ctx, objKey, &secret); err != nil {
-		return nil, err
-	}
-
-	return &secret, nil
-}
-
-// GetAccessKeysFromSecret gets the access key IDs for the OBJ buckets from a Secret.
-func (s *ObjectStorageBucketScope) GetAccessKeysFromSecret(ctx context.Context, secret *corev1.Secret) ([NumAccessKeys]int, error) {
-	var keyIDs [NumAccessKeys]int
-
-	permissions := [NumAccessKeys]string{"read_write", "read_only"}
-	for idx, permission := range permissions {
-		secretDataForKey, ok := secret.Data[permission]
-		if !ok {
-			return keyIDs, fmt.Errorf("secret %s missing data field %s", secret.Name, permission)
-		}
-
-		var key linodego.ObjectStorageKey
-		if err := json.Unmarshal(secretDataForKey, &key); err != nil {
-			return keyIDs, fmt.Errorf("error unmarshaling key: %w", err)
-		}
-
-		keyIDs[idx] = key.ID
-	}
-
-	return keyIDs, nil
-}
-
 func (s *ObjectStorageBucketScope) ShouldRotateKeys() bool {
 	return *s.Bucket.Spec.KeyGeneration != *s.Bucket.Status.LastKeyGeneration
 }

cloud/scope/object_storage_bucket.go

@@ -7,7 +7,6 @@ import (
 	"net/http"
 
 	"github.com/linode/linodego"
-	corev1 "k8s.io/api/core/v1"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 
 	"github.com/linode/cluster-api-provider-linode/cloud/scope"
@@ -17,7 +16,7 @@ func EnsureObjectStorageBucket(ctx context.Context, bScope *scope.ObjectStorageB
 	bucket, err := bScope.LinodeClient.GetObjectStorageBucket(
 		ctx,
 		bScope.Bucket.Spec.Cluster,
-		*bScope.Bucket.Spec.Label,
+		bScope.Bucket.Name,
 	)
 	linodeErr := &linodego.Error{}
 	if errors.As(err, linodeErr) && linodeErr.StatusCode() != http.StatusNotFound {
@@ -31,7 +30,7 @@ func EnsureObjectStorageBucket(ctx context.Context, bScope *scope.ObjectStorageB
 
 	opts := linodego.ObjectStorageBucketCreateOptions{
 		Cluster: bScope.Bucket.Spec.Cluster,
-		Label:   *bScope.Bucket.Spec.Label,
+		Label:   bScope.Bucket.Name,
 		ACL:     linodego.ACLPrivate,
 	}
 
@@ -54,7 +53,7 @@ func RotateObjectStorageKeys(ctx context.Context, bScope *scope.ObjectStorageBuc
 		{"read_write", "rw"},
 		{"read_only", "ro"},
 	} {
-		keyLabel := fmt.Sprintf("%s-%s", *bScope.Bucket.Spec.Label, permission.suffix)
+		keyLabel := fmt.Sprintf("%s-%s", bScope.Bucket.Name, permission.suffix)
 		key, err := createObjectStorageKey(ctx, bScope, keyLabel, permission.name)
 		if err != nil {
 			return newKeys, err
@@ -65,12 +64,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() {
-		secret, err := bScope.GetAccessKeySecret(ctx)
-		if err != nil {
-			bScope.Logger.Error(err, "Failed to read secret with access keys to revoke; keys must be manually revoked")
-		}
-
-		if err := RevokeObjectStorageKeys(ctx, bScope, secret); err != nil {
+		if err := RevokeObjectStorageKeys(ctx, bScope); err != nil {
 			bScope.Logger.Error(err, "Failed to revoke access keys; keys must be manually revoked")
 		}
 	}
@@ -83,7 +77,7 @@ func createObjectStorageKey(ctx context.Context, bScope *scope.ObjectStorageBuck
 		Label: label,
 		BucketAccess: &[]linodego.ObjectStorageKeyBucketAccess{
 			{
-				BucketName:  *bScope.Bucket.Spec.Label,
+				BucketName:  bScope.Bucket.Name,
 				Cluster:     bScope.Bucket.Spec.Cluster,
 				Permissions: permission,
 			},
@@ -102,20 +96,9 @@ func createObjectStorageKey(ctx context.Context, bScope *scope.ObjectStorageBuck
 	return key, nil
 }
 
-func RevokeObjectStorageKeys(ctx context.Context, bScope *scope.ObjectStorageBucketScope, secret *corev1.Secret) error {
-	if secret == nil {
-		return errors.New("unable to read access keys from nil secret")
-	}
-
-	keyIDs, err := bScope.GetAccessKeysFromSecret(ctx, secret)
-	if err != nil {
-		bScope.Logger.Error(err, "Failed to read secret with access keys to revoke; must be manually revoked")
-
-		return fmt.Errorf("failed to read secret %s with access keys: %w", secret.Name, err)
-	}
-
+func RevokeObjectStorageKeys(ctx context.Context, bScope *scope.ObjectStorageBucketScope) error {
 	var errs []error
-	for _, keyID := range keyIDs {
+	for _, keyID := range bScope.Bucket.Status.AccessKeyRefs {
 		if err := revokeObjectStorageKey(ctx, bScope, keyID); err != nil {
 			errs = append(errs, err)
 		}

cloud/services/object_storage_buckets.go

@@ -83,23 +83,19 @@ spec:
                 description: KeyGeneration may be modified to trigger rotations of
                   access keys created for the bucket.
                 type: integer
-              label:
-                description: |-
-                  Label specifies the name of the Object Storage Bucket.
-                  If not supplied then the name of the LinodeObjectStorageBucket resource will be used.
-                maxLength: 63
-                minLength: 3
-                type: string
-                x-kubernetes-validations:
-                - message: Value is immutable
-                  rule: self == oldSelf
             required:
             - cluster
             type: object
           status:
             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:

config/crd/bases/infrastructure.cluster.x-k8s.io_linodeobjectstoragebuckets.yaml

@@ -10,15 +10,5 @@ metadata:
     app.kubernetes.io/created-by: cluster-api-provider-linode
   name: linodeobjectstoragebucket-sample
 spec:
-  credentialsRef:
-    name: api-key-secret
-    namespace: default
   keyGeneration: 0
   cluster: us-ord-1
----
-apiVersion: v1
-kind: Secret
-metadata:
-  name: api-key-secret
-stringData:
-  apiToken: "changeme"

config/samples/infrastructure_v1alpha1_linodeobjectstoragebucket.yaml

@@ -18,6 +18,7 @@ package controller
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"time"
 
@@ -143,10 +144,6 @@ func (r *LinodeObjectStorageBucketReconciler) reconcileApply(ctx context.Context
 		return err
 	}
 
-	if bScope.Bucket.Spec.Label == nil {
-		bScope.Bucket.Spec.Label = util.Pointer(bScope.Bucket.Name)
-	}
-
 	bucket, err := services.EnsureObjectStorageBucket(ctx, bScope)
 	if err != nil {
 		bScope.Logger.Error(err, "Failed to ensure bucket exists")
@@ -165,8 +162,9 @@ func (r *LinodeObjectStorageBucketReconciler) reconcileApply(ctx context.Context
 
 			return err
 		}
+		bScope.Bucket.Status.AccessKeyRefs = []int{keys[0].ID, keys[1].ID}
 
-		secretName := fmt.Sprintf(scope.AccessKeyNameTemplate, *bScope.Bucket.Spec.Label)
+		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")
 			r.setFailure(bScope, err)
@@ -188,38 +186,16 @@ func (r *LinodeObjectStorageBucketReconciler) reconcileApply(ctx context.Context
 func (r *LinodeObjectStorageBucketReconciler) reconcileDelete(ctx context.Context, bScope *scope.ObjectStorageBucketScope) error {
 	bScope.Logger.Info("Reconciling delete")
 
-	secret, err := bScope.GetAccessKeySecret(ctx)
-	if err != nil {
-		bScope.Logger.Error(err, "Failed to read secret with access keys to revoke")
-		r.setFailure(bScope, err)
-
-		return err
-	}
-
-	if err := services.RevokeObjectStorageKeys(ctx, bScope, secret); err != nil {
-		bScope.Logger.Error(err, "Failed to revoke access keys; keys must be manually revoked")
-		r.setFailure(bScope, err)
-
-		return err
-	}
-
-	// Only permit Secret and LinodeObjectStorageBucket deletion if keys were revoked
-	if !controllerutil.RemoveFinalizer(secret, infrav1alpha1.GroupVersion.String()) {
-		bScope.Logger.Error(err, "Failed to remove finalizer from secret; will not be deleted")
-		r.setFailure(bScope, err)
-
-		return err
-	}
-
-	if err := r.Client.Update(ctx, secret); err != nil {
-		bScope.Logger.Error(err, "Failed to remove finalizer from secret; will not be deleted")
+	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, infrav1alpha1.GroupVersion.String()) {
-		bScope.Logger.Error(err, "Failed to remove finalizer from bucket; will not be deleted")
+		err := errors.New("failed to remove finalizer from bucket; unable to delete")
+		bScope.Logger.Error(err, "controllerutil.RemoveFinalizer")
 		r.setFailure(bScope, err)
 
 		return err

controller/linodeobjectstoragebucket_controller.go

@@ -56,13 +56,12 @@ var _ = Describe("LinodeObjectStorageBucket controller", func() {
 		},
 		Spec: infrav1.LinodeObjectStorageBucketSpec{
 			Cluster: "cluster",
-			Label:   util.Pointer("sample"),
 		},
 	}
 
 	recorder := record.NewFakeRecorder(3)
 
-	secretName := fmt.Sprintf(scope.AccessKeyNameTemplate, *obj.Spec.Label)
+	secretName := fmt.Sprintf(scope.AccessKeyNameTemplate, obj.Name)
 
 	var secret corev1.Secret
 	var mockCtrl *gomock.Controller
@@ -89,7 +88,7 @@ var _ = Describe("LinodeObjectStorageBucket controller", func() {
 		createBucketCall := mockClient.EXPECT().
 			CreateObjectStorageBucket(gomock.Any(), gomock.Any()).
 			Return(&linodego.ObjectStorageBucket{
-				Label:    *obj.Spec.Label,
+				Label:    obj.Name,
 				Cluster:  obj.Spec.Cluster,
 				Created:  util.Pointer(time.Now()),
 				Hostname: "hostname",
@@ -107,7 +106,7 @@ var _ = Describe("LinodeObjectStorageBucket controller", func() {
 							return false
 						}
 
-						return createOpt.Label == fmt.Sprintf("%s-%s", *obj.Spec.Label, permission)
+						return createOpt.Label == fmt.Sprintf("%s-%s", obj.Name, permission)
 					}),
 				).
 				Return(&linodego.ObjectStorageKey{ID: idx}, nil).