From cfb490e3a199b48f88b325de6e49b721daa2a250 Mon Sep 17 00:00:00 2001
From: Jordan Liggitt <jliggitt@redhat.com>
Date: Mon, 9 Oct 2017 17:30:13 -0400
Subject: [PATCH] PodSecurityPolicy: avoid unnecessary mutation of container
 capabilities

---
 .../capabilities/mustrunas.go                 | 18 +++++---
 .../capabilities/mustrunas_test.go            | 44 +++++++++++++++----
 2 files changed, 46 insertions(+), 16 deletions(-)

diff --git a/pkg/security/podsecuritypolicy/capabilities/mustrunas.go b/pkg/security/podsecuritypolicy/capabilities/mustrunas.go
index d8efe066db2..ed88883d8ad 100644
--- a/pkg/security/podsecuritypolicy/capabilities/mustrunas.go
+++ b/pkg/security/podsecuritypolicy/capabilities/mustrunas.go
@@ -48,13 +48,17 @@ 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)
 	}
@@ -62,17 +66,17 @@ func (s *defaultCapabilities) Generate(pod *api.Pod, container *api.Container) (
 	// 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
 }
 
diff --git a/pkg/security/podsecuritypolicy/capabilities/mustrunas_test.go b/pkg/security/podsecuritypolicy/capabilities/mustrunas_test.go
index 7e9bfa51551..8f002d4008f 100644
--- a/pkg/security/podsecuritypolicy/capabilities/mustrunas_test.go
+++ b/pkg/security/podsecuritypolicy/capabilities/mustrunas_test.go
@@ -31,6 +31,10 @@ func TestGenerateAdds(t *testing.T) {
 		expectedCaps   *api.Capabilities
 	}{
 		"no required, no container requests": {},
+		"no required, no container requests, non-nil": {
+			containerCaps: &api.Capabilities{},
+			expectedCaps:  &api.Capabilities{},
+		},
 		"required, no container requests": {
 			defaultAddCaps: []api.Capability{"foo"},
 			expectedCaps: &api.Capabilities{
@@ -64,13 +68,22 @@ func TestGenerateAdds(t *testing.T) {
 				Add: []api.Capability{"bar", "foo"},
 			},
 		},
-		"generation dedupes": {
-			defaultAddCaps: []api.Capability{"foo", "foo", "foo", "foo"},
+		"generation does not mutate unnecessarily": {
+			defaultAddCaps: []api.Capability{"foo", "bar"},
 			containerCaps: &api.Capabilities{
-				Add: []api.Capability{"foo", "foo", "foo"},
+				Add: []api.Capability{"foo", "foo", "bar", "baz"},
 			},
 			expectedCaps: &api.Capabilities{
-				Add: []api.Capability{"foo"},
+				Add: []api.Capability{"foo", "foo", "bar", "baz"},
+			},
+		},
+		"generation dedupes": {
+			defaultAddCaps: []api.Capability{"foo", "bar"},
+			containerCaps: &api.Capabilities{
+				Add: []api.Capability{"foo", "baz"},
+			},
+			expectedCaps: &api.Capabilities{
+				Add: []api.Capability{"bar", "baz", "foo"},
 			},
 		},
 		"generation is case sensitive - will not dedupe": {
@@ -121,6 +134,10 @@ 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{
@@ -128,12 +145,21 @@ func TestGenerateDrops(t *testing.T) {
 			},
 		},
 		"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": {
@@ -167,12 +193,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": {