🐛 When loading client config, check os/user.HomeDir if $HOME is unset

[joelanford]

Following up on the regression introduced in #642 and discussed in #748, this PR detects if $HOME is empty and, if so, automatically sets $HOME to user.Current before using client-go's default loading rules, which only checks $HOME.

As a follow-on, we could submit an issue upstream to see if it makes sense for client-go to use user.Current as a fallback so that we don't have to do it here in controller-runtime.

Closes #748

[gerred]

/lgtm

This should be upstream but makes sense here for now.

[DirectXMan12]

does this actually work? It looks like homedir is called from a static variable initializer -- https://github.com/kubernetes/client-go/blob/master/tools/clientcmd/loader.go#L51

[joelanford]

Oh good catch. Yeah this won't work. It looks like changing RecommendedHomeFile will work though since that's what actually ends up getting used: https://github.com/kubernetes/client-go/blob/7ec8a74ae980246e9d1a5734f45bd44fde93a5c1/tools/clientcmd/loader.go#L153

I'll update to just set that directly instead of the HOME env var.

[DirectXMan12]

is that thread-safe?

I'm worried that we may have to do a more invasive solution here, like customizing the result of (or not using at all) the NewDefaultBlahBlahBlah method.

[joelanford]

I believe this is now thread-safe. Now, we no longer change any global variables. Instead we create an instance of loadingRules and then append the extra file derived from u.HomeDir to the precedence list.

I manually tested this and it still works as expected.

Suggestions on adding a unit test for this? I'm not opposed, but the use of user.Current() and u.HomeDir makes it a little awkward. I don't love the idea of creating temporary files and directories in the home directories of anyone that runs the unit tests.

[joelanford]

/test pull-controller-runtime-test

[gerred]

/lgtm

[DirectXMan12]

/hold

thread safety

[kensipe]

@DirectXMan12 could you elaborate on the thread safety concern? I'm not seeing it. There is the concern that $HOME does not exist by default on windows.

since this is a bug fix... it would also be good to have a test

[joelanford]

@kensipe I think the thead-safety concern is that this would change a global variable clientcmd.RecommendedHomeFile without a lock.

If multiple callers call any of the methods that use loadConfig concurrently without HOME set, it is possible that we would not correctly reset clientcmd.RecommendedHomeFile back to its original value:

1. caller A sets"
   • oldRecommendedHomeFile = <original>
   • clientcmd.RecommendedHomeFile = <foobarA>
2. caller B sets
   • oldRecommendedHomeFile = <foobarA>
   • clientcmd.RecommendedHomeFile = <foobarB>
3. caller A resets clientcmd.RecommendedHomeFile = <original>
4. caller B resets clientcmd.RecommendedHomeFile = <foobarB>

Final value of clientcmd.RecommendedHomeFile is <foobarB> when it should be <original>.

@DirectXMan12 I'll look into alternatives.

[DirectXMan12]

/approve
/lgtm

[vincepri]

/hold cancel

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12, joelanford

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:

• OWNERS [DirectXMan12]

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