Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nested arrays in SecurityGroupIngress property of EC2_SecurityGroup #72

Closed
tbenade opened this issue Dec 8, 2014 · 15 comments
Closed
Labels

Comments

@tbenade
Copy link
Contributor

tbenade commented Dec 8, 2014

When trying to add ingress rules directly as a property on a SecurityGroup the array gets nested with an outer array.

EC2_SecurityGroup("SampleGroup"){
    VpcId Ref("VpcId")
    GroupDescription "Sample group"
    SecurityGroupIngress [{:IpProtocol=>"tcp"}]
  }

Will render the following.

"SecurityGroupIngress": [
          [
            {
              "IpProtocol": "tcp"
            }
          ]
        ]

I can see where it is all happening...well I think so. https://github.com/stevenjack/cfndsl/blob/master/lib/cfndsl/Plurals.rb#L7 I have not figured out how to fix it, if someone would like to give me some pointers I am happy to have a crack.

@howech
Copy link
Contributor

howech commented Jan 7, 2015

Try this

EC2_SecurityGroup("SampleGroup") {
  VpcId Ref("VPC")
  GroupDescription "My group"
  SecurityGroupIngress { IpProtocol "tcp" }
}

Renders:

"SampleGroup": {
  "Properties":{
    "VpcId":"asdfasdf",
    "GroupDescription":"Sample group",
    "SecurityGroupIngress":[{"IpProtocol":"tcp"}]
  },
  "Type":"AWS::EC2::SecurityGroup"}}}
}

@howech
Copy link
Contributor

howech commented Jan 7, 2015

(If you insist on wanting to pass in a hash to the SecurityGroupIngress property, effectively bypassing cfndsl's type contructor for the property, you have to do it this way

EC2_SecurityGroup("SampleGroup"){
    VpcId Ref("VpcId")
    GroupDescription "Sample group"
    SecurityGroupIngress( {:IpProtocol=>"tcp"} )
  }

There is lots of weirdness going on here. First, array type property declarations are designed to append a single item on to the list of items. If you pass it an array, it dutifully pushes the array on to the collected list of objects. Also, when you use this form, cfndsl assumes that you know what you are doing, sometimes this is a good thing in that it lets you pass in a hash representing an AWS type that has not been fully supported in cfndsl yet, but often, it just confuses people.

Another part of the weirdness has to do with ruby. Ruby lets you make function calls on a single argument without explicit parenthesis. This is what your original SecurityGroupIngess declaration was doing, with an array as an argument. However, you cannot pass a hash this way, as the ruby interpreter thinks that you are passing a code block. This is why my second example has explicit paranthesis...

@stevenjack
Copy link
Collaborator

@tbenade did this help you out?

@tbenade
Copy link
Contributor Author

tbenade commented Feb 10, 2015

My apologies @stevenjack & @howech I have not had a try yet. I will look at it shortly. One thing I would say is the types yaml https://github.com/stevenjack/cfndsl/blob/master/lib/cfndsl/aws_types.yaml states the expectation is an array, hence me wanting to use an array.

Given CloudFormation supports an array in the API will the DSL support defining more than one rule?

@yumpy
Copy link
Contributor

yumpy commented Jul 2, 2015

NetworkInterfaces in EC_Instance is of a similar format and this seem to generate without the nested array.

AWS_EC2_Instance("myInstance") {

  ImageId "myAMI"
  InstanceType "m3.medium"
  KeyName "myKey"

  NetworkInterfaces [
    {
      "AssociatePublicIpAddress" => "true",
      "DeleteOnTermination"      => "true",
      "Description"              => "Primary network interface",
      "DeviceIndex"              => 0,
      "GroupSet"                 => ["mySG1"],
      "SubnetId"                 => "mySubnet1"
    },
    {
      "AssociatePublicIpAddress" => "true",
      "DeleteOnTermination"      => "true",
      "Description"              => "Secondary network interface",
      "DeviceIndex"              => 1,
      "GroupSet"                 => ["mySG2"],
      "SubnetId"                 => "mySubnet2"
    }
  ]
}

generates

      "myInstance" : {
         "Type" : "AWS::EC2::Instance",
         "Properties" : {
            "NetworkInterfaces" : [
               {
                  "DeviceIndex" : 0,
                  "SubnetId" : "mySubnet1",
                  "Description" : "Primary network interface",
                  "GroupSet" : [
                     "mySG1"
                  ],
                  "AssociatePublicIpAddress" : "true",
                  "DeleteOnTermination" : "true"
               },
               {
                  "SubnetId" : "mySubnet2",
                  "Description" : "Secondary network interface",
                  "DeviceIndex" : 1,
                  "GroupSet" : [
                     "mySG2"
                  ],
                  "DeleteOnTermination" : "true",
                  "AssociatePublicIpAddress" : "true"
               }
            ],
            "KeyName" : "myKey",
            "ImageId" : "myAMI",
            "InstanceType" : "m3.medium"
         }

from aws_types.yaml

  "AWS::EC2::SecurityGroup" :
    Properties:
     ....
     SecurityGroupIngress: [ EC2SecurityGroupRule ]
     SecurityGroupEgress: [ EC2SecurityGroupRule ]


  "AWS::EC2::Instance" :
   Properties:
    ....
    NetworkInterfaces : [ NetworkInterfaceType ]
    ....

@yumpy
Copy link
Contributor

yumpy commented Jul 2, 2015

As mentioned in #109 my workaround (for the moment) is to remove the [ ... ] from aws_types.yaml

  "AWS::EC2::SecurityGroup" :
    Properties:
     ....
     SecurityGroupIngress:  EC2SecurityGroupRule 
     SecurityGroupEgress:  EC2SecurityGroupRule 

then I can use

  EC2_SecurityGroup("mySG") {
    VpcId "myVPC"
    SecurityGroupIngress [
      {
        "CidrIp"     => "10.0.0.0/8",
        "IpProtocol" => "tcp",
        "FromPort"   => "22",
        "ToPort"     => "2"
      },
      {
        "CidrIp"     => "10.0.0.0/8",
        "IpProtocol" => "tcp",
        "FromPort"   => "80",
        "ToPort"     => "80"
      },
      {
        "CidrIp"     => "10.0.0.0/8",
        "IpProtocol" => "tcp",
        "FromPort"   => "443",
        "ToPort"     => "443"
      },
      {
        "CidrIp"     => "10.0.0.0/8",
        "IpProtocol" => "tcp",
        "FromPort"   => "3389",
        "ToPort"     => "3389"
      },      
    ]
  }

@yumpy
Copy link
Contributor

yumpy commented Jul 21, 2015

I've looked at the comments in #107 but am still having problems with SecurityGroupIngress/SecurityGroupEgress where a list of rules is to be applied. (see previous comment in this issue for an example of what I'm trying to do).

@windlass
Copy link

I admit I am having problems with this too. A list of rules in Ingress/Egress only works if I do what @yumpy is doing above and remove the brackets around EC2SecurityGroupRule. I'm not sure if I'm working around a real problem or just not seeing a Better Way to pass a list of rules to a security group.

@yumpy
Copy link
Contributor

yumpy commented Dec 7, 2015

I've still not found a way to get this to work without removing the brackets around EC2SecurityGroupRule. If anyone has managed to get it working please could you post an example here. Thanks.

@ghost
Copy link

ghost commented Dec 7, 2015

@yumpy Here's how I got it to work with a single entry:

SecurityGroupIngress( { :IpProtocol=>"tcp", :FromPort=>"22", :ToPort=>"22", :CidrIp=>"0.0.0.0/0" } )

@yumpy
Copy link
Contributor

yumpy commented Dec 7, 2015

updated due to mistake

Thanks @rmurphy-stelligent. That works well. Any ideas on multiple entries?

eg to generate

                "SecurityGroupIngress": [
                    {
                        "CidrIp": "0.0.0.0/0",
                        "FromPort": 22,
                        "IpProtocol": "tcp",
                        "ToPort": 22
                    },
                    {
                        "CidrIp": "0.0.0.0/0",
                        "FromPort": 3389,
                        "IpProtocol": "tcp",
                        "ToPort": 3389
                    }
                ],

@johnf
Copy link
Collaborator

johnf commented Feb 20, 2016

I think I may have identified the issue here. aws_types has the following entries

"AWS::EC2::SecurityGroup" :
  Properties:
   SecurityGroupIngress: [ EC2SecurityGroupRule ]
"AWS::ElasticLoadBalancing::LoadBalancer" :
  Properties:
    Listeners: [ Listener ]

So you would expect the following to work

EC2_SecurityGroup('WebSecurityGroup') { 
  SecurityGroupIngress [
     EC2SecurityGroupRule {
      IpProtocol                 'tcp'
      ToPort                     '80'
    } 
    EC2SecurityGroupRule {
      IpProtocol 'tcp'
      ToPort     '22'
     },
  ] 
}

ElasticLoadBalancing_LoadBalancer('WebLoadBalancer') {
  Listeners [
    Listener {
      LoadBalancerPort 80
    } 
    Listener {
      LoadBalancerPort 80
    },
  ]
}

The LoadBalancer works fine but the SecurityGroup complains about EC2SecurityGroupRule being not found.

The issue is at https://github.com/stevenjack/cfndsl/blob/master/lib/cfndsl/CloudFormationTemplate.rb#L100-L129
it takes Listeners converts to singular and expects the internal entry to be called Listener. This means you can make the first example work by doing the following

EC2_SecurityGroup('WebSecurityGroup') { 
  SecurityGroupIngress [
     SecurityGroupIngress {
      IpProtocol                 'tcp'
      ToPort                     '80'
    } 
    SecurityGroupIngress {
      IpProtocol 'tcp'
      ToPort     '22'
     },
  ] 
}

So I'm not sure here what is actually wanted. Does aws_types.yaml define what things should be called. So should it be called EC2SecurityGroupRole or is it always just the singular?

@johnf
Copy link
Collaborator

johnf commented Feb 20, 2016

I have a feeling we should actually use EC2SecurityGroupRole since I get a wierd double show up when I try the other way. I have some code locally to make that work if it is what we want.

@yumpy
Copy link
Contributor

yumpy commented Mar 3, 2016

@johnf thanks for running with this. I look forward to your PR being merged.

@ghost
Copy link

ghost commented Apr 27, 2016

FYI 0.8.3 implemented breaking changes for those who used the workaround I mentioned in December of 2015. Be ready to update the syntax before upgrading.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants