Skip to content

Commit

Permalink
migrate: support migration of FBC to latest preferred FBC (#1144)
Browse files Browse the repository at this point in the history 
* 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]>
  • Loading branch information
@joelanford
joelanford authored Oct 10, 2023
1 parent eaebcf6 commit 5e3fa99
Show file tree
Hide file tree
Showing 15 changed files with 464 additions and 300 deletions.
11 changes: 4 additions & 7 deletions alpha/action/migrate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
174 changes: 160 additions & 14 deletions alpha/action/migrate_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
},
},
{
Expand All @@ -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",
Expand All @@ -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
})
})
}
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -280,7 +294,7 @@ schema: olm.bundle
`
}

func migrateBarCatalog() string {
func migrateBarCatalogSqlite() string {
return `---
defaultChannel: alpha
name: bar
Expand Down Expand Up @@ -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
`
}
52 changes: 52 additions & 0 deletions alpha/action/render.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -52,6 +53,7 @@ type Render struct {
Refs []string
Registry image.Registry
AllowedRefMask RefType
Migrate bool

skipSqliteDeprecationLog bool
}
Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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 {
Expand Down
Loading

0 comments on commit 5e3fa99

Please sign in to comment.