review feedback

Signed-off-by: Anish Ramasekar <anish.ramasekar@gmail.com>

Author: aramase

keps/sig-auth/4412-projected-service-account-tokens-for-kubelet-image-credential-providers/README.md

@@ -316,6 +316,7 @@ the design is written to meet the following criteria:
 - No changes to any registry
 - Minimal changes to Kubelet
 - Minimal changes to Kubelet credential providers
+- Minimal changes to Kube API Server
 
 ## Design Details
 
@@ -333,9 +334,10 @@ Adding a new field `TokenAttributes` to the `CredentialProvider` struct in the k
 1. `ServiceAccountTokenAudience` is the intended audience for the credentials. If set, the kubelet will generate a service account token for the audience and pass it to the plugin.
    1. Only a single audience can be configured.
    2. The credential provider can be set up multiple times for each audience.
+      1. The name in the credential provider config must match the name of the provider executable as seen by the kubelet. To configure multiple instances of the same provider with different audiences, the name of the provider executable must be different and this can be done by creating a symlink to the same executable with a different name.
    3. By setting this field, the credential provider is opting into using service account tokens for image pull.
 2. `ServiceAccountAnnotationKeys` is the list of annotation keys that the plugin is interested in. The keys defined in this list will be extracted from the corresponding service account
-   and passed to the plugin as part of `CredentialProviderRequest`. Plugins can use to extract additional information required to fetch credentials. The plugin is responsible for validating the existence of annotations and their values.
+   and passed to the plugin as part of `CredentialProviderRequest`. Plugins can use to extract additional information required to fetch credentials. The plugin is responsible for validating the existence of annotations and their values. There are existing integrations like Azure and GCP that use the extra metadata to link the KSA name to a cloud identity, and this field will allow them to continue to do so. None of the existing integrations use this metadata as any form of security decision, it is simply to aid in doing token exchanges with the KSA token.
    1. Service account annotations are significantly more stable (and map naturally to the service account being the partioning dimension).
    2. This field makes the provider opt into getting the annotations it wants, avoids sending arbitrary annotation contents down to the plugin (including stuff like client-side apply annotations) and shrinks the set of annotations that could invalidate the cache.
    3. This field can't be set without setting the `ServiceAccountTokenAudience` field.
@@ -357,7 +359,7 @@ The configuration will not support a plugin to function in 2 modes simultaneousl
 +       //
 +       // The service account metadata and token attributes will be used as a dimension to cache
 +       // the credentials in kubelet. The cache key is generated by combining the service account metadata
-+       // (namespace, name, UID, and annotations map with keys defined in ServiceAccountTokenAttribute.serviceAccountAnnotationKeys).
++       // (namespace, name, UID, and annotations key+value for the keys defined in ServiceAccountTokenAttribute.serviceAccountAnnotationKeys).
 +       TokenAttributes *ServiceAccountTokenAttributes
 +}
 +
@@ -397,6 +399,7 @@ providers:
       serviceAccountAnnotationKeys:
       - domain.io/identity-id
       - domain.io/identity-type
+      - domain.io/annotation-that-does-not-exist
 ```
 
 ##### Kubernetes API Server (KAS) Changes
@@ -406,6 +409,9 @@ Add a new flag `--allowed-kubelet-audiences` to KAS to allow configuring the lis
 The node audience restriction will be enforced by the [NodeRestriction admission controller](https://kubernetes.io/docs/reference/access-authn-authz/admission-controllers/#noderestriction)
 and kubelet will only be allowed to generate a service account token for the audience configured in the credential provider configuration if it is present in the list of audiences configured in KAS.
 
+Today we only verify the SA in KAS and allow the kubelet to generate a token for any audience. We want to start verifying the audience as well, so we need to be explicit here about what audiences are allowed.
+The other sources of audiences for the SA token can be observed via the Kubernetes API but this one can't, so we're adding the flag.
+
 #### Credential Provider Request API
 
 Add `ServiceAccountToken` and `ServiceAccountAnnotations` fields to `CredentialProviderRequest`.
@@ -460,6 +466,10 @@ Example credential provider request:
   "serviceAccountAnnotations": {
     "domain.io/identity-id": "12345",
     "domain.io/identity-type": "user"
+    // In the tokenAttributes.serviceAccountAnnotationKeys field in the credential provider configuration above we have configured
+    // domain.io/annotation-that-does-not-exist which is not present in the service account annotations. Because of this, this annotation
+    // will not be passed to the plugin. The plugin is responsible for validating the existence of annotations and their values that's required
+    // to fetch credentials.
   }
 }
 ```
@@ -493,7 +503,9 @@ Notes from reviewing the current KEP and discussion with @stlaz:
 2. Same KSA for the image will result in allowing pod to use the image.
    1. How would expiry work in this scenario?
       1. I think it'll just be tied to the KSA and not so much to the expiry of the token because we're doing the same thing with image pull secrets (considered valid until deleted and recreated). Deletion and recreation of KSA will result in change in UID and that'll result in KSA not found in cache for the image (assuming the key used to store in cache is consistent with the cache key used in the credential provider cache that takes UID into consideration). Need to share the cache key generation logic to be consistent.
-3. Need an update to `ImagePullCredentials` struct to also store coordinates of the KSA
+3. Need an update to `ImagePullCredentials` struct to also store coordinates of the KSA.
+
+Until the two implementations are updated to work together, the alpha implementation of this KEP will use the KSA token based flow unless the pod is using image pull policy set to `Always`. This keeps the feature from misbehaving until we fix the implementations.
 
 ### Test Plan
 
@@ -689,7 +701,17 @@ enhancement:
   CRI or CNI may require updating that component before the kubelet.
 -->
 
-Not applicable because this feature is contained to only the kubelet and does not require communication to other components.
+<!-- What about skew between kubelet and old/new credential plugins? How does a kubelet admin migrate to the new approach, one workload at a time? I assume all kubelets would need to be updated with new config/plugins and then restarted, followed by upgrades to workloads?
+
+Also API server config needs to be updated because kubelets can safely use this feature. -->
+
+To migrate to the new approach,
+
+1. KAS will need to be updated to the new version to configure the list of audiences for which the kubelet is allowed to generate service account tokens for image pulls via the `--allowed-kubelet-audiences` flag.
+2. kubelet on the node needs to be updated to enable the feature flag and configure the credential provider to use service account tokens for image pull via the `TokenAttributes` field in the kubelet credential provider configuration.
+   1. The credential provider plugin will need to be updated to use the service account token for the audience configured in the kubelet credential provider configuration.
+
+Migration of the workloads to the new approach can be done per image or per registry basis by configuring the credential provider multiple times with different names for the same plugin (w or w/o service account tokens, different audiences).
 
 ## Production Readiness Review Questionnaire
 
@@ -955,6 +977,7 @@ Focusing mostly on:
 -->
 
 Yes. Kubelet will request a KSA token from KAS for pod + KSA for every provider (audience) that is configured to use service account tokens for image pull.
+The load is small because the token generation happens only once during pod startup.
 
 ###### Will enabling / using this feature result in introducing new API types?
 
@@ -975,7 +998,10 @@ Describe them, providing:
   - Estimated increase:
 -->
 
-No.
+This depends on the credential provider plugin implementation on how it fetches the credentials using the service account token.
+The plugin will now be called for every unique KSA + (image/registry) combination that is being pulled if not already cached unlike
+today where the cache is based on the image/registry URL. This could result in more calls to the cloud provider from the plugin to
+exchange the KSA token for the credentials.
 
 ###### Will enabling / using this feature result in increasing size or count of the existing API objects?
 
@@ -999,7 +1025,9 @@ Think about adding additional work or introducing new steps in between
 [existing SLIs/SLOs]: https://git.k8s.io/community/sig-scalability/slos/slos.md#kubernetes-slisslos
 -->
 
-No.
+SLIs/SLOs for image pull times might be impacted with the credential plugin using KSA token for fetching credentials. The credentials are
+granular in scope (per service account) like image pull secrets, but the plugin must dynamically fetch the credentials using the KSA token
+and these credentials could have shorter TTLs resulting in more frequent fetching of credentials.
 
 ###### Will enabling / using this feature result in non-negligible increase of resource usage (CPU, RAM, disk, IO, ...) in any components?
 

keps/sig-auth/4412-projected-service-account-tokens-for-kubelet-image-credential-providers/kep.yaml

@@ -1,5 +1,5 @@
 title: Projected Service Account Tokens for Kubelet Image Credential Providers
-kep-number: "4412"
+kep-number: 4412
 authors:
 - "@aramase"
 - "@mainred"
@@ -12,6 +12,8 @@ reviewers:
 approvers:
 - "@enj"
 - "@liggitt"
+see-also:
+- "/keps/sig-node/2535-ensure-secret-pulled-images"
 creation-date: "2024-09-09"
 status: implementable
 stage: alpha