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.

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

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Jan 28, 2019

Conversation

@olljanat
Copy link
Contributor

commented Dec 16, 2018

- 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 docker/swarmkit#1030
Replaces #26849
Dependency for docker/swarmkit#2795

@codecov

This comment has been minimized.

Copy link

commented Dec 16, 2018

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 olljanat:capabilities-support branch from b6b0685 to 9500e14 Dec 16, 2018

@olljanat olljanat changed the title WIP: Add support for exact list of capabilities Add support for exact list of capabilities Dec 16, 2018

@olljanat

This comment has been minimized.

Copy link
Contributor Author

commented Dec 16, 2018

@thaJeztah looks working fine with docker/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
Copy link
Member

left a comment

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

oci/oci.go Outdated Show resolved Hide resolved
oci/caps/utils.go Outdated Show resolved Hide resolved
oci/caps/utils.go Outdated Show resolved Hide resolved
oci/caps/utils.go Outdated Show resolved Hide resolved
oci/oci.go Outdated Show resolved Hide resolved
api/types/container/host_config.go Outdated Show resolved Hide resolved

@olljanat olljanat force-pushed the olljanat:capabilities-support branch 3 times, most recently from e5eda15 to 912512b Dec 17, 2018

@olljanat

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2018

@thaJeztah updated based on feedback 馃槑

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

@olljanat olljanat force-pushed the olljanat:capabilities-support branch 2 times, most recently from 6836f4e to 51ab0bc Dec 18, 2018

@olljanat olljanat requested a review from vdemeester as a code owner Dec 19, 2018

@olljanat olljanat force-pushed the olljanat:capabilities-support branch 6 times, most recently from ef14e5b to 9a77b03 Dec 20, 2018

@derek derek bot added the rebuild/powerpc label Dec 22, 2018

@thaJeztah
Copy link
Member

left a comment

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 馃憤

integration/container/create_test.go Outdated Show resolved Hide resolved
integration/container/create_test.go Outdated Show resolved Hide resolved
integration/container/create_test.go Outdated Show resolved Hide resolved
oci/oci.go Show resolved Hide resolved
integration/container/create_test.go Outdated Show resolved Hide resolved
oci/caps/utils.go Outdated Show resolved Hide resolved
oci/caps/utils.go Outdated Show resolved Hide resolved
daemon/oci_windows.go Show resolved Hide resolved
daemon/oci_linux.go Outdated Show resolved Hide resolved
daemon/oci_windows.go Outdated Show resolved Hide resolved
@thaJeztah

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

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

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

oh, hold on, pushed one outdated commit; fixing

@thaJeztah

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

done 馃憤

@olljanat

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2019

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 olljanat:capabilities-support branch from fb46376 to 2fc46c0 Jan 22, 2019

@olljanat

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2019

@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).

oci/caps/utils.go Outdated Show resolved Hide resolved
@thaJeztah

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

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)

Capabilities refactor
- 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 olljanat:capabilities-support branch from 2fc46c0 to 80d7bfd Jan 22, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2019

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

left a comment

LGTM (saw I still had "changes requested")

@thaJeztah

This comment has been minimized.

Copy link
Member

commented Jan 25, 2019

(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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2019

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

@thaJeztah

This comment has been minimized.

Copy link
Member

commented Jan 28, 2019

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

8 of 9 checks passed

codecov/patch 16.66% of diff hit (target 50%)
Details
codecov/project 36.61% (+0.01%) compared to 3449b12
Details
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 43886 has succeeded
Details
janky Jenkins build Docker-PRs 52707 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 13123 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 23856 has succeeded
Details
windowsRS5-process Jenkins build Docker-PRs-WoW-RS5-Process 1266 has succeeded
Details
z Jenkins build Docker-PRs-s390x 13011 has succeeded
Details
@AkihiroSuda
Copy link
Member

left a comment

LGTM

@olljanat

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2019

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

Target is get complete solution out part of 19.06.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.