Merge pull request #30722 from timstclair/aa-validation

pkg/api/validation/validation.go

@@ -36,6 +36,7 @@ import (
 	"k8s.io/kubernetes/pkg/api/v1"
 	"k8s.io/kubernetes/pkg/capabilities"
 	"k8s.io/kubernetes/pkg/labels"
+	"k8s.io/kubernetes/pkg/security/apparmor"
 	"k8s.io/kubernetes/pkg/util/intstr"
 	"k8s.io/kubernetes/pkg/util/sets"
 	"k8s.io/kubernetes/pkg/util/validation"
@@ -110,7 +111,7 @@ func ValidateDNS1123Subdomain(value string, fldPath *field.Path) field.ErrorList
 	return allErrs
 }
 
-func ValidatePodSpecificAnnotations(annotations map[string]string, fldPath *field.Path) field.ErrorList {
+func ValidatePodSpecificAnnotations(annotations map[string]string, spec *api.PodSpec, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
 	if annotations[api.AffinityAnnotationKey] != "" {
 		allErrs = append(allErrs, ValidateAffinityInPodAnnotations(annotations, fldPath)...)
@@ -129,10 +130,36 @@ func ValidatePodSpecificAnnotations(annotations map[string]string, fldPath *fiel
 	}
 
 	allErrs = append(allErrs, ValidateSeccompPodAnnotations(annotations, fldPath)...)
+	allErrs = append(allErrs, ValidateAppArmorPodAnnotations(annotations, spec, fldPath)...)
 
 	return allErrs
 }
 
+func ValidatePodSpecificAnnotationUpdates(newPod, oldPod *api.Pod, fldPath *field.Path) field.ErrorList {
+	allErrs := field.ErrorList{}
+	newAnnotations := newPod.Annotations
+	oldAnnotations := oldPod.Annotations
+	for k, oldVal := range oldAnnotations {
+		if newAnnotations[k] == oldVal {
+			continue // No change.
+		}
+		if strings.HasPrefix(k, apparmor.ContainerAnnotationKeyPrefix) {
+			allErrs = append(allErrs, field.Forbidden(fldPath.Key(k), "may not update AppArmor annotations"))
+		}
+	}
+	// Check for removals.
+	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, ValidatePodSpecificAnnotations(newAnnotations, &newPod.Spec, fldPath)...)
+	return allErrs
+}
+
 func ValidateEndpointsSpecificAnnotations(annotations map[string]string, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
 	// TODO: remove this after we EOL the annotation.
@@ -1717,7 +1744,7 @@ func validateTolerations(tolerations []api.Toleration, fldPath *field.Path) fiel
 func ValidatePod(pod *api.Pod) field.ErrorList {
 	fldPath := field.NewPath("metadata")
 	allErrs := ValidateObjectMeta(&pod.ObjectMeta, true, ValidatePodName, fldPath)
-	allErrs = append(allErrs, ValidatePodSpecificAnnotations(pod.ObjectMeta.Annotations, fldPath.Child("annotations"))...)
+	allErrs = append(allErrs, ValidatePodSpecificAnnotations(pod.ObjectMeta.Annotations, &pod.Spec, fldPath.Child("annotations"))...)
 	allErrs = append(allErrs, ValidatePodSpec(&pod.Spec, field.NewPath("spec"))...)
 	return allErrs
 }
@@ -2033,6 +2060,39 @@ func ValidateSeccompPodAnnotations(annotations map[string]string, fldPath *field
 	return allErrs
 }
 
+func ValidateAppArmorPodAnnotations(annotations map[string]string, spec *api.PodSpec, fldPath *field.Path) field.ErrorList {
+	allErrs := field.ErrorList{}
+	for k, p := range annotations {
+		if !strings.HasPrefix(k, apparmor.ContainerAnnotationKeyPrefix) {
+			continue
+		}
+		containerName := strings.TrimPrefix(k, apparmor.ContainerAnnotationKeyPrefix)
+		if !podSpecHasContainer(spec, containerName) {
+			allErrs = append(allErrs, field.Invalid(fldPath.Child(k), containerName, "container not found"))
+		}
+
+		if err := apparmor.ValidateProfileFormat(p); err != nil {
+			allErrs = append(allErrs, field.Invalid(fldPath.Child(k), p, err.Error()))
+		}
+	}
+
+	return allErrs
+}
+
+func podSpecHasContainer(spec *api.PodSpec, containerName string) bool {
+	for _, c := range spec.InitContainers {
+		if c.Name == containerName {
+			return true
+		}
+	}
+	for _, c := range spec.Containers {
+		if c.Name == containerName {
+			return true
+		}
+	}
+	return false
+}
+
 // ValidatePodSecurityContext test that the specified PodSecurityContext has valid data.
 func ValidatePodSecurityContext(securityContext *api.PodSecurityContext, spec *api.PodSpec, specPath, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
@@ -2081,7 +2141,7 @@ func ValidateContainerUpdates(newContainers, oldContainers []api.Container, fldP
 func ValidatePodUpdate(newPod, oldPod *api.Pod) field.ErrorList {
 	fldPath := field.NewPath("metadata")
 	allErrs := ValidateObjectMetaUpdate(&newPod.ObjectMeta, &oldPod.ObjectMeta, fldPath)
-	allErrs = append(allErrs, ValidatePodSpecificAnnotations(newPod.ObjectMeta.Annotations, fldPath.Child("annotations"))...)
+	allErrs = append(allErrs, ValidatePodSpecificAnnotationUpdates(newPod, oldPod, fldPath.Child("annotations"))...)
 	specPath := field.NewPath("spec")
 
 	// validate updateable fields:
@@ -2473,7 +2533,7 @@ func ValidatePodTemplateSpec(spec *api.PodTemplateSpec, fldPath *field.Path) fie
 	allErrs := field.ErrorList{}
 	allErrs = append(allErrs, unversionedvalidation.ValidateLabels(spec.Labels, fldPath.Child("labels"))...)
 	allErrs = append(allErrs, ValidateAnnotations(spec.Annotations, fldPath.Child("annotations"))...)
-	allErrs = append(allErrs, ValidatePodSpecificAnnotations(spec.Annotations, fldPath.Child("annotations"))...)
+	allErrs = append(allErrs, ValidatePodSpecificAnnotations(spec.Annotations, &spec.Spec, fldPath.Child("annotations"))...)
 	allErrs = append(allErrs, ValidatePodSpec(&spec.Spec, fldPath.Child("spec"))...)
 	return allErrs
 }

pkg/apis/apps/validation/validation.go

@@ -57,7 +57,7 @@ func ValidatePodTemplateSpecForPetSet(template *api.PodTemplateSpec, selector la
 		// allErrs = append(allErrs, apivalidation.ValidatePodTemplateSpec(template, fldPath)...)
 		allErrs = append(allErrs, unversionedvalidation.ValidateLabels(template.Labels, fldPath.Child("labels"))...)
 		allErrs = append(allErrs, apivalidation.ValidateAnnotations(template.Annotations, fldPath.Child("annotations"))...)
-		allErrs = append(allErrs, apivalidation.ValidatePodSpecificAnnotations(template.Annotations, fldPath.Child("annotations"))...)
+		allErrs = append(allErrs, apivalidation.ValidatePodSpecificAnnotations(template.Annotations, &template.Spec, fldPath.Child("annotations"))...)
 	}
 	return allErrs
 }

pkg/kubelet/kubelet.go

@@ -73,7 +73,6 @@ import (
 	"k8s.io/kubernetes/pkg/kubelet/util/queue"
 	"k8s.io/kubernetes/pkg/kubelet/volumemanager"
 	"k8s.io/kubernetes/pkg/runtime"
-	"k8s.io/kubernetes/pkg/security/apparmor"
 	"k8s.io/kubernetes/pkg/securitycontext"
 	"k8s.io/kubernetes/pkg/types"
 	"k8s.io/kubernetes/pkg/util/bandwidth"
@@ -601,7 +600,7 @@ func NewMainKubelet(
 	klet.AddPodSyncLoopHandler(activeDeadlineHandler)
 	klet.AddPodSyncHandler(activeDeadlineHandler)
 
-	klet.AddPodAdmitHandler(apparmor.NewValidator(containerRuntime))
+	klet.AddPodAdmitHandler(lifecycle.NewAppArmorAdmitHandler(containerRuntime))
 
 	// apply functional Option's
 	for _, opt := range kubeOptions {

pkg/kubelet/lifecycle/handlers.go

@@ -30,6 +30,7 @@ import (
 	kubetypes "k8s.io/kubernetes/pkg/kubelet/types"
 	"k8s.io/kubernetes/pkg/kubelet/util/format"
 	"k8s.io/kubernetes/pkg/kubelet/util/ioutils"
+	"k8s.io/kubernetes/pkg/security/apparmor"
 	"k8s.io/kubernetes/pkg/types"
 	"k8s.io/kubernetes/pkg/util/intstr"
 )
@@ -142,3 +143,25 @@ func getHttpRespBody(resp *http.Response) string {
 	}
 	return ""
 }
+
+func NewAppArmorAdmitHandler(runtime string) PodAdmitHandler {
+	return &appArmorAdmitHandler{
+		Validator: apparmor.NewValidator(runtime),
+	}
+}
+
+type appArmorAdmitHandler struct {
+	apparmor.Validator
+}
+
+func (a *appArmorAdmitHandler) Admit(attrs *PodAdmitAttributes) PodAdmitResult {
+	err := a.Validate(attrs.Pod)
+	if err == nil {
+		return PodAdmitResult{Admit: true}
+	}
+	return PodAdmitResult{
+		Admit:   false,
+		Reason:  "AppArmor",
+		Message: fmt.Sprintf("Cannot enforce AppArmor: %v", err),
+	}
+}

pkg/security/apparmor/validate.go

@@ -26,7 +26,6 @@ import (
 	"strings"
 
 	"k8s.io/kubernetes/pkg/api"
-	"k8s.io/kubernetes/pkg/kubelet/lifecycle"
 	"k8s.io/kubernetes/pkg/util"
 )
 
@@ -34,11 +33,12 @@ import (
 // Set to true if the wrong build tags are set (see validate_disabled.go).
 var isDisabledBuild bool
 
-const (
-	rejectReason = "AppArmor"
-)
+// Interface for validating that a pod with with an AppArmor profile can be run by a Node.
+type Validator interface {
+	Validate(pod *api.Pod) error
+}
 
-func NewValidator(runtime string) lifecycle.PodAdmitHandler {
+func NewValidator(runtime string) Validator {
 	if err := validateHost(runtime); err != nil {
 		return &validator{validateHostErr: err}
 	}
@@ -58,21 +58,7 @@ type validator struct {
 	appArmorFS      string
 }
 
-// TODO(timstclair): Refactor the PodAdmitInterface to return a (Admit, Reason Message) rather than
-// the PodAdmitResult struct so that the interface can be implemented without importing lifecycle.
-func (v *validator) Admit(attrs *lifecycle.PodAdmitAttributes) lifecycle.PodAdmitResult {
-	err := v.validate(attrs.Pod)
-	if err == nil {
-		return lifecycle.PodAdmitResult{Admit: true}
-	}
-	return lifecycle.PodAdmitResult{
-		Admit:   false,
-		Reason:  rejectReason,
-		Message: fmt.Sprintf("Cannot enforce AppArmor: %v", err),
-	}
-}
-
-func (v *validator) validate(pod *api.Pod) error {
+func (v *validator) Validate(pod *api.Pod) error {
 	if !isRequired(pod) {
 		return nil
 	}
@@ -122,18 +108,27 @@ func validateHost(runtime string) error {
 
 // Verify that the profile is valid and loaded.
 func validateProfile(profile string, loadedProfiles map[string]bool) error {
+	if err := ValidateProfileFormat(profile); err != nil {
+		return err
+	}
+
+	if strings.HasPrefix(profile, ProfileNamePrefix) {
+		profileName := strings.TrimPrefix(profile, ProfileNamePrefix)
+		if !loadedProfiles[profileName] {
+			return fmt.Errorf("profile %q is not loaded", profileName)
+		}
+	}
+
+	return nil
+}
+
+func ValidateProfileFormat(profile string) error {
 	if profile == "" || profile == ProfileRuntimeDefault {
 		return nil
 	}
 	if !strings.HasPrefix(profile, ProfileNamePrefix) {
 		return fmt.Errorf("invalid AppArmor profile name: %q", profile)
 	}
-
-	profileName := strings.TrimPrefix(profile, ProfileNamePrefix)
-	if !loadedProfiles[profileName] {
-		return fmt.Errorf("profile %q is not loaded", profileName)
-	}
-
 	return nil
 }
 

pkg/security/apparmor/validate_test.go

@@ -21,7 +21,6 @@ import (
 	"testing"
 
 	"k8s.io/kubernetes/pkg/api"
-	"k8s.io/kubernetes/pkg/kubelet/lifecycle"
 
 	"github.com/stretchr/testify/assert"
 )
@@ -37,7 +36,7 @@ func TestGetAppArmorFS(t *testing.T) {
 	assert.Equal(t, expectedPath, actualPath)
 }
 
-func TestAdmitHost(t *testing.T) {
+func TestValidateHost(t *testing.T) {
 	// This test only passes on systems running AppArmor with the default configuration.
 	// The test should be manually run if modifying the getAppArmorFS function.
 	t.Skip()
@@ -46,7 +45,7 @@ func TestAdmitHost(t *testing.T) {
 	assert.Error(t, validateHost("rkt"))
 }
 
-func TestAdmitProfile(t *testing.T) {
+func TestValidateProfile(t *testing.T) {
 	loadedProfiles := map[string]bool{
 		"docker-default":                                true,
 		"foo-bar":                                       true,
@@ -79,7 +78,7 @@ func TestAdmitProfile(t *testing.T) {
 	}
 }
 
-func TestAdmitBadHost(t *testing.T) {
+func TestValidateBadHost(t *testing.T) {
 	hostErr := errors.New("expected host error")
 	v := &validator{
 		validateHostErr: hostErr,
@@ -95,20 +94,16 @@ func TestAdmitBadHost(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		result := v.Admit(&lifecycle.PodAdmitAttributes{
-			Pod: getPodWithProfile(test.profile),
-		})
+		err := v.Validate(getPodWithProfile(test.profile))
 		if test.expectValid {
-			assert.True(t, result.Admit, "Pod with profile %q should be admitted", test.profile)
+			assert.NoError(t, err, "Pod with profile %q should be valid", test.profile)
 		} else {
-			assert.False(t, result.Admit, "Pod with profile %q should be rejected", test.profile)
-			assert.Equal(t, rejectReason, result.Reason, "Pod with profile %q", test.profile)
-			assert.Contains(t, result.Message, hostErr.Error(), "Pod with profile %q", test.profile)
+			assert.Equal(t, hostErr, err, "Pod with profile %q should trigger a host validation error", test.profile)
 		}
 	}
 }
 
-func TestAdmitValidHost(t *testing.T) {
+func TestValidateValidHost(t *testing.T) {
 	v := &validator{
 		appArmorFS: "./testdata/",
 	}
@@ -128,15 +123,11 @@ func TestAdmitValidHost(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		result := v.Admit(&lifecycle.PodAdmitAttributes{
-			Pod: getPodWithProfile(test.profile),
-		})
+		err := v.Validate(getPodWithProfile(test.profile))
 		if test.expectValid {
-			assert.True(t, result.Admit, "Pod with profile %q should be admitted", test.profile)
+			assert.NoError(t, err, "Pod with profile %q should be valid", test.profile)
 		} else {
-			assert.False(t, result.Admit, "Pod with profile %q should be rejected", test.profile)
-			assert.Equal(t, rejectReason, result.Reason, "Pod with profile %q", test.profile)
-			assert.NotEmpty(t, result.Message, "Pod with profile %q", test.profile)
+			assert.Error(t, err, "Pod with profile %q should trigger a validation error", test.profile)
 		}
 	}
 
@@ -160,16 +151,10 @@ func TestAdmitValidHost(t *testing.T) {
 			},
 		},
 	}
-	assert.True(t, v.Admit(&lifecycle.PodAdmitAttributes{Pod: pod}).Admit,
-		"Multi-container pod should be admitted")
+	assert.NoError(t, v.Validate(pod), "Multi-container pod should validate")
 	for k, val := range pod.Annotations {
 		pod.Annotations[k] = val + "-bad"
-
-		result := v.Admit(&lifecycle.PodAdmitAttributes{Pod: pod})
-		assert.False(t, result.Admit, "Multi-container pod with invalid profile should be rejected")
-		assert.Equal(t, rejectReason, result.Reason, "Multi-container pod with invalid profile")
-		assert.NotEmpty(t, result.Message, "Multi-container pod with invalid profile")
-
+		assert.Error(t, v.Validate(pod), "Multi-container pod with invalid profile %s:%s", k, pod.Annotations[k])
 		pod.Annotations[k] = val // Restore.
 	}
 }