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

Jump to bottom

App Infra pipelines #337

Merged
merged 10 commits into from
Feb 23, 2021
Merged

App Infra pipelines #337

merged 10 commits into from
Feb 23, 2021

Conversation

bharathkkb
Copy link
Member

@bharathkkb bharathkkb commented Feb 12, 2021 •
edited

fixes #316

  • Adds a common build project per BU in 4-projects
  • Accepts a list of apps and per app creates a CSR, 2x pipeline (plan/apply), TF runner image, artifact and state bucket
  • single_project which wraps project factory can optionally give cloudbuild_sa role to impersonate the newly created project_sa (with a list of roles as needed)

TODO:

  • add 5-app-infra to showcase application infra deployment using this pipeline
  • granular roles for configuring CSR/pipelines access

@bharathkkb bharathkkb requested review from rjerrems and a team as code owners February 12, 2021 15:51
@bharathkkb bharathkkb marked this pull request as draft February 12, 2021 15:51
@bharathkkb bharathkkb marked this pull request as ready for review February 12, 2021 18:38
@bharathkkb bharathkkb mentioned this pull request Feb 12, 2021
Closed
rjerrems
rjerrems reviewed Feb 17, 2021
View reviewed changes
4-projects/modules/infra_pipelines/cloudbuild_builder/Dockerfile
@@ -0,0 +1,44 @@
# Copyright 2021 Google LLC
Copy link
Collaborator

@rjerrems rjerrems Feb 17, 2021 •
edited

Choose a reason for hiding this comment

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

It might be worthwhile thinking about sharing the image/s created in bootstrap rather than building specific image per app infra project, so it is consistent throughout the foundation and less to maintain.

Copy link
Member Author

Choose a reason for hiding this comment

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

ack, I have opened terraform-google-modules/terraform-google-bootstrap#91 to track

rjerrems
rjerrems reviewed Feb 18, 2021
View reviewed changes
build/cloudbuild-tf-apply.yaml
@@ -22,8 +22,11 @@ steps:
args:
- -c
- |
echo "Setting up gcloud for impersonation"
gcloud config set auth/impersonate_service_account ${_TF_SA_EMAIL}
tf_sa_email=${_TF_SA_EMAIL}
Copy link
Collaborator

Choose a reason for hiding this comment

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

we could probably just use the substitution directly, rather than setting to a variable first

Copy link
Member Author

Choose a reason for hiding this comment

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

I was initially trying to use it directly like if [[ -n ${_TF_SA_EMAIL} ]]; then , but if _TF_SA_EMAIL is not set (in the case of app infra pipelines) cloudbuild fails to start build and complains about a missing substitution. I tried this as a workaround which seemed to work.

rjerrems
rjerrems reviewed Feb 18, 2021
View reviewed changes
build/cloudbuild-tf-plan.yaml
echo "Setting up gcloud for impersonation"
gcloud config set auth/impersonate_service_account ${_TF_SA_EMAIL}
tf_sa_email=${_TF_SA_EMAIL}
if [[ -n ${tf_sa_email} ]]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as comment for apply.yaml

rjerrems
rjerrems reviewed Feb 18, 2021
View reviewed changes
build/cloudbuild-tf-apply.yaml
@@ -22,8 +22,11 @@ steps:
args:
- -c
- |
echo "Setting up gcloud for impersonation"
gcloud config set auth/impersonate_service_account ${_TF_SA_EMAIL}
tf_sa_email=${_TF_SA_EMAIL}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also it looks like this substitution won't be available for app infra triggers?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct, that was my reason for wrapping in a if statement

@rjerrems
Copy link
Collaborator

Overall looking good @bharathkkb, nice work! A few small things to look at, not sure whether we need to build an image per app infra project and could cause some confusion / drift vs the image used for the overall foundation.

@bharathkkb
Copy link
Member Author

Thanks for the review @rjerrems
I have opened an issue terraform-google-modules/terraform-google-bootstrap#91 to store the bootstrap runner image in a GAR which would allow for us to have more restrictive ACLs. Another reason was in case app developers wanted a custom TF image with additional tooling/custom providers preinstalled.

@bharathkkb
Copy link
Member Author

@rjerrems I will expose an option to use the foundation runner image in GAR once we have a new bootstrap release

rjerrems
rjerrems approved these changes Feb 23, 2021
View reviewed changes
Copy link
Collaborator

@rjerrems rjerrems left a comment

Choose a reason for hiding this comment

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

LGTM - Lets just confirm tests are working prior to merge.

@bharathkkb bharathkkb merged commit a3a413d into develop Feb 23, 2021
@bharathkkb bharathkkb deleted the infra-pipelines branch February 23, 2021 16:51
@chris-ingrisani chris-ingrisani mentioned this pull request Feb 23, 2021
Closed
@chris-ingrisani chris-ingrisani mentioned this pull request Mar 26, 2021
Closed
bharathkkb added a commit that referenced this pull request Mar 30, 2021
@bharathkkb
feat: App Infra pipelines (#337)
a83bce3 
* initial impl

* add tests

* fmt

* address comments

* fix checksums for 0.13.6

* fix validator version

* update readmes

* update docs
bharathkkb added a commit that referenced this pull request Mar 30, 2021
@bharathkkb
feat: App Infra pipelines (#337)
fccce96 
* initial impl

* add tests

* fmt

* address comments

* fix checksums for 0.13.6

* fix validator version

* update readmes

* update docs
bharathkkb added a commit that referenced this pull request Mar 30, 2021
@bharathkkb
feat: App Infra pipelines (#337)
47ebf9e 
* initial impl

* add tests

* fmt

* address comments

* fix checksums for 0.13.6

* fix validator version

* update readmes

* update docs
bharathkkb added a commit that referenced this pull request Mar 31, 2021
@bharathkkb
feat: App Infra pipelines (#337)
c3b19e8 
* initial impl

* add tests

* fmt

* address comments

* fix checksums for 0.13.6

* fix validator version

* update readmes

* update docs
@bharathkkb bharathkkb mentioned this pull request Apr 9, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rjerrems rjerrems rjerrems approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@bharathkkb @rjerrems