Cfn-python-lint: LoadBalancer doesn't verify SecurityGroups

[viraptor]

cfn-lint version: 0.10.3

Description of issue.

AWS::ElasticLoadBalancingV2::LoadBalancer doesn't verify that the SecurityGroups property lists group ids. This is wrong and will pass:

Elb:
  Type: "AWS::ElasticLoadBalancingV2::LoadBalancer"
  Properties:
    SecurityGroups:
      - !Ref SecurityGroup

This is better (also passes):

Elb:
  Type: "AWS::ElasticLoadBalancingV2::LoadBalancer"
  Properties:
    SecurityGroups:
      - !GetAtt SecurityGroup.GroupId

[cmmeyer]

Thanks for the feedback. The problem is, it would be valid to pass in the group IDs as a parameter and reference it the way you do in you first example:

Parameters:
  SecurityGroup: 
    Description: Group ID of an existing security group
    Type: String
Resources:
  Elb: 
    Type: "AWS::ElasticLoadBalancingV2::LoadBalancer"
    Properties: 
      SecurityGroups: 
      - !Ref SecurityGroup

...is perfectly valid.

[cmmeyer]

Now, that said -- It might be valuable to have a deep check on the object associated with the Ref to see it is a string or a logical ID for a resource, then check if that resource is a SecurityGroup and what the return value for Ref vs. GetAtt are. But that's all forward thinking.

Do you mind if I flag this as an enhancement while we ponder how we could help you here?

[cmmeyer]

Strike that -- talking to @kddejong it looks like this may be a data issue with the way we enriched the linter. We'll dig in on that.

[viraptor]

Sure. Sorry for not making this more specific. Yes, I meant only the case where the ref points at a known resource, not just an arbitrary parameter.

[kddejong]

https://github.com/awslabs/cfn-python-lint/blob/master/src/cfnlint/data/ExtendedSpecs/all/03_value_types.json#L958-L972

I think we just need to add the appropriate item. I may have to rename a few things here though.

[kddejong]

oh and I kind of remember why this got crazy.

When you specify an AWS::EC2::SecurityGroup type as an argument to the Ref function, AWS CloudFormation returns the security group name or the security group ID (for EC2-VPC security groups that are not in a default VPC).

We could check that you are REFing a Security Group (as opposed to a completely random resource) but I'm not sure we would be able to know if the security group is in a default VPC or not. I'm open to thoughts here and I should probably do some extra testing to validate that is actually true.

[viraptor]

Since one approach is "sometimes fine" and the other "definitely correct", would it be valid to always recommend the second one? (Or with a comment that it's a more resilient interface?)

[cmmeyer]

We could drop it in as an "Ixxxx" rule for governance. I think the problem is that too many existing templates (some of them canonical examples from AWS) get flagged as wrong if we set it at any other level.