client: ContainerCreate: normalize CapAdd, CapDrop capabilities#48551
client: ContainerCreate: normalize CapAdd, CapDrop capabilities#48551thaJeztah merged 1 commit intomoby:masterfrom
Conversation
thaJeztah
added
area/api
Before this change, capabilities would be sent un-normalized, un-sorted,
and could contain duplicates;
docker create --name foo --cap-add SYS_ADMIN --cap-add sys_admin --cap-add cap_sys_admin --cap-add ALL busybox
docker container inspect --format '{{json .HostConfig.CapAdd }}' foo
["SYS_ADMIN","sys_admin","cap_sys_admin","ALL"]
After this change, capabilities are sent in their normalized form, sorted,
and with duplicates removed;
docker create --name foo --cap-add SYS_ADMIN --cap-add sys_admin --cap-add cap_sys_admin --cap-add ALL busybox
docker container inspect --format '{{json .HostConfig.CapAdd }}' foo
["ALL", "CAP_SYS_ADMIN"]
Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
normalize_caps
branch
from
bffd405 to
5bdbc2f
Compare
| // It is similar to [github.com/docker/docker/oci/caps.NormalizeLegacyCapabilities], | ||
| // but performs no validation based on supported capabilities. | ||
| func normalizeCapabilities(caps []string) []string { |
There was a problem hiding this comment.
FWIW, I want to have a look if we can somehow unify these implementations, and put this code somewhere where it can be used. I didn't yet decide on the best place for this; also considering to put similar normalisation logic in the API endpoint (before we send it all to the daemon).
Pending that, I kept it as a non-exported function here.
|
|
||
| unique := make(map[string]struct{}) | ||
| for _, c := range caps { | ||
| c = normalizeCap(c) |
There was a problem hiding this comment.
would it make sense to move up the check for ALL capabilities from normalizeCap and just early return here?
Or is it a case of even with ALL, the rest should still be returned alongside it?
There was a problem hiding this comment.
Yeah, technically it would probably be fine to only keep ALL, but there's logic in the daemon to handle the ALL special case, and to hydrate the list of capabilities; see TweakCapabilities
Line 83 in 4001d07
I tried to avoid (at least for now) putting too much logic on the client side (other than normalising the format) and to preserve information where possible; also having situations in the back of my mind where a user could pass a capability in the list that's not (yet) known by the engine, and I'd want it to produce an error in that case instead of silently taking ALL.
austinvazquez
left a comment
austinvazquez
left a comment
There was a problem hiding this comment.
LGTM. Can always rehome later 👏🏼
laurazard
left a comment
laurazard
left a comment
There was a problem hiding this comment.
Looks nice! Appreciate the tests too :)
Wondering if we can put the whole implementation under something like container/capabilities.go or something of the sort.
Yes, something along those lines, or even a separate package there. I need to look how we can slice-and-dice the code (as there's some existing code that's exported), and make sure that refactoring doesn't pull in other (unwanted) dependencies. #48551 (comment) |
|
I'll bring this one in; thanks for review everyone! |
4 participants
Before this change, capabilities would be sent un-normalized, un-sorted, and could contain duplicates;
After this change, capabilities are sent in their normalized form, sorted, and with duplicates removed;
- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)