Add `kops create secret dockerconfig` feature #3087

blakebarnett · 2017-07-29T00:42:55Z

This adds a well-known secret name dockerconfig which will automatically
be used if present to create /root/.docker/config.json on all nodes. This will
allow private registries to be used for kops hooks as well as any k8s images
without the need to define imagePullSecrets in every namespace.

closes #2505

This adds a well-known secret name `nodedockercfg` which will automatically be used if present to create /root/.docker/config.json on all nodes. This will allow private registries to be used for kops hooks as well as any k8s images without the need to define `imagePullSecrets` in every namespace. closes kubernetes#2505

k8s-ci-robot · 2017-07-29T00:43:02Z

Hi @blakebarnett. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Also don't include the nodedockercfg in all_tokens.csv

move the tasks up so they apply to all nodes

blakebarnett · 2017-07-29T20:04:01Z

I'm not sure if there's a way to do cleanup in the case where this secret gets deleted from the secretStore. Also it doesn't appear there's currently a way to delete secrets either, I'll wait for the refactor in #3054 to figure out what to do about those 2 things.

Add HOME=/root to kubelet sysconfig

blakebarnett · 2017-07-29T20:55:34Z

/assign @chrislovecnm
I think this is good to go

justinsb · 2017-08-01T05:12:46Z

cmd/kops/create_secret_nodedockerconfig.go

+	}
+	secret, err := fi.CreateSecret()
+	if err != nil {
+		return fmt.Errorf("error creating node docker config secret %v: %v", secret, err)


Nit: you probably don't want to print secret - it'll probably be nil.. Also it is only ever going to be random gibberish...

And I don't think you want it anyway - just build the Secret directly, as you set Data below!

justinsb · 2017-08-01T05:14:48Z

cmd/kops/create_secret.go

@@ -33,6 +33,9 @@ var (
 	# Create an new ssh public key called admin.
 	kops create secret sshpublickey admin -i ~/.ssh/id_rsa.pub \
 		--name k8s-cluster.example.com --state s3://example.com
+
+	kops create secret nodedockercfg -i ~/.docker/config.json \


So we use -i for SSH because that's the arg for ssh public keys. Except of course it's actually the arg for SSH private keys. I suspect -i was my mistake, and we should probably use -f...

agreed, I'll switch it to -f

justinsb · 2017-08-01T05:17:48Z

cmd/kops/create_secret_nodedockerconfig.go

+
+	create_secret_nodedockerconfig_example = templates.Examples(i18n.T(`
+	# Create an new node docker config.
+	kops create secret nodedockerconfig -i /path/to/docker/config.json \


Wondering why this is _node_dockerconfig specifically. We probably do want to put it on the masters; if we didn't we would probably still call it dockerconfig but allow specific configuration (per instancegroup maybe?). kubectl calls it docker-registry. Thoughts?

Yeah I guess I wanted to make it clear that it was at the node level and wasn't going to end up inside k8s. Happy to change it to dockerconfig though.

justinsb · 2017-08-01T05:19:59Z

docs/security.md

@@ -20,6 +20,15 @@ To change the SSH public key on an existing cluster:
 * `kops update cluster --yes` to reconfigure the auto-scaling groups
 * `kops rolling-update cluster --name <clustername> --yes` to immediately roll all the machines so they have the new key (optional)

+## Node Docker Configuration
+
+If you are using a private registry such as quay.io, you may be familiar with the inconvenience of managing the `imagePullSecrets` for each namespace. It can also be a pain to use [Kops Hooks ](cluster_spec.md#hooks) with private images. To configure docker on all nodes with access to one or more private registries:


Total nit: extra space after Kops Hooks

justinsb · 2017-08-01T05:21:31Z

nodeup/pkg/model/kubelet.go

@@ -146,6 +146,8 @@ func (b *KubeletBuilder) buildSystemdEnvironmentFile(kubeletConfig *kops.Kubelet
 	}

 	sysconfig := "DAEMON_ARGS=\"" + flags + "\"\n"
+	// Makes kubelet read /root/.docker/config.json properly
+	sysconfig = sysconfig + "HOME=\"/root" + "\"\n"


Came across this with the ~/.aws directory as well - it is a pain...

Wondering if we should give kubelet etc their own directories, but if we do we can just change it when we need it. Thanks for fixing :-)

justinsb · 2017-08-01T05:22:09Z

nodeup/pkg/model/secrets.go

+			return err
+		}
+		if dockercfg == nil {
+			return fmt.Errorf("node docker config not found: %q", key)


Is this an error? Presumably not... I suspect we'll fail e2e with this (but we'll see I guess!)

oops I meant to have this as a guard when nodedockercfg is stored but the lookup failed. I think you're right, I'll just remove it and rely on the string check.

justinsb · 2017-08-01T05:24:19Z

upup/pkg/fi/cloudup/apply_cluster.go

+		if secret != nil {
+			nodeDockerConfig, err = secret.AsString()
+			if err != nil {
+				return fmt.Errorf("error node docker config not a string? %q: %v", nodeDockerConfig, err)


I think nodeDockerConfig is going be nil / "". So I don't think we print it, and I think that gets rid of nodeDockerConfig entirely.

Also, can we have a comment here - like // Verify that dockercfg is a string

in a previous iteration I had it actually checking if the json is parsable, I think maybe I'll switch this back to that and maybe move it into the CLI code since that seems like a better place to provide the error.

justinsb · 2017-08-01T05:24:57Z

/ok-to-test

justinsb · 2017-08-01T05:25:45Z

Thanks - this is awesome!

A few comments; I think some of them will need to be tweaked to pass e2e. Biggest questions (and they are questions) are about naming of the command / secret / flags - the hardest thing :-)

- Rename to just DockerConfig / dockerconfig everywhere for consistency - Check if the config is valid JSON - Update docs

blakebarnett · 2017-08-01T20:55:10Z

Thanks :) Let me know if this latest commit doesn't address all your comments!

blakebarnett · 2017-08-01T21:14:35Z

/test pull-kops-e2e-kubernetes-aws

blakebarnett · 2017-08-01T21:55:02Z

@justinsb not sure what that e2e error is about? Seems to have nothing to do with this PR...

blakebarnett · 2017-08-01T23:29:11Z

nevermind, I realized it was related to one of your original comments. Didn't notice the build log had the real reason for that error about log-dump.sh :)

justinsb · 2017-08-04T01:58:22Z

/lgtm

Nice work - thanks @blakebarnett

k8s-github-robot · 2017-08-04T01:58:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: blakebarnett, justinsb

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [justinsb]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-08-04T01:58:35Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-08-04T02:33:42Z

Automatic merge from submit-queue

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 29, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 29, 2017

Blake added 2 commits July 28, 2017 22:03

Kubelet $HOME is /var/lib/kubelet

760311f

Also don't include the nodedockercfg in all_tokens.csv

symlink to /root/.docker/config.json for hooks

7946f08

move the tasks up so they apply to all nodes

blakebarnett force-pushed the bdb/add_node_docker_config_secret branch from c413236 to cf1bae8 Compare July 29, 2017 07:57

Move task up so so it applies to all nodes

bd779e7

Add HOME=/root to kubelet sysconfig

blakebarnett force-pushed the bdb/add_node_docker_config_secret branch from cf1bae8 to bd779e7 Compare July 29, 2017 20:05

Blake added 2 commits July 29, 2017 13:29

Add documentation and update text fixture

4d9c69c

Don't error when no docker config is supplied

e8c6e50

k8s-ci-robot assigned chrislovecnm Jul 29, 2017

justinsb reviewed Aug 1, 2017

View reviewed changes

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 1, 2017

justinsb assigned justinsb and unassigned chrislovecnm Aug 1, 2017

Incorporate review feedback

4f95fe4

- Rename to just DockerConfig / dockerconfig everywhere for consistency - Check if the config is valid JSON - Update docs

blakebarnett changed the title ~~Add kops create secret nodedockercfg feature~~ Add kops create secret dockerconfig feature Aug 1, 2017

Don't error if the dockerconfig isn't present

e093702

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 4, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 4, 2017

k8s-github-robot merged commit 5577fab into kubernetes:master Aug 4, 2017

a0x8o mentioned this pull request Aug 8, 2017

Docker configuration example does not work - '-f' flag not supported #3163

Closed

blakebarnett deleted the bdb/add_node_docker_config_secret branch August 14, 2017 20:12

erickt mentioned this pull request Aug 29, 2017

Customizing docker config for a particular instance group #3311

Closed

chrislovecnm mentioned this pull request Sep 15, 2017

Support encryption-at-rest for the kube-apiserver #3368

Merged

blakebarnett mentioned this pull request Nov 15, 2021

Containerd repo mirror auth #12562

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `kops create secret dockerconfig` feature #3087

Add `kops create secret dockerconfig` feature #3087

blakebarnett commented Jul 29, 2017 •

edited

Loading

k8s-ci-robot commented Jul 29, 2017

blakebarnett commented Jul 29, 2017

blakebarnett commented Jul 29, 2017

justinsb Aug 1, 2017

justinsb Aug 1, 2017

blakebarnett Aug 1, 2017

justinsb Aug 1, 2017

blakebarnett Aug 1, 2017

justinsb Aug 1, 2017

blakebarnett Aug 1, 2017

justinsb Aug 1, 2017

justinsb Aug 1, 2017

justinsb Aug 1, 2017

blakebarnett Aug 1, 2017

justinsb Aug 1, 2017

blakebarnett Aug 1, 2017

justinsb commented Aug 1, 2017

justinsb commented Aug 1, 2017

blakebarnett commented Aug 1, 2017

blakebarnett commented Aug 1, 2017

blakebarnett commented Aug 1, 2017

blakebarnett commented Aug 1, 2017

justinsb commented Aug 4, 2017

k8s-github-robot commented Aug 4, 2017

k8s-github-robot commented Aug 4, 2017

k8s-github-robot commented Aug 4, 2017

Add kops create secret dockerconfig feature #3087

Add kops create secret dockerconfig feature #3087

Conversation

blakebarnett commented Jul 29, 2017 • edited Loading

k8s-ci-robot commented Jul 29, 2017

blakebarnett commented Jul 29, 2017

blakebarnett commented Jul 29, 2017

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

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

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

Choose a reason for hiding this comment

justinsb commented Aug 1, 2017

justinsb commented Aug 1, 2017

blakebarnett commented Aug 1, 2017

blakebarnett commented Aug 1, 2017

blakebarnett commented Aug 1, 2017

blakebarnett commented Aug 1, 2017

justinsb commented Aug 4, 2017

k8s-github-robot commented Aug 4, 2017

k8s-github-robot commented Aug 4, 2017

k8s-github-robot commented Aug 4, 2017

Add `kops create secret dockerconfig` feature #3087

Add `kops create secret dockerconfig` feature #3087

blakebarnett commented Jul 29, 2017 •

edited

Loading