From d3be37434cb15b1884e9da10565d0ef92b45c804 Mon Sep 17 00:00:00 2001 From: Ankita Thomas Date: Wed, 29 Sep 2021 16:12:50 -0400 Subject: [PATCH 1/3] clarify pruned bundle message Signed-off-by: Ankita Thomas --- pkg/lib/registry/registry.go | 6 +++++- pkg/lib/registry/registry_test.go | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/pkg/lib/registry/registry.go b/pkg/lib/registry/registry.go index 145923873..490707512 100644 --- a/pkg/lib/registry/registry.go +++ b/pkg/lib/registry/registry.go @@ -425,7 +425,11 @@ func checkForBundles(ctx context.Context, q *sqlite.SQLQuerier, g registry.Graph errs = append(errs, fmt.Errorf("could not validate pruned bundle %s (%s) as deprecated: %v", bundle.CsvName, bundle.BundlePath, err)) } if !deprecated { - errs = append(errs, fmt.Errorf("added bundle %s pruned from package %s, channel %s: this may be due to incorrect channel head (%s)", bundle.BundlePath, pkg.Name, channel, graph.Channels[channel].Head.CsvName)) + headSkips := []string{} + for b := range graph.Channels[channel].Nodes[graph.Channels[channel].Head] { + headSkips = append(headSkips, b.CsvName) + } + errs = append(errs, fmt.Errorf("add prunes bundle %s (%s, %s) from package %s, channel %s: this may be due to incorrect channel head (%s, skips/replaces %v)", bundle.CsvName, bundle.Version, bundle.BundlePath, pkg.Name, channel, graph.Channels[channel].Head.CsvName, headSkips)) } } } diff --git a/pkg/lib/registry/registry_test.go b/pkg/lib/registry/registry_test.go index edb7efe9d..c037da767 100644 --- a/pkg/lib/registry/registry_test.go +++ b/pkg/lib/registry/registry_test.go @@ -444,7 +444,7 @@ func TestCheckForBundles(t *testing.T) { }, }, }, - wantErr: fmt.Errorf("added bundle unorderedReplaces-1.0.0 pruned from package testpkg, channel stable: this may be due to incorrect channel head (unorderedReplaces-1.1.0)"), + wantErr: fmt.Errorf("add prunes bundle unorderedReplaces-1.0.0 (1.0.0, unorderedReplaces-1.0.0) from package testpkg, channel stable: this may be due to incorrect channel head (unorderedReplaces-1.1.0, skips/replaces [])"), }, { description: "ignoreDeprecated", From 12d4f7cdde47e05240c64fc036b668c8292987d2 Mon Sep 17 00:00:00 2001 From: Ankita Thomas Date: Wed, 29 Sep 2021 17:40:37 -0400 Subject: [PATCH 2/3] check only newly added bundles for pruning Signed-off-by: Ankita Thomas --- pkg/lib/registry/registry.go | 141 +++++------------ pkg/lib/registry/registry_test.go | 253 ++++++------------------------ 2 files changed, 89 insertions(+), 305 deletions(-) diff --git a/pkg/lib/registry/registry.go b/pkg/lib/registry/registry.go index 490707512..6a5b4b65e 100644 --- a/pkg/lib/registry/registry.go +++ b/pkg/lib/registry/registry.go @@ -159,12 +159,6 @@ func populate(ctx context.Context, loader registry.Load, graphLoader registry.Gr } } - expectedBundles, err := expectedGraphBundles(imagesToAdd, graphLoader, overwrite) - if err != nil { - - return err - - } populator := registry.NewDirectoryPopulator(loader, graphLoader, querier, unpackedImageMap, overwrittenBundles) if err := populator.Populate(mode); err != nil { @@ -172,7 +166,7 @@ func populate(ctx context.Context, loader registry.Load, graphLoader registry.Gr return err } - return checkForBundles(ctx, querier.(*sqlite.SQLQuerier), graphLoader, expectedBundles) + return checkForBundles(ctx, querier.(*sqlite.SQLQuerier), graphLoader, imagesToAdd) } type DeleteFromRegistryRequest struct { @@ -399,38 +393,57 @@ func checkForBundlePaths(querier registry.GRPCQuery, bundlePaths []string) ([]st // replaces mode selects highest version as channel head and // prunes any bundles in the upgrade chain after the channel head. -// check for the presence of all bundles after a replaces-mode add. -func checkForBundles(ctx context.Context, q *sqlite.SQLQuerier, g registry.GraphLoader, required map[string]*registry.Package) error { +// check for the presence of newly added bundles after a replaces-mode add. +func checkForBundles(ctx context.Context, q *sqlite.SQLQuerier, g registry.GraphLoader, required []*registry.Bundle) error { var errs []error - for _, pkg := range required { - graph, err := g.Generate(pkg.Name) - if err != nil { - errs = append(errs, fmt.Errorf("unable to verify added bundles for package %s: %v", pkg.Name, err)) - continue - } +checkRequired: + for _, bundle := range required { + var graph *registry.Package + var deprecated *bool + var err error + checkForBundleInChannel: + for _, channel := range bundle.Channels { + if graph == nil { + graph, err = g.Generate(bundle.Package) + if err != nil { + errs = append(errs, fmt.Errorf("unable to verify added bundles for package %s: %v", bundle.Package, err)) + continue + } + } - for channel, missing := range pkg.Channels { - // trace replaces chain for reachable bundles for next := []registry.BundleKey{graph.Channels[channel].Head}; len(next) > 0; next = next[1:] { - delete(missing.Nodes, next[0]) + if next[0].BundlePath == bundle.BundleImage { + continue checkForBundleInChannel + } for edge := range graph.Channels[channel].Nodes[next[0]] { next = append(next, edge) } } - for bundle := range missing.Nodes { - // check if bundle is deprecated. Bundles readded after deprecation should not be present in index and can be ignored. - deprecated, err := isDeprecated(ctx, q, bundle) + if deprecated == nil { + version, err := bundle.Version() if err != nil { - errs = append(errs, fmt.Errorf("could not validate pruned bundle %s (%s) as deprecated: %v", bundle.CsvName, bundle.BundlePath, err)) + errs = append(errs, fmt.Errorf("could not check pruned bundle %s (%s) for deprecation: %v", bundle.Name, bundle.BundleImage, err)) + continue checkRequired } - if !deprecated { - headSkips := []string{} - for b := range graph.Channels[channel].Nodes[graph.Channels[channel].Head] { - headSkips = append(headSkips, b.CsvName) - } - errs = append(errs, fmt.Errorf("add prunes bundle %s (%s, %s) from package %s, channel %s: this may be due to incorrect channel head (%s, skips/replaces %v)", bundle.CsvName, bundle.Version, bundle.BundlePath, pkg.Name, channel, graph.Channels[channel].Head.CsvName, headSkips)) + depr, err := isDeprecated(ctx, q, registry.BundleKey{ + BundlePath: bundle.BundleImage, + Version: version, + CsvName: bundle.Name, + }) + if err != nil { + errs = append(errs, fmt.Errorf("could not validate pruned bundle %s (%s) as deprecated: %v", bundle.Name, bundle.BundleImage, err)) + continue checkRequired + } + deprecated = &depr + } + + if !*deprecated { + headSkips := []string{} + for b := range graph.Channels[channel].Nodes[graph.Channels[channel].Head] { + headSkips = append(headSkips, b.CsvName) } + errs = append(errs, fmt.Errorf("add prunes bundle %s (%s) from package %s, channel %s: this may be due to incorrect channel head (%s, skips/replaces %v)", bundle.Name, bundle.BundleImage, bundle.Package, channel, graph.Channels[channel].Head.CsvName, headSkips)) } } } @@ -449,75 +462,3 @@ func isDeprecated(ctx context.Context, q *sqlite.SQLQuerier, bundle registry.Bun } return false, nil } - -// expectedGraphBundles returns a set of package-channel-bundle tuples that MUST be present following an add. -/* opm index add drops bundles that replace a channel head, and since channel head selection heuristics -* choose the bundle with the greatest semver as the channel head, any bundle that replaces such a bundle -* will be dropped from the graph following an add. -* eg: 1.0.1 <- 1.0.1-new -* -* 1.0.1-new replaces 1.0.1 but will not be chosen as the channel head because of its non-empty pre-release version. -* expectedGraphBundles gives a set of bundles (old bundles from the graphLoader and the newly added set of bundles from -* imagesToAdd) that must be present following an add to ensure no bundle is dropped. -* -* Overwritten bundles will only be verified on the channels of the newly added version. -* Any inherited channels due to addition of a new bundle on its tail bundles may not be verified -* eg: 1.0.1 (alpha) <-[1.0.2 (alpha, stable)] -* When 1.0.2 in alpha and stable channels is added replacing 1.0.1, 1.0.1's presence will only be marked as expected on -* the alpha channel, not on the inherited stable channel. - */ -func expectedGraphBundles(imagesToAdd []*registry.Bundle, graphLoader registry.GraphLoader, overwrite bool) (map[string]*registry.Package, error) { - expectedBundles := map[string]*registry.Package{} - for _, bundle := range imagesToAdd { - version, err := bundle.Version() - if err != nil { - return nil, err - } - newBundleKey := registry.BundleKey{ - BundlePath: bundle.BundleImage, - Version: version, - CsvName: bundle.Name, - } - var pkg *registry.Package - var ok bool - if pkg, ok = expectedBundles[bundle.Package]; !ok { - var err error - if pkg, err = graphLoader.Generate(bundle.Package); err != nil { - if err != registry.ErrPackageNotInDatabase { - return nil, err - } - pkg = ®istry.Package{ - Name: bundle.Package, - Channels: map[string]registry.Channel{}, - } - } - } - for c, channelEntries := range pkg.Channels { - for oldBundle := range channelEntries.Nodes { - if oldBundle.CsvName == bundle.Name { - if overwrite { - delete(pkg.Channels[c].Nodes, oldBundle) - if len(pkg.Channels[c].Nodes) == 0 { - delete(pkg.Channels, c) - } - } else { - return nil, registry.BundleImageAlreadyAddedErr{ErrorString: fmt.Sprintf("Bundle %s already exists", bundle.BundleImage)} - } - } - } - } - for _, c := range bundle.Channels { - if _, ok := pkg.Channels[c]; !ok { - pkg.Channels[c] = registry.Channel{ - Nodes: map[registry.BundleKey]map[registry.BundleKey]struct{}{}, - } - } - // This can miss out on some channels, when a new bundle has channels that the one it replaces does not. - // eg: When bundle A in channel A replaces bundle B in channel B is added, bundle B is also added to channel A - // but it is only expected to be in channel B, presence in channel A will be ignored - pkg.Channels[c].Nodes[newBundleKey] = nil - } - expectedBundles[bundle.Package] = pkg - } - return expectedBundles, nil -} diff --git a/pkg/lib/registry/registry_test.go b/pkg/lib/registry/registry_test.go index c037da767..8dd6a010e 100644 --- a/pkg/lib/registry/registry_test.go +++ b/pkg/lib/registry/registry_test.go @@ -24,7 +24,6 @@ import ( "github.com/operator-framework/operator-registry/pkg/image" "github.com/operator-framework/operator-registry/pkg/lib/bundle" "github.com/operator-framework/operator-registry/pkg/registry" - "github.com/operator-framework/operator-registry/pkg/registry/registryfakes" "github.com/operator-framework/operator-registry/pkg/sqlite" "github.com/operator-framework/operator-registry/pkg/sqlite/sqlitefakes" ) @@ -351,9 +350,8 @@ type bundleDir struct { func TestCheckForBundles(t *testing.T) { type step struct { - bundles map[string]bundleDir - action int - expected map[string]*registry.Package + bundles map[string]bundleDir + action int } const ( actionAdd = iota @@ -369,7 +367,7 @@ func TestCheckForBundles(t *testing.T) { { // 1.1.0 -> 1.0.0 pruned channel 1 // \-> 1.2.0 ok channel 2 - description: "partialPruning", + description: "ErrorOnNewPrunedBundle", steps: []step{ { bundles: map[string]bundleDir{ @@ -402,49 +400,42 @@ func TestCheckForBundles(t *testing.T) { }, }, action: actionAdd, - expected: map[string]*registry.Package{ - "testpkg": { - Name: "testpkg", - Channels: map[string]registry.Channel{ - "alpha": { - Nodes: map[registry.BundleKey]map[registry.BundleKey]struct{}{ - registry.BundleKey{ - BundlePath: "unorderedReplaces-1.0.0", - Version: "1.0.0", - CsvName: "unorderedReplaces-1.0.0", - }: nil, - registry.BundleKey{ - BundlePath: "unorderedReplaces-1.1.0", - Version: "1.1.0", - CsvName: "unorderedReplaces-1.1.0", - }: nil, - registry.BundleKey{ - BundlePath: "unorderedReplaces-1.2.0", - Version: "1.2.0", - CsvName: "unorderedReplaces-1.2.0", - }: nil, - }, - }, - "stable": { - Nodes: map[registry.BundleKey]map[registry.BundleKey]struct{}{ - registry.BundleKey{ - BundlePath: "unorderedReplaces-1.0.0", - Version: "1.0.0", - CsvName: "unorderedReplaces-1.0.0", - }: nil, - registry.BundleKey{ - BundlePath: "unorderedReplaces-1.1.0", - Version: "1.1.0", - CsvName: "unorderedReplaces-1.1.0", - }: nil, - }, - }, + }, + }, + wantErr: fmt.Errorf("add prunes bundle unorderedReplaces-1.0.0 (unorderedReplaces-1.0.0) from package testpkg, channel stable: this may be due to incorrect channel head (unorderedReplaces-1.1.0, skips/replaces [])"), + }, + { + description: "silentPruneForExistingBundle", + steps: []step{ + { + bundles: map[string]bundleDir{ + "unorderedReplaces-1.0.0": { + csvSpec: json.RawMessage(`{"version":"1.0.0"}`), + annotations: registry.Annotations{ + PackageName: "testpkg", + Channels: "stable,alpha", + DefaultChannelName: "stable", + }, + version: "1.0.0", + }, + }, + action: actionAdd, + }, + { + bundles: map[string]bundleDir{ + "unorderedReplaces-1.2.0": { + csvSpec: json.RawMessage(`{"version":"1.2.0"}`), + annotations: registry.Annotations{ + PackageName: "testpkg", + Channels: "alpha", + DefaultChannelName: "stable", }, + version: "1.2.0", }, }, + action: actionAdd, }, }, - wantErr: fmt.Errorf("add prunes bundle unorderedReplaces-1.0.0 (1.0.0, unorderedReplaces-1.0.0) from package testpkg, channel stable: this may be due to incorrect channel head (unorderedReplaces-1.1.0, skips/replaces [])"), }, { description: "ignoreDeprecated", @@ -477,59 +468,12 @@ func TestCheckForBundles(t *testing.T) { }, }, action: actionAdd, - expected: map[string]*registry.Package{ - "testpkg": { - Name: "testpkg", - Channels: map[string]registry.Channel{ - "stable": { - Nodes: map[registry.BundleKey]map[registry.BundleKey]struct{}{ - registry.BundleKey{ - BundlePath: "ignoreDeprecated-1.0.0", - Version: "1.0.0", - CsvName: "ignoreDeprecated-1.0.0", - }: nil, - registry.BundleKey{ - BundlePath: "ignoreDeprecated-1.1.0", - Version: "1.1.0", - CsvName: "ignoreDeprecated-1.1.0", - }: nil, - registry.BundleKey{ - BundlePath: "ignoreDeprecated-1.2.0", - Version: "1.2.0", - CsvName: "ignoreDeprecated-1.2.0", - }: nil, - }, - }, - }, - }, - }, }, { bundles: map[string]bundleDir{ "ignoreDeprecated-1.1.0": {}, }, action: actionDeprecate, - expected: map[string]*registry.Package{ - "testpkg": { - Name: "testpkg", - Channels: map[string]registry.Channel{ - "stable": { - Nodes: map[registry.BundleKey]map[registry.BundleKey]struct{}{ - registry.BundleKey{ - BundlePath: "ignoreDeprecated-1.1.0", - Version: "1.1.0", - CsvName: "ignoreDeprecated-1.1.0", - }: nil, - registry.BundleKey{ - BundlePath: "ignoreDeprecated-1.2.0", - Version: "1.2.0", - CsvName: "ignoreDeprecated-1.2.0", - }: nil, - }, - }, - }, - }, - }, }, { bundles: map[string]bundleDir{ @@ -544,31 +488,6 @@ func TestCheckForBundles(t *testing.T) { }, }, action: actionOverwrite, - expected: map[string]*registry.Package{ - "testpkg": { - Name: "testpkg", - Channels: map[string]registry.Channel{ - "stable": { - Nodes: map[registry.BundleKey]map[registry.BundleKey]struct{}{ - registry.BundleKey{ - BundlePath: "ignoreDeprecated-1.1.0", - Version: "1.1.0", - CsvName: "ignoreDeprecated-1.1.0", - }: nil, - }, - }, - "alpha": { - Nodes: map[registry.BundleKey]map[registry.BundleKey]struct{}{ - registry.BundleKey{ - BundlePath: "ignoreDeprecated-1.2.0-overwrite", - Version: "1.2.0", - CsvName: "ignoreDeprecated-1.2.0", - }: nil, - }, - }, - }, - }, - }, }, }, }, @@ -596,6 +515,7 @@ func TestCheckForBundles(t *testing.T) { case actionAdd, actionOverwrite: overwriteRefs := map[string][]string{} refs := map[image.Reference]string{} + expected := []*registry.Bundle{} for name, b := range step.bundles { dir, _, err := newUnpackedTestBundle(tmpdir, name, b.csvSpec, b.annotations, true) require.NoError(t, err) @@ -606,8 +526,13 @@ func TestCheckForBundles(t *testing.T) { // bundles to remove for overwrite. Only one per package is permitted. if step.action == actionOverwrite { bundleImage += "-overwrite" - img, err := registry.NewImageInput(image.SimpleReference(bundleImage), dir) - require.NoError(t, err) + } + + img, err := registry.NewImageInput(image.SimpleReference(bundleImage), dir) + require.NoError(t, err) + expected = append(expected, img.Bundle) + + if step.action == actionOverwrite { overwriteRefs[img.Bundle.Package] = append(overwriteRefs[img.Bundle.Package], name) } refs[image.SimpleReference(bundleImage)] = dir @@ -619,14 +544,13 @@ func TestCheckForBundles(t *testing.T) { query, refs, overwriteRefs).Populate(registry.ReplacesMode)) - - } - err = checkForBundles(context.TODO(), query, graphLoader, step.expected) - if tt.wantErr == nil { - require.NoError(t, err, fmt.Sprintf("%d", step.action)) - continue + err = checkForBundles(context.TODO(), query, graphLoader, expected) + if tt.wantErr == nil { + require.NoError(t, err, fmt.Sprintf("%d", step.action)) + continue + } + require.EqualError(t, err, tt.wantErr.Error()) } - require.EqualError(t, err, tt.wantErr.Error()) } }) } @@ -678,84 +602,3 @@ func TestDeprecated(t *testing.T) { require.Equal(t, deprecated[b], isDeprecated) } } - -func TestExpectedGraphBundles(t *testing.T) { - testBundle, err := registry.NewBundleFromStrings("testBundle", "0.0.1", "testPkg", "default", "default", "") - require.NoError(t, err) - testBundle.BundleImage = "testImage" - testBundleKey := registry.BundleKey{ - BundlePath: testBundle.BundleImage, - Version: "0.0.1", - CsvName: testBundle.Name, - } - newTestPackage := func(name string, channelEntries map[string]registry.BundleKey) *registry.Package { - channels := map[string]registry.Channel{} - for channelName, node := range channelEntries { - if _, ok := channels[channelName]; !ok { - channels[channelName] = registry.Channel{ - Nodes: map[registry.BundleKey]map[registry.BundleKey]struct{}{}, - } - } - channels[channelName].Nodes[node] = nil - } - return ®istry.Package{ - Name: name, - Channels: channels, - } - } - - tests := []struct { - description string - graphLoader registry.GraphLoader - bundles []*registry.Bundle - overwrite bool - wantErr error - wantGraphBundles map[string]*registry.Package - }{ - { - description: "GraphLoaderError", - graphLoader: ®istryfakes.FakeGraphLoader{GenerateStub: func(string) (*registry.Package, error) { return nil, fmt.Errorf("graphLoader error") }}, - bundles: []*registry.Bundle{testBundle}, - wantErr: fmt.Errorf("graphLoader error"), - }, - { - description: "NewPackage", - graphLoader: ®istryfakes.FakeGraphLoader{GenerateStub: func(string) (*registry.Package, error) { return nil, registry.ErrPackageNotInDatabase }}, - bundles: []*registry.Bundle{testBundle}, - wantGraphBundles: map[string]*registry.Package{ - "testPkg": newTestPackage("testPkg", map[string]registry.BundleKey{"default": testBundleKey}), - }, - }, - { - description: "OverwriteWithoutFlag", - graphLoader: ®istryfakes.FakeGraphLoader{GenerateStub: func(string) (*registry.Package, error) { - return newTestPackage("testPkg", map[string]registry.BundleKey{"alpha": testBundleKey}), nil - }}, - bundles: []*registry.Bundle{testBundle}, - wantErr: registry.BundleImageAlreadyAddedErr{ErrorString: fmt.Sprintf("Bundle %s already exists", testBundle.BundleImage)}, - }, - { - description: "OverwriteWithFlag", - graphLoader: ®istryfakes.FakeGraphLoader{GenerateStub: func(string) (*registry.Package, error) { - return newTestPackage("testPkg", map[string]registry.BundleKey{"alpha": testBundleKey}), nil - }}, - bundles: []*registry.Bundle{testBundle}, - overwrite: true, - wantGraphBundles: map[string]*registry.Package{ - "testPkg": newTestPackage("testPkg", map[string]registry.BundleKey{"default": testBundleKey}), - }, - }, - } - for _, tt := range tests { - t.Run(tt.description, func(t *testing.T) { - graphBundles, err := expectedGraphBundles(tt.bundles, tt.graphLoader, tt.overwrite) - if tt.wantErr != nil { - require.EqualError(t, err, tt.wantErr.Error()) - return - } - require.NoError(t, err) - - require.EqualValues(t, graphBundles, tt.wantGraphBundles) - }) - } -} From a4f3f46e63f2948c494a81158ea30c15ebaa98f6 Mon Sep 17 00:00:00 2001 From: Ankita Thomas Date: Thu, 30 Sep 2021 18:26:54 -0400 Subject: [PATCH 3/3] update tests, remove deprecation check on added bundles Signed-off-by: Ankita Thomas --- pkg/lib/registry/registry.go | 64 ++------ pkg/lib/registry/registry_test.go | 247 ++++++++++++++++++++---------- 2 files changed, 183 insertions(+), 128 deletions(-) diff --git a/pkg/lib/registry/registry.go b/pkg/lib/registry/registry.go index 6a5b4b65e..073c622a6 100644 --- a/pkg/lib/registry/registry.go +++ b/pkg/lib/registry/registry.go @@ -396,69 +396,35 @@ func checkForBundlePaths(querier registry.GRPCQuery, bundlePaths []string) ([]st // check for the presence of newly added bundles after a replaces-mode add. func checkForBundles(ctx context.Context, q *sqlite.SQLQuerier, g registry.GraphLoader, required []*registry.Bundle) error { var errs []error -checkRequired: for _, bundle := range required { - var graph *registry.Package - var deprecated *bool - var err error - checkForBundleInChannel: - for _, channel := range bundle.Channels { - if graph == nil { - graph, err = g.Generate(bundle.Package) - if err != nil { - errs = append(errs, fmt.Errorf("unable to verify added bundles for package %s: %v", bundle.Package, err)) - continue - } - } + graph, err := g.Generate(bundle.Package) + if err != nil { + errs = append(errs, fmt.Errorf("unable to verify added bundles for package %s: %v", bundle.Package, err)) + continue + } + for _, channel := range bundle.Channels { + var foundImage bool for next := []registry.BundleKey{graph.Channels[channel].Head}; len(next) > 0; next = next[1:] { if next[0].BundlePath == bundle.BundleImage { - continue checkForBundleInChannel + foundImage = true + break } for edge := range graph.Channels[channel].Nodes[next[0]] { next = append(next, edge) } } - if deprecated == nil { - version, err := bundle.Version() - if err != nil { - errs = append(errs, fmt.Errorf("could not check pruned bundle %s (%s) for deprecation: %v", bundle.Name, bundle.BundleImage, err)) - continue checkRequired - } - depr, err := isDeprecated(ctx, q, registry.BundleKey{ - BundlePath: bundle.BundleImage, - Version: version, - CsvName: bundle.Name, - }) - if err != nil { - errs = append(errs, fmt.Errorf("could not validate pruned bundle %s (%s) as deprecated: %v", bundle.Name, bundle.BundleImage, err)) - continue checkRequired - } - deprecated = &depr + if foundImage { + continue } - if !*deprecated { - headSkips := []string{} - for b := range graph.Channels[channel].Nodes[graph.Channels[channel].Head] { - headSkips = append(headSkips, b.CsvName) - } - errs = append(errs, fmt.Errorf("add prunes bundle %s (%s) from package %s, channel %s: this may be due to incorrect channel head (%s, skips/replaces %v)", bundle.Name, bundle.BundleImage, bundle.Package, channel, graph.Channels[channel].Head.CsvName, headSkips)) + var headSkips []string + for b := range graph.Channels[channel].Nodes[graph.Channels[channel].Head] { + headSkips = append(headSkips, b.CsvName) } + errs = append(errs, fmt.Errorf("add prunes bundle %s (%s) from package %s, channel %s: this may be due to incorrect channel head (%s, skips/replaces %v)", bundle.Name, bundle.BundleImage, bundle.Package, channel, graph.Channels[channel].Head.CsvName, headSkips)) } } return utilerrors.NewAggregate(errs) } - -func isDeprecated(ctx context.Context, q *sqlite.SQLQuerier, bundle registry.BundleKey) (bool, error) { - props, err := q.GetPropertiesForBundle(ctx, bundle.CsvName, bundle.Version, bundle.BundlePath) - if err != nil { - return false, err - } - for _, prop := range props { - if prop.Type == registry.DeprecatedType { - return true, nil - } - } - return false, nil -} diff --git a/pkg/lib/registry/registry_test.go b/pkg/lib/registry/registry_test.go index 8dd6a010e..4ff3b812e 100644 --- a/pkg/lib/registry/registry_test.go +++ b/pkg/lib/registry/registry_test.go @@ -25,7 +25,6 @@ import ( "github.com/operator-framework/operator-registry/pkg/lib/bundle" "github.com/operator-framework/operator-registry/pkg/registry" "github.com/operator-framework/operator-registry/pkg/sqlite" - "github.com/operator-framework/operator-registry/pkg/sqlite/sqlitefakes" ) func fakeBundlePathFromName(name string) string { @@ -346,12 +345,15 @@ type bundleDir struct { version string csvSpec json.RawMessage annotations registry.Annotations + isOverwrite bool } func TestCheckForBundles(t *testing.T) { type step struct { - bundles map[string]bundleDir - action int + bundles map[string]bundleDir + action int + expected []*registry.Bundle // For testing pruning after deprecation + wantErr error } const ( actionAdd = iota @@ -361,17 +363,16 @@ func TestCheckForBundles(t *testing.T) { tests := []struct { description string steps []step - wantErr error init func() (*sql.DB, func()) }{ { - // 1.1.0 -> 1.0.0 pruned channel 1 - // \-> 1.2.0 ok channel 2 + // 1.1.0 -> 1.2.0 ok channel 1 + // \-> 1.2.0-1 pruned channel 2 description: "ErrorOnNewPrunedBundle", steps: []step{ { bundles: map[string]bundleDir{ - "unorderedReplaces-1.1.0": { + "newPruned-1.1.0": { csvSpec: json.RawMessage(`{"version":"1.1.0"}`), annotations: registry.Annotations{ PackageName: "testpkg", @@ -380,8 +381,42 @@ func TestCheckForBundles(t *testing.T) { }, version: "1.1.0", }, - "unorderedReplaces-1.0.0": { - csvSpec: json.RawMessage(`{"version":"1.0.0","replaces":"unorderedReplaces-1.1.0"}`), + }, + action: actionAdd, + }, + { + bundles: map[string]bundleDir{ + "newPruned-1.2.0": { + csvSpec: json.RawMessage(`{"version":"1.2.0","replaces":"newPruned-1.1.0"}`), + annotations: registry.Annotations{ + PackageName: "testpkg", + Channels: "alpha", + DefaultChannelName: "stable", + }, + version: "1.2.0", + }, + "newPruned-1.2.0-1": { + csvSpec: json.RawMessage(`{"version":"1.2.0-1","replaces":"newPruned-1.2.0"}`), + annotations: registry.Annotations{ + PackageName: "testpkg", + Channels: "stable,alpha", + DefaultChannelName: "stable", + }, + version: "1.2.0-1", + }, + }, + action: actionAdd, + wantErr: fmt.Errorf("add prunes bundle newPruned-1.2.0-1 (newPruned-1.2.0-1) from package testpkg, channel alpha: this may be due to incorrect channel head (newPruned-1.2.0, skips/replaces [newPruned-1.1.0])"), + }, + }, + }, + { + description: "silentPruneForExistingBundle", + steps: []step{ + { + bundles: map[string]bundleDir{ + "silentPrune-1.0.0": { + csvSpec: json.RawMessage(`{"version":"1.0.0"}`), annotations: registry.Annotations{ PackageName: "testpkg", Channels: "stable,alpha", @@ -389,8 +424,22 @@ func TestCheckForBundles(t *testing.T) { }, version: "1.0.0", }, - "unorderedReplaces-1.2.0": { - csvSpec: json.RawMessage(`{"version":"1.2.0","replaces":"unorderedReplaces-1.0.0"}`), + "silentPrune-1.1.0": { + csvSpec: json.RawMessage(`{"version":"1.1.0","replaces":"silentPrune-1.0.0"}`), + annotations: registry.Annotations{ + PackageName: "testpkg", + Channels: "stable,alpha", + DefaultChannelName: "stable", + }, + version: "1.1.0", + }, + }, + action: actionAdd, + }, + { + bundles: map[string]bundleDir{ + "silentPrune-1.2.0": { + csvSpec: json.RawMessage(`{"version":"1.2.0","replaces":"silentPrune-1.0.0"}`), annotations: registry.Annotations{ PackageName: "testpkg", Channels: "alpha", @@ -402,10 +451,11 @@ func TestCheckForBundles(t *testing.T) { action: actionAdd, }, }, - wantErr: fmt.Errorf("add prunes bundle unorderedReplaces-1.0.0 (unorderedReplaces-1.0.0) from package testpkg, channel stable: this may be due to incorrect channel head (unorderedReplaces-1.1.0, skips/replaces [])"), }, { - description: "silentPruneForExistingBundle", + // 1.0.0 <- 1.0.1 <- 1.0.1-1 <- 1.0.2 (head) + // No pruning despite chain being out of order for 1.0.1 <- 1.0.1-1 + description: "allowUnorderedWithMaxChannelHead", steps: []step{ { bundles: map[string]bundleDir{ @@ -418,13 +468,31 @@ func TestCheckForBundles(t *testing.T) { }, version: "1.0.0", }, + "unorderedReplaces-1.1.0": { + csvSpec: json.RawMessage(`{"version":"1.1.0","replaces":"unorderedReplaces-1.0.0"}`), + annotations: registry.Annotations{ + PackageName: "testpkg", + Channels: "stable,alpha", + DefaultChannelName: "stable", + }, + version: "1.1.0", + }, }, action: actionAdd, }, { bundles: map[string]bundleDir{ + "unorderedReplaces-1.1.0-1": { + csvSpec: json.RawMessage(`{"version":"1.1.0-1","replaces":"unorderedReplaces-1.1.0"}`), + annotations: registry.Annotations{ + PackageName: "testpkg", + Channels: "alpha", + DefaultChannelName: "stable", + }, + version: "1.1.0-1", + }, "unorderedReplaces-1.2.0": { - csvSpec: json.RawMessage(`{"version":"1.2.0"}`), + csvSpec: json.RawMessage(`{"version":"1.2.0","replaces":"unorderedReplaces-1.1.0-1"}`), annotations: registry.Annotations{ PackageName: "testpkg", Channels: "alpha", @@ -438,11 +506,12 @@ func TestCheckForBundles(t *testing.T) { }, }, { - description: "ignoreDeprecated", + // If a pruned bundle was deprecated, ignore + description: "withDeprecated", steps: []step{ { bundles: map[string]bundleDir{ - "ignoreDeprecated-1.0.0": { + "withDeprecated-1.0.0": { csvSpec: json.RawMessage(`{"version":"1.0.0"}`), annotations: registry.Annotations{ PackageName: "testpkg", @@ -450,16 +519,16 @@ func TestCheckForBundles(t *testing.T) { }, version: "1.0.0", }, - "ignoreDeprecated-1.1.0": { - csvSpec: json.RawMessage(`{"version":"1.1.0","replaces":"ignoreDeprecated-1.0.0"}`), + "withDeprecated-1.1.0": { + csvSpec: json.RawMessage(`{"version":"1.1.0","replaces":"withDeprecated-1.0.0"}`), annotations: registry.Annotations{ PackageName: "testpkg", Channels: "stable", }, version: "1.1.0", }, - "ignoreDeprecated-1.2.0": { - csvSpec: json.RawMessage(`{"version":"1.2.0","replaces":"ignoreDeprecated-1.1.0"}`), + "withDeprecated-1.2.0": { + csvSpec: json.RawMessage(`{"version":"1.2.0","replaces":"withDeprecated-1.1.0"}`), annotations: registry.Annotations{ PackageName: "testpkg", Channels: "stable", @@ -471,26 +540,92 @@ func TestCheckForBundles(t *testing.T) { }, { bundles: map[string]bundleDir{ - "ignoreDeprecated-1.1.0": {}, + "withDeprecated-1.1.0": {}, }, action: actionDeprecate, + expected: []*registry.Bundle{ + { + Name: "withDeprecated-1.1.0", + Package: "testpkg", + Channels: []string{"stable"}, + BundleImage: "withDeprecated-1.1.0", + }, + { + Name: "withDeprecated-1.2.0", + Package: "testpkg", + Channels: []string{"stable"}, + BundleImage: "withDeprecated-1.2.0", + }, + }, }, { bundles: map[string]bundleDir{ - "ignoreDeprecated-1.2.0": { + "withDeprecated-1.2.0": { csvSpec: json.RawMessage(`{"version":"1.2.0","replaces":""}`), annotations: registry.Annotations{ PackageName: "testpkg", Channels: "alpha", DefaultChannelName: "alpha", }, - version: "1.2.0", + version: "1.2.0", + isOverwrite: true, }, }, action: actionOverwrite, }, }, }, + { + // bundle version should be immutable anyway, but only csv name is required to stay unchanged in overwrite + description: "overwritePruning", + steps: []step{ + { + bundles: map[string]bundleDir{ + "withOverwrite-1.0.0": { + csvSpec: json.RawMessage(`{"version":"1.0.0"}`), + annotations: registry.Annotations{ + PackageName: "testpkg", + Channels: "stable", + }, + version: "1.0.0", + }, + "withOverwrite-1.1.0": { + csvSpec: json.RawMessage(`{"version":"1.1.0","replaces":"withOverwrite-1.0.0"}`), + annotations: registry.Annotations{ + PackageName: "testpkg", + Channels: "stable", + }, + version: "1.1.0", + }, + }, + action: actionAdd, + }, + { + bundles: map[string]bundleDir{ + "withOverwrite-1.1.0": { + csvSpec: json.RawMessage(`{"version":"1.0.0-1","replaces":"withOverwrite-1.0.0"}`), + annotations: registry.Annotations{ + PackageName: "testpkg", + Channels: "stable", + }, + version: "1.0.0-1", + isOverwrite: true, + }, + "withOverwrite-1.2.0": { + csvSpec: json.RawMessage(`{"version":"1.2.0","replaces":"withOverwrite-1.1.0"}`), + annotations: registry.Annotations{ + PackageName: "testpkg", + Channels: "alpha", + DefaultChannelName: "stable", + }, + version: "1.2.0", + }, + }, + action: actionOverwrite, + wantErr: fmt.Errorf("add prunes bundle withOverwrite-1.1.0 (withOverwrite-1.1.0-overwrite) from package testpkg, channel stable: this may be due to incorrect channel head (withOverwrite-1.0.0, skips/replaces [])"), + }, + }, + }, } for _, tt := range tests { @@ -507,15 +642,16 @@ func TestCheckForBundles(t *testing.T) { require.NoError(t, err) for _, step := range tt.steps { + expected := []*registry.Bundle{} switch step.action { case actionDeprecate: for deprecate := range step.bundles { require.NoError(t, load.DeprecateBundle(deprecate)) } + expected = step.expected case actionAdd, actionOverwrite: overwriteRefs := map[string][]string{} refs := map[image.Reference]string{} - expected := []*registry.Bundle{} for name, b := range step.bundles { dir, _, err := newUnpackedTestBundle(tmpdir, name, b.csvSpec, b.annotations, true) require.NoError(t, err) @@ -524,7 +660,7 @@ func TestCheckForBundles(t *testing.T) { bundleImage := name // bundles to remove for overwrite. Only one per package is permitted. - if step.action == actionOverwrite { + if step.action == actionOverwrite && b.isOverwrite { bundleImage += "-overwrite" } @@ -532,7 +668,7 @@ func TestCheckForBundles(t *testing.T) { require.NoError(t, err) expected = append(expected, img.Bundle) - if step.action == actionOverwrite { + if step.action == actionOverwrite && b.isOverwrite { overwriteRefs[img.Bundle.Package] = append(overwriteRefs[img.Bundle.Package], name) } refs[image.SimpleReference(bundleImage)] = dir @@ -544,61 +680,14 @@ func TestCheckForBundles(t *testing.T) { query, refs, overwriteRefs).Populate(registry.ReplacesMode)) - err = checkForBundles(context.TODO(), query, graphLoader, expected) - if tt.wantErr == nil { - require.NoError(t, err, fmt.Sprintf("%d", step.action)) - continue - } - require.EqualError(t, err, tt.wantErr.Error()) } + err = checkForBundles(context.TODO(), query, graphLoader, expected) + if step.wantErr == nil { + require.NoError(t, err, fmt.Sprintf("%d", step.action)) + continue + } + require.EqualError(t, err, step.wantErr.Error()) } }) } } - -func TestDeprecated(t *testing.T) { - deprecated := map[string]bool{ - "deprecatedBundle": true, - "otherBundle": false, - } - q := &sqlitefakes.FakeQuerier{ - QueryContextStub: func(ctx context.Context, query string, args ...interface{}) (sqlite.RowScanner, error) { - bundleName := args[2].(string) - if len(bundleName) == 0 { - return nil, fmt.Errorf("empty bundle name") - } - hasNext := true - return &sqlitefakes.FakeRowScanner{ScanStub: func(args ...interface{}) error { - if deprecated[bundleName] { - *args[0].(*sql.NullString) = sql.NullString{ - String: registry.DeprecatedType, - Valid: true, - } - *args[1].(*sql.NullString) = sql.NullString{ - Valid: true, - } - } - return nil - }, - NextStub: func() bool { - if hasNext { - hasNext = false - return true - } - return false - }, - }, nil - }, - } - - querier := sqlite.NewSQLLiteQuerierFromDBQuerier(q) - - _, err := isDeprecated(context.TODO(), querier, registry.BundleKey{}) - require.Error(t, err) - - for b := range deprecated { - isDeprecated, err := isDeprecated(context.TODO(), querier, registry.BundleKey{BundlePath: b}) - require.NoError(t, err) - require.Equal(t, deprecated[b], isDeprecated) - } -}