Network Connectivity Center module

[juliodiez]

Creation and management of NCC-based hub-and-spoke architectures.

[LucaPrete]

Good stuff @juliodiez !
My comments are all meant to propose to modify this into a single NCC spoke (RA) module. I think it would make the module much simpler and useful (btw, we would need exactly that in the new networking stage of FAST).

[juliocc]

Hey @juliodiez, overall I like the idea of this PR but I'm wondering if this makes more sense as a blueprint.

Consider that:

• The NCC hub resource itself is rather simple, most of the complexity is actually on the Cloud Router.
• In other modules where a CR is required, we always give the option to reuse an existing one. Would that make sense here?

With that in mind, of you remove the CR creation/management from this module, then it becomes a very thin layer around the two NCC resources.

wdyt?

[juliodiez]

@LucaPrete I don't see good reasons to do that. The module would be simpler, sure, but also for sure it wouldn't be more useful to be able to create one spoke only instead of multiple.
@juliocc Right, complexity is mostly on the CR but linked to NCC with NVAs. Enough in my opinion as to have this module. Creating a blueprint where you configure NCC and multiple spokes "by hand" would show this complexity. I did it, and that's why I wanted to encapsulate it and created this module. Also as said, these CRs are closely linked to NCC & NVAs and they can't be reused for other tasks (you can't interface them to e.g. a VPN tunnel).

[LucaPrete]

@juliodiez what if we'll need to add different spokes to the hub? Also, I'd honestly prefer to call 1 ncc-ra-spoke module per spoke from outside and iterate with a for_each to create more, then having one huge module with multiple spokes...but hey, personal preference

@juliocc I think it makes sense to have a module for this. We talked about this for a long time in the new net stage of FAST. It will cleanup the code quite a lot

[juliodiez]

@LucaPrete

• "what if we'll need to add different spokes to the hub?": sorry, I don't get the question, that's exactly what the module can easily do, see the README.
• The downside I see in having one ncc-ra-spoke module and iterate with for_each is that the user needs to manage complexity in code creating the logic by themselves, whereas the module does this work for them offering a data interface instead.

[ludoo]

@juliodiez can we add a blueprint that shows how to use the module, and why?

[juliodiez]

@ludoo Sure, I will create one. I would ask if having a blueprint is not independent of the module, but I presume you also doubt whether this should be a module. I see NCC as a design resource with elements quite coupled without benefit by breaking them apart, let's see if a blueprint adds to this argument.

[ludoo]

@ludoo Sure, I will create one. I would ask if having a blueprint is not independent of the module, but I presume you also doubt whether this should be a module. I see NCC as a design resource with elements quite coupled without benefit by breaking them apart, let's see if a blueprint adds to this argument.

Exactly. We won't know if the design is the best one until we actually use the module. So while the module will undoubtedly change a few times throughout its life, we should start with a good fit with at least one use case. I know this is a bit of a PITA, but it would be my preferred way of bringing this in. :)

[LucaPrete]

@juliodiez apologies I wrote my comment quickly in between call and might have been misunderstood.

My doubt is how to integrate this with other types of spokes, without making this module becoming a "huge monster" :)
For example, what if you'll need to add a VPC or a VPN type of NCC spoke? That's why I'd really recommend, btw reflecting what Fabric does in many other places, to make the hub at a very least optional. At that point, I think using a module construct for each spoke and a hub in common would give us the ability to keep the module useful but simple enough.

Ideally, in a blueprint (or in the stage02) I'd like to see something like

resource "hub" "my_hub" {
...
}

module "spoke_ra_1" {
   hub    = module.my_hub.id
   region = "europe-west1"
   routes = ["192.168.0.0/24", "192.168.1.0/24"]
   vms    = ["vm1", "vm2"]
   ...
}

module "spoke_ra_2" {
   hub    = module.my_hub.id
   region = "europe-west1"
   routes = ["192.168.0.0/24", "192.168.1.0/24"]
   vms    = ["vm1", "vm2"]
   ...
}
   ...
}

In the future things like

module "spoke_vpc_1" {
   hub    = module.my_hub.id
   region = "europe-west1"
   vpc    = "my_vpc"
   ...
}

and so on...

...I think the blueprint will end up being at this stage the same of the readme :)
Of course, open to other options. Just my2c.

[LucaPrete]

Besides the minor comments, it looks great @juliodiez !
Waiting for it to be merged so we can use it right away in stage-2 :)

[juliodiez]

All comments addressed, PTAL.
One point: in custom_advertise I defined ip_ranges as a map of descriptions and address ranges, e.g.
"peered-vpc" = "10.10.0.0/24"
when it's usually the opposite, a map of address ranges and descriptions. That doesn't seem intuitive to me but if you force me I will change it.

[ludoo]

All comments addressed, PTAL.

Can you resolve them?

One point: in custom_advertise I defined ip_ranges as a map of descriptions and address ranges, e.g. "peered-vpc" = "10.10.0.0/24" when it's usually the opposite, a map of address ranges and descriptions. That doesn't seem intuitive to me but if you force me I will change it.

in the VPN module we use ranges as keys. The idea is that values are then marked optional in the type. I agree it's counterintituive and am ok changing to names as keys but if you want to do that, we need to do it for every module. :)

[juliodiez]

"values are then marked optional in the type" -> I don't get it. In both cases, vpn or my module, the type is map(string) and the semantics are in the assignment, kind of:

  range           = range.key
  description = range.value

vs

  range           = range.value
  description = range.key

[juliodiez]

(btw, I've always thought that who opens a comment should resolve it :)

[ludoo]

"values are then marked optional in the type" -> I don't get it. In both cases, vpn or my module, the type is map(string) and the semantics are in the assignment, kind of:

  range           = range.key
  description = range.value

vs

  range           = range.value
  description = range.key

true :) maybe in the CR advertisement? I'm pretty sure that was the rationale; regardless, we should have all modules use the same interface

[ludoo]

(btw, I've always thought that who opens a comment should resolve it :)

Niet :) once you address resolve, burden is on you not on the reviewer

[LucaPrete]

Besides last minor comments on the outputs lgtm!

[juliodiez]

Ofc I'll abide by the rules and resolve, but don't agree ;) (e.g. random ref.)

[ludoo]

Ofc I'll abide by the rules and resolve, but don't agree ;) (e.g. random ref.)

Good! Just send a PR after this one to invert the type anywhere we're using ranges. Tests should make it pretty easy to spot issues.

[juliodiez]

No, no. I changed semantics and left as the rest of code (IP range = key). I meant I don't agree with who should resolve a comment

[ludoo]

No, no. I changed semantics and left as the rest of code (IP range = key). I meant I don't agree with who should resolve a comment

let me put it this way: as a reviewer, I trust you to do the right thing once a comment has been sent :)