-
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
WIP Work around arbitrary user IDs on OpenShift #13561
Conversation
Tested with the following dev-file (should be a replacement of the
As @amisevsk mentioned the approach will work only if I tested against output for the s2i based:
output for
In any case, this is a great enchantment 👍 but we still need to think about the language runtime images we would use. |
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.
-
Should we have an ability to configure default
supportArbitraryUser
value for all workspaces with property and then override default behavior with workspace attributes? -
I've checked and
supportArbitraryUser
attribute can be specified in Devfile. Should it be listed in Devfile document? Somewhere here https://github.com/eclipse/che/blob/master/wsmaster/che-core-api-workspace/src/main/resources/schema/devfile.json, https://github.com/eclipse/che/blob/master/wsmaster/che-core-api-workspace/README-devfile.md
...rg/eclipse/che/workspace/infrastructure/openshift/provision/OpenShiftCommandProvisioner.java
Outdated
Show resolved
Hide resolved
* OpenShift default of starting containers with an arbitrary UID. Should be set/read from {@link | ||
* WorkspaceConfig#getAttributes()}. | ||
* | ||
* <p>Value is expected to be boolean; if it is set to 'true', then the recipe containers in the |
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 why only recipe containers should be affected?
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.
There are a few reasons I want to keep it to recipe containers only.
-
It's kind of a rough proxy for only affecting user-specified containers; we don't want to run the provisioner for broker containers.
-
I tried to keep this PR a relatively minimal set of changes. The issue we're trying to solve affects workspace containers primarily and so I focussed on those. If similar issues exist for typical sidecar containers then we could include those as well. I'd generally prefer to not rewrite commands unless we have to since it introduces potential errors and issues
- Additionally, our plugin runner containers don't specify a command/argument so this wouldn't work (it would actually break workspaces since the entrypoint would no longer be terminating)
-
Since plugin runner containers and similar are 'write once, run everywhere' containers, I think a better solution if a similar change is needed is to create the containers with the fixes built in. We control those dockerfiles and entrypoints already so a kind of sloppy work around is not necessary.
This does bring up a good point though; I think this PR as it currently is will break the devfile demo, since the db
container doesn't list an entrypoint. I'll have to address that (only do the overwriting if there is an command specified.
@sleshchenko Regarding the other comments
|
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.
LGTM I like @sleshchenko idea to make it configurable at wsmaster level but @amisevsk you have a good point as well. I would consider it for another PR after GA.
ci-test |
Results of automated E2E tests of Eclipse Che Multiuser on OCP: |
Add the workspace attribute 'supportArbitraryUser' to enable an attempt at working around containers running with arbitrary user ID on OpenShift. When the attribute is set to 'true', Che will modify the recipe containers' command and arguments to attempt to add an entry to /etc/passwd for the current user ID. This resolves various problems around programs that depend on username being available when running (e.g. maven, bash). If the command fails (e.g. because /etc/passwd is not writable), the original container command will still execute. Signed-off-by: Angel Misevski <[email protected]>
8e5117c
to
9963f94
Compare
It was decided to put this PR on hold and reconsider after Che 7 GA release. An alternative solution for GA is to patch existing community images and make them OpenShift compatible - eclipse-che/che-devfile-registry#24 |
+1 on @ibuziuk's comment, this PR is not good enough for many images (since |
could we somehow mount |
@sunix That was the post-7.0 plan -- try to do an overlay that is writeable. |
Closing this PR as current understanding of the issue means this fix won't work (most images don't have a writable |
What does this PR do?
Add the workspace attribute
supportArbitraryUser
to enable an attempt at working around containers running with arbitrary user ID on OpenShift.When the attribute is set to
true
, Che will modify the recipe containers' command and arguments to attempt to add an entry to/etc/passwd
for the current user ID. This resolves various problems around programs that depend on username being available when running (e.g. maven, bash).If the command fails (e.g. because
/etc/passwd
is not writable), the original container command will still execute.Note that this PR does not fully resolve #13454 since most default images do not have a root group writable
/etc/passwd
.Testing
To test you will need to use an image with a writeable
/etc/passwd
:Modified Java Maven devfile
What issues does this PR fix or reference?
#13454
Docs PR
TODO