fix(aws-apigateway): CloudWatch logging should be disabled by default…

… (under feature flag) (#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 #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*
aws · Aug 12, 2022 · 78c858f · 78c858f
1 parent 16c0c98
commit 78c858f
Show file tree

Hide file tree

Showing 161 changed files with 1,772 additions and 333 deletions.
diff --git a/packages/@aws-cdk/aws-apigateway/README.md b/packages/@aws-cdk/aws-apigateway/README.md
@@ -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

diff --git a/packages/@aws-cdk/aws-apigateway/lib/restapi.ts b/packages/@aws-cdk/aws-apigateway/lib/restapi.ts
@@ -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';
@@ -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;
 
@@ -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);
   }
@@ -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);
     }
@@ -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);
     }

diff --git a/packages/@aws-cdk/aws-apigateway/package.json b/packages/@aws-cdk/aws-apigateway/package.json
@@ -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"

diff --git a/packages/@aws-cdk/aws-apigateway/test/api-definition.asset.integ.snapshot/cdk.out b/packages/@aws-cdk/aws-apigateway/test/api-definition.asset.integ.snapshot/cdk.out
@@ -1 +1 @@
-{"version":"17.0.0"}
+{"version":"20.0.0"}
diff --git a/packages/@aws-cdk/aws-apigateway/test/api-definition.asset.integ.snapshot/integ.json b/packages/@aws-cdk/aws-apigateway/test/api-definition.asset.integ.snapshot/integ.json
@@ -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
+  }
 }
diff --git a/.../api-definition.asset.integ.snapshot/integtest-restapi-fromdefinition-asset.template.json b/.../api-definition.asset.integ.snapshot/integtest-restapi-fromdefinition-asset.template.json
@@ -143,7 +143,9 @@
       ]
      }
     ]
-   }
+   },
+   "UpdateReplacePolicy": "Retain",
+   "DeletionPolicy": "Retain"
   },
   "myapiAccountEC421A0A": {
    "Type": "AWS::ApiGateway::Account",
@@ -157,7 +159,9 @@
    },
    "DependsOn": [
     "myapi4C7BF186"
-   ]
+   ],
+   "UpdateReplacePolicy": "Retain",
+   "DeletionPolicy": "Retain"
   }
  },
  "Outputs": {

diff --git a/packages/@aws-cdk/aws-apigateway/test/api-definition.asset.integ.snapshot/manifest.json b/packages/@aws-cdk/aws-apigateway/test/api-definition.asset.integ.snapshot/manifest.json
@@ -1,5 +1,5 @@
 {
-  "version": "17.0.0",
+  "version": "20.0.0",
   "artifacts": {
     "Tree": {
       "type": "cdk:tree",
@@ -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"
     }
   }
 }
diff --git a/...n.asset.integ.snapshot/restapifromdefinitionDefaultTestDeployAssertDF3B0845.template.json b/...n.asset.integ.snapshot/restapifromdefinitionDefaultTestDeployAssertDF3B0845.template.json
@@ -0,0 +1 @@
+{}
diff --git a/packages/@aws-cdk/aws-apigateway/test/api-definition.asset.integ.snapshot/tree.json b/packages/@aws-cdk/aws-apigateway/test/api-definition.asset.integ.snapshot/tree.json
@@ -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": {
@@ -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": {
@@ -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": {

diff --git a/packages/@aws-cdk/aws-apigateway/test/api-definition.inline.integ.snapshot/cdk.out b/packages/@aws-cdk/aws-apigateway/test/api-definition.inline.integ.snapshot/cdk.out
@@ -1 +1 @@
-{"version":"17.0.0"}
+{"version":"20.0.0"}
diff --git a/...on.inline.integ.snapshot/inlineapidefinitionDefaultTestDeployAssert923CAC29.template.json b/...on.inline.integ.snapshot/inlineapidefinitionDefaultTestDeployAssert923CAC29.template.json
@@ -0,0 +1 @@
+{}
diff --git a/packages/@aws-cdk/aws-apigateway/test/api-definition.inline.integ.snapshot/integ.json b/packages/@aws-cdk/aws-apigateway/test/api-definition.inline.integ.snapshot/integ.json
@@ -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
+  }
 }
diff --git a/...pi-definition.inline.integ.snapshot/integtest-restapi-fromdefinition-inline.template.json b/...pi-definition.inline.integ.snapshot/integtest-restapi-fromdefinition-inline.template.json
@@ -103,7 +103,9 @@
       ]
      }
     ]
-   }
+   },
+   "UpdateReplacePolicy": "Retain",
+   "DeletionPolicy": "Retain"
   },
   "myapiAccountEC421A0A": {
    "Type": "AWS::ApiGateway::Account",
@@ -117,7 +119,9 @@
    },
    "DependsOn": [
     "myapi4C7BF186"
-   ]
+   ],
+   "UpdateReplacePolicy": "Retain",
+   "DeletionPolicy": "Retain"
   }
  },
  "Outputs": {

diff --git a/packages/@aws-cdk/aws-apigateway/test/api-definition.inline.integ.snapshot/manifest.json b/packages/@aws-cdk/aws-apigateway/test/api-definition.inline.integ.snapshot/manifest.json
@@ -1,5 +1,5 @@
 {
-  "version": "17.0.0",
+  "version": "20.0.0",
   "artifacts": {
     "Tree": {
       "type": "cdk:tree",
@@ -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"
     }
   }
 }
diff --git a/packages/@aws-cdk/aws-apigateway/test/api-definition.inline.integ.snapshot/tree.json b/packages/@aws-cdk/aws-apigateway/test/api-definition.inline.integ.snapshot/tree.json
@@ -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": {
@@ -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": {

diff --git a/...thorizers/cognito-authorizer.integ.snapshot/CognitoUserPoolsAuthorizerInteg.template.json b/...thorizers/cognito-authorizer.integ.snapshot/CognitoUserPoolsAuthorizerInteg.template.json
@@ -85,7 +85,9 @@
       ]
      }
     ]
-   }
+   },
+   "UpdateReplacePolicy": "Retain",
+   "DeletionPolicy": "Retain"
   },
   "myrestapiAccountA49A05BE": {
    "Type": "AWS::ApiGateway::Account",
@@ -99,7 +101,9 @@
    },
    "DependsOn": [
     "myrestapi551C8392"
-   ]
+   ],
+   "UpdateReplacePolicy": "Retain",
+   "DeletionPolicy": "Retain"
   },
   "myrestapiDeployment419B1464b903292b53d7532ca4296973bcb95b1a": {
    "Type": "AWS::ApiGateway::Deployment",

diff --git a/packages/@aws-cdk/aws-apigateway/test/authorizers/cognito-authorizer.integ.snapshot/cdk.out b/packages/@aws-cdk/aws-apigateway/test/authorizers/cognito-authorizer.integ.snapshot/cdk.out
@@ -1 +1 @@
-{"version":"17.0.0"}
+{"version":"20.0.0"}
diff --git a/...-authorizer.integ.snapshot/cognitoauthorizerDefaultTestDeployAssert4551574C.template.json b/...-authorizer.integ.snapshot/cognitoauthorizerDefaultTestDeployAssert4551574C.template.json
@@ -0,0 +1 @@
+{}