Merge pull request #4431 from pacoxu/ensure-image-pull-secret

kep-2535: not persist on disk and add PullImageSecretRecheckDuration flag to define different behaviors

Author: k8s-ci-robot

keps/sig-node/2535-ensure-secret-pulled-images/README.md

@@ -15,6 +15,9 @@
   - [Notes/Constraints/Caveats (Optional)](#notesconstraintscaveats-optional)
   - [Risks and Mitigations](#risks-and-mitigations)
 - [Design Details](#design-details)
+  - [Test Plan](#test-plan)
+      - [Prerequisite testing updates](#prerequisite-testing-updates)
+      - [Unit tests](#unit-tests)
       - [Integration tests](#integration-tests)
       - [e2e tests](#e2e-tests)
   - [Graduation Criteria](#graduation-criteria)
@@ -54,13 +57,10 @@ Items marked with (R) are required *prior to targeting to a milestone / release*
 - [ ] User-facing documentation has been created in [kubernetes/website], for publication to [kubernetes.io]
 - [ ] Supporting documentation—e.g., additional design documents, links to mailing list discussions/SIG meetings, relevant PRs/issues, release notes
 
-
 [kubernetes.io]: https://kubernetes.io/
 [kubernetes/enhancements]: https://git.k8s.io/enhancements
-[kubernetes/kubernetes]: https://git.k8s.io/kubernetes
 [kubernetes/website]: https://git.k8s.io/website
 
-
 ## Summary
 
 We will add support in kubelet for the `pullIfNotPresent` image pull policy, for
@@ -133,6 +133,9 @@ runtimes through the CRI wrt. how they should treat the caching of images on a
 node. Such as store for public use but only if encrypted. Or Store for private
 use un-encrypted...
 
+This feature will not change the behavior of pod with image pull policy `Always`
+and `Never`.
+
 ## Proposal
 
 For alpha `kubelet` will keep a list, across reboots of host and restart of
@@ -143,18 +146,18 @@ For beta an API will be considered to manage the ensure metadata.
 `kubelet` will ensure any image in the list is always pulled if an authentication
 used is not present, thus enforcing authentication / re-authentication.
 
-
 ### User Stories
 
 #### Story 1
+
 User with multiple tenants will be able to support all image pull policies without
 concern that one tenant will gain access to an image that they don't have rights to.
 
 #### Story 2
+
 User will will no longer have to inject the Pull Always Image Pull Policy to
 ensure all tenants have rights to the images that are already present on a host.
 
-
 ### Notes/Constraints/Caveats (Optional)
 
 With the default of the feature gate being off, users / cloud providers will have
@@ -173,42 +176,36 @@ Since images can be pre-loaded, loaded outside the `kubelet` process, and
 garbage collected.. the list of images that required authentication in `kubelet`
 will not be a source of truth for how all images were pulled that are in the
 container runtime cache. To mitigate, images can be garbage collected at boot.
-And we will persist ensure metadata across reboot of host, and restart
+And for alpha, we will not persist ensure metadata across reboot of host, and restart
 of kubelet, and possibly look at a way to add ensure metadata for images loaded
 outside of kubelet. In beta we will add a switch to enable re-auth on boot for
 admins seeking that instead of having to garbage collect where they do not use
 or expect preloaded images since boot.
 
-
 ## Design Details
 
-Kubelet will track, in memory, a hash map for the credentials that were successfully used to pull an image. It has been decided that the hash map will be persisted to disk, in alpha.
+Kubelet will track, in memory, a hash map for the credentials that were successfully
+used to pull an image.
 
-The persisted "cache" will undergo cleanup operations on a timely basis (by default once an hour).
+See PR linked above for detailed design / behavior documentation.
 
-The persistence of the on storage cache is mainly for restarting kubelet and/or node reboot.
+Kubelet will add a new flag, named `PullImageSecretRecheckDuration` to make
+the expired duration configurable. The default value could be 1d. For a pod with
+IfNotPresent image pull policy and an image pull secret, kubelet will recheck
+the secret after `PullImageSecretRecheckDuration`.
 
-The max size of the cache will scale with the number of unique cache entries * the number of unique images that have not been garbage collected. It is not expected that this will be a significant number of bytes. Will be verified by actual use in Alpha and subsequent metrics in Beta.
+Use image pull policy `Always` if user want to recheck the secret everytime.
 
-See `/var/lib/kubelet/image_manager_state` in [kubernetes/kubernetes#114847](https://github.com/kubernetes/kubernetes/pull/114847)
+For image pull policy "if not present", when admin/user doesn't want to automatically
+recheck the secret, set `PullImageSecretRecheckDuration` to 0 to disable it(which means
+never recheck).
 
-> ```
-> {
->   "images": {
->     "sha256:eb6cbbefef909d52f4b2b29f8972bbb6d86fc9dba6528e65aad4f119ce469f7a": {
->       "authHash": { ** per review comment use SHA256 here vs hash **
->         "115b8808c3e7f073": {
->           "ensured": true,
->           "dueDate": "2023-05-30T05:26:53.76740982+08:00"
->         }
->       },
->       "name": "daocloud.io/daocloud/dce-registry-tool:3.0.8"
->     }
->   }
-> }
-> ```
+Note: using the tag `:latest` is equivalent to using the image pull policy `Always.`
 
-See PR linked above for detailed design / behavior documentation.
+Note: since the cache is not persisted to disk, a recheck will happen every kubelet restart.
+This is acceptable because kubelet only restarts during upgrades or in maintenance modes.
+In other words, it should be relatively infrequent(and much less frequent than the default
+value of `PullImageSecretRecheckDuration`).
 
 ### Test Plan
 
@@ -218,41 +215,43 @@ necessary to implement this enhancement.
 
 ##### Prerequisite testing updates
 
-
 ##### Unit tests
 
 For alpha, exhaustive Kubelet unit tests will be provided. Functions affected by the feature gate will be run with the feature gate on and with the feature gate off. Unit buckets will be provided for:
+
 - HashAuth - (new, small) returns a hash code for a CRI pull image auth [link](https://github.com/kubernetes/kubernetes/pull/94899/files#diff-ca08601dfd2fdf846f066d0338dc332beddd5602ab3a71b8fac95b419842da63R704-R751) ** per review comment will use SHA256 **
 - shouldPullImage - (modified, large sized change) determines if image should be pulled based on presence, and image pull policy, and now with the feature gate on if the image has been pulled/ensured by a secret. A unit test bucket did not exist for this function. The unit bucket will cover a matrix for:
-```
-	pullIfNotPresent := &v1.Container{
+
+```golang
+  pullIfNotPresent := &v1.Container{
     ..
-   	ImagePullPolicy: v1.PullIfNotPresent,
- 	}
- 	pullNever := &v1.Container{
+     ImagePullPolicy: v1.PullIfNotPresent,
+   }
+   pullNever := &v1.Container{
     ..
     ImagePullPolicy: v1.PullNever,
- 	}
- 	pullAlways := &v1.Container{
- 		..
+   }
+   pullAlways := &v1.Container{
+     ..
     ImagePullPolicy: v1.PullAlways,
- 	}
- 	tests := []struct {
- 		description       string
- 		container         *v1.Container
- 		imagePresent      bool
- 		pulledBySecret    bool
- 		ensuredBySecret   bool
- 		expectedWithFGOff bool
- 		expectedWithFGOn  bool
- 	}
+   }
+   tests := []struct {
+     description       string
+     container         *v1.Container
+     imagePresent      bool
+     pulledBySecret    bool
+     ensuredBySecret   bool
+     expectedWithFGOff bool
+     expectedWithFGOn  bool
+   }
 ```
-[TestShouldPullImage link](https://github.com/kubernetes/kubernetes/pull/94899/files#diff-7297f08c72da9bf6479e80c03b45e24ea92ccb11c0031549e51b51f88a91f813R311-R438)
 
-PersistHashMeta() ** will be persisting SHA256 entries vs hash **
+[TestShouldPullImage link](https://github.com/kubernetes/kubernetes/pull/94899/files#diff-7297f08c72da9bf6479e80c03b45e24ea92ccb11c0031549e51b51f88a91f813R311-R438)
 
 Additionally, for Alpha we will update this readme with an enumeration of the core packages being touched by the PR to implement this enhancement and provide the current unit coverage for those in the form of:
+
 - <package>: <date> - <current test coverage>
+
 The data will be read from:
 https://testgrid.k8s.io/sig-testing-canaries#ci-kubernetes-coverage-unit
 
@@ -314,6 +313,7 @@ TBD subsequent to alpha
 ## Production Readiness Review Questionnaire
 
 ### Feature Enablement and Rollback
+
 - At Alpha this feature will be disabled by default with a feature gate.
 - At Beta this feature will be enabled by default with the feature gate.
 - At GA the ability to gate the feature will be removed leaving the feature enabled.
@@ -342,6 +342,7 @@ Will go back to working as designed.
 Yes, tests run both enabled and disabled.
 
 ### Rollout, Upgrade and Rollback Planning
+
 TBD
 
 ###### How can a rollout or rollback fail? Can it impact already running workloads?

keps/sig-node/2535-ensure-secret-pulled-images/kep.yaml

@@ -2,25 +2,25 @@ title: Ensure Secret Pulled Images
 kep-number: 2535
 authors:
   - "@mikebrow"
+  - "@pacoxu"
 owning-sig: sig-node
-participating-sigs:
-  - sig-node
 status: implementable
 creation-date: 2021-03-10
 reviewers:
   - "@Random-Liu"
   - "@yujuhong"
   - "@mrunalp"
   - "@adisky"
+  - "@haircommander"
 approvers:
   - "@dchen1107"
   - "@derekwaynecarr"
 stage: alpha
-latest-milestone: "v1.29"
+latest-milestone: "v1.30"
 milestone:
-  alpha: "v1.29"
-  beta: "v1.30"
-  stable: "v1.32"
+  alpha: "v1.30"
+  beta: "v1.31"
+  stable: "v1.33"
 feature-gates:
   - name: KubeletEnsureSecretPulledImages
     components: