Move to for_each + revamp interface/functionality

[Dev25]

This PR consists of multiple new features + moving to for_each and is something that i've been using in production for managing complex LBs (effecitvely a multi gke cluster ingress with multiple backends + specific routing)

Do we agree on the new interface? Once there is a consensus i can look at updating examples/fixing TODOs etc

Changes:

• Move to for_each for backends/health check state Add support for Terraform 0.12 #51 (comment)
• Allow existing IP address to be used: fixes Support reserved IP address #25
• Health Checks: Allow specifying custom http/https/http2 health checks fixes HTTPS backend not available #34 fixes Possibility to add host for the LB health_check #31
• NEG backends: max_connections_per_endpoint + max_rate_per_endpoint
• Combine backend variables into a map/object fixes Combine backend variables #53
• Allow ssl_policy to be set for restricting TLS ciphers/protocols on frontend
• Allow QUIC to be enabled on frontend
• Outputs:
  • Expose http/https proxy allowing end users to create forwarding rules for linking multiple IPs
  • Convert backend_service to output all resources as a map
    • This allows end users to have custom URL Map routing when working with multiple backends

Below is an example of usage, for better readability with lots of backends i merge a default null map for group backend params

resource "google_compute_url_map" "global" {
  name            = "gke-lb"
  default_service = "${module.lb_http.backend_services["default"].self_link}"
}

module "lb_http" {
  project = var.project
  name    = "example"
  
  http_forward         = false
  ssl                  = true
  ssl_policy           = google_compute_ssl_policy.default.self_link
  use_ssl_certificates = true
  ssl_certificates = [ data.google_compute_ssl_certificate.default.self_link ]
  security_policy = null

  quic = true
  cdn  = false

  firewall_networks = []
  target_tags       = []

  // Use self managed address
  create_address = false
  address        = google_compute_global_address.gke.address

  // Use self manged url map
  url_map        = google_compute_url_map.global.self_link
  create_url_map = false

  backends = {
    "default" = {
      protocol    = "HTTP2"
      port_name   = "https"
      timeout_sec = 60
      enable_cdn  = false

      health_check = {
        check_interval_sec  = 5
        timeout_sec         = 5
        healthy_threshold   = 1
        unhealthy_threshold = 3

        http2_health_check = {
          request_path = "/healthz"
          port         = 443
        }
      }

      groups = [
        merge(var.default_backend_params, {
          group                 = data.google_compute_network_endpoint_group.belgium_d.self_link
          balancing_mode        = "RATE"
          max_rate_per_endpoint = 100
          description           = "GKE managed standlone NEG for nginx-ingress"
        }),
        merge(var.default_backend_params, {
          group                 = data.google_compute_network_endpoint_group.belgium_c.self_link
          balancing_mode        = "RATE"
          max_rate_per_endpoint = 100
          description           = "GKE managed standlone NEG for nginx-ingress"
        }),
      ]
    },
  }
}

variable "default_backend_params" {
  type = "map"
  default = {
    balancing_mode               = null
    capacity_scaler              = null
    description                  = null
    max_connections              = null
    max_connections_per_instance = null
    max_connections_per_endpoint = null
    max_rate                     = null
    max_rate_per_instance        = null
    max_rate_per_endpoint        = null
    max_utilization              = null
  }
}

Fixes #59.

[onetwopunch]

Other than a few nits, I personally agree with most the interface changes except the health checks. It does make things more readable IMO. I'll let one of the other contributers weigh in as well since this would be a breaking change, but I'd suggest you go ahead and uncomment the TODOs and re-push and we can continue the discussion.

[morgante]

Overall I think the new interface seems reasonable and would be happy to look at a full PR.

[Dev25]

Cool, i'll fix the nits raised and give you both a shout when its ready.

re: health checks.
I already moved to a cleaned up interface which infers the type using backend.protocol (see Dev25@206394a) which i will add to this PR, how does that look to you @onetwopunch

      health_check = {
        check_interval_sec  = 5
        timeout_sec         = 5
        healthy_threshold   = 1
        unhealthy_threshold = 3
        port                = 443
        request_path        = "/healthz"
        host                =  "localhost"
      }

[onetwopunch]

@Dev25 I think in general to adhere to a dependency injection pattern and for maintainability of the module, it makes more sense to allow the user to pass in a google_compute_health_check reference as opposed to passing in all the options and creating one in the module. This gives users more flexibility and removes the need to keep this object and documentation up to date over time with the options of google_compute_health_check.

[Dev25]

@onetwopunch

For whats its worth i do see value in this module creating basic health checks internally rather then forcing all users to create them outside. Easier to get started with this module and it's unlikely most users would want very specialised checks e.g. response body checking/setting a proxy header where your right we would probably just want to pass a self_link override.

Given GCP health checks have already migrated from google_compute_http*_health_check to a single unified resource it would be unlikely there would be much change over time to the interface (maybe HTTP3 in a couple years) given the resource itself is fairly basic/non moving target.

Perhaps meet in the middle and do both? Define a basic set of params allowed (thresholds + path + port + host) but also allow users to provide a self_link override when they want to specialise further

Regarding fixing the tests etc, i assume we want to wait for PR #58 to be merged in first and then i can update this/get CI tests passing?

[onetwopunch]

I definitely see your point about wanting to make the health checks simpler for users but my concern is that this adds much more complexity to the module which could be more difficult to maintain especially with the new interface for compute_health_check. Every module needs examples where we can show users how to do this. Personally I don't think it's unreasonable to expect users to create their own health checks. For example:

resource "google_compute_health_check" "http2" {
 name = "https-health-check"
 http2_health_check {
   port = "443"
   request_path = "/health"
 }
}

module "lb_http" {
  ...
  backends = {
    "default" = {
        protocol    = "HTTP2"
        port_name   = "https"
        timeout_sec = 60
        enable_cdn  = false

        health_check = google_compute_https_health_check.http2.self_link
     }
  }
...
}

IMO, the health check piece seems unnecessarily complex to add this logic into the module itself. For the same reason we don't create a VPC, etc. in the terraform-google-vm or this module even though a VPC resource is expected to exist and be passed in by the user. I know this module currently allows the user to create health checks, but if we're massively changing the interface anyway, I think it's a good time to remove this explicit dependency to adhere better to separation of concerns. I'm happy to be overruled by @morgante or @aaron-lane if they disagree though.

[morgante]

I understand the complexity involved, but I'm in favor of keeping health checks embedded inside the module for now. We might remove it in the future, but for now I want to minimize breaking changes.

[Dev25]

FYI the join is a workaround from hashicorp/terraform#16726 (comment) otherwise users will get errors whenever they try to import a resource into their tfstate (any resource, happened to me for GKE node pools)

Error: Invalid index

  on .terraform/modules/lb_http/main.tf line 22, in locals:
  22:   address = compact(concat([var.address], google_compute_global_address.default.*.address), )[0]
    |----------------
    | google_compute_global_address.default is empty tuple
    | var.address is null

The given key does not identify an element in this collection value.


Error: Invalid index

  on .terraform/modules/lb_http/main.tf line 23, in locals:
  23:   url_map = compact(concat([var.url_map], google_compute_url_map.default.*.self_link), )[0]
    |----------------
    | google_compute_url_map.default is empty tuple
    | var.url_map is null

The given key does not identify an element in this collection value.


Error: Invalid index

  on .terraform/modules/gke/modules/beta-private-cluster/main.tf line 37, in locals:
  37:   region   = var.region == null ? join("-", slice(split("-", var.zones[0]), 0, 2)) : var.region
    |----------------
    | var.zones is empty list of string

The given key does not identify an element in this collection value.

[Dev25]

Took a while but CI tests pass, PTAL @morgante @onetwopunch will do a CHANGELOG now.

[morgante]

Thanks for your patience! Unfortunately right now it's challenging to allow public access to Cloud Build logs unfortunately.

[Dev25]

Updated with latest master, one thing i noticed is the README/make generate_docs is busted on both master and this PR.

Looks like it doesn't play nice with the backends object map

	gcr.io/cloud-foundation-cicd/cft/developer-tools:0.5.2 \
	/bin/bash -c 'source /usr/local/bin/task_helper_functions.sh && generate_docs'
Generating markdown docs with terraform-docs
Working in . ...
2019/11/06 14:02:22 At 53:39: Unknown token: 53:39 IDENT string
Warning! Exit code: 1

I've managed to get it to run by temporarily setting the backends type to just type = map

[morgante]

@Dev25 Good catch! I've opened #66 to track.

[onetwopunch]

Thanks a ton for all your work on this @Dev25

[onetwopunch]

@aaron-lane and @morgante I added an PR for an upgrade guide for upgrading from 2.0 using the multi-mig example. #68.

@Dev25 this upgrade process was not trivial due to the interdependence of url_map and backend_service. If url_map is created by terraform (maybe even if not?), there are several errors emitted in upgrading. Since:

1. backend_service needs to be destroyed and re-created for the migration,
2. the url_map is using the backend_service, and
3. the url_map is not a dependency of backend_service (according to Terraform),

the url_map is not updated first with the new backend. So when creating, the url_map is dependent on the backend existing, but when migrating, the backend is dependent on url_map. Meaning you will have to update the url_map's default_service manually then re-apply. I'm not sure how to get around this, but if you have ideas, please let me know.

To verify this, try the following:

git checkout master
cd examples/mig-nat-http-lb
terraform apply
git checkout Dev25/v2
terraform apply

[Dev25]

@onetwopunch

The interconnected nature of a global LB definately complicates things here in a single module, prior/outside of this module for complex LBs (multiple backends + IPs) i have a module dedicated just for backends, another for front end (binding a url map to forwarding rules) and then manage the URLMap on its own. Maybe this module could be broken down into sub modules to promote that style? I find it easier to keep backends separate and then wire them together, especially when dealing with multiple domains/forwarding rules (max 15 certs per IP atm) fronting the same backends.

Regarding upgrading from 2->3, i didn't have to do the upgrade myself but if you had to do it in place did you consider manually removing the old state, creating the new backends/HC via apply -target=module.lb-http.resource and then finally doing the URLMap apply to pick up the new backend resources?

Definitely not the most straight forward upgrade process, especially with having to manually remove resources outside of terraform, but would at least be graceful.

[naseemkullah]

Can' t wait to use this 👍
Is there an ETA for merging this?

[onetwopunch]

@morgante is there anything else you see that needs to be done here other than @Dev25 rebasing on master to remove the conflicts?

[Dev25]

PR has been updated with latest master.

[naseemkullah]

What's holding this off?

[naseemkullah]

🎉