Merge pull request #164 from estroz/bz/1989431

Bug 1989431: fix(opm): clarify that bundle declcfgs are not valid refs alone

Author: openshift-ci[bot]

staging/operator-registry/cmd/opm/alpha/diff/cmd.go

@@ -45,7 +45,10 @@ func NewCmd() *cobra.Command {
 Diff a set of old and new catalog references ("refs") to produce a
 declarative config containing only packages channels, and versions not present
 in the old set, and versions that differ between the old and new sets. This is known as "latest" mode.
+
 These references are passed through 'opm render' to produce a single declarative config.
+Bundle image refs are not supported directly; a valid "olm.package" declarative config object
+referring to the bundle's package must exist in all input refs.
 
 This command has special behavior when old-refs are omitted, called "heads-only" mode:
 instead of the output being that of 'opm render refs...'

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

@@ -2,6 +2,7 @@ package action
 
 import (
 	"context"
+	"errors"
 	"fmt"
 
 	"github.com/sirupsen/logrus"
@@ -25,12 +26,18 @@ func (a Diff) Run(ctx context.Context) (*declcfg.DeclarativeConfig, error) {
 		return nil, err
 	}
 
+	// Disallow bundle refs.
+	mask := RefDCDir | RefDCImage | RefSqliteFile | RefSqliteImage
+
 	// Heads-only mode does not require an old ref, so there may be nothing to render.
 	var oldModel model.Model
 	if len(a.OldRefs) != 0 {
-		oldRender := Render{Refs: a.OldRefs, Registry: a.Registry}
+		oldRender := Render{Refs: a.OldRefs, Registry: a.Registry, AllowedRefMask: mask}
 		oldCfg, err := oldRender.Run(ctx)
 		if err != nil {
+			if errors.Is(err, ErrNotAllowed) {
+				return nil, fmt.Errorf("%w (diff does not permit direct bundle references)", err)
+			}
 			return nil, fmt.Errorf("error rendering old refs: %v", err)
 		}
 		oldModel, err = declcfg.ConvertToModel(*oldCfg)
@@ -39,9 +46,12 @@ func (a Diff) Run(ctx context.Context) (*declcfg.DeclarativeConfig, error) {
 		}
 	}
 
-	newRender := Render{Refs: a.NewRefs, Registry: a.Registry}
+	newRender := Render{Refs: a.NewRefs, Registry: a.Registry, AllowedRefMask: mask}
 	newCfg, err := newRender.Run(ctx)
 	if err != nil {
+		if errors.Is(err, ErrNotAllowed) {
+			return nil, fmt.Errorf("%w (diff does not permit direct bundle references)", err)
+		}
 		return nil, fmt.Errorf("error rendering new refs: %v", err)
 	}
 	newModel, err := declcfg.ConvertToModel(*newCfg)

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

@@ -3,12 +3,14 @@ package action_test
 import (
 	"context"
 	"embed"
+	"errors"
 	"io/fs"
 	"path"
 	"path/filepath"
 	"strings"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"github.com/operator-framework/operator-registry/internal/action"
@@ -49,6 +51,37 @@ func TestDiff(t *testing.T) {
 			expectedCfg: loadDirFS(t, indicesDir, filepath.Join("testdata", "index-declcfgs", "exp-headsonly")),
 			assertion:   require.NoError,
 		},
+		{
+			name: "Fail/NewBundleImage",
+			diff: action.Diff{
+				Registry: registry,
+				NewRefs:  []string{"test.registry/foo-operator/foo-bundle:v0.1.0"},
+			},
+			assertion: func(t require.TestingT, err error, _ ...interface{}) {
+				if !assert.Error(t, err) {
+					require.Fail(t, "expected an error")
+				}
+				if !errors.Is(err, action.ErrNotAllowed) {
+					require.Fail(t, "err is not action.ErrNotAllowed", err)
+				}
+			},
+		},
+		{
+			name: "Fail/OldBundleImage",
+			diff: action.Diff{
+				Registry: registry,
+				OldRefs:  []string{"test.registry/foo-operator/foo-bundle:v0.1.0"},
+				NewRefs:  []string{filepath.Join("testdata", "index-declcfgs", "latest")},
+			},
+			assertion: func(t require.TestingT, err error, _ ...interface{}) {
+				if !assert.Error(t, err) {
+					require.Fail(t, "expected an error")
+				}
+				if !errors.Is(err, action.ErrNotAllowed) {
+					require.Fail(t, "err is not action.ErrNotAllowed", err)
+				}
+			},
+		},
 	}
 
 	for _, s := range specs {

staging/operator-registry/internal/action/list.go

@@ -206,7 +206,7 @@ func indexRefToModel(ctx context.Context, ref string) (model.Model, error) {
 	}
 	cfg, err := render.Run(ctx)
 	if err != nil {
-		if errors.Is(err, &ErrNotAllowed{}) {
+		if errors.Is(err, ErrNotAllowed) {
 			return nil, fmt.Errorf("cannot list non-index %q", ref)
 		}
 		return nil, err

staging/operator-registry/internal/action/render.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"database/sql"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io/ioutil"
 	"os"
@@ -41,13 +42,7 @@ func (r RefType) Allowed(refType RefType) bool {
 	return r == RefAll || r&refType == refType
 }
 
-var _ error = &ErrNotAllowed{}
-
-type ErrNotAllowed struct{}
-
-func (_ *ErrNotAllowed) Error() string {
-	return "not allowed"
-}
+var ErrNotAllowed = errors.New("not allowed")
 
 type Render struct {
 	Refs           []string
@@ -108,7 +103,7 @@ func (r Render) renderReference(ctx context.Context, ref string) (*declcfg.Decla
 	if stat, serr := os.Stat(ref); serr == nil {
 		if stat.IsDir() {
 			if !r.AllowedRefMask.Allowed(RefDCDir) {
-				return nil, fmt.Errorf("cannot render DC directory: %w", &ErrNotAllowed{})
+				return nil, fmt.Errorf("cannot render declarative config directory: %w", ErrNotAllowed)
 			}
 			return declcfg.LoadFS(os.DirFS(ref))
 		} else {
@@ -118,7 +113,7 @@ func (r Render) renderReference(ctx context.Context, ref string) (*declcfg.Decla
 				return nil, err
 			}
 			if !r.AllowedRefMask.Allowed(RefSqliteFile) {
-				return nil, fmt.Errorf("cannot render sqlite file: %w", &ErrNotAllowed{})
+				return nil, fmt.Errorf("cannot render sqlite file: %w", ErrNotAllowed)
 			}
 			return sqliteToDeclcfg(ctx, ref)
 		}
@@ -147,23 +142,23 @@ func (r Render) imageToDeclcfg(ctx context.Context, imageRef string) (*declcfg.D
 	var cfg *declcfg.DeclarativeConfig
 	if dbFile, ok := labels[containertools.DbLocationLabel]; ok {
 		if !r.AllowedRefMask.Allowed(RefSqliteImage) {
-			return nil, fmt.Errorf("cannot render sqlite image: %w", &ErrNotAllowed{})
+			return nil, fmt.Errorf("cannot render sqlite image: %w", ErrNotAllowed)
 		}
 		cfg, err = sqliteToDeclcfg(ctx, filepath.Join(tmpDir, dbFile))
 		if err != nil {
 			return nil, err
 		}
 	} else if configsDir, ok := labels[containertools.ConfigsLocationLabel]; ok {
 		if !r.AllowedRefMask.Allowed(RefDCImage) {
-			return nil, fmt.Errorf("cannot render DC image: %w", &ErrNotAllowed{})
+			return nil, fmt.Errorf("cannot render declarative config image: %w", ErrNotAllowed)
 		}
 		cfg, err = declcfg.LoadFS(os.DirFS(filepath.Join(tmpDir, configsDir)))
 		if err != nil {
 			return nil, err
 		}
 	} else if _, ok := labels[bundle.PackageLabel]; ok {
 		if !r.AllowedRefMask.Allowed(RefBundleImage) {
-			return nil, fmt.Errorf("cannot render bundle image: %w", &ErrNotAllowed{})
+			return nil, fmt.Errorf("cannot render bundle image: %w", ErrNotAllowed)
 		}
 		img, err := registry.NewImageInput(ref, tmpDir)
 		if err != nil {

staging/operator-registry/internal/action/render_test.go

@@ -448,7 +448,7 @@ func TestAllowRefMask(t *testing.T) {
 				Registry:       reg,
 				AllowedRefMask: action.RefDCImage | action.RefDCDir | action.RefSqliteFile | action.RefBundleImage,
 			},
-			expectErr: &action.ErrNotAllowed{},
+			expectErr: action.ErrNotAllowed,
 		},
 		{
 			name: "SqliteFile/Allowed",
@@ -466,7 +466,7 @@ func TestAllowRefMask(t *testing.T) {
 				Registry:       reg,
 				AllowedRefMask: action.RefDCImage | action.RefDCDir | action.RefSqliteImage | action.RefBundleImage,
 			},
-			expectErr: &action.ErrNotAllowed{},
+			expectErr: action.ErrNotAllowed,
 		},
 		{
 			name: "DeclcfgImage/Allowed",
@@ -484,7 +484,7 @@ func TestAllowRefMask(t *testing.T) {
 				Registry:       reg,
 				AllowedRefMask: action.RefDCDir | action.RefSqliteImage | action.RefSqliteFile | action.RefBundleImage,
 			},
-			expectErr: &action.ErrNotAllowed{},
+			expectErr: action.ErrNotAllowed,
 		},
 		{
 			name: "DeclcfgDir/Allowed",
@@ -502,7 +502,7 @@ func TestAllowRefMask(t *testing.T) {
 				Registry:       reg,
 				AllowedRefMask: action.RefDCImage | action.RefSqliteImage | action.RefSqliteFile | action.RefBundleImage,
 			},
-			expectErr: &action.ErrNotAllowed{},
+			expectErr: action.ErrNotAllowed,
 		},
 		{
 			name: "BundleImage/Allowed",
@@ -520,7 +520,7 @@ func TestAllowRefMask(t *testing.T) {
 				Registry:       reg,
 				AllowedRefMask: action.RefDCImage | action.RefDCDir | action.RefSqliteImage | action.RefSqliteFile,
 			},
-			expectErr: &action.ErrNotAllowed{},
+			expectErr: action.ErrNotAllowed,
 		},
 		{
 			name: "All/Allowed",