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.

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

ci: include additional AMI build resources #668

Merged
merged 3 commits into from
Jan 21, 2020
Merged

Conversation

jahkeup
Copy link
Member

@jahkeup jahkeup commented Jan 20, 2020

Issue #, if available:

#624

Description of changes:

This adds amiize.sh to the container image for use in the builder environment (in the pipeline's build projects). The build depends on the availability of the permissions as set in the cloudformation template which limits the process to, roughly speaking, "create only" actions.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@jahkeup jahkeup self-assigned this Jan 20, 2020
@jahkeup
Copy link
Member Author

jahkeup commented Jan 20, 2020

The deploy-stack helper tool is currently in history here (40891a9) but will not be included in a tidied-up/rebased PR branch history.

Copy link
Contributor

@zmrow zmrow left a comment

Choose a reason for hiding this comment

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

Just a few questions.

tools/infra/stacks/hack/deploy-stack Outdated Show resolved Hide resolved
tools/infra/stacks/hack/deploy-stack Outdated Show resolved Hide resolved
Comment on lines +196 to +209
- Sid: "keypairDecrypt"
Effect: Allow
Action:
- kms:Decrypt
Resource: !Sub "arn:${AWS::Partition}:kms:${AWS::Region}:${AWS::AccountId}:alias/aws/ssm"
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this policy invoked during testing? kms:Decrypt did not work for me when I tried using aliases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes! This was used by both ensure-key-pair and create-image-ami scripts when managing the /infra/ami-build/instance/key-pair parameter (which is a SecureString).

@@ -0,0 +1,193 @@
#!/usr/bin/env bash
##
## deploy-stack - Simplified 'cfn deploy' interface prioritizing iterative use
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome! Thanks for making this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Heh, you're welcome to use it! I've been cherry picking it where I've wanted to use it though I don't intend to commit this to the repository at this time. This lives on a pushed branch and probably could stand to be put in a Draft PR - if for nothing more than a symbol for discussion about larger stack orchestration and coordination.

Comment on lines 79 to 81
variables:
BUILD_IMAGE_ROOT: thar-x86_64-aws-k8s.img.lz4
BUILD_IMAGE_DATA: thar-x86_64-aws-k8s-data.img.lz4
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better if we define these environment variables for the CodeBuild project instead of here in the buildspec?

Copy link
Member Author

Choose a reason for hiding this comment

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

There was no rhyme or reason to these being place here this way, this is crumbs from me working through the last bit of duct taping needed to wire up the step. I've since moved these all into the Build Project definition. Read on for my thoughts here.

I think that we should use the Build Project to hold the variables as its near but not the lowest priority location referenced for environment variables at build time. This leaves some room for migrating project configuration by shifting variables higher up (to maintain an override) or lower (to help smooth over transitions of variables or their values). I'm basing this opinion entirely on the docs, which suggest that this is how the variables are sourced and given priority:

If an environment variable with the same name is defined in multiple places, the value is determined as follows:

  • The value in the start build operation call takes highest precedence. You can add or override environment variables when you create a build. For more information, see Run a Build in CodeBuild.

  • The value in the build project definition takes next precedence. You can add environment variables at the project level when you create or edit a project. For more information, see Create a Build Project in CodeBuild and Change a Build Project's Settings in CodeBuild .

  • The value in the build spec declaration takes lowest precedence.

from the build spec reference documentation

This would mean that for Pipeline driven builds, we have the follow ordering (highest preference first):

  1. Pipeline Action-Specified Environment Variables
  2. Backing-CodeBuild-Project Environment Variables
  3. Backing-CodeBuild-Project's Buildspec Environment Variables (in buildspec.yml or inlined)

So if that's the order, then I would say, yes - I think we should be using the Build Project to define our SOP-mode variables and use the other slots as needed in operations.

TimeoutInMinutes: 30
Source:
Type: CODEPIPELINE
BuildSpec: !Sub |
Copy link
Member Author

Choose a reason for hiding this comment

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

Hey, this isn't supposed to be here, will remove.

amiize.sh is required by `create-image-ami` for turning images in
on-disk raw filesystem image form into AMIs. Embedding amiize.sh allows
the stage to be decoupled from the input top level sources at runtime,
depending only on the builder environment to run against a set of disk
images as inputs.

Signed-off-by: Jacob Vallejo <[email protected]>
@jahkeup jahkeup marked this pull request as ready for review January 20, 2020 22:55
Copy link
Contributor

@zmrow zmrow left a comment

Choose a reason for hiding this comment

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

🚴

@jahkeup jahkeup merged commit 897de87 into develop Jan 21, 2020
@jahkeup jahkeup deleted the ami-build-support branch January 21, 2020 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants