aws / serverless-application-model

The AWS Serverless Application Model (AWS SAM) transform is a AWS CloudFormation macro that transforms SAM templates into CloudFormation templates.
https://aws.amazon.com/serverless/sam
Apache License 2.0
9.32k stars 2.37k forks source link

API Usage Plan Generates (Unnecessary?) 'DependsOn' #1541

Open thbishop-intuit opened 4 years ago

thbishop-intuit commented 4 years ago

Description: Specifying an API usage plan will populate DependsOn with resources that are already included as a Ref in Properties. There are a couple of places where this currently occurs. To be clear, this doesn't break functionality but it does report as a warning in cfn-lint (which is how I came across this).

Is this extra DependsOn perhaps needed to handle a very specific use case? I'm more than happy to submit a fix if these extra declarations are not needed.

Steps to reproduce the issue:

  1. This is demonstrated within the translator tests. For example, this input (which is head of develop at the time of this bug).

Observed result: These are from the translator test outputs:

A couple of examples for usage plan resource: https://github.com/awslabs/serverless-application-model/blob/1b2d6f25514780d918b1bebf262ea05b89c5e646/tests/translator/output/api_with_usageplans.json#L392-L430

https://github.com/awslabs/serverless-application-model/blob/1b2d6f25514780d918b1bebf262ea05b89c5e646/tests/translator/output/api_with_usageplans.json#L365-L391

And a couple of examples for usage plan key resource: https://github.com/awslabs/serverless-application-model/blob/1b2d6f25514780d918b1bebf262ea05b89c5e646/tests/translator/output/api_with_usageplans.json#L83-L97

https://github.com/awslabs/serverless-application-model/blob/1b2d6f25514780d918b1bebf262ea05b89c5e646/tests/translator/output/api_with_usageplans.json#L530-L544

Expected result: I would expect the usage plan resource to look like:

    "MyApiTwoUsagePlan": {
      "Type": "AWS::ApiGateway::UsagePlan",
      "Properties": {
        "ApiStages": [
          {
            "ApiId": {
              "Ref": "MyApiTwo"
            },
            "Stage": {
              "Ref": "MyApiTwoProdStage"
            }
          }
        ],
        "Description": "Description for usage plan",
        "Tags": [
          {
            "Value": "value1",
            "Key": "key1"
          },
          {
            "Value": "value2",
            "Key": "key2"
          }
        ],
        "Quota": {
          "Limit": 10,
          "Period": "MONTH",
          "Offset": 10
        },
        "Throttle": {
          "RateLimit": 1000,
          "BurstLimit": 1000
        },
        "UsagePlanName": "SomeRandomName"
      }
    },
    "ServerlessUsagePlan": {
      "Type": "AWS::ApiGateway::UsagePlan",
      "Properties": {
        "ApiStages": [
          {
            "ApiId": {
              "Ref": "MyApiThree"
            },
            "Stage": {
              "Ref": "MyApiThreeProdStage"
            }
          },
          {
            "ApiId": {
              "Ref": "ServerlessRestApi"
            },
            "Stage": {
              "Ref": "ServerlessRestApiProdStage"
            }
          }
        ]
      }
    },

And for usage plan key resources:

    "MyApiTwoUsagePlanKey": {
      "Type": "AWS::ApiGateway::UsagePlanKey",
      "Properties": {
        "KeyType": "API_KEY",
        "KeyId": {
          "Ref": "MyApiTwoApiKey"
        },
        "UsagePlanId": {
          "Ref": "MyApiTwoUsagePlan"
        }
      }
    },
    "ServerlessUsagePlanKey": {
      "Type": "AWS::ApiGateway::UsagePlanKey",
      "Properties": {
        "KeyType": "API_KEY",
        "KeyId": {
          "Ref": "ServerlessApiKey"
        },
        "UsagePlanId": {
          "Ref": "ServerlessUsagePlan"
        }
      }
    },
chrisoverzero commented 3 years ago

I re-implemented this, having failed to find this ticket and its PR beforehand. Oops! I am also eager to see this done – there are linting tools (including AWS' own cfn-lint) which complain about the redundant DependsOns.

TobySaundersGDS commented 1 year ago

This can be resolved by adding a .cfnlintrc.yaml file with the following:

 ignore_checks:
   - W3005

But it would be better not to have to ignore that check globally to fix this

HCrane commented 7 months ago

Bump as this is still happening. Any updates as of now?

dominikj-cf commented 5 months ago

Also bump, still happening and breaking lint.

Will apply ignore fix from above but that of course disables the check on user written template code also.