Merge pull request #45775 from liggitt/mirror-pod-validation

Automatic merge from submit-queue (batch tested with PRs 44337, 45775, 45832, 45574, 45758)

Tighten validation of mirror pod annotations

Tightens validation for pods with a mirror pod annotation:
1. spec.nodeName must be set
2. makes the mirror pod annotation immutable
3. starts validating pod-specific annotations during pod status update

None of these changes affect usage of the mirror pod annotation by kubelets, which only set it on pod creation (verified this is true back to 1.5.x)

the second commit updates the pod validation tests to look for specific error messages (best reviewed ignoring whitespace changes)

This is the validation portion of https://github.com/kubernetes/community/blob/master/contributors/design-proposals/kubelet-authorizer.md and https://github.com/kubernetes/features/issues/279

```release-note
Mirror pods must now indicate the nodeName they are bound to on creation. The mirror pod annotation is now treated as immutable and cannot be added to an existing pod, removed from a pod, or modified.
```

pkg/api/annotation_key_constants.go

@@ -17,6 +17,9 @@ limitations under the License.
 package api
 
 const (
+	// MirrorAnnotationKey represents the annotation key set by kubelets when creating mirror pods
+	MirrorPodAnnotationKey string = "kubernetes.io/config.mirror"
+
 	// TolerationsAnnotationKey represents the key of tolerations data (json serialized)
 	// in the Annotations of a Pod.
 	TolerationsAnnotationKey string = "scheduler.alpha.kubernetes.io/tolerations"

pkg/api/validation/validation.go

@@ -111,6 +111,12 @@ func ValidatePodSpecificAnnotations(annotations map[string]string, spec *api.Pod
 		allErrs = append(allErrs, ValidateAffinityInPodAnnotations(annotations, fldPath)...)
 	}
 
+	if value, isMirror := annotations[api.MirrorPodAnnotationKey]; isMirror {
+		if len(spec.NodeName) == 0 {
+			allErrs = append(allErrs, field.Invalid(fldPath.Key(api.MirrorPodAnnotationKey), value, "must set spec.nodeName if mirror pod annotation is set"))
+		}
+	}
+
 	if annotations[api.TolerationsAnnotationKey] != "" {
 		allErrs = append(allErrs, ValidateTolerationsInPodAnnotations(annotations, fldPath)...)
 	}
@@ -177,20 +183,26 @@ func ValidatePodSpecificAnnotationUpdates(newPod, oldPod *api.Pod, fldPath *fiel
 	newAnnotations := newPod.Annotations
 	oldAnnotations := oldPod.Annotations
 	for k, oldVal := range oldAnnotations {
-		if newAnnotations[k] == oldVal {
+		if newVal, exists := newAnnotations[k]; exists && newVal == oldVal {
 			continue // No change.
 		}
 		if strings.HasPrefix(k, apparmor.ContainerAnnotationKeyPrefix) {
-			allErrs = append(allErrs, field.Forbidden(fldPath.Key(k), "may not update AppArmor annotations"))
+			allErrs = append(allErrs, field.Forbidden(fldPath.Key(k), "may not remove or update AppArmor annotations"))
+		}
+		if k == api.MirrorPodAnnotationKey {
+			allErrs = append(allErrs, field.Forbidden(fldPath.Key(k), "may not remove or update mirror pod annotation"))
 		}
 	}
-	// Check for removals.
+	// Check for additions
 	for k := range newAnnotations {
 		if _, ok := oldAnnotations[k]; ok {
 			continue // No change.
 		}
 		if strings.HasPrefix(k, apparmor.ContainerAnnotationKeyPrefix) {
-			allErrs = append(allErrs, field.Forbidden(fldPath.Key(k), "may not remove AppArmor annotations"))
+			allErrs = append(allErrs, field.Forbidden(fldPath.Key(k), "may not add AppArmor annotations"))
+		}
+		if k == api.MirrorPodAnnotationKey {
+			allErrs = append(allErrs, field.Forbidden(fldPath.Key(k), "may not add mirror pod annotation"))
 		}
 	}
 	allErrs = append(allErrs, ValidatePodSpecificAnnotations(newAnnotations, &newPod.Spec, fldPath)...)
@@ -2548,9 +2560,10 @@ func ValidatePodUpdate(newPod, oldPod *api.Pod) field.ErrorList {
 // ValidatePodStatusUpdate tests to see if the update is legal for an end user to make. newPod is updated with fields
 // that cannot be changed.
 func ValidatePodStatusUpdate(newPod, oldPod *api.Pod) field.ErrorList {
-	allErrs := ValidateObjectMetaUpdate(&newPod.ObjectMeta, &oldPod.ObjectMeta, field.NewPath("metadata"))
+	fldPath := field.NewPath("metadata")
+	allErrs := ValidateObjectMetaUpdate(&newPod.ObjectMeta, &oldPod.ObjectMeta, fldPath)
+	allErrs = append(allErrs, ValidatePodSpecificAnnotationUpdates(newPod, oldPod, fldPath.Child("annotations"))...)
 
-	// TODO: allow change when bindings are properly decoupled from pods
 	if newPod.Spec.NodeName != oldPod.Spec.NodeName {
 		allErrs = append(allErrs, field.Forbidden(field.NewPath("status", "nodeName"), "may not be changed directly"))
 	}

pkg/kubelet/types/pod_update.go

@@ -26,7 +26,7 @@ import (
 
 const (
 	ConfigSourceAnnotationKey    = "kubernetes.io/config.source"
-	ConfigMirrorAnnotationKey    = "kubernetes.io/config.mirror"
+	ConfigMirrorAnnotationKey    = kubeapi.MirrorPodAnnotationKey
 	ConfigFirstSeenAnnotationKey = "kubernetes.io/config.seen"
 	ConfigHashAnnotationKey      = "kubernetes.io/config.hash"
 	CriticalPodAnnotationKey     = "scheduler.alpha.kubernetes.io/critical-pod"

plugin/pkg/admission/serviceaccount/BUILD

@@ -22,7 +22,6 @@ go_library(
         "//pkg/client/informers/informers_generated/internalversion:go_default_library",
         "//pkg/client/listers/core/internalversion:go_default_library",
         "//pkg/kubeapiserver/admission:go_default_library",
-        "//pkg/kubelet/types:go_default_library",
         "//pkg/serviceaccount:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",

plugin/pkg/admission/serviceaccount/admission.go

@@ -36,7 +36,6 @@ import (
 	informers "k8s.io/kubernetes/pkg/client/informers/informers_generated/internalversion"
 	corelisters "k8s.io/kubernetes/pkg/client/listers/core/internalversion"
 	kubeapiserveradmission "k8s.io/kubernetes/pkg/kubeapiserver/admission"
-	kubelet "k8s.io/kubernetes/pkg/kubelet/types"
 	"k8s.io/kubernetes/pkg/serviceaccount"
 )
 
@@ -146,7 +145,7 @@ func (s *serviceAccount) Admit(a admission.Attributes) (err error) {
 	// Don't modify the spec of mirror pods.
 	// That makes the kubelet very angry and confused, and it immediately deletes the pod (because the spec doesn't match)
 	// That said, don't allow mirror pods to reference ServiceAccounts or SecretVolumeSources either
-	if _, isMirrorPod := pod.Annotations[kubelet.ConfigMirrorAnnotationKey]; isMirrorPod {
+	if _, isMirrorPod := pod.Annotations[api.MirrorPodAnnotationKey]; isMirrorPod {
 		if len(pod.Spec.ServiceAccountName) != 0 {
 			return admission.NewForbidden(a, fmt.Errorf("a mirror pod may not reference service accounts"))
 		}

staging/src/k8s.io/client-go/pkg/api/annotation_key_constants.go

@@ -17,6 +17,9 @@ limitations under the License.
 package api
 
 const (
+	// MirrorAnnotationKey represents the annotation key set by kubelets when creating mirror pods
+	MirrorPodAnnotationKey string = "kubernetes.io/config.mirror"
+
 	// TolerationsAnnotationKey represents the key of tolerations data (json serialized)
 	// in the Annotations of a Pod.
 	TolerationsAnnotationKey string = "scheduler.alpha.kubernetes.io/tolerations"