Consider bipolarity conditions in Ready condition summarization

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))