Route Rules need to be sorted in priority order in module net-lb-app-ext

[cavila-evoliq]

Describe the bug
In the urlmap_config variable, the route_rules nested under path_matchers are being used as an iterator for a dynamic block here: link.

We are having problems with this approach. When defining priority, the order of the elements is important as the GCP API requires rules be sorted in increasing-priority order. Using toset() does not allow us to pass the list in any particular order, so Terraform sorts it. In our case, it kept sorting a rule with priority 120 above a rule with 119, breaking it.

Environment

Terraform v1.7.1
on darwin_arm64
+ provider registry.terraform.io/hashicorp/archive v2.4.0
+ provider registry.terraform.io/hashicorp/google v4.84.0
+ provider registry.terraform.io/hashicorp/google-beta v4.84.0
+ provider registry.terraform.io/hashicorp/random v3.5.1
+ provider registry.terraform.io/hashicorp/tfe v0.50.0

output from `git rev-parse --short HEAD`

9f5bf954

module source = "github.com/GoogleCloudPlatform/cloud-foundation-fabric//modules/net-lb-app-ext?ref=v28.0.0"

To Reproduce
Define a URL Map Config and create multiple route rules of varying priorities under a single path_matcher.

Expected behavior
Route Rules are defined in the supplied order/priority

Result

Error: Error updating UrlMap "projects/my-project/global/urlMaps/my-lb": googleapi: Error 400: Invalid value for field 'resource.pathMatchers[0].routeRules[1].priority': '119'. Within a pathMatcher, a route rule must have a priority that's higher than the previous route rule's priority, invalid

Additional context
I was able to work around the problem by converting the route_rules variable type from list(object) to map(object) and removing toset() from the the module in a fork and set the priority as the key but this is also a flawed approach as maps are sorted in lexicographical order so a map of {1: "test", 3: "test", 120: "test" is sorted

"1" = "test"
"120" = "test"
"3" = "test"

I'm not sure what the right solution to this is, hence this issue.

[wiktorn]

Full repro, using examples:

module "compute-vm-group-b" {
  source     = "./fabric/modules/compute-vm"
  project_id = var.project_id
  zone       = "${var.region}-b"
  name       = "my-ig-b"
  network_interfaces = [{
    network    = var.vpc.self_link
    subnetwork = var.subnet.self_link
  }]
  boot_disk = {
    initialize_params = {
      image = "cos-cloud/cos-stable"
    }
  }
  group = { named_ports = {} }
}

module "compute-vm-group-c" {
  source     = "./fabric/modules/compute-vm"
  project_id = var.project_id
  zone       = "${var.region}-c"
  name       = "my-ig-c"
  network_interfaces = [{
    network    = var.vpc.self_link
    subnetwork = var.subnet.self_link
  }]
  boot_disk = {
    initialize_params = {
      image = "cos-cloud/cos-stable"
    }
  }
  group = { named_ports = {} }
}


module "glb-0" {
  source     = "./fabric/modules/net-lb-app-ext"
  project_id = var.project_id
  name       = "glb-test-0"
  backend_service_configs = {
    default = {
      backends = [{
        backend = module.compute-vm-group-b.group.id
      }]
    }
    other = {
      backends = [{
        backend = module.compute-vm-group-c.group.id
      }]
    }
  }
  urlmap_config = {
    default_service = "default"
    host_rules = [{
      hosts        = ["*"]
      path_matcher = "pathmap"
    }]
    path_matchers = {
      pathmap = {
        default_service = "default"
        route_rules = [
          {
            priority = 3
            service = "default"
            header_action = {
              request_add = {
                test-priority-3 = {
                  value = "prio-3"
                }
              }
            }
          },
          {
            priority = 10
            service = "default"
            header_action = {
              request_add = {
                test-priority-1 = {
                  value = "prio-1"
                }
              }
            }
          },
          {
            priority = 2
            service = "default"
            header_action = {
              request_add = {
                test-priority-2 = {
                  value = "prio-2"
                }
              }
            }
          }
        ]
      }
    }
  }
}

Fails with expected:

│ Error: Error updating UrlMap "projects/*/global/urlMaps/glb-test-0": googleapi: Error 400: Invalid value for field 'resource.pathMatchers[0].routeRules[1].priority': '2'. Within a pathMatcher, a route rule must have a priority that's higher than the previous route rule's priority, invalid

[wiktorn]

@cavila-evoliq
My idea how to solve this, is to replace the problematic for_each with:

        for_each = { for value in m.value.route_rules : format("%010d", value.priority) => value }

This way, the keys when sorted in lexicographical are still in the same order as numerical order. Can you confirm that this solves your issue?

[juliocc]

Wouldn't it be easier to just use the list in the for_each?

This seems to work:

locals {
  rules = [
    { priority = 2 },
    { priority = 3 },
    { priority = 10 },
    { priority = 150 },
  ]
}

resource "google_compute_url_map" "urlmap" {
  name            = "urlmap"
  default_service = "service"
  project         = "myproject"

  path_matcher {
    name = "name"
    dynamic "route_rules" {
      for_each = local.rules
      content {
        priority = route_rules.value.priority
      }
    }
  }
}

Terraform will perform the following actions:

  # google_compute_url_map.urlmap will be created
  + resource "google_compute_url_map" "urlmap" {
      + creation_timestamp = (known after apply)
      + default_service    = "service"
      + fingerprint        = (known after apply)
      + id                 = (known after apply)
      + map_id             = (known after apply)
      + name               = "urlmap"
      + project            = "myproject"
      + self_link          = (known after apply)

      + path_matcher {
          + name = "name"

          + route_rules {
              + priority = 2
            }
          + route_rules {
              + priority = 3
            }
          + route_rules {
              + priority = 10
            }
          + route_rules {
              + priority = 150
            }
        }
    }

[cavila-evoliq]

For some reason I thought dynamic blocks had the same for_each restriction as resources and would require being converted to a set, I assumed that's why it was done in the first place. But yeah, lists maintain their order so since the dynamic block accepts a list, that would be the ideal solution.

[juliocc]

@cavila-evoliq can you try #2096 and confirm if it works for you?

[juliocc]

For some reason I thought dynamic blocks had the same for_each restriction as resources and would require being converted to a set, I assumed that's why it was done in the first place. But yeah, lists maintain their order so since the dynamic block accepts a list, that would be the ideal solution.

I was under that impression too but the docs say The for_each value must be a collection with one element per desired nested block. So a list is fine.

@wiktorn We should probably check if we have the same issue in other dynamic blocks elsewhere

[cavila-evoliq]

Just tested the changes in our dev environment and it works great!

[juliocc]

Awesome. It's merged into main now so I'm closing this.

Thanks for the report @cavila-evoliq!