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

credentialprovider: Sets homeDirPath to os.UserHomeDir() #78528

Merged

Conversation

@bclau
Copy link
Contributor

commented May 30, 2019

What type of PR is this?

/kind bug

/sig windows

What this PR does / why we need it:

Currently, the credential provider will look in the path set in the $HOME env variable, but that environment does not exist on Windows, but $HOMEPATH / $USERPROFILE does. Because of this, if credentials are set in ~/.docker on Windows, they will not be used by kubelet when pulling images.

The function os.UserHomeDir can solve this problem [1].

[1] https://golang.org/pkg/os/#UserHomeDir

Which issue(s) this PR fixes:

Fixes #76077

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Windows Kubelet nodes will now correctly search the default location for Docker credentials (`%USERPROFILE%\.docker\config.json`) when pulling images from a private registry (https://kubernetes.io/docs/concepts/containers/images/#configuring-nodes-to-authenticate-to-a-private-registry)
credentialprovider: Sets homeDirPath to os.UserHomeDir()
Currently, the credential provider will look in the path set in
the $HOME env variable, but that environment does not exist on
Windows, but $HOMEPATH does. Because of this, if credentials are
set in ~/.docker on Windows, they will not be used by kubelet
when pulling images.

The function os.UserHomeDir can solve this problem [1].

[1] https://golang.org/pkg/os/#UserHomeDir
@PatrickLang

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

/milestone v1.15
/lgtm

This is a clear bug - using that function is the right approach.

@k8s-ci-robot k8s-ci-robot added this to the v1.15 milestone May 30, 2019

@k8s-ci-robot k8s-ci-robot added the lgtm label May 30, 2019

@PatrickLang

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

/assign @liggitt
Can you approve this one? It's using the right utility method so that we can fix stored logins for the Docker config referenced by the kubelet

@PatrickLang PatrickLang moved this from Backlog to In Progress+Review in SIG-Windows May 30, 2019

@bclau

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

/retest

@liggitt

This comment has been minimized.

Copy link
Member

commented May 30, 2019

it looks like the only supported platform this will cause changes on is windows. are there installations setting $HOME in order to use the existing config that will be affected? at the very least, we need to describe the change in a release note and update https://kubernetes.io/docs/concepts/containers/images/#configuring-nodes-to-authenticate-to-a-private-registry with platform-specific details

@liggitt

This comment has been minimized.

Copy link
Member

commented May 30, 2019

/lgtm
/approve
/retest

please update the release note and link the doc PR

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bclau, liggitt

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@PatrickLang

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

$HOME wouldn't be set in the context of a service, it's a legacy (pre-Windows 7) setting applying to logged in user profiles only. The current codepath was actually resolving that to c:\.docker\config per the kubelet logs in #76077 because it's not set.

@bclau

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

/lgtm
/approve
/retest

please update the release note and link the doc PR

Done. Let me know if it's ok or not.

@liggitt

This comment has been minimized.

Copy link
Member

commented May 30, 2019

/retest

1 similar comment
@PatrickLang

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

/retest

@PatrickLang PatrickLang moved this from In Progress+Review to Done (v1.15) in SIG-Windows May 31, 2019

@k8s-ci-robot k8s-ci-robot merged commit d910055 into kubernetes:master Jun 1, 2019

21 checks passed

cla/linuxfoundation bclau authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

@bclau bclau deleted the bclau:credentialprovider/windows-homedir branch Jun 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.