Add CLI option `--kube-context` to override the kubecontext in Skaffold by corneliusweig · Pull Request #2447 · GoogleContainerTools/skaffold

corneliusweig · 2019-07-10T19:57:51Z

This PR adds a new CLI option --kube-context to Skaffold run, dev, debug, delete, deploy, and build. It's the frist step in providing several options to override the kubecontext to use in Skaffold.

See #2384
~~Depends on #2509~~

corneliusweig · 2019-07-10T20:00:00Z

pkg/skaffold/kubernetes/context/context.go

-	currentConfigErr  error
+	kubeConfigOnce sync.Once
+	kubeConfig     clientcmd.ClientConfig
+	kubeContext    string


I'm a bit sad about introducing some global state here. However, I have considered several alternatives and this appeared to be the one with the least head-ache overall. Let me know your thoughts...

it was already a global state, it is fine.
Maybe in the future we can think about how to combine it with RunContext.

I was already investigating in that direction. That requires to pass down the RunContext in quite a few places, so I decided against it. But it's a good idea nevertheless.

codecov · 2019-07-10T20:46:15Z

Codecov Report

Merging #2447 into master will increase coverage by 0.03%.
The diff coverage is 96.15%.

Impacted Files	Coverage Δ
pkg/skaffold/config/options.go	`100% <ø> (ø)`	⬆️
cmd/skaffold/app/cmd/flags.go	`100% <ø> (ø)`	⬆️
pkg/skaffold/kubernetes/context/context.go	`100% <100%> (ø)`	⬆️
pkg/skaffold/docker/parse.go	`87.36% <100%> (ø)`	⬆️
cmd/skaffold/app/cmd/cmd.go	`68.25% <100%> (+0.25%)`	⬆️
pkg/skaffold/kubernetes/client.go	`40% <50%> (-18.83%)`	⬇️

cmd/skaffold/app/cmd/flags.go

balopat · 2019-07-12T18:53:17Z

pkg/skaffold/kubernetes/context/context.go

-	currentConfigErr  error
+	kubeConfigOnce sync.Once
+	kubeConfig     clientcmd.ClientConfig
+	kubeContext    string


it was already a global state, it is fine.
Maybe in the future we can think about how to combine it with RunContext.

pkg/skaffold/kubernetes/context/context.go

balopat · 2019-07-12T20:16:35Z

pkg/skaffold/kubernetes/context/context.go

+
+// getCurrentConfig retrieves the kubeconfig file. If UseKubeContext was called before, the CurrentContext will be overridden.
 func getCurrentConfig() (clientcmdapi.Config, error) {
-	currentConfigOnce.Do(func() {


why did we change the semantics of the currentConfigOnce?
Doesn't this open up the possibilities of inconsitencies in a skaffold process that started off version1 of a kubeconfig file but then someone mutated that file and a second read would be of version2?

The once-semantic is now encapsulated in getKubeConfig. So for the moment we are safe here. However, in the future when the kube-context is also overridden by skaffold.yaml, we will have to re-read the kubeconfig. This will occur during the skaffold.yaml parsing/profile phase and should not take very long. Do you think Skaffold should take extra precautions to work with the identical kubeconfig as in the first kubeconfig read?

I found a nice way to ensure that Skaffold always uses the kubeconfig from the first read. Note that this has not been the case with respect to the kubernetes REST client so far, and is much more consistent now.

Note that there is still the possibility, that the kubecontext changes due to a change in skaffold.yaml (via UseKubeContext). However, that is the desired behavior.

balopat · 2019-07-18T05:52:28Z

I will have a look tomorrow - but as a tip: can you rebase on top of master? Sometimes changes might impact Travis vs local - as Travis does merge master in your branch. It does look odd that test-dev is around, I see you added logs to see if it gets cleaned up from the other test...

corneliusweig · 2019-07-18T07:03:09Z

@balopat 🤦‍♂️ Thanks that gives me a lead as to what is going wrong. You don't have to waste your time, I can debug further now.

corneliusweig · 2019-07-18T08:40:35Z

Alright, I found what is going on:
When my new test-case runs, the deployment test-dev from the previous test is still around. This deployment is erroneously picked up by pkg/skaffold/deploy/status_check.go:80 (the namespace in my test is "", which means all namespaces). From there on, the test is doomed.

I think this is mainly a problem how the status_check code determines the deployments to consider for watching. It would be better, if the status-check was looking for a UUID label which is generated for each Skaffold run. @tejal29 WDYT?

EDIT I opened #2496 with more details about this issue.

balopat

I tried out locally with merging with the latest from master and I'm having port forwarding issues.

skaffold dev --default-repo gcr.io/balintp-gcp-lab --rpc-http-port 8080 --port-forward=true on microservices example times out. Maybe we're missing a kubecontext passed down there somewhere?

balopat · 2019-07-19T23:43:27Z

yup, we're missing kubecontext here: https://github.com/GoogleContainerTools/skaffold/blob/master/pkg/skaffold/kubernetes/portforward/kubectl_forwarder.go#L54

It could be useful to just do a mechanical search for anything kubectl and double check if kubecontext is missing.

corneliusweig · 2019-07-20T22:35:04Z

Oh no! I wasn't aware that so many places called kubectl directly without going through pkg/skaffold/deploy/kubectl. I think the cleaner approach would be to consistently use our wrapper to call kubectl, but I will have to see how much refactoring this will require.

Thanks for catching this early!

balopat

LGTM after rebase.

corneliusweig · 2019-08-14T22:35:44Z

Rebase is done. Thanks everyone for reviewing and helping out!

Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>

Many places in the code assume that kubeConfig.CurrentContext is the correct kubeContext to use. In order to avoid passing down the configured value, the cached kubeConfig is modified with the true kubeContext to use. For that, an override kubeContext is configured early on. Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>

This ensures that overridden defaults are consistent when using the kubeconfig and the REST client. Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>

Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>

… kubeconfig Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>

Fix gocritic issue

corneliusweig · 2019-08-15T09:00:28Z

Looks like another glitch in the CI. I've rebased one more time :)

tejal29 · 2019-08-15T15:48:30Z

Sorry @corneliusweig, we changed from travis-ci.org to premium. We cancelled all jobs in the old free travis and hence you must have seen a failure.
You job is green on Travis. I just started kokoro job

corneliusweig requested review from balopat, dgageot, nkubala, priyawadhwa and tejal29 as code owners July 10, 2019 19:57

googlebot added the cla: yes label Jul 10, 2019

corneliusweig changed the title ~~Add CLI option to override the kubecontext~~ Add CLI option --kube-context to override the kubecontext in Skaffold Jul 10, 2019

corneliusweig commented Jul 10, 2019

View reviewed changes

balopat added the kokoro:run runs the kokoro jobs on a PR label Jul 11, 2019

kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Jul 11, 2019

balopat suggested changes Jul 12, 2019

View reviewed changes

corneliusweig force-pushed the w/kubecontext/cli branch 4 times, most recently from af067a1 to f756516 Compare July 16, 2019 10:53

This comment has been minimized.

Sign in to view

corneliusweig force-pushed the w/kubecontext/cli branch from f756516 to f03f295 Compare July 18, 2019 07:33

kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Jul 18, 2019

corneliusweig force-pushed the w/kubecontext/cli branch from f03f295 to 35d3f77 Compare July 19, 2019 10:12

corneliusweig mentioned this pull request Jul 19, 2019

Integration tests with reduced privileges #2502

Closed

balopat added the kokoro:run runs the kokoro jobs on a PR label Jul 19, 2019

kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Jul 19, 2019

balopat approved these changes Jul 19, 2019

View reviewed changes

balopat suggested changes Jul 19, 2019

View reviewed changes

corneliusweig force-pushed the w/kubecontext/cli branch from 8c02de9 to b0c6771 Compare August 13, 2019 22:01

balopat added the kokoro:run runs the kokoro jobs on a PR label Aug 14, 2019

kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Aug 14, 2019

balopat approved these changes Aug 14, 2019

View reviewed changes

balopat added the kokoro:run runs the kokoro jobs on a PR label Aug 14, 2019

kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Aug 14, 2019

corneliusweig force-pushed the w/kubecontext/cli branch from 6ea807d to 19a7e44 Compare August 14, 2019 22:34

balopat closed this Aug 14, 2019

balopat reopened this Aug 14, 2019

balopat added the kokoro:run runs the kokoro jobs on a PR label Aug 14, 2019

kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Aug 14, 2019

corneliusweig added 9 commits August 15, 2019 11:00

Add --kube-context CLI flag

286738c

Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>

Also use cached kubeconfig for REST client calls

7b2c647

This ensures that overridden defaults are consistent when using the kubeconfig and the REST client. Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>

Reduce loglevel for dockerfile parser

c9ab8c5

Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>

Add integration test for CLI override of kubecontext

fa1177a

Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>

Use different context name when running on kind

7cb287a

Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>

Improvements from first review

69ceaa0

Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>

Use overridden kubecontext in REST ClientConfig without reloading the…

6b164d3

… kubeconfig Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>

Avoid calling context.CurrentConfig in integration-test

090374e

Fix gocritic issue

corneliusweig force-pushed the w/kubecontext/cli branch from 19a7e44 to 090374e Compare August 15, 2019 09:00

tejal29 added the kokoro:run runs the kokoro jobs on a PR label Aug 15, 2019

kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Aug 15, 2019

balopat added the kokoro:run runs the kokoro jobs on a PR label Aug 15, 2019

balopat merged commit ba24064 into GoogleContainerTools:master Aug 15, 2019

corneliusweig deleted the w/kubecontext/cli branch August 15, 2019 20:31

corneliusweig mentioned this pull request Oct 6, 2019

Add page about kube-context handling in docs concepts section #2992

Merged

7 tasks

Conversation

corneliusweig commented Jul 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

corneliusweig Jul 10, 2019

Choose a reason for hiding this comment

Uh oh!

balopat Jul 12, 2019

Choose a reason for hiding this comment

Uh oh!

corneliusweig Jul 12, 2019

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jul 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

balopat Jul 12, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

balopat Jul 12, 2019

Choose a reason for hiding this comment

Uh oh!

corneliusweig Jul 12, 2019

Choose a reason for hiding this comment

Uh oh!

corneliusweig Jul 13, 2019

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

balopat commented Jul 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

corneliusweig commented Jul 18, 2019

Uh oh!

corneliusweig commented Jul 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

balopat left a comment

Choose a reason for hiding this comment

Uh oh!

balopat commented Jul 19, 2019

Uh oh!

corneliusweig commented Jul 20, 2019

Uh oh!

balopat left a comment

Choose a reason for hiding this comment

Uh oh!

corneliusweig commented Aug 14, 2019

Uh oh!

corneliusweig commented Aug 15, 2019

Uh oh!

tejal29 commented Aug 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

corneliusweig commented Jul 10, 2019 •

edited

Loading

codecov bot commented Jul 10, 2019 •

edited

Loading

balopat commented Jul 18, 2019 •

edited

Loading

corneliusweig commented Jul 18, 2019 •

edited

Loading