✨ refactor ClusterExtensionReconciler.reconcile() (operator-framework#1068)

* update reconciler with logical interface abstractions to simply unit testing

by moving bundle validation logic to the resolver, moving installation
of content behind a new Applier interface, and using the new
contentmanager.Watcher interface for watching managed objects

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

* update testing

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

* update testing

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

* remove bundle validation unit tests from controller package

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

* make linter happy

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

* remove postrenderer, replace with map[string]string for labels

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

* update debugging error string

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

* rebase fixes

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

---------

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

Author: everettraven

cmd/manager/main.go

@@ -47,6 +47,7 @@ import (
 
 	ocv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1"
 	"github.com/operator-framework/operator-controller/internal/action"
+	"github.com/operator-framework/operator-controller/internal/applier"
 	"github.com/operator-framework/operator-controller/internal/authentication"
 	"github.com/operator-framework/operator-controller/internal/catalogmetadata/cache"
 	catalogclient "github.com/operator-framework/operator-controller/internal/catalogmetadata/client"
@@ -191,6 +192,7 @@ func main() {
 		}
 		return tempConfig, nil
 	}
+
 	cfgGetter, err := helmclient.NewActionConfigGetter(mgr.GetConfig(), mgr.GetRESTMapper(),
 		helmclient.StorageNamespaceMapper(installNamespaceMapper),
 		helmclient.ClientNamespaceMapper(installNamespaceMapper),
@@ -232,16 +234,6 @@ func main() {
 		os.Exit(1)
 	}
 
-	aeClient, err := apiextensionsv1client.NewForConfig(mgr.GetConfig())
-	if err != nil {
-		setupLog.Error(err, "unable to create apiextensions client")
-		os.Exit(1)
-	}
-
-	preflights := []controllers.Preflight{
-		crdupgradesafety.NewPreflight(aeClient.CustomResourceDefinitions()),
-	}
-
 	cl := mgr.GetClient()
 
 	catalogsCachePath := filepath.Join(cachePath, "catalogs")
@@ -264,16 +256,33 @@ func main() {
 			},
 			catalogClient.GetPackage,
 		),
+		Validations: []resolve.ValidationFunc{
+			resolve.NoDependencyValidation,
+		},
+	}
+
+	aeClient, err := apiextensionsv1client.NewForConfig(mgr.GetConfig())
+	if err != nil {
+		setupLog.Error(err, "unable to create apiextensions client")
+		os.Exit(1)
+	}
+
+	preflights := []applier.Preflight{
+		crdupgradesafety.NewPreflight(aeClient.CustomResourceDefinitions()),
+	}
+
+	applier := &applier.Helm{
+		ActionClientGetter: acg,
+		Preflights:         preflights,
 	}
 
 	if err = (&controllers.ClusterExtensionReconciler{
 		Client:                cl,
 		Resolver:              resolver,
-		ActionClientGetter:    acg,
 		Unpacker:              unpacker,
+		Applier:               applier,
 		InstalledBundleGetter: &controllers.DefaultInstalledBundleGetter{ActionClientGetter: acg},
 		Finalizers:            clusterExtensionFinalizers,
-		Preflights:            preflights,
 		Watcher:               contentmanager.New(restConfigMapper, mgr.GetConfig(), mgr.GetRESTMapper()),
 	}).SetupWithManager(mgr); err != nil {
 		setupLog.Error(err, "unable to create controller", "controller", "ClusterExtension")

internal/applier/helm.go

@@ -0,0 +1,166 @@
+package applier
+
+import (
+	"context"
+	"errors"
+	"fmt"
+	"io/fs"
+	"strings"
+
+	"helm.sh/helm/v3/pkg/action"
+	"helm.sh/helm/v3/pkg/chart"
+	"helm.sh/helm/v3/pkg/chartutil"
+	"helm.sh/helm/v3/pkg/release"
+	"helm.sh/helm/v3/pkg/storage/driver"
+	corev1 "k8s.io/api/core/v1"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client"
+
+	ocv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1"
+	"github.com/operator-framework/operator-controller/internal/rukpak/convert"
+	"github.com/operator-framework/operator-controller/internal/rukpak/preflights/crdupgradesafety"
+	"github.com/operator-framework/operator-controller/internal/rukpak/util"
+)
+
+const (
+	StateNeedsInstall     string = "NeedsInstall"
+	StateNeedsUpgrade     string = "NeedsUpgrade"
+	StateUnchanged        string = "Unchanged"
+	StateError            string = "Error"
+	maxHelmReleaseHistory        = 10
+)
+
+// Preflight is a check that should be run before making any changes to the cluster
+type Preflight interface {
+	// Install runs checks that should be successful prior
+	// to installing the Helm release. It is provided
+	// a Helm release and returns an error if the
+	// check is unsuccessful
+	Install(context.Context, *release.Release) error
+
+	// Upgrade runs checks that should be successful prior
+	// to upgrading the Helm release. It is provided
+	// a Helm release and returns an error if the
+	// check is unsuccessful
+	Upgrade(context.Context, *release.Release) error
+}
+
+type Helm struct {
+	ActionClientGetter helmclient.ActionClientGetter
+	Preflights         []Preflight
+}
+
+func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1alpha1.ClusterExtension, labels map[string]string) ([]client.Object, string, error) {
+	chrt, err := convert.RegistryV1ToHelmChart(ctx, contentFS, ext.Spec.InstallNamespace, []string{corev1.NamespaceAll})
+	if err != nil {
+		return nil, "", err
+	}
+	values := chartutil.Values{}
+
+	ac, err := h.ActionClientGetter.ActionClientFor(ctx, ext)
+	if err != nil {
+		return nil, "", err
+	}
+
+	rel, desiredRel, state, err := h.getReleaseState(ac, ext, chrt, values, labels)
+	if err != nil {
+		return nil, "", err
+	}
+
+	for _, preflight := range h.Preflights {
+		if ext.Spec.Preflight != nil && ext.Spec.Preflight.CRDUpgradeSafety != nil {
+			if _, ok := preflight.(*crdupgradesafety.Preflight); ok && ext.Spec.Preflight.CRDUpgradeSafety.Disabled {
+				// Skip this preflight check because it is of type *crdupgradesafety.Preflight and the CRD Upgrade Safety
+				// preflight check has been disabled
+				continue
+			}
+		}
+		switch state {
+		case StateNeedsInstall:
+			err := preflight.Install(ctx, desiredRel)
+			if err != nil {
+				return nil, state, err
+			}
+		case StateNeedsUpgrade:
+			err := preflight.Upgrade(ctx, desiredRel)
+			if err != nil {
+				return nil, state, err
+			}
+		}
+	}
+
+	switch state {
+	case StateNeedsInstall:
+		rel, err = ac.Install(ext.GetName(), ext.Spec.InstallNamespace, chrt, values, func(install *action.Install) error {
+			install.CreateNamespace = false
+			install.Labels = labels
+			return nil
+		})
+		if err != nil {
+			return nil, state, err
+		}
+	case StateNeedsUpgrade:
+		rel, err = ac.Upgrade(ext.GetName(), ext.Spec.InstallNamespace, chrt, values, func(upgrade *action.Upgrade) error {
+			upgrade.MaxHistory = maxHelmReleaseHistory
+			upgrade.Labels = labels
+			return nil
+		})
+		if err != nil {
+			return nil, state, err
+		}
+	case StateUnchanged:
+		if err := ac.Reconcile(rel); err != nil {
+			return nil, state, err
+		}
+	default:
+		return nil, state, fmt.Errorf("unexpected release state %q", state)
+	}
+
+	relObjects, err := util.ManifestObjects(strings.NewReader(rel.Manifest), fmt.Sprintf("%s-release-manifest", rel.Name))
+	if err != nil {
+		return nil, state, err
+	}
+
+	return relObjects, state, nil
+}
+
+func (h *Helm) getReleaseState(cl helmclient.ActionInterface, ext *ocv1alpha1.ClusterExtension, chrt *chart.Chart, values chartutil.Values, labels map[string]string) (*release.Release, *release.Release, string, error) {
+	currentRelease, err := cl.Get(ext.GetName())
+	if err != nil && !errors.Is(err, driver.ErrReleaseNotFound) {
+		return nil, nil, StateError, err
+	}
+	if errors.Is(err, driver.ErrReleaseNotFound) {
+		return nil, nil, StateNeedsInstall, nil
+	}
+
+	if errors.Is(err, driver.ErrReleaseNotFound) {
+		desiredRelease, err := cl.Install(ext.GetName(), ext.Spec.InstallNamespace, chrt, values, func(i *action.Install) error {
+			i.DryRun = true
+			i.DryRunOption = "server"
+			i.Labels = labels
+			return nil
+		})
+		if err != nil {
+			return nil, nil, StateError, err
+		}
+		return nil, desiredRelease, StateNeedsInstall, nil
+	}
+	desiredRelease, err := cl.Upgrade(ext.GetName(), ext.Spec.InstallNamespace, chrt, values, func(upgrade *action.Upgrade) error {
+		upgrade.MaxHistory = maxHelmReleaseHistory
+		upgrade.DryRun = true
+		upgrade.DryRunOption = "server"
+		upgrade.Labels = labels
+		return nil
+	})
+	if err != nil {
+		return currentRelease, nil, StateError, err
+	}
+	relState := StateUnchanged
+	if desiredRelease.Manifest != currentRelease.Manifest ||
+		currentRelease.Info.Status == release.StatusFailed ||
+		currentRelease.Info.Status == release.StatusSuperseded {
+		relState = StateNeedsUpgrade
+	}
+	return currentRelease, desiredRelease, relState, nil
+}

internal/catalogmetadata/filter/successors.go

@@ -20,17 +20,17 @@ func SuccessorsOf(installedBundle *ocv1alpha1.BundleMetadata, channels ...declcf
 
 	installedBundleVersion, err := mmsemver.NewVersion(installedBundle.Version)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("parsing installed bundle %q version %q: %w", installedBundle.Name, installedBundle.Version, err)
 	}
 
 	installedVersionConstraint, err := mmsemver.NewConstraint(installedBundleVersion.String())
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("parsing installed version constraint %q: %w", installedBundleVersion.String(), err)
 	}
 
 	successorsPredicate, err := successors(installedBundle, channels...)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("getting successorsPredicate: %w", err)
 	}
 
 	// We need either successors or current version (no upgrade)