Secret masking in output_schema feature is added #5250
Conversation
pull-request-size
bot
added
the
size/L
|
|
adding a missed import
There was a problem hiding this comment.
This is a good start. But from the current PR, I don't see how this is wired to the API and CLI masking the result in the output on execution get. Can you write a unit test that can show this is working for GET method of action execution in the API? Also, the secret masking of action execution output shouldn't affect action execution in a workflow, can you also add a test case for workflow execution where the output of an action execution of a task isn't masked?
pull-request-size
bot
added
size/M
pull-request-size
bot
added
size/XS
pull-request-size
bot
added
size/M
|
Thanks @m4dcoder for the comments. I have pushed the new changes in /st2common/models/api/execution.py and they are wired to the API and CLI masking the result in the output on execution get. Please review the changes. |
| :rtype: class: ``ActionExecutionAPI`` | ||
| """ | ||
|
|
||
| doc = cls._from_model(model, mask_secrets=mask_secrets) |
There was a problem hiding this comment.
@mahesh-orch If you follow _from_model, you'll see that there is already a line that mask secrets at https://github.com/StackStorm/st2/blob/master/st2common/st2common/models/api/base.py#L118. This calls a model specific function to mask secrets. The specific function for ActionExecutionDB is at https://github.com/StackStorm/st2/blob/master/st2common/st2common/models/db/execution.py#L107. Can you move the logic for output schema into this function?
pull-request-size
bot
added
size/XS
pull-request-size
bot
added
size/M
|
@m4dcoder I have moved the logic for masking secret parameters in output schema from https://github.com/StackStorm/st2/blob/master/st2common/st2common/models/api/execution.py to https://github.com/StackStorm/st2/blob/master/st2common/st2common/models/db/execution.py. Also added a test case in https://github.com/orchestral-st2/st2/blob/master/st2common/tests/unit/test_db_execution.py. Please review the added code. Thanks. |
There was a problem hiding this comment.
Nice! This is coming along, and I'm really looking forward to using it.
Some whitespace needs to be cleaned up.
@m4dcoder Can you enable running tests on this PR? This is @mahesh-orch's first PR on st2, so github is blocking running actions until someone with write access enables them.
Co-authored-by: Jacob Floyd <cognifloyd@gmail.com>
Co-authored-by: Jacob Floyd <cognifloyd@gmail.com>
What do you mean? Can you share an example of what was getting masked when masking the output of an orquesta workflow? |
The current mask_secret_output function in the output_schema util module is too fragile with the multiple if else on checking runner type to determine if there is an output schema defined. For example, orquesta runner has an output_schema and output_key defined but then the if else already introduced a bug. The function is refactored to evaluate if output key and output schema is defined and then proceed to masking secret value if an output parameter is marked as secret in the output schema. Additional unit test is added to cover orquesta runner. The unit tests for the mask_secret_output function is also simplified.
New actions were added to dummy_pack_1. The number of actions have changed in some of the checks in tests.
There was a problem hiding this comment.
I have refactored your PR please review my changes. The if/else check on runner type is too fragile and again is bug prone if someone goes and change the schema for the runner type of add new runner types. Your change already introduced a bug for the orquesta runner (workflow). I refactored it and also added a unit test for orquesta runner in st2actions/tests/unit/test_output_schema.py to catch that. There are other changes I did to simplify the PR. Also, I've fixed tests so CI succeeded now.
There are a still open items in that PR you should follow up on. 1) Please add a test for workflow. Add a task with secrets in the output schema, publish the result of the task to the workflow output and make sure the value is not masked in the task. You can add this test at https://github.com/StackStorm/st2/blob/master/contrib/runners/orquesta_runner/tests/unit/test_data_flow.py. 2) Please add a unit test in the API to make sure output with secrets is masked at https://github.com/StackStorm/st2/blob/master/st2api/tests/unit/controllers/v1/test_executions.py.
@cognifloyd Thank you for the comments and inputs. Previous my changes without if condition to orquesta runner were causing masking the secret parameter in workflow task output but @m4dcoder had improved that piece of code and now it is working fine. |
pull-request-size
bot
added
size/XL
|
@m4dcoder, @cognifloyd Hi, I am pushing code of unit test to show in workflow secret parameters in output_schema in action should not be masked in task output. I need help as unit test is failing at the task which contain python action. |
to test secret parameters masked in GET API
|
Hi @m4dcoder, I have added unit test for testing GET APIs containing output schema secret parameters as masked. Please review changes. |
Use the existing data flow test to include the test case for secret output in task. Add output schema and secret output to the main workflow as well.
Add the action.output_schema and runner.output_key to attributes to be included in the API query otherwise secrets are not masked in the output of workflow action. Update the output schema util to be more robust in terms of handling result that is either empty, wrong type, or missing output key.
With this implementation, action responses marked as secret in output schema are masked in CLI and UI.