Merge pull request #17185 from smarterclayton/scc_cant_be_patched

Automatic merge from submit-queue (batch tested with PRs 17160, 17185).

SCC can't be patched via JSONPatch because users is nil

When users or groups are nil, standard JSONPatch can't be used to add a
new item to the list because the array is nil instead of empty. Alter
the serialization of SCC so that there is always a user or group
array returned.

```
oc patch "securitycontextconstraints.v1.security.openshift.io" "hostnetwork" --type=json --patch="[{\"op\":\"add\",\"path\":\"/users/-\",\"value\":\"system:serviceaccount:myproject:router\"}]"
Error from server: jsonpatch add operation does not apply: doc is missing path: /users/-
```

This allows us to do declarative patching against SCC until we move to
PSP in a future release.

@liggitt realized this while trying to switch router to a declarative model - patch is our best option for update, but you can't actually do a safe addition without JSONPatch and without this change.

/kind bug

api/swagger-spec/api-v1.json

@@ -22774,7 +22774,9 @@
      "allowHostPorts",
      "allowHostPID",
      "allowHostIPC",
-     "readOnlyRootFilesystem"
+     "readOnlyRootFilesystem",
+     "users",
+     "groups"
     ],
     "properties": {
      "kind": {

pkg/security/apis/security/v1/defaults.go

@@ -23,6 +23,13 @@ func SetDefaults_SCC(scc *SecurityContextConstraints) {
 		scc.SupplementalGroups.Type = SupplementalGroupsStrategyRunAsAny
 	}
 
+	if scc.Users == nil {
+		scc.Users = []string{}
+	}
+	if scc.Groups == nil {
+		scc.Groups = []string{}
+	}
+
 	var defaultAllowedVolumes sets.String
 	switch {
 	case scc.Volumes == nil:

pkg/security/apis/security/v1/types.go

@@ -79,9 +79,11 @@ type SecurityContextConstraints struct {
 	ReadOnlyRootFilesystem bool `json:"readOnlyRootFilesystem" protobuf:"varint,17,opt,name=readOnlyRootFilesystem"`
 
 	// The users who have permissions to use this security context constraints
-	Users []string `json:"users,omitempty" protobuf:"bytes,18,rep,name=users"`
+	// +optional
+	Users []string `json:"users" protobuf:"bytes,18,rep,name=users"`
 	// The groups that have permission to use this security context constraints
-	Groups []string `json:"groups,omitempty" protobuf:"bytes,19,rep,name=groups"`
+	// +optional
+	Groups []string `json:"groups" protobuf:"bytes,19,rep,name=groups"`
 
 	// SeccompProfiles lists the allowed profiles that may be set for the pod or
 	// container's seccomp annotations.  An unset (nil) or empty value means that no profiles may

pkg/security/apis/security/v1/zz_generated.conversion.go

@@ -518,8 +518,16 @@ func autoConvert_security_SecurityContextConstraints_To_v1_SecurityContextConstr
 	}
 	out.ReadOnlyRootFilesystem = in.ReadOnlyRootFilesystem
 	out.SeccompProfiles = *(*[]string)(unsafe.Pointer(&in.SeccompProfiles))
-	out.Users = *(*[]string)(unsafe.Pointer(&in.Users))
-	out.Groups = *(*[]string)(unsafe.Pointer(&in.Groups))
+	if in.Users == nil {
+		out.Users = make([]string, 0)
+	} else {
+		out.Users = *(*[]string)(unsafe.Pointer(&in.Users))
+	}
+	if in.Groups == nil {
+		out.Groups = make([]string, 0)
+	} else {
+		out.Groups = *(*[]string)(unsafe.Pointer(&in.Groups))
+	}
 	return nil
 }
 

pkg/security/registry/securitycontextconstraints/strategy.go

@@ -50,7 +50,27 @@ func (strategy) PrepareForCreate(_ genericapirequest.Context, obj runtime.Object
 func (strategy) PrepareForUpdate(_ genericapirequest.Context, obj, old runtime.Object) {
 }
 
+// Canonicalize removes duplicate user and group values, preserving order.
 func (strategy) Canonicalize(obj runtime.Object) {
+	scc := obj.(*securityapi.SecurityContextConstraints)
+	scc.Users = uniqueStrings(scc.Users)
+	scc.Groups = uniqueStrings(scc.Groups)
+}
+
+func uniqueStrings(values []string) []string {
+	if len(values) < 2 {
+		return values
+	}
+	updated := make([]string, 0, len(values))
+	existing := make(map[string]struct{})
+	for _, value := range values {
+		if _, ok := existing[value]; ok {
+			continue
+		}
+		existing[value] = struct{}{}
+		updated = append(updated, value)
+	}
+	return updated
 }
 
 func (strategy) Validate(ctx genericapirequest.Context, obj runtime.Object) field.ErrorList {

pkg/security/registry/securitycontextconstraints/strategy_test.go

@@ -0,0 +1,54 @@
+package securitycontextconstraints
+
+import (
+	"reflect"
+	"testing"
+
+	securityapi "github.com/openshift/origin/pkg/security/apis/security"
+)
+
+func TestCanonicalize(t *testing.T) {
+	testCases := []struct {
+		obj    *securityapi.SecurityContextConstraints
+		expect *securityapi.SecurityContextConstraints
+	}{
+		{
+			obj:    &securityapi.SecurityContextConstraints{},
+			expect: &securityapi.SecurityContextConstraints{},
+		},
+		{
+			obj: &securityapi.SecurityContextConstraints{
+				Users: []string{"a"},
+			},
+			expect: &securityapi.SecurityContextConstraints{
+				Users: []string{"a"},
+			},
+		},
+		{
+			obj: &securityapi.SecurityContextConstraints{
+				Users:  []string{"a", "a"},
+				Groups: []string{"b", "b"},
+			},
+			expect: &securityapi.SecurityContextConstraints{
+				Users:  []string{"a"},
+				Groups: []string{"b"},
+			},
+		},
+		{
+			obj: &securityapi.SecurityContextConstraints{
+				Users:  []string{"a", "b", "a"},
+				Groups: []string{"c", "d", "c"},
+			},
+			expect: &securityapi.SecurityContextConstraints{
+				Users:  []string{"a", "b"},
+				Groups: []string{"c", "d"},
+			},
+		},
+	}
+	for i, testCase := range testCases {
+		Strategy.Canonicalize(testCase.obj)
+		if !reflect.DeepEqual(testCase.expect, testCase.obj) {
+			t.Errorf("%d: unexpected object: %#v", i, testCase.obj)
+		}
+	}
+}