Skip to content

Commit 3403f43

Browse files
openshift-merge-robotopenshift-merge-robot
authored
Merge pull request #384 from gallettilance/check-before-insert
Check before insert
2 parents 6e502a2 + 129c3f6 commit 3403f43

File tree

6 files changed

+174
-9
lines changed

6 files changed

+174
-9
lines changed

‎cmd/opm/main.go

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,13 @@ import (
55

66
"github.com/sirupsen/logrus"
77
"github.com/spf13/cobra"
8+
utilerrors "k8s.io/apimachinery/pkg/util/errors"
89

910
"github.com/operator-framework/operator-registry/cmd/opm/alpha"
1011
"github.com/operator-framework/operator-registry/cmd/opm/index"
1112
"github.com/operator-framework/operator-registry/cmd/opm/registry"
1213
"github.com/operator-framework/operator-registry/cmd/opm/version"
14+
registrylib "github.com/operator-framework/operator-registry/pkg/registry"
1315
)
1416

1517
func main() {
@@ -35,6 +37,18 @@ func main() {
3537
}
3638

3739
if err := rootCmd.Execute(); err != nil {
40+
agg, ok := err.(utilerrors.Aggregate)
41+
if !ok {
42+
os.Exit(1)
43+
}
44+
for _, e := range agg.Errors() {
45+
if _, ok := e.(registrylib.BundleImageAlreadyAddedErr); ok {
46+
os.Exit(2)
47+
}
48+
if _, ok := e.(registrylib.PackageVersionAlreadyAddedErr); ok {
49+
os.Exit(3)
50+
}
51+
}
3852
os.Exit(1)
3953
}
4054
}

‎pkg/lib/indexer/indexer.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,7 @@ func (i ImageIndexer) AddToIndex(request AddToIndexRequest) error {
8585
// Add the bundles to the registry
8686
err = i.RegistryAdder.AddToRegistry(addToRegistryReq)
8787
if err != nil {
88+
i.Logger.WithError(err).Debugf("unable to add bundle to registry")
8889
return err
8990
}
9091

@@ -222,7 +223,6 @@ func (i ImageIndexer) PruneFromIndex(request PruneFromIndexRequest) error {
222223
return nil
223224
}
224225

225-
226226
// extractDatabase sets a temp directory for unpacking an image
227227
func (i ImageIndexer) extractDatabase(buildDir, fromIndex string) (string, error) {
228228
tmpDir, err := ioutil.TempDir("./", tmpDirPrefix)

‎pkg/lib/registry/registry.go

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,16 +5,16 @@ import (
55
"crypto/x509"
66
"database/sql"
77
"fmt"
8-
"github.com/operator-framework/operator-registry/pkg/containertools"
9-
"github.com/operator-framework/operator-registry/pkg/image/execregistry"
108
"io/ioutil"
119
"os"
1210

1311
"github.com/sirupsen/logrus"
1412
utilerrors "k8s.io/apimachinery/pkg/util/errors"
1513

14+
"github.com/operator-framework/operator-registry/pkg/containertools"
1615
"github.com/operator-framework/operator-registry/pkg/image"
1716
"github.com/operator-framework/operator-registry/pkg/image/containerdregistry"
17+
"github.com/operator-framework/operator-registry/pkg/image/execregistry"
1818
"github.com/operator-framework/operator-registry/pkg/registry"
1919
"github.com/operator-framework/operator-registry/pkg/sqlite"
2020
)
@@ -34,8 +34,6 @@ type AddToRegistryRequest struct {
3434
}
3535

3636
func (r RegistryUpdater) AddToRegistry(request AddToRegistryRequest) error {
37-
var errs []error
38-
3937
db, err := sql.Open("sqlite3", request.InputDatabase)
4038
if err != nil {
4139
return err
@@ -96,16 +94,17 @@ func (r RegistryUpdater) AddToRegistry(request AddToRegistryRequest) error {
9694
}
9795

9896
if err := populate(context.TODO(), dbLoader, graphLoader, dbQuerier, reg, simpleRefs, request.Mode); err != nil {
99-
err = fmt.Errorf("error loading bundle from image: %s", err)
97+
r.Logger.Debugf("unable to populate database: %s", err)
98+
10099
if !request.Permissive {
101100
r.Logger.WithError(err).Error("permissive mode disabled")
102-
errs = append(errs, err)
101+
return err
103102
} else {
104103
r.Logger.WithError(err).Warn("permissive mode enabled")
105104
}
106105
}
107106

108-
return utilerrors.NewAggregate(errs) // nil if no errors
107+
return nil
109108
}
110109

111110
func populate(ctx context.Context, loader registry.Load, graphLoader registry.GraphLoader, querier registry.Query, reg image.Registry, refs []image.Reference, mode registry.Mode) error {

‎pkg/registry/populator.go

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,50 @@ func (i *DirectoryPopulator) Populate(mode Mode) error {
6363
return nil
6464
}
6565

66+
func (i *DirectoryPopulator) globalSanityCheck(imagesToAdd []*ImageInput) error {
67+
var errs []error
68+
images := make(map[string]struct{})
69+
for _, image := range imagesToAdd {
70+
images[image.bundle.BundleImage] = struct{}{}
71+
}
72+
73+
for _, image := range imagesToAdd {
74+
bundlePaths, err := i.querier.GetBundlePathsForPackage(context.TODO(), image.bundle.Package)
75+
if err != nil {
76+
// Assume that this means that the bundle is empty
77+
// Or that this is the first time the package is loaded.
78+
return nil
79+
}
80+
for _, bundlePath := range bundlePaths {
81+
if _, ok := images[bundlePath]; ok {
82+
errs = append(errs, BundleImageAlreadyAddedErr{ErrorString: fmt.Sprintf("Bundle %s already exists", image.bundle.BundleImage)})
83+
continue
84+
}
85+
}
86+
for _, channel := range image.bundle.Channels {
87+
bundle, err := i.querier.GetBundle(context.TODO(), image.bundle.Package, channel, image.bundle.csv.GetName())
88+
if err != nil {
89+
// Assume that if we can not find a bundle for the package, channel and or CSV Name that this is safe to add
90+
continue
91+
}
92+
if bundle != nil {
93+
// raise error that this package + channel + csv combo is already in the db
94+
errs = append(errs, PackageVersionAlreadyAddedErr{ErrorString: "Bundle already added that provides package and csv"})
95+
break
96+
}
97+
}
98+
}
99+
100+
return utilerrors.NewAggregate(errs)
101+
}
102+
66103
func (i *DirectoryPopulator) loadManifests(imagesToAdd []*ImageInput, mode Mode) error {
104+
// global sanity checks before insertion
105+
err := i.globalSanityCheck(imagesToAdd)
106+
if err != nil {
107+
return err
108+
}
109+
67110
switch mode {
68111
case ReplacesMode:
69112
// TODO: This is relatively inefficient. Ideally, we should be able to use a replaces

‎pkg/registry/populator_test.go

Lines changed: 92 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"fmt"
77
"math/rand"
88
"os"
9+
"reflect"
910
"testing"
1011
"time"
1112

@@ -16,6 +17,8 @@ import (
1617
"github.com/operator-framework/operator-registry/pkg/image"
1718
"github.com/operator-framework/operator-registry/pkg/registry"
1819
"github.com/operator-framework/operator-registry/pkg/sqlite"
20+
21+
utilerrors "k8s.io/apimachinery/pkg/util/errors"
1922
)
2023

2124
func init() {
@@ -246,6 +249,7 @@ func TestImageLoading(t *testing.T) {
246249
addImage img
247250
wantPackages []*registry.Package
248251
wantErr bool
252+
err error
249253
}{
250254
{
251255
name: "OneChannel/AddBundleToTwoChannels",
@@ -386,6 +390,76 @@ func TestImageLoading(t *testing.T) {
386390
},
387391
wantErr: false,
388392
},
393+
{
394+
name: "AddBundleAlreadyExists",
395+
initImages: []img{
396+
{
397+
// this is in the "preview" channel
398+
ref: image.SimpleReference("quay.io/prometheus/operator:0.14.0"),
399+
dir: "../../bundles/prometheus.0.14.0",
400+
},
401+
},
402+
addImage: img{
403+
//Adding same bundle different bundle
404+
ref: image.SimpleReference("quay.io/prometheus/operator-test:testing"),
405+
dir: "../../bundles/prometheus.0.14.0",
406+
},
407+
wantPackages: []*registry.Package{
408+
{
409+
Name: "prometheus",
410+
DefaultChannel: "stable",
411+
Channels: map[string]registry.Channel{
412+
"preview": {
413+
Head: registry.BundleKey{
414+
BundlePath: "quay.io/prometheus/operator:0.14.0",
415+
Version: "0.14.0",
416+
CsvName: "prometheusoperator.0.14.0",
417+
},
418+
Nodes: map[registry.BundleKey]map[registry.BundleKey]struct{}{
419+
{BundlePath: "quay.io/prometheus/operator:0.14.0", Version: "0.14.0", CsvName: "prometheusoperator.0.14.0"}: {},
420+
},
421+
},
422+
},
423+
},
424+
},
425+
wantErr: true,
426+
err: registry.PackageVersionAlreadyAddedErr{},
427+
},
428+
{
429+
name: "AddExactBundleAlreadyExists",
430+
initImages: []img{
431+
{
432+
// this is in the "preview" channel
433+
ref: image.SimpleReference("quay.io/prometheus/operator:0.14.0"),
434+
dir: "../../bundles/prometheus.0.14.0",
435+
},
436+
},
437+
addImage: img{
438+
// Add the same package
439+
ref: image.SimpleReference("quay.io/prometheus/operator:0.14.0"),
440+
dir: "../../bundles/prometheus.0.14.0",
441+
},
442+
wantPackages: []*registry.Package{
443+
{
444+
Name: "prometheus",
445+
DefaultChannel: "stable",
446+
Channels: map[string]registry.Channel{
447+
"preview": {
448+
Head: registry.BundleKey{
449+
BundlePath: "quay.io/prometheus/operator:0.14.0",
450+
Version: "0.14.0",
451+
CsvName: "prometheusoperator.0.14.0",
452+
},
453+
Nodes: map[registry.BundleKey]map[registry.BundleKey]struct{}{
454+
{BundlePath: "quay.io/prometheus/operator:0.14.0", Version: "0.14.0", CsvName: "prometheusoperator.0.14.0"}: {},
455+
},
456+
},
457+
},
458+
},
459+
},
460+
wantErr: true,
461+
err: registry.BundleImageAlreadyAddedErr{},
462+
},
389463
}
390464
for _, tt := range tests {
391465
t.Run(tt.name, func(t *testing.T) {
@@ -411,7 +485,12 @@ func TestImageLoading(t *testing.T) {
411485
graphLoader,
412486
query,
413487
map[image.Reference]string{tt.addImage.ref: tt.addImage.dir})
414-
require.NoError(t, add.Populate(registry.ReplacesMode))
488+
err = add.Populate(registry.ReplacesMode)
489+
if tt.wantErr {
490+
require.True(t, checkAggErr(err, tt.err))
491+
return
492+
}
493+
require.NoError(t, err)
415494

416495
for _, p := range tt.wantPackages {
417496
graphLoader, err := sqlite.NewSQLGraphLoaderFromDB(db)
@@ -426,6 +505,18 @@ func TestImageLoading(t *testing.T) {
426505
}
427506
}
428507

508+
func checkAggErr(aggErr, wantErr error) bool {
509+
if a, ok := aggErr.(utilerrors.Aggregate); ok {
510+
for _, e := range a.Errors() {
511+
if reflect.TypeOf(e).String() == reflect.TypeOf(wantErr).String() {
512+
return true
513+
}
514+
}
515+
return false
516+
}
517+
return reflect.TypeOf(aggErr).String() == reflect.TypeOf(wantErr).String()
518+
}
519+
429520
func TestQuerierForDependencies(t *testing.T) {
430521
logrus.SetLevel(logrus.DebugLevel)
431522
db, cleanup := CreateTestDb(t)

‎pkg/registry/types.go

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,24 @@ var (
1414
ErrPackageNotInDatabase = errors.New("Package not in database")
1515
)
1616

17+
// BundleImageAlreadyAddedErr is an error that describes a bundle is already added
18+
type BundleImageAlreadyAddedErr struct {
19+
ErrorString string
20+
}
21+
22+
func (e BundleImageAlreadyAddedErr) Error() string {
23+
return e.ErrorString
24+
}
25+
26+
// PackageVersionAlreadyAddedErr is an error that describes that a bundle that is already in the databse that provides this package and version
27+
type PackageVersionAlreadyAddedErr struct {
28+
ErrorString string
29+
}
30+
31+
func (e PackageVersionAlreadyAddedErr) Error() string {
32+
return e.ErrorString
33+
}
34+
1735
const (
1836
GVKType = "olm.gvk"
1937
PackageType = "olm.package"

0 commit comments

Comments
 (0)