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

BGP peer options not applied in FAST? (02-networking-peering) #1223

Closed
MichaelHusbyn opened this issue Mar 8, 2023 · 4 comments · Fixed by #1228
Closed

BGP peer options not applied in FAST? (02-networking-peering) #1223

MichaelHusbyn opened this issue Mar 8, 2023 · 4 comments · Fixed by #1228
Assignees
Labels
bug Something isn't working

Comments

@MichaelHusbyn
Copy link

MichaelHusbyn commented Mar 8, 2023

Hi

Not sure exactly how this is supposed to work but from my experience I did not get it to work in standard fast. Will try to explain

Goal is to add custom routing to VPN that goes from Google Cloud to on-prem. I am using v20, but from what I can tell it seems nothing have changed in later revisions.

Using https://github.com/GoogleCloudPlatform/cloud-foundation-fabric/tree/master/fast/stages/2-networking-a-peering
I use terraform.tfvars, but example is straight from fast (variables.tf)

variable "vpn_onprem_configs" {
..
  default = {
    landing-primary = {
      adv = {
        default = false
        custom = [
          "cloud_dns", "googleapis_private", "googleapis_restricted", "gcp_all"
        ]
      }

So basicly this should indicate that you want to have those networks routed on the VPN. But this never happen.

vpn-onprem.tf does a number out of reading this variable and converting it to new variable: bgp_peer_options_onprem

<snip>
  router_config = {
    name = "landing-onprem-vpn-${local.region_shortnames[var.regions.primary]}"
    asn  = var.router_onprem_configs.landing-primary.asn
  }
  peer_gateways = {
    default = {
      external = var.vpn_onprem_configs.landing-primary.peer_external_gateway
    }
  }
  tunnels = {
    for t in var.vpn_onprem_configs.landing-primary.tunnels :
    "remote-${t.vpn_gateway_interface}-${t.peer_external_gateway_interface}" => {
      bgp_peer = {
        address = cidrhost(t.session_range, 1)
        asn     = t.peer_asn
      }
      bgp_peer_options                = local.bgp_peer_options_onprem.landing-primary
      bgp_session_range               = "${cidrhost(t.session_range, 2)}/30"
      peer_external_gateway_interface = t.peer_external_gateway_interface
      shared_secret                   = t.secret
      vpn_gateway_interface           = t.vpn_gateway_interface
    }
  }

Here is bgp_peer_options the key. This contains the networks that you want to route.
But module net-vpn-ha does not know of any bgp_peer_options. Not in tunnel or in router_config. So it is simply ignored.


variable "router_config" {
  description = "Cloud Router configuration for the VPN. If you want to reuse an existing router, set create to false and use name to specify the desired router."
  type = object({
    create    = optional(bool, true)
    asn       = number
    name      = optional(string)
    keepalive = optional(number)
    custom_advertise = optional(object({
      all_subnets = bool
      ip_ranges   = map(string)
    }))
  })
  nullable = false
}

variable "tunnels" {
  description = "VPN tunnel configurations."
  type = map(object({
    bgp_peer = object({
      address        = string
      asn            = number
      route_priority = optional(number, 1000)
      custom_advertise = optional(object({
        all_subnets          = bool
        all_vpc_subnets      = bool
        all_peer_vpc_subnets = bool
        ip_ranges            = map(string)
      }))
    })
    # each BGP session on the same Cloud Router must use a unique /30 CIDR
    # from the 169.254.0.0/16 block.
    bgp_session_range               = string
    ike_version                     = optional(number, 2)
    peer_external_gateway_interface = optional(number)
    peer_gateway                    = optional(string, "default")
    router                          = optional(string)
    shared_secret                   = optional(string)
    vpn_gateway_interface           = number
  }))
  default  = {}
  nullable = false
}

It knows about custom_advertise in the other hand. And to my knowledge this is where this IP ranges should be

So in my setup I did this in the 02-networking-peering part (vpn-onprem.tf)

module "landing-to-onprem-en1-vpn" {
  count      = local.enable_onprem_vpn ? 1 : 0
  source     = "<snipped due to github integration>"
  project_id = module.landing-project.project_id
  network    = module.landing-vpc.self_link
  region     = "europe-north1"
  name       = "vpn-to-onprem-en1"
  router_config = {
    name = "landing-onprem-vpn-en1"
    asn  = var.router_onprem_configs.landing-en1.asn
    custom_advertise = {
      all_subnets = false
      ip_ranges   = local.bgp_peer_options_onprem.landing-en1.advertise_ip_ranges
    }
  }

custom_advertise is new. Rebranded from ew1 to en1 to be clear but still the same concept. Also different names on variables from last revision since this is based on v20.

This way I get the cloud router managing the VPN to publish the routes I wan to. Did not push any routes directly on the tunnel (they are running default inherited from cloud router)

Sorry for all the pasting.

This is not a problem with using VPN between the shared VPC projects, default is to publish all known routes/networks.

So question, if at all clear from the above, do I do something wrong, should standard work or is the missing 4 lines the key?

Cheers, thanks for good framework

// Michael

@ludoo
Copy link
Collaborator

ludoo commented Mar 9, 2023

Michael, thanks for reporting this. The VPN variables in network stages need a serious overhaul, I started on it last week then got sidetracked, this is just further proof. They made sense to us when we originally wrote these stages, but the VPN module then evolved and these have become problematic.

My plan is to remove support for the (broken) templating support and make them more transparent vs the module variables, and to add support for multiple peer gateways which I added to the VPN module a few days ago.

Sorry for this, it's top on our list of things to fix.

@ludoo ludoo added the bug Something isn't working label Mar 9, 2023
@ludoo ludoo self-assigned this Mar 9, 2023
@MichaelHusbyn
Copy link
Author

Ok, thanks for working on it. Got to learn more TF with this issue, so not all bad :)

@MichaelHusbyn
Copy link
Author

Checked your fixes and it looks good.
Is it a good option to still have example with routing for cloud_dns, or the other networks that was in the example before?

@ludoo
Copy link
Collaborator

ludoo commented Mar 9, 2023

good point, I'll add those ranges to the example

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants