Set SHELL env var in launcher if unset to avoid issues in Code's shell detection #284

amisevsk · 2023-10-11T20:43:48Z

What does this PR do?

If the SHELL environment variable is unset when running the Code launcher, set it:

If we detect that bash is installed, to /bin/bash
Otherwise, fallback to setting it to /bin/sh

This is necessary due to shell-detection logic within Code itself, which will fallback to parsing /etc/passwd when SHELL is not set (see [1]). When running on e.g. OpenShift with a normal container, cri-o will add an /etc/passwd entry for the current user with /sbin/nologin, which results the terminal failing to launch.

What issues does this PR fix?

Closes eclipse-che/che#22524

How to test this PR?

Changes from this PR are built into quay.io/amisevsk/che-code:dev via the following Dockerfile:

FROM registry.access.redhat.com/ubi8/nodejs-18:1-71 as launcher-builder

USER root

COPY . /checode-launcher
WORKDIR /checode-launcher

RUN npm install -g yarn@1.22.17

RUN yarn \
    && mkdir /test-launcher \
    && cp -r out/src/*.js /test-launcher \
    && chgrp -R 0 /test-launcher && chmod -R g+rwX /test-launcher

FROM quay.io/che-incubator/che-code:next
COPY --from=launcher-builder --chown=0:0 /test-launcher /checode-linux-libc/launcher

To test the changes on an existing workspace, edit the DevWorkspace yaml on the cluster, updating the .spec.contributions field:

  spec:
    contributions:
      - name: editor
        kubernetes:
          name: che-code-java-maven
+       components:
+         - container:
+             image: 'quay.io/amisevsk/che-code:dev'
+           name: che-code-injector

(This will use quay.io/amisevsk/che-code:dev in place of the default injector image)

We should verify three cases before merging:

Samples that do not set the SHELL environment variable have It is not possible to start a terminal when workspace started from a devfile from the registry.devfile.io eclipse-che/che#22524 resolved.
- To make this easier (since links to raw devfiles in registry.devfile.io do not seem to work in Che), you should be able to use the raw link on this gist and paste it into the dashboard: https://gist.github.com/amisevsk/c4ffd5185d81420be2d8e73cc1db9d45 (taken from https://registry.devfile.io/viewer/devfiles/community/java-maven), then update the DevWorkspace as above
It's still possible to override the shell by setting the SHELL env var in the devfile or container
Default Che samples are unimpacted

Before launching Code, if the SHELL environment variable is unset, set it to something before launching: * If SHELL is unset and /bin/bash is installed, use that * Otherwise, fall back to /bin/sh as a 'safe' option This is necessary to avoid internal shell-detection logic in Code, which reads /etc/passwd when SHELL is not set. This is an issue when running in e.g. OpenShift, where cri-o will add a user entry to /etc/passwd containing /sbin/nologin as the login shell. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Signed-off-by: Angel Misevski <amisevsk@redhat.com>

github-actions · 2023-10-11T20:44:02Z

Click here to review and test in web IDE:

github-actions · 2023-10-11T22:19:17Z

Pull Request images published ✨
image: quay.io/che-incubator-pull-requests/che-code:pr-284-amd64
Dev image: quay.io/che-incubator-pull-requests/che-code-dev:pr-284-dev-amd64

github-actions · 2023-10-12T10:09:17Z

Pull Request images published ✨
image: quay.io/che-incubator-pull-requests/che-code:pr-284-amd64
Dev image: quay.io/che-incubator-pull-requests/che-code-dev:pr-284-dev-amd64

l0rd · 2023-10-12T10:11:16Z

launcher/src/vscode-launcher.ts

@@ -66,6 +66,17 @@ export class VSCodeLauncher {
      env.NODE_EXTRA_CA_CERTS = NODE_EXTRA_CERTIFICATE;
    }

+    if (!env.SHELL) {


What about checking if the user shell (in /etc/passwd) is /sbin/nologin or not? If cri-o hasn't patched /etc/passwd (i.e. containerd is the container runtime) there may be a valid shell for current user anyway.

Beyond that I have tested the PR using image quay.io/che-incubator-pull-requests/che-code:pr-284-amd64 and the quarkus devfile from registry.devfile.io and the problem got solved 🎉 . I had an unrelated problem though and I have filed a separate issue.

This is probably a good idea -- I didn't initially do it since I was trying to keep it simple (since IIRC the default UDI image sets SHELL) but better safe than sorry :)

Updated the PR now. If /etc/passwd contains anything other than /sbin/nologin, we no longer override it by setting SHELL if it's not already set.

github-actions · 2023-10-12T11:38:05Z

Pull Request images published ✨
image: quay.io/che-incubator-pull-requests/che-code:pr-284-amd64
Dev image: quay.io/che-incubator-pull-requests/che-code-dev:pr-284-dev-amd64

github-actions · 2023-10-12T19:18:40Z

Pull Request images published ✨
image: quay.io/che-incubator-pull-requests/che-code:pr-284-amd64
Dev image: quay.io/che-incubator-pull-requests/che-code-dev:pr-284-dev-amd64

github-actions · 2023-10-12T19:21:29Z

Pull Request images published ✨
image: quay.io/che-incubator-pull-requests/che-code:pr-284-amd64
Dev image: quay.io/che-incubator-pull-requests/che-code-dev:pr-284-dev-amd64

github-actions · 2023-10-13T11:21:22Z

Pull Request images published ✨
image: quay.io/che-incubator-pull-requests/che-code:pr-284-amd64
Dev image: quay.io/che-incubator-pull-requests/che-code-dev:pr-284-dev-amd64

amisevsk · 2023-10-16T18:08:19Z

@vitaliy-guliy @l0rd Updated this PR after Mario's suggestion, please take a look before I merge.

When launching Code, add an additional check to read /etc/passwd and check if the user has a (non-/sbin/nologin) shell defined there. If /etc/passwd contains any other shell (e.g. /bin/zsh, /bin/bash, etc.), then let Code read that shell from /etc/passwd instead of overriding it by setting the SHELL environment variable. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

github-actions · 2023-10-16T21:17:17Z

Pull Request images published ✨
image: quay.io/che-incubator-pull-requests/che-code:pr-284-amd64
Dev image: quay.io/che-incubator-pull-requests/che-code-dev:pr-284-dev-amd64

devstudio-release · 2023-10-17T16:22:16Z

Build 3.10 :: code_3.x/959: Console, Changes, Git Data

devstudio-release · 2023-10-17T16:29:08Z

Build 3.10 :: sync-to-downstream_3.x/4970: Console, Changes, Git Data

devstudio-release · 2023-10-17T17:54:57Z

Build 3.10 :: get-sources-rhpkg-container-build_3.x/4794: Console, Changes, Git Data

devstudio-release · 2023-10-17T20:19:07Z

Build 3.10 :: get-sources-rhpkg-container-build_3.x/4794:

code : 3.x ::
; copied to quay

github-actions · 2023-10-20T17:44:43Z

Pull Request images published ✨
image: quay.io/che-incubator-pull-requests/che-code:pr-284-amd64
Dev image: quay.io/che-incubator-pull-requests/che-code-dev:pr-284-dev-amd64

amisevsk added 3 commits October 11, 2023 16:43

Add test cases to cover setting SHELL env var in launcher

c6a8e86

Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Fixup typo in workspace file creation logs

b83d6e7

Signed-off-by: Angel Misevski <amisevsk@redhat.com>

amisevsk requested review from azatsarynnyy, RomanNikitenko, vitaliy-guliy and l0rd as code owners October 11, 2023 20:43

l0rd approved these changes Oct 12, 2023

View reviewed changes

vitaliy-guliy approved these changes Oct 13, 2023

View reviewed changes

amisevsk force-pushed the avoid-noshell branch from 778c229 to 7544fdb Compare October 16, 2023 19:49

l0rd approved these changes Oct 17, 2023

View reviewed changes

amisevsk merged commit b71a618 into che-incubator:main Oct 17, 2023
2 checks passed

amisevsk deleted the avoid-noshell branch October 17, 2023 16:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set SHELL env var in launcher if unset to avoid issues in Code's shell detection #284

Set SHELL env var in launcher if unset to avoid issues in Code's shell detection #284

amisevsk commented Oct 11, 2023

github-actions bot commented Oct 11, 2023 •

edited

Loading

github-actions bot commented Oct 11, 2023

github-actions bot commented Oct 12, 2023

l0rd Oct 12, 2023

amisevsk Oct 12, 2023

amisevsk Oct 16, 2023

github-actions bot commented Oct 12, 2023

github-actions bot commented Oct 12, 2023

github-actions bot commented Oct 12, 2023

github-actions bot commented Oct 13, 2023

amisevsk commented Oct 16, 2023

github-actions bot commented Oct 16, 2023

devstudio-release commented Oct 17, 2023

devstudio-release commented Oct 17, 2023

devstudio-release commented Oct 17, 2023

devstudio-release commented Oct 17, 2023

github-actions bot commented Oct 20, 2023

Set SHELL env var in launcher if unset to avoid issues in Code's shell detection #284

Set SHELL env var in launcher if unset to avoid issues in Code's shell detection #284

Conversation

amisevsk commented Oct 11, 2023

What does this PR do?

What issues does this PR fix?

How to test this PR?

github-actions bot commented Oct 11, 2023 • edited Loading

github-actions bot commented Oct 11, 2023

github-actions bot commented Oct 12, 2023

l0rd Oct 12, 2023

Choose a reason for hiding this comment

amisevsk Oct 12, 2023

Choose a reason for hiding this comment

amisevsk Oct 16, 2023

Choose a reason for hiding this comment

github-actions bot commented Oct 12, 2023

github-actions bot commented Oct 12, 2023

github-actions bot commented Oct 12, 2023

github-actions bot commented Oct 13, 2023

amisevsk commented Oct 16, 2023

github-actions bot commented Oct 16, 2023

devstudio-release commented Oct 17, 2023

devstudio-release commented Oct 17, 2023

devstudio-release commented Oct 17, 2023

devstudio-release commented Oct 17, 2023

github-actions bot commented Oct 20, 2023

github-actions bot commented Oct 11, 2023 •

edited

Loading