From 9d02dabfe0f1c58d39e8370a8ec1c2803df3a484 Mon Sep 17 00:00:00 2001 From: Garry Taylor Date: Sun, 7 Jan 2024 18:06:35 +0000 Subject: [PATCH] Improve error message under 401 condition with Azure ACR (#6974) # Description Improve error message returned when publishing to a private ACR ## Type of change - This pull request is a minor refactor, code cleanup, test improvement, or other maintenance task and doesn't change the functionality of Radius (issue link optional). Fixes: #6902 #6297 ## Auto-generated summary copilot:all --------- Signed-off-by: Garry Taylor Co-authored-by: Ryan Nowak Signed-off-by: willdavsmith --- pkg/cli/clierrors/types.go | 4 +- pkg/cli/cmd/bicep/publish/publish.go | 34 +++++++++++++++- pkg/cli/cmd/bicep/publish/publish_test.go | 49 +++++++++++++++++++++++ 3 files changed, 84 insertions(+), 3 deletions(-) diff --git a/pkg/cli/clierrors/types.go b/pkg/cli/clierrors/types.go index d81f0453b8a..46b273c9e62 100644 --- a/pkg/cli/clierrors/types.go +++ b/pkg/cli/clierrors/types.go @@ -16,6 +16,8 @@ limitations under the License. package clierrors +import "strings" + // FriendlyError defines an interface for errors that should be gracefully handled by the CLI and // display a friendly error message to the user. type FriendlyError interface { @@ -42,7 +44,7 @@ func (e *ErrorMessage) Error() string { return e.Message } - return e.Message + " Cause: " + e.Cause.Error() + "." + return e.Message + " Cause: " + strings.TrimSpace(e.Cause.Error()) + "." } // IsFriendlyError returns true for ErrorMessage. These errors are always handled gracefully by the CLI. diff --git a/pkg/cli/cmd/bicep/publish/publish.go b/pkg/cli/cmd/bicep/publish/publish.go index fcbd16234b3..b03bdf52e47 100644 --- a/pkg/cli/cmd/bicep/publish/publish.go +++ b/pkg/cli/cmd/bicep/publish/publish.go @@ -20,6 +20,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "strings" @@ -30,6 +31,8 @@ import ( "github.com/radius-project/radius/pkg/cli/framework" "github.com/radius-project/radius/pkg/cli/output" + "net/http" + "github.com/opencontainers/go-digest" specs "github.com/opencontainers/image-spec/specs-go" ocispec "github.com/opencontainers/image-spec/specs-go/v1" @@ -39,6 +42,7 @@ import ( "oras.land/oras-go/v2/registry" "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" ) @@ -167,14 +171,40 @@ func (r *Runner) Run(ctx context.Context) error { r.Destination = dest err = r.publish(ctx) - if err != nil { - return clierrors.MessageWithCause(err, "Failed to publish Bicep file %q to %q.", r.File, r.Target) + var httpErr *errcode.ErrorResponse + if errors.As(err, &httpErr) { + message := fmt.Sprintf("Failed to publish Bicep file %q to %q", r.File, r.Target) + return handleErrorResponse(httpErr, message) + } else if err != nil { + return clierrors.MessageWithCause(err, "Failed to publish Bicep file %q to %q", r.File, r.Target) } r.Output.LogInfo("Successfully published Bicep file %q to %q", r.File, r.Target) return nil } +func handleErrorResponse(httpErr *errcode.ErrorResponse, message string) error { + acrInfo := "" + if strings.Contains(httpErr.URL.Host, "azurecr.io") { + acrInfo = "For more details visit: https://learn.microsoft.com/en-us/azure/container-registry/container-registry-authentication?tabs=azure-cli" + } + + switch httpErr.StatusCode { + case http.StatusUnauthorized: + if acrInfo == "" { + return clierrors.MessageWithCause(httpErr, "%s\nUnauthorized: Please login to %q", message, httpErr.URL.Host) + } else { + return clierrors.MessageWithCause(httpErr, "%s\nUnauthorized: Please login to %q\n%s", message, httpErr.URL.Host, acrInfo) + } + case http.StatusForbidden: + return clierrors.MessageWithCause(httpErr, "%s\nForbidden: You don't have permission to push to %q", message, httpErr.URL.Host) + case http.StatusNotFound: + return clierrors.MessageWithCause(httpErr, "%s\nNot Found: Unable to find registry %q", message, httpErr.URL.Host) + default: + return clierrors.MessageWithCause(httpErr, "%s\nSomething went wrong", message) + } +} + func (r *Runner) publish(ctx context.Context) error { // Prepare Source src, err := r.prepareSource(ctx) diff --git a/pkg/cli/cmd/bicep/publish/publish_test.go b/pkg/cli/cmd/bicep/publish/publish_test.go index 25c00d37b8b..462d3ad3cf6 100644 --- a/pkg/cli/cmd/bicep/publish/publish_test.go +++ b/pkg/cli/cmd/bicep/publish/publish_test.go @@ -19,6 +19,9 @@ package bicep import ( "context" "encoding/json" + "fmt" + "net/http" + "net/url" "reflect" "testing" @@ -32,6 +35,7 @@ import ( "oras.land/oras-go/v2/content/memory" "oras.land/oras-go/v2/registry" "oras.land/oras-go/v2/registry/remote" + "oras.land/oras-go/v2/registry/remote/errcode" ) func TestRunner_extractDestination(t *testing.T) { @@ -313,3 +317,48 @@ func TestRunner_Validate(t *testing.T) { } radcli.SharedValidateValidation(t, NewCommand, tests) } + +func getError(registerUrl string, statusCode int) *errcode.ErrorResponse { + + err := &errcode.ErrorResponse{ + URL: &url.URL{Host: registerUrl}, + StatusCode: statusCode, + } + + return err +} + +func TestHandleErrorResponse(t *testing.T) { + + httpErrA := getError("myregistry.azurecr.io", http.StatusUnauthorized) + httpErrB := getError("otherregistry.gcr.io", http.StatusUnauthorized) + httpErrC := getError("myregistry.azurecr.io", http.StatusForbidden) + httpErrD := getError("myregistry.azurecr.io", http.StatusNotFound) + httpErrE := getError("myregistry.azurecr.io", http.StatusInternalServerError) + + testCases := []struct { + httpErr *errcode.ErrorResponse + message string + expectedError string + }{ + {httpErrA, "Unauthorized - Includes Azure login info", fmt.Sprintf("Unauthorized: Please login to %q\nFor more details visit: https://learn.microsoft.com/en-us/azure/container-registry/container-registry-authentication?tabs=azure-cli Cause: \"//myregistry.azurecr.io\": response status code 401: Unauthorized.", httpErrA.URL.Host)}, + {httpErrB, "Standard unauthorized message", fmt.Sprintf("Unauthorized: Please login to %q Cause: %q: response status code 401: Unauthorized.", httpErrB.URL.Host, "//"+httpErrB.URL.Host)}, + {httpErrC, "Forbidden error", fmt.Sprintf("Forbidden: You don't have permission to push to %q Cause: %q: response status code 403: Forbidden.", httpErrC.URL.Host, "//"+httpErrC.URL.Host)}, + {httpErrD, "Not found error", fmt.Sprintf("Not Found: Unable to find registry %q Cause: %q: response status code 404: Not Found.", httpErrD.URL.Host, "//"+httpErrD.URL.Host)}, + {httpErrE, "Internal server error", fmt.Sprintf("Something went wrong Cause: %q: response status code 500: Internal Server Error.", "//"+httpErrE.URL.Host)}, + } + + for _, tc := range testCases { + t.Run(tc.message, func(t *testing.T) { + httpErr := &errcode.ErrorResponse{ + Method: tc.httpErr.Method, + URL: tc.httpErr.URL, + StatusCode: tc.httpErr.StatusCode, + } + + result := handleErrorResponse(httpErr, tc.message) + expected := fmt.Sprintf("%s\n%s", tc.message, tc.expectedError) + require.Equal(t, expected, result.Error()) + }) + } +}