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

add support for private ipv4 addresses in subnet mapping of ELB V2 #11404

Merged
merged 6 commits into from Jul 31, 2020
Merged

add support for private ipv4 addresses in subnet mapping of ELB V2 #11404

merged 6 commits into from Jul 31, 2020

Conversation

ghost
Copy link

@ghost ghost commented Dec 22, 2019

Please let me know if there's anything else I should update, this is my first PR to terraform-provider-aws. I've tested this feature by creating an aws_lb like the example below. I've verified that the subnet mapping in the aws console shows the private ipv4 address.

steps to test;

provider "aws" {
  region  = "us-east-1"
  profile = "default"
}

resource "aws_vpc" "test" {
  cidr_block = "10.0.0.0/20"

  tags = {
    Name = "test-vpc"
  }
}

resource "aws_subnet" "test" {
  vpc_id     = "${aws_vpc.test.id}"
  cidr_block = "10.0.1.0/24"

  tags = {
    Name = "test-subnet"
  }
}

resource "aws_lb" "test" {
  name               = "test-lb-tf"
  internal           = true
  load_balancer_type = "network"
  subnet_mapping {
    subnet_id            = aws_subnet.test.id
    private_ipv4_address = "10.0.1.15"
  }
}

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

fixes #11403
fixes #11044

Release note for CHANGELOG:

add support for private ipv4 address in subnet mappings of resource aws_lb

Output from acceptance testing:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSLB_NLB_privateipv4address'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSLB_NLB_privateipv4address -timeout 120m
=== RUN   TestAccAWSLB_NLB_privateipv4address
=== PAUSE TestAccAWSLB_NLB_privateipv4address
=== CONT  TestAccAWSLB_NLB_privateipv4address
--- PASS: TestAccAWSLB_NLB_privateipv4address (228.60s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       228.635s

@ghost ghost self-requested a review December 22, 2019 02:37
@ghost ghost added size/XS Managed by automation to categorize the size of a PR. needs-triage Waiting for first response or review from a maintainer. service/elbv2 Issues and PRs that pertain to the elbv2 service. labels Dec 22, 2019
@ewbankkit
Copy link
Contributor

ewbankkit commented Dec 22, 2019

Closes #11044.
Closes #11887.
Closes #11403.

@ghost ghost added documentation Introduces or discusses updates to documentation. size/S Managed by automation to categorize the size of a PR. and removed size/XS Managed by automation to categorize the size of a PR. labels Dec 25, 2019
@bflad bflad added the enhancement Requests to existing resources that expand the functionality or scope. label Jan 2, 2020
@cSNiHab
Copy link

cSNiHab commented Jan 21, 2020

Hi, we're currently deciding if we should wait for this PR to be merged or if we should implement a null_resource workaround to get private static IPs in.

Could you give us an indication if this PR will be merged? (and if so, are there any timelines?)

@ghost
Copy link
Author

ghost commented Jan 25, 2020

Currently awaiting maintainer review.

@TimJDFletcher
Copy link

We have just run in this problem during a roll out of access to our on-premise resources from AWS.

Any news from the code review on this?

@ghost
Copy link
Author

ghost commented Mar 21, 2020

We have just run in this problem during a roll out of access to our on-premise resources from AWS.

Any news from the code review on this?

@bflad

@florianlocqueneux
Copy link

florianlocqueneux commented Apr 1, 2020

Hello,
Any chance to get this reviewed ?
It could be really helpfull to get this merged.

@bmathieu33
Copy link

Should this PR also mark #3007 as closed?

@ewbankkit
Copy link
Contributor

@bmathieu33 It looks like #3007 is more related to #2901 / #11000 but given the decision on that solution, yes it should probably be closed (it does contain a lot of great workarounds but will still be searchable). Thanks.

@mohamed-rekiba
Copy link

is there any timelines for this feature?

@florianlocqueneux
Copy link

This improvement would be really welcome here :)
Any visibility on the timeline would help as well.
Thank you so much

@florianlocqueneux
Copy link

Hi @kadenlnelson , unfortunately, your last commit is not successful.

@dthvt
Copy link
Contributor

dthvt commented May 12, 2020

The CI job is not particularly forthcoming w/ the root cause of the failure. Is it because the = signs aren't aligned in the documentation HCL snippet? That's about the only thing I can see that looks like it might be a lint failure.

Copy link
Collaborator

@DrFaust92 DrFaust92 left a comment

Choose a reason for hiding this comment

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

Hey @kadenlnelson,
thanks for submitting this, a few comments to shorten the review cycle by mainteners when they get to this

"private_ipv4_address": {
Type: schema.TypeString,
Optional: true,
ForceNew: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

add validation here ValidateFunc: validation.IsIPv4Address,

{
Config: testAccAWSLBConfig_networkLoadBalancerPrivateIPV4Address(lbName),
Check: resource.ComposeAggregateTestCheckFunc(
testAccCheckAWSLBExists(resourceName, &conf),
Copy link
Collaborator

Choose a reason for hiding this comment

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

most of these check are irrelevant to the specific case, lets leave only the relevant check

resource.TestCheckResourceAttr(resourceName, "internal", "true"),
resource.TestCheckResourceAttr(resourceName, "load_balancer_type", "network"),
resource.TestCheckResourceAttr(resourceName, "subnet_mapping.#", "1"),

resource.TestCheckResourceAttrSet(resourceName, "zone_id"),
resource.TestCheckResourceAttr(resourceName, "subnet_mapping.#", "1"),
),
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

add import step:

			{
				ResourceName:      resourceName,
				ImportState:       true,
				ImportStateVerify: true,
			},

vpc_id = "${aws_vpc.alb_test.id}"
cidr_block = "10.10.0.0/21"
map_public_ip_on_launch = true
availability_zone = "us-west-2a"
Copy link
Collaborator

Choose a reason for hiding this comment

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

to make test region agnostic and to avoid environment specific failures, can please add the following to the test config:

data "aws_availability_zones" "available" {
  state = "available"

  filter {
    name   = "opt-in-status"
    values = ["opt-in-not-required"]
  }
}

and change the hardcoded az to "${data.aws_availability_zones.available.names[0]}"

@DrFaust92
Copy link
Collaborator

also Closes #11674

Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Hi @kadenlnelson 👋 Thank you for submitting this and apologies for the long review cycle. Overall this is looking really good, once you address @DrFaust92's review items we should be able to get this in. Please reach out with any questions or if you do not have time to implement them.

@bflad bflad added this to the v2.68.0 milestone Jun 24, 2020
@breathingdust breathingdust modified the milestones: v2.68.0, v2.69.0 Jun 25, 2020
@breathingdust breathingdust modified the milestones: v2.69.0, v2.70.0, v3.1.0 Jul 2, 2020
@abohne
Copy link

abohne commented Jul 10, 2020

@kadenlnelson Are you able to make the requested changes?

@Floble
Copy link

Floble commented Jul 16, 2020

Any progress on this? Is there an approximate time schedule indicating when to expect the merge? We would appreciate this solution.

@bflad
Copy link
Contributor

bflad commented Jul 16, 2020

The maintainers are actively working on the 3.0 major release for the next week or two, then this is up for final review. If we do not see updates from @kadenlnelson to adjust the requested changes in the meantime, we will make the changes on top of this contribution to merge it in for release in 3.1.

Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

After fixing the merge conflict and addressing @DrFaust92's feedback (thanks!), looks good to me 🚀

Output from acceptance testing:

--- PASS: TestAccAWSLB_ALB_AccessLogs (336.66s)
--- PASS: TestAccAWSLB_ALB_AccessLogs_Prefix (303.22s)
--- PASS: TestAccAWSLB_ALB_basic (213.75s)
--- PASS: TestAccAWSLB_applicationLoadBalancer_updateDeletionProtection (290.62s)
--- PASS: TestAccAWSLB_applicationLoadBalancer_updateDropInvalidHeaderFields (287.40s)
--- PASS: TestAccAWSLB_applicationLoadBalancer_updateHttp2 (318.53s)
--- PASS: TestAccAWSLB_BackwardsCompatibility (220.15s)
--- PASS: TestAccAWSLB_generatedName (203.47s)
--- PASS: TestAccAWSLB_generatesNameForZeroValue (205.38s)
--- PASS: TestAccAWSLB_namePrefix (193.39s)
--- PASS: TestAccAWSLB_networkLoadbalancer_subnet_change (260.37s)
--- PASS: TestAccAWSLB_networkLoadbalancer_updateCrossZone (323.38s)
--- PASS: TestAccAWSLB_networkLoadbalancerEIP (269.79s)
--- PASS: TestAccAWSLB_NLB_AccessLogs (414.90s)
--- PASS: TestAccAWSLB_NLB_AccessLogs_Prefix (380.57s)
--- PASS: TestAccAWSLB_NLB_basic (256.98s)
--- PASS: TestAccAWSLB_NLB_privateipv4address (252.14s)
--- PASS: TestAccAWSLB_noSecurityGroup (275.40s)
--- PASS: TestAccAWSLB_tags (274.26s)
--- PASS: TestAccAWSLB_updatedIpAddressType (234.58s)
--- PASS: TestAccAWSLB_updatedSecurityGroups (233.02s)
--- PASS: TestAccAWSLB_updatedSubnets (263.78s)

bflad added a commit that referenced this pull request Jul 31, 2020
Reference: #11404 (review)

Output from acceptance testing:

```
--- PASS: TestAccAWSLB_ALB_AccessLogs (336.66s)
--- PASS: TestAccAWSLB_ALB_AccessLogs_Prefix (303.22s)
--- PASS: TestAccAWSLB_ALB_basic (213.75s)
--- PASS: TestAccAWSLB_applicationLoadBalancer_updateDeletionProtection (290.62s)
--- PASS: TestAccAWSLB_applicationLoadBalancer_updateDropInvalidHeaderFields (287.40s)
--- PASS: TestAccAWSLB_applicationLoadBalancer_updateHttp2 (318.53s)
--- PASS: TestAccAWSLB_BackwardsCompatibility (220.15s)
--- PASS: TestAccAWSLB_generatedName (203.47s)
--- PASS: TestAccAWSLB_generatesNameForZeroValue (205.38s)
--- PASS: TestAccAWSLB_namePrefix (193.39s)
--- PASS: TestAccAWSLB_networkLoadbalancer_subnet_change (260.37s)
--- PASS: TestAccAWSLB_networkLoadbalancer_updateCrossZone (323.38s)
--- PASS: TestAccAWSLB_networkLoadbalancerEIP (269.79s)
--- PASS: TestAccAWSLB_NLB_AccessLogs (414.90s)
--- PASS: TestAccAWSLB_NLB_AccessLogs_Prefix (380.57s)
--- PASS: TestAccAWSLB_NLB_basic (256.98s)
--- PASS: TestAccAWSLB_NLB_privateipv4address (252.14s)
--- PASS: TestAccAWSLB_noSecurityGroup (275.40s)
--- PASS: TestAccAWSLB_tags (274.26s)
--- PASS: TestAccAWSLB_updatedIpAddressType (234.58s)
--- PASS: TestAccAWSLB_updatedSecurityGroups (233.02s)
--- PASS: TestAccAWSLB_updatedSubnets (263.78s)
```
bflad added a commit that referenced this pull request Jul 31, 2020
@bflad bflad merged commit 2a71545 into hashicorp:master Jul 31, 2020
@ghost
Copy link

ghost commented Aug 7, 2020

This has been released in version 3.1.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

@TimJDFletcher
Copy link

TimJDFletcher commented Aug 10, 2020

We have just upgraded to the aws provider version 3.1 to try and use this functionality.

We are now getting the following errors at the plan stage:

Error: error setting subnet_mapping: Invalid address to set: []string{"subnet_mapping", "0", "private_ipv4_address"}

I believe it could be related to this type of bug: #4986

Should I open a new bug for this?

Edit thinking about this a little more I wonder if this only impacts NLBs with pinned IPs?

We create the NLBs via the aws cli and then use a data lookup to find the ARN to configure them.

@bflad
Copy link
Contributor

bflad commented Aug 10, 2020

@TimJDFletcher yes please create a new bug report. Thanks.

@ghost
Copy link

ghost commented Aug 30, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Aug 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. enhancement Requests to existing resources that expand the functionality or scope. service/elbv2 Issues and PRs that pertain to the elbv2 service. size/M Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet