Cannot set HttpApi Event Type on AWS::Serverless::StateMachine

[Jacco]

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:

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

Examples?

See integration test that was added

TODO

• [x] test with various auth options
• [x] Error, Cause, Input, Name, TraceHeader parameters for action
• [x] discuss role naming/scope
• [x] discuss Action property
• [x] discuss possibility of cross region integration api (eu-west-1) -> statemachine (us-east-1)
• [x] timeOutMilles is missing
• [x] 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.

[Jacco]

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]

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]

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]

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

[codecov-io]

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%)	:arrow_down:
samtranslator/model/stepfunctions/events.py	89.61% <84.31%> (-0.95%)	:arrow_down:
samtranslator/model/eventsources/push.py	91.09% <100.00%> (+0.08%)	:arrow_up:
samtranslator/model/iam.py	96.07% <100.00%> (+0.33%)	:arrow_up:
samtranslator/sdk/parameter.py	100.00% <0.00%> (ø)
samtranslator/translator/translator.py	98.49% <0.00%> (+0.03%)	:arrow_up:
samtranslator/translator/arn_generator.py	94.44% <0.00%> (+1.34%)	:arrow_up:
... 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]

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