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

ACR Wizard fix, Please allow me to set --admin-enabled to true as part of creating a ACR through Docker Extension wizard #1668

Closed
softchris opened this issue Feb 26, 2020 · 11 comments

Comments

@softchris
Copy link

I want to deploy an image:

To do that from scratch I need to

  • Create an ACR through the tool
  • Push local image to ACR
  • Right-click image in ACR and choose to create a Web App

This won't work as --admin-enabled is false when you create it through the Docker Extension

Suggestion:

  • Make --admin-enabled a question in the wizard and set it to true by default, maybe an info even that says it needs to be true in order to deploy images from it, most users will want to deploy from it, is my guess

Now:
I remove the ACR created through the Docker Extension
I recreate an ACR through azure-cli

This is a really bad UX experience in my mind, please fix

@dbreshears dbreshears added this to the 1.1.0 milestone Feb 26, 2020
@karolz-ms
Copy link
Contributor

Admin user is generally not recommended, there are better options for authenticating to ACR https://docs.microsoft.com/en-us/azure/container-registry/container-registry-authentication#authentication-options

Also, admin user can be enabled after the ACR has been created https://docs.microsoft.com/en-us/azure/container-registry/container-registry-authentication#admin-account

Suggest "won't fix"

@bwateratmsft
Copy link
Collaborator

@karolz-ms Isn't admin user required to make our extension work?

@karolz-ms
Copy link
Contributor

@bwateratmsft nope, I have an ACR that does not have admin user enabled and it works just fine

@bwateratmsft
Copy link
Collaborator

bwateratmsft commented Feb 26, 2020

Ah, I see. It is needed for deploy app service though, which is the scenario the user was talking about. https://github.com/microsoft/vscode-docker/blob/master/src/commands/registries/azure/deployImageToAzure.ts#L95

@karolz-ms
Copy link
Contributor

@bwateratmsft What you showed here is probably a bug in our code--I would be surprised if an AAD service principal with pull authorization would not work equally well as admin user.

For more on service principal authN with ACR see https://docs.microsoft.com/en-us/azure/container-registry/container-registry-auth-kubernetes (disregard the K8s aspect and just note that all it is necessary to pull images is registry URL, service principal ID and password)

@softchris
Copy link
Author

https://docs.microsoft.com/en-us/azure/container-registry/container-registry-authentication#admin-account

Can we please have that as quick info on how to fix the ACR after you have created it then?. I mean on hover. Right now it says something like admin is not enabled, tough luck. I appreciate the link above but I couldn't find how to fix it so I'm really grateful you were able to provide the link above inside of VS Code, just to avoid the user feels like they got a "go fish" answer?

@karolz-ms
Copy link
Contributor

https://docs.microsoft.com/en-us/azure/container-registry/container-registry-authentication#admin-account describes how to enable admin account after ACR is created. Does this help?

@berndverst
Copy link
Member

berndverst commented Feb 28, 2020

@karolz-ms if the extension can facilitate the following admin-enabled won't be necessary

  1. Create Managed Identity for Web App
  2. Assign ACR Pull role to that identity for the ACR in question.

Example for my own app and registry using the CLI:

# Create and assign a managed Service Principal to a Web App
az webapp identity assign --resource-group beverst_rg_Linux_westus2 --name rockpaperscissorslizardspock
# Give webapp identity access to pull from ACR
az role assignment create --assignee 01eaae16-c9a7-4eba-bf27-05820c4fc163 --scope /subscriptions/59985d08-67e1-493c-82c4-7ad711fdba2b/resourceGroups/kubeflowrelease/providers/Microsoft.ContainerRegistry/registries/kubeflowregistry --role "AcrPull"

References:
https://docs.microsoft.com/azure/active-directory/managed-identities-azure-resources/services-support-managed-identities#azure-services-that-support-managed-identities-for-azure-resources#azure-app-service
https://docs.microsoft.com/azure/container-registry/container-registry-authentication-managed-identity#example-2-access-with-a-system-assigned-identity

@berndverst
Copy link
Member

@karolz-ms by the way I think what @softchris is getting at is that you need to use the CLI to modify the ACR created through the extension, when the extension should allow it or guide the correct choices for this use case supported by the extension.

Of course using the CLI az acr update -n <acrName> --admin-enabled true is possible.

If you implement the AAD approach I outlined in my previous comment we won't need to insecurely enable admin everywhere.

@berndverst
Copy link
Member

For better tracking I created #1685

@karolz-ms
Copy link
Contributor

@berndverst thank you and yes, agreed. The manual admin mode enablement was just a workaround till we fix the ACR creation scenario comprehensively.

@bwateratmsft bwateratmsft removed this from the 1.1.0 milestone Mar 9, 2020
@vscodebot vscodebot bot locked and limited conversation to collaborators Apr 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants