[feat] Disable changing key secret type

linode · Aug 19, 2024 · 40116a6 · 40116a6
1 parent dd76b1f
commit 40116a6
Show file tree

Hide file tree

Showing 6 changed files with 22 additions and 315 deletions.
diff --git a/api/v1alpha2/linodeobjectstoragekey_types.go b/api/v1alpha2/linodeobjectstoragekey_types.go
@@ -52,6 +52,7 @@ type LinodeObjectStorageKeySpec struct {
 	// SecretType instructs the controller what type of secret to generate containing access key details.
 	// +kubebuilder:validation:Enum=Opaque;addons.cluster.x-k8s.io/resource-set
 	// +kubebuilder:default=Opaque
+	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="Value is immutable"
 	// +optional
 	SecretType corev1.SecretType `json:"secretType,omitempty"`
 }

diff --git a/cloud/scope/object_storage_key.go b/cloud/scope/object_storage_key.go
@@ -8,11 +8,9 @@ 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"
 	clusteraddonsv1 "sigs.k8s.io/cluster-api/exp/addons/api/v1beta1"
 	"sigs.k8s.io/cluster-api/util/patch"
-	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	infrav1alpha2 "github.com/linode/cluster-api-provider-linode/api/v1alpha2"
@@ -115,11 +113,6 @@ stringData:
   secret_key: %s`
 )
 
-var secretTypeExpectedKey = map[corev1.SecretType]string{
-	corev1.SecretTypeOpaque:                      "access_key",
-	clusteraddonsv1.ClusterResourceSetSecretType: ClusterResourceSetSecretFilename,
-}
-
 // 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 *ObjectStorageKeyScope) GenerateKeySecret(ctx context.Context, key *linodego.ObjectStorageKey) (*corev1.Secret, error) {
@@ -191,35 +184,3 @@ func (s *ObjectStorageKeyScope) ShouldRotateKey() bool {
 	return s.Key.Status.LastKeyGeneration != nil &&
 		s.Key.Spec.KeyGeneration != *s.Key.Status.LastKeyGeneration
 }
-
-func (s *ObjectStorageKeyScope) ShouldReconcileKeySecret(ctx context.Context) (bool, error) {
-	if s.Key.Status.SecretName == nil {
-		return false, nil
-	}
-
-	secret := &corev1.Secret{}
-	key := client.ObjectKey{Namespace: s.Key.Namespace, Name: *s.Key.Status.SecretName}
-	err := s.Client.Get(ctx, key, secret)
-	if apierrors.IsNotFound(err) {
-		return true, nil
-	}
-	if err != nil {
-		return false, err
-	}
-
-	// Identify an expected key in secret.Data for the desired secret type.
-	// If it is missing, we must recreate the secret since the secret.type field is immutable.
-	expectedKey, ok := secretTypeExpectedKey[s.Key.Spec.SecretType]
-	if !ok {
-		return false, errors.New("unsupported secret type configured in LinodeObjectStorageKey")
-	}
-	if _, ok := secret.Data[expectedKey]; !ok {
-		if err := s.Client.Delete(ctx, secret); err != nil {
-			return false, err
-		}
-
-		return true, nil
-	}
-
-	return false, nil
-}
diff --git a/cloud/scope/object_storage_key_test.go b/cloud/scope/object_storage_key_test.go
@@ -12,10 +12,8 @@ import (
 	"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"
 	clusteraddonsv1 "sigs.k8s.io/cluster-api/exp/addons/api/v1beta1"
@@ -550,198 +548,3 @@ func TestShouldRotateKey(t *testing.T) {
 		},
 	}).ShouldRotateKey())
 }
-
-func TestShouldReconcileKeySecret(t *testing.T) {
-	t.Parallel()
-
-	tests := []struct {
-		name        string
-		key         *infrav1alpha2.LinodeObjectStorageKey
-		expects     func(k8s *mock.MockK8sClient)
-		want        bool
-		expectedErr error
-	}{
-		{
-			name: "status has no secret name",
-			key: &infrav1alpha2.LinodeObjectStorageKey{
-				ObjectMeta: metav1.ObjectMeta{
-					Namespace: "ns",
-				},
-				Status: infrav1alpha2.LinodeObjectStorageKeyStatus{
-					SecretName: nil,
-				},
-			},
-			want: false,
-		},
-		{
-			name: "secret has expected key",
-			key: &infrav1alpha2.LinodeObjectStorageKey{
-				ObjectMeta: metav1.ObjectMeta{
-					Namespace: "ns",
-				},
-				Spec: infrav1alpha2.LinodeObjectStorageKeySpec{
-					SecretType: corev1.SecretTypeOpaque,
-				},
-				Status: infrav1alpha2.LinodeObjectStorageKeyStatus{
-					SecretName: ptr.To("secret"),
-				},
-			},
-			expects: func(k8s *mock.MockK8sClient) {
-				k8s.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).
-					DoAndReturn(func(ctx context.Context, key client.ObjectKey, obj *corev1.Secret, opts ...client.GetOption) error {
-						*obj = corev1.Secret{
-							Data: map[string][]byte{
-								"access_key": {},
-							},
-						}
-						return nil
-					}).AnyTimes()
-			},
-			want: false,
-		},
-		{
-			name: "secret is missing expected key",
-			key: &infrav1alpha2.LinodeObjectStorageKey{
-				ObjectMeta: metav1.ObjectMeta{
-					Namespace: "ns",
-				},
-				Spec: infrav1alpha2.LinodeObjectStorageKeySpec{
-					SecretType: corev1.SecretTypeOpaque,
-				},
-				Status: infrav1alpha2.LinodeObjectStorageKeyStatus{
-					SecretName: ptr.To("secret"),
-				},
-			},
-			expects: func(k8s *mock.MockK8sClient) {
-				k8s.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).
-					DoAndReturn(func(ctx context.Context, key client.ObjectKey, obj *corev1.Secret, opts ...client.GetOption) error {
-						*obj = corev1.Secret{
-							Data: map[string][]byte{
-								"not_access_key": {},
-							},
-						}
-						return nil
-					}).AnyTimes()
-				k8s.EXPECT().Delete(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
-			},
-			want: true,
-		},
-		{
-			name: "secret is missing",
-			key: &infrav1alpha2.LinodeObjectStorageKey{
-				ObjectMeta: metav1.ObjectMeta{
-					Namespace: "ns",
-				},
-				Status: infrav1alpha2.LinodeObjectStorageKeyStatus{
-					SecretName: 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",
-			key: &infrav1alpha2.LinodeObjectStorageKey{
-				ObjectMeta: metav1.ObjectMeta{
-					Namespace: "ns",
-				},
-				Status: infrav1alpha2.LinodeObjectStorageKeyStatus{
-					SecretName: 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,
-			expectedErr: errors.New("unexpected error"),
-		},
-		{
-			name: "unsupported secret type",
-			key: &infrav1alpha2.LinodeObjectStorageKey{
-				ObjectMeta: metav1.ObjectMeta{
-					Namespace: "ns",
-				},
-				Spec: infrav1alpha2.LinodeObjectStorageKeySpec{
-					SecretType: "unsupported secret type",
-				},
-				Status: infrav1alpha2.LinodeObjectStorageKeyStatus{
-					SecretName: ptr.To("secret"),
-				},
-			},
-			expects: func(k8s *mock.MockK8sClient) {
-				k8s.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).
-					DoAndReturn(func(ctx context.Context, key client.ObjectKey, obj *corev1.Secret, opts ...client.GetOption) error {
-						*obj = corev1.Secret{
-							Data: map[string][]byte{
-								"not_access_key": {},
-							},
-						}
-						return nil
-					}).AnyTimes()
-			},
-			want:        false,
-			expectedErr: errors.New("unsupported secret type configured in LinodeObjectStorageKey"),
-		},
-		{
-			name: "failed delete",
-			key: &infrav1alpha2.LinodeObjectStorageKey{
-				ObjectMeta: metav1.ObjectMeta{
-					Namespace: "ns",
-				},
-				Spec: infrav1alpha2.LinodeObjectStorageKeySpec{
-					SecretType: corev1.SecretTypeOpaque,
-				},
-				Status: infrav1alpha2.LinodeObjectStorageKeyStatus{
-					SecretName: ptr.To("secret"),
-				},
-			},
-			expects: func(k8s *mock.MockK8sClient) {
-				k8s.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).
-					DoAndReturn(func(ctx context.Context, key client.ObjectKey, obj *corev1.Secret, opts ...client.GetOption) error {
-						*obj = corev1.Secret{
-							Data: map[string][]byte{
-								"not_access_key": {},
-							},
-						}
-						return nil
-					}).AnyTimes()
-				k8s.EXPECT().Delete(gomock.Any(), gomock.Any(), gomock.Any()).Return(errors.New("failed delete"))
-			},
-			want:        false,
-			expectedErr: errors.New("failed delete"),
-		},
-	}
-	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)
-			}
-
-			keyScope := &ObjectStorageKeyScope{
-				Client: mockClient,
-				Key:    testcase.key,
-			}
-
-			restore, err := keyScope.ShouldReconcileKeySecret(context.TODO())
-			if testcase.expectedErr != nil {
-				require.ErrorContains(t, err, testcase.expectedErr.Error())
-			}
-
-			assert.Equal(t, testcase.want, restore)
-		})
-	}
-}
diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_linodeobjectstoragekeys.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_linodeobjectstoragekeys.yaml
@@ -105,6 +105,9 @@ spec:
                 - Opaque
                 - addons.cluster.x-k8s.io/resource-set
                 type: string
+                x-kubernetes-validations:
+                - message: Value is immutable
+                  rule: self == oldSelf
             required:
             - bucketAccess
             - keyGeneration

diff --git a/controller/linodeobjectstoragekey_controller.go b/controller/linodeobjectstoragekey_controller.go
@@ -173,16 +173,26 @@ func (r *LinodeObjectStorageKeyReconciler) reconcileApply(ctx context.Context, k
 		r.Recorder.Event(keyScope.Key, corev1.EventTypeNormal, "KeyAssigned", "Object storage key assigned")
 
 	// Ensure the generated secret still exists
-	case keyScope.Key.Status.AccessKeyRef != nil:
-		ok, err := keyScope.ShouldReconcileKeySecret(ctx)
-		if err != nil {
-			keyScope.Logger.Error(err, "Failed check for access key secret")
-			r.setFailure(keyScope, err)
+	case keyScope.Key.Status.AccessKeyRef != nil && keyScope.Key.Status.SecretName != nil:
+		secret := &corev1.Secret{}
+		key := client.ObjectKey{
+			Namespace: keyScope.Key.Namespace,
+			Name:      *keyScope.Key.Status.SecretName,
+		}
 
-			return err
+		var shouldRestoreSecret bool
+		if err := keyScope.Client.Get(ctx, key, secret); err != nil {
+			if apierrors.IsNotFound(err) {
+				shouldRestoreSecret = true
+			} else {
+				keyScope.Logger.Error(err, "Failed check for access key secret")
+				r.setFailure(keyScope, fmt.Errorf("failed check for access key secret: %w", err))
+
+				return err
+			}
 		}
 
-		if ok {
+		if shouldRestoreSecret {
 			key, err := services.GetObjectStorageKey(ctx, keyScope)
 			if err != nil {
 				keyScope.Logger.Error(err, "Failed to restore access key for modified/deleted secret")