aws-cdk icon indicating copy to clipboard operation
aws-cdk copied to clipboard

fix(aws-apigateway): CloudWatch logging should be disabled by default (under feature flag)

Open corymhall opened this issue 2 years ago • 2 comments

Currently when you create a RestApi cloudwatch logging is enabled by default. This will create an IAM role and a AWS::ApiGateway::Account resource, which is what is used to allow API Gateway to write API logs to CloudWatch logs. There can only be a single API Gateway account per AWS environment (account/region), but CloudFormation will not throw an error if you try to create additional accounts. Instead it will update the existing account with the new configuration.

This can cause issues if customers create more than 1 RestApi. The following scenario is an example.

  1. Create a single RestApi A new AWS::ApiGateway::Account and IAM role is created.
  2. Create a second RestApi Another AWS::ApiGateway::Account/IAM role is created which overwrites the first one. The first RestApi now uses the account/role created by this RestApi.
  3. Delete the second RestApi The AWS::ApiGateway::Account/IAM role is deleted along with the second RestApi. The first RestApi no longer has access to write to CloudWatch logs.

Because of this behavior, the correct thing to do is to disable CloudWatch logs by default so that the user has to create the global resource separately. This new behavior is behind a feature flag @aws-cdk/aws-apigateway:disableCloudWatchLogs.

In addition, the default retention policy for both the API Gateway account and IAM role has been set to RETAIN so that existing implementations that do not use the feature flag can avoid the above scenario. The resources will be unmanaged, but existing RestApis will not break.

I've updated all the existing integration tests to use the old behavior by explicitly setting cloudWatchLogs: true. I then added a new integration test for the new behavior.

closes #10878


All Submissions:

Adding new Unconventional Dependencies:

  • [ ] This PR adds new unconventional dependencies following the process described here

New Features

  • [x] Have you added the new feature to an integration test?
    • [x] Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

corymhall avatar Aug 10 '22 18:08 corymhall

gitpod-io[bot] avatar Aug 10 '22 18:08 gitpod-io[bot]

Might this be a good opportunity to update the integration tests to the new style?

TheRealAmazonKendra avatar Aug 11 '22 19:08 TheRealAmazonKendra

Might this be a good opportunity to update the integration tests to the new style?

I should have been the one to suggest this 🤣

corymhall avatar Aug 12 '22 12:08 corymhall

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[bot] avatar Aug 12 '22 13:08 mergify[bot]

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[bot] avatar Aug 12 '22 15:08 mergify[bot]

AWS CodeBuild CI Report

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

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

aws-cdk-automation avatar Aug 12 '22 18:08 aws-cdk-automation

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[bot] avatar Aug 12 '22 18:08 mergify[bot]