Merge pull request #907 from fluxcd/summarize-with-bipolarity

Consider bipolarity conditions in Ready condition summarization

Author: stefanprodan

controllers/gitrepository_controller.go

@@ -194,6 +194,7 @@ func (r *GitRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 		summarizeHelper := summarize.NewHelper(r.EventRecorder, patchHelper)
 		summarizeOpts := []summarize.Option{
 			summarize.WithConditions(gitRepositoryReadyCondition),
+			summarize.WithBiPolarityConditionTypes(sourcev1.SourceVerifiedCondition),
 			summarize.WithReconcileResult(recResult),
 			summarize.WithReconcileError(retErr),
 			summarize.WithIgnoreNotFound(),
@@ -430,6 +431,15 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context,
 			errors.New("libgit2 managed transport not initialized"), "Libgit2TransportNotEnabled",
 		)
 	}
+
+	// Remove previously failed source verification status conditions. The
+	// failing verification should be recalculated. But an existing successful
+	// verification need not be removed as it indicates verification of previous
+	// version.
+	if conditions.IsFalse(obj, sourcev1.SourceVerifiedCondition) {
+		conditions.Delete(obj, sourcev1.SourceVerifiedCondition)
+	}
+
 	// Configure authentication strategy to access the source
 	var authOpts *git.AuthOptions
 	var err error

controllers/ocirepository_controller.go

@@ -194,6 +194,7 @@ func (r *OCIRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 		summarizeHelper := summarize.NewHelper(r.EventRecorder, patchHelper)
 		summarizeOpts := []summarize.Option{
 			summarize.WithConditions(ociRepositoryReadyCondition),
+			summarize.WithBiPolarityConditionTypes(sourcev1.SourceVerifiedCondition),
 			summarize.WithReconcileResult(recResult),
 			summarize.WithReconcileError(retErr),
 			summarize.WithIgnoreNotFound(),
@@ -297,6 +298,14 @@ func (r *OCIRepositoryReconciler) reconcileSource(ctx context.Context, obj *sour
 	ctxTimeout, cancel := context.WithTimeout(ctx, obj.Spec.Timeout.Duration)
 	defer cancel()
 
+	// Remove previously failed source verification status conditions. The
+	// failing verification should be recalculated. But an existing successful
+	// verification need not be removed as it indicates verification of previous
+	// version.
+	if conditions.IsFalse(obj, sourcev1.SourceVerifiedCondition) {
+		conditions.Delete(obj, sourcev1.SourceVerifiedCondition)
+	}
+
 	options := r.craneOptions(ctxTimeout, obj.Spec.Insecure)
 
 	// Generate the registry credential keychain either from static credentials or using cloud OIDC
@@ -710,7 +719,7 @@ func (r *OCIRepositoryReconciler) keychain(ctx context.Context, obj *sourcev1.OC
 		imagePullSecret := corev1.Secret{}
 		err := r.Get(ctx, types.NamespacedName{Namespace: obj.Namespace, Name: imagePullSecretName}, &imagePullSecret)
 		if err != nil {
-			r.eventLogf(ctx, obj, events.EventSeverityTrace, sourcev1.AuthenticationFailedReason,
+			r.eventLogf(ctx, obj, events.EventTypeTrace, sourcev1.AuthenticationFailedReason,
 				"auth secret '%s' not found", imagePullSecretName)
 			return nil, err
 		}

internal/reconcile/summarize/summary.go

@@ -90,6 +90,9 @@ type HelperOptions struct {
 	// PatchFieldOwner defines the field owner configuration for the Kubernetes
 	// patch operation.
 	PatchFieldOwner string
+	// BiPolarityConditionTypes is a list of bipolar conditions in the order
+	// of priority.
+	BiPolarityConditionTypes []string
 }
 
 // Option is configuration that modifies SummarizeAndPatch.
@@ -149,6 +152,14 @@ func WithPatchFieldOwner(fieldOwner string) Option {
 	}
 }
 
+// WithBiPolarityConditionTypes sets the BiPolarityConditionTypes used to
+// calculate the value of Ready condition in SummarizeAndPatch.
+func WithBiPolarityConditionTypes(types ...string) Option {
+	return func(s *HelperOptions) {
+		s.BiPolarityConditionTypes = types
+	}
+}
+
 // SummarizeAndPatch summarizes and patches the result to the target object.
 // When used at the very end of a reconciliation, the result builder must be
 // specified using the Option WithResultBuilder(). The returned result and error
@@ -206,6 +217,26 @@ func (h *Helper) SummarizeAndPatch(ctx context.Context, obj conditions.Setter, o
 		)
 	}
 
+	// Check any BiPolarity conditions in the status that are False. Failing
+	// BiPolarity condition should be set as the Ready condition value to
+	// reflect the actual cause of the reconciliation failure.
+	// NOTE: This is applicable to Ready condition only because it is a special
+	// condition in kstatus that reflects the overall state of an object.
+	// IMPLEMENTATION NOTE: An implementation of this within the
+	// conditions.merge() exists in fluxcd/pkg repo branch `bipolarity`
+	// (https://github.com/fluxcd/pkg/commit/756b9e6d253a4fae93c05419b7019d0169454858).
+	// If that gets added to conditions.merge, the following can be removed.
+	var failedBiPolarity []string
+	for _, c := range opts.BiPolarityConditionTypes {
+		if conditions.IsFalse(obj, c) {
+			failedBiPolarity = append(failedBiPolarity, c)
+		}
+	}
+	if len(failedBiPolarity) > 0 {
+		topFailedBiPolarity := conditions.Get(obj, failedBiPolarity[0])
+		conditions.MarkFalse(obj, meta.ReadyCondition, topFailedBiPolarity.Reason, topFailedBiPolarity.Message)
+	}
+
 	// If object is not stalled, result is success and runtime error is nil,
 	// ensure that Ready=True. Else, use the Ready failure message as the
 	// runtime error message. This ensures that the reconciliation would be

internal/reconcile/summarize/summary_test.go

@@ -44,18 +44,26 @@ import (
 // This tests the scenario where SummarizeAndPatch is used at the very end of a
 // reconciliation.
 func TestSummarizeAndPatch(t *testing.T) {
+	testBipolarCondition1 := "FooChecked1"
+	testBipolarCondition2 := "FooChecked2"
 	var testReadyConditions = Conditions{
 		Target: meta.ReadyCondition,
 		Owned: []string{
 			sourcev1.FetchFailedCondition,
 			sourcev1.ArtifactOutdatedCondition,
+			sourcev1.SourceVerifiedCondition,
+			testBipolarCondition1,
+			testBipolarCondition2,
 			meta.ReadyCondition,
 			meta.ReconcilingCondition,
 			meta.StalledCondition,
 		},
 		Summarize: []string{
 			sourcev1.FetchFailedCondition,
 			sourcev1.ArtifactOutdatedCondition,
+			sourcev1.SourceVerifiedCondition,
+			testBipolarCondition1,
+			testBipolarCondition2,
 			meta.StalledCondition,
 			meta.ReconcilingCondition,
 		},
@@ -66,6 +74,7 @@ func TestSummarizeAndPatch(t *testing.T) {
 			meta.ReconcilingCondition,
 		},
 	}
+	var testBipolarConditions = []string{sourcev1.SourceVerifiedCondition, testBipolarCondition1, testBipolarCondition2}
 	var testFooConditions = Conditions{
 		Target: "Foo",
 		Owned: []string{
@@ -83,15 +92,16 @@ func TestSummarizeAndPatch(t *testing.T) {
 	}
 
 	tests := []struct {
-		name             string
-		generation       int64
-		beforeFunc       func(obj conditions.Setter)
-		result           reconcile.Result
-		reconcileErr     error
-		conditions       []Conditions
-		wantErr          bool
-		afterFunc        func(t *WithT, obj client.Object)
-		assertConditions []metav1.Condition
+		name              string
+		generation        int64
+		beforeFunc        func(obj conditions.Setter)
+		result            reconcile.Result
+		reconcileErr      error
+		conditions        []Conditions
+		bipolarConditions []string
+		wantErr           bool
+		afterFunc         func(t *WithT, obj client.Object)
+		assertConditions  []metav1.Condition
 	}{
 		// Success/Fail indicates if a reconciliation succeeded or failed.
 		// The object generation is expected to match the observed generation in
@@ -250,6 +260,64 @@ func TestSummarizeAndPatch(t *testing.T) {
 			},
 			wantErr: true,
 		},
+		{
+			name:       "Fail, reconciling with bipolar condition False, Ready gets bipolar failure value",
+			generation: 2,
+			beforeFunc: func(obj conditions.Setter) {
+				conditions.MarkReconciling(obj, "NewRevision", "new index revision")
+				conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, "VerifyFailed", "verify failed")
+			},
+			result:            reconcile.ResultEmpty,
+			reconcileErr:      errors.New("failed to verify source"),
+			conditions:        []Conditions{testReadyConditions},
+			bipolarConditions: testBipolarConditions,
+			wantErr:           true,
+			assertConditions: []metav1.Condition{
+				*conditions.FalseCondition(meta.ReadyCondition, "VerifyFailed", "verify failed"),
+				*conditions.FalseCondition(sourcev1.SourceVerifiedCondition, "VerifyFailed", "verify failed"),
+				*conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new index revision"),
+			},
+		},
+		{
+			name:       "Fail, bipolar condition True, negative polarity True, Ready gets negative polarity value",
+			generation: 2,
+			beforeFunc: func(obj conditions.Setter) {
+				conditions.MarkReconciling(obj, "NewGeneration", "new obj gen")
+				conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", "new digest")
+				conditions.MarkTrue(obj, sourcev1.SourceVerifiedCondition, "Success", "verified")
+			},
+			result:            reconcile.ResultEmpty,
+			reconcileErr:      errors.New("failed to create dir"),
+			conditions:        []Conditions{testReadyConditions},
+			bipolarConditions: testBipolarConditions,
+			wantErr:           true,
+			assertConditions: []metav1.Condition{
+				*conditions.FalseCondition(meta.ReadyCondition, "NewRevision", "new digest"),
+				*conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new digest"),
+				*conditions.TrueCondition(meta.ReconcilingCondition, "NewGeneration", "new obj gen"),
+				*conditions.TrueCondition(sourcev1.SourceVerifiedCondition, "Success", "verified"),
+			},
+		},
+		{
+			name:       "Fail, multiple bipolar conditions False, Ready gets the bipolar with high priority",
+			generation: 2,
+			beforeFunc: func(obj conditions.Setter) {
+				conditions.MarkTrue(obj, sourcev1.SourceVerifiedCondition, "Success", "verified")
+				conditions.MarkFalse(obj, testBipolarCondition1, "AAA", "aaa")
+				conditions.MarkFalse(obj, testBipolarCondition2, "BBB", "bbb")
+			},
+			result:            reconcile.ResultEmpty,
+			reconcileErr:      errors.New("some failure"),
+			conditions:        []Conditions{testReadyConditions},
+			bipolarConditions: testBipolarConditions,
+			wantErr:           true,
+			assertConditions: []metav1.Condition{
+				*conditions.FalseCondition(meta.ReadyCondition, "AAA", "aaa"),
+				*conditions.FalseCondition(testBipolarCondition1, "AAA", "aaa"),
+				*conditions.FalseCondition(testBipolarCondition2, "BBB", "bbb"),
+				*conditions.TrueCondition(sourcev1.SourceVerifiedCondition, "Success", "verified"),
+			},
+		},
 	}
 
 	for _, tt := range tests {
@@ -289,6 +357,9 @@ func TestSummarizeAndPatch(t *testing.T) {
 				WithProcessors(RecordContextualError, RecordReconcileReq),
 				WithResultBuilder(reconcile.AlwaysRequeueResultBuilder{RequeueAfter: obj.Spec.Interval.Duration}),
 			}
+			if tt.bipolarConditions != nil {
+				summaryOpts = append(summaryOpts, WithBiPolarityConditionTypes(tt.bipolarConditions...))
+			}
 			_, gotErr := summaryHelper.SummarizeAndPatch(ctx, obj, summaryOpts...)
 			g.Expect(gotErr != nil).To(Equal(tt.wantErr))