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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP Work around arbitrary user IDs on OpenShift #13561

Closed
wants to merge 1 commit into from

Conversation

amisevsk
Copy link
Contributor

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
  metadata:
    name: maven-s2i
  projects: []
  attributes:
    persistVolumes: 'true'
    supportArbitraryUser: 'true'
  components:
    - mountSources: false
      endpoints: []
      command: []
      id: redhat/java/latest
      args: []
      preferences: {}
      selector: {}
      type: chePlugin
      entrypoints: []
      volumes: []
      env: []
    - mountSources: true
      endpoints:
        - name: 8080/tcp
          port: 8080
          attributes: {}
      command:
        - sleep
      args:
        - infinity
      memoryLimit: 512Mi
      preferences: {}
      selector: {}
      type: dockerimage
      entrypoints: []
      volumes:
        - name: m2
          containerPath: /home/user/.m2
      image: 'fabric8/s2i-java:latest-java11'
      alias: maven
      env:
        - value: /home/user/.m2
          name: MAVEN_CONFIG
        - value: >-
            -XX:MaxRAMPercentage=50 -XX:+UseParallelGC -XX:MinHeapFreeRatio=10
            -XX:MaxHeapFreeRatio=20 -XX:GCTimeRatio=4
            -XX:AdaptiveSizePolicyWeight=90 -Dsun.zip.disableMemoryMapping=true
            -Xms20m -Djava.security.egd=file:/dev/./urandom -Duser.home=/home/user
          name: MAVEN_OPTS
        - value: >-
            -XX:MaxRAMPercentage=50 -XX:+UseParallelGC -XX:MinHeapFreeRatio=10
            -XX:MaxHeapFreeRatio=20 -XX:GCTimeRatio=4
            -XX:AdaptiveSizePolicyWeight=90 -Dsun.zip.disableMemoryMapping=true
            -Xms20m -Djava.security.egd=file:/dev/./urandom
          name: JAVA_OPTS
        - value: >-
            -XX:MaxRAMPercentage=50 -XX:+UseParallelGC -XX:MinHeapFreeRatio=10
            -XX:MaxHeapFreeRatio=20 -XX:GCTimeRatio=4
            -XX:AdaptiveSizePolicyWeight=90 -Dsun.zip.disableMemoryMapping=true
            -Xms20m -Djava.security.egd=file:/dev/./urandom
          name: JAVA_TOOL_OPTIONS
        - value: '$(echo ${0})\\$'
          name: PS1
        - value: /home/user
          name: HOME
  apiVersion: 1.0.0
  commands: []

What issues does this PR fix or reference?

#13454

Docs PR

TODO

@amisevsk amisevsk added the kind/enhancement A feature request - must adhere to the feature request template. label Jun 17, 2019
@ibuziuk
Copy link
Member

ibuziuk commented Jun 18, 2019

Tested with the following dev-file (should be a replacement of the Vaadin factory from getting started [1])


apiVersion: 1.0.0
metadata:
name: vaadin-addressbook
attributes:
persistVolumes: 'false'
supportArbitraryUser: 'true'
projects:

  • name: addressbook
    source:
    type: git
    location: 'https://github.com/vaadin/addressbook'
    components:
  • type: chePlugin
    id: redhat/java/latest
  • type: dockerimage
    alias: maven
    image: fabric8/s2i-java:latest-java11
    command: ['sleep']
    args: ['infinity']
    env:
    • name: MAVEN_CONFIG
      value: /home/user/.m2
    • name: MAVEN_OPTS
      value: "-XX:MaxRAMPercentage=50 -XX:+UseParallelGC -XX:MinHeapFreeRatio=10
      -XX:MaxHeapFreeRatio=20 -XX:GCTimeRatio=4 -XX:AdaptiveSizePolicyWeight=90
      -Dsun.zip.disableMemoryMapping=true -Xms20m -Djava.security.egd=file:/dev/./urandom
      -Duser.home=/home/user"
    • name: JAVA_OPTS
      value: "-XX:MaxRAMPercentage=50 -XX:+UseParallelGC -XX:MinHeapFreeRatio=10
      -XX:MaxHeapFreeRatio=20 -XX:GCTimeRatio=4 -XX:AdaptiveSizePolicyWeight=90
      -Dsun.zip.disableMemoryMapping=true -Xms20m -Djava.security.egd=file:/dev/./urandom"
    • name: JAVA_TOOL_OPTIONS
      value: "-XX:MaxRAMPercentage=50 -XX:+UseParallelGC -XX:MinHeapFreeRatio=10
      -XX:MaxHeapFreeRatio=20 -XX:GCTimeRatio=4 -XX:AdaptiveSizePolicyWeight=90
      -Dsun.zip.disableMemoryMapping=true -Xms20m -Djava.security.egd=file:/dev/./urandom"
    • name: PS1
      value: $(echo ${0})\$
    • name: HOME
      value: /home/user
      memoryLimit: 512Mi
      endpoints:
    • name: '8080/tcp'
      port: 8080
      mountSources: true
      volumes:
    • name: m2
      containerPath: /home/user/.m2
      commands:
  • name: run
    actions:
    • type: exec
      component: maven
      command: "mvn -f ${CHE_PROJECTS_ROOT}/addressbook jetty:run"
  • name: stop
    actions:
    • type: exec
      component: maven
      command: "mvn -f ${CHE_PROJECTS_ROOT}/addressbook jetty:stop"

As @amisevsk mentioned the approach will work only if /etc/passwd is writable which is not the case for most of the images e.g. maven:3.6.0-jdk-11 and even with this fix we will need to use images like s2i e.g. fabric8/s2i-java:latest-java11 with writable /etc/passwd

I tested against fabric8/s2i-java:latest-java11 and maven:3.6.0-jdk-11

output for the s2i based:

bash-4.2$ whoami
user

output for maven:3.6.0-jdk-11 still:

sh$whoami
whoami: cannot find name for user ID 1000880000

In any case, this is a great enchantment 👍 but we still need to think about the language runtime images we would use.

[1] https://www.eclipse.org/che/getting-started/cloud/

Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

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

  1. Should we have an ability to configure default supportArbitraryUser value for all workspaces with property and then override default behavior with workspace attributes?

  2. 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

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

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?

Copy link
Contributor Author

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.

  1. It's kind of a rough proxy for only affecting user-specified containers; we don't want to run the provisioner for broker containers.

  2. 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)
  3. 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.

@amisevsk
Copy link
Contributor Author

@sleshchenko Regarding the other comments

  • I'm open to allowing a config property to specify the default here when no attribute is present, but worry that it could make debugging issues more difficult (it's another config setting we have to be aware of when figuring out what's wrong) and also without a UI aspect it could confuse users.
  • Good point around documentation, I will add that.

Copy link
Contributor

@l0rd l0rd left a 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.

@amisevsk
Copy link
Contributor Author

ci-test

@che-bot
Copy link
Contributor

che-bot commented Jun 20, 2019

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:13561
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

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 <amisevsk@redhat.com>
@amisevsk amisevsk changed the title Work around arbitrary user IDs on OpenShift WIP Work around arbitrary user IDs on OpenShift Jun 27, 2019
@che-bot che-bot added the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Jun 27, 2019
@ibuziuk
Copy link
Member

ibuziuk commented Jul 2, 2019

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

@amisevsk
Copy link
Contributor Author

+1 on @ibuziuk's comment, this PR is not good enough for many images (since /etc/passwd is typically not writeable and images with writable /etc/passwd are not commonly available -- it is hard to find compatible images).

@sunix
Copy link
Contributor

sunix commented Jul 15, 2019

could we somehow mount /etc/passwd ?

@amisevsk
Copy link
Contributor Author

@sunix That was the post-7.0 plan -- try to do an overlay that is writeable.

@eclipse-che eclipse-che deleted a comment from che-bot Aug 6, 2019
@amisevsk
Copy link
Contributor Author

Closing this PR as current understanding of the issue means this fix won't work (most images don't have a writable /etc/passwd). Solving this problem would be an entirely new PR.

@amisevsk amisevsk closed this Aug 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A feature request - must adhere to the feature request template. status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Most public images used in devfile registry do not work as the base for Che 7 workspaces on OpenShift
6 participants