Moving secondary_ranges to a key under subnets.

[tfhartmann]

This PR removes secondary_ranges as an input variable and moves it to the subnets list. I wasn't able to use dynamics blocs which was my original intent, the https://www.terraform.io/docs/providers/google/r/compute_subnetwork.html#secondary_ip_range parameter uses attributes as blocks rather then dynamic blocks for backwards compatibility.

I also changed the subnets to be a list of objects rather than a list of maps, since I needed to mix and match types. Because I wanted the secondary_ranges entry in the object be a list of maps, I had to define it's type, which has the result of making the secondary_ranges key required. Which is a bummer because I know that was just made optional recently.

I think this PR will require a bit of clean up, examples and test, but I wanted to open it to highlight my proposed change.

The subnets parameter would look something like this :

  subnets          = [
    {
      subnet_name           = "subnet1"
      subnet_ip             = "10.175.0.0/16"
      subnet_region         = "asia-east1"
      subnet_flow_logs      = "true"
      subnet_private_access = "true"
      secondary_ranges  = []
    },
    {
      subnet_name           = "subnet2"
      subnet_ip             = "10.178.112.0/20"
      subnet_region         = "asia-east1"
      subnet_flow_logs      = "true"
      subnet_private_access = "true"
      secondary_ranges =       [{
              range_name    = "secondary_range1"
              ip_cidr_range = "10.183.0.0/16"
            },
            {
              range_name    = "secondary_range2"
              ip_cidr_range = "10.178.128.0/20"
            },
          ]
    },

[morgante]

Overall I like this change and think it provides a much better API! Assuming you can get everything to work properly (and pass tests), I'd be happy to approve.

[tfhartmann]

I've been working on getting tests running locally with make docker_test_integration I'm getting Error: Unassigned variable because the project_id variable isn't' being set. Is there a suggested way to pass that to the docker container / test scripts? I've set PROJECT_ID, GOOGLE_PROJECT, TF_VAR_project_id in my local env, but don't see anyway those could get passed into the containers. Is there something I'm missing?

[morgante]

@tfhartmann The recommended approach is actually to set them in a tfvars file: https://github.com/terraform-google-modules/terraform-google-network/blob/master/CONTRIBUTING.md#terraform-integration-tests

You'll have to symlink that tfvars file: #67

Apologies for the painful process here, we're working on improving it.

[tfhartmann]

🤣 No worries! Thanks! At least now I know I'm not going mad!

[tfhartmann]

Changing subnets to a object seems to change the behavior of things like the lookup function, the tldr is that I had to make a number of other parameters required as opposed to optional. I also want to add a couple more tests, at least one for description, which I don't think is working quite right in my branch, because of the change to an object. Also a test to verify that a subnets in different regions can have the same name.

[morgante]

That's unfortunate, I'd really like to avoid making optional params required. Could we see if it's possible to keep the existing map-based approach?

Apologies if you stated that up front, but if we're making all optional params required it might be too big of a change.

[tfhartmann]

@morgante Agreed, it's not ideal. Heres what I'm seeing. I changed the subnets variable to an object so that it could support multiple types. The secondary_ranges parameter - a list of objects (more on that in a moment) and the rest of the parameters are strings. - subnets as you'll recall started as list(map(string)) so changing it to an object is required to pass in strings and lists. I had to make secondary_ranges a list of objects for reasons that are somewhat unclear to me, however, when I try to make that parameter a list of maps, Terraform errors out with this:

Error: Incorrect attribute value type

  on /Users/timh/git/terraform-google-network/main.tf line 51, in resource "google_compute_subnetwork" "subnetwork":
  51:   secondary_ip_range       = var.subnets[count.index]["secondary_ranges"]

Inappropriate value for attribute "secondary_ip_range": incorrect list element
type: object required.

😠 Maybe I need to loop through it differently then I have been? Honestly I'd love to be able to keep it a list of maps until we are able to pass in defaults to objects.

Here's the issue opened to add the ability to add defaults to Object, which frankly would make the whole object VS map thing a non-issue for this module.
hashicorp/terraform#22449

There are two reasons I'd still like to propose this as a change even if the optional parameters become required.

Firstly, currently the module doesn't support having subnets in different regions with the same name. Becausesubnet_name is used to lookup the secondary range info, one of the subnets is going to get incorrect secondary rang info.

Secondly, IMO, the improved API is still worthwhile, especially for sites that have lots of subnets to manage. For example we have ~43 most with at least two secondary networks 😭

I'll keep at it and see if I can get the module working with this config:

variable "subnets" {
  type = list(object(
    {
      subnet_name           = string
      subnet_ip             = string
      subnet_region         = string
      subnet_flow_logs      = string
      subnet_private_access = string
      secondary_ranges      = list(map(string))
      #secondary_ranges      = list(object({ range_name = string, ip_cidr_range = string }))
    }
  ))
  description = "The list of subnets being created"
}

if I can get this working, it would be ideal. Thanks for the reviews and feedback!

[tfhartmann]

Doh, I think I'm wrong, it's not the inner object type thats causing this result, it's the outer. and that can't be a map because secondary_ranges needs to be a list of maps :sad: I guess I could make the input of secondary ranges a string, like a csv then transform it in the resource... (sorry some of this is me thinking out loud)

variable "subnets" {
  type = list(object(

[morgante]

Thanks for iterating on this. Does the lookup style not work?

You could also try switching to the block for_each syntax so we directly construct the secondary_ranges block by iterating through the secondary_ranges value.

[morgante]

Also, one note:

Firstly, currently the module doesn't support having subnets in different regions with the same name. Because subnet_name is used to lookup the secondary range info, one of the subnets is going to get incorrect secondary rang info.

This is not a desired goal of the module, actually. Our prescriptive guidance is that subnets in different regions should have unique names, so it's probably not going to be supported for a variety of reasons.

[tfhartmann]

Thanks for iterating on this. Does the lookup style not work?

Apparently not with an object? The behavior I'm seeing when setting the variable like this

variable "subnets" {
  type = list(object(
    {
      subnet_name           = string
      subnet_ip             = string
      subnet_region         = string
      secondary_ranges      = list(object({ range_name = string, ip_cidr_range = string }))
    }
  ))
  description = "The list of subnets being created"
}

then set my lookups:

  private_ip_google_access = lookup(var.subnets[count.index], "subnet_private_access", "false")
  enable_flow_logs         = lookup(var.subnets[count.index], "subnet_flow_logs", "false")

The result is that regardless of what is passed into the module, the lookup default always takes precedence. So both are set "false", even if "true" is passed in.

You could also try switching to the block for_each syntax so we directly construct the secondary_ranges block by iterating through the secondary_ranges value.

Actually changing secondary_ranges to a dynamic block works when secondary_ranges is a list of objects or a list of maps, I'm going to keep it a dynamic block I think it's easier to read that way actually. I think I'm still stuck with subnets being a list of objects though, I can't mix types in a map.

[tfhartmann]

@morgante So here's where i've gotten to. I wasn't able to make keys in the subnet object optional, or set defaults. It may perhaps be possible, however I wasn't able to figure out how to do it. 😢

I do want to propose the changes in this PR as a contribution to this module though. I totally understand the reservations, and hope you'll consider them, even though it's a pretty significant change to the API, and a major release.

I think the API changes, even with the addition of the required keys in the subnets data structure still has some nice value. IMO, having all the subnet and secondary configuration in the same place in the manifest makes managing the networks, and secondary networks clearer especially in installations with more than few networks. In addition, as hashicorp/terraform#22449 progresses, we'll be able to set sane defaults again in that object. I totally agree it's not ideal, as I also prefer modules with opinions and sane defaults.

Somewhat related, my observation of how this module as it's currently working, is that it will allow the user to create subnets in different regions with the same name, but currently creates the secondary subnets wrong - when the module does the lookup it sets the secondary networks to whichever subnet name key it finds first in secondary_ranges duplicating the wrong secondary subnets in one of the networks. In my opinion the desired behavior should either be to throw an error when a duplicate key exists in subnets or to create them correctly. Since GCP also allows this behavior, (and since I have a legacy subnet that was typo'ed back in the dark days of history and had a duplicate name to another net in a different region 😄 ) thats the route I went with. TBH, I'm not sure how I'd throw the duplicate key error in Terraform, I think this is now possible with 0.12.x but I've not done it yet.

[morgante]

I'm definitely willing to consider this change, but it does introduce a significantly different API for users. For now, my inclination is to hold off for the time being and hope Terraform improves objects to allow defaults sometime soon. In particular, what I want to avoid is a scenario where we release this module, force everyone to start being explicit, then have defaults available in the future.

So for now I think we're going to hold off and see if core can get some traction.

In my opinion the desired behavior should either be to throw an error when a duplicate key exists in subnets or to create them correctly.

I'm fine with either, but my concern with allowing them in different regions is that we then have to include the region in the key—making moving subnets between regions more complicated.

[tfhartmann]

So for now I think we're going to hold off and see if core can get some traction.

Cool, I'll keep an eye on that issue too.

I'm fine with either, but my concern with allowing them in different regions is that we then have to include the region in the key—making moving subnets between regions more complicated.

I'm not sure I understand this concern. Can subnets be moved between regions currently? When I tried it looks like the networks will be destroyed and recreated in a different region.

Thanks for taking a look at this! This has been a fun exercise! I'm looking forward to the glorious future when defaults get added to objects! 😃

[morgante]

I'm not sure I understand this concern. Can subnets be moved between regions currently? When I tried it looks like the networks will be destroyed and recreated in a different region.

True, I guess the difference is that if we key based on it it will show up as a delete + create rather than a create/delete. Essentially Terraform state location will move.

Not a big deal though.

[tfhartmann]

While looking up something for #70 in the terraform changelog, I noticed that in 0.12.7 the lookup function was enhanced to work on objects! I may bump the TF version in the tests and give it a try 😄 https://github.com/hashicorp/terraform/blob/master/CHANGELOG.md#0127-august-22-2019

[Dev25]

I didn't realise this PR + for each was already open and worked on, FWIW i ended up doing a fork that does both of those changes using a map approach

One thing to point out from that is we can have a slightly cleaner interface (imo) for secondary ranges e.g.

  subnets = {
    belgium-gke = {
      subnet_name           = "belgium-gke"
      subnet_ip             = "10.228.0.0/20"
      subnet_region         = "europe-west1"
      subnet_private_access = "true"
      subnet_flow_logs      = "false"
      secondary_ranges = {
        test = "10.240.0.0/14"        
        belgium-gke-services = "10.250.0.0/20"
      }
    },
 }

Where secondary_ip_range is generated via:

  secondary_ip_range = [
    for range_name, ip_cidr_range in lookup(each.value, "secondary_ranges", {}) : {
      range_name    = range_name
      ip_cidr_range = ip_cidr_range
    }
  ]

  # Alternativety we could also do
  dynamic "secondary_ip_range" {
    for_each =  lookup(each.value, "secondary_ranges", {})
    content {
      range_name    = secondary_ip_range.key
      ip_cidr_range = secondary_ip_range.value
    }
  }

[tfhartmann]

@Dev25 Yeah, using a map instead of a list of maps would totally sidestep the problem I ran into with objects.

I really like this simplification I'm totally going to change it!

 secondary_ranges = {
   test = "10.240.0.0/14"        
   belgium-gke-services = "10.250.0.0/20"

[jhulndev]

Hey @tfhartmann! Were you able to try this out by any chance?

@Dev25 Yeah, using a map instead of a list of maps would totally sidestep the problem I ran into with objects.

I really like this simplification I'm totally going to change it!

 secondary_ranges = {
   test = "10.240.0.0/14"        
   belgium-gke-services = "10.250.0.0/20"

@morgante I actually don't have an alternative for what @tfhartmann ran into with the lack of defaults for object values. I opted to still use the object representation, but understand your hesitation.

[tfhartmann]

Hey @tfhartmann! Were you able to try this out by any chance?

Oh boy, it's been so long since I've worked on this branch, I don't actually recall. I put this on hold a bit until #73 and #86 got added. I think changing the secondary networks from a list of maps, to a direct map is just cleaner and I liked it, I don't however recall if it solved the problem I ran into with setting defaults on an object. I suspect not, since (if I recall correct) the reason I needed to use an object in the first place was to be able to have a data structure with a mix of types, So I think you'd still need to use an object to represent a map that included values of both string and map type, but honestly I'm not sure 😢

[Dev25]

Can we look at getting this interface updated/merged in now?, 2.0.0 was released without it.

For reference i have used this specific implementation for more then a couple of months now.
https://github.com/Dev25/terraform-google-network/blob/for-each/main.tf#L52-L58

  secondary_ip_range = [
    for range_name, ip_cidr_range in lookup(each.value, "secondary_ranges", {}) : {
      range_name    = range_name
      ip_cidr_range = ip_cidr_range
    }
  ]

[morgante]

@Dev25 Does that mean you have to define every value for subnets? From my recollection, we never got around the issue of objects lacking defaults.

If your map-based approach works without changing the interface for the rest of the module, I'd be happy to review a PR including it.

[tfhartmann]

Would you like me to refactor this now that 2.0.0 is released?

[morgante]

@tfhartmann I don't think we ever found a solution to being forced to fill in all values (because objects lack defaults)?

[Dev25]

@morgante
Just checked and even with the map you are still forced to define each key for subnets (at least secondary_ranges = {} if not creating a secondary range)

I still think this is a worthy of doing and cutting a 3.0.0 release?

[tfhartmann]

FWIW, the behavior without defaults would track to the UI on other modules in this GitHub org, I was just using terraform-google-modules/terraform-google-cloud-nat and noticed that I had to pass in the object with a "secondary_ip_range_names" = null https://github.com/terraform-google-modules/terraform-google-cloud-nat/blob/master/variables.tf#L90 so, prior art and all that :)

[morgante]

Especially given the addition of the various subnet_flow_log inputs, I'm still concerned about the verbosity of forcing all values to be configured for all subnets. Especially since the only meaningful gain is not having to keep secondary_ranges in a separate map.

[github-actions]

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days