Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ApiGateway] RestApi updates account level role used for ApiGateway CloudWatch logging #10878

Closed
lanefelker opened this issue Oct 14, 2020 · 10 comments · Fixed by #21546
Closed
Assignees
Labels
@aws-cdk/aws-apigateway Related to Amazon API Gateway bug This issue is a bug. effort/medium Medium work item – several days of effort p1

Comments

@lanefelker
Copy link

lanefelker commented Oct 14, 2020

This code in RestApi

const resource = new CfnAccount(this, 'Account', {

will update the account level role for cloudwatch logging used for all ApiGateways.

The problem we are seeing is that each new API we create will replace the role used for the account with the new role created.

If the stack that last updated the account level role gets deleted for some reason then the account level role will no longer exist and all apigateway cloudwatch logging is broken for the account 😱

Reproduction Steps

  1. create a new RestApi without passing a cloudWatchRole prop
  2. Deploy the new API - see the account level role change to the role associated with this new API
  3. Delete the stack
  4. All account level API logging no longer works because the role is deleted.

What did you expect to happen?

I would expect each apigateway logging role to be only used for a given API Gateway

or I would want the apigateway account level role to be rolled back to the previous role on deletion

What actually happened?

Described above

Environment

  • CLI Version :
  • Framework Version:
  • Node.js Version:
  • OS :
  • Language (Version):

Other

Is passing a role into each API the best option to resolve this?


This is 🐛 Bug Report

@lanefelker lanefelker added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Oct 14, 2020
@SomayaB SomayaB changed the title RestApi updates account level role used for ApiGateway CloudWatch logging [ApiGateway] RestApi updates account level role used for ApiGateway CloudWatch logging Oct 15, 2020
@github-actions github-actions bot added the @aws-cdk/aws-apigateway Related to Amazon API Gateway label Oct 15, 2020
@nija-at
Copy link
Contributor

nija-at commented Oct 21, 2020

Indeed this is a broken experience.

I would expect each apigateway logging role to be only used for a given API Gateway

or I would want the apigateway account level role to be rolled back to the previous role on deletion

Unfortunately, neither of these options will work. The APIGateway service does not provide a way to specify individual roles per API endpoint. The CDK is a stateless system and does not know what the previously value here is.

I recommend setting cloudWatchRole to false in all your APIs, and explicitly using CfnAccount to set one role per AWS account.

Apologies for the poor experience. We'll need to see how to re-adjust the RestApi construct to make this experience better.

@nija-at nija-at added effort/medium Medium work item – several days of effort p1 and removed needs-triage This issue or PR still needs to be triaged. labels Oct 21, 2020
@gsdwait
Copy link

gsdwait commented Jan 4, 2021

We keep running into this issue. Why should the default action be to create the role? Why not set the default for cloudWatchRole to false in RestApi and LambdaRestApi? That would mean in unusual case where you are only deploying a single rest api in an account you would simply set the cloudWatchRole to true.

@amirmohsen
Copy link

I agree with @gsdwait. The current default behaviour of having this option set to true is rather problematic as it overrides the global setting which applies to all APIs. This has caused a lot of headache for us in our company. Our team shares an AWS account with many other teams and suddenly our stack (which is the only CDK one currently) started overriding this role and having an impact on other teams. Please consider changing the default to false.

@straygar
Copy link

Any chance this could be made the default behavior for CDK v2? There have been so many times I accidentally and silently broke logs for all my API Gateways this way.

@adrian-skybaker
Copy link

Just to echo this - have we missed the window to fix this for v2?

Really hoping this isn't going to be a replay of #7140 (comment) ( "Since the API has been out there for a long time and our API Gateway module is stable, we cannot change this default").

@johnnynanjiang
Copy link

Hi @nija-at , would you reckon the simplest option for now, before a proper solution come up in an uncertain period of time, would be that the role can be retained in the function below?

  protected _configureCloudWatchRole(apiResource: CfnRestApi) {
    const role = new iam.Role(this, 'CloudWatchRole', {
      assumedBy: new iam.ServicePrincipal('apigateway.amazonaws.com'),
      managedPolicies: [iam.ManagedPolicy.fromAwsManagedPolicyName('service-role/AmazonAPIGatewayPushToCloudWatchLogs')],
    });

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

    this.cloudWatchAccount.node.addDependency(apiResource);
  }

@sekrupke
Copy link

This behaviour is indeed very annoying and may cause that devs search for the API logs and wondering why them are nowhere to be found. Would be great if that could be fixed.

@andrei-cdl
Copy link

Same here, we has this happen in our environment and silently changed the behaviour. Thankfully it was a side effect and this issue is open. There's probably a few teams out there not even realizing this behaviour is happening in their infrastructure.

@jpSimkins
Copy link

jpSimkins commented Jul 27, 2022

I just noticed this issue today as well... We've had this issue for over a year without even knowing it... yup, one of those teams that wasn't aware of this bug feature.

This affects PROD systems, I really hope this get addressed. I am testing if cloudWatchRole to false will solve our issue. Either way, defaulting to an account wide CloudWatch role seems like a very bad decision. I really hope this gets fixed but given how old this ticket is... I am doubtful.

Also, setting a single CloudWatch Role for the entire account is not ideal and raises many other concerns. I'm surprised this is recommended.

An example of this breaking a PROD system:

  • Deploy an API Gateway using LambdaRestApi from the api-gateway module.
  • Assign a role to the lambda function
  • Deploy

Now all API Gateways are using the role for the lambda function! This is dangerous for the default behavior.

Update:

Yes, using cloudWatchRole: false, stops this issue. Why is this is not the default?

@corymhall corymhall self-assigned this Aug 11, 2022
@mergify mergify bot closed this as completed in #21546 Aug 12, 2022
mergify bot pushed a commit that referenced this issue Aug 12, 2022
… (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*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

josephedward pushed a commit to josephedward/aws-cdk that referenced this issue Aug 30, 2022
… (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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-apigateway Related to Amazon API Gateway bug This issue is a bug. effort/medium Medium work item – several days of effort p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.