From 1bbf0be02129ed6cc854dc45c07a0ab71fae8b5d Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Wed, 4 Aug 2021 10:31:22 -0500 Subject: [PATCH 01/36] protect more datatypes in output_value inspection --- st2common/st2common/util/output_schema.py | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/st2common/st2common/util/output_schema.py b/st2common/st2common/util/output_schema.py index d4489d3e2b..a151f1ed0f 100644 --- a/st2common/st2common/util/output_schema.py +++ b/st2common/st2common/util/output_schema.py @@ -54,10 +54,7 @@ def _validate_action(action_schema, result, output_key): def mask_secret_output(ac_ex, output_value): - if not output_value: - return output_value - - if not isinstance(output_value, dict) and not isinstance(output_value, list): + if not output_value or not isinstance(output_value, Collection): return output_value output_key = ac_ex["runner"].get("output_key") @@ -66,13 +63,14 @@ def mask_secret_output(ac_ex, output_value): if not output_key or not output_schema: return output_value - if not output_value.get(output_key): + if not output_value.get(output_key) or not isinstance( + output_value[output_key], Collection + ): return output_value for key, spec in output_schema.items(): - if isinstance(output_value[output_key], Collection): - if key in output_value[output_key] and spec.get("secret", False): - output_value[output_key][key] = MASKED_ATTRIBUTE_VALUE + if key in output_value[output_key] and spec.get("secret", False): + output_value[output_key][key] = MASKED_ATTRIBUTE_VALUE return output_value From 44874e4b3661c36106868642884fb4a58aa71225 Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Wed, 4 Aug 2021 11:54:46 -0500 Subject: [PATCH 02/36] add tests for output_schema validation --- st2common/st2common/util/output_schema.py | 24 +++++++++-- .../tests/unit/test_util_output_schema.py | 42 ++++++++++++++++++- 2 files changed, 62 insertions(+), 4 deletions(-) diff --git a/st2common/st2common/util/output_schema.py b/st2common/st2common/util/output_schema.py index a151f1ed0f..424a9cde9e 100644 --- a/st2common/st2common/util/output_schema.py +++ b/st2common/st2common/util/output_schema.py @@ -18,7 +18,7 @@ import traceback import jsonschema -from collections.abc import Collection +from collections.abc import Mapping from st2common.util import schema from st2common.constants import action as action_constants from st2common.constants.secrets import MASKED_ATTRIBUTE_VALUE @@ -54,21 +54,39 @@ def _validate_action(action_schema, result, output_key): def mask_secret_output(ac_ex, output_value): - if not output_value or not isinstance(output_value, Collection): + # We only support output_schema validation when the output_value is a JSON object. + # Invididual keys of that object can be marked secret, but the entire output + # object cannot be marked as secret. + # FIXME: Should we support non-objects under output_key? + # Changing that will require changes in one or more of: + # st2common/util/schema/action_output_schema.json + # st2common/util/schema/__init__.py + # st2common/models/api/action.py + + if not output_value or not isinstance(output_value, Mapping): return output_value output_key = ac_ex["runner"].get("output_key") output_schema = ac_ex["action"].get("output_schema") + # nothing to validate if not output_key or not output_schema: return output_value + # The schema is for the keys of an object if not output_value.get(output_key) or not isinstance( - output_value[output_key], Collection + output_value[output_key], Mapping ): return output_value + # malformed schema + if not isinstance(schema, Mapping): + return output_value + for key, spec in output_schema.items(): + # malformed schema spec for this property/key + if not isinstance(spec, Mapping): + continue if key in output_value[output_key] and spec.get("secret", False): output_value[output_key][key] = MASKED_ATTRIBUTE_VALUE diff --git a/st2common/tests/unit/test_util_output_schema.py b/st2common/tests/unit/test_util_output_schema.py index 64ed13237e..fc056633b1 100644 --- a/st2common/tests/unit/test_util_output_schema.py +++ b/st2common/tests/unit/test_util_output_schema.py @@ -220,14 +220,54 @@ def test_mask_secret_output_noop(self): masked_output = output_schema.mask_secret_output(ac_ex, ac_ex_result) self.assertDictEqual(masked_output, expected_masked_output) - # The output is not type of dict or list. + # The output is string type: not dict or list ac_ex_result = {"output": "foobar"} expected_masked_output = {"output": "foobar"} masked_output = output_schema.mask_secret_output(ac_ex, ac_ex_result) self.assertDictEqual(masked_output, expected_masked_output) + # The output is number / int type: not dict or list + ac_ex_result = {"output": 42} + expected_masked_output = {"output": 42} + masked_output = output_schema.mask_secret_output(ac_ex, ac_ex_result) + self.assertDictEqual(masked_output, expected_masked_output) + + # The output is number / float type: not dict or list + ac_ex_result = {"output": 4.2} + expected_masked_output = {"output": 4.2} + masked_output = output_schema.mask_secret_output(ac_ex, ac_ex_result) + self.assertDictEqual(masked_output, expected_masked_output) + + # The output is True (bool type): not dict or list + ac_ex_result = {"output": True} + expected_masked_output = {"output": True} + masked_output = output_schema.mask_secret_output(ac_ex, ac_ex_result) + self.assertDictEqual(masked_output, expected_masked_output) + + # The output is False (bool type): not dict or list + ac_ex_result = {"output": False} + expected_masked_output = {"output": False} + masked_output = output_schema.mask_secret_output(ac_ex, ac_ex_result) + self.assertDictEqual(masked_output, expected_masked_output) + + # The output is list type + ac_ex_result = {"output": ["foobar"]} + expected_masked_output = {"output": ["foobar"]} + masked_output = output_schema.mask_secret_output(ac_ex, ac_ex_result) + self.assertDictEqual(masked_output, expected_masked_output) + # The output key is missing. ac_ex_result = {"output1": None} expected_masked_output = {"output1": None} masked_output = output_schema.mask_secret_output(ac_ex, ac_ex_result) self.assertDictEqual(masked_output, expected_masked_output) + + # Malformed schema can't be used to validate. + malformed_schema_ac_ex = copy.deepcopy(ac_ex) + malformed_schema_ac_ex["action"]["output_schema"] = {"output_1": "bool"} + ac_ex_result = {"output_1": "foobar"} + expected_masked_output = {"output_1": "foobar"} + masked_output = output_schema.mask_secret_output( + malformed_schema_ac_ex, ac_ex_result + ) + self.assertDictEqual(masked_output, expected_masked_output) From 61f10a63fb6af4c6bad8721b6ef190bff3af5a3a Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Wed, 4 Aug 2021 12:18:05 -0500 Subject: [PATCH 03/36] adjust test comments --- st2common/tests/unit/test_util_output_schema.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/st2common/tests/unit/test_util_output_schema.py b/st2common/tests/unit/test_util_output_schema.py index fc056633b1..f3f4ba072a 100644 --- a/st2common/tests/unit/test_util_output_schema.py +++ b/st2common/tests/unit/test_util_output_schema.py @@ -220,37 +220,37 @@ def test_mask_secret_output_noop(self): masked_output = output_schema.mask_secret_output(ac_ex, ac_ex_result) self.assertDictEqual(masked_output, expected_masked_output) - # The output is string type: not dict or list + # The output is string type: not dict ac_ex_result = {"output": "foobar"} expected_masked_output = {"output": "foobar"} masked_output = output_schema.mask_secret_output(ac_ex, ac_ex_result) self.assertDictEqual(masked_output, expected_masked_output) - # The output is number / int type: not dict or list + # The output is number / int type: not dict ac_ex_result = {"output": 42} expected_masked_output = {"output": 42} masked_output = output_schema.mask_secret_output(ac_ex, ac_ex_result) self.assertDictEqual(masked_output, expected_masked_output) - # The output is number / float type: not dict or list + # The output is number / float type: not dict ac_ex_result = {"output": 4.2} expected_masked_output = {"output": 4.2} masked_output = output_schema.mask_secret_output(ac_ex, ac_ex_result) self.assertDictEqual(masked_output, expected_masked_output) - # The output is True (bool type): not dict or list + # The output is True (bool type): not dict ac_ex_result = {"output": True} expected_masked_output = {"output": True} masked_output = output_schema.mask_secret_output(ac_ex, ac_ex_result) self.assertDictEqual(masked_output, expected_masked_output) - # The output is False (bool type): not dict or list + # The output is False (bool type): not dict ac_ex_result = {"output": False} expected_masked_output = {"output": False} masked_output = output_schema.mask_secret_output(ac_ex, ac_ex_result) self.assertDictEqual(masked_output, expected_masked_output) - # The output is list type + # The output is list type: not dict ac_ex_result = {"output": ["foobar"]} expected_masked_output = {"output": ["foobar"]} masked_output = output_schema.mask_secret_output(ac_ex, ac_ex_result) From a129c8d0f3bc8691ad6af643f0e843fcf01056df Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Wed, 4 Aug 2021 12:53:45 -0500 Subject: [PATCH 04/36] Fix var usage --- st2common/st2common/util/output_schema.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/st2common/st2common/util/output_schema.py b/st2common/st2common/util/output_schema.py index 424a9cde9e..c3e944535c 100644 --- a/st2common/st2common/util/output_schema.py +++ b/st2common/st2common/util/output_schema.py @@ -80,7 +80,7 @@ def mask_secret_output(ac_ex, output_value): return output_value # malformed schema - if not isinstance(schema, Mapping): + if not isinstance(output_schema, Mapping): return output_value for key, spec in output_schema.items(): From 6524d74b9603db2bdc73ffbd363e5cc14e85abc0 Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Thu, 5 Aug 2021 22:44:44 -0500 Subject: [PATCH 05/36] Add recursive _get_masked_value function This should handle any nested objects/arrays etc. We might need to adjust the output_schema to support non-objects but that can be done separately, I hope. --- st2common/st2common/util/output_schema.py | 120 ++++++++++++++++++---- 1 file changed, 102 insertions(+), 18 deletions(-) diff --git a/st2common/st2common/util/output_schema.py b/st2common/st2common/util/output_schema.py index c3e944535c..29405df5ac 100644 --- a/st2common/st2common/util/output_schema.py +++ b/st2common/st2common/util/output_schema.py @@ -14,6 +14,7 @@ # limitations under the License. import logging +import re import sys import traceback import jsonschema @@ -53,15 +54,88 @@ def _validate_action(action_schema, result, output_key): schema.validate(final_result, action_schema, cls=schema.get_validator("custom")) +def _get_masked_value(spec, value): + # malformed schema + if not isinstance(spec, Mapping): + return value + + if spec.get("secret", False): + return MASKED_ATTRIBUTE_VALUE + + kind = spec.get("type") + + if kind in ("boolean", "integer", "null", "number", "string"): + # already checked for spec["secret"] above; nothing else to check. + return value + + elif kind == "object": + properties_schema = spec.get("properties", {}) + if properties_schema and isinstance(properties_schema, Mapping): + # properties is not empty or malformed + for key, property_spec in properties_schema.items(): + if key in value: + value[key] = _get_masked_value(property_spec, value[key]) + unhandled_keys = set(value.keys()) - set(properties_schema.keys()) + else: + # properties is empty or malformed + unhandled_keys = set(value.keys()) + + pattern_properties_schema = spec.get("patternProperties") + if pattern_properties_schema and isinstance(pattern_properties_schema, Mapping): + # patternProperties is not malformed + for key_pattern, pattern_property_spec in pattern_properties_schema: + key_re = re.compile(key_pattern) + for key in list(unhandled_keys): + if key_re.search(key): + value[key] = _get_masked_value( + pattern_property_spec, value[key] + ) + unhandled_keys.remove(key) + + additional_properties_schema = spec.get("additionalProperties") + if additional_properties_schema and isinstance( + additional_properties_schema, Mapping + ): + # additionalProperties is a schema, not a boolean, and not malformed + for key in unhandled_keys: + value[key] = _get_masked_value(additional_properties_schema, value[key]) + + return value + + elif kind == "array": + items_schema = spec.get("items", {}) + output_count = len(value) + if isinstance(items_schema, Mapping): + # explicit schema for each item + for i, item_spec in enumerate(items_schema): + if i >= output_count: + break + value[i] = _get_masked_value(item_spec, value[key]) + handled_count = len(items_schema) + else: + for i in range(output_count): + value[i] = _get_masked_value(items_schema, value[key]) + handled_count = output_count + + if handled_count >= output_count: + return value + + additional_items_schema = spec.get("additionalItems") + if additional_items_schema and isinstance(additional_items_schema, Mapping): + # additionalItems is a schema, not a boolean + for i in range(handled_count, output_count): + value[i] = _get_masked_value(additional_items_schema, value[i]) + + return value + else: + # "type" is not defined or is invalid: ignore it + return value + + def mask_secret_output(ac_ex, output_value): # We only support output_schema validation when the output_value is a JSON object. # Invididual keys of that object can be marked secret, but the entire output # object cannot be marked as secret. - # FIXME: Should we support non-objects under output_key? - # Changing that will require changes in one or more of: - # st2common/util/schema/action_output_schema.json - # st2common/util/schema/__init__.py - # st2common/models/api/action.py if not output_value or not isinstance(output_value, Mapping): return output_value @@ -73,23 +147,33 @@ def mask_secret_output(ac_ex, output_value): if not output_key or not output_schema: return output_value - # The schema is for the keys of an object - if not output_value.get(output_key) or not isinstance( - output_value[output_key], Mapping - ): - return output_value - # malformed schema if not isinstance(output_schema, Mapping): return output_value - for key, spec in output_schema.items(): - # malformed schema spec for this property/key - if not isinstance(spec, Mapping): - continue - if key in output_value[output_key] and spec.get("secret", False): - output_value[output_key][key] = MASKED_ATTRIBUTE_VALUE - + # TODO: a better way to see if only the values are valid json schemas, or the whole thing? + if "type" not in output_schema: + # see st2common/st2common/models/api/action.py + # output_schema_schema = { + # "description": "Schema for the runner's/action's output.", + # "type": "object", + # "patternProperties": {r"^\w+$": customized_draft4_jsonschema} + # "additionalProperties": False, + # "default": {}, + # } + # This implies the following schema (as in _validate_runner/_validate_action above) + implied_schema = { + "type": "object", + "properties": output_schema, + "additionalProperties": False, + } + output_value[output_key] = _get_masked_value( + implied_schema, output_value[output_key] + ) + else: + output_value[output_key] = _get_masked_value( + output_schema, output_value[output_key] + ) return output_value From 3becc3ea985f891442c5d0e21b77c9fe9c0d4dc0 Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Fri, 6 Aug 2021 09:12:08 -0500 Subject: [PATCH 06/36] return quickly if output_value[output_key] is wrong type --- st2common/st2common/util/output_schema.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/st2common/st2common/util/output_schema.py b/st2common/st2common/util/output_schema.py index 29405df5ac..9077d3e378 100644 --- a/st2common/st2common/util/output_schema.py +++ b/st2common/st2common/util/output_schema.py @@ -19,7 +19,7 @@ import traceback import jsonschema -from collections.abc import Mapping +from collections.abc import Collection, Mapping from st2common.util import schema from st2common.constants import action as action_constants from st2common.constants.secrets import MASKED_ATTRIBUTE_VALUE @@ -69,6 +69,10 @@ def _get_masked_value(spec, value): return value elif kind == "object": + if not isinstance(value, Mapping): + # we can't process it unless it matches the expected type + return value + properties_schema = spec.get("properties", {}) if properties_schema and isinstance(properties_schema, Mapping): # properties is not empty or malformed @@ -103,6 +107,10 @@ def _get_masked_value(spec, value): return value elif kind == "array": + if not isinstance(value, Collection): + # we can't process it unless it matches the expected type + return value + items_schema = spec.get("items", {}) output_count = len(value) if isinstance(items_schema, Mapping): From 49508d2f6f90584147a00753e7eb841f3c0276e4 Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Fri, 6 Aug 2021 11:56:16 -0500 Subject: [PATCH 07/36] Add coments explaining output_schema validating objects --- st2common/st2common/models/api/action.py | 6 ++++++ st2common/st2common/util/output_schema.py | 14 +++++++------- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/st2common/st2common/models/api/action.py b/st2common/st2common/models/api/action.py index 6c4149745a..33c2956cfe 100644 --- a/st2common/st2common/models/api/action.py +++ b/st2common/st2common/models/api/action.py @@ -116,9 +116,12 @@ class RunnerTypeAPI(BaseAPI): "type": "string", "required": False, }, + # Runners must always output json objects with action output in output_key property "output_schema": { "description": "Schema for the runner's output.", "type": "object", + # using patternProperties like this implies that output_schema defines + # the "properties" schema of an object where each key is a property name. "patternProperties": {r"^\w+$": util_schema.get_action_output_schema()}, "additionalProperties": False, "default": {}, @@ -228,9 +231,12 @@ class ActionAPI(BaseAPI, APIUIDMixin): "additionalProperties": False, "default": {}, }, + # TODO: support validation for non-object action output, possibly w/ anyOf "output_schema": { "description": "Schema for the action's output.", "type": "object", + # using patternProperties like this implies that output_schema defines + # the "properties" schema of an object where each key is a property name. "patternProperties": {r"^\w+$": util_schema.get_action_output_schema()}, "additionalProperties": False, "default": {}, diff --git a/st2common/st2common/util/output_schema.py b/st2common/st2common/util/output_schema.py index 9077d3e378..346a4eefdb 100644 --- a/st2common/st2common/util/output_schema.py +++ b/st2common/st2common/util/output_schema.py @@ -31,6 +31,7 @@ def _validate_runner(runner_schema, result): LOG.debug("Validating runner output: %s", runner_schema) + # runner's output is always an object. runner_schema = { "type": "object", "properties": runner_schema, @@ -45,6 +46,8 @@ def _validate_action(action_schema, result, output_key): final_result = result[output_key] + # TODO: support action output_schema for non-objects + # (see also st2common/models/api/action.py) action_schema = { "type": "object", "properties": action_schema, @@ -141,10 +144,7 @@ def _get_masked_value(spec, value): def mask_secret_output(ac_ex, output_value): - # We only support output_schema validation when the output_value is a JSON object. - # Invididual keys of that object can be marked secret, but the entire output - # object cannot be marked as secret. - + # runners have to return a JSON object, with action output under a subkey. if not output_value or not isinstance(output_value, Mapping): return output_value @@ -152,7 +152,7 @@ def mask_secret_output(ac_ex, output_value): output_schema = ac_ex["action"].get("output_schema") # nothing to validate - if not output_key or not output_schema: + if not output_key or output_key not in output_value or not output_schema: return output_value # malformed schema @@ -163,13 +163,13 @@ def mask_secret_output(ac_ex, output_value): if "type" not in output_schema: # see st2common/st2common/models/api/action.py # output_schema_schema = { - # "description": "Schema for the runner's/action's output.", + # "description": "Schema for the action's output.", # "type": "object", # "patternProperties": {r"^\w+$": customized_draft4_jsonschema} # "additionalProperties": False, # "default": {}, # } - # This implies the following schema (as in _validate_runner/_validate_action above) + # This implies the following schema (as in _validate_action above) implied_schema = { "type": "object", "properties": output_schema, From dd05bc03ac2b4641e755ec18e3f33b02b52262be Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Fri, 6 Aug 2021 14:03:23 -0500 Subject: [PATCH 08/36] check for unhandled_keys before compiling regex --- st2common/st2common/util/output_schema.py | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/st2common/st2common/util/output_schema.py b/st2common/st2common/util/output_schema.py index 346a4eefdb..e2445b65a9 100644 --- a/st2common/st2common/util/output_schema.py +++ b/st2common/st2common/util/output_schema.py @@ -88,9 +88,16 @@ def _get_masked_value(spec, value): unhandled_keys = set(value.keys()) pattern_properties_schema = spec.get("patternProperties") - if pattern_properties_schema and isinstance(pattern_properties_schema, Mapping): + if ( + unhandled_keys + and pattern_properties_schema + and isinstance(pattern_properties_schema, Mapping) + ): # patternProperties is not malformed for key_pattern, pattern_property_spec in pattern_properties_schema: + if not unhandled_keys: + # nothing to check, don't compile the next pattern + break key_re = re.compile(key_pattern) for key in list(unhandled_keys): if key_re.search(key): @@ -100,8 +107,10 @@ def _get_masked_value(spec, value): unhandled_keys.remove(key) additional_properties_schema = spec.get("additionalProperties") - if additional_properties_schema and isinstance( - additional_properties_schema, Mapping + if ( + unhandled_keys + and additional_properties_schema + and isinstance(additional_properties_schema, Mapping) ): # additionalProperties is a schema, not a boolean, and not malformed for key in unhandled_keys: From 2bb40d7b38ca782512a583faf021114631754e32 Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Fri, 6 Aug 2021 14:05:18 -0500 Subject: [PATCH 09/36] Allow action output_schema to be a full jsonschema --- st2common/st2common/models/api/action.py | 19 ++++++++---- st2common/st2common/util/output_schema.py | 35 ++++++++++++++--------- 2 files changed, 34 insertions(+), 20 deletions(-) diff --git a/st2common/st2common/models/api/action.py b/st2common/st2common/models/api/action.py index 33c2956cfe..c55ef969c9 100644 --- a/st2common/st2common/models/api/action.py +++ b/st2common/st2common/models/api/action.py @@ -231,14 +231,21 @@ class ActionAPI(BaseAPI, APIUIDMixin): "additionalProperties": False, "default": {}, }, - # TODO: support validation for non-object action output, possibly w/ anyOf "output_schema": { "description": "Schema for the action's output.", - "type": "object", - # using patternProperties like this implies that output_schema defines - # the "properties" schema of an object where each key is a property name. - "patternProperties": {r"^\w+$": util_schema.get_action_output_schema()}, - "additionalProperties": False, + "anyOf": [ + util_schema.get_action_output_schema(), + { + "type": "object", + # using patternProperties like this implies that output_schema + # defines the "properties" schema of an object where each key + # is a property name. + "patternProperties": { + r"^\w+$": util_schema.get_action_output_schema() + }, + "additionalProperties": False, + }, + ], "default": {}, }, "tags": { diff --git a/st2common/st2common/util/output_schema.py b/st2common/st2common/util/output_schema.py index e2445b65a9..74e4b309de 100644 --- a/st2common/st2common/util/output_schema.py +++ b/st2common/st2common/util/output_schema.py @@ -27,6 +27,10 @@ LOG = logging.getLogger(__name__) +_JSON_BASIC_TYPES = {"boolean", "integer", "null", "number", "string"} +_JSON_COMPLEX_TYPES = {"object", "array"} +_JSON_TYPES = _JSON_BASIC_TYPES | _JSON_COMPLEX_TYPES + def _validate_runner(runner_schema, result): LOG.debug("Validating runner output: %s", runner_schema) @@ -46,13 +50,14 @@ def _validate_action(action_schema, result, output_key): final_result = result[output_key] - # TODO: support action output_schema for non-objects - # (see also st2common/models/api/action.py) - action_schema = { - "type": "object", - "properties": action_schema, - "additionalProperties": False, - } + # FIXME: is there a better way to check if action_schema is only a partial object schema? + if "type" not in action_schema or action_schema["type"] not in _JSON_TYPES: + # we have a partial object schema with jsonschemas for the properties + action_schema = { + "type": "object", + "properties": action_schema, + "additionalProperties": False, + } schema.validate(final_result, action_schema, cls=schema.get_validator("custom")) @@ -67,7 +72,7 @@ def _get_masked_value(spec, value): kind = spec.get("type") - if kind in ("boolean", "integer", "null", "number", "string"): + if kind in _JSON_BASIC_TYPES: # already checked for spec["secret"] above; nothing else to check. return value @@ -168,8 +173,14 @@ def mask_secret_output(ac_ex, output_value): if not isinstance(output_schema, Mapping): return output_value - # TODO: a better way to see if only the values are valid json schemas, or the whole thing? - if "type" not in output_schema: + # FIXME: is there a better way to check if output_schema is only a partial object schema? + if "type" in output_schema and output_schema["type"] in _JSON_TYPES: + # we have a full jsonschema + output_value[output_key] = _get_masked_value( + output_schema, output_value[output_key] + ) + else: + # we have a partial object schema with jsonschemas for the properties # see st2common/st2common/models/api/action.py # output_schema_schema = { # "description": "Schema for the action's output.", @@ -187,10 +198,6 @@ def mask_secret_output(ac_ex, output_value): output_value[output_key] = _get_masked_value( implied_schema, output_value[output_key] ) - else: - output_value[output_key] = _get_masked_value( - output_schema, output_value[output_key] - ) return output_value From 8f830b685cccd6a91beb75e6a1891fd35bed84f8 Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Fri, 6 Aug 2021 14:40:29 -0500 Subject: [PATCH 10/36] Regenerate action schema for new action_output format --- contrib/schemas/action.json | 240 +++++++++++++++++++++++++++++++++++- 1 file changed, 235 insertions(+), 5 deletions(-) diff --git a/contrib/schemas/action.json b/contrib/schemas/action.json index 06049d348a..6c15a2ef98 100644 --- a/contrib/schemas/action.json +++ b/contrib/schemas/action.json @@ -280,9 +280,8 @@ }, "output_schema": { "description": "Schema for the action's output.", - "type": "object", - "patternProperties": { - "^\\w+$": { + "anyOf": [ + { "id": "http://json-schema.org/draft-04/schema#", "$schema": "http://json-schema.org/draft-04/schema#", "description": "Core schema meta-schema", @@ -507,9 +506,240 @@ ] }, "default": {} + }, + { + "type": "object", + "patternProperties": { + "^\\w+$": { + "id": "http://json-schema.org/draft-04/schema#", + "$schema": "http://json-schema.org/draft-04/schema#", + "description": "Core schema meta-schema", + "definitions": { + "schemaArray": { + "type": "array", + "minItems": 1, + "items": { + "$ref": "#" + } + }, + "positiveInteger": { + "type": "integer", + "minimum": 0 + }, + "positiveIntegerDefault0": { + "allOf": [ + { + "$ref": "#/definitions/positiveInteger" + }, + { + "default": 0 + } + ] + }, + "simpleTypes": { + "enum": [ + "array", + "boolean", + "integer", + "null", + "number", + "object", + "string" + ] + }, + "stringArray": { + "type": "array", + "items": { + "type": "string" + }, + "minItems": 1, + "uniqueItems": true + } + }, + "type": "object", + "properties": { + "id": { + "type": "string", + "format": "uri" + }, + "$schema": { + "type": "string", + "format": "uri" + }, + "title": { + "type": "string" + }, + "description": { + "type": "string" + }, + "default": {}, + "multipleOf": { + "type": "number", + "minimum": 0, + "exclusiveMinimum": true + }, + "maximum": { + "type": "number" + }, + "exclusiveMaximum": { + "type": "boolean", + "default": false + }, + "minimum": { + "type": "number" + }, + "exclusiveMinimum": { + "type": "boolean", + "default": false + }, + "maxLength": { + "$ref": "#/definitions/positiveInteger" + }, + "minLength": { + "$ref": "#/definitions/positiveIntegerDefault0" + }, + "pattern": { + "type": "string", + "format": "regex" + }, + "additionalItems": { + "anyOf": [ + { + "type": "boolean" + }, + { + "$ref": "#" + } + ], + "default": {} + }, + "items": { + "anyOf": [ + { + "$ref": "#" + }, + { + "$ref": "#/definitions/schemaArray" + } + ], + "default": {} + }, + "maxItems": { + "$ref": "#/definitions/positiveInteger" + }, + "minItems": { + "$ref": "#/definitions/positiveIntegerDefault0" + }, + "uniqueItems": { + "type": "boolean", + "default": false + }, + "maxProperties": { + "$ref": "#/definitions/positiveInteger" + }, + "minProperties": { + "$ref": "#/definitions/positiveIntegerDefault0" + }, + "required": { + "type": "boolean", + "default": false + }, + "secret": { + "type": "boolean", + "default": false + }, + "additionalProperties": { + "anyOf": [ + { + "type": "boolean" + }, + { + "$ref": "#" + } + ], + "default": {} + }, + "definitions": { + "type": "object", + "additionalProperties": { + "$ref": "#" + }, + "default": {} + }, + "properties": { + "type": "object", + "additionalProperties": { + "$ref": "#" + }, + "default": {} + }, + "patternProperties": { + "type": "object", + "additionalProperties": { + "$ref": "#" + }, + "default": {} + }, + "dependencies": { + "type": "object", + "additionalProperties": { + "anyOf": [ + { + "$ref": "#" + }, + { + "$ref": "#/definitions/stringArray" + } + ] + } + }, + "enum": { + "type": "array", + "minItems": 1, + "uniqueItems": true + }, + "type": { + "anyOf": [ + { + "$ref": "#/definitions/simpleTypes" + } + ] + }, + "position": { + "type": "number", + "minimum": 0 + }, + "immutable": { + "type": "boolean", + "default": false + }, + "allOf": { + "$ref": "#/definitions/schemaArray" + }, + "anyOf": { + "$ref": "#/definitions/schemaArray" + }, + "oneOf": { + "$ref": "#/definitions/schemaArray" + }, + "not": { + "$ref": "#" + } + }, + "dependencies": { + "exclusiveMaximum": [ + "maximum" + ], + "exclusiveMinimum": [ + "minimum" + ] + }, + "default": {} + } + }, + "additionalProperties": false } - }, - "additionalProperties": false, + ], "default": {} }, "tags": { From 6e7ec738a0f0f19c8d5d7da0828e093cdf76862d Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Mon, 28 Mar 2022 20:42:41 -0500 Subject: [PATCH 11/36] try simplifying output_schema --- contrib/schemas/action.json | 650 +++++++---------------- st2common/st2common/models/api/action.py | 54 +- 2 files changed, 234 insertions(+), 470 deletions(-) diff --git a/contrib/schemas/action.json b/contrib/schemas/action.json index 6c15a2ef98..c8a8562b67 100644 --- a/contrib/schemas/action.json +++ b/contrib/schemas/action.json @@ -279,467 +279,229 @@ "default": {} }, "output_schema": { - "description": "Schema for the action's output.", - "anyOf": [ - { - "id": "http://json-schema.org/draft-04/schema#", - "$schema": "http://json-schema.org/draft-04/schema#", - "description": "Core schema meta-schema", - "definitions": { - "schemaArray": { - "type": "array", - "minItems": 1, - "items": { - "$ref": "#" - } - }, - "positiveInteger": { - "type": "integer", - "minimum": 0 - }, - "positiveIntegerDefault0": { - "allOf": [ - { - "$ref": "#/definitions/positiveInteger" - }, - { - "default": 0 - } - ] - }, - "simpleTypes": { - "enum": [ - "array", - "boolean", - "integer", - "null", - "number", - "object", - "string" - ] + "id": "http://json-schema.org/draft-04/schema#", + "$schema": "http://json-schema.org/draft-04/schema#", + "description": "Core schema meta-schema", + "definitions": { + "schemaArray": { + "type": "array", + "minItems": 1, + "items": { + "$ref": "#" + } + }, + "positiveInteger": { + "type": "integer", + "minimum": 0 + }, + "positiveIntegerDefault0": { + "allOf": [ + { + "$ref": "#/definitions/positiveInteger" }, - "stringArray": { - "type": "array", - "items": { - "type": "string" - }, - "minItems": 1, - "uniqueItems": true + { + "default": 0 } + ] + }, + "simpleTypes": { + "enum": [ + "array", + "boolean", + "integer", + "null", + "number", + "object", + "string" + ] + }, + "stringArray": { + "type": "array", + "items": { + "type": "string" }, - "type": "object", - "properties": { - "id": { - "type": "string", - "format": "uri" - }, - "$schema": { - "type": "string", - "format": "uri" - }, - "title": { - "type": "string" - }, - "description": { - "type": "string" - }, - "default": {}, - "multipleOf": { - "type": "number", - "minimum": 0, - "exclusiveMinimum": true - }, - "maximum": { - "type": "number" - }, - "exclusiveMaximum": { - "type": "boolean", - "default": false - }, - "minimum": { - "type": "number" - }, - "exclusiveMinimum": { - "type": "boolean", - "default": false - }, - "maxLength": { - "$ref": "#/definitions/positiveInteger" - }, - "minLength": { - "$ref": "#/definitions/positiveIntegerDefault0" - }, - "pattern": { - "type": "string", - "format": "regex" - }, - "additionalItems": { - "anyOf": [ - { - "type": "boolean" - }, - { - "$ref": "#" - } - ], - "default": {} - }, - "items": { - "anyOf": [ - { - "$ref": "#" - }, - { - "$ref": "#/definitions/schemaArray" - } - ], - "default": {} - }, - "maxItems": { - "$ref": "#/definitions/positiveInteger" - }, - "minItems": { - "$ref": "#/definitions/positiveIntegerDefault0" - }, - "uniqueItems": { - "type": "boolean", - "default": false - }, - "maxProperties": { - "$ref": "#/definitions/positiveInteger" - }, - "minProperties": { - "$ref": "#/definitions/positiveIntegerDefault0" - }, - "required": { - "type": "boolean", - "default": false - }, - "secret": { - "type": "boolean", - "default": false - }, - "additionalProperties": { - "anyOf": [ - { - "type": "boolean" - }, - { - "$ref": "#" - } - ], - "default": {} - }, - "definitions": { - "type": "object", - "additionalProperties": { - "$ref": "#" - }, - "default": {} - }, - "properties": { - "type": "object", - "additionalProperties": { - "$ref": "#" - }, - "default": {} - }, - "patternProperties": { - "type": "object", - "additionalProperties": { - "$ref": "#" - }, - "default": {} - }, - "dependencies": { - "type": "object", - "additionalProperties": { - "anyOf": [ - { - "$ref": "#" - }, - { - "$ref": "#/definitions/stringArray" - } - ] - } - }, - "enum": { - "type": "array", - "minItems": 1, - "uniqueItems": true - }, - "type": { - "anyOf": [ - { - "$ref": "#/definitions/simpleTypes" - } - ] - }, - "position": { - "type": "number", - "minimum": 0 - }, - "immutable": { - "type": "boolean", - "default": false - }, - "allOf": { - "$ref": "#/definitions/schemaArray" + "minItems": 1, + "uniqueItems": true + } + }, + "type": "object", + "properties": { + "id": { + "type": "string", + "format": "uri" + }, + "$schema": { + "type": "string", + "format": "uri" + }, + "title": { + "type": "string" + }, + "description": { + "type": "string" + }, + "default": {}, + "multipleOf": { + "type": "number", + "minimum": 0, + "exclusiveMinimum": true + }, + "maximum": { + "type": "number" + }, + "exclusiveMaximum": { + "type": "boolean", + "default": false + }, + "minimum": { + "type": "number" + }, + "exclusiveMinimum": { + "type": "boolean", + "default": false + }, + "maxLength": { + "$ref": "#/definitions/positiveInteger" + }, + "minLength": { + "$ref": "#/definitions/positiveIntegerDefault0" + }, + "pattern": { + "type": "string", + "format": "regex" + }, + "additionalItems": { + "anyOf": [ + { + "type": "boolean" }, - "anyOf": { - "$ref": "#/definitions/schemaArray" + { + "$ref": "#" + } + ], + "default": {} + }, + "items": { + "anyOf": [ + { + "$ref": "#" }, - "oneOf": { + { "$ref": "#/definitions/schemaArray" + } + ], + "default": {} + }, + "maxItems": { + "$ref": "#/definitions/positiveInteger" + }, + "minItems": { + "$ref": "#/definitions/positiveIntegerDefault0" + }, + "uniqueItems": { + "type": "boolean", + "default": false + }, + "maxProperties": { + "$ref": "#/definitions/positiveInteger" + }, + "minProperties": { + "$ref": "#/definitions/positiveIntegerDefault0" + }, + "required": { + "type": "boolean", + "default": false + }, + "secret": { + "type": "boolean", + "default": false + }, + "additionalProperties": { + "anyOf": [ + { + "type": "boolean" }, - "not": { + { "$ref": "#" } + ], + "default": {} + }, + "definitions": { + "type": "object", + "additionalProperties": { + "$ref": "#" }, - "dependencies": { - "exclusiveMaximum": [ - "maximum" - ], - "exclusiveMinimum": [ - "minimum" - ] + "default": {} + }, + "properties": { + "type": "object", + "additionalProperties": { + "$ref": "#" }, "default": {} }, - { + "patternProperties": { "type": "object", - "patternProperties": { - "^\\w+$": { - "id": "http://json-schema.org/draft-04/schema#", - "$schema": "http://json-schema.org/draft-04/schema#", - "description": "Core schema meta-schema", - "definitions": { - "schemaArray": { - "type": "array", - "minItems": 1, - "items": { - "$ref": "#" - } - }, - "positiveInteger": { - "type": "integer", - "minimum": 0 - }, - "positiveIntegerDefault0": { - "allOf": [ - { - "$ref": "#/definitions/positiveInteger" - }, - { - "default": 0 - } - ] - }, - "simpleTypes": { - "enum": [ - "array", - "boolean", - "integer", - "null", - "number", - "object", - "string" - ] - }, - "stringArray": { - "type": "array", - "items": { - "type": "string" - }, - "minItems": 1, - "uniqueItems": true - } - }, - "type": "object", - "properties": { - "id": { - "type": "string", - "format": "uri" - }, - "$schema": { - "type": "string", - "format": "uri" - }, - "title": { - "type": "string" - }, - "description": { - "type": "string" - }, - "default": {}, - "multipleOf": { - "type": "number", - "minimum": 0, - "exclusiveMinimum": true - }, - "maximum": { - "type": "number" - }, - "exclusiveMaximum": { - "type": "boolean", - "default": false - }, - "minimum": { - "type": "number" - }, - "exclusiveMinimum": { - "type": "boolean", - "default": false - }, - "maxLength": { - "$ref": "#/definitions/positiveInteger" - }, - "minLength": { - "$ref": "#/definitions/positiveIntegerDefault0" - }, - "pattern": { - "type": "string", - "format": "regex" - }, - "additionalItems": { - "anyOf": [ - { - "type": "boolean" - }, - { - "$ref": "#" - } - ], - "default": {} - }, - "items": { - "anyOf": [ - { - "$ref": "#" - }, - { - "$ref": "#/definitions/schemaArray" - } - ], - "default": {} - }, - "maxItems": { - "$ref": "#/definitions/positiveInteger" - }, - "minItems": { - "$ref": "#/definitions/positiveIntegerDefault0" - }, - "uniqueItems": { - "type": "boolean", - "default": false - }, - "maxProperties": { - "$ref": "#/definitions/positiveInteger" - }, - "minProperties": { - "$ref": "#/definitions/positiveIntegerDefault0" - }, - "required": { - "type": "boolean", - "default": false - }, - "secret": { - "type": "boolean", - "default": false - }, - "additionalProperties": { - "anyOf": [ - { - "type": "boolean" - }, - { - "$ref": "#" - } - ], - "default": {} - }, - "definitions": { - "type": "object", - "additionalProperties": { - "$ref": "#" - }, - "default": {} - }, - "properties": { - "type": "object", - "additionalProperties": { - "$ref": "#" - }, - "default": {} - }, - "patternProperties": { - "type": "object", - "additionalProperties": { - "$ref": "#" - }, - "default": {} - }, - "dependencies": { - "type": "object", - "additionalProperties": { - "anyOf": [ - { - "$ref": "#" - }, - { - "$ref": "#/definitions/stringArray" - } - ] - } - }, - "enum": { - "type": "array", - "minItems": 1, - "uniqueItems": true - }, - "type": { - "anyOf": [ - { - "$ref": "#/definitions/simpleTypes" - } - ] - }, - "position": { - "type": "number", - "minimum": 0 - }, - "immutable": { - "type": "boolean", - "default": false - }, - "allOf": { - "$ref": "#/definitions/schemaArray" - }, - "anyOf": { - "$ref": "#/definitions/schemaArray" - }, - "oneOf": { - "$ref": "#/definitions/schemaArray" - }, - "not": { - "$ref": "#" - } - }, - "dependencies": { - "exclusiveMaximum": [ - "maximum" - ], - "exclusiveMinimum": [ - "minimum" - ] + "additionalProperties": { + "$ref": "#" + }, + "default": {} + }, + "dependencies": { + "type": "object", + "additionalProperties": { + "anyOf": [ + { + "$ref": "#" }, - "default": {} + { + "$ref": "#/definitions/stringArray" + } + ] + } + }, + "enum": { + "type": "array", + "minItems": 1, + "uniqueItems": true + }, + "type": { + "anyOf": [ + { + "$ref": "#/definitions/simpleTypes" } - }, - "additionalProperties": false + ] + }, + "position": { + "type": "number", + "minimum": 0 + }, + "immutable": { + "type": "boolean", + "default": false + }, + "allOf": { + "$ref": "#/definitions/schemaArray" + }, + "anyOf": { + "$ref": "#/definitions/schemaArray" + }, + "oneOf": { + "$ref": "#/definitions/schemaArray" + }, + "not": { + "$ref": "#" } - ], + }, + "dependencies": { + "exclusiveMaximum": [ + "maximum" + ], + "exclusiveMinimum": [ + "minimum" + ] + }, "default": {} }, "tags": { diff --git a/st2common/st2common/models/api/action.py b/st2common/st2common/models/api/action.py index c55ef969c9..2d5e1264fb 100644 --- a/st2common/st2common/models/api/action.py +++ b/st2common/st2common/models/api/action.py @@ -117,15 +117,16 @@ class RunnerTypeAPI(BaseAPI): "required": False, }, # Runners must always output json objects with action output in output_key property - "output_schema": { - "description": "Schema for the runner's output.", - "type": "object", - # using patternProperties like this implies that output_schema defines - # the "properties" schema of an object where each key is a property name. - "patternProperties": {r"^\w+$": util_schema.get_action_output_schema()}, - "additionalProperties": False, - "default": {}, - }, + "output_schema": util_schema.get_action_output_schema(), + # "output_schema": { + # "description": "Schema for the runner's output.", + # "type": "object", + # # using patternProperties like this implies that output_schema defines + # # the "properties" schema of an object where each key is a property name. + # "patternProperties": {r"^\w+$": util_schema.get_action_output_schema()}, + # "additionalProperties": False, + # "default": {}, + # }, }, "additionalProperties": False, } @@ -231,23 +232,24 @@ class ActionAPI(BaseAPI, APIUIDMixin): "additionalProperties": False, "default": {}, }, - "output_schema": { - "description": "Schema for the action's output.", - "anyOf": [ - util_schema.get_action_output_schema(), - { - "type": "object", - # using patternProperties like this implies that output_schema - # defines the "properties" schema of an object where each key - # is a property name. - "patternProperties": { - r"^\w+$": util_schema.get_action_output_schema() - }, - "additionalProperties": False, - }, - ], - "default": {}, - }, + "output_schema": util_schema.get_action_output_schema(), + # "output_schema": { + # "description": "Schema for the action's output.", + # "anyOf": [ + # util_schema.get_action_output_schema(), + # { + # "type": "object", + # # using patternProperties like this implies that output_schema + # # defines the "properties" schema of an object where each key + # # is a property name. + # "patternProperties": { + # r"^\w+$": util_schema.get_action_output_schema() + # }, + # "additionalProperties": False, + # }, + # ], + # "default": {}, + # }, "tags": { "description": "User associated metadata assigned to this object.", "type": "array", From 95062c0888a4fc7ff3d571ebe4931f10eb05beff Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Mon, 28 Mar 2022 20:56:24 -0500 Subject: [PATCH 12/36] turn runner output_schema into full jsonschema --- .../action_chain_runner/runner.yaml | 10 +++-- .../http_runner/http_runner/runner.yaml | 32 ++++++++-------- .../orquesta_runner/runner.yaml | 28 +++++++------- .../python_runner/python_runner/runner.yaml | 38 ++++++++++--------- 4 files changed, 58 insertions(+), 50 deletions(-) diff --git a/contrib/runners/action_chain_runner/action_chain_runner/runner.yaml b/contrib/runners/action_chain_runner/action_chain_runner/runner.yaml index e30470a148..a10d8c8f9a 100644 --- a/contrib/runners/action_chain_runner/action_chain_runner/runner.yaml +++ b/contrib/runners/action_chain_runner/action_chain_runner/runner.yaml @@ -14,7 +14,9 @@ type: array output_key: published output_schema: - published: - type: "object" - tasks: - type: "array" + type: object + properties: + published: + type: "object" + tasks: + type: "array" diff --git a/contrib/runners/http_runner/http_runner/runner.yaml b/contrib/runners/http_runner/http_runner/runner.yaml index 168f68bbb2..4ce56b50cd 100644 --- a/contrib/runners/http_runner/http_runner/runner.yaml +++ b/contrib/runners/http_runner/http_runner/runner.yaml @@ -54,18 +54,20 @@ type: string output_key: body output_schema: - status_code: - type: integer - body: - anyOf: - - type: "object" - - type: "string" - - type: "integer" - - type: "number" - - type: "boolean" - - type: "array" - - type: "null" - parsed: - type: boolean - headers: - type: object + type: object + properties: + status_code: + type: integer + body: + anyOf: + - type: "object" + - type: "string" + - type: "integer" + - type: "number" + - type: "boolean" + - type: "array" + - type: "null" + parsed: + type: boolean + headers: + type: object diff --git a/contrib/runners/orquesta_runner/orquesta_runner/runner.yaml b/contrib/runners/orquesta_runner/orquesta_runner/runner.yaml index 45a80d2621..18e59c98e7 100644 --- a/contrib/runners/orquesta_runner/orquesta_runner/runner.yaml +++ b/contrib/runners/orquesta_runner/orquesta_runner/runner.yaml @@ -16,16 +16,18 @@ default: [] output_key: output output_schema: - errors: - anyOf: - - type: "object" - - type: "array" - output: - anyOf: - - type: "object" - - type: "string" - - type: "integer" - - type: "number" - - type: "boolean" - - type: "array" - - type: "null" + type: object + properties: + errors: + anyOf: + - type: "object" + - type: "array" + output: + anyOf: + - type: "object" + - type: "string" + - type: "integer" + - type: "number" + - type: "boolean" + - type: "array" + - type: "null" diff --git a/contrib/runners/python_runner/python_runner/runner.yaml b/contrib/runners/python_runner/python_runner/runner.yaml index 92ee7e15a7..586f0949a6 100644 --- a/contrib/runners/python_runner/python_runner/runner.yaml +++ b/contrib/runners/python_runner/python_runner/runner.yaml @@ -4,24 +4,26 @@ runner_module: python_runner output_key: result output_schema: - result: - anyOf: - - type: "object" - - type: "string" - - type: "integer" - - type: "number" - - type: "boolean" - - type: "array" - - type: "null" - stderr: - type: string - required: true - stdout: - type: string - required: true - exit_code: - type: integer - required: true + type: object + properties: + result: + anyOf: + - type: "object" + - type: "string" + - type: "integer" + - type: "number" + - type: "boolean" + - type: "array" + - type: "null" + stderr: + type: string + required: true + stdout: + type: string + required: true + exit_code: + type: integer + required: true runner_parameters: debug: description: Enable runner debug mode. From 2461a4d7bb02c732d27215ce321b8578e83e7ad8 Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Tue, 29 Mar 2022 19:05:39 -0500 Subject: [PATCH 13/36] turn test runner/action output_schema into full jsonschema --- .../fixtures/generic/runners/run-local.yaml | 22 ++++++++------- .../dummy_pack_1/actions/my_http_action.yaml | 20 +++++++------ .../dummy_pack_1/actions/my_py_action.yaml | 20 +++++++------ .../orquesta_tests/actions/data-flow.yaml | 28 ++++++++++--------- .../actions/py-secret-output.yaml | 10 ++++--- .../sequential_with_broken_schema.yaml | 6 ++-- .../actions/sequential_with_schema.yaml | 6 ++-- 7 files changed, 63 insertions(+), 49 deletions(-) diff --git a/st2tests/st2tests/fixtures/generic/runners/run-local.yaml b/st2tests/st2tests/fixtures/generic/runners/run-local.yaml index 557dcb99fc..d9339b7760 100644 --- a/st2tests/st2tests/fixtures/generic/runners/run-local.yaml +++ b/st2tests/st2tests/fixtures/generic/runners/run-local.yaml @@ -15,13 +15,15 @@ runner_parameters: default: false type: boolean output_schema: - succeeded: - type: boolean - failed: - type: boolean - return_code: - type: integer - stderr: - type: string - stdout: - type: string + type: object + properties: + succeeded: + type: boolean + failed: + type: boolean + return_code: + type: integer + stderr: + type: string + stdout: + type: string diff --git a/st2tests/st2tests/fixtures/packs/dummy_pack_1/actions/my_http_action.yaml b/st2tests/st2tests/fixtures/packs/dummy_pack_1/actions/my_http_action.yaml index dadeb4b59c..7be8b8da33 100644 --- a/st2tests/st2tests/fixtures/packs/dummy_pack_1/actions/my_http_action.yaml +++ b/st2tests/st2tests/fixtures/packs/dummy_pack_1/actions/my_http_action.yaml @@ -11,12 +11,14 @@ parameters: immutable: true default: https://api.stackstorm.com output_schema: - k1: - type: string - required: true - k2: - type: string - required: true - secret: true - k3: - type: boolean + type: object + properties: + k1: + type: string + required: true + k2: + type: string + required: true + secret: true + k3: + type: boolean diff --git a/st2tests/st2tests/fixtures/packs/dummy_pack_1/actions/my_py_action.yaml b/st2tests/st2tests/fixtures/packs/dummy_pack_1/actions/my_py_action.yaml index 87553c9a76..e661c1ae40 100644 --- a/st2tests/st2tests/fixtures/packs/dummy_pack_1/actions/my_py_action.yaml +++ b/st2tests/st2tests/fixtures/packs/dummy_pack_1/actions/my_py_action.yaml @@ -5,12 +5,14 @@ description: A simple python action for testing purposes. enabled: true entry_point: my_py_action.py output_schema: - k1: - type: string - required: true - k2: - type: string - required: true - secret: true - k3: - type: boolean + type: object + properties: + k1: + type: string + required: true + k2: + type: string + required: true + secret: true + k3: + type: boolean diff --git a/st2tests/st2tests/fixtures/packs/orquesta_tests/actions/data-flow.yaml b/st2tests/st2tests/fixtures/packs/orquesta_tests/actions/data-flow.yaml index 85105f435e..2a7a282ebb 100644 --- a/st2tests/st2tests/fixtures/packs/orquesta_tests/actions/data-flow.yaml +++ b/st2tests/st2tests/fixtures/packs/orquesta_tests/actions/data-flow.yaml @@ -10,16 +10,18 @@ parameters: required: true type: string output_schema: - a6: - type: string - required: true - b6: - type: string - required: true - a7: - type: string - required: true - b7: - type: string - required: true - secret: true + type: object + properties: + a6: + type: string + required: true + b6: + type: string + required: true + a7: + type: string + required: true + b7: + type: string + required: true + secret: true diff --git a/st2tests/st2tests/fixtures/packs/orquesta_tests/actions/py-secret-output.yaml b/st2tests/st2tests/fixtures/packs/orquesta_tests/actions/py-secret-output.yaml index b6126e8ce1..3fef0be885 100644 --- a/st2tests/st2tests/fixtures/packs/orquesta_tests/actions/py-secret-output.yaml +++ b/st2tests/st2tests/fixtures/packs/orquesta_tests/actions/py-secret-output.yaml @@ -9,7 +9,9 @@ parameters: type: string required: true output_schema: - k2: - type: string - required: true - secret: true + type: object + properties: + k2: + type: string + required: true + secret: true diff --git a/st2tests/st2tests/fixtures/packs/orquesta_tests/actions/sequential_with_broken_schema.yaml b/st2tests/st2tests/fixtures/packs/orquesta_tests/actions/sequential_with_broken_schema.yaml index 81b3a38289..d4adb746c3 100644 --- a/st2tests/st2tests/fixtures/packs/orquesta_tests/actions/sequential_with_broken_schema.yaml +++ b/st2tests/st2tests/fixtures/packs/orquesta_tests/actions/sequential_with_broken_schema.yaml @@ -11,5 +11,7 @@ parameters: type: string default: Stanley output_schema: - notakey: - type: boolean + type: object + properties: + notakey: + type: boolean diff --git a/st2tests/st2tests/fixtures/packs/orquesta_tests/actions/sequential_with_schema.yaml b/st2tests/st2tests/fixtures/packs/orquesta_tests/actions/sequential_with_schema.yaml index cd0d5b8b34..41a9b5e141 100644 --- a/st2tests/st2tests/fixtures/packs/orquesta_tests/actions/sequential_with_schema.yaml +++ b/st2tests/st2tests/fixtures/packs/orquesta_tests/actions/sequential_with_schema.yaml @@ -11,5 +11,7 @@ parameters: type: string default: Stanley output_schema: - msg: - type: string + type: object + properties: + msg: + type: string From 94e78b46649dd57cd2bfe35e36f199af6a2ae4f2 Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Tue, 29 Mar 2022 20:20:27 -0500 Subject: [PATCH 14/36] schema validation: inject defaults only if dependencies are met. --- st2common/st2common/util/schema/__init__.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/st2common/st2common/util/schema/__init__.py b/st2common/st2common/util/schema/__init__.py index 063b963629..ef180d90ab 100644 --- a/st2common/st2common/util/schema/__init__.py +++ b/st2common/st2common/util/schema/__init__.py @@ -207,9 +207,23 @@ def assign_default_values(instance, schema): return instance properties = schema.get("properties", {}) + dependencies = schema.get("dependencies", {}) for property_name, property_data in six.iteritems(properties): has_default_value = "default" in property_data + # only populate default if dependencies are met + # eg: exclusiveMaximum depends on maximum which does not have a default. + # so we don't want to apply exclusiveMaximum's default unless maximum. + if has_default_value and property_name in dependencies: + for required_property in dependencies[property_name]: + if "default" in properties.get(required_property, {}): + # we depend on something that has a default. Apply this default. + continue + if required_property not in instance: + # we depend on something that does not have a default. + # do not apply this default. + has_default_value = False + break default_value = property_data.get("default", None) # Assign default value on the instance so the validation doesn't fail if requires is true From cc32381dede5621703f9b4edbde516b440d2189c Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Tue, 29 Mar 2022 23:24:13 -0500 Subject: [PATCH 15/36] schema validation: runner schema is always a full schema now --- st2common/st2common/util/output_schema.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/st2common/st2common/util/output_schema.py b/st2common/st2common/util/output_schema.py index 74e4b309de..c2e72d391e 100644 --- a/st2common/st2common/util/output_schema.py +++ b/st2common/st2common/util/output_schema.py @@ -35,12 +35,13 @@ def _validate_runner(runner_schema, result): LOG.debug("Validating runner output: %s", runner_schema) - # runner's output is always an object. - runner_schema = { - "type": "object", - "properties": runner_schema, - "additionalProperties": False, - } + if "type" not in runner_schema or runner_schema["type"] not in _JSON_TYPES: + # we have a partial object schema with jsonschemas for the properties + runner_schema = { + "type": "object", + "properties": runner_schema, + "additionalProperties": False, + } schema.validate(result, runner_schema, cls=schema.get_validator("custom")) From 36cb3b10efecd73f2abd400dc4a057a75616eabd Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Wed, 30 Mar 2022 00:30:25 -0500 Subject: [PATCH 16/36] adjust more output_schema tests --- .../unit/controllers/v1/test_executions.py | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/st2api/tests/unit/controllers/v1/test_executions.py b/st2api/tests/unit/controllers/v1/test_executions.py index fb80a35917..478b1295fa 100644 --- a/st2api/tests/unit/controllers/v1/test_executions.py +++ b/st2api/tests/unit/controllers/v1/test_executions.py @@ -180,8 +180,15 @@ "runner_type": "python-script", "parameters": {}, "output_schema": { - "secret_param_1": {"type": "string", "required": True, "secret": True}, - "secret_param_2": {"type": "string", "required": True, "secret": True}, + "type": "object", + "properties": { + "secret_param_1": { + "type": "string", "required": True, "secret": True + }, + "secret_param_2": { + "type": "string", "required": True, "secret": True + }, + }, }, } @@ -194,8 +201,11 @@ "runner_type": "python-script", "parameters": {}, "output_schema": { - "non_secret_param_1": {"type": "string", "required": True}, - "non_secret_param_2": {"type": "string", "required": True}, + "type": "object", + "properties": { + "non_secret_param_1": {"type": "string", "required": True}, + "non_secret_param_2": {"type": "string", "required": True}, + }, }, } ACTION_DEFAULT_ENCRYPT_AND_BOOL = { From 066ff8b377d60102e455e14b9c0c3cf4f979c100 Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Wed, 30 Mar 2022 09:03:39 -0500 Subject: [PATCH 17/36] Complete output_schemas including additionalProperties --- .../action_chain_runner/runner.yaml | 1 + .../http_runner/http_runner/runner.yaml | 1 + .../orquesta_runner/runner.yaml | 1 + .../python_runner/python_runner/runner.yaml | 1 + .../unit/controllers/v1/test_executions.py | 10 ++-- st2common/tests/unit/services/test_packs.py | 12 ++-- st2common/tests/unit/test_db_execution.py | 12 +++- .../tests/unit/test_util_output_schema.py | 60 ++++++++++++------- .../fixtures/generic/runners/run-local.yaml | 1 + .../dummy_pack_1/actions/my_http_action.yaml | 1 + .../dummy_pack_1/actions/my_py_action.yaml | 1 + .../orquesta_tests/actions/data-flow.yaml | 1 + .../actions/py-secret-output.yaml | 1 + .../sequential_with_broken_schema.yaml | 1 + .../actions/sequential_with_schema.yaml | 1 + 15 files changed, 73 insertions(+), 32 deletions(-) diff --git a/contrib/runners/action_chain_runner/action_chain_runner/runner.yaml b/contrib/runners/action_chain_runner/action_chain_runner/runner.yaml index a10d8c8f9a..64b3d25156 100644 --- a/contrib/runners/action_chain_runner/action_chain_runner/runner.yaml +++ b/contrib/runners/action_chain_runner/action_chain_runner/runner.yaml @@ -20,3 +20,4 @@ type: "object" tasks: type: "array" + additionalProperties: false diff --git a/contrib/runners/http_runner/http_runner/runner.yaml b/contrib/runners/http_runner/http_runner/runner.yaml index 4ce56b50cd..1f45c3689f 100644 --- a/contrib/runners/http_runner/http_runner/runner.yaml +++ b/contrib/runners/http_runner/http_runner/runner.yaml @@ -71,3 +71,4 @@ type: boolean headers: type: object + additionalProperties: false diff --git a/contrib/runners/orquesta_runner/orquesta_runner/runner.yaml b/contrib/runners/orquesta_runner/orquesta_runner/runner.yaml index 18e59c98e7..e666158664 100644 --- a/contrib/runners/orquesta_runner/orquesta_runner/runner.yaml +++ b/contrib/runners/orquesta_runner/orquesta_runner/runner.yaml @@ -31,3 +31,4 @@ - type: "boolean" - type: "array" - type: "null" + additionalProperties: false diff --git a/contrib/runners/python_runner/python_runner/runner.yaml b/contrib/runners/python_runner/python_runner/runner.yaml index 586f0949a6..b9baf2e5a4 100644 --- a/contrib/runners/python_runner/python_runner/runner.yaml +++ b/contrib/runners/python_runner/python_runner/runner.yaml @@ -24,6 +24,7 @@ exit_code: type: integer required: true + additionalProperties: false runner_parameters: debug: description: Enable runner debug mode. diff --git a/st2api/tests/unit/controllers/v1/test_executions.py b/st2api/tests/unit/controllers/v1/test_executions.py index 478b1295fa..f31d158cdf 100644 --- a/st2api/tests/unit/controllers/v1/test_executions.py +++ b/st2api/tests/unit/controllers/v1/test_executions.py @@ -182,13 +182,10 @@ "output_schema": { "type": "object", "properties": { - "secret_param_1": { - "type": "string", "required": True, "secret": True - }, - "secret_param_2": { - "type": "string", "required": True, "secret": True - }, + "secret_param_1": {"type": "string", "required": True, "secret": True}, + "secret_param_2": {"type": "string", "required": True, "secret": True}, }, + "additionalProperties": False, }, } @@ -206,6 +203,7 @@ "non_secret_param_1": {"type": "string", "required": True}, "non_secret_param_2": {"type": "string", "required": True}, }, + "additionalProperties": False, }, } ACTION_DEFAULT_ENCRYPT_AND_BOOL = { diff --git a/st2common/tests/unit/services/test_packs.py b/st2common/tests/unit/services/test_packs.py index 4186a8d29c..07d08ff7da 100644 --- a/st2common/tests/unit/services/test_packs.py +++ b/st2common/tests/unit/services/test_packs.py @@ -186,10 +186,14 @@ "name": "data-flow", "notify": {}, "output_schema": { - "a6": {"type": "string", "required": True}, - "b6": {"type": "string", "required": True}, - "a7": {"type": "string", "required": True}, - "b7": {"type": "string", "required": True, "secret": "********"}, + "type": "object", + "properties": { + "a6": {"type": "string", "required": True}, + "b6": {"type": "string", "required": True}, + "a7": {"type": "string", "required": True}, + "b7": {"type": "string", "required": True, "secret": True}, + }, + "additionalProperties": False, }, "pack": TEST_SOURCE_WORKFLOW_PACK, "parameters": {"a1": {"required": True, "type": "string"}}, diff --git a/st2common/tests/unit/test_db_execution.py b/st2common/tests/unit/test_db_execution.py index 84f40ce813..ca322c0f1b 100644 --- a/st2common/tests/unit/test_db_execution.py +++ b/st2common/tests/unit/test_db_execution.py @@ -105,8 +105,16 @@ "action": { "uid": "action:core:ask", "output_schema": { - "os_secret_param": {"type": "string", "required": True, "secret": True}, - "os_non_secret_param": {"type": "string", "required": True}, + "type": "object", + "properties": { + "os_secret_param": { + "type": "string", + "required": True, + "secret": True, + }, + "os_non_secret_param": {"type": "string", "required": True}, + }, + "additionalProperties": False, }, }, "status": "succeeded", diff --git a/st2common/tests/unit/test_util_output_schema.py b/st2common/tests/unit/test_util_output_schema.py index f3f4ba072a..8086b5a909 100644 --- a/st2common/tests/unit/test_util_output_schema.py +++ b/st2common/tests/unit/test_util_output_schema.py @@ -37,46 +37,66 @@ } RUNNER_OUTPUT_SCHEMA = { - "output": {"type": "object"}, - "error": {"type": "array"}, + "type": "object", + "properties": { + "output": {"type": "object"}, + "error": {"type": "array"}, + }, + "additionalProperties": False, } ACTION_OUTPUT_SCHEMA = { - "output_1": {"type": "string"}, - "output_2": {"type": "integer"}, - "output_3": {"type": "string"}, - "deep_output": { - "type": "object", - "parameters": { - "deep_item_1": { - "type": "string", + "type": "object", + "properties": { + "output_1": {"type": "string"}, + "output_2": {"type": "integer"}, + "output_3": {"type": "string"}, + "deep_output": { + "type": "object", + "parameters": { + "deep_item_1": { + "type": "string", + }, }, }, }, + "additionalProperties": False, } RUNNER_OUTPUT_SCHEMA_FAIL = { - "not_a_key_you_have": {"type": "string"}, + "type": "object", + "properties": { + "not_a_key_you_have": {"type": "string"}, + }, + "additionalProperties": False, } ACTION_OUTPUT_SCHEMA_FAIL = { - "not_a_key_you_have": {"type": "string"}, + "type": "object", + "properties": { + "not_a_key_you_have": {"type": "string"}, + }, + "additionalProperties": False, } OUTPUT_KEY = "output" ACTION_OUTPUT_SCHEMA_WITH_SECRET = { - "output_1": {"type": "string"}, - "output_2": {"type": "integer"}, - "output_3": {"type": "string", "secret": True}, - "deep_output": { - "type": "object", - "parameters": { - "deep_item_1": { - "type": "string", + "type": "object", + "properties": { + "output_1": {"type": "string"}, + "output_2": {"type": "integer"}, + "output_3": {"type": "string", "secret": True}, + "deep_output": { + "type": "object", + "parameters": { + "deep_item_1": { + "type": "string", + }, }, }, }, + "additionalProperties": False, } diff --git a/st2tests/st2tests/fixtures/generic/runners/run-local.yaml b/st2tests/st2tests/fixtures/generic/runners/run-local.yaml index d9339b7760..afa5e8971f 100644 --- a/st2tests/st2tests/fixtures/generic/runners/run-local.yaml +++ b/st2tests/st2tests/fixtures/generic/runners/run-local.yaml @@ -27,3 +27,4 @@ output_schema: type: string stdout: type: string + additionalProperties: false diff --git a/st2tests/st2tests/fixtures/packs/dummy_pack_1/actions/my_http_action.yaml b/st2tests/st2tests/fixtures/packs/dummy_pack_1/actions/my_http_action.yaml index 7be8b8da33..1c76030bbd 100644 --- a/st2tests/st2tests/fixtures/packs/dummy_pack_1/actions/my_http_action.yaml +++ b/st2tests/st2tests/fixtures/packs/dummy_pack_1/actions/my_http_action.yaml @@ -22,3 +22,4 @@ output_schema: secret: true k3: type: boolean + additionalProperties: false diff --git a/st2tests/st2tests/fixtures/packs/dummy_pack_1/actions/my_py_action.yaml b/st2tests/st2tests/fixtures/packs/dummy_pack_1/actions/my_py_action.yaml index e661c1ae40..75abb34f30 100644 --- a/st2tests/st2tests/fixtures/packs/dummy_pack_1/actions/my_py_action.yaml +++ b/st2tests/st2tests/fixtures/packs/dummy_pack_1/actions/my_py_action.yaml @@ -16,3 +16,4 @@ output_schema: secret: true k3: type: boolean + additionalProperties: false diff --git a/st2tests/st2tests/fixtures/packs/orquesta_tests/actions/data-flow.yaml b/st2tests/st2tests/fixtures/packs/orquesta_tests/actions/data-flow.yaml index 2a7a282ebb..18474222c4 100644 --- a/st2tests/st2tests/fixtures/packs/orquesta_tests/actions/data-flow.yaml +++ b/st2tests/st2tests/fixtures/packs/orquesta_tests/actions/data-flow.yaml @@ -25,3 +25,4 @@ output_schema: type: string required: true secret: true + additionalProperties: false diff --git a/st2tests/st2tests/fixtures/packs/orquesta_tests/actions/py-secret-output.yaml b/st2tests/st2tests/fixtures/packs/orquesta_tests/actions/py-secret-output.yaml index 3fef0be885..05529a5554 100644 --- a/st2tests/st2tests/fixtures/packs/orquesta_tests/actions/py-secret-output.yaml +++ b/st2tests/st2tests/fixtures/packs/orquesta_tests/actions/py-secret-output.yaml @@ -15,3 +15,4 @@ output_schema: type: string required: true secret: true + additionalProperties: false diff --git a/st2tests/st2tests/fixtures/packs/orquesta_tests/actions/sequential_with_broken_schema.yaml b/st2tests/st2tests/fixtures/packs/orquesta_tests/actions/sequential_with_broken_schema.yaml index d4adb746c3..a05d73dcf5 100644 --- a/st2tests/st2tests/fixtures/packs/orquesta_tests/actions/sequential_with_broken_schema.yaml +++ b/st2tests/st2tests/fixtures/packs/orquesta_tests/actions/sequential_with_broken_schema.yaml @@ -15,3 +15,4 @@ output_schema: properties: notakey: type: boolean + additionalProperties: false diff --git a/st2tests/st2tests/fixtures/packs/orquesta_tests/actions/sequential_with_schema.yaml b/st2tests/st2tests/fixtures/packs/orquesta_tests/actions/sequential_with_schema.yaml index 41a9b5e141..68bd7b0970 100644 --- a/st2tests/st2tests/fixtures/packs/orquesta_tests/actions/sequential_with_schema.yaml +++ b/st2tests/st2tests/fixtures/packs/orquesta_tests/actions/sequential_with_schema.yaml @@ -15,3 +15,4 @@ output_schema: properties: msg: type: string + additionalProperties: false From dd9e03200e5e8cdcce49a17d8a214eb0f3d14d9f Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Wed, 30 Mar 2022 09:34:16 -0500 Subject: [PATCH 18/36] Clean up action_output_schema usage --- contrib/schemas/action.json | 5 +-- st2common/st2common/models/api/action.py | 35 ++++----------------- st2common/st2common/util/schema/__init__.py | 7 +++-- 3 files changed, 14 insertions(+), 33 deletions(-) diff --git a/contrib/schemas/action.json b/contrib/schemas/action.json index c8a8562b67..f0e85b1feb 100644 --- a/contrib/schemas/action.json +++ b/contrib/schemas/action.json @@ -281,7 +281,7 @@ "output_schema": { "id": "http://json-schema.org/draft-04/schema#", "$schema": "http://json-schema.org/draft-04/schema#", - "description": "Core schema meta-schema", + "description": "Action Output Schema (based on Core schema meta-schema)", "definitions": { "schemaArray": { "type": "array", @@ -502,7 +502,8 @@ "minimum" ] }, - "default": {} + "default": {}, + "additionalProperties": false }, "tags": { "description": "User associated metadata assigned to this object.", diff --git a/st2common/st2common/models/api/action.py b/st2common/st2common/models/api/action.py index 2d5e1264fb..4f9f789128 100644 --- a/st2common/st2common/models/api/action.py +++ b/st2common/st2common/models/api/action.py @@ -116,17 +116,9 @@ class RunnerTypeAPI(BaseAPI): "type": "string", "required": False, }, - # Runners must always output json objects with action output in output_key property - "output_schema": util_schema.get_action_output_schema(), - # "output_schema": { - # "description": "Schema for the runner's output.", - # "type": "object", - # # using patternProperties like this implies that output_schema defines - # # the "properties" schema of an object where each key is a property name. - # "patternProperties": {r"^\w+$": util_schema.get_action_output_schema()}, - # "additionalProperties": False, - # "default": {}, - # }, + "output_schema": util_schema.get_action_output_schema( + description="Runner Output Schema" + ), }, "additionalProperties": False, } @@ -232,24 +224,9 @@ class ActionAPI(BaseAPI, APIUIDMixin): "additionalProperties": False, "default": {}, }, - "output_schema": util_schema.get_action_output_schema(), - # "output_schema": { - # "description": "Schema for the action's output.", - # "anyOf": [ - # util_schema.get_action_output_schema(), - # { - # "type": "object", - # # using patternProperties like this implies that output_schema - # # defines the "properties" schema of an object where each key - # # is a property name. - # "patternProperties": { - # r"^\w+$": util_schema.get_action_output_schema() - # }, - # "additionalProperties": False, - # }, - # ], - # "default": {}, - # }, + "output_schema": util_schema.get_action_output_schema( + description="Action Output Schema" + ), "tags": { "description": "User associated metadata assigned to this object.", "type": "array", diff --git a/st2common/st2common/util/schema/__init__.py b/st2common/st2common/util/schema/__init__.py index ef180d90ab..95fbf16e98 100644 --- a/st2common/st2common/util/schema/__init__.py +++ b/st2common/st2common/util/schema/__init__.py @@ -86,13 +86,16 @@ def get_draft_schema(version="custom", additional_properties=False): return schema -def get_action_output_schema(additional_properties=True): +def get_action_output_schema(additional_properties=False, description=None): """ Return a generic schema which is used for validating action output. """ - return get_draft_schema( + schema = get_draft_schema( version="action_output_schema", additional_properties=additional_properties ) + if description: + schema["description"] = f"{description} (based on {schema['description']})" + return schema def get_action_parameters_schema(additional_properties=False): From 10c828c3c4b9086ce65002bb04c3d4ddf6fb6472 Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Wed, 30 Mar 2022 09:48:58 -0500 Subject: [PATCH 19/36] secret_masking typing fixes --- st2common/st2common/util/output_schema.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/st2common/st2common/util/output_schema.py b/st2common/st2common/util/output_schema.py index c2e72d391e..b790c7e660 100644 --- a/st2common/st2common/util/output_schema.py +++ b/st2common/st2common/util/output_schema.py @@ -19,7 +19,7 @@ import traceback import jsonschema -from collections.abc import Collection, Mapping +from collections.abc import Mapping, MutableMapping, MutableSequence from st2common.util import schema from st2common.constants import action as action_constants from st2common.constants.secrets import MASKED_ATTRIBUTE_VALUE @@ -78,7 +78,7 @@ def _get_masked_value(spec, value): return value elif kind == "object": - if not isinstance(value, Mapping): + if not isinstance(value, MutableMapping): # we can't process it unless it matches the expected type return value @@ -125,7 +125,7 @@ def _get_masked_value(spec, value): return value elif kind == "array": - if not isinstance(value, Collection): + if not isinstance(value, MutableSequence): # we can't process it unless it matches the expected type return value @@ -136,11 +136,11 @@ def _get_masked_value(spec, value): for i, item_spec in enumerate(items_schema): if i >= output_count: break - value[i] = _get_masked_value(item_spec, value[key]) + value[i] = _get_masked_value(item_spec, value[i]) handled_count = len(items_schema) else: for i in range(output_count): - value[i] = _get_masked_value(items_schema, value[key]) + value[i] = _get_masked_value(items_schema, value[i]) handled_count = output_count if handled_count >= output_count: From cab0548fb23fa11a7173ef28d9a9dcb713dbac97 Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Wed, 30 Mar 2022 10:17:35 -0500 Subject: [PATCH 20/36] drop legacy partial object output_schema support --- st2common/st2common/util/output_schema.py | 81 +++++++++-------------- 1 file changed, 32 insertions(+), 49 deletions(-) diff --git a/st2common/st2common/util/output_schema.py b/st2common/st2common/util/output_schema.py index b790c7e660..0c8e07030a 100644 --- a/st2common/st2common/util/output_schema.py +++ b/st2common/st2common/util/output_schema.py @@ -32,16 +32,22 @@ _JSON_TYPES = _JSON_BASIC_TYPES | _JSON_COMPLEX_TYPES +def _schema_is_valid(_schema): + if not isinstance(_schema, Mapping): + # malformed schema + return False + if "type" not in _schema or not _schema["type"] in _JSON_TYPES: + # legacy partial object schema with jsonschemas for the properties + return False + return True + + def _validate_runner(runner_schema, result): LOG.debug("Validating runner output: %s", runner_schema) - if "type" not in runner_schema or runner_schema["type"] not in _JSON_TYPES: - # we have a partial object schema with jsonschemas for the properties - runner_schema = { - "type": "object", - "properties": runner_schema, - "additionalProperties": False, - } + if not _schema_is_valid(runner_schema): + LOG.warning("Ignoring invalid runner schema: %s", runner_schema) + return schema.validate(result, runner_schema, cls=schema.get_validator("custom")) @@ -49,16 +55,11 @@ def _validate_runner(runner_schema, result): def _validate_action(action_schema, result, output_key): LOG.debug("Validating action output: %s", action_schema) - final_result = result[output_key] + if not _schema_is_valid(action_schema): + LOG.warning("Ignoring invalid action schema: %s", action_schema) + return - # FIXME: is there a better way to check if action_schema is only a partial object schema? - if "type" not in action_schema or action_schema["type"] not in _JSON_TYPES: - # we have a partial object schema with jsonschemas for the properties - action_schema = { - "type": "object", - "properties": action_schema, - "additionalProperties": False, - } + final_result = result[output_key] schema.validate(final_result, action_schema, cls=schema.get_validator("custom")) @@ -159,46 +160,28 @@ def _get_masked_value(spec, value): def mask_secret_output(ac_ex, output_value): - # runners have to return a JSON object, with action output under a subkey. - if not output_value or not isinstance(output_value, Mapping): + if not output_value: return output_value output_key = ac_ex["runner"].get("output_key") output_schema = ac_ex["action"].get("output_schema") - # nothing to validate - if not output_key or output_key not in output_value or not output_schema: + if ( + # no action output_schema defined + not output_schema + # malformed action output_schema + or not _schema_is_valid(output_schema) + # if the runner does not provide an output_key, we can't use output_schema. + or not output_key + # cannot access output_key on which to apply output_schema + or (isinstance(output_value, Mapping) and output_key not in output_value) + ): + # nothing to validate return output_value - # malformed schema - if not isinstance(output_schema, Mapping): - return output_value - - # FIXME: is there a better way to check if output_schema is only a partial object schema? - if "type" in output_schema and output_schema["type"] in _JSON_TYPES: - # we have a full jsonschema - output_value[output_key] = _get_masked_value( - output_schema, output_value[output_key] - ) - else: - # we have a partial object schema with jsonschemas for the properties - # see st2common/st2common/models/api/action.py - # output_schema_schema = { - # "description": "Schema for the action's output.", - # "type": "object", - # "patternProperties": {r"^\w+$": customized_draft4_jsonschema} - # "additionalProperties": False, - # "default": {}, - # } - # This implies the following schema (as in _validate_action above) - implied_schema = { - "type": "object", - "properties": output_schema, - "additionalProperties": False, - } - output_value[output_key] = _get_masked_value( - implied_schema, output_value[output_key] - ) + output_value[output_key] = _get_masked_value( + output_schema, output_value[output_key] + ) return output_value From b904b1213598a680c6d9490bf765145b0432a79e Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Wed, 30 Mar 2022 10:29:20 -0500 Subject: [PATCH 21/36] Clarify why mask_output returns early --- st2common/st2common/util/output_schema.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/st2common/st2common/util/output_schema.py b/st2common/st2common/util/output_schema.py index 0c8e07030a..9415ac640d 100644 --- a/st2common/st2common/util/output_schema.py +++ b/st2common/st2common/util/output_schema.py @@ -164,6 +164,7 @@ def mask_secret_output(ac_ex, output_value): return output_value output_key = ac_ex["runner"].get("output_key") + # output_schema has masking info for data in output_value[output_key] output_schema = ac_ex["action"].get("output_schema") if ( @@ -171,12 +172,14 @@ def mask_secret_output(ac_ex, output_value): not output_schema # malformed action output_schema or not _schema_is_valid(output_schema) - # if the runner does not provide an output_key, we can't use output_schema. + # without output_key we cannot use output_schema or not output_key - # cannot access output_key on which to apply output_schema - or (isinstance(output_value, Mapping) and output_key not in output_value) + # cannot access output_key if output_value is not a dict + or not isinstance(output_value, Mapping) + # cannot mask output if it is missing + or output_key not in output_value ): - # nothing to validate + # nothing to mask return output_value output_value[output_key] = _get_masked_value( From d15db8896dbbaaba217ddd7d85a1e9698490ea2c Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Wed, 30 Mar 2022 10:36:22 -0500 Subject: [PATCH 22/36] Fix test output_schema in python_runner tests --- .../python_runner/tests/unit/test_output_schema.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/contrib/runners/python_runner/tests/unit/test_output_schema.py b/contrib/runners/python_runner/tests/unit/test_output_schema.py index 218a8f0732..e997d8b60b 100644 --- a/contrib/runners/python_runner/tests/unit/test_output_schema.py +++ b/contrib/runners/python_runner/tests/unit/test_output_schema.py @@ -44,10 +44,14 @@ MOCK_EXECUTION = mock.Mock() MOCK_EXECUTION.id = "598dbf0c0640fd54bffc688b" -FAIL_SCHEMA = { - "notvalid": { - "type": "string", +FAIL_OUTPUT_SCHEMA = { + "type": "object", + "properties": { + "notvalid": { + "type": "string", + }, }, + "additionalProperties": False, } @@ -78,7 +82,7 @@ def test_fail_incorrect_output_schema(self): runner.pre_run() (status, output, _) = runner.run({"row_index": 5}) with self.assertRaises(jsonschema.ValidationError): - output_schema._validate_runner(FAIL_SCHEMA, output) + output_schema._validate_runner(FAIL_OUTPUT_SCHEMA, output) def _get_mock_runner_obj(self, pack=None, sandbox=None): runner = python_runner.get_runner() From 3cf35af653d648f7fed4838f4325e0a2c8eaf69e Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Wed, 30 Mar 2022 11:01:32 -0500 Subject: [PATCH 23/36] Add more malformed output_schema tests --- .../tests/unit/test_util_output_schema.py | 54 ++++++++++++++++--- 1 file changed, 48 insertions(+), 6 deletions(-) diff --git a/st2common/tests/unit/test_util_output_schema.py b/st2common/tests/unit/test_util_output_schema.py index 8086b5a909..b775c3df4b 100644 --- a/st2common/tests/unit/test_util_output_schema.py +++ b/st2common/tests/unit/test_util_output_schema.py @@ -99,6 +99,19 @@ "additionalProperties": False, } +# Legacy schemas were implicitly a "properties" schema of an object. +# Now, this should be ignored as a malformed schema. +LEGACY_ACTION_OUTPUT_SCHEMA = ACTION_OUTPUT_SCHEMA_WITH_SECRET["properties"] + +MALFORMED_ACTION_OUTPUT_SCHEMA_1 = {"output_1": "bool"} +MALFORMED_ACTION_OUTPUT_SCHEMA_2 = { + "type": "object", + "properties": { + "output_1": "bool", + }, + "additionalProperties": False, +} + class OutputSchemaTestCase(unittest2.TestCase): def test_valid_schema(self): @@ -282,12 +295,41 @@ def test_mask_secret_output_noop(self): masked_output = output_schema.mask_secret_output(ac_ex, ac_ex_result) self.assertDictEqual(masked_output, expected_masked_output) - # Malformed schema can't be used to validate. - malformed_schema_ac_ex = copy.deepcopy(ac_ex) - malformed_schema_ac_ex["action"]["output_schema"] = {"output_1": "bool"} + def test_mask_secret_output_noop_legacy_schema(self): + ac_ex = { + "action": { + "output_schema": LEGACY_ACTION_OUTPUT_SCHEMA, + }, + "runner": { + "output_key": OUTPUT_KEY, + "output_schema": RUNNER_OUTPUT_SCHEMA, + }, + } ac_ex_result = {"output_1": "foobar"} expected_masked_output = {"output_1": "foobar"} - masked_output = output_schema.mask_secret_output( - malformed_schema_ac_ex, ac_ex_result - ) + + # Legacy schemas should be ignored since they aren't full json schemas. + masked_output = output_schema.mask_secret_output(ac_ex, ac_ex_result) + self.assertDictEqual(masked_output, expected_masked_output) + + def test_mask_secret_output_noop_malformed_schema(self): + # Malformed schemas can't be used to validate. + ac_ex = { + "action": { + "output_schema": {}, + }, + "runner": { + "output_key": OUTPUT_KEY, + "output_schema": RUNNER_OUTPUT_SCHEMA, + }, + } + ac_ex_result = {"output_1": "foobar"} + expected_masked_output = {"output_1": "foobar"} + + ac_ex["action"]["output_schema"] = MALFORMED_ACTION_OUTPUT_SCHEMA_1 + masked_output = output_schema.mask_secret_output(ac_ex, ac_ex_result) + self.assertDictEqual(masked_output, expected_masked_output) + + ac_ex["action"]["output_schema"] = MALFORMED_ACTION_OUTPUT_SCHEMA_2 + masked_output = output_schema.mask_secret_output(ac_ex, ac_ex_result) self.assertDictEqual(masked_output, expected_masked_output) From dc6af0e38c039f8714348c011b0a913ea56b9f4c Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Wed, 30 Mar 2022 11:23:35 -0500 Subject: [PATCH 24/36] More reliable output_schema._schema_is_valid "type" is not required, so relying on that is likely to run into edge cases. Instead, validate the schema before validating with the schema. --- st2common/st2common/util/output_schema.py | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/st2common/st2common/util/output_schema.py b/st2common/st2common/util/output_schema.py index 9415ac640d..0428631de9 100644 --- a/st2common/st2common/util/output_schema.py +++ b/st2common/st2common/util/output_schema.py @@ -36,8 +36,14 @@ def _schema_is_valid(_schema): if not isinstance(_schema, Mapping): # malformed schema return False - if "type" not in _schema or not _schema["type"] in _JSON_TYPES: - # legacy partial object schema with jsonschemas for the properties + try: + schema.validate( + _schema, + schema.get_action_output_schema(), + cls=schema.get_validator("custom"), + ) + except jsonschema.ValidationError: + # likely a legacy partial object schema (only defines properties) return False return True @@ -168,16 +174,16 @@ def mask_secret_output(ac_ex, output_value): output_schema = ac_ex["action"].get("output_schema") if ( - # no action output_schema defined - not output_schema - # malformed action output_schema - or not _schema_is_valid(output_schema) # without output_key we cannot use output_schema - or not output_key + not output_key # cannot access output_key if output_value is not a dict or not isinstance(output_value, Mapping) # cannot mask output if it is missing or output_key not in output_value + # no action output_schema defined + or not output_schema + # malformed action output_schema + or not _schema_is_valid(output_schema) ): # nothing to mask return output_value From 9dd72457fec9b5b727e0db02609221f1d1578e98 Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Wed, 30 Mar 2022 11:29:14 -0500 Subject: [PATCH 25/36] Refactor for clarity --- st2common/st2common/util/output_schema.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/st2common/st2common/util/output_schema.py b/st2common/st2common/util/output_schema.py index 0428631de9..82a24c9410 100644 --- a/st2common/st2common/util/output_schema.py +++ b/st2common/st2common/util/output_schema.py @@ -32,7 +32,7 @@ _JSON_TYPES = _JSON_BASIC_TYPES | _JSON_COMPLEX_TYPES -def _schema_is_valid(_schema): +def _output_schema_is_valid(_schema): if not isinstance(_schema, Mapping): # malformed schema return False @@ -51,7 +51,7 @@ def _schema_is_valid(_schema): def _validate_runner(runner_schema, result): LOG.debug("Validating runner output: %s", runner_schema) - if not _schema_is_valid(runner_schema): + if not _output_schema_is_valid(runner_schema): LOG.warning("Ignoring invalid runner schema: %s", runner_schema) return @@ -61,7 +61,7 @@ def _validate_runner(runner_schema, result): def _validate_action(action_schema, result, output_key): LOG.debug("Validating action output: %s", action_schema) - if not _schema_is_valid(action_schema): + if not _output_schema_is_valid(action_schema): LOG.warning("Ignoring invalid action schema: %s", action_schema) return @@ -177,13 +177,13 @@ def mask_secret_output(ac_ex, output_value): # without output_key we cannot use output_schema not output_key # cannot access output_key if output_value is not a dict - or not isinstance(output_value, Mapping) + or not isinstance(output_value, MutableMapping) # cannot mask output if it is missing or output_key not in output_value # no action output_schema defined or not output_schema # malformed action output_schema - or not _schema_is_valid(output_schema) + or not _output_schema_is_valid(output_schema) ): # nothing to mask return output_value From 5fc52f28ba5310467623f8265b5b1f33f0363085 Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Wed, 30 Mar 2022 11:58:13 -0500 Subject: [PATCH 26/36] add changelog entry --- CHANGELOG.rst | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 7c271c570f..bd4055f465 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -155,6 +155,41 @@ Added * Added garbage collection for rule_enforcement and trace models #5596 Contributed by Amanda McGuinness (@amanda11 intive) +Changed +~~~~~~~ + +* ``output_schema`` must be a full jsonschema now. If a schema is not well-formed, we ignore it. + Now, ``output`` can be types other than object such as list, bool, int, etc. + This also means that all of an action's output can be masked as a secret. + + To get the same behavior, you'll need to update your output schema. + For example, this schema: + + .. code-block:: yaml + + output_schema: + property1: + type: bool + property2: + type: str + + should be updated like this: + + .. code-block:: yaml + + output_schema: + type: object + properties: + property1: + type: bool + property2: + type: str + additionalProperties: false + + #5319 + + Contributed by @cognifloyd + Fixed ~~~~~ From 0f52eda3b2bb323517164e6bd76d7bde0d980942 Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Wed, 30 Mar 2022 13:43:23 -0500 Subject: [PATCH 27/36] revert action_output_schema additional_properties change --- contrib/schemas/action.json | 3 +-- st2common/st2common/util/schema/__init__.py | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/contrib/schemas/action.json b/contrib/schemas/action.json index f0e85b1feb..233bf56147 100644 --- a/contrib/schemas/action.json +++ b/contrib/schemas/action.json @@ -502,8 +502,7 @@ "minimum" ] }, - "default": {}, - "additionalProperties": false + "default": {} }, "tags": { "description": "User associated metadata assigned to this object.", diff --git a/st2common/st2common/util/schema/__init__.py b/st2common/st2common/util/schema/__init__.py index 95fbf16e98..cdd0418279 100644 --- a/st2common/st2common/util/schema/__init__.py +++ b/st2common/st2common/util/schema/__init__.py @@ -86,7 +86,7 @@ def get_draft_schema(version="custom", additional_properties=False): return schema -def get_action_output_schema(additional_properties=False, description=None): +def get_action_output_schema(additional_properties=True, description=None): """ Return a generic schema which is used for validating action output. """ From 4144013eda0aca17e95559a4a2afa9c965bc724f Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Wed, 30 Mar 2022 18:20:59 -0500 Subject: [PATCH 28/36] add test for top-level output secret masking --- .../tests/unit/test_util_output_schema.py | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/st2common/tests/unit/test_util_output_schema.py b/st2common/tests/unit/test_util_output_schema.py index b775c3df4b..4e19f761e5 100644 --- a/st2common/tests/unit/test_util_output_schema.py +++ b/st2common/tests/unit/test_util_output_schema.py @@ -36,6 +36,16 @@ } } +ACTION_RESULT_ALT_TYPES = { + "boolean": {"output": True}, + "integer": {"output": 42}, + "null": {"output": None}, + "number": {"output": 1.234}, + "string": {"output": "foobar"}, + "object": ACTION_RESULT, + "array": {"output": [ACTION_RESULT]}, +} + RUNNER_OUTPUT_SCHEMA = { "type": "object", "properties": { @@ -197,6 +207,28 @@ def test_mask_secret_output(self): ) self.assertDictEqual(masked_output, expected_masked_output) + def test_mask_secret_output_all_output(self): + ac_ex = { + "action": { + "output_schema": { + "secret": True, + }, + }, + "runner": { + "output_key": OUTPUT_KEY, + "output_schema": RUNNER_OUTPUT_SCHEMA, + }, + } + + expected_masked_output = {"output": MASKED_ATTRIBUTE_VALUE} + + for kind, action_result in ACTION_RESULT_ALT_TYPES.items(): + ac_ex["action"]["output_schema"]["type"] = kind + masked_output = output_schema.mask_secret_output( + ac_ex, copy.deepcopy(action_result) + ) + self.assertDictEqual(masked_output, expected_masked_output) + def test_mask_secret_output_no_secret(self): ac_ex = { "action": { From 398d2738fbe7d65e2d41d5852f5c4847842fa03e Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Wed, 30 Mar 2022 18:35:33 -0500 Subject: [PATCH 29/36] Reduce duplication in output_schema tests --- .../tests/unit/test_util_output_schema.py | 61 ++++++------------- 1 file changed, 20 insertions(+), 41 deletions(-) diff --git a/st2common/tests/unit/test_util_output_schema.py b/st2common/tests/unit/test_util_output_schema.py index 4e19f761e5..ddf061b965 100644 --- a/st2common/tests/unit/test_util_output_schema.py +++ b/st2common/tests/unit/test_util_output_schema.py @@ -37,7 +37,6 @@ } ACTION_RESULT_ALT_TYPES = { - "boolean": {"output": True}, "integer": {"output": 42}, "null": {"output": None}, "number": {"output": 1.234}, @@ -46,6 +45,11 @@ "array": {"output": [ACTION_RESULT]}, } +ACTION_RESULT_BOOLEANS = { + True: {"output": True}, + False: {"output": False}, +} + RUNNER_OUTPUT_SCHEMA = { "type": "object", "properties": { @@ -229,6 +233,13 @@ def test_mask_secret_output_all_output(self): ) self.assertDictEqual(masked_output, expected_masked_output) + for _, action_result in ACTION_RESULT_BOOLEANS.items(): + ac_ex["action"]["output_schema"]["type"] = "boolean" + masked_output = output_schema.mask_secret_output( + ac_ex, copy.deepcopy(action_result) + ) + self.assertDictEqual(masked_output, expected_masked_output) + def test_mask_secret_output_no_secret(self): ac_ex = { "action": { @@ -279,47 +290,15 @@ def test_mask_secret_output_noop(self): masked_output = output_schema.mask_secret_output(ac_ex, ac_ex_result) self.assertDictEqual(masked_output, expected_masked_output) - # The output is type of None. - ac_ex_result = {"output": None} - expected_masked_output = {"output": None} - masked_output = output_schema.mask_secret_output(ac_ex, ac_ex_result) - self.assertDictEqual(masked_output, expected_masked_output) - - # The output is string type: not dict - ac_ex_result = {"output": "foobar"} - expected_masked_output = {"output": "foobar"} - masked_output = output_schema.mask_secret_output(ac_ex, ac_ex_result) - self.assertDictEqual(masked_output, expected_masked_output) - - # The output is number / int type: not dict - ac_ex_result = {"output": 42} - expected_masked_output = {"output": 42} - masked_output = output_schema.mask_secret_output(ac_ex, ac_ex_result) - self.assertDictEqual(masked_output, expected_masked_output) + # output_schema covers objects but output is not a dict/object. + for _, action_result in ACTION_RESULT_ALT_TYPES.items(): + masked_output = output_schema.mask_secret_output(ac_ex, action_result) + self.assertDictEqual(masked_output, action_result) - # The output is number / float type: not dict - ac_ex_result = {"output": 4.2} - expected_masked_output = {"output": 4.2} - masked_output = output_schema.mask_secret_output(ac_ex, ac_ex_result) - self.assertDictEqual(masked_output, expected_masked_output) - - # The output is True (bool type): not dict - ac_ex_result = {"output": True} - expected_masked_output = {"output": True} - masked_output = output_schema.mask_secret_output(ac_ex, ac_ex_result) - self.assertDictEqual(masked_output, expected_masked_output) - - # The output is False (bool type): not dict - ac_ex_result = {"output": False} - expected_masked_output = {"output": False} - masked_output = output_schema.mask_secret_output(ac_ex, ac_ex_result) - self.assertDictEqual(masked_output, expected_masked_output) - - # The output is list type: not dict - ac_ex_result = {"output": ["foobar"]} - expected_masked_output = {"output": ["foobar"]} - masked_output = output_schema.mask_secret_output(ac_ex, ac_ex_result) - self.assertDictEqual(masked_output, expected_masked_output) + # output_schema covers objects but output is a bool. + for _, action_result in ACTION_RESULT_BOOLEANS.items(): + masked_output = output_schema.mask_secret_output(ac_ex, action_result) + self.assertDictEqual(masked_output, action_result) # The output key is missing. ac_ex_result = {"output1": None} From accd6796e363996a537183623841b832a399dea3 Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Wed, 30 Mar 2022 18:49:31 -0500 Subject: [PATCH 30/36] Fix typo in ouptut_schema tests --- st2common/tests/unit/test_util_output_schema.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/st2common/tests/unit/test_util_output_schema.py b/st2common/tests/unit/test_util_output_schema.py index ddf061b965..9de0b2568d 100644 --- a/st2common/tests/unit/test_util_output_schema.py +++ b/st2common/tests/unit/test_util_output_schema.py @@ -67,7 +67,7 @@ "output_3": {"type": "string"}, "deep_output": { "type": "object", - "parameters": { + "properties": { "deep_item_1": { "type": "string", }, @@ -103,7 +103,7 @@ "output_3": {"type": "string", "secret": True}, "deep_output": { "type": "object", - "parameters": { + "properties": { "deep_item_1": { "type": "string", }, From dfad436413ffca12daf6139718c1d22f298216b3 Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Wed, 30 Mar 2022 20:02:06 -0500 Subject: [PATCH 31/36] Fix issues with output_schema array handling --- st2common/st2common/util/output_schema.py | 4 +- st2common/st2common/util/schema/__init__.py | 24 +++-- .../tests/unit/test_util_output_schema.py | 99 +++++++++++++++---- 3 files changed, 101 insertions(+), 26 deletions(-) diff --git a/st2common/st2common/util/output_schema.py b/st2common/st2common/util/output_schema.py index 82a24c9410..24f4f8b336 100644 --- a/st2common/st2common/util/output_schema.py +++ b/st2common/st2common/util/output_schema.py @@ -19,7 +19,7 @@ import traceback import jsonschema -from collections.abc import Mapping, MutableMapping, MutableSequence +from collections.abc import Mapping, MutableMapping, MutableSequence, Sequence from st2common.util import schema from st2common.constants import action as action_constants from st2common.constants.secrets import MASKED_ATTRIBUTE_VALUE @@ -138,7 +138,7 @@ def _get_masked_value(spec, value): items_schema = spec.get("items", {}) output_count = len(value) - if isinstance(items_schema, Mapping): + if isinstance(items_schema, Sequence): # explicit schema for each item for i, item_spec in enumerate(items_schema): if i >= output_count: diff --git a/st2common/st2common/util/schema/__init__.py b/st2common/st2common/util/schema/__init__.py index cdd0418279..94936df0b7 100644 --- a/st2common/st2common/util/schema/__init__.py +++ b/st2common/st2common/util/schema/__init__.py @@ -16,6 +16,7 @@ from __future__ import absolute_import import os +from typing import Mapping, Sequence import six import jsonschema @@ -247,16 +248,25 @@ def assign_default_values(instance, schema): if ( is_attribute_type_array(attribute_type) and schema_items - and schema_items.get("properties", {}) ): array_instance = instance.get(property_name, None) - array_schema = schema["properties"][property_name]["items"] - + # Note: We don't perform subschema assignment if no value is provided if array_instance is not None: - # Note: We don't perform subschema assignment if no value is provided - instance[property_name] = assign_default_values( - instance=array_instance, schema=array_schema - ) + if ( + isinstance(schema_items, Mapping) + and schema_items.get("properties", {}) + ): + instance[property_name] = assign_default_values( + instance=array_instance, schema=schema_items + ) + elif array_instance and isinstance(schema_items, Sequence): + array_instance_count = len(array_instance) + for i, item_schema in enumerate(schema_items): + if i > array_instance_count: + break + instance[property_name][i] = assign_default_values( + instance=array_instance[i], schema=item_schema + ) # Object if is_attribute_type_object(attribute_type) and property_data.get( diff --git a/st2common/tests/unit/test_util_output_schema.py b/st2common/tests/unit/test_util_output_schema.py index 9de0b2568d..d29cfbdbeb 100644 --- a/st2common/tests/unit/test_util_output_schema.py +++ b/st2common/tests/unit/test_util_output_schema.py @@ -33,6 +33,12 @@ "deep_output": { "deep_item_1": "Jindal", }, + "array_output_1": [ + {"deep_item_1": "foo"}, + {"deep_item_1": "bar"}, + {"deep_item_1": "baz"}, + ], + "array_output_2": ["answer", 4.2, True, False], } } @@ -41,8 +47,8 @@ "null": {"output": None}, "number": {"output": 1.234}, "string": {"output": "foobar"}, - "object": ACTION_RESULT, - "array": {"output": [ACTION_RESULT]}, + "object": {"output": {"prop": "value"}}, + "array": {"output": [{"prop": "value"}]}, } ACTION_RESULT_BOOLEANS = { @@ -68,11 +74,26 @@ "deep_output": { "type": "object", "properties": { - "deep_item_1": { - "type": "string", - }, + "deep_item_1": {"type": "string"}, }, }, + "array_output_1": { + "type": "array", + "items": { + "type": "object", + "properties": { + "deep_item_1": {"type": "string"}, + }, + } + }, + "array_output_2": { + "type": "array", + "items": [ + {"type": "string"}, + {"type": "number"}, + ], + "additionalItems": {"type": "boolean"}, + }, }, "additionalProperties": False, } @@ -104,11 +125,26 @@ "deep_output": { "type": "object", "properties": { - "deep_item_1": { - "type": "string", - }, + "deep_item_1": {"type": "string"}, }, }, + "array_output_1": { + "type": "array", + "items": { + "type": "object", + "properties": { + "deep_item_1": {"type": "string", "secret": True}, + }, + } + }, + "array_output_2": { + "type": "array", + "items": [ + {"type": "string"}, + {"type": "number", "secret": True}, + ], + "additionalItems": {"type": "boolean", "secret": True}, + }, }, "additionalProperties": False, } @@ -152,11 +188,19 @@ def test_invalid_runner_schema(self): expected_result = { "error": ( "Additional properties are not allowed ('output' was unexpected)\n\n" - "Failed validating 'additionalProperties' in schema:\n " - "{'additionalProperties': False,\n 'properties': {'not_a_key_you_have': " - "{'type': 'string'}},\n 'type': 'object'}\n\nOn instance:\n {'output': " - "{'deep_output': {'deep_item_1': 'Jindal'},\n 'output_1': 'Bobby'," - "\n 'output_2': 5,\n 'output_3': 'shhh!'}}" + "Failed validating 'additionalProperties' in schema:\n" + " {'additionalProperties': False,\n" + " 'properties': {'not_a_key_you_have': {'type': 'string'}},\n" + " 'type': 'object'}\n\n" + "On instance:\n" + " {'output': {'array_output_1': [{'deep_item_1': 'foo'},\n" + " {'deep_item_1': 'bar'},\n" + " {'deep_item_1': 'baz'}],\n" + " 'array_output_2': ['answer', 4.2, True, False],\n" + " 'deep_output': {'deep_item_1': 'Jindal'},\n" + " 'output_1': 'Bobby',\n" + " 'output_2': 5,\n" + " 'output_3': 'shhh!'}}" ), "message": "Error validating output. See error output for more details.", } @@ -203,6 +247,17 @@ def test_mask_secret_output(self): "deep_output": { "deep_item_1": "Jindal", }, + "array_output_1": [ + {"deep_item_1": MASKED_ATTRIBUTE_VALUE}, + {"deep_item_1": MASKED_ATTRIBUTE_VALUE}, + {"deep_item_1": MASKED_ATTRIBUTE_VALUE}, + ], + "array_output_2": [ + "answer", + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE, + ], } } @@ -259,6 +314,12 @@ def test_mask_secret_output_no_secret(self): "deep_output": { "deep_item_1": "Jindal", }, + "array_output_1": [ + {"deep_item_1": "foo"}, + {"deep_item_1": "bar"}, + {"deep_item_1": "baz"}, + ], + "array_output_2": ["answer", 4.2, True, False], } } @@ -292,13 +353,17 @@ def test_mask_secret_output_noop(self): # output_schema covers objects but output is not a dict/object. for _, action_result in ACTION_RESULT_ALT_TYPES.items(): - masked_output = output_schema.mask_secret_output(ac_ex, action_result) - self.assertDictEqual(masked_output, action_result) + ac_ex_result = copy.deepcopy(action_result) + expected_masked_output = copy.deepcopy(action_result) + masked_output = output_schema.mask_secret_output(ac_ex, ac_ex_result) + self.assertDictEqual(masked_output, expected_masked_output) # output_schema covers objects but output is a bool. for _, action_result in ACTION_RESULT_BOOLEANS.items(): - masked_output = output_schema.mask_secret_output(ac_ex, action_result) - self.assertDictEqual(masked_output, action_result) + ac_ex_result = copy.deepcopy(action_result) + expected_masked_output = copy.deepcopy(action_result) + masked_output = output_schema.mask_secret_output(ac_ex, ac_ex_result) + self.assertDictEqual(masked_output, expected_masked_output) # The output key is missing. ac_ex_result = {"output1": None} From 8e925d093b5b6d66428714d332c5ac88ff9d3d42 Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Wed, 30 Mar 2022 21:47:17 -0500 Subject: [PATCH 32/36] Add debug log for schema validation errors --- st2common/st2common/util/output_schema.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/st2common/st2common/util/output_schema.py b/st2common/st2common/util/output_schema.py index 24f4f8b336..4bdf369c3c 100644 --- a/st2common/st2common/util/output_schema.py +++ b/st2common/st2common/util/output_schema.py @@ -42,7 +42,8 @@ def _output_schema_is_valid(_schema): schema.get_action_output_schema(), cls=schema.get_validator("custom"), ) - except jsonschema.ValidationError: + except jsonschema.ValidationError as e: + LOG.debug("output_schema not valid: %s", e) # likely a legacy partial object schema (only defines properties) return False return True From 91ddd421c5ebb98c20b70ec97be55b7f38225994 Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Wed, 30 Mar 2022 21:47:35 -0500 Subject: [PATCH 33/36] test output_schema with additionalProperties schema --- .../tests/unit/test_util_output_schema.py | 27 +++++++------------ 1 file changed, 10 insertions(+), 17 deletions(-) diff --git a/st2common/tests/unit/test_util_output_schema.py b/st2common/tests/unit/test_util_output_schema.py index d29cfbdbeb..4afce48b18 100644 --- a/st2common/tests/unit/test_util_output_schema.py +++ b/st2common/tests/unit/test_util_output_schema.py @@ -32,6 +32,8 @@ "output_3": "shhh!", "deep_output": { "deep_item_1": "Jindal", + "extra_item_1": 42, + "extra_item_2": 33, }, "array_output_1": [ {"deep_item_1": "foo"}, @@ -76,6 +78,7 @@ "properties": { "deep_item_1": {"type": "string"}, }, + "additionalProperties": {"type": "integer"}, }, "array_output_1": { "type": "array", @@ -127,6 +130,7 @@ "properties": { "deep_item_1": {"type": "string"}, }, + "additionalProperties": {"type": "integer", "secret": True}, }, "array_output_1": { "type": "array", @@ -197,7 +201,9 @@ def test_invalid_runner_schema(self): " {'deep_item_1': 'bar'},\n" " {'deep_item_1': 'baz'}],\n" " 'array_output_2': ['answer', 4.2, True, False],\n" - " 'deep_output': {'deep_item_1': 'Jindal'},\n" + " 'deep_output': {'deep_item_1': 'Jindal',\n" + " 'extra_item_1': 42,\n" + " 'extra_item_2': 33},\n" " 'output_1': 'Bobby',\n" " 'output_2': 5,\n" " 'output_3': 'shhh!'}}" @@ -246,6 +252,8 @@ def test_mask_secret_output(self): "output_3": MASKED_ATTRIBUTE_VALUE, "deep_output": { "deep_item_1": "Jindal", + "extra_item_1": MASKED_ATTRIBUTE_VALUE, + "extra_item_2": MASKED_ATTRIBUTE_VALUE, }, "array_output_1": [ {"deep_item_1": MASKED_ATTRIBUTE_VALUE}, @@ -306,22 +314,7 @@ def test_mask_secret_output_no_secret(self): }, } - expected_masked_output = { - "output": { - "output_1": "Bobby", - "output_2": 5, - "output_3": "shhh!", - "deep_output": { - "deep_item_1": "Jindal", - }, - "array_output_1": [ - {"deep_item_1": "foo"}, - {"deep_item_1": "bar"}, - {"deep_item_1": "baz"}, - ], - "array_output_2": ["answer", 4.2, True, False], - } - } + expected_masked_output = copy.deepcopy(ACTION_RESULT) masked_output = output_schema.mask_secret_output( ac_ex, copy.deepcopy(ACTION_RESULT) From 5618d34ca200f19e17c7e7c14119f1c990937c4b Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Wed, 30 Mar 2022 22:01:37 -0500 Subject: [PATCH 34/36] fix output_schema with patternProperties schema --- st2common/st2common/util/output_schema.py | 2 +- .../tests/unit/test_util_output_schema.py | 27 ++++++++++++++++++- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/st2common/st2common/util/output_schema.py b/st2common/st2common/util/output_schema.py index 4bdf369c3c..902908b653 100644 --- a/st2common/st2common/util/output_schema.py +++ b/st2common/st2common/util/output_schema.py @@ -108,7 +108,7 @@ def _get_masked_value(spec, value): and isinstance(pattern_properties_schema, Mapping) ): # patternProperties is not malformed - for key_pattern, pattern_property_spec in pattern_properties_schema: + for key_pattern, pattern_property_spec in pattern_properties_schema.items(): if not unhandled_keys: # nothing to check, don't compile the next pattern break diff --git a/st2common/tests/unit/test_util_output_schema.py b/st2common/tests/unit/test_util_output_schema.py index 4afce48b18..34cce73d73 100644 --- a/st2common/tests/unit/test_util_output_schema.py +++ b/st2common/tests/unit/test_util_output_schema.py @@ -35,6 +35,11 @@ "extra_item_1": 42, "extra_item_2": 33, }, + "pattern_output": { + "a": "x", + "b": "y", + "c": "z", + }, "array_output_1": [ {"deep_item_1": "foo"}, {"deep_item_1": "bar"}, @@ -80,6 +85,13 @@ }, "additionalProperties": {"type": "integer"}, }, + "pattern_output": { + "type": "object", + "patternProperties": { + "^\\w$": {"type": "string"}, + }, + "additionalProperties": False, + }, "array_output_1": { "type": "array", "items": { @@ -132,6 +144,13 @@ }, "additionalProperties": {"type": "integer", "secret": True}, }, + "pattern_output": { + "type": "object", + "patternProperties": { + "^\\w$": {"type": "string", "secret": True}, + }, + "additionalProperties": False, + }, "array_output_1": { "type": "array", "items": { @@ -206,7 +225,8 @@ def test_invalid_runner_schema(self): " 'extra_item_2': 33},\n" " 'output_1': 'Bobby',\n" " 'output_2': 5,\n" - " 'output_3': 'shhh!'}}" + " 'output_3': 'shhh!',\n" + " 'pattern_output': {'a': 'x', 'b': 'y', 'c': 'z'}}}" ), "message": "Error validating output. See error output for more details.", } @@ -255,6 +275,11 @@ def test_mask_secret_output(self): "extra_item_1": MASKED_ATTRIBUTE_VALUE, "extra_item_2": MASKED_ATTRIBUTE_VALUE, }, + "pattern_output": { + "a": MASKED_ATTRIBUTE_VALUE, + "b": MASKED_ATTRIBUTE_VALUE, + "c": MASKED_ATTRIBUTE_VALUE, + }, "array_output_1": [ {"deep_item_1": MASKED_ATTRIBUTE_VALUE}, {"deep_item_1": MASKED_ATTRIBUTE_VALUE}, From 3e1a052f27d8deb2d53402ee60e75126e10f6c69 Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Wed, 30 Mar 2022 22:02:09 -0500 Subject: [PATCH 35/36] reformat with black --- st2common/st2common/util/schema/__init__.py | 10 +++------- st2common/tests/unit/test_util_output_schema.py | 4 ++-- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/st2common/st2common/util/schema/__init__.py b/st2common/st2common/util/schema/__init__.py index 94936df0b7..569cc4c248 100644 --- a/st2common/st2common/util/schema/__init__.py +++ b/st2common/st2common/util/schema/__init__.py @@ -245,16 +245,12 @@ def assign_default_values(instance, schema): schema_items = property_data.get("items", {}) # Array - if ( - is_attribute_type_array(attribute_type) - and schema_items - ): + if is_attribute_type_array(attribute_type) and schema_items: array_instance = instance.get(property_name, None) # Note: We don't perform subschema assignment if no value is provided if array_instance is not None: - if ( - isinstance(schema_items, Mapping) - and schema_items.get("properties", {}) + if isinstance(schema_items, Mapping) and schema_items.get( + "properties", {} ): instance[property_name] = assign_default_values( instance=array_instance, schema=schema_items diff --git a/st2common/tests/unit/test_util_output_schema.py b/st2common/tests/unit/test_util_output_schema.py index 34cce73d73..c4db4921b7 100644 --- a/st2common/tests/unit/test_util_output_schema.py +++ b/st2common/tests/unit/test_util_output_schema.py @@ -99,7 +99,7 @@ "properties": { "deep_item_1": {"type": "string"}, }, - } + }, }, "array_output_2": { "type": "array", @@ -158,7 +158,7 @@ "properties": { "deep_item_1": {"type": "string", "secret": True}, }, - } + }, }, "array_output_2": { "type": "array", From 975c24234707bb3cdc7aeb0b10824f7db0106498 Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Sat, 16 Jul 2022 17:58:26 -0500 Subject: [PATCH 36/36] highlight that the output_schema change is a breaking change --- CHANGELOG.rst | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 6178ce718c..440c65b4a9 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -28,7 +28,11 @@ Added Changed ~~~~~~~ -* ``output_schema`` must be a full jsonschema now. If a schema is not well-formed, we ignore it. +* BREAKING CHANGE for anyone that uses ``output_schema``, which is disabled by default. + If you have ``[system].validate_output_schema = True`` in st2.conf AND you have added + ``output_schema`` to any of your packs, then you must update your action metadata. + + ``output_schema`` must be a full jsonschema now. If a schema is not well-formed, we ignore it. Now, ``output`` can be types other than object such as list, bool, int, etc. This also means that all of an action's output can be masked as a secret.