use a script instead of piping to stdin in run_docker_build #337

minrk · 2016-10-26T22:45:16Z

Puts the script in build_artefacts/conda-forge-build.sh for use in the image.

It would appear that some tests interact with the stdin pipe, causing run_docker_build to finish early with apparent success, skipping the upload step. They seem to be doing this by consuming or detaching stdin somehow.

By using a regular script, pipes aren't involved, fixing at least one known cause of tests preventing upload.

Further, future similar failures should be protected against by writing a canary file in the build_artefacts dir at the end of the script. This is checked after docker run, in case other bugs reintroduce early exit with apparent success.

This is confirmed to fix the failure I was seeing on the new petsc-feedstock. The problematic test involved mpiexec, if that's relevant.

Other possibly affected cases:

jakirkham · 2016-10-27T02:23:00Z

Good stuff @minrk ! Thanks for putting this together. 😀

Have definitely been suspicious of the pipe redirection for a while. I believe part of @pelson 's motivation for that design was to be security conscious. So we will need to evaluate these changes from that standpoint. Though maybe there are other concerns that he can share.

We should probably do something similar at staged-recipes. Though before we go down that road let me take a closer look at this implementation. Expect I should have some time later next week.

jakirkham · 2016-10-27T02:27:02Z

Also do you have any links to builds, commits and/or PRs that are relevant from the petsc feedstock?

minrk · 2016-10-27T11:37:58Z

This build was run with the re-render and re-enabling the problematic test: https://circleci.com/gh/minrk/petsc-feedstock/3

This build is otherwise identical, rendered with smithy-1.3.3: https://circleci.com/gh/minrk/petsc-feedstock/4

Both pass, but note how the latter doesn't show the "No BINSTAR_TOKEN present..." output of run_docker_build.sh, indicating that it stopped at conda-build.

minrk · 2016-10-27T16:30:37Z

I'm not sure that writing a script in the feedstock can introduce a new security issue, because run_test.sh, etc. are mounted from the same directory and can be modified (after launching the build, even) to change what code the container runs. Although I guess now there is a file containing the BINSTAR_TOKEN, which may be what was meant to be avoided. I'm not sure how to avoid that with the script.

Things I wasn't so sure about:

where to put the build script / canary. I used build_artefacts because it's already git-ignored, but maybe it should be a new ignored folder. I aimed for minimal new things to start in a relatively unfamiliar codebase.
whether the canary is really necessary - I would be surprised if the issue is ever reintroduced, so maybe the canary check is too much.

jakirkham · 2016-10-27T22:30:42Z

conda_smithy/templates/run_docker_build.tmpl

 EOF

+docker run -i \


Given we are giving up redirection, what do you think about enabling TTY? Have already had a few issues where not having a TTY has caused problems (e.g. ncurses).

sure, makes sense

TTY enabled

jakirkham · 2016-10-27T22:31:39Z

Although I guess now there is a file containing the BINSTAR_TOKEN, which may be what was meant to be avoided. I'm not sure how to avoid that with the script.

Yes, this is what I meant. Sorry if that was cryptic. Not sure ATM either, but will try to think on it.

jakirkham · 2016-10-27T22:32:50Z

where to put the build script / canary. I used build_artefacts because it's already git-ignored, but maybe it should be a new ignored folder. I aimed for minimal new things to start in a relatively unfamiliar codebase.

My inclination would be to put it in ci_support directory. We can also just add the file to .gitignore.

minrk · 2016-10-28T08:49:01Z

We can pass environment variables to docker on the CLI with -e and through files with --env-file, but both seem similarly snoopable. I don't see a way to tell docker run to 'use my environment variable' without echoing it somewhere other than the pipe.

Two pipey solutions if the file is to be avoided:

write the script via pipe to docker build - so we get a docker image with the token-having script
keep the pipe as-is, but write the whole content to a script in the container before running it

Both of these would avoid the token at rest on the host filesystem, but snoops with access to the docker service would be able to reach in and retrieve it

jakirkham · 2016-12-16T23:02:22Z

This needs a rebase. Probably broken by PR ( #285 ). Should be an easy fix.

minrk · 2017-01-02T13:05:23Z

Sorry for forgetting about this one. Rebased and TTY enabled, per comment above.

jakirkham · 2017-01-09T07:29:29Z

Thanks @minrk . I do want to take a closer look at this again, but it may not be for a couple weeks due to the other big changes with CI providers and conda-build.

jakirkham · 2017-01-11T18:43:41Z

Tentatively scheduling this for 2.1.0. I know we still have some more discussion and review to do, but am hopeful we should get this resolved soon. Please let me know if you think this is reasonable.

jakirkham · 2017-01-11T18:44:16Z

@gqmelo , I'd be curious to hear your thoughts on this change. Would you have a chance to take a look?

gqmelo · 2017-01-11T20:18:15Z

I like the idea, but I'm sorry for my ignorance: is there a good reason to keep only one template? I mean, couldn't we have a build.tmpl so that conda smithy would render a ci_support/build.sh and ci_support/run_docker_build.sh? What would be the implications of rendering a ci_support/build.sh and commit it?

It is just that this would be simpler and make it easy to use build.sh without docker. I've needed this sometimes to investigate some errors that happened inside docker but not on my native machine (but then I had to extract only the conda-build stuff from run_docker_build.sh to try to reproduce it).

Anyway this would be probably another issue. I think these changes are nice

minrk · 2017-02-08T09:07:00Z

What would be the implications of rendering a ci_support/build.sh and commit it?

Exactly as it is, build.sh includes the BINSTAR_TOKEN env from circle, which we don't have at render time. I think it could be generated as a template and use an env-file to pass the token, though.

minrk · 2017-02-08T09:10:30Z

Actually, I might have a better idea - if we can do the upload from the host environment instead of the build environment, this should be even simpler, and not have any issues with tokens.

gqmelo · 2017-02-08T11:40:43Z

Actually, I might have a better idea - if we can do the upload from the host environment instead of the build environment, this should be even simpler, and not have any issues with tokens.

That would be nice but you would have to setup the host environment with dependencies needed by the upload script, right?

dalcinl · 2017-02-08T14:44:40Z

@minrk @jakirkham What about running docker two times? The first one you build using a script, the second one you upload passing the token through stdin. I think this is even better, as you never expose the token to the build scripts.

jakirkham · 2017-02-08T16:33:49Z

Was thinking about the same thing, @dalcinl.

minrk · 2017-02-08T16:33:56Z

@dalcinl good idea. This now runs docker twice:

run the build with a plain script and proper tty (should fix mpiexec issues)
run the upload step in a second container, passing the token via stdin

minrk · 2018-01-16T18:47:58Z

Rebased on master

Puts the script in build_artefacts/conda-forge-build.sh It would appear that some tests interact with the stdin pipe, causing run_docker_build to finish early with apparent success, skipping the upload step. They seem to be doing this by consuming or detaching stdin somehow. By using a regular script, no pipes are involved, fixing at least one known cause of tests preventing upload. Further, future similar failures should be protected against by writing a canary file in the build_artefacts dir at the end of the script. This is checked, in case other bugs reintroduce early exit with apparent success.

and ignore it in .gitignore

1. build script doesn't have access to the token 2. upload gets token via stdin, but doesn't run user code commit build container to an image, so upload can run in the same env

- build.sh goes in .circleci - preserve set -x

not needed now that circle uses a real build matrix

minrk · 2018-03-02T10:08:36Z

rebased. This has gotten simpler since the original PR now that the matrix env stuff is no longer needed.

ocefpaf · 2018-03-02T10:19:45Z

Ouch. Sorry @minrk another PR that we dropped the ball.

isuruf · 2018-03-02T14:55:58Z

conda_smithy/templates/run_docker_build.tmpl

+           -v "${RECIPE_ROOT}":/home/conda/recipe_root \
+           -v "${FEEDSTOCK_ROOT}":/home/conda/feedstock_root \
+           --env-file ${FEEDSTOCK_ROOT}/env \
+           -e HOST_USER_ID="${HOST_USER_ID}" \


You've missed the CONFIG env variable here.

isuruf · 2018-03-02T14:56:32Z

conda_smithy/templates/run_docker_build.tmpl

-# see https://github.com/conda-forge/conda-smithy/pull/337
-# for a possible fix
+set +x
+echo "BINSTAR_TOKEN=${BINSTAR_TOKEN}" > "${FEEDSTOCK_ROOT}/env"


Put this inside .circleci folder?

Actually would it make sense to save this in a directory that is not mounted in the container? To propose an arbitrary option, what about ~/.conda-smithy/upload.token?

missed in rebase of conda-forge#337

jakirkham · 2018-03-02T15:45:05Z

conda_smithy/templates/run_docker_build.tmpl

+CONTAINER=$(docker create -t \
+           -v "${RECIPE_ROOT}":/home/conda/recipe_root \
+           -v "${FEEDSTOCK_ROOT}":/home/conda/feedstock_root \
+           --env-file ${FEEDSTOCK_ROOT}/env \


Was not aware of this option. Nice fix!

isuruf · 2018-03-03T09:08:51Z

This doesn't work in circleci and hangs with no output after the build.sh is run.

jakirkham · 2018-03-03T14:39:59Z

We can change back to docker run and just point to an env file outside the feedstock.

isuruf · 2018-03-03T21:07:21Z

I'm going to revert it as I can't find anyway to fix it

jakirkham · 2018-03-03T21:20:46Z

That seems reasonable, @isuruf. FWIW I do want us to solve this issue. Just think trying to tackle it in conda-smithy 3 when we are very close to releasing (and thus getting conda-build 3 support), is a bit ambitious personally.

minrk · 2018-03-04T11:10:11Z

FWIW, after the single typo fixed in #692 I can confirm that this PR results in working builds on circleci with no issues. successful build here.

minrk changed the title ~~use a script instead of stdin-pipe in run_docker_build~~ use a script instead of piping to stdin in run_docker_build Oct 26, 2016

jakirkham reviewed Oct 27, 2016

View reviewed changes

minrk force-pushed the docker-build-script branch from 4a64d00 to 9a9fe6b Compare January 2, 2017 13:00

minrk mentioned this pull request Jan 2, 2017

Build long prefix package using conda-build 2.x conda-forge/petsc-feedstock#5

Merged

jakirkham mentioned this pull request Jan 8, 2017

Linux Python 3.6 builds not present conda-forge/yt-feedstock#16

Closed

jakirkham added this to the 2.1.0 milestone Jan 11, 2017

This was referenced Feb 8, 2017

openmpi [linux] conda-forge/openmpi-feedstock#8

Closed

skip mpiexec tests on Linux [travis skip] conda-forge/openmpi-feedstock#10

Merged

skip mpiexec tests on Linux conda-forge/mpich-feedstock#10

Merged

minrk mentioned this pull request Feb 8, 2017

verify that docker script reached its end #454

Merged

minrk force-pushed the docker-build-script branch from 734034b to df12dee Compare February 8, 2017 16:31

pelson approved these changes Jan 11, 2018

View reviewed changes

minrk force-pushed the docker-build-script branch 2 times, most recently from 65b93a3 to 3976569 Compare January 16, 2018 18:46

jakirkham mentioned this pull request Feb 19, 2018

Upgrade to 20.2 conda-forge/erlang-feedstock#26

Merged

minrk added 7 commits March 2, 2018 10:51

put build.sh in ci_support

97b5b51

and ignore it in .gitignore

run docker build with TTY

7a79123

run docker twice

dac8e1e

1. build script doesn't have access to the token 2. upload gets token via stdin, but doesn't run user code commit build container to an image, so upload can run in the same env

use set -e to simplify exit codes in docker build

4fc4957

use env file to pass binstar token to docker

7d18fff

fixes after rebase

762f420

- build.sh goes in .circleci - preserve set -x

minrk force-pushed the docker-build-script branch from 3976569 to 762f420 Compare March 2, 2018 10:02

remove docker build matrix templates

bef0f87

not needed now that circle uses a real build matrix

minrk force-pushed the docker-build-script branch from 096e7dc to bef0f87 Compare March 2, 2018 10:06

ocefpaf merged commit 6d479f9 into conda-forge:master Mar 2, 2018

minrk deleted the docker-build-script branch March 2, 2018 11:48

isuruf reviewed Mar 2, 2018

View reviewed changes

minrk mentioned this pull request Mar 2, 2018

add CONFIG env back to run_docker_build #692

Closed

minrk added a commit to minrk/conda-smithy that referenced this pull request Mar 2, 2018

add CONFIG env back to run_docker_build

8a96eea

missed in rebase of conda-forge#337

jakirkham reviewed Mar 2, 2018

View reviewed changes

isuruf mentioned this pull request Mar 3, 2018

Revert "use a script instead of piping to stdin in run_docker_build" #695

Merged

minrk mentioned this pull request Mar 4, 2018

reissue: remove pipes from docker #701

Merged

use a script instead of piping to stdin in run_docker_build #337

use a script instead of piping to stdin in run_docker_build #337

Conversation

minrk commented Oct 26, 2016

jakirkham commented Oct 27, 2016

jakirkham commented Oct 27, 2016

minrk commented Oct 27, 2016

minrk commented Oct 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakirkham commented Oct 27, 2016

jakirkham commented Oct 27, 2016

minrk commented Oct 28, 2016

jakirkham commented Dec 16, 2016

minrk commented Jan 2, 2017

jakirkham commented Jan 9, 2017

jakirkham commented Jan 11, 2017

jakirkham commented Jan 11, 2017

gqmelo commented Jan 11, 2017

minrk commented Feb 8, 2017

minrk commented Feb 8, 2017

gqmelo commented Feb 8, 2017

dalcinl commented Feb 8, 2017

jakirkham commented Feb 8, 2017

minrk commented Feb 8, 2017 • edited Loading

minrk commented Jan 16, 2018

minrk commented Mar 2, 2018 • edited Loading

ocefpaf commented Mar 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isuruf commented Mar 3, 2018

jakirkham commented Mar 3, 2018

isuruf commented Mar 3, 2018

jakirkham commented Mar 3, 2018

minrk commented Mar 4, 2018 • edited Loading

minrk commented Oct 27, 2016 •

edited

Loading

minrk commented Feb 8, 2017 •

edited

Loading

minrk commented Mar 2, 2018 •

edited

Loading

minrk commented Mar 4, 2018 •

edited

Loading