diff --git a/go.mod b/go.mod index b50546a9d..a43fddbc7 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index 9ef9920b9..d54f5fb44 100644 --- a/go.sum +++ b/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= diff --git a/internal/action/helm.go b/internal/action/helm.go index 0ac18cf79..d54c469da 100644 --- a/internal/action/helm.go +++ b/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)) } diff --git a/internal/action/helm_test.go b/internal/action/helm_test.go index d07dc53bc..3f1f02f9d 100644 --- a/internal/action/helm_test.go +++ b/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") diff --git a/internal/controllers/clusterextension_controller.go b/internal/controllers/clusterextension_controller.go index bb3a1494b..ef8099569 100644 --- a/internal/controllers/clusterextension_controller.go +++ b/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, nil) 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, nil) 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, err) 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, nil) 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 } diff --git a/internal/controllers/clusterextension_controller_test.go b/internal/controllers/clusterextension_controller_test.go index 8f7331384..c17390a57 100644 --- a/internal/controllers/clusterextension_controller_test.go +++ b/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) + } +} diff --git a/internal/controllers/common_controller.go b/internal/controllers/common_controller.go index d04cd0a01..8367ee64a 100644 --- a/internal/controllers/common_controller.go +++ b/internal/controllers/common_controller.go @@ -18,6 +18,7 @@ package controllers import ( "errors" + "fmt" apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -26,6 +27,26 @@ import ( ocv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1" ) +// setInstalledStatusFromBundle sets the installed status based on the given installedBundle. +func setInstalledStatusFromBundle(ext *ocv1alpha1.ClusterExtension, installedBundle *InstalledBundle, err error) { + // Something is installed + if installedBundle != nil { + installStatus := &ocv1alpha1.ClusterExtensionInstallStatus{ + Bundle: installedBundle.BundleMetadata, + } + setInstallStatus(ext, installStatus) + setInstalledStatusConditionSuccess(ext, fmt.Sprintf("Installed bundle %s successfully", installedBundle.Image)) + return + } + // Nothing is installed, if there's no error, it means no installed bundle was found + setInstallStatus(ext, nil) + if err != nil { + setInstalledStatusConditionFailed(ext, err.Error()) + return + } + setInstalledStatusConditionFailed(ext, "No installed bundle found") +} + // setInstalledStatusConditionSuccess sets the installed status condition to success. func setInstalledStatusConditionSuccess(ext *ocv1alpha1.ClusterExtension, message string) { apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{ @@ -48,6 +69,17 @@ func setInstalledStatusConditionFailed(ext *ocv1alpha1.ClusterExtension, message }) } +// setInstalledStatusConditionUnknown sets the installed status condition to unknown. +func setInstalledStatusConditionUnknown(ext *ocv1alpha1.ClusterExtension, message string) { + apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{ + Type: ocv1alpha1.TypeInstalled, + Status: metav1.ConditionUnknown, + Reason: ocv1alpha1.ReasonFailed, + Message: message, + ObservedGeneration: ext.GetGeneration(), + }) +} + func setInstallStatus(ext *ocv1alpha1.ClusterExtension, installStatus *ocv1alpha1.ClusterExtensionInstallStatus) { ext.Status.Install = installStatus } diff --git a/internal/controllers/suite_test.go b/internal/controllers/suite_test.go index 9fa70e457..97ea3c427 100644 --- a/internal/controllers/suite_test.go +++ b/internal/controllers/suite_test.go @@ -73,14 +73,14 @@ func newClient(t *testing.T) client.Client { } type MockInstalledBundleGetter struct { - bundle *ocv1alpha1.BundleMetadata + bundle *controllers.InstalledBundle } -func (m *MockInstalledBundleGetter) SetBundle(bundle *ocv1alpha1.BundleMetadata) { +func (m *MockInstalledBundleGetter) SetBundle(bundle *controllers.InstalledBundle) { m.bundle = bundle } -func (m *MockInstalledBundleGetter) GetInstalledBundle(ctx context.Context, ext *ocv1alpha1.ClusterExtension) (*ocv1alpha1.BundleMetadata, error) { +func (m *MockInstalledBundleGetter) GetInstalledBundle(ctx context.Context, ext *ocv1alpha1.ClusterExtension) (*controllers.InstalledBundle, error) { return m.bundle, nil } diff --git a/internal/labels/labels.go b/internal/labels/labels.go index 59061f339..63bf0c493 100644 --- a/internal/labels/labels.go +++ b/internal/labels/labels.go @@ -1,9 +1,10 @@ package labels const ( - OwnerKindKey = "olm.operatorframework.io/owner-kind" - OwnerNameKey = "olm.operatorframework.io/owner-name" - PackageNameKey = "olm.operatorframework.io/package-name" - BundleNameKey = "olm.operatorframework.io/bundle-name" - BundleVersionKey = "olm.operatorframework.io/bundle-version" + OwnerKindKey = "olm.operatorframework.io/owner-kind" + OwnerNameKey = "olm.operatorframework.io/owner-name" + PackageNameKey = "olm.operatorframework.io/package-name" + BundleNameKey = "olm.operatorframework.io/bundle-name" + BundleVersionKey = "olm.operatorframework.io/bundle-version" + BundleReferenceKey = "olm.operatorframework.io/bundle-reference" )