From 8c8d1ae67f06f96de2546305a78635c8cc0cd7d5 Mon Sep 17 00:00:00 2001 From: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Date: Mon, 27 Jul 2026 13:29:44 -0700 Subject: [PATCH] fix: classify invalid strategy component errors as unrecoverable Motivation: When a CSV's install strategy contains a permanently malformed component (e.g. a Deployment with an invalid name), the Kubernetes API server rejects the create/update with an "Invalid" (422) error. The deployment installer returned this error unclassified, so install.IsErrorUnrecoverable() treated it as retryable. The CSV state machine in operator.go then cycled InstallReady -> Failed (CSVReasonComponentFailed, retryable) -> Pending (CSVReasonNeedsReinstall) -> InstallReady forever, hot-looping instead of settling in Failed. This was the root cause of the flakiness in the FailForward e2e test disabled by PR #3572. The effect is wasted reconciles, CSV phase oscillation, and event/log spam -- the CSV controller itself does not crash or otherwise affect other CSVs. Approach: - Add a classifyInstallError helper in pkg/controller/install/deployment.go that maps apierrors.IsInvalid (in addition to the existing apierrors.IsForbidden) to a StrategyError, and apply it to both the installCertRequirements and installDeployments error paths in Install(). - Add StrategyErrReasonComponentInvalid in pkg/controller/install/errors.go and register it in unrecoverableErrors, so IsErrorUnrecoverable() returns true and the CSV is correctly marked CSVReasonComponentFailedNoRetry, which is terminal (the existing top-of-transitionCSVState guard in operator.go keeps it in Failed rather than reinstalling). Validation: go build ./... go test ./pkg/controller/install/... ./pkg/controller/operators/olm/... Both pass, including a new regression test, TestInstallStrategyDeploymentInstallInvalidDeployment, which asserts that an apierrors.NewInvalid error from the deployment client causes Install() to return StrategyErrReasonComponentInvalid and that IsErrorUnrecoverable() reports it as unrecoverable. Note: the disabled e2e regression test in test/e2e/fail_forward_e2e_test.go (XWhen at line ~324, referencing this issue) requires a live cluster and was intentionally left disabled, as it cannot be validated in this environment. Fixes #3573 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> --- pkg/controller/install/deployment.go | 20 +++++++++++++----- pkg/controller/install/deployment_test.go | 25 +++++++++++++++++++++++ pkg/controller/install/errors.go | 2 ++ 3 files changed, 42 insertions(+), 5 deletions(-) diff --git a/pkg/controller/install/deployment.go b/pkg/controller/install/deployment.go index 714badb2ea..2cbf8ffb73 100644 --- a/pkg/controller/install/deployment.go +++ b/pkg/controller/install/deployment.go @@ -199,20 +199,30 @@ func (i *StrategyDeploymentInstaller) Install(s Strategy) error { // Install owned APIServices and update strategy with serving cert data updatedStrategy, err := i.installCertRequirements(strategy) if err != nil { - return err + return classifyInstallError(err) } if err := i.installDeployments(updatedStrategy.DeploymentSpecs); err != nil { - if apierrors.IsForbidden(err) { - return StrategyError{Reason: StrategyErrInsufficientPermissions, Message: fmt.Sprintf("install strategy failed: %s", err)} - } - return err + return classifyInstallError(err) } // Clean up orphaned deployments return i.cleanupOrphanedDeployments(updatedStrategy.DeploymentSpecs) } +// classifyInstallError maps errors encountered while creating or updating strategy +// components to a StrategyError so callers can distinguish unrecoverable failures +// (e.g. a permanently malformed or forbidden component) from ones worth retrying. +func classifyInstallError(err error) error { + if apierrors.IsForbidden(err) { + return StrategyError{Reason: StrategyErrInsufficientPermissions, Message: fmt.Sprintf("install strategy failed: %s", err)} + } + if apierrors.IsInvalid(err) { + return StrategyError{Reason: StrategyErrReasonComponentInvalid, Message: fmt.Sprintf("install strategy failed: %s", err)} + } + return err +} + // CheckInstalled can return nil (installed), or errors // Errors can indicate: some component missing (keep installing), unable to query (check again later), or unrecoverable (failed in a way we know we can't recover from) func (i *StrategyDeploymentInstaller) CheckInstalled(s Strategy) (installed bool, err error) { diff --git a/pkg/controller/install/deployment_test.go b/pkg/controller/install/deployment_test.go index 2a7c907248..0529448567 100644 --- a/pkg/controller/install/deployment_test.go +++ b/pkg/controller/install/deployment_test.go @@ -10,6 +10,7 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" k8slabels "k8s.io/apimachinery/pkg/labels" @@ -397,6 +398,30 @@ func TestInstallStrategyDeploymentCheckInstallErrors(t *testing.T) { } } +func TestInstallStrategyDeploymentInstallInvalidDeployment(t *testing.T) { + namespace := "olm-test-deployment" + mockOwner := v1alpha1.ClusterServiceVersion{ + TypeMeta: metav1.TypeMeta{ + Kind: v1alpha1.ClusterServiceVersionKind, + APIVersion: v1alpha1.ClusterServiceVersionAPIVersion, + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "clusterserviceversion-owner", + Namespace: namespace, + }, + } + + fakeClient := new(clientfakes.FakeInstallStrategyDeploymentInterface) + fakeClient.CreateOrUpdateDeploymentReturns(nil, apierrors.NewInvalid(appsv1.SchemeGroupVersion.WithKind("Deployment").GroupKind(), "Bad_Deployment_Name", nil)) + + installer := NewStrategyDeploymentInstaller(fakeClient, nil, &mockOwner, nil, nil, nil, nil) + + err := installer.Install(strategy(1, namespace, &mockOwner)) + require.Error(t, err) + require.Equal(t, StrategyErrReasonComponentInvalid, ReasonForError(err)) + require.True(t, IsErrorUnrecoverable(err), "an invalid deployment spec should be treated as an unrecoverable install error") +} + func TestInstallStrategyDeploymentCleanupDeployments(t *testing.T) { var ( mockOwner = v1alpha1.ClusterServiceVersion{ diff --git a/pkg/controller/install/errors.go b/pkg/controller/install/errors.go index f0067f3523..f287bd4c87 100644 --- a/pkg/controller/install/errors.go +++ b/pkg/controller/install/errors.go @@ -10,6 +10,7 @@ const ( StrategyErrBadPatch = "PatchUnsuccessful" StrategyErrDeploymentUpdated = "DeploymentUpdated" StrategyErrInsufficientPermissions = "InsufficentPermissions" + StrategyErrReasonComponentInvalid = "ComponentInvalid" ) // unrecoverableErrors are the set of errors that mean we can't recover an install strategy @@ -18,6 +19,7 @@ var unrecoverableErrors = map[string]struct{}{ StrategyErrReasonTimeout: {}, StrategyErrBadPatch: {}, StrategyErrInsufficientPermissions: {}, + StrategyErrReasonComponentInvalid: {}, } // StrategyError is used to represent error types for install strategies