-
Notifications
You must be signed in to change notification settings - Fork 360
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
GCP Batch Google Secret Manager [AN-193] #7573
Conversation
echo "hello" | ||
} | ||
runtime { | ||
docker: "broadinstitute/cloud-cromwell:dev" |
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 image is so old it does not pull or run in Docker Desktop. Obviously not the only place we use it, but it might be worth not propagating it into the new backend.
> docker run -it --entrypoint /bin/bash broadinstitute/cloud-cromwell:dev
Unable to find image 'broadinstitute/cloud-cromwell:dev' locally
dev: Pulling from broadinstitute/cloud-cromwell
docker: [DEPRECATION NOTICE] Docker Image Format v1 and Docker Image manifest version 2, schema 1 support is disabled by default and will be removed in an upcoming release. Suggest the author of docker.io/broadinstitute/cloud-cromwell:dev to upgrade the image to the OCI Format or Docker Image manifest v2, schema 2. More information at https://docs.docker.com/go/deprecated-image-specs/.
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 image does pull and run on both PAPI v2 and Batch, but I'll see if I can push something a little less archaic.
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.
It looks like you pushed updates to it in August, seems like we might just change the tag
https://hub.docker.com/repository/docker/broadinstitute/cloud-cromwell/general
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.
updated to reference one of those August images 🤞
``` | ||
|
||
Note that as per the Google Secret Manager docs, the compute service account for the project in which the GCP Batch | ||
jobs will run will need to be assigned the `Secret Manager Secret Accessor` IAM role. |
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 sounds like it might be a change we need to make in RBS, could you file a ticket to make sure we remember to do this? Or maybe update https://broadworkbench.atlassian.net/browse/AN-186 to cover this as well if it is indeed an RBS change.
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.
Updated AN-186, inserted a new item 6 into the referenced Google doc to cover the GSM-related config.
# encoded but the format becomes | ||
# <Path to GSM username secret>:<Path to GSM password secret> | ||
# This test is exercising the GCP Batch Google Secret Manager support. | ||
token = "{{$cromwellDockerhub.Data.google_secret_manager_token}}" |
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.
Would you mind also creating a ticket to cover the changes we'll need to make to get this working in Terra? Currently we store a docker_token
that is b64 encoded username:password
, we'll need to add a secret containing just the password and maybe another containing the new token, and update Cromwell config to use the new secret.
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.
GCPBATCHGoogleSecretManager { | ||
actor-factory = "cromwell.backend.google.batch.GcpBatchBackendLifecycleActorFactory" | ||
config { | ||
include "gcp_batch_provider_config.inc.conf" |
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.
Nice, looks like since this is configured per backend, it won't be a problem to have the PAPI and Batch backends authing with different tokens while they're both in use.
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.
Docker image considerations definitely optional and not a blocker
Description
Release Notes Confirmation
CHANGELOG.md
CHANGELOG.md
in this PRCHANGELOG.md
because it doesn't impact community usersTerra Release Notes