Serverless-application-model: Using intrinsic functions in AWS::Serverless::Function Policies section puts the output in ManagedPolicyArns instead of Policies in resultant role

[maynardflies]

Description:

The Policies section of an AWS::Serverless::Function resource accepts both Managed policy names, and policies themselves.

In my template, I wish to exclude a policy from the list when a resource isn't created, and that is determined by a parameter. I do this with a Fn::If statement thusly

"Policies": [
          {
            "Fn::If": [
              "CreateDaxCluster",
              {
                "Version": "2012-10-17",
                "Statement": [
                  {
                    "Action": [
                      "dax:*"
                    ],
                    "Effect": "Allow",
                    "Resource": [
                      { "Fn::GetAtt": [ "Dax", "Arn" ] }
                    ]
                  }
                ]
              },
              { "Ref": "AWS::NoValue" }
            ]
          },
          {
            "Version": "2012-10-17",
            "Statement": [
              {
                "Action": [
                  "dynamodb:BatchGetItem",
                  "dynamodb:BatchWriteItem",
                  "dynamodb:PutItem",
                  "dynamodb:DescribeTable",
                  "dynamodb:GetItem",
                  "dynamodb:Scan",
                  "dynamodb:UpdateItem",
                  "dynamodb:UpdateTable"
                ],
                "Resource": { "Fn::GetAtt": [ "DynamoTable", "Arn" ] },
                "Effect": "Allow"
              }
            ]
          },
          "AWSLambdaBasicExecutionRole",
          "AWSLambdaVPCAccessExecutionRole",
          "AWSXrayWriteOnlyAccess"
        ],

When I set "CreateDaxCluster" to false, this works fine, and the Dax policy is not created. However, when it is set to true, because the Fn::If block mistakenly gets put into the ManagedPolicyArns section of the resultant template, instead of into the Policies section, an error is thrown saying that ManagedPolicyArns needs to be a List of string. the resultant role ends up looking like this

{
  "BulkGetRole": {
    "Type": "AWS::IAM::Role",
      "Properties": {
        "ManagedPolicyArns": ["arn:aws:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole", "arn:aws:iam::aws:policy/AWSXrayWriteOnlyAccess", {
          "Fn::If": ["CreateDaxCluster", {
            "Version": "2012-10-17",
            "Statement": [{
              "Action": ["dax:*"],
              "Resource": [{
                "Fn::GetAtt": ["Dax", "Arn"]
              }],
              "Effect": "Allow"
            }]
          }, {
            "Ref": "AWS::NoValue"
          }]
        }, "arn:aws:iam::aws:policy/service-role/AWSLambdaVPCAccessExecutionRole"],
        "Policies": [{
          "PolicyName": "BulkGetRolePolicy0",
          "PolicyDocument": {
            "Version": "2012-10-17",
            "Statement": [{
              "Action": ["dynamodb:BatchGetItem", "dynamodb:DescribeTable", "dynamodb:GetItem", "dynamodb:Scan", "dynamodb:Query"],
              "Resource": {
                "Fn::GetAtt": ["DynamoTable", "Arn"]
              },
              "Effect": "Allow"
            }]
          }
        }],
        "AssumeRolePolicyDocument": {
          "Version": "2012-10-17",
          "Statement": [{
            "Action": ["sts:AssumeRole"],
            "Effect": "Allow",
            "Principal": {
              "Service": ["lambda.amazonaws.com"]
            }
          }]
        }
      }
   }
}

Steps to reproduce the issue:

1. Create a function with a mix of Manged policy names and policy documents
2. Add a Fn::If intrinsic function to include/not include a policy document
3. deploy the serverless application

Observed result:
Deployment fails because policy document gets put into managedpolicyarns section of the generated role

Expected result:
Deployment succeeds and includes the policy in the Policies section of the generated role

[jlhood]

Thanks for reporting this issue! The problem seems to be an assumption the code is making that if an intrinsic function is being used within the Policies entries, it will resolve to a managed policy ARN, but this assumption is incorrect in the above case.

These fixes can be a bit tricky, because we don't want to reimplement CloudFormation intrinsic functions except in simple cases. However, there's probably a targeted fix that can be made so it works for this specific case (Fn::If).

The fix would be made here:

https://github.com/awslabs/serverless-application-model/blob/master/samtranslator/model/function_policies.py#L117-L118

Happy to work with someone who wants to pick up this bug fix!

[rohandalvi]

@jlhood I'd like to pick this up as my first issue.

[jlhood]

@rohandalvi Thanks so much! I think the change will be to check if the intrinsic is Fn::If, then call _get_type() recursively on the 2nd and 3rd arguments. When you resolve the types of those args, there can be a few cases:

1. Invalid template, e.g., there aren't 3 arguments passed to Fn::If -> raise InvalidTemplateException.
2. Both args are !Ref AWS::NoValue. This is essentially a no-op so it doesn't matter where it goes.
3. One arg is !Ref AWS::NoValue. Use the type returned by the other arg.
4. Both args resolve to the same type. Use that type.
5. args resolve to different types. This is tricky. The simple thing to do is to not support this case and raise InvalidTemplateException. However, ideally, we would support it. I think this can be done by converting the single Fn::If into two separate Fn::Ifs, where each argument is replaced with !Ref AWS::NoValue. Then you can treat them like case 3 above.

Hope this helps! Feel free to reach out if you have more questions.

[parimaldeshmukh]

Implemented in pr according to @jlhood 's suggestions.

bin/sam-translate.py fails when the !If function resolves to the policy template type.

This seems to be because the PolicyTemplatesForFunctionPlugin class expects a policy template object to be passed to it, but in this case we pass the entire If object. How should I proceed?

[jlhood]

@parimaldeshmukh Thanks for the PR! I've added comments and a suggestion on how to proceed.

[ShreyaGangishetty]

Closing this issue as this is released in v1.14.0