Skip to content

Conversation

@aramase
Copy link
Member

@aramase aramase commented Sep 12, 2024

  • One-line PR description: Add Projected Service Account Tokens for Kubelet Image Credential Providers alpha KEP

@k8s-ci-robot k8s-ci-robot added 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 sig/auth Categorizes an issue or PR as relevant to SIG Auth. labels Sep 12, 2024
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 12, 2024
@aramase aramase force-pushed the aramase/f/kep_4412_alpha branch 2 times, most recently from 335b23c to 65a5019 Compare September 12, 2024 06:39
@aramase aramase force-pushed the aramase/f/kep_4412_alpha branch 2 times, most recently from 581f7e2 to 049905b Compare September 30, 2024 07:50
@aramase
Copy link
Member Author

aramase commented Sep 30, 2024

/cc @enj

@k8s-ci-robot k8s-ci-robot requested a review from enj September 30, 2024 07:52
@aramase
Copy link
Member Author

aramase commented Sep 30, 2024

@aramase aramase force-pushed the aramase/f/kep_4412_alpha branch from 049905b to fd08023 Compare September 30, 2024 07:54
@aramase aramase force-pushed the aramase/f/kep_4412_alpha branch from 19842b8 to 2c0f66f Compare October 2, 2024 16:13
@aramase aramase force-pushed the aramase/f/kep_4412_alpha branch from 2c0f66f to 1eb7a85 Compare October 3, 2024 14:42
@aramase
Copy link
Member Author

aramase commented Oct 3, 2024

/assign @deads2k
for PRR review

/assign enj liggitt haircommander

@aramase aramase force-pushed the aramase/f/kep_4412_alpha branch from 1eb7a85 to c44af73 Compare October 3, 2024 16:48
Copy link
Member

@enj enj left a comment

Choose a reason for hiding this comment

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

First pass.

@aramase aramase force-pushed the aramase/f/kep_4412_alpha branch from 72d5fe0 to 49094d3 Compare October 4, 2024 20:13
We will expand the on-disk kubelet credential provider configuration to allow an
optional `tokenAttribute` field to be configured. When this field is not set, no KSA
token will be sent to the plugin. When it is set, the Kubelet will provision
a token with the given audience bound to the current pod and its service
Copy link
Member

Choose a reason for hiding this comment

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

will the token be time-bound? If so - how long?

Copy link
Member

Choose a reason for hiding this comment

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

will it work nicely with things like stargz snapshotter where secret can be stored for a long time like: https://github.com/containerd/stargz-snapshotter/blob/main/docs/overview.md#cri-based-authentication?

cc: @samuelkarp

Copy link
Member Author

Choose a reason for hiding this comment

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

will the token be time-bound? If so - how long?

Yes, the token will be time-bound. The duration will be fixed at 1h (?). If the cloud provider associates this token duration to the credential lifetime, we don't want it to be too short-lived. 1h seems reasonable to cover those scenarios to prevent frequent calls to cloud provider for token exchange.

Copy link
Member Author

Choose a reason for hiding this comment

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

will it work nicely with things like stargz snapshotter where secret can be stored for a long time like: https://github.com/containerd/stargz-snapshotter/blob/main/docs/overview.md#cri-based-authentication?

IIUC, the stargz snapshotter in CRI-based authentication gets the same registry credentials as the CRI from kubelet, so that wouldn't change here? The credentials could now be short-lived if the lifetime is tied to the KSA token lifetime but otherwise should work the same way. Please correct me if I'm wrong.

Copy link
Member

Choose a reason for hiding this comment

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

stargz is not downloading the whole image in one go. So it may keep streaming pieces after 1h. So it will end up with the stale credentials

Copy link
Member Author

Choose a reason for hiding this comment

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

stargz is not downloading the whole image in one go. So it may keep streaming pieces after 1h. So it will end up with the stale credentials

The credential provider plugin gets the PSAT and exchanges that for a registry credential. The registry credential is the one that stargz uses and it's not the PSAT right?

Copy link
Contributor

Choose a reason for hiding this comment

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

correct. I think this can be compared to current behavior without specific PSAT: if a node wide credential expires while the image is being pulled with stargz, what does the runtime do? I'd expect the same here.

Copy link
Member

Choose a reason for hiding this comment

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

oh, ok. So we are not giving time limited token to runtime. If this is the case, it will work

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if there is any test we can write that will validate that and ensure we are not breaking this scenario?

Copy link
Member Author

Choose a reason for hiding this comment

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

The tests in kubelet credential provider wouldn't validate this because it fetches the registry credentials and passes it to CRI (no change in behavior).

Tests in stargz would be good to confirm it works as expected when the registry credentials expire/it updates its cache when the credentials for the image change.
@SergeyKanzhelev do you know who can confirm if there are tests for this in stargz?

@aramase aramase force-pushed the aramase/f/kep_4412_alpha branch from 49094d3 to 9a88044 Compare October 7, 2024 14:52
@aramase aramase requested review from SergeyKanzhelev and enj October 7, 2024 14:53
Copy link
Contributor

@deads2k deads2k left a comment

Choose a reason for hiding this comment

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

some updates for beta criteria and PRR please.

@aramase aramase force-pushed the aramase/f/kep_4412_alpha branch 2 times, most recently from 6416856 to b124de2 Compare October 8, 2024 01:51
@aramase aramase requested a review from deads2k October 8, 2024 01:52
Copy link
Member

@enj enj left a comment

Choose a reason for hiding this comment

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

Looks inverted?

@aramase aramase force-pushed the aramase/f/kep_4412_alpha branch from 15baea4 to 9b15829 Compare October 8, 2024 19:27
@aramase aramase requested review from deads2k and enj October 8, 2024 21:49
@aramase aramase force-pushed the aramase/f/kep_4412_alpha branch from 9c027e3 to 6a0f049 Compare October 8, 2024 22:07
@deads2k
Copy link
Contributor

deads2k commented Oct 9, 2024

PRR lgtm. KEP also lgtm, but I'll leave lgtm to @enj and whoever is looking from node.

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 9, 2024
Copy link
Member

@enj enj left a comment

Choose a reason for hiding this comment

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

typo

…ential Providers

Signed-off-by: Anish Ramasekar <anish.ramasekar@gmail.com>
@aramase aramase force-pushed the aramase/f/kep_4412_alpha branch from 6a0f049 to 57865a0 Compare October 9, 2024 19:49
@enj
Copy link
Member

enj commented Oct 9, 2024

SIG node LGTM: #4846 (comment)

/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 Oct 9, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aramase, deads2k, enj

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 merged commit 3e3a2a0 into kubernetes:master Oct 9, 2024
4 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.32 milestone Oct 9, 2024
@aramase aramase deleted the aramase/f/kep_4412_alpha branch October 9, 2024 19:55
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. 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/auth Categorizes an issue or PR as relevant to SIG Auth. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

9 participants