-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
Conversation
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?) |
Currently awaiting maintainer review. |
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? |
|
Hello, |
Should this PR also mark #3007 as closed? |
@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. |
is there any timelines for this feature? |
This improvement would be really welcome here :) |
Hi @kadenlnelson , unfortunately, your last commit is not successful. |
The CI job is not particularly forthcoming w/ the root cause of the failure. Is it because the |
…ivate-ipv4-address
There was a problem hiding this 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, |
There was a problem hiding this comment.
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), |
There was a problem hiding this comment.
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"), | ||
), | ||
}, |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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]}"
also Closes #11674 |
There was a problem hiding this 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.
@kadenlnelson Are you able to make the requested changes? |
Any progress on this? Is there an approximate time schedule indicating when to expect the merge? We would appreciate this solution. |
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. |
There was a problem hiding this 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)
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) ```
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! |
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:
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. |
@TimJDFletcher yes please create a new bug report. Thanks. |
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! |
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 anaws_lb
like the example below. I've verified that the subnet mapping in the aws console shows the private ipv4 address.steps to test;
Community Note
fixes #11403
fixes #11044
Release note for CHANGELOG:
Output from acceptance testing: