Implement ensure secret pulled images feature

Reference: https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/2535-ensure-secret-pulled-images#proposal

Signed-off-by: Sai Ramesh Vanka <[email protected]>

Author: sairameshv

pkg/features/kube_features.go

@@ -369,9 +369,9 @@ const (
 	// fallback to using it's cgroupDriver option.
 	KubeletCgroupDriverFromCRI featuregate.Feature = "KubeletCgroupDriverFromCRI"
 
-	// owner: @mikebrow @pacoxu
+	// owner: @mikebrow @pacoxu @sairameshv
 	// kep: http://kep.k8s.io/2535
-	// alpha: v1.30
+	// alpha: v1.31
 	//
 	// Enables kubelet to ensure images pulled with pod imagePullSecrets are authenticated
 	// by other pods that do not have the same credentials.

pkg/kubelet/apis/config/scheme/testdata/KubeletConfiguration/after/v1beta1.yaml

@@ -76,8 +76,8 @@ oomScoreAdj: -999
 podLogsDir: /var/log/pods
 podPidsLimit: -1
 port: 10250
-pullImageSecretRecheckPeriod: 0s
 pullImageSecretRecheck: false
+pullImageSecretRecheckPeriod: 0s
 registerNode: true
 registryBurst: 10
 registryPullQPS: 5

pkg/kubelet/apis/config/scheme/testdata/KubeletConfiguration/roundtrip/default/v1beta1.yaml

@@ -76,8 +76,8 @@ oomScoreAdj: -999
 podLogsDir: /var/log/pods
 podPidsLimit: -1
 port: 10250
-pullImageSecretRecheckPeriod: 0s
 pullImageSecretRecheck: false
+pullImageSecretRecheckPeriod: 0s
 registerNode: true
 registryBurst: 10
 registryPullQPS: 5

pkg/kubelet/container/helpers.go

@@ -127,7 +127,10 @@ func pickFieldsToHash(container *v1.Container) map[string]string {
 	return retval
 }
 
-// HashAuth - returns a hash code for a CRI pull image auth, and an error if the hash cannot be calculated.
+// HashAuth returns a hash code of a CRI pull image auth,
+// and an error if the hash cannot be calculated.
+// This hash code would be used as a metadata of the image pull info
+// to determine if an image is ensured and has to be pulled or not
 func HashAuth(auth *runtimeapi.AuthConfig) (string, error) {
 	hash := fnv.New64a()
 	authJSON, err := json.Marshal(auth)

pkg/kubelet/container/helpers_test.go

@@ -1037,6 +1037,6 @@ func TestHashAuth(t *testing.T) {
 	for _, tc := range testCases {
 		hashVal, err := HashAuth(tc.auth)
 		assert.Equal(t, tc.expectedHash, hashVal, "the hash value here should not be changed.")
-		assert.Nil(t, err)
+		assert.NoError(t, err)
 	}
 }

pkg/kubelet/images/image_manager.go

@@ -88,7 +88,7 @@ func NewImageManager(recorder record.EventRecorder, imageService kubecontainer.I
 		puller = newParallelImagePuller(imageService, maxParallelImagePulls)
 	}
 	var pullImageSecretCheck bool
-	if pullImageSecretRecheck != nil {
+	if utilfeature.DefaultFeatureGate.Enabled((features.KubeletEnsureSecretPulledImages)) && pullImageSecretRecheck != nil {
 		pullImageSecretCheck = *pullImageSecretRecheck
 	}
 	return &imageManager{
@@ -106,7 +106,7 @@ func NewImageManager(recorder record.EventRecorder, imageService kubecontainer.I
 
 // shouldPullImage returns whether we should pull an image according to
 // the presence and pull policy of the image.
-func shouldPullImage(container *v1.Container, imagePresent, pulledBySecret, ensuredBySecret bool) bool {
+func shouldPullImage(container *v1.Container, imagePresent, pulledBySecret, ensuredBySecret, pullImageSecretRecheck bool) bool {
 	switch container.ImagePullPolicy {
 	case v1.PullNever:
 		return false
@@ -122,10 +122,8 @@ func shouldPullImage(container *v1.Container, imagePresent, pulledBySecret, ensu
 		// pulled the image successfully. Otherwise pod B could use pod A's images
 		// without auth. So in this case if pulledBySecret but not ensured by matching
 		// secret auth for a pull again for the pod B scenario where the auth does not match
-		if utilfeature.DefaultFeatureGate.Enabled((features.KubeletEnsureSecretPulledImages)) {
-			if pulledBySecret && !ensuredBySecret {
-				return true // noting here that old behaviour returns false in this case indicating the image should not be pulled
-			}
+		if pullImageSecretRecheck && pulledBySecret && !ensuredBySecret {
+			return true // noting here that old behaviour returns false in this case indicating the image should not be pulled
 		}
 		return false
 	}
@@ -141,6 +139,7 @@ func (m *imageManager) logIt(ref *v1.ObjectReference, eventtype, event, prefix,
 	}
 }
 
+// EnsuredInfo contains the data if an image is ensured and the last ensured time
 type EnsuredInfo struct {
 	// true for ensured secret
 	Ensured bool `json:"ensured"`
@@ -197,7 +196,7 @@ func (m *imageManager) EnsureImageExists(ctx context.Context, pod *v1.Pod, conta
 	}
 	present := imageRef != ""
 	var pulledBySecret, ensuredBySecret bool
-	if utilfeature.DefaultFeatureGate.Enabled(features.KubeletEnsureSecretPulledImages) && m.pullImageSecretRecheck {
+	if m.pullImageSecretRecheck {
 		m.loadEnsureSecretPulledImagesData()
 		if present {
 			pulledBySecret, ensuredBySecret, err = m.isEnsuredBySecret(imageRef, spec, pullSecrets)
@@ -207,10 +206,10 @@ func (m *imageManager) EnsureImageExists(ctx context.Context, pod *v1.Pod, conta
 		}
 		klog.V(5).InfoS("Get ensured check by secret", "image", image, "imageRef", imageRef, "pulledBySecret", pulledBySecret, "ensuredBySecret", ensuredBySecret)
 	}
-	if !shouldPullImage(container, present, pulledBySecret, ensuredBySecret) {
+	if !shouldPullImage(container, present, pulledBySecret, ensuredBySecret, m.pullImageSecretRecheck) {
 		// should not pull when pull never, or if present and correctly authenticated
 		if present {
-			if utilfeature.DefaultFeatureGate.Enabled(features.KubeletEnsureSecretPulledImages) && m.pullImageSecretRecheck && !ensuredBySecret {
+			if pulledBySecret && m.pullImageSecretRecheck && !ensuredBySecret {
 				// TODO: add integration test for this thrown error message
 				msg := fmt.Sprintf("Container image %q is present with pull policy of %q but does not have the proper auth (image secret) that was used to pull the image", container.Image, container.ImagePullPolicy)
 				m.logIt(ref, v1.EventTypeWarning, events.ErrImageNotEnsured, logPrefix, msg, klog.Warning)
@@ -253,7 +252,7 @@ func (m *imageManager) EnsureImageExists(ctx context.Context, pod *v1.Pod, conta
 	metrics.ImagePullDuration.WithLabelValues(metrics.GetImageSizeBucket(imagePullResult.imageSize)).Observe(imagePullDuration.Seconds())
 	m.backOff.GC()
 
-	if utilfeature.DefaultFeatureGate.Enabled(features.KubeletEnsureSecretPulledImages) && m.pullImageSecretRecheck {
+	if m.pullImageSecretRecheck {
 		m.lock.Lock()
 		defer m.lock.Unlock()
 		// Get the ImageRef after the Image Pull
@@ -287,7 +286,11 @@ func (m *imageManager) storeEnsureSecretPulledImagesData() {
 			klog.ErrorS(err, "Failed to create path", "path", imageStateManagerPath)
 			return
 		}
-		defer imageFile.Close()
+		defer func() {
+			if err := imageFile.Close(); err != nil {
+				klog.ErrorS(err, "Failed to close the file", "file-path", imageStateManagerPath)
+			}
+		}()
 	}
 	byteData, err := utiljson.Marshal(m.ensureSecretPulledImages)
 	if err != nil {
@@ -306,7 +309,7 @@ func (m *imageManager) storeEnsureSecretPulledImagesData() {
 func (m *imageManager) loadEnsureSecretPulledImagesData() {
 	_, err := os.Stat(imageStateManagerPath)
 	if err != nil && !os.IsExist(err) {
-		klog.ErrorS(err, "Failed to stat file", "file", imageStateManagerPath)
+		klog.InfoS("Failed to stat file", "file", imageStateManagerPath, "error", err)
 		return
 	}
 	m.fsLock.Lock()
@@ -379,43 +382,39 @@ func applyDefaultImageTag(image string) (string, error) {
 // 2. ensuredBySecret: true if the secret for an auth used to pull an
 // image has already been authenticated through a successful pull request
 // and the same auth exists for this podSandbox/image.
-func (m *imageManager) isEnsuredBySecret(imageRef string, image kubecontainer.ImageSpec, pullSecrets []v1.Secret) (bool, bool, error) {
+func (m *imageManager) isEnsuredBySecret(imageRef string, image kubecontainer.ImageSpec, pullSecrets []v1.Secret) (pulledBySecret bool, ensuredBySecret bool, _ error) {
 	m.lock.Lock()
 	defer m.lock.Unlock()
-	if utilfeature.DefaultFeatureGate.Enabled(features.KubeletEnsureSecretPulledImages) && m.pullImageSecretRecheck {
+	if m.pullImageSecretRecheck {
 		m.refreshCache()
 	}
 	if imageRef == "" {
-		return false, false, errors.New("imageRef is empty")
+		return pulledBySecret, ensuredBySecret, errors.New("imageRef is empty")
 	}
 
 	// if the image is in the ensured secret pulled image list, it is pulled by secret
-	pulledBySecret := m.ensureSecretPulledImages[imageRef] != nil
+	pulledBySecret = m.ensureSecretPulledImages[imageRef] != nil
 
 	img := image.Image
 	repoToPull, _, _, err := parsers.ParseImageName(img)
 	if err != nil {
-		return pulledBySecret, false, err
+		return pulledBySecret, ensuredBySecret, err
 	}
 
 	if m.keyring == nil {
-		return pulledBySecret, false, nil
+		return pulledBySecret, ensuredBySecret, nil
 	}
 	keyring, err := credentialprovidersecrets.MakeDockerKeyring(pullSecrets, *m.keyring)
 	if err != nil {
-		return pulledBySecret, false, err
+		return pulledBySecret, ensuredBySecret, err
 	}
 
 	creds, withCredentials := keyring.Lookup(repoToPull)
 	if !withCredentials {
-		return pulledBySecret, false, nil
-	}
-	// if the pullImageSecretRecheckPeriod is 0, we don't need to check the secret again
-	if m.pullImageSecretRecheckPeriod.Duration == 0 {
-		return pulledBySecret, true, nil
+		return pulledBySecret, ensuredBySecret, nil
 	}
 
-	if utilfeature.DefaultFeatureGate.Enabled(features.KubeletEnsureSecretPulledImages) && m.pullImageSecretRecheck {
+	if m.pullImageSecretRecheck {
 		for _, currentCreds := range creds {
 			auth := &runtimeapi.AuthConfig{
 				Username:      currentCreds.Username,
@@ -434,13 +433,13 @@ func (m *imageManager) isEnsuredBySecret(imageRef string, image kubecontainer.Im
 			digest := m.ensureSecretPulledImages[imageRef]
 			if digest != nil {
 				ensuredInfo := digest.Auths[hash]
-				if ensuredInfo != nil && ensuredInfo.Ensured {
-					return pulledBySecret, true, nil
+				if ensuredBySecret = ensuredInfo != nil && ensuredInfo.Ensured; ensuredBySecret {
+					return pulledBySecret, ensuredBySecret, nil
 				}
 			}
 		}
 	}
-	return pulledBySecret, false, nil
+	return pulledBySecret, ensuredBySecret, nil
 }
 
 func (m *imageManager) refreshCache() {

pkg/kubelet/images/image_manager_test.go

@@ -694,14 +694,14 @@ func TestShouldPullImage(t *testing.T) {
 	t.Run("disable_ensure_secret_pull_image_features", func(t *testing.T) {
 		featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.KubeletEnsureSecretPulledImages, false)
 		for i, test := range tests {
-			sp := shouldPullImage(test.container, test.imagePresent, test.pulledBySecret, test.ensuredBySecret)
+			sp := shouldPullImage(test.container, test.imagePresent, test.pulledBySecret, test.ensuredBySecret, false)
 			assert.Equal(t, test.expectedWithFGOff, sp, "TestCase[%d]: %s ensured image fg disabled", i, test.description)
 		}
 	})
 	t.Run("enable_ensure_secret_pull_image_features", func(t *testing.T) {
 		featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.KubeletEnsureSecretPulledImages, true)
 		for i, test := range tests {
-			sp := shouldPullImage(test.container, test.imagePresent, test.pulledBySecret, test.ensuredBySecret)
+			sp := shouldPullImage(test.container, test.imagePresent, test.pulledBySecret, test.ensuredBySecret, true)
 			assert.Equal(t, test.expectedWithFGOn, sp, "TestCase[%d]: %s ensured image fg enabled", i, test.description)
 		}
 	})

pkg/kubelet/images/puller.go

@@ -27,11 +27,10 @@ import (
 )
 
 type pullResult struct {
-	imageRef     string
-	imageSize    uint64
-	err          error
-	pullDuration time.Duration
-	// pullCredentialsHash of successful pull with auth, nil if no auth used or error
+	imageRef            string
+	imageSize           uint64
+	err                 error
+	pullDuration        time.Duration
 	pullCredentialsHash string
 }