From ba1ef7a6c489b016fdc204e92f6b7fa6e3e58170 Mon Sep 17 00:00:00 2001
From: Lennart Espe <lnsp@users.noreply.github.com>
Date: Wed, 7 Mar 2018 21:41:27 +0100
Subject: [PATCH] Improve PodSecurityPolicy group validate error message on
 out-of-range group IDs

---
 .../podsecuritypolicy/group/mustrunas.go      |  2 +-
 .../podsecuritypolicy/group/mustrunas_test.go | 26 +++++++++++--------
 .../podsecuritypolicy/provider_test.go        |  4 +--
 3 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/pkg/security/podsecuritypolicy/group/mustrunas.go b/pkg/security/podsecuritypolicy/group/mustrunas.go
index 8fc48ac294e..0ec73053dd3 100644
--- a/pkg/security/podsecuritypolicy/group/mustrunas.go
+++ b/pkg/security/podsecuritypolicy/group/mustrunas.go
@@ -70,7 +70,7 @@ func (s *mustRunAs) Validate(_ *api.Pod, groups []int64) field.ErrorList {
 
 	for _, group := range groups {
 		if !s.isGroupValid(group) {
-			detail := fmt.Sprintf("%d is not an allowed group", group)
+			detail := fmt.Sprintf("group %d must be in the ranges: %v", group, s.ranges)
 			allErrs = append(allErrs, field.Invalid(field.NewPath(s.field), groups, detail))
 		}
 	}
diff --git a/pkg/security/podsecuritypolicy/group/mustrunas_test.go b/pkg/security/podsecuritypolicy/group/mustrunas_test.go
index 462344cdd13..1e7dfe779eb 100644
--- a/pkg/security/podsecuritypolicy/group/mustrunas_test.go
+++ b/pkg/security/podsecuritypolicy/group/mustrunas_test.go
@@ -17,8 +17,10 @@ limitations under the License.
 package group
 
 import (
-	"k8s.io/kubernetes/pkg/apis/extensions"
+	"strings"
 	"testing"
+
+	"k8s.io/kubernetes/pkg/apis/extensions"
 )
 
 func TestMustRunAsOptions(t *testing.T) {
@@ -108,19 +110,21 @@ func TestGenerate(t *testing.T) {
 
 func TestValidate(t *testing.T) {
 	tests := map[string]struct {
-		ranges []extensions.GroupIDRange
-		groups []int64
-		pass   bool
+		ranges        []extensions.GroupIDRange
+		groups        []int64
+		expectedError string
 	}{
 		"nil security context": {
 			ranges: []extensions.GroupIDRange{
 				{Min: 1, Max: 3},
 			},
+			expectedError: "unable to validate empty groups against required ranges",
 		},
 		"empty groups": {
 			ranges: []extensions.GroupIDRange{
 				{Min: 1, Max: 3},
 			},
+			expectedError: "unable to validate empty groups against required ranges",
 		},
 		"not in range": {
 			groups: []int64{5},
@@ -128,34 +132,31 @@ func TestValidate(t *testing.T) {
 				{Min: 1, Max: 3},
 				{Min: 4, Max: 4},
 			},
+			expectedError: "group 5 must be in the ranges: [{1 3} {4 4}]",
 		},
 		"in range 1": {
 			groups: []int64{2},
 			ranges: []extensions.GroupIDRange{
 				{Min: 1, Max: 3},
 			},
-			pass: true,
 		},
 		"in range boundary min": {
 			groups: []int64{1},
 			ranges: []extensions.GroupIDRange{
 				{Min: 1, Max: 3},
 			},
-			pass: true,
 		},
 		"in range boundary max": {
 			groups: []int64{3},
 			ranges: []extensions.GroupIDRange{
 				{Min: 1, Max: 3},
 			},
-			pass: true,
 		},
 		"singular range": {
 			groups: []int64{4},
 			ranges: []extensions.GroupIDRange{
 				{Min: 4, Max: 4},
 			},
-			pass: true,
 		},
 	}
 
@@ -165,11 +166,14 @@ func TestValidate(t *testing.T) {
 			t.Errorf("error creating strategy for %s: %v", k, err)
 		}
 		errs := s.Validate(nil, v.groups)
-		if v.pass && len(errs) > 0 {
+		if v.expectedError == "" && len(errs) > 0 {
 			t.Errorf("unexpected errors for %s: %v", k, errs)
 		}
-		if !v.pass && len(errs) == 0 {
-			t.Errorf("expected no errors for %s but got: %v", k, errs)
+		if v.expectedError != "" && len(errs) == 0 {
+			t.Errorf("expected errors for %s but got: %v", k, errs)
+		}
+		if v.expectedError != "" && len(errs) > 0 && !strings.Contains(errs[0].Error(), v.expectedError) {
+			t.Errorf("expected error for %s: %v, but got: %v", k, v.expectedError, errs[0])
 		}
 	}
 }
diff --git a/pkg/security/podsecuritypolicy/provider_test.go b/pkg/security/podsecuritypolicy/provider_test.go
index aabaf9ffb9b..b651654dec2 100644
--- a/pkg/security/podsecuritypolicy/provider_test.go
+++ b/pkg/security/podsecuritypolicy/provider_test.go
@@ -291,7 +291,7 @@ func TestValidatePodSecurityContextFailures(t *testing.T) {
 		"failSupplementalGroupOutOfRange": {
 			pod:           failSupplementalGroupPod,
 			psp:           failSupplementalGroupPSP,
-			expectedError: "999 is not an allowed group",
+			expectedError: "group 999 must be in the ranges: [{1 1}]",
 		},
 		"failSupplementalGroupEmpty": {
 			pod:           defaultPod(),
@@ -301,7 +301,7 @@ func TestValidatePodSecurityContextFailures(t *testing.T) {
 		"failFSGroupOutOfRange": {
 			pod:           failFSGroupPod,
 			psp:           failFSGroupPSP,
-			expectedError: "999 is not an allowed group",
+			expectedError: "group 999 must be in the ranges: [{1 1}]",
 		},
 		"failFSGroupEmpty": {
 			pod:           defaultPod(),