Merge pull request #16006 from php-coder/scc_volumes_error_message

Automatic merge from submit-queue Validate pod's volumes only once and also fix field path in the error message This is the same change as kubernetes/kubernetes#31927 but for the `SecurityContextConstraints`. PTAL @liggitt @pweil- CC @simo5
openshift · Sep 1, 2017 · 1260606 · 1260606
2 parents 98c26b3 + 81af0e1
commit 1260606
Show file tree

Hide file tree

Showing 2 changed files with 35 additions and 35 deletions.
diff --git a/pkg/security/securitycontextconstraints/provider.go b/pkg/security/securitycontextconstraints/provider.go
@@ -269,6 +269,23 @@ func (s *simpleProvider) ValidatePodSecurityContext(pod *api.Pod, fldPath *field
 
 	allErrs = append(allErrs, s.sysctlsStrategy.Validate(pod)...)
 
+	if len(pod.Spec.Volumes) > 0 && !sccutil.SCCAllowsAllVolumes(s.scc) {
+		allowedVolumes := sccutil.FSTypeToStringSet(s.scc.Volumes)
+		for i, v := range pod.Spec.Volumes {
+			fsType, err := sccutil.GetVolumeFSType(v)
+			if err != nil {
+				allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "volumes").Index(i), string(fsType), err.Error()))
+				continue
+			}
+
+			if !allowedVolumes.Has(string(fsType)) {
+				allErrs = append(allErrs, field.Invalid(
+					field.NewPath("spec", "volumes").Index(i), string(fsType),
+					fmt.Sprintf("%s volumes are not allowed to be used", string(fsType))))
+			}
+		}
+	}
+
 	return allErrs
 }
 
@@ -292,23 +309,6 @@ func (s *simpleProvider) ValidateContainerSecurityContext(pod *api.Pod, containe
 
 	allErrs = append(allErrs, s.capabilitiesStrategy.Validate(pod, container)...)
 
-	if len(pod.Spec.Volumes) > 0 && !sccutil.SCCAllowsAllVolumes(s.scc) {
-		allowedVolumes := sccutil.FSTypeToStringSet(s.scc.Volumes)
-		for i, v := range pod.Spec.Volumes {
-			fsType, err := sccutil.GetVolumeFSType(v)
-			if err != nil {
-				allErrs = append(allErrs, field.Invalid(fldPath.Child("volumes").Index(i), string(fsType), err.Error()))
-				continue
-			}
-
-			if !allowedVolumes.Has(string(fsType)) {
-				allErrs = append(allErrs, field.Invalid(
-					fldPath.Child("volumes").Index(i), string(fsType),
-					fmt.Sprintf("%s volumes are not allowed to be used", string(fsType))))
-			}
-		}
-	}
-
 	if !s.scc.AllowHostNetwork && pod.Spec.SecurityContext.HostNetwork {
 		allErrs = append(allErrs, field.Invalid(fldPath.Child("hostNetwork"), pod.Spec.SecurityContext.HostNetwork, "Host network is not allowed to be used"))
 	}

diff --git a/pkg/security/securitycontextconstraints/provider_test.go b/pkg/security/securitycontextconstraints/provider_test.go
@@ -220,6 +220,16 @@ func TestValidatePodSecurityContextFailures(t *testing.T) {
 	failUnsafeSysctlFooPod := defaultPod()
 	failUnsafeSysctlFooPod.Annotations[api.UnsafeSysctlsPodAnnotationKey] = "foo=1"
 
+	failHostDirPod := defaultPod()
+	failHostDirPod.Spec.Volumes = []api.Volume{
+		{
+			Name: "bad volume",
+			VolumeSource: api.VolumeSource{
+				HostPath: &api.HostPathVolumeSource{},
+			},
+		},
+	}
+
 	errorCases := map[string]struct {
 		pod           *api.Pod
 		scc           *securityapi.SecurityContextConstraints
@@ -300,6 +310,11 @@ func TestValidatePodSecurityContextFailures(t *testing.T) {
 			scc:           failOtherSysctlsAllowedSCC,
 			expectedError: "sysctl \"foo\" is not allowed",
 		},
+		"failHostDirSCC": {
+			pod:           failHostDirPod,
+			scc:           defaultSCC(),
+			expectedError: "hostPath volumes are not allowed to be used",
+		},
 	}
 	for k, v := range errorCases {
 		provider, err := NewSimpleProvider(v.scc)
@@ -351,16 +366,6 @@ func TestValidateContainerSecurityContextFailures(t *testing.T) {
 		Add: []api.Capability{"foo"},
 	}
 
-	failHostDirPod := defaultPod()
-	failHostDirPod.Spec.Volumes = []api.Volume{
-		{
-			Name: "bad volume",
-			VolumeSource: api.VolumeSource{
-				HostPath: &api.HostPathVolumeSource{},
-			},
-		},
-	}
-
 	failHostPortPod := defaultPod()
 	failHostPortPod.Spec.Containers[0].Ports = []api.ContainerPort{{HostPort: 1}}
 
@@ -406,11 +411,6 @@ func TestValidateContainerSecurityContextFailures(t *testing.T) {
 			scc:           defaultSCC(),
 			expectedError: "capability may not be added",
 		},
-		"failHostDirSCC": {
-			pod:           failHostDirPod,
-			scc:           defaultSCC(),
-			expectedError: "hostPath volumes are not allowed to be used",
-		},
 		"failHostPortSCC": {
 			pod:           failHostPortPod,
 			scc:           defaultSCC(),
@@ -926,7 +926,7 @@ func TestValidateAllowedVolumes(t *testing.T) {
 		}
 
 		// expect a denial for this SCC and test the error message to ensure it's related to the volumesource
-		errs := provider.ValidateContainerSecurityContext(pod, &pod.Spec.Containers[0], field.NewPath(""))
+		errs := provider.ValidatePodSecurityContext(pod, field.NewPath(""))
 		if len(errs) != 1 {
 			t.Errorf("expected exactly 1 error for %s but got %v", fieldVal.Name, errs)
 		} else {
@@ -937,14 +937,14 @@ func TestValidateAllowedVolumes(t *testing.T) {
 
 		// now add the fstype directly to the scc and it should validate
 		scc.Volumes = []securityapi.FSType{fsType}
-		errs = provider.ValidateContainerSecurityContext(pod, &pod.Spec.Containers[0], field.NewPath(""))
+		errs = provider.ValidatePodSecurityContext(pod, field.NewPath(""))
 		if len(errs) != 0 {
 			t.Errorf("directly allowing volume expected no errors for %s but got %v", fieldVal.Name, errs)
 		}
 
 		// now change the scc to allow any volumes and the pod should still validate
 		scc.Volumes = []securityapi.FSType{securityapi.FSTypeAll}
-		errs = provider.ValidateContainerSecurityContext(pod, &pod.Spec.Containers[0], field.NewPath(""))
+		errs = provider.ValidatePodSecurityContext(pod, field.NewPath(""))
 		if len(errs) != 0 {
 			t.Errorf("wildcard volume expected no errors for %s but got %v", fieldVal.Name, errs)
 		}