🐛 CRD Upgrade Safety pre-flight fixes (#1104)

* (bugfix): crd preflight passes on old crd not found

crd preflight updated to run on both install and upgrade
e2e test added to ensure fixing error in an initial install
failure results in successful install (or upgrade in
helm semantics)

Signed-off-by: everettraven <[email protected]>

* address reviews

Signed-off-by: everettraven <[email protected]>

* add comment regarding eating errors

Signed-off-by: everettraven <[email protected]>

* sanity

Signed-off-by: everettraven <[email protected]>

* sanity pt.2

Signed-off-by: everettraven <[email protected]>

* address comment about comments

Signed-off-by: everettraven <[email protected]>

* fix lint error

Signed-off-by: everettraven <[email protected]>

---------

Signed-off-by: everettraven <[email protected]>

Author: everettraven

internal/controllers/clusterextension_controller.go

@@ -271,6 +271,15 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
 	}
 
 	l.V(1).Info("applying bundle contents")
+	// NOTE: We need to be cautious of eating errors here.
+	// We should always return any error that occurs during an
+	// attempt to apply content to the cluster. Only when there is
+	// a verifiable reason to eat the error (i.e it is recoverable)
+	// should an exception be made.
+	// The following kinds of errors should be returned up the stack
+	// to ensure exponential backoff can occur:
+	//   - Permission errors (it is not possible to watch changes to permissions.
+	//     The only way to eventually recover from permission errors is to keep retrying).
 	managedObjs, state, err := r.Applier.Apply(ctx, unpackResult.Bundle, ext, lbls)
 	if err != nil {
 		setInstalledStatusConditionFailed(ext, err.Error())

internal/rukpak/preflights/crdupgradesafety/crdupgradesafety.go

@@ -10,6 +10,7 @@ import (
 	"helm.sh/helm/v3/pkg/release"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	apiextensionsv1client "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/typed/apiextensions/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 
@@ -64,11 +65,15 @@ func NewPreflight(crdCli apiextensionsv1client.CustomResourceDefinitionInterface
 	return p
 }
 
-func (p *Preflight) Install(_ context.Context, _ *release.Release) error {
-	return nil
+func (p *Preflight) Install(ctx context.Context, rel *release.Release) error {
+	return p.runPreflight(ctx, rel)
 }
 
 func (p *Preflight) Upgrade(ctx context.Context, rel *release.Release) error {
+	return p.runPreflight(ctx, rel)
+}
+
+func (p *Preflight) runPreflight(ctx context.Context, rel *release.Release) error {
 	if rel == nil {
 		return nil
 	}
@@ -96,6 +101,11 @@ func (p *Preflight) Upgrade(ctx context.Context, rel *release.Release) error {
 
 		oldCrd, err := p.crdClient.Get(ctx, newCrd.Name, metav1.GetOptions{})
 		if err != nil {
+			// if there is no existing CRD, there is nothing to break
+			// so it is immediately successful.
+			if apierrors.IsNotFound(err) {
+				continue
+			}
 			return fmt.Errorf("getting existing resource for CRD %q: %w", newCrd.Name, err)
 		}
 

internal/rukpak/preflights/crdupgradesafety/crdupgradesafety_test.go

@@ -12,8 +12,10 @@ import (
 	"helm.sh/helm/v3/pkg/release"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	apiextensionsv1client "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/typed/apiextensions/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 
 	"github.com/operator-framework/operator-controller/internal/rukpak/preflights/crdupgradesafety"
 	"github.com/operator-framework/operator-controller/internal/rukpak/util"
@@ -71,8 +73,140 @@ func getManifestString(t *testing.T, crdFile string) string {
 // TestInstall exists only for completeness as Install() is currently a no-op. It can be used as
 // a template for real tests in the future if the func is implemented.
 func TestInstall(t *testing.T) {
-	preflight := newMockPreflight(nil, nil, nil)
-	require.Nil(t, preflight.Install(context.Background(), nil))
+	tests := []struct {
+		name          string
+		oldCrdPath    string
+		validator     *kappcus.Validator
+		release       *release.Release
+		wantErrMsgs   []string
+		wantCrdGetErr error
+	}{
+		{
+			name: "nil release",
+		},
+		{
+			name: "release with no objects",
+			release: &release.Release{
+				Name: "test-release",
+			},
+		},
+		{
+			name: "release with invalid manifest",
+			release: &release.Release{
+				Name:     "test-release",
+				Manifest: "abcd",
+			},
+			wantErrMsgs: []string{"json: cannot unmarshal string into Go value of type unstructured.detector"},
+		},
+		{
+			name: "release with no CRD objects",
+			release: &release.Release{
+				Name:     "test-release",
+				Manifest: getManifestString(t, "no-crds.json"),
+			},
+		},
+		{
+			name: "fail to get old crd other than not found error",
+			release: &release.Release{
+				Name:     "test-release",
+				Manifest: getManifestString(t, "crd-valid-upgrade.json"),
+			},
+			wantCrdGetErr: fmt.Errorf("error!"),
+			wantErrMsgs:   []string{"error!"},
+		},
+		{
+			name: "fail to get old crd, not found error",
+			release: &release.Release{
+				Name:     "test-release",
+				Manifest: getManifestString(t, "crd-valid-upgrade.json"),
+			},
+			wantCrdGetErr: apierrors.NewNotFound(schema.GroupResource{Group: apiextensionsv1.SchemeGroupVersion.Group, Resource: "customresourcedefinitions"}, "not found"),
+		},
+		{
+			name: "invalid crd manifest file",
+			release: &release.Release{
+				Name:     "test-release",
+				Manifest: getManifestString(t, "crd-invalid"),
+			},
+			wantErrMsgs: []string{"json: cannot unmarshal"},
+		},
+		{
+			name:       "custom validator",
+			oldCrdPath: "old-crd.json",
+			release: &release.Release{
+				Name:     "test-release",
+				Manifest: getManifestString(t, "old-crd.json"),
+			},
+			validator: &kappcus.Validator{
+				Validations: []kappcus.Validation{
+					kappcus.NewValidationFunc("test", func(old, new apiextensionsv1.CustomResourceDefinition) error {
+						return fmt.Errorf("custom validation error!!")
+					}),
+				},
+			},
+			wantErrMsgs: []string{"custom validation error!!"},
+		},
+		{
+			name:       "valid upgrade",
+			oldCrdPath: "old-crd.json",
+			release: &release.Release{
+				Name:     "test-release",
+				Manifest: getManifestString(t, "crd-valid-upgrade.json"),
+			},
+		},
+		{
+			name: "new crd validation failures (all except existing field removal)",
+			// Not really intended to test kapp validators, although it does anyway to a large extent.
+			// This test is primarily meant to ensure that we are actually using all of them.
+			oldCrdPath: "old-crd.json",
+			release: &release.Release{
+				Name:     "test-release",
+				Manifest: getManifestString(t, "crd-invalid-upgrade.json"),
+			},
+			wantErrMsgs: []string{
+				`"NoScopeChange"`,
+				`"NoStoredVersionRemoved"`,
+				`enums added`,
+				`new required fields added`,
+				`maximum constraint added when one did not exist previously`,
+				`maximum items constraint added`,
+				`maximum length constraint added`,
+				`maximum properties constraint added`,
+				`minimum constraint added when one did not exist previously`,
+				`minimum items constraint added`,
+				`minimum length constraint added`,
+				`minimum properties constraint added`,
+				`new value added as default`,
+			},
+		},
+		{
+			name: "new crd validation failure for existing field removal",
+			// Separate test from above as this error will cause the validator to
+			// return early and skip some of the above validations.
+			oldCrdPath: "old-crd.json",
+			release: &release.Release{
+				Name:     "test-release",
+				Manifest: getManifestString(t, "crd-field-removed.json"),
+			},
+			wantErrMsgs: []string{
+				`"NoExistingFieldRemoved"`,
+			},
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			preflight := newMockPreflight(getCrdFromManifestFile(t, tc.oldCrdPath), tc.wantCrdGetErr, tc.validator)
+			err := preflight.Install(context.Background(), tc.release)
+			if len(tc.wantErrMsgs) != 0 {
+				for _, expectedErrMsg := range tc.wantErrMsgs {
+					require.ErrorContainsf(t, err, expectedErrMsg, "")
+				}
+			} else {
+				require.NoError(t, err)
+			}
+		})
+	}
 }
 
 func TestUpgrade(t *testing.T) {
@@ -109,14 +243,22 @@ func TestUpgrade(t *testing.T) {
 			},
 		},
 		{
-			name: "fail to get old crd",
+			name: "fail to get old crd, other than not found error",
 			release: &release.Release{
 				Name:     "test-release",
 				Manifest: getManifestString(t, "crd-valid-upgrade.json"),
 			},
 			wantCrdGetErr: fmt.Errorf("error!"),
 			wantErrMsgs:   []string{"error!"},
 		},
+		{
+			name: "fail to get old crd, not found error",
+			release: &release.Release{
+				Name:     "test-release",
+				Manifest: getManifestString(t, "crd-valid-upgrade.json"),
+			},
+			wantCrdGetErr: apierrors.NewNotFound(schema.GroupResource{Group: apiextensionsv1.SchemeGroupVersion.Group, Resource: "customresourcedefinitions"}, "not found"),
+		},
 		{
 			name: "invalid crd manifest file",
 			release: &release.Release{

test/e2e/cluster_extension_install_test.go

@@ -50,9 +50,14 @@ func createServiceAccount(ctx context.Context, name types.NamespacedName, cluste
 	if err != nil {
 		return nil, err
 	}
+
+	return sa, createClusterRoleAndBindingForSA(ctx, name.Name, sa, clusterExtensionName)
+}
+
+func createClusterRoleAndBindingForSA(ctx context.Context, name string, sa *corev1.ServiceAccount, clusterExtensionName string) error {
 	cr := &rbacv1.ClusterRole{
 		ObjectMeta: metav1.ObjectMeta{
-			Name: name.Name,
+			Name: name,
 		},
 		Rules: []rbacv1.PolicyRule{
 			{
@@ -144,33 +149,33 @@ func createServiceAccount(ctx context.Context, name types.NamespacedName, cluste
 			},
 		},
 	}
-	err = c.Create(ctx, cr)
+	err := c.Create(ctx, cr)
 	if err != nil {
-		return nil, err
+		return err
 	}
 	crb := &rbacv1.ClusterRoleBinding{
 		ObjectMeta: metav1.ObjectMeta{
-			Name: name.Name,
+			Name: name,
 		},
 		Subjects: []rbacv1.Subject{
 			{
 				Kind:      "ServiceAccount",
-				Name:      name.Name,
-				Namespace: name.Namespace,
+				Name:      sa.Name,
+				Namespace: sa.Namespace,
 			},
 		},
 		RoleRef: rbacv1.RoleRef{
 			APIGroup: "rbac.authorization.k8s.io",
 			Kind:     "ClusterRole",
-			Name:     name.Name,
+			Name:     name,
 		},
 	}
 	err = c.Create(ctx, crb)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
-	return sa, nil
+	return nil
 }
 
 func testInit(t *testing.T) (*ocv1alpha1.ClusterExtension, *catalogd.ClusterCatalog, *corev1.ServiceAccount) {
@@ -605,6 +610,94 @@ func TestClusterExtensionInstallReResolvesWhenManagedContentChanged(t *testing.T
 	}, pollDuration, pollInterval)
 }
 
+func TestClusterExtensionRecoversFromInitialInstallFailedWhenFailureFixed(t *testing.T) {
+	t.Log("When a cluster extension is installed from a catalog")
+	t.Log("When the extension bundle format is registry+v1")
+
+	clusterExtension, extensionCatalog, _ := testInit(t)
+	name := rand.String(10)
+	sa := &corev1.ServiceAccount{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      name,
+			Namespace: "default",
+		},
+	}
+	err := c.Create(context.Background(), sa)
+	require.NoError(t, err)
+
+	defer testCleanup(t, extensionCatalog, clusterExtension, sa)
+	defer getArtifactsOutput(t)
+
+	clusterExtension.Spec = ocv1alpha1.ClusterExtensionSpec{
+		PackageName:      "prometheus",
+		InstallNamespace: "default",
+		ServiceAccount: ocv1alpha1.ServiceAccountReference{
+			Name: sa.Name,
+		},
+	}
+	t.Log("It resolves the specified package with correct bundle path")
+	t.Log("By creating the ClusterExtension resource")
+	require.NoError(t, c.Create(context.Background(), clusterExtension))
+
+	t.Log("By eventually reporting a successful resolution and bundle path")
+	require.EventuallyWithT(t, func(ct *assert.CollectT) {
+		assert.NoError(ct, c.Get(context.Background(), types.NamespacedName{Name: clusterExtension.Name}, clusterExtension))
+		assert.Len(ct, clusterExtension.Status.Conditions, len(conditionsets.ConditionTypes))
+		cond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeResolved)
+		if !assert.NotNil(ct, cond) {
+			return
+		}
+		assert.Equal(ct, metav1.ConditionTrue, cond.Status)
+		assert.Equal(ct, ocv1alpha1.ReasonSuccess, cond.Reason)
+		assert.Contains(ct, cond.Message, "resolved to")
+		assert.Equal(ct, &ocv1alpha1.BundleMetadata{Name: "prometheus-operator.1.2.0", Version: "1.2.0"}, clusterExtension.Status.ResolvedBundle)
+	}, pollDuration, pollInterval)
+
+	t.Log("By eventually reporting a successful unpacked")
+	require.EventuallyWithT(t, func(ct *assert.CollectT) {
+		assert.NoError(ct, c.Get(context.Background(), types.NamespacedName{Name: clusterExtension.Name}, clusterExtension))
+		cond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeUnpacked)
+		if !assert.NotNil(ct, cond) {
+			return
+		}
+		assert.Equal(ct, metav1.ConditionTrue, cond.Status)
+		assert.Equal(ct, ocv1alpha1.ReasonUnpackSuccess, cond.Reason)
+		assert.Contains(ct, cond.Message, "unpack successful")
+	}, pollDuration, pollInterval)
+
+	t.Log("By eventually failing to install the package successfully due to insufficient ServiceAccount permissions")
+	require.EventuallyWithT(t, func(ct *assert.CollectT) {
+		assert.NoError(ct, c.Get(context.Background(), types.NamespacedName{Name: clusterExtension.Name}, clusterExtension))
+		cond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeInstalled)
+		if !assert.NotNil(ct, cond) {
+			return
+		}
+		assert.Equal(ct, metav1.ConditionFalse, cond.Status)
+		assert.Equal(ct, ocv1alpha1.ReasonInstallationFailed, cond.Reason)
+		assert.Contains(ct, cond.Message, "forbidden")
+	}, pollDuration, pollInterval)
+
+	t.Log("By fixing the ServiceAccount permissions")
+	require.NoError(t, createClusterRoleAndBindingForSA(context.Background(), name, sa, clusterExtension.Name))
+
+	// NOTE: In order to ensure predictable results we need to ensure we have a single
+	// known failure with a singular fix operation. Additionally, due to the exponential
+	// backoff of this eventually check we MUST ensure we do not touch the ClusterExtension
+	// after creating and binding the needed permissions to the ServiceAccount.
+	t.Log("By eventually installing the package successfully")
+	require.EventuallyWithT(t, func(ct *assert.CollectT) {
+		assert.NoError(ct, c.Get(context.Background(), types.NamespacedName{Name: clusterExtension.Name}, clusterExtension))
+		cond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeInstalled)
+		if !assert.NotNil(ct, cond) {
+			return
+		}
+		assert.Equal(ct, metav1.ConditionTrue, cond.Status)
+		assert.Equal(ct, ocv1alpha1.ReasonSuccess, cond.Reason)
+		assert.Contains(ct, cond.Message, "Installed bundle")
+		assert.NotEmpty(ct, clusterExtension.Status.InstalledBundle)
+	}, pollDuration, pollInterval)
+}
+
 // getArtifactsOutput gets all the artifacts from the test run and saves them to the artifact path.
 // Currently it saves:
 // - clusterextensions