[carry 2663] Add capabilities support to stack/service commands by thaJeztah · Pull Request #2687 · docker/cli

thaJeztah · 2020-08-25T11:54:46Z

carries #2663

closes #1940
closes #2199
closes #2663

- What I did

This PR supersedes #1940 and #2199. I removed the support of privileged option from the original PR as I believe privileged containers involve more than just having the complete set of capabilities (eg. no user namespace, the unconfined AppArmor profile is used, etc...).

docker stack deploy now supports cap_add and cap_drop options ;
docker service create|update now support --cap-add and --cap-drop flags. For the update command and unlike most of other options, those flags don't have -add & -rm variants as this would make a poor UX (eg. --cap-add-add & --cap-drop-rm). Instead, updateCapabilities takes care of stripping caps provided to --cap-add from CapabilityDrop (and vice versa) ;
docker service inspect --pretty displays the list of added caps and dropped caps ;

- How I did it

The service converter transforming compose types into API types has been updated to add capabilities to ServiceSpec;
A new function with the logic described above and named updateCapabilities() has been introduced and is called by updateService ;
The pretty formatter has been updated to include added and dropped caps ;

- How to verify it

docker stack deploy

$ cat docker-compose.yaml
version: "3.7"
services:
  test:
    image: ollijanatuinen/capsh
    cap_add:
      - NET_ADMIN
    cap_drop:
      - CAP_MKNOD
$ build/docker stack deploy --compose-file docker-compose.yaml t1
$ docker inspect t1_test.1.m0jmdufuo93fotyyrhm00xx16
        "CapAdd": [
            "CAP_NET_ADMIN"
        ],
        "CapDrop": [
            "CAP_MKNOD"
        ],

docker service create

$ build/docker service create --name test --cap-add NET_ADMIN --cap-drop CAP_MKNOD ollijanatuinen/capsh
$ docker inspect test.1.li7llrgyp8v4mfvo5tvimhzxt
        "CapAdd": [
            "CAP_NET_ADMIN"
        ],
        "CapDrop": [
            "CAP_MKNOD"
        ],

docker service update

$ build/docker service update --cap-add CAP_MKNOD t1_test
$ docker inspect t1_test.1.92sd9omniksbenuv6wm17ft78
        "CapAdd": [
            "CAP_NET_ADMIN",
            "CAP_MKNOD"
        ],
        "CapDrop": null,

$ build/docker service update --cap-drop CAP_MKNOD --cap-drop NET_ADMIN t1_test
        "CapAdd": null,
        "CapDrop": [
            "CAP_MKNOD",
            "CAP_NET_ADMIN"
        ],

docker service inspect

$ build/docker service inspect t1_test
        "CapabilityAdd": [
            "CAP_NET_ADMIN"
        ],
        "CapabilityDrop": [
            "CAP_MKNOD"
        ]
$ build/docker service inspect --pretty t1_test
Capabilities:
 Add: CAP_NET_ADMIN
 Drop: CAP_MKNOD

- Description for the changelog

Add cap_add and cap_drop support to docker stack deploy and --cap-add and --cap-drop flags to docker service create|update.

- A picture of a cute animal (not mandatory but encouraged)

codecov-commenter · 2020-08-25T12:35:19Z

Codecov Report

Merging #2687 into master will increase coverage by 0.11%.
The diff coverage is 88.23%.

@@            Coverage Diff             @@
##           master    #2687      +/-   ##
==========================================
+ Coverage   58.42%   58.54%   +0.11%     
==========================================
  Files         295      296       +1     
  Lines       21201    21286      +85     
==========================================
+ Hits        12387    12462      +75     
- Misses       7906     7915       +9     
- Partials      908      909       +1

thaJeztah · 2020-08-25T17:06:42Z

Ok, so I'm a bit in doubt what would be more logical; when updating a service that had previous cap-add/cap-drops, should we do:

A. Update both "previous" and "new" state at once

Given the following service;

CapDrop	CapAdd
CAP_SOME_CAP

When updating the service, and applying --cap-add CAP_SOME_CAP, the previously dropped capability is removed, and CAP_SOME_CAP is added:

CapDrop	CapAdd
	CAP_SOME_CAP

Downside of this is that there's no option to reset the cap-add/cap-drop through docker service update (note that this would be possible through docker stack up as it's "declarative", and doesn't take the previous state into account).

B. Update as a "tri-state" (for better words)

Given the following service;

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

So upside is that it's possible to "reset" previous capabilities without adding new ones. Downside is that it requires multiple updates if that is the desired result (given, probably a rare condition)

C. Introduce a special "RESET" value

Given the following service;

CapDrop	CapAdd
CAP_SOME_CAP

When updating the service, and applying --cap-drop RESET:

CapDrop	CapAdd

When updating the service, and applying --cap-drop RESET, combined with --cap-add CAP_SOME_CAP and --cap-drop CAP_SOME_OTHER_CAP:

CapDrop	CapAdd
CAP_FOO_CAP	CAP_SOME_CAP

D. Combine B + C

Apply the diff to the existing list of capabilities first, but also allow using RESET to "start from scratch"

thaJeztah · 2020-08-27T11:08:45Z

As to my comment above, I added two commits that implement B and C

thaJeztah · 2020-08-27T11:12:50Z

contrib/completion/bash/docker

 		NET_RAW
+		RESET
 		SETFCAP


I noticed that our completion scripts use the capabilities without the CAP_ prefix; I'm on the fence what we should use as "normalised" format: with, or without the CAP_. From a UX perspective, possibly without is more friendly (less to type, and less characters to type to use auto-completion)

@tiborvass @silvin-lubecki WDYT?

Perhaps it doesn't make a big difference (as the values entered by the user will be normalised anyway, so the only difference will be in what's shown in docker service inspect. Looks like that's currently consistent with (e.g.) what's documented for docker plugin capabilities.

I'm totally fine with values without the CAP_ 👍

When completing signals, we offer both SIGXXX and XXX.
So for consistency, we could switch to supporting both variants here as well.
I'll create a PR for that.

Created #2706

silvin-lubecki · 2020-09-01T09:54:04Z

cli/command/service/update.go

+// 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 the a list has


typo: the/a list

silvin-lubecki · 2020-09-02T14:40:53Z

cli/command/service/update_test.go

+			spec: &swarm.ContainerSpec{
+				CapabilityDrop: []string{"ALL", "CAP_MOUNT", "CAP_NET_ADMIN"},
+			},
+			expectedDrop: []string{"ALL"},


nit: should we also test ALL in cap-add AND cap-drop?

silvin-lubecki · 2020-09-02T14:47:12Z

cli/command/service/update_test.go

+			flagDrop: []string{},
+			spec: &swarm.ContainerSpec{
+				CapabilityDrop: []string{"CAP_NET_ADMIN"},
+			},


nit: maybe add explicit empty expectedDrop?

Yeah, guess it makes it slightly more understandable; I'll add explicit expectedDrop / expectedAdd lines

silvin-lubecki · 2020-09-02T14:50:36Z

cli/command/service/update_test.go

+		},
+		{
+			name:     "Reset capabilities, and update after",
+			flagAdd:  []string{"RESET", "CAP_ADD_ONE", "CAP_FOO"},


nit: test also with "RESET", "ALL" ?

thaJeztah · 2020-09-08T12:47:32Z

even if I'm a bit on the fence with the UX (RESET looks weird to me, but I can't find a better way), so let's move with that 👍

I can move the last commit to a separate PR; I don't think it's critical to have it, and we can add it after that, so if it requires more discussion, we can do so separately

thaJeztah · 2020-09-08T12:52:37Z

@silvin-lubecki @albers updated this one, and moved the RESET commit to #2709

silvin-lubecki

LGTM

thaJeztah · 2020-09-08T15:59:44Z

@tiborvass @cpuguy83 PTAL

cpuguy83

LGTM

kaysond · 2020-09-27T06:53:36Z

I've been waiting for this! Is it going to be in 19.03 or a newer version? Any idea when it will get released?

thaJeztah · 2020-09-27T18:28:59Z

this will be in the upcoming 20.xx release

robertnisipeanu · 2020-12-06T11:45:36Z

Isn't this in the master branch? I tried installing docker-ce docker-ce-cli and containerd.io from the nightly build, however I still get "Ignoring unsupported options: cap_add"

thaJeztah · 2020-12-07T14:07:06Z

I think there's an issue with the nightly builds currently not being updated; this should be in the docker v20.10 release candidates in the testing channel (GA should be released soon)

liangyuanpeng · 2020-12-08T07:16:30Z

Look forward it!

georgmay · 2020-12-16T03:14:09Z

I'm getting the same "Ignoring unsupported options: cap_add" on 20.10

trajano · 2020-12-17T23:02:28Z

It does not work on Docker Desktop for Windows 3.0. I tried

  haveged:
    image: hortonworks/haveged:1.1.0
    deploy:
      restart_policy:
        max_attempts: 3
    cap_add:
      - SYS_ADMIN
      - MKNOD
      - DAC_OVERRIDE
      - NET_ADMIN
      - SYS_MODULE
      - SYS_PACCT

I don't get the data in

docker service inspect api_haveged --pretty

ID:             bcjakjq6i1xfuwdw729eix5sh
Name:           api_haveged
Labels:
 com.docker.stack.image=hortonworks/haveged:1.1.0
 com.docker.stack.namespace=api
Service Mode:   Replicated
 Replicas:      1
Placement:
UpdateConfig:
 Parallelism:   1
 On failure:    pause
 Monitoring Period: 5s
 Max failure ratio: 0
 Update order:      stop-first
RollbackConfig:
 Parallelism:   1
 On failure:    pause
 Monitoring Period: 5s
 Max failure ratio: 0
 Rollback order:    stop-first
ContainerSpec:
 Image:         hortonworks/haveged:1.1.0
Resources:
Networks: api_default
Endpoint Mode:  vip

This is in regards to https://stackoverflow.com/questions/65241151/running-haveged-in-docker-swarm

thaJeztah added impact/changelog status/2-code-review area/swarm impact/documentation area/stack impact/compose-file labels Aug 25, 2020

thaJeztah force-pushed the carry_service_caps branch 3 times, most recently from 23e9121 to 4410116 Compare August 25, 2020 12:28

thaJeztah added the docs/revisit label Aug 25, 2020

thaJeztah force-pushed the carry_service_caps branch 2 times, most recently from 31cd380 to e336b77 Compare August 26, 2020 17:36

This comment has been minimized.

Sign in to view

thaJeztah force-pushed the carry_service_caps branch 4 times, most recently from ce622d6 to d85589d Compare August 27, 2020 11:06

thaJeztah marked this pull request as ready for review August 27, 2020 11:07

thaJeztah requested a review from albers as a code owner August 27, 2020 11:07

thaJeztah commented Aug 27, 2020

View reviewed changes

thaJeztah added this to the 20.03.0 milestone Aug 27, 2020

thaJeztah force-pushed the carry_service_caps branch from d85589d to 10b529e Compare August 27, 2020 17:43

silvin-lubecki reviewed Sep 1, 2020

View reviewed changes

cpuguy83 mentioned this pull request Sep 2, 2020

docker swarm does not support '--cap-add' and '--cap-drop' moby/moby#41408

Closed

silvin-lubecki reviewed Sep 2, 2020

View reviewed changes

thaJeztah force-pushed the carry_service_caps branch from 10b529e to 9503729 Compare September 8, 2020 12:50

thaJeztah mentioned this pull request Sep 8, 2020

Service cap-add/cap-drop: add special "RESET" value #2709

Merged

silvin-lubecki approved these changes Sep 8, 2020

View reviewed changes

albers approved these changes Sep 8, 2020

View reviewed changes

cpuguy83 approved these changes Sep 8, 2020

View reviewed changes

cpuguy83 merged commit 1648029 into docker:master Sep 8, 2020

thaJeztah deleted the carry_service_caps branch September 8, 2020 20:02

thaJeztah mentioned this pull request Sep 11, 2020

[epic] add more options to service create / service update moby/moby#25303

Open

olljanat mentioned this pull request Sep 14, 2020

Adding the ability to configure default capabilities moby/moby#39297

Open

AnandChowdhary mentioned this pull request Oct 8, 2020

Netdata in swarm mode caprover/caprover#854

Open

Aniket-Singla mentioned this pull request Nov 2, 2020

docker swarm - no need to access host possible without cap_add? kylemanna/docker-openvpn#595

Open

clement-z mentioned this pull request Nov 26, 2020

RTNETLINK answers: Operation not permitted / Cannot ioctl TUNSETIFF tun haugene/docker-transmission-openvpn#1541

Closed

This was referenced Dec 18, 2020

Unable to get capabilities to work in Docker swarm. docker/for-win#9853

Closed

Unable to get capabilities working in Docker swarm under Windows WSL2 #2893

Closed

matusnovak mentioned this pull request Apr 28, 2021

Add capability add/drop option for creating and updating services docker/docker-py#2831

Closed

This was referenced May 8, 2021

App Suggestion: OpenVPN-AS caprover/one-click-apps#219

Closed

Add support for --cap-add arguments caprover/caprover#1100

Closed

webash mentioned this pull request Aug 30, 2022

cap_add and cap_drop have had support added, but docs still say no docker/docs#15522

Closed

trajano mentioned this pull request Sep 29, 2022

Docker integration: Exposing Docker socket to Traefik container is a serious security risk traefik/traefik#4174

Closed

thaJeztah mentioned this pull request Apr 3, 2024

Proposal: decompose --privileged into individual --security-opt options moby/moby#47663

Open

Conversation

thaJeztah commented Aug 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Aug 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

thaJeztah commented Aug 25, 2020

A. Update both "previous" and "new" state at once

B. Update as a "tri-state" (for better words)

C. Introduce a special "RESET" value

D. Combine B + C

Uh oh!

This comment has been minimized.

thaJeztah commented Aug 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Sep 8, 2020

Uh oh!

thaJeztah commented Sep 8, 2020

Uh oh!

silvin-lubecki left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Sep 8, 2020

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

kaysond commented Sep 27, 2020

Uh oh!

thaJeztah commented Sep 27, 2020

Uh oh!

robertnisipeanu commented Dec 6, 2020

Uh oh!

thaJeztah commented Dec 7, 2020

Uh oh!

liangyuanpeng commented Dec 8, 2020

Uh oh!

georgmay commented Dec 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trajano commented Dec 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

thaJeztah commented Aug 25, 2020 •

edited

Loading

codecov-commenter commented Aug 25, 2020 •

edited

Loading

georgmay commented Dec 16, 2020 •

edited

Loading

trajano commented Dec 17, 2020 •

edited

Loading