Prevent resources getting stuck on transient err

This commit ensures that resources will only return early if they are
already in a `Ready==True` state. If not, but the status object somehow
still reports that it has an artifact, the reconciliation will continue
to ensure and/or guarantee state, and to prevent a deadlock from
happening.

api/v1alpha1/condition_types.go

@@ -76,3 +76,13 @@ const (
 	// is underway.
 	ProgressingReason string = "Progressing"
 )
+
+// InReadyCondition returns if the given SourceCondition slice has a ReadyCondition with
+// a 'True' status.
+func InReadyCondition(conditions []SourceCondition) bool {
+	condition := getCondition(conditions, ReadyCondition)
+	if condition == nil {
+		return false
+	}
+	return condition.Status == corev1.ConditionTrue
+}

api/v1alpha1/source.go

@@ -26,3 +26,15 @@ func filterOutSourceCondition(conditions []SourceCondition, condition string) []
 	}
 	return newConditions
 }
+
+// getCondition returns the SourceCondition from the given slice that matches
+// the given condition.
+func getCondition(conditions []SourceCondition, condition string) *SourceCondition {
+	for i := range conditions {
+		c := conditions[i]
+		if c.Type == condition {
+			return &c
+		}
+	}
+	return nil
+}

controllers/bucket_controller.go

@@ -211,7 +211,7 @@ func (r *BucketReconciler) reconcile(ctx context.Context, bucket sourcev1.Bucket
 
 	// return early on unchanged revision
 	artifact := r.Storage.NewArtifactFor(bucket.Kind, bucket.GetObjectMeta(), revision, fmt.Sprintf("%s.tar.gz", revision))
-	if bucket.GetArtifact().HasRevision(artifact.Revision) {
+	if sourcev1.InReadyCondition(bucket.Status.Conditions) && bucket.GetArtifact().HasRevision(artifact.Revision) {
 		if artifact.URL != bucket.GetArtifact().URL {
 			r.Storage.SetArtifactURL(bucket.GetArtifact())
 			bucket.Status.URL = r.Storage.SetHostname(bucket.Status.URL)
@@ -317,7 +317,8 @@ func (r *BucketReconciler) checksum(root string) (string, error) {
 // resetStatus returns a modified v1alpha1.Bucket and a boolean indicating
 // if the status field has been reset.
 func (r *BucketReconciler) resetStatus(bucket sourcev1.Bucket) (sourcev1.Bucket, bool) {
-	if bucket.GetArtifact() != nil && !r.Storage.ArtifactExist(*bucket.GetArtifact()) {
+	// We do not have an artifact, or it does no longer exist
+	if bucket.GetArtifact() == nil || !r.Storage.ArtifactExist(*bucket.GetArtifact()) {
 		bucket = sourcev1.BucketProgressing(bucket)
 		bucket.Status.Artifact = nil
 		return bucket, true

controllers/gitrepository_controller.go

@@ -198,7 +198,7 @@ func (r *GitRepositoryReconciler) reconcile(ctx context.Context, repository sour
 
 	// return early on unchanged revision
 	artifact := r.Storage.NewArtifactFor(repository.Kind, repository.GetObjectMeta(), revision, fmt.Sprintf("%s.tar.gz", commit.Hash.String()))
-	if repository.GetArtifact().HasRevision(artifact.Revision) {
+	if sourcev1.InReadyCondition(repository.Status.Conditions) && repository.GetArtifact().HasRevision(artifact.Revision) {
 		if artifact.URL != repository.GetArtifact().URL {
 			r.Storage.SetArtifactURL(repository.GetArtifact())
 			repository.Status.URL = r.Storage.SetHostname(repository.Status.URL)
@@ -276,7 +276,8 @@ func (r *GitRepositoryReconciler) verify(ctx context.Context, publicKeySecret ty
 // resetStatus returns a modified v1alpha1.GitRepository and a boolean indicating
 // if the status field has been reset.
 func (r *GitRepositoryReconciler) resetStatus(repository sourcev1.GitRepository) (sourcev1.GitRepository, bool) {
-	if repository.GetArtifact() != nil && !r.Storage.ArtifactExist(*repository.GetArtifact()) {
+	// We do not have an artifact, or it does no longer exist
+	if repository.GetArtifact() == nil || !r.Storage.ArtifactExist(*repository.GetArtifact()) {
 		repository = sourcev1.GitRepositoryProgressing(repository)
 		repository.Status.Artifact = nil
 		return repository, true

controllers/helmchart_controller.go

@@ -240,7 +240,7 @@ func (r *HelmChartReconciler) reconcileFromHelmRepository(ctx context.Context,
 	// Return early if the revision is still the same as the current artifact
 	newArtifact := r.Storage.NewArtifactFor(chart.Kind, chart.GetObjectMeta(), cv.Version,
 		fmt.Sprintf("%s-%s.tgz", cv.Name, cv.Version))
-	if !force && repository.GetArtifact().HasRevision(newArtifact.Revision) {
+	if !force && sourcev1.InReadyCondition(chart.Status.Conditions) && chart.GetArtifact().HasRevision(newArtifact.Revision) {
 		if newArtifact.URL != repository.GetArtifact().URL {
 			r.Storage.SetArtifactURL(chart.GetArtifact())
 			chart.Status.URL = r.Storage.SetHostname(chart.Status.URL)
@@ -425,9 +425,9 @@ func (r *HelmChartReconciler) reconcileFromTarballArtifact(ctx context.Context,
 	// Return early if the revision is still the same as the current chart artifact
 	newArtifact := r.Storage.NewArtifactFor(chart.Kind, chart.ObjectMeta.GetObjectMeta(), chartMetadata.Version,
 		fmt.Sprintf("%s-%s.tgz", chartMetadata.Name, chartMetadata.Version))
-	if !force && chart.GetArtifact().HasRevision(newArtifact.Revision) {
+	if !force && sourcev1.InReadyCondition(chart.Status.Conditions) && chart.GetArtifact().HasRevision(newArtifact.Revision) {
 		if newArtifact.URL != artifact.URL {
-			r.Storage.SetArtifactURL(&newArtifact)
+			r.Storage.SetArtifactURL(chart.GetArtifact())
 			chart.Status.URL = r.Storage.SetHostname(chart.Status.URL)
 		}
 		return chart, nil
@@ -483,8 +483,8 @@ func (r *HelmChartReconciler) reconcileFromTarballArtifact(ctx context.Context,
 // resetStatus returns a modified v1alpha1.HelmChart and a boolean indicating
 // if the status field has been reset.
 func (r *HelmChartReconciler) resetStatus(chart sourcev1.HelmChart) (sourcev1.HelmChart, bool) {
-	// The artifact does no longer exist
+	// We do not have an artifact, or it does no longer exist
-	if chart.GetArtifact() != nil && !r.Storage.ArtifactExist(*chart.GetArtifact()) {
+	if chart.GetArtifact() == nil || !r.Storage.ArtifactExist(*chart.GetArtifact()) {
 		chart = sourcev1.HelmChartProgressing(chart)
 		chart.Status.Artifact = nil
 		return chart, true

controllers/helmchart_controller_test.go

@@ -279,7 +279,7 @@ var _ = Describe("HelmChartReconciler", func() {
 			})
 			helmServer.Start()
 
-			Expect(helmServer.PackageChart(path.Join("testdata/helmchart"))).Should(Succeed())
+			Expect(helmServer.PackageChartWithVersion(path.Join("testdata/helmchart"), "0.1.0")).Should(Succeed())
 			Expect(helmServer.GenerateIndex()).Should(Succeed())
 
 			secretKey := types.NamespacedName{
@@ -298,6 +298,7 @@ var _ = Describe("HelmChartReconciler", func() {
 			}
 			Expect(k8sClient.Create(context.Background(), secret)).Should(Succeed())
 
+			By("Creating repository and waiting for artifact")
 			repositoryKey := types.NamespacedName{
 				Name:      "helmrepository-sample-" + randStringRunes(5),
 				Namespace: namespace.Name,
@@ -312,12 +313,21 @@ var _ = Describe("HelmChartReconciler", func() {
 					SecretRef: &corev1.LocalObjectReference{
 						Name: secretKey.Name,
 					},
-					Interval: metav1.Duration{Duration: time.Hour * 1},
+					Interval: metav1.Duration{Duration: pullInterval},
 				},
 			}
 			Expect(k8sClient.Create(context.Background(), repository)).Should(Succeed())
 			defer k8sClient.Delete(context.Background(), repository)
 
+			Eventually(func() bool {
+				_ = k8sClient.Get(context.Background(), repositoryKey, repository)
+				return repository.Status.Artifact != nil
+			}, timeout, interval).Should(BeTrue())
+
+			By("Deleting secret before applying HelmChart")
+			Expect(k8sClient.Delete(context.Background(), secret)).Should(Succeed())
+
+			By("Applying HelmChart")
 			key := types.NamespacedName{
 				Name:      "helmchart-sample-" + randStringRunes(5),
 				Namespace: namespace.Name,
@@ -340,35 +350,24 @@ var _ = Describe("HelmChartReconciler", func() {
 			Expect(k8sClient.Create(context.Background(), chart)).Should(Succeed())
 			defer k8sClient.Delete(context.Background(), chart)
 
-			By("Expecting artifact")
+			By("Expecting missing secret error")
-			Expect(k8sClient.Update(context.Background(), secret)).Should(Succeed())
-			Eventually(func() bool {
-				got := &sourcev1.HelmChart{}
-				_ = k8sClient.Get(context.Background(), key, got)
-				return got.Status.Artifact != nil &&
-					storage.ArtifactExist(*got.Status.Artifact)
-			}, timeout, interval).Should(BeTrue())
-
-			delete(secret.Data, "username")
-			Expect(k8sClient.Update(context.Background(), secret)).Should(Succeed())
-
-			By("Expecting missing field error")
-			delete(secret.Data, "username")
-			Expect(k8sClient.Update(context.Background(), secret)).Should(Succeed())
 			got := &sourcev1.HelmChart{}
 			Eventually(func() bool {
 				_ = k8sClient.Get(context.Background(), key, got)
 				for _, c := range got.Status.Conditions {
-					if c.Reason == sourcev1.AuthenticationFailedReason {
+					if c.Reason == sourcev1.AuthenticationFailedReason &&
+						strings.Contains(c.Message, "auth secret error") {
 						return true
 					}
 				}
 				return false
 			}, timeout, interval).Should(BeTrue())
-			Expect(got.Status.Artifact).ToNot(BeNil())
 
-			delete(secret.Data, "password")
+			By("Applying secret with missing keys")
-			Expect(k8sClient.Update(context.Background(), secret)).Should(Succeed())
+			secret.ResourceVersion = ""
+			secret.Data["username"] = []byte{}
+			secret.Data["password"] = []byte{}
+			Expect(k8sClient.Create(context.Background(), secret)).Should(Succeed())
 
 			By("Expecting 401")
 			Eventually(func() bool {
@@ -383,20 +382,36 @@ var _ = Describe("HelmChartReconciler", func() {
 				return false
 			}, timeout, interval).Should(BeTrue())
 
-			By("Expecting missing secret error")
+			By("Adding username key")
-			Expect(k8sClient.Delete(context.Background(), secret)).Should(Succeed())
+			secret.Data["username"] = []byte(username)
-			got = &sourcev1.HelmChart{}
+			Expect(k8sClient.Update(context.Background(), secret)).Should(Succeed())
+
+			By("Expecting missing field error")
 			Eventually(func() bool {
 				_ = k8sClient.Get(context.Background(), key, got)
 				for _, c := range got.Status.Conditions {
-					if c.Reason == sourcev1.AuthenticationFailedReason &&
+					if c.Reason == sourcev1.AuthenticationFailedReason {
-						strings.Contains(c.Message, "auth secret error") {
 						return true
 					}
 				}
 				return false
 			}, timeout, interval).Should(BeTrue())
-			Expect(got.Status.Artifact).ShouldNot(BeNil())
+
+			By("Adding password key")
+			secret.Data["password"] = []byte(password)
+			Expect(k8sClient.Update(context.Background(), secret)).Should(Succeed())
+
+			By("Expecting artifact")
+			Eventually(func() bool {
+				_ = k8sClient.Get(context.Background(), key, got)
+				for _, c := range got.Status.Conditions {
+					if c.Type == sourcev1.ReadyCondition && c.Status == corev1.ConditionTrue {
+						return true
+					}
+				}
+				return false
+			}, timeout, interval).Should(BeTrue())
+			Expect(got.Status.Artifact).ToNot(BeNil())
 		})
 	})
 

controllers/helmrepository_controller.go

@@ -217,7 +217,7 @@ func (r *HelmRepositoryReconciler) reconcile(ctx context.Context, repository sou
 	// return early on unchanged generation
 	artifact := r.Storage.NewArtifactFor(repository.Kind, repository.ObjectMeta.GetObjectMeta(), i.Generated.Format(time.RFC3339Nano),
 		fmt.Sprintf("index-%s.yaml", url.PathEscape(i.Generated.Format(time.RFC3339Nano))))
-	if repository.GetArtifact().HasRevision(artifact.Revision) {
+	if sourcev1.InReadyCondition(repository.Status.Conditions) && repository.GetArtifact().HasRevision(artifact.Revision) {
 		if artifact.URL != repository.GetArtifact().URL {
 			r.Storage.SetArtifactURL(repository.GetArtifact())
 			repository.Status.URL = r.Storage.SetHostname(repository.Status.URL)
@@ -266,7 +266,8 @@ func (r *HelmRepositoryReconciler) reconcile(ctx context.Context, repository sou
 // resetStatus returns a modified v1alpha1.HelmRepository and a boolean indicating
 // if the status field has been reset.
 func (r *HelmRepositoryReconciler) resetStatus(repository sourcev1.HelmRepository) (sourcev1.HelmRepository, bool) {
-	if repository.GetArtifact() != nil && !r.Storage.ArtifactExist(*repository.GetArtifact()) {
+	// We do not have an artifact, or it does no longer exist
+	if repository.GetArtifact() == nil || !r.Storage.ArtifactExist(*repository.GetArtifact()) {
 		repository = sourcev1.HelmRepositoryProgressing(repository)
 		repository.Status.Artifact = nil
 		return repository, true

controllers/helmrepository_controller_test.go

@@ -174,6 +174,7 @@ var _ = Describe("HelmRepositoryReconciler", func() {
 				},
 			}
 			Expect(k8sClient.Create(context.Background(), created)).Should(Succeed())
+			defer k8sClient.Delete(context.Background(), created)
 
 			By("Expecting index download to succeed")
 			Eventually(func() bool {
@@ -257,6 +258,7 @@ var _ = Describe("HelmRepositoryReconciler", func() {
 				},
 			}
 			Expect(k8sClient.Create(context.Background(), created)).Should(Succeed())
+			defer k8sClient.Delete(context.Background(), created)
 
 			By("Expecting 401")
 			Eventually(func() bool {
@@ -348,6 +350,7 @@ var _ = Describe("HelmRepositoryReconciler", func() {
 				},
 			}
 			Expect(k8sClient.Create(context.Background(), created)).Should(Succeed())
+			defer k8sClient.Delete(context.Background(), created)
 
 			By("Expecting unknown authority error")
 			Eventually(func() bool {