From e5d3aa37011df206112bd733d7894c0c658e944e Mon Sep 17 00:00:00 2001 From: Sunny Date: Thu, 22 Sep 2022 03:27:53 +0530 Subject: [PATCH 1/3] summarize: consider bipolarity in status condition This introduces the consideration of bipolarity conditions in the status condition summary for Ready condition. The summarize.HelperOptions can now be configured with a list of bipolarity conditions which are used in SummarizeAndPatch() to set the Ready condition to failing bipolarity condition with the highest priority. Bipolarity condition is not a typical status property. It is a mix of positive and negative polarities. It's "normal-true" and "abnormal-false". Failing bipolarity conditions are prioritized over other conditions to show the actual reason of failure on the Ready status. Signed-off-by: Sunny --- internal/reconcile/summarize/summary.go | 31 +++++++ internal/reconcile/summarize/summary_test.go | 89 ++++++++++++++++++-- 2 files changed, 111 insertions(+), 9 deletions(-) diff --git a/internal/reconcile/summarize/summary.go b/internal/reconcile/summarize/summary.go index d274d03d5..6a556a18e 100644 --- a/internal/reconcile/summarize/summary.go +++ b/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 diff --git a/internal/reconcile/summarize/summary_test.go b/internal/reconcile/summarize/summary_test.go index b16d19e37..67af44c80 100644 --- a/internal/reconcile/summarize/summary_test.go +++ b/internal/reconcile/summarize/summary_test.go @@ -44,11 +44,16 @@ 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, @@ -56,6 +61,9 @@ func TestSummarizeAndPatch(t *testing.T) { 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)) From 64bd34f116a4149140b96b9b694e053c8ba0e964 Mon Sep 17 00:00:00 2001 From: Sunny Date: Thu, 22 Sep 2022 03:28:51 +0530 Subject: [PATCH 2/3] Use bipolarity option in gitrepo and ocirepo Use the bipolarity condition options in OCIRepository and GitRepository reconcilers. Signed-off-by: Sunny --- controllers/gitrepository_controller.go | 10 ++++++++++ controllers/ocirepository_controller.go | 9 +++++++++ 2 files changed, 19 insertions(+) diff --git a/controllers/gitrepository_controller.go b/controllers/gitrepository_controller.go index 3fba8bc02..a0a5cee9f 100644 --- a/controllers/gitrepository_controller.go +++ b/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 diff --git a/controllers/ocirepository_controller.go b/controllers/ocirepository_controller.go index 1ebc1eb74..0fb35a73c 100644 --- a/controllers/ocirepository_controller.go +++ b/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 From 90b7cec915b4462fbf3ef51c2d9d1f10f86e6aed Mon Sep 17 00:00:00 2001 From: Sunny Date: Thu, 22 Sep 2022 03:19:59 +0530 Subject: [PATCH 3/3] ocirepo: Fix event trace type value Signed-off-by: Sunny --- controllers/ocirepository_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/ocirepository_controller.go b/controllers/ocirepository_controller.go index 0fb35a73c..1003a574b 100644 --- a/controllers/ocirepository_controller.go +++ b/controllers/ocirepository_controller.go @@ -719,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 }