From 5dd63fdb713a2b0869a8257705775d45d86639e7 Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Thu, 28 Dec 2023 08:09:12 +0000 Subject: [PATCH 01/42] chore(ux): show registry error and hint for dockehub Signed-off-by: Billy Zha --- cmd/oras/internal/errors/errors.go | 34 ++++++++++++++++++++++++++++++ cmd/oras/internal/option/target.go | 33 +++++++++++++++++++++++++++++ cmd/oras/root/pull.go | 8 +++---- 3 files changed, 71 insertions(+), 4 deletions(-) diff --git a/cmd/oras/internal/errors/errors.go b/cmd/oras/internal/errors/errors.go index b93144428..cd0507dcc 100644 --- a/cmd/oras/internal/errors/errors.go +++ b/cmd/oras/internal/errors/errors.go @@ -16,10 +16,12 @@ limitations under the License. package errors import ( + "errors" "fmt" "github.com/spf13/cobra" "oras.land/oras-go/v2/registry" + "oras.land/oras-go/v2/registry/remote/errcode" ) // Error is the error type for CLI error messaging. @@ -60,6 +62,38 @@ func CheckArgs(checker func(args []string) (bool, string), Usage string) cobra.P } } +func handleRegistryErr(err error, cmd *cobra.Command) *errcode.ErrorResponse { + var errResp *errcode.ErrorResponse + if errors.As(err, &errResp) { + cmd.SetErrPrefix("Error response from registry:") + } + return errResp +} + +// Handle handles error during cmd execution. +func Handle(cmd *cobra.Command, recommend func(err error, callPath string) string) *cobra.Command { + runE := cmd.RunE + cmd.RunE = func(cmd *cobra.Command, args []string) error { + err := runE(cmd, args) + if err == nil { + return nil + } + + errResp := handleRegistryErr(err, cmd) + if errResp == nil { + return nil + } + + // 2. return scrubbed error + return &Error{ + Err: errResp.Errors, + Recommendation: recommend(errResp, cmd.CommandPath()), + } + } + + return cmd +} + // NewErrEmptyTagOrDigest creates a new error based on the reference string. func NewErrEmptyTagOrDigest(ref registry.Reference) error { return NewErrEmptyTagOrDigestStr(ref.String()) diff --git a/cmd/oras/internal/option/target.go b/cmd/oras/internal/option/target.go index a2f0e6995..25cb0cd93 100644 --- a/cmd/oras/internal/option/target.go +++ b/cmd/oras/internal/option/target.go @@ -32,6 +32,7 @@ import ( "oras.land/oras-go/v2/content/oci" "oras.land/oras-go/v2/registry" "oras.land/oras-go/v2/registry/remote" + "oras.land/oras-go/v2/registry/remote/errcode" oerrors "oras.land/oras/cmd/oras/internal/errors" "oras.land/oras/cmd/oras/internal/fileref" ) @@ -239,6 +240,30 @@ func (opts *Target) EnsureReferenceNotEmpty() error { return nil } +// Recommend returns a recommendation for known errors. +func (opts *Target) Recommend(err error, callPath string) string { + if opts.IsOCILayout { + return "" + } + ref, parseErr := registry.ParseReference(opts.Path) + if parseErr != nil { + // this should not happen + return "" + } + + // docker.io/xxx -> docker.io/library/xxx + if respErr, ok := err.(*errcode.ErrorResponse); ok { + if ref.Registry == "docker.io" && respErr.URL.Host == ref.Host() { + if !strings.Contains(ref.Repository, "/") { + ref.Repository = "library/" + ref.Repository + return fmt.Sprintf("Namespace is missing, do you mean `%s %s`?", callPath, ref) + } + } + } + + return "" +} + // BinaryTarget struct contains flags and arguments specifying two registries or // image layouts. type BinaryTarget struct { @@ -269,3 +294,11 @@ func (opts *BinaryTarget) Parse() error { opts.To.resolveFlag = append(opts.resolveFlag, opts.To.resolveFlag...) return Parse(opts) } + +// Recommend returns a recommendation for known errors. +func (opts *BinaryTarget) Recommend(err error, callPath string) string { + if recommendation := opts.From.Recommend(err, callPath); recommendation != "" { + return recommendation + } + return opts.To.Recommend(err, callPath) +} diff --git a/cmd/oras/root/pull.go b/cmd/oras/root/pull.go index bf39e05e6..b85d2f8f6 100644 --- a/cmd/oras/root/pull.go +++ b/cmd/oras/root/pull.go @@ -92,7 +92,7 @@ Example - Pull artifact files from an OCI layout archive 'layout.tar': return option.Parse(&opts) }, RunE: func(cmd *cobra.Command, args []string) error { - return runPull(cmd.Context(), opts) + return runPull(cmd.Context(), &opts) }, } @@ -103,10 +103,10 @@ Example - Pull artifact files from an OCI layout archive 'layout.tar': cmd.Flags().StringVarP(&opts.ManifestConfigRef, "config", "", "", "output manifest config file") cmd.Flags().IntVarP(&opts.concurrency, "concurrency", "", 3, "concurrency level") option.ApplyFlags(&opts, cmd.Flags()) - return cmd + return oerrors.Handle(cmd, opts.Target.Recommend) } -func runPull(ctx context.Context, opts pullOptions) error { +func runPull(ctx context.Context, opts *pullOptions) error { ctx, logger := opts.WithContext(ctx) // Copy Options copyOptions := oras.DefaultCopyOptions @@ -133,7 +133,7 @@ func runPull(ctx context.Context, opts pullOptions) error { dst.AllowPathTraversalOnWrite = opts.PathTraversal dst.DisableOverwrite = opts.KeepOldFiles - desc, layerSkipped, err := doPull(ctx, src, dst, copyOptions, &opts) + desc, layerSkipped, err := doPull(ctx, src, dst, copyOptions, opts) if err != nil { if errors.Is(err, file.ErrPathTraversalDisallowed) { err = fmt.Errorf("%s: %w", "use flag --allow-path-traversal to allow insecurely pulling files outside of working directory", err) From 69941672ddd80335ef8ef24ab556c1f0085a1561 Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Thu, 28 Dec 2023 08:11:14 +0000 Subject: [PATCH 02/42] code clean Signed-off-by: Billy Zha --- cmd/oras/internal/errors/errors.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cmd/oras/internal/errors/errors.go b/cmd/oras/internal/errors/errors.go index cd0507dcc..72dc68f37 100644 --- a/cmd/oras/internal/errors/errors.go +++ b/cmd/oras/internal/errors/errors.go @@ -79,12 +79,13 @@ func Handle(cmd *cobra.Command, recommend func(err error, callPath string) strin return nil } + // 1. try extract registry error errResp := handleRegistryErr(err, cmd) if errResp == nil { return nil } - // 2. return scrubbed error + // 2.recommend & return scrubbed error return &Error{ Err: errResp.Errors, Recommendation: recommend(errResp, cmd.CommandPath()), From 3c10c7dd6102fb48f56487e2776df0220af193cf Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Fri, 29 Dec 2023 07:12:55 +0000 Subject: [PATCH 03/42] support not found error Signed-off-by: Billy Zha --- cmd/oras/internal/errors/errors.go | 41 ++++++++++---------- cmd/oras/internal/option/target.go | 61 +++++++++++++++++++++++------- cmd/oras/root/pull.go | 2 +- 3 files changed, 69 insertions(+), 35 deletions(-) diff --git a/cmd/oras/internal/errors/errors.go b/cmd/oras/internal/errors/errors.go index 72dc68f37..ceb28d0fa 100644 --- a/cmd/oras/internal/errors/errors.go +++ b/cmd/oras/internal/errors/errors.go @@ -16,14 +16,15 @@ limitations under the License. package errors import ( - "errors" "fmt" "github.com/spf13/cobra" "oras.land/oras-go/v2/registry" - "oras.land/oras-go/v2/registry/remote/errcode" ) +// RegistryErrorPrefix is the commandline prefix for errors from registry. +const RegistryErrorPrefix = "Error response from registry:" + // Error is the error type for CLI error messaging. type Error struct { Err error @@ -62,36 +63,36 @@ func CheckArgs(checker func(args []string) (bool, string), Usage string) cobra.P } } -func handleRegistryErr(err error, cmd *cobra.Command) *errcode.ErrorResponse { - var errResp *errcode.ErrorResponse - if errors.As(err, &errResp) { - cmd.SetErrPrefix("Error response from registry:") - } - return errResp +// Handler handles error during cmd execution. +type Handler interface { + // Handle handles error during cmd execution. + // If returned processor is nil, error requires no further processing. + Handle(err error, cmd *cobra.Command) (Processor, error) +} + +// Processor processes error. +type Processor interface { + Process(error) error + Recommend(err error, callPath string) string } -// Handle handles error during cmd execution. -func Handle(cmd *cobra.Command, recommend func(err error, callPath string) string) *cobra.Command { +// Command returns an error-handled for cobra command. +func Command(cmd *cobra.Command, handler Handler) *cobra.Command { runE := cmd.RunE cmd.RunE = func(cmd *cobra.Command, args []string) error { err := runE(cmd, args) if err == nil { return nil } - - // 1. try extract registry error - errResp := handleRegistryErr(err, cmd) - if errResp == nil { - return nil + processor, err := handler.Handle(err, cmd) + if processor == nil { + return err } - - // 2.recommend & return scrubbed error return &Error{ - Err: errResp.Errors, - Recommendation: recommend(errResp, cmd.CommandPath()), + Err: processor.Process(err), + Recommendation: processor.Recommend(err, cmd.CommandPath()), } } - return cmd } diff --git a/cmd/oras/internal/option/target.go b/cmd/oras/internal/option/target.go index 25cb0cd93..cfa5b0bf5 100644 --- a/cmd/oras/internal/option/target.go +++ b/cmd/oras/internal/option/target.go @@ -21,15 +21,18 @@ import ( "fmt" "io" "io/fs" + "net/http" "os" "strings" "sync" "github.com/sirupsen/logrus" + "github.com/spf13/cobra" "github.com/spf13/pflag" "oras.land/oras-go/v2" "oras.land/oras-go/v2/content" "oras.land/oras-go/v2/content/oci" + "oras.land/oras-go/v2/errdef" "oras.land/oras-go/v2/registry" "oras.land/oras-go/v2/registry/remote" "oras.land/oras-go/v2/registry/remote/errcode" @@ -44,6 +47,7 @@ const ( // Target struct contains flags and arguments specifying one registry or image // layout. +// Target implements errors.Handler and errors.Processor interface. type Target struct { Remote RawReference string @@ -240,32 +244,61 @@ func (opts *Target) EnsureReferenceNotEmpty() error { return nil } +// Handle handles error during cmd execution. +func (opts *Target) Handle(err error, cmd *cobra.Command) (oerrors.Processor, error) { + // handle registry error + if opts.IsOCILayout { + return nil, err + } + + // handle not found error from registry + if errors.Is(err, errdef.ErrNotFound) { + cmd.SetErrPrefix(oerrors.RegistryErrorPrefix) + return opts, err + } + + // handle error response + var errResp *errcode.ErrorResponse + if errors.As(err, &errResp) { + cmd.SetErrPrefix(oerrors.RegistryErrorPrefix) + return opts, errResp + } + return opts, err +} + +// Process returns a scrubbed error. +func (opts *Target) Process(err error) error { + if errResp, ok := err.(*errcode.ErrorResponse); ok { + // remove HTTP related info + return errResp.Errors + } + return err +} + // Recommend returns a recommendation for known errors. func (opts *Target) Recommend(err error, callPath string) string { if opts.IsOCILayout { return "" } - ref, parseErr := registry.ParseReference(opts.Path) + ref, parseErr := registry.ParseReference(opts.RawReference) if parseErr != nil { // this should not happen return "" } - // docker.io/xxx -> docker.io/library/xxx - if respErr, ok := err.(*errcode.ErrorResponse); ok { - if ref.Registry == "docker.io" && respErr.URL.Host == ref.Host() { - if !strings.Contains(ref.Repository, "/") { - ref.Repository = "library/" + ref.Repository - return fmt.Sprintf("Namespace is missing, do you mean `%s %s`?", callPath, ref) - } + if respErr, ok := err.(*errcode.ErrorResponse); ok && ref.Registry == "docker.io" && respErr.URL.Host == ref.Host() && respErr.StatusCode == http.StatusUnauthorized { + if !strings.Contains(ref.Repository, "/") { + // docker.io/xxx -> docker.io/library/xxx + ref.Repository = "library/" + ref.Repository + return fmt.Sprintf("Namespace is missing, do you mean `%s %s`?", callPath, ref) } } - return "" } // BinaryTarget struct contains flags and arguments specifying two registries or // image layouts. +// BinaryTarget implements errors.Handler interface. type BinaryTarget struct { From Target To Target @@ -295,10 +328,10 @@ func (opts *BinaryTarget) Parse() error { return Parse(opts) } -// Recommend returns a recommendation for known errors. -func (opts *BinaryTarget) Recommend(err error, callPath string) string { - if recommendation := opts.From.Recommend(err, callPath); recommendation != "" { - return recommendation +// Handle handles error during cmd execution. +func (opts *BinaryTarget) Handle(err error, cmd *cobra.Command) (oerrors.Processor, error) { + if processor, err := opts.From.Handle(err, cmd); processor != nil { + return processor, err } - return opts.To.Recommend(err, callPath) + return opts.To.Handle(err, cmd) } diff --git a/cmd/oras/root/pull.go b/cmd/oras/root/pull.go index b85d2f8f6..d59343002 100644 --- a/cmd/oras/root/pull.go +++ b/cmd/oras/root/pull.go @@ -103,7 +103,7 @@ Example - Pull artifact files from an OCI layout archive 'layout.tar': cmd.Flags().StringVarP(&opts.ManifestConfigRef, "config", "", "", "output manifest config file") cmd.Flags().IntVarP(&opts.concurrency, "concurrency", "", 3, "concurrency level") option.ApplyFlags(&opts, cmd.Flags()) - return oerrors.Handle(cmd, opts.Target.Recommend) + return oerrors.Command(cmd, &opts.Target) } func runPull(ctx context.Context, opts *pullOptions) error { From 0925a45625cda2fde1f3967199ec8b7c3544e6ba Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Fri, 29 Dec 2023 09:09:01 +0000 Subject: [PATCH 04/42] support `oras ls` and `oras pull` Signed-off-by: Billy Zha --- cmd/oras/internal/errors/errors.go | 8 +--- cmd/oras/internal/option/remote.go | 35 ++++++++++++++++- cmd/oras/internal/option/target.go | 62 +++++++++++++----------------- cmd/oras/root/login.go | 2 +- cmd/oras/root/repo/ls.go | 2 +- 5 files changed, 64 insertions(+), 45 deletions(-) diff --git a/cmd/oras/internal/errors/errors.go b/cmd/oras/internal/errors/errors.go index ceb28d0fa..fe0f2164d 100644 --- a/cmd/oras/internal/errors/errors.go +++ b/cmd/oras/internal/errors/errors.go @@ -72,8 +72,7 @@ type Handler interface { // Processor processes error. type Processor interface { - Process(error) error - Recommend(err error, callPath string) string + Process(err error, callPath string) *Error } // Command returns an error-handled for cobra command. @@ -88,10 +87,7 @@ func Command(cmd *cobra.Command, handler Handler) *cobra.Command { if processor == nil { return err } - return &Error{ - Err: processor.Process(err), - Recommendation: processor.Recommend(err, cmd.CommandPath()), - } + return processor.Process(err, cmd.CommandPath()) } return cmd } diff --git a/cmd/oras/internal/option/remote.go b/cmd/oras/internal/option/remote.go index 5547569e5..e3a02f647 100644 --- a/cmd/oras/internal/option/remote.go +++ b/cmd/oras/internal/option/remote.go @@ -30,11 +30,14 @@ import ( credentials "github.com/oras-project/oras-credentials-go" "github.com/sirupsen/logrus" + "github.com/spf13/cobra" "github.com/spf13/pflag" "oras.land/oras-go/v2/errdef" "oras.land/oras-go/v2/registry/remote" "oras.land/oras-go/v2/registry/remote/auth" + "oras.land/oras-go/v2/registry/remote/errcode" "oras.land/oras-go/v2/registry/remote/retry" + oerrors "oras.land/oras/cmd/oras/internal/errors" "oras.land/oras/internal/credential" "oras.land/oras/internal/crypto" onet "oras.land/oras/internal/net" @@ -42,7 +45,8 @@ import ( "oras.land/oras/internal/version" ) -// Remote options struct. +// Remote options struct contains flags and arguments specifying one registry. +// Remote implements oerrors.Handler and oerrors.Processor interface. type Remote struct { DistributionSpec CACertFilePath string @@ -322,3 +326,32 @@ func (opts *Remote) isPlainHttp(registry string) bool { } return plainHTTP } + +// Handle handles error during cmd execution. +func (opts *Remote) Handle(err error, cmd *cobra.Command) (oerrors.Processor, error) { + // handle not found error from registry + if errors.Is(err, errdef.ErrNotFound) { + cmd.SetErrPrefix(oerrors.RegistryErrorPrefix) + return opts, err + } + + // handle error response + var errResp *errcode.ErrorResponse + if errors.As(err, &errResp) { + cmd.SetErrPrefix(oerrors.RegistryErrorPrefix) + return opts, errResp + } + return nil, err +} + +// Process processes error into oerrors.Error. +func (opts *Remote) Process(err error, _ string) *oerrors.Error { + ret := oerrors.Error{ + Err: err, + } + if errResp, ok := err.(*errcode.ErrorResponse); ok { + // remove HTTP related info + ret.Err = errResp.Errors + } + return &ret +} diff --git a/cmd/oras/internal/option/target.go b/cmd/oras/internal/option/target.go index cfa5b0bf5..2d66af5b7 100644 --- a/cmd/oras/internal/option/target.go +++ b/cmd/oras/internal/option/target.go @@ -32,7 +32,6 @@ import ( "oras.land/oras-go/v2" "oras.land/oras-go/v2/content" "oras.land/oras-go/v2/content/oci" - "oras.land/oras-go/v2/errdef" "oras.land/oras-go/v2/registry" "oras.land/oras-go/v2/registry/remote" "oras.land/oras-go/v2/registry/remote/errcode" @@ -47,7 +46,7 @@ const ( // Target struct contains flags and arguments specifying one registry or image // layout. -// Target implements errors.Handler and errors.Processor interface. +// Target implements oerrors.Handler and oerrors.Processor interface. type Target struct { Remote RawReference string @@ -251,49 +250,40 @@ func (opts *Target) Handle(err error, cmd *cobra.Command) (oerrors.Processor, er return nil, err } - // handle not found error from registry - if errors.Is(err, errdef.ErrNotFound) { - cmd.SetErrPrefix(oerrors.RegistryErrorPrefix) - return opts, err + processor, err := opts.Remote.Handle(err, cmd) + if processor != nil { + processor = opts } - - // handle error response - var errResp *errcode.ErrorResponse - if errors.As(err, &errResp) { - cmd.SetErrPrefix(oerrors.RegistryErrorPrefix) - return opts, errResp - } - return opts, err + return processor, err } -// Process returns a scrubbed error. -func (opts *Target) Process(err error) error { - if errResp, ok := err.(*errcode.ErrorResponse); ok { - // remove HTTP related info - return errResp.Errors +// Process processes error into oerrors.Error. +func (opts *Target) Process(err error, callPath string) *oerrors.Error { + ret := oerrors.Error{ + Err: err, } - return err -} - -// Recommend returns a recommendation for known errors. -func (opts *Target) Recommend(err error, callPath string) string { if opts.IsOCILayout { - return "" - } - ref, parseErr := registry.ParseReference(opts.RawReference) - if parseErr != nil { - // this should not happen - return "" + return &ret } + if errResp, ok := err.(*errcode.ErrorResponse); ok { + // remove HTTP related info + ret.Err = errResp.Errors - if respErr, ok := err.(*errcode.ErrorResponse); ok && ref.Registry == "docker.io" && respErr.URL.Host == ref.Host() && respErr.StatusCode == http.StatusUnauthorized { - if !strings.Contains(ref.Repository, "/") { - // docker.io/xxx -> docker.io/library/xxx - ref.Repository = "library/" + ref.Repository - return fmt.Sprintf("Namespace is missing, do you mean `%s %s`?", callPath, ref) + ref, parseErr := registry.ParseReference(opts.RawReference) + if parseErr != nil { + // this should not happen + return &ret + } + + if ref.Registry == "docker.io" && errResp.URL.Host == ref.Host() && errResp.StatusCode == http.StatusUnauthorized { + if !strings.Contains(ref.Repository, "/") { + // docker.io/xxx -> docker.io/library/xxx + ref.Repository = "library/" + ref.Repository + ret.Recommendation = fmt.Sprintf("Namespace is missing, do you mean `%s %s`?", callPath, ref) + } } } - return "" + return &ret } // BinaryTarget struct contains flags and arguments specifying two registries or diff --git a/cmd/oras/root/login.go b/cmd/oras/root/login.go index 4eca66986..07e3f43e0 100644 --- a/cmd/oras/root/login.go +++ b/cmd/oras/root/login.go @@ -73,7 +73,7 @@ Example - Log in with username and password in an interactive terminal and no TL }, } option.ApplyFlags(&opts, cmd.Flags()) - return cmd + return oerrors.Command(cmd, &opts.Remote) } func runLogin(ctx context.Context, opts loginOptions) (err error) { diff --git a/cmd/oras/root/repo/ls.go b/cmd/oras/root/repo/ls.go index ea64b1968..84ffe2e06 100644 --- a/cmd/oras/root/repo/ls.go +++ b/cmd/oras/root/repo/ls.go @@ -68,7 +68,7 @@ Example - List the repositories under the registry that include values lexically cmd.Flags().StringVar(&opts.last, "last", "", "start after the repository specified by `last`") option.ApplyFlags(&opts, cmd.Flags()) - return cmd + return oerrors.Command(cmd, &opts.Remote) } func listRepository(ctx context.Context, opts repositoryOptions) error { From 11cb9cfa722ac952c0c63fceed1a831dc0d1c84d Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Fri, 29 Dec 2023 09:24:29 +0000 Subject: [PATCH 05/42] add e2e test Signed-off-by: Billy Zha --- test/e2e/suite/auth/auth.go | 8 ++++++++ test/e2e/suite/command/repo.go | 5 +++++ 2 files changed, 13 insertions(+) diff --git a/test/e2e/suite/auth/auth.go b/test/e2e/suite/auth/auth.go index ada87713f..8ef13e1a8 100644 --- a/test/e2e/suite/auth/auth.go +++ b/test/e2e/suite/auth/auth.go @@ -90,6 +90,14 @@ var _ = Describe("Common registry user", func() { WithInput(strings.NewReader(fmt.Sprintf("%s\n\n", Username))).ExpectFailure().Exec() }) + It("should fail if password is wrong with registry error prefix", func() { + ORAS("login", ZOTHost, "--registry-config", filepath.Join(GinkgoT().TempDir(), tmpConfigName)). + WithTimeOut(20*time.Second). + MatchKeyWords("Username: ", "Password: "). + MatchErrKeyWords("Error response from registry: "). + WithInput(strings.NewReader(fmt.Sprintf("%s\n???\n", Username))).ExpectFailure().Exec() + }) + It("should fail if no token input", func() { ORAS("login", ZOTHost, "--registry-config", filepath.Join(GinkgoT().TempDir(), tmpConfigName)). WithTimeOut(20*time.Second). diff --git a/test/e2e/suite/command/repo.go b/test/e2e/suite/command/repo.go index 0aab9b2bc..ea263f9ff 100644 --- a/test/e2e/suite/command/repo.go +++ b/test/e2e/suite/command/repo.go @@ -52,6 +52,11 @@ var _ = Describe("ORAS beginners:", func() { gomega.Expect(err).Should(gbytes.Say("\n")) gomega.Expect(err).Should(gbytes.Say(`Run "oras repo ls -h"`)) }) + + It("should fail if password is wrong with registry error prefix", func() { + ORAS("repo", "ls", ZOTHost, "-u", Username, "-p", "???"). + MatchErrKeyWords("Error response from registry: ").ExpectFailure().Exec() + }) }) When("running `repo tags`", func() { It("should show help description with feature flags", func() { From dbdaa590efb264f252de02a119a8fd2cd4eff57b Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Fri, 29 Dec 2023 09:25:33 +0000 Subject: [PATCH 06/42] code clean Signed-off-by: Billy Zha --- cmd/oras/internal/errors/errors.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/oras/internal/errors/errors.go b/cmd/oras/internal/errors/errors.go index fe0f2164d..b9d743508 100644 --- a/cmd/oras/internal/errors/errors.go +++ b/cmd/oras/internal/errors/errors.go @@ -75,7 +75,7 @@ type Processor interface { Process(err error, callPath string) *Error } -// Command returns an error-handled for cobra command. +// Command returns an error-handled cobra command. func Command(cmd *cobra.Command, handler Handler) *cobra.Command { runE := cmd.RunE cmd.RunE = func(cmd *cobra.Command, args []string) error { From c19a844d9d8366dae860719154d3222dffda4df1 Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Fri, 29 Dec 2023 09:41:46 +0000 Subject: [PATCH 07/42] add error test for pull Signed-off-by: Billy Zha --- test/e2e/suite/command/pull.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/test/e2e/suite/command/pull.go b/test/e2e/suite/command/pull.go index c841820a9..e8936838a 100644 --- a/test/e2e/suite/command/pull.go +++ b/test/e2e/suite/command/pull.go @@ -74,6 +74,22 @@ var _ = Describe("ORAS beginners:", func() { gomega.Expect(err).Should(gbytes.Say("\n")) gomega.Expect(err).Should(gbytes.Say(`Run "oras pull -h"`)) }) + + It("should fail if password is wrong with registry error prefix", func() { + ORAS("pull", RegistryRef(ZOTHost, ArtifactRepo, empty.Tag), "-u", Username, "-p", "???"). + MatchErrKeyWords("Error response from registry: ").ExpectFailure().Exec() + }) + + It("should fail if artifact is not found with registry error prefix", func() { + ORAS("pull", RegistryRef(ZOTHost, ArtifactRepo, "i-dont-think-this-tag-exists")). + MatchErrKeyWords("Error response from registry: ").ExpectFailure().Exec() + }) + + It("should fail if artifact is not found from OCI layout", func() { + root := PrepareTempOCI(ArtifactRepo) + ORAS("pull", Flags.Layout, LayoutRef(root, "i-dont-think-this-tag-exists")). + MatchErrKeyWords("Error: ").ExpectFailure().Exec() + }) }) }) From 354df3e5e0cf3064b8bd24542f1adb3651ace2f7 Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Tue, 2 Jan 2024 03:00:47 +0000 Subject: [PATCH 08/42] add unit test Signed-off-by: Billy Zha --- cmd/oras/internal/option/target.go | 2 +- cmd/oras/internal/option/target_test.go | 117 ++++++++++++++++++++++++ 2 files changed, 118 insertions(+), 1 deletion(-) diff --git a/cmd/oras/internal/option/target.go b/cmd/oras/internal/option/target.go index 2d66af5b7..9677ed5c6 100644 --- a/cmd/oras/internal/option/target.go +++ b/cmd/oras/internal/option/target.go @@ -276,7 +276,7 @@ func (opts *Target) Process(err error, callPath string) *oerrors.Error { } if ref.Registry == "docker.io" && errResp.URL.Host == ref.Host() && errResp.StatusCode == http.StatusUnauthorized { - if !strings.Contains(ref.Repository, "/") { + if ref.Repository != "" && !strings.Contains(ref.Repository, "/") { // docker.io/xxx -> docker.io/library/xxx ref.Repository = "library/" + ref.Repository ret.Recommendation = fmt.Sprintf("Namespace is missing, do you mean `%s %s`?", callPath, ref) diff --git a/cmd/oras/internal/option/target_test.go b/cmd/oras/internal/option/target_test.go index fd13221ae..f8271c64e 100644 --- a/cmd/oras/internal/option/target_test.go +++ b/cmd/oras/internal/option/target_test.go @@ -16,7 +16,14 @@ limitations under the License. package option import ( + "errors" + "net/http" + "net/url" + "reflect" "testing" + + "oras.land/oras-go/v2/registry/remote/errcode" + oerrors "oras.land/oras/cmd/oras/internal/errors" ) func TestTarget_Parse_oci(t *testing.T) { @@ -75,3 +82,113 @@ func Test_parseOCILayoutReference(t *testing.T) { }) } } + +func TestTarget_Process_ociLayout(t *testing.T) { + errClient := errors.New("client error") + opts := &Target{ + IsOCILayout: true, + } + if got := opts.Process(errClient, ""); got.Err != errClient || got.Recommendation != "" { + t.Errorf("unexpected output from Target.Process() = %v", got) + } +} + +func TestTarget_Process_hint(t *testing.T) { + type fields struct { + Remote Remote + RawReference string + Type string + Reference string + Path string + IsOCILayout bool + } + type args struct { + err error + callPath string + } + errs := errcode.Errors{ + errcode.Error{ + Code: "000", + Message: "mocked message", + Detail: map[string]string{"mocked key": "mocked value"}, + }, + } + tests := []struct { + name string + fields fields + args args + want *oerrors.Error + }{ + { + "namespace already exists", + fields{RawReference: "docker.io/library/alpine:latest"}, + args{ + err: &errcode.ErrorResponse{ + URL: &url.URL{Host: "registry-1.docker.io"}, + StatusCode: http.StatusUnauthorized, + Errors: errs, + }, + }, + &oerrors.Error{Err: errs}, + }, + { + "no namespace", + fields{RawReference: "docker.io"}, + args{ + err: &errcode.ErrorResponse{ + URL: &url.URL{Host: "registry-1.docker.io"}, + StatusCode: http.StatusUnauthorized, + Errors: errs, + }, + }, + &oerrors.Error{Err: errs}, + }, + { + "not 401", + fields{RawReference: "docker.io"}, + args{ + err: &errcode.ErrorResponse{ + URL: &url.URL{Host: "registry-1.docker.io"}, + StatusCode: http.StatusConflict, + Errors: errs, + }, + }, + &oerrors.Error{Err: errs}, + }, + { + "should hint", + fields{ + RawReference: "docker.io/alpine", + Path: "oras test", + }, + args{ + err: &errcode.ErrorResponse{ + URL: &url.URL{Host: "registry-1.docker.io"}, + StatusCode: http.StatusUnauthorized, + Errors: errs, + }, + callPath: "oras test", + }, + &oerrors.Error{ + Err: errs, + Recommendation: "Namespace is missing, do you mean `oras test docker.io/library/alpine`?", + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + opts := &Target{ + Remote: tt.fields.Remote, + RawReference: tt.fields.RawReference, + Type: tt.fields.Type, + Reference: tt.fields.Reference, + Path: tt.fields.Path, + IsOCILayout: tt.fields.IsOCILayout, + } + got := opts.Process(tt.args.err, tt.args.callPath) + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("Target.Process() = %v, want %v", got, tt.want) + } + }) + } +} From 20e23e3c9d20c05455539a341b86b41551967531 Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Tue, 2 Jan 2024 07:04:05 +0000 Subject: [PATCH 09/42] support error prefix in cp and resolve Signed-off-by: Billy Zha --- cmd/oras/internal/option/target.go | 6 +++- cmd/oras/root/cp.go | 2 +- cmd/oras/root/resolve.go | 2 +- .../e2e/internal/utils/{flags.go => const.go} | 30 +++++++++++-------- test/e2e/suite/auth/auth.go | 2 +- test/e2e/suite/command/cp.go | 15 +++++++++- test/e2e/suite/command/pull.go | 8 ++--- test/e2e/suite/command/repo.go | 2 +- test/e2e/suite/command/resolve.go | 2 +- test/e2e/suite/command/tag.go | 2 +- 10 files changed, 46 insertions(+), 25 deletions(-) rename test/e2e/internal/utils/{flags.go => const.go} (60%) diff --git a/cmd/oras/internal/option/target.go b/cmd/oras/internal/option/target.go index 9677ed5c6..d3e6a2f71 100644 --- a/cmd/oras/internal/option/target.go +++ b/cmd/oras/internal/option/target.go @@ -321,7 +321,11 @@ func (opts *BinaryTarget) Parse() error { // Handle handles error during cmd execution. func (opts *BinaryTarget) Handle(err error, cmd *cobra.Command) (oerrors.Processor, error) { if processor, err := opts.From.Handle(err, cmd); processor != nil { - return processor, err + if errResp, ok := err.(*errcode.ErrorResponse); ok { + if ref, _ := registry.ParseReference(opts.From.RawReference); errResp.URL.Host == ref.Host() { + return processor, err + } + } } return opts.To.Handle(err, cmd) } diff --git a/cmd/oras/root/cp.go b/cmd/oras/root/cp.go index 28d61f275..94eb52044 100644 --- a/cmd/oras/root/cp.go +++ b/cmd/oras/root/cp.go @@ -102,7 +102,7 @@ Example - Copy an artifact with multiple tags with concurrency tuned: cmd.Flags().IntVarP(&opts.concurrency, "concurrency", "", 3, "concurrency level") opts.EnableDistributionSpecFlag() option.ApplyFlags(&opts, cmd.Flags()) - return cmd + return oerrors.Command(cmd, &opts.BinaryTarget) } func runCopy(ctx context.Context, opts copyOptions) error { diff --git a/cmd/oras/root/resolve.go b/cmd/oras/root/resolve.go index 976fac3cc..c1d533d34 100644 --- a/cmd/oras/root/resolve.go +++ b/cmd/oras/root/resolve.go @@ -58,7 +58,7 @@ Example - Resolve digest of the target artifact: cmd.Flags().BoolVarP(&opts.fullRef, "full-reference", "l", false, "print the full artifact reference with digest") option.ApplyFlags(&opts, cmd.Flags()) - return cmd + return oerrors.Command(cmd, &opts.Target) } func runResolve(ctx context.Context, opts resolveOptions) error { diff --git a/test/e2e/internal/utils/flags.go b/test/e2e/internal/utils/const.go similarity index 60% rename from test/e2e/internal/utils/flags.go rename to test/e2e/internal/utils/const.go index 893fa575d..63b0fadea 100644 --- a/test/e2e/internal/utils/flags.go +++ b/test/e2e/internal/utils/const.go @@ -15,16 +15,20 @@ limitations under the License. package utils -var Flags = struct { - Layout string - FromLayout string - ToLayout string - DistributionSpec string - ImageSpec string -}{ - "--oci-layout", - "--from-oci-layout", - "--to-oci-layout", - "--distribution-spec", - "--image-spec", -} +var ( + Flags = struct { + Layout string + FromLayout string + ToLayout string + DistributionSpec string + ImageSpec string + }{ + "--oci-layout", + "--from-oci-layout", + "--to-oci-layout", + "--distribution-spec", + "--image-spec", + } + RegistryErrorPrefix = "Error response from registry: " + InvalidTag = "i-dont-think-this-tag-exists" +) diff --git a/test/e2e/suite/auth/auth.go b/test/e2e/suite/auth/auth.go index 8ef13e1a8..9c0988c2a 100644 --- a/test/e2e/suite/auth/auth.go +++ b/test/e2e/suite/auth/auth.go @@ -94,7 +94,7 @@ var _ = Describe("Common registry user", func() { ORAS("login", ZOTHost, "--registry-config", filepath.Join(GinkgoT().TempDir(), tmpConfigName)). WithTimeOut(20*time.Second). MatchKeyWords("Username: ", "Password: "). - MatchErrKeyWords("Error response from registry: "). + MatchErrKeyWords(RegistryErrorPrefix). WithInput(strings.NewReader(fmt.Sprintf("%s\n???\n", Username))).ExpectFailure().Exec() }) diff --git a/test/e2e/suite/command/cp.go b/test/e2e/suite/command/cp.go index 45718fa29..85420360c 100644 --- a/test/e2e/suite/command/cp.go +++ b/test/e2e/suite/command/cp.go @@ -57,7 +57,7 @@ var _ = Describe("ORAS beginners:", func() { }) It("should fail when source doesn't exist", func() { - ORAS("cp", RegistryRef(ZOTHost, ImageRepo, "i-dont-think-this-tag-exists"), RegistryRef(ZOTHost, cpTestRepo("nonexistent-source"), "")).ExpectFailure().MatchErrKeyWords("Error:").Exec() + ORAS("cp", RegistryRef(ZOTHost, ImageRepo, InvalidTag), RegistryRef(ZOTHost, cpTestRepo("nonexistent-source"), "")).ExpectFailure().MatchErrKeyWords("Error:").Exec() }) It("should fail and show detailed error description if no argument provided", func() { @@ -75,6 +75,19 @@ var _ = Describe("ORAS beginners:", func() { Expect(err).Should(gbytes.Say("\n")) Expect(err).Should(gbytes.Say(`Run "oras cp -h"`)) }) + + It("should fail and show registry error prefix if source not found", func() { + src := RegistryRef(ZOTHost, ArtifactRepo, InvalidTag) + dst := GinkgoT().TempDir() + ORAS("cp", src, Flags.ToLayout, dst).MatchErrKeyWords(RegistryErrorPrefix).ExpectFailure().Exec() + }) + + It("should fail and show registry error prefix if target registry is not logged in", Focus, func() { + src := RegistryRef(ZOTHost, ArtifactRepo, foobar.Tag) + dst := RegistryRef(ZOTHost, cpTestRepo("dest-not-logged-in"), "") + ORAS("cp", src, dst, "--to-username", Username, "--to-password", Password+"?"). + MatchErrKeyWords(RegistryErrorPrefix).ExpectFailure().Exec() + }) }) }) diff --git a/test/e2e/suite/command/pull.go b/test/e2e/suite/command/pull.go index e8936838a..ebd5cf61b 100644 --- a/test/e2e/suite/command/pull.go +++ b/test/e2e/suite/command/pull.go @@ -77,17 +77,17 @@ var _ = Describe("ORAS beginners:", func() { It("should fail if password is wrong with registry error prefix", func() { ORAS("pull", RegistryRef(ZOTHost, ArtifactRepo, empty.Tag), "-u", Username, "-p", "???"). - MatchErrKeyWords("Error response from registry: ").ExpectFailure().Exec() + MatchErrKeyWords(RegistryErrorPrefix).ExpectFailure().Exec() }) It("should fail if artifact is not found with registry error prefix", func() { - ORAS("pull", RegistryRef(ZOTHost, ArtifactRepo, "i-dont-think-this-tag-exists")). - MatchErrKeyWords("Error response from registry: ").ExpectFailure().Exec() + ORAS("pull", RegistryRef(ZOTHost, ArtifactRepo, InvalidTag)). + MatchErrKeyWords(RegistryErrorPrefix).ExpectFailure().Exec() }) It("should fail if artifact is not found from OCI layout", func() { root := PrepareTempOCI(ArtifactRepo) - ORAS("pull", Flags.Layout, LayoutRef(root, "i-dont-think-this-tag-exists")). + ORAS("pull", Flags.Layout, LayoutRef(root, InvalidTag)). MatchErrKeyWords("Error: ").ExpectFailure().Exec() }) }) diff --git a/test/e2e/suite/command/repo.go b/test/e2e/suite/command/repo.go index ea263f9ff..525884ca3 100644 --- a/test/e2e/suite/command/repo.go +++ b/test/e2e/suite/command/repo.go @@ -55,7 +55,7 @@ var _ = Describe("ORAS beginners:", func() { It("should fail if password is wrong with registry error prefix", func() { ORAS("repo", "ls", ZOTHost, "-u", Username, "-p", "???"). - MatchErrKeyWords("Error response from registry: ").ExpectFailure().Exec() + MatchErrKeyWords(RegistryErrorPrefix).ExpectFailure().Exec() }) }) When("running `repo tags`", func() { diff --git a/test/e2e/suite/command/resolve.go b/test/e2e/suite/command/resolve.go index fac8266b4..1344cf48f 100644 --- a/test/e2e/suite/command/resolve.go +++ b/test/e2e/suite/command/resolve.go @@ -47,7 +47,7 @@ var _ = Describe("ORAS beginners:", func() { ORAS("resolve", RegistryRef(ZOTHost, ImageRepo, "")).ExpectFailure().MatchErrKeyWords("Error:", "no tag or digest when expecting ").Exec() }) It("should fail when provided manifest reference is not found", func() { - ORAS("resolve", RegistryRef(ZOTHost, ImageRepo, "i-dont-think-this-tag-exists")).ExpectFailure().MatchErrKeyWords("Error: failed to resolve digest:", "not found").Exec() + ORAS("resolve", RegistryRef(ZOTHost, ImageRepo, InvalidTag)).ExpectFailure().MatchErrKeyWords("Error: failed to resolve digest:", "not found").Exec() }) It("should fail and show detailed error description if no argument provided", func() { diff --git a/test/e2e/suite/command/tag.go b/test/e2e/suite/command/tag.go index 3c98dcea3..2fd556b9e 100644 --- a/test/e2e/suite/command/tag.go +++ b/test/e2e/suite/command/tag.go @@ -33,7 +33,7 @@ var _ = Describe("ORAS beginners:", func() { }) It("should fail when provided manifest reference is not found", func() { - ORAS("tag", RegistryRef(ZOTHost, ImageRepo, "i-dont-think-this-tag-exists"), "tagged").ExpectFailure().MatchErrKeyWords("Error:").Exec() + ORAS("tag", RegistryRef(ZOTHost, ImageRepo, InvalidTag), "tagged").ExpectFailure().MatchErrKeyWords("Error:").Exec() }) It("should fail and show detailed error description if no argument provided", func() { From 859441182626409fc2bb88594e32494e1b74ca2b Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Tue, 2 Jan 2024 07:05:17 +0000 Subject: [PATCH 10/42] support push Signed-off-by: Billy Zha --- cmd/oras/root/push.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/oras/root/push.go b/cmd/oras/root/push.go index 0ee581a5f..c4825de09 100644 --- a/cmd/oras/root/push.go +++ b/cmd/oras/root/push.go @@ -132,7 +132,7 @@ Example - Push file "hi.txt" into an OCI image layout folder 'layout-dir' with t cmd.Flags().IntVarP(&opts.concurrency, "concurrency", "", 5, "concurrency level") option.ApplyFlags(&opts, cmd.Flags()) - return cmd + return oerrors.Command(cmd, &opts.Target) } func runPush(ctx context.Context, opts pushOptions) error { From 6d0a53341e90f980850b51fba895444dc0292246 Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Fri, 5 Jan 2024 09:04:44 +0000 Subject: [PATCH 11/42] fix e2e Signed-off-by: Billy Zha --- test/e2e/suite/command/cp.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e/suite/command/cp.go b/test/e2e/suite/command/cp.go index 85420360c..4640281b0 100644 --- a/test/e2e/suite/command/cp.go +++ b/test/e2e/suite/command/cp.go @@ -82,7 +82,7 @@ var _ = Describe("ORAS beginners:", func() { ORAS("cp", src, Flags.ToLayout, dst).MatchErrKeyWords(RegistryErrorPrefix).ExpectFailure().Exec() }) - It("should fail and show registry error prefix if target registry is not logged in", Focus, func() { + It("should fail and show registry error prefix if target registry is not logged in", func() { src := RegistryRef(ZOTHost, ArtifactRepo, foobar.Tag) dst := RegistryRef(ZOTHost, cpTestRepo("dest-not-logged-in"), "") ORAS("cp", src, dst, "--to-username", Username, "--to-password", Password+"?"). From 317556b47df853bdbec4c0e964e362329c2c0672 Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Fri, 5 Jan 2024 11:46:20 +0000 Subject: [PATCH 12/42] scrub debug info & handle empty response body Signed-off-by: Billy Zha --- cmd/oras/internal/errors/errors.go | 19 +++++++++++++++++++ cmd/oras/internal/option/remote.go | 10 ++++++---- cmd/oras/internal/option/target.go | 4 +++- 3 files changed, 28 insertions(+), 5 deletions(-) diff --git a/cmd/oras/internal/errors/errors.go b/cmd/oras/internal/errors/errors.go index b9d743508..cc9b566ce 100644 --- a/cmd/oras/internal/errors/errors.go +++ b/cmd/oras/internal/errors/errors.go @@ -17,9 +17,11 @@ package errors import ( "fmt" + "strings" "github.com/spf13/cobra" "oras.land/oras-go/v2/registry" + "oras.land/oras-go/v2/registry/remote/errcode" ) // RegistryErrorPrefix is the commandline prefix for errors from registry. @@ -92,6 +94,23 @@ func Command(cmd *cobra.Command, handler Handler) *cobra.Command { return cmd } +// GetInnerError gets the inner error from the error response. +// nil is returned if no valid inner error is found. +func GetInnerError(err error, errResp *errcode.ErrorResponse) error { + inner := errResp.Errors + if len(inner) == 0 { + return fmt.Errorf("empty response body: %w", errResp) + } else { + errContent := err.Error() + errRespContent := errResp.Error() + if idx := strings.Index(errContent, errRespContent); idx > 0 { + // remove HTTP related info + return fmt.Errorf("%s: %w", errContent[:idx], errResp) + } + } + return nil +} + // NewErrEmptyTagOrDigest creates a new error based on the reference string. func NewErrEmptyTagOrDigest(ref registry.Reference) error { return NewErrEmptyTagOrDigestStr(ref.String()) diff --git a/cmd/oras/internal/option/remote.go b/cmd/oras/internal/option/remote.go index e3a02f647..3814b0106 100644 --- a/cmd/oras/internal/option/remote.go +++ b/cmd/oras/internal/option/remote.go @@ -339,7 +339,7 @@ func (opts *Remote) Handle(err error, cmd *cobra.Command) (oerrors.Processor, er var errResp *errcode.ErrorResponse if errors.As(err, &errResp) { cmd.SetErrPrefix(oerrors.RegistryErrorPrefix) - return opts, errResp + return opts, err } return nil, err } @@ -349,9 +349,11 @@ func (opts *Remote) Process(err error, _ string) *oerrors.Error { ret := oerrors.Error{ Err: err, } - if errResp, ok := err.(*errcode.ErrorResponse); ok { - // remove HTTP related info - ret.Err = errResp.Errors + var errResp *errcode.ErrorResponse + if errors.As(err, &errResp) { + if innerErr := oerrors.GetInnerError(err, errResp); innerErr != nil { + ret.Err = innerErr + } } return &ret } diff --git a/cmd/oras/internal/option/target.go b/cmd/oras/internal/option/target.go index d3e6a2f71..7908c064a 100644 --- a/cmd/oras/internal/option/target.go +++ b/cmd/oras/internal/option/target.go @@ -267,7 +267,9 @@ func (opts *Target) Process(err error, callPath string) *oerrors.Error { } if errResp, ok := err.(*errcode.ErrorResponse); ok { // remove HTTP related info - ret.Err = errResp.Errors + if innerErr := oerrors.GetInnerError(err, errResp); innerErr != nil { + ret.Err = innerErr + } ref, parseErr := registry.ParseReference(opts.RawReference) if parseErr != nil { From 40766353f2f42e386e16aeb6c2f5bb69440d370f Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Fri, 5 Jan 2024 12:53:40 +0000 Subject: [PATCH 13/42] fix e2e Signed-off-by: Billy Zha --- test/e2e/suite/command/cp.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e/suite/command/cp.go b/test/e2e/suite/command/cp.go index 4640281b0..ba09d4bef 100644 --- a/test/e2e/suite/command/cp.go +++ b/test/e2e/suite/command/cp.go @@ -57,7 +57,7 @@ var _ = Describe("ORAS beginners:", func() { }) It("should fail when source doesn't exist", func() { - ORAS("cp", RegistryRef(ZOTHost, ImageRepo, InvalidTag), RegistryRef(ZOTHost, cpTestRepo("nonexistent-source"), "")).ExpectFailure().MatchErrKeyWords("Error:").Exec() + ORAS("cp", RegistryRef(ZOTHost, ImageRepo, InvalidTag), RegistryRef(ZOTHost, cpTestRepo("nonexistent-source"), "")).ExpectFailure().MatchErrKeyWords(InvalidTag).Exec() }) It("should fail and show detailed error description if no argument provided", func() { From 1d3fefa213bbb2158adda5fdbf1915468baebd58 Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Fri, 5 Jan 2024 13:23:18 +0000 Subject: [PATCH 14/42] fix e2e Signed-off-by: Billy Zha --- test/e2e/suite/command/resolve.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e/suite/command/resolve.go b/test/e2e/suite/command/resolve.go index 1344cf48f..aa6e009db 100644 --- a/test/e2e/suite/command/resolve.go +++ b/test/e2e/suite/command/resolve.go @@ -47,7 +47,7 @@ var _ = Describe("ORAS beginners:", func() { ORAS("resolve", RegistryRef(ZOTHost, ImageRepo, "")).ExpectFailure().MatchErrKeyWords("Error:", "no tag or digest when expecting ").Exec() }) It("should fail when provided manifest reference is not found", func() { - ORAS("resolve", RegistryRef(ZOTHost, ImageRepo, InvalidTag)).ExpectFailure().MatchErrKeyWords("Error: failed to resolve digest:", "not found").Exec() + ORAS("resolve", RegistryRef(ZOTHost, ImageRepo, InvalidTag)).ExpectFailure().MatchErrKeyWords(RegistryErrorPrefix, "failed to resolve digest:", "not found").Exec() }) It("should fail and show detailed error description if no argument provided", func() { From 1c01b0aa2dc6c259c96d83ebe34cbf56d596e9eb Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Sun, 7 Jan 2024 09:47:27 +0000 Subject: [PATCH 15/42] fix e2e Signed-off-by: Billy Zha --- cmd/oras/internal/option/target.go | 4 +++- test/e2e/internal/utils/const.go | 1 + test/e2e/suite/command/resolve.go | 8 +++++++- 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/cmd/oras/internal/option/target.go b/cmd/oras/internal/option/target.go index 7908c064a..f39866ef9 100644 --- a/cmd/oras/internal/option/target.go +++ b/cmd/oras/internal/option/target.go @@ -265,7 +265,9 @@ func (opts *Target) Process(err error, callPath string) *oerrors.Error { if opts.IsOCILayout { return &ret } - if errResp, ok := err.(*errcode.ErrorResponse); ok { + + var errResp *errcode.ErrorResponse + if errors.As(err, &errResp) { // remove HTTP related info if innerErr := oerrors.GetInnerError(err, errResp); innerErr != nil { ret.Err = innerErr diff --git a/test/e2e/internal/utils/const.go b/test/e2e/internal/utils/const.go index 63b0fadea..f13d3a4de 100644 --- a/test/e2e/internal/utils/const.go +++ b/test/e2e/internal/utils/const.go @@ -30,5 +30,6 @@ var ( "--image-spec", } RegistryErrorPrefix = "Error response from registry: " + EmptyBodyPrefix = "empty response body: " InvalidTag = "i-dont-think-this-tag-exists" ) diff --git a/test/e2e/suite/command/resolve.go b/test/e2e/suite/command/resolve.go index aa6e009db..52f85c64d 100644 --- a/test/e2e/suite/command/resolve.go +++ b/test/e2e/suite/command/resolve.go @@ -47,7 +47,13 @@ var _ = Describe("ORAS beginners:", func() { ORAS("resolve", RegistryRef(ZOTHost, ImageRepo, "")).ExpectFailure().MatchErrKeyWords("Error:", "no tag or digest when expecting ").Exec() }) It("should fail when provided manifest reference is not found", func() { - ORAS("resolve", RegistryRef(ZOTHost, ImageRepo, InvalidTag)).ExpectFailure().MatchErrKeyWords(RegistryErrorPrefix, "failed to resolve digest:", "not found").Exec() + ORAS("resolve", RegistryRef(ZOTHost, ImageRepo, "i-dont-think-this-tag-exists")).ExpectFailure().MatchErrKeyWords("Error: failed to resolve digest:", "not found").Exec() + }) + It("should fail with empty response when returned response doesn't have body", func() { + ORAS("resolve", RegistryRef(ZOTHost, ImageRepo, InvalidTag), "-u", Username, "-p", Password+"1"). + ExpectFailure(). + MatchErrKeyWords(RegistryErrorPrefix, EmptyBodyPrefix, "response status code 401: Unauthorized"). + Exec() }) It("should fail and show detailed error description if no argument provided", func() { From 937d80c5e10e38e3b78fff0ac633fa1fff9a0c4a Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Sun, 7 Jan 2024 10:36:03 +0000 Subject: [PATCH 16/42] fix e2e Signed-off-by: Billy Zha --- test/e2e/suite/command/resolve.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e/suite/command/resolve.go b/test/e2e/suite/command/resolve.go index 52f85c64d..9a345f11e 100644 --- a/test/e2e/suite/command/resolve.go +++ b/test/e2e/suite/command/resolve.go @@ -47,7 +47,7 @@ var _ = Describe("ORAS beginners:", func() { ORAS("resolve", RegistryRef(ZOTHost, ImageRepo, "")).ExpectFailure().MatchErrKeyWords("Error:", "no tag or digest when expecting ").Exec() }) It("should fail when provided manifest reference is not found", func() { - ORAS("resolve", RegistryRef(ZOTHost, ImageRepo, "i-dont-think-this-tag-exists")).ExpectFailure().MatchErrKeyWords("Error: failed to resolve digest:", "not found").Exec() + ORAS("resolve", RegistryRef(ZOTHost, ImageRepo, "i-dont-think-this-tag-exists")).ExpectFailure().MatchErrKeyWords(RegistryErrorPrefix, "failed to resolve digest:", "not found").Exec() }) It("should fail with empty response when returned response doesn't have body", func() { ORAS("resolve", RegistryRef(ZOTHost, ImageRepo, InvalidTag), "-u", Username, "-p", Password+"1"). From ae3dd7b8ba087da9178a682f25df049663914f8a Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Mon, 8 Jan 2024 02:08:21 +0000 Subject: [PATCH 17/42] support all commands and add e2e Signed-off-by: Billy Zha --- cmd/oras/root/attach.go | 2 +- cmd/oras/root/blob/delete.go | 2 +- cmd/oras/root/blob/fetch.go | 2 +- cmd/oras/root/blob/push.go | 2 +- cmd/oras/root/discover.go | 2 +- cmd/oras/root/manifest/delete.go | 2 +- cmd/oras/root/manifest/fetch.go | 2 +- cmd/oras/root/manifest/fetch_config.go | 2 +- cmd/oras/root/manifest/push.go | 2 +- cmd/oras/root/repo/tags.go | 2 +- cmd/oras/root/tag.go | 2 +- test/e2e/suite/auth/auth.go | 23 +++++++++++++++++++++++ 12 files changed, 34 insertions(+), 11 deletions(-) diff --git a/cmd/oras/root/attach.go b/cmd/oras/root/attach.go index 528a01b47..f16d0fc4d 100644 --- a/cmd/oras/root/attach.go +++ b/cmd/oras/root/attach.go @@ -96,7 +96,7 @@ Example - Attach file to the manifest tagged 'v1' in an OCI image layout folder _ = cmd.MarkFlagRequired("artifact-type") opts.EnableDistributionSpecFlag() option.ApplyFlags(&opts, cmd.Flags()) - return cmd + return oerrors.Command(cmd, &opts.Target) } func runAttach(ctx context.Context, opts attachOptions) error { diff --git a/cmd/oras/root/blob/delete.go b/cmd/oras/root/blob/delete.go index fe22a1389..866aaf40f 100644 --- a/cmd/oras/root/blob/delete.go +++ b/cmd/oras/root/blob/delete.go @@ -69,7 +69,7 @@ Example - Delete a blob and print its descriptor: } option.ApplyFlags(&opts, cmd.Flags()) - return cmd + return oerrors.Command(cmd, &opts.Target) } func deleteBlob(ctx context.Context, opts deleteBlobOptions) (err error) { diff --git a/cmd/oras/root/blob/fetch.go b/cmd/oras/root/blob/fetch.go index 3ebb4187f..fdfdb7abc 100644 --- a/cmd/oras/root/blob/fetch.go +++ b/cmd/oras/root/blob/fetch.go @@ -89,7 +89,7 @@ Example - Fetch and print a blob from OCI image layout archive file 'layout.tar' cmd.Flags().StringVarP(&opts.outputPath, "output", "o", "", "output file `path`, use - for stdout") option.ApplyFlags(&opts, cmd.Flags()) - return cmd + return oerrors.Command(cmd, &opts.Target) } func fetchBlob(ctx context.Context, opts fetchBlobOptions) (fetchErr error) { diff --git a/cmd/oras/root/blob/push.go b/cmd/oras/root/blob/push.go index 801874048..1f892a6c2 100644 --- a/cmd/oras/root/blob/push.go +++ b/cmd/oras/root/blob/push.go @@ -97,7 +97,7 @@ Example - Push blob 'hi.txt' into an OCI image layout folder 'layout-dir': cmd.Flags().Int64VarP(&opts.size, "size", "", -1, "provide the blob size") cmd.Flags().StringVarP(&opts.mediaType, "media-type", "", ocispec.MediaTypeImageLayer, "specify the returned media type in the descriptor if --descriptor is used") option.ApplyFlags(&opts, cmd.Flags()) - return cmd + return oerrors.Command(cmd, &opts.Target) } func pushBlob(ctx context.Context, opts pushBlobOptions) (err error) { diff --git a/cmd/oras/root/discover.go b/cmd/oras/root/discover.go index 6b98d4a22..a35db60a2 100644 --- a/cmd/oras/root/discover.go +++ b/cmd/oras/root/discover.go @@ -89,7 +89,7 @@ Example - Discover referrers of the manifest tagged 'v1' in an OCI image layout cmd.Flags().StringVarP(&opts.outputType, "output", "o", "table", "format in which to display referrers (table, json, or tree). tree format will also show indirect referrers") opts.EnableDistributionSpecFlag() option.ApplyFlags(&opts, cmd.Flags()) - return cmd + return oerrors.Command(cmd, &opts.Target) } func runDiscover(ctx context.Context, opts discoverOptions) error { diff --git a/cmd/oras/root/manifest/delete.go b/cmd/oras/root/manifest/delete.go index b8e43f0c6..fa4cf75fe 100644 --- a/cmd/oras/root/manifest/delete.go +++ b/cmd/oras/root/manifest/delete.go @@ -73,7 +73,7 @@ Example - Delete a manifest by digest 'sha256:99e4703fbf30916f549cd6bfa9cdbab614 opts.EnableDistributionSpecFlag() option.ApplyFlags(&opts, cmd.Flags()) - return cmd + return oerrors.Command(cmd, &opts.Target) } func deleteManifest(ctx context.Context, opts deleteOptions) error { diff --git a/cmd/oras/root/manifest/fetch.go b/cmd/oras/root/manifest/fetch.go index dad6917c1..4862891b8 100644 --- a/cmd/oras/root/manifest/fetch.go +++ b/cmd/oras/root/manifest/fetch.go @@ -88,7 +88,7 @@ Example - Fetch raw manifest from an OCI layout archive file 'layout.tar': cmd.Flags().StringSliceVarP(&opts.mediaTypes, "media-type", "", nil, "accepted media types") cmd.Flags().StringVarP(&opts.outputPath, "output", "o", "", "file `path` to write the fetched manifest to, use - for stdout") option.ApplyFlags(&opts, cmd.Flags()) - return cmd + return oerrors.Command(cmd, &opts.Target) } func fetchManifest(ctx context.Context, opts fetchOptions) (fetchErr error) { diff --git a/cmd/oras/root/manifest/fetch_config.go b/cmd/oras/root/manifest/fetch_config.go index 2ce4a8e6b..2519e491b 100644 --- a/cmd/oras/root/manifest/fetch_config.go +++ b/cmd/oras/root/manifest/fetch_config.go @@ -84,7 +84,7 @@ Example - Fetch and print the prettified descriptor of the config: cmd.Flags().StringVarP(&opts.outputPath, "output", "o", "", "file `path` to write the fetched config to, use - for stdout") option.ApplyFlags(&opts, cmd.Flags()) - return cmd + return oerrors.Command(cmd, &opts.Target) } func fetchConfig(ctx context.Context, opts fetchConfigOptions) (fetchErr error) { diff --git a/cmd/oras/root/manifest/push.go b/cmd/oras/root/manifest/push.go index d001ffd2f..2043b3625 100644 --- a/cmd/oras/root/manifest/push.go +++ b/cmd/oras/root/manifest/push.go @@ -102,7 +102,7 @@ Example - Push a manifest to an OCI image layout folder 'layout-dir' and tag wit option.ApplyFlags(&opts, cmd.Flags()) cmd.Flags().StringVarP(&opts.mediaType, "media-type", "", "", "media type of manifest") cmd.Flags().IntVarP(&opts.concurrency, "concurrency", "", 5, "concurrency level") - return cmd + return oerrors.Command(cmd, &opts.Target) } func pushManifest(ctx context.Context, opts pushOptions) error { diff --git a/cmd/oras/root/repo/tags.go b/cmd/oras/root/repo/tags.go index 6eddc504b..eacb87594 100644 --- a/cmd/oras/root/repo/tags.go +++ b/cmd/oras/root/repo/tags.go @@ -76,7 +76,7 @@ Example - [Experimental] Show tags associated with a digest: cmd.Flags().StringVar(&opts.last, "last", "", "start after the tag specified by `last`") cmd.Flags().BoolVar(&opts.excludeDigestTag, "exclude-digest-tags", false, "[Preview] exclude all digest-like tags such as 'sha256-aaaa...'") option.ApplyFlags(&opts, cmd.Flags()) - return cmd + return oerrors.Command(cmd, &opts.Target) } func showTags(ctx context.Context, opts showTagsOptions) error { diff --git a/cmd/oras/root/tag.go b/cmd/oras/root/tag.go index dcc857f1f..51c6d81cb 100644 --- a/cmd/oras/root/tag.go +++ b/cmd/oras/root/tag.go @@ -88,7 +88,7 @@ Example - Tag the manifest 'v1.0.1' to 'v1.0.2' in an OCI image layout folder 'l option.ApplyFlags(&opts, cmd.Flags()) cmd.Flags().IntVarP(&opts.concurrency, "concurrency", "", 5, "concurrency level") - return cmd + return oerrors.Command(cmd, &opts.Target) } func tagManifest(ctx context.Context, opts tagOptions) error { diff --git a/test/e2e/suite/auth/auth.go b/test/e2e/suite/auth/auth.go index 9c0988c2a..5d192dab6 100644 --- a/test/e2e/suite/auth/auth.go +++ b/test/e2e/suite/auth/auth.go @@ -49,6 +49,23 @@ var _ = Describe("Common registry user", func() { }) }) + When("credential is invalid", func() { + It("should fail with registry error", func() { + RunWithInvalidCreds("attach", ZOTHost+"/repo:tag", "-a", "test=true", "--artifact-type", "doc/example") + RunWithInvalidCreds("discover", ZOTHost+"/repo:tag") + RunWithInvalidCreds("push", "-a", "key=value", ZOTHost+"/repo:tag") + RunWithInvalidCreds("pull", ZOTHost+"/repo:tag") + RunWithInvalidCreds("manifest", "fetch", ZOTHost+"/repo:tag") + RunWithInvalidCreds("blob", "delete", ZOTHost+"/repo@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa") + RunWithInvalidCreds("blob", "push", ZOTHost+"/repo", WriteTempFile("blob", "test")) + RunWithInvalidCreds("tag", ZOTHost+"/repo:tag", "tag1") + RunWithInvalidCreds("resolve", ZOTHost+"/repo:tag") + RunWithInvalidCreds("repo", "ls", ZOTHost) + RunWithInvalidCreds("repo", "tags", RegistryRef(ZOTHost, "repo", "")) + RunWithInvalidCreds("manifest", "fetch-config", ZOTHost+"/repo:tag") + }) + }) + When("logging in", func() { tmpConfigName := "test.config" It("should succeed to use basic auth", func() { @@ -137,3 +154,9 @@ func RunWithoutLogin(args ...string) { MatchErrKeyWords("Error:", "credential required"). WithDescription("fail without logging in").Exec() } + +func RunWithInvalidCreds(args ...string) { + ORAS(append(args, "-u", Username, "-p", Password+"1")...).ExpectFailure(). + MatchErrKeyWords(RegistryErrorPrefix). + WithDescription("fail with invalid credentials").Exec() +} From 70ac3a040be3f617052e3e3606f70c8b52786771 Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Mon, 8 Jan 2024 02:20:09 +0000 Subject: [PATCH 18/42] bug fix Signed-off-by: Billy Zha --- cmd/oras/internal/errors/errors.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/oras/internal/errors/errors.go b/cmd/oras/internal/errors/errors.go index cc9b566ce..1b5ab1a39 100644 --- a/cmd/oras/internal/errors/errors.go +++ b/cmd/oras/internal/errors/errors.go @@ -105,10 +105,10 @@ func GetInnerError(err error, errResp *errcode.ErrorResponse) error { errRespContent := errResp.Error() if idx := strings.Index(errContent, errRespContent); idx > 0 { // remove HTTP related info - return fmt.Errorf("%s: %w", errContent[:idx], errResp) + return fmt.Errorf("%s: %w", errContent[:idx], inner) } } - return nil + return inner } // NewErrEmptyTagOrDigest creates a new error based on the reference string. From 92588f53f3ba5a528198738a2a1df70e0fb71a0b Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Mon, 8 Jan 2024 02:54:15 +0000 Subject: [PATCH 19/42] improve readability Signed-off-by: Billy Zha --- cmd/oras/internal/errors/errors.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/oras/internal/errors/errors.go b/cmd/oras/internal/errors/errors.go index 1b5ab1a39..f90f2bccd 100644 --- a/cmd/oras/internal/errors/errors.go +++ b/cmd/oras/internal/errors/errors.go @@ -105,7 +105,7 @@ func GetInnerError(err error, errResp *errcode.ErrorResponse) error { errRespContent := errResp.Error() if idx := strings.Index(errContent, errRespContent); idx > 0 { // remove HTTP related info - return fmt.Errorf("%s: %w", errContent[:idx], inner) + return fmt.Errorf("%s%w", errContent[:idx], inner) } } return inner From 668152d6751c67e31325bc4d471943040d361479 Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Mon, 8 Jan 2024 03:04:46 +0000 Subject: [PATCH 20/42] resolve empty resp body Signed-off-by: Billy Zha --- cmd/oras/internal/errors/errors.go | 4 ++-- cmd/oras/internal/option/remote.go | 4 +--- cmd/oras/internal/option/target.go | 4 +--- 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/cmd/oras/internal/errors/errors.go b/cmd/oras/internal/errors/errors.go index f90f2bccd..e9bfb8947 100644 --- a/cmd/oras/internal/errors/errors.go +++ b/cmd/oras/internal/errors/errors.go @@ -94,9 +94,9 @@ func Command(cmd *cobra.Command, handler Handler) *cobra.Command { return cmd } -// GetInnerError gets the inner error from the error response. +// GetInner gets the inner error from the error response. // nil is returned if no valid inner error is found. -func GetInnerError(err error, errResp *errcode.ErrorResponse) error { +func GetInner(err error, errResp *errcode.ErrorResponse) error { inner := errResp.Errors if len(inner) == 0 { return fmt.Errorf("empty response body: %w", errResp) diff --git a/cmd/oras/internal/option/remote.go b/cmd/oras/internal/option/remote.go index 3814b0106..906ca2835 100644 --- a/cmd/oras/internal/option/remote.go +++ b/cmd/oras/internal/option/remote.go @@ -351,9 +351,7 @@ func (opts *Remote) Process(err error, _ string) *oerrors.Error { } var errResp *errcode.ErrorResponse if errors.As(err, &errResp) { - if innerErr := oerrors.GetInnerError(err, errResp); innerErr != nil { - ret.Err = innerErr - } + ret.Err = oerrors.GetInner(err, errResp) } return &ret } diff --git a/cmd/oras/internal/option/target.go b/cmd/oras/internal/option/target.go index f39866ef9..dafb0df60 100644 --- a/cmd/oras/internal/option/target.go +++ b/cmd/oras/internal/option/target.go @@ -269,9 +269,7 @@ func (opts *Target) Process(err error, callPath string) *oerrors.Error { var errResp *errcode.ErrorResponse if errors.As(err, &errResp) { // remove HTTP related info - if innerErr := oerrors.GetInnerError(err, errResp); innerErr != nil { - ret.Err = innerErr - } + ret.Err = oerrors.GetInner(err, errResp) ref, parseErr := registry.ParseReference(opts.RawReference) if parseErr != nil { From fa3adf1eeb73a1980b7cb6a011271a5ee48c1e8a Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Mon, 8 Jan 2024 03:18:09 +0000 Subject: [PATCH 21/42] resolve e2e Signed-off-by: Billy Zha --- test/e2e/suite/command/manifest.go | 2 +- test/e2e/suite/command/repo.go | 2 +- test/e2e/suite/command/tag.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/test/e2e/suite/command/manifest.go b/test/e2e/suite/command/manifest.go index 6f7c10ebc..48e29356a 100644 --- a/test/e2e/suite/command/manifest.go +++ b/test/e2e/suite/command/manifest.go @@ -557,7 +557,7 @@ var _ = Describe("1.0 registry users:", func() { It("should fail to fetch image if media type assertion fails", func() { ORAS("manifest", "fetch", RegistryRef(FallbackHost, ImageRepo, multi_arch.LinuxAMD64.Digest.String()), "--media-type", "this.will.not.be.found"). ExpectFailure(). - MatchErrKeyWords(multi_arch.LinuxAMD64.Digest.String(), "error: ", "not found").Exec() + MatchErrKeyWords(multi_arch.LinuxAMD64.Digest.String(), RegistryErrorPrefix, "not found").Exec() }) }) diff --git a/test/e2e/suite/command/repo.go b/test/e2e/suite/command/repo.go index 525884ca3..a505f50b1 100644 --- a/test/e2e/suite/command/repo.go +++ b/test/e2e/suite/command/repo.go @@ -42,7 +42,7 @@ var _ = Describe("ORAS beginners:", func() { }) It("should fail listing repositories if wrong registry provided", func() { - ORAS("repo", "ls", RegistryRef(ZOTHost, ImageRepo, "some-tag")).ExpectFailure().MatchErrKeyWords("Error:").Exec() + ORAS("repo", "ls", RegistryRef(ZOTHost, ImageRepo, "some-tag")).ExpectFailure().MatchErrKeyWords(RegistryErrorPrefix).Exec() }) It("should fail and show detailed error description if no argument provided", func() { diff --git a/test/e2e/suite/command/tag.go b/test/e2e/suite/command/tag.go index 2fd556b9e..4532d7655 100644 --- a/test/e2e/suite/command/tag.go +++ b/test/e2e/suite/command/tag.go @@ -33,7 +33,7 @@ var _ = Describe("ORAS beginners:", func() { }) It("should fail when provided manifest reference is not found", func() { - ORAS("tag", RegistryRef(ZOTHost, ImageRepo, InvalidTag), "tagged").ExpectFailure().MatchErrKeyWords("Error:").Exec() + ORAS("tag", RegistryRef(ZOTHost, ImageRepo, InvalidTag), "tagged").ExpectFailure().MatchErrKeyWords(RegistryErrorPrefix).Exec() }) It("should fail and show detailed error description if no argument provided", func() { From cc42b046ba3be9b55f77b0a907f1361596776a15 Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Mon, 8 Jan 2024 03:23:14 +0000 Subject: [PATCH 22/42] code clean Signed-off-by: Billy Zha --- cmd/oras/internal/errors/errors.go | 1 - cmd/oras/internal/option/target.go | 3 --- 2 files changed, 4 deletions(-) diff --git a/cmd/oras/internal/errors/errors.go b/cmd/oras/internal/errors/errors.go index e9bfb8947..7b66170ba 100644 --- a/cmd/oras/internal/errors/errors.go +++ b/cmd/oras/internal/errors/errors.go @@ -95,7 +95,6 @@ func Command(cmd *cobra.Command, handler Handler) *cobra.Command { } // GetInner gets the inner error from the error response. -// nil is returned if no valid inner error is found. func GetInner(err error, errResp *errcode.ErrorResponse) error { inner := errResp.Errors if len(inner) == 0 { diff --git a/cmd/oras/internal/option/target.go b/cmd/oras/internal/option/target.go index dafb0df60..2a9f43241 100644 --- a/cmd/oras/internal/option/target.go +++ b/cmd/oras/internal/option/target.go @@ -245,7 +245,6 @@ func (opts *Target) EnsureReferenceNotEmpty() error { // Handle handles error during cmd execution. func (opts *Target) Handle(err error, cmd *cobra.Command) (oerrors.Processor, error) { - // handle registry error if opts.IsOCILayout { return nil, err } @@ -268,9 +267,7 @@ func (opts *Target) Process(err error, callPath string) *oerrors.Error { var errResp *errcode.ErrorResponse if errors.As(err, &errResp) { - // remove HTTP related info ret.Err = oerrors.GetInner(err, errResp) - ref, parseErr := registry.ParseReference(opts.RawReference) if parseErr != nil { // this should not happen From 666c5186ef84964f701db7a9a106c3577074e811 Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Mon, 8 Jan 2024 03:35:27 +0000 Subject: [PATCH 23/42] fix e2e Signed-off-by: Billy Zha --- test/e2e/suite/command/repo.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/e2e/suite/command/repo.go b/test/e2e/suite/command/repo.go index a505f50b1..cc0d06ab7 100644 --- a/test/e2e/suite/command/repo.go +++ b/test/e2e/suite/command/repo.go @@ -42,7 +42,7 @@ var _ = Describe("ORAS beginners:", func() { }) It("should fail listing repositories if wrong registry provided", func() { - ORAS("repo", "ls", RegistryRef(ZOTHost, ImageRepo, "some-tag")).ExpectFailure().MatchErrKeyWords(RegistryErrorPrefix).Exec() + ORAS("repo", "ls", RegistryRef(ZOTHost, ImageRepo, "some-tag")).ExpectFailure().MatchErrKeyWords("Error:").Exec() }) It("should fail and show detailed error description if no argument provided", func() { @@ -70,7 +70,7 @@ var _ = Describe("ORAS beginners:", func() { It("should fail listing repositories if wrong registry provided", func() { ORAS("repo", "tags").ExpectFailure().MatchErrKeyWords("Error:").Exec() - ORAS("repo", "tags", ZOTHost).ExpectFailure().MatchErrKeyWords("Error:").Exec() + ORAS("repo", "tags", ZOTHost).ExpectFailure().MatchErrKeyWords(RegistryErrorPrefix).Exec() ORAS("repo", "tags", RegistryRef(ZOTHost, ImageRepo, "some-tag")).ExpectFailure().MatchErrKeyWords("Error:").Exec() }) From 028beb011bc08e4a89240901f955da8e3ec4b7f0 Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Mon, 8 Jan 2024 03:46:04 +0000 Subject: [PATCH 24/42] fix e2e Signed-off-by: Billy Zha --- test/e2e/suite/command/repo.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/e2e/suite/command/repo.go b/test/e2e/suite/command/repo.go index cc0d06ab7..8bc924498 100644 --- a/test/e2e/suite/command/repo.go +++ b/test/e2e/suite/command/repo.go @@ -68,10 +68,10 @@ var _ = Describe("ORAS beginners:", func() { ORAS("repository", "show-tags", "--help").MatchKeyWords(ExampleDesc).Exec() }) - It("should fail listing repositories if wrong registry provided", func() { + It("should fail listing repositories if wrong reference provided", func() { ORAS("repo", "tags").ExpectFailure().MatchErrKeyWords("Error:").Exec() - ORAS("repo", "tags", ZOTHost).ExpectFailure().MatchErrKeyWords(RegistryErrorPrefix).Exec() - ORAS("repo", "tags", RegistryRef(ZOTHost, ImageRepo, "some-tag")).ExpectFailure().MatchErrKeyWords("Error:").Exec() + ORAS("repo", "tags", ZOTHost).ExpectFailure().MatchErrKeyWords("Error:").Exec() + ORAS("repo", "tags", RegistryRef(ZOTHost, ImageRepo, "some-tag")).ExpectFailure().MatchErrKeyWords(RegistryErrorPrefix).Exec() }) It("should fail and show detailed error description if no argument provided", func() { From 716e62a8522e74075f2d30e53bab08ed3db4adaf Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Tue, 9 Jan 2024 03:38:06 +0000 Subject: [PATCH 25/42] resolve comment Signed-off-by: Billy Zha --- cmd/oras/internal/errors/errors.go | 16 +- cmd/oras/internal/option/remote.go | 30 +--- cmd/oras/internal/option/target.go | 41 ++--- cmd/oras/internal/option/target_test.go | 223 ++++++++++++------------ 4 files changed, 135 insertions(+), 175 deletions(-) diff --git a/cmd/oras/internal/errors/errors.go b/cmd/oras/internal/errors/errors.go index 7b66170ba..23ff98590 100644 --- a/cmd/oras/internal/errors/errors.go +++ b/cmd/oras/internal/errors/errors.go @@ -68,13 +68,8 @@ func CheckArgs(checker func(args []string) (bool, string), Usage string) cobra.P // Handler handles error during cmd execution. type Handler interface { // Handle handles error during cmd execution. - // If returned processor is nil, error requires no further processing. - Handle(err error, cmd *cobra.Command) (Processor, error) -} - -// Processor processes error. -type Processor interface { - Process(err error, callPath string) *Error + // if nil is returned, then err cannot be handled. + Handle(err error, cmd *cobra.Command) error } // Command returns an error-handled cobra command. @@ -85,11 +80,10 @@ func Command(cmd *cobra.Command, handler Handler) *cobra.Command { if err == nil { return nil } - processor, err := handler.Handle(err, cmd) - if processor == nil { - return err + if handled := handler.Handle(err, cmd); handled != nil { + return handled } - return processor.Process(err, cmd.CommandPath()) + return err } return cmd } diff --git a/cmd/oras/internal/option/remote.go b/cmd/oras/internal/option/remote.go index 906ca2835..45ce9c997 100644 --- a/cmd/oras/internal/option/remote.go +++ b/cmd/oras/internal/option/remote.go @@ -328,30 +328,16 @@ func (opts *Remote) isPlainHttp(registry string) bool { } // Handle handles error during cmd execution. -func (opts *Remote) Handle(err error, cmd *cobra.Command) (oerrors.Processor, error) { - // handle not found error from registry +func (opts *Remote) Handle(err error, cmd *cobra.Command) error { + var errResp *errcode.ErrorResponse if errors.Is(err, errdef.ErrNotFound) { cmd.SetErrPrefix(oerrors.RegistryErrorPrefix) - return opts, err - } - - // handle error response - var errResp *errcode.ErrorResponse - if errors.As(err, &errResp) { + return err + } else if errors.As(err, &errResp) { cmd.SetErrPrefix(oerrors.RegistryErrorPrefix) - return opts, err - } - return nil, err -} - -// Process processes error into oerrors.Error. -func (opts *Remote) Process(err error, _ string) *oerrors.Error { - ret := oerrors.Error{ - Err: err, - } - var errResp *errcode.ErrorResponse - if errors.As(err, &errResp) { - ret.Err = oerrors.GetInner(err, errResp) + return &oerrors.Error{ + Err: oerrors.GetInner(err, errResp), + } } - return &ret + return nil } diff --git a/cmd/oras/internal/option/target.go b/cmd/oras/internal/option/target.go index 2a9f43241..dc65439a5 100644 --- a/cmd/oras/internal/option/target.go +++ b/cmd/oras/internal/option/target.go @@ -32,6 +32,7 @@ import ( "oras.land/oras-go/v2" "oras.land/oras-go/v2/content" "oras.land/oras-go/v2/content/oci" + "oras.land/oras-go/v2/errdef" "oras.land/oras-go/v2/registry" "oras.land/oras-go/v2/registry/remote" "oras.land/oras-go/v2/registry/remote/errcode" @@ -244,45 +245,35 @@ func (opts *Target) EnsureReferenceNotEmpty() error { } // Handle handles error during cmd execution. -func (opts *Target) Handle(err error, cmd *cobra.Command) (oerrors.Processor, error) { +func (opts *Target) Handle(err error, cmd *cobra.Command) error { if opts.IsOCILayout { - return nil, err - } - - processor, err := opts.Remote.Handle(err, cmd) - if processor != nil { - processor = opts + return nil } - return processor, err -} - -// Process processes error into oerrors.Error. -func (opts *Target) Process(err error, callPath string) *oerrors.Error { - ret := oerrors.Error{ + ret := &oerrors.Error{ Err: err, } - if opts.IsOCILayout { - return &ret - } var errResp *errcode.ErrorResponse - if errors.As(err, &errResp) { + if errors.Is(err, errdef.ErrNotFound) { + cmd.SetErrPrefix(oerrors.RegistryErrorPrefix) + } else if errors.As(err, &errResp) { + cmd.SetErrPrefix(oerrors.RegistryErrorPrefix) ret.Err = oerrors.GetInner(err, errResp) ref, parseErr := registry.ParseReference(opts.RawReference) if parseErr != nil { // this should not happen - return &ret + return ret } if ref.Registry == "docker.io" && errResp.URL.Host == ref.Host() && errResp.StatusCode == http.StatusUnauthorized { if ref.Repository != "" && !strings.Contains(ref.Repository, "/") { // docker.io/xxx -> docker.io/library/xxx ref.Repository = "library/" + ref.Repository - ret.Recommendation = fmt.Sprintf("Namespace is missing, do you mean `%s %s`?", callPath, ref) + ret.Recommendation = fmt.Sprintf("Namespace is missing, do you mean `%s %s`?", cmd.CommandPath(), ref) } } } - return &ret + return ret } // BinaryTarget struct contains flags and arguments specifying two registries or @@ -318,13 +309,9 @@ func (opts *BinaryTarget) Parse() error { } // Handle handles error during cmd execution. -func (opts *BinaryTarget) Handle(err error, cmd *cobra.Command) (oerrors.Processor, error) { - if processor, err := opts.From.Handle(err, cmd); processor != nil { - if errResp, ok := err.(*errcode.ErrorResponse); ok { - if ref, _ := registry.ParseReference(opts.From.RawReference); errResp.URL.Host == ref.Host() { - return processor, err - } - } +func (opts *BinaryTarget) Handle(err error, cmd *cobra.Command) error { + if err := opts.From.Handle(err, cmd); err != nil { + return err } return opts.To.Handle(err, cmd) } diff --git a/cmd/oras/internal/option/target_test.go b/cmd/oras/internal/option/target_test.go index f8271c64e..01d6fbcdb 100644 --- a/cmd/oras/internal/option/target_test.go +++ b/cmd/oras/internal/option/target_test.go @@ -16,14 +16,7 @@ limitations under the License. package option import ( - "errors" - "net/http" - "net/url" - "reflect" "testing" - - "oras.land/oras-go/v2/registry/remote/errcode" - oerrors "oras.land/oras/cmd/oras/internal/errors" ) func TestTarget_Parse_oci(t *testing.T) { @@ -83,112 +76,112 @@ func Test_parseOCILayoutReference(t *testing.T) { } } -func TestTarget_Process_ociLayout(t *testing.T) { - errClient := errors.New("client error") - opts := &Target{ - IsOCILayout: true, - } - if got := opts.Process(errClient, ""); got.Err != errClient || got.Recommendation != "" { - t.Errorf("unexpected output from Target.Process() = %v", got) - } -} +// func TestTarget_Process_ociLayout(t *testing.T) { +// errClient := errors.New("client error") +// opts := &Target{ +// IsOCILayout: true, +// } +// if got := opts.Process(errClient, ""); got.Err != errClient || got.Recommendation != "" { +// t.Errorf("unexpected output from Target.Process() = %v", got) +// } +// } -func TestTarget_Process_hint(t *testing.T) { - type fields struct { - Remote Remote - RawReference string - Type string - Reference string - Path string - IsOCILayout bool - } - type args struct { - err error - callPath string - } - errs := errcode.Errors{ - errcode.Error{ - Code: "000", - Message: "mocked message", - Detail: map[string]string{"mocked key": "mocked value"}, - }, - } - tests := []struct { - name string - fields fields - args args - want *oerrors.Error - }{ - { - "namespace already exists", - fields{RawReference: "docker.io/library/alpine:latest"}, - args{ - err: &errcode.ErrorResponse{ - URL: &url.URL{Host: "registry-1.docker.io"}, - StatusCode: http.StatusUnauthorized, - Errors: errs, - }, - }, - &oerrors.Error{Err: errs}, - }, - { - "no namespace", - fields{RawReference: "docker.io"}, - args{ - err: &errcode.ErrorResponse{ - URL: &url.URL{Host: "registry-1.docker.io"}, - StatusCode: http.StatusUnauthorized, - Errors: errs, - }, - }, - &oerrors.Error{Err: errs}, - }, - { - "not 401", - fields{RawReference: "docker.io"}, - args{ - err: &errcode.ErrorResponse{ - URL: &url.URL{Host: "registry-1.docker.io"}, - StatusCode: http.StatusConflict, - Errors: errs, - }, - }, - &oerrors.Error{Err: errs}, - }, - { - "should hint", - fields{ - RawReference: "docker.io/alpine", - Path: "oras test", - }, - args{ - err: &errcode.ErrorResponse{ - URL: &url.URL{Host: "registry-1.docker.io"}, - StatusCode: http.StatusUnauthorized, - Errors: errs, - }, - callPath: "oras test", - }, - &oerrors.Error{ - Err: errs, - Recommendation: "Namespace is missing, do you mean `oras test docker.io/library/alpine`?", - }, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - opts := &Target{ - Remote: tt.fields.Remote, - RawReference: tt.fields.RawReference, - Type: tt.fields.Type, - Reference: tt.fields.Reference, - Path: tt.fields.Path, - IsOCILayout: tt.fields.IsOCILayout, - } - got := opts.Process(tt.args.err, tt.args.callPath) - if !reflect.DeepEqual(got, tt.want) { - t.Errorf("Target.Process() = %v, want %v", got, tt.want) - } - }) - } -} +// func TestTarget_Process_hint(t *testing.T) { +// type fields struct { +// Remote Remote +// RawReference string +// Type string +// Reference string +// Path string +// IsOCILayout bool +// } +// type args struct { +// err error +// callPath string +// } +// errs := errcode.Errors{ +// errcode.Error{ +// Code: "000", +// Message: "mocked message", +// Detail: map[string]string{"mocked key": "mocked value"}, +// }, +// } +// tests := []struct { +// name string +// fields fields +// args args +// want *oerrors.Error +// }{ +// { +// "namespace already exists", +// fields{RawReference: "docker.io/library/alpine:latest"}, +// args{ +// err: &errcode.ErrorResponse{ +// URL: &url.URL{Host: "registry-1.docker.io"}, +// StatusCode: http.StatusUnauthorized, +// Errors: errs, +// }, +// }, +// &oerrors.Error{Err: errs}, +// }, +// { +// "no namespace", +// fields{RawReference: "docker.io"}, +// args{ +// err: &errcode.ErrorResponse{ +// URL: &url.URL{Host: "registry-1.docker.io"}, +// StatusCode: http.StatusUnauthorized, +// Errors: errs, +// }, +// }, +// &oerrors.Error{Err: errs}, +// }, +// { +// "not 401", +// fields{RawReference: "docker.io"}, +// args{ +// err: &errcode.ErrorResponse{ +// URL: &url.URL{Host: "registry-1.docker.io"}, +// StatusCode: http.StatusConflict, +// Errors: errs, +// }, +// }, +// &oerrors.Error{Err: errs}, +// }, +// { +// "should hint", +// fields{ +// RawReference: "docker.io/alpine", +// Path: "oras test", +// }, +// args{ +// err: &errcode.ErrorResponse{ +// URL: &url.URL{Host: "registry-1.docker.io"}, +// StatusCode: http.StatusUnauthorized, +// Errors: errs, +// }, +// callPath: "oras test", +// }, +// &oerrors.Error{ +// Err: errs, +// Recommendation: "Namespace is missing, do you mean `oras test docker.io/library/alpine`?", +// }, +// }, +// } +// for _, tt := range tests { +// t.Run(tt.name, func(t *testing.T) { +// opts := &Target{ +// Remote: tt.fields.Remote, +// RawReference: tt.fields.RawReference, +// Type: tt.fields.Type, +// Reference: tt.fields.Reference, +// Path: tt.fields.Path, +// IsOCILayout: tt.fields.IsOCILayout, +// } +// got := opts.Process(tt.args.err, tt.args.callPath) +// if !reflect.DeepEqual(got, tt.want) { +// t.Errorf("Target.Process() = %v, want %v", got, tt.want) +// } +// }) +// } +// } From 4503268cfbe4b9fce334953a096d64ebb45752f6 Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Tue, 9 Jan 2024 03:42:27 +0000 Subject: [PATCH 26/42] code clean Signed-off-by: Billy Zha --- cmd/oras/internal/option/remote.go | 2 +- cmd/oras/internal/option/target.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/oras/internal/option/remote.go b/cmd/oras/internal/option/remote.go index 45ce9c997..13a8ae543 100644 --- a/cmd/oras/internal/option/remote.go +++ b/cmd/oras/internal/option/remote.go @@ -46,7 +46,7 @@ import ( ) // Remote options struct contains flags and arguments specifying one registry. -// Remote implements oerrors.Handler and oerrors.Processor interface. +// Remote implements oerrors.Handler and interface. type Remote struct { DistributionSpec CACertFilePath string diff --git a/cmd/oras/internal/option/target.go b/cmd/oras/internal/option/target.go index dc65439a5..f5bab20a6 100644 --- a/cmd/oras/internal/option/target.go +++ b/cmd/oras/internal/option/target.go @@ -47,7 +47,7 @@ const ( // Target struct contains flags and arguments specifying one registry or image // layout. -// Target implements oerrors.Handler and oerrors.Processor interface. +// Target implements oerrors.Handler interface. type Target struct { Remote RawReference string From 9d299a93a75029c3594dac3970d604832ca39b2f Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Tue, 9 Jan 2024 03:56:05 +0000 Subject: [PATCH 27/42] bug fix Signed-off-by: Billy Zha --- cmd/oras/internal/option/target.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/cmd/oras/internal/option/target.go b/cmd/oras/internal/option/target.go index f5bab20a6..b030f7f72 100644 --- a/cmd/oras/internal/option/target.go +++ b/cmd/oras/internal/option/target.go @@ -257,15 +257,19 @@ func (opts *Target) Handle(err error, cmd *cobra.Command) error { if errors.Is(err, errdef.ErrNotFound) { cmd.SetErrPrefix(oerrors.RegistryErrorPrefix) } else if errors.As(err, &errResp) { - cmd.SetErrPrefix(oerrors.RegistryErrorPrefix) - ret.Err = oerrors.GetInner(err, errResp) ref, parseErr := registry.ParseReference(opts.RawReference) if parseErr != nil { // this should not happen return ret } + if errResp.URL.Host != ref.Host() { + // not handle if the error is not from the target + return nil + } + cmd.SetErrPrefix(oerrors.RegistryErrorPrefix) + ret.Err = oerrors.GetInner(err, errResp) - if ref.Registry == "docker.io" && errResp.URL.Host == ref.Host() && errResp.StatusCode == http.StatusUnauthorized { + if ref.Registry == "docker.io" && errResp.StatusCode == http.StatusUnauthorized { if ref.Repository != "" && !strings.Contains(ref.Repository, "/") { // docker.io/xxx -> docker.io/library/xxx ref.Repository = "library/" + ref.Repository From 3607c163e0fe4310c2d9329c7ad2beec3f92f5d8 Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Tue, 9 Jan 2024 15:24:22 +0000 Subject: [PATCH 28/42] resolve comments Signed-off-by: Billy Zha --- cmd/oras/internal/errors/errors.go | 17 ++++++++--------- cmd/oras/internal/option/remote.go | 13 +++++++------ cmd/oras/internal/option/target.go | 14 +++++++------- 3 files changed, 22 insertions(+), 22 deletions(-) diff --git a/cmd/oras/internal/errors/errors.go b/cmd/oras/internal/errors/errors.go index 23ff98590..0b1449993 100644 --- a/cmd/oras/internal/errors/errors.go +++ b/cmd/oras/internal/errors/errors.go @@ -65,22 +65,21 @@ func CheckArgs(checker func(args []string) (bool, string), Usage string) cobra.P } } -// Handler handles error during cmd execution. -type Handler interface { - // Handle handles error during cmd execution. - // if nil is returned, then err cannot be handled. - Handle(err error, cmd *cobra.Command) error +// Modifier modifies the error during cmd execution. +// If nil is returned, err should be used by caller. +type Modifier interface { + Modify(cmd *cobra.Command, err error) error } // Command returns an error-handled cobra command. -func Command(cmd *cobra.Command, handler Handler) *cobra.Command { +func Command(cmd *cobra.Command, handler Modifier) *cobra.Command { runE := cmd.RunE cmd.RunE = func(cmd *cobra.Command, args []string) error { err := runE(cmd, args) if err == nil { return nil } - if handled := handler.Handle(err, cmd); handled != nil { + if handled := handler.Modify(cmd, err); handled != nil { return handled } return err @@ -88,8 +87,8 @@ func Command(cmd *cobra.Command, handler Handler) *cobra.Command { return cmd } -// GetInner gets the inner error from the error response. -func GetInner(err error, errResp *errcode.ErrorResponse) error { +// Trim trims the error response detail from error message. +func Trim(err error, errResp *errcode.ErrorResponse) error { inner := errResp.Errors if len(inner) == 0 { return fmt.Errorf("empty response body: %w", errResp) diff --git a/cmd/oras/internal/option/remote.go b/cmd/oras/internal/option/remote.go index 13a8ae543..e960b191b 100644 --- a/cmd/oras/internal/option/remote.go +++ b/cmd/oras/internal/option/remote.go @@ -327,17 +327,18 @@ func (opts *Remote) isPlainHttp(registry string) bool { return plainHTTP } -// Handle handles error during cmd execution. -func (opts *Remote) Handle(err error, cmd *cobra.Command) error { - var errResp *errcode.ErrorResponse +// Modify modifies error during cmd execution. +func (opts *Remote) Modify(cmd *cobra.Command, err error) error { if errors.Is(err, errdef.ErrNotFound) { cmd.SetErrPrefix(oerrors.RegistryErrorPrefix) return err - } else if errors.As(err, &errResp) { + } + var errResp *errcode.ErrorResponse + if errors.As(err, &errResp) { cmd.SetErrPrefix(oerrors.RegistryErrorPrefix) return &oerrors.Error{ - Err: oerrors.GetInner(err, errResp), + Err: oerrors.Trim(err, errResp), } } - return nil + return err } diff --git a/cmd/oras/internal/option/target.go b/cmd/oras/internal/option/target.go index b030f7f72..990a4d2cf 100644 --- a/cmd/oras/internal/option/target.go +++ b/cmd/oras/internal/option/target.go @@ -244,8 +244,8 @@ func (opts *Target) EnsureReferenceNotEmpty() error { return nil } -// Handle handles error during cmd execution. -func (opts *Target) Handle(err error, cmd *cobra.Command) error { +// Modify handles error during cmd execution. +func (opts *Target) Modify(cmd *cobra.Command, err error) error { if opts.IsOCILayout { return nil } @@ -267,7 +267,7 @@ func (opts *Target) Handle(err error, cmd *cobra.Command) error { return nil } cmd.SetErrPrefix(oerrors.RegistryErrorPrefix) - ret.Err = oerrors.GetInner(err, errResp) + ret.Err = oerrors.Trim(err, errResp) if ref.Registry == "docker.io" && errResp.StatusCode == http.StatusUnauthorized { if ref.Repository != "" && !strings.Contains(ref.Repository, "/") { @@ -312,10 +312,10 @@ func (opts *BinaryTarget) Parse() error { return Parse(opts) } -// Handle handles error during cmd execution. -func (opts *BinaryTarget) Handle(err error, cmd *cobra.Command) error { - if err := opts.From.Handle(err, cmd); err != nil { +// Modify handles error during cmd execution. +func (opts *BinaryTarget) Modify(cmd *cobra.Command, err error) error { + if err := opts.From.Modify(cmd, err); err != nil { return err } - return opts.To.Handle(err, cmd) + return opts.To.Modify(cmd, err) } From a9664b35e1b3cc1ad31fb0201cbf73d912830435 Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Thu, 11 Jan 2024 02:46:40 +0000 Subject: [PATCH 29/42] resolve comments Signed-off-by: Billy Zha --- cmd/oras/internal/errors/errors.go | 13 +++++-------- cmd/oras/internal/option/remote.go | 8 ++++---- cmd/oras/internal/option/target.go | 24 ++++++++++++------------ 3 files changed, 21 insertions(+), 24 deletions(-) diff --git a/cmd/oras/internal/errors/errors.go b/cmd/oras/internal/errors/errors.go index 0b1449993..ab1947f0c 100644 --- a/cmd/oras/internal/errors/errors.go +++ b/cmd/oras/internal/errors/errors.go @@ -66,9 +66,8 @@ func CheckArgs(checker func(args []string) (bool, string), Usage string) cobra.P } // Modifier modifies the error during cmd execution. -// If nil is returned, err should be used by caller. type Modifier interface { - Modify(cmd *cobra.Command, err error) error + Modify(cmd *cobra.Command, err error) (modifiedErr error, modified bool) } // Command returns an error-handled cobra command. @@ -76,13 +75,11 @@ func Command(cmd *cobra.Command, handler Modifier) *cobra.Command { runE := cmd.RunE cmd.RunE = func(cmd *cobra.Command, args []string) error { err := runE(cmd, args) - if err == nil { - return nil + if err != nil { + err, _ = handler.Modify(cmd, err) + return err } - if handled := handler.Modify(cmd, err); handled != nil { - return handled - } - return err + return nil } return cmd } diff --git a/cmd/oras/internal/option/remote.go b/cmd/oras/internal/option/remote.go index e960b191b..67df6ab90 100644 --- a/cmd/oras/internal/option/remote.go +++ b/cmd/oras/internal/option/remote.go @@ -328,17 +328,17 @@ func (opts *Remote) isPlainHttp(registry string) bool { } // Modify modifies error during cmd execution. -func (opts *Remote) Modify(cmd *cobra.Command, err error) error { +func (opts *Remote) Modify(cmd *cobra.Command, err error) (error, bool) { if errors.Is(err, errdef.ErrNotFound) { cmd.SetErrPrefix(oerrors.RegistryErrorPrefix) - return err + return err, true } var errResp *errcode.ErrorResponse if errors.As(err, &errResp) { cmd.SetErrPrefix(oerrors.RegistryErrorPrefix) return &oerrors.Error{ Err: oerrors.Trim(err, errResp), - } + }, true } - return err + return err, false } diff --git a/cmd/oras/internal/option/target.go b/cmd/oras/internal/option/target.go index 990a4d2cf..8aa4ea594 100644 --- a/cmd/oras/internal/option/target.go +++ b/cmd/oras/internal/option/target.go @@ -245,29 +245,29 @@ func (opts *Target) EnsureReferenceNotEmpty() error { } // Modify handles error during cmd execution. -func (opts *Target) Modify(cmd *cobra.Command, err error) error { +func (opts *Target) Modify(cmd *cobra.Command, err error) (error, bool) { if opts.IsOCILayout { - return nil - } - ret := &oerrors.Error{ - Err: err, + return err, false } var errResp *errcode.ErrorResponse if errors.Is(err, errdef.ErrNotFound) { cmd.SetErrPrefix(oerrors.RegistryErrorPrefix) + return err, true } else if errors.As(err, &errResp) { ref, parseErr := registry.ParseReference(opts.RawReference) if parseErr != nil { // this should not happen - return ret + return err, false } if errResp.URL.Host != ref.Host() { // not handle if the error is not from the target - return nil + return err, false } cmd.SetErrPrefix(oerrors.RegistryErrorPrefix) - ret.Err = oerrors.Trim(err, errResp) + ret := &oerrors.Error{ + Err: oerrors.Trim(err, errResp), + } if ref.Registry == "docker.io" && errResp.StatusCode == http.StatusUnauthorized { if ref.Repository != "" && !strings.Contains(ref.Repository, "/") { @@ -277,7 +277,7 @@ func (opts *Target) Modify(cmd *cobra.Command, err error) error { } } } - return ret + return err, false } // BinaryTarget struct contains flags and arguments specifying two registries or @@ -313,9 +313,9 @@ func (opts *BinaryTarget) Parse() error { } // Modify handles error during cmd execution. -func (opts *BinaryTarget) Modify(cmd *cobra.Command, err error) error { - if err := opts.From.Modify(cmd, err); err != nil { - return err +func (opts *BinaryTarget) Modify(cmd *cobra.Command, err error) (error, bool) { + if modifiedErr, modified := opts.From.Modify(cmd, err); modified { + return modifiedErr, modified } return opts.To.Modify(cmd, err) } From 0131f020e2d69dd8bacb903dd275b364067d0b0e Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Thu, 11 Jan 2024 03:09:26 +0000 Subject: [PATCH 30/42] bug fix Signed-off-by: Billy Zha --- cmd/oras/internal/errors/errors.go | 2 +- cmd/oras/internal/option/target.go | 1 + test/e2e/internal/utils/const.go | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/cmd/oras/internal/errors/errors.go b/cmd/oras/internal/errors/errors.go index ab1947f0c..afc83fbe2 100644 --- a/cmd/oras/internal/errors/errors.go +++ b/cmd/oras/internal/errors/errors.go @@ -88,7 +88,7 @@ func Command(cmd *cobra.Command, handler Modifier) *cobra.Command { func Trim(err error, errResp *errcode.ErrorResponse) error { inner := errResp.Errors if len(inner) == 0 { - return fmt.Errorf("empty response body: %w", errResp) + return fmt.Errorf("recognizable error message not found: %w", errResp) } else { errContent := err.Error() errRespContent := errResp.Error() diff --git a/cmd/oras/internal/option/target.go b/cmd/oras/internal/option/target.go index 8aa4ea594..8ebb4ecc6 100644 --- a/cmd/oras/internal/option/target.go +++ b/cmd/oras/internal/option/target.go @@ -276,6 +276,7 @@ func (opts *Target) Modify(cmd *cobra.Command, err error) (error, bool) { ret.Recommendation = fmt.Sprintf("Namespace is missing, do you mean `%s %s`?", cmd.CommandPath(), ref) } } + return ret, true } return err, false } diff --git a/test/e2e/internal/utils/const.go b/test/e2e/internal/utils/const.go index f13d3a4de..6c247aa29 100644 --- a/test/e2e/internal/utils/const.go +++ b/test/e2e/internal/utils/const.go @@ -30,6 +30,6 @@ var ( "--image-spec", } RegistryErrorPrefix = "Error response from registry: " - EmptyBodyPrefix = "empty response body: " + EmptyBodyPrefix = "recognizable error message not found: " InvalidTag = "i-dont-think-this-tag-exists" ) From e47e0d3e5c3d5a802c71ecbec3c2332990cf69a5 Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Thu, 11 Jan 2024 03:25:22 +0000 Subject: [PATCH 31/42] add coverage Signed-off-by: Billy Zha --- test/e2e/suite/command/cp.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/test/e2e/suite/command/cp.go b/test/e2e/suite/command/cp.go index ba09d4bef..a0838a10d 100644 --- a/test/e2e/suite/command/cp.go +++ b/test/e2e/suite/command/cp.go @@ -83,9 +83,16 @@ var _ = Describe("ORAS beginners:", func() { }) It("should fail and show registry error prefix if target registry is not logged in", func() { - src := RegistryRef(ZOTHost, ArtifactRepo, foobar.Tag) + src := PrepareTempOCI(ArtifactRepo) dst := RegistryRef(ZOTHost, cpTestRepo("dest-not-logged-in"), "") - ORAS("cp", src, dst, "--to-username", Username, "--to-password", Password+"?"). + ORAS("cp", Flags.FromLayout, src, dst, "--to-username", Username, "--to-password", Password+"?"). + MatchErrKeyWords(RegistryErrorPrefix).ExpectFailure().Exec() + }) + + It("should fail and show registry error prefix if target registry is not logged in", func() { + src := RegistryRef(ZOTHost, cpTestRepo("dest-not-logged-in"), "") + dst := RegistryRef(ZOTHost, ArtifactRepo, foobar.Tag) + ORAS("cp", src, dst, "--from-username", Username, "--from-password", Password+"?"). MatchErrKeyWords(RegistryErrorPrefix).ExpectFailure().Exec() }) }) From 0494f779dbec799d46ed2adca6d2319196dc4dbd Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Thu, 11 Jan 2024 03:39:45 +0000 Subject: [PATCH 32/42] add unit test Signed-off-by: Billy Zha --- cmd/oras/internal/option/target_test.go | 26 ++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/cmd/oras/internal/option/target_test.go b/cmd/oras/internal/option/target_test.go index 01d6fbcdb..86e1316b5 100644 --- a/cmd/oras/internal/option/target_test.go +++ b/cmd/oras/internal/option/target_test.go @@ -16,7 +16,10 @@ limitations under the License. package option import ( + "errors" "testing" + + "github.com/spf13/cobra" ) func TestTarget_Parse_oci(t *testing.T) { @@ -76,15 +79,20 @@ func Test_parseOCILayoutReference(t *testing.T) { } } -// func TestTarget_Process_ociLayout(t *testing.T) { -// errClient := errors.New("client error") -// opts := &Target{ -// IsOCILayout: true, -// } -// if got := opts.Process(errClient, ""); got.Err != errClient || got.Recommendation != "" { -// t.Errorf("unexpected output from Target.Process() = %v", got) -// } -// } +func TestTarget_Modify_ociLayout(t *testing.T) { + errClient := errors.New("client error") + opts := &Target{ + IsOCILayout: true, + } + got, modified := opts.Modify(&cobra.Command{}, errClient) + + if modified { + t.Errorf("expect error not to be modified but received true") + } + if got != errClient { + t.Errorf("unexpected output from Target.Process() = %v", got) + } +} // func TestTarget_Process_hint(t *testing.T) { // type fields struct { From 19b7ec13f8224d30058f32cbe15b9c8e3301f963 Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Thu, 11 Jan 2024 04:02:00 +0000 Subject: [PATCH 33/42] fix e2e Signed-off-by: Billy Zha --- test/e2e/suite/command/cp.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/e2e/suite/command/cp.go b/test/e2e/suite/command/cp.go index a0838a10d..c0bb31f57 100644 --- a/test/e2e/suite/command/cp.go +++ b/test/e2e/suite/command/cp.go @@ -82,16 +82,16 @@ var _ = Describe("ORAS beginners:", func() { ORAS("cp", src, Flags.ToLayout, dst).MatchErrKeyWords(RegistryErrorPrefix).ExpectFailure().Exec() }) - It("should fail and show registry error prefix if target registry is not logged in", func() { + It("should fail and show registry error prefix if destination registry is not logged in", func() { src := PrepareTempOCI(ArtifactRepo) dst := RegistryRef(ZOTHost, cpTestRepo("dest-not-logged-in"), "") - ORAS("cp", Flags.FromLayout, src, dst, "--to-username", Username, "--to-password", Password+"?"). + ORAS("cp", Flags.FromLayout, LayoutRef(src, foobar.Tag), dst, "--to-username", Username, "--to-password", Password+"?"). MatchErrKeyWords(RegistryErrorPrefix).ExpectFailure().Exec() }) - It("should fail and show registry error prefix if target registry is not logged in", func() { - src := RegistryRef(ZOTHost, cpTestRepo("dest-not-logged-in"), "") - dst := RegistryRef(ZOTHost, ArtifactRepo, foobar.Tag) + It("should fail and show registry error prefix if source registry is not logged in", func() { + src := RegistryRef(ZOTHost, cpTestRepo("src-not-logged-in"), foobar.Tag) + dst := RegistryRef(ZOTHost, ArtifactRepo, "") ORAS("cp", src, dst, "--from-username", Username, "--from-password", Password+"?"). MatchErrKeyWords(RegistryErrorPrefix).ExpectFailure().Exec() }) From e530def3c1726afbb13ead75e3cf168e5661d55d Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Thu, 11 Jan 2024 05:19:08 +0000 Subject: [PATCH 34/42] fix full reference match and add UT Signed-off-by: Billy Zha --- cmd/oras/internal/option/target.go | 26 ++-- cmd/oras/internal/option/target_test.go | 199 ++++++++++++------------ 2 files changed, 117 insertions(+), 108 deletions(-) diff --git a/cmd/oras/internal/option/target.go b/cmd/oras/internal/option/target.go index 8ebb4ecc6..bcf183da7 100644 --- a/cmd/oras/internal/option/target.go +++ b/cmd/oras/internal/option/target.go @@ -250,20 +250,28 @@ func (opts *Target) Modify(cmd *cobra.Command, err error) (error, bool) { return err, false } - var errResp *errcode.ErrorResponse if errors.Is(err, errdef.ErrNotFound) { cmd.SetErrPrefix(oerrors.RegistryErrorPrefix) return err, true - } else if errors.As(err, &errResp) { - ref, parseErr := registry.ParseReference(opts.RawReference) - if parseErr != nil { - // this should not happen - return err, false - } + } + + var errResp *errcode.ErrorResponse + if errors.As(err, &errResp) { + ref := registry.Reference{Registry: opts.RawReference} if errResp.URL.Host != ref.Host() { - // not handle if the error is not from the target - return err, false + // raw reference is not registry host + var parseErr error + ref, parseErr = registry.ParseReference(opts.RawReference) + if parseErr != nil { + // this should not happen + return err, false + } + if errResp.URL.Host != ref.Host() { + // not handle if the error is not from the target + return err, false + } } + cmd.SetErrPrefix(oerrors.RegistryErrorPrefix) ret := &oerrors.Error{ Err: oerrors.Trim(err, errResp), diff --git a/cmd/oras/internal/option/target_test.go b/cmd/oras/internal/option/target_test.go index 86e1316b5..bbad419fb 100644 --- a/cmd/oras/internal/option/target_test.go +++ b/cmd/oras/internal/option/target_test.go @@ -17,9 +17,14 @@ package option import ( "errors" + "net/http" + "net/url" + "reflect" "testing" "github.com/spf13/cobra" + "oras.land/oras-go/v2/registry/remote/errcode" + oerrors "oras.land/oras/cmd/oras/internal/errors" ) func TestTarget_Parse_oci(t *testing.T) { @@ -94,102 +99,98 @@ func TestTarget_Modify_ociLayout(t *testing.T) { } } -// func TestTarget_Process_hint(t *testing.T) { -// type fields struct { -// Remote Remote -// RawReference string -// Type string -// Reference string -// Path string -// IsOCILayout bool -// } -// type args struct { -// err error -// callPath string -// } -// errs := errcode.Errors{ -// errcode.Error{ -// Code: "000", -// Message: "mocked message", -// Detail: map[string]string{"mocked key": "mocked value"}, -// }, -// } -// tests := []struct { -// name string -// fields fields -// args args -// want *oerrors.Error -// }{ -// { -// "namespace already exists", -// fields{RawReference: "docker.io/library/alpine:latest"}, -// args{ -// err: &errcode.ErrorResponse{ -// URL: &url.URL{Host: "registry-1.docker.io"}, -// StatusCode: http.StatusUnauthorized, -// Errors: errs, -// }, -// }, -// &oerrors.Error{Err: errs}, -// }, -// { -// "no namespace", -// fields{RawReference: "docker.io"}, -// args{ -// err: &errcode.ErrorResponse{ -// URL: &url.URL{Host: "registry-1.docker.io"}, -// StatusCode: http.StatusUnauthorized, -// Errors: errs, -// }, -// }, -// &oerrors.Error{Err: errs}, -// }, -// { -// "not 401", -// fields{RawReference: "docker.io"}, -// args{ -// err: &errcode.ErrorResponse{ -// URL: &url.URL{Host: "registry-1.docker.io"}, -// StatusCode: http.StatusConflict, -// Errors: errs, -// }, -// }, -// &oerrors.Error{Err: errs}, -// }, -// { -// "should hint", -// fields{ -// RawReference: "docker.io/alpine", -// Path: "oras test", -// }, -// args{ -// err: &errcode.ErrorResponse{ -// URL: &url.URL{Host: "registry-1.docker.io"}, -// StatusCode: http.StatusUnauthorized, -// Errors: errs, -// }, -// callPath: "oras test", -// }, -// &oerrors.Error{ -// Err: errs, -// Recommendation: "Namespace is missing, do you mean `oras test docker.io/library/alpine`?", -// }, -// }, -// } -// for _, tt := range tests { -// t.Run(tt.name, func(t *testing.T) { -// opts := &Target{ -// Remote: tt.fields.Remote, -// RawReference: tt.fields.RawReference, -// Type: tt.fields.Type, -// Reference: tt.fields.Reference, -// Path: tt.fields.Path, -// IsOCILayout: tt.fields.IsOCILayout, -// } -// got := opts.Process(tt.args.err, tt.args.callPath) -// if !reflect.DeepEqual(got, tt.want) { -// t.Errorf("Target.Process() = %v, want %v", got, tt.want) -// } -// }) -// } -// } +func TestTarget_Modify(t *testing.T) { + type fields struct { + Remote Remote + RawReference string + Type string + Reference string + Path string + IsOCILayout bool + } + errs := errcode.Errors{ + errcode.Error{ + Code: "000", + Message: "mocked message", + Detail: map[string]string{"mocked key": "mocked value"}, + }, + } + tests := []struct { + name string + fields fields + err error + modifiedErr *oerrors.Error + }{ + { + "namespace already exists", + fields{RawReference: "docker.io/library/alpine:latest"}, + &errcode.ErrorResponse{ + URL: &url.URL{Host: "registry-1.docker.io"}, + StatusCode: http.StatusUnauthorized, + Errors: errs, + }, + &oerrors.Error{Err: errs}, + }, + { + "no namespace", + fields{RawReference: "docker.io"}, + &errcode.ErrorResponse{ + URL: &url.URL{Host: "registry-1.docker.io"}, + StatusCode: http.StatusUnauthorized, + Errors: errs, + }, + &oerrors.Error{Err: errs}, + }, + { + "not 401", + fields{RawReference: "docker.io"}, + &errcode.ErrorResponse{ + URL: &url.URL{Host: "registry-1.docker.io"}, + StatusCode: http.StatusConflict, + Errors: errs, + }, + &oerrors.Error{Err: errs}, + }, + { + "should hint", + fields{ + RawReference: "docker.io/alpine", + Path: "oras test", + }, + &errcode.ErrorResponse{ + URL: &url.URL{Host: "registry-1.docker.io"}, + StatusCode: http.StatusUnauthorized, + Errors: errs, + }, + &oerrors.Error{ + Err: errs, + Recommendation: "Namespace is missing, do you mean ` docker.io/library/alpine`?", + }, + }, + } + + cmd := &cobra.Command{} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + opts := &Target{ + Remote: tt.fields.Remote, + RawReference: tt.fields.RawReference, + Type: tt.fields.Type, + Reference: tt.fields.Reference, + Path: tt.fields.Path, + IsOCILayout: tt.fields.IsOCILayout, + } + got, modified := opts.Modify(cmd, tt.err) + gotErr, ok := got.(*oerrors.Error) + if !ok { + t.Errorf("expecting error to be *oerrors.Error but received %T", got) + } + if !reflect.DeepEqual(gotErr.Err, tt.modifiedErr.Err) || gotErr.Usage != tt.modifiedErr.Usage || gotErr.Recommendation != tt.modifiedErr.Recommendation { + t.Errorf("Target.Modify() error = %v, wantErr %v", gotErr, tt.modifiedErr) + } + if !modified { + t.Errorf("Failed to modify %v", tt.err) + } + }) + } +} From ff6a9bafe052d607722e200ace75c2147c1a7e58 Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Thu, 11 Jan 2024 05:30:18 +0000 Subject: [PATCH 35/42] cover more Signed-off-by: Billy Zha --- cmd/oras/internal/option/target_test.go | 41 ++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/cmd/oras/internal/option/target_test.go b/cmd/oras/internal/option/target_test.go index bbad419fb..c021b86eb 100644 --- a/cmd/oras/internal/option/target_test.go +++ b/cmd/oras/internal/option/target_test.go @@ -99,7 +99,46 @@ func TestTarget_Modify_ociLayout(t *testing.T) { } } -func TestTarget_Modify(t *testing.T) { +func TestTarget_Modify_errInvalidReference(t *testing.T) { + errClient := errors.New("client error") + opts := &Target{ + IsOCILayout: true, + RawReference: "invalid-reference", + } + got, modified := opts.Modify(&cobra.Command{}, errClient) + + if modified { + t.Errorf("expect error not to be modified but received true") + } + if got != errClient { + t.Errorf("unexpected output from Target.Process() = %v", got) + } +} + +func TestTarget_Modify_errHostNotMatching(t *testing.T) { + errResp := &errcode.ErrorResponse{ + URL: &url.URL{Host: "registry-1.docker.io"}, + StatusCode: http.StatusUnauthorized, + Errors: errcode.Errors{ + errcode.Error{ + Code: "000", + Message: "mocked message", + Detail: map[string]string{"mocked key": "mocked value"}, + }, + }, + } + + opts := &Target{ + IsOCILayout: true, + RawReference: "registry-2.docker.io/test:tag", + } + _, modified := opts.Modify(&cobra.Command{}, errResp) + if modified { + t.Errorf("expect error not to be modified but received true") + } +} + +func TestTarget_Modify_dockerHint(t *testing.T) { type fields struct { Remote Remote RawReference string From b12d29ec6b88f2f27acb49f578c2c3e512a90810 Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Thu, 11 Jan 2024 05:31:54 +0000 Subject: [PATCH 36/42] code clean Signed-off-by: Billy Zha --- cmd/oras/internal/option/remote.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/cmd/oras/internal/option/remote.go b/cmd/oras/internal/option/remote.go index 67df6ab90..255393934 100644 --- a/cmd/oras/internal/option/remote.go +++ b/cmd/oras/internal/option/remote.go @@ -329,10 +329,6 @@ func (opts *Remote) isPlainHttp(registry string) bool { // Modify modifies error during cmd execution. func (opts *Remote) Modify(cmd *cobra.Command, err error) (error, bool) { - if errors.Is(err, errdef.ErrNotFound) { - cmd.SetErrPrefix(oerrors.RegistryErrorPrefix) - return err, true - } var errResp *errcode.ErrorResponse if errors.As(err, &errResp) { cmd.SetErrPrefix(oerrors.RegistryErrorPrefix) From ce2582aa419b976e1dc7550ca77a7892d23c1bba Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Thu, 11 Jan 2024 06:01:52 +0000 Subject: [PATCH 37/42] add TODO Signed-off-by: Billy Zha --- cmd/oras/internal/errors/errors.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cmd/oras/internal/errors/errors.go b/cmd/oras/internal/errors/errors.go index afc83fbe2..b2b12e240 100644 --- a/cmd/oras/internal/errors/errors.go +++ b/cmd/oras/internal/errors/errors.go @@ -90,6 +90,8 @@ func Trim(err error, errResp *errcode.ErrorResponse) error { if len(inner) == 0 { return fmt.Errorf("recognizable error message not found: %w", errResp) } else { + // TODO: trim dedicated error type when + // https://github.com/oras-project/oras-go/issues/677 is done errContent := err.Error() errRespContent := errResp.Error() if idx := strings.Index(errContent, errRespContent); idx > 0 { From 7c9933062e4918b3ad5d7aa4ede33a698c564319 Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Thu, 11 Jan 2024 06:06:50 +0000 Subject: [PATCH 38/42] add coverage Signed-off-by: Billy Zha --- cmd/oras/internal/option/target_test.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/cmd/oras/internal/option/target_test.go b/cmd/oras/internal/option/target_test.go index c021b86eb..1006edf1c 100644 --- a/cmd/oras/internal/option/target_test.go +++ b/cmd/oras/internal/option/target_test.go @@ -86,9 +86,7 @@ func Test_parseOCILayoutReference(t *testing.T) { func TestTarget_Modify_ociLayout(t *testing.T) { errClient := errors.New("client error") - opts := &Target{ - IsOCILayout: true, - } + opts := &Target{} got, modified := opts.Modify(&cobra.Command{}, errClient) if modified { @@ -102,7 +100,6 @@ func TestTarget_Modify_ociLayout(t *testing.T) { func TestTarget_Modify_errInvalidReference(t *testing.T) { errClient := errors.New("client error") opts := &Target{ - IsOCILayout: true, RawReference: "invalid-reference", } got, modified := opts.Modify(&cobra.Command{}, errClient) From 162a9dc9e5d43a36a694b47b3cff73e6cba25f50 Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Thu, 11 Jan 2024 06:37:34 +0000 Subject: [PATCH 39/42] increase coverage Signed-off-by: Billy Zha --- cmd/oras/internal/option/target_test.go | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/cmd/oras/internal/option/target_test.go b/cmd/oras/internal/option/target_test.go index 1006edf1c..40a6f058f 100644 --- a/cmd/oras/internal/option/target_test.go +++ b/cmd/oras/internal/option/target_test.go @@ -98,16 +98,26 @@ func TestTarget_Modify_ociLayout(t *testing.T) { } func TestTarget_Modify_errInvalidReference(t *testing.T) { - errClient := errors.New("client error") + errResp := &errcode.ErrorResponse{ + URL: &url.URL{Host: "registry-1.docker.io"}, + StatusCode: http.StatusUnauthorized, + Errors: errcode.Errors{ + errcode.Error{ + Code: "000", + Message: "mocked message", + Detail: map[string]string{"mocked key": "mocked value"}, + }, + }, + } opts := &Target{ RawReference: "invalid-reference", } - got, modified := opts.Modify(&cobra.Command{}, errClient) + got, modified := opts.Modify(&cobra.Command{}, errResp) if modified { t.Errorf("expect error not to be modified but received true") } - if got != errClient { + if got != errResp { t.Errorf("unexpected output from Target.Process() = %v", got) } } @@ -126,7 +136,6 @@ func TestTarget_Modify_errHostNotMatching(t *testing.T) { } opts := &Target{ - IsOCILayout: true, RawReference: "registry-2.docker.io/test:tag", } _, modified := opts.Modify(&cobra.Command{}, errResp) From c15c3f5c63f4b02dc7fb9da9eda475a0624d46a4 Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Thu, 11 Jan 2024 10:37:23 +0000 Subject: [PATCH 40/42] refactor trimming Signed-off-by: Billy Zha --- cmd/oras/internal/errors/errors.go | 43 +++++++++++++++++++++--------- 1 file changed, 30 insertions(+), 13 deletions(-) diff --git a/cmd/oras/internal/errors/errors.go b/cmd/oras/internal/errors/errors.go index b2b12e240..11e66b4fc 100644 --- a/cmd/oras/internal/errors/errors.go +++ b/cmd/oras/internal/errors/errors.go @@ -84,24 +84,41 @@ func Command(cmd *cobra.Command, handler Modifier) *cobra.Command { return cmd } -// Trim trims the error response detail from error message. -func Trim(err error, errResp *errcode.ErrorResponse) error { - inner := errResp.Errors - if len(inner) == 0 { - return fmt.Errorf("recognizable error message not found: %w", errResp) - } else { - // TODO: trim dedicated error type when - // https://github.com/oras-project/oras-go/issues/677 is done - errContent := err.Error() - errRespContent := errResp.Error() - if idx := strings.Index(errContent, errRespContent); idx > 0 { - // remove HTTP related info - return fmt.Errorf("%s%w", errContent[:idx], inner) +// Trim tries to trim toTrim from err. +func Trim(err error, toTrim error) error { + var inner error + if errResp, ok := toTrim.(*errcode.ErrorResponse); ok { + if len(errResp.Errors) == 0 { + return fmt.Errorf("recognizable error message not found: %w", toTrim) } + inner = errResp.Errors + } else { + return err + } + + if rewrapped := reWrap(err, toTrim, inner); rewrapped != nil { + return rewrapped } return inner } +// reWrap re-wraps errA to errC and trims out errB, returns nil if scrub fails. +// +---------- errA ----------+ +// | +---- errB ----+ | +---- errA ----+ +// | | errC | | => | errC | +// | +--------------+ | +--------------+ +// +--------------------------+ +func reWrap(errA error, errB error, errC error) error { + // TODO: trim dedicated error type when + // https://github.com/oras-project/oras-go/issues/677 is done + contentA := errA.Error() + contentB := errB.Error() + if idx := strings.Index(contentA, contentB); idx > 0 { + return fmt.Errorf("%s%w", contentA[:idx], errC) + } + return nil +} + // NewErrEmptyTagOrDigest creates a new error based on the reference string. func NewErrEmptyTagOrDigest(ref registry.Reference) error { return NewErrEmptyTagOrDigestStr(ref.String()) From ddc979496cd31c299972ad205ace7733ae4a8c7e Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Mon, 15 Jan 2024 08:29:15 +0000 Subject: [PATCH 41/42] resolve comments Signed-off-by: Billy Zha --- cmd/oras/internal/errors/errors.go | 2 +- cmd/oras/internal/option/target.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/oras/internal/errors/errors.go b/cmd/oras/internal/errors/errors.go index 11e66b4fc..8af80303d 100644 --- a/cmd/oras/internal/errors/errors.go +++ b/cmd/oras/internal/errors/errors.go @@ -108,7 +108,7 @@ func Trim(err error, toTrim error) error { // | | errC | | => | errC | // | +--------------+ | +--------------+ // +--------------------------+ -func reWrap(errA error, errB error, errC error) error { +func reWrap(errA, errB, errC error) error { // TODO: trim dedicated error type when // https://github.com/oras-project/oras-go/issues/677 is done contentA := errA.Error() diff --git a/cmd/oras/internal/option/target.go b/cmd/oras/internal/option/target.go index bcf183da7..d4b8878fc 100644 --- a/cmd/oras/internal/option/target.go +++ b/cmd/oras/internal/option/target.go @@ -281,7 +281,7 @@ func (opts *Target) Modify(cmd *cobra.Command, err error) (error, bool) { if ref.Repository != "" && !strings.Contains(ref.Repository, "/") { // docker.io/xxx -> docker.io/library/xxx ref.Repository = "library/" + ref.Repository - ret.Recommendation = fmt.Sprintf("Namespace is missing, do you mean `%s %s`?", cmd.CommandPath(), ref) + ret.Recommendation = fmt.Sprintf("Namespace seems missing. Do you mean `%s %s`?", cmd.CommandPath(), ref) } } return ret, true From e602b8980feacd5a791e38fb9a21de2976a3d9f7 Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Mon, 15 Jan 2024 09:02:55 +0000 Subject: [PATCH 42/42] fix unit test Signed-off-by: Billy Zha --- cmd/oras/internal/option/target_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/oras/internal/option/target_test.go b/cmd/oras/internal/option/target_test.go index 40a6f058f..8ab6f8459 100644 --- a/cmd/oras/internal/option/target_test.go +++ b/cmd/oras/internal/option/target_test.go @@ -209,7 +209,7 @@ func TestTarget_Modify_dockerHint(t *testing.T) { }, &oerrors.Error{ Err: errs, - Recommendation: "Namespace is missing, do you mean ` docker.io/library/alpine`?", + Recommendation: "Namespace seems missing. Do you mean ` docker.io/library/alpine`?", }, }, }