host-containers-pipeline: buildspecs, pipeline cfn, refactor paths #590

etungsten · 2019-12-17T01:01:55Z

Issue #, if available: Fixes #514
Fixes #465

Description of changes:
Creates a host-container pipeline:

Adds a CloudFormation template for setting up three host-container pipelines
One for the thar-admin container, one for the thar-control container, one for the dogswatch container.
Modify dogswatch Makefile release-container target to tag the dogswatch image with latest tag
Adds buildspec for the build, test, deploy step in the host-container pipeline
Adds a version file, VERSION, for admin and control container folders to keep track of container version.
Adds CTR_VER build arguments to thar-admin and thar-control Dockerfiles to set up version env var.
Moves host containers to new paths

Testing:

Created CloudFormation stacks successfully using host-container-piplines.yml.
The pipelines are invoked correctly by pushing changes to the container-version file for thar-admin and thar-control as well as the Makefile for dogswatch (since that's where the dogswatch version lives)
The resultant images are pushed to external ECR image repositories tagged with their version + the git commit ID

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

tools/infra/buildspec/host-containers-pipeline-build.yml

extras/host-containers/thar-admin/container-version

tools/infra/buildspec/host-containers-pipeline-deploy.yml

tools/infra/stacks/pipelines/host-containers-pipelines.yml

etungsten · 2019-12-17T20:04:46Z

Addresses @zmrow 's comments.

Split the single cloudformation stack into three for the three pipelines.
Names with "Deploy" are now "BetaDeploy"
Renamed container-version file to just VERSION

etungsten · 2019-12-17T20:56:50Z

Created a separate buildspec for dogswatch beta deploy

zmrow

❄️

etungsten · 2019-12-20T22:56:24Z

New commit breaks out the CodeBuild projects for each of the pipeline steps into separate CloudFormation stack for each host container pipeline. The buildspec for those CodeBuild project are now inlined instead of being sourced at CodeBuild runtime from the repository.

extras/host-containers/thar-control/VERSION

jahkeup · 2019-12-23T22:37:13Z

extras/dogswatch/Makefile

-release-container: container
-	docker tag $(DOCKER_IMAGE_REF) $(DOCKER_IMAGE_REF_RELEASE)
+release-container:
+	docker build --network=host \


With the removed code, the Makefile target can reuse the prior image with the container dependency and lets developers iterate nicely (nicely to me at least :P). Can/should the buildspec use the container target and docker tag as needed for pipeline based release? Is there another goal in making a standalone target?

I couldn't use the container target in a CodeBuild buildspec because there is a dependency on git when setting DOCKER_IMAGE_REF and the CodeBuild source doesn't have the git repo information.
https://github.com/amazonlinux/PRIVATE-thar/blob/74e8bce8b4fac1f69da7f818451513b5e71c0760/extras/dogswatch/Makefile#L9

Oh good point, I think we can instead lift the git rev-parse into its own variable and overridde with the CI environment variables provided. That way the build will be less likely to diverge unexpectedly from the "base" build.

jahkeup · 2019-12-23T22:39:21Z

tools/infra/stacks/pipelines/dogswatch-container-pipeline-codebuild-projects.yml

@@ -0,0 +1,260 @@
+AWSTemplateFormatVersion: "2010-09-09"


I didn't see the conversation regarding the project/pipeline split, so forgive me if this was discussed elsewhere:

What's the reasoning behind the multiple stacks for a Pipeline and its constituent CodeBuild-Stage Projects? Can these be combined into a single logical namespace - a single CloudFormation Stack? Were there ordering/dependency issues encountered that lead to this split?

This was done to address #590 (comment).
Having the pipelines in separate stacks allows for flexibility down the road if we decide to separate out each host container to their own git repository.

The CodeBuild projects were separated out mostly due to wanting the buildspec to be inlined instead of being sourced during CodeBuild runtime. This alleviates the security concern of running arbitrary code in our CI/CD pipeline when a PR has changes against the buildspec files.

Gotcha, my thoughts as I read over that were that we'd have separate stacks for each pipeline, not that it'd be each pipeline and another for their supporting resources. I don't think merging the two prevents the Stage's definitions from containing the same inlined buildspec, does it?

You're right, there's nothing that prevents us from doing that. I went with this mostly to reduce duplication and also thinking that it would be easier to manage.

If we merged it then both admin and control container pipeline template would need their own set of codebuild project definitions as opposed to having a single template that can be deployed for either and cross referenced within the pipeline stack.

jahkeup · 2019-12-23T22:43:41Z

tools/infra/stacks/pipelines/dogswatch-container-pipeline-codebuild-projects.yml

+                - docker load -i ${CODEBUILD_SRC_DIR_BuildArtifact}/${CONTAINER_IMAGE}.tar.gz
+                - $(aws ecr get-login --registry-ids ${ECR_ACCT_ID} --no-include-email --region us-west-2)
+                - IMG_VER=$(grep "DOGSWATCH_VERSION=" ${EXTRAS_DIR}/dogswatch/Makefile | sed 's:.*=::')
+                - docker tag "${CONTAINER_IMAGE}":latest "${ECR_ACCT_ID}".dkr.ecr.us-west-2.amazonaws.com/"${CONTAINER_IMAGE}":"${IMG_VER}_${CODEBUILD_RESOLVED_SOURCE_VERSION}"


We'll want to shorten the SHA at least, otherwise this tag will be very long and annoying to carry around in issues and discussion. I'm torn on wanting bare short-sha tagged images dogswatch:abcdef12 vs the version adorned dogswatch:v0.1.1-abcdef12; I'm curious about what you think about this, what thoughts did you have that landed you on the adorned tag name?

nit: I've observed _ to be unconventional and uncommon to use when compared to - in image tags.

Suggested change

- docker tag "${CONTAINER_IMAGE}":latest "${ECR_ACCT_ID}".dkr.ecr.us-west-2.amazonaws.com/"${CONTAINER_IMAGE}":"${IMG_VER}_${CODEBUILD_RESOLVED_SOURCE_VERSION}"

- docker tag "${CONTAINER_IMAGE}":latest "${ECR_ACCT_ID}".dkr.ecr.us-west-2.amazonaws.com/"${CONTAINER_IMAGE}":"${IMG_VER}-${CODEBUILD_RESOLVED_SOURCE_VERSION:0:8}"

I think the idea was that we want release candidates in a Beta ECR repo before pushing to a prod ECR repository.

Since these containers are versioned, I thought it would make sense to tag them with the container version and the source commit SHA for clarity during the manual approval step (not part of this PR) when pushing out to the prod ECR repo.

I think the idea was that we want release candidates in a Beta ECR repo before pushing to a prod ECR repository.

To me, this suggests that the unadorned sha be chosen as, if used anywhere, is less likely to misrepresent its RC vs. !RC status as there's no version to misinterpret (as RC or release build, between which there hopefully wouldn't be a difference). The bare SHA suggests nothing more than a locked version aside from its nearest released tagged build.

Additionally, I think the unadorned would also aid the RC strategy in allowing a later optimization where a pre-build docker pull $registry/dogswatch:$CI_SHORT_SHA || : can be run prior to the "release" build.

Since these containers are versioned, I thought it would make sense to tag them with the container version and the source commit SHA for clarity during the manual approval step (not part of this PR) when pushing out to the prod ECR repo.

I do see the benefit of being able to sight-read + spot-check the version being released, but I also think the chances of mucking up a SHA and releasing the wrong one isn't high - especially if we use clean room release environments (which I suspect is what will happen).

I am leaning towards the unadorned, bare short-sha as its still fully representative of the build at hand. Either way, I think we should copy in the VERSION file to the container image to check versions without relying on running the container - a task for another time!

tools/infra/stacks/pipelines/dogswatch-container-pipeline.yml

etungsten · 2019-12-23T23:46:48Z

Addresses a subset of @jahkeup 's comments.
Down-scope's the pipeline service role permissions.
Adds an adjacent version file to dogswatch
Tag images with a short sha of the source commit when pushing out to beta ECR

jahkeup

Left my thoughts and feedback for these changes - there's some suggestions that are easy enough to change later if we end up wanting to and those should be quickly deferred in tracking issues to keep us moving on 👍

jahkeup · 2020-01-10T00:07:06Z

extras/host-containers/thar-admin/Dockerfile

@@ -21,6 +21,11 @@ RUN cp bash /opt/bash

 FROM amazonlinux:2

+ARG CTR_IMG_VER


I think this naming may be confused to have something to do with containerd (with its CLI called ctr) and I think based on its usage here (which seems to be unused in the image) that a Image Label might be more appropriate for this use case. There are some pre-defined labels in the OCI image-spec, but I'm not 100% those are the labels we'd want to use exactly. I think we should start with those annotations (specifically org.opencontainers.image.version for this case) and rework them later if we determine they're not appropriate for use here.

Nice. One last nit here: let's also update the remaining CTR_*'s - how about making them align with the label and simplify: IMAGE_VERSION?

extras/host-containers/thar-control/Dockerfile

jahkeup · 2020-01-10T00:09:40Z

tools/infra/stacks/pipelines/admin-container-pipeline.yml

+    Type: String
+    Default: amazonlinux
+    AllowedPattern: "[A-Za-z0-9-]+"
+  GitHubSecretToken:


I really don't want this to make its way into the stacks because this ties the Pipeline to the user that sets it up (we'll want to use a robot if we stick with this) and because then credentials are having to be input and consumed in the clear (at some points, I do know they're stored "not in clear text").

Can we "stub" in the correct values and connect the "inputs" manually with the right approvals?

Does having the permissions already present in the account (ie: the oauth2 authorization flow has been performed already) obviate the need to provide Personal Access Tokens?

* Can we "stub" in the correct values and connect the "inputs" manually with the right approvals?

I'm not sure if I understand. Can you elaborate on what to stub and what inputs to connect?

* Does having the permissions already present in the account (ie: the oauth2 authorization flow has been performed already) obviate the need to provide Personal Access Tokens?

No, to create the CodePipeline itself in CFN and have a github repository as a source, a AWS::CodePipeline::Webhook is requried with GITHUB_HMAC authentication which needs the personal access token. This differs from the manual CodePipeline github source set up which doesn't require the access token as it set up the webhook through some github web portal thing.

I'm not sure if I understand. Can you elaborate on what to stub and what inputs to connect?

Sure: for example, when testing stacks that want to hook up to the repository (for setting up Webhooks and such) I use another repository to initially set them up and then switch the repository to what I really want later (manually in the console).

This approach probably isn't maintainable and would cause all kinds of issues when these templates are regularly deployed automatically.

No, to create the CodePipeline itself in CFN and have a github repository as a source, a AWS::CodePipeline::Webhook is requried with GITHUB_HMAC authentication which needs the personal access token.

Oh, so it this a limitation of provisioning with CloudFormation specifically? I was able to use the oauth2 flow for setting up a pipeline - but that was with the console when we were feeling out the solution. I guess we'll want to get ourselves a bot user setup soon with the right permissions, that's kinda scary.. 👻

Oh, so it this a limitation of provisioning with CloudFormation specifically? I was able to use the oauth2 flow for setting up a pipeline - but that was with the console when we were feeling out the solution. I guess we'll want to get ourselves a bot user setup soon with the right permissions, that's kinda scary.. ghost

Yup! I was pretty dismayed when I realized this as well.

jahkeup · 2020-01-10T00:12:32Z

tools/infra/stacks/pipelines/admin-container-pipeline.yml

+                  - 'sns:*'
+                  - 'cloudformation:*'
+                  - 'ecs:*'
+                Resource: '*'


The permissions granted should be limited to the related resources, such as: bucket(s), ASGs, ELBs, and the other services' resources.

I monitored the access advisor on this role, and none of these actions were ever invoked, so I'll go ahead and remove them

tools/infra/stacks/pipelines/admin-container-pipeline.yml

tools/infra/stacks/pipelines/dogswatch-container-pipeline-codebuild-projects.yml

tools/infra/stacks/pipelines/host-container-pipeline-codebuild-projects.yml

etungsten · 2020-01-14T00:50:28Z

Addresses @jahkeup 's comments.

zmrow · 2020-01-17T16:56:29Z

As far as I can tell, this is ready to push over the line, minus a few things we should create issues for. @etungsten @jahkeup is there anything else outstanding?

Modify dogswatch makefile `release-container` target to tag the dogswatch image with `latest` Adds buildspec for the build, test, deploy step in the host-container pipeline Adds a version file, `VERSION`, for admin and control container folders to keep track of container version. Moved host containers to new paths, add CTR_VER build arguments to `thar-admin` and `thar-control`

jahkeup · 2020-01-17T22:44:13Z

extras/host-containers/thar-admin/Dockerfile

@@ -21,6 +21,11 @@ RUN cp bash /opt/bash

 FROM amazonlinux:2

+ARG CTR_IMG_VER


Nice. One last nit here: let's also update the remaining CTR_*'s - how about making them align with the label and simplify: IMAGE_VERSION?

jahkeup · 2020-01-17T22:48:00Z

tools/infra/stacks/pipelines/control-container-pipeline.yml

+              Service:
+                - codepipeline.amazonaws.com
+            Action: 'sts:AssumeRole'
+      Path: /


Let's place this under the stack's namespace:

Suggested change

Path: /

Path: !Sub "/${AWS::StackName}/"

jahkeup · 2020-01-17T22:48:16Z

tools/infra/stacks/pipelines/admin-container-pipeline.yml

+              Service:
+                - codepipeline.amazonaws.com
+            Action: 'sts:AssumeRole'
+      Path: /


Let's place this under the stack's namespace:

Suggested change

Path: /

Path: !Sub "/${AWS::StackName}/"

jahkeup · 2020-01-17T22:49:49Z

tools/infra/stacks/pipelines/dogswatch-container-pipeline-codebuild-projects.yml

+            Effect: Allow
+            Principal:
+              Service: codebuild.amazonaws.com
+      Path: /


Let's place this under the stack's namespace:

Suggested change

Path: /

Path: !Sub "/${AWS::StackName}/"

jahkeup · 2020-01-17T22:51:16Z

tools/infra/stacks/pipelines/dogswatch-container-pipeline-codebuild-projects.yml

+                Effect: Allow
+                Resource:
+                  - !GetAtt PipelineArtifactBucket.Arn
+                  - Fn::Join:


I realized I was being silly (where you copied this from) and fixed this in later revs of those other stacks, this can be a simpler !Sub:

Suggested change

- Fn::Join:

- !Sub "${PipelineArtifactBucket.Arn}/*"

jahkeup · 2020-01-17T22:52:55Z

tools/infra/stacks/pipelines/dogswatch-container-pipeline-codebuild-projects.yml

+              # Path to infra tooling directory.
+              INFRA_DIR: "./tools/infra"


This can be removed with the update to use the builder container.

jahkeup · 2020-01-17T22:55:12Z

tools/infra/stacks/pipelines/dogswatch-container-pipeline-codebuild-projects.yml

+            files:
+              - '*.tar.gz'
+            secondary-artifacts:
+              meta:


There's no config for gathering the "meta" secondary artifacts, you might want to drop this as I don't think we'll need it here for the present (the write-build-meta command can be removed too).

jahkeup · 2020-01-17T22:56:16Z

tools/infra/stacks/pipelines/dogswatch-container-pipeline-codebuild-projects.yml

+                - . "${INFRA_DIR}/env/lib/environment-setup"
+            pre_build:
+              commands:
+                - write-build-meta


See the artifact comment below, I think this should be removed and have environment-report in its place - that'll print the details of the docker versions used and such that may be helpful when later debugging.

jahkeup · 2020-01-17T22:56:36Z

tools/infra/stacks/pipelines/dogswatch-container-pipeline-codebuild-projects.yml

+      ServiceRole: !GetAtt BuildRole.Arn
+      Source:
+        BuildSpec: |
+          version: 0.2


I'd make similar edits as the other build specs here.

jahkeup · 2020-01-17T23:00:56Z

tools/infra/stacks/pipelines/dogswatch-container-pipeline-codebuild-projects.yml

+        PrivilegedMode: true
+        Type: LINUX_CONTAINER
+        EnvironmentVariables:
+          - Name: 'ECR_ACCT_ID'


The builder container has amazon-ecr-credential-helper setup in it, so there shouldn't be need to reference the Account ID directly, instead the script can simply:

docker build -t $ECR_IMAGE_REPOSITORY:$TAG ... docker push $ECR_IMAGE_REPOSITORY:$TAG

etungsten · 2020-01-21T01:24:24Z

Addresses @jahkeup 's comments

jahkeup

There's a couple things that we can come back and fixup later, but one question that may or may not need fixing - until then, let's get container images movin' :D

extras/host-containers/thar-control/Makefile

tools/infra/stacks/pipelines/dogswatch-container-pipeline-codebuild-projects.yml

jahkeup · 2020-01-21T21:29:17Z

tools/infra/stacks/pipelines/host-container-pipeline-codebuild-projects.yml

+                - mkdir -p "${IMAGES_DIR}"
+            build:
+              commands:
+                - SHORT_SHA="$(head -c 8 <<< "$CODEBUILD_RESOLVED_SOURCE_VERSION")"


I forgot, did this end up working inside of the container without switching over to bash?

Yes, it works now that we're in the build container, nothing else necessary. :)

I did a quick run to figure out why, I didn't confirm this exactly, but my guess is that whatever the /bin/sh executable was (emulating or otherwise) not backed by bash - the builder environment's /bin/sh is emulated by bash. I think the emulation isn't fully insulated from the bash side and permits this despite it not being a feature of sh (I think?).

Breaks out the Codebuild projects for the codepipeline steps in a separate CloudFormation stacks that output references. Use those references in the CodePipeline stacks. Deletes the buildspec files as they are now inline in the CloudFormation template

etungsten · 2020-01-21T22:09:46Z

Fixed an issue in the policy where the resources were not being specified correctly, tested and pipeline kick-off correctly again.

etungsten requested review from jahkeup, zmrow and jhaynes December 17, 2019 01:02

etungsten force-pushed the host-containers-pipeline branch from f34b6a2 to 16d8009 Compare December 17, 2019 01:17

zmrow reviewed Dec 17, 2019

View reviewed changes

etungsten force-pushed the host-containers-pipeline branch from 16d8009 to c2d430f Compare December 17, 2019 20:02

etungsten force-pushed the host-containers-pipeline branch 4 times, most recently from ba401d4 to 8b7d3d8 Compare December 17, 2019 20:45

zmrow approved these changes Dec 17, 2019

View reviewed changes

etungsten force-pushed the host-containers-pipeline branch from 13d162a to 74e8bce Compare December 20, 2019 22:49

etungsten changed the title ~~host-containers-pipeline: buildpsecs, pipeline cfn, refactor paths~~ host-containers-pipeline: buildspecs, pipeline cfn, refactor paths Dec 20, 2019

etungsten removed the request for review from jhaynes December 23, 2019 19:15

jahkeup reviewed Dec 23, 2019

View reviewed changes

etungsten force-pushed the host-containers-pipeline branch from 74e8bce to 0e49e3d Compare December 23, 2019 23:45

etungsten force-pushed the host-containers-pipeline branch 4 times, most recently from 898f7ae to fbafc65 Compare December 24, 2019 00:45

etungsten force-pushed the host-containers-pipeline branch 3 times, most recently from 158bf1a to 233dd73 Compare January 9, 2020 23:53

jahkeup reviewed Jan 10, 2020

View reviewed changes

etungsten force-pushed the host-containers-pipeline branch 2 times, most recently from 56786f9 to c71dc25 Compare January 11, 2020 00:00

etungsten force-pushed the host-containers-pipeline branch 3 times, most recently from 97731d7 to 6185170 Compare January 14, 2020 00:48

etungsten force-pushed the host-containers-pipeline branch from 6185170 to ab1c01d Compare January 20, 2020 18:23

jahkeup reviewed Jan 20, 2020

View reviewed changes

etungsten force-pushed the host-containers-pipeline branch from ab1c01d to be1e515 Compare January 21, 2020 01:24

etungsten force-pushed the host-containers-pipeline branch from be1e515 to 3918d2d Compare January 21, 2020 01:42

jahkeup approved these changes Jan 21, 2020

View reviewed changes

etungsten force-pushed the host-containers-pipeline branch from 3918d2d to 4391913 Compare January 21, 2020 21:42

etungsten force-pushed the host-containers-pipeline branch from 4391913 to e571695 Compare January 21, 2020 22:09

etungsten merged commit 3273aa9 into develop Jan 21, 2020

etungsten deleted the host-containers-pipeline branch January 21, 2020 22:18

	- docker tag "${CONTAINER_IMAGE}":latest "${ECR_ACCT_ID}".dkr.ecr.us-west-2.amazonaws.com/"${CONTAINER_IMAGE}":"${IMG_VER}_${CODEBUILD_RESOLVED_SOURCE_VERSION}"
	- docker tag "${CONTAINER_IMAGE}":latest "${ECR_ACCT_ID}".dkr.ecr.us-west-2.amazonaws.com/"${CONTAINER_IMAGE}":"${IMG_VER}-${CODEBUILD_RESOLVED_SOURCE_VERSION:0:8}"

		@@ -21,6 +21,11 @@ RUN cp bash /opt/bash

		FROM amazonlinux:2

		ARG CTR_IMG_VER

		# Path to infra tooling directory.
		INFRA_DIR: "./tools/infra"

host-containers-pipeline: buildspecs, pipeline cfn, refactor paths #590

host-containers-pipeline: buildspecs, pipeline cfn, refactor paths #590

Conversation

etungsten commented Dec 17, 2019 • edited Loading

etungsten commented Dec 17, 2019

etungsten commented Dec 17, 2019

zmrow left a comment

Choose a reason for hiding this comment

etungsten commented Dec 20, 2019

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

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

etungsten commented Dec 23, 2019

jahkeup left a 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

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

etungsten commented Jan 14, 2020

zmrow commented Jan 17, 2020

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

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

etungsten commented Jan 21, 2020

jahkeup left a 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

etungsten commented Jan 21, 2020 • edited Loading

etungsten commented Dec 17, 2019 •

edited

Loading

etungsten commented Jan 21, 2020 •

edited

Loading