Controller-runtime: config.loadConfig Breaking Change introduced in 0.4.0

[edwardecook]

As part of https://github.com/kubernetes-sigs/controller-runtime/commit/840ac8a2ef0d9bab7e4f721970bda72cae3b2979 there was a switch for the fallback behaviour of config.loadConfig (when not specifying via flag, env var or internal config) from identifying the default home directory with os/user to the k8s.io/client-go/util/homedir package (called via clientcmd.NewDefaultClientConfigLoadingRules()).

This has the effect of switching from grabbing the os home config to relying on the HOME env var to be set which is not always the case.

This is not listed as a breaking change within the v0.4.0 release notes however it will effect anyone using the GetConfig or GetConfigOrDie functions.

Let me know if there are any other details needed!

Cheers

cc @gmrodgers

[DirectXMan12]

Thanks so much for reporting this!

I'll add it to the release notes. In the mean time, I think this is an unintended regression, and we might want to fix it (although I suspect this brings us in line with what kubectl does, so maybe not that bad?)

cc @joelanford who authored that change.

Out of curiosity, what are the cases where $HOME isn't set?

[DirectXMan12]

/kind bug
/priority critical-urgent
/kind regression

[k8s-ci-robot]

@DirectXMan12: The label(s) kind/regression cannot be applied, because the repository doesn't have them

In response to this:

/kind bug
/priority critical-urgent
/kind regression

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[DirectXMan12]

release notes updated

[edwardecook]

Cheers for the speedy response!

$HOME isn't set when running an external command via the os/exec package, for instance exec.Command(binPath).Start() which is how we run a controller as part of acceptance testing.

[joelanford]

Good catch @edwardecook!

It seems like a quick fix _could_ be to check if $HOME is set, and if not, use the original method to set it, which would then allow the rest of the clientcmd code to work as is. Or alternatively, as @DirectXMan12 mentioned, the current implementation is likely more in line with other components that use client-go, so I could see an argument to leave it as is.

Thoughts @DirectXMan12?

[DirectXMan12]

I'd lean towards detecting if $HOME is empty and, if so, falling back to user.Current. I think upstream should probably do that too, but that's a story for another day.