feat: Add HttpApi Event as new event type for AWS::Serverless::StateMachine #1925

Jacco · 2021-02-11T19:02:43Z

Issue #1851

Description of changes:

integration test
unit test(s)
added a full role to also support stop and startSync
added action property to event source (can be start, stop, startSync)
error added when you try to add stop to EXPRESS state machine
error added when you try to add startSync to STANDARD state machine
refactored adding auth to HttpApi from eventsources/push.py and stepfunctions/events.py
added TimeoutMillis support

Description of how you validated changes:

used the integration test to verify deployed httpapi
visually verified that all the integrations were added properly
exported openapi yaml and checked all is there
tested if the API responded correctly using Postman

Checklist:

Write/update tests
make pr passes
Update documentation
Verify transformed template deploys and application functions as expected

Examples?

See integration test that was added

TODO

test with various auth options
Error, Cause, Input, Name, TraceHeader parameters for action
discuss role naming/scope
discuss Action property
discuss possibility of cross region integration api (eu-west-1) -> statemachine (us-east-1)
timeOutMilles is missing
fix code coverage (get open_api.py) green
discuss output parameters

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

symmetrie: also added action parameter to swagger routine _construct_full_role in addition to _construct_role

Jacco · 2021-02-12T06:40:37Z

Discussion about role naming/scope:

Name

It used to be:

StateMachineName + EventName + 'Role'

I changed role naming to:

ApiName + StateMachineName + 'Role'

If one StateMachine has more than one events to an API the same role will be reused (it will be generated multiple times, so it is exported only once), instead of a new one for every event. Still with this naming a new Role will be created per API. So I propose a improvement:

I think it would even be better StateMachineName + 'StartStopRole'

Then only one role would be created. This role can then also be (re)used for other event sources than API.

Scope

I created a role that allows for all three actions for the StateMachine even if only one is used. Is that OK?

@jfuss

Jacco · 2021-02-12T06:52:37Z

Discussion about new "Action" property on EventSource:

So I added an "Action" property to the event source. There are three possible values:

start for IntegrationSubtype StepFunctions-StartExecution
stop for IntegrationSubtype StepFunctions-StopExecution
startSync for IntegrationSubtype StepFunctions-StartSyncExecution

stop is not allowed for EXPRESS StateMachines
startSync is not allowed for STANDARD StateMachines

Decision

IN SCOPE

Questions

If no "Action" property is specified start is assumed. Maybe it makes sense that for EXPRESS StateMachines the default would be startSync?
Are chosen names OK?
Should the action names be CAPS?

Jacco · 2021-02-12T21:31:14Z

Discussion extra parameters

Name - for ExecutionName in start & startSync (Name actually does not work for startSync, but console shows it also...)
TraceHeader - for startSync
Input - for start & startSync
Error - for stop
Cause - for stop

To support these parameter I added:

Parameters optional property that is a dict (no checks are performed, but the Parameters are passed requestParameters in the integration)

Example:

Resources:
  MyStateMachine:
    Type: AWS::Serverless::StateMachine
    Properties:
      Type: EXPRESS
      DefinitionUri: ${statemachineurl}
      Events:
        NormalPost:
          Type: HttpApi
          Properties:
            Path: /normal
            Method: post
            Parameters:
              Name: $request.body.Name
              Input: $request.body.Input
        SyncPost:
          Type: HttpApi
          Properties:
            Path: /sync
            Method: post
            Action: startSync
            Parameters:
              Name: $request.body.Name
              Input: $request.body.Input
              TraceHeader: $request.header.x-trace

Decision

IN SCOPE

Questions

Name of "Parameters"
Wether name and types are valildated by SAM or not (also we could require $request.(query|path|body|header).* for some)
Wether use per Action is vallidated by SAM or not

Jacco · 2021-02-13T07:13:41Z

Discussion HttpApi -> (any) StateMachine (cross region)

By default the API integration will assume the specified ARN is in the same region as the API is, but the integrations support calling a StateMachine in a different region.

If you specify the Region parameter (which cannot be specified from the request.header/body/path) the API will look for the specified ARN in the Region specified. This means the region in the ARN should match the Region specified otherwise com.amazonaws.swf.service.v2.model#StateMachineDoesNotExist is returned.

I think the way to support this with SAM is as follows:

Resources:
  MyStateMachine:
    Type: AWS::Serverless::StateMachineReference
    Properties:
      Name: MyStateMachine
      Region: us-east-1
      Account: 123454567 # is cross account supported??
      Events:
        SyncPost:
          Type: HttpApi
          Properties:
            Path: /sync
            Method: post
            Action: startSync    # static checking if resource is EXPRESS??

The integration would then use the StateMachine ARN it can contruct from the StateMachineReference properties and also specify the Region in the integration so the integration looks in the correct region for the StateMachine.

To be able to point request to different StateMachines:

Resources:
  MyStateMachine:
    Type: AWS::Serverless::StateMachineReference
    Properties:
      Events:
        SyncPost:
          Type: HttpApi
          Properties:
            Path: /sync
            Method: post
            StateMachine: $request.header.machine
            Action: startSync    # static checking if resource is EXPRESS??

Here MyStateMachine is not one particular StateMachine (no name/region/account). It will just create a integration on the API without a reference to a specific StateMachine. The Role created my reflect this.

To test this out I want to change the integration tests in the SAM project to support specifying the region in which to deploy.

Decision

OUT OF SCOPE Since AWS::Serverless::StateMachineReference is part of this issue

Error added for Action startSync with a STANDARD StateMachine

- moved to open api changed one ValueError to InvalidDocumentException

codecov-io · 2021-02-13T17:00:57Z

Codecov Report

Merging #1925 (42120fe) into develop (b8dbf3f) will decrease coverage by 0.12%.
The diff coverage is 82.90%.

@@             Coverage Diff             @@
##           develop    #1925      +/-   ##
===========================================
- Coverage    93.85%   93.72%   -0.13%     
===========================================
  Files           89       89              
  Lines         5887     5993     +106     
  Branches      1205     1230      +25     
===========================================
+ Hits          5525     5617      +92     
- Misses         166      174       +8     
- Partials       196      202       +6

Impacted Files	Coverage Δ
samtranslator/swagger/swagger.py	`93.09% <ø> (ø)`
samtranslator/open_api/open_api.py	`90.21% <79.66%> (-2.36%)`	⬇️
samtranslator/model/stepfunctions/events.py	`89.61% <84.31%> (-0.95%)`	⬇️
samtranslator/model/eventsources/push.py	`91.09% <100.00%> (+0.08%)`	⬆️
samtranslator/model/iam.py	`96.07% <100.00%> (+0.33%)`	⬆️
samtranslator/sdk/parameter.py	`100.00% <0.00%> (ø)`
samtranslator/translator/translator.py	`98.49% <0.00%> (+0.03%)`	⬆️
samtranslator/translator/arn_generator.py	`94.44% <0.00%> (+1.34%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b8dbf3f...42120fe. Read the comment docs.

mndeveci · 2022-09-14T20:03:26Z

Closing this due to inactivity. To reopen this please create an issue first to get it prioritized with the SAM team.

codecov-commenter · 2024-12-04T07:21:43Z

Codecov Report

Attention: Patch coverage is 91.96429% with 9 lines in your changes missing coverage. Please review.

Project coverage is 94.08%. Comparing base (b8dbf3f) to head (ce95291).
Report is 1173 commits behind head on develop.

Files with missing lines	Patch %	Lines
samtranslator/model/stepfunctions/events.py	84.31%	3 Missing and 5 partials ⚠️
samtranslator/open_api/open_api.py	98.14%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1925      +/-   ##
===========================================
+ Coverage    93.85%   94.08%   +0.23%     
===========================================
  Files           89       89              
  Lines         5887     6019     +132     
  Branches      1205     1228      +23     
===========================================
+ Hits          5525     5663     +138     
+ Misses         166      164       -2     
+ Partials       196      192       -4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Jacco Kulman added 4 commits February 11, 2021 19:23

intergrationtest statemachine triggered by httpapi

efc5e77

unit test for statemachine triggered by httpapi

fd1133a

added full role in addition to start only role

abae26e

added add_state_machine_integration to open_api

201d580

symmetrie: also added action parameter to swagger routine _construct_full_role in addition to _construct_role

Jacco marked this pull request as draft February 12, 2021 21:45

Jacco Kulman added 4 commits February 13, 2021 11:39

Parameters added to EventSource

c432cd2

Error added for Action startSync with a STANDARD StateMachine

refactor _add_auth_to_openapi_integration

fbc0dff

- moved to open api changed one ValueError to InvalidDocumentException

code coverage

73eba67

black

2d3a373

Jacco Kulman added 8 commits February 14, 2021 09:26

added auth test (coverage)

b87398c

lambda integration + statemachine now same

9fb933c

black

1da951b

timeOutMillis supported + extra unit tests

293934f

black

42120fe

Removed dead code

36cf47e

added tests for code coverage

4776495

more coverage

ce95291

CoshUS added the pr/external label Mar 22, 2021

aws-sam-cli-bot added the maintainer/need-response label Nov 23, 2021

moelasmar changed the title ~~Cannot set HttpApi Event Type on AWS::Serverless::StateMachine~~ fat: Add HttpApi Event as new event type for AWS::Serverless::StateMachine Sep 13, 2022

moelasmar changed the title ~~fat: Add HttpApi Event as new event type for AWS::Serverless::StateMachine~~ feat: Add HttpApi Event as new event type for AWS::Serverless::StateMachine Sep 13, 2022

mndeveci closed this Sep 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add HttpApi Event as new event type for AWS::Serverless::StateMachine #1925

feat: Add HttpApi Event as new event type for AWS::Serverless::StateMachine #1925

Jacco commented Feb 11, 2021 •

edited

Loading

Jacco commented Feb 12, 2021 •

edited

Loading

Jacco commented Feb 12, 2021 •

edited

Loading

Jacco commented Feb 12, 2021 •

edited

Loading

Jacco commented Feb 13, 2021 •

edited

Loading

codecov-io commented Feb 13, 2021 •

edited

Loading

mndeveci commented Sep 14, 2022

codecov-commenter commented Dec 4, 2024

feat: Add HttpApi Event as new event type for AWS::Serverless::StateMachine #1925

feat: Add HttpApi Event as new event type for AWS::Serverless::StateMachine #1925

Conversation

Jacco commented Feb 11, 2021 • edited Loading

Jacco commented Feb 12, 2021 • edited Loading

Discussion about role naming/scope:

Name

Scope

Jacco commented Feb 12, 2021 • edited Loading

Discussion about new "Action" property on EventSource:

Decision

Questions

Jacco commented Feb 12, 2021 • edited Loading

Discussion extra parameters

Decision

Questions

Jacco commented Feb 13, 2021 • edited Loading

Discussion HttpApi -> (any) StateMachine (cross region)

Decision

codecov-io commented Feb 13, 2021 • edited Loading

Codecov Report

mndeveci commented Sep 14, 2022

codecov-commenter commented Dec 4, 2024

Codecov Report

Jacco commented Feb 11, 2021 •

edited

Loading

Jacco commented Feb 12, 2021 •

edited

Loading

Jacco commented Feb 12, 2021 •

edited

Loading

Jacco commented Feb 12, 2021 •

edited

Loading

Jacco commented Feb 13, 2021 •

edited

Loading

codecov-io commented Feb 13, 2021 •

edited

Loading