set PullImageSecretRecheckPeriod default to 0; default not recheck fo…

…r IfNotPresent Image
kubernetes · Mar 7, 2024 · 9d2b749 · 9d2b749
1 parent 9ca0e9c
commit 9d2b749
Show file tree

Hide file tree

Showing 11 changed files with 65 additions and 60 deletions.
diff --git a/pkg/generated/openapi/zz_generated.openapi.go b/pkg/generated/openapi/zz_generated.openapi.go
diff --git a/pkg/kubelet/apis/config/fuzzer/fuzzer.go b/pkg/kubelet/apis/config/fuzzer/fuzzer.go
@@ -120,7 +120,7 @@ func Funcs(codecs runtimeserializer.CodecFactory) []interface{} {
 			obj.EnableSystemLogHandler = true
 			obj.MemoryThrottlingFactor = utilpointer.Float64(rand.Float64())
 			obj.LocalStorageCapacityIsolation = true
-			obj.PullImageSecretRecheckPeriod = metav1.Duration{Duration: kubeletconfigv1beta1.DefaultImagePullRecheckPeriod}
+			obj.PullImageSecretRecheckPeriod = metav1.Duration{}
 		},
 	}
 }
diff --git a/pkg/kubelet/apis/config/scheme/testdata/KubeletConfiguration/after/v1beta1.yaml b/pkg/kubelet/apis/config/scheme/testdata/KubeletConfiguration/after/v1beta1.yaml
@@ -76,7 +76,7 @@ oomScoreAdj: -999
 podLogsDir: /var/log/pods
 podPidsLimit: -1
 port: 10250
-pullImageSecretRecheckPeriod: 24h0m0s
+pullImageSecretRecheckPeriod: 0s
 registerNode: true
 registryBurst: 10
 registryPullQPS: 5

diff --git a/pkg/kubelet/apis/config/scheme/testdata/KubeletConfiguration/roundtrip/default/v1beta1.yaml b/pkg/kubelet/apis/config/scheme/testdata/KubeletConfiguration/roundtrip/default/v1beta1.yaml
@@ -76,7 +76,7 @@ oomScoreAdj: -999
 podLogsDir: /var/log/pods
 podPidsLimit: -1
 port: 10250
-pullImageSecretRecheckPeriod: 24h0m0s
+pullImageSecretRecheckPeriod: 0s
 registerNode: true
 registryBurst: 10
 registryPullQPS: 5

diff --git a/pkg/kubelet/apis/config/types.go b/pkg/kubelet/apis/config/types.go
@@ -496,7 +496,8 @@ type KubeletConfiguration struct {
 	ImageServiceEndpoint string
 
 	// PullImageSecretRecheckPeriod defines the duration to recheck the pull image secret.
-	// By default, the kubelet will recheck the pull image secret every 24 hours(1d).
+	// By default, the kubelet will not recheck the pull image secret.
+	// For security reasons, we recommend rechecking the pull image secret, ideally every 24 hours (1d).
 	PullImageSecretRecheckPeriod metav1.Duration
 }
 

diff --git a/pkg/kubelet/apis/config/v1beta1/defaults.go b/pkg/kubelet/apis/config/v1beta1/defaults.go
@@ -39,9 +39,6 @@ const (
 	DefaultPodLogsDir            = "/var/log/pods"
 	// See https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/2570-memory-qos
 	DefaultMemoryThrottlingFactor = 0.9
-
-	// See https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/2535-ensure-secret-pulled-images
-	DefaultImagePullRecheckPeriod = 24 * time.Hour
 )
 
 var (
@@ -287,6 +284,6 @@ func SetDefaults_KubeletConfiguration(obj *kubeletconfigv1beta1.KubeletConfigura
 		obj.PodLogsDir = DefaultPodLogsDir
 	}
 	if obj.PullImageSecretRecheckPeriod == nil {
-		obj.PullImageSecretRecheckPeriod = &metav1.Duration{Duration: DefaultImagePullRecheckPeriod}
+		obj.PullImageSecretRecheckPeriod = &metav1.Duration{}
 	}
 }
diff --git a/pkg/kubelet/apis/config/v1beta1/defaults_test.go b/pkg/kubelet/apis/config/v1beta1/defaults_test.go
@@ -130,7 +130,7 @@ func TestSetDefaultsKubeletConfiguration(t *testing.T) {
 				RegisterNode:                  utilpointer.Bool(true),
 				LocalStorageCapacityIsolation: utilpointer.Bool(true),
 				PodLogsDir:                    DefaultPodLogsDir,
-				PullImageSecretRecheckPeriod:  &metav1.Duration{Duration: DefaultImagePullRecheckPeriod},
+				PullImageSecretRecheckPeriod:  &metav1.Duration{},
 			},
 		},
 		{
@@ -363,7 +363,7 @@ func TestSetDefaultsKubeletConfiguration(t *testing.T) {
 				RegisterNode:                  utilpointer.Bool(false),
 				LocalStorageCapacityIsolation: utilpointer.Bool(false),
 				PodLogsDir:                    DefaultPodLogsDir,
-				PullImageSecretRecheckPeriod:  &metav1.Duration{Duration: DefaultImagePullRecheckPeriod},
+				PullImageSecretRecheckPeriod:  &metav1.Duration{},
 			},
 		},
 		{
@@ -759,7 +759,7 @@ func TestSetDefaultsKubeletConfiguration(t *testing.T) {
 				RegisterNode:                  utilpointer.Bool(true),
 				LocalStorageCapacityIsolation: utilpointer.Bool(true),
 				PodLogsDir:                    DefaultPodLogsDir,
-				PullImageSecretRecheckPeriod:  &metav1.Duration{Duration: DefaultImagePullRecheckPeriod},
+				PullImageSecretRecheckPeriod:  &metav1.Duration{},
 			},
 		},
 		{
@@ -852,7 +852,7 @@ func TestSetDefaultsKubeletConfiguration(t *testing.T) {
 				RegisterNode:                  utilpointer.Bool(true),
 				LocalStorageCapacityIsolation: utilpointer.Bool(true),
 				PodLogsDir:                    DefaultPodLogsDir,
-				PullImageSecretRecheckPeriod:  &metav1.Duration{Duration: DefaultImagePullRecheckPeriod},
+				PullImageSecretRecheckPeriod:  &metav1.Duration{},
 			},
 		},
 		{
@@ -945,7 +945,7 @@ func TestSetDefaultsKubeletConfiguration(t *testing.T) {
 				RegisterNode:                  utilpointer.Bool(true),
 				LocalStorageCapacityIsolation: utilpointer.Bool(true),
 				PodLogsDir:                    DefaultPodLogsDir,
-				PullImageSecretRecheckPeriod:  &metav1.Duration{Duration: DefaultImagePullRecheckPeriod},
+				PullImageSecretRecheckPeriod:  &metav1.Duration{},
 			},
 		},
 	}

diff --git a/pkg/kubelet/container/runtime.go b/pkg/kubelet/container/runtime.go
@@ -151,7 +151,7 @@ type ImageService interface {
 	// PullImage pulls an image from the network to local storage using the supplied
 	// secrets if necessary. It returns a reference (digest or ID) to the pulled image
 	// and if applicable a hash for the auth used to successfully pull the image.
-	PullImage(ctx context.Context, image ImageSpec, pullSecrets []v1.Secret, podSandboxConfig *runtimeapi.PodSandboxConfig) (string, string, error)
+	PullImage(ctx context.Context, image ImageSpec, pullSecrets []v1.Secret, podSandboxConfig *runtimeapi.PodSandboxConfig) (imageRef, pullCredentialsHash string, err error)
 	// GetImageRef gets the reference (digest or ID) of the image which has already been in
 	// the local storage. It returns ("", nil) if the image isn't in the local storage.
 	GetImageRef(ctx context.Context, image ImageSpec) (string, error)

diff --git a/pkg/kubelet/images/image_manager.go b/pkg/kubelet/images/image_manager.go
@@ -182,16 +182,16 @@ func (m *imageManager) EnsureImageExists(ctx context.Context, pod *v1.Pod, conta
 	if err != nil {
 		msg := fmt.Sprintf("Failed to inspect image %q: %v", container.Image, err)
 		m.logIt(ref, v1.EventTypeWarning, events.FailedToInspectImage, logPrefix, msg, klog.Warning)
-		return imageRef, msg, ErrImageInspect
+		return "", msg, ErrImageInspect
 	}
 
 	present := imageRef != ""
 
 	var pulledBySecret, ensuredBySecret bool
-	if present {
+	if present && utilfeature.DefaultFeatureGate.Enabled(features.KubeletEnsureSecretPulledImages) {
 		pulledBySecret, ensuredBySecret, err := m.isEnsuredBySecret(imageRef, spec, pullSecrets)
 		if err != nil {
-			return imageRef, "Error get ensured check by secret", err
+			return "", "Error get ensured check by secret", err
 		}
 		klog.V(5).InfoS("Get ensured check by secret", "image", image, "imageRef", imageRef, "pulledBySecret", pulledBySecret, "ensuredBySecret", ensuredBySecret)
 	}
@@ -203,7 +203,7 @@ func (m *imageManager) EnsureImageExists(ctx context.Context, pod *v1.Pod, conta
 				// 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)
-				return imageRef, msg, ErrImageNotEnsured
+				return "", msg, ErrImageNotEnsured
 			}
 			msg := fmt.Sprintf("Container image %q already present on machine", container.Image)
 			m.logIt(ref, v1.EventTypeNormal, events.PulledImage, logPrefix, msg, klog.Info)
@@ -213,7 +213,7 @@ func (m *imageManager) EnsureImageExists(ctx context.Context, pod *v1.Pod, conta
 		// when not present on node, we do not pull image only if the image pull policy is Never
 		msg := fmt.Sprintf("Container image %q is not present with pull policy of Never", container.Image)
 		m.logIt(ref, v1.EventTypeWarning, events.ErrImageNeverPullPolicy, logPrefix, msg, klog.Warning)
-		return imageRef, msg, ErrImageNeverPull
+		return "", msg, ErrImageNeverPull
 	}
 
 	backOffKey := fmt.Sprintf("%s_%s", pod.UID, container.Image)
@@ -242,23 +242,27 @@ func (m *imageManager) EnsureImageExists(ctx context.Context, pod *v1.Pod, conta
 	metrics.ImagePullDuration.WithLabelValues(metrics.GetImageSizeBucket(imagePullResult.imageSize)).Observe(imagePullDuration.Seconds())
 	m.backOff.GC()
 
-	m.lock.Lock()
-	if imagePullResult.pullCredentialsHash == "" {
-		// successful pull no auth hash returned, auth was not required so we should reset the hashmap for this
-		// imageref since auth is no longer required for the local image cache, allowing use of the ImageRef
-		// by other pods if it remains cached and pull policy is PullIfNotPresent
-		delete(m.ensureSecretPulledImages, imageRef)
-	} else {
-		// store/create hashMatch map entry for auth config hash key used to pull the image
-		// for this imageref (digest)
-		digest := m.ensureSecretPulledImages[imageRef]
-		if digest == nil {
-			digest = &imagePullInfo{Auths: make(map[string]*ensuredInfo)}
-			m.ensureSecretPulledImages[imageRef] = digest
-		}
-		digest.Auths[imagePullResult.pullCredentialsHash] = &ensuredInfo{true, time.Now()}
+	if utilfeature.DefaultFeatureGate.Enabled(features.KubeletEnsureSecretPulledImages) {
+		func() {
+			m.lock.Lock()
+			if imagePullResult.pullCredentialsHash == "" {
+				// successful pull no auth hash returned, auth was not required so we should reset the hashmap for this
+				// imageref since auth is no longer required for the local image cache, allowing use of the ImageRef
+				// by other pods if it remains cached and pull policy is PullIfNotPresent
+				delete(m.ensureSecretPulledImages, imageRef)
+			} else {
+				// store/create hashMatch map entry for auth config hash key used to pull the image
+				// for this imageref (digest)
+				digest := m.ensureSecretPulledImages[imageRef]
+				if digest == nil {
+					digest = &imagePullInfo{Auths: make(map[string]*ensuredInfo)}
+					m.ensureSecretPulledImages[imageRef] = digest
+				}
+				digest.Auths[imagePullResult.pullCredentialsHash] = &ensuredInfo{true, time.Now()}
+			}
+			defer m.lock.Unlock()
+		}()
 	}
-	m.lock.Unlock()
 
 	return imagePullResult.imageRef, "", nil
 }
@@ -352,26 +356,28 @@ func (m *imageManager) isEnsuredBySecret(imageRef string, image kubecontainer.Im
 		return pulledBySecret, true, nil
 	}
 
-	for _, currentCreds := range creds {
-		auth := &runtimeapi.AuthConfig{
-			Username:      currentCreds.Username,
-			Password:      currentCreds.Password,
-			Auth:          currentCreds.Auth,
-			ServerAddress: currentCreds.ServerAddress,
-			IdentityToken: currentCreds.IdentityToken,
-			RegistryToken: currentCreds.RegistryToken,
-		}
+	if utilfeature.DefaultFeatureGate.Enabled(features.KubeletEnsureSecretPulledImages) {
+		for _, currentCreds := range creds {
+			auth := &runtimeapi.AuthConfig{
+				Username:      currentCreds.Username,
+				Password:      currentCreds.Password,
+				Auth:          currentCreds.Auth,
+				ServerAddress: currentCreds.ServerAddress,
+				IdentityToken: currentCreds.IdentityToken,
+				RegistryToken: currentCreds.RegistryToken,
+			}
 
-		hash, err := kubecontainer.HashAuth(auth)
-		if err != nil {
-			klog.ErrorS(err, "Failed to hash auth", "auth", auth)
-			continue
-		}
-		digest := m.ensureSecretPulledImages[imageRef]
-		if digest != nil {
-			ensuredInfo := digest.Auths[hash]
-			if ensuredInfo != nil && ensuredInfo.ensured && ensuredInfo.lastEnsuredDate.Add(m.pullImageSecretRecheckPeriod.Duration).After(time.Now()) {
-				return pulledBySecret, true, nil
+			hash, err := kubecontainer.HashAuth(auth)
+			if err != nil {
+				klog.ErrorS(err, "Failed to hash auth", "auth", auth)
+				continue
+			}
+			digest := m.ensureSecretPulledImages[imageRef]
+			if digest != nil {
+				ensuredInfo := digest.Auths[hash]
+				if ensuredInfo != nil && ensuredInfo.ensured && ensuredInfo.lastEnsuredDate.Add(m.pullImageSecretRecheckPeriod.Duration).After(time.Now()) {
+					return pulledBySecret, true, nil
+				}
 			}
 		}
 	}

diff --git a/pkg/kubelet/images/puller.go b/pkg/kubelet/images/puller.go
@@ -60,7 +60,7 @@ func (pip *parallelImagePuller) pullImage(ctx context.Context, spec kubecontaine
 			defer func() { <-pip.tokens }()
 		}
 		startTime := time.Now()
-		imageRef, hash, err := pip.imageService.PullImage(ctx, spec, pullSecrets, podSandboxConfig)
+		imageRef, pullCredentialsHash, err := pip.imageService.PullImage(ctx, spec, pullSecrets, podSandboxConfig)
 		var size uint64
 		if err == nil && imageRef != "" {
 			// Getting the image size with best effort, ignoring the error.
@@ -71,7 +71,7 @@ func (pip *parallelImagePuller) pullImage(ctx context.Context, spec kubecontaine
 			imageSize:           size,
 			err:                 err,
 			pullDuration:        time.Since(startTime),
-			pullCredentialsHash: hash,
+			pullCredentialsHash: pullCredentialsHash,
 		}
 	}()
 }
@@ -111,7 +111,7 @@ func (sip *serialImagePuller) pullImage(ctx context.Context, spec kubecontainer.
 func (sip *serialImagePuller) processImagePullRequests() {
 	for pullRequest := range sip.pullRequests {
 		startTime := time.Now()
-		imageRef, hash, err := sip.imageService.PullImage(pullRequest.ctx, pullRequest.spec, pullRequest.pullSecrets, pullRequest.podSandboxConfig)
+		imageRef, pullCredentialsHash, err := sip.imageService.PullImage(pullRequest.ctx, pullRequest.spec, pullRequest.pullSecrets, pullRequest.podSandboxConfig)
 		var size uint64
 		if err == nil && imageRef != "" {
 			// Getting the image size with best effort, ignoring the error.
@@ -123,7 +123,7 @@ func (sip *serialImagePuller) processImagePullRequests() {
 			err:       err,
 			// Note: pullDuration includes credential resolution and getting the image size.
 			pullDuration:        time.Since(startTime),
-			pullCredentialsHash: hash,
+			pullCredentialsHash: pullCredentialsHash,
 		}
 	}
 }
diff --git a/staging/src/k8s.io/kubelet/config/v1beta1/types.go b/staging/src/k8s.io/kubelet/config/v1beta1/types.go
@@ -853,7 +853,8 @@ type KubeletConfiguration struct {
 	ImageServiceEndpoint string `json:"imageServiceEndpoint,omitempty"`
 
 	// PullImageSecretRecheckPeriod defines the duration to recheck the pull image secret.
-	// By default, the kubelet will recheck the pull image secret every 24 hours(1d).
+	// By default, the kubelet will not recheck the pull image secret.
+	// For security reasons, we recommend rechecking the pull image secret, ideally every 24 hours (1d).
 	PullImageSecretRecheckPeriod *metav1.Duration `json:"pullImageSecretRecheckPeriod"`
 }