-
Notifications
You must be signed in to change notification settings - Fork 273
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add signing parameters to cfn template #1263
base: main
Are you sure you want to change the base?
Conversation
7b04b23
to
c02bc4d
Compare
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.
Thanks for doing the heave lifting here 🙇♂️
Do we actually need to upload the template to S3? Did you try writing it to a file and using the file://
url instead?
Ok, I tried it, and it did not work either, the same limit applies. I think you should mention that neither --template-body
nor --template-file file://
, work in the PR description or in some comments.
.buildkite/steps/launch.sh
Outdated
echo "--- Validating templates" | ||
make validate | ||
aws --no-cli-pager cloudformation validate-template \ |
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.
IIRC we did something to not require putting --no-cli-pager
everywhere. Is it not working?
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.
ah, i just copied this straight from the makefile - it'll be running on CI instances, so maybe we wanna be double sure?
.buildkite/steps/launch.sh
Outdated
upload_location="s3://${s3_bucket}/${s3_key}" | ||
download_location="https://s3.amazonaws.com/${s3_bucket}/${s3_key}" | ||
|
||
aws s3 cp --content-type 'text/yaml' "build/aws-stack.yml" "$upload_location" |
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.
See my comment regarding using OIDC. It would be nice to do new things the new way.
For what needs to be done on this side, see https://github.com/buildkite/buildkite-agent-scaler/pull/82/files. TL;DR, the assume-role-with-web-identity plugin takes care of things.
--output text >"$keyfile" | ||
|
||
echo "Setting ownership of $keyfile to buildkite-agent..." | ||
chown buildkite-agent: "$keyfile" |
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.
We should probably chmod 400
it as well. Though, I think preventing writes is a fool's errand if you own the file. Perhaps it should be owned by root, and readable only by a buildkite-agent
group, which I don't think exists, but we should check.
This means it should be chmod 640
and owned by root:buildkite-agent
.
.buildkite/steps/launch.sh
Outdated
echo "--- Validating templates" | ||
make validate |
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.
Apparently, some customers use the Makefile after forking, so I think it would prevent a future support escalation if you maintain parity with the Makefile. If you do that, you may as well use it directly.
At the very least, you should remove the non-working recipes from the Makefile.
c02bc4d
to
5a35925
Compare
d5b5ceb
to
dde4d51
Compare
No description provided.