Support for deployment in Azure Government Cloud (Leonardo) #4813
Conversation
This reverts commit aa1586e.
This reverts commit f58c237.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #4813 +/- ##
===========================================
- Coverage 74.77% 74.77% -0.01%
===========================================
Files 165 165
Lines 14954 14955 +1
Branches 1187 1234 +47
===========================================
Hits 11182 11182
- Misses 3772 3773 +1
Continue to review full report in Codecov by Sentry.
|
| tokenOpt, | ||
| AppCreationException(s"Pet not found for user ${params.app.auditInfo.creator}", Some(ctx.traceId)) | ||
| ) | ||
| userToken <- F.pure(tokenOpt.getOrElse("")) // Empty token when running on Azure. |
There was a problem hiding this comment.
It would be good to differentiate between the azure case that does not require the token, and a true failure in the GCP case where the token is not found. Without the error message it is going to be trickier for us to debug potential user issues
| @@ -99,6 +94,10 @@ class CromwellRunnerAppInstall[F[_]](config: CromwellRunnerAppConfig, | |||
| .map(v => raw"config.concurrentJobLimit=${v}") | |||
| } | |||
|
|
|||
| // Get the pet userToken | |||
| tokenOpt <- samDao.getCachedArbitraryPetAccessToken(params.app.auditInfo.creator) | |||
| userToken <- F.pure(tokenOpt.getOrElse("")) // Empty token when running on Azure. | |||
There was a problem hiding this comment.
Same comment as above, it would be good to preserve the error message in case of a true failure on the GCP side
| raw"bard.enabled=${config.bardEnabled}" | ||
| raw"bard.enabled=${config.bardEnabled}", | ||
|
|
||
| // TEMPORARY HELM OVERRIDE VALUES WHILE WAITING FOR PR |
There was a problem hiding this comment.
Could you link the PR? if it is the terra helmfile one you should be good to merge I think
|
|
||
| // Get Vpa enabled tag | ||
| vpaEnabled <- F.pure(params.landingZoneResources.aksCluster.tags.getOrElse("aks-cost-vpa-enabled", false)) | ||
|
|
||
| // Get the pet userToken | ||
| tokenOpt <- samDao.getCachedArbitraryPetAccessToken(params.app.auditInfo.creator) | ||
| userToken <- F.pure(tokenOpt.getOrElse("")) // Empty token when running on Azure. |
There was a problem hiding this comment.
Same comment re preserving the error message
| tokenOpt, | ||
| AppCreationException(s"Pet not found for user ${params.app.auditInfo.creator}", Some(ctx.traceId)) | ||
| ) | ||
| userToken <- F.pure(tokenOpt.getOrElse("")) // Empty token when running on Azure. |
There was a problem hiding this comment.
Same comment re preserving the error message
| @@ -163,12 +163,14 @@ final class LeoAppServiceInterp[F[_]: Parallel](config: AppServiceConfig, | |||
| // Retrieve parent workspaceId for the google project | |||
| parentWorkspaceId <- samService.lookupWorkspaceParentForGoogleProject(userInfo.accessToken.token, googleProject) | |||
|
|
|||
| leoToken <- authProvider.getLeoAuthToken | |||
| leoEmail <- samService.getUserEmail(leoToken) | |||
There was a problem hiding this comment.
It would be good to leave a comment here explaining what the leoEmail will be used for. I am assuming that the answer is in the sam client, but would be good to isolate the GCP from the Azure case, even in a small comment
| workspaceDescOpt <- tokenOpt.flatTraverse { token => | ||
| wsmClientProvider.getWorkspace(token, workspaceId) | ||
| } | ||
| leoAuth <- samDAO.getLeoAuthToken |
There was a problem hiding this comment.
Why the change here for both GCP and Azure case? I would like to keep using the cached token wherever possible.
Yeah, we've been running this branch in our dev environment for a while, it will need some final validation as I don't think we've tested the most recent commits though.
Yep, makes sense. I was trying to avoid the branching logic if possible to keep it consistent/simpler but I will try to update shortly for those cases you pointed out. |
Part of large effort to add support for services to run in Azure Government cloud.
Two parts:
https://broadworkbench.atlassian.net/browse/TOAZ-372
See related pull requests here:
workbench-libs sam bpm wsm cromwell terra-helmfile