Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bad error message when not logged in to container registry #6297

Closed
rynowak opened this issue May 24, 2023 · 11 comments
Closed

Bad error message when not logged in to container registry #6297

rynowak opened this issue May 24, 2023 · 11 comments
Labels
good first issue Good for newcomers

Comments

@rynowak
Copy link
Contributor

rynowak commented May 24, 2023

Bug information

Steps to reproduce (required)

  • Let your ACR login expire
  • Run the following command
rad bicep publish --file test/functional/corerp/resources/testdata/recipes/test-recipes/dapr-state-store.bicep --target br:rynowak.azurecr.io/test/functional/corerp/recipes/dapr-state-store:latest

Any source and destination are fine, as long as you are not logged in to the destination.

Observed behavior (required)

Building test/functional/corerp/resources/testdata/recipes/test-recipes/dapr-state-store.bicep...
Error: Failed to publish Bicep file: HEAD "https://rynowak.azurecr.io/v2/test/functional/corerp/recipes/dapr-state-store/manifests/sha256:10131c538619da7466e8ee9fbf573295cecfab81cd46b544bfbe058cd3347cee": POST "https://rynowak.azurecr.io/oauth2/token": response status code 401: Unauthorized

Desired behavior (required)

A better error message, that explains the problem and tells you how to log in. If I were an external customer I wouldn't be sure how to fix this.

AB#7968

AB#9502

AB#10830

@rynowak
Copy link
Contributor Author

rynowak commented May 24, 2023

/cc @ytimocin

@shalabhms shalabhms added the good first issue Good for newcomers label May 25, 2023
@willtsai willtsai transferred this issue from radius-project/radius Sep 19, 2023
@AaronCrawfis AaronCrawfis transferred this issue from another repository Sep 19, 2023
@lechnerc77
Copy link
Contributor

lechnerc77 commented Dec 21, 2023

Is this already fixed? I just retried it and the result is:

Building test/functional/samples/testdata/tutorial-environment.bicep...
Failed to publish Bicep file "test/functional/samples/testdata/tutorial-environment.bicep" to "rynowak.azurecr.io/test/functional/corerp/recipes/dapr-state-store:latest". Cause: POST "https://rynowak.azurecr.io/v2/test/functional/corerp/recipes/dapr-state-store/blobs/uploads/": response status code 401: unauthorized: authentication required, visit https://aka.ms/acr/authorization for more information.: [map[Action:pull Name:test/functional/corerp/recipes/dapr-state-store Type:repository] map[Action:push Name:test/functional/corerp/recipes/dapr-state-store Type:repository]].

Still quite some text, but the reference to https://aka.ms/acr/authorization is helpful, so wondering if the error message should be further improved?

@rynowak
Copy link
Contributor Author

rynowak commented Dec 21, 2023

The error message coming from ACR does include a link to the instructions, but most users don't notice when they hit this. The ACR team did a good job with the error message they provide, but I don't think that's the case for every registry.

We had a similar issue reported here: #6902 which is evidence that this is still confusing.


If you look at the code, we're not doing anything special to handle failed logins: https://github.com/radius-project/radius/blob/main/pkg/cli/cmd/bicep/publish/publish.go#L300

We're just trying to publish and then printing whatever error comes back from the server.


I think a good fix to this would attempt to authenticate up-front, and then "pretty print" the error that comes out using clierrors.MessageWithCause: https://github.com/radius-project/radius/blob/main/pkg/cli/clierrors/errors.go#L33

The error we print to the console should make a few things clear:

  • Logging into your container registry failed (and which registry).
  • You need to find instructions for your container registry and follow them.

Since users can use rad bicep publish with any OCI registry we can't build in knowledge of how login works to all of them. The users needs to do this separately from the rad CLI.

I think it some of these cases users aren't aware of what's happening behind the scenes (pulling/pushing from an OCI registry) so the error message is confusing.

@rynowak
Copy link
Contributor Author

rynowak commented Dec 21, 2023

/cc @ytimocin in case you have additional thoughts

@ytimocin
Copy link
Contributor

Sounds good to me. The result of the call to target.Push should be "prettified" in a way that the Radius user can understand. cc/ @rynowak

@rynowak
Copy link
Contributor Author

rynowak commented Dec 21, 2023

Sounds good to me. The result of the call to target.Push should be "prettified" in a way that the Radius user can understand. cc/ @rynowak

I think that's a good option too. I was thinking we'd explicitly try to auth (new code) so we know whether it not it succeeds easily.

@lechnerc77
Copy link
Contributor

Thanks for the input. If that's okay, I would try to provide a fix for that.

@gpltaylor
Copy link
Contributor

Is this already fixed? I just retried it and the result is:

Building test/functional/samples/testdata/tutorial-environment.bicep...
Failed to publish Bicep file "test/functional/samples/testdata/tutorial-environment.bicep" to "rynowak.azurecr.io/test/functional/corerp/recipes/dapr-state-store:latest". Cause: POST "https://rynowak.azurecr.io/v2/test/functional/corerp/recipes/dapr-state-store/blobs/uploads/": response status code 401: unauthorized: authentication required, visit https://aka.ms/acr/authorization for more information.: [map[Action:pull Name:test/functional/corerp/recipes/dapr-state-store Type:repository] map[Action:push Name:test/functional/corerp/recipes/dapr-state-store Type:repository]].

Still quite some text, but the reference to https://aka.ms/acr/authorization is helpful, so wondering if the error message should be further improved?

This URL is useless. It doesn't document how to login to the ACR.

@rynowak
Copy link
Contributor Author

rynowak commented Dec 26, 2023

Still quite some text, but the reference to https://aka.ms/acr/authorization is helpful, so wondering if the error message should be further improved?

This URL is useless. It doesn't document how to login to the ACR.

Good to know, I didn't click it 😆

@lechnerc77 - looks like @gpltaylor already started working on a fix for this here: #6974 (comment)

rynowak added a commit that referenced this issue Jan 7, 2024
# Description

Improve error message returned when publishing to a private ACR

## Type of change

<!--

Please select **one** of the following options that describes your
change and delete the others. Clearly identifying the type of change you
are making will help us review your PR faster, and is used in authoring
release notes.

If you are making a bug fix or functionality change to Radius and do not
have an associated issue link please create one now.

-->

- 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).

<!--

Please update the following to link the associated issue. This is
required for some kinds of changes (see above).

-->

Fixes: #6902 #6297

## Auto-generated summary

<!--
GitHub Copilot for docs will auto-generate a summary of the PR
-->

copilot:all

---------

Signed-off-by: Garry Taylor <[email protected]>
Co-authored-by: Ryan Nowak <[email protected]>
@rynowak
Copy link
Contributor Author

rynowak commented Jan 7, 2024

Fixed by #6974

@rynowak rynowak closed this as completed Jan 7, 2024
@rynowak
Copy link
Contributor Author

rynowak commented Jan 7, 2024

Thanks @gpltaylor !

willdavsmith pushed a commit to willdavsmith/radius that referenced this issue Jan 17, 2024
…ect#6974)

# Description

Improve error message returned when publishing to a private ACR

## Type of change

<!--

Please select **one** of the following options that describes your
change and delete the others. Clearly identifying the type of change you
are making will help us review your PR faster, and is used in authoring
release notes.

If you are making a bug fix or functionality change to Radius and do not
have an associated issue link please create one now.

-->

- 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).

<!--

Please update the following to link the associated issue. This is
required for some kinds of changes (see above).

-->

Fixes: radius-project#6902 radius-project#6297

## Auto-generated summary

<!--
GitHub Copilot for docs will auto-generate a summary of the PR
-->

copilot:all

---------

Signed-off-by: Garry Taylor <[email protected]>
Co-authored-by: Ryan Nowak <[email protected]>
Signed-off-by: willdavsmith <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

5 participants