Fix decrypting pack config keys under additionalProperties #5225
Conversation
pull-request-size
bot
added
the
size/S
copart-jafloyd
force-pushed
the
pack-config-additionalproperties
branch
from
09b104b to
bffccc5
Compare
pull-request-size
bot
added
size/L
copart-jafloyd
force-pushed
the
pack-config-additionalproperties
branch
from
c5ecc71 to
3543ad1
Compare
copart-jafloyd
force-pushed
the
pack-config-additionalproperties
branch
from
9378252 to
e1ef44b
Compare
copart-jafloyd
force-pushed
the
pack-config-additionalproperties
branch
from
585fa46 to
8bf7d2a
Compare
This comment has been minimized.
This comment has been minimized.
|
All GHA tests are passing. CircleCI tests are broken due to a pbr+logshipper issue (known based on conversation in slack). |
|
Nice work 👍 I will have a more detailed look later this week. |
|
Weird. I'm patching a v3.2 StackStorm running python2.7 and this delta was insufficient. afaik, the behavior of defaultdict should be the same under python3, so I'm not sure why the unit test is passing. In any case, I will push a commit with the fix momentarily. |
copart-jafloyd
force-pushed
the
pack-config-additionalproperties
branch
from
1bafc58 to
a2644c2
Compare
| "host": "127.1.2.7", | ||
| "port": 8282, | ||
| # encrypted in datastore | ||
| "token": "{{st2kv.system.k1_encrypted}}", |
There was a problem hiding this comment.
If user wants that value in the config to actually be decrypted, they still need to use decrypt_kv Jinja filter, right?
There was a problem hiding this comment.
No. Config is automatically decrypted when the schema lists the key as secret: true
There was a problem hiding this comment.
See _get_datastore_value_for_expression() in config_loader.py
There was a problem hiding this comment.
Thanks for clarification - I was just somewhat confused by the test case aka something didn't add up.
There was a problem hiding this comment.
I pushed a fix for this test - 574034f.
That's the part which confused me a bit.
I assumed we don't do decryption because the value which was stored and the one which was retrieved was supposed to be the same (but the KVP model layer doesn't perform any encryption and we need to encrypt the value ourselves when storing it).
There was a problem hiding this comment.
Ah! That's why it seemed to be working before, but wasn't. Thanks for the fix!
|
Overall LGTM, just had some questions / comments on internal implementation details. |
| for key in init_additional_properties: | ||
| property_schema.__missing__(key) | ||
| # ensure that these keys are present in the object | ||
| for key in additional_properties_keys: |
There was a problem hiding this comment.
Thanks, if that works it should be much more straight forward to understand the code now - well, at least for me :)
There was a problem hiding this comment.
And re - using copy - I actually verified, if we didn't use copy with defaultdict version, we also don't need to use it here - it's returning by reference version in both scenarios.
And since I believe we indeed never manipulate those values, only read / access them, copy is probably not needed.
There was a problem hiding this comment.
Yes. We modify the config object, but we don't modify (and probably should not modify) the schema itself. So, references are perfect in this case.
copart-jafloyd
force-pushed
the
pack-config-additionalproperties
branch
from
408e14f to
e69fbae
Compare
copart-jafloyd
force-pushed
the
pack-config-additionalproperties
branch
from
7bd6933 to
d67b3c9
Compare
|
Some of those integration tests still seem to be failing quite often due to race / similar, quite annoying. We should probably modify that job step to try to retry up to 2-3 times on failure to avoid annoying and manual re-run of the complete workflow. |
quite often due to races, etc. on failure. This saves us a bunch of time manually re-running the whole workflow.
|
GHA is passing now. Hooray. |
|
All tests are green. Quick, let's merge before anything else gets merged into master. :) |
The pack config loader uses "secret: true" in a schema to trigger decrypting a key from the datastore. However, it does not look at the schema under additionalProperties, so those properties can't use encrypted keys.
This issue was first documented in StackStorm-Exchange/stackstorm-jira#19 (comment)
This PR fixes that so that the pack config loader handles additionalProperties when defined.