Service cap-add/cap-drop: handle updates as "tri-state"

Adding/removing capabilities when updating a service is considered a tri-state; - if the capability was previously "dropped", then remove it from "CapabilityDrop", but do NOT add it to "CapabilityAdd". However, if the capability was not yet in the service's "CapabilityDrop", then simply add it to the service's "CapabilityAdd" - likewise, if the capability was previously "added", then remove it from "CapabilityAdd", but do NOT add it to "CapabilityDrop". If the capability was not yet in the service's "CapabilityAdd", then simply add it to the service's "CapabilityDrop". In other words, given a service with the following: | CapDrop | CapAdd | | -------------- | ------------- | | CAP_SOME_CAP | | When updating the service, and applying `--cap-add CAP_SOME_CAP`, the previously dropped capability is removed: | CapDrop | CapAdd | | -------------- | ------------- | | | | When updating the service a second time, applying `--cap-add CAP_SOME_CAP`, capability is now added: | CapDrop | CapAdd | | -------------- | ------------- | | | CAP_SOME_CAP | Signed-off-by: Sebastiaan van Stijn <[email protected]>
docker · Aug 26, 2020 · b9f9bc9 · b9f9bc9
1 parent 6190750
commit b9f9bc9
Show file tree

Hide file tree

Showing 2 changed files with 69 additions and 8 deletions.
diff --git a/cli/command/service/update.go b/cli/command/service/update.go
@@ -1376,6 +1376,37 @@ func updateCredSpecConfig(flags *pflag.FlagSet, containerSpec *swarm.ContainerSp
 //    both `--cap-add` and `--cap-drop` are specifying the same capability.
 // b) Both options accept a magic "ALL" capability; adding "ALL" capabilities
 //    defeats "dropping" any capability.
+//
+// Adding/removing capabilities when updating a service is handled as a tri-state;
+//
+// - if the capability was previously "dropped", then remove it from "CapabilityDrop",
+//   but do NOT add it to "CapabilityAdd". However, if the capability was not yet in
+//   the service's "CapabilityDrop", then simply add it to the service's "CapabilityAdd"
+// - likewise, if the capability was previously "added", then remove it from
+//   "CapabilityAdd", but do NOT add it to "CapabilityDrop". If the capability was
+//   not yet in the service's "CapabilityAdd", then simply add it to the service's
+//   "CapabilityDrop".
+//
+// In other words, given a service with the following:
+//
+// | CapDrop        | CapAdd        |
+// | -------------- | ------------- |
+// | CAP_SOME_CAP   |               |
+//
+// When updating the service, and applying `--cap-add CAP_SOME_CAP`, the previously
+// dropped capability is removed:
+//
+// | CapDrop        | CapAdd        |
+// | -------------- | ------------- |
+// |                |               |
+//
+// After updating the service a second time, applying `--cap-add CAP_SOME_CAP`,
+// capability is now added:
+//
+// | CapDrop        | CapAdd        |
+// | -------------- | ------------- |
+// |                | CAP_SOME_CAP  |
+//
 func updateCapabilities(flags *pflag.FlagSet, containerSpec *swarm.ContainerSpec) {
 	var (
 		toAdd, toDrop map[string]struct{}
@@ -1405,15 +1436,29 @@ func updateCapabilities(flags *pflag.FlagSet, containerSpec *swarm.ContainerSpec
 	if flags.Changed(flagCapDrop) {
 		// First remove the capabilities to "drop" from the service's exiting
 		// list of capabilities to "add".
+		//
+		// Dropping a capability when updating a service is considered a tri-state;
+		//
+		// - if the capability was previously "added", then remove it from
+		//   "CapabilityAdd", and do NOT add it to "CapabilityDrop"
+		// - if the capability was not yet in the service's "CapabilityAdd",
+		//   then simply add it to the service's "CapabilityDrop"
 		if _, ok := toDrop[opts.AllCapabilities]; ok {
-			capDrop = toDrop
+			if _, ok := capAdd[opts.AllCapabilities]; ok {
+				capDrop = make(map[string]struct{})
+			} else {
+				capDrop = toDrop
+			}
 			// Dropping ALL capabilities resets the existing "CapabilityAdd".
 			// New capabilities can be added further down, if `--cap-add` is set.
 			capAdd = make(map[string]struct{})
 		} else {
 			for c := range toDrop {
-				delete(capAdd, c)
-				capDrop[c] = struct{}{}
+				if _, ok := capAdd[c]; ok {
+					delete(capAdd, c)
+				} else {
+					capDrop[c] = struct{}{}
+				}
 			}
 		}
 	}
@@ -1427,8 +1472,17 @@ func updateCapabilities(flags *pflag.FlagSet, containerSpec *swarm.ContainerSpec
 		// capability is set both as "add" and "drop", remove the capability from
 		// the service's list of dropped capabilities (if present).
 		for c := range toAdd {
-			delete(capDrop, c)
-			capAdd[c] = struct{}{}
+			// Adding a capability when updating a service is considered a tri-state;
+			//
+			// - if the capability was previously "dropped", then remove it from
+			//   "CapabilityDrop", and do NOT add it to "CapabilityAdd"
+			// - if the capability was not yet in the service's "CapabilityDrop",
+			//   then simply add it to the service's "CapabilityAdd"
+			if _, ok := capDrop[c]; ok {
+				delete(capDrop, c)
+			} else {
+				capAdd[c] = struct{}{}
+			}
 		}
 	}
 

diff --git a/cli/command/service/update_test.go b/cli/command/service/update_test.go
@@ -1392,7 +1392,6 @@ func TestUpdateCaps(t *testing.T) {
 			spec: &swarm.ContainerSpec{
 				CapabilityDrop: []string{"CAP_NET_ADMIN"},
 			},
-			expectedAdd: []string{"CAP_NET_ADMIN"},
 		},
 		{
 			name:     "Drop a previously requested cap, and add a new one",
@@ -1401,8 +1400,7 @@ func TestUpdateCaps(t *testing.T) {
 			spec: &swarm.ContainerSpec{
 				CapabilityAdd: []string{"CAP_NET_ADMIN"},
 			},
-			expectedDrop: []string{"CAP_NET_ADMIN"},
-			expectedAdd:  []string{"CAP_CHOWN"},
+			expectedAdd: []string{"CAP_CHOWN"},
 		},
 		{
 			name:    "Add caps to service that has ALL caps has no effect",
@@ -1412,6 +1410,15 @@ func TestUpdateCaps(t *testing.T) {
 			},
 			expectedAdd: []string{"ALL"},
 		},
+		{
+			name:     "Drop ALL caps, then add new caps to service that has ALL caps",
+			flagAdd:  []string{"CAP_NET_ADMIN"},
+			flagDrop: []string{"ALL"},
+			spec: &swarm.ContainerSpec{
+				CapabilityAdd: []string{"ALL"},
+			},
+			expectedAdd: []string{"CAP_NET_ADMIN"},
+		},
 		{
 			name:        "Add takes precedence on empty spec",
 			flagAdd:     []string{"CAP_NET_ADMIN"},