Merge pull request #5225 from cognifloyd/pack-config-additionalproper…

…ties

Fix decrypting pack config keys under additionalProperties

.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' }}"

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
 ~~~~~~~
 

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

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"
 

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

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 : [email protected]