fix(deprecate): explicit pkg rm drops latest deprecated (#746)

- When using opm index|registry rm <pkg>, drop the "latest" related bundles
from the deprecated table. This makes "un-deprecating" a bundle
possible, which is useful if that  bundle was deprecated by
accident
- Add a migration and logic on deprecatetruncate that drops
truncated bundles (i.e. w/o assoc. channel_entry rows) from the deprecated
table, clearing the "deprecation history" so that operations post package
removal don't assume a truncated bundle should be deprecated

See https://bugzilla.redhat.com/show_bug.cgi?id=1982781 for motivation

Signed-off-by: Nick Hale <[email protected]>

Author: njhale

pkg/lib/registry/registry.go

@@ -221,7 +221,7 @@ func (r RegistryUpdater) DeleteFromRegistry(request DeleteFromRegistryRequest) e
 	}
 	defer db.Close()
 
-	dbLoader, err := sqlite.NewSQLLiteLoader(db)
+	dbLoader, err := sqlite.NewDeprecationAwareLoader(db)
 	if err != nil {
 		return err
 	}

pkg/sqlite/load.go

@@ -28,7 +28,7 @@ type MigratableLoader interface {
 
 var _ MigratableLoader = &sqlLoader{}
 
-func NewSQLLiteLoader(db *sql.DB, opts ...DbOption) (MigratableLoader, error) {
+func newSQLLoader(db *sql.DB, opts ...DbOption) (*sqlLoader, error) {
 	options := defaultDBOptions()
 	for _, o := range opts {
 		o(options)
@@ -46,6 +46,10 @@ func NewSQLLiteLoader(db *sql.DB, opts ...DbOption) (MigratableLoader, error) {
 	return &sqlLoader{db: db, migrator: migrator, enableAlpha: options.EnableAlpha}, nil
 }
 
+func NewSQLLiteLoader(db *sql.DB, opts ...DbOption) (MigratableLoader, error) {
+	return newSQLLoader(db, opts...)
+}
+
 func (s *sqlLoader) Migrate(ctx context.Context) error {
 	if s.migrator == nil {
 		return fmt.Errorf("no migrator configured")
@@ -1462,6 +1466,13 @@ func (s *sqlLoader) DeprecateBundle(path string) error {
 		return err
 	}
 
+	// Clean up the deprecated table by dropping all truncated bundles
+	// (see pkg/sqlite/migrations/013_rm_truncated_deprecations.go for more details)
+	_, err = tx.Exec(`DELETE FROM deprecated WHERE deprecated.operatorbundle_name NOT IN (SELECT DISTINCT deprecated.operatorbundle_name FROM (deprecated INNER JOIN channel_entry ON deprecated.operatorbundle_name = channel_entry.operatorbundle_name))`)
+	if err != nil {
+		return err
+	}
+
 	return tx.Commit()
 }
 
@@ -1536,3 +1547,46 @@ func (s *sqlLoader) deprecated(tx *sql.Tx, name string) (bool, error) {
 	// Ignore any deprecated bundles
 	return err == nil, err
 }
+
+// DeprecationAwareLoader understands how bundle deprecations are handled in SQLite and decorates
+// the sqlLoader with proxy methods that handle deprecation related table housekeeping.
+type DeprecationAwareLoader struct {
+	*sqlLoader
+}
+
+// NewDeprecationAwareLoader returns a new DeprecationAwareLoader.
+func NewDeprecationAwareLoader(db *sql.DB, opts ...DbOption) (*DeprecationAwareLoader, error) {
+	loader, err := newSQLLoader(db, opts...)
+	if err != nil {
+		return nil, err
+	}
+
+	return &DeprecationAwareLoader{sqlLoader: loader}, nil
+}
+
+func (d *DeprecationAwareLoader) clearLastDeprecatedInPackage(pkg string) error {
+	tx, err := d.db.Begin()
+	if err != nil {
+		return err
+	}
+	defer func() {
+		tx.Rollback()
+	}()
+
+	// The last deprecated bundles for a package will still have "tombstone" records in channel_entry (among other tables).
+	// Use that info to relate the package to a set of rows in the deprecated table.
+	_, err = tx.Exec(`DELETE FROM deprecated WHERE deprecated.operatorbundle_name IN (SELECT DISTINCT deprecated.operatorbundle_name FROM (deprecated INNER JOIN channel_entry ON deprecated.operatorbundle_name = channel_entry.operatorbundle_name) WHERE channel_entry.package_name = ?)`, pkg)
+	if err != nil {
+		return err
+	}
+
+	return tx.Commit()
+}
+
+func (d *DeprecationAwareLoader) RemovePackage(pkg string) error {
+	if err := d.clearLastDeprecatedInPackage(pkg); err != nil {
+		return err
+	}
+
+	return d.sqlLoader.RemovePackage(pkg)
+}

pkg/sqlite/load_test.go

@@ -264,6 +264,218 @@ func TestRMBundle(t *testing.T) {
 	require.NoError(t, loader.rmBundle(tx, "non-existent"))
 }
 
+func TestDeprecationAwareLoader(t *testing.T) {
+	withBundleImage := func(image string, bundle *registry.Bundle) *registry.Bundle {
+		bundle.BundleImage = image
+		return bundle
+	}
+	type fields struct {
+		bundles         []*registry.Bundle
+		pkgs            []registry.PackageManifest
+		deprecatedPaths []string
+	}
+	type args struct {
+		pkg string
+	}
+	type expected struct {
+		err        error
+		deprecated map[string]struct{}
+	}
+	tests := []struct {
+		description string
+		fields      fields
+		args        args
+		expected    expected
+	}{
+		{
+			description: "NoDeprecation",
+			fields: fields{
+				bundles: []*registry.Bundle{
+					withBundleImage("quay.io/my/bundle-a", newBundle(t, "csv-a", "pkg-0", []string{"stable"}, newUnstructuredCSV(t, "csv-a", ""))),
+				},
+				pkgs: []registry.PackageManifest{
+					{
+						PackageName: "pkg-0",
+						Channels: []registry.PackageChannel{
+							{
+								Name:           "stable",
+								CurrentCSVName: "csv-a",
+							},
+						},
+						DefaultChannelName: "stable",
+					},
+				},
+				deprecatedPaths: []string{},
+			},
+			args: args{
+				pkg: "pkg-0",
+			},
+			expected: expected{
+				err:        nil,
+				deprecated: map[string]struct{}{},
+			},
+		},
+		{
+			description: "RemovePackage/DropsDeprecated",
+			fields: fields{
+				bundles: []*registry.Bundle{
+					withBundleImage("quay.io/my/bundle-a", newBundle(t, "csv-a", "pkg-0", []string{"stable"}, newUnstructuredCSV(t, "csv-a", ""))),
+					withBundleImage("quay.io/my/bundle-aa", newBundle(t, "csv-aa", "pkg-0", []string{"stable"}, newUnstructuredCSV(t, "csv-aa", "csv-a"))),
+				},
+				pkgs: []registry.PackageManifest{
+					{
+						PackageName: "pkg-0",
+						Channels: []registry.PackageChannel{
+							{
+								Name:           "stable",
+								CurrentCSVName: "csv-aa",
+							},
+						},
+						DefaultChannelName: "stable",
+					},
+				},
+				deprecatedPaths: []string{
+					"quay.io/my/bundle-a",
+				},
+			},
+			args: args{
+				pkg: "pkg-0",
+			},
+			expected: expected{
+				err:        nil,
+				deprecated: map[string]struct{}{},
+			},
+		},
+		{
+			description: "RemovePackage/IgnoresOtherPackages",
+			fields: fields{
+				bundles: []*registry.Bundle{
+					withBundleImage("quay.io/my/bundle-a", newBundle(t, "csv-a", "pkg-0", []string{"stable"}, newUnstructuredCSV(t, "csv-a", ""))),
+					withBundleImage("quay.io/my/bundle-aa", newBundle(t, "csv-aa", "pkg-0", []string{"stable"}, newUnstructuredCSV(t, "csv-aa", "csv-a"))),
+					withBundleImage("quay.io/my/bundle-b", newBundle(t, "csv-b", "pkg-1", []string{"stable"}, newUnstructuredCSV(t, "csv-b", ""))),
+					withBundleImage("quay.io/my/bundle-bb", newBundle(t, "csv-bb", "pkg-1", []string{"stable"}, newUnstructuredCSV(t, "csv-bb", "csv-b"))),
+				},
+				pkgs: []registry.PackageManifest{
+					{
+						PackageName: "pkg-0",
+						Channels: []registry.PackageChannel{
+							{
+								Name:           "stable",
+								CurrentCSVName: "csv-aa",
+							},
+						},
+						DefaultChannelName: "stable",
+					},
+					{
+						PackageName: "pkg-1",
+						Channels: []registry.PackageChannel{
+							{
+								Name:           "stable",
+								CurrentCSVName: "csv-bb",
+							},
+						},
+						DefaultChannelName: "stable",
+					},
+				},
+				deprecatedPaths: []string{
+					"quay.io/my/bundle-a",
+					"quay.io/my/bundle-b",
+				},
+			},
+			args: args{
+				pkg: "pkg-0", // Should result in a alone being dropped from the deprecated table
+			},
+			expected: expected{
+				err: nil,
+				deprecated: map[string]struct{}{
+					"csv-b": struct{}{},
+				},
+			},
+		},
+		{
+			description: "DeprecateTruncate/DropsTruncated",
+			fields: fields{
+				bundles: []*registry.Bundle{
+					withBundleImage("quay.io/my/bundle-a", newBundle(t, "csv-a", "pkg-0", []string{"stable"}, newUnstructuredCSV(t, "csv-a", ""))),
+					withBundleImage("quay.io/my/bundle-aa", newBundle(t, "csv-aa", "pkg-0", []string{"stable"}, newUnstructuredCSV(t, "csv-aa", "csv-a"))),
+					withBundleImage("quay.io/my/bundle-aaa", newBundle(t, "csv-aaa", "pkg-0", []string{"stable"}, newUnstructuredCSV(t, "csv-aaa", "csv-aa"))),
+				},
+				pkgs: []registry.PackageManifest{
+					{
+						PackageName: "pkg-0",
+						Channels: []registry.PackageChannel{
+							{
+								Name:           "stable",
+								CurrentCSVName: "csv-aaa",
+							},
+						},
+						DefaultChannelName: "stable",
+					},
+				},
+				deprecatedPaths: []string{
+					"quay.io/my/bundle-a",
+					"quay.io/my/bundle-aa", // Should truncate a, dropping it from the deprecated table
+				},
+			},
+			expected: expected{
+				err: nil,
+				deprecated: map[string]struct{}{
+					"csv-aa": struct{}{}, // csv-b remains in the deprecated table since it has been truncated and hasn't been removed
+				},
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.description, func(t *testing.T) {
+			db, cleanup := CreateTestDb(t)
+			defer cleanup()
+			store, err := NewDeprecationAwareLoader(db)
+			require.NoError(t, err)
+			err = store.Migrate(context.TODO())
+			require.NoError(t, err)
+
+			for _, bundle := range tt.fields.bundles {
+				require.NoError(t, store.AddOperatorBundle(bundle))
+			}
+
+			for _, pkg := range tt.fields.pkgs {
+				require.NoError(t, store.AddPackageChannels(pkg))
+			}
+
+			for _, deprecatedPath := range tt.fields.deprecatedPaths {
+				require.NoError(t, store.DeprecateBundle(deprecatedPath))
+			}
+
+			if tt.args.pkg != "" {
+				err = store.RemovePackage(tt.args.pkg)
+				if tt.expected.err != nil {
+					require.Error(t, err)
+				} else {
+					require.NoError(t, err)
+				}
+			}
+
+			tx, err := db.Begin()
+			require.NoError(t, err)
+
+			rows, err := tx.Query(`SELECT operatorbundle_name FROM deprecated`)
+			require.NoError(t, err)
+			require.NotNil(t, rows)
+
+			var bundleName string
+			for rows.Next() {
+				require.NoError(t, rows.Scan(&bundleName))
+				_, ok := tt.expected.deprecated[bundleName]
+				require.True(t, ok, "bundle shouldn't be in the deprecated table: %s", bundleName)
+				delete(tt.expected.deprecated, bundleName)
+			}
+
+			require.Len(t, tt.expected.deprecated, 0, "not all expected bundles exist in deprecated table: %v", tt.expected.deprecated)
+		})
+	}
+}
+
 func TestGetTailFromBundle(t *testing.T) {
 	type fields struct {
 		bundles []*registry.Bundle

pkg/sqlite/migrations/013_rm_truncated_deprecations.go

@@ -0,0 +1,31 @@
+package migrations
+
+import (
+	"context"
+	"database/sql"
+)
+
+const RmTruncatedDeprecationsMigrationKey = 13
+
+// Register this migration
+func init() {
+	registerMigration(RmTruncatedDeprecationsMigrationKey, rmTruncatedDeprecationsMigration)
+}
+
+var rmTruncatedDeprecationsMigration = &Migration{
+	Id: RmTruncatedDeprecationsMigrationKey,
+	Up: func(ctx context.Context, tx *sql.Tx) error {
+
+		// Delete deprecation history for all bundles that no longer exist in the channel_entries table
+		// These bundles have been truncated by more recent deprecations and would only confuse future operations on an index;
+		// e.g. adding a previously truncated bundle to a package removed via `opm index|registry rm` would lead to that bundle
+		// being deprecated
+		_, err := tx.ExecContext(ctx, `DELETE FROM deprecated WHERE deprecated.operatorbundle_name NOT IN (SELECT DISTINCT deprecated.operatorbundle_name FROM (deprecated INNER JOIN channel_entry ON deprecated.operatorbundle_name = channel_entry.operatorbundle_name))`)
+
+		return err
+	},
+	Down: func(ctx context.Context, tx *sql.Tx) error {
+		// No-op
+		return nil
+	},
+}

pkg/sqlite/migrations/013_rm_truncated_deprecations_test.go

@@ -0,0 +1,49 @@
+package migrations_test
+
+import (
+	"context"
+	"database/sql"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/operator-framework/operator-registry/pkg/sqlite/migrations"
+)
+
+func TestRmTruncatedDeprecations(t *testing.T) {
+	db, migrator, cleanup := CreateTestDbAt(t, migrations.RmTruncatedDeprecationsMigrationKey-1)
+	defer cleanup()
+
+	// Insert fixtures to satisfy foreign key constraints
+	insertBundle := "INSERT INTO operatorbundle(name, version, bundlepath, csv) VALUES (?, ?, ?, ?)"
+	insertChannel := "INSERT INTO channel(name, package_name, head_operatorbundle_name) VALUES (?, ?, ?)"
+	insertChannelEntry := "INSERT INTO channel_entry(entry_id, channel_name, package_name, operatorbundle_name) VALUES (?, ?, ?, ?)"
+	insertDeprecated := "INSERT INTO deprecated(operatorbundle_name) VALUES (?)"
+
+	// Add a deprecated bundle
+	_, err := db.Exec(insertBundle, "operator.v1.0.0", "1.0.0", "quay.io/operator:v1.0.0", "operator.v1.0.0's csv")
+	require.NoError(t, err)
+	_, err = db.Exec(insertChannel, "stable", "apple", "operator.v1.0.0")
+	require.NoError(t, err)
+	_, err = db.Exec(insertChannelEntry, 0, "stable", "apple", "operator.v1.0.0")
+	require.NoError(t, err)
+	_, err = db.Exec(insertDeprecated, "operator.v1.0.0")
+	require.NoError(t, err)
+
+	// Add a truncated bundle; i.e. doesn't exist in the channel_entry table
+	_, err = db.Exec(insertDeprecated, "operator.v1.0.0-pre")
+
+	// This migration should delete all bundles that are not referenced by the channel_entry table
+	require.NoError(t, migrator.Up(context.Background(), migrations.Only(migrations.RmTruncatedDeprecationsMigrationKey)))
+
+	deprecated, err := db.Query("SELECT * FROM deprecated")
+	require.NoError(t, err)
+	defer deprecated.Close()
+
+	require.True(t, deprecated.Next(), "failed to detect deprecated bundle")
+	var name sql.NullString
+	require.NoError(t, deprecated.Scan(&name))
+	require.True(t, name.Valid)
+	require.Equal(t, "operator.v1.0.0", name.String)
+	require.False(t, deprecated.Next(), "incorrect number of deprecated bundles")
+}