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

staging/operator-registry/alpha/action/diff.go

@@ -5,6 +5,7 @@ import (
 	"errors"
 	"fmt"
 
+	"github.com/blang/semver/v4"
 	"github.com/sirupsen/logrus"
 
 	"github.com/operator-framework/operator-registry/alpha/declcfg"

staging/operator-registry/alpha/action/diff_test.go

@@ -10,6 +10,7 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/blang/semver/v4"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 

staging/operator-registry/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"

staging/operator-registry/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"
 

staging/operator-registry/alpha/declcfg/diff_include.go

@@ -1,6 +1,13 @@
 package declcfg
 
 import (
+	"fmt"
+	"strings"
+
+	"github.com/blang/semver/v4"
+	"github.com/sirupsen/logrus"
+	utilerrors "k8s.io/apimachinery/pkg/util/errors"
+
 	"github.com/operator-framework/operator-registry/alpha/model"
 )
 

staging/operator-registry/alpha/declcfg/diff_test.go

@@ -3,6 +3,7 @@ package declcfg
 import (
 	"testing"
 
+	"github.com/blang/semver/v4"
 	"github.com/stretchr/testify/require"
 
 	"github.com/operator-framework/operator-registry/alpha/model"

staging/operator-registry/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"

staging/operator-registry/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.8

staging/operator-registry/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.

staging/operator-registry/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"
 )
 

staging/operator-registry/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

staging/operator-registry/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"

staging/operator-registry/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

staging/operator-registry/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"
 )
 

staging/operator-registry/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"
 )
 

staging/operator-registry/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"
 
@@ -142,6 +143,9 @@ func (i *DirectoryPopulator) globalSanityCheck(imagesToAdd []*ImageInput) error
 		}
 	}
 
+	if err := i.ValidateEdgeBundlePackage(imagesToAdd); err != nil {
+		errs = append(errs, err)
+	}
 	return utilerrors.NewAggregate(errs)
 }
 
@@ -408,3 +412,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
+}

staging/operator-registry/pkg/registry/populator_test.go

@@ -12,11 +12,14 @@ import (
 	"testing"
 	"time"
 
+	"github.com/blang/semver/v4"
 	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/require"
 	"k8s.io/apimachinery/pkg/util/errors"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 
+	"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/registry"
@@ -2621,3 +2624,142 @@ func TestEnableAlpha(t *testing.T) {
 		})
 	}
 }
+
+func newUnpackedTestBundle(root, dir, name string, csvSpec json.RawMessage, annotations registry.Annotations) (string, func(), error) {
+	bundleDir := filepath.Join(root, dir)
+	cleanup := func() {
+		os.RemoveAll(bundleDir)
+	}
+	if err := os.Mkdir(bundleDir, 0755); err != nil {
+		return bundleDir, cleanup, err
+	}
+	if err := os.Mkdir(filepath.Join(bundleDir, bundle.ManifestsDir), 0755); err != nil {
+		return bundleDir, cleanup, err
+	}
+	if err := os.Mkdir(filepath.Join(bundleDir, bundle.MetadataDir), 0755); err != nil {
+		return bundleDir, cleanup, err
+	}
+	if len(csvSpec) == 0 {
+		csvSpec = json.RawMessage(`{}`)
+	}
+
+	rawCSV, err := json.Marshal(registry.ClusterServiceVersion{
+		TypeMeta: v1.TypeMeta{
+			Kind: sqlite.ClusterServiceVersionKind,
+		},
+		ObjectMeta: v1.ObjectMeta{
+			Name: name,
+		},
+		Spec: csvSpec,
+	})
+	if err != nil {
+		return bundleDir, cleanup, err
+	}
+
+	rawObj := unstructured.Unstructured{}
+	if err := json.Unmarshal(rawCSV, &rawObj); err != nil {
+		return bundleDir, cleanup, err
+	}
+	rawObj.SetCreationTimestamp(v1.Time{})
+
+	jsonout, err := rawObj.MarshalJSON()
+	out, err := yaml.JSONToYAML(jsonout)
+	if err != nil {
+		return bundleDir, cleanup, err
+	}
+	if err := ioutil.WriteFile(filepath.Join(bundleDir, bundle.ManifestsDir, "csv.yaml"), out, 0666); err != nil {
+		return bundleDir, cleanup, err
+	}
+
+	out, err = yaml.Marshal(registry.AnnotationsFile{Annotations: annotations})
+	if err != nil {
+		return bundleDir, cleanup, err
+	}
+	if err := ioutil.WriteFile(filepath.Join(bundleDir, bundle.MetadataDir, "annotations.yaml"), out, 0666); err != nil {
+		return bundleDir, cleanup, err
+	}
+	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())
+		})
+	}
+}

staging/operator-registry/pkg/registry/types.go

@@ -7,7 +7,7 @@ import (
 	"sort"
 	"strings"
 
-	"github.com/blang/semver"
+	"github.com/blang/semver/v4"
 )
 
 var (

staging/operator-registry/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"