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 1 commit
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
28 changes: 27 additions & 1 deletion aws/resource_aws_instance.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,19 @@ func resourceAwsInstance() *schema.Resource {
validation.StringIsEmpty,
validation.IsIPv4Address,
),
ConflictsWith: []string{"private_ips"},
},

"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,
ForceNew: true,
Computed: true,
Elem: &schema.Schema{
Type: schema.TypeString,
ValidateFunc: validation.IsIPv4Address,
},
ConflictsWith: []string{"private_ip"},
},

"source_dest_check": {
Expand Down Expand Up @@ -206,7 +219,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", "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 +820,7 @@ func resourceAwsInstanceRead(d *schema.ResourceData, meta interface{}) error {
}
}

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

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

for _, address := range primaryNetworkInterface.PrivateIpAddresses {
privateIPs = append(privateIPs, aws.StringValue(address.PrivateIpAddress))
}

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

if err := d.Set("private_ips", privateIPs); 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 @@ -1687,6 +1709,10 @@ func buildNetworkInterfaceOpts(d *schema.ResourceData, groups []*string, nInterf
ni.PrivateIpAddress = aws.String(v.(string))
}

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

if v, ok := d.GetOk("ipv6_address_count"); ok {
ni.Ipv6AddressCount = aws.Int64(int64(v.(int)))
}
Expand Down
66 changes: 66 additions & 0 deletions aws/resource_aws_instance_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2014,6 +2014,44 @@ func TestAccAWSInstance_addSecurityGroupNetworkInterface(t *testing.T) {
})
}

// Reference: https://github.com/terraform-providers/terraform-provider-aws/issues/7063
func TestAccAWSInstance_newNetworkInterfacePrivateIPs(t *testing.T) {
var v ec2.Instance
resourceName := "aws_instance.test"
rName := acctest.RandomWithPrefix("tf-acc-test")

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckInstanceDestroy,
Steps: []resource.TestStep{
{
Config: testAccInstanceConfigPublicAndPrivateIPs(rName, true),
Check: resource.ComposeTestCheckFunc(
testAccCheckInstanceExists(resourceName, &v),
resource.TestCheckResourceAttr(resourceName, "associate_public_ip_address", "true"),
resource.TestCheckResourceAttrSet(resourceName, "public_ip"),
resource.TestCheckResourceAttr(resourceName, "private_ips.#", "2"),
),
},
{
Config: testAccInstanceConfigPublicAndPrivateIPs(rName, false),
Check: resource.ComposeTestCheckFunc(
testAccCheckInstanceExists(resourceName, &v),
resource.TestCheckResourceAttr(resourceName, "associate_public_ip_address", "false"),
resource.TestCheckResourceAttr(resourceName, "public_ip", ""),
resource.TestCheckResourceAttr(resourceName, "private_ips.#", "2"),
),
},
{
ResourceName: resourceName,
ImportState: true,
ImportStateVerify: true,
},
},
})
}

// https://github.com/terraform-providers/terraform-provider-aws/issues/227
func TestAccAWSInstance_associatePublic_defaultPrivate(t *testing.T) {
var v ec2.Instance
Expand Down Expand Up @@ -4280,6 +4318,34 @@ resource "aws_network_interface" "test" {
`, rName)
}

func testAccInstanceConfigPublicAndPrivateIPs(rName string, isPublic bool) string {
return testAccLatestAmazonLinuxHvmEbsAmiConfig() + testAccAwsInstanceVpcConfig(rName, false) + fmt.Sprintf(`
resource "aws_security_group" "test" {
vpc_id = "${aws_vpc.test.id}"
description = "%[1]s"
name = "%[1]s"
}

resource "aws_instance" "test" {
ami = "${data.aws_ami.amzn-ami-minimal-hvm-ebs.id}"
instance_type = "t2.micro"
subnet_id = "${aws_subnet.test.id}"

associate_public_ip_address = %[2]t

private_ips = ["10.1.1.42", "10.1.1.43"]

vpc_security_group_ids = [
"${aws_security_group.test.id}"
]

tags = {
Name = %[1]q
}
}
`, rName, isPublic)
}

func testAccInstanceConfig_associatePublic_defaultPrivate(rName string) string {
return testAccLatestAmazonLinuxHvmEbsAmiConfig() + testAccAwsInstanceVpcConfig(rName, false) + fmt.Sprintf(`
resource "aws_instance" "test" {
Expand Down
1 change: 1 addition & 0 deletions website/docs/r/instance.html.markdown
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ instances. See [Shutdown Behavior](https://docs.aws.amazon.com/AWSEC2/latest/Use
* `associate_public_ip_address` - (Optional) Associate a public ip address with an instance in a VPC. Boolean value.
* `private_ip` - (Optional) Private IP address to associate with the
instance in a VPC.
* `private_ips` - (Optional, Forces new resource) A list of private IPv4 addresses to assign to the instance's associated network interface. Can only be assigned to a new network interface, not an existing one i.e. set in a `network_interface` block. By default, the first entry in the list is designated as `primary`.
* `source_dest_check` - (Optional) Controls if traffic is routed to the instance when
the destination address does not match the instance. Used for NAT or VPNs. Defaults true.
* `user_data` - (Optional) The user data to provide when launching the instance. Do not pass gzip-compressed data via this argument; see `user_data_base64` instead.
Expand Down