Add annotations to BundleDeployment

This adds annotations to `BundleDeployment` objects created by operator-controller in response to `Operator` objects. This makes it easier to filter bundles during resolution and makes us less dependant on image bundle format. Signed-off-by: Mikalai Radchuk <[email protected]>
operator-framework · Oct 6, 2023 · 7d2a8f7 · 7d2a8f7
1 parent 65bfc69
commit 7d2a8f7
Show file tree

Hide file tree

Showing 8 changed files with 77 additions and 49 deletions.
diff --git a/internal/catalogmetadata/filter/bundle_predicates.go b/internal/catalogmetadata/filter/bundle_predicates.go
@@ -13,6 +13,12 @@ func WithPackageName(packageName string) Predicate[catalogmetadata.Bundle] {
 	}
 }
 
+func WithName(bundleName string) Predicate[catalogmetadata.Bundle] {
+	return func(bundle *catalogmetadata.Bundle) bool {
+		return bundle.Name == bundleName
+	}
+}
+
 func InMastermindsSemverRange(semverRange *mmsemver.Constraints) Predicate[catalogmetadata.Bundle] {
 	return func(bundle *catalogmetadata.Bundle) bool {
 		bVersion, err := bundle.Version()

diff --git a/internal/controllers/operator_controller.go b/internal/controllers/operator_controller.go
@@ -179,9 +179,15 @@ func (r *OperatorReconciler) reconcile(ctx context.Context, op *operatorsv1alpha
 		setInstalledStatusConditionFailed(&op.Status.Conditions, err.Error(), op.GetGeneration())
 		return ctrl.Result{}, err
 	}
+
 	// Ensure a BundleDeployment exists with its bundle source from the bundle
 	// image we just looked up in the solution.
-	dep := r.generateExpectedBundleDeployment(*op, bundle.Image, bundleProvisioner)
+	dep, err := r.generateExpectedBundleDeployment(*op, bundle, bundleProvisioner)
+	if err != nil {
+		op.Status.InstalledBundleResource = ""
+		setInstalledStatusConditionFailed(&op.Status.Conditions, err.Error(), op.GetGeneration())
+		return ctrl.Result{}, err
+	}
 	if err := r.ensureBundleDeployment(ctx, dep); err != nil {
 		// originally Reason: operatorsv1alpha1.ReasonInstallationFailed
 		op.Status.InstalledBundleResource = ""
@@ -264,18 +270,28 @@ func (r *OperatorReconciler) bundleFromSolution(solution *solver.Solution, packa
 	return nil, fmt.Errorf("bundle for package %q not found in solution", packageName)
 }
 
-func (r *OperatorReconciler) generateExpectedBundleDeployment(o operatorsv1alpha1.Operator, bundlePath string, bundleProvisioner string) *unstructured.Unstructured {
+func (r *OperatorReconciler) generateExpectedBundleDeployment(o operatorsv1alpha1.Operator, bundle *catalogmetadata.Bundle, bundleProvisioner string) (*unstructured.Unstructured, error) {
 	// 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
 	// unstructured ensures that the patch contains only what is specified. Using unstructured like this is basically
 	// identical to "kubectl apply -f"
 
+	bundleVersion, err := bundle.Version()
+	if err != nil {
+		return nil, err
+	}
+
 	bd := &unstructured.Unstructured{Object: map[string]interface{}{
 		"apiVersion": rukpakv1alpha1.GroupVersion.String(),
 		"kind":       rukpakv1alpha1.BundleDeploymentKind,
 		"metadata": map[string]interface{}{
 			"name": o.GetName(),
+			"annotations": map[string]string{
+				"operators.operatorframework.io/package-name":   bundle.Package,
+				"operators.operatorframework.io/bundle-name":    bundle.Name,
+				"operators.operatorframework.io/bundle-version": bundleVersion.String(),
+			},
 		},
 		"spec": map[string]interface{}{
 			// TODO: Don't assume plain provisioner
@@ -287,7 +303,7 @@ func (r *OperatorReconciler) generateExpectedBundleDeployment(o operatorsv1alpha
 						// TODO: Don't assume image type
 						"type": string(rukpakv1alpha1.SourceTypeImage),
 						"image": map[string]interface{}{
-							"ref": bundlePath,
+							"ref": bundle.Image,
 						},
 					},
 				},
@@ -304,7 +320,7 @@ func (r *OperatorReconciler) generateExpectedBundleDeployment(o operatorsv1alpha
 			BlockOwnerDeletion: pointer.Bool(true),
 		},
 	})
-	return bd
+	return bd, nil
 }
 
 // SetupWithManager sets up the controller with the Manager.

diff --git a/internal/resolution/variablesources/bundle_deployment.go b/internal/resolution/variablesources/bundle_deployment.go
@@ -36,20 +36,15 @@ func (o *BundleDeploymentVariableSource) GetVariables(ctx context.Context) ([]de
 		return nil, err
 	}
 
-	processed := map[string]struct{}{}
 	for _, bundleDeployment := range bundleDeployments.Items {
-		sourceImage := bundleDeployment.Spec.Template.Spec.Source.Image
-		if sourceImage != nil && sourceImage.Ref != "" {
-			if _, ok := processed[sourceImage.Ref]; ok {
-				continue
-			}
-			processed[sourceImage.Ref] = struct{}{}
-			ips, err := NewInstalledPackageVariableSource(o.catalogClient, bundleDeployment.Spec.Template.Spec.Source.Image.Ref)
-			if err != nil {
-				return nil, err
-			}
-			variableSources = append(variableSources, ips)
+		pkgName := bundleDeployment.Annotations["operators.operatorframework.io/package-name"]
+		bundleName := bundleDeployment.Annotations["operators.operatorframework.io/bundle-name"]
+		bundleVersion := bundleDeployment.Annotations["operators.operatorframework.io/bundle-version"]
+		if pkgName == "" || bundleName == "" || bundleVersion == "" {
+			continue
 		}
+		ips := NewInstalledPackageVariableSource(o.catalogClient, pkgName, bundleName, bundleVersion)
+		variableSources = append(variableSources, ips)
 	}
 
 	return variableSources.GetVariables(ctx)

diff --git a/internal/resolution/variablesources/bundle_deployment_test.go b/internal/resolution/variablesources/bundle_deployment_test.go
@@ -31,10 +31,15 @@ func BundleDeploymentFakeClient(objects ...client.Object) client.Client {
 	return fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build()
 }
 
-func bundleDeployment(name, image string) *rukpakv1alpha1.BundleDeployment {
+func bundleDeployment(pkgName, bundleName, bundleVersion, image string) *rukpakv1alpha1.BundleDeployment {
 	return &rukpakv1alpha1.BundleDeployment{
 		ObjectMeta: metav1.ObjectMeta{
-			Name: name,
+			Name: pkgName,
+			Annotations: map[string]string{
+				"operators.operatorframework.io/package-name":   pkgName,
+				"operators.operatorframework.io/bundle-name":    bundleName,
+				"operators.operatorframework.io/bundle-version": bundleVersion,
+			},
 		},
 		Spec: rukpakv1alpha1.BundleDeploymentSpec{
 			ProvisionerClassName: "core-rukpak-io-plain",
@@ -115,10 +120,10 @@ var _ = Describe("BundleDeploymentVariableSource", func() {
 		fakeCatalogClient = testutil.NewFakeCatalogClient(testBundleList)
 	})
 
-	It("should produce RequiredPackage variables", func() {
-		cl := BundleDeploymentFakeClient(bundleDeployment("prometheus", "quay.io/operatorhubio/prometheus@sha256:3e281e587de3d03011440685fc4fb782672beab044c1ebadc42788ce05a21c35"))
+	It("should produce InstalledPackage variables", func() {
+		cl := BundleDeploymentFakeClient(bundleDeployment("prometheus", "operatorhub/prometheus/0.37.0", "0.37.0", "quay.io/operatorhubio/prometheus@sha256:3e281e587de3d03011440685fc4fb782672beab044c1ebadc42788ce05a21c35"))
 
-		bdVariableSource := variablesources.NewBundleDeploymentVariableSource(cl, &fakeCatalogClient, &MockRequiredPackageSource{})
+		bdVariableSource := variablesources.NewBundleDeploymentVariableSource(cl, &fakeCatalogClient, &MockVariableSource{})
 		variables, err := bdVariableSource.GetVariables(context.Background())
 		Expect(err).ToNot(HaveOccurred())
 
@@ -137,10 +142,10 @@ var _ = Describe("BundleDeploymentVariableSource", func() {
 		})))
 	})
 	It("should return an error if the bundleDeployment image doesn't match any operator resource", func() {
-		cl := BundleDeploymentFakeClient(bundleDeployment("prometheus", "quay.io/operatorhubio/prometheus@sha256:nonexistent"))
+		cl := BundleDeploymentFakeClient(bundleDeployment("prometheus", "operatorhub/prometheus/9.9.9", "9.9.9", "quay.io/operatorhubio/prometheus@sha256:nonexistent"))
 
-		bdVariableSource := variablesources.NewBundleDeploymentVariableSource(cl, &fakeCatalogClient, &MockRequiredPackageSource{})
+		bdVariableSource := variablesources.NewBundleDeploymentVariableSource(cl, &fakeCatalogClient, &MockVariableSource{})
 		_, err := bdVariableSource.GetVariables(context.Background())
-		Expect(err.Error()).To(Equal("bundleImage \"quay.io/operatorhubio/prometheus@sha256:nonexistent\" not found"))
+		Expect(err.Error()).To(Equal(`bundle for package "prometheus" with name "operatorhub/prometheus/9.9.9" at version "9.9.9" not found`))
 	})
 })
diff --git a/internal/resolution/variablesources/bundles_and_dependencies_test.go b/internal/resolution/variablesources/bundles_and_dependencies_test.go
@@ -225,7 +225,7 @@ var _ = Describe("BundlesAndDepsVariableSource", func() {
 		fakeCatalogClient = testutil.NewFakeCatalogClient(testBundleList)
 		bdvs = variablesources.NewBundlesAndDepsVariableSource(
 			&fakeCatalogClient,
-			&MockRequiredPackageSource{
+			&MockVariableSource{
 				ResultSet: []deppy.Variable{
 					// must match data in fakeCatalogClient
 					olmvariables.NewRequiredPackageVariable("test-package", []*catalogmetadata.Bundle{
@@ -257,7 +257,7 @@ var _ = Describe("BundlesAndDepsVariableSource", func() {
 					}),
 				},
 			},
-			&MockRequiredPackageSource{
+			&MockVariableSource{
 				ResultSet: []deppy.Variable{
 					// must match data in fakeCatalogClient
 					olmvariables.NewRequiredPackageVariable("test-package-2", []*catalogmetadata.Bundle{
@@ -339,7 +339,7 @@ var _ = Describe("BundlesAndDepsVariableSource", func() {
 
 		bdvs = variablesources.NewBundlesAndDepsVariableSource(
 			&emptyCatalogClient,
-			&MockRequiredPackageSource{
+			&MockVariableSource{
 				ResultSet: []deppy.Variable{
 					// must match data in fakeCatalogClient
 					olmvariables.NewRequiredPackageVariable("test-package", []*catalogmetadata.Bundle{
@@ -380,20 +380,20 @@ var _ = Describe("BundlesAndDepsVariableSource", func() {
 	It("should return error if an inner variable source returns an error", func() {
 		bdvs = variablesources.NewBundlesAndDepsVariableSource(
 			&fakeCatalogClient,
-			&MockRequiredPackageSource{Error: errors.New("fake error")},
+			&MockVariableSource{Error: errors.New("fake error")},
 		)
 		_, err := bdvs.GetVariables(context.TODO())
 		Expect(err).To(HaveOccurred())
 		Expect(err).To(MatchError("fake error"))
 	})
 })
 
-type MockRequiredPackageSource struct {
+type MockVariableSource struct {
 	ResultSet []deppy.Variable
 	Error     error
 }
 
-func (m *MockRequiredPackageSource) GetVariables(_ context.Context) ([]deppy.Variable, error) {
+func (m *MockVariableSource) GetVariables(_ context.Context) ([]deppy.Variable, error) {
 	return m.ResultSet, m.Error
 }
 

diff --git a/internal/resolution/variablesources/installed_package.go b/internal/resolution/variablesources/installed_package.go
@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"sort"
 
+	mmsemver "github.com/Masterminds/semver/v3"
 	"github.com/operator-framework/deppy/pkg/deppy"
 	"github.com/operator-framework/deppy/pkg/deppy/input"
 
@@ -17,7 +18,9 @@ var _ input.VariableSource = &InstalledPackageVariableSource{}
 
 type InstalledPackageVariableSource struct {
 	catalogClient BundleProvider
-	bundleImage   string
+	pkgName       string
+	bundleName    string
+	bundleVersion string
 }
 
 func (r *InstalledPackageVariableSource) GetVariables(ctx context.Context) ([]deppy.Variable, error) {
@@ -26,18 +29,20 @@ func (r *InstalledPackageVariableSource) GetVariables(ctx context.Context) ([]de
 		return nil, err
 	}
 
+	vr, err := mmsemver.NewConstraint(r.bundleVersion)
+	if err != nil {
+		return nil, err
+	}
+
 	// find corresponding bundle for the installed content
-	resultSet := catalogfilter.Filter(allBundles, catalogfilter.WithBundleImage(r.bundleImage))
+	resultSet := catalogfilter.Filter(allBundles, catalogfilter.And(
+		catalogfilter.WithPackageName(r.pkgName),
+		catalogfilter.WithName(r.bundleName),
+		catalogfilter.InMastermindsSemverRange(vr),
+	))
 	if len(resultSet) == 0 {
 		return nil, r.notFoundError()
 	}
-
-	// TODO: fast follow - we should check whether we are already supporting the channel attribute in the operator spec.
-	//       if so, we should take the value from spec of the operator CR in the owner ref of the bundle deployment.
-	//       If that channel is set, we need to update the filter above to filter by channel as well.
-	sort.SliceStable(resultSet, func(i, j int) bool {
-		return catalogsort.ByVersion(resultSet[i], resultSet[j])
-	})
 	installedBundle := resultSet[0]
 
 	// now find the bundles that replace the installed bundle
@@ -56,12 +61,14 @@ func (r *InstalledPackageVariableSource) GetVariables(ctx context.Context) ([]de
 }
 
 func (r *InstalledPackageVariableSource) notFoundError() error {
-	return fmt.Errorf("bundleImage %q not found", r.bundleImage)
+	return fmt.Errorf("bundle for package %q with name %q at version %q not found", r.pkgName, r.bundleName, r.bundleVersion)
 }
 
-func NewInstalledPackageVariableSource(catalogClient BundleProvider, bundleImage string) (*InstalledPackageVariableSource, error) {
+func NewInstalledPackageVariableSource(catalogClient BundleProvider, pkgName, bundleName, bundleVersion string) *InstalledPackageVariableSource {
 	return &InstalledPackageVariableSource{
 		catalogClient: catalogClient,
-		bundleImage:   bundleImage,
-	}, nil
+		pkgName:       pkgName,
+		bundleName:    bundleName,
+		bundleVersion: bundleVersion,
+	}
 }
diff --git a/internal/resolution/variablesources/installed_package_test.go b/internal/resolution/variablesources/installed_package_test.go
@@ -20,7 +20,6 @@ var _ = Describe("InstalledPackageVariableSource", func() {
 	var (
 		ipvs              *variablesources.InstalledPackageVariableSource
 		fakeCatalogClient testutil.FakeCatalogClient
-		bundleImage       string
 	)
 
 	BeforeEach(func() {
@@ -95,11 +94,11 @@ var _ = Describe("InstalledPackageVariableSource", func() {
 				InChannels: []*catalogmetadata.Channel{&channel},
 			},
 		}
-		var err error
-		bundleImage = "registry.io/repo/[email protected]"
+		pkgName := "test-package"
+		bundleName := "test-package.v2.0.0"
+		bundleVersion := "2.0.0"
 		fakeCatalogClient = testutil.NewFakeCatalogClient(bundleList)
-		ipvs, err = variablesources.NewInstalledPackageVariableSource(&fakeCatalogClient, bundleImage)
-		Expect(err).NotTo(HaveOccurred())
+		ipvs = variablesources.NewInstalledPackageVariableSource(&fakeCatalogClient, pkgName, bundleName, bundleVersion)
 	})
 
 	It("should return the correct package variable", func() {

diff --git a/internal/resolution/variablesources/operator_test.go b/internal/resolution/variablesources/operator_test.go
@@ -116,7 +116,7 @@ var _ = Describe("OperatorVariableSource", func() {
 	It("should produce RequiredPackage variables", func() {
 		cl := FakeClient(operator("prometheus"), operator("packageA"))
 		fakeCatalogClient := testutil.NewFakeCatalogClient(testBundleList)
-		opVariableSource := variablesources.NewOperatorVariableSource(cl, &fakeCatalogClient, &MockRequiredPackageSource{})
+		opVariableSource := variablesources.NewOperatorVariableSource(cl, &fakeCatalogClient, &MockVariableSource{})
 		variables, err := opVariableSource.GetVariables(context.Background())
 		Expect(err).ToNot(HaveOccurred())