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". This also implies that *adding* "ALL" capabilities
  defeats any capability that was added to be "dropped" (and thus equivalent
  to an empty "drop" list).
- the final lists should be sorted and normalized to reduce service churn
- Except for special handling of the "ALL" value, 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 <[email protected]>

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
 }
@@ -1353,40 +1356,82 @@ 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.
+//
+// Capabilities are normalized, sorted, and duplicates are removed to prevent
+// service tasks from being updated if no changes are made.
+//
+// Calculating the effective list of capabilities to add/drop is done in two steps:
+//
+// First, 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").
+//
+// After that, capabilities to "drop" or "add" are appended to the existing lists,
+// taking into account that:
+//
+// a) Adding capabilities takes precedence over "dropping" the same capability
+// b) Both options accept a magic "ALL" capability; adding "ALL" capabilities
+//    defeats "dropping" any capability.
 func updateCapabilities(flags *pflag.FlagSet, containerSpec *swarm.ContainerSpec) {
-	var addToRemove, dropToRemove map[string]struct{}
-	capAdd := containerSpec.CapabilityAdd
-	capDrop := containerSpec.CapabilityDrop
-
-	// First add the capabilities passed to --cap-add to the list of requested caps
+	var toAdd map[string]struct{}
 	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())
+	}
+	if _, ok := toAdd[opts.AllCapabilities]; ok {
+		// Special case: "adding" capabilities takes precedence over "dropping",
+		// therefore adding "ALL" capabilities resets all dropped capabilities.
+		containerSpec.CapabilityDrop = nil
+		containerSpec.CapabilityAdd = []string{opts.AllCapabilities}
+		return
 	}
 
-	// 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...)
+	var (
+		capDrop = opts.CapabilitiesMap(containerSpec.CapabilityDrop)
+		capAdd  = opts.CapabilitiesMap(containerSpec.CapabilityAdd)
+	)
 
-		addToRemove = buildToRemoveSet(flags, flagCapDrop)
+	if flags.Changed(flagCapDrop) {
+		// First remove the capabilities to "drop" from the service's exiting
+		// list of capabilities to "add".
+		toDrop := opts.CapabilitiesMap(flags.Lookup(flagCapDrop).Value.(*opts.ListOpts).GetAll())
+		if _, ok := toDrop[opts.AllCapabilities]; ok {
+			// Dropping ALL capabilities resets the existing "CapabilityAdd"
+			capAdd = make(map[string]struct{})
+			capDrop = toDrop
+		} else {
+			for c := range toDrop {
+				delete(capAdd, c)
+				capDrop[c] = struct{}{}
+			}
+		}
 	}
 
-	// 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)
-		}
+	// And remove the capabilities to "add" 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] = struct{}{}
 	}
 
-	// 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)
-		}
+	// 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 = asSortedSlice(capDrop)
+	containerSpec.CapabilityAdd = asSortedSlice(capAdd)
+}
+
+func asSortedSlice(vals map[string]struct{}) []string {
+	var o []string
+	for c := range vals {
+		o = append(o, c)
 	}
+	sort.Strings(o)
+	return o
 }

cli/command/service/update_test.go

@@ -1327,51 +1327,134 @@ func TestUpdateCaps(t *testing.T) {
 		expectedDrop []string
 	}{
 		{
-			name:         "Add new caps",
-			flagAdd:      []string{"NET_ADMIN"},
-			flagDrop:     []string{},
-			spec:         &swarm.ContainerSpec{},
-			expectedAdd:  []string{"NET_ADMIN"},
-			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"},
+		},
+		{
+			// 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{"CAP_NET_ADMIN"},
+			flagDrop:    []string{},
+			spec:        &swarm.ContainerSpec{},
+			expectedAdd: []string{"CAP_NET_ADMIN"},
 		},
 		{
 			name:         "Drop new caps",
 			flagAdd:      []string{},
-			flagDrop:     []string{"CAP_MKNOD"},
+			flagDrop:     []string{"CAP_NET_ADMIN"},
 			spec:         &swarm.ContainerSpec{},
-			expectedAdd:  []string{},
-			expectedDrop: []string{"CAP_MKNOD"},
+			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"},
 		},
 		{
 			name:     "Drop a previously requested cap",
 			flagAdd:  []string{},
-			flagDrop: []string{"CAP_MKNOD"},
+			flagDrop: []string{"CAP_NET_ADMIN"},
+			spec: &swarm.ContainerSpec{
+				CapabilityAdd: []string{"CAP_NET_ADMIN"},
+			},
+			expectedDrop: []string{"CAP_NET_ADMIN"},
+		},
+		{
+			name:     "Add takes precedence",
+			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"},
+		},
+		{
+			name:     "Drop all before adding 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"},
+			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"},
+		},
+		{
+			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,

cli/compose/convert/service_test.go

@@ -625,27 +625,42 @@ func TestConvertUpdateConfigParallelism(t *testing.T) {
 }
 
 func TestConvertServiceCapAddAndCapDrop(t *testing.T) {
-	// test default behavior
-	result, err := Service("1.41", Namespace{name: "foo"}, composetypes.ServiceConfig{}, nil, nil, nil, nil)
-	assert.NilError(t, err)
-	assert.Check(t, is.DeepEqual(result.TaskTemplate.ContainerSpec.CapabilityAdd, []string(nil)))
-	assert.Check(t, is.DeepEqual(result.TaskTemplate.ContainerSpec.CapabilityDrop, []string(nil)))
-
-	// with some values
-	service := composetypes.ServiceConfig{
-		CapAdd: []string{
-			"SYS_NICE",
-			"CAP_NET_ADMIN",
+	tests := []struct {
+		title   string
+		in, out composetypes.ServiceConfig
+	}{
+		{
+			title: "default behavior",
 		},
-		CapDrop: []string{
-			"CHOWN",
-			"DAC_OVERRIDE",
-			"CAP_FSETID",
-			"CAP_FOWNER",
+		{
+			title: "some values",
+			in: composetypes.ServiceConfig{
+				CapAdd:  []string{"SYS_NICE", "CAP_NET_ADMIN"},
+				CapDrop: []string{"CHOWN", "CAP_NET_ADMIN", "DAC_OVERRIDE", "CAP_FSETID", "CAP_FOWNER"},
+			},
+			out: composetypes.ServiceConfig{
+				CapAdd:  []string{"CAP_NET_ADMIN", "CAP_SYS_NICE"},
+				CapDrop: []string{"CAP_CHOWN", "CAP_DAC_OVERRIDE", "CAP_FOWNER", "CAP_FSETID"},
+			},
+		},
+		{
+			title: "adding ALL capabilities",
+			in: composetypes.ServiceConfig{
+				CapAdd:  []string{"ALL", "CAP_NET_ADMIN"},
+				CapDrop: []string{"CHOWN", "CAP_NET_ADMIN", "DAC_OVERRIDE", "CAP_FSETID", "CAP_FOWNER"},
+			},
+			out: composetypes.ServiceConfig{
+				CapAdd: []string{"ALL"},
+			},
 		},
 	}
-	result, err = Service("1.41", Namespace{name: "foo"}, service, nil, nil, nil, nil)
-	assert.NilError(t, err)
-	assert.Check(t, is.DeepEqual(result.TaskTemplate.ContainerSpec.CapabilityAdd, service.CapAdd))
-	assert.Check(t, is.DeepEqual(result.TaskTemplate.ContainerSpec.CapabilityDrop, service.CapDrop))
+	for _, tc := range tests {
+		tc := tc
+		t.Run(tc.title, func(t *testing.T) {
+			result, err := Service("1.41", Namespace{name: "foo"}, tc.in, nil, nil, nil, nil)
+			assert.NilError(t, err)
+			assert.Check(t, is.DeepEqual(result.TaskTemplate.ContainerSpec.CapabilityAdd, tc.out.CapAdd))
+			assert.Check(t, is.DeepEqual(result.TaskTemplate.ContainerSpec.CapabilityDrop, tc.out.CapDrop))
+		})
+	}
 }