From 799c4e959883665085ad8b935ccca75d56c90b04 Mon Sep 17 00:00:00 2001 From: Mikalai Radchuk Date: Fri, 6 Oct 2023 09:41:24 +0100 Subject: [PATCH] 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 --- .../filter/bundle_predicates.go | 12 +++---- .../filter/bundle_predicates_test.go | 24 +++++++------- internal/controllers/operator_controller.go | 25 +++++++++++--- .../variablesources/bundle_deployment.go | 25 +++++++------- .../variablesources/bundle_deployment_test.go | 31 ++++++++++++----- .../bundles_and_dependencies_test.go | 12 +++---- .../variablesources/installed_package.go | 33 +++++++++++-------- .../variablesources/installed_package_test.go | 9 +++-- .../variablesources/operator_test.go | 2 +- 9 files changed, 106 insertions(+), 67 deletions(-) diff --git a/internal/catalogmetadata/filter/bundle_predicates.go b/internal/catalogmetadata/filter/bundle_predicates.go index e62df9187..cd55c8998 100644 --- 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() @@ -52,12 +58,6 @@ func InChannel(channelName string) Predicate[catalogmetadata.Bundle] { } } -func WithBundleImage(bundleImage string) Predicate[catalogmetadata.Bundle] { - return func(bundle *catalogmetadata.Bundle) bool { - return bundle.Image == bundleImage - } -} - func Replaces(bundleName string) Predicate[catalogmetadata.Bundle] { return func(bundle *catalogmetadata.Bundle) bool { for _, ch := range bundle.InChannels { diff --git a/internal/catalogmetadata/filter/bundle_predicates_test.go b/internal/catalogmetadata/filter/bundle_predicates_test.go index 95e9482f0..5a879ba14 100644 --- a/internal/catalogmetadata/filter/bundle_predicates_test.go +++ b/internal/catalogmetadata/filter/bundle_predicates_test.go @@ -26,6 +26,18 @@ func TestWithPackageName(t *testing.T) { assert.False(t, f(b3)) } +func TestWithName(t *testing.T) { + b1 := &catalogmetadata.Bundle{Bundle: declcfg.Bundle{Name: "package1.v1"}} + b2 := &catalogmetadata.Bundle{Bundle: declcfg.Bundle{Name: "package2.v1"}} + b3 := &catalogmetadata.Bundle{} + + f := filter.WithName("package1.v1") + + assert.True(t, f(b1)) + assert.False(t, f(b2)) + assert.False(t, f(b3)) +} + func TestInMastermindsSemverRange(t *testing.T) { b1 := &catalogmetadata.Bundle{Bundle: declcfg.Bundle{ Properties: []property.Property{ @@ -114,18 +126,6 @@ func TestInChannel(t *testing.T) { assert.False(t, f(b3)) } -func TestWithBundleImage(t *testing.T) { - b1 := &catalogmetadata.Bundle{Bundle: declcfg.Bundle{Image: "fake-image-uri-1"}} - b2 := &catalogmetadata.Bundle{Bundle: declcfg.Bundle{Image: "fake-image-uri-2"}} - b3 := &catalogmetadata.Bundle{} - - f := filter.WithBundleImage("fake-image-uri-1") - - assert.True(t, f(b1)) - assert.False(t, f(b2)) - assert.False(t, f(b3)) -} - func TestReplaces(t *testing.T) { fakeChannel := &catalogmetadata.Channel{ Channel: declcfg.Channel{ diff --git a/internal/controllers/operator_controller.go b/internal/controllers/operator_controller.go index 3b0ef5067..ecdb53328 100644 --- a/internal/controllers/operator_controller.go +++ b/internal/controllers/operator_controller.go @@ -47,6 +47,7 @@ import ( "github.com/operator-framework/operator-controller/internal/catalogmetadata" "github.com/operator-framework/operator-controller/internal/controllers/validators" olmvariables "github.com/operator-framework/operator-controller/internal/resolution/variables" + "github.com/operator-framework/operator-controller/internal/resolution/variablesources" ) // OperatorReconciler reconciles a Operator object @@ -179,9 +180,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 +271,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{ + variablesources.AnnotationPackageName: bundle.Package, + variablesources.AnnotationBundleName: bundle.Name, + variablesources.AnnotationBundleVersion: bundleVersion.String(), + }, }, "spec": map[string]interface{}{ // TODO: Don't assume plain provisioner @@ -287,7 +304,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 +321,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 index 25c287087..07ff6c9d0 100644 --- a/internal/resolution/variablesources/bundle_deployment.go +++ b/internal/resolution/variablesources/bundle_deployment.go @@ -9,6 +9,12 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) +const ( + AnnotationPackageName = "operators.operatorframework.io/package-name" + AnnotationBundleName = "operators.operatorframework.io/bundle-name" + AnnotationBundleVersion = "operators.operatorframework.io/bundle-version" +) + var _ input.VariableSource = &BundleDeploymentVariableSource{} type BundleDeploymentVariableSource struct { @@ -36,20 +42,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[AnnotationPackageName] + bundleName := bundleDeployment.Annotations[AnnotationBundleName] + bundleVersion := bundleDeployment.Annotations[AnnotationBundleVersion] + 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 index 0e640210e..cb670e989 100644 --- 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{ + variablesources.AnnotationPackageName: pkgName, + variablesources.AnnotationBundleName: bundleName, + variablesources.AnnotationBundleVersion: 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()) @@ -136,11 +141,21 @@ var _ = Describe("BundleDeploymentVariableSource", func() { deppy.IdentifierFromString("installed package prometheus"): 2, }))) }) + It("should not produce InstalledPackage variables when annotations on BundleDeployment are not set", func() { + cl := BundleDeploymentFakeClient(bundleDeployment("prometheus", "", "", "quay.io/operatorhubio/prometheus@sha256:3e281e587de3d03011440685fc4fb782672beab044c1ebadc42788ce05a21c35")) + + bdVariableSource := variablesources.NewBundleDeploymentVariableSource(cl, &fakeCatalogClient, &MockVariableSource{}) + variables, err := bdVariableSource.GetVariables(context.Background()) + Expect(err).ToNot(HaveOccurred()) + + installedPackageVariable := filterVariables[*olmvariables.InstalledPackageVariable](variables) + Expect(installedPackageVariable).To(BeEmpty()) + }) 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).To(MatchError(`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 index 78805f75e..c893658f6 100644 --- 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,7 +380,7 @@ 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()) @@ -388,12 +388,12 @@ var _ = Describe("BundlesAndDepsVariableSource", func() { }) }) -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 index 3687c04e5..55fada068 100644 --- 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 index 1f435f908..0cb958f23 100644 --- 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/test-package@v2.0.0" + 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 index 426cf82c2..89d269418 100644 --- 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())