bazaarvoice / cloudformation-ruby-dsl

Ruby DSL for creating Cloudformation templates
Apache License 2.0
210 stars 76 forks source link

Handle attributes split on dot in name #125

Open icy-arctic-fox opened 6 years ago

icy-arctic-fox commented 6 years ago

Description

Attributes that have a dot in the name causing Fn::GetAtt to have more than two elements generates bad Ruby code.

Steps to Test or Reproduce

  1. Create a CloudFormation template that uses Fn::GetAtt. Using an example from the documentation reproduces this.
    {
    "AWSTemplateFormatVersion": "2010-09-09",
    "Resources": {
        "myELB": {
            "Type": "AWS::ElasticLoadBalancing::LoadBalancer",
            "Properties": {
                "AvailabilityZones": [
                    "eu-west-1a"
                ],
                "Listeners": [
                    {
                        "LoadBalancerPort": "80",
                        "InstancePort": "80",
                        "Protocol": "HTTP"
                    }
                ]
            }
        },
        "myELBIngressGroup": {
            "Type": "AWS::EC2::SecurityGroup",
            "Properties": {
                "GroupDescription": "ELB ingress group",
                "SecurityGroupIngress": [
                    {
                        "IpProtocol": "tcp",
                        "FromPort": "80",
                        "ToPort": "80",
                        "SourceSecurityGroupOwnerId": {
                            "Fn::GetAtt": [
                                "myELB",
                                "SourceSecurityGroup",
                                "OwnerAlias"
                            ]
                        },
                        "SourceSecurityGroupName": {
                            "Fn::GetAtt": [
                                "myELB",
                                "SourceSecurityGroup",
                                "GroupName"
                            ]
                        }
                    }
                ]
            }
        }
    }
    }
  2. Run cfntemplate-to-ruby on the template.
  3. Notice that there are three arguments given to get_att()
    get_att('myELB', 'SourceSecurityGroup', 'OwnerAlias')
  4. Run ruby on the generated Ruby code. An error occurs:
    .../cloudformation-ruby-dsl-1.4.6/lib/cloudformation-ruby-dsl/dsl.rb:264:in `get_att': wrong number of arguments (given 3, expected 2) (ArgumentError)

Environment

Running on Ubuntu ruby 2.4.0p0 (2016-12-24 revision 57164) [x86_64-linux] Environment shouldn't matter for this issue.

Deploy Notes

Minor fix for something that could be worked around.

Related Issues and PRs

Issues

Todos

Request to Review

@jonaf/@temujin9

jonaf commented 6 years ago

Hm... Based on your example, it looks like the bug exists in cfntemplate-to-ruby, not the DSL. Fn::GetAtt only accepts two arguments, so keeping the method signature compatible with the Cloudformation intrinsic function is preferable since it reduces the burden on template authors. Have you considered updating cfntemplate-to-ruby to produce the correct Fn::GetAtt invocation instead?

icy-arctic-fox commented 6 years ago

I originally fixed it in cfntemplate-to-ruby with b806c87. Then I decided to also have get_att() from the DSL handle more than two parameters. The official documentation from AWS shows that attribute names can be split on the dot into multiple array elements.

For instance: SourceSecurityGroup.GroupName

"SourceSecurityGroupName": {
  "Fn::GetAtt": [
    "myELB",
    "SourceSecurityGroup",
    "GroupName"
  ]
}
jonaf commented 6 years ago

Oh, I see. The method signature described in the official documentation doesn't indicate that Fn::GetAtt has variable arity, but the example you pointed out does have n>2 arity. In that case, would you mind additionally updating the main README that describes the get_att method signature as well?

I know it's a lot to ask, but a test case would be phenomenal. I know the library testing is lacking a bit, so if you find yourself fighting with it, we can skip the test case for now.

icy-arctic-fox commented 6 years ago

Certainly, no problem!

icy-arctic-fox commented 6 years ago

Sorry for the long delay. Updated the README and added the complex test, which includes cases for GetAtt.

jonaf commented 6 years ago

Changes look good to me. @ohhatiya Since you guys own this project now, I don't know how you feel about my merging of changes into it, but this PR looks ready to me.