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

Improve windows home directory selection #73923

Merged

Conversation

@liggitt
Copy link
Member

commented Feb 11, 2019

What type of PR is this?
/kind feature

What this PR does / why we need it:
Alternative to #67403

Improves the selected home directory on windows, while maintaining compatibility with existing configurations that already point at a location containing a kubeconfig file, or a user-writeable directory.

Which issue(s) this PR fixes:
Fixes #61946

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

On Windows systems, %USERPROFILE% is now preferred over %HOMEDRIVE%\%HOMEPATH% as the home folder if %HOMEDRIVE%\%HOMEPATH% does not contain a .kube\config file, and %USERPROFILE% exists and is writeable.

/sig windows

}
}
if homeDrive, homePath := os.Getenv("HOMEDRIVE"), os.Getenv("HOMEPATH"); len(homeDrive) > 0 && len(homePath) > 0 {
homeDir := homeDrive + homePath
if _, err := os.Stat(homeDir); err == nil {
return homeDir
possiblePaths = append(possiblePaths, homeDir)
}
}
if userProfile := os.Getenv("USERPROFILE"); len(userProfile) > 0 {

This comment has been minimized.

Copy link
@davidovich

davidovich Feb 12, 2019

I believe USERPROFILE should be the second test in this list. HOME is never present by default and is set by the user. USERPROFILE is always present (in modern Windows versions).

This comment has been minimized.

Copy link
@liggitt

liggitt Feb 12, 2019

Author Member

reordering breaks backward compatibility with currently working configurations. an attempt was made to do that in #67403, but is a behavioral change I think we should avoid.

This comment has been minimized.

Copy link
@davidovich

davidovich Feb 12, 2019

LGTM, as I don't have enough data to disagree. Having the writable test on HOMEDRIVE/HOMEPATH will do the job for us, but this combination seems a relic from an old past. By curiosity, how many users would be broken by this ?

This comment has been minimized.

Copy link
@mbrancato

mbrancato Feb 14, 2019

I believe that the only thing that will fix #61946 is purely order. The problem is that the order should match that of other clients that interact with the .kube folder. If HOMEDRIVE/HOMEPATH is writable it should still be ignored if it does not have a .kube folder. Documentation (while dated, and sparse) on why was provided here: #61946 (comment)

This comment has been minimized.

Copy link
@neolit123

neolit123 Feb 15, 2019

Member

If HOMEDRIVE/HOMEPATH is writable it should still be ignored if it does not have a .kube folder

i think this is the key for backwards compat here.

  • check HOME
  • check if HOMEDRIVE + HOMEPATH exists
    • write a flag (e.g. isHomeDriveHomePathValid) that this path exists (i.e ENV variables are sane)
    • if .kube exists there respect that (backwards compat)
    • if not falltrough
  • check USERPROFILE
  • if isHomeDriveHomePathValid == true, use that
  • return HOME (backwards compat)

would that work?

also is it possible to bump client-go with action required type of a release note in case of a breaking change?

This comment has been minimized.

Copy link
@mbrancato

mbrancato Feb 16, 2019

The only issue I see is that it will still prefer HOMEDRIVE/HOMEPATH if it is writable. The biggest issue here is that other tools and languages that modify the .kube (cloud provider auth tools) do not follow this preference.

To deprecate HOMEDRIVE/HOMEPATH but still support it, I think it needs to prefer USERPROFILE when both are writable and no .kube exists.

This comment has been minimized.

Copy link
@liggitt

liggitt Feb 16, 2019

Author Member

ok, took a stab at that:

  1. the first of %HOME%, %HOMEDRIVE%%HOMEPATH%, %USERPROFILE% containing a .kube\config file is returned.
  2. if none of those locations contain a .kube\config file, the first of %HOME%, %USERPROFILE%, %HOMEDRIVE%%HOMEPATH% that exists and is writeable is returned.
  3. if none of those locations are writeable, the first of %HOME%, %USERPROFILE%, %HOMEDRIVE%%HOMEPATH% that exists is returned.
  4. if none of those locations exists, the first of %HOME%, %USERPROFILE%, %HOMEDRIVE%%HOMEPATH% that is set is returned.

This comment has been minimized.

Copy link
@liggitt

liggitt Feb 16, 2019

Author Member

if that looks right, I'll add unit tests

This comment has been minimized.

Copy link
@mbrancato

mbrancato Feb 16, 2019

That sounds correct to me. There are probably some edge cases that could exist from the comments above, but I think that would provide suitable protection for backwards compatibility in environments that use alternate home paths.

This comment has been minimized.

Copy link
@davidovich
@PatrickLang

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

@PatrickLang PatrickLang added this to In Review in SIG-Windows Feb 14, 2019

@liggitt liggitt force-pushed the liggitt:prefer-writeable-windows-homedir branch from 1321ca7 to 11ff290 Feb 16, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

@liggitt liggitt force-pushed the liggitt:prefer-writeable-windows-homedir branch from 11ff290 to b6ff90e Feb 16, 2019

@liggitt liggitt force-pushed the liggitt:prefer-writeable-windows-homedir branch from b6ff90e to 448e3f7 Feb 16, 2019

@liggitt liggitt added this to the v1.15 milestone Mar 7, 2019

@liggitt liggitt changed the title WIP - Improve windows home directory selection Improve windows home directory selection Apr 2, 2019

@liggitt

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2019

/retest

@tallclair

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

BTW, go 1.12 has a new UserHomeDir() function

@dims

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

@PatrickLang

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

HOMEDRIVE/HOMEDIR isn't even documented in current OS versions. I think the preference should always be USERPROFILE.

If we're worried about back compat, then I would simplify this to:

  • if %HOMEDIR%%HOMEDRIVE%\.kube\config exists, return it
  • else return %USERPROFILE%

Even on systems that are set up with temporary profiles that don't persist state across logins - USERPROFILE will be writeable as key apps use well-known folders under it https://docs.microsoft.com/en-us/windows/desktop/shell/knownfolderid

if _, err := os.Stat(homeDir); err == nil {
return homeDir
if _, err := os.Stat(filepath.Join(p, ".kube", "config")); err != nil {
continue

This comment has been minimized.

Copy link
@PatrickLang

PatrickLang May 30, 2019

Contributor

shouldn't this be return, not continue?

This comment has been minimized.

Copy link
@mbrancato

mbrancato May 30, 2019

If err is nil then the file was found. So I think the logic is correct in the attempt to preserve backward compatibility with previous behavior.
If no .kube/config exists for any, it returns %USERPROFILE%, the last value.

This comment has been minimized.

Copy link
@PatrickLang

PatrickLang May 31, 2019

Contributor

oh yes, you're right I misread that

firstSetPath := ""
firstExistingPath := ""

// Prefer %USERPROFILE% over %HOMEDRIVE%/%HOMEPATH% for compatibility with other auth-writing tools

This comment has been minimized.

Copy link
@PatrickLang

PatrickLang May 30, 2019

Contributor

no need to do checks here, USERPROFILE exists and is writeable

@soggiest

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

/milestone v1.16

@k8s-ci-robot k8s-ci-robot modified the milestones: v1.15, v1.16 Jun 3, 2019

@PatrickLang

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Jul 15, 2019

@fejta-bot

This comment has been minimized.

Copy link

commented Jul 15, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 98c68df into kubernetes:master Jul 16, 2019

24 checks passed

cla/linuxfoundation liggitt 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-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Context retired. Status moved to "pull-kubernetes-dependencies".
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Context retired without replacement.
Details
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

SIG-Windows automation moved this from In Progress+Review to Done (v1.16) Jul 16, 2019

@liggitt liggitt deleted the liggitt:prefer-writeable-windows-homedir branch Jul 25, 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.