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

corymhall · 2022-08-10T18:39:41Z

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.

Create a single RestApi
A new AWS::ApiGateway::Account and IAM role is created.
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.
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:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- 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

… (under feature flag) 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. closes #10878

gitpod-io · 2022-08-10T18:39:48Z

mrgrain · 2022-08-11T17:09:00Z

packages/@aws-cdk/aws-apigateway/README.md

+set to `false`. To change this default behavior you can enable the feature flag
+`@aws-cdk/aws-apigateway:disableCloudWatchRole`


Double checking - is this how we want to phrase feature flagged defaults? All new apps will have the flag enabled and it might confuse people. Maybe it'd be better to directly call out that the FF in the first sentence?!

Good point. Just pushed an update, let me know what you think or feel free to suggest different wording.

mrgrain · 2022-08-11T17:19:43Z

packages/@aws-cdk/aws-apigateway/test/integ.cloudwatch-disabled.ts

+export class Test extends cdk.Stack {
+  constructor(scope: cdk.App, id: string) {
+    super(scope, id);
+    const api = new apigateway.RestApi(this, 'my-api', {
+      retainDeployments: true,
+    });
+    api.root.addMethod('GET'); // must have at least one method or an API definition
+  }
+}
+
+const app = new cdk.App();
+new IntegTest(app, 'cloudwatch-logs-disabled', {
+  testCases: [
+    new Test(app, 'default-api'),
+  ],
+});


Is this not missing the FF in the context?

All features flags are enabled by default for integration tests.

TheRealAmazonKendra · 2022-08-11T19:51:49Z

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

TheRealAmazonKendra

Updating the tests certainly isn't mandatory for this change, so giving approval either way, but adding do-not-merge just in case you want to do that first. If not, we can remove the label and let this merge.

corymhall · 2022-08-12T12:58:32Z

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

I should have been the one to suggest this 🤣

mergify · 2022-08-12T13:43:07Z

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 · 2022-08-12T15:21:12Z

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

aws-cdk-automation · 2022-08-12T18:11:47Z

AWS CodeBuild CI Report

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

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

mergify · 2022-08-12T18:11:48Z

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

… (under feature flag) (aws#21546) 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 aws#10878 ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [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*

github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p1 labels Aug 10, 2022

mergify bot added the contribution/core This is a PR that came from AWS. label Aug 10, 2022

aws-cdk-automation requested a review from a team August 10, 2022 18:41

updating downstream integration tests

75a6b6a

mrgrain reviewed Aug 11, 2022

View reviewed changes

updating readme

9254740

TheRealAmazonKendra added the pr/do-not-merge This PR should not be merged at this time. label Aug 11, 2022

TheRealAmazonKendra approved these changes Aug 11, 2022

View reviewed changes

updating integ tests to use new IntegTest class

d62c2ec

corymhall removed the pr/do-not-merge This PR should not be merged at this time. label Aug 12, 2022

mergify bot and others added 2 commits August 12, 2022 13:43

Merge branch 'main' into corymhall/apigateway/disable-cloudwatchrole

48c3962

fixing changes introduced in rebase

e6aa347

Merge branch 'main' into corymhall/apigateway/disable-cloudwatchrole

746b9e6

mergify bot merged commit 78c858f into aws:main Aug 12, 2022

peterwoodworth mentioned this pull request Nov 18, 2022

aws_apigateway: cloudWatchRole update in unrelated APIs #22954

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

corymhall commented Aug 10, 2022 •

edited

Loading

gitpod-io bot commented Aug 10, 2022

mrgrain Aug 11, 2022

corymhall Aug 11, 2022

mrgrain Aug 11, 2022

corymhall Aug 11, 2022

TheRealAmazonKendra commented Aug 11, 2022

TheRealAmazonKendra left a comment

corymhall commented Aug 12, 2022

mergify bot commented Aug 12, 2022

mergify bot commented Aug 12, 2022

aws-cdk-automation commented Aug 12, 2022

mergify bot commented Aug 12, 2022

		set to `false`. To change this default behavior you can enable the feature flag
		`@aws-cdk/aws-apigateway:disableCloudWatchRole`

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

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

Conversation

corymhall commented Aug 10, 2022 • edited Loading

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Aug 10, 2022

mrgrain Aug 11, 2022

Choose a reason for hiding this comment

corymhall Aug 11, 2022

Choose a reason for hiding this comment

mrgrain Aug 11, 2022

Choose a reason for hiding this comment

corymhall Aug 11, 2022

Choose a reason for hiding this comment

TheRealAmazonKendra commented Aug 11, 2022

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

corymhall commented Aug 12, 2022

mergify bot commented Aug 12, 2022

mergify bot commented Aug 12, 2022

aws-cdk-automation commented Aug 12, 2022

AWS CodeBuild CI Report

mergify bot commented Aug 12, 2022

corymhall commented Aug 10, 2022 •

edited

Loading