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

Fix setting kubeContext in skaffold #6024

Merged
merged 2 commits into from
Jun 24, 2021

Conversation

gsquared94
Copy link
Contributor

@gsquared94 gsquared94 commented Jun 16, 2021

Fixes: #5334, #4347

Description

Skaffold has multiple ways to set the current cluster context and it does not work as intended. The main issues are:

  • The global config tries to read the kubeconfig before the flags --kube-context and --kubeconfig have been processed, and stores a singleton result that doesn't get recomputed ever.
  • The runcontext tries to figure out if the cluster is local or remote heuristically from the kubecontext but even this happens before the flags --kube-context and --kubeconfig have been processed
  • These flags are only processed after all skaffold configs have been parsed and this is to support reading a third source of kubecontext from the skaffold config's Deploy.KubeContext field.

This PR fixes these issues and will result in the following state:

  • Current set kubecontext is always overridden by the flags --kube-context and --kubeconfig and as soon as the command starts.
  • We no longer treat the value provided in a skaffold config's Deploy.KubeContext as an override for the entire process' kubecontext value. Rather it only overrides the kubecontext provided to each deployer only at the time of deployment.
  • This allows deploying to different kubecontexts in different skaffold modules.

Follow up work:

  • Defining multiple kubeContext in different skaffold configs in a multi-config project is still disallowed with an artificial check as before, since it needs further changes to the Logger and PortForwarder constructs.

@google-cla google-cla bot added the cla: yes label Jun 16, 2021
@gsquared94 gsquared94 added this to the v1.27.0 milestone Jun 16, 2021
Copy link
Contributor

@tejal29 tejal29 left a comment

Choose a reason for hiding this comment

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

  • Current set kubecontext is always overridden by the flags --kube-context and --kubeconfig and as soon as the command starts.
  • We no longer treat the value provided in a skaffold config's Deploy.KubeContext as an override for the entire process' kubecontext value. Rather it only overrides the kubecontext provided to each deployer only at the time of deployment.

If this is the case, should we even evaluate kubecontextat the start of the run via congifure.Once?
Could we not just create the runner with kubeContext either from CLI or current config selected by user. Once this is done for all command requiring deploys, skaffold code should only rely on Runcontext.KubeContext field?

Comment on lines +98 to +101
// Setup kubeContext and kubeConfig
kubectx.ConfigureKubeConfig(opts.KubeConfig, opts.KubeContext)

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel this should happen only for dev, run and deploy commands.
Not sure if this is the current behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unfortunately we evaluate the global config for all skaffold commands when we evaluate config.ShouldDisplaySurveyPrompt. We need to interpret the resolved kubeContext prior to evaluating the global config. So we need to do this check here also.

pkg/skaffold/kubernetes/context/context.go Outdated Show resolved Hide resolved
if cliKubeContext != "" {
newKubeContext = cliKubeContext
}
func ConfigureKubeConfig(cliKubeConfig, cliKubeContext string) {
configureOnce.Do(func() {
Copy link
Member

Choose a reason for hiding this comment

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

In an ideal world, pkg/skaffold should be treated like a library, and these values should be computed in cmd/skaffold and provided into the library.

There are also the KUBECONFIG environment variable that should influencing this decision (and note that it is a PATH-like variable that can specify multiple files).

@codecov
Copy link

codecov bot commented Jun 24, 2021

Codecov Report

Merging #6024 (719304b) into master (d658049) will decrease coverage by 0.06%.
The diff coverage is 68.51%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6024      +/-   ##
==========================================
- Coverage   70.25%   70.19%   -0.07%     
==========================================
  Files         473      475       +2     
  Lines       18110    18147      +37     
==========================================
+ Hits        12724    12739      +15     
- Misses       4454     4471      +17     
- Partials      932      937       +5     
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/runner.go 57.69% <ø> (-0.80%) ⬇️
pkg/skaffold/deploy/kubectl/cli.go 88.09% <ø> (ø)
pkg/skaffold/kubernetes/status/status_check.go 49.68% <ø> (ø)
pkg/skaffold/runner/deployer.go 61.36% <61.36%> (ø)
cmd/skaffold/app/cmd/cmd.go 68.57% <100.00%> (+0.18%) ⬆️
pkg/skaffold/deploy/helm/deploy.go 78.22% <100.00%> (ø)
pkg/skaffold/deploy/kpt/kpt.go 80.92% <100.00%> (ø)
pkg/skaffold/deploy/kubectl/kubectl.go 68.31% <100.00%> (ø)
pkg/skaffold/deploy/kustomize/kustomize.go 74.34% <100.00%> (ø)
pkg/skaffold/kubernetes/context/context.go 78.94% <100.00%> (-2.45%) ⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d658049...719304b. Read the comment docs.

@gsquared94 gsquared94 changed the title [WIP] Fix setting kubeContext in skaffold Fix setting kubeContext in skaffold Jun 24, 2021
@gsquared94 gsquared94 marked this pull request as ready for review June 24, 2021 13:58
@gsquared94 gsquared94 requested review from yuwenma and a team as code owners June 24, 2021 13:58
@gsquared94 gsquared94 linked an issue Jun 24, 2021 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[1.17+] Build does not push images to remote repository kube-context / kubeconfig not applied
3 participants