Added mask of rule action secret parameters #4788
Conversation
|
|
pull-request-size
bot
added
the
size/M
|
Thanks for the contribution. The change looks good to me, but we would need some tests for it before we can merge it (including model level tests and API level tests). We will likely also need to update mandatory include fields for rule API endpoints to ensure secrets masking works correctly when |
Nicodemos305
force-pushed
the
fix/mask-secrets-rule-parameters
branch
2 times, most recently
from
f00fb0c
to
7cfb390
Compare
…es and ?exclude_attributes
…rs for test_get_all_parameters_mask_with_include_parameters and test_get_one_parameters_mask_with_exclude_parameters for test_get_all_parameters_mask_with_exclude_parameters
jeansfelix
force-pushed
the
fix/mask-secrets-rule-parameters
branch
from
7cfb390
to
42eb393
Compare
|
Dear @Kami , the tests on SO Ubuntu and Centos failed but I couldn't see the error log, would you help me? |
|
Merging updates from |
|
Thanks @blag for the PR rescue! 👍 @Nicodemos305 Please add a Changelog for this fix to make sure this PR is 💯 ready for final review and merge. |
|
@nicholasamorim Fixed, sorry. Confused by Github autocomplete for the first 3 characters in @ |
…ject/st2 into fix/mask-secrets-rule-parameters
|
@armab Done! |
| result = copy.deepcopy(value) | ||
| if('action' not in result): | ||
| return result | ||
| action_db = self._get_referenced_models(rule=result) |
There was a problem hiding this comment.
Overall this change looks good to me, my only concern is performance impact of this change.
This change means we now need to perform 1 additional query for each Rule DB object.
This is not an issue for "get one" operation (only one additional query), but when retrieving multiple rules aka "get all" API operations, this would result in N additional queries where N is number of rules on the page.
| return action_db | ||
|
|
||
| def _get_entity(self, model_persistence, ref, query_args): | ||
| q = Q(**query_args(ref)) |
There was a problem hiding this comment.
It looks like we really only need parameters field from the action model, so we should update this query to only retrieve that field by utilizing only_fields argument / query.only(**fields) function.
There was a problem hiding this comment.
On a related note - is there a specific reason why you used Q object directly?
To not break the abstraction, we should probably utilize Action.query() method instead.
There was a problem hiding this comment.
Just a heads up. I will look into making the changes mentioned above^.
This way we can hopefully get this across the finish line and merge it soon.
|
I had a look at it and this needed a bit more work, so I tried to finish it. I made the following changes:
Unless I missed something, this is now ready to be merged. My original comment with regards to performance impact / overhead still stands, but that's probably fine for now. In the future, we can implement some kind of caching / similar to avoid this action retrieval overhead on get / list all API operation. |
…ters /v1/rules API endpoint action parameters secrets masking (additional changes on top of #4788)
|
Merged into master via #4807. Thanks again for the contribution. |
Closes #4784