diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 46f8508d98..409f5bd93e 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -464,9 +464,30 @@ jobs: run: | ./scripts/ci/print-versions.sh - name: make + 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: | - 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. + # 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}/${MAX_ATTEMPTS}" + + script -e -c "make ${TASK}" && exit 0 + + echo "Command failed, will retry in ${RETRY_DELAY} seconds..." + sleep ${RETRY_DELAY} + done + + set -e + 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) if: "${{ success() && env.ENABLE_COVERAGE == 'yes' && env.TASK == 'ci-integration' }}" diff --git a/CHANGELOG.rst b/CHANGELOG.rst index ba3a877491..3c17f7fdd8 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -44,6 +44,11 @@ Added * Make redis the default coordinator backend. +* 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. #5225 + + Contributed by @cognifloyd. + Changed ~~~~~~~ diff --git a/st2common/st2common/util/config_loader.py b/st2common/st2common/util/config_loader.py index 30db039bdc..aec424d75e 100644 --- a/st2common/st2common/util/config_loader.py +++ b/st2common/st2common/util/config_loader.py @@ -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: + 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 @@ -127,21 +144,26 @@ 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_key)] + # 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, + 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( @@ -150,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, @@ -182,18 +202,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 diff --git a/st2common/tests/unit/test_config_loader.py b/st2common/tests/unit/test_config_loader.py index e1849d7868..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 @@ -43,7 +45,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 +520,61 @@ 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) + + encrypted_value = crypto.symmetric_encrypt( + KeyValuePairAPI.crypto_key, "v1_encrypted" + ) + KeyValuePair.add_or_update( + KeyValuePairDB(name="k1_encrypted", value=encrypted_value, 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", + }, + "prod": { + "host": "127.1.2.7", + "port": 8282, + # encrypted in datastore + "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. + }, + } + } + 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", + }, + "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..863a14e1df --- /dev/null +++ b/st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_additional_properties_1/config.schema.yaml @@ -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 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