aws-cloudformation / cfn-lint

CloudFormation Linter
MIT No Attribution
2.4k stars 576 forks source link

Add warning for singleton resource types (AWS::ApiGateway::Account) #1757

Open ConnorKirk opened 3 years ago

ConnorKirk commented 3 years ago

Feature Request

Summary

The behaviour of the AWS::ApiGateway::Account resource is surprising when compared to most CloudFormation resources. The resource has side effects, and can overwrite an existing value for the resource. I suggest adding a rule to cfn-lint to warn users when this resource is used in a template.

If there is consensus about adding this rule, I am happy to make the PR.

Reasoning

Most CloudFormation resources do not have side effects. Creating an EC2 instance, then deleting it, does not affect other resources in an account (for good reason).

The AWS::ApiGateway::Account resource updates the role used by API Gateway to put logs into Cloudwatch. This is an account wide setting, more like a global variable than a standard CloudFormation resource If a role is already specified, it will be overwritten when a template containing this resource is deployed. This is surprising to users, and affects all API Gateways in an account.

As an example, here is a CloudFormation template. If this template is deployed, it's not clear what the value of the API Gateway Cloudwatch Role Arn will be. Defining two resources means one will get overwritten.

Resources:
  CloudWatchRole:
    Type: 'AWS::IAM::Role'
    Properties:
      AssumeRolePolicyDocument:
        Version: 2012-10-17
        Statement:
          - Effect: Allow
            Principal:
              Service:
                - apigateway.amazonaws.com
            Action: 'sts:AssumeRole'
      Path: /
      ManagedPolicyArns:
        - >-
          arn:aws:iam::aws:policy/service-role/AmazonAPIGatewayPushToCloudWatchLogs
  Account:
    Type: 'AWS::ApiGateway::Account'
    Properties:
      CloudWatchRoleArn: !GetAtt 
        - CloudWatchRole
        - Arn

  CloudWatchRole2:
    Type: 'AWS::IAM::Role'
    Properties:
      AssumeRolePolicyDocument:
        Version: 2012-10-17
        Statement:
          - Effect: Allow
            Principal:
              Service:
                - apigateway.amazonaws.com
            Action: 'sts:AssumeRole'
      Path: /
      ManagedPolicyArns:
        - >-
          arn:aws:iam::aws:policy/service-role/AmazonAPIGatewayPushToCloudWatchLogs
  Account2:
    Type: 'AWS::ApiGateway::Account'
    Properties:
      CloudWatchRoleArn: !GetAtt 
        - CloudWatchRole2
        - Arn

Whilst this scenario is unlikely, it highlights the problematic behaviour of the resource.

ConnorKirk commented 3 years ago

Hey Pat, I am inferring from the title change that you're envisaging a general warning for singleton resources. That's a good idea 👍

Did you want me to try a PR?

benbridts commented 3 years ago

Telling Pat what he probably already knows, but it might be interesting for others.

When both https://github.com/aws-cloudformation/cfn-python-lint/pull/1732 and https://github.com/aws-cloudformation/aws-cloudformation-resource-schema/pull/86 land, you should be able to look in the schema for (some?) Singleton resources. As by definition, it's impossible to create two singletons, so they should always use the [delete, create] replacement strategy