Skip to content

Commit f40a24f

Browse files
KamiKami
authored
Merge pull request #5225 from cognifloyd/pack-config-additionalproperties
Fix decrypting pack config keys under additionalProperties
2 parents 5a08b5b + 1d613bc commit f40a24f

File tree

6 files changed

+151
-12
lines changed

6 files changed

+151
-12
lines changed

‎.github/workflows/ci.yaml

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -464,9 +464,30 @@ jobs:
464464
run: |
465465
./scripts/ci/print-versions.sh
466466
- name: make
467+
timeout-minutes: 25 # may die if rabbitmq fails to start
468+
env:
469+
MAX_ATTEMPTS: 3
470+
RETRY_DELAY: 5
467471
# use: script -e -c to print colors
468472
run: |
469-
script -e -c "make ${TASK}"
473+
# There is a race in some orequesta integration tests so they tend to fail quite often.
474+
# To avoid needed to re-run whole workflow in such case, we should try to retry this
475+
# specific step. This saves us a bunch of time manually re-running the whole workflow.
476+
# TODO: Try to identify problematic tests (iirc mostly orquesta ones) and only retry /
477+
# re-run those.
478+
set +e
479+
for i in $(seq 1 ${MAX_ATTEMPTS}); do
480+
echo "Attempt: ${i}/${MAX_ATTEMPTS}"
481+
482+
script -e -c "make ${TASK}" && exit 0
483+
484+
echo "Command failed, will retry in ${RETRY_DELAY} seconds..."
485+
sleep ${RETRY_DELAY}
486+
done
487+
488+
set -e
489+
echo "Failed after ${MAX_ATTEMPTS} attempts, failing the job."
490+
exit 1
470491
- name: Codecov
471492
# 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)
472493
if: "${{ success() && env.ENABLE_COVERAGE == 'yes' && env.TASK == 'ci-integration' }}"

‎CHANGELOG.rst

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,11 @@ Added
4444

4545
* Make redis the default coordinator backend.
4646

47+
* Fix a bug in the pack config loader so that objects covered by an additionalProperties schema
48+
can use encrypted datastore keys and have their default values applied correctly. #5225
49+
50+
Contributed by @cognifloyd.
51+
4752
Changed
4853
~~~~~~~
4954

‎st2common/st2common/util/config_loader.py

Lines changed: 36 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,23 @@ def _get_values_for_config(self, config_schema_db, config_db):
9898
config = self._assign_default_values(schema=schema_values, config=config)
9999
return config
100100

101+
@staticmethod
102+
def _get_object_property_schema(object_schema, additional_properties_keys=None):
103+
"""
104+
Create a schema for an object property using both additionalProperties and properties.
105+
106+
:rtype: ``dict``
107+
"""
108+
property_schema = {}
109+
additional_properties = object_schema.get("additionalProperties", {})
110+
# additionalProperties can be a boolean or a dict
111+
if additional_properties and isinstance(additional_properties, dict):
112+
# ensure that these keys are present in the object
113+
for key in additional_properties_keys:
114+
property_schema[key] = additional_properties
115+
property_schema.update(object_schema.get("properties", {}))
116+
return property_schema
117+
101118
def _assign_dynamic_config_values(self, schema, config, parent_keys=None):
102119
"""
103120
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):
127144
is_dictionary = isinstance(config_item_value, dict)
128145
is_list = isinstance(config_item_value, list)
129146

147+
# pass a copy of parent_keys so the loop doesn't add sibling keys
148+
current_keys = parent_keys + [str(config_item_key)]
149+
130150
# Inspect nested object properties
131151
if is_dictionary:
132-
parent_keys += [str(config_item_key)]
152+
property_schema = self._get_object_property_schema(
153+
schema_item,
154+
additional_properties_keys=config_item_value.keys(),
155+
)
133156
self._assign_dynamic_config_values(
134-
schema=schema_item.get("properties", {}),
157+
schema=property_schema,
135158
config=config[config_item_key],
136-
parent_keys=parent_keys,
159+
parent_keys=current_keys,
137160
)
138161
# Inspect nested list items
139162
elif is_list:
140-
parent_keys += [str(config_item_key)]
141163
self._assign_dynamic_config_values(
142164
schema=schema_item.get("items", {}),
143165
config=config[config_item_key],
144-
parent_keys=parent_keys,
166+
parent_keys=current_keys,
145167
)
146168
else:
147169
is_jinja_expression = jinja_utils.is_jinja_expression(
@@ -150,9 +172,7 @@ def _assign_dynamic_config_values(self, schema, config, parent_keys=None):
150172

151173
if is_jinja_expression:
152174
# Resolve / render the Jinja template expression
153-
full_config_item_key = ".".join(
154-
parent_keys + [str(config_item_key)]
155-
)
175+
full_config_item_key = ".".join(current_keys)
156176
value = self._get_datastore_value_for_expression(
157177
key=full_config_item_key,
158178
value=config_item_value,
@@ -182,18 +202,24 @@ def _assign_default_values(self, schema, config):
182202
default_value = schema_item.get("default", None)
183203
is_object = schema_item.get("type", None) == "object"
184204
has_properties = schema_item.get("properties", None)
205+
has_additional_properties = schema_item.get("additionalProperties", None)
185206

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

190211
# Inspect nested object properties
191-
if is_object and has_properties:
212+
if is_object and (has_properties or has_additional_properties):
192213
if not config.get(schema_item_key, None):
193214
config[schema_item_key] = {}
194215

216+
property_schema = self._get_object_property_schema(
217+
schema_item,
218+
additional_properties_keys=config[schema_item_key].keys(),
219+
)
220+
195221
self._assign_default_values(
196-
schema=schema_item["properties"], config=config[schema_item_key]
222+
schema=property_schema, config=config[schema_item_key]
197223
)
198224

199225
return config

‎st2common/tests/unit/test_config_loader.py

Lines changed: 58 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,10 @@
1919
from st2common.models.db.keyvalue import KeyValuePairDB
2020
from st2common.exceptions.db import StackStormDBObjectNotFoundError
2121
from st2common.persistence.keyvalue import KeyValuePair
22+
from st2common.models.api.keyvalue import KeyValuePairAPI
2223
from st2common.services.config import set_datastore_value_for_config_key
2324
from st2common.util.config_loader import ContentPackConfigLoader
25+
from st2common.util import crypto
2426

2527
from st2tests.base import CleanDbTestCase
2628

@@ -43,7 +45,7 @@ def test_ensure_local_pack_config_feature_removed(self):
4345

4446
def test_get_config_some_values_overriden_in_datastore(self):
4547
# Test a scenario where some values are overriden in datastore via pack
46-
# flobal config
48+
# global config
4749
kvp_db = set_datastore_value_for_config_key(
4850
pack_name="dummy_pack_5",
4951
key_name="api_secret",
@@ -518,6 +520,61 @@ def test_get_config_dynamic_config_item_nested_list(self):
518520

519521
config_db.delete()
520522

523+
def test_get_config_dynamic_config_item_under_additional_properties(self):
524+
pack_name = "dummy_pack_schema_with_additional_properties_1"
525+
loader = ContentPackConfigLoader(pack_name=pack_name)
526+
527+
encrypted_value = crypto.symmetric_encrypt(
528+
KeyValuePairAPI.crypto_key, "v1_encrypted"
529+
)
530+
KeyValuePair.add_or_update(
531+
KeyValuePairDB(name="k1_encrypted", value=encrypted_value, secret=True)
532+
)
533+
534+
####################
535+
# values in objects under an object with additionalProperties
536+
values = {
537+
"profiles": {
538+
"dev": {
539+
# no host or port to test default value
540+
"token": "hard-coded-secret",
541+
},
542+
"prod": {
543+
"host": "127.1.2.7",
544+
"port": 8282,
545+
# encrypted in datastore
546+
"token": "{{st2kv.system.k1_encrypted}}",
547+
# schema declares `secret: true` which triggers auto-decryption.
548+
# If this were not encrypted, it would try to decrypt it and fail.
549+
},
550+
}
551+
}
552+
config_db = ConfigDB(pack=pack_name, values=values)
553+
config_db = Config.add_or_update(config_db)
554+
555+
config_rendered = loader.get_config()
556+
557+
self.assertEqual(
558+
config_rendered,
559+
{
560+
"region": "us-east-1",
561+
"profiles": {
562+
"dev": {
563+
"host": "127.0.0.3",
564+
"port": 8080,
565+
"token": "hard-coded-secret",
566+
},
567+
"prod": {
568+
"host": "127.1.2.7",
569+
"port": 8282,
570+
"token": "v1_encrypted",
571+
},
572+
},
573+
},
574+
)
575+
576+
config_db.delete()
577+
521578
def test_empty_config_object_in_the_database(self):
522579
pack_name = "dummy_pack_empty_config"
523580

‎st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_additional_properties_1/config.schema.yaml

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
---
2+
region:
3+
type: "string"
4+
required: false
5+
default: "us-east-1"
6+
profiles:
7+
type: "object"
8+
required: false
9+
additionalProperties:
10+
type: object
11+
additionalProperties: false
12+
properties:
13+
host:
14+
type: "string"
15+
required: false
16+
default: "127.0.0.3"
17+
port:
18+
type: "integer"
19+
required: false
20+
default: 8080
21+
token:
22+
type: "string"
23+
required: true
24+
secret: true

‎st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_additional_properties_1/pack.yaml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
name : dummy_pack_schema_with_additional_properties_1
3+
description : dummy pack with nested objects under additionalProperties
4+
version : 0.1.0
5+
author : st2-dev
6+
email : [email protected]

0 commit comments

Comments
 (0)