From 32cdb3c33da0a4ef54ef41fe4e1689a4495a63fc Mon Sep 17 00:00:00 2001 From: Eric Stroczynski Date: Wed, 4 Aug 2021 18:06:28 -0700 Subject: [PATCH 1/2] fix(opm): clarify that bundle declcfgs are not valid refs alone Signed-off-by: Eric Stroczynski --- cmd/opm/alpha/diff/cmd.go | 3 +++ internal/action/diff.go | 19 +++++++++++++++++-- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/cmd/opm/alpha/diff/cmd.go b/cmd/opm/alpha/diff/cmd.go index 67be0cc05..33c27c4ac 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..14897e8fc 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 isNotAllowedError(err) { + return nil, fmt.Errorf("%v (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 isNotAllowedError(err) { + return nil, fmt.Errorf("%v (diff does not permit direct bundle references)", err) + } return nil, fmt.Errorf("error rendering new refs: %v", err) } newModel, err := declcfg.ConvertToModel(*newCfg) @@ -64,3 +74,8 @@ func (p Diff) validate() error { } return nil } + +func isNotAllowedError(err error) bool { + errNotAllowed := &ErrNotAllowed{} + return errors.As(err, &errNotAllowed) +} From f20d176edb3fb0b6a6443ff4a7f620adee38b206 Mon Sep 17 00:00:00 2001 From: Eric Stroczynski Date: Thu, 5 Aug 2021 11:13:35 -0700 Subject: [PATCH 2/2] internal/action: changed ErrNotAllowed to variable for correct errors.Is() comparison Signed-off-by: Eric Stroczynski --- cmd/opm/alpha/diff/cmd.go | 4 ++-- internal/action/diff.go | 13 ++++--------- internal/action/diff_test.go | 33 +++++++++++++++++++++++++++++++++ internal/action/list.go | 2 +- internal/action/render.go | 19 +++++++------------ internal/action/render_test.go | 10 +++++----- 6 files changed, 52 insertions(+), 29 deletions(-) diff --git a/cmd/opm/alpha/diff/cmd.go b/cmd/opm/alpha/diff/cmd.go index 33c27c4ac..809232db6 100644 --- a/cmd/opm/alpha/diff/cmd.go +++ b/cmd/opm/alpha/diff/cmd.go @@ -47,8 +47,8 @@ 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. +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 14897e8fc..6b022a137 100644 --- a/internal/action/diff.go +++ b/internal/action/diff.go @@ -35,8 +35,8 @@ func (a Diff) Run(ctx context.Context) (*declcfg.DeclarativeConfig, error) { oldRender := Render{Refs: a.OldRefs, Registry: a.Registry, AllowedRefMask: mask} oldCfg, err := oldRender.Run(ctx) if err != nil { - if isNotAllowedError(err) { - return nil, fmt.Errorf("%v (diff does not permit direct bundle references)", err) + 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) } @@ -49,8 +49,8 @@ func (a Diff) Run(ctx context.Context) (*declcfg.DeclarativeConfig, error) { newRender := Render{Refs: a.NewRefs, Registry: a.Registry, AllowedRefMask: mask} newCfg, err := newRender.Run(ctx) if err != nil { - if isNotAllowedError(err) { - return nil, fmt.Errorf("%v (diff does not permit direct bundle references)", err) + 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) } @@ -74,8 +74,3 @@ func (p Diff) validate() error { } return nil } - -func isNotAllowedError(err error) bool { - errNotAllowed := &ErrNotAllowed{} - return errors.As(err, &errNotAllowed) -} 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",