Block cross-package skips/replaces on add (#826)

ankitathomas · web-flow · commit 0c3c5ca31a6d · 2021-11-17T20:16:29.000+01:00
* Block cross-package skips/replaces on add

Signed-off-by: Ankita Thomas &lt;ankithom@redhat.com&gt;

* bump blang/semver to v4

Signed-off-by: Ankita Thomas &lt;ankithom@redhat.com&gt;
diff --git a/alpha/action/diff.go b/alpha/action/diff.go
@@ -6,7 +6,7 @@ import (
 	"fmt"
 	"io"
 
-	"github.com/blang/semver"
+	"github.com/blang/semver/v4"
 	"github.com/sirupsen/logrus"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/apimachinery/pkg/util/yaml"
diff --git a/alpha/action/diff_test.go b/alpha/action/diff_test.go
@@ -11,7 +11,7 @@ import (
 	"strings"
 	"testing"
 
-	"github.com/blang/semver"
+	"github.com/blang/semver/v4"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
diff --git a/alpha/declcfg/declcfg_to_model.go b/alpha/declcfg/declcfg_to_model.go
@@ -3,7 +3,7 @@ package declcfg
 import (
 	"fmt"
 
-	"github.com/blang/semver"
+	"github.com/blang/semver/v4"
 	"k8s.io/apimachinery/pkg/util/sets"
 
 	"github.com/operator-framework/operator-registry/alpha/model"
diff --git a/alpha/declcfg/diff.go b/alpha/declcfg/diff.go
@@ -6,7 +6,7 @@ import (
 	"sort"
 	"sync"
 
-	"github.com/blang/semver"
+	"github.com/blang/semver/v4"
 	"github.com/mitchellh/hashstructure/v2"
 	"github.com/sirupsen/logrus"
 
diff --git a/alpha/declcfg/diff_include.go b/alpha/declcfg/diff_include.go
@@ -4,7 +4,7 @@ import (
 	"fmt"
 	"strings"
 
-	"github.com/blang/semver"
+	"github.com/blang/semver/v4"
 	"github.com/sirupsen/logrus"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 
diff --git a/alpha/declcfg/diff_test.go b/alpha/declcfg/diff_test.go
@@ -3,7 +3,7 @@ package declcfg
 import (
 	"testing"
 
-	"github.com/blang/semver"
+	"github.com/blang/semver/v4"
 	"github.com/stretchr/testify/require"
 
 	"github.com/operator-framework/operator-registry/alpha/model"
diff --git a/alpha/model/model.go b/alpha/model/model.go
@@ -6,7 +6,7 @@ import (
 	"sort"
 	"strings"
 
-	"github.com/blang/semver"
+	"github.com/blang/semver/v4"
 	"github.com/h2non/filetype"
 	"github.com/h2non/filetype/matchers"
 	"github.com/h2non/filetype/types"
diff --git a/go.mod b/go.mod
@@ -4,7 +4,7 @@ go 1.16
 
 require (
 	github.com/Microsoft/hcsshim v0.8.9 // indirect
-	github.com/blang/semver v3.5.1+incompatible
+	github.com/blang/semver/v4 v4.0.0
 	github.com/bugsnag/bugsnag-go v1.5.3 // indirect
 	github.com/bugsnag/panicwrap v1.2.0 // indirect
 	github.com/containerd/containerd v1.4.11
diff --git a/pkg/lib/semver/semver.go b/pkg/lib/semver/semver.go
@@ -3,7 +3,7 @@ package semver
 import (
 	"fmt"
 
-	"github.com/blang/semver"
+	"github.com/blang/semver/v4"
 )
 
 // BuildIdCompare compares two versions and returns negative one if the first arg is less than the second arg, positive one if it is larger, and zero if they are equal.
diff --git a/pkg/lib/semver/semver_test.go b/pkg/lib/semver/semver_test.go
@@ -3,7 +3,7 @@ package semver
 import (
 	"testing"
 
-	"github.com/blang/semver"
+	"github.com/blang/semver/v4"
 	"github.com/stretchr/testify/require"
 )
 
diff --git a/pkg/registry/bundlegraphloader.go b/pkg/registry/bundlegraphloader.go
@@ -3,7 +3,7 @@ package registry
 import (
 	"fmt"
 
-	"github.com/blang/semver"
+	"github.com/blang/semver/v4"
 )
 
 // BundleGraphLoader generates updated graphs by adding bundles to them, updating
diff --git a/pkg/registry/bundlegraphloader_test.go b/pkg/registry/bundlegraphloader_test.go
@@ -4,7 +4,7 @@ import (
 	"encoding/json"
 	"testing"
 
-	"github.com/blang/semver"
+	"github.com/blang/semver/v4"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
diff --git a/pkg/registry/csv.go b/pkg/registry/csv.go
@@ -50,7 +50,7 @@ const (
 	description = "description"
 
 	// The yaml attribute that specifies the version of the ClusterServiceVersion
-	// expected to be semver and parseable by blang/semver
+	// expected to be semver and parseable by blang/semver/v4
 	version = "version"
 
 	// The yaml attribute that specifies the related images of the ClusterServiceVersion
diff --git a/pkg/registry/directoryGraphLoader.go b/pkg/registry/directoryGraphLoader.go
@@ -8,7 +8,7 @@ import (
 	"path/filepath"
 	"sort"
 
-	"github.com/blang/semver"
+	"github.com/blang/semver/v4"
 	"github.com/onsi/gomega/gstruct/errors"
 )
 
diff --git a/pkg/registry/helper_test.go b/pkg/registry/helper_test.go
@@ -3,7 +3,7 @@ package registry
 import (
 	"testing"
 
-	"github.com/blang/semver"
+	"github.com/blang/semver/v4"
 	"github.com/stretchr/testify/require"
 )
 
diff --git a/pkg/registry/populator.go b/pkg/registry/populator.go
@@ -5,8 +5,9 @@ import (
 	"errors"
 	"fmt"
 	"os"
+	"sort"
 
-	"github.com/blang/semver"
+	"github.com/blang/semver/v4"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/apimachinery/pkg/util/yaml"
 
@@ -128,6 +129,9 @@ func (i *DirectoryPopulator) globalSanityCheck(imagesToAdd []*ImageInput) error
 		}
 	}
 
+	if err := i.ValidateEdgeBundlePackage(imagesToAdd); err != nil {
+		errs = append(errs, err)
+	}
 	return utilerrors.NewAggregate(errs)
 }
 
@@ -407,3 +411,54 @@ func DecodeFile(path string, into interface{}) error {
 
 	return decoder.Decode(into)
 }
+
+// ValidateEdgeBundlePackage ensures that all bundles in the input will only skip or replace bundles in the same package.
+func (i *DirectoryPopulator) ValidateEdgeBundlePackage(images []*ImageInput) error {
+	// track packages for encountered bundles
+	expectedBundlePackages := map[string]string{}
+	for _, b := range images {
+		r, err := b.Bundle.Replaces()
+		if err != nil {
+			return fmt.Errorf("failed to validate replaces for bundle %s(%s): %v", b.Bundle.Name, b.Bundle.BundleImage, err)
+		}
+
+		skipped, err := b.Bundle.Skips()
+		if err != nil {
+			return fmt.Errorf("failed to validate skipped entries for bundle %s(%s): %v", b.Bundle.Name, b.Bundle.BundleImage, err)
+		}
+
+		for _, bndl := range append(skipped, r, b.Bundle.Name) {
+			if len(bndl) == 0 {
+				continue
+			}
+
+			if pkg, ok := expectedBundlePackages[bndl]; ok && pkg != b.Bundle.Package {
+				pkgs := []string{pkg, b.Bundle.Package}
+				sort.Strings(pkgs)
+				return fmt.Errorf("bundle %s must belong to exactly one package, found on: %v", bndl, pkgs)
+			}
+			expectedBundlePackages[bndl] = b.Bundle.Package
+		}
+	}
+	if len(expectedBundlePackages) == 0 {
+		return nil
+	}
+
+	pkgs, err := i.querier.ListPackages(context.TODO())
+	if err != nil {
+		return fmt.Errorf("unable to verify bundle packages: %v", err)
+	}
+	for _, pkg := range pkgs {
+		entries, err := i.querier.GetChannelEntriesFromPackage(context.TODO(), pkg)
+		if err != nil {
+			return fmt.Errorf("unable to verify bundles for package %v", err)
+		}
+		for _, b := range entries {
+			if bundlePkg, ok := expectedBundlePackages[b.BundleName]; ok && bundlePkg != b.PackageName {
+				return fmt.Errorf("bundle %s belongs to package %s on index, cannot be added as an edge for package %s", b.BundleName, b.PackageName, bundlePkg)
+			}
+		}
+	}
+
+	return nil
+}
diff --git a/pkg/registry/populator_test.go b/pkg/registry/populator_test.go
@@ -14,6 +14,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/blang/semver/v4"
 	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/require"
 	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -22,6 +23,8 @@ import (
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	"sigs.k8s.io/yaml"
 
+	"github.com/operator-framework/api/pkg/lib/version"
+	"github.com/operator-framework/api/pkg/operators/v1alpha1"
 	"github.com/operator-framework/operator-registry/pkg/api"
 	"github.com/operator-framework/operator-registry/pkg/image"
 	"github.com/operator-framework/operator-registry/pkg/lib/bundle"
@@ -3068,3 +3071,86 @@ func newUnpackedTestBundle(root, dir, name string, csvSpec json.RawMessage, anno
 	}
 	return bundleDir, cleanup, nil
 }
+
+func TestValidateEdgeBundlePackage(t *testing.T) {
+	newInput := func(name, versionString, pkg, defaultChannel, channels, replaces string, skips []string) *registry.ImageInput {
+		v, err := semver.Parse(versionString)
+		require.NoError(t, err)
+
+		spec := v1alpha1.ClusterServiceVersionSpec{
+			Replaces: replaces,
+			Skips:    skips,
+			Version:  version.OperatorVersion{v},
+		}
+		specJson, err := json.Marshal(&spec)
+		require.NoError(t, err)
+
+		rawCSV, err := json.Marshal(registry.ClusterServiceVersion{
+			TypeMeta: v1.TypeMeta{
+				Kind: sqlite.ClusterServiceVersionKind,
+			},
+			ObjectMeta: v1.ObjectMeta{
+				Name: name,
+			},
+			Spec: specJson,
+		})
+
+		rawObj := unstructured.Unstructured{}
+		require.NoError(t, json.Unmarshal(rawCSV, &rawObj))
+		rawObj.SetCreationTimestamp(v1.Time{})
+
+		jsonout, err := rawObj.MarshalJSON()
+		require.NoError(t, err)
+
+		b, err := registry.NewBundleFromStrings(name, versionString, pkg, defaultChannel, channels, string(jsonout))
+		require.NoError(t, err)
+		return &registry.ImageInput{Bundle: b}
+	}
+
+	logrus.SetLevel(logrus.DebugLevel)
+	db, cleanup := CreateTestDb(t)
+	defer cleanup()
+
+	store, err := createAndPopulateDB(db)
+	require.NoError(t, err)
+
+	r := registry.NewDirectoryPopulator(nil, nil, store, nil, nil)
+
+	tests := []struct {
+		name    string
+		args    []*registry.ImageInput
+		wantErr error
+	}{
+		{
+			name: "conflictInAdded",
+			args: []*registry.ImageInput{
+				newInput("b1", "0.0.1", "p1", "a", "a", "", []string{}),
+				newInput("b2", "0.0.2", "p2", "a", "a", "", []string{"b1"}),
+			},
+			wantErr: fmt.Errorf("bundle b1 must belong to exactly one package, found on: [p1 p2]"),
+		},
+		{
+			name: "conflictWithExisting",
+			args: []*registry.ImageInput{
+				newInput("b1", "0.0.1", "p1", "a", "a", "", []string{"etcdoperator.v0.9.0"}),
+			},
+			wantErr: fmt.Errorf("bundle etcdoperator.v0.9.0 belongs to package etcd on index, cannot be added as an edge for package p1"),
+		},
+		{
+			name: "passForNonConflicting",
+			args: []*registry.ImageInput{
+				newInput("b1", "0.0.1", "etcd", "a", "a", "", []string{"etcdoperator.v0.9.0"}),
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := r.ValidateEdgeBundlePackage(tt.args)
+			if tt.wantErr == nil {
+				require.NoError(t, err)
+				return
+			}
+			require.EqualError(t, err, tt.wantErr.Error())
+		})
+	}
+}
diff --git a/pkg/registry/types.go b/pkg/registry/types.go
@@ -7,7 +7,7 @@ import (
 	"sort"
 	"strings"
 
-	"github.com/blang/semver"
+	"github.com/blang/semver/v4"
 )
 
 var (
diff --git a/pkg/sqlite/load.go b/pkg/sqlite/load.go
@@ -7,7 +7,7 @@ import (
 	"fmt"
 	"strings"
 
-	"github.com/blang/semver"
+	"github.com/blang/semver/v4"
 	_ "github.com/mattn/go-sqlite3"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 
diff --git a/vendor/modules.txt b/vendor/modules.txt
@@ -42,9 +42,9 @@ github.com/asaskevich/govalidator
 # github.com/beorn7/perks v1.0.1
 github.com/beorn7/perks/quantile
 # github.com/blang/semver v3.5.1+incompatible
-## explicit
 github.com/blang/semver
 # github.com/blang/semver/v4 v4.0.0
+## explicit
 github.com/blang/semver/v4
 # github.com/bshuster-repo/logrus-logstash-hook v0.4.1
 github.com/bshuster-repo/logrus-logstash-hook

Original file line number	Diff line number	Diff line change
`@@ -3,7 +3,7 @@ package semver`
`3`	`3`	`import (`
`4`	`4`	`"fmt"`
`5`	`5`
`6`		`- "github.com/blang/semver"`
	`6`	`+ "github.com/blang/semver/v4"`
`7`	`7`	`)`
`8`	`8`
`9`	`9`	`// BuildIdCompare compares two versions and returns negative one if the first arg is less than the second arg, positive one if it is larger, and zero if they are equal.`
Original file line number	Diff line number	Diff line change
`@@ -3,7 +3,7 @@ package semver`
`3`	`3`	`import (`
`4`	`4`	`"testing"`
`5`	`5`
`6`		`- "github.com/blang/semver"`
	`6`	`+ "github.com/blang/semver/v4"`
`7`	`7`	`"github.com/stretchr/testify/require"`
`8`	`8`	`)`
`9`	`9`