AlwaysPullImages: ignore updates that don't change the images referenced by the pod spec

Signed-off-by: pacoxu <paco.xu@daocloud.io>

plugin/pkg/admission/alwayspullimages/BUILD

@@ -15,8 +15,10 @@ go_library(
         "//pkg/apis/core/pods:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/util/errors:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/admission:go_default_library",
+        "//vendor/k8s.io/klog/v2:go_default_library",
     ],
 )
 

plugin/pkg/admission/alwayspullimages/admission.go

@@ -30,8 +30,10 @@ import (
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/apiserver/pkg/admission"
+	"k8s.io/klog/v2"
 	api "k8s.io/kubernetes/pkg/apis/core"
 	"k8s.io/kubernetes/pkg/apis/core/pods"
 )
@@ -101,12 +103,49 @@ func (*AlwaysPullImages) Validate(ctx context.Context, attributes admission.Attr
 	return nil
 }
 
+// check if it's update and it doesn't change the images referenced by the pod spec
+func isUpdateWithNoNewImages(attributes admission.Attributes) bool {
+	if attributes.GetOperation() != admission.Update {
+		return false
+	}
+
+	pod, ok := attributes.GetObject().(*api.Pod)
+	if !ok {
+		klog.Warningf("Resource was marked with kind Pod but pod was unable to be converted.")
+		return false
+	}
+
+	oldPod, ok := attributes.GetOldObject().(*api.Pod)
+	if !ok {
+		klog.Warningf("Resource was marked with kind Pod but old pod was unable to be converted.")
+		return false
+	}
+
+	oldImages := sets.NewString()
+	pods.VisitContainersWithPath(&oldPod.Spec, field.NewPath("spec"), func(c *api.Container, _ *field.Path) bool {
+		oldImages.Insert(c.Image)
+		return true
+	})
+
+	hasNewImage := false
+	pods.VisitContainersWithPath(&pod.Spec, field.NewPath("spec"), func(c *api.Container, _ *field.Path) bool {
+		if !oldImages.Has(c.Image) {
+			hasNewImage = true
+		}
+		return !hasNewImage
+	})
+	return !hasNewImage
+}
+
 func shouldIgnore(attributes admission.Attributes) bool {
 	// Ignore all calls to subresources or resources other than pods.
 	if len(attributes.GetSubresource()) != 0 || attributes.GetResource().GroupResource() != api.Resource("pods") {
 		return true
 	}
 
+	if isUpdateWithNoNewImages(attributes) {
+		return true
+	}
 	return false
 }
 

plugin/pkg/admission/alwayspullimages/admission_test.go

@@ -165,4 +165,104 @@ func TestOtherResources(t *testing.T) {
 			t.Errorf("%s: image pull policy was changed to %s", tc.name, a)
 		}
 	}
+
+}
+
+// TestUpdatePod ensures that this admission controller is a no-op for update pod if no
+// images were changed in the new pod spec.
+func TestUpdatePod(t *testing.T) {
+	namespace := "testnamespace"
+	name := "testname"
+	oldPod := &api.Pod{
+		ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace},
+		Spec: api.PodSpec{
+			Containers: []api.Container{
+				{Name: "ctr2", Image: "image", ImagePullPolicy: api.PullIfNotPresent},
+			},
+		},
+	}
+	// only add new annotation
+	pod := &api.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      name,
+			Namespace: namespace,
+			Annotations: map[string]string{
+				"test": "test",
+			},
+		},
+		Spec: api.PodSpec{
+			Containers: []api.Container{
+				{Name: "ctr2", Image: "image", ImagePullPolicy: api.PullIfNotPresent},
+			},
+		},
+	}
+	// add new label and change image
+	podWithNewImage := &api.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      name,
+			Namespace: namespace,
+			Annotations: map[string]string{
+				"test": "test",
+			},
+		},
+		Spec: api.PodSpec{
+			Containers: []api.Container{
+				{Name: "ctr2", Image: "image2", ImagePullPolicy: api.PullIfNotPresent},
+			},
+		},
+	}
+	tests := []struct {
+		name         string
+		kind         string
+		resource     string
+		subresource  string
+		object       runtime.Object
+		oldObject    runtime.Object
+		expectError  bool
+		expectIgnore bool
+	}{
+		{
+			name:         "update IfNotPresent pod annotations",
+			kind:         "Pod",
+			resource:     "pods",
+			subresource:  "finalizers",
+			object:       pod,
+			oldObject:    oldPod,
+			expectIgnore: true,
+		},
+		{
+			name:        "update IfNotPresent pod image",
+			kind:        "Pod",
+			resource:    "pods",
+			subresource: "finalizers",
+			object:      podWithNewImage,
+			oldObject:   oldPod,
+		},
+	}
+
+	for _, tc := range tests {
+		handler := admissiontesting.WithReinvocationTesting(t, &AlwaysPullImages{})
+
+		err := handler.Admit(context.TODO(), admission.NewAttributesRecord(tc.object, tc.oldObject, api.Kind(tc.kind).WithVersion("version"), namespace, name, api.Resource(tc.resource).WithVersion("version"), tc.subresource, admission.Create, &metav1.UpdateOptions{}, false, nil), nil)
+
+		if tc.expectError {
+			if err == nil {
+				t.Errorf("%s: unexpected nil error", tc.name)
+			}
+			continue
+		}
+		if tc.expectIgnore {
+			if e, a := api.PullIfNotPresent, pod.Spec.Containers[0].ImagePullPolicy; e != a {
+				t.Errorf("%s: image pull policy was changed to %s", tc.name, a)
+			}
+			continue
+		}
+
+		if err != nil {
+			t.Errorf("%s: unexpected error: %v", tc.name, err)
+			continue
+		}
+
+	}
+
 }