Merge pull request #400 from syntasso/fix/399/handle-empty-namespace-…

…in-state-store-secrets

fix: handle empty namespace in state store secrets

internal/controller/bucketstatestore_controller.go

@@ -75,7 +75,6 @@ func (r *BucketStateStoreReconciler) Reconcile(ctx context.Context, req ctrl.Req
 	return reconcileStateStoreCommon(
 		o,
 		bucketStateStore,
-		bucketStateStore.Spec.SecretRef,
 		"BucketStateStore",
 		r.updateReadyStatusAndCondition,
 	)

internal/controller/bucketstatestore_controller_test.go

@@ -164,36 +164,6 @@ var _ = Describe("BucketStateStore Controller", func() {
 			})
 		})
 
-		When("the referenced secret does not exist", func() {
-			BeforeEach(func() {
-				Expect(fakeK8sClient.Delete(ctx, secret)).To(Succeed())
-
-				bucketStateStore.Status.Status = controller.StatusReady
-				Expect(fakeK8sClient.Status().Update(ctx, bucketStateStore)).To(Succeed())
-
-				result, err = t.reconcileUntilCompletion(reconciler, bucketStateStore)
-			})
-
-			It("updates the status to say the the secret cannot be found", func() {
-				Expect(err).To(MatchError(ContainSubstring("secrets \"store-secret\" not found")))
-				Expect(result).To(Equal(ctrl.Result{}))
-
-				Expect(fakeK8sClient.Get(ctx, testBucketStateStoreName, updatedBucketStateStore)).To(Succeed())
-				Expect(updatedBucketStateStore.Status.Status).To(Equal(controller.StatusNotReady))
-				Expect(updatedBucketStateStore.Status.Conditions).To(ContainElement(SatisfyAll(
-					HaveField("Type", "Ready"),
-					HaveField("Message", "Secret not found: secrets \"store-secret\" not found"),
-					HaveField("Reason", "SecretNotFound"),
-					HaveField("Status", metav1.ConditionFalse),
-				)))
-			})
-
-			It("fires an event to indicate the secret cannot be found", func() {
-				Eventually(eventRecorder.Events).Should(Receive(ContainSubstring(
-					"Warning NotReady BucketStateStore \"default-store\" is not ready: Secret not found: secrets \"store-secret\" not found")))
-			})
-		})
-
 		When("the writer fails to initialise", func() {
 			BeforeEach(func() {
 				bucketStateStore.Status.Status = controller.StatusReady

internal/controller/destination_controller_test.go

@@ -196,7 +196,7 @@ var _ = Describe("DestinationReconciler", func() {
 					})
 
 					It("fails the reconciliation", func() {
-						Expect(reconcileErr).To(MatchError(ContainSubstring("secrets %q not found", stateStoreSecret.GetName())))
+						Expect(reconcileErr).To(MatchError(ContainSubstring("secret %q not found in namespace %q", stateStoreSecret.GetName(), stateStoreSecret.GetNamespace())))
 						Expect(result).To(Equal(ctrl.Result{}))
 					})
 
@@ -211,8 +211,8 @@ var _ = Describe("DestinationReconciler", func() {
 
 					It("publishes a failure event", func() {
 						Expect(eventRecorder.Events).To(Receive(ContainSubstring(
-							"Failed to write test documents to Destination %q: secrets %q not found", testDestination.Name, stateStoreSecret.GetName(),
-						)))
+							"Failed to write test documents to Destination %q: secret %q not found in namespace %q", testDestination.Name, stateStoreSecret.GetName(), stateStoreSecret.GetNamespace()),
+						))
 					})
 				})
 

internal/controller/gitstatestore_controller.go

@@ -76,7 +76,6 @@ func (r *GitStateStoreReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 	return reconcileStateStoreCommon(
 		o,
 		gitStateStore,
-		gitStateStore.Spec.SecretRef,
 		"GitStateStore",
 		r.updateReadyStatusAndCondition,
 	)

internal/controller/gitstatestore_controller_test.go

@@ -163,36 +163,6 @@ var _ = Describe("GitStateStore Controller", func() {
 			})
 		})
 
-		When("the referenced secret does not exist", func() {
-			BeforeEach(func() {
-				Expect(fakeK8sClient.Delete(ctx, secret)).To(Succeed())
-
-				gitStateStore.Status.Status = controller.StatusReady
-				Expect(fakeK8sClient.Status().Update(ctx, gitStateStore)).To(Succeed())
-
-				result, err = t.reconcileUntilCompletion(reconciler, gitStateStore)
-			})
-
-			It("updates the status to say the the secret cannot be found", func() {
-				Expect(err).To(MatchError(ContainSubstring("secrets \"store-secret\" not found")))
-				Expect(result).To(Equal(ctrl.Result{}))
-
-				Expect(fakeK8sClient.Get(ctx, testGitStateStoreName, updatedGitStateStore)).To(Succeed())
-				Expect(updatedGitStateStore.Status.Status).To(Equal(controller.StatusNotReady))
-				Expect(updatedGitStateStore.Status.Conditions).To(ContainElement(SatisfyAll(
-					HaveField("Type", "Ready"),
-					HaveField("Message", "Secret not found: secrets \"store-secret\" not found"),
-					HaveField("Reason", "SecretNotFound"),
-					HaveField("Status", metav1.ConditionFalse),
-				)))
-			})
-
-			It("fires an event to indicate the secret cannot be found", func() {
-				Eventually(eventRecorder.Events).Should(Receive(ContainSubstring(
-					"Warning NotReady GitStateStore \"default-store\" is not ready: Secret not found: secrets \"store-secret\" not found")))
-			})
-		})
-
 		When("the writer fails to initialise", func() {
 			BeforeEach(func() {
 				gitStateStore.Status.Status = controller.StatusReady

internal/controller/shared.go

@@ -11,7 +11,6 @@ import (
 	"github.com/syntasso/kratix/api/v1alpha1"
 	"github.com/syntasso/kratix/lib/resourceutil"
 	"github.com/syntasso/kratix/lib/writers"
-	corev1 "k8s.io/api/core/v1"
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -35,8 +34,6 @@ const (
 	StateStoreReadyConditionType                     = "Ready"
 	StateStoreReadyConditionReason                   = "StateStoreReady"
 	StateStoreReadyConditionMessage                  = "State store is ready"
-	StateStoreNotReadySecretNotFoundReason           = "SecretNotFound"
-	StateStoreNotReadySecretNotFoundMessage          = "Secret not found"
 	StateStoreNotReadyErrorInitialisingWriterReason  = "ErrorInitialisingWriter"
 	StateStoreNotReadyErrorInitialisingWriterMessage = "Error initialising writer"
 	StateStoreNotReadyErrorWritingTestFileReason     = "ErrorWritingTestFile"
@@ -119,7 +116,7 @@ func fetchObjectAndSecret(o opts, stateStoreRef client.ObjectKey, stateStore Sta
 
 	namespace := stateStore.GetSecretRef().Namespace
 	if namespace == "" {
-		namespace = v1alpha1.SystemNamespace
+		namespace = "default"
 	}
 
 	secret := &v1.Secret{}
@@ -130,6 +127,9 @@ func fetchObjectAndSecret(o opts, stateStoreRef client.ObjectKey, stateStore Sta
 
 	if err := o.client.Get(o.ctx, secretRef, secret); err != nil {
 		o.logger.Error(err, "unable to fetch resource", "resourceKind", stateStore.GetObjectKind(), "secretRef", secretRef)
+		if errors.IsNotFound(err) {
+			err = fmt.Errorf("secret %q not found in namespace %q", secretRef.Name, namespace)
+		}
 		return nil, err
 	}
 
@@ -199,22 +199,9 @@ func secretRefIndexKey(secretName, secretNamespace string) string {
 func reconcileStateStoreCommon[T metav1.Object](
 	o opts,
 	stateStore T,
-	secretRef *corev1.SecretReference,
 	resourceType string,
 	updateStatus func(stateStore T, failureReason string, failureMessage string, err error) error,
 ) (ctrl.Result, error) {
-	secret := &corev1.Secret{}
-	objectKey := types.NamespacedName{
-		Name:      secretRef.Name,
-		Namespace: secretRef.Namespace,
-	}
-	if err := o.client.Get(o.ctx, objectKey, secret); err != nil {
-		if err := updateStatus(stateStore, StateStoreNotReadySecretNotFoundReason, StateStoreNotReadySecretNotFoundMessage, err); err != nil {
-			o.logger.Error(err, "error updating state store status")
-		}
-		return ctrl.Result{}, err
-	}
-
 	writer, err := newWriter(o, stateStore.GetName(), resourceType, "")
 	if err != nil {
 		if err := updateStatus(stateStore, StateStoreNotReadyErrorInitialisingWriterReason, StateStoreNotReadyErrorInitialisingWriterMessage, err); err != nil {

test/system/destination_test.go

@@ -41,6 +41,24 @@ var _ = Describe("Destinations", func() {
 		})
 
 		It("properly set the conditions and events", func() {
+			// The test uses the destination-test-store BucketStateStore for testing most
+			// of the logic, but we still need to check that the GitStateStore becomes
+			// ready in case there are git-specific issues with checking readiness.
+			By("showing `Ready` as true in the default GitStateStore", func() {
+				Eventually(func() string {
+					return platform.Kubectl("get", "gitstatestores", "default")
+				}).Should(ContainSubstring("True"))
+			})
+
+			By("firing the success event to the default GitStateStore", func() {
+				Eventually(func() string {
+					describeOutput := strings.Split(platform.Kubectl("describe", "gitstatestores", "default"), "\n")
+					return describeOutput[len(describeOutput)-2]
+				}).Should(ContainSubstring("GitStateStore \"default\" is ready"))
+			})
+
+			// The reconciliation logic is common to both types of state stores, so we
+			// can just test the BucketStateStore from now on.
 			By("showing `Ready` as true in the State Store", func() {
 				Eventually(func() string {
 					return platform.Kubectl("get", "bucketstatestores", "destination-test-store")
@@ -111,7 +129,7 @@ var _ = Describe("Destinations", func() {
 				Eventually(func() string {
 					describeOutput := strings.Split(platform.Kubectl("describe", "bucketstatestores", "destination-test-store"), "\n")
 					return describeOutput[len(describeOutput)-2]
-				}).Should(ContainSubstring("Secret not found"))
+				}).Should(ContainSubstring("Error initialising writer: secret \"non-existent-secret\" not found in namespace \"default\""))
 			})
 
 			By("showing `Ready` as False in the Destination when the State Store secret does not exist", func() {
@@ -128,7 +146,11 @@ var _ = Describe("Destinations", func() {
 			})
 
 			// restore the ready condition
-			platform.Kubectl("apply", "-f", "assets/destination/destination-test-store.yaml")
+			if os.Getenv("LRE") == "true" {
+				platform.Kubectl("patch", "bucketstatestore", "destination-test-store", "--type=merge", "-p", `{"spec":{"secretRef":{"name":"aws-s3-credentials"}}}`)
+			} else {
+				platform.Kubectl("apply", "-f", "assets/destination/destination-test-store.yaml")
+			}
 
 			By("showing `Ready` as true in the State Store", func() {
 				Eventually(func() string {