diff --git a/plugin/pkg/admission/alwayspullimages/BUILD b/plugin/pkg/admission/alwayspullimages/BUILD index ef30baee700..1c350861c4b 100644 --- a/plugin/pkg/admission/alwayspullimages/BUILD +++ b/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", ], ) diff --git a/plugin/pkg/admission/alwayspullimages/admission.go b/plugin/pkg/admission/alwayspullimages/admission.go index 9786b964263..500a3f6db6c 100644 --- a/plugin/pkg/admission/alwayspullimages/admission.go +++ b/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 } diff --git a/plugin/pkg/admission/alwayspullimages/admission_test.go b/plugin/pkg/admission/alwayspullimages/admission_test.go index 4b15c390c0b..73d2375323f 100644 --- a/plugin/pkg/admission/alwayspullimages/admission_test.go +++ b/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 + } + + } + }