Skip to content

Commit

Permalink
fix(aws-apigateway): CloudWatch logging should be disabled by default…
Browse files Browse the repository at this point in the history
… (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*
  • Loading branch information
corymhall authored Aug 12, 2022
1 parent 16c0c98 commit 78c858f
Show file tree
Hide file tree
Showing 161 changed files with 1,772 additions and 333 deletions.
17 changes: 14 additions & 3 deletions packages/@aws-cdk/aws-apigateway/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -842,19 +842,30 @@ API.
The following example will configure API Gateway to emit logs and data traces to
AWS CloudWatch for all API calls:

> By default, an IAM role will be created and associated with API Gateway to
allow it to write logs and metrics to AWS CloudWatch unless `cloudWatchRole` is
set to `false`.
> Note: whether or not this is enabled or disabled by default is controlled by the
`@aws-cdk/aws-apigateway:disableCloudWatchRole` feature flag. When this feature flag
is set to `false` the default behavior will set `cloudWatchRole=true`

This is controlled via the `@aws-cdk/aws-apigateway:disableCloudWatchRole` feature flag and
is disabled by default. When enabled (or `@aws-cdk/aws-apigateway:disableCloudWatchRole=false`),
an IAM role will be created and associated with API Gateway to allow it to write logs and metrics to AWS CloudWatch.

```ts
const api = new apigateway.RestApi(this, 'books', {
cloudWatchRole: true,
deployOptions: {
loggingLevel: apigateway.MethodLoggingLevel.INFO,
dataTraceEnabled: true
}
})
```

> Note: there can only be a single apigateway.CfnAccount per AWS environment
so if you create multiple `RestApi`s with `cloudWatchRole=true` each new `RestApi`
will overwrite the `CfnAccount`. It is recommended to set `cloudWatchRole=false`
(the default behavior if `@aws-cdk/aws-apigateway:disableCloudWatchRole` is enabled)
and only create a single CloudWatch role and account per environment.

### Deep dive: Invalidation of deployments

API Gateway deployments are an immutable snapshot of the API. This means that we
Expand Down
13 changes: 9 additions & 4 deletions packages/@aws-cdk/aws-apigateway/lib/restapi.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import * as cloudwatch from '@aws-cdk/aws-cloudwatch';
import { IVpcEndpoint } from '@aws-cdk/aws-ec2';
import * as iam from '@aws-cdk/aws-iam';
import { ArnFormat, CfnOutput, IResource as IResourceBase, Resource, Stack, Token } from '@aws-cdk/core';
import { ArnFormat, CfnOutput, IResource as IResourceBase, Resource, Stack, Token, FeatureFlags, RemovalPolicy } from '@aws-cdk/core';
import { APIGATEWAY_DISABLE_CLOUDWATCH_ROLE } from '@aws-cdk/cx-api';
import { Construct } from 'constructs';
import { ApiDefinition } from './api-definition';
import { ApiKey, ApiKeyOptions, IApiKey } from './api-key';
Expand Down Expand Up @@ -158,7 +159,7 @@ export interface RestApiBaseProps {
/**
* Automatically configure an AWS CloudWatch role for API Gateway.
*
* @default true
* @default - false if `@aws-cdk/aws-apigateway:disableCloudWatchRole` is enabled, true otherwise
*/
readonly cloudWatchRole?: boolean;

Expand Down Expand Up @@ -531,10 +532,12 @@ export abstract class RestApiBase extends Resource implements IRestApi {
assumedBy: new iam.ServicePrincipal('apigateway.amazonaws.com'),
managedPolicies: [iam.ManagedPolicy.fromAwsManagedPolicyName('service-role/AmazonAPIGatewayPushToCloudWatchLogs')],
});
role.applyRemovalPolicy(RemovalPolicy.RETAIN);

this.cloudWatchAccount = new CfnAccount(this, 'Account', {
cloudWatchRoleArn: role.roleArn,
});
this.cloudWatchAccount.applyRemovalPolicy(RemovalPolicy.RETAIN);

this.cloudWatchAccount.node.addDependency(apiResource);
}
Expand Down Expand Up @@ -663,7 +666,8 @@ export class SpecRestApi extends RestApiBase {
this.addDomainName('CustomDomain', props.domainName);
}

const cloudWatchRole = props.cloudWatchRole ?? true;
const cloudWatchRoleDefault = FeatureFlags.of(this).isEnabled(APIGATEWAY_DISABLE_CLOUDWATCH_ROLE) ? false : true;
const cloudWatchRole = props.cloudWatchRole ?? cloudWatchRoleDefault;
if (cloudWatchRole) {
this._configureCloudWatchRole(resource);
}
Expand Down Expand Up @@ -769,7 +773,8 @@ export class RestApi extends RestApiBase {
this.node.defaultChild = resource;
this.restApiId = resource.ref;

const cloudWatchRole = props.cloudWatchRole ?? true;
const cloudWatchRoleDefault = FeatureFlags.of(this).isEnabled(APIGATEWAY_DISABLE_CLOUDWATCH_ROLE) ? false : true;
const cloudWatchRole = props.cloudWatchRole ?? cloudWatchRoleDefault;
if (cloudWatchRole) {
this._configureCloudWatchRole(resource);
}
Expand Down
1 change: 1 addition & 0 deletions packages/@aws-cdk/aws-apigateway/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@
"@aws-cdk/assertions": "0.0.0",
"@aws-cdk/cdk-build-tools": "0.0.0",
"@aws-cdk/integ-runner": "0.0.0",
"@aws-cdk/integ-tests": "0.0.0",
"@aws-cdk/cfn2ts": "0.0.0",
"@aws-cdk/pkglint": "0.0.0",
"@types/jest": "^27.5.2"
Expand Down
Original file line number Diff line number Diff line change
@@ -1 +1 @@
{"version":"17.0.0"}
{"version":"20.0.0"}
Original file line number Diff line number Diff line change
@@ -1,14 +1,11 @@
{
"version": "20.0.0",
"testCases": {
"integ.api-definition.asset": {
"restapi-fromdefinition/DefaultTest": {
"stacks": [
"integtest-restapi-fromdefinition-asset"
],
"diffAssets": false,
"stackUpdateWorkflow": true
"assertionStack": "restapifromdefinitionDefaultTestDeployAssertDF3B0845"
}
},
"synthContext": {},
"enableLookups": false
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,9 @@
]
}
]
}
},
"UpdateReplacePolicy": "Retain",
"DeletionPolicy": "Retain"
},
"myapiAccountEC421A0A": {
"Type": "AWS::ApiGateway::Account",
Expand All @@ -157,7 +159,9 @@
},
"DependsOn": [
"myapi4C7BF186"
]
],
"UpdateReplacePolicy": "Retain",
"DeletionPolicy": "Retain"
}
},
"Outputs": {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"version": "17.0.0",
"version": "20.0.0",
"artifacts": {
"Tree": {
"type": "cdk:tree",
Expand Down Expand Up @@ -109,6 +109,15 @@
]
},
"displayName": "integtest-restapi-fromdefinition-asset"
},
"restapifromdefinitionDefaultTestDeployAssertDF3B0845": {
"type": "aws:cloudformation:stack",
"environment": "aws://unknown-account/unknown-region",
"properties": {
"templateFile": "restapifromdefinitionDefaultTestDeployAssertDF3B0845.template.json",
"validateOnSynth": false
},
"displayName": "restapi-fromdefinition/DefaultTest/DeployAssert"
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{}
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@
"id": "Tree",
"path": "Tree",
"constructInfo": {
"fqn": "@aws-cdk/core.Construct",
"version": "0.0.0"
"fqn": "constructs.Construct",
"version": "10.1.63"
}
},
"integtest-restapi-fromdefinition-asset": {
Expand Down Expand Up @@ -362,14 +362,14 @@
}
},
"constructInfo": {
"fqn": "@aws-cdk/core.Construct",
"version": "0.0.0"
"fqn": "constructs.Construct",
"version": "10.1.63"
}
}
},
"constructInfo": {
"fqn": "@aws-cdk/core.Construct",
"version": "0.0.0"
"fqn": "constructs.Construct",
"version": "10.1.63"
}
},
"PetsURL": {
Expand All @@ -393,6 +393,42 @@
"fqn": "@aws-cdk/core.Stack",
"version": "0.0.0"
}
},
"restapi-fromdefinition": {
"id": "restapi-fromdefinition",
"path": "restapi-fromdefinition",
"children": {
"DefaultTest": {
"id": "DefaultTest",
"path": "restapi-fromdefinition/DefaultTest",
"children": {
"Default": {
"id": "Default",
"path": "restapi-fromdefinition/DefaultTest/Default",
"constructInfo": {
"fqn": "constructs.Construct",
"version": "10.1.63"
}
},
"DeployAssert": {
"id": "DeployAssert",
"path": "restapi-fromdefinition/DefaultTest/DeployAssert",
"constructInfo": {
"fqn": "@aws-cdk/core.Stack",
"version": "0.0.0"
}
}
},
"constructInfo": {
"fqn": "@aws-cdk/integ-tests.IntegTestCase",
"version": "0.0.0"
}
}
},
"constructInfo": {
"fqn": "@aws-cdk/integ-tests.IntegTest",
"version": "0.0.0"
}
}
},
"constructInfo": {
Expand Down
Original file line number Diff line number Diff line change
@@ -1 +1 @@
{"version":"17.0.0"}
{"version":"20.0.0"}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{}
Original file line number Diff line number Diff line change
@@ -1,14 +1,11 @@
{
"version": "20.0.0",
"testCases": {
"integ.api-definition.inline": {
"inline-api-definition/DefaultTest": {
"stacks": [
"integtest-restapi-fromdefinition-inline"
],
"diffAssets": false,
"stackUpdateWorkflow": true
"assertionStack": "inlineapidefinitionDefaultTestDeployAssert923CAC29"
}
},
"synthContext": {},
"enableLookups": false
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,9 @@
]
}
]
}
},
"UpdateReplacePolicy": "Retain",
"DeletionPolicy": "Retain"
},
"myapiAccountEC421A0A": {
"Type": "AWS::ApiGateway::Account",
Expand All @@ -117,7 +119,9 @@
},
"DependsOn": [
"myapi4C7BF186"
]
],
"UpdateReplacePolicy": "Retain",
"DeletionPolicy": "Retain"
}
},
"Outputs": {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"version": "17.0.0",
"version": "20.0.0",
"artifacts": {
"Tree": {
"type": "cdk:tree",
Expand Down Expand Up @@ -59,6 +59,15 @@
]
},
"displayName": "integtest-restapi-fromdefinition-inline"
},
"inlineapidefinitionDefaultTestDeployAssert923CAC29": {
"type": "aws:cloudformation:stack",
"environment": "aws://unknown-account/unknown-region",
"properties": {
"templateFile": "inlineapidefinitionDefaultTestDeployAssert923CAC29.template.json",
"validateOnSynth": false
},
"displayName": "inline-api-definition/DefaultTest/DeployAssert"
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@
"id": "Tree",
"path": "Tree",
"constructInfo": {
"fqn": "@aws-cdk/core.Construct",
"version": "0.0.0"
"fqn": "constructs.Construct",
"version": "10.1.63"
}
},
"integtest-restapi-fromdefinition-inline": {
Expand Down Expand Up @@ -241,6 +241,42 @@
"fqn": "@aws-cdk/core.Stack",
"version": "0.0.0"
}
},
"inline-api-definition": {
"id": "inline-api-definition",
"path": "inline-api-definition",
"children": {
"DefaultTest": {
"id": "DefaultTest",
"path": "inline-api-definition/DefaultTest",
"children": {
"Default": {
"id": "Default",
"path": "inline-api-definition/DefaultTest/Default",
"constructInfo": {
"fqn": "constructs.Construct",
"version": "10.1.63"
}
},
"DeployAssert": {
"id": "DeployAssert",
"path": "inline-api-definition/DefaultTest/DeployAssert",
"constructInfo": {
"fqn": "@aws-cdk/core.Stack",
"version": "0.0.0"
}
}
},
"constructInfo": {
"fqn": "@aws-cdk/integ-tests.IntegTestCase",
"version": "0.0.0"
}
}
},
"constructInfo": {
"fqn": "@aws-cdk/integ-tests.IntegTest",
"version": "0.0.0"
}
}
},
"constructInfo": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,9 @@
]
}
]
}
},
"UpdateReplacePolicy": "Retain",
"DeletionPolicy": "Retain"
},
"myrestapiAccountA49A05BE": {
"Type": "AWS::ApiGateway::Account",
Expand All @@ -99,7 +101,9 @@
},
"DependsOn": [
"myrestapi551C8392"
]
],
"UpdateReplacePolicy": "Retain",
"DeletionPolicy": "Retain"
},
"myrestapiDeployment419B1464b903292b53d7532ca4296973bcb95b1a": {
"Type": "AWS::ApiGateway::Deployment",
Expand Down
Original file line number Diff line number Diff line change
@@ -1 +1 @@
{"version":"17.0.0"}
{"version":"20.0.0"}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{}
Loading

0 comments on commit 78c858f

Please sign in to comment.