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

Terraform for GCP CI base #144

Merged
merged 6 commits into from
Aug 20, 2024
Merged

Conversation

AlCutter
Copy link
Collaborator

@AlCutter AlCutter commented Aug 15, 2024

This PR adds support for spinning up an instance of the example personality on GCP.

This is intended to be used for CI tests.

Towards #7

@codecov-commenter
Copy link

codecov-commenter commented Aug 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 35.28%. Comparing base (46ec9c2) to head (9c1ec4b).
Report is 72 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #144      +/-   ##
==========================================
- Coverage   35.80%   35.28%   -0.53%     
==========================================
  Files          16       33      +17     
  Lines        1363     2874    +1511     
==========================================
+ Hits          488     1014     +526     
- Misses        801     1750     +949     
- Partials       74      110      +36     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AlCutter AlCutter force-pushed the gcp_tf_ci_base branch 29 times, most recently from b94d163 to cc921d2 Compare August 19, 2024 13:57
@AlCutter AlCutter force-pushed the gcp_tf_ci_base branch 7 times, most recently from 5e05e32 to 179a1fe Compare August 19, 2024 15:47
@AlCutter AlCutter changed the title WIP: GCP CI base Terraform for GCP CI base Aug 19, 2024
@AlCutter AlCutter force-pushed the gcp_tf_ci_base branch 4 times, most recently from db5632a to b20a73e Compare August 20, 2024 15:28
@AlCutter AlCutter marked this pull request as ready for review August 20, 2024 15:37
@AlCutter AlCutter requested a review from mhutchinson August 20, 2024 15:37
@@ -0,0 +1,16 @@
terraform {
source = "${get_repo_root()}/deployment/modules/gcp//example-gcp"
Copy link
Contributor

Choose a reason for hiding this comment

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

This double slash is a bit weird?

Copy link
Collaborator Author

@AlCutter AlCutter Aug 20, 2024

Choose a reason for hiding this comment

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

This is how you tell terraform about module "boundaries", with the double slash it brings in anything under there, without them it fails when you try to reference the sibling gcs from within the example-gc module.

This is a bit of a dusty corner in terraform, but it's why e.g. you had to put double slashes in for your distributor config which referenced other modules: https://github.com/transparency-dev/distributor/blob/main/deployment/modules/distributor/main.tf#L110

project_id = get_env("GOOGLE_PROJECT", "trillian-tessera")
location = get_env("GOOGLE_REGION", "us-central1")
base_name = get_env("TESSERA_BASE_NAME", "example-gcp")
base_name = get_env("TESSERA_BASE_NAME", "${path_relative_to_include()}-example-gcp")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use $env here instead of the path calculation? It'd be clearer for a human reading it, even if the result is the same.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I assumed I couldn't, but rather unexpectedly it seems like you can (or at least ${local.env} you can)...

}

locals {
env = path_relative_to_include()
project_id = get_env("GOOGLE_PROJECT", "trillian-tessera")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: alignment of = has got wonky.

remote_state {
backend = "gcs"

config = {
project = local.project_id
location = local.location
bucket = "${local.project_id}-${local.base_name}-terraform-state"
prefix = "${path_relative_to_include()}/terraform.tfstate"
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, using ${local.env} would be clearer, IMO.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

##
## This will configure all the storage infrastructure required to run a Tessera log on GCP.
module "gcs" {
source = "..//gcs"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the double slash needed here? I think this is one of those cases where there is something needed about the 2 slashes but best to check.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yup, same as above.

Comment on lines 118 to 122
containers {
image = "us-docker.pkg.dev/cloud-ops-agents-artifacts/cloud-run-gmp-sidecar/cloud-run-gmp-sidecar:1.0.0"
name = "collector"
depends_on = ["example-gcp"]
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This does the prometheus integration into GCP monitoring. Not sure if you need it in the example?

It could do with some comments put through here for things like this though, or people will just steal all of this whether it's needed or not. The curse of writing examples that try to strike a balance between "hello world" and showing actual real-life deployment.

"--logtostderr",
"--v=1",
"--bucket=${module.gcs.log_bucket.id}",
"--spanner=projects/${var.project_id}/instances/${module.gcs.log_spanner_instance.name}/databases/${module.gcs.log_spanner_db.name}",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: You could consider evaluation this monster as a local, and even exporting it as an output variable. It would make this line shorter and thus easier to eyeball the flags.

Comment on lines +53 to +56
resource "google_service_account" "cloudrun_service_account" {
account_id = "cloudrun-${var.env}-sa"
display_name = "Service Account for Cloud Run (${var.env})"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't strictly necessary as there's a default service account you could use. It's good practice though. We should probably write in some docs our approach to the example on how to balance best practice vs minimalism.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, guidance is a good idea. I'll merge this as-is while we chew on that.

@AlCutter AlCutter merged commit 38226b6 into transparency-dev:main Aug 20, 2024
8 checks passed
@AlCutter AlCutter deleted the gcp_tf_ci_base branch August 20, 2024 16:53
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