Filter out bundle versions lower than installed (operator-framework#711)

Adds an annotation to the App created for each extension indicating the bundle version, which can then be used on future reconciles to filter out older versions. This can be overridden by setting the UpgradeConstraintPolicy to 'Ignore'.

Signed-off-by: dtfranz <[email protected]>
Co-authored-by: Varsha Prasad Narsing <[email protected]>

Author: dtfranz

cmd/manager/main.go

@@ -18,21 +18,17 @@ package main
 
 import (
 	"flag"
-	"fmt"
 	"net/http"
 	"os"
 	"time"
 
 	"github.com/spf13/pflag"
 	carvelv1alpha1 "github.com/vmware-tanzu/carvel-kapp-controller/pkg/apis/kappctrl/v1alpha1"
 	"go.uber.org/zap/zapcore"
-	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
-	"k8s.io/client-go/discovery"
 	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
 	_ "k8s.io/client-go/plugin/pkg/client/auth"
-	"k8s.io/client-go/rest"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/healthz"
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
@@ -131,16 +127,9 @@ func main() {
 		os.Exit(1)
 	}
 
-	hasKappApis, err := hasKappApis(mgr.GetConfig())
-	if err != nil {
-		setupLog.Error(err, "unable to evaluate if App needs to be created")
-		os.Exit(1)
-	}
-
 	if err = (&controllers.ExtensionReconciler{
 		Client:         cl,
 		BundleProvider: catalogClient,
-		HasKappApis:    hasKappApis,
 	}).SetupWithManager(mgr); err != nil {
 		setupLog.Error(err, "unable to create controller", "controller", "Extension")
 		os.Exit(1)
@@ -162,27 +151,3 @@ func main() {
 		os.Exit(1)
 	}
 }
-
-// hasKappApis checks whether the cluster has Kapp APIs installed in the cluster.
-// This does not guarantee that the controller is present to reconcile the App CRs.
-func hasKappApis(config *rest.Config) (bool, error) {
-	discoveryClient, err := discovery.NewDiscoveryClientForConfig(config)
-	if err != nil {
-		return false, fmt.Errorf("creating discovery client: %v", err)
-	}
-	apiResourceList, err := discoveryClient.ServerResourcesForGroupVersion(carvelv1alpha1.SchemeGroupVersion.String())
-	if err != nil && !errors.IsNotFound(err) {
-		return false, fmt.Errorf("listing resource APIs: %v", err)
-	}
-
-	if apiResourceList == nil {
-		return false, nil
-	}
-
-	for _, resource := range apiResourceList.APIResources {
-		if resource.Kind == "App" {
-			return true, nil
-		}
-	}
-	return false, nil
-}

internal/catalogmetadata/filter/bundle_predicates.go

@@ -33,6 +33,19 @@ func InMastermindsSemverRange(semverRange *mmsemver.Constraints) Predicate[catal
 	}
 }
 
+func HigherBundleVersion(currentVersion *bsemver.Version) Predicate[catalogmetadata.Bundle] {
+	return func(bundle *catalogmetadata.Bundle) bool {
+		if currentVersion == nil {
+			return false
+		}
+		bundleVersion, err := bundle.Version()
+		if err != nil {
+			return false
+		}
+		return bundleVersion.GTE(*currentVersion)
+	}
+}
+
 func InBlangSemverRange(semverRange bsemver.Range) Predicate[catalogmetadata.Bundle] {
 	return func(bundle *catalogmetadata.Bundle) bool {
 		bundleVersion, err := bundle.Version()

internal/catalogmetadata/filter/bundle_predicates_test.go

@@ -2,6 +2,7 @@ package filter_test
 
 import (
 	"encoding/json"
+	"fmt"
 	"testing"
 
 	mmsemver "github.com/Masterminds/semver/v3"
@@ -63,6 +64,67 @@ func TestInMastermindsSemverRange(t *testing.T) {
 	assert.False(t, f(b3))
 }
 
+func TestHigherBundleVersion(t *testing.T) {
+	testCases := []struct {
+		name             string
+		requestedVersion string
+		comparedVersion  string
+		wantResult       bool
+	}{
+		{
+			name:             "includes equal version",
+			requestedVersion: "1.0.0",
+			comparedVersion:  "1.0.0",
+			wantResult:       true,
+		},
+		{
+			name:             "filters out older version",
+			requestedVersion: "1.0.0",
+			comparedVersion:  "0.0.1",
+			wantResult:       false,
+		},
+		{
+			name:             "includes higher version",
+			requestedVersion: "1.0.0",
+			comparedVersion:  "2.0.0",
+			wantResult:       true,
+		},
+		{
+			name:             "filters out broken version",
+			requestedVersion: "1.0.0",
+			comparedVersion:  "broken",
+			wantResult:       false,
+		},
+		{
+			name:             "filter returns false with nil version",
+			requestedVersion: "",
+			comparedVersion:  "1.0.0",
+			wantResult:       false,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			bundle := &catalogmetadata.Bundle{Bundle: declcfg.Bundle{
+				Properties: []property.Property{
+					{
+						Type:  property.TypePackage,
+						Value: json.RawMessage(fmt.Sprintf(`{"packageName": "package1", "version": "%s"}`, tc.comparedVersion)),
+					},
+				},
+			}}
+			var version *bsemver.Version
+			if tc.requestedVersion != "" {
+				parsedVersion := bsemver.MustParse(tc.requestedVersion)
+				// to test with nil requested version
+				version = &parsedVersion
+			}
+			f := filter.HigherBundleVersion(version)
+			assert.Equal(t, tc.wantResult, f(bundle))
+		})
+	}
+}
+
 func TestInBlangSemverRange(t *testing.T) {
 	b1 := &catalogmetadata.Bundle{Bundle: declcfg.Bundle{
 		Properties: []property.Property{

internal/controllers/extension_controller.go

@@ -18,7 +18,6 @@ package controllers
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"sort"
 	"strings"
@@ -29,6 +28,7 @@ import (
 	kappctrlv1alpha1 "github.com/vmware-tanzu/carvel-kapp-controller/pkg/apis/kappctrl/v1alpha1"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/equality"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	apimeta "k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -56,10 +56,11 @@ import (
 type ExtensionReconciler struct {
 	client.Client
 	BundleProvider BundleProvider
-	HasKappApis    bool
 }
 
-var errkappAPIUnavailable = errors.New("kapp-controller apis unavailable on cluster")
+var (
+	bundleVersionKey = "olm.operatorframework.io/bundleVersion"
+)
 
 //+kubebuilder:rbac:groups=olm.operatorframework.io,resources=extensions,verbs=get;list;watch;create;update;patch;delete
 //+kubebuilder:rbac:groups=olm.operatorframework.io,resources=extensions/status,verbs=update;patch
@@ -146,17 +147,6 @@ func (r *ExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alpha1.Ext
 		return ctrl.Result{}, nil
 	}
 
-	if !r.HasKappApis {
-		ext.Status.InstalledBundle = nil
-		setInstalledStatusConditionFailed(&ext.Status.Conditions, errkappAPIUnavailable.Error(), ext.GetGeneration())
-
-		ext.Status.ResolvedBundle = nil
-		setResolvedStatusConditionUnknown(&ext.Status.Conditions, "kapp apis are unavailable", ext.GetGeneration())
-
-		setDeprecationStatusesUnknown(&ext.Status.Conditions, "kapp apis are unavailable", ext.GetGeneration())
-		return ctrl.Result{}, errkappAPIUnavailable
-	}
-
 	// TODO: Improve the resolution logic.
 	bundle, err := r.resolve(ctx, *ext)
 	if err != nil {
@@ -190,7 +180,13 @@ func (r *ExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alpha1.Ext
 		return ctrl.Result{}, nil
 	}
 
-	app := r.GenerateExpectedApp(*ext, bundle.Image)
+	app, err := r.GenerateExpectedApp(*ext, bundle)
+	if err != nil {
+		setInstalledStatusConditionUnknown(&ext.Status.Conditions, err.Error(), ext.GetGeneration())
+		setDeprecationStatusesUnknown(&ext.Status.Conditions, "deprecation checks have not been attempted as installation has failed", ext.GetGeneration())
+		return ctrl.Result{}, err
+	}
+
 	if err := r.ensureApp(ctx, app); err != nil {
 		// originally Reason: ocv1alpha1.ReasonInstallationFailed
 		ext.Status.InstalledBundle = nil
@@ -403,7 +399,13 @@ func extensionRequestsForCatalog(c client.Reader, logger logr.Logger) handler.Ma
 	}
 }
 
-func (r *ExtensionReconciler) GenerateExpectedApp(o ocv1alpha1.Extension, bundlePath string) *unstructured.Unstructured {
+func (r *ExtensionReconciler) GenerateExpectedApp(o ocv1alpha1.Extension, bundle *catalogmetadata.Bundle) (*unstructured.Unstructured, error) {
+	bundleVersion, err := bundle.Version()
+	if err != nil {
+		return nil, fmt.Errorf("failed to generate App from Extension %q with bundle %q: %w", o.GetName(), bundle.Name, err)
+	}
+	bundlePath := bundle.Image
+
 	// We use unstructured here to avoid problems of serializing default values when sending patches to the apiserver.
 	// If you use a typed object, any default values from that struct get serialized into the JSON patch, which could
 	// cause unrelated fields to be patched back to the default value even though that isn't the intention. Using an
@@ -437,6 +439,9 @@ func (r *ExtensionReconciler) GenerateExpectedApp(o ocv1alpha1.Extension, bundle
 			"metadata": map[string]interface{}{
 				"name":      o.GetName(),
 				"namespace": o.GetNamespace(),
+				"annotations": map[string]string{
+					bundleVersionKey: bundleVersion.String(),
+				},
 			},
 			"spec": spec,
 		},
@@ -452,7 +457,24 @@ func (r *ExtensionReconciler) GenerateExpectedApp(o ocv1alpha1.Extension, bundle
 			BlockOwnerDeletion: ptr.To(true),
 		},
 	})
-	return app
+	return app, nil
+}
+
+func (r *ExtensionReconciler) getInstalledVersion(ctx context.Context, namespacedName types.NamespacedName) (*bsemver.Version, error) {
+	existingApp, err := r.existingAppUnstructured(ctx, namespacedName.Name, namespacedName.Namespace)
+	if err != nil {
+		return nil, err
+	}
+	existingVersion, ok := existingApp.GetAnnotations()[bundleVersionKey]
+	if !ok {
+		return nil, fmt.Errorf("existing App %q in Namespace %q missing bundle version", namespacedName.Name, namespacedName.Namespace)
+	}
+
+	existingVersionSemver, err := bsemver.New(existingVersion)
+	if err != nil {
+		return nil, fmt.Errorf("could not determine bundle version of existing App %q in Namespace %q: %w", namespacedName.Name, namespacedName.Namespace, err)
+	}
+	return existingVersionSemver, nil
 }
 
 func (r *ExtensionReconciler) resolve(ctx context.Context, extension ocv1alpha1.Extension) (*catalogmetadata.Bundle, error) {
@@ -481,19 +503,33 @@ func (r *ExtensionReconciler) resolve(ctx context.Context, extension ocv1alpha1.
 		predicates = append(predicates, catalogfilter.InMastermindsSemverRange(vr))
 	}
 
+	var installedVersion string
+	// Do not include bundle versions older than currently installed unless UpgradeConstraintPolicy = 'Ignore'
+	if extension.Spec.Source.Package.UpgradeConstraintPolicy != ocv1alpha1.UpgradeConstraintPolicyIgnore {
+		installedVersionSemver, err := r.getInstalledVersion(ctx, types.NamespacedName{Name: extension.GetName(), Namespace: extension.GetNamespace()})
+		if err != nil && !apierrors.IsNotFound(err) {
+			return nil, err
+		}
+		if installedVersionSemver != nil {
+			installedVersion = installedVersionSemver.String()
+			predicates = append(predicates, catalogfilter.HigherBundleVersion(installedVersionSemver))
+		}
+	}
+
 	resultSet := catalogfilter.Filter(allBundles, catalogfilter.And(predicates...))
 
 	if len(resultSet) == 0 {
-		if versionRange != "" && channelName != "" {
-			return nil, fmt.Errorf("no package %q matching version %q found in channel %q", packageName, versionRange, channelName)
-		}
+		var versionError, channelError, existingVersionError string
 		if versionRange != "" {
-			return nil, fmt.Errorf("no package %q matching version %q found", packageName, versionRange)
+			versionError = fmt.Sprintf(" matching version %q", versionRange)
 		}
 		if channelName != "" {
-			return nil, fmt.Errorf("no package %q found in channel %q", packageName, channelName)
+			channelError = fmt.Sprintf(" in channel %q", channelName)
+		}
+		if installedVersion != "" {
+			existingVersionError = fmt.Sprintf(" which upgrades currently installed version %q", installedVersion)
 		}
-		return nil, fmt.Errorf("no package %q found", packageName)
+		return nil, fmt.Errorf("no package %q%s%s%s found", packageName, versionError, channelError, existingVersionError)
 	}
 
 	sort.SliceStable(resultSet, func(i, j int) bool {