Arvados

The broader problem is that specifying secrets requires some extra work when creating the container request, and this code is essentially duplicated between arvados-cwl-runner and workbench. As a result, a-c-r knows how to do it, and workbench(1|2) doesn't. This almost certainly isn't the only example of inconsistent behavior between them.

I recommend exploring the option of submitting the workflow using a high level API, specifically the GA4GH Workflow Execution Service (WES). The implementation of the high level API can invoke arvados-cwl-runner which already has the logic to construct the low level container request. Submitted for consideration: #15918 #15917

15814-wb2-secrets @ 87f53f0c33e9033636122ac09322229caaee3398

developer-run-tests: #4292

15814-wb2-secrets @ 2ed8ccba7164836f69d15ae6d2cb1cbd2cd03b7c

developer-run-tests: #4301

• All agreed upon points are implemented / addressed.
  • can now use arvados-cwl-runner --create-workflow on workflows with secrets
  • secret inputs are treated as password input and use 'password' type input
  • container request is constructed the same way that arvados-cwl-runner does it to avoid exposing passwords
• Anything not implemented (discovered or discussed during work) has a follow-up story.
  • n/a
• Code is tested and passing, both automated and manual, what manual testing was done is described
  • added a new workbench integration test which goes through the process of creating a container request with a secret input.
• Documentation has been updated.
  • n/a
• Behaves appropriately at the intended scale (describe intended scale).
  • no changes in scale
• Considered backwards and forwards compatibility issues between client and server.
  • the secrets handling feature has been around for many releases, this does not rely on any new API features
• Follows our coding standards and GUI style guidelines.
  • yes

In the process of writing the test, I struggled a bit with Cypress. I made a few changes:

• If run-tests.sh is run in "interactive" mode, then it will launch the cypress tests in "interactive" mode as well.
• I added a "fail fast" plugin which will cause cypress to stop running tests in a particular spec file on the first failure. However, I set it up so it is disabled by default except when launched in interactive mode (previous bullet).
• I ended up making some changes to a couple of "process" tests that were giving me trouble. I observed that some of the flaky tests are flaky because of race conditions where a cached value is displayed while it is waiting to get a new value from the server, but the check fails on the old cached value when it was expecting the new value. These situations require the test to be a bit more aggressive on enforcing sequencing and/or triggering a browser reload to prevent stale values.

15814-wb2-secrets @ bac52a7846a94bc3722c8256c7b8f567b7bd3096

developer-run-tests: #4313

Review comments:

• There's some missing indentation in createWorkflowSecretMounts() definition.
• I believe that instead of writing conditionals like "if (something && something.other) {...}" you can simplify it with "if (something?.other) {...}"
• There's also some funny indentations at
  • services/workbench2/src/store/run-process-panel/run-process-panel-actions.ts lines 160-200.
  • arvados/services/workbench2/src/models/workflow.ts lines 164-171
  • services/workbench2/cypress/e2e/process.cy.js lines 113-128, 212-218, 1335-1337, 1470-1472
  • services/workbench2/cypress.config.ts lines 16-25
  • arvados/services/workbench2/cypress/e2e/create-workflow.cy.js lines 287-335
• When viewing the runner container, the corresponding input value for the secret shows as "Cannot display value" and while this is desirable, that input is not marked as a special one, so if its name doesn't hint the user that the value is a secret, using a better message could be helpful. Something like "Sensitive data hidden" or similar?

Lucas Di Pentima wrote in #note-66:

• I believe that instead of writing conditionals like "if (something && something.other) {...}" you can simplify it with "if (something?.other) {...}"

Done.

• When viewing the runner container, the corresponding input value for the secret shows as "Cannot display value" and while this is desirable, that input is not marked as a special one, so if its name doesn't hint the user that the value is a secret, using a better message could be helpful. Something like "Sensitive data hidden" or similar?

Detected explicitly and says "Cannot display secret" now.

Fixed all the wonky indentation, it was using tabs, I need to fix my untabify-on-save hook.

15814-wb2-secrets @ d8307bae7ebd60c20ecaf445f29bf4e229941e55

developer-run-tests: #4320

15814-wb2-secrets @ cc315e49c103043399d0c78fdec4b9bfab85fa81

developer-run-tests: #4322

This LGTM, thanks!