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

apigateway: Multiple StepFunctionsIntegrations with options lead to "Property MethodResponses contains duplicate values" #26586

Closed
boenhoff opened this issue Aug 1, 2023 · 5 comments · Fixed by #26636
Labels
@aws-cdk/aws-apigateway Related to Amazon API Gateway bug This issue is a bug. effort/medium Medium work item – several days of effort p1

Comments

@boenhoff
Copy link

boenhoff commented Aug 1, 2023

Describe the bug

Using the StepFunctionsIntegration.startExecution() integration with API Gateway with cors options (see reproduction steps) works, as long as only one method has the integration with options.

If a second method uses the same options you will get the error
The stack named TestStack failed to deploy: UPDATE_ROLLBACK_COMPLETE: Property MethodResponses contains duplicate values., Property MethodResponses contains duplicate values. and in the cdk.out it generates multiple ResponseModels.

Expected Behavior

Multiple methods with the StepFunctionsIntegration.startExecution() creates methods with individual options.

Current Behavior

❌ Deployment failed: Error: The stack named TestStack failed to deploy: UPDATE_ROLLBACK_COMPLETE: Property MethodResponses contains duplicate values., Property MethodResponses contains duplicate values.
    at FullCloudFormationDeployment.monitorDeployment (/opt/homebrew/Cellar/aws-cdk/2.89.0/libexec/lib/node_modules/aws-cdk/lib/index.js:426:10236)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async Object.deployStack2 [as deployStack] (/opt/homebrew/Cellar/aws-cdk/2.89.0/libexec/lib/node_modules/aws-cdk/lib/index.js:429:153307)
    at async /opt/homebrew/Cellar/aws-cdk/2.89.0/libexec/lib/node_modules/aws-cdk/lib/index.js:429:136985

The stack named TestStack failed to deploy: UPDATE_ROLLBACK_COMPLETE: Property MethodResponses contains duplicate values., Property MethodResponses contains duplicate values.

Reproduction Steps

  • create a new CDK project dk init app --language typescript
  • insert the following Code in the Stack
  • deploy with cdk deploy --require-approval never
        const methodOptions = {
            methodResponses: [
                {
                    statusCode: '200',
                    responseParameters: {
                        'method.response.header.Access-Control-Allow-Origin': true,
                    }
                },
            ]
        }

        const integrationOptions = {
            integrationResponses: [
                {
                    responseParameters: {
                        'method.response.header.Access-Control-Allow-Origin': "'*'",
                    },
                    statusCode: '200'
                }
            ]
        }
        const api = new RestApi(this, 'api');
        const machine = new StateMachine(this, 'machine', {
            stateMachineType: StateMachineType.EXPRESS,
            definitionBody: DefinitionBody.fromChainable(new Pass(this, 'Pass'))
        })
        // uncomment the following line and comment out the line after that to make it running - but without the cors options
        // api.root.addMethod('POST', StepFunctionsIntegration.startExecution(machine))
        api.root.addMethod('POST', StepFunctionsIntegration.startExecution(machine, integrationOptions), methodOptions)
        api.root.addMethod('GET', StepFunctionsIntegration.startExecution(machine, integrationOptions), methodOptions)

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.89.0 (build 2ad6683)

Framework Version

No response

Node.js Version

v20.5.0

OS

macOS 13.5

Language

Typescript

Language Version

TypeScript (5.1.6)

Other information

No response

@boenhoff boenhoff added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Aug 1, 2023
@github-actions github-actions bot added the @aws-cdk/aws-apigateway Related to Amazon API Gateway label Aug 1, 2023
@pahud
Copy link
Contributor

pahud commented Aug 2, 2023

Looks like it returns duplicate ResponseModels for

 "ResponseModels": {
       "application/json": "Empty"
      },
      "StatusCode": "200"
     },
 "MethodResponses": [
     {
      "ResponseParameters": {
       "method.response.header.Access-Control-Allow-Origin": true
      },
      "StatusCode": "200"
     },
     {
      "ResponseModels": {
       "application/json": "Empty"
      },
      "StatusCode": "200"
     },
     {
      "ResponseModels": {
       "application/json": "Error"
      },
      "StatusCode": "400"
     },
     {
      "ResponseModels": {
       "application/json": "Error"
      },
      "StatusCode": "500"
     },
     {
      "ResponseModels": {
       "application/json": "Empty"
      },
      "StatusCode": "200"
     },
     {
      "ResponseModels": {
       "application/json": "Error"
      },
      "StatusCode": "400"
     },
     {
      "ResponseModels": {
       "application/json": "Error"
      },
      "StatusCode": "500"
     }
    ],

@pahud pahud added p1 effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Aug 2, 2023
@pahud pahud changed the title (api-gateway): Multiple StepFunctionsIntegrations with options lead to "Property MethodResponses contains duplicate values" apigateway: Multiple StepFunctionsIntegrations with options lead to "Property MethodResponses contains duplicate values" Aug 2, 2023
@rix0rrr
Copy link
Contributor

rix0rrr commented Aug 11, 2023

I'm not even sure the user's original request can even work. I just looked at the default integrationResponses of the StepFunctionsIntegration, and it's very complicated, in a way you wouldn't want to have to write yourself:

{
      statusCode: '200',
      responseTemplates: {
        /* eslint-disable */
        'application/json': [
          '#set($inputRoot = $input.path(\'$\'))',
          '#if($input.path(\'$.status\').toString().equals("FAILED"))',
            '#set($context.responseOverride.status = 500)',
            '{',
              '"error": "$input.path(\'$.error\')",',
              '"cause": "$input.path(\'$.cause\')"',
            '}',
          '#else',
            '$input.path(\'$.output\')',
          '#end',
        /* eslint-enable */
        ].join('\n'),
      },
    },
    {
      /**
       * Specifies the regular expression (regex) pattern used to choose
       * an integration response based on the response from the back end.
       * In this case it will match all '4XX' HTTP Errors
       */
      selectionPattern: '4\\d{2}',
      statusCode: '400',
      responseTemplates: {
        'application/json': `{
            "error": "Bad request!"
          }`,
      },
    },
    {
      /**
       * Match all '5XX' HTTP Errors
       */
      selectionPattern: '5\\d{2}',
      statusCode: '500',
      responseTemplates: {
        'application/json': '"error": $input.path(\'$.error\')',
      },
    },

I'm pretty sure that as soon as you pass this:

        const integrationOptions = {
            integrationResponses: [
                { /* ... */ }
            ],
        };

        api.root.addMethod('POST', StepFunctionsIntegration.startExecution(machine, integrationOptions), methodOptions)

You completely override the built-in integrationOptions and you have to rebuild all that Velocity templating.

I have prepared 2 different PRs to take a stab at this:

I'm open to making this work in a nice way because it seems useful, but I would like some design input from someone with API Gateway experience before making any choices here.

rix0rrr added a commit to lpizzinidev/aws-cdk that referenced this issue Aug 17, 2023
@mergify mergify bot closed this as completed in #26636 Aug 17, 2023
mergify bot pushed a commit that referenced this issue Aug 17, 2023
…d between addMethod calls (#26636)

Adding a new method to an API with `addMethod` and passing `methodOptions.methodResponses` was generating duplicate entries using `StepFunctionsIntegration.startExecution` as integration:

For example:
```
const integ = apigw.StepFunctionsIntegration.startExecution(stateMachine, integrationOptions);
api.root.addMethod('GET', integ, methodOptions);
api.root.addMethod('POST', integ, methodOptions);
```

Would generate (fails on deployment):
```
 "MethodResponses": [
     {
      "ResponseParameters": {
       "method.response.header.Access-Control-Allow-Origin": true
      },
      "StatusCode": "200"
     },
     {
      "ResponseModels": {
       "application/json": "Empty"
      },
      "StatusCode": "200"
     },
     {
      "ResponseModels": {
       "application/json": "Error"
      },
      "StatusCode": "400"
     },
     {
      "ResponseModels": {
       "application/json": "Error"
      },
      "StatusCode": "500"
     },
     {
      "ResponseModels": {
       "application/json": "Empty"
      },
      "StatusCode": "200"
     },
     {
      "ResponseModels": {
       "application/json": "Error"
      },
      "StatusCode": "400"
     },
     {
      "ResponseModels": {
       "application/json": "Error"
      },
      "StatusCode": "500"
     }
    ],
```

With this fix, it will keep only the specified `methodResponses`.

Also, the `integrationResponses` option in `StepFunctionsIntegration.startExecution` was not used by the corresponding `Integration`.
This fix will allow to use the specified option value.


Closes #26586.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@redvex
Copy link

redvex commented Oct 12, 2023

@boenhoff @pahud I don't think this is included in the latest shipped version. I'm facing the same issue and I'm quite struggle. Could you help me to use a version where this fix is included? I tried to point npm to install the package from github, but npm hang indefinitely.

@redvex
Copy link

redvex commented Oct 12, 2023

@rix0rrr I'm facing the issue you addressed in your PR: https://github.com/aws/aws-cdk/pull/26720/files
Is it really not useful to anyone?
Without it the example in this issue doesn't work, for example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-apigateway Related to Amazon API Gateway bug This issue is a bug. effort/medium Medium work item – several days of effort p1
Projects
None yet
4 participants