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.

Sign up for GitHub

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

Move image pushing into CircleCI #945

Merged
merged 1 commit into from
Oct 9, 2019
Merged

Move image pushing into CircleCI #945

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 18 additions & 4 deletions .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,16 @@ jobs:
name: "Check README in sync"
command: ./scripts/ci/check_readme_in_sync.sh

build:
build_and_publish:
machine:
enabled: true
steps:
- checkout
- run: ./scripts/publish.sh

build_and_test:
machine:
enabled: true
docker_layer_caching: true
steps:
- checkout
- run:
Expand Down Expand Up @@ -61,8 +67,16 @@ jobs:

workflows:
version: 2
build_and_test:
checks:
jobs:
- check_go_mod
- check_readme_sync
- build
- build_and_test
- build_and_publish:
requires:
- build_and_test
filters:
branches:
only: master
Copy link
Contributor

Choose a reason for hiding this comment

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

I was reading about the filters earlier and came across this post: https://discuss.circleci.com/t/dont-run-deploy-workflow-step-for-forked-pr/23606
It suggests that a PR from the master branch of a fork would pass this filter. We might want to consider expanding the check in the publish script to check for one of the environment variables that are only set for forked PRs: https://circleci.com/docs/2.0/env-vars/#built-in-environment-variables

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great question.

So you're right on the approach (using env vars) and we do this though by checking the CIRCLE_BRANCH. For pull requests it is pull/945 or whatnot.

However, as I was typing my response I realized that for the tagged portion of the bash script we could hit a problem. E.g. If I make a PR that includes a tag on my commit I may be able to trigger that part of the build.

We can make this check a bit more verbose by checking for another env var value about the PR, but we should acknowledge that a malicious user could still manipulate the scripts/env vars to trigger these. I dont know if we can prevent that.

Copy link
Contributor

@zubron zubron Oct 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I just checked the CIRCLE_BRANCH in a test PR and you're right, it sets the branch correctly to be a pull request branch:

#!/bin/bash -eo pipefail
echo $CIRCLE_BRANCH
echo $CIRCLE_PR_NUMBER
echo $CIRCLE_PR_REPONAME
echo $CIRCLE_PR_USERNAME
pull/946
946
sonobuoy
zubron

My concern was that CIRCLE_BRANCH would be set to the source branch, which could be master.

I'm not sure if there is anything that we can do to prevent malicious PRs. But, given that we've prevented the secrets being passed to forked PRs, they could bypass the checks but the steps shouldn't work because the secrets aren't available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, excellent point. The config settings in the UI prevent image pushing from ever actually occurring.

tags:
only: /^v.*/
35 changes: 35 additions & 0 deletions scripts/ci/publish.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
#!/bin/bash

# Don't fail silently when a step doesn't succeed
set -e

if [ -z "$CIRCLECI" ]; then
echo "this script is intended to be run only on travis" >&2
exit 1
fi

function goreleaser() {
curl -sL https://git.io/goreleaser | bash
}

function image_push() {
echo ${DOCKERHUB_TOKEN} | docker login --username sonobuoybot --password-stdin
IMAGE_BRANCH="$CIRCLE_BRANCH" make container push
}

if [ ! -z "$CIRCLE_TAG" ]; then
if [ "$(./sonobuoy version --short)" != "$CIRCLE_TAG" ]; then
echo "sonobuoy version does not match tagged version!" >&2
echo "sonobuoy short version is $(./sonobuoy version --short)" >&2
echo "tag is $CIRCLE_TAG" >&2
echo "sonobuoy full version info is $(./sonobuoy version)" >&2
exit 1
fi

goreleaser --skip-validate
image_push
fi

if [ "$CIRCLE_BRANCH" == "master" ]; then
image_push
fi