Skip to content
This repository has been archived by the owner on Jan 8, 2024. It is now read-only.

docker: support for registry auth in deploy #2245

Closed
wants to merge 2 commits into from
Closed

Conversation

kam1sh
Copy link

@kam1sh kam1sh commented Sep 7, 2021

Hi! This pull request adds encoded_auth setting for docker deploy.

Usage:

  deploy {
    use "docker" {
      encoded_auth = base64encode(
        jsonencode({"username": var.dockeruser, "password" : var.dockerpassword})
      )
    }
  }

closes #2243

@hashicorp-cla
Copy link

hashicorp-cla commented Sep 7, 2021

CLA assistant check
All committers have signed the CLA.

@izaaklauer
Copy link
Contributor

Hi kam1sh,

Thanks for addressing this! This is a good start, but take a look at the authentication setup that we do in docker pull here. There's more work that would need to be done to read from the docker config, for example.

@kam1sh
Copy link
Author

kam1sh commented Sep 8, 2021

I moved authorization code from pushWithDocker to findAuth function, and I reused this code in platform:pullImage:
https://github.com/hashicorp/waypoint/pull/2245/files#diff-9e5a60fe38fb5bbe6aeb7810899f4bf9d43cac4da6971124bc8d2e16c6f9ba75R737

@kam1sh
Copy link
Author

kam1sh commented Sep 8, 2021

Test failed? Weird, I didn't touched this code...

@kam1sh kam1sh changed the title docker: support for encoded_auth in deploy docker: support for registry auth in deploy Sep 9, 2021
@kam1sh
Copy link
Author

kam1sh commented Sep 9, 2021

@izaaklauer can you help me with failing tests, please?

@izaaklauer izaaklauer self-assigned this Sep 10, 2021
Copy link
Contributor

@izaaklauer izaaklauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! A few more changes we'd recommend:

  • Update the docker platform config to accept encoded_auth. Right now, if I try adding encoded_auth to the docker deploy stanza of waypoing.hcl, the deployment fails with Unsupported argument; An argument named "encoded_auth" is not expected here..

  • I'd recommend that you refactor findAuth to be static, so you don't need to instantiate and manually configure the Registry struct just to use this one function.

Your test failure was a flake on our end - we re-ran the tests and they were successful.

@@ -406,7 +406,7 @@ func (p *Platform) resourceContainerCreate(
netState *Resource_Network,
) error {
// Pull the image
err := p.pullImage(cli, log, ui, img, p.config.ForcePull)
err := p.pullImage(cli, ctx, log, ui, img, p.config.ForcePull)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could make ctx the first argument? That's our convention.

@@ -83,7 +122,7 @@ func (r *Registry) pushWithDocker(
// Resolve the Repository name from fqn to RepositoryInfo
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this is the only part of the findAuth function that's currently being used in pullImage, because you're not setting r.config.EncodedAuth or r.config.Password.

Copy link
Contributor

@izaaklauer izaaklauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies, that last comment should have been "Request Changes", not "Approve".

@evanphx
Copy link
Contributor

evanphx commented Jan 19, 2022

This will be resolved by #2895

@xiaolin-ninja
Copy link
Contributor

Closed by #2895

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

deploy with docker from private registry does not honor docker config
6 participants