Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix decrypting pack config keys under additionalProperties #5225

Merged
merged 22 commits into from
Apr 10, 2021
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
bffccc5
handle additionalProperties in pack config
cognifloyd Apr 8, 2021
46a892c
add pack config with additionalProperties test
cognifloyd Apr 8, 2021
6e864f3
reformat with black
cognifloyd Apr 8, 2021
44e0436
fix defaultdict usage
cognifloyd Apr 8, 2021
2354014
handle defaults under additionalProperties
cognifloyd Apr 8, 2021
9d7c1d5
typo
cognifloyd Apr 8, 2021
3543ad1
mark props with default as not required
cognifloyd Apr 8, 2021
e1ef44b
init additionalProperties in schema for defaults
cognifloyd Apr 9, 2021
46ce281
clean up fixture schema
cognifloyd Apr 9, 2021
8bf7d2a
Changelog entry for pack config additionalProperties support
cognifloyd Apr 9, 2021
a2644c2
Handle defaultdict schema correctly
cognifloyd Apr 9, 2021
b7672ed
Simplify based on PR feedback
cognifloyd Apr 9, 2021
26741cd
Merge branch 'master' into pack-config-additionalproperties
cognifloyd Apr 9, 2021
e69fbae
add clarifying comment in test
cognifloyd Apr 10, 2021
a483888
fix parent_keys in config_loader
cognifloyd Apr 10, 2021
d67b3c9
drop test with unencrypted key in secret: true
cognifloyd Apr 10, 2021
4d3be4a
typo
cognifloyd Apr 10, 2021
574034f
Fix failing test - we need to pass encrypted value to the KVP model.
Kami Apr 10, 2021
d0d9924
Add a temporary workaround to try to retry integration tests which fail
Kami Apr 10, 2021
6bfa390
Add TODO note.
Kami Apr 10, 2021
532e15f
Merge branch 'master' into pack-config-additionalproperties
Kami Apr 10, 2021
1d613bc
Update logging, add timeout-minutes to the step as a safe guard.
Kami Apr 10, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,11 @@ Added

Contributed by @Kami.

* Fix a bug in the pack config loader so that objects covered by an additionalProperties schema
cognifloyd marked this conversation as resolved.
Show resolved Hide resolved
can use encrypted datastore keys and have their default values applied correctly. #5225

Contributed by @cognifloyd.

Changed
~~~~~~~

Expand Down
33 changes: 30 additions & 3 deletions st2common/st2common/util/config_loader.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,23 @@ def _get_values_for_config(self, config_schema_db, config_db):
config = self._assign_default_values(schema=schema_values, config=config)
return config

@staticmethod
def _get_object_property_schema(object_schema, additional_properties_keys=None):
"""
Create a schema for an object property using both additionalProperties and properties.

:rtype: ``dict``
"""
property_schema = {}
additional_properties = object_schema.get("additionalProperties", {})
# additionalProperties can be a boolean or a dict
if additional_properties and isinstance(additional_properties, dict):
# ensure that these keys are present in the object
for key in additional_properties_keys:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, if that works it should be much more straight forward to understand the code now - well, at least for me :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

property_schema[key] = additional_properties
property_schema.update(object_schema.get("properties", {}))
return property_schema

def _assign_dynamic_config_values(self, schema, config, parent_keys=None):
"""
Assign dynamic config value for a particular config item if the ite utilizes a Jinja
Expand Down Expand Up @@ -130,8 +147,12 @@ def _assign_dynamic_config_values(self, schema, config, parent_keys=None):
# Inspect nested object properties
if is_dictionary:
parent_keys += [str(config_item_key)]
property_schema = self._get_object_property_schema(
schema_item,
additional_properties_keys=config_item_value.keys(),
)
self._assign_dynamic_config_values(
schema=schema_item.get("properties", {}),
schema=property_schema,
config=config[config_item_key],
parent_keys=parent_keys,
)
Expand Down Expand Up @@ -182,18 +203,24 @@ def _assign_default_values(self, schema, config):
default_value = schema_item.get("default", None)
is_object = schema_item.get("type", None) == "object"
has_properties = schema_item.get("properties", None)
has_additional_properties = schema_item.get("additionalProperties", None)

if has_default_value and not has_config_value:
# Config value is not provided, but default value is, use a default value
config[schema_item_key] = default_value

# Inspect nested object properties
if is_object and has_properties:
if is_object and (has_properties or has_additional_properties):
if not config.get(schema_item_key, None):
config[schema_item_key] = {}

property_schema = self._get_object_property_schema(
schema_item,
additional_properties_keys=config[schema_item_key].keys(),
)

self._assign_default_values(
schema=schema_item["properties"], config=config[schema_item_key]
schema=property_schema, config=config[schema_item_key]
)

return config
Expand Down
64 changes: 63 additions & 1 deletion st2common/tests/unit/test_config_loader.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ def test_ensure_local_pack_config_feature_removed(self):

def test_get_config_some_values_overriden_in_datastore(self):
# Test a scenario where some values are overriden in datastore via pack
# flobal config
# global config
kvp_db = set_datastore_value_for_config_key(
pack_name="dummy_pack_5",
key_name="api_secret",
Expand Down Expand Up @@ -518,6 +518,68 @@ def test_get_config_dynamic_config_item_nested_list(self):

config_db.delete()

def test_get_config_dynamic_config_item_under_additional_properties(self):
pack_name = "dummy_pack_schema_with_additional_properties_1"
loader = ContentPackConfigLoader(pack_name=pack_name)

KeyValuePair.add_or_update(KeyValuePairDB(name="k0", value="v0"))
KeyValuePair.add_or_update(
KeyValuePairDB(name="k1_encrypted", value="v1_encrypted", secret=True)
)

####################
# values in objects under an object with additionalProperties
values = {
"profiles": {
"dev": {
# no host or port to test default value
"token": "hard-coded-secret",
},
"stage": {
"host": "127.0.0.1",
"port": 8181,
# unencrypted in datastore
"token": "{{st2kv.system.k0}}",
},
"prod": {
"host": "127.1.2.7",
"port": 8282,
# encrypted in datastore
"token": "{{st2kv.system.k1_encrypted}}",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If user wants that value in the config to actually be decrypted, they still need to use decrypt_kv Jinja filter, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Config is automatically decrypted when the schema lists the key as secret: true

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See _get_datastore_value_for_expression() in config_loader.py

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarification - I was just somewhat confused by the test case aka something didn't add up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! That's why it seemed to be working before, but wasn't. Thanks for the fix!

},
}
}
config_db = ConfigDB(pack=pack_name, values=values)
config_db = Config.add_or_update(config_db)

config_rendered = loader.get_config()

self.assertEqual(
config_rendered,
{
"region": "us-east-1",
"profiles": {
"dev": {
"host": "127.0.0.3",
"port": 8080,
"token": "hard-coded-secret",
},
"stage": {
"host": "127.0.0.1",
"port": 8181,
"token": "v0",
},
"prod": {
"host": "127.1.2.7",
"port": 8282,
"token": "v1_encrypted",
},
},
},
)

config_db.delete()

def test_empty_config_object_in_the_database(self):
pack_name = "dummy_pack_empty_config"

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
---
region:
type: "string"
required: false
default: "us-east-1"
profiles:
type: "object"
required: false
additionalProperties:
type: object
additionalProperties: false
properties:
host:
type: "string"
required: false
default: "127.0.0.3"
port:
type: "integer"
required: false
default: 8080
token:
type: "string"
required: true
secret: true
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
name : dummy_pack_schema_with_additional_properties_1
description : dummy pack with nested objects under additionalProperties
version : 0.1.0
author : st2-dev
email : [email protected]