TSPS-306 getResult returns signed urls for all outputs #120
Conversation
| } | ||
| return blobHttpUrl.substring( | ||
| blobHttpUrl.indexOf(workspaceId.toString()) + workspaceId.toString().length() + 1); | ||
| return blobHttpUrl.substring(blobHttpUrl.indexOf(delimiter) + delimiter.length() + 1); |
There was a problem hiding this comment.
i think delimiter here is a little misleading cuz we're only using the first instance of it to substring the original string as opposed to every instance of it. maybe it can be called substringStart or blobNameStart? i dont love any of these :(
| * @return blobName | ||
| */ | ||
| public static String getBlobNameFromTerraWorkspaceStorageHttpUrl( | ||
| String blobHttpUrl, UUID workspaceId) { | ||
| if (!blobHttpUrl.contains(workspaceId.toString())) { | ||
| String blobHttpUrl, String delimiter) { |
There was a problem hiding this comment.
probably rename blobHttpUrl to blobUrl
| @@ -70,7 +70,48 @@ public URL generatePutObjectSignedUrl(String projectId, String bucketName, Strin | |||
| Storage.SignUrlOption.withExtHeaders(extensionHeaders), | |||
| Storage.SignUrlOption.withV4Signature())); | |||
|
|
|||
| logger.info("Generated PUT signed URL: {}", url); | |||
| // remove the signature from the URL before logging | |||
| String cleanUrl = url.toString().split("X-Goog-Signature=")[0] + "X-Goog-Signature=REDACTED"; | |||
There was a problem hiding this comment.
do you need to add the rest of elements from the split() to get the full string or is X-Goog-Signature always guaranteed to be the last element in the url? If not you probably have to split the url on & and then ignore the element that has X-Goog-Signature in it
There was a problem hiding this comment.
hmm yeah I was just going by the signed url that was returned by the service, so it does currently seem to be consistently the last element, but there's no guarantee that it'll always be that way. i can make this resilient to different orders of elements.
| @@ -408,6 +409,17 @@ public ApiPipelineRunOutputs formatPipelineRunOutputs(PipelineRun pipelineRun) { | |||
| pipelineRunOutputsAsMap( | |||
| pipelineOutputsRepository.findPipelineOutputsByJobId(pipelineRun.getId()).getOutputs()); | |||
|
|
|||
| // currently all outputs are paths that will need a SAS token | |||
| String workspaceStorageContainerName = pipelineRun.getWorkspaceStorageContainerName(); | |||
| outputsMap.replaceAll( | |||
| @@ -408,6 +409,17 @@ public ApiPipelineRunOutputs formatPipelineRunOutputs(PipelineRun pipelineRun) { | |||
| pipelineRunOutputsAsMap( | |||
| pipelineOutputsRepository.findPipelineOutputsByJobId(pipelineRun.getId()).getOutputs()); | |||
|
|
|||
| // currently all outputs are paths that will need a SAS token | |||
There was a problem hiding this comment.
SAS token -> signed url
|
|
||
| addStep( | ||
| new FetchOutputsFromDataTableStep( | ||
| flightBeanBag.getRawlsService(), flightBeanBag.getSamService())); |
There was a problem hiding this comment.
oh hmm do we want a different retry rule for external services. Ive jsut been using thes ame dbRetryRule for all these steps and maybe thats incorrect?
There was a problem hiding this comment.
ah yeah thanks, i noticed this too but then forgot about it. added an "externalServiceRetryRule" (copied from WSM)
| * @param signedUrl | ||
| * @return | ||
| */ | ||
| public static String cleanSignedUrl(URL signedUrl) { |
| * Use for a short exponential backoff retry, for operations that should be completable within a | ||
| * few seconds. | ||
| */ | ||
| private final RetryRule externalServiceRetryRule = |
| for (PipelineOutputDefinition outputDefinition : outputDefinitions) { | ||
| String keyName = outputDefinition.getName(); | ||
| String wdlVariableName = outputDefinition.getWdlVariableName(); | ||
| outputs.put(keyName, entity.getAttributes().get(wdlVariableName).toString()); |
There was a problem hiding this comment.
as we chatted on slack, it may be worth validating the outputs exist and are in fact not empty
| * @param entity | ||
| * @return a map of pipeline outputs | ||
| */ | ||
| public Map<String, String> extractPipelineOutputsFromEntity( |
There was a problem hiding this comment.
just a random thought i had about the future. I wonder if it would make sense to put these input/output focused function into its own service if this one starts getting too bloated
There was a problem hiding this comment.
I agree, it felt a little odd to put this into PipelineRuns service. I'd gladly move some of these methods into their own service/utility in future
|
Description
We now return signed GET (read-only) urls for pipeline run outputs, when the user calls get pipeline run result.
Added logic to redact the signature string from the signed url when we log its creation (for both PUT and GET signed urls).
To do this, we had to add two steps to the GCP flight:
Jira Ticket
https://broadworkbench.atlassian.net/browse/TSPS-306