From bffccc5a56d752e6e262c44e25e1561cfa2b6375 Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Thu, 8 Apr 2021 13:50:46 -0400 Subject: [PATCH 01/20] handle additionalProperties in pack config --- st2common/st2common/util/config_loader.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/st2common/st2common/util/config_loader.py b/st2common/st2common/util/config_loader.py index 30db039bdc..e835000e07 100644 --- a/st2common/st2common/util/config_loader.py +++ b/st2common/st2common/util/config_loader.py @@ -16,6 +16,8 @@ from __future__ import absolute_import import copy +from collections import defaultdict + import six from oslo_config import cfg @@ -130,8 +132,14 @@ def _assign_dynamic_config_values(self, schema, config, parent_keys=None): # Inspect nested object properties if is_dictionary: parent_keys += [str(config_item_key)] + additional_properties = schema_item.get("additionalProperties", {}) + if isinstance(additional_properties, dict): + property_schema = defaultdict(additional_properties) + else: + property_schema = {} + property_schema.update(schema_item.get("properties", {})) self._assign_dynamic_config_values( - schema=schema_item.get("properties", {}), + schema=property_schema, config=config[config_item_key], parent_keys=parent_keys, ) From 46a892ca66662bf1820f68b84296969c1cc4d86f Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Thu, 8 Apr 2021 16:16:25 -0400 Subject: [PATCH 02/20] add pack config with additionalProperties test --- st2common/tests/unit/test_config_loader.py | 62 ++++++++++++++++++- .../config.schema.yaml | 24 +++++++ .../pack.yaml | 6 ++ 3 files changed, 91 insertions(+), 1 deletion(-) create mode 100644 st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_additional_properties_1/config.schema.yaml create mode 100644 st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_additional_properties_1/pack.yaml diff --git a/st2common/tests/unit/test_config_loader.py b/st2common/tests/unit/test_config_loader.py index e1849d7868..16458948bc 100644 --- a/st2common/tests/unit/test_config_loader.py +++ b/st2common/tests/unit/test_config_loader.py @@ -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", @@ -518,6 +518,66 @@ 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": { + "host": "127.0.0.1", + "token": "hard-coded-secret", + }, + "stage": { + "host": "127.0.0.2", + "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}}", + }, + } + } + 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, + { + "regions": "us-east-1", + "profiles": { + "dev": { + "host": "127.0.0.1", + "port": 8080, + "token": "hard-coded-secret", + }, + "stage": { + "host": "127.0.0.2", + "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" diff --git a/st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_additional_properties_1/config.schema.yaml b/st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_additional_properties_1/config.schema.yaml new file mode 100644 index 0000000000..ec52db7c2d --- /dev/null +++ b/st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_additional_properties_1/config.schema.yaml @@ -0,0 +1,24 @@ +--- + regions: + type: "array" + required: true + default: "us-east-1" + profiles: + type: "object" + required: false + additionalProperties: + type: object + additionalProperties: false + properties: + host: + type: "string" + required: true + default: "127.0.0.3" + port: + type: "integer" + required: true + default: 8080 + token: + type: "string" + required: true + secret: true diff --git a/st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_additional_properties_1/pack.yaml b/st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_additional_properties_1/pack.yaml new file mode 100644 index 0000000000..18f98ce85b --- /dev/null +++ b/st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_additional_properties_1/pack.yaml @@ -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 : info@stackstorm.com From 6e864f398678945602dfdc76f83fb1ceb511613a Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Thu, 8 Apr 2021 16:23:18 -0400 Subject: [PATCH 03/20] reformat with black --- st2common/tests/unit/test_config_loader.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/st2common/tests/unit/test_config_loader.py b/st2common/tests/unit/test_config_loader.py index 16458948bc..75e0634380 100644 --- a/st2common/tests/unit/test_config_loader.py +++ b/st2common/tests/unit/test_config_loader.py @@ -523,7 +523,9 @@ def test_get_config_dynamic_config_item_under_additional_properties(self): 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)) + KeyValuePair.add_or_update( + KeyValuePairDB(name="k1_encrypted", value="v1_encrypted", secret=True) + ) #################### # values in objects under an object with additionalProperties From 44e0436da5ea19ff3994f8d58ff5c0052527832d Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Thu, 8 Apr 2021 16:27:55 -0400 Subject: [PATCH 04/20] fix defaultdict usage --- st2common/st2common/util/config_loader.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/st2common/st2common/util/config_loader.py b/st2common/st2common/util/config_loader.py index e835000e07..4d4372b8af 100644 --- a/st2common/st2common/util/config_loader.py +++ b/st2common/st2common/util/config_loader.py @@ -134,7 +134,7 @@ def _assign_dynamic_config_values(self, schema, config, parent_keys=None): parent_keys += [str(config_item_key)] additional_properties = schema_item.get("additionalProperties", {}) if isinstance(additional_properties, dict): - property_schema = defaultdict(additional_properties) + property_schema = defaultdict(lambda: additional_properties) else: property_schema = {} property_schema.update(schema_item.get("properties", {})) From 23540146178a874ec8b76deb70de9f6141f40f30 Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Thu, 8 Apr 2021 16:48:37 -0400 Subject: [PATCH 05/20] handle defaults under additionalProperties --- st2common/st2common/util/config_loader.py | 24 ++++++++++++++-------- st2common/tests/unit/test_config_loader.py | 8 ++++---- 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/st2common/st2common/util/config_loader.py b/st2common/st2common/util/config_loader.py index 4d4372b8af..d3dfc9f4b1 100644 --- a/st2common/st2common/util/config_loader.py +++ b/st2common/st2common/util/config_loader.py @@ -100,6 +100,16 @@ 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 = object_schema.get("additionalProperties", {}) + if isinstance(additional_properties, dict): + property_schema = defaultdict(lambda: additional_properties) + else: + property_schema = {} + 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 @@ -132,12 +142,7 @@ def _assign_dynamic_config_values(self, schema, config, parent_keys=None): # Inspect nested object properties if is_dictionary: parent_keys += [str(config_item_key)] - additional_properties = schema_item.get("additionalProperties", {}) - if isinstance(additional_properties, dict): - property_schema = defaultdict(lambda: additional_properties) - else: - property_schema = {} - property_schema.update(schema_item.get("properties", {})) + property_schema = self._get_object_property_schema(schema_item) self._assign_dynamic_config_values( schema=property_schema, config=config[config_item_key], @@ -190,18 +195,21 @@ 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) + self._assign_default_values( - schema=schema_item["properties"], config=config[schema_item_key] + schema=propety_schema, config=config[schema_item_key] ) return config diff --git a/st2common/tests/unit/test_config_loader.py b/st2common/tests/unit/test_config_loader.py index 75e0634380..50c8f27f98 100644 --- a/st2common/tests/unit/test_config_loader.py +++ b/st2common/tests/unit/test_config_loader.py @@ -532,11 +532,11 @@ def test_get_config_dynamic_config_item_under_additional_properties(self): values = { "profiles": { "dev": { - "host": "127.0.0.1", + # no host or port to test default value "token": "hard-coded-secret", }, "stage": { - "host": "127.0.0.2", + "host": "127.0.0.1", "port": 8181, # unencrypted in datastore "token": "{{st2kv.system.k0}}", @@ -560,12 +560,12 @@ def test_get_config_dynamic_config_item_under_additional_properties(self): "regions": "us-east-1", "profiles": { "dev": { - "host": "127.0.0.1", + "host": "127.0.0.3", "port": 8080, "token": "hard-coded-secret", }, "stage": { - "host": "127.0.0.2", + "host": "127.0.0.1", "port": 8181, "token": "v0", }, From 9d7c1d5de6e2e56af7e8f92051017a788fd9a4c0 Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Thu, 8 Apr 2021 16:53:20 -0400 Subject: [PATCH 06/20] typo --- st2common/st2common/util/config_loader.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/st2common/st2common/util/config_loader.py b/st2common/st2common/util/config_loader.py index d3dfc9f4b1..ef1d5b4c2e 100644 --- a/st2common/st2common/util/config_loader.py +++ b/st2common/st2common/util/config_loader.py @@ -209,7 +209,7 @@ def _assign_default_values(self, schema, config): property_schema = self._get_object_property_schema(schema_item) self._assign_default_values( - schema=propety_schema, config=config[schema_item_key] + schema=property_schema, config=config[schema_item_key] ) return config From 3543ad11a21ffb622d7be7093eed7857cb35df70 Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Thu, 8 Apr 2021 17:57:09 -0400 Subject: [PATCH 07/20] mark props with default as not required --- st2common/tests/unit/test_config_loader.py | 2 +- .../config.schema.yaml | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/st2common/tests/unit/test_config_loader.py b/st2common/tests/unit/test_config_loader.py index 50c8f27f98..3927fd5a80 100644 --- a/st2common/tests/unit/test_config_loader.py +++ b/st2common/tests/unit/test_config_loader.py @@ -557,7 +557,7 @@ def test_get_config_dynamic_config_item_under_additional_properties(self): self.assertEqual( config_rendered, { - "regions": "us-east-1", + "regions": ["us-east-1"], "profiles": { "dev": { "host": "127.0.0.3", diff --git a/st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_additional_properties_1/config.schema.yaml b/st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_additional_properties_1/config.schema.yaml index ec52db7c2d..bd8510296a 100644 --- a/st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_additional_properties_1/config.schema.yaml +++ b/st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_additional_properties_1/config.schema.yaml @@ -1,7 +1,7 @@ --- regions: type: "array" - required: true + required: false default: "us-east-1" profiles: type: "object" @@ -12,11 +12,11 @@ properties: host: type: "string" - required: true + required: false default: "127.0.0.3" port: type: "integer" - required: true + required: false default: 8080 token: type: "string" From e1ef44b1a0ee641b2673016313b70a0e48d10a59 Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Thu, 8 Apr 2021 20:04:16 -0400 Subject: [PATCH 08/20] init additionalProperties in schema for defaults --- st2common/st2common/util/config_loader.py | 17 ++++++++++++++--- st2common/tests/unit/test_config_loader.py | 2 +- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/st2common/st2common/util/config_loader.py b/st2common/st2common/util/config_loader.py index ef1d5b4c2e..cff48a03c8 100644 --- a/st2common/st2common/util/config_loader.py +++ b/st2common/st2common/util/config_loader.py @@ -101,12 +101,16 @@ def _get_values_for_config(self, config_schema_db, config_db): return config @staticmethod - def _get_object_property_schema(object_schema): + def _get_object_property_schema(object_schema, init_additional_properties=None): additional_properties = object_schema.get("additionalProperties", {}) - if isinstance(additional_properties, dict): + if additional_properties and isinstance(additional_properties, dict): property_schema = defaultdict(lambda: additional_properties) else: property_schema = {} + if init_additional_properties: + # ensure that these keys are present in the object (vs just defaultdict) + for key in init_additional_properties: + property_schema.__missing__(key) property_schema.update(object_schema.get("properties", {})) return property_schema @@ -206,7 +210,14 @@ def _assign_default_values(self, schema, config): if not config.get(schema_item_key, None): config[schema_item_key] = {} - property_schema = self._get_object_property_schema(schema_item) + property_schema = self._get_object_property_schema( + schema_item, + init_additional_properties=( + config[schema_item_key].keys() + if has_additional_properties + else None + ), + ) self._assign_default_values( schema=property_schema, config=config[schema_item_key] diff --git a/st2common/tests/unit/test_config_loader.py b/st2common/tests/unit/test_config_loader.py index 3927fd5a80..50c8f27f98 100644 --- a/st2common/tests/unit/test_config_loader.py +++ b/st2common/tests/unit/test_config_loader.py @@ -557,7 +557,7 @@ def test_get_config_dynamic_config_item_under_additional_properties(self): self.assertEqual( config_rendered, { - "regions": ["us-east-1"], + "regions": "us-east-1", "profiles": { "dev": { "host": "127.0.0.3", From 46ce281327bdf5920ca319fa86773fcb94de5144 Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Thu, 8 Apr 2021 20:36:03 -0400 Subject: [PATCH 09/20] clean up fixture schema --- st2common/tests/unit/test_config_loader.py | 2 +- .../config.schema.yaml | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/st2common/tests/unit/test_config_loader.py b/st2common/tests/unit/test_config_loader.py index 50c8f27f98..3c35821511 100644 --- a/st2common/tests/unit/test_config_loader.py +++ b/st2common/tests/unit/test_config_loader.py @@ -557,7 +557,7 @@ def test_get_config_dynamic_config_item_under_additional_properties(self): self.assertEqual( config_rendered, { - "regions": "us-east-1", + "region": "us-east-1", "profiles": { "dev": { "host": "127.0.0.3", diff --git a/st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_additional_properties_1/config.schema.yaml b/st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_additional_properties_1/config.schema.yaml index bd8510296a..863a14e1df 100644 --- a/st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_additional_properties_1/config.schema.yaml +++ b/st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_additional_properties_1/config.schema.yaml @@ -1,6 +1,6 @@ --- - regions: - type: "array" + region: + type: "string" required: false default: "us-east-1" profiles: From 8bf7d2a79a063082126057958d7fe00e71578f2f Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Thu, 8 Apr 2021 20:38:58 -0400 Subject: [PATCH 10/20] Changelog entry for pack config additionalProperties support --- CHANGELOG.rst | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 0a5bdc5712..31c76c4823 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -42,6 +42,11 @@ Added Contributed by @Kami. +* Fix a bug in the pack config loader so that objects covered by an additionalProperties schema + can use encrypted datastore keys and have their default values applied correctly. + + Contributed by @cognifloyd. + Changed ~~~~~~~ From a2644c2f5ac649ca469111cf15a9fb1176e70607 Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Fri, 9 Apr 2021 15:56:24 -0400 Subject: [PATCH 11/20] Handle defaultdict schema correctly --- st2common/st2common/util/config_loader.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/st2common/st2common/util/config_loader.py b/st2common/st2common/util/config_loader.py index cff48a03c8..e958bc755d 100644 --- a/st2common/st2common/util/config_loader.py +++ b/st2common/st2common/util/config_loader.py @@ -135,7 +135,11 @@ def _assign_dynamic_config_values(self, schema, config, parent_keys=None): for config_item_key, config_item_value in iterator: if config_is_dict: # different schema for each key/value pair - schema_item = schema.get(config_item_key, {}) + try: + # do not use schema.get() as schema might be a defaultdict + schema_item = schema[config_item_key] + except KeyError: + schema_item = {} if config_is_list: # same schema is shared between every item in the list schema_item = schema From b7672edea3ab3c735958e29f373bc1fbe7d726ce Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Fri, 9 Apr 2021 17:17:11 -0400 Subject: [PATCH 12/20] Simplify based on PR feedback --- CHANGELOG.rst | 2 +- st2common/st2common/util/config_loader.py | 38 ++++++++++------------- 2 files changed, 18 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 31c76c4823..293b24bea1 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -43,7 +43,7 @@ Added Contributed by @Kami. * Fix a bug in the pack config loader so that objects covered by an additionalProperties schema - can use encrypted datastore keys and have their default values applied correctly. + can use encrypted datastore keys and have their default values applied correctly. #5225 Contributed by @cognifloyd. diff --git a/st2common/st2common/util/config_loader.py b/st2common/st2common/util/config_loader.py index e958bc755d..9f564c5400 100644 --- a/st2common/st2common/util/config_loader.py +++ b/st2common/st2common/util/config_loader.py @@ -16,8 +16,6 @@ from __future__ import absolute_import import copy -from collections import defaultdict - import six from oslo_config import cfg @@ -101,16 +99,19 @@ def _get_values_for_config(self, config_schema_db, config_db): return config @staticmethod - def _get_object_property_schema(object_schema, init_additional_properties=None): + 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): - property_schema = defaultdict(lambda: additional_properties) - else: - property_schema = {} - if init_additional_properties: - # ensure that these keys are present in the object (vs just defaultdict) - 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: + property_schema[key] = additional_properties property_schema.update(object_schema.get("properties", {})) return property_schema @@ -135,11 +136,7 @@ def _assign_dynamic_config_values(self, schema, config, parent_keys=None): for config_item_key, config_item_value in iterator: if config_is_dict: # different schema for each key/value pair - try: - # do not use schema.get() as schema might be a defaultdict - schema_item = schema[config_item_key] - except KeyError: - schema_item = {} + schema_item = schema.get(config_item_key, {}) if config_is_list: # same schema is shared between every item in the list schema_item = schema @@ -150,7 +147,10 @@ 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) + property_schema = self._get_object_property_schema( + schema_item, + additional_properties_keys=config_item_value.keys(), + ) self._assign_dynamic_config_values( schema=property_schema, config=config[config_item_key], @@ -216,11 +216,7 @@ def _assign_default_values(self, schema, config): property_schema = self._get_object_property_schema( schema_item, - init_additional_properties=( - config[schema_item_key].keys() - if has_additional_properties - else None - ), + additional_properties_keys=config[schema_item_key].keys(), ) self._assign_default_values( From e69fbae4fb83d8eca4f20719d1e1bfa7e6d7d0a2 Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Fri, 9 Apr 2021 20:56:18 -0400 Subject: [PATCH 13/20] add clarifying comment in test --- st2common/tests/unit/test_config_loader.py | 1 + 1 file changed, 1 insertion(+) diff --git a/st2common/tests/unit/test_config_loader.py b/st2common/tests/unit/test_config_loader.py index 3c35821511..47ac1b6ff6 100644 --- a/st2common/tests/unit/test_config_loader.py +++ b/st2common/tests/unit/test_config_loader.py @@ -545,6 +545,7 @@ def test_get_config_dynamic_config_item_under_additional_properties(self): "host": "127.1.2.7", "port": 8282, # encrypted in datastore + # (schema declares `secret: true` which triggers auto-decryption) "token": "{{st2kv.system.k1_encrypted}}", }, } From a4838885dfac4134589162f9eb59c771d5fa24ed Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Fri, 9 Apr 2021 21:46:19 -0400 Subject: [PATCH 14/20] fix parent_keys in config_loader --- st2common/st2common/util/config_loader.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/st2common/st2common/util/config_loader.py b/st2common/st2common/util/config_loader.py index 9f564c5400..00f444437e 100644 --- a/st2common/st2common/util/config_loader.py +++ b/st2common/st2common/util/config_loader.py @@ -144,9 +144,11 @@ def _assign_dynamic_config_values(self, schema, config, parent_keys=None): is_dictionary = isinstance(config_item_value, dict) is_list = isinstance(config_item_value, list) + # pass a copy of parent_keys so the loop doesn't add sibling keys + current_keys = parent_keys + [str(config_item_keys)] + # 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(), @@ -154,15 +156,14 @@ def _assign_dynamic_config_values(self, schema, config, parent_keys=None): self._assign_dynamic_config_values( schema=property_schema, config=config[config_item_key], - parent_keys=parent_keys, + parent_keys=current_keys, ) # Inspect nested list items elif is_list: - parent_keys += [str(config_item_key)] self._assign_dynamic_config_values( schema=schema_item.get("items", {}), config=config[config_item_key], - parent_keys=parent_keys, + parent_keys=current_keys, ) else: is_jinja_expression = jinja_utils.is_jinja_expression( @@ -171,9 +172,7 @@ def _assign_dynamic_config_values(self, schema, config, parent_keys=None): if is_jinja_expression: # Resolve / render the Jinja template expression - full_config_item_key = ".".join( - parent_keys + [str(config_item_key)] - ) + full_config_item_key = ".".join(current_keys) value = self._get_datastore_value_for_expression( key=full_config_item_key, value=config_item_value, From d67b3c957832d30afd143ede45cd507de25e8cdf Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Fri, 9 Apr 2021 21:49:29 -0400 Subject: [PATCH 15/20] drop test with unencrypted key in secret: true --- st2common/tests/unit/test_config_loader.py | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/st2common/tests/unit/test_config_loader.py b/st2common/tests/unit/test_config_loader.py index 47ac1b6ff6..e49bef3960 100644 --- a/st2common/tests/unit/test_config_loader.py +++ b/st2common/tests/unit/test_config_loader.py @@ -522,7 +522,6 @@ 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) ) @@ -535,18 +534,13 @@ def test_get_config_dynamic_config_item_under_additional_properties(self): # 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 - # (schema declares `secret: true` which triggers auto-decryption) "token": "{{st2kv.system.k1_encrypted}}", + # schema declares `secret: true` which triggers auto-decryption. + # If this were not encrypted, it would try to decrypt it and fail. }, } } @@ -565,11 +559,6 @@ def test_get_config_dynamic_config_item_under_additional_properties(self): "port": 8080, "token": "hard-coded-secret", }, - "stage": { - "host": "127.0.0.1", - "port": 8181, - "token": "v0", - }, "prod": { "host": "127.1.2.7", "port": 8282, From 4d3be4a1e50b82ce4c57d3e3d6ae8667d654e7e5 Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Fri, 9 Apr 2021 21:55:16 -0400 Subject: [PATCH 16/20] typo --- st2common/st2common/util/config_loader.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/st2common/st2common/util/config_loader.py b/st2common/st2common/util/config_loader.py index 00f444437e..aec424d75e 100644 --- a/st2common/st2common/util/config_loader.py +++ b/st2common/st2common/util/config_loader.py @@ -145,7 +145,7 @@ def _assign_dynamic_config_values(self, schema, config, parent_keys=None): is_list = isinstance(config_item_value, list) # pass a copy of parent_keys so the loop doesn't add sibling keys - current_keys = parent_keys + [str(config_item_keys)] + current_keys = parent_keys + [str(config_item_key)] # Inspect nested object properties if is_dictionary: From 574034f7636350dbb3ed81c3489278fec3b38fe6 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Sat, 10 Apr 2021 11:22:06 +0200 Subject: [PATCH 17/20] Fix failing test - we need to pass encrypted value to the KVP model. --- st2common/tests/unit/test_config_loader.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/st2common/tests/unit/test_config_loader.py b/st2common/tests/unit/test_config_loader.py index e49bef3960..d8616a3b44 100644 --- a/st2common/tests/unit/test_config_loader.py +++ b/st2common/tests/unit/test_config_loader.py @@ -19,8 +19,10 @@ from st2common.models.db.keyvalue import KeyValuePairDB from st2common.exceptions.db import StackStormDBObjectNotFoundError from st2common.persistence.keyvalue import KeyValuePair +from st2common.models.api.keyvalue import KeyValuePairAPI from st2common.services.config import set_datastore_value_for_config_key from st2common.util.config_loader import ContentPackConfigLoader +from st2common.util import crypto from st2tests.base import CleanDbTestCase @@ -522,8 +524,11 @@ 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) + encrypted_value = crypto.symmetric_encrypt( + KeyValuePairAPI.crypto_key, "v1_encrypted" + ) KeyValuePair.add_or_update( - KeyValuePairDB(name="k1_encrypted", value="v1_encrypted", secret=True) + KeyValuePairDB(name="k1_encrypted", value=encrypted_value, secret=True) ) #################### From d0d99249b5b949084098e0d4477fcc90e2164758 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Sat, 10 Apr 2021 12:42:24 +0200 Subject: [PATCH 18/20] Add a temporary workaround to try to retry integration tests which fail quite often due to races, etc. on failure. This saves us a bunch of time manually re-running the whole workflow. --- .github/workflows/ci.yaml | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 46f8508d98..b71ea33728 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -465,8 +465,22 @@ jobs: ./scripts/ci/print-versions.sh - name: make # use: script -e -c to print colors + env: + MAX_ATTEMPTS: 3 run: | - script -e -c "make ${TASK}" + # There is a race in some orequesta integration tests so they tend to fail quite often. + # To avoid needed to re-run whole workflow in such case, we should try to retry this + # specific step. This saves us a bunch of time manually re-running the whole workflow. + set +e + for i in $(seq 1 ${MAX_ATTEMPTS}); do + echo "Attempt: ${i}" + script -e -c "make ${TASK}" && exit 0 + sleep 5 + done + + set -e + echo "Failed after ${MAX_ATTEMPTS} attempts" + exit 1 - name: Codecov # NOTE: We only generate and submit coverage report for master and version branches and only when the build succeeds (default on GitHub Actions, this was not the case on Travis so we had to explicitly check success) if: "${{ success() && env.ENABLE_COVERAGE == 'yes' && env.TASK == 'ci-integration' }}" From 6bfa39015aee1dda54ac16ac992c2a72fafee100 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Sat, 10 Apr 2021 12:44:04 +0200 Subject: [PATCH 19/20] Add TODO note. --- .github/workflows/ci.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index b71ea33728..60c694f42a 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -471,6 +471,8 @@ jobs: # There is a race in some orequesta integration tests so they tend to fail quite often. # To avoid needed to re-run whole workflow in such case, we should try to retry this # specific step. This saves us a bunch of time manually re-running the whole workflow. + # TODO: Try to identify problematic tests (iirc mostly orquesta ones) and only retry / + # re-run those. set +e for i in $(seq 1 ${MAX_ATTEMPTS}); do echo "Attempt: ${i}" From 1d613bccadc6b1ef604fba7fe275f160a58b41ff Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Sat, 10 Apr 2021 13:02:13 +0200 Subject: [PATCH 20/20] Update logging, add timeout-minutes to the step as a safe guard. --- .github/workflows/ci.yaml | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 60c694f42a..409f5bd93e 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -464,9 +464,11 @@ jobs: run: | ./scripts/ci/print-versions.sh - name: make - # use: script -e -c to print colors + timeout-minutes: 25 # may die if rabbitmq fails to start env: MAX_ATTEMPTS: 3 + RETRY_DELAY: 5 + # use: script -e -c to print colors run: | # There is a race in some orequesta integration tests so they tend to fail quite often. # To avoid needed to re-run whole workflow in such case, we should try to retry this @@ -475,13 +477,16 @@ jobs: # re-run those. set +e for i in $(seq 1 ${MAX_ATTEMPTS}); do - echo "Attempt: ${i}" + echo "Attempt: ${i}/${MAX_ATTEMPTS}" + script -e -c "make ${TASK}" && exit 0 - sleep 5 + + echo "Command failed, will retry in ${RETRY_DELAY} seconds..." + sleep ${RETRY_DELAY} done set -e - echo "Failed after ${MAX_ATTEMPTS} attempts" + echo "Failed after ${MAX_ATTEMPTS} attempts, failing the job." exit 1 - name: Codecov # NOTE: We only generate and submit coverage report for master and version branches and only when the build succeeds (default on GitHub Actions, this was not the case on Travis so we had to explicitly check success)