Skip to content
This repository was archived by the owner on Feb 5, 2020. It is now read-only.

Jenkinsfile,tests/smoke: assume minimum role#974

Merged
squat merged 1 commit intocoreos:masterfrom
squat:assume_role_smoke
Jun 5, 2017
Merged

Jenkinsfile,tests/smoke: assume minimum role#974
squat merged 1 commit intocoreos:masterfrom
squat:assume_role_smoke

Conversation

@squat
Copy link
Contributor

@squat squat commented Jun 3, 2017

We want to ensure that our smoke tests run with limited privileges so we
can catch when a change adds a dependency on a new privilege.

@ggreer @Quentin-M

create() {
common "$1"
make plan | filter
assume_role
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we don't need to plan here. if you want to plan and create then execute two calls: smoke.sh plan... followed by smoke.sh create...

usage
exit 1
fi
main () {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

make this a function so we can make the variables local. this is useful so that sourcing the script does not set a bunch of variables like COMMAND on the calling shell.

}

common() {
DIR="$( cd "$( dirname "$0" )" && pwd )"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these are common dependencies so it makes sense for them to go in common.

@squat squat force-pushed the assume_role_smoke branch 10 times, most recently from 4ddaff5 to 7c35461 Compare June 5, 2017 20:03
@squat squat force-pushed the assume_role_smoke branch 2 times, most recently from 38f9172 to a9d1ea3 Compare June 5, 2017 20:20
Jenkinsfile Outdated
stage("Assume Role") {
steps {
withCredentials(creds) {
sh 'set +x -e && eval "$(${WORKSPACE}/tests/smoke/aws/smoke.sh assume-role "$TECTONIC_INSTALLER_ROLE")"'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

set +x so that the eval'd credentials are not printed out to stdout

Jenkinsfile Outdated
stage("Assume Role") {
steps {
withCredentials(creds) {
sh 'set +x -e && eval "$(${WORKSPACE}/tests/smoke/aws/smoke.sh assume-role "$TECTONIC_INSTALLER_ROLE")"'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we use eval rather than source as the method for passing variables because if we source the script, we try to execute bash-specific commands like set -o pipefail in sh which causes an error.

@Quentin-M
Copy link
Contributor

Quentin-M commented Jun 5, 2017

Is there a simple way to do that? Stages are not meant to run individual steps like assuming a role.
Also, is there a way we can avoid echoing these creds and hiding them at the same time with +x?

@squat
Copy link
Contributor Author

squat commented Jun 5, 2017

@Quentin-M the other option is to run "assume-role" in every parallel stage before the make apply/plan/destroy. Do you prefer that style?

The Jenkins step must call +x to avoid printing the eval'd statements and the smoke.sh must call set +x to avoid printing the echo'd statements. We need both because smoke.sh does set -x at the very top when it is executed.

@squat squat force-pushed the assume_role_smoke branch from a9d1ea3 to f0e0dd8 Compare June 5, 2017 21:15
@Quentin-M
Copy link
Contributor

The reason we can't do that is that, stages/steps are not all going to run on the same machines (see bare-metal vs. AWS) - thus the environment won't be everywhere.

Quentin-M
Quentin-M previously approved these changes Jun 5, 2017
Copy link
Contributor

@Quentin-M Quentin-M left a comment

Choose a reason for hiding this comment

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

Thanks!

We want to ensure that our smoke tests run with limited privileges so we
can catch when a change adds a dependency on a new privilege.
@squat squat merged commit 9bd829c into coreos:master Jun 5, 2017
@squat squat deleted the assume_role_smoke branch June 5, 2017 22:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants