From 857570b3d340a9b98f39f3049c05cf55dfb1fc65 Mon Sep 17 00:00:00 2001 From: Brandon Palm Date: Fri, 17 Jul 2026 13:50:53 -0500 Subject: [PATCH] CM-1114: Add health probes and richer status conditions Add Progressing condition to operator status alongside existing Degraded and Ready conditions. Introduce WithConditionReason/GetConditionReason on ReconcileError for structured condition reason propagation through the error chain. Add liveness and readiness probes to the operator deployment manifest. Add HandleReconcileResult unit tests covering success, irrecoverable, and recoverable error paths including custom ConditionReason propagation, no-change skip optimization, and updateConditionFn error propagation. Apply typed-nil guard consistently to all errors.As call sites in the error classification functions. --- api/operator/v1alpha1/conditions.go | 15 ++ api/operator/v1alpha1/conditions_test.go | 23 +++ ...anager-operator.clusterserviceversion.yaml | 18 ++ config/manager/manager.yaml | 18 ++ pkg/controller/common/errors.go | 27 ++- pkg/controller/common/errors_test.go | 83 ++++++++ pkg/controller/common/reconcile_result.go | 45 +++-- .../common/reconcile_result_test.go | 184 ++++++++++++++++++ pkg/controller/istiocsr/controller_test.go | 42 +++- pkg/controller/istiocsr/install_istiocsr.go | 3 +- pkg/controller/istiocsr/utils.go | 12 +- .../trustmanager/controller_test.go | 33 +++- .../trustmanager/install_trustmanager.go | 6 +- test/e2e/trustmanager_test.go | 10 +- 14 files changed, 481 insertions(+), 38 deletions(-) create mode 100644 pkg/controller/common/reconcile_result_test.go diff --git a/api/operator/v1alpha1/conditions.go b/api/operator/v1alpha1/conditions.go index 46f6047b9..2d20d3456 100644 --- a/api/operator/v1alpha1/conditions.go +++ b/api/operator/v1alpha1/conditions.go @@ -25,6 +25,13 @@ const ( // - Failed // - Ready: operand successfully deployed and ready Ready string = "Ready" + + // Progressing indicates whether the operator is actively reconciling + // toward the desired state. + // Status: + // - True: reconciliation is in progress + // - False: reconciliation is idle (succeeded or permanently failed) + Progressing string = "Progressing" ) const ( @@ -33,6 +40,14 @@ const ( ReasonReady string = "Ready" ReasonInProgress string = "Progressing" + + ReasonReconciling string = "Reconciling" + + ReasonWaitingForDependencies string = "WaitingForDependencies" + + ReasonValidationFailed string = "ValidationFailed" + + ReasonMultipleInstancesFound string = "MultipleInstancesFound" ) func (c *ConditionalStatus) GetCondition(t string) *metav1.Condition { diff --git a/api/operator/v1alpha1/conditions_test.go b/api/operator/v1alpha1/conditions_test.go index 71295e27a..2d0fa6f9c 100644 --- a/api/operator/v1alpha1/conditions_test.go +++ b/api/operator/v1alpha1/conditions_test.go @@ -37,6 +37,29 @@ func TestGetCondition(t *testing.T) { Reason: ReasonReady, }, }, + { + name: "progressing condition present", + conditionalStatus: ConditionalStatus{ + Conditions: []metav1.Condition{ + { + Type: Ready, + Status: metav1.ConditionTrue, + Reason: ReasonReady, + }, + { + Type: Progressing, + Status: metav1.ConditionFalse, + Reason: ReasonReady, + }, + }, + }, + condition: Progressing, + expectedCondition: &metav1.Condition{ + Type: Progressing, + Status: metav1.ConditionFalse, + Reason: ReasonReady, + }, + }, { name: "requested condition not present", conditionalStatus: ConditionalStatus{ diff --git a/bundle/manifests/cert-manager-operator.clusterserviceversion.yaml b/bundle/manifests/cert-manager-operator.clusterserviceversion.yaml index 2832f06b5..867a17d23 100644 --- a/bundle/manifests/cert-manager-operator.clusterserviceversion.yaml +++ b/bundle/manifests/cert-manager-operator.clusterserviceversion.yaml @@ -817,11 +817,29 @@ spec: - name: UNSUPPORTED_ADDON_FEATURES image: openshift.io/cert-manager-operator:latest imagePullPolicy: IfNotPresent + livenessProbe: + failureThreshold: 3 + httpGet: + path: /healthz + port: https + scheme: HTTPS + initialDelaySeconds: 15 + periodSeconds: 10 + timeoutSeconds: 5 name: cert-manager-operator ports: - containerPort: 8443 name: https protocol: TCP + readinessProbe: + failureThreshold: 3 + httpGet: + path: /readyz + port: https + scheme: HTTPS + initialDelaySeconds: 5 + periodSeconds: 10 + timeoutSeconds: 5 resources: requests: cpu: 10m diff --git a/config/manager/manager.yaml b/config/manager/manager.yaml index af071a2b7..e260c2150 100644 --- a/config/manager/manager.yaml +++ b/config/manager/manager.yaml @@ -102,6 +102,24 @@ spec: image: controller:latest imagePullPolicy: IfNotPresent name: cert-manager-operator + livenessProbe: + httpGet: + path: /healthz + port: https + scheme: HTTPS + initialDelaySeconds: 15 + periodSeconds: 10 + timeoutSeconds: 5 + failureThreshold: 3 + readinessProbe: + httpGet: + path: /readyz + port: https + scheme: HTTPS + initialDelaySeconds: 5 + periodSeconds: 10 + timeoutSeconds: 5 + failureThreshold: 3 securityContext: allowPrivilegeEscalation: false capabilities: diff --git a/pkg/controller/common/errors.go b/pkg/controller/common/errors.go index 64e914b55..ef08ef23e 100644 --- a/pkg/controller/common/errors.go +++ b/pkg/controller/common/errors.go @@ -23,9 +23,10 @@ const ( // ReconcileError represents an error that occurred during reconciliation. type ReconcileError struct { - Reason ErrorReason `json:"reason,omitempty"` - Message string `json:"message,omitempty"` - Err error `json:"error,omitempty"` + Reason ErrorReason `json:"reason,omitempty"` + ConditionReason string `json:"conditionReason,omitempty"` + Message string `json:"message,omitempty"` + Err error `json:"error,omitempty"` } var _ error = &ReconcileError{} @@ -117,6 +118,16 @@ func IsMultipleInstanceError(err error) bool { return false } +// WithConditionReason sets the condition reason that will appear in status +// conditions when this error is processed by HandleReconcileResult. +func (e *ReconcileError) WithConditionReason(reason string) *ReconcileError { + if e == nil { + return nil + } + e.ConditionReason = reason + return e +} + // Error implements the error interface. func (e *ReconcileError) Error() string { return fmt.Sprintf("%s: %s", e.Message, e.Err) @@ -126,3 +137,13 @@ func (e *ReconcileError) Error() string { func (e *ReconcileError) Unwrap() error { return e.Err } + +// GetConditionReason extracts the ConditionReason from a ReconcileError in the +// error chain. Returns empty string if not set or not a ReconcileError. +func GetConditionReason(err error) string { + rerr := &ReconcileError{} + if errors.As(err, &rerr) { + return rerr.ConditionReason + } + return "" +} diff --git a/pkg/controller/common/errors_test.go b/pkg/controller/common/errors_test.go index c263d652e..c09a4d989 100644 --- a/pkg/controller/common/errors_test.go +++ b/pkg/controller/common/errors_test.go @@ -284,6 +284,89 @@ func TestIsRetryRequiredError(t *testing.T) { } } +func TestWithConditionReason(t *testing.T) { + tests := []struct { + name string + err *ReconcileError + reason string + wantNil bool + wantReason string + }{ + { + name: "sets condition reason on irrecoverable error", + err: NewIrrecoverableError(fmt.Errorf("x"), "msg"), + reason: "ValidationFailed", + wantReason: "ValidationFailed", + }, + { + name: "sets condition reason on retry error", + err: NewRetryRequiredError(fmt.Errorf("x"), "msg"), + reason: "ResourceApplyFailed", + wantReason: "ResourceApplyFailed", + }, + { + name: "nil error returns nil", + err: nil, + reason: "ValidationFailed", + wantNil: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.err.WithConditionReason(tt.reason) + if tt.wantNil { + if got != nil { + t.Errorf("expected nil, got %v", got) + } + return + } + if got == nil { + t.Fatal("expected non-nil") + } + if got.ConditionReason != tt.wantReason { + t.Errorf("ConditionReason = %q, want %q", got.ConditionReason, tt.wantReason) + } + }) + } +} + +func TestGetConditionReason(t *testing.T) { + tests := []struct { + name string + err error + wantReason string + }{ + { + name: "extracts reason from ReconcileError", + err: NewIrrecoverableError(fmt.Errorf("x"), "msg").WithConditionReason("ValidationFailed"), + wantReason: "ValidationFailed", + }, + { + name: "returns empty for ReconcileError without ConditionReason", + err: NewIrrecoverableError(fmt.Errorf("x"), "msg"), + wantReason: "", + }, + { + name: "returns empty for plain error", + err: fmt.Errorf("plain"), + wantReason: "", + }, + { + name: "returns empty for nil", + err: nil, + wantReason: "", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := GetConditionReason(tt.err) + if got != tt.wantReason { + t.Errorf("GetConditionReason() = %q, want %q", got, tt.wantReason) + } + }) + } +} + func TestIsMultipleInstanceError(t *testing.T) { tests := []struct { name string diff --git a/pkg/controller/common/reconcile_result.go b/pkg/controller/common/reconcile_result.go index b1cad8731..ae47ec70b 100644 --- a/pkg/controller/common/reconcile_result.go +++ b/pkg/controller/common/reconcile_result.go @@ -11,6 +11,15 @@ import ( v1alpha1 "github.com/openshift/cert-manager-operator/api/operator/v1alpha1" ) +// resolveConditionReason extracts a specific condition reason from the error +// chain if one was set via WithConditionReason, otherwise returns the default. +func resolveConditionReason(err error, defaultReason string) string { + if reason := GetConditionReason(err); reason != "" { + return reason + } + return defaultReason +} + // HandleReconcileResult processes the result of a reconciliation attempt and // updates status conditions accordingly. func HandleReconcileResult( @@ -24,52 +33,54 @@ func HandleReconcileResult( if reconcileErr != nil { if IsIrrecoverableError(reconcileErr) { - // Permanent failure - don't retry - // Set Degraded=True, Ready=False - // Set both conditions atomically before updating status - degradedChanged := status.SetCondition(v1alpha1.Degraded, metav1.ConditionTrue, v1alpha1.ReasonFailed, fmt.Sprintf("reconciliation failed with irrecoverable error not retrying: %v", reconcileErr)) - readyChanged := status.SetCondition(v1alpha1.Ready, metav1.ConditionFalse, v1alpha1.ReasonFailed, "") + reason := resolveConditionReason(reconcileErr, v1alpha1.ReasonFailed) + degradedChanged := status.SetCondition(v1alpha1.Degraded, metav1.ConditionTrue, reason, fmt.Sprintf("reconciliation failed with irrecoverable error not retrying: %v", reconcileErr)) + readyChanged := status.SetCondition(v1alpha1.Ready, metav1.ConditionFalse, reason, "") + progressingChanged := status.SetCondition(v1alpha1.Progressing, metav1.ConditionFalse, reason, "") - if degradedChanged || readyChanged { + if degradedChanged || readyChanged || progressingChanged { log.V(2).Info("updating conditions on irrecoverable error", "degradedChanged", degradedChanged, "readyChanged", readyChanged, + "progressingChanged", progressingChanged, + "reason", reason, "error", reconcileErr) errUpdate = updateConditionFn(nil) } return ctrl.Result{}, errUpdate } - // Temporary failure - retry after delay - // Set Degraded=False, Ready=False with "in progress" message - // Set both conditions atomically before updating status + readyReason := resolveConditionReason(reconcileErr, v1alpha1.ReasonInProgress) + progressingReason := resolveConditionReason(reconcileErr, v1alpha1.ReasonReconciling) degradedChanged := status.SetCondition(v1alpha1.Degraded, metav1.ConditionFalse, v1alpha1.ReasonReady, "") - readyChanged := status.SetCondition(v1alpha1.Ready, metav1.ConditionFalse, v1alpha1.ReasonInProgress, fmt.Sprintf("reconciliation failed, retrying: %v", reconcileErr)) + readyChanged := status.SetCondition(v1alpha1.Ready, metav1.ConditionFalse, readyReason, fmt.Sprintf("reconciliation failed, retrying: %v", reconcileErr)) + progressingChanged := status.SetCondition(v1alpha1.Progressing, metav1.ConditionTrue, progressingReason, fmt.Sprintf("reconciliation in progress: %v", reconcileErr)) - if degradedChanged || readyChanged { + if degradedChanged || readyChanged || progressingChanged { log.V(2).Info("updating conditions on recoverable error", "degradedChanged", degradedChanged, "readyChanged", readyChanged, + "progressingChanged", progressingChanged, + "reason", readyReason, "error", reconcileErr) errUpdate = updateConditionFn(reconcileErr) } - // For recoverable errors, either requeue manually or return error, not both. - // If status update failed, return the update error; otherwise requeue. + // Return status-update error or requeue, not both. if errUpdate != nil { return ctrl.Result{}, errUpdate } return ctrl.Result{RequeueAfter: requeueDuration}, nil } - // Success - update status - // Set both conditions atomically before updating status on success degradedChanged := status.SetCondition(v1alpha1.Degraded, metav1.ConditionFalse, v1alpha1.ReasonReady, "") readyChanged := status.SetCondition(v1alpha1.Ready, metav1.ConditionTrue, v1alpha1.ReasonReady, "reconciliation successful") + progressingChanged := status.SetCondition(v1alpha1.Progressing, metav1.ConditionFalse, v1alpha1.ReasonReady, "") - if degradedChanged || readyChanged { + if degradedChanged || readyChanged || progressingChanged { log.V(2).Info("updating conditions on successful reconciliation", "degradedChanged", degradedChanged, - "readyChanged", readyChanged) + "readyChanged", readyChanged, + "progressingChanged", progressingChanged) errUpdate = updateConditionFn(nil) } return ctrl.Result{}, errUpdate diff --git a/pkg/controller/common/reconcile_result_test.go b/pkg/controller/common/reconcile_result_test.go new file mode 100644 index 000000000..24bccc096 --- /dev/null +++ b/pkg/controller/common/reconcile_result_test.go @@ -0,0 +1,184 @@ +package common + +import ( + "fmt" + "testing" + "time" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + v1alpha1 "github.com/openshift/cert-manager-operator/api/operator/v1alpha1" +) + +func conditionByType(status *v1alpha1.ConditionalStatus, ct string) *metav1.Condition { + return status.GetCondition(ct) +} + +func TestHandleReconcileResult(t *testing.T) { + tests := []struct { + name string + + reconcileErr error + + wantDegradedStatus metav1.ConditionStatus + wantDegradedReason string + wantReadyStatus metav1.ConditionStatus + wantReadyReason string + wantProgressingStatus metav1.ConditionStatus + wantProgressingReason string + wantRequeue bool + wantUpdateCalled bool + }{ + { + name: "success sets Ready=True, Degraded=False, Progressing=False", + reconcileErr: nil, + wantDegradedStatus: metav1.ConditionFalse, + wantDegradedReason: v1alpha1.ReasonReady, + wantReadyStatus: metav1.ConditionTrue, + wantReadyReason: v1alpha1.ReasonReady, + wantProgressingStatus: metav1.ConditionFalse, + wantProgressingReason: v1alpha1.ReasonReady, + wantUpdateCalled: true, + }, + { + name: "irrecoverable error sets Degraded=True, Ready=False, Progressing=False", + reconcileErr: NewIrrecoverableError(fmt.Errorf("fatal"), "fatal failure"), + wantDegradedStatus: metav1.ConditionTrue, + wantDegradedReason: v1alpha1.ReasonFailed, + wantReadyStatus: metav1.ConditionFalse, + wantReadyReason: v1alpha1.ReasonFailed, + wantProgressingStatus: metav1.ConditionFalse, + wantProgressingReason: v1alpha1.ReasonFailed, + wantUpdateCalled: true, + }, + { + name: "irrecoverable error with custom ConditionReason uses it", + reconcileErr: NewIrrecoverableError(fmt.Errorf("validation"), "invalid config").WithConditionReason(v1alpha1.ReasonValidationFailed), + wantDegradedStatus: metav1.ConditionTrue, + wantDegradedReason: v1alpha1.ReasonValidationFailed, + wantReadyStatus: metav1.ConditionFalse, + wantReadyReason: v1alpha1.ReasonValidationFailed, + wantProgressingStatus: metav1.ConditionFalse, + wantProgressingReason: v1alpha1.ReasonValidationFailed, + wantUpdateCalled: true, + }, + { + name: "recoverable error sets Progressing=True, Ready=False, Degraded=False", + reconcileErr: NewRetryRequiredError(fmt.Errorf("transient"), "retrying"), + wantDegradedStatus: metav1.ConditionFalse, + wantDegradedReason: v1alpha1.ReasonReady, + wantReadyStatus: metav1.ConditionFalse, + wantReadyReason: v1alpha1.ReasonInProgress, + wantProgressingStatus: metav1.ConditionTrue, + wantProgressingReason: v1alpha1.ReasonReconciling, + wantRequeue: true, + wantUpdateCalled: true, + }, + { + name: "recoverable error with custom ConditionReason uses it for Ready and Progressing", + reconcileErr: NewRetryRequiredError(fmt.Errorf("waiting"), "waiting for dep").WithConditionReason(v1alpha1.ReasonWaitingForDependencies), + wantDegradedStatus: metav1.ConditionFalse, + wantDegradedReason: v1alpha1.ReasonReady, + wantReadyStatus: metav1.ConditionFalse, + wantReadyReason: v1alpha1.ReasonWaitingForDependencies, + wantProgressingStatus: metav1.ConditionTrue, + wantProgressingReason: v1alpha1.ReasonWaitingForDependencies, + wantRequeue: true, + wantUpdateCalled: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + status := &v1alpha1.ConditionalStatus{} + updateCalled := false + + updateFn := func(_ error) error { + updateCalled = true + return nil + } + + result, err := HandleReconcileResult( + status, + tc.reconcileErr, + logr.Discard(), + updateFn, + 10*time.Second, + ) + require.NoError(t, err) + + assert.Equal(t, tc.wantUpdateCalled, updateCalled, "updateConditionFn called") + + if tc.wantRequeue { + assert.NotZero(t, result.RequeueAfter, "expected requeue") + } else { + assert.Zero(t, result.RequeueAfter, "expected no requeue") + } + + degraded := conditionByType(status, v1alpha1.Degraded) + require.NotNil(t, degraded, "Degraded condition should be set") + assert.Equal(t, tc.wantDegradedStatus, degraded.Status) + assert.Equal(t, tc.wantDegradedReason, degraded.Reason) + + ready := conditionByType(status, v1alpha1.Ready) + require.NotNil(t, ready, "Ready condition should be set") + assert.Equal(t, tc.wantReadyStatus, ready.Status) + assert.Equal(t, tc.wantReadyReason, ready.Reason) + + progressing := conditionByType(status, v1alpha1.Progressing) + require.NotNil(t, progressing, "Progressing condition should be set") + assert.Equal(t, tc.wantProgressingStatus, progressing.Status) + assert.Equal(t, tc.wantProgressingReason, progressing.Reason) + }) + } +} + +func TestHandleReconcileResult_NoChangeSkipsUpdate(t *testing.T) { + status := &v1alpha1.ConditionalStatus{} + status.SetCondition(v1alpha1.Degraded, metav1.ConditionFalse, v1alpha1.ReasonReady, "") + status.SetCondition(v1alpha1.Ready, metav1.ConditionTrue, v1alpha1.ReasonReady, "reconciliation successful") + status.SetCondition(v1alpha1.Progressing, metav1.ConditionFalse, v1alpha1.ReasonReady, "") + + updateCalled := false + updateFn := func(_ error) error { + updateCalled = true + return nil + } + + _, err := HandleReconcileResult(status, nil, logr.Discard(), updateFn, 10*time.Second) + require.NoError(t, err) + assert.False(t, updateCalled, "updateConditionFn should not be called when conditions are unchanged") +} + +func TestHandleReconcileResult_UpdateFnErrorPropagated(t *testing.T) { + status := &v1alpha1.ConditionalStatus{} + updateErr := fmt.Errorf("status update failed") + + updateFn := func(_ error) error { + return updateErr + } + + _, err := HandleReconcileResult(status, nil, logr.Discard(), updateFn, 10*time.Second) + require.ErrorIs(t, err, updateErr) +} + +func TestHandleReconcileResult_IrrecoverableUpdateFnErrorPropagated(t *testing.T) { + status := &v1alpha1.ConditionalStatus{} + updateErr := fmt.Errorf("status update failed") + + updateFn := func(_ error) error { + return updateErr + } + + _, err := HandleReconcileResult( + status, + NewIrrecoverableError(fmt.Errorf("fatal"), "fatal"), + logr.Discard(), + updateFn, + 10*time.Second, + ) + require.ErrorIs(t, err, updateErr) +} diff --git a/pkg/controller/istiocsr/controller_test.go b/pkg/controller/istiocsr/controller_test.go index 9d29a2d86..be42c4b31 100644 --- a/pkg/controller/istiocsr/controller_test.go +++ b/pkg/controller/istiocsr/controller_test.go @@ -505,6 +505,11 @@ func TestProcessReconcileRequest(t *testing.T) { Status: metav1.ConditionFalse, Reason: v1alpha1.ReasonReady, }, + { + Type: v1alpha1.Progressing, + Status: metav1.ConditionFalse, + Reason: v1alpha1.ReasonReady, + }, }, expectedAnnotations: map[string]string{ controllerProcessedAnnotation: "true", @@ -566,6 +571,11 @@ func TestProcessReconcileRequest(t *testing.T) { Status: metav1.ConditionFalse, Reason: v1alpha1.ReasonReady, }, + { + Type: v1alpha1.Progressing, + Status: metav1.ConditionFalse, + Reason: v1alpha1.ReasonReady, + }, }, expectedAnnotations: map[string]string{ controllerProcessedAnnotation: "true", @@ -585,7 +595,13 @@ func TestProcessReconcileRequest(t *testing.T) { { Type: v1alpha1.Ready, Status: metav1.ConditionFalse, - Reason: v1alpha1.ReasonFailed, + Reason: v1alpha1.ReasonMultipleInstancesFound, + Message: "multiple instances of istiocsr exists, istiocsr-test-ns/istiocsr-test-resource will not be processed", + }, + { + Type: v1alpha1.Progressing, + Status: metav1.ConditionFalse, + Reason: v1alpha1.ReasonMultipleInstancesFound, Message: "multiple instances of istiocsr exists, istiocsr-test-ns/istiocsr-test-resource will not be processed", }, }, @@ -642,7 +658,13 @@ func TestProcessReconcileRequest(t *testing.T) { { Type: v1alpha1.Ready, Status: metav1.ConditionFalse, - Reason: v1alpha1.ReasonFailed, + Reason: v1alpha1.ReasonMultipleInstancesFound, + Message: "multiple instances of istiocsr exists, istiocsr3/istiocsr-test-resource will not be processed", + }, + { + Type: v1alpha1.Progressing, + Status: metav1.ConditionFalse, + Reason: v1alpha1.ReasonMultipleInstancesFound, Message: "multiple instances of istiocsr exists, istiocsr3/istiocsr-test-resource will not be processed", }, }, @@ -690,7 +712,13 @@ func TestProcessReconcileRequest(t *testing.T) { { Type: v1alpha1.Ready, Status: metav1.ConditionFalse, - Reason: v1alpha1.ReasonFailed, + Reason: v1alpha1.ReasonMultipleInstancesFound, + Message: "multiple instances of istiocsr exists, istiocsr3/istiocsr-test-resource will not be processed", + }, + { + Type: v1alpha1.Progressing, + Status: metav1.ConditionFalse, + Reason: v1alpha1.ReasonMultipleInstancesFound, Message: "multiple instances of istiocsr exists, istiocsr3/istiocsr-test-resource will not be processed", }, }, @@ -739,7 +767,13 @@ func TestProcessReconcileRequest(t *testing.T) { { Type: v1alpha1.Ready, Status: metav1.ConditionFalse, - Reason: v1alpha1.ReasonFailed, + Reason: v1alpha1.ReasonMultipleInstancesFound, + Message: "multiple instances of istiocsr exists, istiocsr3/istiocsr-test-resource will not be processed", + }, + { + Type: v1alpha1.Progressing, + Status: metav1.ConditionFalse, + Reason: v1alpha1.ReasonMultipleInstancesFound, Message: "multiple instances of istiocsr exists, istiocsr3/istiocsr-test-resource will not be processed", }, }, diff --git a/pkg/controller/istiocsr/install_istiocsr.go b/pkg/controller/istiocsr/install_istiocsr.go index 05f1d2457..1079208dc 100644 --- a/pkg/controller/istiocsr/install_istiocsr.go +++ b/pkg/controller/istiocsr/install_istiocsr.go @@ -10,7 +10,8 @@ import ( func (r *Reconciler) reconcileIstioCSRDeployment(istiocsr *v1alpha1.IstioCSR, istioCSRCreateRecon bool) error { if err := validateIstioCSRConfig(istiocsr); err != nil { - return common.NewIrrecoverableError(err, "%s/%s configuration validation failed", istiocsr.GetNamespace(), istiocsr.GetName()) + return common.NewIrrecoverableError(err, "%s/%s configuration validation failed", istiocsr.GetNamespace(), istiocsr.GetName()). + WithConditionReason(v1alpha1.ReasonValidationFailed) } // if user has set custom labels to be added to all resources created by the controller diff --git a/pkg/controller/istiocsr/utils.go b/pkg/controller/istiocsr/utils.go index 896c8bbe6..d7b915683 100644 --- a/pkg/controller/istiocsr/utils.go +++ b/pkg/controller/istiocsr/utils.go @@ -493,10 +493,13 @@ func (r *Reconciler) disallowMultipleIstioCSRInstances(istiocsr *v1alpha1.IstioC statusMessage := fmt.Sprintf("multiple instances of istiocsr exists, %s/%s will not be processed", istiocsr.GetNamespace(), istiocsr.GetName()) if containsProcessingRejectedAnnotation(istiocsr) { - r.log.V(4).Info("%s/%s istiocsr resource contains processing rejected annotation", istiocsr.Namespace, istiocsr.Name) + r.log.V(4).Info("istiocsr resource contains processing rejected annotation", "namespace", istiocsr.Namespace, "name", istiocsr.Name) // ensure status is updated. var updateErr error - if istiocsr.Status.SetCondition(v1alpha1.Ready, metav1.ConditionFalse, v1alpha1.ReasonFailed, statusMessage) { + degradedChanged := istiocsr.Status.SetCondition(v1alpha1.Degraded, metav1.ConditionFalse, v1alpha1.ReasonMultipleInstancesFound, "") + readyChanged := istiocsr.Status.SetCondition(v1alpha1.Ready, metav1.ConditionFalse, v1alpha1.ReasonMultipleInstancesFound, statusMessage) + progressingChanged := istiocsr.Status.SetCondition(v1alpha1.Progressing, metav1.ConditionFalse, v1alpha1.ReasonMultipleInstancesFound, statusMessage) + if degradedChanged || readyChanged || progressingChanged { updateErr = r.updateCondition(istiocsr, nil) } return common.NewMultipleInstanceError(utilerrors.NewAggregate([]error{errors.New(statusMessage), updateErr})) @@ -533,7 +536,10 @@ func (r *Reconciler) disallowMultipleIstioCSRInstances(istiocsr *v1alpha1.IstioC // This instance should be rejected as there's an older or equally old instance var condUpdateErr, annUpdateErr error - if istiocsr.Status.SetCondition(v1alpha1.Ready, metav1.ConditionFalse, v1alpha1.ReasonFailed, statusMessage) { + degradedChanged := istiocsr.Status.SetCondition(v1alpha1.Degraded, metav1.ConditionFalse, v1alpha1.ReasonMultipleInstancesFound, "") + readyChanged := istiocsr.Status.SetCondition(v1alpha1.Ready, metav1.ConditionFalse, v1alpha1.ReasonMultipleInstancesFound, statusMessage) + progressingChanged := istiocsr.Status.SetCondition(v1alpha1.Progressing, metav1.ConditionFalse, v1alpha1.ReasonMultipleInstancesFound, statusMessage) + if degradedChanged || readyChanged || progressingChanged { condUpdateErr = r.updateCondition(istiocsr, nil) } if addProcessingRejectedAnnotation(istiocsr) { diff --git a/pkg/controller/trustmanager/controller_test.go b/pkg/controller/trustmanager/controller_test.go index 3efd817a0..90d5b79d7 100644 --- a/pkg/controller/trustmanager/controller_test.go +++ b/pkg/controller/trustmanager/controller_test.go @@ -181,6 +181,11 @@ func TestProcessReconcileRequest(t *testing.T) { Reason: v1alpha1.ReasonReady, Message: "reconciliation successful", }, + { + Type: v1alpha1.Progressing, + Status: metav1.ConditionFalse, + Reason: v1alpha1.ReasonReady, + }, }, }, { @@ -207,12 +212,17 @@ func TestProcessReconcileRequest(t *testing.T) { { Type: v1alpha1.Degraded, Status: metav1.ConditionTrue, - Reason: v1alpha1.ReasonFailed, + Reason: v1alpha1.ReasonValidationFailed, }, { Type: v1alpha1.Ready, Status: metav1.ConditionFalse, - Reason: v1alpha1.ReasonFailed, + Reason: v1alpha1.ReasonValidationFailed, + }, + { + Type: v1alpha1.Progressing, + Status: metav1.ConditionFalse, + Reason: v1alpha1.ReasonValidationFailed, }, }, }, @@ -250,6 +260,11 @@ func TestProcessReconcileRequest(t *testing.T) { Status: metav1.ConditionFalse, Reason: v1alpha1.ReasonInProgress, }, + { + Type: v1alpha1.Progressing, + Status: metav1.ConditionTrue, + Reason: v1alpha1.ReasonReconciling, + }, }, wantErr: "failed to check if serviceaccount", }, @@ -279,7 +294,7 @@ func TestProcessReconcileRequest(t *testing.T) { { Type: v1alpha1.Degraded, Status: metav1.ConditionTrue, - Reason: v1alpha1.ReasonFailed, + Reason: v1alpha1.ReasonWaitingForDependencies, Message: fmt.Sprintf( "reconciliation failed with irrecoverable error not retrying: trust namespace %q validation failed: trust namespace %q does not exist, create the namespace before creating TrustManager CR", defaultTrustNamespace, @@ -289,7 +304,12 @@ func TestProcessReconcileRequest(t *testing.T) { { Type: v1alpha1.Ready, Status: metav1.ConditionFalse, - Reason: v1alpha1.ReasonFailed, + Reason: v1alpha1.ReasonWaitingForDependencies, + }, + { + Type: v1alpha1.Progressing, + Status: metav1.ConditionFalse, + Reason: v1alpha1.ReasonWaitingForDependencies, }, }, }, @@ -331,6 +351,11 @@ func TestProcessReconcileRequest(t *testing.T) { Reason: v1alpha1.ReasonReady, Message: "reconciliation successful", }, + { + Type: v1alpha1.Progressing, + Status: metav1.ConditionFalse, + Reason: v1alpha1.ReasonReady, + }, }, }, } diff --git a/pkg/controller/trustmanager/install_trustmanager.go b/pkg/controller/trustmanager/install_trustmanager.go index 932253355..a7cc8f66c 100644 --- a/pkg/controller/trustmanager/install_trustmanager.go +++ b/pkg/controller/trustmanager/install_trustmanager.go @@ -10,7 +10,8 @@ import ( func (r *Reconciler) reconcileTrustManagerDeployment(trustManager *v1alpha1.TrustManager) error { if err := validateTrustManagerConfig(trustManager); err != nil { - return common.NewIrrecoverableError(err, "%s configuration validation failed", trustManager.GetName()) + return common.NewIrrecoverableError(err, "%s configuration validation failed", trustManager.GetName()). + WithConditionReason(v1alpha1.ReasonValidationFailed) } resourceLabels := getResourceLabels(trustManager) @@ -18,7 +19,8 @@ func (r *Reconciler) reconcileTrustManagerDeployment(trustManager *v1alpha1.Trus trustNamespace := getTrustNamespace(trustManager) if err := r.validateTrustNamespace(trustNamespace); err != nil { - return common.NewIrrecoverableError(err, "trust namespace %q validation failed", trustNamespace) + return common.NewIrrecoverableError(err, "trust namespace %q validation failed", trustNamespace). + WithConditionReason(v1alpha1.ReasonWaitingForDependencies) } caBundleHash, err := r.createOrApplyDefaultCAPackageConfigMap(trustManager, resourceLabels, resourceAnnotations) diff --git a/test/e2e/trustmanager_test.go b/test/e2e/trustmanager_test.go index 41369a242..6fefc6355 100644 --- a/test/e2e/trustmanager_test.go +++ b/test/e2e/trustmanager_test.go @@ -1315,7 +1315,7 @@ var _ = Describe("TrustManager", Ordered, Label("Platform:Generic", "Feature:Tru degradedCondition := meta.FindStatusCondition(tm.Status.Conditions, v1alpha1.Degraded) g.Expect(degradedCondition).ShouldNot(BeNil()) g.Expect(degradedCondition.Status).Should(Equal(metav1.ConditionTrue)) - g.Expect(degradedCondition.Reason).Should(Equal(v1alpha1.ReasonFailed)) + g.Expect(degradedCondition.Reason).Should(Equal(v1alpha1.ReasonWaitingForDependencies)) g.Expect(degradedCondition.Message).Should(And( ContainSubstring("trust namespace"), ContainSubstring(nonExistentNS), @@ -1325,10 +1325,12 @@ var _ = Describe("TrustManager", Ordered, Label("Platform:Generic", "Feature:Tru readyCondition := meta.FindStatusCondition(tm.Status.Conditions, v1alpha1.Ready) g.Expect(readyCondition).ShouldNot(BeNil()) g.Expect(readyCondition.Status).Should(Equal(metav1.ConditionFalse)) - g.Expect(readyCondition.Reason).Should(Equal(v1alpha1.ReasonFailed)) - // Irrecoverable path: Ready message is left empty; detail is on Degraded only - // (see HandleReconcileResult for IsIrrecoverableError). + g.Expect(readyCondition.Reason).Should(Equal(v1alpha1.ReasonWaitingForDependencies)) g.Expect(readyCondition.Message).Should(BeEmpty()) + + progressingCondition := meta.FindStatusCondition(tm.Status.Conditions, v1alpha1.Progressing) + g.Expect(progressingCondition).ShouldNot(BeNil()) + g.Expect(progressingCondition.Status).Should(Equal(metav1.ConditionFalse)) }, lowTimeout, fastPollInterval).Should(Succeed()) // Irrecoverable errors are not requeued, and Namespace is not watched, so creating the