aws-cloudformation / cfn-lint

CloudFormation Linter
MIT No Attribution
2.43k stars 591 forks source link

SAM Function Global Environment Variables Interact Oddly with `!If` and Conditional Values #1987

Closed chrisoverzero closed 3 years ago

chrisoverzero commented 3 years ago

cfn-lint version: (cfn-lint --version)

cfn-lint 0.46.0

Description of issue.

When the !If intrinsic function is used with conditional values in SAM's Global Environment Variables, cfn-lint warns that the value on the true side of the condition may not be available when the condition is not true. For example, given an AWS::AppConfig::Environment in the current template, an importable instance of same, and an environment variable meant to pass the ID of one of them to a Function:

---
AWSTemplateFormatVersion: 2010-09-09
Transform: AWS::Serverless-2016-10-31
Parameters:
  DeploymentMode:
    Type: String
    AllowedValues:
    - sandbox
    - live
    Default: sandbox
    ConstraintDescription: Value must be a known deployment mode.
Conditions:
  IsLive: !Equals [ !Ref DeploymentMode, live ]
Globals:
  Function:
    Environment:
      Variables:
        AppConfig__Environment: !If
        - IsLive
        - !Ref ConfigEnvironment
        - !ImportValue ConfigEnvironment-blah
Resources:
  ConfigApplication:
    Type: AWS::AppConfig::Application
    Properties:
      Name: Application-A
  ConfigEnvironment:
    Type: AWS::AppConfig::Environment
    Condition: IsLive
    Properties:
      ApplicationId: !Ref ConfigApplication
      Name: Environment-B
  FunctionC:
    Type: AWS::Serverless::Function
    Properties:
      Runtime: provided.al2
      Handler: provided

…cfn-lint reports this:

[cfn-lint] W1001: Ref to resource "ConfigEnvironment" that may not be available when condition "IsLive" is False at Function/Environment/Variables/AppConfig__Environment/Fn::If/1/Ref

Oddly, the error is reported at a location in Function (under Globals, presumably), which I think isn't a location which exists in the transformed template. More oddly, this error is reported only there. If the !If is removed and ConfigEnvironment is !Refed directly, the following are reported:

[cfn-lint] W1001: Ref to resource "ConfigEnvironment" that may not be available when condition "IsLive" is False at Function/Environment/Variables/AppConfig__Environment/Ref

[cfn-lint] W1001: Ref to resource "ConfigEnvironment" that may not be available when condition "IsLive" is False at Resources/FunctionC/Properties/Environment/Variables/AppConfig__Environment/Ref

That is, the error is reported both in the Globals section and in the Environment.Variables of FunctionC itself. But when the !If is present (as it should be), the error remains in Globals. If the environment variables are moved from Globals to directly within FunctionC, no error is reported. Unfortunately, this does not scale. Suppressing W1001 globally for deployments is our current workaround.

Nothing in cfn-lint jumps out at me as responsible for this issue, so I don't have any leads for you to track down. The thing that looked closest was how this section is considered to have "non-strict" values, but that appears to be related to string vs. number, so it didn't seem right to me.

PatMyron commented 3 years ago

Mapping from the violation in the transformed template back to the line in the original source template is a bit best effort

Still looking into the violation itself though

chrisoverzero commented 3 years ago

I think that it's because Line 933 in template.py is key-not-found'ing on text[path[0]]. At that time, path[0] is "Function", which isn't a location which exists in the transformed template. This is causing the path conditions to be empty. When the comparison is made between the scenario count (0) and the number of path conditions (0), the scenario is appended to the results.

Does that sound reasonable? I'm afraid I don't know what the best fix to apply is, if so. Maybe early-return results if len(path_conditions) == 0? It works in my testing, at least. I could whip up that PR. ha ha disregard that fails a bunch of unit tests

kddejong commented 3 years ago

Translated template for reference

{
  "AWSTemplateFormatVersion": null,
  "Conditions": {
    "IsLive": {
      "Fn::Equals": [
        {
          "Ref": "DeploymentMode"
        },
        "live"
      ]
    }
  },
  "Parameters": {
    "DeploymentMode": {
      "AllowedValues": [
        "sandbox",
        "live"
      ],
      "ConstraintDescription": "Value must be a known deployment mode.",
      "Default": "sandbox",
      "Type": "String"
    }
  },
  "Resources": {
    "ConfigApplication": {
      "Properties": {
        "Name": "Application-A"
      },
      "Type": "AWS::AppConfig::Application"
    },
    "ConfigEnvironment": {
      "Condition": "IsLive",
      "Properties": {
        "ApplicationId": {
          "Ref": "ConfigApplication"
        },
        "Name": "Environment-B"
      },
      "Type": "AWS::AppConfig::Environment"
    },
    "FunctionC": {
      "Properties": {
        "Code": {
          "S3Bucket": "bucket",
          "S3Key": "value"
        },
        "Environment": {
          "Variables": {
            "AppConfig__Environment": {
              "Fn::If": [
                "IsLive",
                {
                  "Ref": "ConfigEnvironment"
                },
                {
                  "Fn::ImportValue": "ConfigEnvironment-blah"
                }
              ]
            }
          }
        },
        "Handler": "provided",
        "Role": {
          "Fn::GetAtt": [
            "FunctionCRole",
            "Arn"
          ]
        },
        "Runtime": "provided.al2",
        "Tags": [
          {
            "Key": "lambda:createdBy",
            "Value": "SAM"
          }
        ]
      },
      "Type": "AWS::Lambda::Function"
    },
    "FunctionCRole": {
      "Properties": {
        "AssumeRolePolicyDocument": {
          "Statement": [
            {
              "Action": [
                "sts:AssumeRole"
              ],
              "Effect": "Allow",
              "Principal": {
                "Service": [
                  "lambda.amazonaws.com"
                ]
              }
            }
          ],
          "Version": "2012-10-17"
        },
        "ManagedPolicyArns": [
          "arn:aws:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole"
        ],
        "Tags": [
          {
            "Key": "lambda:createdBy",
            "Value": "SAM"
          }
        ]
      },
      "Type": "AWS::IAM::Role"
    }
  }
}

A first glance this looks okay. I need to look into this a little more.

kddejong commented 3 years ago

Seeing what @chrisoverzero is seeing. Trying to figure out why now. That path is for sure jacked and need to figure out why.

kddejong commented 3 years ago

Figured out the fix. Working on a resolution.

chrisoverzero commented 3 years ago

I confirm the fix. Thanks so much.