Skip to content

Replace capabilities api#2965

Merged
dperny merged 2 commits intomoby:masterfrom
cpuguy83:replace_capabilities_api
Jul 21, 2020
Merged

Replace capabilities api#2965
dperny merged 2 commits intomoby:masterfrom
cpuguy83:replace_capabilities_api

Conversation

@cpuguy83
Copy link
Member

@cpuguy83 cpuguy83 commented Jul 16, 2020

After extended discussion with moby/moby maintainers, we feel the existing API (not yet released) for supplying a capabilities list is not very useful since it requires clients to know the full list of capabilities, which usually isn't known... so instead we fall back to a default, which is already defined on the engine, but because the client needs to specify the capabilities, the client also has it's own default list.

With the old method, the full cap list is also encoded in the service spec which means the default cannot be changed except by the client.

If the client still wants to define the full list themselves they can --cap-drop ALL -cap-add CAP_FOO

  • This reverts commit 18d9170
  • Replaces 18d9170 with add/drop lists which get passed to the agent to deal with.

@cpuguy83
Revert "Add capabilities list to container specification"
8c7b14d 
This reverts commit 18d9170.

This is reverted to replace with a different model since a full cap list
specified on clients means the client must specify the base cap spec for
nodes it may not know about.

Instead we intend to split this into add/drop lists.
@cpuguy83
Copy link
Member Author

cpuguy83 commented Jul 16, 2020

ping @dperny @thaJeztah @tonistiigi

@cpuguy83
Copy link
Member Author

cpuguy83 commented Jul 16, 2020

We'd like to get this in for the upcoming docker release.

@cpuguy83 cpuguy83 force-pushed the replace_capabilities_api branch 2 times, most recently from 6bf5542 to e4e95c8 Compare July 16, 2020 23:44
@thaJeztah
Copy link
Member

thaJeztah commented Jul 17, 2020

Looks like protos have to be regenerated;

👹 please run 'make generate' when making changes to proto files

@cpuguy83
Copy link
Member Author

cpuguy83 commented Jul 17, 2020

Grr... I definitely ran this...

@thaJeztah
Copy link
Member

thaJeztah commented Jul 17, 2020

Perhaps CI is using a different version 😞

@cpuguy83 cpuguy83 force-pushed the replace_capabilities_api branch 2 times, most recently from 5e6d7d5 to 5ba713b Compare July 17, 2020 16:15
@dperny
Copy link
Collaborator

dperny commented Jul 17, 2020

I'm in a meeting right now so I can't do a full review, but I will do a full review in about 2 hours. In the meantime:

--- FAIL: TestCapabilityAdd (0.00s)

    container_test.go:276: expected [CAP_NET_RAW CAP_SYS_CHROOT], got [CAP_NET_RAW CAP_SYS_CHROOT]

--- FAIL: TestCapabilityDrop (0.00s)

    container_test.go:296: expected [CAP_KILL], got [CAP_KILL]

image

@cpuguy83
Copy link
Member Author

cpuguy83 commented Jul 17, 2020

Yes, this is pretty funny.
I saw it locally when running on my native go env... but ran again from the make shell container and it worked. 🤔

@cpuguy83
Copy link
Member Author

cpuguy83 commented Jul 17, 2020

Also of note for UX... removing ALL from a cap-drop in an service update CLI is non-trivial.

--rm-cap-drop ALL or something like that...

@cpuguy83 cpuguy83 mentioned this pull request Jul 17, 2020
Closed
@dperny
Copy link
Collaborator

dperny commented Jul 17, 2020

This looks good to me. It will of course need changes to the executor in the engine, but those should be easy.

For fixing the tests, the code in agent/exec/dockerapi/container.go can be removed. Just yank the whole test, if you'd rather not waste time fixing it. The only reason that code exists is so that swarmkit can run stand-alone against a docker engine, which isn't a supported or even fully functional state.

You'll need to change code in the github.com/docker/docker/daemon/cluster/executor/container package, or thereabouts, to make this work once merged into the engine.

@cpuguy83
Add support cap add/drop to services.
06a0d2d 
This allows clients to specify capabilities to add or drop form the
default capability.

Signed-off-by: Brian Goff <[email protected]>
@cpuguy83 cpuguy83 force-pushed the replace_capabilities_api branch from 5ba713b to 06a0d2d Compare July 17, 2020 22:43
@cpuguy83
Copy link
Member Author

cpuguy83 commented Jul 17, 2020

Fixed it. Just for some reason the docker type uses strslice.Strslice instead of a straight up []string... which doesn't make sense at all, but... 🤷

@codecov
Copy link

codecov bot commented Jul 17, 2020

Codecov Report

Merging #2965 into master will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2965      +/-   ##
==========================================
+ Coverage   61.74%   61.79%   +0.05%     
==========================================
  Files         142      142              
  Lines       22998    22999       +1     
==========================================
+ Hits        14199    14212      +13     
+ Misses       7298     7285      -13     
- Partials     1501     1502       +1

thaJeztah
thaJeztah approved these changes Jul 21, 2020
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@dperny PTAL

@dperny dperny merged commit 035d564 into moby:master Jul 21, 2020
@dperny
Copy link
Collaborator

dperny commented Jul 21, 2020

merged

@thaJeztah
Copy link
Member

thaJeztah commented Jul 21, 2020

Thanks!

@cpuguy83 cpuguy83 deleted the replace_capabilities_api branch July 21, 2020 16:17
@thaJeztah thaJeztah mentioned this pull request Jul 26, 2020
Merged
akerouanton added a commit to akerouanton/swarmkit that referenced this pull request Jul 26, 2020
@akerouanton
Fix bad comment on capability_drop field in protobuf def
264af21 
This is a follow-up of moby#2965.

Signed-off-by: Albin Kerouanton <[email protected]>
@akerouanton akerouanton mentioned this pull request Jul 26, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tonistiigi tonistiigi tonistiigi approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

+1 more reviewer

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@cpuguy83 @thaJeztah @dperny @tonistiigi @AkihiroSuda