docker: add capabilities and privileged support #11962
Conversation
There was a problem hiding this comment.
Took a very quick look, the code looks good.
Some comments only, some for duscussion,
- Please add a test ins test/verify/check-docker, (ask on IRC for help with that if needed)
- Remove the signoff line in the commit message pls, we don't use it in this project
- I am ok with the checkboxes, "Add ALL" "Drop ALL" but what's the default state? Some capabilities are added?
artur-rs
force-pushed
the
docker-caps-priv
branch
2 times, most recently
from
53bb050 to
1bf5a51
Compare
|
@artur-rs this still needs a test in test/veify/check-docker in order to get merged. Are you fine to work on it? |
|
@KKoukiou yes, I have tests ready, but the test architecture were a little problematic, so the tests weren't fully verified locally. I can push the code though.
Yes, by default some of the capabilities are added. Full list can be found here: url |
artur-rs
force-pushed
the
docker-caps-priv
branch
from
1391d06 to
f3dd3cd
Compare
|
@KKoukiou rebased and resolved merge conflicts |
|
Needs to rebase to master since #12367 changed tests names |
Added options for running container with: * all Linux capabilities added/dropped * add/drop specific Linux capabilities (multiple) * privileged mode
artur-rs
force-pushed
the
docker-caps-priv
branch
from
f3dd3cd to
fb2fa1e
Compare
There was a problem hiding this comment.
This looks really nice! :)
I would like to hear what does @andreasn think about the design, but I think it fits very nicely.
I have noticed that when I tab through the selections it jumps "randomly" (left section first then right and from left to right - this should be fixable very easily with using display:flex; justify-context:space-between)
Also the tests do not run. Have you been able to run them locally and debug it or can I help you with it?
Lastly can you please squash the commits into one?
I didn't check the code fully right now, but seems rather nice. Lets fix what I mentioned above first, thanks!
|
|
||
| # add ALL capabilities and drop specific one | ||
| b.click("#add-all-capabilities") | ||
| b.click("claim-capabilities") |
There was a problem hiding this comment.
Have you figured out how to run tests? This needs # as it is id. I havn't run anymore tests.
There was a problem hiding this comment.
Great start, but it seems the UI has some weird states that I think we can fix to make it easier to operate.
- Enable seems to be mutually exclusive with everything else. It cancels out all other actions, and removes any other rules. What's the opposite of this state? Is it more of a "bring-out-the-big-hammer" vs. select what capabilities you want? If so, it needs some improved labels and I probably a radio button between this mode and the pick-your-priviledges mode. It would be especially good if it could recall the state of the other mode, so if I switch between them, I don't throw away all the work I put into the fine grained rules.
- Add ALL and Drop ALL seems mutually exclusive, so should probably use radio buttons to select those modes instead of checkboxes.
- How is "Choose specific Linux capabilities" related to Add/Drop ALL? Are the labels below included in the list of ALL?
- The "Choose specific Linux capabilities" ends up in a weird state if I remove the last item in the list. I have to uncheck and then check the checkbox for "Choose specific Linux capabilities" to get the list back. Could be solved by not allowing to delete the last item, if there is only one item in the list. Seems like the other controls works like this as well though, so not as critical.
|
@artur-rs ping, are you block on something that I may help you with? |
|
We announced that we are deprecating cockpit-docker, so this is obsolete. Moving the underlying issue into cockpit-podman. |
Added options for running container with:
Implements https://github.com/cockpit-project/cockpit/issues/10637