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

Add kops create secret dockerconfig feature #3087

Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions cmd/kops/create_secret.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 \
Copy link
Member

Choose a reason for hiding this comment

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

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...

Copy link
Author

Choose a reason for hiding this comment

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

agreed, I'll switch it to -f

--name k8s-cluster.example.com --state s3://example.com
`))

create_secret_short = i18n.T(`Create a secret.`)
Expand All @@ -48,6 +51,7 @@ func NewCmdCreateSecret(f *util.Factory, out io.Writer) *cobra.Command {

// create subcommands
cmd.AddCommand(NewCmdCreateSecretPublicKey(f, out))
cmd.AddCommand(NewCmdCreateSecretNodeDockerConfig(f, out))

return cmd
}
116 changes: 116 additions & 0 deletions cmd/kops/create_secret_nodedockerconfig.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
/*
Copyright 2016 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package main

import (
"fmt"
"io"
"io/ioutil"
"os"

"github.com/spf13/cobra"
"k8s.io/kops/cmd/kops/util"
"k8s.io/kops/pkg/apis/kops/registry"
"k8s.io/kops/upup/pkg/fi"
"k8s.io/kubernetes/pkg/kubectl/cmd/templates"
"k8s.io/kubernetes/pkg/util/i18n"
)

var (
create_secret_nodedockerconfig_long = templates.LongDesc(i18n.T(`
Create a new node docker config, and store it in the state store. Use update
to update it, this command will only create a new entry.`))

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

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

--name k8s-cluster.example.com --state s3://example.com
`))

create_secret_nodedockerconfig_short = i18n.T(`Create a node docker config.`)
)

type CreateSecretDockercfgOptions struct {
ClusterName string
DockerCfgPath string
}

func NewCmdCreateSecretNodeDockerConfig(f *util.Factory, out io.Writer) *cobra.Command {
options := &CreateSecretDockercfgOptions{}

cmd := &cobra.Command{
Use: "nodedockercfg",
Short: create_secret_nodedockerconfig_short,
Long: create_secret_nodedockerconfig_long,
Example: create_secret_nodedockerconfig_example,
Run: func(cmd *cobra.Command, args []string) {
if len(args) != 0 {
exitWithError(fmt.Errorf("syntax: -i <DockerCfgPath>"))
}

err := rootCommand.ProcessArgs(args[0:])
if err != nil {
exitWithError(err)
}

options.ClusterName = rootCommand.ClusterName()

err = RunCreateSecretDockerCfg(f, os.Stdout, options)
if err != nil {
exitWithError(err)
}
},
}

cmd.Flags().StringVarP(&options.DockerCfgPath, "", "i", "", "Path to node docker config")

return cmd
}

func RunCreateSecretDockerCfg(f *util.Factory, out io.Writer, options *CreateSecretDockercfgOptions) error {
if options.DockerCfgPath == "" {
return fmt.Errorf("docker config path is required (use -i)")
}
secret, err := fi.CreateSecret()
if err != nil {
return fmt.Errorf("error creating node docker config secret %v: %v", secret, err)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

good point

}

cluster, err := GetCluster(f, options.ClusterName)
if err != nil {
return err
}

secretStore, err := registry.SecretStore(cluster)
if err != nil {
return err
}

data, err := ioutil.ReadFile(options.DockerCfgPath)
if err != nil {
return fmt.Errorf("error reading node docker config %v: %v", options.DockerCfgPath, err)
}

secret.Data = data

_, _, err = secretStore.GetOrCreateSecret("nodedockercfg", secret)
if err != nil {
return fmt.Errorf("error adding node docker config: %v", err)
}

return nil
}
4 changes: 4 additions & 0 deletions docs/cli/kops_create_secret.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ Create a secret
# 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 \
--name k8s-cluster.example.com --state s3://example.com
```

### Options inherited from parent commands
Expand All @@ -35,5 +38,6 @@ Create a secret

### SEE ALSO
* [kops create](kops_create.md) - Create a resource by command line, filename or stdin.
* [kops create secret nodedockercfg](kops_create_secret_nodedockercfg.md) - Create a node docker config.
* [kops create secret sshpublickey](kops_create_secret_sshpublickey.md) - Create a ssh public key.

48 changes: 48 additions & 0 deletions docs/cli/kops_create_secret_nodedockercfg.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@

<!--- This file is automatically generated by make gen-cli-docs; changes should be made in the go CLI command code (under cmd/kops) -->

## kops create secret nodedockercfg

Create a node docker config.

### Synopsis


Create a new node docker config, and store it in the state store. Use update to update it, this command will only create a new entry.

```
kops create secret nodedockercfg
```

### Examples

```
# Create an new node docker config.
kops create secret nodedockerconfig -i /path/to/docker/config.json \
--name k8s-cluster.example.com --state s3://example.com
```

### Options

```
-i, -- string Path to node docker config
```

### Options inherited from parent commands

```
--alsologtostderr log to standard error as well as files
--config string config file (default is $HOME/.kops.yaml)
--log_backtrace_at traceLocation when logging hits line file:N, emit a stack trace (default :0)
--log_dir string If non-empty, write log files in this directory
--logtostderr log to standard error instead of files (default false)
--name string Name of cluster
--state string Location of state storage
--stderrthreshold severity logs at or above this threshold go to stderr (default 2)
-v, --v Level log level for V logs
--vmodule moduleSpec comma-separated list of pattern=N settings for file-filtered logging
```

### SEE ALSO
* [kops create secret](kops_create_secret.md) - Create a secret.

9 changes: 9 additions & 0 deletions docs/security.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Member

Choose a reason for hiding this comment

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

Total nit: extra space after Kops Hooks


* `kops create secret --name <clustername> nodedockercfg -i ~/.docker/config.json`
* `kops rolling-update cluster --name <clustername> --yes` to immediately roll all the machines so they have the new key (optional)

This stores the `config.json` in `/root/.docker/config.json` on all nodes so that both Kubernetes and system containers may use the registries.

## IAM roles

All Pods running on your cluster have access to underlying instance IAM role.
Expand Down
2 changes: 2 additions & 0 deletions nodeup/pkg/model/kubelet.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Member

Choose a reason for hiding this comment

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

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 :-)


t := &nodetasks.File{
Path: "/etc/sysconfig/kubelet",
Expand Down
23 changes: 23 additions & 0 deletions nodeup/pkg/model/secrets.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,26 @@ func (b *SecretBuilder) Build(c *fi.ModelBuilderContext) error {
c.AddTask(t)
}

if b.SecretStore != nil {
key := "nodedockercfg"
dockercfg, err := b.SecretStore.Secret(key)
if err != nil {
return err
}
if dockercfg == nil {
return fmt.Errorf("node docker config not found: %q", key)
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 an error? Presumably not... I suspect we'll fail e2e with this (but we'll see I guess!)

Copy link
Author

Choose a reason for hiding this comment

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

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.

}
contents := string(dockercfg.Data)

t := &nodetasks.File{
Path: filepath.Join("root", ".docker", "config.json"),
Contents: fi.NewStringResource(contents),
Type: nodetasks.FileType_File,
Mode: s("0600"),
}
c.AddTask(t)
}

// if we are not a master we can stop here
if !b.IsMaster {
return nil
Expand Down Expand Up @@ -129,6 +149,9 @@ func (b *SecretBuilder) Build(c *fi.ModelBuilderContext) error {

var lines []string
for id, token := range allTokens {
if id == "nodedockercfg" {
continue
}
lines = append(lines, token+","+id+","+id)
}
csv := strings.Join(lines, "\n")
Expand Down
1 change: 1 addition & 0 deletions nodeup/pkg/model/tests/kubelet/featuregates/tasks.yaml
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
contents: |
DAEMON_ARGS="--feature-gates=AllowExtTrafficLocalEndpoints=false,ExperimentalCriticalPodAnnotation=true --node-labels=kubernetes.io/role=node,node-role.kubernetes.io/node= --cni-bin-dir=/opt/cni/bin/ --cni-conf-dir=/etc/cni/net.d/ --network-plugin-dir=/opt/cni/bin/"
HOME="/root"
path: /etc/sysconfig/kubelet
type: file
11 changes: 11 additions & 0 deletions upup/pkg/fi/cloudup/apply_cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,17 @@ func (c *ApplyClusterCmd) Run() error {
}
}

var nodeDockerConfig string
{
secret, _ := secretStore.Secret("nodedockercfg")
if secret != nil {
nodeDockerConfig, err = secret.AsString()
if err != nil {
return fmt.Errorf("error node docker config not a string? %q: %v", nodeDockerConfig, err)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

}
}
}

modelContext := &model.KopsModelContext{
Cluster: cluster,
InstanceGroups: c.InstanceGroups,
Expand Down