Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add capabilities support to stack/service commands #2663

Closed
wants to merge 2 commits into from
Closed

Add capabilities support to stack/service commands #2663

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
87a66af
stack: Support cap_add and cap_drop on services
olljanat Sep 9, 2019
afb7014
service: Add --cap-add & --cap-drop to service cmds
akerouanton Jul 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions cli/command/service/formatter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,15 @@ ContainerSpec:
{{- if .ContainerUser }}
User: {{ .ContainerUser }}
{{- end }}
{{- if .HasCapabilities }}
Capabilities:
{{- if .HasCapabilityAdd }}
Add: {{ .CapabilityAdd }}
{{- end }}
{{- if .HasCapabilityDrop }}
Drop: {{ .CapabilityDrop }}
{{- end }}
{{- end }}
{{- if .ContainerSysCtls }}
SysCtls:
{{- range $k, $v := .ContainerSysCtls }}
Expand Down Expand Up @@ -529,6 +538,26 @@ func (ctx *serviceInspectContext) Ports() []swarm.PortConfig {
return ctx.Service.Endpoint.Ports
}

func (ctx *serviceInspectContext) HasCapabilities() bool {
return len(ctx.Service.Spec.TaskTemplate.ContainerSpec.CapabilityAdd) > 0 || len(ctx.Service.Spec.TaskTemplate.ContainerSpec.CapabilityDrop) > 0
}

func (ctx *serviceInspectContext) HasCapabilityAdd() bool {
return len(ctx.Service.Spec.TaskTemplate.ContainerSpec.CapabilityAdd) > 0
}

func (ctx *serviceInspectContext) HasCapabilityDrop() bool {
return len(ctx.Service.Spec.TaskTemplate.ContainerSpec.CapabilityDrop) > 0
}

func (ctx *serviceInspectContext) CapabilityAdd() string {
return strings.Join(ctx.Service.Spec.TaskTemplate.ContainerSpec.CapabilityAdd, ", ")
}

func (ctx *serviceInspectContext) CapabilityDrop() string {
return strings.Join(ctx.Service.Spec.TaskTemplate.ContainerSpec.CapabilityDrop, ", ")
}

const (
defaultServiceTableFormat = "table {{.ID}}\t{{.Name}}\t{{.Mode}}\t{{.Replicas}}\t{{.Image}}\t{{.Ports}}"

Expand Down
12 changes: 12 additions & 0 deletions cli/command/service/opts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -506,6 +506,8 @@ type serviceOptions struct {
dnsOption opts.ListOpts
hosts opts.ListOpts
sysctls opts.ListOpts
capAdd opts.ListOpts
capDrop opts.ListOpts

resources resourceOptions
stopGrace opts.DurationOpt
Expand Down Expand Up @@ -549,6 +551,8 @@ func newServiceOptions() *serviceOptions {
dnsSearch: opts.NewListOpts(opts.ValidateDNSSearch),
hosts: opts.NewListOpts(opts.ValidateExtraHost),
sysctls: opts.NewListOpts(nil),
capAdd: opts.NewListOpts(nil),
capDrop: opts.NewListOpts(nil),
}
}

Expand Down Expand Up @@ -716,6 +720,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(),
},
Networks: networks,
Resources: resources,
Expand Down Expand Up @@ -818,6 +824,10 @@ func addServiceFlags(flags *pflag.FlagSet, opts *serviceOptions, defaultFlagValu
flags.StringVar(&opts.hostname, flagHostname, "", "Container hostname")
flags.SetAnnotation(flagHostname, "version", []string{"1.25"})
flags.Var(&opts.entrypoint, flagEntrypoint, "Overwrite the default ENTRYPOINT of the image")
flags.Var(&opts.capAdd, flagCapAdd, "Add Linux capabilities")
flags.SetAnnotation(flagCapAdd, "version", []string{"1.41"})
flags.Var(&opts.capDrop, flagCapDrop, "Drop Linux capabilities")
flags.SetAnnotation(flagCapDrop, "version", []string{"1.41"})

flags.Var(&opts.resources.limitCPU, flagLimitCPU, "Limit CPUs")
flags.Var(&opts.resources.limitMemBytes, flagLimitMemory, "Limit Memory")
Expand Down Expand Up @@ -1001,6 +1011,8 @@ const (
flagConfigAdd = "config-add"
flagConfigRemove = "config-rm"
flagIsolation = "isolation"
flagCapAdd = "cap-add"
flagCapDrop = "cap-drop"
)

func validateAPIVersion(c swarm.ServiceSpec, serverAPIVersion string) error {
Expand Down
39 changes: 39 additions & 0 deletions cli/command/service/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -505,6 +505,7 @@ func updateService(ctx context.Context, apiClient client.NetworkAPIClient, flags
}

updateString(flagStopSignal, &cspec.StopSignal)
updateCapabilities(flags, cspec)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably should be checked if any changed to prevent churn;

Suggested change
updateCapabilities(flags, cspec)
if anyChanged(flags, flagCapAdd, flagCapDrop) {
updateCapabilities(flags, cspec)
}


return nil
}
Expand Down Expand Up @@ -1351,3 +1352,41 @@ func updateCredSpecConfig(flags *pflag.FlagSet, containerSpec *swarm.ContainerSp
containerSpec.Privileges.CredentialSpec = credSpec
}
}

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
if flags.Changed(flagCapAdd) {
caps := flags.Lookup(flagCapAdd).Value.(*opts.ListOpts).GetAll()
capAdd = append(capAdd, caps...)

dropToRemove = buildToRemoveSet(flags, flagCapAdd)
}

// 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)
}

// 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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this is inconsistent with how docker run handles the combination of add/drop, where add takes precedence;

# default capabilities
docker run -it --rm busybox sh -c 'touch foo && chown 123:456 foo && echo succesfully chowned'
# succesfully chowned

# dropping CAP_CHOWN
docker run -it --rm --cap-drop CAP_CHOWN busybox sh -c 'touch foo && chown 123:456 foo && echo succesfully chowned'
# chown: foo: Operation not permitted

# dropping *and* adding CAP_CHOWN
docker run -it --rm --cap-add CAP_CHOWN --cap-drop CAP_CHOWN busybox sh -c 'touch foo && chown 123:456 foo && echo succesfully chowned'
# succesfully chowned

note in docker run / docker create, duplicates are not removed (we should fix that for consistency)

docker create --name=test --cap-add CAP_CHOWN --cap-add CAP_CHOWN --cap-drop CAP_CHOWN --cap-drop CAP_CHOWN busybox
docker inspect --format 'CapAdd: {{json .HostConfig.CapAdd}} CapDrop: {{json .HostConfig.CapDrop}}' test
# CapAdd: ["CAP_CHOWN","CAP_CHOWN"] CapDrop: ["CAP_CHOWN","CAP_CHOWN"]

Unfortunately, this is all rather involved, but what I think should happen is that;

  • both existing (service spec) and new (values passed through flags or in the compose-file) should be 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 should be 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 should be 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 should be handled by the client. Validation should delegated to the daemon/server where possible.

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.

containerSpec.CapabilityAdd = append(containerSpec.CapabilityAdd, cap)
}
}

// 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)
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may have to sort CapabilityAdd and CapabilityDrop. Order of these should not matter, and if capabilities are updated and end up in a different order, this will likely cause service-tasks to be updated/re-deployed, causing churn where it's not needed.

(Possibly the same will be needed for the docker stack deploy code-paths)

For swarm services, we generally want the CLI/client to generate the spec and handle that spec with no (or very little) modifications on the server-side. The SwarmKit reconciliation loop will look for any change in the Service Spec to consider if a service needs updating or not.

Currently, it the CLI does not do any normalization or sorting, which will mean that;

  • duplicate values can occur
  • capabilities are not sorted; order of capabilities in the list does not matter for the container itself, but a change in order of capabilities will result in all tasks for a service to be re-deployed (in the docker service update case), causing unneeded churn in the service
  • capabilities are included without normalization, so both CAP_CHOWN and CHOWN can be added, and preserves case (both cap_chown, CAP_CHOWN and cAp_cHoWn are accepted)

To prevent unneeded re-creation of tasks I think we should both normalize capabilities, remove duplicates, and sort them alphabetically.

Possibly we could re-use NormalizeLegacyCapabilities, but that may need some work, as I don't think it should validate on the client side, if possible, and it's currently not de-duplicating. A similar implementation could be done on the client side though

Note: this is something we should fix for docker run as well (but that's something to handle separately).

I noticed this when trying using the following;

docker service create --detach --tty --cap-drop cAp_cHoWn --cap-drop CAP_CHOWN --cap-drop ChOwN --cap-drop CHOWN --name foo busybox
docker service inspect --format '{{json .Spec.TaskTemplate.ContainerSpec.CapabilityDrop }}' foo | jq .
[
  "cAp_cHoWn",
  "CAP_CHOWN",
  "ChOwN",
  "CHOWN"
]

71 changes: 71 additions & 0 deletions cli/command/service/update_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1310,3 +1310,74 @@ func TestUpdateCredSpec(t *testing.T) {
})
}
}

func TestUpdateCaps(t *testing.T) {
tests := []struct {
// name is the name of the testcase
name string
// flagAdd is the value passed to --cap-add
flagAdd []string
// flagDrop is the value passed to --cap-drop
flagDrop []string
// spec is the original ContainerSpec, before being updated
spec *swarm.ContainerSpec
// expectedAdd is the set of requested caps the ContainerSpec should have once updated
expectedAdd []string
// expectedDrop is the set of dropped caps the ContainerSpec should have once updated
expectedDrop []string
}{
{
name: "Add new caps",
flagAdd: []string{"NET_ADMIN"},
flagDrop: []string{},
spec: &swarm.ContainerSpec{},
expectedAdd: []string{"NET_ADMIN"},
expectedDrop: []string{},
},
{
name: "Drop new caps",
flagAdd: []string{},
flagDrop: []string{"CAP_MKNOD"},
spec: &swarm.ContainerSpec{},
expectedAdd: []string{},
expectedDrop: []string{"CAP_MKNOD"},
},
{
name: "Add a previously dropped cap",
flagAdd: []string{"NET_ADMIN"},
flagDrop: []string{},
spec: &swarm.ContainerSpec{
CapabilityDrop: []string{"NET_ADMIN"},
},
expectedAdd: []string{"NET_ADMIN"},
expectedDrop: []string{},
},
{
name: "Drop a previously requested cap",
flagAdd: []string{},
flagDrop: []string{"CAP_MKNOD"},
spec: &swarm.ContainerSpec{
CapabilityAdd: []string{"CAP_MKNOD"},
},
expectedAdd: []string{},
expectedDrop: []string{"CAP_MKNOD"},
},
}

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 _, cap := range tc.flagDrop {
flags.Set(flagCapDrop, cap)
}

updateCapabilities(flags, tc.spec)

assert.DeepEqual(t, tc.spec.CapabilityAdd, tc.expectedAdd)
assert.DeepEqual(t, tc.spec.CapabilityDrop, tc.expectedDrop)
})
}
}
2 changes: 2 additions & 0 deletions cli/compose/convert/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,8 @@ func Service(
Isolation: container.Isolation(service.Isolation),
Init: service.Init,
Sysctls: service.Sysctls,
CapabilityAdd: service.CapAdd,
CapabilityDrop: service.CapDrop,
},
LogDriver: logDriver,
Resources: resources,
Expand Down
26 changes: 26 additions & 0 deletions cli/compose/convert/service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -623,3 +623,29 @@ func TestConvertUpdateConfigParallelism(t *testing.T) {
})
assert.Check(t, is.Equal(parallel, updateConfig.Parallelism))
}

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",
},
CapDrop: []string{
"CHOWN",
"DAC_OVERRIDE",
"CAP_FSETID",
"CAP_FOWNER",
},
}
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))
}
2 changes: 0 additions & 2 deletions cli/compose/types/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@ import (
// UnsupportedProperties not yet supported by this implementation of the compose file
var UnsupportedProperties = []string{
"build",
"cap_add",
"cap_drop",
"cgroupns_mode",
"cgroup_parent",
"devices",
Expand Down