feat(apigatewayv2): `HttpStage` access logging by phuhung273 · Pull Request #33977 · aws/aws-cdk

phuhung273 · 2025-03-30T10:54:19Z

Issue # (if applicable)

Closes #11100

Description of changes

apigwv2 HttpStage support access logging to CloudWatch Logs https://docs.aws.amazon.com/apigateway/latest/developerguide/http-api-logging.html
Same logic as V1 without Firehose as not supported.

Description of how you validated changes

Unit + Integ

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

paulhcsun · 2025-04-04T18:56:24Z

packages/aws-cdk-lib/aws-apigatewayv2/lib/http/stage.ts

+    } else {
+      if (accessLogFormat !== undefined &&
+        !Token.isUnresolved(accessLogFormat.toString()) &&
+        !/.*\$context.(requestId|extendedRequestId)\b.*/.test(accessLogFormat.toString())) {


Could we modify this check to check that the accessLofFormat.toString() includes either contextRequestId or contextExtendedRequestId instead of using a regex? Something like this:

if ( accessLogFormat !== undefined && !Token.isUnresolved(accessLogFormat.toString()) && !accessLogFormat.toString().includes('$context.requestId') && !accessLogFormat.toString().includes('$context.extendedRequestId') ) {

Intention is to prevent user from mistakenly using something like $context.requestIdxxx. In such case, .includes('$context.requestId') passes, but later fail on runtime.

I personally think we are fine with this. Let me know your point.

Oh I see, thanks for clarifying. Then maybe we can do a stricter exact match like so:

const allowedValues = new Set([ '$context.requestId', '$context.extendedRequestId' ]); if ( accessLogFormat !== undefined && !Token.isUnresolved(accessLogFormat.toString()) && !allowedValues.has(accessLogFormat.toString()) ) { throw new ValidationError(...); }

I agree the current change would probably be fine but better to satisfy the CodeQL checks where possible. What do you think?

Thanks for the recommendation. I updated to use your first solution.

The first solution would accept an input like $context.requestIdxxx like you mentioned but that's not what we want. I think we should use the second option which uses stricter exact string matching.

format contains multiple parameters, eg: $context.responseLength $context.requestId. So 2nd solution .has doesn't work as it check for the exact string. Anw, i've updated the regex to pass CodeQL. Please let me know what you think.

Good point, the current solution lgtm!

codecov · 2025-03-30T11:03:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.98%. Comparing base (a67c3f5) to head (d67296c).
Report is 69 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #33977      +/-   ##
==========================================
+ Coverage   82.39%   83.98%   +1.58%     
==========================================
  Files         120      120              
  Lines        6960     6976      +16     
  Branches     1175     1178       +3     
==========================================
+ Hits         5735     5859     +124     
+ Misses       1120     1005     -115     
- Partials      105      112       +7

Flag	Coverage Δ
suite.unit	`83.98% <ø> (+1.58%)`	⬆️

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

Components	Coverage Δ
packages/aws-cdk	`∅ <ø> (∅)`
packages/aws-cdk-lib/core	`83.98% <ø> (+1.58%)`	⬆️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

paulhcsun

Hi @phuhung273, thank you for this contribution for this very highly community-requested feature! I've just left a few comments for some changes.

paulhcsun · 2025-04-04T18:56:24Z

packages/aws-cdk-lib/aws-apigatewayv2/lib/http/stage.ts

+    } else {
+      if (accessLogFormat !== undefined &&
+        !Token.isUnresolved(accessLogFormat.toString()) &&
+        !/.*\$context.(requestId|extendedRequestId)\b.*/.test(accessLogFormat.toString())) {


Could we modify this check to check that the accessLofFormat.toString() includes either contextRequestId or contextExtendedRequestId instead of using a regex? Something like this:

if ( accessLogFormat !== undefined && !Token.isUnresolved(accessLogFormat.toString()) && !accessLogFormat.toString().includes('$context.requestId') && !accessLogFormat.toString().includes('$context.extendedRequestId') ) {

packages/aws-cdk-lib/aws-apigatewayv2/lib/http/stage.ts

paulhcsun · 2025-04-04T20:40:27Z

packages/aws-cdk-lib/aws-apigatewayv2/lib/http/stage.ts

+  /**
+   * The CloudWatch Logs log group or Firehose delivery stream where to write access logs.
+   *
+   * @default - No destination
+   */
+  readonly accessLogDestination?: IAccessLogDestination;
+
+  /**
+   * A single line format of access logs of data, as specified by selected $context variables.
+   * The format must include either `AccessLogFormat.contextRequestId()`
+   * or `AccessLogFormat.contextExtendedRequestId()`.
+   *
+   * @see https://docs.aws.amazon.com/apigateway/latest/developerguide/http-api-logging-variables.html
+   *
+   * @default - Common Log Format
+   */
+  readonly accessLogFormat?: AccessLogFormat;


Since the accessLogDestination property must be conditionally set if accessLogFormat is set, let's combine these two properties into an interface so that they must be specified together. Something like

interface IAccessLogOptions { // or IAccessLogSettings readonly accessLogDestnation: IAccessLogDestination; readonly accessLogFormat: AccessLogFormat; }

This way we enforce that they must be set together and then we won't need the validation in the HttpStage constructor.

Note: I see that the old aws-apigateway's implementation for accessLogSettings has these two properties as separate properties, but I think we should still combine the properties even though it would differ from the existing convention. Wherever possible, we would like to enforce conditional properties with the API design rather than runtime validation.

Thanks for your advice. Agree that we should combine.

Another point, what do you think about making accessLogFormat optional with default value AccessLogFormat.clf(). IMO, most user just want to put the destination.

Ya I think that's a good idea. Is it a valid use case to specify the destination without a format? If so then I think this change is fine as it is now.

format is always required according to CloudFormation. But I like the logic of apigwv1 to make it optional by providing default value if not specified, more user-friendly I think. So I kept it.

I see, I think that is fine then if we have the same behaviour in apigv1. Thanks!

Pull request has been modified.

packages/aws-cdk-lib/aws-apigatewayv2/lib/common/base.ts

phuhung273 · 2025-04-05T06:43:55Z

Seeing community demand for the same setting in WebSocketApi, eg: #21935 and comments from original issue. I moved it to StageBase to facilitate adding it to WebSocket later.

I did try to include WebSocket also, but ran into this error CloudWatch Logs role ARN must be set in account settings to enable logging. Found out that we need something like this cloudWatchRole in V1 for convenience

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

Lines 162 to 167 in 74cbe27

    
             /** 
        
              * Automatically configure an AWS CloudWatch role for API Gateway. 
        
              * 
        
              * @default - false if `@aws-cdk/aws-apigateway:disableCloudWatchRole` is enabled, true otherwise 
        
              */ 
        
             readonly cloudWatchRole?: boolean;

Therefore, decided to only change StageBase and support HttpApi for the scope of this PR. Will create another for WebSocket.

packages/aws-cdk-lib/aws-apigatewayv2/lib/common/base.ts

paulhcsun · 2025-04-09T00:13:53Z

Hi @phuhung273, thanks for your quick response to the requested changes. I've just left two more comments:

to remove the check that is no longer needed
to update the condition for the accessLogFormat

Happy to approve after those changes are made!

packages/aws-cdk-lib/aws-apigatewayv2/lib/common/base.ts

paulhcsun

Great work! Thanks for your contribution @phuhung273!

paulhcsun · 2025-04-10T18:53:30Z

@Mergifyio update

mergify · 2025-04-10T18:53:37Z

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 · 2025-04-10T18:53:45Z

update

☑️ Nothing to do

Details

queue-position = -1 [📌 update requirement]
#commits-behind > 0 [📌 update requirement]
-closed [📌 update requirement]
-conflict [📌 update requirement]

aws-cdk-automation · 2025-04-10T19:24:28Z

AWS CodeBuild CI Report

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

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

mergify · 2025-04-10T19:24:36Z

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 · 2025-04-10T19:24:46Z

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.

feat(apigatewayv2): access logging

9d55aad

github-actions bot added admired-contributor [Pilot] contributed between 13-24 PRs to the CDK effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1 labels Mar 30, 2025

aws-cdk-automation requested a review from a team March 30, 2025 10:54

github-advanced-security bot found potential problems Mar 30, 2025

View reviewed changes

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Mar 30, 2025

github-actions bot mentioned this pull request Apr 1, 2025

Monthly PR metrics report #33994

Closed

shikha372 assigned paulhcsun Apr 3, 2025

paulhcsun previously requested changes Apr 4, 2025

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Apr 4, 2025

combine into IAccessLogSettings , move to StageBase

a2e604d

github-advanced-security bot found potential problems Apr 5, 2025

View reviewed changes

packages/aws-cdk-lib/aws-apigatewayv2/lib/common/base.ts Fixed Show fixed Hide fixed

README

687227e

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Apr 5, 2025

phuhung273 requested a review from paulhcsun April 5, 2025 07:45

paulhcsun reviewed Apr 8, 2025

View reviewed changes

packages/aws-cdk-lib/aws-apigatewayv2/lib/common/base.ts Outdated Show resolved Hide resolved

phuhung273 requested a review from paulhcsun April 9, 2025 13:34

phuhung273 force-pushed the apigwv2-accesslog branch from 8877d58 to 53bc7f5 Compare April 10, 2025 00:36

github-advanced-security bot found potential problems Apr 10, 2025

View reviewed changes

packages/aws-cdk-lib/aws-apigatewayv2/lib/common/base.ts Fixed Show fixed Hide fixed

improve regex for codeql

d67296c

phuhung273 force-pushed the apigwv2-accesslog branch from 53bc7f5 to d67296c Compare April 10, 2025 00:49

paulhcsun approved these changes Apr 10, 2025

View reviewed changes

Merge branch 'main' into apigwv2-accesslog

00d25a3

mergify bot merged commit d04e40f into aws:main Apr 10, 2025
10 checks passed

github-actions bot locked as resolved and limited conversation to collaborators Apr 10, 2025

phuhung273 deleted the apigwv2-accesslog branch April 11, 2025 01:11

Conversation

phuhung273 commented Mar 30, 2025

Issue # (if applicable)

Description of changes

Description of how you validated changes

Checklist

Uh oh!

Check failure

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

paulhcsun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

phuhung273 commented Apr 5, 2025

Uh oh!

Uh oh!

paulhcsun commented Apr 9, 2025

Uh oh!

Uh oh!

paulhcsun left a comment

Choose a reason for hiding this comment

Uh oh!

paulhcsun commented Apr 10, 2025

Uh oh!

mergify bot commented Apr 10, 2025

Uh oh!

mergify bot commented Apr 10, 2025

☑️ Nothing to do

Uh oh!

aws-cdk-automation commented Apr 10, 2025

AWS CodeBuild CI Report

Uh oh!

Uh oh!

mergify bot commented Apr 10, 2025

Uh oh!

github-actions bot commented Apr 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Mar 30, 2025 •

edited

Loading