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

provider/aws: add aws_route resource (finish) #3548

Merged
merged 5 commits into from Oct 29, 2015
Merged

provider/aws: add aws_route resource (finish) #3548

merged 5 commits into from Oct 29, 2015

Conversation

BSick7
Copy link
Contributor

@BSick7 BSick7 commented Oct 19, 2015

This work completes, rebases, and squashes work from #2454.

gkze and others added 2 commits October 19, 2015 09:16
Fixing basic acceptance test.
Adding warning to website about mixed mode.
Adding exists to aws_route.
Adding acceptance test for changing destination_cidr_block.
@BSick7
Copy link
Contributor Author

BSick7 commented Oct 19, 2015

@catsby @phinze This is ready for review based on previous PR that was incomplete.

@gkze
Copy link
Contributor

gkze commented Oct 19, 2015

Thank you @BSick7 🙏

@pll
Copy link

pll commented Oct 20, 2015

Great to see this wrapping up. Any idea how long until we see it in a release?

@sarguru
Copy link
Contributor

sarguru commented Oct 20, 2015

Thanks @BSick7 would really love to have this

@pll
Copy link

pll commented Oct 20, 2015

Looking through the code and seeing the example usage, I don't see a way to add route-propagation to this. Currently I'm doing this:

resource "aws_route_table" "private-route-table" {
   vpc_id = "${var.vpc_id}"
   route {
       # Default route via the NAT
       cidr_block  = "0.0.0.0/0"
       instance_id = "${var.nat_id}"
   }
   route {
       # Datacenter traffic via the VGW
       cidr_block  = "${var.datacenter_cidr}"
       gateway_id  = "${var.vgw_id}"
   }
   # Make sure to propagate the VGW routes
   propagating_vgws = ["${var.vgw_id}"]
}

I'd really like to create the route initially like this:

resource "aws_route_table" "private-route-table" {
   vpc_id = "${var.vpc_id}"
   route {
       # Default route via the NAT
       cidr_block  = "0.0.0.0/0"
       instance_id = "${var.nat_id}"
   }
}

Then add a route later like this:

resource "aws_route" "priv-rt" {
  route_table_id   = "${var.privrt_id}"
  destination_cidr_block = "${var.datacenter_cidr}"
  gateway_id  = "${var.vgw_id}"

   # Make sure to propagate the VGW routes
   propagating_vgws = ["${var.vgw_id}"]
}

Does this make sense ?

@BSick7
Copy link
Contributor Author

BSick7 commented Oct 20, 2015

@pll propagating_vgws belongs to a route table. propagating_vgws has no meaning directly attached to aws_route.

Keep in mind, this PR also does not implement mixed mode route tables (inline routes and singular aws_route resources). In #2454, there was a dialogue discussing alternative ways of solving through https://github.com/hashicorp/terraform/issues/2275.

@pll
Copy link

pll commented Oct 20, 2015

@BSick7 - Yeah, I get that the propagating_vgws isn't directly connected to adding a route in the aws_route resource. What I was trying to convey is that in this case, when I add a route of this type, I also want to toggle the propagation from off to on. That action is associated with the addition of this type of route, which means when I employ the aws_route resource, I also need a means up updating the route_table resource for propagating_vgws as well.

It might be solvable with #2275, I'm not sure. Or maybe something analagous to an aws_update_route_table feature.

Thanks

@BSick7
Copy link
Contributor Author

BSick7 commented Oct 20, 2015

@pll propagation of a vgw within an aws_route is more of a status than a configuration point. Note that http://docs.aws.amazon.com/sdk-for-go/api/service/ec2/EC2.html#CreateRoute-instance_method does not allow this type of input.

@pll
Copy link

pll commented Oct 20, 2015

@BSick7 - Interesting. So, really all I need is a way to access client.EnableVgwRoutePropagationRequest(params) from within terraform it seems.

@BSick7
Copy link
Contributor Author

BSick7 commented Oct 21, 2015

@pll correct. That api would tie into aws_route_table.

@@ -0,0 +1,57 @@
---
layout: "aws"
page_title: "AWS: aws_route_table"
Copy link
Contributor

Choose a reason for hiding this comment

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

should just be aws_route, right? Same for sidebar_current

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup! Fixing.

@catsby
Copy link
Contributor

catsby commented Oct 26, 2015

A few questions, otherwise very promising. Thanks!

@BSick7
Copy link
Contributor Author

BSick7 commented Oct 26, 2015

@catsby I removed the code doing a recreate and used ForceNew. The acceptance tests still pass locally.

@BSick7
Copy link
Contributor Author

BSick7 commented Oct 28, 2015

@catsby any plans to review this soon?

@catsby
Copy link
Contributor

catsby commented Oct 29, 2015

Thanks for the updates, @BSick7 . This looks good, pulling it in

catsby added a commit that referenced this pull request Oct 29, 2015
provider/aws: add aws_route resource (finish)
@catsby catsby merged commit 5c3c1e2 into hashicorp:master Oct 29, 2015
@steve-jansen steve-jansen deleted the aws_route branch October 29, 2015 22:11
@steve-jansen
Copy link
Contributor

🚀

@antonosmond
Copy link

Awesome!!! Well done guys there's a lot of us that have been waiting for this for sooooo long! I hope the next terraform release is very soon!

@ryedin
Copy link

ryedin commented Nov 12, 2015

Oh boy oh boy :) - also waiting with baited breath for this release! This is the only thing keeping us from realizing a fully automated workflow right now.

@ghost
Copy link

ghost commented Apr 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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants