Network sanity checks #277

MggMuggins · 2024-04-03T22:54:03Z

I will rebase once #276 is merged, the only new commit here is this one.

microcloud/cmd/microcloud/main_init.go

MggMuggins · 2024-04-04T15:56:17Z

@masnax Would you be willing to take a quick pass at this? I'm looking for feedback on if the tests look reasonable as much as anything. Realize I missed testing the case where other InitSystem addresses conflict with the uplink gateway. I'll add that and fix the linter this afternoon.

microcloud/cmd/microcloud/main_init.go

masnax

@MggMuggins There's one more component to the networking questions: the dns nameservers we pass to LXD.

Can you also include a check that verifies that the input is a comma-separated list of IPs?

It's part of the same UPLINK network struct as the other fields, accessed by the config key dns.nameservers

microcloud/microcloud/service/lxd_config.go

Lines 88 to 90 in a28d437

    
           if dnsServers != "" { 
        
           	finalUplinkCfg.Config["dns.nameservers"] = dnsServers 
        
           }

…ster Fixes canonical#210 1. Invalid OVN ranges in UPLINK networks (range outside gateway subnet, etc) 2. UPLINK OVN ranges that contain any system's management address; 3. Invalid dns nameserver IPs Signed-off-by: Wesley Hershberger <[email protected]>

tomponline · 2024-04-19T07:10:17Z

microcloud/cmd/microcloud/main_init.go

+	var ip4OVNRanges, ip6OVNRanges []*shared.IPRange
+
+	for _, network := range curSystem.Networks {
+		if network.Type == "physical" && network.Name == "UPLINK" {


I'm not enjoying the hardcoded "UPLINK" here, we should be using a constant for "UPLINK" and then ensuring that everywhere that needs to reference the uplink network by name uses this constant - this allows for quick understanding of the uses of that network and updating of it if needed.

I've burying configuration assumptions in lower level functions like this can lead to brittle code in the future if those assumptions change and its forgotten/unknown about where to update.

Alternatively we could have the dependencies of the validateSystems function accept them as arguments, i.e uplinkNetName string and have it passed in by the caller so we aren't tightly coupling things together.

@MggMuggins you can use this method to grab the corresponding network type. These methods are how we keep the payloads that we send to LXD consistent.

You can either instantiate a new LXDService or extract the receiver from handler.Services[types.LXD].(*service.LXDService) from the service.Handler that carries through the whole init.

tomponline

Apart from the hardcoded UPLINK, which can be addressed in a separate PR, this looks great thanks.

changes addressed

MggMuggins commented Apr 3, 2024

View reviewed changes