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.

Sign up for GitHub

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

resource/instance: add secondary_private_ips field #14079

Merged
merged 3 commits into from
Jul 15, 2020
Merged

resource/instance: add secondary_private_ips field #14079

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4b09012
add private_ips field
anGie44 Jul 7, 2020
d9c3513
change private_ips to secondary_private_ips and enable update
anGie44 Jul 8, 2020
d245c1a
update to using expandstringset method
anGie44 Jul 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions aws/data_source_aws_instance.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,11 @@ func dataSourceAwsInstance() *schema.Resource {
Type: schema.TypeString,
Computed: true,
},
"secondary_private_ips": {
Type: schema.TypeSet,
Computed: true,
Elem: &schema.Schema{Type: schema.TypeString},
},
"iam_instance_profile": {
Type: schema.TypeString,
Computed: true,
Expand Down Expand Up @@ -436,6 +441,16 @@ func instanceDescriptionAttributes(d *schema.ResourceData, instance *ec2.Instanc
d.Set("subnet_id", ni.SubnetId)
d.Set("network_interface_id", ni.NetworkInterfaceId)
d.Set("associate_public_ip_address", ni.Association != nil)

secondaryIPs := make([]string, 0, len(ni.PrivateIpAddresses))
for _, ip := range ni.PrivateIpAddresses {
if !aws.BoolValue(ip.Primary) {
secondaryIPs = append(secondaryIPs, aws.StringValue(ip.PrivateIpAddress))
}
}
if err := d.Set("secondary_private_ips", secondaryIPs); err != nil {
return fmt.Errorf("error setting secondary_private_ips: %w", err)
}
}
}
} else {
Expand Down
36 changes: 36 additions & 0 deletions aws/data_source_aws_instance_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,27 @@ func TestAccAWSInstanceDataSource_privateIP(t *testing.T) {
})
}

func TestAccAWSInstanceDataSource_secondaryPrivateIPs(t *testing.T) {
resourceName := "aws_instance.test"
datasourceName := "data.aws_instance.test"
rName := fmt.Sprintf("tf-testacc-instance-%s", acctest.RandStringFromCharSet(12, acctest.CharSetAlphaNum))

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
Steps: []resource.TestStep{
{
Config: testAccInstanceDataSourceConfig_secondaryPrivateIPs(rName),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttrPair(datasourceName, "ami", resourceName, "ami"),
resource.TestCheckResourceAttrPair(datasourceName, "instance_type", resourceName, "instance_type"),
resource.TestCheckResourceAttrPair(datasourceName, "secondary_private_ips", resourceName, "secondary_private_ips"),
),
},
},
})
}

func TestAccAWSInstanceDataSource_keyPair(t *testing.T) {
resourceName := "aws_instance.test"
datasourceName := "data.aws_instance.test"
Expand Down Expand Up @@ -665,6 +686,21 @@ data "aws_instance" "test" {
`)
}

func testAccInstanceDataSourceConfig_secondaryPrivateIPs(rName string) string {
return testAccLatestAmazonLinuxHvmEbsAmiConfig() + testAccAwsInstanceVpcConfig(rName, false) + fmt.Sprintf(`
resource "aws_instance" "test" {
ami = "${data.aws_ami.amzn-ami-minimal-hvm-ebs.id}"
instance_type = "t2.micro"
subnet_id = "${aws_subnet.test.id}"
secondary_private_ips = ["10.1.1.42"]
}

data "aws_instance" "test" {
instance_id = "${aws_instance.test.id}"
}
`)
}

func testAccInstanceDataSourceConfig_keyPair(rName string) string {
return testAccLatestAmazonLinuxHvmEbsAmiConfig() + fmt.Sprintf(`
resource "aws_key_pair" "test" {
Expand Down
138 changes: 110 additions & 28 deletions aws/resource_aws_instance.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,16 @@ func resourceAwsInstance() *schema.Resource {
),
},

"secondary_private_ips": {
Type: schema.TypeSet,
Copy link
Contributor

@bflad bflad Jul 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Does this need to be TypeList to support the "primary" private IP being the first element? I believe the concept of the primary private IP is important for DNS hostnames and allowing secondary private IPs to be re-assigned. Not sure about the API response semantics of ordering once it is configured.

Copy link
Contributor Author

@anGie44 anGie44 Jul 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

ah yes thats more sense! i took the idea from the network_interface resource which uses a TypeSet as well but didn't think about the response ordering. Is it still ok to have the private_ip and private_ips conflict as they are set right now? or would it be better to introduce the breaking change and use the ordering of a list to determine primary vs. secondary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh but then we'd have to still support the empty string in the list...hmm id side with having the 2 fields instead

Copy link
Contributor Author

@anGie44 anGie44 Jul 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

thinking about this more, in d9c3513 i've enabled the use of private_ip with the renamed secondary_private_ips to be more specific and not make things more confusing with IPs specified in the set vs. that provided in private_ip. lmk if it makes more sense then having the 2 conflicting fields

Optional: true,
Computed: true,
Elem: &schema.Schema{
Type: schema.TypeString,
ValidateFunc: validation.IsIPv4Address,
},
},

"source_dest_check": {
Type: schema.TypeBool,
Optional: true,
Expand Down Expand Up @@ -206,7 +216,7 @@ func resourceAwsInstance() *schema.Resource {
},

"network_interface": {
ConflictsWith: []string{"associate_public_ip_address", "subnet_id", "private_ip", "vpc_security_group_ids", "security_groups", "ipv6_addresses", "ipv6_address_count", "source_dest_check"},
ConflictsWith: []string{"associate_public_ip_address", "subnet_id", "private_ip", "secondary_private_ips", "vpc_security_group_ids", "security_groups", "ipv6_addresses", "ipv6_address_count", "source_dest_check"},
Type: schema.TypeSet,
Optional: true,
Computed: true,
Expand Down Expand Up @@ -807,6 +817,7 @@ func resourceAwsInstanceRead(d *schema.ResourceData, meta interface{}) error {
}
}

var secondaryPrivateIPs []string
var ipv6Addresses []string
if len(instance.NetworkInterfaces) > 0 {
var primaryNetworkInterface ec2.InstanceNetworkInterface
Expand Down Expand Up @@ -851,6 +862,12 @@ func resourceAwsInstanceRead(d *schema.ResourceData, meta interface{}) error {

d.Set("associate_public_ip_address", primaryNetworkInterface.Association != nil)

for _, address := range primaryNetworkInterface.PrivateIpAddresses {
if !aws.BoolValue(address.Primary) {
secondaryPrivateIPs = append(secondaryPrivateIPs, aws.StringValue(address.PrivateIpAddress))
}
}

for _, address := range primaryNetworkInterface.Ipv6Addresses {
ipv6Addresses = append(ipv6Addresses, aws.StringValue(address.Ipv6Address))
}
Expand All @@ -862,6 +879,10 @@ func resourceAwsInstanceRead(d *schema.ResourceData, meta interface{}) error {
d.Set("subnet_id", instance.SubnetId)
}

if err := d.Set("secondary_private_ips", secondaryPrivateIPs); err != nil {
return fmt.Errorf("Error setting private_ips for AWS Instance (%s): %w", d.Id(), err)
}

if err := d.Set("ipv6_addresses", ipv6Addresses); err != nil {
log.Printf("[WARN] Error setting ipv6_addresses for AWS Instance (%s): %s", d.Id(), err)
}
Expand Down Expand Up @@ -1109,23 +1130,7 @@ func resourceAwsInstanceUpdate(d *schema.ResourceData, meta interface{}) error {
}
}

if d.HasChange("vpc_security_group_ids") && !d.IsNewResource() {
var groups []*string
if v := d.Get("vpc_security_group_ids").(*schema.Set); v.Len() > 0 {
for _, v := range v.List() {
groups = append(groups, aws.String(v.(string)))
}
}

if len(groups) < 1 {
return fmt.Errorf("VPC-based instances require at least one security group to be attached.")
}
// If a user has multiple network interface attachments on the target EC2 instance, simply modifying the
// instance attributes via a `ModifyInstanceAttributes()` request would fail with the following error message:
// "There are multiple interfaces attached to instance 'i-XX'. Please specify an interface ID for the operation instead."
// Thus, we need to actually modify the primary network interface for the new security groups, as the primary
// network interface is where we modify/create security group assignments during Create.
log.Printf("[INFO] Modifying `vpc_security_group_ids` on Instance %q", d.Id())
if d.HasChanges("secondary_private_ips", "vpc_security_group_ids") && !d.IsNewResource() {
instance, err := resourceAwsInstanceFindByID(conn, d.Id())
if err != nil {
return fmt.Errorf("error retrieving instance %q: %w", d.Id(), err)
Expand All @@ -1137,18 +1142,78 @@ func resourceAwsInstanceUpdate(d *schema.ResourceData, meta interface{}) error {
}
}

if primaryInterface.NetworkInterfaceId == nil {
log.Print("[Error] Attempted to set vpc_security_group_ids on an instance without a primary network interface")
return fmt.Errorf(
"Failed to update vpc_security_group_ids on %q, which does not contain a primary network interface",
d.Id())
if d.HasChange("secondary_private_ips") {
if primaryInterface.NetworkInterfaceId == nil {
return fmt.Errorf("Failed to update secondary_private_ips on %q, which does not contain a primary network interface",
d.Id())
}
o, n := d.GetChange("secondary_private_ips")
if o == nil {
o = new(schema.Set)
}
if n == nil {
n = new(schema.Set)
}

os := o.(*schema.Set)
ns := n.(*schema.Set)

// Unassign old IP addresses
unassignIps := os.Difference(ns)
if unassignIps.Len() != 0 {
input := &ec2.UnassignPrivateIpAddressesInput{
NetworkInterfaceId: primaryInterface.NetworkInterfaceId,
PrivateIpAddresses: expandStringList(unassignIps.List()),
anGie44 marked this conversation as resolved.
Show resolved Hide resolved
}
log.Printf("[INFO] Unassigning secondary_private_ips on Instance %q", d.Id())
_, err := conn.UnassignPrivateIpAddresses(input)
if err != nil {
return fmt.Errorf("Failure to unassign Secondary Private IPs: %w", err)
}
}

// Assign new IP addresses
assignIps := ns.Difference(os)
if assignIps.Len() != 0 {
input := &ec2.AssignPrivateIpAddressesInput{
NetworkInterfaceId: primaryInterface.NetworkInterfaceId,
PrivateIpAddresses: expandStringList(assignIps.List()),
anGie44 marked this conversation as resolved.
Show resolved Hide resolved
}
log.Printf("[INFO] Assigning secondary_private_ips on Instance %q", d.Id())
_, err := conn.AssignPrivateIpAddresses(input)
if err != nil {
return fmt.Errorf("Failure to assign Secondary Private IPs: %w", err)
}
}
}

if _, err := conn.ModifyNetworkInterfaceAttribute(&ec2.ModifyNetworkInterfaceAttributeInput{
NetworkInterfaceId: primaryInterface.NetworkInterfaceId,
Groups: groups,
}); err != nil {
return err
if d.HasChange("vpc_security_group_ids") {
if primaryInterface.NetworkInterfaceId == nil {
return fmt.Errorf("Failed to update vpc_security_group_ids on %q, which does not contain a primary network interface",
d.Id())
}
var groups []*string
if v := d.Get("vpc_security_group_ids").(*schema.Set); v.Len() > 0 {
for _, v := range v.List() {
groups = append(groups, aws.String(v.(string)))
}
}

if len(groups) < 1 {
return fmt.Errorf("VPC-based instances require at least one security group to be attached.")
}
// If a user has multiple network interface attachments on the target EC2 instance, simply modifying the
// instance attributes via a `ModifyInstanceAttributes()` request would fail with the following error message:
// "There are multiple interfaces attached to instance 'i-XX'. Please specify an interface ID for the operation instead."
// Thus, we need to actually modify the primary network interface for the new security groups, as the primary
// network interface is where we modify/create security group assignments during Create.
log.Printf("[INFO] Modifying `vpc_security_group_ids` on Instance %q", d.Id())
if _, err := conn.ModifyNetworkInterfaceAttribute(&ec2.ModifyNetworkInterfaceAttributeInput{
NetworkInterfaceId: primaryInterface.NetworkInterfaceId,
Groups: groups,
}); err != nil {
return err
}
}
}

Expand Down Expand Up @@ -1687,6 +1752,10 @@ func buildNetworkInterfaceOpts(d *schema.ResourceData, groups []*string, nInterf
ni.PrivateIpAddress = aws.String(v.(string))
}

if v, ok := d.GetOk("secondary_private_ips"); ok && v.(*schema.Set).Len() > 0 {
ni.PrivateIpAddresses = expandSecondaryPrivateIPAddresses(v.(*schema.Set).List())
}

if v, ok := d.GetOk("ipv6_address_count"); ok {
ni.Ipv6AddressCount = aws.Int64(int64(v.(int)))
}
Expand Down Expand Up @@ -2312,6 +2381,19 @@ func expandEc2InstanceMetadataOptions(l []interface{}) *ec2.InstanceMetadataOpti
return opts
}

//Expands an array of secondary Private IPs into a ec2 Private IP Address Spec
func expandSecondaryPrivateIPAddresses(ips []interface{}) []*ec2.PrivateIpAddressSpecification {
specs := make([]*ec2.PrivateIpAddressSpecification, 0, len(ips))
for _, v := range ips {
spec := &ec2.PrivateIpAddressSpecification{
PrivateIpAddress: aws.String(v.(string)),
}

specs = append(specs, spec)
}
return specs
}

func flattenEc2InstanceMetadataOptions(opts *ec2.InstanceMetadataOptionsResponse) []interface{} {
if opts == nil {
return nil
Expand Down
Loading