Break ground on the GCP implementation #29
Conversation
AlCutter
force-pushed
the
gcp_impl
branch
19 times, most recently
from
51c988a to
04e290f
Compare
| // expected layout of a tile-based log. | ||
| // | ||
| // A CloudSQL database provides a transactional mechanism to allow multiple | ||
| // frontends to safely update the contents of the log. | ||
| package gcp |
There was a problem hiding this comment.
Maybe add a TODO to reconsider the package naming here. You've taken gcp which is reasonable at the moment, but this is a very specific strategy within the GCP umbrella that may cause naming confusion later on. I don't think it's worth stalling to bikeshed this now, but I think a note that this isn't set in stone could be useful for someone approaching this before v1.0. We can remove the TODO if we haven't cared to change it before a 1.0.
storage/gcp/gcp.go
Outdated
| Bucket string | ||
| // Spanner is the GCP resource URI of the spanner database instance to use. | ||
| Spanner string | ||
| // DBUser is the username for accessing the CloudSQL database. |
There was a problem hiding this comment.
Your vestigial approach is showing.
storage/gcp/gcp.go
Outdated
| @@ -13,14 +13,145 @@ | |||
| // limitations under the License. | |||
|
|
|||
| // Package gcp contains a GCP-based storage implementation for Tessera. | |||
| // | |||
| // This storage implementation uses GCS for long-term storage and serving of | |||
| // entry bundles and log tiles, and CloudSQL for coordinating updates to GCS | |||
storage/gcp/gcp.go
Outdated
| if err := initDB(ctx, dbPool); err != nil { | ||
| return nil, fmt.Errorf("failed to init DB: %v", err) | ||
| } |
There was a problem hiding this comment.
The asymmetry between this initDB being a static function, and bucketExists being a method bothers me. Particularly as this means we check the DB is legit before creating the Storage, and then check the other after. Can we reconcile them to be the same form? Looks easiest to make initDB be a private method on Storage and do it after we construct it?
| @@ -0,0 +1,4 @@ | |||
| output "log_bucket" { | |||
There was a problem hiding this comment.
Do you want to output the spanner resource too as that's needed for the binary?
| processing_units = 100 | ||
| } | ||
|
|
||
| resource "google_spanner_database" "log_db" { |
There was a problem hiding this comment.
We may want to look at using something like https://github.com/golang-migrate/migrate/tree/master/database/spanner to manage this when we do it for realz. This will be painful if this schema ever needs to be updated. I used to mysql version of this library for the experiments and it was cool.
deployment/modules/gcs/main.tf
Outdated
|
|
||
| # Services | ||
| resource "google_project_service" "serviceusage_googleapis_com" { | ||
| service = "serviceusage.googleapis.com" |
There was a problem hiding this comment.
Consider https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/google_project_service#disable_on_destroy - I've used this elsewhere as it seems safer.
deployment/modules/gcs/main.tf
Outdated
|
|
||
| resource "google_service_account" "log_writer" { | ||
| account_id = "${var.base_name}-writer" | ||
| display_name = "Log writer service account" |
There was a problem hiding this comment.
| display_name = "Log writer service account" | |
| display_name = "Transparency log writer service account" |
Just to put it in somewhere that this isn't metrics/stackdriver logs, but tlogs.
| project_id = "trillian-tessera" | ||
| location = "us-central1" | ||
| base_name = "example-gcs" | ||
| } |
There was a problem hiding this comment.
nit: Formatting is a bit dodgy.
| locals { | ||
| project_id = "trillian-tessera" | ||
| location = "us-central1" | ||
| base_name = "example-gcs" |
There was a problem hiding this comment.
nit: maybe intentional, but the one character difference between this and the directory name is a bit offputting (gcp vs gcs).
This PR adds a very skeleton-like outline of a the GCS storage implementation, and an example personality which instantiates it.
Toward #23