Skip to content
This repository has been archived by the owner on Aug 16, 2024. It is now read-only.

Patch all used images for arbitrary users on OpenShift #38

Merged
merged 2 commits into from
Jul 18, 2019

Conversation

amisevsk
Copy link
Contributor

What does this PR do?

  • Patches all (*) images used in devfiles to use arbitrary userID workaround
  • Moves some common env vars ($HOME and $PS1) into the template Dockerfile to simplify devfiles and make the experience more consistent
  • Add building and pushing of these patched images to the default nightly CI job

Issues still in this PR:

  • $PS1 can be inconsistent; ~/.bashrc is only modified if not present. For gradle:5.2.1-jdk11 and registry.centos.org/che-stacks/centos-nodejs, a ~/.bashrc is already present so those images show their default prompt (e.g. user@workspacetouo0g204wgupgjm:/projects$)
    • This could be worked around by appending to ~/.bashrc instead of rewriting only when empty, but may cause issues if ${HOME} is persisted later
    • Other options (such as setting an env var normally) do not seem to work or have strange parsing issues.
  • The image used for the angular stack is based off of node:10.16-alpine, which does not include bash. This devfile is left unmodified. To support this devfile we would either need to
    • Move to a non-alpine image, which would be larger (e.g. node:10.16-jessie is 685MB, but shares layers with other images used here), or
    • Support it as a special case or build a slightly patched image that has bash installed

Testing

A build of the devfile registry that refers to the patched images (in my dockerhub repo) is available at http://che-devfile-registry-eclipse-che.devtools-dev.ext.devshift.net/ so you should be able to try it out by setting the appropriate env var in your Che deployment (CHE_WORKSPACE_DEVFILE__REGISTRY__URL)

What issues does this PR fix or reference?

eclipse-che/che#13837, eclipse-che/che#13454, redhat-developer/rh-che#1455

- Patch all currently used images for arbitrary user support
- Move $HOME and $PS1 definition into arbitrary user patch to simplify
  devfiles and make UX more consistent
- Add additional fixes to patched images:
  - Set chmod g=u on /home to allow write access
  - Create home directory if it does not exist

Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@rhopp
Copy link
Contributor

rhopp commented Jul 17, 2019

So this entrypoint will override entrypoints of all images... Isn't there a chance that this would break some functionality of the images?

Also, if user would like to introduce his own image (for workspace/plugin), he will be required to do this?

@amisevsk
Copy link
Contributor Author

@rhopp There is a chance, but in general we specify command and args in devfiles already, which does the same thing. Our current practice is to always override entrypoint to be a non-terminating command (k8s command overrides dockerfile entrypoint). We're currently only patching main images (i.e. nothing is done for DB images, etc).

Regarding the second question, could you clarify what you mean? For adding an image to this registry a user could either a line to the base_images file and use that, or just add a devfile without patching, as unpatched devfiles are not blocked from working.

E.g. if I want to add a devfile and have an image that has its own workarounds for openshift, I can simply add that devfile to the registry.

@@ -1 +1,10 @@
java11-maven maven:3.6.0-jdk-11
che7-python-3.6 centos/python-36-centos7:1
Copy link
Contributor

Choose a reason for hiding this comment

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

why che7-* and not just che-*?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's mostly a holdover from when I intended these changes to go in the che-dockerfiles repo; I'll switch to plain che-.

Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@rhopp
Copy link
Contributor

rhopp commented Jul 18, 2019

My second question is mostly about any required manual steps required for creating devfiles with own images to be able to run on openshift. (customers of CRW will use their own images and any manual steps needs to be properly documented in the product and can introduce possible problems for customers & cases for our support :-D). But I guess there is no way around this...

@amisevsk
Copy link
Contributor Author

@rhopp Regarding the future path, once Che 7 is stable and release I have a few threads to pull on to figure out a more general solution. I believe our overall goal is that users should not have to patch images to run Che workspaces.

@amisevsk
Copy link
Contributor Author

I've done a reasonably thorough test of all our current devfiles (using eclipse-che:nightly-centos), available here. There are a few outstanding issues I'd like some discussion on:

  1. Workspaces that mount a volume into /home/user when that directory does not exist (e.g. java-maven mounting .m2 there) result in a ${HOME} directory that is not writable. This is because Kubernetes will create the /home/user dir at mount with only read access to the root user group.
    • The fix for this could be to create /home/user in the original dockerfile, but that somewhat binds ${HOME}=/home/user
  2. As mentioned above, some images have a fairly ugly terminal prompt, e.g. user@workspaceb01v80bc8vifu9rc:/projects$; this can be due to existing .bashrc. Does this issue register for the 7.0.0 milestone?
  3. The java-gradle image is the only one that does not use /home/user as ${HOME} (instead it uses /home/gradle. Should this be changed to be consistent?
    • the /home/gradle/.gradle directory is necessary and mounted as a volume, so setting ${HOME}=/home/user could make it harder for a user to find the .gradle directory.
  4. Of course, the nodejs-alpine image, used in the Angular devfile, is unpatched

Apart from that I have tested our current devfiles and they all work fairly well.

@l0rd WDYT?

@amisevsk amisevsk merged commit 94a6c92 into eclipse-che:master Jul 18, 2019
@amisevsk amisevsk deleted the arbitrary-users-patch branch July 18, 2019 21:07
@l0rd
Copy link
Contributor

l0rd commented Jul 18, 2019

I've done a reasonably thorough test of all our current devfiles (using eclipse-che:nightly-centos), available here.

Nice report!

  1. Workspaces that mount a volume into /home/user when that directory does not exist (e.g. java-maven mounting .m2 there) result in a ${HOME} directory that is not writable. This is because Kubernetes will create the /home/user dir at mount with only read access to the root user group.

    • The fix for this could be to create /home/user in the original dockerfile, but that somewhat binds ${HOME}=/home/user

We should create /home/user on every patched image. I don't have better ideas.

  1. As mentioned above, some images have a fairly ugly terminal prompt, e.g. user@workspaceb01v80bc8vifu9rc:/projects$; this can be due to existing .bashrc. Does this issue register for the 7.0.0 milestone?

I think it's perfectly fine for now. And much better than the sh prompt.

  1. The java-gradle image is the only one that does not use /home/user as ${HOME} (instead it uses /home/gradle. Should this be changed to be consistent?

    • the /home/gradle/.gradle directory is necessary and mounted as a volume, so setting ${HOME}=/home/user could make it harder for a user to find the .gradle directory.

What's the benefit (apart the consistency)?

  1. Of course, the nodejs-alpine image, used in the Angular devfile, is unpatched

Why is that?

Apart from that I have tested our current devfiles and they all work fairly well.

👍

@amisevsk
Copy link
Contributor Author

  1. Workspaces that mount a volume into /home/user when that directory does not exist (e.g. java-maven mounting .m2 there) result in a ${HOME} directory that is not writable. This is because Kubernetes will create the /home/user dir at mount with only read access to the root user group.

    • The fix for this could be to create /home/user in the original dockerfile, but that somewhat binds ${HOME}=/home/user

We should create /home/user on every patched image. I don't have better ideas.

Ok I'll do that; I need to patch + add a few images that were added while the original PR was open anyways.

  1. The java-gradle image is the only one that does not use /home/user as ${HOME} (instead it uses /home/gradle. Should this be changed to be consistent?

    • the /home/gradle/.gradle directory is necessary and mounted as a volume, so setting ${HOME}=/home/user could make it harder for a user to find the .gradle directory.

What's the benefit (apart the consistency)?

The issue originally was that /home/gradle was not user writable but I think I've come up with a work-around.

  1. Of course, the nodejs-alpine image, used in the Angular devfile, is unpatched

Why is that?

Alpine does not ship with bash by default so the current patch breaks the image (adds user and sets their default shell to /bin/bash). Supporting alpine would require a special case treatment. An alternative approach would be to make the patch /bin/sh compatible, but would also set the default shell to /bin/sh for all the other images.

@l0rd
Copy link
Contributor

l0rd commented Jul 22, 2019

For nodejs-alpine I think we should just use another image like node:12.6.0

nickboldt pushed a commit to nickboldt/che-devfile-registry that referenced this pull request Aug 9, 2019
* Patch all used images for arbitrary users on OpenShift

- Patch all currently used images for arbitrary user support
- Move $HOME and $PS1 definition into arbitrary user patch to simplify
  devfiles and make UX more consistent
- Add additional fixes to patched images:
  - Set chmod g=u on /home to allow write access
  - Create home directory if it does not exist

Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants