diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 4c9693a425..440c65b4a9 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -25,6 +25,45 @@ Added * Added graceful shutdown for workflow engine. #5463 Contributed by @khushboobhatia01 +Changed +~~~~~~~ + +* 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. + + 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 + 3.7.0 - May 05, 2022 -------------------- @@ -143,6 +182,7 @@ Added * Added garbage collection for rule_enforcement and trace models #5596/5602 Contributed by Amanda McGuinness (@amanda11 intive) + * Added garbage collection for workflow execution and task execution objects #4924 Contributed by @srimandaleeka01 and @amanda11 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..64b3d25156 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,10 @@ type: array output_key: published output_schema: - published: - type: "object" - tasks: - type: "array" + type: object + properties: + published: + 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 168f68bbb2..1f45c3689f 100644 --- a/contrib/runners/http_runner/http_runner/runner.yaml +++ b/contrib/runners/http_runner/http_runner/runner.yaml @@ -54,18 +54,21 @@ 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 + additionalProperties: false diff --git a/contrib/runners/orquesta_runner/orquesta_runner/runner.yaml b/contrib/runners/orquesta_runner/orquesta_runner/runner.yaml index 45a80d2621..e666158664 100644 --- a/contrib/runners/orquesta_runner/orquesta_runner/runner.yaml +++ b/contrib/runners/orquesta_runner/orquesta_runner/runner.yaml @@ -16,16 +16,19 @@ 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" + additionalProperties: false diff --git a/contrib/runners/python_runner/python_runner/runner.yaml b/contrib/runners/python_runner/python_runner/runner.yaml index 92ee7e15a7..b9baf2e5a4 100644 --- a/contrib/runners/python_runner/python_runner/runner.yaml +++ b/contrib/runners/python_runner/python_runner/runner.yaml @@ -4,24 +4,27 @@ 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 + additionalProperties: false runner_parameters: debug: description: Enable runner debug mode. 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() diff --git a/contrib/schemas/action.json b/contrib/schemas/action.json index 06049d348a..233bf56147 100644 --- a/contrib/schemas/action.json +++ b/contrib/schemas/action.json @@ -279,237 +279,229 @@ "default": {} }, "output_schema": { - "description": "Schema for the action's output.", - "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" - ] + "id": "http://json-schema.org/draft-04/schema#", + "$schema": "http://json-schema.org/draft-04/schema#", + "description": "Action Output Schema (based on 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", + "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": "#" } }, - "additionalProperties": false, + "dependencies": { + "exclusiveMaximum": [ + "maximum" + ], + "exclusiveMinimum": [ + "minimum" + ] + }, "default": {} }, "tags": { diff --git a/st2api/tests/unit/controllers/v1/test_executions.py b/st2api/tests/unit/controllers/v1/test_executions.py index a114ce843b..3a718b53c6 100644 --- a/st2api/tests/unit/controllers/v1/test_executions.py +++ b/st2api/tests/unit/controllers/v1/test_executions.py @@ -180,8 +180,12 @@ "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}, + }, + "additionalProperties": False, }, } @@ -194,8 +198,12 @@ "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}, + }, + "additionalProperties": False, }, } ACTION_DEFAULT_ENCRYPT_AND_BOOL = { diff --git a/st2common/st2common/models/api/action.py b/st2common/st2common/models/api/action.py index 6c4149745a..4f9f789128 100644 --- a/st2common/st2common/models/api/action.py +++ b/st2common/st2common/models/api/action.py @@ -116,13 +116,9 @@ class RunnerTypeAPI(BaseAPI): "type": "string", "required": False, }, - "output_schema": { - "description": "Schema for the runner's output.", - "type": "object", - "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, } @@ -228,13 +224,9 @@ class ActionAPI(BaseAPI, APIUIDMixin): "additionalProperties": False, "default": {}, }, - "output_schema": { - "description": "Schema for the action's output.", - "type": "object", - "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/output_schema.py b/st2common/st2common/util/output_schema.py index 7397e91ec3..902908b653 100644 --- a/st2common/st2common/util/output_schema.py +++ b/st2common/st2common/util/output_schema.py @@ -14,11 +14,12 @@ # limitations under the License. import logging +import re import sys import traceback import jsonschema -from collections.abc import Mapping +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 @@ -26,15 +27,34 @@ 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 _output_schema_is_valid(_schema): + if not isinstance(_schema, Mapping): + # malformed schema + return False + try: + schema.validate( + _schema, + schema.get_action_output_schema(), + cls=schema.get_validator("custom"), + ) + 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 + def _validate_runner(runner_schema, result): LOG.debug("Validating runner output: %s", runner_schema) - runner_schema = { - "type": "object", - "properties": runner_schema, - "additionalProperties": False, - } + if not _output_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")) @@ -42,46 +62,136 @@ 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 _output_schema_is_valid(action_schema): + LOG.warning("Ignoring invalid action schema: %s", action_schema) + return - 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")) +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 _JSON_BASIC_TYPES: + # already checked for spec["secret"] above; nothing else to check. + return value + + elif kind == "object": + if not isinstance(value, MutableMapping): + # 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 + 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 ( + 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.items(): + 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): + value[key] = _get_masked_value( + pattern_property_spec, value[key] + ) + unhandled_keys.remove(key) + + additional_properties_schema = spec.get("additionalProperties") + 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: + value[key] = _get_masked_value(additional_properties_schema, value[key]) + + return value + + elif kind == "array": + if not isinstance(value, MutableSequence): + # 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, Sequence): + # 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[i]) + handled_count = len(items_schema) + else: + for i in range(output_count): + value[i] = _get_masked_value(items_schema, value[i]) + 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): if not output_value: return output_value - if not isinstance(output_value, dict) and not isinstance(output_value, list): - 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 not output_key or not output_schema: - return output_value - - if not output_value.get(output_key): + if ( + # 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, 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 _output_schema_is_valid(output_schema) + ): + # nothing to mask return output_value - if not isinstance(output_value[output_key], Mapping): - # TODO: Allow output_schema + masking for non-dict action output. - # Current implementation expects actions to return JSON dicts. - return output_value - - for key, spec in output_schema.items(): - if not isinstance(spec, Mapping): - # TODO: Spec should be a jsonschema object. This will change in a future - # release, so we just ignore invalid schemas for now. - continue - if key in output_value[output_key] and spec.get("secret", False): - output_value[output_key][key] = MASKED_ATTRIBUTE_VALUE - + output_value[output_key] = _get_masked_value( + output_schema, output_value[output_key] + ) return output_value diff --git a/st2common/st2common/util/schema/__init__.py b/st2common/st2common/util/schema/__init__.py index 063b963629..569cc4c248 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 @@ -86,13 +87,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=True, 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): @@ -207,9 +211,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 @@ -227,19 +245,24 @@ def assign_default_values(instance, schema): schema_items = property_data.get("items", {}) # Array - if ( - is_attribute_type_array(attribute_type) - and schema_items - and schema_items.get("properties", {}) - ): + if is_attribute_type_array(attribute_type) and schema_items: 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/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 64ed13237e..c4db4921b7 100644 --- a/st2common/tests/unit/test_util_output_schema.py +++ b/st2common/tests/unit/test_util_output_schema.py @@ -32,51 +32,157 @@ "output_3": "shhh!", "deep_output": { "deep_item_1": "Jindal", + "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"}, + {"deep_item_1": "baz"}, + ], + "array_output_2": ["answer", 4.2, True, False], } } +ACTION_RESULT_ALT_TYPES = { + "integer": {"output": 42}, + "null": {"output": None}, + "number": {"output": 1.234}, + "string": {"output": "foobar"}, + "object": {"output": {"prop": "value"}}, + "array": {"output": [{"prop": "value"}]}, +} + +ACTION_RESULT_BOOLEANS = { + True: {"output": True}, + False: {"output": False}, +} + 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", + "properties": { + "deep_item_1": {"type": "string"}, + }, + "additionalProperties": {"type": "integer"}, + }, + "pattern_output": { + "type": "object", + "patternProperties": { + "^\\w$": {"type": "string"}, + }, + "additionalProperties": False, + }, + "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, } 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", + "properties": { + "deep_item_1": {"type": "string"}, + }, + "additionalProperties": {"type": "integer", "secret": True}, + }, + "pattern_output": { + "type": "object", + "patternProperties": { + "^\\w$": {"type": "string", "secret": True}, }, + "additionalProperties": False, }, + "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, +} + +# 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, } @@ -105,11 +211,22 @@ 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" + " 'extra_item_1': 42,\n" + " 'extra_item_2': 33},\n" + " 'output_1': 'Bobby',\n" + " 'output_2': 5,\n" + " 'output_3': 'shhh!',\n" + " 'pattern_output': {'a': 'x', 'b': 'y', 'c': 'z'}}}" ), "message": "Error validating output. See error output for more details.", } @@ -155,7 +272,25 @@ 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, + }, + "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}, + {"deep_item_1": MASKED_ATTRIBUTE_VALUE}, + ], + "array_output_2": [ + "answer", + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE, + ], } } @@ -164,6 +299,35 @@ 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) + + 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": { @@ -175,16 +339,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", - }, - } - } + expected_masked_output = copy.deepcopy(ACTION_RESULT) masked_output = output_schema.mask_secret_output( ac_ex, copy.deepcopy(ACTION_RESULT) @@ -214,20 +369,61 @@ 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} + # output_schema covers objects but output is not a dict/object. + for _, action_result in ACTION_RESULT_ALT_TYPES.items(): + 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(): + 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} + expected_masked_output = {"output1": None} 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. - ac_ex_result = {"output": "foobar"} - expected_masked_output = {"output": "foobar"} + 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"} + + # 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) - # The output key is missing. - ac_ex_result = {"output1": None} - expected_masked_output = {"output1": None} + 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) diff --git a/st2tests/st2tests/fixtures/generic/runners/run-local.yaml b/st2tests/st2tests/fixtures/generic/runners/run-local.yaml index 557dcb99fc..afa5e8971f 100644 --- a/st2tests/st2tests/fixtures/generic/runners/run-local.yaml +++ b/st2tests/st2tests/fixtures/generic/runners/run-local.yaml @@ -15,13 +15,16 @@ 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 + 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 dadeb4b59c..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 @@ -11,12 +11,15 @@ 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 + 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 87553c9a76..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 @@ -5,12 +5,15 @@ 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 + 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 85105f435e..18474222c4 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,19 @@ 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 + 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 b6126e8ce1..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 @@ -9,7 +9,10 @@ parameters: type: string required: true output_schema: - k2: - type: string - required: true - secret: true + type: object + properties: + k2: + 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 81b3a38289..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 @@ -11,5 +11,8 @@ parameters: type: string default: Stanley output_schema: - notakey: - type: boolean + type: object + 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 cd0d5b8b34..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 @@ -11,5 +11,8 @@ parameters: type: string default: Stanley output_schema: - msg: - type: string + type: object + properties: + msg: + type: string + additionalProperties: false