SecurityContextConstraints: avoid unnecessary mutation of container capabilities.

Author: php-coder

pkg/security/securitycontextconstraints/capabilities/mustrunas.go

@@ -33,31 +33,35 @@ func NewDefaultCapabilities(defaultAddCapabilities, requiredDropCapabilities, al
 // 1.  a capabilities.Add set containing all the required adds (unless the
 // 		container specifically is dropping the cap) and container requested adds
 // 2.  a capabilities.Drop set containing all the required drops and container requested drops
+//
+// Returns the original container capabilities if no changes are required.
 func (s *defaultCapabilities) Generate(pod *api.Pod, container *api.Container) (*api.Capabilities, error) {
 	defaultAdd := makeCapSet(s.defaultAddCapabilities)
 	requiredDrop := makeCapSet(s.requiredDropCapabilities)
 	containerAdd := sets.NewString()
 	containerDrop := sets.NewString()
 
+	var containerCapabilities *api.Capabilities
 	if container.SecurityContext != nil && container.SecurityContext.Capabilities != nil {
+		containerCapabilities = container.SecurityContext.Capabilities
 		containerAdd = makeCapSet(container.SecurityContext.Capabilities.Add)
 		containerDrop = makeCapSet(container.SecurityContext.Capabilities.Drop)
 	}
 
 	// remove any default adds that the container is specifically dropping
 	defaultAdd = defaultAdd.Difference(containerDrop)
 
-	combinedAdd := defaultAdd.Union(containerAdd).List()
-	combinedDrop := requiredDrop.Union(containerDrop).List()
+	combinedAdd := defaultAdd.Union(containerAdd)
+	combinedDrop := requiredDrop.Union(containerDrop)
 
-	// nothing generated?  return nil
-	if len(combinedAdd) == 0 && len(combinedDrop) == 0 {
-		return nil, nil
+	// no changes? return the original capabilities
+	if (len(combinedAdd) == len(containerAdd)) && (len(combinedDrop) == len(containerDrop)) {
+		return containerCapabilities, nil
 	}
 
 	return &api.Capabilities{
-		Add:  capabilityFromStringSlice(combinedAdd),
-		Drop: capabilityFromStringSlice(combinedDrop),
+		Add:  capabilityFromStringSlice(combinedAdd.List()),
+		Drop: capabilityFromStringSlice(combinedDrop.List()),
 	}, nil
 }
 

pkg/security/securitycontextconstraints/capabilities/mustrunas_test.go

@@ -19,6 +19,10 @@ func TestGenerateAdds(t *testing.T) {
 		"no required, no container requests": {
 			expectedCaps: nil,
 		},
+		"no required, no container requests, non-nil": {
+			containerCaps: &api.Capabilities{},
+			expectedCaps:  &api.Capabilities{},
+		},
 		"required, no container requests": {
 			defaultAddCaps: []api.Capability{"foo"},
 			expectedCaps: &api.Capabilities{
@@ -52,13 +56,22 @@ func TestGenerateAdds(t *testing.T) {
 				Add: []api.Capability{"bar", "foo"},
 			},
 		},
+		"generation does not mutate unnecessarily": {
+			defaultAddCaps: []api.Capability{"foo", "bar"},
+			containerCaps: &api.Capabilities{
+				Add: []api.Capability{"foo", "foo", "bar", "baz"},
+			},
+			expectedCaps: &api.Capabilities{
+				Add: []api.Capability{"foo", "foo", "bar", "baz"},
+			},
+		},
 		"generation dedupes": {
-			defaultAddCaps: []api.Capability{"foo", "foo", "foo", "foo"},
+			defaultAddCaps: []api.Capability{"foo", "bar"},
 			containerCaps: &api.Capabilities{
-				Add: []api.Capability{"foo", "foo", "foo"},
+				Add: []api.Capability{"foo", "baz"},
 			},
 			expectedCaps: &api.Capabilities{
-				Add: []api.Capability{"foo"},
+				Add: []api.Capability{"bar", "baz", "foo"},
 			},
 		},
 		"generation is case sensitive - will not dedupe": {
@@ -109,19 +122,32 @@ func TestGenerateDrops(t *testing.T) {
 		"no required, no container requests": {
 			expectedCaps: nil,
 		},
+		"no required, no container requests, non-nil": {
+			containerCaps: &api.Capabilities{},
+			expectedCaps:  &api.Capabilities{},
+		},
 		"required drops are defaulted": {
 			requiredDropCaps: []api.Capability{"foo"},
 			expectedCaps: &api.Capabilities{
 				Drop: []api.Capability{"foo"},
 			},
 		},
 		"required drops are defaulted when making container requests": {
-			requiredDropCaps: []api.Capability{"foo"},
+			requiredDropCaps: []api.Capability{"baz"},
 			containerCaps: &api.Capabilities{
 				Drop: []api.Capability{"foo", "bar"},
 			},
 			expectedCaps: &api.Capabilities{
-				Drop: []api.Capability{"bar", "foo"},
+				Drop: []api.Capability{"bar", "baz", "foo"},
+			},
+		},
+		"required drops do not mutate unnecessarily": {
+			requiredDropCaps: []api.Capability{"baz"},
+			containerCaps: &api.Capabilities{
+				Drop: []api.Capability{"foo", "bar", "baz"},
+			},
+			expectedCaps: &api.Capabilities{
+				Drop: []api.Capability{"foo", "bar", "baz"},
 			},
 		},
 		"can drop a required add": {
@@ -155,12 +181,12 @@ func TestGenerateDrops(t *testing.T) {
 			},
 		},
 		"generation dedupes": {
-			requiredDropCaps: []api.Capability{"bar", "bar", "bar", "bar"},
+			requiredDropCaps: []api.Capability{"baz", "foo"},
 			containerCaps: &api.Capabilities{
-				Drop: []api.Capability{"bar", "bar", "bar"},
+				Drop: []api.Capability{"bar", "foo"},
 			},
 			expectedCaps: &api.Capabilities{
-				Drop: []api.Capability{"bar"},
+				Drop: []api.Capability{"bar", "baz", "foo"},
 			},
 		},
 		"generation is case sensitive - will not dedupe": {