migrate: support migration of FBC to latest preferred FBC (#1144)

* remove defunct ref-style olm.bundle.object

This commit removes a feature of FBC that has never actually been used
in practice: the ability to reference a file in an olm.bundle.object
property, where the path is relative to the directory in which the file
containing the olm.bundle.object property exists. This was originally
intended to be a way to avoid bloating the FBC, but it's presence has
caused two problems:

  1. It hinted that it would be okay for third-party properties and
     schemas to reference files in a similar way.
  2. Because of (1), we have never really been able to make assumptions
     that would enable us to migrate and re-write FBC in a different
     hierarchy, which has been limiting.

In short, it imposes a burden on catalog maintainers to keep a catalog
in a filesystem structure that is imposed by the author of the catalog
contribution.

In practice, ref-style olm.bundle.object properties have never been used
(as far as I'm aware), because no tooling has ever produced that style,
and no one I have heard of is using other methods to render bundles into
an FBC.

Lastly, with the recent addition of the `olm.csv.metadata` property,
the useful life of the `olm.bundle.object` property (which has always
been alpha) is nearing an end.

Signed-off-by: Joe Lanford <[email protected]>

* migrate: support migration of FBC to latest preferred FBC

This commit adds support for migrating FBC to the latest preferred FBC
contents. Note that sqlite and bundle inputs are always rendered using
the latest preferred FBC contents.

The migrate command is updated to now support FBC images and directories
as input (only sqlite was supported prior), such that the written output
will always be migrated.

The render command is updated with a `--migrate` flag that allows a
caller to opt into migration during rendering.

Under the hood, both of these subcommands use the action.Render struct,
which has a new `Migrate` boolean field that callers can use to
enable/disable the migration behavior.

Signed-off-by: Joe Lanford <[email protected]>

---------

Signed-off-by: Joe Lanford <[email protected]>
Upstream-repository: operator-registry
Upstream-commit: 5e3fa99bfd024d4c73a29456e68f1cb9bc4e4504

Author: joelanford

staging/operator-registry/alpha/action/migrate.go

@@ -29,14 +29,11 @@ func (m Migrate) Run(ctx context.Context) error {
 	}
 
 	r := Render{
-		Refs: []string{m.CatalogRef},
+		Refs:    []string{m.CatalogRef},
+		Migrate: true,
 
-		// Only allow sqlite images and files to be migrated. Other types cannot
-		// always be migrated cleanly because they may contain file references.
-		// Rendered sqlite databases never contain file references.
-		AllowedRefMask: RefSqliteImage | RefSqliteFile,
-
-		skipSqliteDeprecationLog: true,
+		// Only allow catalogs to be migrated.
+		AllowedRefMask: RefSqliteImage | RefSqliteFile | RefDCImage | RefDCDir,
 	}
 	if m.Registry != nil {
 		r.Registry = m.Registry

staging/operator-registry/alpha/action/migrate_test.go

@@ -50,8 +50,8 @@ func TestMigrate(t *testing.T) {
 				Registry:   reg,
 			},
 			expectedFiles: map[string]string{
-				"foo/catalog.yaml": migrateFooCatalog(),
-				"bar/catalog.yaml": migrateBarCatalog(),
+				"foo/catalog.yaml": migrateFooCatalogSqlite(),
+				"bar/catalog.yaml": migrateBarCatalogSqlite(),
 			},
 		},
 		{
@@ -64,31 +64,35 @@ func TestMigrate(t *testing.T) {
 				Registry:   reg,
 			},
 			expectedFiles: map[string]string{
-				"foo/catalog.yaml": migrateFooCatalog(),
-				"bar/catalog.yaml": migrateBarCatalog(),
+				"foo/catalog.yaml": migrateFooCatalogSqlite(),
+				"bar/catalog.yaml": migrateBarCatalogSqlite(),
 			},
 		},
 		{
-			name: "DeclcfgImage/Failure",
+			name: "DeclcfgImage/Success",
 			migrate: action.Migrate{
 				CatalogRef: "test.registry/foo-operator/foo-index-declcfg:v0.2.0",
 				OutputDir:  filepath.Join(tmpDir, "declcfg-image"),
 				WriteFunc:  declcfg.WriteYAML,
 				FileExt:    ".yaml",
 				Registry:   reg,
 			},
-			expectErr: action.ErrNotAllowed,
+			expectedFiles: map[string]string{
+				"foo/catalog.yaml": migrateFooCatalogFBC(),
+			},
 		},
 		{
-			name: "DeclcfgDir/Failure",
+			name: "DeclcfgDir/Success",
 			migrate: action.Migrate{
 				CatalogRef: "testdata/foo-index-v0.2.0-declcfg",
 				OutputDir:  filepath.Join(tmpDir, "declcfg-dir"),
 				WriteFunc:  declcfg.WriteYAML,
 				FileExt:    ".yaml",
 				Registry:   reg,
 			},
-			expectErr: action.ErrNotAllowed,
+			expectedFiles: map[string]string{
+				"foo/catalog.yaml": migrateFooCatalogFBC(),
+			},
 		},
 		{
 			name: "BundleImage/Failure",
@@ -106,12 +110,22 @@ func TestMigrate(t *testing.T) {
 		t.Run(s.name, func(t *testing.T) {
 			err := s.migrate.Run(context.Background())
 			require.ErrorIs(t, err, s.expectErr)
-			for file, expectedData := range s.expectedFiles {
-				path := filepath.Join(s.migrate.OutputDir, file)
-				actualData, err := os.ReadFile(path)
+			if s.expectErr != nil {
+				return
+			}
+			actualFS := os.DirFS(s.migrate.OutputDir)
+			fs.WalkDir(actualFS, ".", func(path string, d fs.DirEntry, err error) error {
 				require.NoError(t, err)
+				if d.IsDir() {
+					return nil
+				}
+				actualData, err := fs.ReadFile(actualFS, path)
+				require.NoError(t, err)
+				expectedData, ok := s.expectedFiles[path]
+				require.True(t, ok, "output directory contained unexpected file %q", path)
 				require.Equal(t, expectedData, string(actualData))
-			}
+				return nil
+			})
 		})
 	}
 }
@@ -156,7 +170,7 @@ func newMigrateRegistry(t *testing.T, imageMap map[image.Reference]string) (imag
 	return reg, nil
 }
 
-func migrateFooCatalog() string {
+func migrateFooCatalogSqlite() string {
 	return `---
 defaultChannel: beta
 name: foo
@@ -280,7 +294,7 @@ schema: olm.bundle
 `
 }
 
-func migrateBarCatalog() string {
+func migrateBarCatalogSqlite() string {
 	return `---
 defaultChannel: alpha
 name: bar
@@ -357,3 +371,135 @@ relatedImages:
 schema: olm.bundle
 `
 }
+
+func migrateFooCatalogFBC() string {
+	return `---
+defaultChannel: beta
+name: foo
+properties:
+- type: owner
+  value:
+    group: abc.com
+    name: admin
+schema: olm.package
+---
+entries:
+- name: foo.v0.1.0
+  skipRange: <0.1.0
+- name: foo.v0.2.0
+  replaces: foo.v0.1.0
+  skipRange: <0.2.0
+  skips:
+  - foo.v0.1.1
+  - foo.v0.1.2
+name: beta
+package: foo
+properties:
+- type: user
+  value:
+    group: xyz.com
+    name: account
+schema: olm.channel
+---
+entries:
+- name: foo.v0.2.0
+  replaces: foo.v0.1.0
+  skipRange: <0.2.0
+  skips:
+  - foo.v0.1.1
+  - foo.v0.1.2
+name: stable
+package: foo
+schema: olm.channel
+---
+image: test.registry/foo-operator/foo-bundle:v0.1.0
+name: foo.v0.1.0
+package: foo
+properties:
+- type: olm.gvk
+  value:
+    group: test.foo
+    kind: Foo
+    version: v1
+- type: olm.gvk.required
+  value:
+    group: test.bar
+    kind: Bar
+    version: v1alpha1
+- type: olm.package
+  value:
+    packageName: foo
+    version: 0.1.0
+- type: olm.package.required
+  value:
+    packageName: bar
+    versionRange: <0.1.0
+- type: olm.csv.metadata
+  value:
+    annotations:
+      olm.skipRange: <0.1.0
+    apiServiceDefinitions: {}
+    crdDescriptions:
+      owned:
+      - kind: Foo
+        name: foos.test.foo
+        version: v1
+    displayName: Foo Operator
+    provider: {}
+relatedImages:
+- image: test.registry/foo-operator/foo-bundle:v0.1.0
+  name: ""
+- image: test.registry/foo-operator/foo:v0.1.0
+  name: operator
+schema: olm.bundle
+---
+image: test.registry/foo-operator/foo-bundle:v0.2.0
+name: foo.v0.2.0
+package: foo
+properties:
+- type: olm.gvk
+  value:
+    group: test.foo
+    kind: Foo
+    version: v1
+- type: olm.gvk.required
+  value:
+    group: test.bar
+    kind: Bar
+    version: v1alpha1
+- type: olm.package
+  value:
+    packageName: foo
+    version: 0.2.0
+- type: olm.package.required
+  value:
+    packageName: bar
+    versionRange: <0.1.0
+- type: olm.csv.metadata
+  value:
+    annotations:
+      olm.skipRange: <0.2.0
+    apiServiceDefinitions: {}
+    crdDescriptions:
+      owned:
+      - kind: Foo
+        name: foos.test.foo
+        version: v1
+    displayName: Foo Operator
+    provider: {}
+relatedImages:
+- image: test.registry/foo-operator/foo-2:v0.2.0
+  name: ""
+- image: test.registry/foo-operator/foo-bundle:v0.2.0
+  name: ""
+- image: test.registry/foo-operator/foo-init-2:v0.2.0
+  name: ""
+- image: test.registry/foo-operator/foo-init:v0.2.0
+  name: ""
+- image: test.registry/foo-operator/foo-other:v0.2.0
+  name: other
+- image: test.registry/foo-operator/foo:v0.2.0
+  name: operator
+schema: olm.bundle
+`
+}

staging/operator-registry/alpha/action/render.go

@@ -15,6 +15,7 @@ import (
 
 	"github.com/h2non/filetype"
 	"github.com/h2non/filetype/matchers"
+	"github.com/operator-framework/api/pkg/operators/v1alpha1"
 	"github.com/sirupsen/logrus"
 	"k8s.io/apimachinery/pkg/util/sets"
 
@@ -52,6 +53,7 @@ type Render struct {
 	Refs           []string
 	Registry       image.Registry
 	AllowedRefMask RefType
+	Migrate        bool
 
 	skipSqliteDeprecationLog bool
 }
@@ -90,6 +92,12 @@ func (r Render) Run(ctx context.Context) (*declcfg.DeclarativeConfig, error) {
 			})
 		}
 
+		if r.Migrate {
+			if err := migrate(cfg); err != nil {
+				return nil, fmt.Errorf("migrate: %v", err)
+			}
+		}
+
 		cfgs = append(cfgs, *cfg)
 	}
 
@@ -395,6 +403,50 @@ func moveBundleObjectsToEndOfPropertySlices(cfg *declcfg.DeclarativeConfig) {
 	}
 }
 
+func migrate(cfg *declcfg.DeclarativeConfig) error {
+	migrations := []func(*declcfg.DeclarativeConfig) error{
+		convertObjectsToCSVMetadata,
+	}
+
+	for _, m := range migrations {
+		if err := m(cfg); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+func convertObjectsToCSVMetadata(cfg *declcfg.DeclarativeConfig) error {
+BundleLoop:
+	for bi, b := range cfg.Bundles {
+		if b.Image == "" || b.CsvJSON == "" {
+			continue
+		}
+
+		var csv v1alpha1.ClusterServiceVersion
+		if err := json.Unmarshal([]byte(b.CsvJSON), &csv); err != nil {
+			return err
+		}
+
+		props := b.Properties[:0]
+		for _, p := range b.Properties {
+			switch p.Type {
+			case property.TypeBundleObject:
+				// Get rid of the bundle objects
+			case property.TypeCSVMetadata:
+				// If this bundle already has a CSV metadata
+				// property, we won't mutate the bundle at all.
+				continue BundleLoop
+			default:
+				// Keep all of the other properties
+				props = append(props, p)
+			}
+		}
+		cfg.Bundles[bi].Properties = append(props, property.MustBuildCSVMetadata(csv))
+	}
+	return nil
+}
+
 func combineConfigs(cfgs []declcfg.DeclarativeConfig) *declcfg.DeclarativeConfig {
 	out := &declcfg.DeclarativeConfig{}
 	for _, in := range cfgs {