-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for Docker SecurityOpt (allowing GDB Debugging on unprivileged docker containers) #6856
Add support for Docker SecurityOpt (allowing GDB Debugging on unprivileged docker containers) #6856
Conversation
Build # 4255 - FAILED Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/4255/ to view the results. |
@hkolvenbach could you please run the command
jenkins CI job is failing to invalid format in your modified files. |
@benoitf sorry, probably still formatted with the old version. should be fixed now |
… configuration variable Signed-off-by: [email protected] <[email protected]>
Signed-off-by: Hanno Kolvenbach <[email protected]>
@hkolvenbach yes thanks. |
d40f376
to
33f6079
Compare
Signed-off-by: Hanno Kolvenbach <[email protected]>
33f6079
to
6ef729e
Compare
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/4267/ |
Any volunteers to port to che6? @hkolvenbach @tolusha ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice PR!
You used properties injection correctly.
Can you take a look at my inlined comments?
dockerfiles/init/manifests/che.env
Outdated
# Host SecurityOpt Parameters | ||
# This parameter allows to specify text values for the docker "--security-opt" parameter | ||
# or the HostConfig{SecurityOpt:[]} value of the docker machine API | ||
# (https://docs.docker.com/engine/api/v1.33/#operation/ContainerCreate) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use 1.21 API by default for compatibility with older docker versions. Can you point to that version instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
# This parameter allows to specify custom security options for the created docker container | ||
# seccomp:unconfined is the default for kubernetes, but not for docker. This is needed | ||
# for debugging with gdbserver | ||
che.docker.securityopt=seccomp:unconfined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to change default settings or just allow to change it? @hkolvenbach @tolusha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say yes to behave in the same way as the kubernetes / openshift implementation does. Any comments on this @tolusha?
# This parameter allows to specify custom security options for the created docker container | ||
# seccomp:unconfined is the default for kubernetes, but not for docker. This is needed | ||
# for debugging with gdbserver | ||
che.docker.securityopt=seccomp:unconfined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried it on systems that do not support seccomp? Is it just ignored?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@garagatyi didn't try it yet, any specific system I should test with?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know where to test it and whether it is worth checking, just thought how it can influence users.
I tried to put an invalid value of seccomp profile and another security option. Both failed. So I assume that this would prevent Che starting workspaces if users host doesn't support seccomp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the feedback, i will test with different values for securityopt and see how it behaves. If unsupported values fail, i will leave it empty as default
@@ -333,14 +333,18 @@ public ContainerConfig withExposedPorts(Map<String, Map<String, String>> exposed | |||
return this; | |||
} | |||
|
|||
// there is no "SecurityOpts" in the docker specification, only HostConfig{"SecurityOpt" .[]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, use javadocs comments style to provide documentation, e.g.
/*
* There is no "SecurityOpts" in the docker specification, only HostConfig{"SecurityOpt" .[]}
*/
@@ -161,6 +161,11 @@ che.docker.always_pull_image=true | |||
# to be able to launch their own runtimes which are embedded Docker containers. | |||
che.docker.privileged=false | |||
|
|||
# This parameter allows to specify custom security options for the created docker container | |||
# seccomp:unconfined is the default for kubernetes, but not for docker. This is needed | |||
# for debugging with gdbserver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it is supposed to be injected as an array can you add comment that values are separated by commas
# This parameter allows to specify custom security options for the created docker container | ||
# seccomp:unconfined is the default for kubernetes, but not for docker. This is needed | ||
# for debugging with gdbserver | ||
che.docker.securityopt=seccomp:unconfined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate on how to set this option to default docker behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@garagatyi default would be an empty parameter
dockerfiles/init/manifests/che.env
Outdated
# This is useful if you want to run e.g. Cpp debugging within the container through gdbserver | ||
# See https://github.com/eclipse/che/issues/4284 for details. This is only necessary for docker, | ||
# openshift seems to set "seccomp:unconfined" by default | ||
#CHE_DOCKER_SECURITYOPT="seccomp:unconfined" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that quotes should not be used here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Thanks for the remarks, I will fix them. Regarding the Che6 port: I can look into it, but not sure if I will manage to do it this week. |
Signed-off-by: Hanno Kolvenbach <[email protected]>
bb7a7d3
to
93ff4d4
Compare
@hkolvenbach Che6 has this area massively refactored. If you need any help you can contact me |
@hkolvenbach I think that in case CHE_DOCKER_SECURITYOPT is set to empty value, e.g.:
the current default behavior will be used, right? |
@garagatyi thanks for the support, i will come back to it when i am working on the che6 port. i did a few more tests and can confirm that docker will definitely fail if any unknown value is provided. so i will change it do not provide anything on default. null or empty string seems to work though (at least on linux). i will also check if an empty string will result in the default behavior. |
@garagatyi i just confirmed that container creation will fail with an empty string, only null is allowed. do i understand correctly, that i need to implement a |
I think that unconfined might be OK as a default value. I just wanted to mention it to hear a feedback from the community if any. Wrong values in other places of che.env would definitely brake workspaces start also, so it is not the case. |
@garagatyi @eivantsov from my side I would consider my implementation for che 5.x ready. I have tested it with empty values, single values and multiple values for i will now start to look into the che 6 port. |
@hkolvenbach take a look on direct usage of provider in tests garagatyi@2685f22 |
@garagatyi thanks, i integrated your changes and adapted everything to use CHE_DOCKER_SECURITYOPT again. somehow your commit doesn't pass validation, should I squash and sign it? |
@garagatyi I am a bit lost in the Che6 source code. Could you point me to the file where the configuration for the docker container is generated? I hope i can take it from there |
@hkolvenbach yes, you can squash my commit. I've signed Eclipse agreement so there are no legal issues with that. |
Just checked gdb on OpenShift. I was able to start gdb and debug a binary using command line. |
Great! @eivantsov maybe we should change defaults to that behavior? |
@garagatyi i think it's safe enough to do so. |
Rework injection Signed-off-by: Hanno Kolvenbach <[email protected]>
Signed-off-by: Hanno Kolvenbach <[email protected]>
9b8f2a2
to
d60e2d5
Compare
@garagatyi I finally found time to port this pull request to che6. Tested it with empty configuration, 1 configuration value here it is #7203 |
@skabashnyuk there is a port for che6 already |
@benoitf can you take a look? |
ci-build |
thanks @hkolvenbach |
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/4313/ |
Could someone please review the use of config variables? I am not entirely sure if I understand the translation from
CHE_DOCKER_SECURITYOPT
inche.env
to@Named('che.docker.securityopt')
correctly.This also needs #6855 to be fixed to be tested
What does this PR do?
This PR allows to specify the
securityOpt
parameter inHostConfig
to pass parameters to the docker daemon during container creation. thesecurityOpt
parameter is equal to the command line option--security-opt
of the command line. The valueseccomp:unconfined
allows gdb to run in unprivileged containers. Kubernetes usesseccomp:unconfined
by default, but docker does not.What issues does this PR fix or reference?
Related issues:
#4719
#4524
#5254
#4284
che.docker.securityopt
/CHE_DOCKER_SECURITYOPT
Release Notes
Docs PR
eclipse-che/che-docs#305