8000 Add support for exact list of capabilities + capAdd / capDrop refactor by olljanat · Pull Request #38380 · moby/moby · GitHub
[go: up one dir, main page]
Skip to content

Add support for exact list of capabilities + capAdd / capDrop refactor #38380

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

Jump to bottom
Merged
merged 1 commit into from
Jan 28, 2019
Merged

Add support for exact list of capabilities + capAdd / capDrop refactor #38380

merged 1 commit into from
Jan 28, 2019

Conversation

olljanat
Copy link
Contributor
@olljanat olljanat commented Dec 16, 2018 •
edited
Loading

- What I did
Added support for exact list of capabilities list of container capabilities which can be used instead of these CapAdd and CapDrop like discussed on #26849 (comment)

and refactored capAdd and capDrop to share as much code as possible with this new feature.

- How I did it
In cooperation with @thaJeztah 😉

- How to verify it
CI makes sure that existing functionality works like expected and new integration tests verify that new functionality works correctly.

- Description for the changelog

  • Add support for exact list of capabilities, support only OCI model
  • Support OCI model on CapAdd and CapDrop but remain backward compatibility
  • Create variable locally instead of declaring it at the top
  • Use const for magic "ALL" value
  • Rename cap variable as it overlaps with cap() built-in
  • Normalize and validate capabilities before use
  • Move validation for conflicting options to validateHostConfig()
  • TweakCapabilities: simplify logic to calculate capabilities

- Comments
First step to solve #25885 , #24862 and moby/swarmkit#1030
Replaces #26849
Dependency for moby/swarmkit#2795

This was referenced Dec 16, 2018
Merged
Closed
@olljanat olljanat force-pushed the capabilities-support branch from 59df37d to b6b0685 Compare December 16, 2018 17:51
@codecov
Copy link
codecov bot commented Dec 16, 2018 •
edited
Loading

Codecov Report

Merging #38380 into master will increase coverage by 0.01%.
The diff coverage is 16.66%.

@@            Coverage Diff             @@
##           master   #38380      +/-   ##
==========================================
+ Coverage    36.6%   36.61%   +0.01%     
==========================================
  Files         608      608              
  Lines       45224    45241      +17     
==========================================
+ Hits        16553    16567      +14     
+ Misses      26384    26383       -1     
- Partials     2287     2291       +4

@olljanat olljanat force-pushed the capabilities-support branch from b6b0685 to 9500e14 Compare December 16, 2018 19:14
@olljanat olljanat changed the title WIP: Add support for exact list of capabilities Add support for exact list of capabilities Dec 16, 2018
@olljanat
Copy link
Contributor Author

@thaJeztah looks working fine with moby/swarmkit#2795 so can you review and give your thoughts?

Docs updates are not included yet on purpose because I would get #37940 merged first and don't want cause conflicts with it.

thaJeztah
thaJeztah requested changes Dec 17, 2018
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.

Thanks for working on this! I left some comments inline.

@thaJeztah thaJeztah mentioned this pull request Dec 17, 2018
Closed
@olljanat oll 8000 janat force-pushed the capabilities-support branch 3 times, most recently from e5eda15 to 912512b Compare December 17, 2018 20:14
@olljanat
Copy link
Contributor Author
olljanat commented Dec 17, 2018 •
edited
Loading

@thaJeztah updated based on feedback 😎

EDIT: Also modified to API comment to use same format than on #38381

@olljanat olljanat force-pushed the capabilities-support branch 2 times, most recently from 6836f4e to 51ab0bc Compare December 19, 2018 22:11
@olljanat olljanat requested a review from vdemeester as a code owner December 19, 2018 22:11
@olljanat olljanat force-pushed the capabilities-support branch 6 times, most recently from ef14e5b to 9a77b03 Compare December 22, 2018 17:55
thaJeztah
thaJeztah requested changes Jan 22, 2019
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.

Thanks @olljanat!

I left comments inline, but was going through these changes and that the validation only take place on docker start (not on docker create).

I have some ideas, but probably faster to show those instead of trying to explain them; I'll work on a branch so that you can incorporate those changes in your PR if you agree with them 🤗

Will post a link once if made those changes 👍

@thaJeztah
Copy link
Member

Pushed a branch here; I split my changes into several commits, to make it easier to grasp what I did 😅 master...thaJeztah:capabilities_support_move_to_create

(or diff compared to your branch; olljanat/moby@capabilities-support...thaJeztah:capabilities_support_move_to_create)

@thaJeztah
Copy link
Member

oh, hold on, pushed one outdated commit; fixing

@thaJeztah
Copy link
Member

done 👍

@olljanat
Copy link
Contributor Author

I have some ideas, but probably faster to show those instead of trying to explain them; I'll work on a branch so that you can incorporate those changes in your PR if you agree with them

@thaJeztah those looks good on quick view. Will do some testing with them but I included them already so we can see result of CI.

@olljanat
Copy link
Contributor Author
olljanat commented Jan 22, 2019 •
edited
Loading

One "minor" thing. --cap-add or --cap-drop does not work anymore so CI should fail to it. Let's see. I will look if I find why it is so.

EDIT: Found it. Fixed on olljanat@d5404a7 but I will wait that CI finishes so we can see that it really finds those failures.

EDIT 2: Yes. Looks correct: https://jenkins.dockerproject.org/job/Docker-PRs/52704/console

@olljanat olljanat force-pushed the capabilities-support branch from fb46376 to 2fc46c0 Compare January 22, 2019 18:58
@olljanat
Copy link
Contributor Author
olljanat commented Jan 22, 2019 •
edited
Loading

@thaJeztah I'm not sure if that is correct way but I rebased this now to one commit which is signed off by both of us (have seen that on some old commits too).

thaJeztah
thaJeztah reviewed Jan 22, 2019
View reviewed changes
@thaJeztah
Copy link
Member

but I rebased this now to one commit which is signed off by both of us (have seen that on some old commits too).

That sounds ok 👍
(I don't need the credits for this, so no need to keep my commits separate)

@olljanat
Capabilities refactor
80d7bfd 
- Add support for exact list of capabilities, support only OCI model
- Support OCI model on CapAdd and CapDrop but remain backward compatibility
- Create variable locally instead of declaring it at the top
- Use const for magic "ALL" value
- Rename `cap` variable as it overlaps with `cap()` built-in
- Normalize and validate capabilities before use
- Move validation for conflicting options to validateHostConfig()
- TweakCapabilities: simplify logic to calculate capabilities

Signed-off-by: Olli Janatuinen <olli.janatuinen@gmail.com>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@olljanat olljanat force-pushed the capabilities-support branch from 2fc46c0 to 80d7bfd Compare January 22, 2019 19:51
@olljanat olljanat changed the title Add support for exact list of capabilities Add support for exact list of capabilities + capAdd / capDrop refactor Jan 22, 2019
@olljanat
Copy link
Contributor Author

LGTM, but I do think we should fix the issue that you can't say CAP_SYS_ADMIN you have to say SYS_ADMIN before we ship this, and accept either form as this won't break anything.

@justincormack now this is handled. PTAL

thaJeztah
thaJeztah approved these changes Jan 25, 2019
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 (saw I still had "changes requested")

@thaJeztah
Copy link
Member

(We can look at the client normalising the values in a follow-up (probably limiting to uppercasing values before sending them over the API))

@olljanat
Copy link
Contributor Author
9E88 olljanat commented Jan 25, 2019

@AkihiroSuda can you look this one again as it was heavily modified after your last review?

@thaJeztah
Copy link
Member

I think this should be ready to go; merging.

@AkihiroSuda let me know if there's follow-ups to do after this is merged 🤗

@thaJeztah thaJeztah merged commit 5801c04 into moby:master Jan 28, 2019
AkihiroSuda
AkihiroSuda reviewed Jan 28, 2019
View reviewed changes
Copy link
Member
@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

LGTM

@olljanat
Copy link
Contributor Author

@presidenten no. up to date status is visible on this message: #25885 (comment)

Target is get complete solution out part of 19.06.

6D38
@thaJeztah thaJeztah mentioned this pull request Apr 3, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

@vdemeester vdemeester Awaiting requested review from vdemeester

+1 more reviewer

@justincormack justincormack justincormack approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@crosbymichael crosbymichael

Labels
area/api API area/swarm impact/changelog status/1-design-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@olljanat @justincormack @thaJeztah @AkihiroSuda @crosbymichael @GordonTheTurtle
0