diff --git a/cmd/opm/alpha/diff/cmd.go b/cmd/opm/alpha/diff/cmd.go index 4420ead2b..5da7e9288 100644 --- a/cmd/opm/alpha/diff/cmd.go +++ b/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...' diff --git a/internal/action/diff.go b/internal/action/diff.go index 27c06432f..6b022a137 100644 --- a/internal/action/diff.go +++ b/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) diff --git a/internal/action/diff_test.go b/internal/action/diff_test.go index 71aa55db6..67951d6b9 100644 --- a/internal/action/diff_test.go +++ b/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 { diff --git a/internal/action/list.go b/internal/action/list.go index 972489226..ae44405cd 100644 --- a/internal/action/list.go +++ b/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 diff --git a/internal/action/render.go b/internal/action/render.go index d6e620790..2c9c9b3da 100644 --- a/internal/action/render.go +++ b/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,7 +142,7 @@ 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 { @@ -155,7 +150,7 @@ func (r Render) imageToDeclcfg(ctx context.Context, imageRef string) (*declcfg.D } } 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 { @@ -163,7 +158,7 @@ func (r Render) imageToDeclcfg(ctx context.Context, imageRef string) (*declcfg.D } } 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 { diff --git a/internal/action/render_test.go b/internal/action/render_test.go index 0f04c7592..a9017f4f4 100644 --- a/internal/action/render_test.go +++ b/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",