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.

Sign up for GitHub

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

Jump to bottom

kubelet: add alpha credential provider plugins #94196

Merged
merged 9 commits into from
Nov 12, 2020
Merged

kubelet: add alpha credential provider plugins #94196

merged 9 commits into from
Nov 12, 2020

Conversation

andrewsykim
Copy link
Member

@andrewsykim andrewsykim commented Aug 24, 2020 •
edited
Loading

What type of PR is this?
/kind feature

What this PR does / why we need it:
Support kubelet credential provider exec plugins that will replace the built-in cloud-based credential providers. This PR introduces the alpha implementation of the external credential providers KEP.

This PR also supersedes #88813

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Introduce alpha support for exec-based container registry credential provider plugins in the kubelet.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Signed-off-by: Andrew Sy Kim kim.andrewsy@gmail.com
Co-authored-by: Nick Turner nic@amazon.com

@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. release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/kubelet kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/node Categorizes an issue or PR as relevant to SIG Node. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 24, 2020
@andrewsykim andrewsykim force-pushed the registry-creds branch 3 times, most recently from b780bcb to b9bba71 Compare August 27, 2020 01:18
@andrewsykim
Copy link
Member Author

andrewsykim commented Nov 10, 2020 •
edited
Loading

/retest

(weird git issue)

@andrewsykim
kubelet: add CredentialProviderConfig API
359c6e2 
Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>
@andrewsykim
kubelet: update pkg/kubelet/apis/config/OWNERS to include api approve…
c23638c 
…rs and reviewers

Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>
@andrewsykim
kubelet: support alpha credential provider exec plugins
51441fd 
Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>
@andrewsykim
feature gates: add KubeletCredentialProviders feature gate
ab04386 
Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>
@andrewsykim
kubelet: add initial credentialprovider v1alpha1 APIs
2d0dd26 
Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>
@andrewsykim
pkg/credentialprovider: add initial exec-based credential provider pl…
5344afd 
…ugin

Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>
@andrewsykim
pkg/credentialprovider: export URL parsing and matching helper functions
aadc1d2 
Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>
@andrewsykim
hack/.golint: ignore golint for new kubelet and credentialprovider AP…
91aae6e 
…Is package

Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>
@andrewsykim
import restrictions: allow k8s.io/kubelet to import credentialprovider
f3192c3 
apis

Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>
@kikisdeliveryservice
Copy link
Member

As mentioned here this PR is related to a conditionally approved KEP which must be updated as requested in the exception approval for inclusion in 1.20.

@andrewsykim
Copy link
Member Author

As mentioned here this PR is related to a conditionally approved KEP which must be updated as requested in the exception approval for inclusion in 1.20.

Yup, I will update the KEP by end of week based on feedback I received during review.

@andrewsykim
Copy link
Member Author

As mentioned here this PR is related to a conditionally approved KEP which must be updated as requested in the exception approval for inclusion in 1.20.

Just for the record, the KEP was approved in a prior release (v1.17) but it was missing an official tracking issue, hence the exception process was required with the newly created issue.

derekwaynecarr
derekwaynecarr approved these changes Nov 11, 2020
View reviewed changes
Copy link
Member

@derekwaynecarr derekwaynecarr left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

pkg/kubelet/config/flags.go
@@ -65,6 +65,20 @@ type ContainerRuntimeOptions struct {
// CNICacheDir is the full path of the directory in which CNI should store
// cache files
CNICacheDir string

// Image credential provider plugin options
Copy link
Member

Choose a reason for hiding this comment

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

thinking about @liggitt comment: re relative to root-dir.

if it were relative, i would assume we would not have to expose a flag at all in favor of prescribed default.

i do not think we know enough yet to say if that makes sense as its not clear to me how cloud vendors may configure their default os image building one way or the other, and that varies with each cloud. as a result, a separate flag seems fine for moment.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 11, 2020
@liggitt
Copy link
Member

liggitt commented Nov 11, 2020

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim, derekwaynecarr, liggitt

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 11, 2020
@andrewsykim
Copy link
Member Author

/retest

1 similar comment
@andrewsykim
Copy link
Member Author

/retest

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Nov 12, 2020 •
edited
Loading

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

Test name Commit Details Rerun command
pull-kubernetes-e2e-aks-engine-azure 61d02de092902456ce22973508eeaedfbecce5ff link /test pull-kubernetes-e2e-aks-engine-azure
pull-kubernetes-e2e-azure-disk 61d02de092902456ce22973508eeaedfbecce5ff link /test pull-kubernetes-e2e-azure-disk

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.

@andrewsykim
Copy link
Member Author

/retest

@k8s-ci-robot k8s-ci-robot merged commit d233111 into kubernetes:master Nov 12, 2020
@github-actions github-actions bot mentioned this pull request Nov 18, 2020
Open
This was referenced Nov 19, 2020
Merged
Closed
@DangerOnTheRanger DangerOnTheRanger mentioned this pull request Dec 23, 2020
Merged
@DangerOnTheRanger DangerOnTheRanger mentioned this pull request Feb 1, 2021
Closed
@dims dims mentioned this pull request Jan 7, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SergeyKanzhelev SergeyKanzhelev SergeyKanzhelev left review comments

@andyzhangx andyzhangx andyzhangx left review comments

@DangerOnTheRanger DangerOnTheRanger DangerOnTheRanger left review comments

@liggitt liggitt liggitt left review comments

@cheftako cheftako cheftako left review comments

@samuelkarp samuelkarp samuelkarp left review comments

@feiskyer feiskyer feiskyer left review comments

@nckturner nckturner nckturner requested changes

@derekwaynecarr derekwaynecarr derekwaynecarr approved these changes

@chrislovecnm chrislovecnm Awaiting requested review from chrislovecnm

Assignees

@cheftako cheftako

@liggitt liggitt

@mikedanese mikedanese

@derekwaynecarr derekwaynecarr

@dchen1107 dchen1107

@SergeyKanzhelev SergeyKanzhelev

@tallclair tallclair

Labels
api-review Categorizes an issue or PR as actively needing an API review. approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet area/release-eng Issues or PRs related to the Release Engineering subproject cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/release Categorizes an issue or PR as relevant to SIG Release. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.20
Development

Successfully merging this pull request may close these issues.