-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
KEP-4412: Projected Service Account Tokens for Kubelet Image Credential Providers #4846
base: master
Are you sure you want to change the base?
Conversation
aramase
commented
Sep 12, 2024
- One-line PR description: Add Projected Service Account Tokens for Kubelet Image Credential Providers alpha KEP
- Issue link: Projected service account tokens for Kubelet image credential providers #4412
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: aramase The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
1bcb8d8
to
335b23c
Compare
…ential Providers Signed-off-by: Anish Ramasekar <anish.ramasekar@gmail.com>
335b23c
to
65a5019
Compare
|
||
#### Credential Provider Response API | ||
|
||
When the `audience` field is set in the kubelet's credential provider configuration, the plugin must return a `CredentialProviderResponse` with the `cacheMode` field set to `ServiceAccount` or `None`. If the cache mode is not set, kubelet will error out and not use the response. The field is required and serves as a marker to indicate that the plugin understands the new fields in the request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's questionable to send service account information (either dimensions or an actual token) to a credential provider and make it possible for it to return a result that is cached across service accounts
Need to think through how to transition from a provider that isn't service-account-aware to one that is matching the same images / registries ... not sure duplicate providers configured with overlapping matchImages works as we want / expect here.
+ ServiceAccountToken string | ||
+ | ||
+ // podAnnotations is a map of annotations on the pod for which the image is being pulled. | ||
+ // This field is optional and may be empty. Plugins may use this field to extract |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about service account annotations?
|
||
#### Credential Provider Response API | ||
|
||
When the `audience` field is set in the kubelet's credential provider configuration, the plugin must return a `CredentialProviderResponse` with the `cacheMode` field set to `ServiceAccount` or `None`. If the cache mode is not set, kubelet will error out and not use the response. The field is required and serves as a marker to indicate that the plugin understands the new fields in the request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cacheMode
is maybe too vague to describe the new cache dimension of which service accounts the response applies to
If the `Audience` field is configured in the kubelet's credential provider configuration, the token will be sent to the plugin. | ||
2. `PodAnnotations` is a map of annotations on the pod for which the image is being pulled. | ||
This field is optional and may be empty. Plugin may use this field to extract additional information required to fetch credentials. | ||
For example, a plugin may use annotations to determine the identity ID to use when fetching credentials. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plumbing arbitrary pod annotations down to the credential provider makes an effectively unlimited surface area... this seems likely to be used to plumb possibly-untrustworthy data (arbitrarily settable by any pod writer) down to credential providers and have it accidentally be treated as trustworthy
@ahmedtd also mentioned that it isn't pod annotations that are used in some cases, it's service account annotations.
An alternative is to have the credential provider look up out of band info from the Kubernetes API directly.
``` | ||
|
||
For the pod to be able to use the projected service account token for image pulls, the pod spec must include a projected service account token volume with the audience set to the credential provider audience. | ||
Only the volume is required in the pod spec, and it does not need to be mounted. The node audience restriction will be enforced by the [NodeRestriction admission controller](https://kubernetes.io/docs/reference/access-authn-authz/admission-controllers/#noderestriction) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the only reason for this is to inform kube-apiserver of the audiences the kubelet should be allowed to request, giving it that information directly (via config/invocation or dedicated API) seems better than a dummy unmounted volume.
- cache mode does not affect the cache key generation since it is designed to provide credentials for all images. | ||
2. `RegistryPluginCacheKeyType` (Registry) | ||
- `ServiceAccount` cache mode: | ||
- The cache key will be generated based on the image registry URL, service account token bound to the pod (namespace, name, UID), hash of the pod annotations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pod annotations are questionable as a cache key... admission and controllers put weird random values in those that differ across replicas
service account annotations would be significantly more stable (and would map much more naturally to the service account being the partitioning dimension), though I don't love pushing opaque user-controlled values down to providers
making the credential provider opt into getting the annotations (maybe even the specific annotations it wants) avoids sending arbitrary annotation content down to it (including stuff like client-side apply annotations), and shrinks the set of annotations that could invalidate caching if they change
and make progress. | ||
--> | ||
|
||
- Passthrough mode where KSA tokens are sent directly to the CRI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe move from "non-goals" to "possible future work", maybe with a sketch of what it would look like ... not sure we care about making it part of initial graduation criteria for this, but ensuring we don't block expanding in this direction would be ideal