Use Helm List operator to determine Deployed status (#1379)

Signed-off-by: Todd Short <tshort@redhat.com>

Author: tmshort

go.mod

@@ -18,7 +18,7 @@ require (
 	github.com/opencontainers/go-digest v1.0.0
 	github.com/operator-framework/api v0.27.0
 	github.com/operator-framework/catalogd v0.35.0
-	github.com/operator-framework/helm-operator-plugins v0.5.0
+	github.com/operator-framework/helm-operator-plugins v0.7.0
 	github.com/operator-framework/operator-registry v1.47.0
 	github.com/spf13/pflag v1.0.5
 	github.com/stretchr/testify v1.9.0

go.sum

@@ -537,8 +537,8 @@ github.com/operator-framework/api v0.27.0 h1:OrVaGKZJvbZo58HTv2guz7aURkhVKYhFqZ/
 github.com/operator-framework/api v0.27.0/go.mod h1:lg2Xx+S8NQWGYlEOvFwQvH46E5EK5IrAIL7HWfAhciM=
 github.com/operator-framework/catalogd v0.35.0 h1:2lQPyHIzEfcciUjQ/fo4i/GE/sX2LBzd8S+nYxlvEHU=
 github.com/operator-framework/catalogd v0.35.0/go.mod h1:anZurjcFMBvbkuyqlJ98v9z+yjniPKqmhlyitk9DuBQ=
-github.com/operator-framework/helm-operator-plugins v0.5.0 h1:qph2OoECcI9mpuUBtOsWOMgvpx52mPTTSvzVxICsT04=
-github.com/operator-framework/helm-operator-plugins v0.5.0/go.mod h1:yVncrZ/FJNqedMil+055fk6sw8aMKRrget/AqGM0ig0=
+github.com/operator-framework/helm-operator-plugins v0.7.0 h1:YmtIWFc9BaNaDc5mk/dkG0P2BqPZOqpDvjWih5Fczuk=
+github.com/operator-framework/helm-operator-plugins v0.7.0/go.mod h1:fUUCJR3bWtMBZ1qdDhbwjacsBHi9uT576tF4u/DwOgQ=
 github.com/operator-framework/operator-lib v0.15.0 h1:0QeRM4PMtThqINpcFGCEBnIV3Z8u7/8fYLEx6mUtdcM=
 github.com/operator-framework/operator-lib v0.15.0/go.mod h1:ZxLvFuQ7bRWiTNBOqodbuNvcsy/Iq0kOygdxhlbNdI0=
 github.com/operator-framework/operator-registry v1.47.0 h1:Imr7X/W6FmXczwpIOXfnX8d6Snr1dzwWxkMG+lLAfhg=

internal/action/helm.go

@@ -75,6 +75,12 @@ func (a ActionClient) Get(name string, opts ...actionclient.GetOption) (*release
 	return resp, err
 }
 
+func (a ActionClient) History(name string, opts ...actionclient.HistoryOption) ([]*release.Release, error) {
+	resp, err := a.ActionInterface.History(name, opts...)
+	err = a.actionClientErrorTranslator(err)
+	return resp, err
+}
+
 func (a ActionClient) Reconcile(rel *release.Release) error {
 	return a.actionClientErrorTranslator(a.ActionInterface.Reconcile(rel))
 }

internal/action/helm_test.go

@@ -30,6 +30,17 @@ func (m *mockActionClient) Get(name string, opts ...actionclient.GetOption) (*re
 	return args.Get(0).(*release.Release), args.Error(1)
 }
 
+func (m *mockActionClient) History(name string, opts ...actionclient.HistoryOption) ([]*release.Release, error) {
+	args := m.Called(name, opts)
+	if args.Get(0) == nil {
+		return nil, args.Error(1)
+	}
+	rel := []*release.Release{
+		args.Get(0).(*release.Release),
+	}
+	return rel, args.Error(1)
+}
+
 func (m *mockActionClient) Install(name, namespace string, chrt *chart.Chart, vals map[string]interface{}, opts ...actionclient.InstallOption) (*release.Release, error) {
 	args := m.Called(name, namespace, chrt, vals, opts)
 	if args.Get(0) == nil {
@@ -82,6 +93,7 @@ func TestActionClientErrorTranslation(t *testing.T) {
 
 	ac := new(mockActionClient)
 	ac.On("Get", mock.Anything, mock.Anything).Return(nil, originalError)
+	ac.On("History", mock.Anything, mock.Anything).Return(nil, originalError)
 	ac.On("Install", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, originalError)
 	ac.On("Uninstall", mock.Anything, mock.Anything).Return(nil, originalError)
 	ac.On("Upgrade", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, originalError)
@@ -93,6 +105,10 @@ func TestActionClientErrorTranslation(t *testing.T) {
 	_, err := wrappedAc.Get("something")
 	assert.Equal(t, expectedErr, err, "expected Get() to return translated error")
 
+	// History
+	_, err = wrappedAc.History("something")
+	assert.Equal(t, expectedErr, err, "expected History() to return translated error")
+
 	// Install
 	_, err = wrappedAc.Install("something", "somethingelse", nil, nil)
 	assert.Equal(t, expectedErr, err, "expected Install() to return translated error")

internal/controllers/clusterextension_controller.go

@@ -84,7 +84,7 @@ type Applier interface {
 }
 
 type InstalledBundleGetter interface {
-	GetInstalledBundle(ctx context.Context, ext *ocv1alpha1.ClusterExtension) (*ocv1alpha1.BundleMetadata, error)
+	GetInstalledBundle(ctx context.Context, ext *ocv1alpha1.ClusterExtension) (*InstalledBundle, error)
 }
 
 //+kubebuilder:rbac:groups=olm.operatorframework.io,resources=clusterextensions,verbs=get;list;watch;update;patch
@@ -206,19 +206,22 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
 	installedBundle, err := r.InstalledBundleGetter.GetInstalledBundle(ctx, ext)
 	if err != nil {
 		setInstallStatus(ext, nil)
-		// TODO: use Installed=Unknown
-		setInstalledStatusConditionFailed(ext, err.Error())
-		setStatusProgressing(ext, err)
+		setInstalledStatusConditionUnknown(ext, err.Error())
+		setStatusProgressing(ext, errors.New("retrying to get installed bundle"))
 		return ctrl.Result{}, err
 	}
 
 	// run resolution
 	l.Info("resolving bundle")
-	resolvedBundle, resolvedBundleVersion, resolvedDeprecation, err := r.Resolver.Resolve(ctx, ext, installedBundle)
+	var bm *ocv1alpha1.BundleMetadata
+	if installedBundle != nil {
+		bm = &installedBundle.BundleMetadata
+	}
+	resolvedBundle, resolvedBundleVersion, resolvedDeprecation, err := r.Resolver.Resolve(ctx, ext, bm)
 	if err != nil {
 		// Note: We don't distinguish between resolution-specific errors and generic errors
-		setInstallStatus(ext, nil)
 		setStatusProgressing(ext, err)
+		setInstalledStatusFromBundle(ext, installedBundle)
 		ensureAllConditionsWithReason(ext, ocv1alpha1.ReasonFailed, err.Error())
 		return ctrl.Result{}, err
 	}
@@ -255,6 +258,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
 		// installed since we intend for the progressing condition to replace the resolved condition
 		// and will be removing the .status.resolution field from the ClusterExtension status API
 		setStatusProgressing(ext, wrapErrorWithResolutionInfo(resolvedBundleMetadata, err))
+		setInstalledStatusFromBundle(ext, installedBundle)
 		return ctrl.Result{}, err
 	}
 
@@ -268,9 +272,10 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
 	}
 
 	storeLbls := map[string]string{
-		labels.BundleNameKey:    resolvedBundle.Name,
-		labels.PackageNameKey:   resolvedBundle.Package,
-		labels.BundleVersionKey: resolvedBundleVersion.String(),
+		labels.BundleNameKey:      resolvedBundle.Name,
+		labels.PackageNameKey:     resolvedBundle.Package,
+		labels.BundleVersionKey:   resolvedBundleVersion.String(),
+		labels.BundleReferenceKey: resolvedBundle.Image,
 	}
 
 	l.Info("applying bundle contents")
@@ -286,18 +291,17 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
 	managedObjs, _, err := r.Applier.Apply(ctx, unpackResult.Bundle, ext, objLbls, storeLbls)
 	if err != nil {
 		setStatusProgressing(ext, wrapErrorWithResolutionInfo(resolvedBundleMetadata, err))
-		// If bundle is not already installed, set Installed status condition to False
-		if installedBundle == nil {
-			setInstalledStatusConditionFailed(ext, err.Error())
-		}
+		// Now that we're actually trying to install, use the error
+		setInstalledStatusFromBundle(ext, installedBundle)
 		return ctrl.Result{}, err
 	}
 
-	installStatus := &ocv1alpha1.ClusterExtensionInstallStatus{
-		Bundle: resolvedBundleMetadata,
+	newInstalledBundle := &InstalledBundle{
+		BundleMetadata: resolvedBundleMetadata,
+		Image:          resolvedBundle.Image,
 	}
-	setInstallStatus(ext, installStatus)
-	setInstalledStatusConditionSuccess(ext, fmt.Sprintf("Installed bundle %s successfully", resolvedBundle.Image))
+	// Successful install
+	setInstalledStatusFromBundle(ext, newInstalledBundle)
 
 	l.Info("watching managed objects")
 	cache, err := r.Manager.Get(ctx, ext)
@@ -466,32 +470,45 @@ type DefaultInstalledBundleGetter struct {
 	helmclient.ActionClientGetter
 }
 
-func (d *DefaultInstalledBundleGetter) GetInstalledBundle(ctx context.Context, ext *ocv1alpha1.ClusterExtension) (*ocv1alpha1.BundleMetadata, error) {
+type InstalledBundle struct {
+	ocv1alpha1.BundleMetadata
+	Image string
+}
+
+func (d *DefaultInstalledBundleGetter) GetInstalledBundle(ctx context.Context, ext *ocv1alpha1.ClusterExtension) (*InstalledBundle, error) {
 	cl, err := d.ActionClientFor(ctx, ext)
 	if err != nil {
 		return nil, err
 	}
 
-	rel, err := cl.Get(ext.GetName())
+	relhis, err := cl.History(ext.GetName())
 	if err != nil && !errors.Is(err, driver.ErrReleaseNotFound) {
 		return nil, err
 	}
-	if rel == nil {
+	if len(relhis) == 0 {
 		return nil, nil
 	}
 
-	switch rel.Info.Status {
-	case release.StatusUnknown:
-		return nil, fmt.Errorf("installation status is unknown")
-	case release.StatusDeployed, release.StatusUninstalled, release.StatusSuperseded, release.StatusFailed:
-	case release.StatusUninstalling, release.StatusPendingInstall, release.StatusPendingRollback, release.StatusPendingUpgrade:
-		return nil, fmt.Errorf("installation is still pending: %s", rel.Info.Status)
-	default:
-		return nil, fmt.Errorf("unknown installation status: %s", rel.Info.Status)
+	// relhis[0].Info.Status is the status of the most recent install attempt.
+	// But we need to look for the most-recent _Deployed_ release
+	for _, rel := range relhis {
+		if rel.Info != nil && rel.Info.Status == release.StatusDeployed {
+			// If there are blank values, we should consider this as not installed
+			if n, ok := rel.Labels[labels.BundleNameKey]; !ok || n == "" {
+				return nil, nil
+			}
+			if v, ok := rel.Labels[labels.BundleVersionKey]; !ok || v == "" {
+				return nil, nil
+			}
+			// Not checking BundleReferenceKey, as it's new; upgrade test would fail
+			return &InstalledBundle{
+				BundleMetadata: ocv1alpha1.BundleMetadata{
+					Name:    rel.Labels[labels.BundleNameKey],
+					Version: rel.Labels[labels.BundleVersionKey],
+				},
+				Image: rel.Labels[labels.BundleReferenceKey],
+			}, nil
+		}
 	}
-
-	return &ocv1alpha1.BundleMetadata{
-		Name:    rel.Labels[labels.BundleNameKey],
-		Version: rel.Labels[labels.BundleVersionKey],
-	}, nil
+	return nil, nil
 }

internal/controllers/clusterextension_controller_test.go

@@ -12,6 +12,9 @@ import (
 	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"helm.sh/helm/v3/pkg/chart"
+	"helm.sh/helm/v3/pkg/release"
+	"helm.sh/helm/v3/pkg/storage/driver"
 	apimeta "k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
@@ -21,12 +24,14 @@ import (
 	crfinalizer "sigs.k8s.io/controller-runtime/pkg/finalizer"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
+	helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client"
 	"github.com/operator-framework/operator-registry/alpha/declcfg"
 
 	ocv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1"
 	"github.com/operator-framework/operator-controller/internal/conditionsets"
 	"github.com/operator-framework/operator-controller/internal/controllers"
 	"github.com/operator-framework/operator-controller/internal/finalizers"
+	"github.com/operator-framework/operator-controller/internal/labels"
 	"github.com/operator-framework/operator-controller/internal/resolve"
 	"github.com/operator-framework/operator-controller/internal/rukpak/source"
 )
@@ -392,7 +397,10 @@ func TestClusterExtensionApplierFailsWithBundleInstalled(t *testing.T) {
 		cache: &MockManagedContentCache{},
 	}
 	reconciler.InstalledBundleGetter = &MockInstalledBundleGetter{
-		bundle: &ocv1alpha1.BundleMetadata{Name: "prometheus.v1.0.0", Version: "1.0.0"},
+		bundle: &controllers.InstalledBundle{
+			BundleMetadata: ocv1alpha1.BundleMetadata{Name: "prometheus.v1.0.0", Version: "1.0.0"},
+			Image:          "quay.io/operatorhubio/prometheus@fake1.0.0",
+		},
 	}
 	reconciler.Applier = &MockApplier{
 		objs: []client.Object{},
@@ -745,7 +753,10 @@ func TestClusterExtensionDeleteFinalizerFails(t *testing.T) {
 		cache: &MockManagedContentCache{},
 	}
 	reconciler.InstalledBundleGetter = &MockInstalledBundleGetter{
-		bundle: &ocv1alpha1.BundleMetadata{Name: "prometheus.v1.0.0", Version: "1.0.0"},
+		bundle: &controllers.InstalledBundle{
+			BundleMetadata: ocv1alpha1.BundleMetadata{Name: "prometheus.v1.0.0", Version: "1.0.0"},
+			Image:          "quay.io/operatorhubio/prometheus@fake1.0.0",
+		},
 	}
 	err = reconciler.Finalizers.Register(fakeFinalizer, finalizers.FinalizerFunc(func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) {
 		return crfinalizer.Result{}, errors.New(finalizersMessage)
@@ -1400,3 +1411,133 @@ func TestSetDeprecationStatus(t *testing.T) {
 		})
 	}
 }
+
+type MockActionGetter struct {
+	description    string
+	rels           []*release.Release
+	err            error
+	expectedBundle *controllers.InstalledBundle
+	expectedError  error
+}
+
+func (mag *MockActionGetter) ActionClientFor(ctx context.Context, obj client.Object) (helmclient.ActionInterface, error) {
+	return mag, nil
+}
+
+func (mag *MockActionGetter) Get(name string, opts ...helmclient.GetOption) (*release.Release, error) {
+	return nil, nil
+}
+
+// This is the function we are really testing
+func (mag *MockActionGetter) History(name string, opts ...helmclient.HistoryOption) ([]*release.Release, error) {
+	return mag.rels, mag.err
+}
+
+func (mag *MockActionGetter) Install(name, namespace string, chrt *chart.Chart, vals map[string]interface{}, opts ...helmclient.InstallOption) (*release.Release, error) {
+	return nil, nil
+}
+
+func (mag *MockActionGetter) Upgrade(name, namespace string, chrt *chart.Chart, vals map[string]interface{}, opts ...helmclient.UpgradeOption) (*release.Release, error) {
+	return nil, nil
+}
+
+func (mag *MockActionGetter) Uninstall(name string, opts ...helmclient.UninstallOption) (*release.UninstallReleaseResponse, error) {
+	return nil, nil
+}
+
+func (mag *MockActionGetter) Reconcile(rel *release.Release) error {
+	return nil
+}
+
+func TestGetInstalledBundleHistory(t *testing.T) {
+	getter := controllers.DefaultInstalledBundleGetter{}
+
+	ext := ocv1alpha1.ClusterExtension{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "test-ext",
+		},
+	}
+
+	mag := []MockActionGetter{
+		{
+			"No return",
+			nil, nil,
+			nil, nil,
+		},
+		{
+			"ErrReleaseNotFound (special case)",
+			nil, driver.ErrReleaseNotFound,
+			nil, nil,
+		},
+		{
+			"Error from History",
+			nil, fmt.Errorf("generic error"),
+			nil, fmt.Errorf("generic error"),
+		},
+		{
+			"One item in history",
+			[]*release.Release{
+				{
+					Name: "test-ext",
+					Info: &release.Info{
+						Status: release.StatusDeployed,
+					},
+					Labels: map[string]string{
+						labels.BundleNameKey:      "test-ext",
+						labels.BundleVersionKey:   "1.0",
+						labels.BundleReferenceKey: "bundle-ref",
+					},
+				},
+			}, nil,
+			&controllers.InstalledBundle{
+				BundleMetadata: ocv1alpha1.BundleMetadata{
+					Name:    "test-ext",
+					Version: "1.0",
+				},
+				Image: "bundle-ref",
+			}, nil,
+		},
+		{
+			"Two items in history",
+			[]*release.Release{
+				{
+					Name: "test-ext",
+					Info: &release.Info{
+						Status: release.StatusFailed,
+					},
+					Labels: map[string]string{
+						labels.BundleNameKey:      "test-ext",
+						labels.BundleVersionKey:   "2.0",
+						labels.BundleReferenceKey: "bundle-ref-2",
+					},
+				},
+				{
+					Name: "test-ext",
+					Info: &release.Info{
+						Status: release.StatusDeployed,
+					},
+					Labels: map[string]string{
+						labels.BundleNameKey:      "test-ext",
+						labels.BundleVersionKey:   "1.0",
+						labels.BundleReferenceKey: "bundle-ref-1",
+					},
+				},
+			}, nil,
+			&controllers.InstalledBundle{
+				BundleMetadata: ocv1alpha1.BundleMetadata{
+					Name:    "test-ext",
+					Version: "1.0",
+				},
+				Image: "bundle-ref-1",
+			}, nil,
+		},
+	}
+
+	for _, tst := range mag {
+		t.Log(tst.description)
+		getter.ActionClientGetter = &tst
+		md, err := getter.GetInstalledBundle(context.Background(), &ext)
+		require.Equal(t, tst.expectedError, err)
+		require.Equal(t, tst.expectedBundle, md)
+	}
+}