fix(apigatewayv2): incorrect arn function causing unwanted behavior

[IkeNefcy]

fixes: #33218

Reason for this change

In websocket APIs in aws-apigatewayv2, the function arnForExecuteApi has essentially the same exact functionality as a REST API, which is not appropriate for Websockets which are fundamentally different.

The way I found this issue was I used arnForExecuteApi to put the arn of the api into an IAM Role. The reason for this was because I was trying to use an IAM authorizer, which from a React standpoint meant signing iam credentials from my Cognito id pool using Amplify lib. When doing this I used arnForExecuteApi from CDK to write the policy, I did not include any arguments, just the default.

The issue was that this was not working. I spent time diving deep on the issue in case it was the method in which I was signing the credentials, since I was not too familiar with this process. I also got the assistance of a Cloud Support Engineer from AWS to try and identify the problem.

Shout-out Mike Sacks.

The problem ended up being that that the resource policy was not correct. The policy that was generated by the function arnForExecuteApi was

"Resource": "arn:aws:execute-api:us-east-1:acc:apiId/*/*/*",

This is because the function itself has 3 values, stage, method and path, so when all are left in default states, this indicates all or *. So when adding each value at default you get /*/*/*, 3 x /*.

This is an issue because Websocket arns are not structured like this, and as it turns out iam prevents access if you have too many wild cards than applicable. This means the reason for getting access denied was not because of my signed url, but because having 1 extra /* means that you no longer have permissions.

Websocket arns are structured like this

arn:aws:execute-api:us-east-1:acc:apiId/*/$connect

In this example, * is the stage (this is what it shows on the console) and $connect is the route.
You can add as many routes as you want, but the main ones by default are $connect, $disconnect and $default for no matching route. So if I want to grant an IAM role to have access to all routes and all stages, I would use this:

"Resource": "arn:aws:execute-api:us-east-1:acc:apiId/*/*",

Note 2 x /* instead of 3.
Simply changing this by hand (deleting 2 characters) was enough to get the websocket to begin connecting via my signed url.

Description of changes

A re-write of the function for creating the arn. This is implemented as arnForExecuteApiV2, the original function has been changes to include the deprecated tag. This is to avoid making a breaking change since the new function only has 2 args and the original had 3.

  /**
   * Get the "execute-api" ARN.
   *
   * @default - The default behavior applies when no specific route, or stage is provided.
   * In this case, the ARN will cover all routes, and all stages of this API.
   * Specifically, if 'route' is not specified, it defaults to '*', representing all routes.
   * If 'stage' is not specified, it also defaults to '*', representing all stages.
   */
  public arnForExecuteApiV2(route?: string, stage?: string): string {
    return Stack.of(this).formatArn({
      service: 'execute-api',
      resource: this.apiId,
      arnFormat: ArnFormat.SLASH_RESOURCE_NAME,
      resourceName: `${stage ?? '*'}/${route ?? '*'}`,
    });
  }

I removed "Method" and "Path" entirely since these are not even appropriate to use as terms for websockets. I used Route instead.

Description of how you validated changes

Updated Tests, there were 4 tests before:

• get arnForExecuteApi
  • is now using route, and intentionally uses a route with no $ to check that the $ is being added correctly.
• get arnForExecuteApi with default values
  • is now using route
• get arnForExecuteApi with ANY method (removed)
  • doesn't make any sense here because "ANY" is not a term used with websockets, and method does not exist. Thus, removed this test
• throws when call arnForExecuteApi method with specifing a string that does not start with / for the path argument. (removed)
  • This test is checking for a specific format for path, which is not needed since the route can be anything. Also path does not exist.

This leaves 2 total tests now.

Added a new integ function, integ.api-grant-invoke.ts and used --update-on-failed with my personal account to bootstrap new snapshots to match. For this test I included an iam role and 2 arns, one with default settings and one with
.arnForExecuteApi('connect', 'prod')
Intentionally left off the $ to check that it's being added.

All tests and integ are passing.

Checklist

• My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

---

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

[aws-cdk-automation]

The pull request linter fails with the following errors:

❌ The title of the pull request should omit 'aws-' from the name of modified packages. Use 'apigatewayv2' instead of 'aws-apigatewayv2'.

If you believe this pull request should receive an exemption, please comment and provide a justification. A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed, add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

[IkeNefcy]

err - METHOD aws-cdk-lib.aws_apigatewayv2.WebSocketApi.arnForExecuteApi: argument stage, not accepted anymore. [removed-argument:aws-cdk-lib.aws_apigatewayv2.WebSocketApi.arnForExecuteApi]
The build is marking this as a breaking change since the arguments of the function are changing. However this function wasn't working at all for Websockets before this, so it's unclear.
I'll leave it for approver to give advice on what to do in this case.

For clarity, "argument stage" mentioned is not the specific issue, stage in the old method was the 3rd argument, therefore this error should be interpreted as, breaking change due to argument count change.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.84%. Comparing base (d32baf6) to head (e768a02).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #33100   +/-   ##
=======================================
  Coverage   80.84%   80.84%           
=======================================
  Files         232      232           
  Lines       14135    14135           
  Branches     2460     2460           
=======================================
  Hits        11428    11428           
  Misses       2427     2427           
  Partials      280      280           

Flag	Coverage Δ
suite.unit	80.84% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	79.64% <ø> (ø)
packages/aws-cdk-lib/core	82.17% <ø> (ø)

[IkeNefcy]

Summarizing status: all checks have passed except the build.
Check the build logs, all tests and integ are passing for items related to apigv2.
The build fails during compatibility check because I changed the number of arguments in this function.
To be perfectly clear, this is a bug (no bug tag because no issue opened) and that function was not made correctly to begin with, in fact it was likely copy pasted from apigv1.
So yes in these terms it is a breaking change, but I'm convinced we should push it anyways because the functionality before would require a user to call the function then trim off the last 2 characters of the string for it to work.

[gracelu0]

Thanks for your PR @IkeNefcy ! I do suggest we introduce a new function instead of changing the existing one to avoid breaking existing projects (even though it is incorrect).

Pull request has been modified.

[IkeNefcy]

Just from curiosity, what is "collect" in the pending workflows? It wasn't there at first and it does not look like one of the package's workflows.

[gracelu0]

Just from curiosity, what is "collect" in the pending workflows? It wasn't there at first and it does not look like one of the package's workflows.

I believe it's one of the Codecov workflows to check testing coverage

[gracelu0]

Just a few more nits to address and then I'm happy to approve.

Pull request has been modified.

[aws-cdk-automation]

AWS CodeBuild CI Report

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

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

[gracelu0]

Thank you!

[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).

[github-actions]

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.