fix(apigateway): duplicate methodResponses if the same array is reused between addMethod calls

[lpizzinidev]

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

[rix0rrr]

What if methodOptions.methodResponses is different between the two calls? Won't ignoring it the second time do something unexpected?

Wouldn't it be better to disallow the second call, and force you to add both methods in a single call?

[lpizzinidev]

@rix0rrr
What if instead of falling back to undefined we default to METHOD_RESPONSES in renderMethodResponses?
This would centralize the handling of the variable in the Method class and remove the need for populating the array on binding (which to me looks like the source of the problem).
And this should still work.

[rix0rrr]

I didn't understand why this would be happening because addMethod('GET', ...) and addMethod('POST', ...) should be adding 2 different Methods, each with their own MethodResponses.

So I added more validation and as far as I can tell, we also have duplicate method responses even with a single integration?

I have pushed my changes to this branch so you can see. Am I misunderstanding the API Gateway contract, or something else screwy going on?

[rix0rrr]

For example, I'm now seeing this error in a bunch of places:

[aws-cdk-aws-apigateway-stepfunctions-startexecution/my-rest-api/Default/GET] 3 methodResponses for code 200:
{"statusCode":"200","responseParameters":{"method.response.header.Access-Control-Allow-Origin":true}},
{"statusCode":"200","responseModels":{"application/json":{"modelId":"Empty"}}},
{"statusCode":"200","responseModels":{"application/json":{"modelId":"Empty"}}}

Is this expected?

[rix0rrr]

Uh oh. It seems like the CloudFormation should have been rejecting this:

[
{"statusCode":"200","responseParameters":{"method.response.header.Access-Control-Allow-Origin":true}},
{"statusCode":"200","responseModels":{"application/json":{"modelId":"Empty"}}},
]

But didn't. But it does throw an error on duplicate entries in the array, because it's trying to convert them to a set and throws an error if that conversion is lossy.

[rix0rrr]

After looking into it a bit more, something entirely different is happening.

In the code example, the methodOptions argument is reused for both StartExecutions. Then inside the Method constructor, we assign the array from the options to become the private member:

aws-cdk/packages/aws-cdk-lib/aws-apigateway/lib/method.ts

Line 206 in e593229

this.methodResponses = options.methodResponses ?? defaultMethodOptions.methodResponses ?? [];

Since arrays are reference values, this leads to object sharing! Both the GET and POST method refer to the exact same array, and so the integration being executed twice adds the same elements to the same array, twice.

Had the code been written like this:

api.root.addMethod('POST', integration, {
  methodResponses: [ ... ],
});
api.root.addMethod('GET', integration, {
  methodResponses: [ ... ],
});

Both methods would have used a different methodResponses array, and the duplicate adding of identical elements would not have occurred.

---

That's the immediate problem. But there is another problem, that I alluded to before. If there are multiple methodResponses for the same statusCode, CloudFormation will apply them one by one and you'll end up with one of them, arbitrarily. CloudFormation only detects problems when the elements in the array are exactly the same, which happened to happen here.

So already doing the following:

api.root.addMethod('Get', StepFunctionsIntegration.startExecution(machine, integrationOptions), {
  methodResponses: [
    statusCode: '200',
    /* anything here */
  ]
});

Becomes a problem, because:

• You are adding a methodResponse with statusCode = 200
• The integration is also adding a methodResponse with statusCode = 200

You now have multiple methodResponses with the same status code, and after deployment your API will end up with one of them, but not both.

So the original use case of passing methodResponses in the method to add a header to all responses isn't even supported.

[lpizzinidev]

@rix0rrr

There are 2 problems here:

1. The addMethod function passes the same reference of options to successive calls if declared as a variable.
   For example, considering this scenario:

    const methodOptions = {
      methodResponses: [
        {
          statusCode: '200',
          responseParameters: {
            'method.response.header.Access-Control-Allow-Origin': true,
          },
        },
      ],
    };

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

The methodResponses of both Methods will reference the same array, causing the duplicates that you mentioned here.

2. This doesn't take into consideration methods passed via properties, basically overriding in any case, causing the duplicates that you mentioned here

I pushed a tentative solution that should solve both issues.
Let me know if further changes are needed and if this validation should be kept (I will add unit tests if so).

PS
I saw this after pushing the last changes and writing the comment.😄

[rix0rrr]

I'm going to split this into 2 PRs:

• One for the false sharing
• One for the response merging (only applies to the stepfunctions integration, because that's the only integration that calls addMethodResponse).

After lunch 😉

Pull request has been modified.

Pull request has been modified.

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Pull request has been modified.

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: d18700f
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).