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

All CLI commands should launch a standard shell #7496

Closed
smarterclayton opened this issue Feb 20, 2016 · 13 comments
Closed

All CLI commands should launch a standard shell #7496

smarterclayton opened this issue Feb 20, 2016 · 13 comments

Comments

@smarterclayton
Copy link
Contributor

We recently identified an issue with the default shell being chosen for the new post-commit build hook. It is using /bin/sh. oc rsh currently uses /bin/bash, and the new oc debug command is using /bin/sh.

When we invoke a default shell in an image, we should be using a consistent value. Currently, RedHat and Ubuntu based images symlink /bin/sh to /bin/bash or to dash. Alpine and Busybox images lack /bin/bash altogether. Therefore, /bin/bash is not an appropriate default for oc rsh in the larger Docker ecosystem.

However, the SCL images currently lack the ability to invoke /bin/sh and enter the correct environment for SCL. Since the SCL images exist only to provide an environment for a language, it is a requirement that shell access to an SCL container be able to a) drop the user into an environment where the correct SCL environment is setup when /bin/bash is invoked, and b) do the same thing for /bin/sh.

We need to

  1. Identify any other compatibility issues with the default of /bin/sh for all containers
  2. Fix the SCL images to setup the correct environment when /bin/sh is invoked.
  3. Fix oc rsh to use /bin/sh.
@smarterclayton
Copy link
Contributor Author

Feel free to split this into other issues on the appropriate teams @bparees.

@rhcarvalho and @csrwng FYI.

I don't think this blocks 1.2 but the priority is high to correct this before we introduce the post-commit hook.

@bparees
Copy link
Contributor

bparees commented Feb 20, 2016

We can't block on (2), @mfojtik already spent a huge effort getting the SCL enablement as good as it is, it's not going to get better in the near term. I've started the discussion w/ SCL about what can be done on their side to improve the situation, but that's likely 6 months out at best.

So if you want SCL to work, the default has to be "/bin/sh -i". Which is a reasonable default for oc rsh. It's less reasonable for the build hook, but it's all we've got at this point, other than your suggestion that we introspect the image to determine if /bin/bash is present and use it if so, otherwise use /bin/sh.

I don't know exactly what oc debug does, so i can't speak to what makes sense there. I thought it was just supposed to spin up the pod in a running state, at which point you'd oc rsh into it. If so, /bin/sh is a sufficent command (as would pretty much any command that doesn't fail), since you don't need SCL enabled for that.

@smarterclayton smarterclayton modified the milestone: 1.2.0 Feb 20, 2016
@bparees
Copy link
Contributor

bparees commented Feb 22, 2016

issue for rsh is here:
#7514

still not clear on what the issue is for oc debug, if there even is an issue.

we can continue this issue for resolving the post-build commit command, so assigning to @rhcarvalho

@bparees bparees assigned rhcarvalho and unassigned bparees Feb 22, 2016
@mfojtik
Copy link
Contributor

mfojtik commented Feb 22, 2016

FYI: #7519

@mfojtik
Copy link
Contributor

mfojtik commented Feb 22, 2016

@smarterclayton as @bparees said, there is not that much we can do for plain /bin/sh, to inject SCL you have to run at least interactive /bin/sh which means adding -i to invocation. It makes sense for oc rsh because that is IMHO expected to be interactive.

It makes less sense for things like oc exec /bin/sh. The possible options for now are:

  1. Create wrapper for /bin/sh in openshift images (smells horribly)
  2. Fix scl enable to do global enablement, so we can run it during Docker build and never bother about that again. The container should already have everything set/linked/copied/etc.

@smarterclayton
Copy link
Contributor Author

smarterclayton commented Feb 22, 2016 via email

@rhcarvalho
Copy link
Contributor

I had thought about a shell wrapper as well, and so today I wrote a small PoC, ugly:

#!/bin/bash.orig                
echo Hello world               
exec -a $0 /bin/bash.orig "$@" 

If we rename /bin/bash -> /bin/bash.orig and save the script above as /bin/bash, both /bin/sh and /bin/bash are affected (since the former is a symlink to the latter).

This seems to work for echo Hello, but led to infinite recursion when I replaced that for source scl_source enable ruby200, I believe because scl_source is a bash script with a shebang #!/bin/bash.

Might be possible to prevent the loop. Would there be other inconveniences with a wrapper?
(PIDs, signal handling, ?!)

@smarterclayton
Copy link
Contributor Author

smarterclayton commented Feb 22, 2016 via email

@rhcarvalho
Copy link
Contributor

I believe a binary would need to be packaged for us to be able to use that on RHEL... but then having a binary that replaces /bin/sh would not be acceptable per the packaging constraints (two packages cannot provide the same file), right?

@mfojtik
Copy link
Contributor

mfojtik commented Feb 22, 2016

@rhcarvalho we can alter the image and customize as we need to. having wrapper for /bin/sh is not the nicest thing in the world, but it can fix the SCL problem, which as Clayton pointed out should be really SCL team problem to solve :-)

We can just copy the static binary after we rpm install (to replace the sh).

@bparees
Copy link
Contributor

bparees commented Feb 22, 2016

regardless, for 3.2 it seems like "/bin/sh -i" is the only realistic solution, so we should switch to that. If we aren't comfortable with that, then we should mark it experimental so we are free to change the "default" command in the future.

@smarterclayton acceptable? What are all the places where it needs to be marked experimental?

@bparees
Copy link
Contributor

bparees commented Feb 22, 2016

(also when we doc it as experimental we should mention why, and how it is likely to change in the future, so people aren't scared away from using it. it's unlikely the change in the future will break anyone)

@bparees
Copy link
Contributor

bparees commented Feb 23, 2016

build-hooks now use "/bin/sh -i" (necessarily for SCL) (#7379)
oc rsh now uses "/bin/sh" (but results in an interactive shell)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants