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

feat(router): static routes support #404

Merged
merged 5 commits into from
Oct 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ See updating [Changelog example here](https://keepachangelog.com/en/1.0.0/)
- kubernetes: `control_plane_ip_filter` field to `upcloud_kubernetes_cluster` resource
- dbaas: `upcloud_managed_database_mysql_sessions`, `upcloud_managed_database_postgresql_sessions` and `upcloud_managed_database_redis_sessions` data sources
- network: `dhcp_routes` field to `ip_network` block in `upcloud_network` resource
- router: `static_routes` block to `upcloud_router` resource

### Changed
- kubernetes: remove node group maximum value validation. The maximum number of nodes (in the cluster) is determined by the cluster plan and the validation is done on the API side.
Expand Down
6 changes: 6 additions & 0 deletions examples/resources/upcloud_gateway/resource.tf
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
// Create router for the gateway
resource "upcloud_router" "this" {
name = "gateway-example-router"

# UpCloud Network Gateway Service will add a static route to this router to ensure gateway networking is working as intended.
# You need to ignore changes to it, otherwise TF will attempt to remove the static routes on subsequent applies
lifecycle {
ignore_changes = [static_route]
}
}

// Create network for the gateway
Expand Down
107 changes: 94 additions & 13 deletions internal/service/router/router.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,13 @@ import (
"fmt"

"github.com/UpCloudLtd/terraform-provider-upcloud/internal/utils"

"github.com/UpCloudLtd/upcloud-go-api/v6/upcloud"
"github.com/UpCloudLtd/upcloud-go-api/v6/upcloud/request"
"github.com/UpCloudLtd/upcloud-go-api/v6/upcloud/service"
"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
)

func ResourceRouter() *schema.Resource {
Expand Down Expand Up @@ -42,20 +45,59 @@ func ResourceRouter() *schema.Resource {
Type: schema.TypeString,
},
},
"static_route": {
Description: "A collection of static routes for this router",
Optional: true,
Type: schema.TypeSet,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"name": {
Description: "Name or description of the route.",
Type: schema.TypeString,
Optional: true,
Computed: true,
},
"nexthop": {
Description: "Next hop address. NOTE: For static route to be active the next hop has to be an address of a reachable running Cloud Server in one of the Private Networks attached to the router.",
Type: schema.TypeString,
Required: true,
ValidateFunc: validation.Any(validation.IsIPv4Address, validation.IsIPv6Address),
},
"route": {
Description: "Destination prefix of the route.",
Type: schema.TypeString,
Required: true,
ValidateFunc: validation.Any(validation.IsCIDR),
},
},
},
},
},
}
}

func resourceRouterCreate(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics {
func resourceRouterCreate(ctx context.Context, d *schema.ResourceData, meta interface{}) (diags diag.Diagnostics) {
client := meta.(*service.Service)

var diags diag.Diagnostics

opts := &request.CreateRouterRequest{
req := &request.CreateRouterRequest{
Name: d.Get("name").(string),
}

router, err := client.CreateRouter(ctx, opts)
if v, ok := d.GetOk("static_route"); ok {
for _, staticRoute := range v.(*schema.Set).List() {
staticRouteData := staticRoute.(map[string]interface{})

r := upcloud.StaticRoute{
Name: staticRouteData["name"].(string),
Nexthop: staticRouteData["nexthop"].(string),
Route: staticRouteData["route"].(string),
}

req.StaticRoutes = append(req.StaticRoutes, r)
}
}

router, err := client.CreateRouter(ctx, req)
if err != nil {
return diag.FromErr(err)
}
Expand All @@ -70,16 +112,27 @@ func resourceRouterCreate(ctx context.Context, d *schema.ResourceData, meta inte
return diag.FromErr(err)
}

var staticRoutes []map[string]interface{}
for _, staticRoute := range router.StaticRoutes {
staticRoutes = append(staticRoutes, map[string]interface{}{
"name": staticRoute.Name,
"nexthop": staticRoute.Nexthop,
"route": staticRoute.Route,
})
}

if err := d.Set("static_route", staticRoutes); err != nil {
return diag.FromErr(err)
}

d.SetId(router.UUID)

return diags
}

func resourceRouterRead(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics {
func resourceRouterRead(ctx context.Context, d *schema.ResourceData, meta interface{}) (diags diag.Diagnostics) {
client := meta.(*service.Service)

var diags diag.Diagnostics

opts := &request.GetRouterDetailsRequest{
UUID: d.Id(),
}
Expand All @@ -105,31 +158,59 @@ func resourceRouterRead(ctx context.Context, d *schema.ResourceData, meta interf
return diag.FromErr(err)
}

var staticRoutes []map[string]interface{}
for _, staticRoute := range router.StaticRoutes {
staticRoutes = append(staticRoutes, map[string]interface{}{
"name": staticRoute.Name,
"nexthop": staticRoute.Nexthop,
"route": staticRoute.Route,
})
}

if err := d.Set("static_route", staticRoutes); err != nil {
return diag.FromErr(err)
}

return diags
}

func resourceRouterUpdate(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics {
client := meta.(*service.Service)

opts := &request.ModifyRouterRequest{
req := &request.ModifyRouterRequest{
UUID: d.Id(),
}

if v, ok := d.GetOk("name"); ok {
opts.Name = v.(string)
req.Name = v.(string)
}

_, err := client.ModifyRouter(ctx, opts)
var staticRoutes []upcloud.StaticRoute

if v, ok := d.GetOk("static_route"); ok {
for _, staticRoute := range v.(*schema.Set).List() {
staticRouteData := staticRoute.(map[string]interface{})

staticRoutes = append(staticRoutes, upcloud.StaticRoute{
Name: staticRouteData["name"].(string),
Nexthop: staticRouteData["nexthop"].(string),
Route: staticRouteData["route"].(string),
})
}
}

req.StaticRoutes = &staticRoutes

_, err := client.ModifyRouter(ctx, req)
if err != nil {
return diag.FromErr(err)
}

return resourceRouterRead(ctx, d, meta)
}

func resourceRouterDelete(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics {
func resourceRouterDelete(ctx context.Context, d *schema.ResourceData, meta interface{}) (diags diag.Diagnostics) {
client := meta.(*service.Service)
var diags diag.Diagnostics

router, err := client.GetRouterDetails(ctx, &request.GetRouterDetailsRequest{
UUID: d.Id(),
Expand Down
55 changes: 48 additions & 7 deletions upcloud/resource_upcloud_router_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,22 @@ func TestAccUpCloudRouter(t *testing.T) {
var router upcloud.Router
name := fmt.Sprintf("tf-test-%s", acctest.RandString(10))

staticRoutes := []upcloud.StaticRoute{{Name: "my-example-route", Nexthop: "10.0.0.100", Route: "0.0.0.0/0"}}
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
ProviderFactories: testAccProviderFactories(&providers),
CheckDestroy: testAccCheckRouterDestroy,
Steps: []resource.TestStep{
{
Config: testAccRouterConfig(name),
Config: testAccRouterConfig(name, staticRoutes),
Check: resource.ComposeTestCheckFunc(
testAccCheckRouterExists("upcloud_router.my_example_router", &router),
testAccCheckUpCloudRouterAttributes(&router, name),
resource.TestCheckTypeSetElemNestedAttrs("upcloud_router.my_example_router", "static_route.*", map[string]string{
"name": "my-example-route",
"nexthop": "10.0.0.100",
"route": "0.0.0.0/0",
}),
),
},
},
Expand All @@ -43,23 +49,36 @@ func TestAccUpCloudRouter_update(t *testing.T) {
name := fmt.Sprintf("tf-test-%s", acctest.RandString(10))
updateName := fmt.Sprintf("tf-test-update-%s", acctest.RandString(10))

staticRoutes := []upcloud.StaticRoute{{Nexthop: "10.0.0.100", Route: "0.0.0.0/0"}}
updateStaticRoutes := []upcloud.StaticRoute{{Name: "my-example-route-2", Nexthop: "10.0.0.101", Route: "0.0.0.0/0"}}

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
ProviderFactories: testAccProviderFactories(&providers),
CheckDestroy: testAccCheckRouterDestroy,
Steps: []resource.TestStep{
{
Config: testAccRouterConfig(name),
Config: testAccRouterConfig(name, staticRoutes),
Check: resource.ComposeTestCheckFunc(
testAccCheckRouterExists("upcloud_router.my_example_router", &router),
testAccCheckUpCloudRouterAttributes(&router, name),
resource.TestCheckTypeSetElemNestedAttrs("upcloud_router.my_example_router", "static_route.*", map[string]string{
"name": "static-route-0",
"nexthop": "10.0.0.100",
"route": "0.0.0.0/0",
}),
),
},
{
Config: testAccRouterConfig(updateName),
Config: testAccRouterConfig(updateName, updateStaticRoutes),
Check: resource.ComposeTestCheckFunc(
testAccCheckRouterExists("upcloud_router.my_example_router", &router),
testAccCheckUpCloudRouterAttributes(&router, updateName),
resource.TestCheckTypeSetElemNestedAttrs("upcloud_router.my_example_router", "static_route.*", map[string]string{
"name": "my-example-route-2",
"nexthop": "10.0.0.101",
"route": "0.0.0.0/0",
}),
),
},
},
Expand All @@ -77,7 +96,7 @@ func TestAccUpCloudRouter_import(t *testing.T) {
CheckDestroy: testAccCheckRouterDestroy,
Steps: []resource.TestStep{
{
Config: testAccRouterConfig(name),
Config: testAccRouterConfig(name, nil),
Check: resource.ComposeTestCheckFunc(
testAccCheckRouterExists("upcloud_router.my_example_router", &router),
),
Expand Down Expand Up @@ -373,11 +392,33 @@ func testAccCheckRouterNetworkDestroy(s *terraform.State) error {
return nil
}

func testAccRouterConfig(name string) string {
return fmt.Sprintf(`
func testAccRouterConfig(name string, staticRoutes []upcloud.StaticRoute) string {
s := fmt.Sprintf(`
resource "upcloud_router" "my_example_router" {
name = "%s"
}`, name)
`, name)

if len(staticRoutes) > 0 {
for _, staticRoute := range staticRoutes {
s = s + fmt.Sprintf(`
static_route {
nexthop = "%s"
route = "%s"
`, staticRoute.Nexthop, staticRoute.Route)

if len(staticRoute.Name) > 0 {
s = s + fmt.Sprintf(`
name = "%s"
`, staticRoute.Name)
}
}
s = s + `
}`
}
s = s + `
}
`
return s
}

func testAccNetworkRouterAttached(network *upcloud.Network, router *upcloud.Router) resource.TestCheckFunc {
Expand Down
4 changes: 4 additions & 0 deletions upcloud/testdata/upcloud_gateway/gateway_s1.tf
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ variable "zone" {

resource "upcloud_router" "this" {
name = "${var.prefix}router"

lifecycle {
ignore_changes = [static_route]
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the gateway automatically set static_routes to the router? Could we somehow handle that automatic route so that end-user does not have to use lifecycle.ignore_changes 🤔 Or is it something we could instruct user to do themselves, like the router creation with Kubernetes clusters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment to the documentation for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

The static routes automatically set by the gateway seem to be in link-local address space (169.254.0.0/16). Thus, they should not collide with values expected from user. Maybe some sort special value would work here, but that would require more investigating/testing.

Copy link
Contributor

@kangasta kangasta Oct 16, 2023

Choose a reason for hiding this comment

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

Probably good to merge this like this to unblock the release and then try to add fix or workaround for this later 🤔 I was also trying to think of some nice way of fixing this, but no good ideas this far.

}
}

resource "upcloud_network" "this" {
Expand Down
4 changes: 4 additions & 0 deletions upcloud/testdata/upcloud_gateway/gateway_s2.tf
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ variable "zone" {

resource "upcloud_router" "this" {
name = "${var.prefix}router"

lifecycle {
ignore_changes = [static_route]
}
}

resource "upcloud_network" "this" {
Expand Down