From 95037299cb02bf2f431949e4d08f44bd05ece8dc Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 25 Aug 2020 17:28:59 +0200 Subject: [PATCH] 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 --- cli/command/service/update.go | 59 ++++++++++++++++++++++++++++-- cli/command/service/update_test.go | 16 ++++++-- 2 files changed, 68 insertions(+), 7 deletions(-) diff --git a/cli/command/service/update.go b/cli/command/service/update.go index d7f7d95bd9..8adcdfa8f2 100644 --- a/cli/command/service/update.go +++ b/cli/command/service/update.go @@ -1369,6 +1369,37 @@ func updateCredSpecConfig(flags *pflag.FlagSet, containerSpec *swarm.ContainerSp // 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. +// +// 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 NOT added to "CapabilityAdd". However, if the capability was not yet in +// the service's "CapabilityDrop", then it's simply added to the service's "CapabilityAdd" +// - likewise, if the capability was previously "added", then it's removed from +// "CapabilityAdd", but NOT added 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]bool @@ -1386,10 +1417,20 @@ func updateCapabilities(flags *pflag.FlagSet, containerSpec *swarm.ContainerSpec // 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. + // + // 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" for c := range toDrop { if !toAdd[c] { - delete(capAdd, c) - capDrop[c] = true + if capAdd[c] { + delete(capAdd, c) + } else { + capDrop[c] = true + } } } @@ -1399,9 +1440,19 @@ func updateCapabilities(flags *pflag.FlagSet, containerSpec *swarm.ContainerSpec // "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). + // + // 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" for c := range toAdd { - delete(capDrop, c) - capAdd[c] = true + if capDrop[c] { + delete(capDrop, c) + } else { + capAdd[c] = true + } } // Now that the service's existing lists are updated, apply the new diff --git a/cli/command/service/update_test.go b/cli/command/service/update_test.go index c2a2357c9f..cbae2ddd11 100644 --- a/cli/command/service/update_test.go +++ b/cli/command/service/update_test.go @@ -1427,7 +1427,7 @@ func TestUpdateCaps(t *testing.T) { spec: &swarm.ContainerSpec{ CapabilityDrop: []string{"CAP_NET_ADMIN"}, }, - expectedAdd: []string{"CAP_NET_ADMIN"}, + expectedAdd: nil, expectedDrop: nil, }, { @@ -1437,8 +1437,8 @@ func TestUpdateCaps(t *testing.T) { spec: &swarm.ContainerSpec{ CapabilityAdd: []string{"CAP_NET_ADMIN"}, }, - expectedDrop: []string{"CAP_NET_ADMIN"}, expectedAdd: []string{"CAP_CHOWN"}, + expectedDrop: nil, }, { name: "Add caps to service that has ALL caps has no effect", @@ -1449,6 +1449,16 @@ func TestUpdateCaps(t *testing.T) { expectedAdd: []string{"ALL"}, expectedDrop: nil, }, + { + 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"}, + expectedDrop: nil, + }, { name: "Add takes precedence on empty spec", flagAdd: []string{"CAP_NET_ADMIN"}, @@ -1488,7 +1498,7 @@ func TestUpdateCaps(t *testing.T) { CapabilityDrop: []string{"CAP_CHOWN"}, }, expectedAdd: []string{"ALL"}, - expectedDrop: []string{"CAP_CHOWN", "CAP_NET_ADMIN", "CAP_SYS_ADMIN"}, + expectedDrop: []string{"CAP_CHOWN", "CAP_SYS_ADMIN"}, }, { name: "Drop all, and add all",