Add support for AAD auth for docker-acr #395

rutvijmehta-harness · 2023-08-15T07:48:41Z

This PR adds support for AAD auth for docker-acr. The previous implementation assumes admin credentials and uses them as username and password. However that may not be the case always.

The new implementation uses clientId, clientSecret / clientCert, tenantId to get the ACR token which can be used as password along with the default username 00000000-0000-0000-0000-000000000000. If subscriptionId is provided, it also retrieves a url to view the artifact. If not provided, the registry url is used.

Screenshot of how the artifact would look using the above url:

rutvijmehta-harness · 2023-08-15T07:52:49Z

cmd/drone-docker/main.go

+			Name:   "daemon.registry",
+			Usage:  "daemon registry",
+			Value:  "https://index.docker.io/v1/",
+			EnvVar: "DAEMON_REGISTRY,PLUGIN_REGISTRY,DOCKER_REGISTRY",


For non-acr builds, DAEMON_REGISTRY will not be set and we will use PLUGIN_REGISTRY

i am not particularly happy with this, as we are changing the default behaviour for the "docker.registry" why dont we just use the env variable PLUGIN_REGISTRY like we do for the other providers.

or is DAEMON_REGISTRY something totally new ?

We have a hard assumption that docker.registry will point to a page where we can view the artifact. This is not the case for ACR. We get the url from Azure using the API implemented in the PR.

For kaniko, we have a separate struct for artifact, which is why it is straight forward there.

Ref:

https://github.com/drone/drone-kaniko/blob/main/kaniko.go#L239

https://github.com/drone/drone-kaniko/blob/main/cmd/kaniko-acr/main.go#L261

We cannot update PLUGIN_REGISTRY as the push still needs to go to the input registry.

I'm okay with adding ArtifactRegistry field to docker.Daemon and use it while writing the artifact file.

drone.WritePluginArtifactFile(p.Daemon.RegistryType, p.ArtifactFile, p.Daemon.ArtifactRegistry, p.Build.Repo, digest, p.Build.Tags)

i was thinking of keeping the changes to main.go as https://github.com/drone-plugins/drone-docker/compare/acr...tphoney:drone-docker:acr?expand=1 and then adding logic into acr for what ever is needed.
changing the handling inside of main.go and overriding what is standard is dangerous.
inside of the acr we can change as much as necessary.

Moved to use p.Daemon.ArtifactRegistry which is a different path and not standard: c64f19d

Please approve and merge if it looks okay.

rutvijmehta-harness · 2023-08-15T08:05:04Z

@tphoney we might have to update the go version (1.18+) for the azure sdk to work.

go vet ./... is failing in drone (go version 1.17) but passing on local (go version 1.20)

Ref:

[email protected] "build constraints exclude all Go files" due to // +build go1.18 Azure/azure-sdk-for-go#17472
blob/azblob: Migrate to new SDK module google/go-cloud#3141 (comment)

Let me know if that's okay. I'll update the go version in drone yml.

tphoney · 2023-08-15T12:20:05Z

@rutvijmehta-harness can you bump the go version, as you suggested in the go.mod file and the .drone.yml file in a separate commit .
I will look at the test of the pr now

tphoney · 2023-08-15T13:03:22Z

cmd/drone-docker/main.go

+			Name:   "daemon.registry",
+			Usage:  "daemon registry",
+			Value:  "https://index.docker.io/v1/",
+			EnvVar: "DAEMON_REGISTRY,PLUGIN_REGISTRY,DOCKER_REGISTRY",


i am not particularly happy with this, as we are changing the default behaviour for the "docker.registry" why dont we just use the env variable PLUGIN_REGISTRY like we do for the other providers.

or is DAEMON_REGISTRY something totally new ?

eoinmcafee00 · 2023-08-17T10:09:00Z

cmd/drone-acr/main.go

@@ -62,3 +263,9 @@ func getenv(key ...string) (s string) {
 	}
 	return
 }
+
+type strct struct {


Naming? What exactly is this?

This code was taken from drone-kaniko: https://github.com/drone/drone-kaniko/blob/main/cmd/kaniko-acr/main.go#L442

Once the implementation is approved, I can make this change and take approval from US team and merge this PR.

tphoney

This is still not good enough.
my objection is to the changes in the drone-docker/main.go adding artifact.registry EnvVar: "ARTIFACT_REGISTRY,PLUGIN_REGISTRY,DOCKER_REGISTRY", make it EnvVar: "ARTIFACT_REGISTRY". What would happen if a user passes those 3 env variables ?

the changing of docker.go
fmt.Printf("Got artifact reg %s and digest %s\n", p.Daemon.ArtifactRegistry, digest) will only work for ACR, not other docker registrys. thus printing out broken information for gcr/docker ...

if you want to make these kinds of changes to core parts of the docker plugin. You will need to test against docker / acr / gcr / ecr / heroku.

please only make additive changes to drone-docker and docker.go. Additive changes are fine.
but what you are doing now only works for acr, and may have unforseen affects on the other registry types.

rutvijmehta-harness · 2023-08-17T18:29:56Z

This is still not good enough. my objection is to the changes in the drone-docker/main.go adding artifact.registry EnvVar: "ARTIFACT_REGISTRY,PLUGIN_REGISTRY,DOCKER_REGISTRY", make it EnvVar: "ARTIFACT_REGISTRY". What would happen if a user passes those 3 env variables ?

ARTIFACT_REGISTRY would take precedence. Why would user set this to the env if drone-docker does not support it? This will be true for any new env variables we add.

the changing of docker.go fmt.Printf("Got artifact reg %s and digest %s\n", p.Daemon.ArtifactRegistry, digest) will only work for ACR, not other docker registrys. thus printing out broken information for gcr/docker ...

This was only for debugging. I'll remove it.

if you want to make these kinds of changes to core parts of the docker plugin. You will need to test against docker / acr / gcr / ecr / heroku.

I've already tested against docker. We have the fallback env variable PLUGIN_REGISTRY which will take care of any regression.

please only make additive changes to drone-docker and docker.go. Additive changes are fine. but what you are doing now only works for acr, and may have unforseen affects on the other registry types.

Why do you think it only works for acr?

This reverts commit be7bda4.

Add support for AAD auth for docker-acr

d3c1ebe

rutvijmehta-harness commented Aug 15, 2023

View reviewed changes

rutvijmehta-harness requested review from raghavharness and tphoney August 15, 2023 07:53

tphoney suggested changes Aug 15, 2023

View reviewed changes

rutvijmehta-harness added 2 commits August 15, 2023 12:48

Update go version

c0927ac

Add Artifact Registry to the Daemon struct

c64f19d

eoinmcafee00 reviewed Aug 17, 2023

View reviewed changes

tphoney suggested changes Aug 17, 2023

View reviewed changes

Address comments

53c630c

rutvijmehta-harness requested a review from tphoney August 21, 2023 21:31

Update docker.go

5fbbde3

tphoney approved these changes Aug 22, 2023

View reviewed changes

tphoney merged commit be7bda4 into master Aug 22, 2023

delete-merged-branch bot deleted the acr branch August 22, 2023 11:07

tphoney pushed a commit that referenced this pull request Aug 25, 2023

Revert "Add support for AAD auth for docker-acr (#395)"

3623071

This reverts commit be7bda4.

tphoney mentioned this pull request Aug 25, 2023

Revert "Add support for AAD auth for docker-acr" #398

Merged

tphoney pushed a commit that referenced this pull request Aug 25, 2023

Revert "Add support for AAD auth for docker-acr (#395)" (#398)

8734c9c

This reverts commit be7bda4.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for AAD auth for docker-acr #395

Add support for AAD auth for docker-acr #395

rutvijmehta-harness commented Aug 15, 2023 •

edited

Loading

rutvijmehta-harness Aug 15, 2023

tphoney Aug 15, 2023

rutvijmehta-harness Aug 15, 2023

rutvijmehta-harness Aug 15, 2023

rutvijmehta-harness Aug 15, 2023 •

edited

Loading

tphoney Aug 16, 2023

rutvijmehta-harness Aug 16, 2023

rutvijmehta-harness commented Aug 15, 2023

tphoney commented Aug 15, 2023

tphoney Aug 15, 2023

eoinmcafee00 Aug 17, 2023

rutvijmehta-harness Aug 17, 2023

tphoney left a comment

rutvijmehta-harness commented Aug 17, 2023

Add support for AAD auth for docker-acr #395

Add support for AAD auth for docker-acr #395

Conversation

rutvijmehta-harness commented Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rutvijmehta-harness Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rutvijmehta-harness commented Aug 15, 2023

tphoney commented Aug 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tphoney left a comment

Choose a reason for hiding this comment

rutvijmehta-harness commented Aug 17, 2023

rutvijmehta-harness commented Aug 15, 2023 •

edited

Loading

rutvijmehta-harness Aug 15, 2023 •

edited

Loading