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

kubeadm-token: search for existing kubeconfig files #62850

Merged
merged 1 commit into from May 9, 2018

Conversation

@neolit123
Member

neolit123 commented Apr 19, 2018

What this PR does / why we need it:

Add means to search the current user home path and
the environment variable KUBECONFIG for existing files if the
user does not provide a --kubeconfig flag.

If the user provides a --kubeconfig flag respect it.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes kubernetes/kubeadm#198

Special notes for your reviewer:

i'm not 100% sure if that's the correct approach and if that's what's requested. so let's discuss it.

Release note:

Search standard KubeConfig file locations when using `kubeadm token` without `--kubeconfig`.
@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Apr 19, 2018

Hi @neolit123. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

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.

@neolit123

This comment has been minimized.

Member

neolit123 commented Apr 19, 2018

@neolit123

This comment has been minimized.

Member

neolit123 commented Apr 19, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Apr 19, 2018

@neolit123: Reiterating the mentions to trigger a notification:
@kubernetes/sig-cluster-lifecycle-pr-reviews

In response to this:

/cc @kubernetes/sig-cluster-lifecycle-pr-reviews

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.

@@ -381,3 +392,32 @@ func getClientset(file string, dryRun bool) (clientset.Interface, error) {
}
return kubeconfigutil.ClientSetFromFile(file)
}
func findExistingKubeConfig(file string) string {

This comment has been minimized.

@timothysc

timothysc Apr 19, 2018

Member

Most of this code exists in clientcmd.NewNonInteractiveDeferredLoadingClientConfig and should be leveraged using the client tooling.

@@ -85,7 +92,7 @@ func NewCmdToken(out io.Writer, errW io.Writer) *cobra.Command {
}
tokenCmd.PersistentFlags().StringVar(&kubeConfigFile,
"kubeconfig", "/etc/kubernetes/admin.conf", "The KubeConfig file to use when talking to the cluster")
"kubeconfig", defaultKubeConfig, "The KubeConfig file to use when talking to the cluster. If the flag is not set the environment variable "+kubeConfigEnv+" and the user home directory and will be searched. If no existing KubeConfig files are found there the default location "+defaultKubeConfig+" will be used.")

This comment has been minimized.

@xiangpengzhao

xiangpengzhao Apr 20, 2018

Member

If the flag is not set the environment variable "+kubeConfigEnv+" and the user home directory and will be searched.

I don't understand this. Guess the last and is extra?

If no existing KubeConfig files are found there the default location "+defaultKubeConfig+" will be used."

I'm not sure if defaultKubeConfig can be printed in this format. Let's see if the CI will be happy.

/ok-to-test

This comment has been minimized.

@neolit123

neolit123 Apr 20, 2018

Member

yes, there is an extra and - typo.
i will probably replace this comment based on what clientcmd.NewNonInteractiveDeferredLoadingClientConfig (what a name...) does exactly behind the lines. on a quick look the cmdutil.GetJoinCommand call might have a problem with that...

@neolit123

This comment has been minimized.

Member

neolit123 commented Apr 20, 2018

@xiangpengzhao @timothysc

updated the cobra flag description and also clientcmd.NewDefaultClientConfigLoadingRules() is now used to probe the env. variables and user home path.

for ref:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/tools/clientcmd/loader.go

@timothysc

/assign @timothysc
/approve

Could you please add a UT then lgtm

@neolit123

This comment has been minimized.

Member

neolit123 commented Apr 20, 2018

Could you please add a UT then lgtm

ok, i need the other UT PR for token merged as a basis.

@neolit123

This comment has been minimized.

Member

neolit123 commented Apr 24, 2018

link for my other PR that is needed before this:
#59220

@k8s-ci-robot k8s-ci-robot added size/M and removed size/S labels Apr 26, 2018

kubeadm-token: search for existing kubeconfig files
Use clientcmd.NewDefaultClientConfigLoadingRules()
to search the standard locations for existing KubeConfig
files if the user did not provide a --kubeconfig flag.

If the user provides a --kubeconfig flag respect it.

Also do the following:
- Add a unit test in token_test.go for the
NewDefaultClientConfigLoadingRules() functionality.
- Rename the TestConfig and related constants to make
them unique to this file.

Fixes kubernetes/kubeadm#198
@neolit123

This comment has been minimized.

Member

neolit123 commented Apr 26, 2018

@timothysc

update:

  • added a unit test for the NewDefaultClientConfigLoadingRules() call using kubeadm token delete by setting a KUBECONFIG env. variable and not passing --kubeconfig.
  • renamed some variables in token_test.go to make them more unique.
@timothysc

/lgtm

thx for the patch + update.

@k8s-ci-robot k8s-ci-robot added the lgtm label May 7, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented May 8, 2018

@fejta-bot: you can't request testing unless you are a kubernetes member.

In response to this:

/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 comment for consistent failures.

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.

@fejta-bot

This comment has been minimized.

fejta-bot commented May 8, 2018

/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 comment for consistent failures.

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented May 8, 2018

@fejta-bot: you can't request testing unless you are a kubernetes member.

In response to this:

/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 comment for consistent failures.

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.

@fejta-bot

This comment has been minimized.

fejta-bot commented May 8, 2018

/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 comment for consistent failures.

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented May 8, 2018

@fejta-bot: you can't request testing unless you are a kubernetes member.

In response to this:

/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 comment for consistent failures.

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.

@fejta-bot

This comment has been minimized.

fejta-bot commented May 8, 2018

/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 comment for consistent failures.

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented May 8, 2018

@fejta-bot: you can't request testing unless you are a kubernetes member.

In response to this:

/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 comment for consistent failures.

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.

@fejta-bot

This comment has been minimized.

fejta-bot commented May 9, 2018

/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 comment for consistent failures.

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented May 9, 2018

@fejta-bot: you can't request testing unless you are a kubernetes member.

In response to this:

/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 comment for consistent failures.

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.

@fejta-bot

This comment has been minimized.

fejta-bot commented May 9, 2018

/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 comment for consistent failures.

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented May 9, 2018

@fejta-bot: you can't request testing unless you are a kubernetes member.

In response to this:

/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 comment for consistent failures.

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.

@fejta-bot

This comment has been minimized.

fejta-bot commented May 9, 2018

/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 comment for consistent failures.

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented May 9, 2018

@fejta-bot: you can't request testing unless you are a kubernetes member.

In response to this:

/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 comment for consistent failures.

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.

@fejta-bot

This comment has been minimized.

fejta-bot commented May 9, 2018

/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 comment for consistent failures.

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented May 9, 2018

@fejta-bot: you can't request testing unless you are a kubernetes member.

In response to this:

/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 comment for consistent failures.

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.

@fejta-bot

This comment has been minimized.

fejta-bot commented May 9, 2018

/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 comment for consistent failures.

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented May 9, 2018

@fejta-bot: you can't request testing unless you are a kubernetes member.

In response to this:

/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 comment for consistent failures.

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.

@fejta-bot

This comment has been minimized.

fejta-bot commented May 9, 2018

/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 comment for consistent failures.

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented May 9, 2018

@fejta-bot: you can't request testing unless you are a kubernetes member.

In response to this:

/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 comment for consistent failures.

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.

@dims

This comment has been minimized.

Member

dims commented May 9, 2018

/retest

@fejta-bot

This comment has been minimized.

fejta-bot commented May 9, 2018

/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 comment for consistent failures.

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented May 9, 2018

@fejta-bot: you can't request testing unless you are a kubernetes member.

In response to this:

/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 comment for consistent failures.

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.

@BenTheElder

This comment has been minimized.

Member

BenTheElder commented May 9, 2018

/ok-to-test

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented May 9, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@neolit123

This comment has been minimized.

Member

neolit123 commented May 9, 2018

/test pull-kubernetes-e2e-gce

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented May 9, 2018

Automatic merge from submit-queue (batch tested with PRs 62850, 63504). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 24e5265 into kubernetes:master May 9, 2018

14 of 16 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
pull-kubernetes-e2e-gce Job triggered.
Details
cla/linuxfoundation neolit123 authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
@@ -50,6 +51,8 @@ import (
api "k8s.io/kubernetes/pkg/apis/core"
)
const defaultKubeConfig = "/etc/kubernetes/admin.conf"

This comment has been minimized.

@luxas

luxas May 11, 2018

Member

Could you put this in constants.go?

func findExistingKubeConfig(file string) string {
// The user did provide a --kubeconfig flag. Respect that and threat it as an
// explicit path without building a DefaultClientConfigLoadingRules object.
if file != defaultKubeConfig {

This comment has been minimized.

@luxas

luxas May 11, 2018

Member

@neolit123 can you move this function to cmd/kubeadm/app/cmd/util please?
We have other places in kubeadm where it's used as a Kubernetes client tool, and we could/should leverage the same functionality to be consistent

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment