-
Notifications
You must be signed in to change notification settings - Fork 104
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
fix: fixed bug on ipv6_subnets_cwan_routed local variable #112
fix: fixed bug on ipv6_subnets_cwan_routed local variable #112
Conversation
…ences incorrect keys
Thanks for reporting this issue, opening a PR, and for the kind words! This looks right to me but I would like @pablo19sc to confirm since he did the majority of the work involving ipv6. However, he is currently away on leave. We may not hear from him for a short stint |
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.
Need to revert a few undesirable changes. We use the Terraform Registry paths in the examples instead of relative paths so that customers can copy & paste to use them without the expectation of cloning or downloading the repo.
examples/cloud_wan/README.md
Outdated
| <a name="module_ireland_vpc"></a> [ireland\_vpc](#module\_ireland\_vpc) | ../.. | n/a | | ||
| <a name="module_nvirginia_vpc"></a> [nvirginia\_vpc](#module\_nvirginia\_vpc) | ../.. | n/a | |
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.
Please revert
examples/cloud_wan/main.tf
Outdated
@@ -1,8 +1,7 @@ | |||
|
|||
# VPC module (North Virginia) | |||
module "nvirginia_vpc" { | |||
source = "aws-ia/vpc/aws" | |||
version = ">= 4.2.0" | |||
source = "../.." |
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.
Please revert
That makes sense , will revert those changes ! @tlindsay42 |
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.
lgtm & thanks much for your contribution, @darrenhorwitz1.
@pablo19sc: I haven't done functional testing on this one and will defer to you.
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! Thanks for catching the mistake!! The changes in the data.tf piece looks okay.
The only thing is the cloud_wan example, why did you change the vpc_assign_generated_ipv6_cidr_block
and assign_ip6_cidr
variables to false
? Can you revert back to true
? If not the Core Network resources is going to error as it will try to create an IPv6 ENIs in an IPv4-only subnet (core_network) - and also the IPv6 route will be created in an IPv4-only subnet (workload).
@pablo19sc my mistake , my thoughts were to show that the changes work with ipv4 but did not actually deploy the changes . Thanks for pointing that out ! I will revert them now thanks. Just out of curiosity , I haven't worked with ipv6 much at all but when creating tgw's or cwan's with ipv6 support enabled, can one not attach ipv4 networks to the them without the vpc having ipv6 support ? |
Thanks @darrenhorwitz1! Let me ping the reviewers to merge this ASAP. Sorry for the delay, I went PTO for some weeks. Regarding your question, both TGW and CWAN will need a dual-stack subnet to enable IPv6. That means that you can use IPv4 as well to consume IPv4 services connected to Cloud WAN. What you cannot do is enable IPv6 in the attachments is the subnet is only IPv4-only - and it will not work if the subnet is IPv6-only. |
I noticed there was a minor bug on core_network_routes , when one passes a map of the routes to go to the cwan , the ipv6 routes local variable references the the ipv4 routes which throws an unexpected error whilst trying to deploy ipv4 routes only it also tries to deploy ipv6 routes.
Moreover, I updated the example to have a hybrid sort of example to test the expected behavior .
Love the module btw!