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

New resource: aws_network_interface_sg_attachment #860

Merged
1 change: 1 addition & 0 deletions aws/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,7 @@ func Provider() terraform.ResourceProvider {
"aws_s3_bucket_object": resourceAwsS3BucketObject(),
"aws_s3_bucket_notification": resourceAwsS3BucketNotification(),
"aws_security_group": resourceAwsSecurityGroup(),
"aws_security_group_attachment": resourceAwsSecurityGroupAttachment(),
"aws_default_security_group": resourceAwsDefaultSecurityGroup(),
"aws_security_group_rule": resourceAwsSecurityGroupRule(),
"aws_simpledb_domain": resourceAwsSimpleDBDomain(),
Expand Down
173 changes: 173 additions & 0 deletions aws/resource_aws_security_group_attachment.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,173 @@
package aws

import (
"fmt"
"log"
"reflect"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/ec2"
"github.com/hashicorp/terraform/helper/schema"
)

func resourceAwsSecurityGroupAttachment() *schema.Resource {
return &schema.Resource{
Create: resourceAwsSecurityGroupAttachmentCreate,
Read: resourceAwsSecurityGroupAttachmentRead,
Delete: resourceAwsSecurityGroupAttachmentDelete,
Schema: map[string]*schema.Schema{
"security_group_id": {
Type: schema.TypeString,
Required: true,
ForceNew: true,
},
"network_interface_id": {
Type: schema.TypeString,
Optional: true,
ForceNew: true,
},
},
}
}

func resourceAwsSecurityGroupAttachmentCreate(d *schema.ResourceData, meta interface{}) error {
if err := attachSecurityGroupToInterface(d, meta); err != nil {
return err
}

return resourceAwsSecurityGroupAttachmentRead(d, meta)
}

func attachSecurityGroupToInterface(d *schema.ResourceData, meta interface{}) error {
sgID := d.Get("security_group_id").(string)
interfaceID := d.Get("network_interface_id").(string)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we rely on this field, shouldn't it be Required in the schema then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated - this was like this because this resource supported both ENIs and actual instances at one point in time.


log.Printf("[INFO] Attaching security group %s to network interface ID %s", sgID, interfaceID)

conn := meta.(*AWSClient).ec2conn

dniParams := &ec2.DescribeNetworkInterfacesInput{
NetworkInterfaceIds: aws.StringSlice([]string{interfaceID}),
}

dniResp, err := conn.DescribeNetworkInterfaces(dniParams)
if err != nil {
return err
}

return addSGToENI(conn, sgID, dniResp.NetworkInterfaces[0])
}

func fetchNetworkInterface(conn *ec2.EC2, ifaceID string) (*ec2.NetworkInterface, error) {
dniParams := &ec2.DescribeNetworkInterfacesInput{
NetworkInterfaceIds: aws.StringSlice([]string{ifaceID}),
}

dniResp, err := conn.DescribeNetworkInterfaces(dniParams)
if err != nil {
return nil, err
}
return dniResp.NetworkInterfaces[0], nil
}

func addSGToENI(conn *ec2.EC2, sgID string, iface *ec2.NetworkInterface) error {
if sgExistsInENI(sgID, iface) {
return fmt.Errorf("security group %s already attached to interface ID %s", sgID, *iface.NetworkInterfaceId)
}
var groupIDs []string
for _, v := range iface.Groups {
groupIDs = append(groupIDs, *v.GroupId)
}
groupIDs = append(groupIDs, sgID)
params := &ec2.ModifyNetworkInterfaceAttributeInput{
NetworkInterfaceId: iface.NetworkInterfaceId,
Groups: aws.StringSlice(groupIDs),
}

_, err := conn.ModifyNetworkInterfaceAttribute(params)
return err
}

func sgExistsInENI(sgID string, iface *ec2.NetworkInterface) bool {
for _, v := range iface.Groups {
if *v.GroupId == sgID {
return true
}
}
return false
}

func resourceAwsSecurityGroupAttachmentRead(d *schema.ResourceData, meta interface{}) error {
return refreshSecurityGroupWithInterface(d, meta)
}

func refreshSecurityGroupWithInterface(d *schema.ResourceData, meta interface{}) error {
sgID := d.Get("security_group_id").(string)
interfaceID := d.Get("network_interface_id").(string)

log.Printf("[INFO] Checking association of security group %s to network interface ID %s", sgID, interfaceID)

conn := meta.(*AWSClient).ec2conn

iface, err := fetchNetworkInterface(conn, interfaceID)
if err != nil {
return err
}

if sgExistsInENI(sgID, iface) {
d.SetId(fmt.Sprintf("%s_%s", sgID, interfaceID))
} else {
// The assocation does not exist when it should, taint this resource.
log.Printf("[WARN] Security group %s not associated with network interface ID %s, tainting", sgID, interfaceID)
d.SetId("")
}
return nil
}

func resourceAwsSecurityGroupAttachmentDelete(d *schema.ResourceData, meta interface{}) error {
if err := detachSecurityGroupFromInterface(d, meta); err != nil {
return err
}

d.SetId("")
return nil
}

func detachSecurityGroupFromInterface(d *schema.ResourceData, meta interface{}) error {
sgID := d.Get("security_group_id").(string)
interfaceID := d.Get("network_interface_id").(string)

log.Printf("[INFO] Removing security group %s from instance ID %s", sgID, interfaceID)

conn := meta.(*AWSClient).ec2conn

iface, err := fetchNetworkInterface(conn, interfaceID)
if err != nil {
return err
}

return delSGFromENI(conn, sgID, iface)
}

func delSGFromENI(conn *ec2.EC2, sgID string, iface *ec2.NetworkInterface) error {
old := iface.Groups
var new []*string
for _, v := range iface.Groups {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 Isn't it dangerous to delete groups which we do not manage in this context by default? I don't mind having force_delete bool field which enables such behaviour but I think this is kind of breaking a promise to never touch infrastructure it didn't create or doesn't manage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, let me see if there's a safer way to do this. The main issue is that I don't think ModifyNetworkInterfaceAttribute allows you to individually add/remove security groups, but that's something I need to double check. So to effectively remove the security group we need to loop over the groups, skip the group we are removing, and build a set off the rest.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, that's a bit annoying (the API limitation) 😢 Let us know if you find a better way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't find a better way I think we'll need to use mutex to avoid modifying the same ENI from different resources in parallel.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack - good catch 😨

Do we have a provider-level mutex for this kind of thing?

Copy link
Contributor Author

@vancluever vancluever Jun 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like mutexkv is my answer. :) Will roll this in if there's no other way (I did check the SDK docs though and could not see anything more granular).

EDIT: Is the global awsMutexKV safe to use?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The safety just depends on the name of the mutex you pick - if we assume ENI IDs are globally unique (across regions) then that should be sufficiently safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All done adding in the locks and yeah it does look like it was racing, but not anymore! 😀

if *v.GroupId == sgID {
continue
}
new = append(new, v.GroupId)
}
if reflect.DeepEqual(old, new) {
// The interface already didn't have the security group, nothing to do
return nil
}

params := &ec2.ModifyNetworkInterfaceAttributeInput{
NetworkInterfaceId: iface.NetworkInterfaceId,
Groups: new,
}

_, err := conn.ModifyNetworkInterfaceAttribute(params)
return err
}
120 changes: 120 additions & 0 deletions aws/resource_aws_security_group_attachment_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
package aws

import (
"fmt"
"testing"

"github.com/hashicorp/terraform/helper/resource"
"github.com/hashicorp/terraform/terraform"
)

func TestAccAwsSecurityGroupAttachment(t *testing.T) {
cases := []struct {
Name string
External bool
}{
{
Name: "instance primary interface",
External: false,
},
{
Name: "externally supplied instance through data source",
External: true,
},
}
for _, tc := range cases {
t.Run(tc.Name, func(t *testing.T) {
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
Steps: []resource.TestStep{
resource.TestStep{
Config: testAccAwsSecurityGroupAttachmentConfig(tc.External, true),
Check: checkSecurityGroupAttachment(tc.External, true),
},
resource.TestStep{
Config: testAccAwsSecurityGroupAttachmentConfig(tc.External, false),
Check: checkSecurityGroupAttachment(tc.External, false),
},
},
})
})
}
}

func testAccAwsSecurityGroupAttachmentConfig(external bool, attach bool) string {
baseConfig := `
data "aws_ami" "ami" {
most_recent = true

filter {
name = "name"
values = ["amzn-ami-hvm-*"]
}

owners = ["amazon"]
}

resource "aws_instance" "instance" {
instance_type = "t2.micro"
ami = "${data.aws_ami.ami.id}"

tags = {
"type" = "terraform-test-instance"
}
}

data "aws_instance" "external_instance" {
instance_id = "${aws_instance.instance.id}"
}

resource "aws_security_group" "sg" {
tags = {
"type" = "terraform-test-security-group"
}
}

`
optionalConfig := `
resource "aws_security_group_attachment" "sg_attachment" {
security_group_id = "${aws_security_group.sg.id}"
network_interface_id = "${%saws_instance.%sinstance.%snetwork_interface_id}"
}
`

if attach {
externalResPre := ""
externalDataPre := ""
externalAttrPre := "primary_"
if external {
externalResPre = "data."
externalDataPre = "external_"
externalAttrPre = ""
}
return baseConfig + fmt.Sprintf(optionalConfig, externalResPre, externalDataPre, externalAttrPre)
}
return baseConfig
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took me a while to understand the logic here (+ imagine what exactly is the sprintf going to do) and what's the reason for the condition.

It may be just me, but I personally think for a test case this logic is overly complicated - having two, nearly identical tests with similar configs would IMO work better. Duplication isn't always bad. 😉

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And after all the two use cases are sufficiently different (data source vs resource) that we should test them separately anyway, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

De-coupled the configuration generation functions for the most part... I still parameterize on enabling the attachment so that we can test removal on the second step, but things should hopefully be a lot more clear now. Let me know if it isn't though. 🙂


func checkSecurityGroupAttachment(external bool, expected bool) resource.TestCheckFunc {
return func(s *terraform.State) error {
conn := testAccProvider.Meta().(*AWSClient).ec2conn

ifAttr := "primary_network_interface_id"
if external {
ifAttr = "network_interface_id"
}
interfaceID := s.Modules[0].Resources["aws_instance.instance"].Primary.Attributes[ifAttr]
sgID := s.Modules[0].Resources["aws_security_group.sg"].Primary.ID

iface, err := fetchNetworkInterface(conn, interfaceID)
if err != nil {
return err
}
actual := sgExistsInENI(sgID, iface)
if expected != actual {
return fmt.Errorf("expected existence of security group in ENI to be %t, got %t", expected, actual)
}
return nil
}
}
6 changes: 5 additions & 1 deletion website/aws.erb
Original file line number Diff line number Diff line change
Expand Up @@ -1349,7 +1349,7 @@
</li>


<li<%= sidebar_current("docs-aws-resource-(default|customer|egress-only-internet-gateway|flow|internet-gateway|main-route|network|route-|security-group|subnet|vpc|vpn)") %>>
<li<%= sidebar_current("docs-aws-resource-(default|customer|egress-only-internet-gateway|flow|internet-gateway|main-route|network|route-|security-group|security-group-attachment|subnet|vpc|vpn)") %>>
<a href="#">VPC Resources</a>
<ul class="nav nav-visible">

Expand Down Expand Up @@ -1431,6 +1431,10 @@
<a href="/docs/providers/aws/r/security_group.html">aws_security_group</a>
</li>

<li<%= sidebar_current("docs-aws-resource-security-group-attachment") %>>
<a href="/docs/providers/aws/r/security_group_attachment.html">aws_security_group_attachment</a>
</li>

<li<%= sidebar_current("docs-aws-resource-security-group-rule") %>>
<a href="/docs/providers/aws/r/security_group_rule.html">aws_security_group_rule</a>
</li>
Expand Down
Loading