Validate that the workflow payload is correct #136

agrare · 2023-10-27T18:31:28Z

part of: #130

Rules from ASL spec https://states-language.net/

Top-level fields

A State Machine MUST have an object field named "States", whose fields represent the states.
A State Machine MUST have a string field named "StartAt", whose value MUST exactly match one of names of the "States" fields. The interpreter starts running the the machine at the named state.

States

The state name, whose length MUST BE less than or equal to 80 Unicode characters, is the field name
State names MUST be unique within the scope of the whole state machine. (NOTE JSON.parse() only returns the last entry)
All states MUST have a "Type" field. This document refers to the values of this field as a state’s type, and to a state such as the one in the example above as a Task State.
Any state except for Choice, Succeed, and Fail MAY have a field named "End" whose value MUST be a boolean. The term "Terminal State" means a state with with { "End": true }, or a state with { "Type": "Succeed" }, or a state with { "Type": "Fail" }.

Transitions

All non-terminal states MUST have a "Next" field, except for the Choice State. The value of the "Next" field MUST exactly and case-sensitively match the name of another state.
Choice "Default" MUST match the name of another state.
Choice Choices MUST have a "Next" field that MUST match the name of another state.

Timestamps

These are strings which MUST conform to the RFC3339 profile of ISO 8601, with the further restrictions that an uppercase "T" character MUST be used to separate date and time, and an uppercase "Z" character MUST be present in the absence of a numeric time zone offset, for example "2016-03-14T01:59:00Z".

Data

The interpreter passes data between states to perform calculations or to dynamically control the state machine’s flow. All such data MUST be expressed in JSON.
As each state is executed, it receives a JSON text as input and can produce arbitrary output, which MUST be a JSON text.

Reference Paths

all Reference Paths MUST be unambiguous references to a single value, array, or object (subtree).

Payload Template

A Payload Template MUST be a JSON object; it has no required fields. (I believe this is covered already)
If the field value begins with only one "$", the value MUST be a Path.
If the field value begins with "$$", the first dollar sign is stripped and the remainder MUST be a Path
If the field value does not begin with "$", it MUST be an Intrinsic Function (see below)
A JSON object MUST NOT have duplicate field names after fields ending with the characters ".$" are renamed to strip the ".$" suffix. (I believe this is covered already)

Intrinsic Functions

An Intrinsic Function MUST be a string.
The Intrinsic Function MUST begin with an Intrinsic Function name.
An Intrinsic Function name MUST contain only the characters A through Z, a through z, 0 through 9, ".", and "_".
The Intrinsic Function name MUST be followed immediately by a list of zero or more arguments, enclosed by "(" and ")", and separated by commas.
Intrinsic Function arguments may be strings enclosed by apostrophe (') characters, numbers, null, Paths, or nested Intrinsic Functions.
The following characters are reserved for all Intrinsic Functions and MUST be escaped: ' { } \

There are more...

#130

Fryguy · 2023-10-27T19:53:40Z

lib/floe/workflow/states/pass.rb

@@ -13,6 +13,9 @@ def initialize(workflow, name, payload)
          @end         = !!payload["End"]
          @result      = payload["Result"]

+          raise Floe::InvalidWorkflowError, "Missing \"Next\" field in state [#{name}]" if @next.nil? && !@end


We might want a check whether End is actually defined since above we only do !!, meaning the key may not exist? Not sure if that actually matters or not.

That was intentional actually, End missing implicitly means the state is not a terminal state which means Next is required.

kbrock · 2023-10-30T23:43:01Z

Why is this WIP?
This is a beauty and needs to get merged stat ;)

Maybe we should move the criteria into the parent issue? and add check boxes?
Also noticed (in AWS specs) that some of the specs are across all states, and others are state specific. Maybe we can separate these 2 concepts so it is easier to follow?

agrare · 2023-10-31T13:18:28Z

Why is this WIP?
Maybe we should move the criteria into the parent issue? and add check boxes?

Yes I was seeing how far I could get through all of the rules but I think this is in a pretty good spot, I'll move the list to the parent issue and take this out of WIP

agrare · 2023-10-31T15:17:59Z

Okay I wanted to get the Next state checks for Choice done, have added that now

Fryguy · 2023-10-31T20:35:11Z

Are there any ordering issues here? That is, are the states validated as they are being processed, and if so, could a state defined after a reference to it cause issues? For example, I see the Choice state validates the "Default", but what if the Default state is defined after the Choice state. I feel like that is a common layout and would have been caught in specs, but the way I'm reading the code, it feels like it could still happen?

agrare · 2023-10-31T20:49:57Z

That's why we're validating the payload rather than the parsed states objects when checking for things like "does Default exist in States"

Fryguy · 2023-10-31T20:51:58Z

Oh I see, so for https://github.com/ManageIQ/floe/pull/136/files#diff-76feb017cc942fa02bf72a61cb066c70342a42039ace6610b861b9de0a5463a9R52, the payload of the higher order workflow is already parsed and validated at this point.

Thanks 👍

Fryguy · 2023-10-31T20:53:11Z

super-super-minor, but it makes me wonder if the method should be named validate_payload to make it clear we are validating against the raw payload.

agrare · 2023-11-01T12:45:14Z

but it makes me wonder if the method should be named validate_payload to make it clear we are validating against the raw payload.

Possibly? I was thinking there could be a number of things that we are validating and the raw payload is just one of them though

miq-bot · 2023-11-01T16:15:34Z

Checked commit agrare@8b0590d with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
14 files checked, 0 offenses detected
Everything looks fine. 👍

Added - Prefix pod names with 'floe-' (#132) - Validate that the workflow payload is correct (#136) Fixed - Fix issue where certain docker image names cannot be pod names (#134) - Fix uninitialized constant RSpec::Support::Differ in tests (#137) - Handle ImagePullErr/ImagePullBackOff as errors (#135) Changed - Add task spec helper (#123) - Rename State#run_wait to just #wait (#139) - Refactor the Podman runner to be a Docker subclass (#140)

agrare requested a review from Fryguy as a code owner October 27, 2023 18:31

Fryguy reviewed Oct 27, 2023

View reviewed changes

agrare force-pushed the workflow_payload_validation branch 4 times, most recently from e3b12c9 to 101276e Compare October 30, 2023 19:45

agrare mentioned this pull request Oct 30, 2023

[WIP] Abort if StartAt or Next [state] is not valid #125

Closed

agrare force-pushed the workflow_payload_validation branch 2 times, most recently from d790c24 to 83dc861 Compare October 30, 2023 20:38

agrare changed the title ~~[WIP] Validate that the workflow payload is correct~~ Validate that the workflow payload is correct Oct 31, 2023

agrare assigned kbrock Oct 31, 2023

agrare added the enhancement New feature or request label Oct 31, 2023

agrare force-pushed the workflow_payload_validation branch 2 times, most recently from bd48efe to a9decfa Compare October 31, 2023 15:16

agrare force-pushed the workflow_payload_validation branch 2 times, most recently from fbf0dc1 to 33d5ca5 Compare October 31, 2023 19:44

miq-bot added the unmergeable label Nov 1, 2023

agrare force-pushed the workflow_payload_validation branch 2 times, most recently from 8240c85 to 68d47ee Compare November 1, 2023 13:41

miq-bot removed the unmergeable label Nov 1, 2023

Validate that the workflow payload is correct

8b0590d

agrare force-pushed the workflow_payload_validation branch from 68d47ee to 8b0590d Compare November 1, 2023 16:11

kbrock merged commit 97b81d3 into ManageIQ:master Nov 1, 2023
5 checks passed

agrare deleted the workflow_payload_validation branch November 1, 2023 17:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate that the workflow payload is correct #136

Validate that the workflow payload is correct #136

agrare commented Oct 27, 2023 •

edited by kbrock

Loading

Fryguy Oct 27, 2023

agrare Oct 28, 2023

kbrock commented Oct 30, 2023 •

edited

Loading

agrare commented Oct 31, 2023

agrare commented Oct 31, 2023

Fryguy commented Oct 31, 2023

agrare commented Oct 31, 2023

Fryguy commented Oct 31, 2023

Fryguy commented Oct 31, 2023

agrare commented Nov 1, 2023

miq-bot commented Nov 1, 2023

Validate that the workflow payload is correct #136

Validate that the workflow payload is correct #136

Conversation

agrare commented Oct 27, 2023 • edited by kbrock Loading

Top-level fields

States

Transitions

Timestamps

Data

Reference Paths

Payload Template

Intrinsic Functions

Fryguy Oct 27, 2023

Choose a reason for hiding this comment

agrare Oct 28, 2023

Choose a reason for hiding this comment

kbrock commented Oct 30, 2023 • edited Loading

agrare commented Oct 31, 2023

agrare commented Oct 31, 2023

Fryguy commented Oct 31, 2023

agrare commented Oct 31, 2023

Fryguy commented Oct 31, 2023

Fryguy commented Oct 31, 2023

agrare commented Nov 1, 2023

miq-bot commented Nov 1, 2023

agrare commented Oct 27, 2023 •

edited by kbrock

Loading

kbrock commented Oct 30, 2023 •

edited

Loading