-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Run post build hook with /bin/sh -ic
#7379
Run post build hook with /bin/sh -ic
#7379
Conversation
@bparees @smarterclayton @mfojtik PTAL [test][extended:core(build without output image)] |
@@ -145,6 +145,9 @@ | |||
"name": "origin-ruby-sample:latest" | |||
} | |||
}, | |||
"postCommit": { | |||
"script": "bundle exec rake test" | |||
}, |
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.
Perhaps we should omit this example, as the hook is not run by default for custom builds, @bparees wdyt?
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.
yeah, unless we're going to update the origin-custom-docker-builder image to actually respect/invoke the hook, we probably shouldn't define it since it implies behavior that isn't present.
Um, the problem you described is a problem with the sti-base image, and isn't a reason to switch to /bin/bash. By convention people call /bin/sh - so we need to as well. |
We started with We've already spent a lot of effort trying to make SCL packages work in our favor, nothing much left to squeeze (#2001 was my original complaint on SCL-powered images not working out-of-the-box). The changes to the hook that would make our images work are the two I mentioned:
The latter seems clumsy. I did some grepping here and there and found that some actions in OpenShift use |
@smarterclayton the pure For post-build, I think it is OK to use "/bin/bash" as default (when user does not specify something else, because user can do that). That way, we enable our own images to work, without breaking other folks too much :) |
When I argued for In practice, images built on CentOS, RHEL, Ubuntu, etc, will all work the same. For the minimalist images, they'd need to go for the Command/Args syntax. Really minimal images (without |
@rhcarvalho I think that as long as there is a way to allow minimal images to roll (by updating Args/Command), we're fine. |
@rhcarvalho what are the negatives associated with /bin/sh -ic? I agree with you that using /bin/sh seems preferable for more universal support (though as noted people can always specific /bin/sh if their image doesn't have bash) |
As we discussed on IRC, mainly that the script (and whatever it may invoke) would be thinking it is running in an interactive shell, and might behave differently than if the shell was non-interactive. |
lgtm but will need a docs pull I believe also? |
For the record, all tests passed. I'm removing the example from |
3e0fdb1
to
78a0c5b
Compare
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/5070/) (Image: devenv-rhel7_3512) |
No, the original docs has not merged yet. I've already synced with @PI-Victor this morning to include this update. |
Merge removed, still not comfortable with this. |
So the information I need to hear from you guys is:
|
Notice that I changed the tests to use |
So the impact of this change is that Alpine and Busybox images (which are POSIX compatibility mode is not significantly different than regular bash, I think I need more detail on why -i is dangerous. On Wed, Feb 17, 2016 at 12:51 PM, Rodolfo Carvalho <notifications@github.com
|
On Feb 17, 2016 19:21, "Clayton Coleman" notifications@github.com wrote:
Alpine does not use SCL. In worst case we can default to /bin/sh -ic
|
This PR is not changing
I can't say "-i is dangerous". Either using The former seemed more reasonable than the latter, but I don't have strong options for any. What we discussed so far today, "-i" is at least running scripts in an "unexpected way" from the script's point of view, since normally scripts are not run in interactive mode, and there might be some code somewhere that checks whether the shell is interactive or not to do something different. |
Can you quantify that concern? How are we going to fix SCL in the future? I think this needs to be fixed On Wed, Feb 17, 2016 at 1:46 PM, Rodolfo Carvalho notifications@github.com
|
@hhorak ^^ can you clarify the situation with SCL and requirement for enabling collections before using them? |
i don't see that this is a "problem" in SCL, the whole point of SCL is that i should be able to install multiple versions and pick which one i want to use. That necessitates a mechanism for setting up my paths correctly based on the version i want to use. The challenge for us (as image authors) is having a way to ensure those paths are always setup correctly. Depending what exactly SCL enable is doing, could we perhaps just update the appropriate env vars in the image as the default values? (does scl enable do anything other than setup env vars?) |
@smarterclayton please note that using Bash for the Script form is not preventing Busybox and Alpine users from using build hooks. We baked in full control to the image entrypoint and cmd, and the Script is just a shortcut for the common cases. Let's consider an Alpine-based golang image with the Go toolchain and some source code. The API allows for so many possibilities that we could easily run our tests with either: spec:
postCommit:
Command: ["go test"] or spec:
postCommit:
Args: ["go test"] |
The choice of default shell is a foundational value that will guide On Wed, Feb 17, 2016 at 2:57 PM, Rodolfo Carvalho notifications@github.com
|
@smarterclayton @rhcarvalho given that @smarterclayton do you consider it a bug that |
Yes, I consider oc rsh depending on bin/bash a bug. Do we know what On Fri, Feb 19, 2016 at 10:55 AM, Ben Parees notifications@github.com
|
@smarterclayton we aren't aware of any, the only thing we have right now is theoretical speculation that certain scripts/programs might behave differently(and incorrectly) when they think they are running in an interactive shell. eg attempting to prompt the user for information. |
Can we do some research and prove it? On Fri, Feb 19, 2016 at 12:06 PM, Ben Parees notifications@github.com
|
it's trivial to prove the problem is possible, we can write a script today that exhibits that behavior. Determining whether or not it's commonplace is pretty hard/impossible. (we might be able to find examples that prove it is a problem, we'll never be able to prove it's not). |
Is it a problem on our images. Has anyone opened an issue on this for us Need data to make the right decision. On Fri, Feb 19, 2016 at 1:23 PM, Ben Parees notifications@github.com
|
@smarterclayton it's not a matter of it being a problem for a given image, it's not. It all depends what you are trying to run under a "/bin/sh -i" environment. That thing may or may not be broken. In all likelihood, it won't be. if it is broken, the user would have to switch to "/bin/sh" at which point scl enablement is broken, so then they'd have to switch to "/bin/bash" if they also need the SCL content enabled. This really just comes down to two distinct options:
So if you don't like (1), let's just do (2) and move on, with a longer term term of getting SCL to provide a way to explicitly enable within an image, so we don't need to do either of these things. |
Is /bin/sh -i going to break alpine / busy box containers that have .bashrc On Fri, Feb 19, 2016 at 1:47 PM, Ben Parees notifications@github.com
|
why would an alpline or busybox container have a .bashrc when it doesn't have bash? regardless: so .bash_profile isn't going to be run for any of these cases (/bin/bash or /bin/sh -i), and .bashrc would be run for "/bin/sh -i" but not for "/bin/bash". |
ubuntu based image:
alpine:
docker library images:
our image:
Basically no shell binary is happy about running with "-i" when there's not actually a terminal, but they all seem to tolerate it. |
Incidentally: we do set ENV, just like we set BASH_ENV, so i don't know why this is not working with /bin/sh for our SCL images. It sounds like ENV should be exactly what we need to work with "/bin/sh" and get scl enabled. (this may be what @rhcarvalho was discussing earlier, that ENV is only processed for interactive shells) |
I do remember there was a problem with ENV, to make it work you need -i :(
|
A non-interactive shell invoked with the name sh does not attempt to read (ignoring ENV as well, which is why you have to pass -i). I think oc rsh is
|
and for the record, I consider using ENV as pretty ugly hack. however I
|
I've updated this to use |
We need either `/bin/bash -c` or `/bin/sh -ic` to make our supported SCL-powered images to work properly. That's due to a hack to auto-enable SCLs: https://github.com/openshift/sti-base/blob/8d95148/Dockerfile#L23-L29 In CentOS and RHEL images, `/bin/sh` is a symlink to `/bin/bash`. In that case, the ENV environment variable is only evaluated when the shell is started in interactive mode, which happens when there's and attached tty (not the case for post build hook) or when the `-i` argument is passed explicitly. Making the shell interactive with `-i` is ugly, but is the only solution we know so far without requiring Bash.
78a0c5b
to
46e7e91
Compare
Build logs of sample-app with S2I build:
@bparees FYI no funny messages like you observed with |
/bin/sh -ic
Evaluated for origin test up to 46e7e91 |
@bparees @smarterclayton I've updated the PR title/description/code to use No problems running |
Also works for the Python image (django-ex):
|
Thanks @rhcarvalho, lgtm. @smarterclayton are you ok with this or do you want it marked experimental? |
lgtm
|
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/1482/) (Extended Tests: core(build without output image)) |
It's ok but let's release note it / caveat it in the docs
|
[merge] |
Evaluated for origin merge up to 46e7e91 |
Merged by openshift-bot
@richm as I mentioned in IRC, seems that you're using the newest version of the template (master), with an older version of The solution, if anyone else runs into the same problem, is to use a version of the template that is not $ oc new-app https://raw.githubusercontent.com/openshift/origin/v1.1.3/examples/sample-app/application-template-stibuild.json I'll update the sample-app README to include a note about that. |
We need either
/bin/bash -c
or/bin/sh -ic
to make our supported SCL-powered images to work properly. That's due to a hack to auto-enable SCLs:https://github.com/openshift/sti-base/blob/8d95148/Dockerfile#L23-L29
In CentOS and RHEL images,
/bin/sh
is a symlink to/bin/bash
. In that case, the ENV environment variable is only evaluated when the shell is started in interactive mode, which happens when there's and attached tty (not the case for post build hook) or when the-i
argument is passed explicitly.Making the shell interactive with
-i
is ugly, but is the only solution we know so far without requiring Bash.