fix: classify invalid strategy component errors as unrecoverable

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

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{

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