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.

Sign up for GitHub

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

Jump to bottom

Fix new docker config format for private registries #12717

Merged
merged 1 commit into from Sep 18, 2015
Merged

Fix new docker config format for private registries #12717

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 9 additions & 0 deletions pkg/api/types.go
View file
Open in desktop
Expand Up @@ -2073,6 +2073,15 @@ const (

// DockerConfigKey is the key of the required data for SecretTypeDockercfg secrets
DockerConfigKey = ".dockercfg"

// SecretTypeDockerConfigJson contains a dockercfg file that follows the same format rules as ~/.docker/config.json
//
// Required fields:
// - Secret.Data[".dockerconfigjson"] - a serialized ~/.docker/config.json file
SecretTypeDockerConfigJson SecretType = "kubernetes.io/dockerconfigjson"

// DockerConfigJsonKey is the key of the required data for SecretTypeDockerConfigJson secrets
DockerConfigJsonKey = ".dockerconfigjson"
)

type SecretList struct {
Expand Down
48 changes: 47 additions & 1 deletion pkg/credentialprovider/config.go
View file
Open in desktop
Expand Up @@ -30,6 +30,13 @@ import (
"github.com/golang/glog"
)

// DockerConfigJson represents ~/.docker/config.json file info
// see https://github.com/docker/docker/pull/12009
type DockerConfigJson struct {
Auths DockerConfig `json:"auths"`
HttpHeaders map[string]string `json:"HttpHeaders,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment about what this is for? (Based on the docs here: https://docs.docker.com/reference/commandline/cli/, it looks like a set of blindly attached headers) and an indication that we don't use or respect them (yet).

@erictune Are you willing to take the HttpHeaders support as a follow on item? I think we'd gain a lot of benefit even without plumbing them through.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This struct DockerConfigJson is used to store content of ~/.docker/config.json, it is the same structure as ConfigFile struct in docker/cliconfig:

// ConfigFile ~/.docker/config.json file info
type ConfigFile struct {
    AuthConfigs map[string]AuthConfig `json:"auths"`
    HTTPHeaders map[string]string     `json:"HttpHeaders,omitempty"`
    PsFormat    string                `json:"psFormat,omitempty"`
    filename    string                // Note: not serialized - for internal use only
}

In the case we don't use HTTPHeaders and PsFormat, then we do not need that struct. And as in your comment below, a single method func DockerConfigFrom(bytes []bytes) (DockerConfig, error) which parse both new and old type of secrets will make code easier to maintain.

Copy link
Member

Choose a reason for hiding this comment

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

@deads2k Can you explain what you mean by "...take the HttpHeaders support as a follow on item... without plumbing them through.".

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd drop HttpHeaders until they are plumbed through.

Copy link
Contributor

Choose a reason for hiding this comment

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

@deads2k Can you explain what you mean by "...take the HttpHeaders support as a follow on item... without plumbing them through.".

The HTTPHeaders field is used by docker to add opaque headers to client requests. The new format supports this field, but actually implementing in the kubelet will take additional work. I'd prefer to avoid having that feature block this pull. This pull is very useful even if the HTTPHeaders field doesn't work as expected.

}

// DockerConfig represents the config file used by the docker CLI.
// This config that represents the credentials that should be used
// when pulling images from specific image repositories.
Expand All @@ -47,8 +54,11 @@ var (
workingDirPath = ""
homeDirPath = os.Getenv("HOME")
rootDirPath = "/"
homeJsonDirPath = filepath.Join(homeDirPath, ".docker")
rootJsonDirPath = filepath.Join(rootDirPath, ".docker")

configFileName = ".dockercfg"
configFileName = ".dockercfg"
configJsonFileName = "config.json"
)

func SetPreferredDockercfgPath(path string) {
Expand All @@ -64,6 +74,32 @@ func GetPreferredDockercfgPath() string {
}

func ReadDockerConfigFile() (cfg DockerConfig, err error) {
// Try happy path first - latest config file
Copy link
Contributor

Choose a reason for hiding this comment

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

How is docker handling the transition? Are the files complementary? If so, we should probably have two different provider instances to build the keyring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/docker/docker/blob/master/cliconfig/config.go#L159
In func Load(configDir string) (*ConfigFile, error), first, they try to load new configuration file at ~/.docker/config.json and parse by configFile.LoadFromReader(file), if the config file is not found, then they fallback to load the old one at ~/.dockercfg by configFile.LegacyLoadFromReader(file).

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/docker/docker/blob/master/cliconfig/config.go#L159
In func Load(configDir string) (*ConfigFile, error), first, they try to load new configuration file at ~/.docker/config.json and parse by configFile.LoadFromReader(file), if the config file is not found, then they fallback to load the old one at ~/.dockercfg by configFile.LegacyLoadFromReader(file).

Can you add a comment explaining the behavior and why we're doing it? It struck me as a little odd when I read it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function func ReadDockerConfigFile() (cfg DockerConfig, err error) is used to parse config which is stored in:

  • GetPreferredDockercfgPath() + "/config.json"
  • workingDirPath + "/config.json"
  • $HOME/.docker/config.json
  • /.docker/config.json
  • GetPreferredDockercfgPath() + "/.dockercfg"
  • workingDirPath + "/.dockercfg"
  • $HOME/.dockercfg
  • /.dockercfg

The first four one are new type of secret, and the last four one are the old type.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd agree that having multiple providers, one that passes ".docker/config.json" and one that passes ".dockercfg" would be preferable because it blends the keyrings. This is what I did here: #13894

However, I didn't realize this existed when I wrote that.

Copy link

Choose a reason for hiding this comment

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

I think having two separate providers for .docker/config.json" and ".dockercfg" makes sense. It clearly states in the issue that the format is also changed between the two so we cannot use the same secret for both providers.

Copy link
Contributor

Choose a reason for hiding this comment

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

The new format is a strict superset of the old, so these are not mutually exclusive options.

The new format adds a level of JSON around an identical structure, and at the top-level can now also include HttpHeaders, which you aren't using yet.

This is clearly possible via credentialprovider, as I linked you a PR that does precisely this (ignoring HttpHeaders).

To support HttpHeaders, the credentialprovider interface will have to change anyways.

dockerConfigJsonLocations := []string{GetPreferredDockercfgPath(), workingDirPath, homeJsonDirPath, rootJsonDirPath}
for _, configPath := range dockerConfigJsonLocations {
absDockerConfigFileLocation, err := filepath.Abs(filepath.Join(configPath, configJsonFileName))
if err != nil {
glog.Errorf("while trying to canonicalize %s: %v", configPath, err)
continue
}
glog.V(4).Infof("looking for .docker/config.json at %s", absDockerConfigFileLocation)
contents, err := ioutil.ReadFile(absDockerConfigFileLocation)
if os.IsNotExist(err) {
continue
}
if err != nil {
glog.V(4).Infof("while trying to read %s: %v", absDockerConfigFileLocation, err)
continue
}
cfg, err := readDockerConfigJsonFileFromBytes(contents)
if err == nil {
glog.V(4).Infof("found .docker/config.json at %s", absDockerConfigFileLocation)
return cfg, nil
}
}
glog.V(4).Infof("couldn't find valid .docker/config.json after checking in %v", dockerConfigJsonLocations)

// Can't find latest config file so check for the old one
dockerConfigFileLocations := []string{GetPreferredDockercfgPath(), workingDirPath, homeDirPath, rootDirPath}
for _, configPath := range dockerConfigFileLocations {
absDockerConfigFileLocation, err := filepath.Abs(filepath.Join(configPath, configFileName))
Expand Down Expand Up @@ -147,6 +183,16 @@ func readDockerConfigFileFromBytes(contents []byte) (cfg DockerConfig, err error
return
}

func readDockerConfigJsonFileFromBytes(contents []byte) (cfg DockerConfig, err error) {
var cfgJson DockerConfigJson
if err = json.Unmarshal(contents, &cfgJson); err != nil {
glog.Errorf("while trying to parse blob %q: %v", contents, err)
return nil, err
}
cfg = cfgJson.Auths
return
}

// dockerConfigEntryWithAuth is used solely for deserializing the Auth field
// into a dockerConfigEntry during JSON deserialization.
type dockerConfigEntryWithAuth struct {
Expand Down
29 changes: 29 additions & 0 deletions pkg/credentialprovider/config_test.go
View file
Open in desktop
Expand Up @@ -22,6 +22,35 @@ import (
"testing"
)

func TestDockerConfigJsonJSONDecode(t *testing.T) {
input := []byte(`{"auths": {"http://foo.example.com":{"username": "foo", "password": "bar", "email": "foo@example.com"}, "http://bar.example.com":{"username": "bar", "password": "baz", "email": "bar@example.com"}}}`)

expect := DockerConfigJson{
Auths: DockerConfig(map[string]DockerConfigEntry{
"http://foo.example.com": {
Username: "foo",
Password: "bar",
Email: "foo@example.com",
},
"http://bar.example.com": {
Username: "bar",
Password: "baz",
Email: "bar@example.com",
},
}),
}

var output DockerConfigJson
err := json.Unmarshal(input, &output)
if err != nil {
t.Errorf("Received unexpected error: %v", err)
}

if !reflect.DeepEqual(expect, output) {
t.Errorf("Received unexpected output. Expected %#v, got %#v", expect, output)
}
}

func TestDockerConfigJSONDecode(t *testing.T) {
input := []byte(`{"http://foo.example.com":{"username": "foo", "password": "bar", "email": "foo@example.com"}, "http://bar.example.com":{"username": "bar", "password": "baz", "email": "bar@example.com"}}`)

Expand Down
9 changes: 8 additions & 1 deletion pkg/credentialprovider/keyring.go
View file
Open in desktop
Expand Up @@ -277,7 +277,14 @@ func (k *unionDockerKeyring) Lookup(image string) ([]docker.AuthConfiguration, b
func MakeDockerKeyring(passedSecrets []api.Secret, defaultKeyring DockerKeyring) (DockerKeyring, error) {
passedCredentials := []DockerConfig{}
for _, passedSecret := range passedSecrets {
if dockercfgBytes, dockercfgExists := passedSecret.Data[api.DockerConfigKey]; (passedSecret.Type == api.SecretTypeDockercfg) && dockercfgExists && (len(dockercfgBytes) > 0) {
if dockerConfigJsonBytes, dockerConfigJsonExists := passedSecret.Data[api.DockerConfigJsonKey]; (passedSecret.Type == api.SecretTypeDockerConfigJson) && dockerConfigJsonExists && (len(dockerConfigJsonBytes) > 0) {
dockerConfigJson := DockerConfigJson{}
if err := json.Unmarshal(dockerConfigJsonBytes, &dockerConfigJson); err != nil {
return nil, err
}

passedCredentials = append(passedCredentials, dockerConfigJson.Auths)
} else if dockercfgBytes, dockercfgExists := passedSecret.Data[api.DockerConfigKey]; (passedSecret.Type == api.SecretTypeDockercfg) && dockercfgExists && (len(dockercfgBytes) > 0) {
dockercfg := DockerConfig{}
if err := json.Unmarshal(dockercfgBytes, &dockercfg); err != nil {
return nil, err
Expand Down
12 changes: 12 additions & 0 deletions pkg/kubelet/dockertools/docker_test.go
View file
Open in desktop
Expand Up @@ -283,6 +283,12 @@ func TestPullWithSecrets(t *testing.T) {
t.Errorf("unexpected error: %v", err)
}

dockerConfigJson := map[string]map[string]map[string]string{"auths": dockerCfg}
dockerConfigJsonContent, err := json.Marshal(dockerConfigJson)
if err != nil {
t.Errorf("unexpected error: %v", err)
}

tests := map[string]struct {
imageName string
passedSecrets []api.Secret
Expand Down Expand Up @@ -313,6 +319,12 @@ func TestPullWithSecrets(t *testing.T) {
credentialprovider.DockerConfig(map[string]credentialprovider.DockerConfigEntry{"index.docker.io/v1/": {"built-in", "password", "email"}}),
[]string{`ubuntu:latest using {"username":"passed-user","password":"passed-password","email":"passed-email"}`},
},
"builtin keyring secrets, but use passed with new docker config": {
"ubuntu",
[]api.Secret{{Type: api.SecretTypeDockerConfigJson, Data: map[string][]byte{api.DockerConfigJsonKey: dockerConfigJsonContent}}},
credentialprovider.DockerConfig(map[string]credentialprovider.DockerConfigEntry{"index.docker.io/v1/": {"built-in", "password", "email"}}),
[]string{`ubuntu:latest using {"username":"passed-user","password":"passed-password","email":"passed-email"}`},
},
}
for _, test := range tests {
builtInKeyRing := &credentialprovider.BasicDockerKeyring{}
Expand Down