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 env vars for registry credentials in kp import #279

Merged

Conversation

ncarlson
Copy link
Contributor

@ncarlson ncarlson commented Feb 7, 2023

kp import will attempt to read registry credentials from these environment variables as a fallback to docker/.config during kp import.

Examples:

	# Passing a single registry credential using environment variables
	REGISTRY_USER=bob REGISTRY_PASSWORD=ross REGISTRY_URL=single-registry.io \
	kp import -f descriptor.yaml

	# Passing multiple registry credentials using the _N suffix
	REGISTRY_USER_0=pablo REGISTRY_PASSWORD_0=picasso REGISTRY_URL_0=first-registry.io \
	REGISTRY_USER_1=henri REGISTRY_PASSWORD_1=matisse REGISTRY_URL_1=another-registry.io \
	kp import -f descriptor.yaml

kp import will attempt to read registry credentials from these
environment variables as a fallback to docker/.config during
`kp import`:
* REGISTRY_USER
* REGISTRY_PASSWORD
@ncarlson ncarlson linked an issue Feb 7, 2023 that may be closed by this pull request
@ncarlson ncarlson marked this pull request as draft February 7, 2023 15:17
Examples:
```
	# Passing multiple registry credentials using the _N suffix
	REGISTRY_USER_0=pablo REGISTRY_PASSWORD_0=picasso REGISTRY_URL_0=first-registry.io \
	REGISTRY_USER_1=henri REGISTRY_PASSWORD_1=matisse REGISTRY_URL_1=another-registry.io \
	kp import -f descriptor.yaml

	# Passing a single registry credential by omitting the _N suffix
	REGISTRY_USER=bob REGISTRY_PASSWORD=ross REGISTRY_URL=single-registry.io \
	kp import -f descriptor.yaml

```
@ncarlson ncarlson marked this pull request as ready for review February 7, 2023 19:55
go.mod Outdated Show resolved Hide resolved
pkg/import/helpers.go Outdated Show resolved Hide resolved
@tomkennedy513
Copy link
Contributor

We will also need to add docs to the help message so that users know how to utilize this feature

Copy link
Contributor

@chenbh chenbh left a comment

Choose a reason for hiding this comment

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

I agree with #251 (comment) that this should apply to every command that interacts with a registry

pkg/commands/import/import.go Outdated Show resolved Hide resolved
@ncarlson
Copy link
Contributor Author

dockercreds.NewKeychainFromDefaultEnvVarsWithDefault().Keychain is a bit of a mouthful, and any/all suggestions are welcome :-)

@ncarlson
Copy link
Contributor Author

We will also need to add docs to the help message so that users know how to utilize this feature

Addressing this now...

* Simplified access to new Keychain
* Renamed env vars to:
  - KP_REGISTRY_HOSTNAME
  - KP_REGISTRY_USERNAME
  - KP_REGISTRY_PASSWORD
* Removed superfluous keychain_helper wrapper and tests

Signed-off-by: Nicholas Carlson <carlsonn@vmware.com>
@ncarlson ncarlson force-pushed the 251-support-env-vars-for-registry-auth-when-executing-kp-import branch from 914a3f8 to 2b67240 Compare February 22, 2023 00:22
Copy link
Contributor

@tomkennedy513 tomkennedy513 left a comment

Choose a reason for hiding this comment

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

This is great!

@ncarlson ncarlson requested a review from chenbh February 22, 2023 16:58
@ncarlson ncarlson merged commit 0858848 into main Feb 22, 2023
Comment on lines +17 to +19
EnvVarRegistryUrl = "KP_REGISTRY_HOSTNAME"
EnvVarRegistryUser = "KP_REGISTRY_USERNAME"
EnvVarRegistryPassword = "KP_REGISTRY_PASSWORD"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these need to be exported?

Comment on lines +1 to +3
package dockercreds

var NewCredHelperFromEnvVars = newCredHelperFromEnvVars
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm reading this correctly, the whole point of this file is to expose the newCredHelperFromEnvVars function for tests? In that case a better idea is to just use package dockercreds in cred_helper_test.go.

The way go works, this would expose the function to any tests, including ones from outside this module.

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, I've found testing to be somewhat inconsistent across even files of the same project.

The rule of thumb is that if you purely want to test public functions, then you can use package my_module_test, if you want to test internals, then it's perfectly fine to use package my_module inside a *_test.go file

Comment on lines +79 to +83
it.After(func() {
require.NoError(t, os.Unsetenv(envVarRegistryUrl))
require.NoError(t, os.Unsetenv(envVarRegistryUser))
require.NoError(t, os.Unsetenv(envVarRegistryPassword))
})
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to worry about cleaning up env vars since child processes cannot interact with the parent's env vars (other than inheriting a copy).

For example, go run-ing this program won't unset $HOME nor add $FOOBAR to the shell

package main
import "os"
func main() {
	os.Setenv("FOOBAR", "BAZ")
	os.Unsetenv("HOME")
}

@ncarlson ncarlson deleted the 251-support-env-vars-for-registry-auth-when-executing-kp-import branch February 22, 2023 20:38
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.

Support env vars for registry auth when executing kp import
3 participants