Service cap-add/cap-drop: improve handling of combinations and specia…

…l "ALL" value

When creating and updating services, we need to avoid unneeded service churn.

The interaction of separate lists to "add" and "drop" capabilities, a special
("ALL") capability, as well as a "relaxed" format for accepted capabilities
(case-insensitive, `CAP_` prefix optional) make this rather involved.

This patch updates how we handle `--cap-add` / `--cap-drop` when  _creating_ as
well as _updating_, with the following rules/assumptions applied:

- both existing (service spec) and new (values passed through flags or in
  the compose-file) are normalized and de-duplicated before use.
- the special "ALL" capability is equivalent to "all capabilities" and taken
  into account when normalizing capabilities. Combining "ALL" capabilities
  and other capabilities is therefore equivalent to just specifying "ALL".
- adding capabilities takes precedence over dropping, which means that if
  a capability is both set to be "dropped" and to be "added", it is removed
  from the list to "drop".
- the final lists should be sorted and normalized to reduce service churn
- no validation of capabilities is handled by the client. Validation is
  delegated to the daemon/server.

When deploying a service using a docker-compose file, the docker-compose file
is *mostly* handled as being "declarative". However, many of the issues outlined
above also apply to compose-files, so similar handling is applied to compose
files as well to prevent service churn.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

cli/command/service/opts.go

@@ -689,6 +689,8 @@ func (options *serviceOptions) ToService(ctx context.Context, apiClient client.N
 		return service, err
 	}
 
+	capAdd, capDrop := opts.EffectiveCapAddCapDrop(options.capAdd.GetAll(), options.capDrop.GetAll())
+
 	service = swarm.ServiceSpec{
 		Annotations: swarm.Annotations{
 			Name:   options.name,
@@ -720,8 +722,8 @@ func (options *serviceOptions) ToService(ctx context.Context, apiClient client.N
 				Healthcheck:     healthConfig,
 				Isolation:       container.Isolation(options.isolation),
 				Sysctls:         opts.ConvertKVStringsToMap(options.sysctls.GetAll()),
-				CapabilityAdd:   options.capAdd.GetAll(),
-				CapabilityDrop:  options.capDrop.GetAll(),
+				CapabilityAdd:   capAdd,
+				CapabilityDrop:  capDrop,
 			},
 			Networks:      networks,
 			Resources:     resources,

cli/command/service/update.go

@@ -505,7 +505,10 @@ func updateService(ctx context.Context, apiClient client.NetworkAPIClient, flags
 	}
 
 	updateString(flagStopSignal, &cspec.StopSignal)
-	updateCapabilities(flags, cspec)
+
+	if anyChanged(flags, flagCapAdd, flagCapDrop) {
+		updateCapabilities(flags, cspec)
+	}
 
 	return nil
 }
@@ -1351,40 +1354,71 @@ func updateCredSpecConfig(flags *pflag.FlagSet, containerSpec *swarm.ContainerSp
 	}
 }
 
+// updateCapabilities calculates the list of capabilities to "drop" and to "add"
+// after applying the capabilities passed through `--cap-add` and `--cap-drop`
+// to the existing list of added/dropped capabilities in the service spec.
+//
+// Adding capabilities takes precedence over "dropping" the same capability, so
+// if both `--cap-add` and `--cap-drop` are specifying the same capability, the
+// `--cap-drop` is ignored.
+//
+// Capabilities to "drop" are removed from the existing list of "added"
+// capabilities, and vice-versa (capabilities to "add" are removed from the existing
+// list of capabilities to "drop").
+//
+// Capabilities are normalized, sorted, and duplicates are removed to prevent
+// service tasks from being updated if no changes are made. If a list has the "ALL"
+// capability set, then any other capability is removed from that list.
 func updateCapabilities(flags *pflag.FlagSet, containerSpec *swarm.ContainerSpec) {
-	var addToRemove, dropToRemove map[string]struct{}
-	capAdd := containerSpec.CapabilityAdd
-	capDrop := containerSpec.CapabilityDrop
+	var (
+		toAdd, toDrop map[string]bool
 
-	// First add the capabilities passed to --cap-add to the list of requested caps
+		capDrop = opts.CapabilitiesMap(containerSpec.CapabilityDrop)
+		capAdd  = opts.CapabilitiesMap(containerSpec.CapabilityAdd)
+	)
 	if flags.Changed(flagCapAdd) {
-		caps := flags.Lookup(flagCapAdd).Value.(*opts.ListOpts).GetAll()
-		capAdd = append(capAdd, caps...)
-
-		dropToRemove = buildToRemoveSet(flags, flagCapAdd)
+		toAdd = opts.CapabilitiesMap(flags.Lookup(flagCapAdd).Value.(*opts.ListOpts).GetAll())
 	}
-
-	// And add the capabilities passed to --cap-drop to the list of dropped caps
 	if flags.Changed(flagCapDrop) {
-		caps := flags.Lookup(flagCapDrop).Value.(*opts.ListOpts).GetAll()
-		capDrop = append(capDrop, caps...)
-
-		addToRemove = buildToRemoveSet(flags, flagCapDrop)
+		toDrop = opts.CapabilitiesMap(flags.Lookup(flagCapDrop).Value.(*opts.ListOpts).GetAll())
 	}
 
-	// Then take care of removing caps passed to --cap-drop from the list of requested caps
-	containerSpec.CapabilityAdd = make([]string, 0, len(capAdd))
-	for _, cap := range capAdd {
-		if _, exists := addToRemove[cap]; !exists {
-			containerSpec.CapabilityAdd = append(containerSpec.CapabilityAdd, cap)
+	// First remove the capabilities to "drop" from the service's exiting
+	// list of capabilities to "add". If a capability is both added and dropped
+	// on update, then "adding" takes precedence.
+	for c := range toDrop {
+		if !toAdd[c] {
+			delete(capAdd, c)
+			capDrop[c] = true
 		}
 	}
 
-	// And remove the caps passed to --cap-add from the list of caps to drop
-	containerSpec.CapabilityDrop = make([]string, 0, len(capDrop))
-	for _, cap := range capDrop {
-		if _, exists := dropToRemove[cap]; !exists {
-			containerSpec.CapabilityDrop = append(containerSpec.CapabilityDrop, cap)
-		}
+	// And remove the capabilities we're "adding" from the service's existing
+	// list of capabilities to "drop".
+	//
+	// "Adding" capabilities takes precedence over "dropping" them, so if a
+	// 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] = true
+	}
+
+	// Now that the service's existing lists are updated, apply the new
+	// capabilities to add/drop to both lists. Sort the lists to prevent
+	// unneeded updates to service-tasks.
+	containerSpec.CapabilityDrop = capsList(capDrop)
+	containerSpec.CapabilityAdd = capsList(capAdd)
+}
+
+func capsList(caps map[string]bool) []string {
+	if caps[opts.AllCapabilities] {
+		return []string{opts.AllCapabilities}
+	}
+	var out []string
+	for c := range caps {
+		out = append(out, c)
 	}
+	sort.Strings(out)
+	return out
 }

cli/command/service/update_test.go

@@ -1358,52 +1358,170 @@ func TestUpdateCaps(t *testing.T) {
 		// expectedDrop is the set of dropped caps the ContainerSpec should have once updated
 		expectedDrop []string
 	}{
+		{
+			// Note that this won't be run as updateCapabilities is gated by anyChanged(flags, flagCapAdd, flagCapDrop)
+			name: "Empty spec, no updates",
+			spec: &swarm.ContainerSpec{},
+		},
+		{
+			// Note that this won't be run as updateCapabilities is gated by anyChanged(flags, flagCapAdd, flagCapDrop)
+			name: "No updates",
+			spec: &swarm.ContainerSpec{
+				CapabilityAdd:  []string{"CAP_MOUNT", "CAP_NET_ADMIN"},
+				CapabilityDrop: []string{"CAP_CHOWN", "CAP_SYS_ADMIN"},
+			},
+			expectedAdd:  []string{"CAP_MOUNT", "CAP_NET_ADMIN"},
+			expectedDrop: []string{"CAP_CHOWN", "CAP_SYS_ADMIN"},
+		},
+		{
+			// Note that this won't be run as updateCapabilities is gated by anyChanged(flags, flagCapAdd, flagCapDrop)
+			name:     "Empty updates",
+			flagAdd:  []string{},
+			flagDrop: []string{},
+			spec: &swarm.ContainerSpec{
+				CapabilityAdd:  []string{"CAP_MOUNT", "CAP_NET_ADMIN"},
+				CapabilityDrop: []string{"CAP_CHOWN", "CAP_SYS_ADMIN"},
+			},
+			expectedAdd:  []string{"CAP_MOUNT", "CAP_NET_ADMIN"},
+			expectedDrop: []string{"CAP_CHOWN", "CAP_SYS_ADMIN"},
+		},
+		{
+			// Note that this won't be run as updateCapabilities is gated by anyChanged(flags, flagCapAdd, flagCapDrop)
+			name:     "Normalize cap-add only",
+			flagAdd:  []string{},
+			flagDrop: []string{},
+			spec: &swarm.ContainerSpec{
+				CapabilityAdd: []string{"ALL", "CAP_MOUNT", "CAP_NET_ADMIN"},
+			},
+			expectedAdd:  []string{"ALL"},
+			expectedDrop: nil,
+		},
+		{
+			// Note that this won't be run as updateCapabilities is gated by anyChanged(flags, flagCapAdd, flagCapDrop)
+			name: "Normalize cap-drop only",
+			spec: &swarm.ContainerSpec{
+				CapabilityDrop: []string{"ALL", "CAP_MOUNT", "CAP_NET_ADMIN"},
+			},
+			expectedDrop: []string{"ALL"},
+		},
 		{
 			name:         "Add new caps",
-			flagAdd:      []string{"NET_ADMIN"},
+			flagAdd:      []string{"CAP_NET_ADMIN"},
 			flagDrop:     []string{},
 			spec:         &swarm.ContainerSpec{},
-			expectedAdd:  []string{"NET_ADMIN"},
-			expectedDrop: []string{},
+			expectedAdd:  []string{"CAP_NET_ADMIN"},
+			expectedDrop: nil,
 		},
 		{
 			name:         "Drop new caps",
 			flagAdd:      []string{},
-			flagDrop:     []string{"CAP_MKNOD"},
+			flagDrop:     []string{"CAP_NET_ADMIN"},
 			spec:         &swarm.ContainerSpec{},
-			expectedAdd:  []string{},
-			expectedDrop: []string{"CAP_MKNOD"},
+			expectedAdd:  nil,
+			expectedDrop: []string{"CAP_NET_ADMIN"},
 		},
 		{
 			name:     "Add a previously dropped cap",
-			flagAdd:  []string{"NET_ADMIN"},
+			flagAdd:  []string{"CAP_NET_ADMIN"},
 			flagDrop: []string{},
 			spec: &swarm.ContainerSpec{
-				CapabilityDrop: []string{"NET_ADMIN"},
+				CapabilityDrop: []string{"CAP_NET_ADMIN"},
 			},
-			expectedAdd:  []string{"NET_ADMIN"},
-			expectedDrop: []string{},
+			expectedAdd:  []string{"CAP_NET_ADMIN"},
+			expectedDrop: nil,
 		},
 		{
-			name:     "Drop a previously requested cap",
-			flagAdd:  []string{},
-			flagDrop: []string{"CAP_MKNOD"},
+			name:     "Drop a previously requested cap, and add a new one",
+			flagAdd:  []string{"CAP_CHOWN"},
+			flagDrop: []string{"CAP_NET_ADMIN"},
+			spec: &swarm.ContainerSpec{
+				CapabilityAdd: []string{"CAP_NET_ADMIN"},
+			},
+			expectedDrop: []string{"CAP_NET_ADMIN"},
+			expectedAdd:  []string{"CAP_CHOWN"},
+		},
+		{
+			name:    "Add caps to service that has ALL caps has no effect",
+			flagAdd: []string{"CAP_NET_ADMIN"},
+			spec: &swarm.ContainerSpec{
+				CapabilityAdd: []string{"ALL"},
+			},
+			expectedAdd:  []string{"ALL"},
+			expectedDrop: nil,
+		},
+		{
+			name:         "Add takes precedence on empty spec",
+			flagAdd:      []string{"CAP_NET_ADMIN"},
+			flagDrop:     []string{"CAP_NET_ADMIN"},
+			spec:         &swarm.ContainerSpec{},
+			expectedAdd:  []string{"CAP_NET_ADMIN"},
+			expectedDrop: nil,
+		},
+		{
+			name:     "Add takes precedence on existing spec",
+			flagAdd:  []string{"CAP_NET_ADMIN"},
+			flagDrop: []string{"CAP_NET_ADMIN"},
+			spec: &swarm.ContainerSpec{
+				CapabilityAdd:  []string{"CAP_NET_ADMIN"},
+				CapabilityDrop: []string{"CAP_NET_ADMIN"},
+			},
+			expectedAdd:  []string{"CAP_NET_ADMIN"},
+			expectedDrop: nil,
+		},
+		{
+			name:     "Drop all, and add new caps",
+			flagAdd:  []string{"CAP_CHOWN"},
+			flagDrop: []string{"ALL"},
+			spec: &swarm.ContainerSpec{
+				CapabilityAdd:  []string{"CAP_NET_ADMIN", "CAP_MOUNT"},
+				CapabilityDrop: []string{"CAP_NET_ADMIN", "CAP_MOUNT"},
+			},
+			expectedAdd:  []string{"CAP_CHOWN", "CAP_MOUNT", "CAP_NET_ADMIN"},
+			expectedDrop: []string{"ALL"},
+		},
+		{
+			name:     "Add all caps",
+			flagAdd:  []string{"ALL"},
+			flagDrop: []string{"CAP_NET_ADMIN", "CAP_SYS_ADMIN"},
+			spec: &swarm.ContainerSpec{
+				CapabilityAdd:  []string{"CAP_NET_ADMIN"},
+				CapabilityDrop: []string{"CAP_CHOWN"},
+			},
+			expectedAdd:  []string{"ALL"},
+			expectedDrop: []string{"CAP_CHOWN", "CAP_NET_ADMIN", "CAP_SYS_ADMIN"},
+		},
+		{
+			name:     "Drop all, and add all",
+			flagAdd:  []string{"ALL"},
+			flagDrop: []string{"ALL"},
+			spec: &swarm.ContainerSpec{
+				CapabilityAdd:  []string{"CAP_NET_ADMIN"},
+				CapabilityDrop: []string{"CAP_CHOWN"},
+			},
+			expectedAdd:  []string{"ALL"},
+			expectedDrop: []string{"CAP_CHOWN"},
+		},
+		{
+			name:     "Caps are normalized and sorted",
+			flagAdd:  []string{"bbb", "aaa", "cAp_bBb", "cAp_aAa"},
+			flagDrop: []string{"zzz", "yyy", "cAp_yYy", "cAp_yYy"},
 			spec: &swarm.ContainerSpec{
-				CapabilityAdd: []string{"CAP_MKNOD"},
+				CapabilityAdd:  []string{"ccc", "CAP_DDD"},
+				CapabilityDrop: []string{"www", "CAP_XXX"},
 			},
-			expectedAdd:  []string{},
-			expectedDrop: []string{"CAP_MKNOD"},
+			expectedAdd:  []string{"CAP_AAA", "CAP_BBB", "CAP_CCC", "CAP_DDD"},
+			expectedDrop: []string{"CAP_WWW", "CAP_XXX", "CAP_YYY", "CAP_ZZZ"},
 		},
 	}
 
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
 			flags := newUpdateCommand(nil).Flags()
-			for _, cap := range tc.flagAdd {
-				flags.Set(flagCapAdd, cap)
+			for _, c := range tc.flagAdd {
+				_ = flags.Set(flagCapAdd, c)
 			}
-			for _, cap := range tc.flagDrop {
-				flags.Set(flagCapDrop, cap)
+			for _, c := range tc.flagDrop {
+				_ = flags.Set(flagCapDrop, c)
 			}
 
 			updateCapabilities(flags, tc.spec)

cli/compose/convert/service.go

@@ -117,6 +117,8 @@ func Service(
 		}
 	}
 
+	capAdd, capDrop := opts.EffectiveCapAddCapDrop(service.CapAdd, service.CapDrop)
+
 	serviceSpec := swarm.ServiceSpec{
 		Annotations: swarm.Annotations{
 			Name:   name,
@@ -147,8 +149,8 @@ func Service(
 				Isolation:       container.Isolation(service.Isolation),
 				Init:            service.Init,
 				Sysctls:         service.Sysctls,
-				CapabilityAdd:   service.CapAdd,
-				CapabilityDrop:  service.CapDrop,
+				CapabilityAdd:   capAdd,
+				CapabilityDrop:  capDrop,
 			},
 			LogDriver:     logDriver,
 			Resources:     resources,