Addresses Issue #248 (Add Support for Exporting PUPIs - Network Peering Module)

[rglenn-accenture]

This introduces support for the export_subnet_routes_with_public_ip and import_subnet_routes_with_public_ip variables in the google_compute_network_peering resources. This is necessary when peering VPCs that are using Privately Used Public IPs (PUPIs) as the private subnet IPs.

Note - the defaults proposed in this PR are false for both variables, but the provider's default for export_subnet_routes_with_public_ip is true (the import variable is also set to false, by default, here); because these come in pairs (defining each end of the peering), and because the current Network Peering submodule doesn't override these defaults, it will cause each peering will attempt to export these subnets but neither peering will attempt to import them; not a problem, really, but the proposed solution also addresses this incongruence, by default.

Fixes #248

[comment-bot-dev]

Thanks for the PR! 🚀
✅ Lint checks have passed.

[morgante]

Let's make the variable names and defaults match the provider.

[rglenn-accenture]

Let's make the variable names and defaults match the provider.

A full match for the names doesn't make sense, given the provider handles one side of the connection, but the Network Peering module handles both sides. I'll match the naming convention of the other such variables in this module (introducing the interstitial "peer" or "local" designations).

[morgante]

Good point, yes let's match the existing convention.

[rglenn-accenture]

Let's make the variable names and defaults match the provider.

Actually, for the default values, note that to make export_subnet_routes_with_public_ip true for both local and remote connections, we must negate the values for one side of the peering connection for it to match the provider defaults:

resource "google_compute_network_peering" "local_network_peering" {
  provider             = google-beta
  name                 = local.local_network_peering_name
  network              = var.local_network
  peer_network         = var.peer_network
  export_custom_routes = var.export_local_custom_routes
  import_custom_routes = var.export_peer_custom_routes

  # export_local_subnet_routes_with_public_ip defaults to true, to match the provider default
  export_subnet_routes_with_public_ip = var.export_local_subnet_routes_with_public_ip
  # export_peer_subnet_routes_with_public_ip defaults to false, to match the provider default
  import_subnet_routes_with_public_ip = var.export_peer_subnet_routes_with_public_ip

  depends_on = [null_resource.module_depends_on]
}

resource "google_compute_network_peering" "peer_network_peering" {
  provider             = google-beta
  name                 = local.peer_network_peering_name
  network              = var.peer_network
  peer_network         = var.local_network
  export_custom_routes = var.export_peer_custom_routes
  import_custom_routes = var.export_local_custom_routes

  # export_peer_subnet_routes_with_public_ip must be negated, to match the provider default
  export_subnet_routes_with_public_ip = var.export_peer_subnet_routes_with_public_ip ? false : true
  # export_local_subnet_routes_with_public_ip must be negated, to match the provider default
  import_subnet_routes_with_public_ip = var.export_local_subnet_routes_with_public_ip ? false : true

  depends_on = [null_resource.module_depends_on, google_compute_network_peering.local_network_peering]
}

I believe this might actually prevent certain valid route imports/exports.

I would imagine a prudent alternative would be to align the default values to the local peering, only, and acknowledge in the documentation that the remote peering (by default) will have exact-opposite-from-default controls for the subnet routes with public ips.

Another alternative would be to have export_peer_subnet_routes_with_public_ip, import_peer_subnet_routes_with_public_ip, export_local_subnet_routes_with_public_ip, import_local_subnet_routes_with_public_ip, but this seems unnecessary given the problem this module seems to address (which is ostensibly to simplify creating the peering connection, and avoid partial configuration due to exported routes that are not imported on the other end).

I will target the former option in an updated PR, but feel free to suggest a better approach.

[morgante]

The former sounds good, thanks. Let's just document it clearly.

[rglenn-accenture]

I'm not sure why the lint trigger failed. The make docker_test_lint command seems to succeed for me locally, and I think my local copy of the gcr.io/cloud-foundation-cicd/cft/developer-tools:0.13 should be the same as what is used in the cloudbuild trigger (assuming it uses this ./build/lint.cloudbuild.yaml in the lint-trigger).
Is there any additional information in the CloudBuild logs?