From 6a8da1a05fffe47271462e5f0f4f2a3d1f1653be Mon Sep 17 00:00:00 2001 From: Lance <lgallett@redhat.com> Date: Wed, 8 Jul 2020 16:29:08 -0400 Subject: [PATCH 1/4] Updating: adding sanity checks before insert into index --- pkg/registry/populator.go | 42 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/pkg/registry/populator.go b/pkg/registry/populator.go index 7c6dbd95c..a86ee6ea1 100644 --- a/pkg/registry/populator.go +++ b/pkg/registry/populator.go @@ -63,7 +63,49 @@ func (i *DirectoryPopulator) Populate(mode Mode) error { return nil } +func (i *DirectoryPopulator) globalSanityCheck(imagesToAdd []*ImageInput) error { + var errs []error + images := make(map[string]bool) + for _, image := range imagesToAdd { + images[image.bundle.BundleImage] = true + } + for _, image := range imagesToAdd { + bundlePaths, err := i.querier.GetBundlePathsForPackage(context.TODO(), image.bundle.Package) + if err != nil { + errs = append(errs, err) + return utilerrors.NewAggregate(errs) + } + for _, bundlePath := range bundlePaths { + if _, ok := images[bundlePath]; ok { + // raise error that bundlePath is already in the db + errs = append(errs, fmt.Errorf(" TODO ")) + continue + } + } + for _, channel := range image.bundle.Channels { + bundle, err := i.querier.GetBundle(context.TODO(), image.bundle.Package, channel, image.bundle.csv.GetName()) + if err != nil { + errs = append(errs, err) + return utilerrors.NewAggregate(errs) + } + if bundle != nil { + // raise error that this package + channel + csv combo is already in the db + errs = append(errs, fmt.Errorf(" TODO ")) + continue + } + } + } + + return utilerrors.NewAggregate(errs) +} + func (i *DirectoryPopulator) loadManifests(imagesToAdd []*ImageInput, mode Mode) error { + // global sanity checks before insertion + err := i.globalSanityCheck(imagesToAdd) + if err != nil { + return err + } + switch mode { case ReplacesMode: // TODO: This is relatively inefficient. Ideally, we should be able to use a replaces From 98ff6831fefd910f0ddefc4fe5c4831114479648 Mon Sep 17 00:00:00 2001 From: Shawn Hurley <smhurley00@gmail.com> Date: Thu, 9 Jul 2020 13:58:01 -0400 Subject: [PATCH 2/4] Adding sanity check and error message for inserting the same bundle * Added case where bundle image is different but provides same info * Added case where bundle was exactly the same --- pkg/registry/populator.go | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/pkg/registry/populator.go b/pkg/registry/populator.go index a86ee6ea1..fee585d52 100644 --- a/pkg/registry/populator.go +++ b/pkg/registry/populator.go @@ -69,17 +69,17 @@ func (i *DirectoryPopulator) globalSanityCheck(imagesToAdd []*ImageInput) error for _, image := range imagesToAdd { images[image.bundle.BundleImage] = true } + for _, image := range imagesToAdd { bundlePaths, err := i.querier.GetBundlePathsForPackage(context.TODO(), image.bundle.Package) if err != nil { errs = append(errs, err) return utilerrors.NewAggregate(errs) } + isExactImage := false for _, bundlePath := range bundlePaths { if _, ok := images[bundlePath]; ok { - // raise error that bundlePath is already in the db - errs = append(errs, fmt.Errorf(" TODO ")) - continue + isExactImage = true } } for _, channel := range image.bundle.Channels { @@ -88,9 +88,14 @@ func (i *DirectoryPopulator) globalSanityCheck(imagesToAdd []*ImageInput) error errs = append(errs, err) return utilerrors.NewAggregate(errs) } - if bundle != nil { + if bundle != nil && !isExactImage { + // raise error that this package + channel + csv combo is already in the db + errs = append(errs, fmt.Errorf("bundle already added that provides package: %v and CSV: %v", image.bundle.Package, image.bundle.csv.GetName())) + continue + } + if bundle != nil && isExactImage { // raise error that this package + channel + csv combo is already in the db - errs = append(errs, fmt.Errorf(" TODO ")) + errs = append(errs, fmt.Errorf("bundle already added %v", image.bundle.BundleImage)) continue } } From a21d9e95e00961abe4110c6a06bd254849a46e4b Mon Sep 17 00:00:00 2001 From: Shawn Hurley <smhurley00@gmail.com> Date: Mon, 13 Jul 2020 16:28:19 -0400 Subject: [PATCH 3/4] fixing up to return exit codes * adding testcases * updated based on feedback --- cmd/opm/main.go | 12 +++++ pkg/lib/indexer/indexer.go | 2 +- pkg/lib/registry/registry.go | 14 ++--- pkg/registry/populator.go | 15 +++--- pkg/registry/populator_test.go | 93 +++++++++++++++++++++++++++++++++- pkg/registry/types.go | 18 +++++++ 6 files changed, 138 insertions(+), 16 deletions(-) diff --git a/cmd/opm/main.go b/cmd/opm/main.go index 963467d20..546612864 100644 --- a/cmd/opm/main.go +++ b/cmd/opm/main.go @@ -10,6 +10,8 @@ import ( "github.com/operator-framework/operator-registry/cmd/opm/index" "github.com/operator-framework/operator-registry/cmd/opm/registry" "github.com/operator-framework/operator-registry/cmd/opm/version" + registrylib "github.com/operator-framework/operator-registry/pkg/registry" + utilerrors "k8s.io/apimachinery/pkg/util/errors" ) func main() { @@ -35,6 +37,16 @@ func main() { } if err := rootCmd.Execute(); err != nil { + if agg, ok := err.(utilerrors.Aggregate); ok { + for _, e := range agg.Errors() { + if _, ok := e.(registrylib.BundleAlreadyAddedError); ok { + os.Exit(2) + } + if _, ok := e.(registrylib.BundleAlreadyInDatabaseError); ok { + os.Exit(3) + } + } + } os.Exit(1) } } diff --git a/pkg/lib/indexer/indexer.go b/pkg/lib/indexer/indexer.go index fb2a5ebc4..e30a48a1d 100644 --- a/pkg/lib/indexer/indexer.go +++ b/pkg/lib/indexer/indexer.go @@ -85,6 +85,7 @@ func (i ImageIndexer) AddToIndex(request AddToIndexRequest) error { // Add the bundles to the registry err = i.RegistryAdder.AddToRegistry(addToRegistryReq) if err != nil { + i.Logger.WithError(err).Debugf("unable to add bundle to registry") return err } @@ -222,7 +223,6 @@ func (i ImageIndexer) PruneFromIndex(request PruneFromIndexRequest) error { return nil } - // extractDatabase sets a temp directory for unpacking an image func (i ImageIndexer) extractDatabase(buildDir, fromIndex string) (string, error) { tmpDir, err := ioutil.TempDir("./", tmpDirPrefix) diff --git a/pkg/lib/registry/registry.go b/pkg/lib/registry/registry.go index e0899722e..d50f191b8 100644 --- a/pkg/lib/registry/registry.go +++ b/pkg/lib/registry/registry.go @@ -5,11 +5,12 @@ import ( "crypto/x509" "database/sql" "fmt" - "github.com/operator-framework/operator-registry/pkg/containertools" - "github.com/operator-framework/operator-registry/pkg/image/execregistry" "io/ioutil" "os" + "github.com/operator-framework/operator-registry/pkg/containertools" + "github.com/operator-framework/operator-registry/pkg/image/execregistry" + "github.com/sirupsen/logrus" utilerrors "k8s.io/apimachinery/pkg/util/errors" @@ -34,8 +35,6 @@ type AddToRegistryRequest struct { } func (r RegistryUpdater) AddToRegistry(request AddToRegistryRequest) error { - var errs []error - db, err := sql.Open("sqlite3", request.InputDatabase) if err != nil { return err @@ -96,16 +95,17 @@ func (r RegistryUpdater) AddToRegistry(request AddToRegistryRequest) error { } if err := populate(context.TODO(), dbLoader, graphLoader, dbQuerier, reg, simpleRefs, request.Mode); err != nil { - err = fmt.Errorf("error loading bundle from image: %s", err) + r.Logger.Debugf("unable to populate database: %s", err) + if !request.Permissive { r.Logger.WithError(err).Error("permissive mode disabled") - errs = append(errs, err) + return err } else { r.Logger.WithError(err).Warn("permissive mode enabled") } } - return utilerrors.NewAggregate(errs) // nil if no errors + return nil } func populate(ctx context.Context, loader registry.Load, graphLoader registry.GraphLoader, querier registry.Query, reg image.Registry, refs []image.Reference, mode registry.Mode) error { diff --git a/pkg/registry/populator.go b/pkg/registry/populator.go index fee585d52..f1ab52136 100644 --- a/pkg/registry/populator.go +++ b/pkg/registry/populator.go @@ -73,8 +73,9 @@ func (i *DirectoryPopulator) globalSanityCheck(imagesToAdd []*ImageInput) error for _, image := range imagesToAdd { bundlePaths, err := i.querier.GetBundlePathsForPackage(context.TODO(), image.bundle.Package) if err != nil { - errs = append(errs, err) - return utilerrors.NewAggregate(errs) + // Assume that this means that the bundle is empty + // Or that this is the first time the package is loaded. + return nil } isExactImage := false for _, bundlePath := range bundlePaths { @@ -85,17 +86,17 @@ func (i *DirectoryPopulator) globalSanityCheck(imagesToAdd []*ImageInput) error for _, channel := range image.bundle.Channels { bundle, err := i.querier.GetBundle(context.TODO(), image.bundle.Package, channel, image.bundle.csv.GetName()) if err != nil { - errs = append(errs, err) - return utilerrors.NewAggregate(errs) + // Assume that if we can not find a bundle for the package, channel and or CSV Name that this is safe to add + continue } if bundle != nil && !isExactImage { // raise error that this package + channel + csv combo is already in the db - errs = append(errs, fmt.Errorf("bundle already added that provides package: %v and CSV: %v", image.bundle.Package, image.bundle.csv.GetName())) + errs = append(errs, BundleAlreadyInDatabaseError{ErrorString: "Bundle already added that provides package and csv"}) continue } if bundle != nil && isExactImage { - // raise error that this package + channel + csv combo is already in the db - errs = append(errs, fmt.Errorf("bundle already added %v", image.bundle.BundleImage)) + // raise error that this bundle and channel is already in the db + errs = append(errs, BundleAlreadyAddedError{ErrorString: fmt.Sprintf("Bundle %s already exists", image.bundle.BundleImage)}) continue } } diff --git a/pkg/registry/populator_test.go b/pkg/registry/populator_test.go index 85e515018..3ff7c9ca3 100644 --- a/pkg/registry/populator_test.go +++ b/pkg/registry/populator_test.go @@ -6,6 +6,7 @@ import ( "fmt" "math/rand" "os" + "reflect" "testing" "time" @@ -16,6 +17,8 @@ import ( "github.com/operator-framework/operator-registry/pkg/image" "github.com/operator-framework/operator-registry/pkg/registry" "github.com/operator-framework/operator-registry/pkg/sqlite" + + utilerrors "k8s.io/apimachinery/pkg/util/errors" ) func init() { @@ -246,6 +249,7 @@ func TestImageLoading(t *testing.T) { addImage img wantPackages []*registry.Package wantErr bool + err error }{ { name: "OneChannel/AddBundleToTwoChannels", @@ -386,6 +390,76 @@ func TestImageLoading(t *testing.T) { }, wantErr: false, }, + { + name: "AddBundleAlreadyExists", + initImages: []img{ + { + // this is in the "preview" channel + ref: image.SimpleReference("quay.io/prometheus/operator:0.14.0"), + dir: "../../bundles/prometheus.0.14.0", + }, + }, + addImage: img{ + //Adding same bundle different bundle + ref: image.SimpleReference("quay.io/prometheus/operator-test:testing"), + dir: "../../bundles/prometheus.0.14.0", + }, + wantPackages: []*registry.Package{ + { + Name: "prometheus", + DefaultChannel: "stable", + Channels: map[string]registry.Channel{ + "preview": { + Head: registry.BundleKey{ + BundlePath: "quay.io/prometheus/operator:0.14.0", + Version: "0.14.0", + CsvName: "prometheusoperator.0.14.0", + }, + Nodes: map[registry.BundleKey]map[registry.BundleKey]struct{}{ + {BundlePath: "quay.io/prometheus/operator:0.14.0", Version: "0.14.0", CsvName: "prometheusoperator.0.14.0"}: {}, + }, + }, + }, + }, + }, + wantErr: true, + err: registry.BundleAlreadyInDatabaseError{}, + }, + { + name: "AddExactBundleAlreadyExists", + initImages: []img{ + { + // this is in the "preview" channel + ref: image.SimpleReference("quay.io/prometheus/operator:0.14.0"), + dir: "../../bundles/prometheus.0.14.0", + }, + }, + addImage: img{ + // Add the same package + ref: image.SimpleReference("quay.io/prometheus/operator:0.14.0"), + dir: "../../bundles/prometheus.0.14.0", + }, + wantPackages: []*registry.Package{ + { + Name: "prometheus", + DefaultChannel: "stable", + Channels: map[string]registry.Channel{ + "preview": { + Head: registry.BundleKey{ + BundlePath: "quay.io/prometheus/operator:0.14.0", + Version: "0.14.0", + CsvName: "prometheusoperator.0.14.0", + }, + Nodes: map[registry.BundleKey]map[registry.BundleKey]struct{}{ + {BundlePath: "quay.io/prometheus/operator:0.14.0", Version: "0.14.0", CsvName: "prometheusoperator.0.14.0"}: {}, + }, + }, + }, + }, + }, + wantErr: true, + err: registry.BundleAlreadyAddedError{}, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -411,7 +485,12 @@ func TestImageLoading(t *testing.T) { graphLoader, query, map[image.Reference]string{tt.addImage.ref: tt.addImage.dir}) - require.NoError(t, add.Populate(registry.ReplacesMode)) + err = add.Populate(registry.ReplacesMode) + if tt.wantErr { + require.True(t, checkAggErr(err, tt.err)) + return + } + require.NoError(t, err) for _, p := range tt.wantPackages { graphLoader, err := sqlite.NewSQLGraphLoaderFromDB(db) @@ -426,6 +505,18 @@ func TestImageLoading(t *testing.T) { } } +func checkAggErr(aggErr, wantErr error) bool { + if a, ok := aggErr.(utilerrors.Aggregate); ok { + for _, e := range a.Errors() { + if reflect.TypeOf(e).String() == reflect.TypeOf(wantErr).String() { + return true + } + } + return false + } + return reflect.TypeOf(aggErr).String() == reflect.TypeOf(wantErr).String() +} + func TestQuerierForDependencies(t *testing.T) { logrus.SetLevel(logrus.DebugLevel) db, cleanup := CreateTestDb(t) diff --git a/pkg/registry/types.go b/pkg/registry/types.go index e6f5afe29..9eb817215 100644 --- a/pkg/registry/types.go +++ b/pkg/registry/types.go @@ -14,6 +14,24 @@ var ( ErrPackageNotInDatabase = errors.New("Package not in database") ) +// BundleAlreadyAddedError is an error that describes a bundle is already added +type BundleAlreadyAddedError struct { + ErrorString string +} + +func (e BundleAlreadyAddedError) Error() string { + return e.ErrorString +} + +// BundleAlreadyInDatabseError is an error that describes that a bundle that is already in the databse that provides this CSV +type BundleAlreadyInDatabaseError struct { + ErrorString string +} + +func (e BundleAlreadyInDatabaseError) Error() string { + return e.ErrorString +} + const ( GVKType = "olm.gvk" PackageType = "olm.package" From 129c3f6ffc0f8173111a3da39b2b72cc75b1e8b3 Mon Sep 17 00:00:00 2001 From: Shawn Hurley <smhurley00@gmail.com> Date: Thu, 16 Jul 2020 13:37:17 -0400 Subject: [PATCH 4/4] Updating based on feedback * Cleaning up imports * Updating names to be more clear * code optimizations. --- cmd/opm/main.go | 20 +++++++++++--------- pkg/lib/registry/registry.go | 5 ++--- pkg/registry/populator.go | 19 +++++++------------ pkg/registry/populator_test.go | 4 ++-- pkg/registry/types.go | 12 ++++++------ 5 files changed, 28 insertions(+), 32 deletions(-) diff --git a/cmd/opm/main.go b/cmd/opm/main.go index 546612864..550adad8c 100644 --- a/cmd/opm/main.go +++ b/cmd/opm/main.go @@ -5,13 +5,13 @@ import ( "github.com/sirupsen/logrus" "github.com/spf13/cobra" + utilerrors "k8s.io/apimachinery/pkg/util/errors" "github.com/operator-framework/operator-registry/cmd/opm/alpha" "github.com/operator-framework/operator-registry/cmd/opm/index" "github.com/operator-framework/operator-registry/cmd/opm/registry" "github.com/operator-framework/operator-registry/cmd/opm/version" registrylib "github.com/operator-framework/operator-registry/pkg/registry" - utilerrors "k8s.io/apimachinery/pkg/util/errors" ) func main() { @@ -37,14 +37,16 @@ func main() { } if err := rootCmd.Execute(); err != nil { - if agg, ok := err.(utilerrors.Aggregate); ok { - for _, e := range agg.Errors() { - if _, ok := e.(registrylib.BundleAlreadyAddedError); ok { - os.Exit(2) - } - if _, ok := e.(registrylib.BundleAlreadyInDatabaseError); ok { - os.Exit(3) - } + agg, ok := err.(utilerrors.Aggregate) + if !ok { + os.Exit(1) + } + for _, e := range agg.Errors() { + if _, ok := e.(registrylib.BundleImageAlreadyAddedErr); ok { + os.Exit(2) + } + if _, ok := e.(registrylib.PackageVersionAlreadyAddedErr); ok { + os.Exit(3) } } os.Exit(1) diff --git a/pkg/lib/registry/registry.go b/pkg/lib/registry/registry.go index d50f191b8..618e7040a 100644 --- a/pkg/lib/registry/registry.go +++ b/pkg/lib/registry/registry.go @@ -8,14 +8,13 @@ import ( "io/ioutil" "os" - "github.com/operator-framework/operator-registry/pkg/containertools" - "github.com/operator-framework/operator-registry/pkg/image/execregistry" - "github.com/sirupsen/logrus" utilerrors "k8s.io/apimachinery/pkg/util/errors" + "github.com/operator-framework/operator-registry/pkg/containertools" "github.com/operator-framework/operator-registry/pkg/image" "github.com/operator-framework/operator-registry/pkg/image/containerdregistry" + "github.com/operator-framework/operator-registry/pkg/image/execregistry" "github.com/operator-framework/operator-registry/pkg/registry" "github.com/operator-framework/operator-registry/pkg/sqlite" ) diff --git a/pkg/registry/populator.go b/pkg/registry/populator.go index f1ab52136..8e3e2eb5a 100644 --- a/pkg/registry/populator.go +++ b/pkg/registry/populator.go @@ -65,9 +65,9 @@ func (i *DirectoryPopulator) Populate(mode Mode) error { func (i *DirectoryPopulator) globalSanityCheck(imagesToAdd []*ImageInput) error { var errs []error - images := make(map[string]bool) + images := make(map[string]struct{}) for _, image := range imagesToAdd { - images[image.bundle.BundleImage] = true + images[image.bundle.BundleImage] = struct{}{} } for _, image := range imagesToAdd { @@ -77,10 +77,10 @@ func (i *DirectoryPopulator) globalSanityCheck(imagesToAdd []*ImageInput) error // Or that this is the first time the package is loaded. return nil } - isExactImage := false for _, bundlePath := range bundlePaths { if _, ok := images[bundlePath]; ok { - isExactImage = true + errs = append(errs, BundleImageAlreadyAddedErr{ErrorString: fmt.Sprintf("Bundle %s already exists", image.bundle.BundleImage)}) + continue } } for _, channel := range image.bundle.Channels { @@ -89,15 +89,10 @@ func (i *DirectoryPopulator) globalSanityCheck(imagesToAdd []*ImageInput) error // Assume that if we can not find a bundle for the package, channel and or CSV Name that this is safe to add continue } - if bundle != nil && !isExactImage { + if bundle != nil { // raise error that this package + channel + csv combo is already in the db - errs = append(errs, BundleAlreadyInDatabaseError{ErrorString: "Bundle already added that provides package and csv"}) - continue - } - if bundle != nil && isExactImage { - // raise error that this bundle and channel is already in the db - errs = append(errs, BundleAlreadyAddedError{ErrorString: fmt.Sprintf("Bundle %s already exists", image.bundle.BundleImage)}) - continue + errs = append(errs, PackageVersionAlreadyAddedErr{ErrorString: "Bundle already added that provides package and csv"}) + break } } } diff --git a/pkg/registry/populator_test.go b/pkg/registry/populator_test.go index 3ff7c9ca3..9552e81af 100644 --- a/pkg/registry/populator_test.go +++ b/pkg/registry/populator_test.go @@ -423,7 +423,7 @@ func TestImageLoading(t *testing.T) { }, }, wantErr: true, - err: registry.BundleAlreadyInDatabaseError{}, + err: registry.PackageVersionAlreadyAddedErr{}, }, { name: "AddExactBundleAlreadyExists", @@ -458,7 +458,7 @@ func TestImageLoading(t *testing.T) { }, }, wantErr: true, - err: registry.BundleAlreadyAddedError{}, + err: registry.BundleImageAlreadyAddedErr{}, }, } for _, tt := range tests { diff --git a/pkg/registry/types.go b/pkg/registry/types.go index 9eb817215..0cf5e83eb 100644 --- a/pkg/registry/types.go +++ b/pkg/registry/types.go @@ -14,21 +14,21 @@ var ( ErrPackageNotInDatabase = errors.New("Package not in database") ) -// BundleAlreadyAddedError is an error that describes a bundle is already added -type BundleAlreadyAddedError struct { +// BundleImageAlreadyAddedErr is an error that describes a bundle is already added +type BundleImageAlreadyAddedErr struct { ErrorString string } -func (e BundleAlreadyAddedError) Error() string { +func (e BundleImageAlreadyAddedErr) Error() string { return e.ErrorString } -// BundleAlreadyInDatabseError is an error that describes that a bundle that is already in the databse that provides this CSV -type BundleAlreadyInDatabaseError struct { +// PackageVersionAlreadyAddedErr is an error that describes that a bundle that is already in the databse that provides this package and version +type PackageVersionAlreadyAddedErr struct { ErrorString string } -func (e BundleAlreadyInDatabaseError) Error() string { +func (e PackageVersionAlreadyAddedErr) Error() string { return e.ErrorString }