diff --git a/api/v1alpha2/linodeobjectstoragekey_types.go b/api/v1alpha2/linodeobjectstoragekey_types.go index 83473120..da151a52 100644 --- 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 index 6158852b..eeb59980 100644 --- 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 index 12fedc5b..232e8b84 100644 --- 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 index b7851fcb..a451898e 100644 --- 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 index cca2c848..09fd94ce 100644 --- 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") diff --git a/controller/linodeobjectstoragekey_controller_test.go b/controller/linodeobjectstoragekey_controller_test.go index b4867ea1..4a878ccd 100644 --- a/controller/linodeobjectstoragekey_controller_test.go +++ b/controller/linodeobjectstoragekey_controller_test.go @@ -19,7 +19,6 @@ package controller import ( "context" "errors" - "fmt" "github.com/linode/linodego" "go.uber.org/mock/gomock" @@ -252,78 +251,8 @@ var _ = Describe("lifecycle", Ordered, Label("key", "lifecycle"), func() { ), Once("secretType set to cluster resource set", func(ctx context.Context, _ Mock) { key.Spec.SecretType = clusteraddonsv1.ClusterResourceSetSecretType - Expect(k8sClient.Update(ctx, &key)).To(Succeed()) + Expect(k8sClient.Update(ctx, &key)).NotTo(Succeed()) }), - OneOf( - Path( - Call("(secretType set to cluster resource set) > key is not retrieved", func(ctx context.Context, mck Mock) { - mck.LinodeClient.EXPECT().GetObjectStorageKey(gomock.Any(), 2).Return(nil, errors.New("get key error")) - }), - Result("error", func(ctx context.Context, mck Mock) { - keyScope.LinodeClient = mck.LinodeClient - _, err := reconciler.reconcile(ctx, &keyScope) - Expect(err.Error()).To(ContainSubstring("get key error")) - }), - ), - Path( - Call("(secretType set to cluster resource set) > key is retrieved", func(ctx context.Context, mck Mock) { - mck.LinodeClient.EXPECT().GetObjectStorageKey(gomock.Any(), 2). - Return(&linodego.ObjectStorageKey{ - ID: 2, - AccessKey: "access-key-2", - SecretKey: "secret-key-2", - }, nil) - }), - OneOf( - Path( - Call("bucket is not retrieved", func(ctx context.Context, mck Mock) { - mck.LinodeClient.EXPECT().GetObjectStorageBucket(gomock.Any(), "us-ord", "mybucket").Return(nil, errors.New("get bucket error")) - }), - Result("error", func(ctx context.Context, mck Mock) { - keyScope.LinodeClient = mck.LinodeClient - _, err := reconciler.reconcile(ctx, &keyScope) - Expect(err.Error()).To(ContainSubstring("get bucket error")) - }), - ), - Path( - Call("bucket is retrieved", func(ctx context.Context, mck Mock) { - mck.LinodeClient.EXPECT().GetObjectStorageBucket(gomock.Any(), "us-ord", "mybucket").Return(&linodego.ObjectStorageBucket{ - Label: "mybucket", - Region: "us-ord", - Hostname: "mybucket.us-ord-1.linodeobjects.com", - }, nil) - }), - Result("secret is recreated as cluster resource set type", func(ctx context.Context, mck Mock) { - keyScope.LinodeClient = mck.LinodeClient - _, err := reconciler.reconcile(ctx, &keyScope) - Expect(err).NotTo(HaveOccurred()) - - var secret corev1.Secret - secretKey := client.ObjectKey{Namespace: "default", Name: *key.Status.SecretName} - Expect(k8sClient.Get(ctx, secretKey, &secret)).To(Succeed()) - Expect(secret.Data).To(HaveLen(1)) - Expect(string(secret.Data[scope.ClusterResourceSetSecretFilename])).To(Equal(fmt.Sprintf(scope.BucketKeySecret, - *key.Status.SecretName, - "mybucket", - "us-ord", - "mybucket.us-ord-1.linodeobjects.com", - "access-key-2", - "secret-key-2", - ))) - - events := mck.Events() - Expect(events).To(ContainSubstring("Object storage key retrieved")) - Expect(events).To(ContainSubstring("Object storage key stored in secret")) - Expect(events).To(ContainSubstring("Object storage key synced")) - - logOutput := mck.Logs() - Expect(logOutput).To(ContainSubstring("Reconciling apply")) - Expect(logOutput).To(ContainSubstring("Secret %s was created with access key", *key.Status.SecretName)) - }), - ), - ), - ), - ), Once("resource is deleted", func(ctx context.Context, _ Mock) { // nb: client.Delete does not set DeletionTimestamp on the object, so re-fetch from the apiserver. objectKey := client.ObjectKeyFromObject(&key)