Merge pull request juju#16999 from Aflynn50/move-network-discovery-logic

juju#16999

<!-- Why this change is needed and what it does. -->
The network discovery logic used by the machiner worker, the provisioner worker and the container broker was located in api/common. This is not a good place for it and means that api/common has dependencies on netlink which are not needed.

The discovery logic is moved to core/network, and the return type of the primary function is changed from relying on a type in `params` because this created a cyclic dependency and it is the resposiblity of the call site translate to the serialisable type. 
## Checklist

<!-- If an item is not applicable, use `~strikethrough~`. -->

- [x] Code style: imports ordered, good names, simple structure, etc
- [x] Comments saying why design decisions were made
- [x] Go unit tests, with comments saying what you're testing
- [ ] [Integration tests](https://github.com/juju/juju/tree/main/tests), with comments saying what you're testing

## QA steps
#### Unit tests
```
go test -check.v ./core/network/...
go test -check.v ./worker/machiner/...
go test -check.v ./worker/provisioner/...
go test -check.v ./worker/containerbroker/...
go test -check.v ./container/broker/...
```
#### Integration tests
Bootstrap a manual machine and check that network discovery works:
```
lxc launch ubuntu:22.04
# Note down machine name and ip
lxc list
lxc exec <machine-name> bash
<copy your local ~/.ssh/key.pub onto the machine in /home/ubuntu/.ssh/authorized_keys>
# Bootstrap juju on the manual machine
juju bootstrap manual/ubuntu@<ip>
juju switch controller
# Check that network-interfaces are listed (e.g. eth0)
juju show-machine 0
```
<!-- Describe steps to verify that the change works. -->

## Documentation changes

<!-- How it affects user workflow (CLI or API). -->

## Links

<!-- Link to all relevant specification, documentation, bug, issue or JIRA card. -->

**Jira card:** JUJU-5482

api/common/package_test.go

@@ -9,8 +9,6 @@ import (
 	gc "gopkg.in/check.v1"
 )
 
-//go:generate go run go.uber.org/mock/mockgen -package common_test -destination network_mock_test.go github.com/juju/juju/core/network ConfigSource,ConfigSourceNIC,ConfigSourceAddr
-
 func TestAll(t *testing.T) {
 	gc.TestingT(t)
 }

container/broker/instance_broker.go

@@ -33,7 +33,7 @@ var (
 )
 
 // NetConfigFunc returns a slice of NetworkConfig from a source config.
-type NetConfigFunc func(corenetwork.ConfigSource) ([]params.NetworkConfig, error)
+type NetConfigFunc func(corenetwork.ConfigSource) (corenetwork.InterfaceInfos, error)
 
 // Config describes the resources used by the instance broker.
 type Config struct {
@@ -170,7 +170,11 @@ func acquireLock(config Config) func(string, <-chan struct{}) (func(), error) {
 
 func observeNetwork(config Config) func() ([]params.NetworkConfig, error) {
 	return func() ([]params.NetworkConfig, error) {
-		return config.GetNetConfig(corenetwork.DefaultConfigSource())
+		interfaceInfos, err := config.GetNetConfig(corenetwork.DefaultConfigSource())
+		if err != nil {
+			return nil, err
+		}
+		return params.NetworkConfigFromInterfaceInfo(interfaceInfos), nil
 	}
 }
 

api/common/network.go → core/network/discovery.go

@@ -1,20 +1,14 @@
 // Copyright 2015 Canonical Ltd.
 // Licensed under the AGPLv3, see LICENCE file for details.
 
-package common
+package network
 
 import (
 	"strings"
 
 	"github.com/juju/errors"
-	"github.com/juju/loggo"
-
-	"github.com/juju/juju/core/network"
-	"github.com/juju/juju/rpc/params"
 )
 
-var logger = loggo.GetLogger("juju.api.common")
-
 // GetObservedNetworkConfig uses the given source to find all available network
 // interfaces and their assigned addresses, and returns the result as
 // []params.NetworkConfig. In addition to what the source returns, a few
@@ -33,7 +27,7 @@ var logger = loggo.GetLogger("juju.api.common")
 //     have their type forced to bridge and their virtual port type set to
 //     OvsPort.
 //   - TODO: IPv6 link-local addresses will be ignored and treated as empty ATM.
-func GetObservedNetworkConfig(source network.ConfigSource) ([]params.NetworkConfig, error) {
+func GetObservedNetworkConfig(source ConfigSource) (InterfaceInfos, error) {
 	logger.Tracef("discovering observed machine network config...")
 
 	interfaces, err := source.Interfaces()
@@ -57,26 +51,26 @@ func GetObservedNetworkConfig(source network.ConfigSource) ([]params.NetworkConf
 		return nil, errors.Annotate(err, "retrieving default route")
 	}
 
-	var configs []params.NetworkConfig
+	var configs InterfaceInfos
 	var bridgeNames []string
 	var noAddressesNics []string
 
 	for _, nic := range interfaces {
-		virtualPortType := network.NonVirtualPort
+		virtualPortType := NonVirtualPort
 		if knownOVSBridges.Contains(nic.Name()) {
-			virtualPortType = network.OvsPort
+			virtualPortType = OvsPort
 		}
 
-		nicConfig := interfaceToNetworkConfig(nic, virtualPortType)
+		nicConfig := createInterfaceInfo(nic, virtualPortType)
 
 		if nicConfig.InterfaceName == defaultRouteDevice {
 			nicConfig.IsDefaultGateway = true
-			nicConfig.GatewayAddress = defaultRoute.String()
+			nicConfig.GatewayAddress = NewMachineAddress(defaultRoute.String()).AsProviderAddress()
 		}
 
 		// Collect all the bridge device names. We will use these to update all
 		// the parent device names for the bridge's port devices at the end.
-		if nic.Type() == network.BridgeDevice {
+		if nic.Type() == BridgeDevice {
 			bridgeNames = append(bridgeNames, nic.Name())
 		}
 
@@ -94,8 +88,8 @@ func GetObservedNetworkConfig(source network.ConfigSource) ([]params.NetworkConf
 			// the device, should we ever need that detail.
 			// At present we do not - we only use it to determine if an address
 			// has a configuration method of "loopback".
-			if nic.Type() != network.LoopbackDevice {
-				nicConfig.ConfigType = string(network.ConfigStatic)
+			if nic.Type() != LoopbackDevice {
+				nicConfig.ConfigType = ConfigStatic
 			}
 
 			nicConfig.Addresses, err = addressesToConfig(nicConfig, nicAddrs)
@@ -115,33 +109,33 @@ func GetObservedNetworkConfig(source network.ConfigSource) ([]params.NetworkConf
 	return configs, nil
 }
 
-func interfaceToNetworkConfig(nic network.ConfigSourceNIC,
-	virtualPortType network.VirtualPortType,
-) params.NetworkConfig {
-	configType := network.ConfigManual
-	if nic.Type() == network.LoopbackDevice {
-		configType = network.ConfigLoopback
+func createInterfaceInfo(nic ConfigSourceNIC,
+	virtualPortType VirtualPortType,
+) InterfaceInfo {
+	configType := ConfigManual
+	if nic.Type() == LoopbackDevice {
+		configType = ConfigLoopback
 	}
 
 	isUp := nic.IsUp()
 
 	// TODO (dimitern): Add DNS servers and search domains.
-	return params.NetworkConfig{
+	return InterfaceInfo{
 		DeviceIndex:     nic.Index(),
 		MACAddress:      nic.HardwareAddr().String(),
-		ConfigType:      string(configType),
+		ConfigType:      configType,
 		MTU:             nic.MTU(),
 		InterfaceName:   nic.Name(),
-		InterfaceType:   string(nic.Type()),
+		InterfaceType:   nic.Type(),
 		NoAutoStart:     !isUp,
 		Disabled:        !isUp,
-		VirtualPortType: string(virtualPortType),
-		NetworkOrigin:   params.NetworkOrigin(network.OriginMachine),
+		VirtualPortType: virtualPortType,
+		Origin:          OriginMachine,
 	}
 }
 
-func addressesToConfig(nic params.NetworkConfig, nicAddrs []network.ConfigSourceAddr) ([]params.Address, error) {
-	var res []params.Address
+func addressesToConfig(nic InterfaceInfo, nicAddrs []ConfigSourceAddr) ([]ProviderAddress, error) {
+	var res ProviderAddresses
 
 	for _, nicAddr := range nicAddrs {
 		if nicAddr == nil {
@@ -156,24 +150,24 @@ func addressesToConfig(nic params.NetworkConfig, nicAddrs []network.ConfigSource
 			continue
 		}
 
-		opts := []func(mutator network.AddressMutator){
-			network.WithConfigType(network.AddressConfigType(nic.ConfigType)),
-			network.WithSecondary(nicAddr.IsSecondary()),
+		opts := []func(mutator AddressMutator){
+			WithConfigType(nic.ConfigType),
+			WithSecondary(nicAddr.IsSecondary()),
 		}
 
 		if ipNet := nicAddr.IPNet(); ipNet != nil && ipNet.Mask != nil {
-			opts = append(opts, network.WithCIDR(network.NetworkCIDRFromIPAndMask(ip, ipNet.Mask)))
+			opts = append(opts, WithCIDR(NetworkCIDRFromIPAndMask(ip, ipNet.Mask)))
 		}
 
-		// Constructing a core network.Address like this first,
+		// Constructing a core Address like this first,
 		// then converting, populates the scope and type.
-		res = append(res, params.FromMachineAddress(network.NewMachineAddress(ip.String(), opts...)))
+		res = append(res, NewMachineAddress(ip.String(), opts...).AsProviderAddress())
 	}
 
 	return res, nil
 }
 
-func updateParentsForBridgePorts(config []params.NetworkConfig, bridgeNames []string, source network.ConfigSource) {
+func updateParentsForBridgePorts(config []InterfaceInfo, bridgeNames []string, source ConfigSource) {
 	for _, bridgeName := range bridgeNames {
 		for _, portName := range source.GetBridgePorts(bridgeName) {
 			for i := range config {

api/common/network_test.go → core/network/discovery_test.go

@@ -1,7 +1,7 @@
 // Copyright 2016 Canonical Ltd.
 // Licensed under the AGPLv3, see LICENCE file for details.
 
-package common_test
+package network_test
 
 import (
 	"errors"
@@ -13,9 +13,7 @@ import (
 	"go.uber.org/mock/gomock"
 	gc "gopkg.in/check.v1"
 
-	"github.com/juju/juju/api/common"
 	"github.com/juju/juju/core/network"
-	"github.com/juju/juju/rpc/params"
 )
 
 type networkConfigSuite struct {
@@ -45,7 +43,7 @@ func (s *networkConfigSuite) TestGetObservedNetworkConfigInterfacesError(c *gc.C
 
 	s.source.EXPECT().Interfaces().Return(nil, errors.New("boom"))
 
-	observedConfig, err := common.GetObservedNetworkConfig(s.source)
+	observedConfig, err := network.GetObservedNetworkConfig(s.source)
 	c.Check(err, gc.ErrorMatches, "detecting network interfaces: boom")
 	c.Check(observedConfig, gc.IsNil)
 }
@@ -66,7 +64,7 @@ func (s *networkConfigSuite) TestGetObservedNetworkConfigInterfaceAddressesError
 
 	s.source.EXPECT().Interfaces().Return([]network.ConfigSourceNIC{nic}, nil)
 
-	observedConfig, err := common.GetObservedNetworkConfig(s.source)
+	observedConfig, err := network.GetObservedNetworkConfig(s.source)
 	c.Check(err, gc.ErrorMatches, `detecting addresses for "eth0": bam`)
 	c.Check(observedConfig, gc.IsNil)
 }
@@ -87,7 +85,7 @@ func (s *networkConfigSuite) TestGetObservedNetworkConfigNilAddressError(c *gc.C
 
 	s.source.EXPECT().Interfaces().Return([]network.ConfigSourceNIC{nic}, nil)
 
-	observedConfig, err := common.GetObservedNetworkConfig(s.source)
+	observedConfig, err := network.GetObservedNetworkConfig(s.source)
 	c.Check(err, gc.ErrorMatches, `cannot parse nil address on interface "eth1"`)
 	c.Check(observedConfig, gc.IsNil)
 }
@@ -110,17 +108,17 @@ func (s *networkConfigSuite) TestGetObservedNetworkConfigNoInterfaceAddresses(c
 
 	s.source.EXPECT().Interfaces().Return([]network.ConfigSourceNIC{nic}, nil)
 
-	observedConfig, err := common.GetObservedNetworkConfig(s.source)
+	observedConfig, err := network.GetObservedNetworkConfig(s.source)
 	c.Assert(err, jc.ErrorIsNil)
 
-	c.Check(observedConfig, jc.DeepEquals, []params.NetworkConfig{{
+	c.Check(observedConfig, jc.DeepEquals, network.InterfaceInfos{{
 		DeviceIndex:   2,
 		MACAddress:    "aa:bb:cc:dd:ee:ff",
 		MTU:           1500,
 		InterfaceName: "eth1",
 		InterfaceType: "ethernet",
 		ConfigType:    "manual",
-		NetworkOrigin: "machine",
+		Origin:        network.OriginMachine,
 	}})
 }
 
@@ -156,10 +154,10 @@ func (s *networkConfigSuite) TestGetObservedNetworkConfigDefaultGatewayWithAddre
 
 	s.source.EXPECT().Interfaces().Return([]network.ConfigSourceNIC{nic}, nil)
 
-	observedConfig, err := common.GetObservedNetworkConfig(s.source)
+	observedConfig, err := network.GetObservedNetworkConfig(s.source)
 	c.Assert(err, jc.ErrorIsNil)
 
-	c.Check(observedConfig, jc.DeepEquals, []params.NetworkConfig{
+	c.Check(observedConfig, jc.DeepEquals, network.InterfaceInfos{
 		{
 			DeviceIndex:      2,
 			MACAddress:       "aa:bb:cc:dd:ee:ff",
@@ -168,22 +166,21 @@ func (s *networkConfigSuite) TestGetObservedNetworkConfigDefaultGatewayWithAddre
 			InterfaceType:    "ethernet",
 			ConfigType:       "static",
 			IsDefaultGateway: true,
-			GatewayAddress:   "1.2.3.4",
-			NetworkOrigin:    "machine",
-			Addresses: []params.Address{
-				{
-					Value:      "1.2.3.4",
-					CIDR:       "1.2.3.0/24",
-					ConfigType: "static",
-					Type:       "ipv4",
-					Scope:      "public",
-				}, {
-					Value:       "559c:f8c5:812a:fa1f:21fe:5613:3f20:b081",
-					ConfigType:  "static",
-					IsSecondary: true,
-					Type:        "ipv6",
-					Scope:       "public",
-				},
+			GatewayAddress:   network.NewMachineAddress("1.2.3.4").AsProviderAddress(),
+			Origin:           network.OriginMachine,
+			Addresses: []network.ProviderAddress{
+				network.NewMachineAddress(
+					"1.2.3.4",
+					network.WithCIDR("1.2.3.0/24"),
+					network.WithConfigType("static"),
+					network.WithScope("public"),
+				).AsProviderAddress(),
+				network.NewMachineAddress(
+					"559c:f8c5:812a:fa1f:21fe:5613:3f20:b081",
+					network.WithConfigType("static"),
+					network.WithSecondary(true),
+					network.WithScope("public"),
+				).AsProviderAddress(),
 			},
 		},
 	})
@@ -208,18 +205,18 @@ func (s *networkConfigSuite) TestGetObservedNetworkConfigForOVSDevice(c *gc.C) {
 
 	s.source.EXPECT().Interfaces().Return([]network.ConfigSourceNIC{nic}, nil)
 
-	observedConfig, err := common.GetObservedNetworkConfig(s.source)
+	observedConfig, err := network.GetObservedNetworkConfig(s.source)
 	c.Assert(err, jc.ErrorIsNil)
 
-	c.Check(observedConfig, jc.DeepEquals, []params.NetworkConfig{{
+	c.Check(observedConfig, jc.DeepEquals, network.InterfaceInfos{{
 		DeviceIndex:     2,
 		MACAddress:      "aa:bb:cc:dd:ee:ff",
 		MTU:             1500,
 		InterfaceName:   "ovsbr0",
 		InterfaceType:   "bridge",
 		VirtualPortType: "openvswitch",
 		ConfigType:      "manual",
-		NetworkOrigin:   "machine",
+		Origin:          network.OriginMachine,
 	}})
 }
 
@@ -253,10 +250,10 @@ func (s *networkConfigSuite) TestGetObservedNetworkConfigBridgePortsHaveParentSe
 
 	s.source.EXPECT().Interfaces().Return([]network.ConfigSourceNIC{nic1, nic2}, nil)
 
-	observedConfig, err := common.GetObservedNetworkConfig(s.source)
+	observedConfig, err := network.GetObservedNetworkConfig(s.source)
 	c.Assert(err, jc.ErrorIsNil)
 
-	c.Check(observedConfig, jc.DeepEquals, []params.NetworkConfig{
+	c.Check(observedConfig, jc.DeepEquals, network.InterfaceInfos{
 		{
 			DeviceIndex:         2,
 			MACAddress:          "aa:bb:cc:dd:ee:ff",
@@ -265,7 +262,7 @@ func (s *networkConfigSuite) TestGetObservedNetworkConfigBridgePortsHaveParentSe
 			InterfaceType:       "ethernet",
 			ParentInterfaceName: "br-eth1",
 			ConfigType:          "manual",
-			NetworkOrigin:       "machine",
+			Origin:              network.OriginMachine,
 		},
 		{
 			DeviceIndex:   3,
@@ -274,7 +271,7 @@ func (s *networkConfigSuite) TestGetObservedNetworkConfigBridgePortsHaveParentSe
 			InterfaceName: "br-eth1",
 			InterfaceType: "bridge",
 			ConfigType:    "manual",
-			NetworkOrigin: "machine",
+			Origin:        network.OriginMachine,
 		},
 	})
 }

core/network/package_test.go

@@ -10,6 +10,8 @@ import (
 	gc "gopkg.in/check.v1"
 )
 
+//go:generate go run go.uber.org/mock/mockgen -package network_test -destination discovery_mock_test.go github.com/juju/juju/core/network ConfigSource,ConfigSourceNIC,ConfigSourceAddr
+
 func TestPackage(t *testing.T) {
 	gc.TestingT(t)
 }