Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow output_schema to validate non-object action output (int, bool, etc) #5319

Merged
merged 44 commits into from
Jul 18, 2022

Conversation

cognifloyd
Copy link
Member

@cognifloyd cognifloyd commented Aug 4, 2021

This PR turns output_schema into a full jsonschema which provides several benefits.

  • validate action output even if it output is a boolean, integer, or other non-object type.
  • mask entire output of an action (mark all of an action's output as secret)
  • improve secret masking in action output for more object types

We ignore schemas that are not well-formed to avoid throwing errors. But, we now log a warning when a schema is invalid as an invalid schema prevents validation and secret masking from working.

If anyone is using output_schema, they will need to update metadata to keep using ouptut validation and secret masking.

For example, this schema:

    output_schema:
      property1:
        type: bool
      property2:
        type: str

should be updated like this:

    output_schema:
      type: object
      properties:
        property1:
          type: bool
        property2:
          type: str
      additionalProperties: false

This builds on #5309 to refactor the output_schema validation and add missing tests.

Ultimately, this is a follow-up for #5250 which added secret-masking to the output_schema validation, but missed some of the possible data types for output_values.

Before this PR, ouptut_schema could only do output_validation and secret masking for properties of objects.

The runner schema required that output_schema be an object and patternProperties here allowed each property to be a schema:

"output_schema": {
"description": "Schema for the runner's output.",
"type": "object",
"patternProperties": {r"^\w+$": util_schema.get_action_output_schema()},
"additionalProperties": False,
"default": {},
},

And the same applies to action schema:
"output_schema": {
"description": "Schema for the action's output.",
"type": "object",
"patternProperties": {r"^\w+$": util_schema.get_action_output_schema()},
"additionalProperties": False,
"default": {},
},

This is where the schema for each output_schema property schema is loaded:
def get_action_output_schema(additional_properties=True):
"""
Return a generic schema which is used for validating action output.
"""
return get_draft_schema(
version="action_output_schema", additional_properties=additional_properties
)

"action_output_schema": jsonify.load_file(
os.path.join(PATH, "action_output_schema.json")
),

And action_output_schema.json is the full json schema spec with our extensions (secret masking, etc)
"id": "http://json-schema.org/draft-04/schema#",
"$schema": "http://json-schema.org/draft-04/schema#",
"description": "Core schema meta-schema",

"required": {
"type": "boolean",
"default": false
},
"secret": {
"type": "boolean",
"default": false
},

Since output_schema used patternProperties, it could only validate objects - the only type that has properties. If your action or runner needed to output any other types (list, int, bool, ...), then output_validation and secret masking was unavailable. And, if your action provided an object whose keys needed to be masked (vs the property values), then that was also not possible.

@pull-request-size pull-request-size bot added the size/M PR that changes 30-99 lines. Good size to review. label Aug 4, 2021
@cognifloyd cognifloyd force-pushed the patch-7 branch 2 times, most recently from b0fc392 to 71e1a8c Compare August 4, 2021 18:10
Copy link
Contributor

@m4dcoder m4dcoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got some questions.

st2common/st2common/util/output_schema.py Outdated Show resolved Hide resolved
st2common/st2common/util/output_schema.py Outdated Show resolved Hide resolved
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.
@pull-request-size pull-request-size bot added size/L PR that changes 100-499 lines. Requires some effort to review. and removed size/M PR that changes 30-99 lines. Good size to review. labels Aug 6, 2021
@cognifloyd

This comment was marked as outdated.

@guzzijones
Copy link
Contributor

---
name: "array_to_file_newline"
runner_type: "python-script"
description: "convert json to string and dump to file"
enabled: true
entry_point: "array_to_file_newline.py"
parameters:
  json: 
    type: array 
    description: "listjson to convert to newline string"
    required: true
  output_file:
    type: string
    description: "file name to output"
    required: true
output_schema:
  output:
    status: bool 

Here is the offending yaml file. Yes it was malformed.

st2common/st2common/models/api/action.py Outdated Show resolved Hide resolved
@cognifloyd cognifloyd requested a review from a team July 10, 2022 00:28
@cognifloyd cognifloyd dismissed m4dcoder’s stale review July 18, 2022 17:37

I addressed the feedback and asked for permission to dismiss the review on slack. @m4dcoder responded with 🆗

@cognifloyd
Copy link
Member Author

Documentation added in StackStorm/st2docs#1137

@cognifloyd cognifloyd merged commit ecc7d71 into StackStorm:master Jul 18, 2022
cognifloyd added a commit to StackStorm/st2docs that referenced this pull request Jul 20, 2022
document output_schema and output_schema masking per StackStorm/st2#5319
@cognifloyd cognifloyd deleted the patch-7 branch February 12, 2024 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fix enhancement output schema size/XXL PR that changes 1000+ lines. You should absolutely split your PR into several.
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants