provider/openstack: static routing entries for routers

[drebes]

Hi,

This is an attempt at adding support to static routes for OpenStack router resources. The trick here is that the static route entries are not first class citizens in OpenStack, but instead are created as an update to the routers once they are already attached to the subnets with the next hops.

I worked around that by creating a openstack_networking_router_table_v2 resource with a router_id argument, which specifies which router to apply the rules to. Here's the example I'm using for the acceptance tests:

        resource "openstack_networking_router_v2" "router_1" {
                name = "router_1"
                admin_state_up = "true"
        }

        resource "openstack_networking_network_v2" "network_1" {
                        name = "network_1"
                        admin_state_up = "true"
        }

        resource "openstack_networking_subnet_v2" "subnet_1" {
                        network_id = "${openstack_networking_network_v2.network_1.id}"
                        cidr = "192.168.199.0/24"
                        ip_version = 4
        }

        resource "openstack_networking_port_v2" "port_1" {
                name = "port_1"
                network_id = "${openstack_networking_network_v2.network_1.id}"
                admin_state_up = "true"
                fixed_ip {
                        subnet_id = "${openstack_networking_subnet_v2.subnet_1.id}"
                        ip_address = "192.168.199.1"
                }
        }

        resource "openstack_networking_router_interface_v2" "int_1" {
                        router_id = "${openstack_networking_router_v2.router_1.id}"
                        port_id = "${openstack_networking_port_v2.port_1.id}"
        }

        resource "openstack_networking_network_v2" "network_2" {
                        name = "network_2"
                        admin_state_up = "true"
        }

        resource "openstack_networking_subnet_v2" "subnet_2" {
                        network_id = "${openstack_networking_network_v2.network_2.id}"
                        cidr = "192.168.200.0/24"
                        ip_version = 4
        }

        resource "openstack_networking_port_v2" "port_2" {
                name = "port_2"
                network_id = "${openstack_networking_network_v2.network_2.id}"
                admin_state_up = "true"
                fixed_ip {
                        subnet_id = "${openstack_networking_subnet_v2.subnet_2.id}"
                        ip_address = "192.168.200.1"
                }
        }

        resource "openstack_networking_router_interface_v2" "int_2" {
                        router_id = "${openstack_networking_router_v2.router_1.id}"
                        port_id = "${openstack_networking_port_v2.port_2.id}"
        }

        resource "openstack_networking_router_table_v2" "router_table_1" {
                        depends_on = ["openstack_networking_router_interface_v2.int_1", "openstack_networking_router_interface_v2.int_2"]
                        router_id = "${openstack_networking_router_v2.router_1.id}"

                        route {
                                destination_cidr = "10.0.1.0/24"
                                next_hop = "192.168.199.254"
                        }

                        route {
                                destination_cidr = "10.0.2.0/24"
                                next_hop = "192.168.200.254"
                        }
        }

That seems to work, except for the case in which two openstack_networking_router_table_v2 resources try to configure the same router (through the same router_id). I'd assume that setting resource IDs consistently related to the router would allow the detection of conflicting resources (as they would have the same ID), but it doesn't seem to be the case.

The problem I'm having with the acceptance tasks is that the CheckDestroy function is called only once the whole configuration is destroyed, which includes the router to which the route entries are attached, which is more than what I want to check the deletion for. Is there a way I can limit that the CheckDestroy function is called after the destroy of the openstack_networking_router_table_v2 resource, but still before the destruction of the other resources?

I'm also open to implement it in a different way, but I'm not sure how to do it, due to the way the lifecycle of the routes need to be managed. For that reason, I still haven't written any documentation, but plan to do so.

[jtopjian]

@drebes This is really cool - thank you 😄

A couple questions / thoughts:

1. With regard to the acceptance test failure, I was able to fix it by making the testAccCheckNetworkingV2RouterTableDestroy function look like this:

func testAccCheckNetworkingV2RouterTableDestroy(s *terraform.State) error {
  config := testAccProvider.Meta().(*Config)
  networkingClient, err := config.networkingV2Client(OS_REGION_NAME)
  if err != nil {
    return fmt.Errorf("(testAccCheckNetworkingV2RouterTableDestroy) Error creating OpenStack networking client: %s", err)
  }

  for _, rs := range s.RootModule().Resources {
    if rs.Type != "openstack_networking_router_table_v2" {
      continue
    }

    router, err := routers.Get(networkingClient, rs.Primary.Attributes["router_id"]).Extract()
    if err != nil {
      httpError, ok := err.(*gophercloud.UnexpectedResponseCodeError)
      if !ok {
        return fmt.Errorf("Error retrieving OpenStack Neutron Router: %s", err)
      }

      if httpError.Actual == 404 {
        // The router doesn't exist,
        // therefore the routing table is inherently destroyed
        return nil
      }
    }

    if router.ID != rs.Primary.Attributes["router_id"] {
      return fmt.Errorf("Router for table not found")
    }

    if len(router.Routes) != 0 {
      return fmt.Errorf("Invalid number of route entries: %d", len(router.Routes))
    }
  }

  return nil
}

2. With regard to multiple routing table resources that point to the same router... that sounds like a messy situation. Right now when you're deleting, you're setting the (shared) routing table's routes to nil. In the case where only one routing table resource is deleted, the shared routing table will be clobbered and erased. Upon the next read/refresh, the table should be updated with the second routing table resource's table, but there's probably going to be a window where the table was erased.

With that in mind, what are the use-cases of having multiple tables pointing to the same router? Is it safe to say that there should never be?

3. Will there ever be a time someone wants to interpolate or do any type of dynamic route configuration in a routing table? If so, consider breaking the route argument out into its own resource. route will then be a single route that points to an upstream router or routing table. For a historical example, look at the openstack_lb_pool_v1 resource. It used to have a member argument until it was discovered that this does not work well when you create a dynamic number of instances and want to tie them back to a load balancer. To remedy, a openstack_lb_member_v1 resource was created:

• https://www.terraform.io/docs/providers/openstack/r/lb_pool_v1.html#member
• https://www.terraform.io/docs/providers/openstack/r/lb_member_v1.html

4. With the above in mind, would it be easier to either add a new TypeSet argument of route to the router resource or create a openstack_networking_router_route_v2 resource that ties back to an upstream router? The CRUD on that resource would then only manage a single route entry rather than a whole table which might make management easier.

Thoughts?

[drebes]

Thanks for the feedback.

1. This is OK to pass the test, but I don't think it would be actually testing the destroy functionality. Another approach I tested was to remove the CheckDestroy hook and replace it by a second resource.TestStep, which would first apply an updated configuration without the route table entry, then run the RouterTableDestroy test. But for some reason the second step doesn't seem to run. I can update the PR with that for you to take a look, if you're interested.

2. I don't think there is any need for one to set two routing table resources on the same router, as on one resource one can already have all the route entries one need. It's more an issue of not allowing the user to do something stupid. Maybe just adding an explicit note on the documentation is enough, saying "don't do this".

3 &4) This actually makes sense, and would solve the problem with point 2 above. I'll take a look on how to do it that way.

Thanks

[drebes]

Hi, regarding creating separate openstack_networking_router_route_v2 resources for each routing entry, I'll need to query the existing routes from the router before creating or updating a specific entry, as the router update API only allows me to send them all at once. Is there some example on how I can set a mutex when talking to the OpenStack client so that the operations on two route resources on the same router will not overlap?

[jtopjian]

Good catch. I recently implemented the following in the a Cobbler resource. It causes all resources of the same type to be processed one at a time rather than in parallel:

https://github.com/hashicorp/terraform/blob/master/builtin/providers/cobbler/resource_cobbler_system.go#L422-L423

Off the top of my head, this should also work here.

[drebes]

I did the refactoring to split the route entries in their own resources: now multiple route resources shouldn't be a problem on the same router. I also fixed the acceptance tests to better test adding/deleting just the route entries. Let me know if it looks good this way and I'll update the documentation.

2016/04/17 17:54:56 [INFO] Adding route {192.168.199.254 10.0.1.0/24}
2016/04/17 17:54:56 [DEBUG] Updating Router 904c6469-d2b5-481c-b8c6-73c74e612100 with options: {Name: AdminStateUp:<nil> Distributed:<nil> GatewayInfo:<nil> Routes:[{NextHop:192.168.199.254 DestinationCIDR:10.0.1.0/24}]}
2016/04/17 17:54:58 [INFO] Adding route {192.168.200.254 10.0.2.0/24}
2016/04/17 17:54:58 [DEBUG] Updating Router 904c6469-d2b5-481c-b8c6-73c74e612100 with options: {Name: AdminStateUp:<nil> Distributed:<nil> GatewayInfo:<nil> Routes:[{NextHop:192.168.199.254 DestinationCIDR:10.0.1.0/24} {NextHop:192.168.200.254 DestinationCIDR:10.0.2.0/24}]}
2016/04/17 17:55:00 [INFO] Deleting route {192.168.200.254 10.0.2.0/24}
2016/04/17 17:55:00 [DEBUG] Updating Router 904c6469-d2b5-481c-b8c6-73c74e612100 with options: {Name: AdminStateUp:<nil> Distributed:<nil> GatewayInfo:<nil> Routes:[{NextHop:192.168.199.254 DestinationCIDR:10.0.1.0/24}]}
2016/04/17 17:55:00 [INFO] Deleting route {192.168.199.254 10.0.1.0/24}
2016/04/17 17:55:00 [DEBUG] Updating Router 904c6469-d2b5-481c-b8c6-73c74e612100 with options: {Name: AdminStateUp:<nil> Distributed:<nil> GatewayInfo:<nil> Routes:[]}

[jtopjian]

Cool!

[jtopjian]

This looks really cool! I left a few comments for some small changes. Those + docs and this is good to go!

[jtopjian]

@drebes Looks like the inline comments got clobbered.

Would this apply in any case where the function returns a nil, or only on Delete? If so, I can remote those from the Read and Create error handling as well...

I think this only applies to Delete. If you'd like to play it safe, I'm totally fine leaving all SetId("") in place and it can be reviewed later on.

[drebes]

I've removed the SetId("")s from delete, kept them in the other cases, and added the resource documentation. Should now be good to go.

[jtopjian]

Looks good! Can you do one more thing and squash the commits into a single commit? This would prevent the original resource and refactor from being in the git history.

[jtopjian]

@drebes I forgot to leave a comment when I merged. Thank you very much for this -- it's a great addition!

[ghost]

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.