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

Use Docker credentials file to log in to registry #461

Merged
merged 5 commits into from
Jan 20, 2023

Conversation

guineveresaenger
Copy link
Contributor

Fixes #442.

This PR includes a slight tweak to the schema, requiring only the server field of the registry type to be required when using registry.
SDKs were regenerated accordingly.
Adds new logic to use Docker config file credentials when username and passworkd are not provided via the Pulumi program.

  • Do not make registry Username and Password required
  • Enable use of Docker configuration for push registry authentication

When we obtain authentication for Docker registries from the
environment, we only need to set the registry server field.
We've used Docker api auth type to authenticate a Buildkit session.
For pushing to the registry, we can use this as well, using the registry
servername provided in the pulumi program as the lookup for which
registry to authenticate against.
If the program or provider config supplies credentials (username and
password), we use those preferentially.
@guineveresaenger guineveresaenger requested review from AaronFriel and a team January 19, 2023 22:28
@github-actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

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

I left one nit. Otherwise LGTM!

Comment on lines +183 to +187
if img.Registry.Username != "" && img.Registry.Password != "" {
pushAuthConfig.Username = img.Registry.Username
pushAuthConfig.Password = img.Registry.Password
pushAuthConfig.ServerAddress = img.Registry.Server
} else {
Copy link
Member

Choose a reason for hiding this comment

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

Is it valid to pass a username without a password or vice versa? If not, do we warn?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

either is valid - neither field is required per the schema. If either field is empty, we default to the host credentials. 🤔 warning might make sense because using host credentials isn't as obvious. 👍

Copy link
Member

Choose a reason for hiding this comment

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

"valid" was perhaps the wrong word to use. What I should have said, and what I meant, was "does it ever make sense for the user to set a username without a password or vice versa."

My goal is to make sure that if the user does something that doesn't make sense, we will let them know.

@@ -208,7 +215,7 @@ func (p *dockerNativeProvider) dockerBuild(ctx context.Context,
if err != nil {
return "", nil, err
}
err = p.host.LogStatus(ctx, "info", urn, info)
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes - thank you for catching!

Copy link
Member

@t0yv0 t0yv0 left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

@github-actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

@github-actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

@guineveresaenger guineveresaenger merged commit a359430 into master Jan 20, 2023
@guineveresaenger guineveresaenger deleted the credential-login branch January 20, 2023 18:07
Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

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

Thanks for adding the warning.

Comment on lines +183 to +187
if img.Registry.Username != "" && img.Registry.Password != "" {
pushAuthConfig.Username = img.Registry.Username
pushAuthConfig.Password = img.Registry.Password
pushAuthConfig.ServerAddress = img.Registry.Server
} else {
Copy link
Member

Choose a reason for hiding this comment

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

"valid" was perhaps the wrong word to use. What I should have said, and what I meant, was "does it ever make sense for the user to set a username without a password or vice versa."

My goal is to make sure that if the user does something that doesn't make sense, we will let them know.

// send warning if user is attempting to use in-program credentials
if img.Registry.Username == "" && img.Registry.Password != "" {
msg := "username was not set, although password was; using host credentials file"
err = p.host.LogStatus(ctx, "warning", urn, msg)
Copy link
Member

Choose a reason for hiding this comment

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

My understanding is that LogStatus is a transient message, but I think a permanent message would be appropriate here. We want the warning to be discoverable by the user after the program has been run.

Suggested change
err = p.host.LogStatus(ctx, "warning", urn, msg)
err = p.host.Log(ctx, "warning", urn, msg)

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

Successfully merging this pull request may close these issues.

Provider authentication from Docker config
3 participants