-
-
Notifications
You must be signed in to change notification settings - Fork 118
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
Add pudl usage metrics gcp infrastructure #3841
Conversation
- Mount pudl-usage-metrics db to superset cloud run - Create a new bucket for raw usage metrics archives - Give github action service account permision to write to the bucket
resource "google_secret_manager_secret" "pudl_usage_metrics_db_connection_string" { | ||
secret_id = "pudl-usage-metrics-db-connection-string" | ||
replication { | ||
auto {} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured I'd save the connection string in case we need to reconnect the db to superset.
resource "google_service_account" "usage_metrics_archiver" { | ||
account_id = "usage-metrics-archiver" | ||
display_name = "PUDL usage metrics archiver github action service account" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did create a service account key for the GitHub action in the business repo. @jdangerx would love a WIF tutorial soon!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've forgotten everything I know about WIF but could re-learn it!
…rchive bucket and list buckets, previous commit was missing a role
} | ||
|
||
resource "google_storage_bucket_iam_member" "usage_metrics_etl_gcs_iam" { | ||
for_each = toset(["roles/storage.legacyBucketReader", "roles/storage.objectViewer"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find a non legacy role that gives a principle the storage.buckets.get
and storage.objects.get
permissions. Seems like the GCS python client wants both to access objects in a bucket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably fine. If we want to switch to non-legacy roles, it looks like we could give roles/storage.objectUser
for objects.get
and the confusingly named roles/storage.insightsCollectorService
gets you buckets.get
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks fine, just wondering if we should manage the s3-logs bucket via terraform as well. Other than this you're good to go!
resource "google_service_account" "usage_metrics_archiver" { | ||
account_id = "usage-metrics-archiver" | ||
display_name = "PUDL usage metrics archiver github action service account" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've forgotten everything I know about WIF but could re-learn it!
} | ||
|
||
resource "google_storage_bucket_iam_member" "usage_metrics_etl_gcs_iam" { | ||
for_each = toset(["roles/storage.legacyBucketReader", "roles/storage.objectViewer"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably fine. If we want to switch to non-legacy roles, it looks like we could give roles/storage.objectUser
for objects.get
and the confusingly named roles/storage.insightsCollectorService
gets you buckets.get
.
resource "google_storage_bucket_iam_member" "usage_metrics_etl_s3_logs_gcs_iam" { | ||
for_each = toset(["roles/storage.legacyBucketReader", "roles/storage.objectViewer"]) | ||
|
||
bucket = "pudl-s3-logs.catalyst.coop" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we manage this bucket via TF too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably! How can we manage a resource in terraform that has already been created in the UI? Also, we should probably move the contents of pudl-s3-logs.catalyst.coop
to pudl-usage-metrics-archives.catalyst.coop/s3/
for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the documentation about handling "resource drift"!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need 4 different name components on that bucket? Is there a non-archives bucket that we need to differentiate it from? Do we foresee having non-PUDL usage metrics that would need to be stored somewhere else? Or could we put all usage metrics data under usage-metrics.catalyst.coop
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we probably don't need the pudl
part because the bucket lives in the pudl-catalyst-cooperative
GCP project. It would be nice to keep archive
because I could imagine having a bucket to store the outputs of our usage metric ETL to parquet files if we ever move to Big Query. How about usage-metrics-archive.catalyst.coop
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bendnorman Would love to coordinate a name change with the flight of PRs in pudl-usage-metrics
as much as possible, so just let me know what you're thinking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'll keep it as is for now but I created an issue to rename it down the line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also created an issue for moving the pudl-s3-logs.catalyst.coop
bucket.
@bendnorman A question from @jdangerx on this (Tldr - do we need to terraform the account credentials set up on the daily raw logs archiver?) |
I think, if the name of the bucket is still under discussion & we're considering moving buckets, then we should just do that as a separate PR and get this merged in. |
Overview
pudl-usage-metrics-db
to Superset cloud run instanceTesting
The usage metrics data can be viewed in superset and the github metrics are being saved to the new bucket.
To-do list