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

Support authentication helper for kubectl #9667

Merged
merged 2 commits into from
Aug 31, 2020

Conversation

justinsb
Copy link
Member

@justinsb justinsb commented Aug 2, 2020

We create a simple exec plugin command which can create and renew
short-lived admin credentials on the fly, essentially leveraging the
security of the underlying cloud credentials.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 2, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb

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:

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

@justinsb
Copy link
Member Author

justinsb commented Aug 2, 2020

cc @johngmyers

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 2, 2020
"kubectl-auth",
"--cluster=" + clusterName,
"--state=" + kopsStateStore,
"--api-version=" + "v1alpha1",
Copy link
Member Author

Choose a reason for hiding this comment

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

We might be able to default to v1beta1 now

cmd/kops/export_kubecfg.go Outdated Show resolved Hide resolved
cmd/kops/main.go Show resolved Hide resolved
pkg/commands/helpers/kubectl_auth.go Outdated Show resolved Hide resolved
pkg/commands/helpers/kubectl_auth.go Outdated Show resolved Hide resolved
cacheFilePath := cacheFilePath(f.KopsStateStore(), options.ClusterName)
cached, err := loadCachedExecCredential(cacheFilePath)
if err != nil {
klog.Warningf("cached credential %q was not valid: %v", cacheFilePath, err)
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this and the wrong API version count as warnings.

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe this is when the credential is an error, not just expired, but you're right - going with Infof

pkg/commands/helpers/kubectl_auth.go Show resolved Hide resolved
pkg/commands/helpers/kubectl_auth.go Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 10, 2020
@justinsb justinsb force-pushed the kubectl_auth_helper branch from 6f5f714 to 0eb3860 Compare August 16, 2020 15:49
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 16, 2020
@justinsb justinsb force-pushed the kubectl_auth_helper branch from 0eb3860 to 6b307a0 Compare August 16, 2020 17:59
@justinsb justinsb changed the title WIP: Support authentication helper for kubectl Support authentication helper for kubectl Aug 16, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 16, 2020
@justinsb justinsb force-pushed the kubectl_auth_helper branch from 6b307a0 to bd2aaa7 Compare August 16, 2020 18:29
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 16, 2020
@justinsb
Copy link
Member Author

This seems to work, marking as no longer WIP. Thanks for the feedback @johngmyers and @rifelpet

Also added a test!

@justinsb justinsb force-pushed the kubectl_auth_helper branch from bd2aaa7 to 4827817 Compare August 16, 2020 18:38
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 17, 2020
@justinsb justinsb force-pushed the kubectl_auth_helper branch from 4827817 to 2b65dec Compare August 28, 2020 17:37
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 28, 2020
@justinsb justinsb force-pushed the kubectl_auth_helper branch 2 times, most recently from 85897f6 to d021004 Compare August 29, 2020 12:44
Copy link
Member

@johngmyers johngmyers left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold to give opportunity to address review comment

pkg/kubeconfig/create_kubecfg.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 29, 2020
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 29, 2020
@justinsb justinsb force-pushed the kubectl_auth_helper branch from d021004 to f7c1685 Compare August 30, 2020 14:08
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2020
justinsb and others added 2 commits August 30, 2020 15:16
We create a simple exec plugin command which can create and renew
short-lived admin credentials on the fly, essentially leveraging the
security of the underlying cloud credentials.

Co-authored-by: John Gardiner Myers <[email protected]>
Also slightly simplify the tests and Kubecfg Builder signature by
passing in the ConfigAccess only when needed.
@justinsb justinsb force-pushed the kubectl_auth_helper branch from f7c1685 to 8757a2c Compare August 30, 2020 19:19
@johngmyers
Copy link
Member

/retest

1 similar comment
@johngmyers
Copy link
Member

/retest

@johngmyers
Copy link
Member

/lgtm
/hold cancel

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Aug 30, 2020
@fejta-bot
Copy link

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

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Aug 31, 2020

@justinsb: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kops-e2e-k8s-containerd 4827817 link /test pull-kops-e2e-k8s-containerd

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@johngmyers
Copy link
Member

/retest

@k8s-ci-robot k8s-ci-robot merged commit 5d09a9a into kubernetes:master Aug 31, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone Aug 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/documentation cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants