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

KEP-2133: kubelet credential provider plugins #2151

Merged
merged 1 commit into from
Jan 8, 2021
Merged

KEP-2133: kubelet credential provider plugins #2151

merged 1 commit into from
Jan 8, 2021

Conversation

andrewsykim
Copy link
Member

Signed-off-by: Andrew Sy Kim kim.andrewsy@gmail.com

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 13, 2020
@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/node Categorizes an issue or PR as relevant to SIG Node. labels Nov 13, 2020
@andrewsykim andrewsykim force-pushed the kubelet-credential-provider branch 2 times, most recently from 163b349 to 17cee38 Compare November 13, 2020 20:11
@andrewsykim
Copy link
Member Author

/assign @cheftako @liggitt @derekwaynecarr

@kikisdeliveryservice kikisdeliveryservice mentioned this pull request Nov 13, 2020
Closed
4 tasks
@kikisdeliveryservice
Copy link
Member

Thanks for updating this as requested for your 1.20 Exception Request!

liggitt
liggitt reviewed Nov 13, 2020
View reviewed changes
Copy link
Member

@liggitt liggitt left a comment

Choose a reason for hiding this comment

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

A few nits, looks good overall for alpha.

keps/sig-cloud-provider/20191004-out-of-tree-credential-providers.md Outdated
@@ -20,7 +20,7 @@ approvers:
editor: TBD
creation-date: 2019-10-04
last-updated: 2019-12-10
status: implementable
status: replaced
Copy link
Member

Choose a reason for hiding this comment

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

point at the replacement?

keps/sig-node/2133-kubelet-credential-providers/README.md Outdated

### Upgrade / Downgrade Strategy

This feature is feature gated so explicit opt-in is required on upgrade and explcit opt-out is required on downgrade.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This feature is feature gated so explicit opt-in is required on upgrade and explcit opt-out is required on downgrade.
This feature is feature gated so explicit opt-in is required on upgrade and explicit opt-out is required on downgrade.

keps/sig-node/2133-kubelet-credential-providers/README.md Outdated
Comment on lines 451 to 452
Yes, but not from kubelet directly. The plugin invoked by the kubelet may be responsible
for making new requests to the cloud provider.
Copy link
Member

Choose a reason for hiding this comment

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

I would say no here. The feature does not make calls to a cloud provider, and different plugin implementations may or may not.

keps/sig-node/2133-kubelet-credential-providers/README.md Outdated

* **How does this feature react if the API server and/or etcd is unavailable?**

TBD for beta.
Copy link
Member

Choose a reason for hiding this comment

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

this feature has no dependencies on the API server or etcd

keps/sig-node/2133-kubelet-credential-providers/kep.yaml Outdated
Comment on lines 46 to 48
# The following PRR answers are required at beta release
metrics:
- my_feature_metric
Copy link
Member

Choose a reason for hiding this comment

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

comment out until populated

keps/sig-node/2133-kubelet-credential-providers/README.md Outdated
Comment on lines 43 to 44
- [ ] (R) Test plan is in place, giving consideration to SIG Architecture and SIG Testing input
- [ ] (R) Graduation criteria is in place
Copy link
Member

Choose a reason for hiding this comment

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

these are in place for alpha

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 24, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 6, 2021
@andrewsykim
KEP-2133: kubelet credential provider plugins
e882a17 
Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>
@derekwaynecarr
Copy link
Member

looks good for sig-node and captures present state.

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 8, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 Jan 8, 2021
@k8s-ci-robot k8s-ci-robot merged commit 48a70c6 into kubernetes:master Jan 8, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.21 milestone Jan 8, 2021
DangerOnTheRanger
DangerOnTheRanger reviewed Jan 12, 2021
View reviewed changes
keps/sig-node/2133-kubelet-credential-providers/README.md

* in contrast to existing built-in implementations, credentials for a image registry is now passed
through stdio of a process invoked by the kubelet, as opposed to those credentials only remaining in-memory.
* exec-ing plugins for image credentials can be expensive for the kubelet.
Copy link
Contributor

Choose a reason for hiding this comment

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

Might this be an appropriate place to mention the strategy that the current implementation takes of timing out plugins after 1 minute to prevent plugin processes from becoming too long-lived? LGTM otherwise, I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! Will make sure to include that for the v1.21 beta update of this KEP :)

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

@liggitt liggitt liggitt left review comments

@DangerOnTheRanger DangerOnTheRanger DangerOnTheRanger left review comments

@cheftako cheftako Awaiting requested review from cheftako

@dchen1107 dchen1107 Awaiting requested review from dchen1107

Assignees

@cheftako cheftako

@liggitt liggitt

@derekwaynecarr derekwaynecarr

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/node Categorizes an issue or PR as relevant to SIG Node. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v1.21
Development

Successfully merging this pull request may close these issues.
7 participants
@andrewsykim @kikisdeliveryservice @derekwaynecarr @k8s-ci-robot @liggitt @DangerOnTheRanger @cheftako