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

Supporting EIPs with specified BYOIP addresses #8876

Merged
merged 10 commits into from
Apr 28, 2021

Conversation

arwilczek90
Copy link
Contributor

@arwilczek90 arwilczek90 commented Jun 5, 2019

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" comments, they generate extra noise for pull request followers and do not help prioritize the request

Fixes #8004

Release note for CHANGELOG:

ENHANCEMENTS

* Added support for EIPs with a specified BYOIP address

Output from acceptance testing:

$  make testacc TEST=./aws TESTARGS='-run=TestAccAWSEIP_BYOIPAddress'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSEIP_BYOIPAddress -timeout 120m
=== RUN   TestAccAWSEIP_BYOIPAddress_default
=== PAUSE TestAccAWSEIP_BYOIPAddress_default
=== RUN   TestAccAWSEIP_BYOIPAddress_custom
--- PASS: TestAccAWSEIP_BYOIPAddress_custom (23.06s)
=== RUN   TestAccAWSEIP_BYOIPAddress_custom_with_PublicIpv4Pool
--- PASS: TestAccAWSEIP_BYOIPAddress_custom_with_PublicIpv4Pool (21.54s)
=== CONT  TestAccAWSEIP_BYOIPAddress_default
--- PASS: TestAccAWSEIP_BYOIPAddress_default (22.48s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       67.099s

@ghost ghost added size/M Managed by automation to categorize the size of a PR. service/ec2 Issues and PRs that pertain to the ec2 service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. documentation Introduces or discusses updates to documentation. labels Jun 5, 2019
@arwilczek90 arwilczek90 changed the title [WIP] Supporting EIPs with specified BYOIP addresses Supporting EIPs with specified BYOIP addresses Jun 10, 2019
@arwilczek90
Copy link
Contributor Author

This has been tested with my company's IP block.

@ravenbyron
Copy link

I have tested this and it makes my workflow 10000% easier!!! Thanks

@bflad bflad added the enhancement Requests to existing resources that expand the functionality or scope. label Jun 10, 2019
@aeschright aeschright requested a review from a team June 28, 2019 18:14
@ravenbyron
Copy link

Any idea when this may get reviewed/merged?

@arwilczek90
Copy link
Contributor Author

any idea when we can see this reviewed?

@hsmithatemma
Copy link

This is a great idea!

@stobias123
Copy link

Bump - really like this feature

@z0ph
Copy link

z0ph commented Nov 12, 2019

This will be really useful for us. Thanks

@robzr
Copy link

robzr commented Dec 3, 2019

Please consider adding this!

@aaronmell
Copy link

Why hasn't this been merged?

@arwilczek90
Copy link
Contributor Author

I'm waiting on code review, I don't know why it hasn't gotten any review though.

@aaronmell
Copy link

@arwilczek90 Sorry, that wasn't directed at you. I've faced a similar issue with a PR in a different provider and I was venting a little bit in that regard.

@arwilczek90
Copy link
Contributor Author

@aaronmell No worries 😃

@mnovacyu
Copy link

Would greatly appreciate having this! As an alternative, we've had to create a null_resource which calls a custom python script. When can this be reviewed?

@ravenbyron
Copy link

Can we please get an update on this !!!!

@gyoza
Copy link

gyoza commented Jun 29, 2020

Any chance that this will ever get added?

@hsmithatemma
Copy link

Any updates on this front? We just tripped over the 1year mark.

@ravenbyron
Copy link

@bflad any chance we can get a review and merge on this. If you need to test I have access to a dev env that has a set of BYOIPs that I could share. (I am pinging you as you merged a similar pr for outpost)

@midN
Copy link

midN commented Dec 16, 2020

I guess it's time to migrate to CDK?
Terraform is now reaching that CloudFormation threshold with features lagging behing over a year

@ravenbyron
Copy link

Any chance for any feed back on this? cc: @bflad

@gwvandesteeg
Copy link

This looks good, code diff looks good, just needs an appropriate person to merge, be nice if it was done before my BYOIP block goes live, manually creating the EIPs without terraform is going to suck in managing it.

Base automatically changed from master to main January 23, 2021 00:56
@breathingdust breathingdust requested a review from a team as a code owner January 23, 2021 00:56
@midN
Copy link

midN commented Feb 9, 2021

Can we get some movement on this?
It's been open for almost 2 years now, is that the new state of Terraform where updates can't be merged in for 2 years despite having an open PR?

@YakDriver @bflad

@YakDriver
Copy link
Member

One concern with this PR is the ability to perform meaningful testing of this new functionality in an automated way. Currently we do not have access to test with a public block of IPs. Perhaps the risk/reward still weighs in favor of adding the functionality. Do you have any suggestions?

Potential ideas for ways forward:

  1. Some workaround for a BYOIP (using another EIP?)
  2. Rather than nightly, automated tests, rely on occasional manual tests
  3. Procure a public block for testing

https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ec2-byoip.html#byoip-provision

@bflad
Copy link
Contributor

bflad commented Apr 15, 2021

In the past when we have had acceptance tests which the HashiCorp maintainers cannot reasonably run, we have provided code reviews/updates for consistency, then asked the contributor and another community member to run and verify the acceptance testing. Might be the best path forward here.

@arwilczek90
Copy link
Contributor Author

Unfortunately I'm not longer at the company I wrote this for(and like most people don't have an IPv4 block to play with), but @ravenbyron or @hsmithatemma are still there and possibly have access to a couple of IP addresses that can be used to provide test results.

@ghost ghost added size/L Managed by automation to categorize the size of a PR. and removed size/M Managed by automation to categorize the size of a PR. labels Apr 28, 2021
Copy link
Member

@YakDriver YakDriver left a comment

Choose a reason for hiding this comment

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

Looks great! 🎉

Output of acceptance tests (GovCloud):

--- PASS: TestAccAWSEIP_basic (32.33s)
--- PASS: TestAccAWSEIP_BYOIPAddress_default (32.32s)
--- PASS: TestAccAWSEIP_disappears (25.79s)
--- PASS: TestAccAWSEIP_Instance (137.48s)
--- PASS: TestAccAWSEIP_Instance_associatedUserPrivateIP (236.63s)
--- PASS: TestAccAWSEIP_Instance_notAssociated (146.57s)
--- PASS: TestAccAWSEIP_Instance_reassociate (122.72s)
--- PASS: TestAccAWSEIP_networkBorderGroup (32.43s)
--- PASS: TestAccAWSEIP_NetworkInterface (85.66s)
--- PASS: TestAccAWSEIP_NetworkInterface_twoEIPsOneInterface (85.66s)
--- PASS: TestAccAWSEIP_PublicIPv4Pool_default (32.33s)
--- PASS: TestAccAWSEIP_Tags_EC2VPC_withoutVPCTrue (45.92s)
--- PASS: TestAccAWSEIP_Tags_EC2VPC_withVPCTrue (45.99s)
--- PASS: TestAccAWSEIPAssociation_basic (164.12s)
--- PASS: TestAccAWSEIPAssociation_disappears (100.33s)
--- PASS: TestAccAWSEIPAssociation_instance (126.54s)
--- PASS: TestAccAWSEIPAssociation_networkInterface (80.73s)
--- PASS: TestAccAWSEIPAssociation_spotInstance (92.08s)
--- SKIP: TestAccAWSEIP_BYOIPAddress_custom (0.00s)
--- SKIP: TestAccAWSEIP_BYOIPAddress_custom_with_PublicIpv4Pool (0.00s)
--- SKIP: TestAccAWSEIP_carrierIP (1.66s)
--- SKIP: TestAccAWSEIP_CustomerOwnedIPv4Pool (2.05s)
--- SKIP: TestAccAWSEIP_Instance_ec2Classic (2.73s)
--- SKIP: TestAccAWSEIP_PublicIPv4Pool_custom (0.00s)
--- SKIP: TestAccAWSEIP_Tags_EC2Classic_withoutVPCTrue (2.71s)
--- SKIP: TestAccAWSEIP_Tags_EC2Classic_withVPCTrue (2.73s)
--- SKIP: TestAccAWSEIPAssociation_ec2Classic (0.00s)

Output of acceptance tests (us-west-2):

--- PASS: TestAccAWSEIP_basic (33.09s)
--- PASS: TestAccAWSEIP_BYOIPAddress_default (32.61s)
--- PASS: TestAccAWSEIP_carrierIP (33.82s)
--- PASS: TestAccAWSEIP_disappears (26.80s)
--- PASS: TestAccAWSEIP_Instance (131.53s)
--- PASS: TestAccAWSEIP_Instance_associatedUserPrivateIP (240.46s)
--- PASS: TestAccAWSEIP_Instance_ec2Classic (238.25s)
--- PASS: TestAccAWSEIP_Instance_notAssociated (168.78s)
--- PASS: TestAccAWSEIP_Instance_reassociate (141.37s)
--- PASS: TestAccAWSEIP_networkBorderGroup (17.84s)
--- PASS: TestAccAWSEIP_NetworkInterface (84.06s)
--- PASS: TestAccAWSEIP_NetworkInterface_twoEIPsOneInterface (84.78s)
--- PASS: TestAccAWSEIP_PublicIPv4Pool_default (30.83s)
--- PASS: TestAccAWSEIP_Tags_EC2Classic_withoutVPCTrue (13.65s)
--- PASS: TestAccAWSEIP_Tags_EC2Classic_withVPCTrue (42.84s)
--- PASS: TestAccAWSEIP_Tags_EC2VPC_withoutVPCTrue (45.06s)
--- PASS: TestAccAWSEIP_Tags_EC2VPC_withVPCTrue (45.09s)
--- PASS: TestAccAWSEIPAssociation_basic (230.01s)
--- PASS: TestAccAWSEIPAssociation_disappears (115.57s)
--- PASS: TestAccAWSEIPAssociation_ec2Classic (246.68s)
--- PASS: TestAccAWSEIPAssociation_instance (109.89s)
--- PASS: TestAccAWSEIPAssociation_networkInterface (73.05s)
--- PASS: TestAccAWSEIPAssociation_spotInstance (117.94s)
--- SKIP: TestAccAWSEIP_BYOIPAddress_custom (0.00s)
--- SKIP: TestAccAWSEIP_BYOIPAddress_custom_with_PublicIpv4Pool (0.00s)
--- SKIP: TestAccAWSEIP_CustomerOwnedIPv4Pool (1.45s)
--- SKIP: TestAccAWSEIP_PublicIPv4Pool_custom (0.00s)

Output of acceptance tests (us-east-1):

--- PASS: TestAccAWSEIP_basic (8.96s)
--- PASS: TestAccAWSEIP_BYOIPAddress_default (12.36s)
--- PASS: TestAccAWSEIP_carrierIP (10.35s)
--- PASS: TestAccAWSEIP_disappears (6.82s)
--- PASS: TestAccAWSEIP_Instance (352.90s)
--- PASS: TestAccAWSEIP_Instance_associatedUserPrivateIP (196.47s)
--- PASS: TestAccAWSEIP_Instance_ec2Classic (218.39s)
--- PASS: TestAccAWSEIP_Instance_notAssociated (107.95s)
--- PASS: TestAccAWSEIP_Instance_reassociate (115.58s)
--- PASS: TestAccAWSEIP_networkBorderGroup (9.04s)
--- PASS: TestAccAWSEIP_NetworkInterface (57.57s)
--- PASS: TestAccAWSEIP_NetworkInterface_twoEIPsOneInterface (58.20s)
--- PASS: TestAccAWSEIP_PublicIPv4Pool_default (9.92s)
--- PASS: TestAccAWSEIP_Tags_EC2Classic_withoutVPCTrue (2.92s)
--- PASS: TestAccAWSEIP_Tags_EC2Classic_withVPCTrue (16.32s)
--- PASS: TestAccAWSEIPAssociation_disappears (208.39s)
--- PASS: TestAccAWSEIPAssociation_ec2Classic (218.31s)
--- PASS: TestAccAWSEIPAssociation_instance (302.58s)
--- PASS: TestAccAWSEIPAssociation_networkInterface (55.67s)
--- PASS: TestAccAWSEIPAssociation_spotInstance (301.23s)
--- SKIP: TestAccAWSEIP_BYOIPAddress_custom (0.00s)
--- SKIP: TestAccAWSEIP_BYOIPAddress_custom_with_PublicIpv4Pool (0.00s)
--- SKIP: TestAccAWSEIP_CustomerOwnedIPv4Pool (0.32s)
--- SKIP: TestAccAWSEIP_PublicIPv4Pool_custom (0.00s)
--- SKIP: TestAccAWSEIP_Tags_EC2VPC_withoutVPCTrue (0.00s)
--- SKIP: TestAccAWSEIP_Tags_EC2VPC_withVPCTrue (0.00s)
--- SKIP: TestAccAWSEIPAssociation_basic (0.00s)

@YakDriver YakDriver added this to the v3.38.0 milestone Apr 28, 2021
@YakDriver YakDriver merged commit f605b70 into hashicorp:main Apr 28, 2021
@ghost
Copy link

ghost commented Apr 30, 2021

This has been released in version 3.38.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!

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 31, 2021
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/ec2 Issues and PRs that pertain to the ec2 service. size/L 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
Development

Successfully merging this pull request may close these issues.

Specify explicit address from BYOIP pool