diff --git a/README.md b/README.md index cc8d408..6365848 100644 --- a/README.md +++ b/README.md @@ -37,14 +37,14 @@ backend is logged but does not abort the others. **Network configuration (persistence)** -| Backend | Detection | -| ------------------- | ------------------------------------------- | -| netplan | `netplan` binary on `PATH` | -| cloud-init | cloud-init network config present | -| NetworkManager | `NetworkManager.service` active | -| systemd-networkd | `systemd-networkd.service` active | -| RHEL network-scripts| `network.service` active (config dir on legacy hosts) | -| ifupdown | `/etc/network/interfaces` | +| Backend | Detection | +| ------------------- | ----------------------------------------------------- | +| netplan | `netplan` binary on `PATH`, and `netplan info` succeeds | +| cloud-init | cloud-init network config present | +| NetworkManager | `NetworkManager` service running or enabled, or its pid file present | +| systemd-networkd | `systemd-networkd` service running or enabled | +| RHEL network-scripts| `network` service running or enabled | +| ifupdown | `/etc/network/interfaces`, and the `networking` service running or enabled | **Control panels** @@ -54,11 +54,12 @@ backend is logged but does not abort the others. | Plesk | `/usr/sbin/plesk` | | InterWorx | `/usr/bin/nodeworx` | -Detection of the service-managed backends (NetworkManager, systemd-networkd, -and RHEL network-scripts) uses systemd over D-Bus, with a fallback for legacy -systems that lack a usable systemd D-Bus interface (Ubuntu 14.04 Upstart, -CentOS 5/6). On that fallback path network-scripts is detected by the presence -of its `/etc/sysconfig/network-scripts` directory. +### How service-managed backends are detected + +A service counts as managing the network when it is **running now** or +**enabled to start at boot**. Enablement matters on its own: this library writes +configuration that must survive a reboot, and a manager that is enabled but not +yet started still owns the network after the next boot. ## Platforms @@ -71,7 +72,7 @@ of its `/etc/sysconfig/network-scripts` directory. ```go type Configurator interface { // Enumerate interfaces with their addresses, gateways, static routes, - // DNS servers, and search domains. + // DNS servers, search domains, and DHCP client state. GetInterfaces(ctx context.Context) ([]*Interface, error) // Add an IP address (optionally setting/replacing the default gateway). @@ -89,15 +90,23 @@ type Configurator interface { // Set the DNS servers and search domains for an interface. SetDNS(ctx context.Context, iface string, servers []net.IP, searchDomains []string) error + + // Turn each address family's DHCP client on or off. + SetDHCP(ctx context.Context, iface string, dhcp4, dhcp6 bool) error } // Construct the configurator for the current OS, auto-detecting backends. -// Behaviour is tuned with Option values (see Options below). -func NewConfigurator(opts ...Option) (Configurator, error) +// ctx bounds the detection; behaviour is tuned with Option values (see +// Options below). +func NewConfigurator(ctx context.Context, opts ...Option) (Configurator, error) // Resolve an interface by name, or by the special "public-internet" / // "public-internet-6" selectors. func FindInterfaceByName(name string, ifaces []*Interface) *Interface + +// Filter to the hardware-backed interfaces, best candidate for internet +// configuration first. +func FindPhysicalInterfaces(ifaces []*Interface) []*Interface ``` Every operation takes a `context.Context`. The slow steps — the ICMP probe of a @@ -110,6 +119,16 @@ deadlines, so a caller can bound how long a change may block. resolver so they take effect immediately, and persists them through whichever network management backends are detected on the host so they survive a reboot. +### DHCP + +`SetDHCP` turns each address family's DHCP client on or off independently — +moving an interface to DHCPv4 while keeping a static IPv6 address is +`SetDHCP(ctx, "eth0", true, false)`. Enabling a family acquires a lease now +through the detected manager; disabling only rewrites the configuration, leaving +an existing lease to expire so a caller connected over the leased address is not +cut off. `AddAddress` never changes DHCP state — it adds static addresses only. +`Interface.DHCP4` and `Interface.DHCP6` report the state back. + ### Logging The package uses a single, package-wide logger for non-fatal diagnostics from @@ -125,7 +144,7 @@ netconfig.SetLogger(myLogger) // anything with Printf/Println, e.g. *log.Logger `NewConfigurator` accepts functional options: ```go -c, err := netconfig.NewConfigurator( +c, err := netconfig.NewConfigurator(ctx, netconfig.WithTestAddress("http://my-canary/health"), // connectivity-test URL netconfig.WithConnectivityCheck(false), // skip the post-change probe + rollback netconfig.WithSkipConnectivityCheck(), // same as WithConnectivityCheck(false) @@ -135,6 +154,8 @@ c, err := netconfig.NewConfigurator( netconfig.WithBackupRetention(10), // .bak.* copies kept per config file (0 = keep all) netconfig.WithAllowPrimaryRemoval(true), // let RemoveAddress remove the primary IP netconfig.WithSkipPanels(true), // skip all panel interaction (add/primary/remove) + netconfig.WithAllowNoBackends(true), // don't fail when no persistence backend is detected + netconfig.WithServiceReadyTimeout(90 * time.Second), // how long to wait for a daemon-backed backend (default 60s, 0 = don't wait) ) ``` @@ -155,12 +176,45 @@ the override only when you are deliberately tearing down the current primary. InterWorx to reload, set the main IP, or release an IP. Only the running system and the network-manager configuration files are changed. +### Choosing an interface + `FindInterfaceByName` accepts two well-known selectors in addition to a literal interface name: - `"public-internet"` — the interface that carries the IPv4 default gateway. - `"public-internet-6"` — the interface that carries the IPv6 default gateway. +Those answer "which interface is already on the internet". When the host is not +on the internet yet — a freshly provisioned VM, an image whose NIC name is not +known in advance — `FindPhysicalInterfaces` answers "which interface should be". +It drops the devices that should never carry a public address (bridges, bonds, +VLANs, tunnels, container veth pairs) and returns what is left in the order a +caller should try them: + +1. Interfaces that are up, before those that are down. +2. Interfaces already carrying a default gateway, IPv4 ahead of IPv6-only. +3. Wired ahead of wireless. +4. By name as a person reads it, so `eth0` precedes `eth1` and `eth2` + precedes `eth10`. + +Paravirtual NICs (virtio, vmxnet, Xen) count as physical: a VM's only real +interface is still the one to configure. The result is a ranking, not a +decision — a caller with a further requirement, such as an interface not already +holding a public address, filters the slice and takes the first survivor: + +```go +ifaces, err := c.GetInterfaces(ctx) +for _, iface := range netconfig.FindPhysicalInterfaces(ifaces) { + if !hasPublicAddress(iface) { + return iface.Name + } +} +``` + +`Interface.Up` and `Interface.Physical` carry the two facts this ranking rests +on, and are readable on their own. Both describe the running system, so they are +only set by `GetInterfaces`. + ## Usage ```go @@ -177,7 +231,7 @@ import ( func main() { ctx := context.Background() - c, err := netconfig.NewConfigurator() + c, err := netconfig.NewConfigurator(ctx) if err != nil { log.Fatal(err) } diff --git a/cloudinit.go b/cloudinit.go index 3d48739..82a03d7 100644 --- a/cloudinit.go +++ b/cloudinit.go @@ -65,6 +65,31 @@ type ciInterface struct { Optional bool `yaml:"optional,omitempty" json:"optional,omitempty"` } +// dhcpState reports whether each family's DHCP client is enabled. An absent key +// means the schema default, which is off. +func (c *ciInterface) dhcpState() (dhcp4, dhcp6 bool) { + return c.DHCP4 != nil && *c.DHCP4, c.DHCP6 != nil && *c.DHCP6 +} + +// setDHCP records the requested DHCP client state for both families, dropping +// the overrides of a family whose client is being turned off. A family being +// disabled that was already absent stays absent rather than being written out +// as false. +func (c *ciInterface) setDHCP(dhcp4, dhcp6 bool) { + if dhcp4 || c.DHCP4 != nil { + c.DHCP4 = boolPtr(dhcp4) + } + if !dhcp4 { + c.DHCP4Overrides = nil + } + if dhcp6 || c.DHCP6 != nil { + c.DHCP6 = boolPtr(dhcp6) + } + if !dhcp6 { + c.DHCP6Overrides = nil + } +} + // Physical ethernet interface. type ciEthernet struct { ciPhysical `yaml:",inline" json:",inline"` @@ -375,6 +400,7 @@ func (*cloudInit) ConvertInterface(name string, MAC string, config ciInterface, i.Name = name i.MAC = mac i.Link = foundLink + i.DHCP4, i.DHCP6 = config.dhcpState() // Parse addresses. for _, addr := range config.Addresses { @@ -473,6 +499,15 @@ func (ci *cloudInit) GetInterfaces() (interfaces []*Interface, err error) { // Parse subnet configs. for _, subnet := range config.Subnets { + // A v1 subnet encodes the DHCP client in its type rather than in a + // dhcp4/dhcp6 key. "dhcp" with no suffix is the IPv4 client. + switch subnet.Type { + case "dhcp", "dhcp4": + i.DHCP4 = true + case "dhcp6", "ipv6_dhcpv6-stateful", "ipv6_dhcpv6-stateless": + i.DHCP6 = true + } + // Add DNS servers and search domains. for _, dns := range subnet.DNSNameservers { if ip := net.ParseIP(dns); ip != nil { @@ -1024,6 +1059,89 @@ func (ci *cloudInit) SetIfaceAddresses(_ context.Context, iface string, addrs [] return } +// ciDHCPSubnetTypes are the v1 subnet types that describe a DHCP client rather +// than a static address, keyed by whether they are removed when that family's +// client is disabled. +var ciDHCPSubnetTypes = map[string]bool{ + "dhcp": true, // IPv4, the unsuffixed spelling. + "dhcp4": true, + "dhcp6": false, + "ipv6_dhcpv6-stateful": false, + "ipv6_dhcpv6-stateless": false, +} + +// Set the DHCP client state on an interface. +func (ci *cloudInit) SetIfaceDHCP(_ context.Context, iface string, dhcp4, dhcp6 bool) (err error) { + // Apply changes to v1 configs, where the DHCP client is a subnet whose type + // names it rather than a key. Static subnets are left in place: cloud-init + // accepts a dhcp subnet and a static subnet on the same interface. + for c, config := range ci.Network.Configs { + switch config.Type { + case "physical", "bond", "bridge", "vlan": + default: + continue + } + if config.Name != iface { + continue + } + + // Drop the existing DHCP subnets of both families, then re-add the ones + // that were asked for. Rebuilding rather than editing in place keeps a + // config that listed several spellings of the same client from ending up + // with a stale one alongside the new one. + subnets := make([]ciSubnet, 0, len(config.Subnets)+2) + if dhcp4 { + subnets = append(subnets, ciSubnet{Type: "dhcp4"}) + } + if dhcp6 { + subnets = append(subnets, ciSubnet{Type: "dhcp6"}) + } + for _, subnet := range config.Subnets { + if _, isDHCP := ciDHCPSubnetTypes[subnet.Type]; isDHCP { + continue + } + subnets = append(subnets, subnet) + } + config.Subnets = subnets + ci.Network.Configs[c] = config + } + + // Apply changes to v2 ethernets. + for name, config := range ci.Network.Ethernets { + ifName := name + if config.SetName != "" { + ifName = config.SetName + } + if ifName != iface { + continue + } + config.setDHCP(dhcp4, dhcp6) + ci.Network.Ethernets[name] = config + } + + // Apply changes to v2 bridges. + if config, ok := ci.Network.Bridges[iface]; ok { + config.setDHCP(dhcp4, dhcp6) + ci.Network.Bridges[iface] = config + } + + // Apply changes to v2 bonds. + if config, ok := ci.Network.Bonds[iface]; ok { + config.setDHCP(dhcp4, dhcp6) + ci.Network.Bonds[iface] = config + } + + // Apply changes to v2 VLAN interfaces. + if config, ok := ci.Network.VLANs[iface]; ok { + config.setDHCP(dhcp4, dhcp6) + ci.Network.VLANs[iface] = config + } + + // Try to save changes. + err = ci.Save() + return +} + // Set static routes to interface. func (ci *cloudInit) SetIfaceRoutes(_ context.Context, iface string, routes []*Route) (err error) { // Apply changes to v1 configs. diff --git a/cloudinit_test.go b/cloudinit_test.go index 9821cd0..4589f50 100644 --- a/cloudinit_test.go +++ b/cloudinit_test.go @@ -6,43 +6,33 @@ import ( "os" "path/filepath" "testing" + + "github.com/stretchr/testify/require" ) // Validate the cloud-init configuration parser/writer functions. func TestCloudinit(t *testing.T) { // Setup test file. tmpDir, err := os.MkdirTemp("", "") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) configPath := filepath.Join(tmpDir, "network.yaml") testDir, err := filepath.Abs("./tests/cloudinit") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) resultsDir := filepath.Join(testDir, "results") err = fileCopy(filepath.Join(testDir, "network.yaml"), configPath) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Setup ifupdown and parse test file. ci, err := newCloudInitWith(configPath, filepath.Join(tmpDir, "nothing.json"), filepath.Join(tmpDir, "cloud.cfg.d")) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Get the interfaces state. interfaces, err := ci.GetInterfaces() - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Verify interfaces read from file. err = testVerifyInterfaces(interfaces, resultsDir, 1) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Test setting the IP addresses on an interface. err = ci.SetIfaceAddresses(context.Background(), "test_eth0.1556", []*net.IPNet{ @@ -59,9 +49,7 @@ func TestCloudinit(t *testing.T) { Mask: net.CIDRMask(64, 128), }, }, net.ParseIP("1.2.3.1"), net.ParseIP("fc00::1")) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Test setting routes on an interface. err = ci.SetIfaceRoutes(context.Background(), "test_eth3", []*Route{ @@ -82,27 +70,19 @@ func TestCloudinit(t *testing.T) { Metric: 100, }, }) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Get the interfaces state. interfaces, err = ci.GetInterfaces() - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Verify interfaces read from file. err = testVerifyInterfaces(interfaces, resultsDir, 2) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Read the current file and expected state. err = testVerifyResults(resultsDir, tmpDir, 1) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Test setting the IP addresses on an interface. err = ci.SetIfaceAddresses(context.Background(), "test_eth0", []*net.IPNet{ @@ -111,76 +91,52 @@ func TestCloudinit(t *testing.T) { Mask: net.CIDRMask(24, 32), }, }, net.ParseIP("1.2.10.254"), nil) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Test setting routes on an interface. err = ci.SetIfaceRoutes(context.Background(), "test_eth0.1556", []*Route{}) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Get the interfaces state. interfaces, err = ci.GetInterfaces() - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Verify interfaces read from file. err = testVerifyInterfaces(interfaces, resultsDir, 3) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Read the current file and expected state. err = testVerifyResults(resultsDir, tmpDir, 2) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Cleanup. err = os.RemoveAll(tmpDir) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) } // Validate the cloudbase-init configuration parser/writer functions. func TestCloudbaseinit(t *testing.T) { // Setup test file. tmpDir, err := os.MkdirTemp("", "") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) configPath := filepath.Join(tmpDir, "network.json") testDir, err := filepath.Abs("./tests/cloudinit/cloudbase") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) resultsDir := filepath.Join(testDir, "results") err = fileCopy(filepath.Join(testDir, "network.json"), configPath) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Setup ifupdown and parse test file. ci, err := newCloudInitWith(filepath.Join(tmpDir, "nothing.yaml"), configPath, filepath.Join(tmpDir, "cloud.cfg.d")) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Get the interfaces state. interfaces, err := ci.GetInterfaces() - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Verify interfaces read from file. err = testVerifyInterfaces(interfaces, resultsDir, 1) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Test setting the IP addresses on an interface. err = ci.SetIfaceAddresses(context.Background(), "test_eth0.1556", []*net.IPNet{ @@ -197,9 +153,7 @@ func TestCloudbaseinit(t *testing.T) { Mask: net.CIDRMask(64, 128), }, }, net.ParseIP("1.2.3.1"), net.ParseIP("fc00::1")) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Test setting routes on an interface. err = ci.SetIfaceRoutes(context.Background(), "test_eth3", []*Route{ @@ -220,27 +174,19 @@ func TestCloudbaseinit(t *testing.T) { Metric: 100, }, }) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Get the interfaces state. interfaces, err = ci.GetInterfaces() - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Verify interfaces read from file. err = testVerifyInterfaces(interfaces, resultsDir, 2) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Read the current file and expected state. err = testVerifyResults(resultsDir, tmpDir, 1) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Test setting the IP addresses on an interface. err = ci.SetIfaceAddresses(context.Background(), "test_eth0", []*net.IPNet{ @@ -249,37 +195,25 @@ func TestCloudbaseinit(t *testing.T) { Mask: net.CIDRMask(24, 32), }, }, net.ParseIP("1.2.10.254"), nil) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Test setting routes on an interface. err = ci.SetIfaceRoutes(context.Background(), "test_eth0.1556", []*Route{}) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Get the interfaces state. interfaces, err = ci.GetInterfaces() - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Verify interfaces read from file. err = testVerifyInterfaces(interfaces, resultsDir, 3) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Read the current file and expected state. err = testVerifyResults(resultsDir, tmpDir, 2) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Cleanup. err = os.RemoveAll(tmpDir) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) } diff --git a/configurator_linux.go b/configurator_linux.go index eb7965c..da5b924 100644 --- a/configurator_linux.go +++ b/configurator_linux.go @@ -3,12 +3,12 @@ package netconfig import ( "bytes" "context" + "errors" "fmt" "net" "os" - "os/exec" + "syscall" - dbus "github.com/coreos/go-systemd/dbus" "github.com/vishvananda/netlink" ) @@ -21,86 +21,83 @@ type linuxConfigurator struct { *configOptions } -// Returns the linux network configurator. Behaviour is tuned with Option -// values such as WithTestAddress and WithConnectivityCheck; use SetLogger to -// replace the package-wide logger. -func NewConfigurator(opts ...Option) (configurator Configurator, err error) { +// Returns the linux network configurator. Backends are auto-detected; ctx +// bounds that detection, which queries systemd over D-Bus and may shell out to +// chkconfig and netplan. Behaviour is tuned with Option values such as +// WithTestAddress and WithConnectivityCheck; use SetLogger to replace the +// package-wide logger. +func NewConfigurator(ctx context.Context, opts ...Option) (Configurator, error) { options := newConfigOptions(opts...) - // Connect to dbus for systemd and list active units to determine which - // network managers are running. This fails on systems without a usable - // systemd D-Bus interface, so we fall back to detecting legacy managers: - // - Ubuntu 14.04: Upstart is PID 1 and org.freedesktop.systemd1 is served - // by systemd-shim, which lacks ListUnits ("No such method 'ListUnits'"). - // - CentOS 5/6: no systemd1 on the bus at all, so ListUnits (or the - // connection itself) fails with ServiceUnknown. - activeUnits := make(map[string]bool) - conn, derr := dbus.NewWithContext(context.Background()) - if derr == nil { - defer conn.Close() - var units []dbus.UnitStatus - units, derr = conn.ListUnitsContext(context.Background()) - for _, unit := range units { - if unit.ActiveState == "active" { - activeUnits[unit.Name] = true - } - } - } - if derr != nil { - logger.Printf("unable to list systemd units, falling back to legacy detection: %v", derr) - activeUnits = legacyActiveUnits() - } - // Determine candidates based on unit status. + // Snapshot what the init system knows about its units once. Every backend + // check below is answered from it, falling back to the SysV mechanisms for + // any service systemd cannot speak for. + initSys := newInitState(ctx) + c := new(linuxConfigurator) c.configOptions = options - configurator = c // Detect each backend. A nil concrete pointer wrapped in an interface is // itself non-nil, so backends are collected as concrete pointers here and - // only registered below when non-nil. + // only registered below when non-nil. Each constructor's error is kept local: + // a backend that fails to parse is skipped, not fatal, and must not leak into + // the error returned to the caller. var ( nd *networkd nm *networkManager ns *networkScripts iud *ifUpDown np *netplan - ci *cloudInit ) - if activeUnits["systemd-networkd.service"] { - nd, err = newNetworkd(options.backupRetention) - if err != nil { + if initSys.detected(ctx, "systemd-networkd") { + var err error + if nd, err = newNetworkd(options.backupRetention); err != nil { logger.Println("error parsing networkd config:", err) } } - if activeUnits["NetworkManager.service"] { - nm, err = newNetworkManager() - if err != nil { + + // NetworkManager's pid file is checked as well as its unit, so the daemon is + // still found on a host whose systemd could not be queried. Detection here + // includes a unit that is enabled but not yet started, so newNetworkManager + // waits for the daemon to reach the bus and finish starting: this program + // may be run early enough in boot to beat it, and NetworkManager is + // configured through that daemon rather than through a file. + if initSys.detected(ctx, "NetworkManager") || networkManagerRunning() { + var err error + if nm, err = newNetworkManager(ctx, options.serviceReadyTimeout); err != nil { logger.Println("error parsing network manager config:", err) } } - if activeUnits["network.service"] { - ns, err = newNetworkScripts(options.backupRetention) - if err != nil { + + // The RHEL-family network-scripts service. newNetworkScripts returns an error + // when /etc/sysconfig/network-scripts is absent, so a host that has the + // service registered but no scripts directory registers no backend. + if initSys.detected(ctx, "network") { + var err error + if ns, err = newNetworkScripts(options.backupRetention); err != nil { logger.Println("error parsing network scripts:", err) } } - // If the ifupdown config exists, add its config. - if _, serr := os.Stat(ifUpDownConfig); serr == nil { - iud, err = newIfUpDown(options.backupRetention) - if err != nil { - logger.Println("error prasing ifupdown:", err) + if ifUpDownDetected(ctx, initSys) { + var err error + if iud, err = newIfUpDown(options.backupRetention); err != nil { + logger.Println("error parsing ifupdown:", err) } } - _, err = exec.LookPath("netplan") - if err == nil { - np, err = newNetplan(options.backupRetention) - if err != nil { + // netplan is gated on its binary resolving before newNetplan runs `netplan + // info`, so a host without netplan pays no subprocess and logs no failure. + if commandExists("netplan") { + var err error + if np, err = newNetplan(ctx, options.backupRetention); err != nil { logger.Println("error parsing netplan:", err) } } - ci, err = newCloudInit(options.backupRetention) + + // cloud-init has no service to detect: it runs once at boot and is found by + // its configuration alone, which newCloudInit reports on. + ci, err := newCloudInit(options.backupRetention) if err != nil { logger.Println("error parsing cloud-init:", err) } @@ -126,6 +123,21 @@ func NewConfigurator(opts ...Option) (configurator Configurator, err error) { c.ifaceBackends = append(c.ifaceBackends, namedIfaceBackend{"IfUpDown", iud}) } + // A host with no file backend can still have its running state changed through + // netlink, but nothing would persist and the change would vanish on the next + // boot. Fail here rather than hand back a configurator that silently writes + // nowhere; WithAllowNoBackends opts into it for read-only callers. + if len(c.ifaceBackends) == 0 { + // A cancelled context aborts every probe above, which looks identical to + // a host that genuinely has no backend. Report the real cause. + if cerr := ctx.Err(); cerr != nil { + return nil, fmt.Errorf("backend detection did not complete: %w", cerr) + } + if !options.allowNoBackends { + return nil, fmt.Errorf("no network configuration backend detected: address changes would apply to the running system but not survive a reboot (use WithAllowNoBackends to proceed anyway)") + } + } + // Register the detected control panels in a stable apply order. if _, serr := os.Stat(cpanelBin); serr == nil { c.panelBackends = append(c.panelBackends, namedPanelBackend{"cPanel", new(cpanel)}) @@ -137,44 +149,63 @@ func NewConfigurator(opts ...Option) (configurator Configurator, err error) { c.panelBackends = append(c.panelBackends, namedPanelBackend{"Interworx", new(interworx)}) } - // Detection failures are logged above; they are not fatal to constructing - // the configurator, so do not propagate them to the caller. - err = nil - return + // Detection failures are logged above; a backend that could not be parsed is + // skipped rather than propagated, so the configurator is returned without an + // error as long as at least one backend was registered. + return c, nil } -// legacyActiveUnits detects active network managers on systems that lack a -// usable systemd D-Bus interface, where ListUnits is unavailable: Upstart on -// Ubuntu 14.04 and SysVinit/Upstart on CentOS 5/6. Detection is by config and -// runtime presence rather than by querying any specific init system, so it is -// init-agnostic and spawns no subprocesses. -// -// systemd-networkd cannot run on these systems, and ifupdown is detected -// separately by config-file presence, so only the RHEL network-scripts service -// and NetworkManager are considered here. The returned keys match the systemd -// unit names used by the caller. -func legacyActiveUnits() map[string]bool { - active := make(map[string]bool) - - // RHEL-family network-scripts has no daemon; presence of its config - // directory is the best available signal that it manages the network. - if fi, serr := os.Stat(networkScriptsPath); serr == nil && fi.IsDir() { - active["network.service"] = true +// ifUpDownDetected reports whether ifupdown manages this host's network. +// /etc/network/interfaces existing is not enough on its own: it is left behind +// on Ubuntu hosts migrated to netplan and on Debian hosts that have handed the +// network to NetworkManager, so the networking service must also be running or +// enabled. On a host with no discoverable init system there is nothing to +// corroborate against, and the config file's presence is taken as sufficient +// rather than dropping a backend that may well be the only one. +func ifUpDownDetected(ctx context.Context, s *initState) bool { + if _, err := os.Stat(ifUpDownConfig); err != nil { + return false } + return s.detected(ctx, "networking") || !initSystemDiscoverable() +} - // NetworkManager manages the network via its own D-Bus interface; a - // runtime pid file indicates the daemon is running. - for _, pidFile := range []string{ - "/var/run/NetworkManager/NetworkManager.pid", - "/run/NetworkManager/NetworkManager.pid", - } { - if _, serr := os.Stat(pidFile); serr == nil { - active["NetworkManager.service"] = true - break - } +// linkIsUp reports whether a link can carry traffic. The operational state is +// the honest answer where the driver reports one: an interface that is +// administratively up but has no carrier — an unplugged cable — cannot. Many +// virtual and paravirtual drivers never report an operational state at all, so +// for those the administrative flag stands in. +func linkIsUp(attrs *netlink.LinkAttrs) bool { + if attrs.OperState == netlink.OperUnknown { + return attrs.Flags&net.FlagUp != 0 } + return attrs.OperState == netlink.OperUp +} - return active +// ensureLinkUp brings an administratively down link up, reporting whether it +// had to. The kernel installs an address's connected route only while its link +// is up, so a gateway added to a down interface is rejected as unreachable and +// the interface has to be raised before its addresses and routes are written. +// Only the administrative flag is consulted, not the operational state that +// linkIsUp prefers: a link that is up and waiting on carrier is already as up +// as this can make it, and setting it up again would say nothing. +func ensureLinkUp(h *netlink.Handle, link netlink.Link) (bool, error) { + if link.Attrs().Flags&net.FlagUp != 0 { + return false, nil + } + if err := h.LinkSetUp(link); err != nil { + return false, fmt.Errorf("failed to bring up interface %s: %w", link.Attrs().Name, err) + } + return true, nil +} + +// linkIsPhysical reports whether a link is backed by a network device rather +// than created by the kernel. rtnetlink names the kind of every software +// device — bridge, bond, veth, vlan, tun, wireguard, dummy — and names no kind +// at all for a driver-backed NIC, which is the "device" this compares against. +// Paravirtual NICs (virtio, vmxnet, xen) are driver-backed and so count as +// physical: a VM's only real NIC is still the one to configure. +func linkIsPhysical(link netlink.Link) bool { + return link.Type() == "device" } // Get list of interfaces and their configs. @@ -206,6 +237,8 @@ func (c *linuxConfigurator) GetInterfaces(ctx context.Context) (interfaces []*In i.Name = link.Attrs().Name i.MAC = link.Attrs().HardwareAddr i.Link = link + i.Up = linkIsUp(link.Attrs()) + i.Physical = linkIsPhysical(link) // Add IP Addresses. addrs, err := h.AddrList(link, netlink.FAMILY_ALL) @@ -261,10 +294,11 @@ func (c *linuxConfigurator) GetInterfaces(ctx context.Context) (interfaces []*In interfaces = append(interfaces, i) } - // DNS servers and search domains are not exposed via netlink, so merge - // them in from the persisted backend configurations. This lets callers - // read back what SetDNS wrote via Interface.DNS and Interface.SearchDomains. - mergeDNSFromBackends(c.ifaceBackends, interfaces) + // DNS servers, search domains, and DHCP client state are not exposed via + // netlink, so merge them in from the persisted backend configurations. This + // lets callers read back what SetDNS and SetDHCP wrote via Interface.DNS, + // Interface.SearchDomains, Interface.DHCP4, and Interface.DHCP6. + mergeBackendState(c.ifaceBackends, interfaces) return } @@ -297,6 +331,26 @@ func (c *linuxConfigurator) AddAddress(ctx context.Context, iface string, addr * } } + // Raise the interface before writing to it. A down link carries no connected + // route for the address about to be added, so the gateway below would be + // rejected as unreachable. A link raised here is part of the pre-change state + // this restores, so put it back down on any path that does not complete. + broughtUp, err := ensureLinkUp(h, link) + if err != nil { + return err + } + applied := false + if broughtUp { + defer func() { + if applied { + return + } + if derr := h.LinkSetDown(link); derr != nil { + logger.Printf("failed to return interface %s to down: %v", iface, derr) + } + }() + } + // Get existing addresses and save the full pre-change state so we can // restore it if the connectivity test fails. exists := false @@ -479,8 +533,14 @@ func (c *linuxConfigurator) AddAddress(ctx context.Context, iface string, addr * return err } } else if addedGateway != nil { + // Deleting the address above takes with it every route that + // resolved its nexthop through the address's connected + // subnet, which is exactly how the gateway added here was + // reachable. The kernel then has nothing left to delete and + // answers ESRCH, so treat an already-gone route as removed + // rather than reporting it over the rollback's own error. err = h.RouteDel(addedGateway) - if err != nil { + if err != nil && !errors.Is(err, syscall.ESRCH) { return err } } @@ -489,6 +549,9 @@ func (c *linuxConfigurator) AddAddress(ctx context.Context, iface string, addr * } } + // The runtime state now holds, so a link raised above stays up. + applied = true + // Update configuration files. Skip control panels when skipPanels is set. applyIfaceAddresses(ctx, c.ifaceBackends, iface, addresses, gateway4, gateway6) if !c.skipPanels { @@ -1016,3 +1079,19 @@ func (c *linuxConfigurator) SetDNS(ctx context.Context, iface string, servers [] applyLiveDNS(ctx, iface, servers, searchDomains) return nil } + +// Enable or disable the DHCP client for each address family on an interface. +func (c *linuxConfigurator) SetDHCP(ctx context.Context, iface string, dhcp4, dhcp6 bool) error { + if !applyIfaceDHCP(ctx, c.ifaceBackends, iface, dhcp4, dhcp6) { + return fmt.Errorf("no backend accepted a DHCP change for %s", iface) + } + + // Enabling a client asks the running system to acquire a lease now. Turning + // one off does not reconfigure the interface: the existing lease is left to + // expire, so a caller connected over the leased address is not cut off by a + // call that was only meant to change what happens on the next boot. + if dhcp4 || dhcp6 { + renewDHCPOnBackends(ctx, c.ifaceBackends, iface) + } + return nil +} diff --git a/configurator_linux_test.go b/configurator_linux_test.go index ba50bd1..435636e 100644 --- a/configurator_linux_test.go +++ b/configurator_linux_test.go @@ -6,9 +6,11 @@ import ( "os" "path/filepath" "runtime" - "strings" "testing" + "time" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/vishvananda/netlink" "github.com/vishvananda/netns" "golang.org/x/sys/unix" @@ -24,18 +26,12 @@ func setupNetlinkTest(t testing.TB) func() { runtime.LockOSThread() ns, err := netns.New() - if err != nil { - t.Fatal("Failed to create new netns", err) - } + require.NoError(t, err, "Failed to create new netns") link, err := netlink.LinkByName("lo") - if err != nil { - t.Fatalf("Failed to find \"lo\" in new netns: %v", err) - } + require.NoError(t, err, "Failed to find \"lo\" in new netns") err = netlink.LinkSetUp(link) - if err != nil { - t.Fatalf("Failed to bring up \"lo\" in new netns: %v", err) - } + require.NoError(t, err, "Failed to bring up \"lo\" in new netns") return func() { ns.Close() @@ -49,9 +45,7 @@ func setupNetlinkTest(t testing.TB) func() { func isPrimary(t testing.TB, h *netlink.Handle, link netlink.Link, ip net.IP) (present, primary bool) { t.Helper() addrs, err := h.AddrList(link, netlink.FAMILY_ALL) - if err != nil { - t.Fatalf("AddrList: %v", err) - } + require.NoError(t, err, "AddrList") for _, a := range addrs { if a.IPNet.IP.Equal(ip) { return true, a.Flags&unix.IFA_F_SECONDARY == 0 @@ -68,9 +62,7 @@ func TestLinuxSetPrimaryAddress(t *testing.T) { defer tearDown() h, err := netlink.NewHandle() - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) defer h.Close() // Setup test link. @@ -79,42 +71,30 @@ func TestLinuxSetPrimaryAddress(t *testing.T) { Name: name, HardwareAddr: net.HardwareAddr{0x52, 0x54, 0x00, 0x8b, 0x0d, 0x93}, }}) - if err = h.LinkAdd(link); err != nil { - t.Fatal(err) - } - if err = h.LinkSetUp(link); err != nil { - t.Fatal(err) - } + require.NoError(t, h.LinkAdd(link)) + require.NoError(t, h.LinkSetUp(link)) // Add two addresses in the same subnet. The first added is the primary. primaryIP := &net.IPNet{IP: net.ParseIP("1.2.3.4"), Mask: net.CIDRMask(24, 32)} secondaryIP := &net.IPNet{IP: net.ParseIP("1.2.3.5"), Mask: net.CIDRMask(24, 32)} - if err = h.AddrAdd(link, &netlink.Addr{IPNet: primaryIP}); err != nil { - t.Fatal(err) - } - if err = h.AddrAdd(link, &netlink.Addr{IPNet: secondaryIP}); err != nil { - t.Fatal(err) - } + require.NoError(t, h.AddrAdd(link, &netlink.Addr{IPNet: primaryIP})) + require.NoError(t, h.AddrAdd(link, &netlink.Addr{IPNet: secondaryIP})) // Add a default route via the subnet so connectivity verification has a // gateway to work with. gw := net.ParseIP("1.2.3.1") - if err = h.RouteAdd(&netlink.Route{ + require.NoError(t, h.RouteAdd(&netlink.Route{ Family: netlink.FAMILY_V4, LinkIndex: link.Attrs().Index, Dst: &net.IPNet{IP: net.IPv4zero, Mask: net.CIDRMask(0, 32)}, Gw: gw, - }); err != nil { - t.Fatal(err) - } + })) // Sanity check the initial primary/secondary assignment. - if _, primary := isPrimary(t, h, link, primaryIP.IP); !primary { - t.Fatal("expected 1.2.3.4 to start as primary") - } - if _, primary := isPrimary(t, h, link, secondaryIP.IP); primary { - t.Fatal("expected 1.2.3.5 to start as secondary") - } + _, primary := isPrimary(t, h, link, primaryIP.IP) + require.True(t, primary, "expected 1.2.3.4 to start as primary") + _, primary = isPrimary(t, h, link, secondaryIP.IP) + require.False(t, primary, "expected 1.2.3.5 to start as secondary") // Configurator with the success sentinel and a capturing backend. backend := &fakeIfaceBackend{} @@ -122,51 +102,131 @@ func TestLinuxSetPrimaryAddress(t *testing.T) { c.ifaceBackends = append(c.ifaceBackends, namedIfaceBackend{"fake", backend}) // Promote the secondary to primary. - if err = c.SetPrimaryAddress(context.Background(), name, secondaryIP); err != nil { - t.Fatalf("SetPrimaryAddress: %v", err) - } + require.NoError(t, c.SetPrimaryAddress(context.Background(), name, secondaryIP), "SetPrimaryAddress") // The kernel should now treat 1.2.3.5 as primary and 1.2.3.4 as secondary, // with both addresses still present and the default route intact. - if present, primary := isPrimary(t, h, link, secondaryIP.IP); !present || !primary { - t.Errorf("after promotion 1.2.3.5: present=%v primary=%v, want both true", present, primary) - } - if present, primary := isPrimary(t, h, link, primaryIP.IP); !present || primary { - t.Errorf("after promotion 1.2.3.4: present=%v primary=%v, want present true primary false", present, primary) - } + present, primary := isPrimary(t, h, link, secondaryIP.IP) + assert.True(t, present && primary, "after promotion 1.2.3.5: present=%v primary=%v, want both true", present, primary) + present, primary = isPrimary(t, h, link, primaryIP.IP) + assert.True(t, present && !primary, "after promotion 1.2.3.4: present=%v primary=%v, want present true primary false", present, primary) routes, err := h.RouteList(link, netlink.FAMILY_V4) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) haveDefault := false for _, r := range routes { if ones, _ := r.Dst.Mask.Size(); ones == 0 && r.Gw.Equal(gw) { haveDefault = true } } - if !haveDefault { - t.Error("default route missing after promotion") - } + assert.True(t, haveDefault, "default route missing after promotion") // The persisted config must receive the reordered list, primary first. - if len(backend.lastAddrs) == 0 || !backend.lastAddrs[0].IP.Equal(secondaryIP.IP) { - t.Errorf("backend primary = %v, want 1.2.3.5 first", backend.lastAddrs) - } + assert.True(t, len(backend.lastAddrs) != 0 && backend.lastAddrs[0].IP.Equal(secondaryIP.IP), "backend primary = %v, want 1.2.3.5 first", backend.lastAddrs) // Promoting the already-primary address is a no-op that still succeeds. - if err = c.SetPrimaryAddress(context.Background(), name, secondaryIP); err != nil { - t.Errorf("SetPrimaryAddress (idempotent): %v", err) - } - if _, primary := isPrimary(t, h, link, secondaryIP.IP); !primary { - t.Error("1.2.3.5 should remain primary after idempotent call") - } + assert.NoError(t, c.SetPrimaryAddress(context.Background(), name, secondaryIP), "SetPrimaryAddress (idempotent)") + _, primary = isPrimary(t, h, link, secondaryIP.IP) + assert.True(t, primary, "1.2.3.5 should remain primary after idempotent call") // Promoting an address not on the interface must error before any change. absent := &net.IPNet{IP: net.ParseIP("1.2.3.99"), Mask: net.CIDRMask(24, 32)} err = c.SetPrimaryAddress(context.Background(), name, absent) - if err == nil || err.Error() != "address not found on interface" { - t.Errorf("absent address error = %v, want \"address not found on interface\"", err) + assert.EqualError(t, err, "address not found on interface") +} + +// adminUp reports whether the named link is administratively up, re-reading it +// from the kernel rather than trusting a cached netlink.Link. +func adminUp(t testing.TB, h *netlink.Handle, name string) bool { + t.Helper() + link, err := h.LinkByName(name) + require.NoError(t, err, "LinkByName") + return link.Attrs().Flags&net.FlagUp != 0 +} + +// hasDefaultRoute reports whether the link carries a default route via gw. +func hasDefaultRoute(t testing.TB, h *netlink.Handle, link netlink.Link, gw net.IP) bool { + t.Helper() + routes, err := h.RouteList(link, netlink.FAMILY_V4) + require.NoError(t, err, "RouteList") + for _, r := range routes { + if ones, _ := r.Dst.Mask.Size(); ones == 0 && r.Gw.Equal(gw) { + return true + } } + return false +} + +// newDownLink creates a dummy link and deliberately leaves it administratively +// down, the starting state for the AddAddress tests below. +func newDownLink(t testing.TB, h *netlink.Handle, name string) netlink.Link { + t.Helper() + link := netlink.Link(&netlink.Dummy{LinkAttrs: netlink.LinkAttrs{Name: name}}) + require.NoError(t, h.LinkAdd(link), "LinkAdd") + require.False(t, adminUp(t, h, name), "%s should start down", name) + return link +} + +// newPingerConfigurator builds a configurator whose ping settings are short +// enough that the unanswerable probes against a dummy link do not stall the +// test for the twenty second default. +func newPingerConfigurator(testAddress string) *linuxConfigurator { + c := &linuxConfigurator{configOptions: &configOptions{ + testAddress: testAddress, + pingCount: 1, + pingTimeout: 100 * time.Millisecond, + }} + c.ifaceBackends = append(c.ifaceBackends, namedIfaceBackend{"fake", &fakeIfaceBackend{}}) + return c +} + +// TestLinuxAddAddressBringsLinkUp covers AddAddress against an administratively +// down interface. The kernel installs no connected route for an address on a +// down link, so the default route would be rejected as unreachable unless the +// link is raised first. +func TestLinuxAddAddressBringsLinkUp(t *testing.T) { + tearDown := setupNetlinkTest(t) + defer tearDown() + + h, err := netlink.NewHandle() + require.NoError(t, err) + defer h.Close() + + name := "test_enp1s0" + link := newDownLink(t, h, name) + addr := &net.IPNet{IP: net.ParseIP("1.2.3.4"), Mask: net.CIDRMask(24, 32)} + gw := net.ParseIP("1.2.3.1") + + require.NoError(t, newPingerConfigurator("test_success").AddAddress(context.Background(), name, addr, gw), "AddAddress") + assert.True(t, adminUp(t, h, name), "interface should be up after AddAddress") + assert.True(t, hasDefaultRoute(t, h, link, gw), "default route via %s missing", gw) + present, _ := isPrimary(t, h, link, addr.IP) + assert.True(t, present, "address missing after AddAddress") +} + +// TestLinuxAddAddressRestoresDownLink verifies that a link AddAddress raised is +// part of the pre-change state it restores: when the connectivity check fails, +// the interface goes back down along with the address and route rolled back. +// Its own namespace keeps the default route it adds from colliding with the one +// TestLinuxAddAddressBringsLinkUp installs, which the kernel refuses as EEXIST. +func TestLinuxAddAddressRestoresDownLink(t *testing.T) { + tearDown := setupNetlinkTest(t) + defer tearDown() + + h, err := netlink.NewHandle() + require.NoError(t, err) + defer h.Close() + + name := "test_enp1s0" + link := newDownLink(t, h, name) + addr := &net.IPNet{IP: net.ParseIP("1.2.3.4"), Mask: net.CIDRMask(24, 32)} + gw := net.ParseIP("1.2.3.1") + + err = newPingerConfigurator("test_fail").AddAddress(context.Background(), name, addr, gw) + assert.EqualError(t, err, "aborted operation due to loss of internet") + assert.False(t, adminUp(t, h, name), "interface should be down again after rollback") + assert.False(t, hasDefaultRoute(t, h, link, gw), "default route should be rolled back") + present, _ := isPrimary(t, h, link, addr.IP) + assert.False(t, present, "address should be rolled back") } // TestLinuxRemovePrimaryRefused verifies that RemoveAddress refuses to remove @@ -177,43 +237,27 @@ func TestLinuxRemovePrimaryRefused(t *testing.T) { defer tearDown() h, err := netlink.NewHandle() - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) defer h.Close() name := "test_enp1s0" link := netlink.Link(&netlink.Dummy{LinkAttrs: netlink.LinkAttrs{Name: name}}) - if err = h.LinkAdd(link); err != nil { - t.Fatal(err) - } - if err = h.LinkSetUp(link); err != nil { - t.Fatal(err) - } + require.NoError(t, h.LinkAdd(link)) + require.NoError(t, h.LinkSetUp(link)) primaryIP := &net.IPNet{IP: net.ParseIP("1.2.3.4"), Mask: net.CIDRMask(24, 32)} - if err = h.AddrAdd(link, &netlink.Addr{IPNet: primaryIP}); err != nil { - t.Fatal(err) - } + require.NoError(t, h.AddrAdd(link, &netlink.Addr{IPNet: primaryIP})) c := &linuxConfigurator{configOptions: &configOptions{testAddress: "test_success"}} c.ifaceBackends = append(c.ifaceBackends, namedIfaceBackend{"fake", &fakeIfaceBackend{}}) // Default: removing the primary is refused. err = c.RemoveAddress(context.Background(), name, primaryIP) - if err == nil || !contains(err.Error(), "refusing to remove primary address") { - t.Errorf("RemoveAddress primary = %v, want refusal error", err) - } + assert.ErrorContains(t, err, "refusing to remove primary address") // With the override enabled, removal succeeds. c.allowPrimaryRemoval = true - if err = c.RemoveAddress(context.Background(), name, primaryIP); err != nil { - t.Errorf("RemoveAddress with allowPrimaryRemoval: %v", err) - } -} - -func contains(s, substr string) bool { - return strings.Contains(s, substr) + assert.NoError(t, c.RemoveAddress(context.Background(), name, primaryIP), "RemoveAddress with allowPrimaryRemoval") } func TestLinuxConfigurator(t *testing.T) { @@ -222,9 +266,7 @@ func TestLinuxConfigurator(t *testing.T) { // Connect to netlink. h, err := netlink.NewHandle() - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) defer h.Close() // Setup test link. @@ -234,15 +276,11 @@ func TestLinuxConfigurator(t *testing.T) { HardwareAddr: net.HardwareAddr{0x52, 0x54, 0x00, 0x8b, 0x0d, 0x93}, }}) err = h.LinkAdd(eth0Link) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Set test link up. err = h.LinkSetUp(eth0Link) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Add test IP. testIP1 := &net.IPNet{ @@ -250,9 +288,7 @@ func TestLinuxConfigurator(t *testing.T) { Mask: net.CIDRMask(24, 32), } err = h.AddrAdd(eth0Link, &netlink.Addr{IPNet: testIP1}) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Add default route. testGW1 := net.ParseIP("1.2.3.1") @@ -266,9 +302,7 @@ func TestLinuxConfigurator(t *testing.T) { Gw: testGW1, } err = h.RouteAdd(defaultRoute) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Setup test link. eth1Name := "test_enp2s0" @@ -277,15 +311,11 @@ func TestLinuxConfigurator(t *testing.T) { HardwareAddr: net.HardwareAddr{0x52, 0x54, 0x00, 0x8b, 0xad, 0x93}, }} err = h.LinkAdd(eth1Link) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Set test link up. err = h.LinkSetUp(eth1Link) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Add test IP. testIP2 := &net.IPNet{ @@ -293,9 +323,7 @@ func TestLinuxConfigurator(t *testing.T) { Mask: net.CIDRMask(64, 128), } err = h.AddrAdd(eth1Link, &netlink.Addr{IPNet: testIP2}) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Add default route. testGW2 := net.ParseIP("fe80::1") @@ -311,9 +339,7 @@ func TestLinuxConfigurator(t *testing.T) { Priority: 200, } err = h.RouteAdd(ipv6Route) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Setup configurator with test URL to test server. c := &linuxConfigurator{configOptions: &configOptions{}} @@ -324,218 +350,146 @@ func TestLinuxConfigurator(t *testing.T) { // Setup test results path. testDir, err := filepath.Abs("./tests/configurator_linux") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) resultsDir := filepath.Join(testDir, "results") tmpDir, err := os.MkdirTemp("", "") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) configPath := filepath.Join(tmpDir, "50-cloud-init.yaml") err = fileCopy(filepath.Join(testDir, "50-cloud-init.yaml"), configPath) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Setup ifupdown and parse test file. ci, err := newCloudInitWith(configPath, filepath.Join(tmpDir, "nothing.json"), filepath.Join(tmpDir, "cloud.cfg.d")) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) c.ifaceBackends = append(c.ifaceBackends, namedIfaceBackend{"cloud-init", ci}) // Get list of interfaces. interfaces, err := c.GetInterfaces(context.Background()) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Verify interfaces read from file. err = testVerifyInterfaces(interfaces, resultsDir, 1) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Test add addresses. testIP3 := &net.IPNet{IP: net.ParseIP("1.2.3.5"), Mask: net.CIDRMask(24, 32)} err = c.AddAddress(context.Background(), eth0Name, testIP3, nil) - if err != nil { - t.Error(err) - } + assert.NoError(t, err) testIP4 := &net.IPNet{IP: net.ParseIP("fc00:5aa8:7160:d9eb:1:0:1:5"), Mask: net.CIDRMask(64, 128)} err = c.AddAddress(context.Background(), eth1Name, testIP4, net.ParseIP("fc00:5aa8:7160:d9eb::")) - if err != nil { - t.Error(err) - } + assert.NoError(t, err) // Get list of interfaces. interfaces, err = c.GetInterfaces(context.Background()) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Verify interfaces read from file. err = testVerifyInterfaces(interfaces, resultsDir, 2) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Read the current file and expected state. err = testVerifyResults(resultsDir, tmpDir, 1) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Test remove addresses. err = c.RemoveAddress(context.Background(), eth0Name, testIP1) - if err != nil { - t.Error(err) - } + assert.NoError(t, err) err = c.RemoveAddress(context.Background(), eth1Name, testIP2) - if err != nil { - t.Error(err) - } + assert.NoError(t, err) // Get list of interfaces. interfaces, err = c.GetInterfaces(context.Background()) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Verify interfaces read from file. err = testVerifyInterfaces(interfaces, resultsDir, 3) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Read the current file and expected state. err = testVerifyResults(resultsDir, tmpDir, 2) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Test rollback on failures. c.testAddress = "test_fail" err = c.AddAddress(context.Background(), eth0Name, testIP1, nil) - if err == nil || err.Error() != "aborted operation due to loss of internet" { - t.Errorf("Failed rollback test: %v", err) - } + assert.EqualError(t, err, "aborted operation due to loss of internet", "Failed rollback test") // Get list of interfaces. interfaces, err = c.GetInterfaces(context.Background()) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Verify interfaces read from file. err = testVerifyInterfaces(interfaces, resultsDir, 3) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Read the current file and expected state. err = testVerifyResults(resultsDir, tmpDir, 2) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Test remove gateway. err = c.AddAddress(context.Background(), eth0Name, testIP3, make(net.IP, 4)) - if err != nil { - t.Error(err) - } + assert.NoError(t, err) // Get list of interfaces. interfaces, err = c.GetInterfaces(context.Background()) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Verify interfaces read from file. err = testVerifyInterfaces(interfaces, resultsDir, 4) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Read the current file and expected state. err = testVerifyResults(resultsDir, tmpDir, 3) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Test add gateway rollback. err = c.AddAddress(context.Background(), eth0Name, testIP3, testGW1) - if err == nil || err.Error() != "aborted operation due to loss of internet" { - t.Errorf("Failed rollback test: %v", err) - } + assert.EqualError(t, err, "aborted operation due to loss of internet", "Failed rollback test") // Get list of interfaces. interfaces, err = c.GetInterfaces(context.Background()) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Verify interfaces read from file. err = testVerifyInterfaces(interfaces, resultsDir, 4) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Read the current file and expected state. err = testVerifyResults(resultsDir, tmpDir, 3) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Test remove gateway. c.testAddress = "test_success" err = c.AddAddress(context.Background(), eth0Name, testIP3, testGW1) - if err != nil { - t.Error(err) - } + assert.NoError(t, err) // Get list of interfaces. interfaces, err = c.GetInterfaces(context.Background()) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Verify interfaces read from file. err = testVerifyInterfaces(interfaces, resultsDir, 3) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Read the current file and expected state. err = testVerifyResults(resultsDir, tmpDir, 2) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Test removing route. err = c.RemoveRoute(context.Background(), eth1Name, ipv6Dest, testGW2) - if err != nil { - t.Error(err) - } + assert.NoError(t, err) // Get list of interfaces. interfaces, err = c.GetInterfaces(context.Background()) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Verify interfaces read from file. err = testVerifyInterfaces(interfaces, resultsDir, 5) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Read the current file and expected state. err = testVerifyResults(resultsDir, tmpDir, 4) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Test add route. ipv4Dest := &net.IPNet{ @@ -544,25 +498,17 @@ func TestLinuxConfigurator(t *testing.T) { } testGW3 := net.ParseIP("1.2.3.254") err = c.AddRoute(context.Background(), eth0Name, ipv4Dest, testGW3, 100) - if err != nil { - t.Error(err) - } + assert.NoError(t, err) // Get list of interfaces. interfaces, err = c.GetInterfaces(context.Background()) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Verify interfaces read from file. err = testVerifyInterfaces(interfaces, resultsDir, 6) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Read the current file and expected state. err = testVerifyResults(resultsDir, tmpDir, 5) - if err != nil { - t.Error(err) - } + require.NoError(t, err) } diff --git a/configurator_windows.go b/configurator_windows.go index bdeef59..c728fda 100644 --- a/configurator_windows.go +++ b/configurator_windows.go @@ -3,10 +3,12 @@ package netconfig import ( "bytes" "context" + "errors" "fmt" "net" "net/netip" "os" + "strings" "golang.org/x/sys/windows" "golang.zx2c4.com/wireguard/windows/tunnel/winipcfg" @@ -21,17 +23,22 @@ type windowsConfigurator struct { *configOptions } -// Returns the windows network configurator. Behaviour is tuned with Option -// values such as WithTestAddress and WithConnectivityCheck; use SetLogger to -// replace the package-wide logger. -func NewConfigurator(opts ...Option) (configurator Configurator, err error) { +// Returns the windows network configurator. ctx is accepted to match the Linux +// constructor, whose backend detection queries the init system; nothing here +// needs it. Behaviour is tuned with Option values such as WithTestAddress and +// WithConnectivityCheck; use SetLogger to replace the package-wide logger. +// +// Unlike Linux, no backend is required: winipcfg writes an adapter's addresses +// and routes straight into the registry, so a change persists across a reboot +// without any configuration file behind it. cloud-init is registered when found +// so an image-provisioned host keeps its two sources of truth in step. +func NewConfigurator(ctx context.Context, opts ...Option) (Configurator, error) { options := newConfigOptions(opts...) c := new(windowsConfigurator) c.configOptions = options - configurator = c var ci *cloudInit - ci, err = newCloudInit(options.backupRetention) + ci, err := newCloudInit(options.backupRetention) if err != nil { logger.Println("error parsing cloud-init:", err) } @@ -44,8 +51,28 @@ func NewConfigurator(opts ...Option) (configurator Configurator, err error) { // Detection failures are logged above; they are not fatal to constructing // the configurator, so do not propagate them to the caller. - err = nil - return + return c, nil +} + +// ipAdapterDHCPState reports whether the adapter runs a DHCP client for each +// address family. Windows exposes the two very differently: DHCPv4 is an +// adapter flag, while there is no DHCPv6 flag at all — an adapter is running a +// DHCPv6 client exactly when one of its addresses says it was learned from one. +func ipAdapterDHCPState(ipAdapter *winipcfg.IPAdapterAddresses) (dhcp4, dhcp6 bool) { + dhcp4 = ipAdapter.Flags&winipcfg.IPAAFlagDhcpv4Enabled != 0 + for unicast := ipAdapter.FirstUnicastAddress; unicast != nil; unicast = unicast.Next { + ip := unicast.Address.IP() + if ip == nil || ip.To4() != nil { + continue + } + // IP_ADAPTER_UNICAST_ADDRESS carries the raw Win32 enum, so compare + // against winipcfg's typed constant for it rather than a bare 3. + if unicast.PrefixOrigin == int32(winipcfg.PrefixOriginDHCP) { + dhcp6 = true + break + } + } + return dhcp4, dhcp6 } // Get IP addresses from ip adapter. @@ -106,6 +133,32 @@ func ipAdapterAddresses(ipAdapter *winipcfg.IPAdapterAddresses) (addresses []*ne return } +// adapterIsPhysical reports whether an adapter is backed by hardware. The +// interface type cannot answer that on its own: Windows hands a Hyper-V virtual +// switch, a VPN client's tunnel, and a real NIC the same Ethernet type. The +// interface row's hardware flag does answer it, and the endpoint flag rules out +// the host's own side of a virtual switch, which claims hardware backing it does +// not have. Adapters whose row cannot be read fall back to the interface type. +func adapterIsPhysical(adapter *winipcfg.IPAdapterAddresses) bool { + row, err := adapter.LUID.Interface() + if err != nil { + return isPhysicalIfType(adapter.IfType) + } + if row.InterfaceAndOperStatusFlags&winipcfg.IAOSFEndPointInterface != 0 { + return false + } + return row.InterfaceAndOperStatusFlags&winipcfg.IAOSFHardwareInterface != 0 +} + +// isPhysicalIfType reports whether an interface type is one a physical NIC uses. +func isPhysicalIfType(ifType winipcfg.IfType) bool { + switch uint32(ifType) { + case windows.IF_TYPE_ETHERNET_CSMACD, windows.IF_TYPE_IEEE80211: + return true + } + return false +} + // Get list of interfaces and their configs. func (c *windowsConfigurator) GetInterfaces(ctx context.Context) (interfaces []*Interface, err error) { // Reference of zero IP addresses. @@ -137,6 +190,9 @@ func (c *windowsConfigurator) GetInterfaces(ctx context.Context) (interfaces []* i.Name = ipAdapter.FriendlyName() i.MAC = ipAdapter.PhysicalAddress() i.Link = ipAdapter.LUID + i.Up = ipAdapter.OperStatus == winipcfg.IfOperStatusUp + i.Physical = adapterIsPhysical(ipAdapter) + i.DHCP4, i.DHCP6 = ipAdapterDHCPState(ipAdapter) for _, addr := range ipAdapterAddresses(ipAdapter) { if !addr.IP.IsLinkLocalUnicast() { i.Addresses = append(i.Addresses, addr) @@ -1008,3 +1064,236 @@ func (c *windowsConfigurator) SetDNS(ctx context.Context, iface string, servers applyLiveDNSWindows(ctx, iface, servers, searchDomains) return nil } + +// runNetsh runs netsh and folds its output into any error. netsh reports its +// failures on stdout, not stderr, so without this a failed call surfaces as a +// bare "exit status 1" with the reason discarded. +func runNetsh(ctx context.Context, args ...string) error { + out, err := runCommand(ctx, "netsh", args...) + if err == nil { + return nil + } + if reason := strings.TrimSpace(strings.Join(out, " ")); reason != "" { + return fmt.Errorf("netsh %s: %w: %s", strings.Join(args, " "), err, reason) + } + return fmt.Errorf("netsh %s: %w", strings.Join(args, " "), err) +} + +// The netsh argument builders below all use the named-parameter form rather than +// netsh's positional shorthand ("set address name=X static 1.2.3.4 255.255.255.0 +// 1.2.3.1"), which is order-sensitive and reads as a puzzle. + +// netshEnableDHCP4Args switches an adapter's IPv4 configuration to DHCP. This +// both records the choice and starts the DHCP client, discarding whatever +// static addresses the adapter held. +func netshEnableDHCP4Args(iface string) []string { + return []string{"interface", "ipv4", "set", "address", "name=" + iface, "source=dhcp"} +} + +// netshDNSFromDHCP4Args makes the adapter take its resolvers from the lease. +// Switching the address source to DHCP does not do this: the DNS servers are a +// separate setting and a statically configured resolver survives the switch. +func netshDNSFromDHCP4Args(iface string) []string { + return []string{"interface", "ipv4", "set", "dnsservers", "name=" + iface, "source=dhcp"} +} + +// netshSetStatic4Args replaces the adapter's IPv4 configuration with a single +// static address. There is no netsh verb for "stop being a DHCP client": an +// adapter leaves DHCP by being given a static address, so this is how DHCPv4 is +// turned off. +// +// A nil gateway is written as "none" rather than omitted. Omitting it leaves the +// previous default gateway in place, which on an adapter that has just stopped +// leasing means keeping a gateway the DHCP server handed out. +func netshSetStatic4Args(iface string, addr *net.IPNet, gateway net.IP) []string { + args := []string{ + "interface", "ipv4", "set", "address", + "name=" + iface, + "source=static", + "address=" + addr.IP.String(), + "mask=" + net.IP(addr.Mask).String(), + } + if gateway != nil { + return append(args, "gateway="+gateway.String()) + } + return append(args, "gateway=none") +} + +// netshAddStatic4Args adds a secondary static IPv4 address. "set address" +// replaces the adapter's entire IPv4 configuration with the one address it is +// given, so every address after the first has to be re-added with this. +func netshAddStatic4Args(iface string, addr *net.IPNet) []string { + return []string{ + "interface", "ipv4", "add", "address", + "name=" + iface, + "address=" + addr.IP.String(), + "mask=" + net.IP(addr.Mask).String(), + } +} + +// netshSetDHCP6Args turns the DHCPv6 client on or off. +// +// IPv6 has no "source=dhcp" counterpart, because Windows does not model DHCPv6 +// as an address source. A host runs a DHCPv6 client when a router advertisement +// tells it to, via the M (managed address) and O (other stateful config) bits; +// these two interface flags are the local override of those bits. Router +// discovery is enabled alongside them because DHCPv6 conveys no default route — +// only router advertisements do — so an interface leasing an address with +// router discovery off would have no way to reach anything. +// +// Disabling leaves router discovery alone: whatever default route the RA +// provides is not this call's to take away. +func netshSetDHCP6Args(iface string, enable bool) []string { + args := []string{"interface", "ipv6", "set", "interface", "interface=" + iface} + if enable { + return append(args, "routerdiscovery=enabled", "managedaddress=enabled", "otherstateful=enabled") + } + return append(args, "managedaddress=disabled", "otherstateful=disabled") +} + +// ipconfigRenewArgs asks the DHCP client to acquire a lease now. Switching the +// address source to DHCP starts the client, but a renew makes the lease arrive +// before this call returns rather than at the client's own pace. +func ipconfigRenewArgs(iface string) []string { + return []string{"/renew", iface} +} + +// requireElevation reports an error unless this process holds an elevated token. +// Every write path below reconfigures a live NIC and fails with a bare access +// denied otherwise; under UAC an account in the Administrators group still runs +// with a filtered standard-user token unless it was explicitly elevated. Saying +// so up front beats a netsh error the caller has to decode. +func requireElevation() error { + if windows.GetCurrentProcessToken().IsElevated() { + return nil + } + return fmt.Errorf("changing an adapter's DHCP configuration requires an elevated process") +} + +// Enable or disable the DHCP client for each address family on an interface. +// +// Windows keeps no configuration file of its own, so unlike Linux the adapter is +// both the persisted configuration and the running system, and there is nothing +// to reconcile later: the netsh calls below record the choice and act on it at +// once. A file backend that is also present (cloudbase-init) is written first, +// so a re-provision does not undo what was just set. +// +// The two families are asymmetric. IPv4 has an address source that can be set to +// dhcp or static. IPv6 does not — DHCPv6 is driven by the router advertisement's +// M and O bits, which netshSetDHCP6Args overrides locally. Note that a DHCPv6 +// lease carries no default route, so an interface's IPv6 reachability still +// depends on router advertisements either way. +func (c *windowsConfigurator) SetDHCP(ctx context.Context, iface string, dhcp4, dhcp6 bool) error { + // Check this before touching anything, so a non-elevated caller is told why + // rather than left with the file backends written and the adapter untouched. + if err := requireElevation(); err != nil { + return err + } + + applyIfaceDHCP(ctx, c.ifaceBackends, iface, dhcp4, dhcp6) + + var errs []error + if err := setAdapterDHCP4(ctx, iface, dhcp4); err != nil { + errs = append(errs, err) + } + if err := runNetsh(ctx, netshSetDHCP6Args(iface, dhcp6)...); err != nil { + errs = append(errs, fmt.Errorf("failed to set dhcp6 on %s: %w", iface, err)) + } + return errors.Join(errs...) +} + +// setAdapterDHCP4 switches the adapter's IPv4 address source. +func setAdapterDHCP4(ctx context.Context, iface string, enable bool) error { + if enable { + if err := runNetsh(ctx, netshEnableDHCP4Args(iface)...); err != nil { + return fmt.Errorf("failed to enable dhcp4 on %s: %w", iface, err) + } + + // The resolvers are a separate setting that survives the address-source + // switch; a statically configured one would otherwise outlive the static + // address it was set alongside. Failing to hand DNS back to the lease + // leaves a working interface, so it is logged rather than returned. + if err := runNetsh(ctx, netshDNSFromDHCP4Args(iface)...); err != nil { + logger.Printf("SetDHCP: %s: enabled dhcp4 but failed to take DNS from the lease: %v", iface, err) + } + + // Setting the source starts the DHCP client; renewing makes the lease + // arrive before this call returns. A renew that finds no DHCP server + // fails, which does not undo the configuration that was just written. + if _, err := runCommand(ctx, "ipconfig", ipconfigRenewArgs(iface)...); err != nil { + logger.Printf("SetDHCP: %s: enabled dhcp4 but no lease acquired yet: %v", iface, err) + } + return nil + } + + // An adapter leaves DHCP by being given a static address. Re-assert the + // addresses it currently holds — which are the leased ones — so it keeps the + // address it is reachable on and simply stops renewing it. + adapter, err := findAdapter(iface) + if err != nil { + return err + } + + // An adapter that is not leasing has nothing to convert. Running the static + // assignment on it anyway would tear its IPv4 configuration down and build + // it back up for no reason. + if dhcp4, _ := ipAdapterDHCPState(adapter); !dhcp4 { + return nil + } + + addrs := ipv4AdapterAddresses(adapter) + if len(addrs) == 0 { + return fmt.Errorf("cannot disable dhcp4 on %s: it holds no IPv4 address to keep statically", iface) + } + + // "set address" replaces the adapter's whole IPv4 configuration with the one + // address it is given, so the rest are re-added after it. + if err := runNetsh(ctx, netshSetStatic4Args(iface, addrs[0], ipAdapterGateway4(adapter))...); err != nil { + return fmt.Errorf("failed to disable dhcp4 on %s: %w", iface, err) + } + var errs []error + for _, addr := range addrs[1:] { + if err := runNetsh(ctx, netshAddStatic4Args(iface, addr)...); err != nil { + errs = append(errs, fmt.Errorf("failed to restore secondary address %s on %s: %w", addr, iface, err)) + } + } + return errors.Join(errs...) +} + +// findAdapter returns the adapter with the given friendly name, which is the +// name netsh and ipconfig also identify it by. +func findAdapter(iface string) (*winipcfg.IPAdapterAddresses, error) { + ipAdapters, err := winipcfg.GetAdaptersAddresses(windows.AF_UNSPEC, winipcfg.GAAFlagIncludePrefix|winipcfg.GAAFlagSkipAnycast|winipcfg.GAAFlagSkipMulticast|winipcfg.GAAFlagSkipDNSServer|winipcfg.GAAFlagSkipFriendlyName|winipcfg.GAAFlagSkipDNSInfo) + if err != nil { + return nil, err + } + for _, ipAdapter := range ipAdapters { + if ipAdapter.FriendlyName() == iface { + return ipAdapter, nil + } + } + return nil, fmt.Errorf("no adapter found with name: %s", iface) +} + +// ipv4AdapterAddresses returns every non-link-local IPv4 address the adapter +// holds, in the order it holds them, which is what converting it from DHCP to +// static must re-assert. +func ipv4AdapterAddresses(adapter *winipcfg.IPAdapterAddresses) (addrs []*net.IPNet) { + for _, addr := range ipAdapterAddresses(adapter) { + if addr.IP.To4() == nil || addr.IP.IsLinkLocalUnicast() { + continue + } + addrs = append(addrs, addr) + } + return addrs +} + +// ipAdapterGateway4 returns the adapter's first IPv4 default gateway, or nil. +func ipAdapterGateway4(ipAdapter *winipcfg.IPAdapterAddresses) net.IP { + for gw := ipAdapter.FirstGatewayAddress; gw != nil; gw = gw.Next { + if ip := gw.Address.IP(); ip != nil && ip.To4() != nil { + return ip + } + } + return nil +} diff --git a/cpanel_test.go b/cpanel_test.go index 2385c44..75a4533 100644 --- a/cpanel_test.go +++ b/cpanel_test.go @@ -4,6 +4,9 @@ import ( "os" "path/filepath" "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) // parseJSON unmarshals a single line of whmapi1 output; verify its guard on @@ -14,33 +17,24 @@ func TestCpanelParseJSON(t *testing.T) { t.Run("valid single line", func(t *testing.T) { var v cpanelBase err := c.parseJSON([]string{`{"metadata":{"command":"listips","result":1}}`}, &v) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if v.Metadata.Command != "listips" || v.Metadata.Result != 1 { - t.Errorf("decoded = %+v, want command=listips result=1", v.Metadata) - } + require.NoError(t, err) + assert.Equal(t, "listips", v.Metadata.Command) + assert.Equal(t, 1, v.Metadata.Result) }) t.Run("no output errors", func(t *testing.T) { var v cpanelBase - if err := c.parseJSON(nil, &v); err == nil { - t.Error("expected error for empty output") - } + assert.Error(t, c.parseJSON(nil, &v)) }) t.Run("multiple lines errors", func(t *testing.T) { var v cpanelBase - if err := c.parseJSON([]string{"{}", "{}"}, &v); err == nil { - t.Error("expected error for multi-line output") - } + assert.Error(t, c.parseJSON([]string{"{}", "{}"}, &v)) }) t.Run("invalid json errors", func(t *testing.T) { var v cpanelBase - if err := c.parseJSON([]string{"not json"}, &v); err == nil { - t.Error("expected error for invalid json") - } + assert.Error(t, c.parseJSON([]string{"not json"}, &v)) }) } @@ -51,93 +45,53 @@ func TestSetWWWAcctAddr(t *testing.T) { t.Run("replaces existing ADDR and preserves other lines", func(t *testing.T) { path := filepath.Join(t.TempDir(), "wwwacct.conf") content := "HOST server.example.com\nADDR 203.0.113.10\nNS ns1.example.com\n" - if err := os.WriteFile(path, []byte(content), 0644); err != nil { - t.Fatal(err) - } - if err := setWWWAcctAddr(path, "ADDR", "203.0.113.20"); err != nil { - t.Fatalf("setWWWAcctAddr: %v", err) - } + require.NoError(t, os.WriteFile(path, []byte(content), 0644)) + require.NoError(t, setWWWAcctAddr(path, "ADDR", "203.0.113.20")) got, err := os.ReadFile(path) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) want := "HOST server.example.com\nADDR 203.0.113.20\nNS ns1.example.com\n" - if string(got) != want { - t.Errorf("got:\n%s\nwant:\n%s", got, want) - } + assert.Equal(t, want, string(got)) }) t.Run("appends ADDR when absent", func(t *testing.T) { path := filepath.Join(t.TempDir(), "wwwacct.conf") - if err := os.WriteFile(path, []byte("HOST server.example.com\n"), 0644); err != nil { - t.Fatal(err) - } - if err := setWWWAcctAddr(path, "ADDR", "203.0.113.20"); err != nil { - t.Fatalf("setWWWAcctAddr: %v", err) - } + require.NoError(t, os.WriteFile(path, []byte("HOST server.example.com\n"), 0644)) + require.NoError(t, setWWWAcctAddr(path, "ADDR", "203.0.113.20")) got, err := os.ReadFile(path) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) want := "HOST server.example.com\nADDR 203.0.113.20\n" - if string(got) != want { - t.Errorf("got:\n%q\nwant:\n%q", got, want) - } + assert.Equal(t, want, string(got)) }) t.Run("setting ADDR does not touch ADDR6", func(t *testing.T) { path := filepath.Join(t.TempDir(), "wwwacct.conf") content := "ADDR 203.0.113.10\nADDR6 2001:db8::1\n" - if err := os.WriteFile(path, []byte(content), 0644); err != nil { - t.Fatal(err) - } - if err := setWWWAcctAddr(path, "ADDR", "203.0.113.20"); err != nil { - t.Fatalf("setWWWAcctAddr: %v", err) - } + require.NoError(t, os.WriteFile(path, []byte(content), 0644)) + require.NoError(t, setWWWAcctAddr(path, "ADDR", "203.0.113.20")) got, err := os.ReadFile(path) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) want := "ADDR 203.0.113.20\nADDR6 2001:db8::1\n" - if string(got) != want { - t.Errorf("got:\n%s\nwant:\n%s", got, want) - } + assert.Equal(t, want, string(got)) }) t.Run("setting ADDR6 replaces it and leaves ADDR alone", func(t *testing.T) { path := filepath.Join(t.TempDir(), "wwwacct.conf") content := "ADDR 203.0.113.10\nADDR6 2001:db8::1\n" - if err := os.WriteFile(path, []byte(content), 0644); err != nil { - t.Fatal(err) - } - if err := setWWWAcctAddr(path, "ADDR6", "2001:db8::20"); err != nil { - t.Fatalf("setWWWAcctAddr: %v", err) - } + require.NoError(t, os.WriteFile(path, []byte(content), 0644)) + require.NoError(t, setWWWAcctAddr(path, "ADDR6", "2001:db8::20")) got, err := os.ReadFile(path) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) want := "ADDR 203.0.113.10\nADDR6 2001:db8::20\n" - if string(got) != want { - t.Errorf("got:\n%s\nwant:\n%s", got, want) - } + assert.Equal(t, want, string(got)) }) t.Run("appends ADDR6 when absent", func(t *testing.T) { path := filepath.Join(t.TempDir(), "wwwacct.conf") - if err := os.WriteFile(path, []byte("ADDR 203.0.113.10\n"), 0644); err != nil { - t.Fatal(err) - } - if err := setWWWAcctAddr(path, "ADDR6", "2001:db8::20"); err != nil { - t.Fatalf("setWWWAcctAddr: %v", err) - } + require.NoError(t, os.WriteFile(path, []byte("ADDR 203.0.113.10\n"), 0644)) + require.NoError(t, setWWWAcctAddr(path, "ADDR6", "2001:db8::20")) got, err := os.ReadFile(path) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) want := "ADDR 203.0.113.10\nADDR6 2001:db8::20\n" - if string(got) != want { - t.Errorf("got:\n%s\nwant:\n%s", got, want) - } + assert.Equal(t, want, string(got)) }) } diff --git a/dhcp_unit_test.go b/dhcp_unit_test.go new file mode 100644 index 0000000..088155e --- /dev/null +++ b/dhcp_unit_test.go @@ -0,0 +1,659 @@ +package netconfig + +import ( + "context" + "net" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gopkg.in/ini.v1" +) + +// cidr is a terse *net.IPNet literal for the tests below. +func cidr(t *testing.T, s string) *net.IPNet { + t.Helper() + ip, ipnet, err := net.ParseCIDR(s) + require.NoError(t, err) + ipnet.IP = ip + return ipnet +} + +// A netplan interface must report and record each family's client +// independently, and must not grow a dhcp4/dhcp6 key that was never there. +func TestNetplanSetDHCP(t *testing.T) { + t.Run("reads absent keys as off", func(t *testing.T) { + dhcp4, dhcp6 := (&npInterface{}).dhcpState() + assert.False(t, dhcp4) + assert.False(t, dhcp6) + }) + + t.Run("enables one family without touching the other", func(t *testing.T) { + c := npInterface{} + c.setDHCP(true, false) + require.NotNil(t, c.DHCP4) + assert.True(t, *c.DHCP4) + assert.Nil(t, c.DHCP6, "dhcp6 was absent and stays absent when disabled") + }) + + t.Run("disabling drops that family's overrides only", func(t *testing.T) { + c := npInterface{ + DHCP4: boolPtr(true), + DHCP6: boolPtr(true), + DHCP4Overrides: map[string]any{"use-dns": false}, + DHCP6Overrides: map[string]any{"use-dns": false}, + } + c.setDHCP(false, true) + assert.False(t, *c.DHCP4) + assert.Nil(t, c.DHCP4Overrides, "overrides of a stopped client are dead config") + assert.True(t, *c.DHCP6) + assert.NotNil(t, c.DHCP6Overrides) + }) + + t.Run("disabling an absent key does not write it", func(t *testing.T) { + c := npInterface{} + c.setDHCP(false, false) + assert.Nil(t, c.DHCP4) + assert.Nil(t, c.DHCP6) + }) + + t.Run("round trips through dhcpState", func(t *testing.T) { + c := npInterface{} + c.setDHCP(true, true) + dhcp4, dhcp6 := c.dhcpState() + assert.True(t, dhcp4) + assert.True(t, dhcp6) + }) +} + +func TestCloudInitSetDHCP(t *testing.T) { + c := ciInterface{DHCP4: boolPtr(true), DHCP6Overrides: map[string]any{"use-dns": false}} + c.setDHCP(false, false) + require.NotNil(t, c.DHCP4) + assert.False(t, *c.DHCP4) + assert.Nil(t, c.DHCP6, "absent stays absent") + assert.Nil(t, c.DHCP6Overrides) +} + +func TestParseNetworkdDHCP(t *testing.T) { + tests := []struct { + value string + dhcp4, dhcp6 bool + }{ + {value: "yes", dhcp4: true, dhcp6: true}, + {value: "true", dhcp4: true, dhcp6: true}, + {value: "both", dhcp4: true, dhcp6: true}, + {value: " YES ", dhcp4: true, dhcp6: true}, + {value: "ipv4", dhcp4: true}, + {value: "v4", dhcp4: true}, + {value: "ipv6", dhcp6: true}, + {value: "no"}, + {value: "none"}, + {value: ""}, + {value: "garbage"}, + } + for _, tt := range tests { + t.Run(tt.value, func(t *testing.T) { + dhcp4, dhcp6 := parseNetworkdDHCP(tt.value) + assert.Equal(t, tt.dhcp4, dhcp4, "dhcp4") + assert.Equal(t, tt.dhcp6, dhcp6, "dhcp6") + }) + } +} + +func TestFormatNetworkdDHCP(t *testing.T) { + tests := []struct { + name string + dhcp4, dhcp6 bool + value string + keep bool + }{ + {name: "both", dhcp4: true, dhcp6: true, value: "yes", keep: true}, + {name: "v4", dhcp4: true, value: "ipv4", keep: true}, + {name: "v6", dhcp6: true, value: "ipv6", keep: true}, + {name: "neither", value: "", keep: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + value, keep := formatNetworkdDHCP(tt.dhcp4, tt.dhcp6) + assert.Equal(t, tt.keep, keep) + assert.Equal(t, tt.value, value) + }) + } +} + +// networkdDHCPValue builds a [Network] section carrying the given DHCP= value +// ("" for no key at all), runs setNetworkdDHCP over it, and reports the +// resulting value plus whether the key survived. +func networkdDHCPValue(t *testing.T, start string, dhcp4, dhcp6 bool) (string, bool) { + t.Helper() + cfg := ini.Empty() + sec, err := cfg.NewSection("Network") + require.NoError(t, err) + if start != "" { + _, err = sec.NewKey("DHCP", start) + require.NoError(t, err) + } + + require.NoError(t, setNetworkdDHCP(sec, dhcp4, dhcp6)) + + if !sec.HasKey("DHCP") { + return "", false + } + return sec.Key("DHCP").String(), true +} + +func TestSetNetworkdDHCP(t *testing.T) { + t.Run("narrows a dual-stack lease to one family", func(t *testing.T) { + value, present := networkdDHCPValue(t, "yes", false, true) + require.True(t, present) + assert.Equal(t, "ipv6", value) + }) + + t.Run("removes the key when no family is left", func(t *testing.T) { + _, present := networkdDHCPValue(t, "ipv4", false, false) + assert.False(t, present, "DHCP= should be removed, not written as no") + }) + + t.Run("creates the key on a config that lacked it", func(t *testing.T) { + value, present := networkdDHCPValue(t, "", true, false) + require.True(t, present) + assert.Equal(t, "ipv4", value) + }) + + t.Run("leaves an absent key absent when disabling", func(t *testing.T) { + _, present := networkdDHCPValue(t, "", false, false) + assert.False(t, present) + }) +} + +// A .network file must round trip through SetIfaceDHCP and GetInterfaces, and +// SetIfaceAddresses must not disturb the DHCP key on the way past. +func TestNetworkdSetIfaceDHCP(t *testing.T) { + newNetworkdDir := func(t *testing.T, body string) (string, string) { + t.Helper() + dir := t.TempDir() + path := filepath.Join(dir, "eth0.network") + require.NoError(t, os.WriteFile(path, []byte(body), 0644)) + return dir, path + } + + t.Run("enables a family and reads it back", func(t *testing.T) { + dir, path := newNetworkdDir(t, "[Match]\nName=eth0\n\n[Network]\nAddress=1.2.3.4/24\n") + + nd, err := readNetworkdConfigDirectory(dir) + require.NoError(t, err) + require.NoError(t, nd.SetIfaceDHCP(context.Background(), "eth0", true, false)) + + cfg, err := ini.Load(path) + require.NoError(t, err) + assert.Equal(t, "ipv4", cfg.Section("Network").Key("DHCP").String()) + assert.Equal(t, "1.2.3.4/24", cfg.Section("Network").Key("Address").String(), + "the static address must survive enabling DHCP") + }) + + t.Run("a DHCP-only network still reports its state", func(t *testing.T) { + dir, _ := newNetworkdDir(t, "[Match]\nName=eth0\n\n[Network]\nDHCP=yes\n") + + nd, err := readNetworkdConfigDirectory(dir) + require.NoError(t, err) + ifaces, err := nd.GetInterfaces() + require.NoError(t, err) + require.Len(t, ifaces, 1) + assert.True(t, ifaces[0].DHCP4, "a network with no Address must still report DHCP") + assert.True(t, ifaces[0].DHCP6) + }) + + t.Run("setting addresses leaves DHCP alone", func(t *testing.T) { + dir, path := newNetworkdDir(t, "[Match]\nName=eth0\n\n[Network]\nDHCP=yes\n") + + nd, err := readNetworkdConfigDirectory(dir) + require.NoError(t, err) + err = nd.SetIfaceAddresses(context.Background(), "eth0", + []*net.IPNet{cidr(t, "1.2.3.4/24")}, net.ParseIP("1.2.3.1"), nil) + require.NoError(t, err) + + cfg, err := ini.Load(path) + require.NoError(t, err) + assert.Equal(t, "yes", cfg.Section("Network").Key("DHCP").String(), + "adding a static address must not disable DHCP") + assert.Equal(t, "1.2.3.4/24", cfg.Section("Network").Key("Address").String()) + }) +} + +func TestNSDHCPState(t *testing.T) { + tests := []struct { + name string + config map[string]string + dhcp4, dhcp6 bool + }{ + {name: "empty", config: map[string]string{}}, + {name: "static", config: map[string]string{"BOOTPROTO": "none"}}, + {name: "dhcp", config: map[string]string{"BOOTPROTO": "dhcp"}, dhcp4: true}, + {name: "uppercase dhcp", config: map[string]string{"BOOTPROTO": "DHCP"}, dhcp4: true}, + {name: "bootp is dynamic", config: map[string]string{"BOOTPROTO": "bootp"}, dhcp4: true}, + {name: "dhcpv6c", config: map[string]string{"DHCPV6C": "yes"}, dhcp6: true}, + { + name: "both", + config: map[string]string{"BOOTPROTO": "dhcp", "DHCPV6C": "yes"}, + dhcp4: true, dhcp6: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dhcp4, dhcp6 := nsDHCPState(tt.config) + assert.Equal(t, tt.dhcp4, dhcp4, "dhcp4") + assert.Equal(t, tt.dhcp6, dhcp6, "dhcp6") + }) + } +} + +func TestNetworkScriptsSetIfaceDHCP(t *testing.T) { + // Re-read the ifcfg file from disk so we assert on what was persisted. + readConfig := func(t *testing.T, dir string) map[string]string { + t.Helper() + ns, err := newNetworkScriptsWithConfig(dir) + require.NoError(t, err) + require.Len(t, ns.Interfaces, 1) + require.Len(t, ns.Interfaces[0].IFFiles, 1) + return ns.Interfaces[0].IFFiles[0].Config + } + + newDir := func(t *testing.T, body string) string { + t.Helper() + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "ifcfg-eth0"), []byte(body), 0644)) + return dir + } + + t.Run("enabling ipv4 keeps the static address", func(t *testing.T) { + dir := newDir(t, "DEVICE=eth0\nBOOTPROTO=none\nIPADDR0=1.2.3.4\nPREFIX0=24\n") + + ns, err := newNetworkScriptsWithConfig(dir) + require.NoError(t, err) + require.NoError(t, ns.SetIfaceDHCP(context.Background(), "eth0", true, false)) + + config := readConfig(t, dir) + assert.Equal(t, "dhcp", config["BOOTPROTO"]) + assert.Equal(t, "1.2.3.4", config["IPADDR0"], "the static address must survive") + }) + + t.Run("disabling ipv4 writes none and drops the dhclient option", func(t *testing.T) { + dir := newDir(t, "DEVICE=eth0\nBOOTPROTO=dhcp\nPERSISTENT_DHCLIENT=1\n") + + ns, err := newNetworkScriptsWithConfig(dir) + require.NoError(t, err) + require.NoError(t, ns.SetIfaceDHCP(context.Background(), "eth0", false, false)) + + config := readConfig(t, dir) + assert.Equal(t, "none", config["BOOTPROTO"]) + _, ok := config["PERSISTENT_DHCLIENT"] + assert.False(t, ok) + }) + + t.Run("the two families are independent", func(t *testing.T) { + dir := newDir(t, "DEVICE=eth0\nBOOTPROTO=dhcp\nDHCPV6C=yes\n") + + ns, err := newNetworkScriptsWithConfig(dir) + require.NoError(t, err) + require.NoError(t, ns.SetIfaceDHCP(context.Background(), "eth0", false, true)) + + config := readConfig(t, dir) + assert.Equal(t, "none", config["BOOTPROTO"]) + assert.Equal(t, "yes", config["DHCPV6C"]) + assert.Equal(t, "yes", config["IPV6INIT"], "DHCPv6 needs IPv6 turned on") + }) + + t.Run("adding an address leaves BOOTPROTO alone", func(t *testing.T) { + dir := newDir(t, "DEVICE=eth0\nBOOTPROTO=dhcp\n") + + ns, err := newNetworkScriptsWithConfig(dir) + require.NoError(t, err) + err = ns.SetIfaceAddresses(context.Background(), "eth0", + []*net.IPNet{cidr(t, "1.2.3.4/24")}, nil, nil) + require.NoError(t, err) + + config := readConfig(t, dir) + assert.Equal(t, "dhcp", config["BOOTPROTO"], + "a static address may coexist with a lease; only SetDHCP turns it off") + assert.Equal(t, "1.2.3.4", config["IPADDR0"]) + }) + + t.Run("reads the state back through GetInterfaces", func(t *testing.T) { + dir := newDir(t, "DEVICE=eth0\nBOOTPROTO=dhcp\nDHCPV6C=yes\n") + + ns, err := newNetworkScriptsWithConfig(dir) + require.NoError(t, err) + ifaces, err := ns.GetInterfaces() + require.NoError(t, err) + require.Len(t, ifaces, 1) + assert.True(t, ifaces[0].DHCP4) + assert.True(t, ifaces[0].DHCP6) + }) +} + +func TestIfUpDownSetDHCP(t *testing.T) { + t.Run("converts the stanza method for its own family", func(t *testing.T) { + iface := &ifInterface{Name: "eth0", Mode: "inet static"} + require.NoError(t, iface.setDHCP(true, false)) + assert.Equal(t, "inet dhcp", iface.Mode) + assert.True(t, iface.isDHCP) + + require.NoError(t, iface.setDHCP(false, false)) + assert.Equal(t, "inet static", iface.Mode) + assert.False(t, iface.isDHCP) + }) + + t.Run("inet6 stanza follows dhcp6", func(t *testing.T) { + iface := &ifInterface{Name: "eth0", Mode: "inet6 static"} + require.NoError(t, iface.setDHCP(false, true)) + assert.Equal(t, "inet6 dhcp", iface.Mode) + assert.True(t, iface.isDHCP) + }) + + t.Run("a stanza-less interface gets one", func(t *testing.T) { + iface := &ifInterface{Name: "eth0"} + require.NoError(t, iface.setDHCP(true, false)) + assert.Equal(t, "inet dhcp", iface.Mode) + }) + + t.Run("errors rather than silently ignoring the other family", func(t *testing.T) { + iface := &ifInterface{Name: "eth0", Mode: "inet static"} + err := iface.setDHCP(false, true) + assert.ErrorContains(t, err, "other family") + assert.Equal(t, "inet static", iface.Mode, "the stanza must be untouched on error") + }) + + t.Run("errors on both families at once", func(t *testing.T) { + iface := &ifInterface{Name: "eth0"} + assert.ErrorContains(t, iface.setDHCP(true, true), "both families") + }) + + t.Run("refuses methods it cannot convert", func(t *testing.T) { + for _, method := range []string{"loopback", "ppp"} { + iface := &ifInterface{Name: "lo", Mode: "inet " + method} + assert.ErrorContains(t, iface.setDHCP(true, false), method) + } + }) + + t.Run("dhcpState reports the stanza's family only", func(t *testing.T) { + dhcp4, dhcp6 := (&ifInterface{Mode: "inet dhcp", isDHCP: true}).dhcpState() + assert.True(t, dhcp4) + assert.False(t, dhcp6) + + dhcp4, dhcp6 = (&ifInterface{Mode: "inet6 dhcp", isDHCP: true}).dhcpState() + assert.False(t, dhcp4) + assert.True(t, dhcp6) + + dhcp4, dhcp6 = (&ifInterface{Mode: "inet static"}).dhcpState() + assert.False(t, dhcp4) + assert.False(t, dhcp6) + }) +} + +// Switching an ifupdown interface to DHCP must drop the static addressing the +// stanza carried, since a "dhcp" stanza has no address option to hold it. +func TestIfUpDownSetIfaceDHCPPersists(t *testing.T) { + dir := t.TempDir() + cfgPath := filepath.Join(dir, "interfaces") + config := "auto eth0\niface eth0 inet static\n\taddress 1.2.3.4/24\n\tgateway 1.2.3.1\n" + require.NoError(t, os.WriteFile(cfgPath, []byte(config), 0644)) + + i := &ifUpDown{BaseConfig: cfgPath, BackupDir: filepath.Join(dir, "backup")} + require.NoError(t, os.Mkdir(i.BackupDir, 0755)) + require.NoError(t, i.ReadFile(cfgPath)) + require.NoError(t, i.SetIfaceDHCP(context.Background(), "eth0", true, false)) + + // Re-read from disk to confirm what was persisted. + reread := &ifUpDown{BaseConfig: cfgPath, BackupDir: i.BackupDir} + require.NoError(t, reread.ReadFile(cfgPath)) + require.Len(t, reread.Interfaces, 1) + eth0 := reread.Interfaces[0] + assert.Equal(t, "inet dhcp", eth0.Mode) + assert.True(t, eth0.isDHCP) + assert.Empty(t, eth0.Addresses, "a dhcp stanza cannot carry a static address") + assert.Nil(t, eth0.Gateway4) + + // And the interface reports itself as DHCPv4. + ifaces, err := reread.GetInterfaces() + require.NoError(t, err) + require.Len(t, ifaces, 1) + assert.True(t, ifaces[0].DHCP4) + assert.False(t, ifaces[0].DHCP6) +} + +// SetIfaceAddresses must leave netplan's dhcp4 alone: a static address and a +// lease coexist, and only SetIfaceDHCP decides otherwise. +func TestNetplanSetIfaceAddressesKeepsDHCP(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "01-netcfg.yaml") + config := "network:\n version: 2\n ethernets:\n eth0:\n dhcp4: true\n" + require.NoError(t, os.WriteFile(path, []byte(config), 0644)) + + np, err := readNetplanConfigDirectory(dir) + require.NoError(t, err) + err = np.SetIfaceAddresses(context.Background(), "eth0", + []*net.IPNet{cidr(t, "1.2.3.4/24")}, net.ParseIP("1.2.3.1"), nil) + require.NoError(t, err) + + reread, err := readNetplanConfigDirectory(dir) + require.NoError(t, err) + require.Len(t, reread.configs, 1) + eth0, ok := reread.configs[0].Network.Ethernets["eth0"] + require.True(t, ok) + require.NotNil(t, eth0.DHCP4) + assert.True(t, *eth0.DHCP4, "adding a static address must not disable DHCP") + assert.Equal(t, []string{"1.2.3.4/24"}, eth0.Addresses) +} + +// SetIfaceDHCP must write netplan's dhcp4/dhcp6 and leave the addresses alone. +func TestNetplanSetIfaceDHCPPersists(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "01-netcfg.yaml") + config := "network:\n version: 2\n ethernets:\n eth0:\n addresses:\n - 1.2.3.4/24\n" + require.NoError(t, os.WriteFile(path, []byte(config), 0644)) + + np, err := readNetplanConfigDirectory(dir) + require.NoError(t, err) + require.NoError(t, np.SetIfaceDHCP(context.Background(), "eth0", true, false)) + + reread, err := readNetplanConfigDirectory(dir) + require.NoError(t, err) + eth0, ok := reread.configs[0].Network.Ethernets["eth0"] + require.True(t, ok) + require.NotNil(t, eth0.DHCP4) + assert.True(t, *eth0.DHCP4) + assert.Nil(t, eth0.DHCP6, "the untouched family must not gain a key") + assert.Equal(t, []string{"1.2.3.4/24"}, eth0.Addresses, + "static addresses coexist with a lease") + + // And it reads back through GetInterfaces. + ifaces, err := reread.GetInterfaces() + require.NoError(t, err) + require.Len(t, ifaces, 1) + assert.True(t, ifaces[0].DHCP4) + assert.False(t, ifaces[0].DHCP6) +} + +// newTestCloudInit builds a cloudInit over a temporary network config file, so +// SetIfaceDHCP has somewhere real to Save to. +func newTestCloudInit(t *testing.T, config string) *cloudInit { + t.Helper() + dir := t.TempDir() + configPath := filepath.Join(dir, "network.yaml") + require.NoError(t, os.WriteFile(configPath, []byte(config), 0644)) + ci, err := newCloudInitWith(configPath, filepath.Join(dir, "nothing.json"), filepath.Join(dir, "cloud.cfg.d")) + require.NoError(t, err) + return ci +} + +// cloud-init's v1 schema encodes DHCP as a subnet type. Enabling a client must +// add that subnet without disturbing the static subnet beside it. +func TestCloudInitSetIfaceDHCPV1(t *testing.T) { + ci := newTestCloudInit(t, `network: + version: 1 + config: + - type: physical + name: eth0 + subnets: + - type: dhcp + - type: static + address: 1.2.3.4/24 + gateway: 1.2.3.1 +`) + + // Turning IPv4 off and IPv6 on must replace the dhcp subnet with a dhcp6 + // one and leave the static subnet in place. + require.NoError(t, ci.SetIfaceDHCP(context.Background(), "eth0", false, true)) + subnets := ci.Network.Configs[0].Subnets + require.Len(t, subnets, 2) + assert.Equal(t, "dhcp6", subnets[0].Type) + assert.Equal(t, "static", subnets[1].Type) + assert.Equal(t, "1.2.3.4/24", subnets[1].Address) + + ifaces, err := ci.GetInterfaces() + require.NoError(t, err) + require.Len(t, ifaces, 1) + assert.False(t, ifaces[0].DHCP4) + assert.True(t, ifaces[0].DHCP6) +} + +// The v1 reader must recognise every spelling of a DHCP subnet. +func TestCloudInitV1DHCPSubnetTypes(t *testing.T) { + for _, tt := range []struct { + subnetType string + dhcp4, dhcp6 bool + }{ + {subnetType: "dhcp", dhcp4: true}, + {subnetType: "dhcp4", dhcp4: true}, + {subnetType: "dhcp6", dhcp6: true}, + {subnetType: "ipv6_dhcpv6-stateful", dhcp6: true}, + {subnetType: "static"}, + } { + t.Run(tt.subnetType, func(t *testing.T) { + ci := newTestCloudInit(t, `network: + version: 1 + config: + - type: physical + name: eth0 + subnets: + - type: `+tt.subnetType+` + address: 1.2.3.4/24 +`) + ifaces, err := ci.GetInterfaces() + require.NoError(t, err) + require.Len(t, ifaces, 1) + assert.Equal(t, tt.dhcp4, ifaces[0].DHCP4, "dhcp4") + assert.Equal(t, tt.dhcp6, ifaces[0].DHCP6, "dhcp6") + }) + } +} + +// The v2 schema keeps DHCP in dhcp4/dhcp6 keys beside the static addresses. +func TestCloudInitSetIfaceDHCPV2(t *testing.T) { + ci := newTestCloudInit(t, `network: + version: 2 + ethernets: + eth0: + addresses: + - 1.2.3.4/24 +`) + require.NoError(t, ci.SetIfaceDHCP(context.Background(), "eth0", true, false)) + + eth0, ok := ci.Network.Ethernets["eth0"] + require.True(t, ok) + require.NotNil(t, eth0.DHCP4) + assert.True(t, *eth0.DHCP4) + assert.Nil(t, eth0.DHCP6, "the untouched family must not gain a key") + assert.Equal(t, []string{"1.2.3.4/24"}, eth0.Addresses) +} + +func TestNMConnectionDHCPState(t *testing.T) { + tests := []struct { + name string + method4, method6 string + dhcp4, dhcp6 bool + }{ + {name: "auto is dhcp", method4: "auto", method6: "auto", dhcp4: true, dhcp6: true}, + {name: "manual is static", method4: "manual", method6: "manual"}, + {name: "ipv6 dhcp method", method4: "manual", method6: "dhcp", dhcp6: true}, + {name: "disabled", method4: "disabled", method6: "link-local"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + conn := &nmConnection{Method4: tt.method4, Method6: tt.method6} + dhcp4, dhcp6 := conn.dhcpState() + assert.Equal(t, tt.dhcp4, dhcp4, "dhcp4") + assert.Equal(t, tt.dhcp6, dhcp6, "dhcp6") + }) + } +} + +// Turning a lease off must leave behind a method that still serves whatever +// static addresses the connection holds, and not strand IPv6 with no address. +func TestNMStaticMethod(t *testing.T) { + assert.Equal(t, "manual", nmStaticMethod4(true)) + assert.Equal(t, "disabled", nmStaticMethod4(false)) + assert.Equal(t, "manual", nmStaticMethod6(true)) + assert.Equal(t, "link-local", nmStaticMethod6(false)) +} + +// applyIfaceDHCP reports whether any backend persisted the change, which is what +// SetDHCP relies on before asking the running system for a lease. +func TestApplyIfaceDHCP(t *testing.T) { + t.Run("reports true when one backend succeeds", func(t *testing.T) { + failing := &fakeIfaceBackend{err: assert.AnError} + ok := &fakeIfaceBackend{} + applied := applyIfaceDHCP(context.Background(), []namedIfaceBackend{ + {name: "broken", backend: failing}, + {name: "good", backend: ok}, + }, "eth0", true, false) + + assert.True(t, applied) + assert.Equal(t, 1, failing.dhcpCalls, "a failing backend must not stop the others") + assert.Equal(t, 1, ok.dhcpCalls) + assert.True(t, ok.lastDHCP4) + assert.False(t, ok.lastDHCP6) + }) + + t.Run("reports false when every backend fails", func(t *testing.T) { + applied := applyIfaceDHCP(context.Background(), []namedIfaceBackend{ + {name: "broken", backend: &fakeIfaceBackend{err: assert.AnError}}, + }, "eth0", true, true) + assert.False(t, applied) + }) + + t.Run("reports false with no backends at all", func(t *testing.T) { + assert.False(t, applyIfaceDHCP(context.Background(), nil, "eth0", true, true)) + }) +} + +// renewDHCPOnBackends must skip backends that cannot reconfigure the running +// system (cloud-init) rather than erroring on them. +func TestRenewDHCPOnBackends(t *testing.T) { + renewer := &fakeRenewerBackend{} + backends := []namedIfaceBackend{ + {name: "cloud-init", backend: &fakeIfaceBackend{}}, // no renewDHCP method + {name: "netplan", backend: renewer}, + } + renewDHCPOnBackends(context.Background(), backends, "eth0") + assert.Equal(t, 1, renewer.renewCalls) + assert.Equal(t, "eth0", renewer.lastIface) +} + +// fakeRenewerBackend is a file backend that can also reconfigure the running +// system, satisfying both ifaceBackend and dhcpRenewer. +type fakeRenewerBackend struct { + fakeIfaceBackend + renewCalls int + lastIface string + renewErr error +} + +func (f *fakeRenewerBackend) renewDHCP(_ context.Context, iface string) error { + f.renewCalls++ + f.lastIface = iface + return f.renewErr +} diff --git a/dns_backends_test.go b/dns_backends_test.go index 11236d9..40a5276 100644 --- a/dns_backends_test.go +++ b/dns_backends_test.go @@ -6,6 +6,9 @@ import ( "os" "path/filepath" "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) // readIfcfg parses a written ifcfg- file back into a key/value map so @@ -13,9 +16,7 @@ import ( func readIfcfg(t *testing.T, dir, iface string) map[string]string { t.Helper() data, err := os.ReadFile(filepath.Join(dir, "ifcfg-"+iface)) - if err != nil { - t.Fatalf("read ifcfg-%s: %v", iface, err) - } + require.NoError(t, err) out := make(map[string]string) for _, line := range splitLines(string(data)) { k, v, ok := cutKV(line) @@ -68,32 +69,21 @@ func TestNetworkScriptsDNSPeerDNS(t *testing.T) { err := ns.SetIfaceDNS(context.Background(), "eth0", []net.IP{net.ParseIP("1.1.1.1"), net.ParseIP("8.8.8.8")}, []string{"example.com"}) - if err != nil { - t.Fatalf("SetIfaceDNS: %v", err) - } + require.NoError(t, err) cfg := readIfcfg(t, dir, "eth0") - if cfg["PEERDNS"] != "no" { - t.Errorf("PEERDNS = %q, want \"no\"", cfg["PEERDNS"]) - } - if cfg["DNS1"] != "1.1.1.1" || cfg["DNS2"] != "8.8.8.8" { - t.Errorf("DNS1/DNS2 = %q/%q, want 1.1.1.1/8.8.8.8", cfg["DNS1"], cfg["DNS2"]) - } - if cfg["DOMAIN"] != "example.com" { - t.Errorf("DOMAIN = %q, want example.com", cfg["DOMAIN"]) - } + assert.Equal(t, "no", cfg["PEERDNS"]) + assert.Equal(t, "1.1.1.1", cfg["DNS1"]) + assert.Equal(t, "8.8.8.8", cfg["DNS2"]) + assert.Equal(t, "example.com", cfg["DOMAIN"]) // Clearing DNS should drop PEERDNS so DHCP-provided resolvers resume. err = ns.SetIfaceDNS(context.Background(), "eth0", nil, nil) - if err != nil { - t.Fatalf("SetIfaceDNS clear: %v", err) - } + require.NoError(t, err) cfg = readIfcfg(t, dir, "eth0") - if _, ok := cfg["PEERDNS"]; ok { - t.Errorf("PEERDNS still present after clear: %q", cfg["PEERDNS"]) - } - if _, ok := cfg["DNS1"]; ok { - t.Errorf("DNS1 still present after clear: %q", cfg["DNS1"]) - } + _, ok := cfg["PEERDNS"] + assert.False(t, ok) + _, ok = cfg["DNS1"] + assert.False(t, ok) } // TestIfUpDownDNSEnsuresInterface verifies that SetIfaceDNS creates the @@ -102,30 +92,21 @@ func TestNetworkScriptsDNSPeerDNS(t *testing.T) { func TestIfUpDownDNSEnsuresInterface(t *testing.T) { dir := t.TempDir() cfgPath := filepath.Join(dir, "interfaces") - if err := os.WriteFile(cfgPath, []byte("auto lo\niface lo inet loopback\n"), 0644); err != nil { - t.Fatal(err) - } + err := os.WriteFile(cfgPath, []byte("auto lo\niface lo inet loopback\n"), 0644) + require.NoError(t, err) i := &ifUpDown{BaseConfig: cfgPath, BackupDir: cfgPath + ".backup"} - if err := i.ReadFile(cfgPath); err != nil { - t.Fatal(err) - } + require.NoError(t, i.ReadFile(cfgPath)) // eth0 is not present in the file; SetIfaceDNS must create it. - err := i.SetIfaceDNS(context.Background(), "eth0", + err = i.SetIfaceDNS(context.Background(), "eth0", []net.IP{net.ParseIP("1.1.1.1")}, []string{"example.com"}) - if err != nil { - t.Fatalf("SetIfaceDNS: %v", err) - } + require.NoError(t, err) // Re-read from disk and confirm the stanzas were persisted. i.Interfaces = nil - if err := i.ReadFile(cfgPath); err != nil { - t.Fatal(err) - } + require.NoError(t, i.ReadFile(cfgPath)) interfaces, err := i.GetInterfaces() - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) var found *Interface for _, iface := range interfaces { if iface.Name == "eth0" { @@ -133,13 +114,8 @@ func TestIfUpDownDNSEnsuresInterface(t *testing.T) { break } } - if found == nil { - t.Fatal("eth0 was not persisted; SetIfaceDNS silently no-opped") - } - if len(found.DNS) != 1 || !found.DNS[0].Equal(net.ParseIP("1.1.1.1")) { - t.Errorf("DNS = %v, want [1.1.1.1]", found.DNS) - } - if len(found.SearchDomains) != 1 || found.SearchDomains[0] != "example.com" { - t.Errorf("SearchDomains = %v, want [example.com]", found.SearchDomains) - } + require.NotNil(t, found, "eth0 was not persisted; SetIfaceDNS silently no-opped") + require.Len(t, found.DNS, 1) + assert.True(t, found.DNS[0].Equal(net.ParseIP("1.1.1.1")), "DNS = %v, want [1.1.1.1]", found.DNS) + assert.Equal(t, []string{"example.com"}, found.SearchDomains) } diff --git a/go.mod b/go.mod index f422a9b..c423ba3 100644 --- a/go.mod +++ b/go.mod @@ -12,6 +12,7 @@ require ( github.com/kylelemons/godebug v1.1.0 github.com/prometheus-community/pro-bing v0.7.0 github.com/sirupsen/logrus v1.9.3 + github.com/stretchr/testify v1.10.0 github.com/vishvananda/netlink v1.3.1 github.com/vishvananda/netns v0.0.5 golang.org/x/sys v0.37.0 @@ -21,8 +22,9 @@ require ( ) require ( + github.com/davecgh/go-spew v1.1.1 // indirect github.com/google/uuid v1.6.0 // indirect - github.com/stretchr/testify v1.10.0 // indirect + github.com/pmezard/go-difflib v1.0.0 // indirect golang.org/x/net v0.46.0 // indirect golang.org/x/sync v0.17.0 // indirect ) diff --git a/ifupdown.go b/ifupdown.go index 7f622ce..ed2a5d1 100644 --- a/ifupdown.go +++ b/ifupdown.go @@ -38,6 +38,96 @@ type ifUpDown struct { backupRetention int } +// stanzaFamily reports the address family this interface's stanza configures. +// "iface eth0 inet dhcp" configures IPv4, "iface eth0 inet6 static" IPv6. An +// interface this backend has not seen has no stanza and reports "". +func (iface *ifInterface) stanzaFamily() string { + fields := strings.Fields(iface.Mode) + if len(fields) == 0 { + return "" + } + switch fields[0] { + case "inet", "inet6": + return fields[0] + } + return "" +} + +// dhcpState reports whether each family's DHCP client is enabled. A stanza only +// ever configures one family, so at most one of these is ever true. +func (iface *ifInterface) dhcpState() (dhcp4, dhcp6 bool) { + if !iface.isDHCP { + return false, false + } + family := iface.stanzaFamily() + return family == "inet", family == "inet6" +} + +// setDHCP switches the stanza's method between "dhcp" and "static" for the +// family the stanza configures. +// +// Unlike netplan or networkd, an ifupdown interface is a set of stanzas and +// each names exactly one family, so this backend cannot express "DHCPv4 on, +// DHCPv6 on" from the single stanza it tracks per interface. Asking it to +// enable DHCP for the family its stanza does not configure is therefore an +// error rather than a silent no-op — the caller would otherwise be told the +// request succeeded while nothing was written. Disabling that other family is +// already true of a stanza that never configured it, so it is accepted. +func (iface *ifInterface) setDHCP(dhcp4, dhcp6 bool) error { + if iface.isPPP { + return fmt.Errorf("cannot change DHCP on PPP interface") + } + + // An interface with no stanza yet gets one for the family being enabled. + family := iface.stanzaFamily() + if family == "" { + switch { + case dhcp4 && dhcp6: + return fmt.Errorf("ifupdown cannot enable DHCP for both families on one stanza") + case dhcp4: + iface.Mode = "inet dhcp" + iface.isDHCP = true + case dhcp6: + iface.Mode = "inet6 dhcp" + iface.isDHCP = true + default: + iface.Mode = "inet static" + iface.isDHCP = false + } + return nil + } + + // Split the request into the family this stanza speaks for and the one it + // does not. Only the former can be honored. + enable, other := dhcp4, dhcp6 + if family == "inet6" { + enable, other = dhcp6, dhcp4 + } + if other { + return fmt.Errorf("ifupdown cannot enable DHCP for the other family on an %q stanza for %s", family, iface.Name) + } + + fields := strings.Fields(iface.Mode) + method := "" + if len(fields) > 1 { + method = fields[1] + } + switch method { + case "loopback", "ppp", "tunnel", "wvdial": + return fmt.Errorf("cannot change DHCP on %q interface %s", method, iface.Name) + } + + switch { + case enable && method != "dhcp": + iface.Mode = family + " dhcp" + iface.isDHCP = true + case !enable && method == "dhcp": + iface.Mode = family + " static" + iface.isDHCP = false + } + return nil +} + // Either retreives an existing interface, or makes a new one. func (i *ifUpDown) EnsureInterface(name string) *ifInterface { // Find existing interface and return it. @@ -637,6 +727,7 @@ func (i *ifUpDown) GetInterfaces() (interfaces []*Interface, err error) { for _, iface := range i.Interfaces { i := new(Interface) i.Name = iface.Name + i.DHCP4, i.DHCP6 = iface.dhcpState() // Discover the interface name and mac address. for _, link := range links { @@ -983,6 +1074,41 @@ func (iface *ifInterface) Save(backupDir string, backupRetention int) error { return nil } +// Set the DHCP client state on an interface. +func (i *ifUpDown) SetIfaceDHCP(_ context.Context, iface string, dhcp4, dhcp6 bool) (err error) { + // Find or create the interface. A backend must not silently no-op just + // because it has not seen this interface before (e.g. a freshly attached + // NIC), otherwise the caller gets a success with no persisted change. + ifc := i.EnsureInterface(iface) + + // Switching to DHCP hands addressing to the lease, so the static addresses + // and gateways the stanza carried are dropped rather than written back + // under a "dhcp" method that has no address option to hold them. + dhcp := dhcp4 || dhcp6 + if err = ifc.setDHCP(dhcp4, dhcp6); err != nil { + return err + } + if dhcp { + ifc.Addresses = nil + ifc.Gateway4 = nil + ifc.Gateway6 = nil + } + + // Save the interface config. + return ifc.Save(i.BackupDir, i.backupRetention) +} + +// renewDHCP restarts the interface through ifupdown so the "dhcp" method that +// was just written to its stanza is acted on and a lease is acquired. ifup on +// an already-up interface is a no-op, so it is taken down first. +func (i *ifUpDown) renewDHCP(ctx context.Context, iface string) error { + // ifdown on an interface that is already down exits non-zero, which is not + // a failure of this call — down is the state ifup wants it in. + _, _ = runCommand(ctx, "ifdown", iface) + _, err := runCommand(ctx, "ifup", iface) + return err +} + // Set addresses to interface. func (i *ifUpDown) SetIfaceAddresses(_ context.Context, iface string, addrs []*net.IPNet, gateway4, gateway6 net.IP) (err error) { // Find the interface. @@ -991,7 +1117,10 @@ func (i *ifUpDown) SetIfaceAddresses(_ context.Context, iface string, addrs []*n continue } - // Ensure we can adjust addresses. + // Ensure we can adjust addresses. An ifupdown "dhcp" stanza takes no + // address option, so unlike netplan or networkd this backend cannot + // carry a static address alongside a lease on the same stanza. Call + // SetIfaceDHCP to convert the stanza to static first. if ifc.isDHCP { return fmt.Errorf("cannot change addresses on DHCP interface") } else if ifc.isPPP { diff --git a/ifupdown_test.go b/ifupdown_test.go index ded374e..33b64c4 100644 --- a/ifupdown_test.go +++ b/ifupdown_test.go @@ -6,46 +6,36 @@ import ( "os" "path/filepath" "testing" + + "github.com/stretchr/testify/require" ) // Validate the netplan configuration parser/writer functions. func TestIfUpDown(t *testing.T) { // Setup test file. tmpDir, err := os.MkdirTemp("", "") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) configPath := filepath.Join(tmpDir, "interfaces") testDir, err := filepath.Abs("./tests/ifupdown") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) resultsDir := filepath.Join(testDir, "results") err = fileCopy(filepath.Join(testDir, "interfaces"), configPath) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Setup ifupdown and parse test file. i := new(ifUpDown) i.BaseConfig = configPath i.BackupDir = configPath + ".backup" err = i.ReadFile(i.BaseConfig) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Get the interfaces state. interfaces, err := i.GetInterfaces() - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Verify interfaces read from file. err = testVerifyInterfaces(interfaces, resultsDir, 1) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Test setting the IP addresses on an interface. err = i.SetIfaceAddresses(context.Background(), "test_eth0", []*net.IPNet{ @@ -62,9 +52,7 @@ func TestIfUpDown(t *testing.T) { Mask: net.CIDRMask(64, 128), }, }, net.ParseIP("203.0.113.6"), net.ParseIP("fc00::1")) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Test setting routes on an interface. err = i.SetIfaceRoutes(context.Background(), "test_eth1", []*Route{ @@ -85,34 +73,24 @@ func TestIfUpDown(t *testing.T) { Metric: 100, }, }) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Verify we can read the modified files. i.Interfaces = nil err = i.ReadFile(i.BaseConfig) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Get the interfaces state. interfaces, err = i.GetInterfaces() - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Verify interfaces read from file. err = testVerifyInterfaces(interfaces, resultsDir, 2) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Read the current file and expected state. err = testVerifyResults(resultsDir, tmpDir, 1) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Test setting the IP addresses on an interface. err = i.SetIfaceAddresses(context.Background(), "test_backend", []*net.IPNet{ @@ -121,44 +99,30 @@ func TestIfUpDown(t *testing.T) { Mask: net.CIDRMask(24, 32), }, }, net.ParseIP("1.2.10.254"), nil) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Test setting routes on an interface. err = i.SetIfaceRoutes(context.Background(), "test_eth0", []*Route{}) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Verify we can read the modified files. i.Interfaces = nil err = i.ReadFile(i.BaseConfig) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Get the interfaces state. interfaces, err = i.GetInterfaces() - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Verify interfaces read from file. err = testVerifyInterfaces(interfaces, resultsDir, 3) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Read the current file and expected state. err = testVerifyResults(resultsDir, tmpDir, 2) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Cleanup. err = os.RemoveAll(tmpDir) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) } diff --git a/netplan.go b/netplan.go index f3448a5..4e6249b 100644 --- a/netplan.go +++ b/netplan.go @@ -94,6 +94,32 @@ type npInterface struct { OVSParameters *map[string]interface{} `yaml:"openvswitch,omitempty"` } +// dhcpState reports whether each family's DHCP client is enabled. An absent +// key means netplan's default, which is off. +func (c *npInterface) dhcpState() (dhcp4, dhcp6 bool) { + return c.DHCP4 != nil && *c.DHCP4, c.DHCP6 != nil && *c.DHCP6 +} + +// setDHCP records the requested DHCP client state for both families. Disabling +// a family drops its dhcp4-overrides/dhcp6-overrides, which netplan only reads +// while that client runs. A family being disabled that was already absent is +// left absent rather than written out as false, so reading and rewriting a +// config that never mentioned DHCP does not churn it. +func (c *npInterface) setDHCP(dhcp4, dhcp6 bool) { + if dhcp4 || c.DHCP4 != nil { + c.DHCP4 = boolPtr(dhcp4) + } + if !dhcp4 { + c.DHCP4Overrides = nil + } + if dhcp6 || c.DHCP6 != nil { + c.DHCP6 = boolPtr(dhcp6) + } + if !dhcp6 { + c.DHCP6Overrides = nil + } +} + // Physical ethernet configurations. type npEthernet struct { npPhysical `yaml:",inline"` @@ -198,10 +224,12 @@ type netplan struct { backupRetention int } -// Verify netplan exists, and try parsing its configurations. -func newNetplan(backupRetention int) (np *netplan, err error) { - // Calling netplan info will verify netplan is on the machine. - _, err = runCommand(context.Background(), "netplan", "info") +// Verify netplan exists, and try parsing its configurations. The `netplan info` +// probe is bound to ctx so a wedged netplan cannot stall construction. +func newNetplan(ctx context.Context, backupRetention int) (np *netplan, err error) { + // Calling netplan info will verify netplan is on the machine and working, + // not merely installed. + _, err = runCommand(ctx, "netplan", "info") if err != nil { return } @@ -408,6 +436,7 @@ func (*netplan) ConvertInterface(name string, config npInterface, links []netlin i.Name = name i.MAC = mac i.Link = foundLink + i.DHCP4, i.DHCP6 = config.dhcpState() // Parse addresses. for _, addr := range config.Addresses { @@ -808,6 +837,79 @@ func (np *netplan) SetIfaceAddresses(_ context.Context, iface string, addrs []*n return } +// Set the DHCP client state on an interface. +func (np *netplan) SetIfaceDHCP(_ context.Context, iface string, dhcp4, dhcp6 bool) (err error) { + // Update each config. + for _, c := range np.configs { + if c.empty { + continue + } + + // Check ethernet interfaces. + for name, config := range c.Network.Ethernets { + // If the name is being changed in the config, use that as the name. + ifName := name + if config.SetName != "" { + ifName = config.SetName + } + if ifName != iface { + continue + } + c.dirty = true + config.setDHCP(dhcp4, dhcp6) + c.Network.Ethernets[name] = config + } + + // Check Wifi interfaces. + for name, config := range c.Network.Wifis { + ifName := name + if config.SetName != "" { + ifName = config.SetName + } + if ifName != iface { + continue + } + c.dirty = true + config.setDHCP(dhcp4, dhcp6) + c.Network.Wifis[name] = config + } + + // Check bridges. + if config, ok := c.Network.Bridges[iface]; ok { + c.dirty = true + config.setDHCP(dhcp4, dhcp6) + c.Network.Bridges[iface] = config + } + + // Check bonds. + if config, ok := c.Network.Bonds[iface]; ok { + c.dirty = true + config.setDHCP(dhcp4, dhcp6) + c.Network.Bonds[iface] = config + } + + // Check VLAN interfaces. + if config, ok := c.Network.VLANs[iface]; ok { + c.dirty = true + config.setDHCP(dhcp4, dhcp6) + c.Network.VLANs[iface] = config + } + } + + // Try to save changes. + err = np.Save() + return +} + +// renewDHCP makes netplan realize the configuration that was just written, so a +// newly enabled DHCP client starts and acquires a lease. `netplan apply` is the +// only supported way in; it reconciles every interface, not just this one, and +// so is a no-op for the interfaces whose configuration did not change. +func (np *netplan) renewDHCP(ctx context.Context, _ string) error { + _, err := runCommand(ctx, "netplan", "apply") + return err +} + // Set static routes to interface. func (np *netplan) SetIfaceRoutes(_ context.Context, iface string, routes []*Route) (err error) { // Update each config. diff --git a/netplan_test.go b/netplan_test.go index 2aeccf0..19031c7 100644 --- a/netplan_test.go +++ b/netplan_test.go @@ -6,47 +6,36 @@ import ( "os" "path/filepath" "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) // Validate the netplan configuration parser/writer functions. func TestNetplan(t *testing.T) { // Setup test file. tmpDir, err := os.MkdirTemp("", "") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) configPath := filepath.Join(tmpDir, "netplan.yaml") testDir, err := filepath.Abs("./tests/netplan") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) resultsDir := filepath.Join(testDir, "results") err = fileCopy(filepath.Join(testDir, "netplan.yaml"), configPath) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) err = fileCopy(filepath.Join(testDir, "netplan2.yaml"), filepath.Join(tmpDir, "netplan2.yaml")) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Setup ifupdown and parse test file. np, err := readNetplanConfigDirectory(tmpDir) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Get the interfaces state. interfaces, err := np.GetInterfaces() - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Verify interfaces read from file. err = testVerifyInterfaces(interfaces, resultsDir, 1) - if err != nil { - t.Error(err) - } + assert.NoError(t, err) // Test setting the IP addresses on an interface. err = np.SetIfaceAddresses(context.Background(), "test_eth0.1556", []*net.IPNet{ @@ -63,9 +52,7 @@ func TestNetplan(t *testing.T) { Mask: net.CIDRMask(64, 128), }, }, net.ParseIP("1.2.3.1"), net.ParseIP("fc00::1")) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Test setting routes on an interface. err = np.SetIfaceRoutes(context.Background(), "test_eth3", []*Route{ @@ -86,27 +73,19 @@ func TestNetplan(t *testing.T) { Metric: 100, }, }) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Get the interfaces state. interfaces, err = np.GetInterfaces() - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Verify interfaces read from file. err = testVerifyInterfaces(interfaces, resultsDir, 2) - if err != nil { - t.Error(err) - } + assert.NoError(t, err) // Read the current file and expected state. err = testVerifyResults(resultsDir, tmpDir, 1) - if err != nil { - t.Error(err) - } + assert.NoError(t, err) // Test setting the IP addresses on an interface. err = np.SetIfaceAddresses(context.Background(), "test_eth0", []*net.IPNet{ @@ -115,37 +94,25 @@ func TestNetplan(t *testing.T) { Mask: net.CIDRMask(24, 32), }, }, net.ParseIP("1.2.10.254"), nil) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Test setting routes on an interface. err = np.SetIfaceRoutes(context.Background(), "test_eth0.1556", []*Route{}) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Get the interfaces state. interfaces, err = np.GetInterfaces() - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Verify interfaces read from file. err = testVerifyInterfaces(interfaces, resultsDir, 3) - if err != nil { - t.Error(err) - } + assert.NoError(t, err) // Read the current file and expected state. err = testVerifyResults(resultsDir, tmpDir, 2) - if err != nil { - t.Error(err) - } + assert.NoError(t, err) // Cleanup. err = os.RemoveAll(tmpDir) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) } diff --git a/networkConfigurator.go b/networkConfigurator.go index 46625da..39f47d5 100644 --- a/networkConfigurator.go +++ b/networkConfigurator.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "net" + "sort" "strings" ) @@ -24,13 +25,32 @@ func (i *Route) String() string { } type Interface struct { - Name string - MAC net.HardwareAddr - Addresses []*net.IPNet - Gateway4 net.IP - Gateway6 net.IP - Routes []*Route - DNS []net.IP + Name string + MAC net.HardwareAddr + Addresses []*net.IPNet + Gateway4 net.IP + Gateway6 net.IP + Routes []*Route + DNS []net.IP + + // Up reports whether the interface can carry traffic right now, and + // Physical whether it is backed by a network device rather than created by + // the kernel or the hypervisor host — a bridge, bond, VLAN, tunnel, or the + // veth pair of a container is not physical, a virtio or vmxnet NIC is. + // Both describe the running system and are only set by GetInterfaces; the + // Interface values the configuration backends read back leave them false. + Up bool + Physical bool + + // DHCP4 and DHCP6 report whether the interface runs a DHCP client for that + // address family. They describe the persisted configuration, not the + // running system: the kernel cannot be asked whether an address arrived + // from a lease, so these are read back from the configuration backends the + // same way DNS is. An interface can run a DHCP client and still carry + // static addresses of the same family. + DHCP4 bool + DHCP6 bool + SearchDomains []string Link any } @@ -59,15 +79,43 @@ type Configurator interface { AddRoute(ctx context.Context, iface string, dst *net.IPNet, gateway net.IP, metric int) error RemoveRoute(ctx context.Context, iface string, dst *net.IPNet, gateway net.IP) error SetDNS(ctx context.Context, iface string, servers []net.IP, searchDomains []string) error + + // SetDHCP turns each address family's DHCP client on or off. Both families + // are stated explicitly, so a caller moving an interface to DHCPv4 while + // keeping a static IPv6 address passes (true, false). + // + // Adding a static address does not imply disabling DHCP: every backend + // except ifupdown can carry static addresses alongside a lease, and which + // of the two the operator wants is not something AddAddress can infer. + // SetDHCP is how that choice is made. + // + // Enabling a family also asks the running system to acquire a lease now, + // rather than at the next reboot. Disabling one only rewrites the + // configuration: an interface's existing lease is left in place until it + // expires or the network is reconfigured, so the call cannot strand a + // caller that is connected over the leased address. + SetDHCP(ctx context.Context, iface string, dhcp4, dhcp6 bool) error } -// ifaceBackend persists interface address, route, and DNS changes to an on-disk -// network configuration backend such as netplan, cloud-init, networkd, +// ifaceBackend persists interface address, route, DNS, and DHCP changes to an +// on-disk network configuration backend such as netplan, cloud-init, networkd, // NetworkManager, RHEL network-scripts, or ifupdown. type ifaceBackend interface { SetIfaceAddresses(ctx context.Context, iface string, addrs []*net.IPNet, gateway4, gateway6 net.IP) error SetIfaceRoutes(ctx context.Context, iface string, routes []*Route) error SetIfaceDNS(ctx context.Context, iface string, servers []net.IP, searchDomains []string) error + SetIfaceDHCP(ctx context.Context, iface string, dhcp4, dhcp6 bool) error +} + +// dhcpRenewer is an optional capability of a file backend: telling the running +// system to pick up a DHCP client that was just enabled in the configuration, +// so the interface acquires a lease without waiting for a reboot. Each backend +// implements it with its own manager's reconfigure command, which is both the +// least disruptive way to do it and the only one that will not fight the +// manager that owns the interface. cloud-init has no such command — it only +// runs at boot — and so does not implement this. +type dhcpRenewer interface { + renewDHCP(ctx context.Context, iface string) error } // panelBackend persists IP changes to a hosting control panel such as @@ -119,6 +167,12 @@ type ifaceDNSReader interface { GetInterfaces() ([]*Interface, error) } +// boolPtr returns a pointer to v, for the optional booleans in the netplan and +// cloud-init schemas where a nil pointer means "key absent" rather than false. +func boolPtr(v bool) *bool { + return &v +} + // ipIsIn reports whether ip is already present in addrs. func ipIsIn(addrs []net.IP, ip net.IP) bool { for _, a := range addrs { @@ -139,14 +193,18 @@ func stringInSlice(list []string, s string) bool { return false } -// mergeDNSFromBackends reads the DNS servers and search domains each backend -// has persisted and merges them into the matching runtime Interface (matched by -// name). The kernel/netlink layer does not track per-interface DNS, so the -// backends are the source of truth for these fields. Entries are de-duplicated -// so a host running several backends (e.g. netplan and cloud-init) does not -// list each resolver more than once. Per-backend errors are logged and do not -// abort the merge. -func mergeDNSFromBackends(backends []namedIfaceBackend, ifaces []*Interface) { +// mergeBackendState reads the DNS servers, search domains, and DHCP client +// state each backend has persisted and merges them into the matching runtime +// Interface (matched by name). The kernel/netlink layer tracks none of these — +// it cannot say which resolver an interface uses, nor whether an address came +// from a lease — so the backends are the source of truth for these fields. +// +// DNS entries are de-duplicated so a host running several backends (e.g. +// netplan and cloud-init) does not list each resolver more than once. The DHCP +// flags are OR'd for the same reason a lease is a property of the interface and +// not of the file describing it: if any backend has the client enabled, the +// interface runs one. Per-backend errors are logged and do not abort the merge. +func mergeBackendState(backends []namedIfaceBackend, ifaces []*Interface) { byName := make(map[string]*Interface, len(ifaces)) for _, i := range ifaces { byName[i.Name] = i @@ -166,6 +224,8 @@ func mergeDNSFromBackends(backends []namedIfaceBackend, ifaces []*Interface) { if target == nil { continue } + target.DHCP4 = target.DHCP4 || bi.DHCP4 + target.DHCP6 = target.DHCP6 || bi.DHCP6 for _, ip := range bi.DNS { if ip == nil || ipIsIn(target.DNS, ip) { continue @@ -182,6 +242,43 @@ func mergeDNSFromBackends(backends []namedIfaceBackend, ifaces []*Interface) { } } +// applyIfaceDHCP pushes the interface's DHCP client state to every registered +// file backend. Unlike the other apply helpers this one reports whether any +// backend accepted the change: a caller that is about to ask the running system +// for a lease needs to know that at least one configuration file now asks for +// one, otherwise the lease would be acquired and then lost on the next reboot. +// Individual failures are logged and do not abort the others, since each +// backend writes an independent configuration file — and ifupdown in particular +// rejects requests its one-stanza-per-family model cannot express. +func applyIfaceDHCP(ctx context.Context, backends []namedIfaceBackend, iface string, dhcp4, dhcp6 bool) (applied bool) { + for _, b := range backends { + if err := b.backend.SetIfaceDHCP(ctx, iface, dhcp4, dhcp6); err != nil { + logger.Printf("%s error: %v", b.name, err) + continue + } + applied = true + } + return applied +} + +// renewDHCPOnBackends asks every backend that can reconfigure the running +// system to do so, so an interface whose DHCP client was just enabled acquires +// a lease now instead of at the next reboot. Backends that cannot — cloud-init, +// which only runs at boot — are skipped. Errors are logged rather than +// returned: the configuration has already been written, and a manager that +// declined to reconfigure has not undone that. +func renewDHCPOnBackends(ctx context.Context, backends []namedIfaceBackend, iface string) { + for _, b := range backends { + renewer, ok := b.backend.(dhcpRenewer) + if !ok { + continue + } + if err := renewer.renewDHCP(ctx, iface); err != nil { + logger.Printf("%s: error renewing DHCP on %s: %v", b.name, iface, err) + } + } +} + // applyIfaceDNS pushes the interface's DNS servers and search domains to every // registered file backend, logging but not aborting on individual errors. func applyIfaceDNS(ctx context.Context, backends []namedIfaceBackend, iface string, servers []net.IP, searchDomains []string) { @@ -243,6 +340,95 @@ func reorderPrimaryAddress(addrs []*net.IPNet, target *net.IPNet) ([]*net.IPNet, return append([]*net.IPNet{target}, reordered...), true } +// Interface name ranks, ordered by how likely a NIC of that kind is to be the +// one carrying a host's internet traffic. +const ( + ifaceRankWired = iota + ifaceRankWireless + ifaceRankOther +) + +// wiredNamePrefixes and wirelessNamePrefixes match the kernel's classic (eth0, +// wlan0) and predictable (eno1, ens3, enp1s0, enx.., em1, wlp2s0) names as well +// as the friendly names Windows reports ("Ethernet 2", "Wi-Fi"). Compared +// against a lower-cased name. +var ( + wiredNamePrefixes = []string{"eth", "en", "em"} + wirelessNamePrefixes = []string{"wl", "wifi", "wi-fi"} +) + +// FindPhysicalInterfaces returns the host's physical interfaces — leaving out +// the bridges, bonds, VLANs, tunnels, and container veth pairs that should +// never be handed a public address — ordered so the interface most likely to be +// the one a caller wants to configure for internet access comes first. +// +// Interfaces that are up sort ahead of those that are down, then those already +// carrying a default gateway (IPv4 ahead of IPv6-only), then wired ahead of +// wireless, and finally by name read the way a human reads it, so eth0 comes +// before eth1 and eth2 before eth10. +// +// This is a ranking rather than a decision: a caller looking for an interface +// that meets some further requirement — one not already holding a public +// address, say — filters the returned slice and takes the first survivor. +func FindPhysicalInterfaces(ifaces []*Interface) []*Interface { + physical := make([]*Interface, 0, len(ifaces)) + for _, iface := range ifaces { + if iface.Physical { + physical = append(physical, iface) + } + } + sort.SliceStable(physical, func(i, j int) bool { + return preferInterface(physical[i], physical[j]) + }) + return physical +} + +// preferInterface reports whether a should be offered ahead of b as the +// interface to configure for internet access. +func preferInterface(a, b *Interface) bool { + if a.Up != b.Up { + return a.Up + } + if ag, bg := gatewayRank(a), gatewayRank(b); ag != bg { + return ag < bg + } + if an, bn := ifaceNameRank(a.Name), ifaceNameRank(b.Name); an != bn { + return an < bn + } + return naturalLess(a.Name, b.Name) +} + +// gatewayRank orders an interface by the default gateway it already carries. +// An interface the host currently reaches the internet over is the surest guess +// at the one it should keep reaching the internet over. +func gatewayRank(iface *Interface) int { + switch { + case iface.Gateway4 != nil: + return 0 + case iface.Gateway6 != nil: + return 1 + } + return 2 +} + +// ifaceNameRank guesses an interface's kind from its name. The name is all +// there is to go on: neither netlink nor the Windows IP Helper API reports +// whether a NIC is wired or wireless in a way that survives both platforms. +func ifaceNameRank(name string) int { + lower := strings.ToLower(name) + for _, p := range wirelessNamePrefixes { + if strings.HasPrefix(lower, p) { + return ifaceRankWireless + } + } + for _, p := range wiredNamePrefixes { + if strings.HasPrefix(lower, p) { + return ifaceRankWired + } + } + return ifaceRankOther +} + // Take a name and a list of interfaces and finds an interface by its name. func FindInterfaceByName(name string, ifaces []*Interface) *Interface { switch name { diff --git a/networkConfigurator_test.go b/networkConfigurator_test.go index 5531778..b230dcb 100644 --- a/networkConfigurator_test.go +++ b/networkConfigurator_test.go @@ -5,14 +5,15 @@ import ( "errors" "net" "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func mustCIDR(t *testing.T, s string) *net.IPNet { t.Helper() _, n, err := net.ParseCIDR(s) - if err != nil { - t.Fatalf("ParseCIDR(%q): %v", s, err) - } + require.NoError(t, err) return n } @@ -22,9 +23,7 @@ func TestRouteString(t *testing.T) { Gateway: net.ParseIP("10.0.0.1"), Metric: 100, } - if got, want := r.String(), "10.0.0.0/24 via 10.0.0.1 metric 100"; got != want { - t.Errorf("Route.String() = %q, want %q", got, want) - } + assert.Equal(t, "10.0.0.0/24 via 10.0.0.1 metric 100", r.String()) } func TestInterfaceString(t *testing.T) { @@ -38,11 +37,8 @@ func TestInterfaceString(t *testing.T) { {Destination: mustCIDR(t, "10.0.0.0/24"), Gateway: net.ParseIP("10.0.0.1"), Metric: 5}, }, } - got := iface.String() want := "Name: eth0 MAC: 00:11:22:33:44:55 Addresses: [192.168.1.0/24] Gateway4: 192.168.1.1 Gateway6: Routes: [10.0.0.0/24 via 10.0.0.1 metric 5]" - if got != want { - t.Errorf("Interface.String()\n got %q\nwant %q", got, want) - } + assert.Equal(t, want, iface.String()) } func TestFindInterfaceByName(t *testing.T) { @@ -52,39 +48,104 @@ func TestFindInterfaceByName(t *testing.T) { ifaces := []*Interface{eth1, eth0, pub6} t.Run("by name", func(t *testing.T) { - if got := FindInterfaceByName("eth1", ifaces); got != eth1 { - t.Errorf("got %v, want eth1", got) - } + assert.Equal(t, eth1, FindInterfaceByName("eth1", ifaces)) }) t.Run("public picks gateway4 interface", func(t *testing.T) { - if got := FindInterfaceByName(Public, ifaces); got != eth0 { - t.Errorf("got %v, want eth0", got) - } + assert.Equal(t, eth0, FindInterfaceByName(Public, ifaces)) }) t.Run("public6 picks gateway6 interface", func(t *testing.T) { - if got := FindInterfaceByName(Public6, ifaces); got != pub6 { - t.Errorf("got %v, want pub6", got) - } + assert.Equal(t, pub6, FindInterfaceByName(Public6, ifaces)) }) t.Run("not found", func(t *testing.T) { - if got := FindInterfaceByName("missing", ifaces); got != nil { - t.Errorf("got %v, want nil", got) - } + assert.Nil(t, FindInterfaceByName("missing", ifaces)) }) t.Run("public with no gateway returns nil", func(t *testing.T) { - if got := FindInterfaceByName(Public, []*Interface{eth1}); got != nil { - t.Errorf("got %v, want nil", got) - } + assert.Nil(t, FindInterfaceByName(Public, []*Interface{eth1})) }) t.Run("public6 with no gateway returns nil", func(t *testing.T) { - if got := FindInterfaceByName(Public6, []*Interface{eth1}); got != nil { - t.Errorf("got %v, want nil", got) + assert.Nil(t, FindInterfaceByName(Public6, []*Interface{eth1})) + }) +} + +// names extracts the interface names of a result slice, for comparing an +// expected ordering in one assertion. +func names(ifaces []*Interface) []string { + out := make([]string, 0, len(ifaces)) + for _, iface := range ifaces { + out = append(out, iface.Name) + } + return out +} + +func TestFindPhysicalInterfaces(t *testing.T) { + t.Run("drops virtual interfaces", func(t *testing.T) { + ifaces := []*Interface{ + {Name: "docker0", Up: true}, + {Name: "eth0", Up: true, Physical: true}, + {Name: "veth1a2b", Up: true}, } + assert.Equal(t, []string{"eth0"}, names(FindPhysicalInterfaces(ifaces))) + }) + + t.Run("up before down", func(t *testing.T) { + ifaces := []*Interface{ + {Name: "eth0", Physical: true}, + {Name: "eth1", Up: true, Physical: true}, + } + assert.Equal(t, []string{"eth1", "eth0"}, names(FindPhysicalInterfaces(ifaces))) + }) + + t.Run("gateway before none, v4 before v6", func(t *testing.T) { + ifaces := []*Interface{ + {Name: "eth0", Up: true, Physical: true}, + {Name: "eth1", Up: true, Physical: true, Gateway6: net.ParseIP("2001:db8::1")}, + {Name: "eth2", Up: true, Physical: true, Gateway4: net.ParseIP("203.0.113.1")}, + } + assert.Equal(t, []string{"eth2", "eth1", "eth0"}, names(FindPhysicalInterfaces(ifaces))) + }) + + t.Run("up outranks a gateway on a down interface", func(t *testing.T) { + ifaces := []*Interface{ + {Name: "eth0", Physical: true, Gateway4: net.ParseIP("203.0.113.1")}, + {Name: "eth1", Up: true, Physical: true}, + } + assert.Equal(t, []string{"eth1", "eth0"}, names(FindPhysicalInterfaces(ifaces))) + }) + + t.Run("wired before wireless before other", func(t *testing.T) { + ifaces := []*Interface{ + {Name: "ppp0", Up: true, Physical: true}, + {Name: "wlp2s0", Up: true, Physical: true}, + {Name: "enp1s0", Up: true, Physical: true}, + } + assert.Equal(t, []string{"enp1s0", "wlp2s0", "ppp0"}, names(FindPhysicalInterfaces(ifaces))) + }) + + t.Run("names ordered as a human reads them", func(t *testing.T) { + ifaces := []*Interface{ + {Name: "eth10", Up: true, Physical: true}, + {Name: "eth2", Up: true, Physical: true}, + {Name: "eth0", Up: true, Physical: true}, + } + assert.Equal(t, []string{"eth0", "eth2", "eth10"}, names(FindPhysicalInterfaces(ifaces))) + }) + + t.Run("windows friendly names rank as wired and wireless", func(t *testing.T) { + ifaces := []*Interface{ + {Name: "Wi-Fi", Up: true, Physical: true}, + {Name: "Ethernet 2", Up: true, Physical: true}, + } + assert.Equal(t, []string{"Ethernet 2", "Wi-Fi"}, names(FindPhysicalInterfaces(ifaces))) + }) + + t.Run("no physical interfaces returns empty", func(t *testing.T) { + assert.Empty(t, FindPhysicalInterfaces([]*Interface{{Name: "br0", Up: true}})) + assert.Empty(t, FindPhysicalInterfaces(nil)) }) } @@ -97,17 +158,6 @@ func TestReorderPrimaryAddress(t *testing.T) { } return out } - equal := func(a, b []string) bool { - if len(a) != len(b) { - return false - } - for i := range a { - if a[i] != b[i] { - return false - } - } - return true - } // hostIP keeps the host bits so IP.Equal matching is exercised, unlike the // network address returned by mustCIDR. @@ -123,43 +173,27 @@ func TestReorderPrimaryAddress(t *testing.T) { t.Run("middle moves to front", func(t *testing.T) { got, ok := reorderPrimaryAddress([]*net.IPNet{a, b, c}, b) - if !ok { - t.Fatal("ok = false, want true") - } - if want := []string{"192.168.1.11", "192.168.1.10", "192.168.1.12"}; !equal(addrStrings(got), want) { - t.Errorf("got %v, want %v", addrStrings(got), want) - } + require.True(t, ok) + assert.Equal(t, []string{"192.168.1.11", "192.168.1.10", "192.168.1.12"}, addrStrings(got)) }) t.Run("already first is unchanged", func(t *testing.T) { got, ok := reorderPrimaryAddress([]*net.IPNet{a, b, c}, a) - if !ok { - t.Fatal("ok = false, want true") - } - if want := []string{"192.168.1.10", "192.168.1.11", "192.168.1.12"}; !equal(addrStrings(got), want) { - t.Errorf("got %v, want %v", addrStrings(got), want) - } + require.True(t, ok) + assert.Equal(t, []string{"192.168.1.10", "192.168.1.11", "192.168.1.12"}, addrStrings(got)) }) t.Run("v6 target preserves v4 relative order", func(t *testing.T) { got, ok := reorderPrimaryAddress([]*net.IPNet{a, b, v6}, v6) - if !ok { - t.Fatal("ok = false, want true") - } - if want := []string{"2001:db8::5", "192.168.1.10", "192.168.1.11"}; !equal(addrStrings(got), want) { - t.Errorf("got %v, want %v", addrStrings(got), want) - } + require.True(t, ok) + assert.Equal(t, []string{"2001:db8::5", "192.168.1.10", "192.168.1.11"}, addrStrings(got)) }) t.Run("absent target returns false", func(t *testing.T) { got, ok := reorderPrimaryAddress([]*net.IPNet{a, b}, c) - if ok { - t.Error("ok = true, want false") - } + assert.False(t, ok) // The original slice is returned unchanged. - if want := []string{"192.168.1.10", "192.168.1.11"}; !equal(addrStrings(got), want) { - t.Errorf("got %v, want %v", addrStrings(got), want) - } + assert.Equal(t, []string{"192.168.1.10", "192.168.1.11"}, addrStrings(got)) }) } @@ -168,10 +202,19 @@ type fakeIfaceBackend struct { addrCalls int routeCalls int dnsCalls int + dhcpCalls int lastAddrs []*net.IPNet + lastDHCP4 bool + lastDHCP6 bool err error } +func (f *fakeIfaceBackend) SetIfaceDHCP(_ context.Context, iface string, dhcp4, dhcp6 bool) error { + f.dhcpCalls++ + f.lastDHCP4, f.lastDHCP6 = dhcp4, dhcp6 + return f.err +} + func (f *fakeIfaceBackend) SetIfaceAddresses(_ context.Context, iface string, addrs []*net.IPNet, gateway4, gateway6 net.IP) error { f.addrCalls++ f.lastAddrs = addrs @@ -221,9 +264,8 @@ func TestApplyIfaceAddresses(t *testing.T) { {name: "ok", backend: ok}, } applyIfaceAddresses(context.Background(), backends, "eth0", nil, nil, nil) - if failing.addrCalls != 1 || ok.addrCalls != 1 { - t.Errorf("addr calls: failing=%d ok=%d, want 1 each", failing.addrCalls, ok.addrCalls) - } + assert.Equal(t, 1, failing.addrCalls) + assert.Equal(t, 1, ok.addrCalls) } func TestApplyIfaceRoutes(t *testing.T) { @@ -234,9 +276,8 @@ func TestApplyIfaceRoutes(t *testing.T) { {name: "ok", backend: ok}, } applyIfaceRoutes(context.Background(), backends, "eth0", nil) - if failing.routeCalls != 1 || ok.routeCalls != 1 { - t.Errorf("route calls: failing=%d ok=%d, want 1 each", failing.routeCalls, ok.routeCalls) - } + assert.Equal(t, 1, failing.routeCalls) + assert.Equal(t, 1, ok.routeCalls) } func TestReloadPanels(t *testing.T) { @@ -247,9 +288,8 @@ func TestReloadPanels(t *testing.T) { {name: "ok", backend: ok}, } reloadPanels(context.Background(), backends) - if failing.reloadCalls != 1 || ok.reloadCalls != 1 { - t.Errorf("reload calls: failing=%d ok=%d, want 1 each", failing.reloadCalls, ok.reloadCalls) - } + assert.Equal(t, 1, failing.reloadCalls) + assert.Equal(t, 1, ok.reloadCalls) } func TestSetMainIPOnPanels(t *testing.T) { @@ -260,9 +300,8 @@ func TestSetMainIPOnPanels(t *testing.T) { {name: "ok", backend: ok}, } setMainIPOnPanels(context.Background(), backends, net.ParseIP("192.0.2.5")) - if failing.setMainIPCalls != 1 || ok.setMainIPCalls != 1 { - t.Errorf("setMainIP calls: failing=%d ok=%d, want 1 each", failing.setMainIPCalls, ok.setMainIPCalls) - } + assert.Equal(t, 1, failing.setMainIPCalls) + assert.Equal(t, 1, ok.setMainIPCalls) } func TestRemoveIPFromPanels(t *testing.T) { @@ -273,9 +312,8 @@ func TestRemoveIPFromPanels(t *testing.T) { {name: "ok", backend: ok}, } removeIPFromPanels(context.Background(), backends, net.ParseIP("192.0.2.5")) - if failing.removeIPCalls != 1 || ok.removeIPCalls != 1 { - t.Errorf("removeIP calls: failing=%d ok=%d, want 1 each", failing.removeIPCalls, ok.removeIPCalls) - } + assert.Equal(t, 1, failing.removeIPCalls) + assert.Equal(t, 1, ok.removeIPCalls) } // fakeDNSReaderBackend is a file backend that also reports back persisted @@ -292,48 +330,46 @@ func (f *fakeDNSReaderBackend) SetIfaceRoutes(context.Context, string, []*Route) func (f *fakeDNSReaderBackend) SetIfaceDNS(context.Context, string, []net.IP, []string) error { return nil } -func (f *fakeDNSReaderBackend) GetInterfaces() ([]*Interface, error) { return f.ifaces, f.err } +func (f *fakeDNSReaderBackend) SetIfaceDHCP(context.Context, string, bool, bool) error { return nil } +func (f *fakeDNSReaderBackend) GetInterfaces() ([]*Interface, error) { return f.ifaces, f.err } -// mergeDNSFromBackends must union DNS from every backend into the matching -// runtime interface (by name), de-duplicate entries so multi-backend hosts do -// not list a resolver twice, skip backends with no matching interface, and keep -// going when one backend errors. -func TestMergeDNSFromBackends(t *testing.T) { +// mergeBackendState must union DNS from every backend into the matching runtime +// interface (by name), de-duplicate entries so multi-backend hosts do not list a +// resolver twice, OR the DHCP flags so a client enabled in any backend is +// reported, skip backends with no matching interface, and keep going when one +// backend errors. +func TestMergeBackendState(t *testing.T) { runtime := []*Interface{{Name: "eth0"}, {Name: "eth1"}} backends := []namedIfaceBackend{ {name: "broken", backend: &fakeDNSReaderBackend{err: errors.New("boom")}}, {name: "netplan", backend: &fakeDNSReaderBackend{ifaces: []*Interface{ - {Name: "eth0", DNS: []net.IP{net.ParseIP("8.8.8.8"), net.ParseIP("2001:4860:4860::8888")}, SearchDomains: []string{"example.com"}}, + {Name: "eth0", DNS: []net.IP{net.ParseIP("8.8.8.8"), net.ParseIP("2001:4860:4860::8888")}, SearchDomains: []string{"example.com"}, DHCP4: true}, }}}, {name: "cloud-init", backend: &fakeDNSReaderBackend{ifaces: []*Interface{ - // Overlaps netplan (must dedupe) and adds one new entry. - {Name: "eth0", DNS: []net.IP{net.ParseIP("8.8.8.8"), net.ParseIP("1.1.1.1")}, SearchDomains: []string{"example.com", "corp.example"}}, + // Overlaps netplan (must dedupe) and adds one new entry. DHCP4 is + // false here and must not clear the true netplan reported. + {Name: "eth0", DNS: []net.IP{net.ParseIP("8.8.8.8"), net.ParseIP("1.1.1.1")}, SearchDomains: []string{"example.com", "corp.example"}, DHCP6: true}, }}}, {name: "ifupdown", backend: &fakeDNSReaderBackend{ifaces: []*Interface{ {Name: "eth1", DNS: []net.IP{net.ParseIP("9.9.9.9")}, SearchDomains: []string{"a.test"}}, // No runtime match; must be ignored. - {Name: "eth9", DNS: []net.IP{net.ParseIP("10.0.0.1")}}, + {Name: "eth9", DNS: []net.IP{net.ParseIP("10.0.0.1")}, DHCP4: true}, }}}, } - mergeDNSFromBackends(backends, runtime) + mergeBackendState(backends, runtime) eth0 := runtime[0] wantDNS := []net.IP{net.ParseIP("8.8.8.8"), net.ParseIP("2001:4860:4860::8888"), net.ParseIP("1.1.1.1")} - if !equalIPs(eth0.DNS, wantDNS) { - t.Errorf("eth0 DNS = %v, want %v", eth0.DNS, wantDNS) - } + assert.True(t, equalIPs(eth0.DNS, wantDNS)) wantSearch := []string{"example.com", "corp.example"} - if len(eth0.SearchDomains) != len(wantSearch) || eth0.SearchDomains[0] != wantSearch[0] || eth0.SearchDomains[1] != wantSearch[1] { - t.Errorf("eth0 SearchDomains = %v, want %v", eth0.SearchDomains, wantSearch) - } + assert.Equal(t, wantSearch, eth0.SearchDomains) + assert.True(t, eth0.DHCP4, "netplan reported DHCP4; cloud-init's false must not clear it") + assert.True(t, eth0.DHCP6, "cloud-init reported DHCP6") eth1 := runtime[1] - if !equalIPs(eth1.DNS, []net.IP{net.ParseIP("9.9.9.9")}) { - t.Errorf("eth1 DNS = %v, want [9.9.9.9]", eth1.DNS) - } - if len(eth1.SearchDomains) != 1 || eth1.SearchDomains[0] != "a.test" { - t.Errorf("eth1 SearchDomains = %v, want [a.test]", eth1.SearchDomains) - } + assert.True(t, equalIPs(eth1.DNS, []net.IP{net.ParseIP("9.9.9.9")})) + assert.Equal(t, []string{"a.test"}, eth1.SearchDomains) + assert.False(t, eth1.DHCP4, "eth9's DHCP4 must not leak onto eth1") } func equalIPs(a, b []net.IP) bool { diff --git a/networkManager.go b/networkManager.go index 574d3cd..6e64ef3 100644 --- a/networkManager.go +++ b/networkManager.go @@ -6,8 +6,10 @@ import ( "fmt" "net" "strings" + "time" "github.com/Wifx/gonetworkmanager/v3" + "github.com/godbus/dbus/v5" "github.com/vishvananda/netlink" ) @@ -15,6 +17,8 @@ type nmConnection struct { ID string Name string UsingData bool + Method4 string + Method6 string Addresses4 []*net.IPNet Addresses6 []*net.IPNet Gateway4 net.IP @@ -25,6 +29,35 @@ type nmConnection struct { DNSSearch []string } +// dhcpState reports whether each family's DHCP client is enabled. IPv4 "auto" +// means DHCPv4. For IPv6, "auto" means router advertisements plus DHCPv6 when +// the router asks for it, and "dhcp" means DHCPv6 alone; both run a client. +func (c *nmConnection) dhcpState() (dhcp4, dhcp6 bool) { + dhcp4 = c.Method4 == "auto" + dhcp6 = c.Method6 == "auto" || c.Method6 == "dhcp" + return dhcp4, dhcp6 +} + +// nmStaticMethod4 is the ipv4.method to leave behind when DHCPv4 is turned off: +// "manual" when the connection still carries static addresses to serve, and +// "disabled" when turning off the lease leaves it with no IPv4 at all. +func nmStaticMethod4(hasAddrs bool) string { + if hasAddrs { + return "manual" + } + return "disabled" +} + +// nmStaticMethod6 is the ipv6.method counterpart. An IPv6 interface with no +// addresses keeps its link-local one rather than losing IPv6 entirely, which is +// what "disabled" would do. +func nmStaticMethod6(hasAddrs bool) string { + if hasAddrs { + return "manual" + } + return "link-local" +} + type networkManager struct { config gonetworkmanager.Settings } @@ -90,6 +123,11 @@ func (*networkManager) ParseConnection(settings gonetworkmanager.ConnectionSetti } conn.Name = name + // Get the addressing method of each family. These decide whether a DHCP + // client runs, independently of any static addresses parsed below. + conn.Method4, _ = settings["ipv4"]["method"].(string) + conn.Method6, _ = settings["ipv6"]["method"].(string) + // Get the IPv4 address map, and confirm the newer configuration style is used. addrMap, ok := settings["ipv4"]["address-data"] if ok { @@ -259,8 +297,62 @@ func (*networkManager) ParseConnection(settings gonetworkmanager.ConnectionSetti return } -// Verify netplan exists, and try parsing its configurations. -func newNetworkManager() (nm *networkManager, err error) { +// nmBusName is the well-known D-Bus name the NetworkManager daemon takes once +// it is running. Until some process owns it, there is nothing on the bus to +// answer a configuration call. +const nmBusName = "org.freedesktop.NetworkManager" + +// nmBusNameOwned reports whether the NetworkManager daemon currently owns its +// bus name — the "is the socket live" question. It is asked before any property +// is read, because NetworkManager is D-Bus activatable: reading a property on +// an unowned name asks the bus to *start* the daemon. Waiting for a service to +// come up on its own must not be the thing that launches it. +func nmBusNameOwned(conn *dbus.Conn) (bool, error) { + var owned bool + err := conn.BusObject().Call("org.freedesktop.DBus.NameHasOwner", 0, nmBusName).Store(&owned) + return owned, err +} + +// networkManagerReady reports whether NetworkManager is on the bus and has +// finished starting up. The two are distinct: the daemon takes its bus name +// early, then spends a while bringing up the connections it is configured to +// activate at boot. Its Startup property stays true for that window, and a +// connection modified during it can be overwritten as startup completes. +// +// A missing system bus is reported as "not ready" rather than as a hard error, +// because a host early enough in boot to beat NetworkManager can also be early +// enough to beat dbus-daemon. +func networkManagerReady() (bool, error) { + conn, err := dbus.SystemBus() + if err != nil { + return false, err + } + owned, err := nmBusNameOwned(conn) + if err != nil || !owned { + return false, err + } + + daemon, err := gonetworkmanager.NewNetworkManager() + if err != nil { + return false, err + } + startup, err := daemon.GetPropertyStartup() + if err != nil { + return false, err + } + return !startup, nil +} + +// newNetworkManager waits for the NetworkManager daemon to be ready, then opens +// its settings interface. Unlike the file backends, NetworkManager is +// configured through a running daemon, so a configurator built while it is +// still starting would hold a settings handle that reports no connections and +// accepts no changes. +func newNetworkManager(ctx context.Context, readyTimeout time.Duration) (nm *networkManager, err error) { + if err = waitForServiceReady(ctx, "NetworkManager", readyTimeout, networkManagerReady); err != nil { + return nil, err + } + config, err := gonetworkmanager.NewSettings() if err != nil { return @@ -326,6 +418,7 @@ func (nm *networkManager) GetInterfaces() (interfaces []*Interface, err error) { i.Name = connection.Name i.MAC = mac i.Link = foundLink + i.DHCP4, i.DHCP6 = connection.dhcpState() // Append addresses. for _, addr := range connection.Addresses4 { @@ -408,44 +501,125 @@ func (nm *networkManager) SetIfaceAddresses(ctx context.Context, iface string, a return } + // Read the current addressing method of each family. NetworkManager + // serves static addresses alongside a lease when the method is "auto", + // so a connection already on DHCP keeps it: changing an address is not + // a request to stop using DHCP, and SetIfaceDHCP is how that is asked + // for. Only a family that is not already leasing is moved to "manual" + // (or off, when it is left with no addresses at all). + method4, _ := settings["ipv4"]["method"].(string) + method6, _ := settings["ipv6"]["method"].(string) + conn := &nmConnection{Method4: method4, Method6: method6} + dhcp4, dhcp6 := conn.dhcpState() + // Update address list for IPv4. - if len(addrs4) == 0 { - _, err = runCommand(ctx, "nmcli", "connection", "modify", id, "ipv4.method", "disabled", "ipv4.addresses", "", "ipv4.gateway", "") - if err != nil { - errs = append(errs, fmt.Errorf("failed to disable ipv4 on %s: %w", id, err)) - } - } else { - gateway4S := "" - if gateway4 != nil { - gateway4S = gateway4.String() - } - _, err = runCommand(ctx, "nmcli", "connection", "modify", id, "ipv4.method", "manual", "ipv4.addresses", strings.Join(addrs4, ","), "ipv4.gateway", gateway4S) - if err != nil { - errs = append(errs, fmt.Errorf("failed to set ipv4.addresses on %s: %w", id, err)) - } + newMethod4 := nmStaticMethod4(len(addrs4) != 0) + if dhcp4 { + newMethod4 = "auto" + } + gateway4S := "" + if gateway4 != nil && len(addrs4) != 0 { + gateway4S = gateway4.String() + } + _, err = runCommand(ctx, "nmcli", "connection", "modify", id, "ipv4.method", newMethod4, "ipv4.addresses", strings.Join(addrs4, ","), "ipv4.gateway", gateway4S) + if err != nil { + errs = append(errs, fmt.Errorf("failed to set ipv4.addresses on %s: %w", id, err)) } // Update address list for IPv6. - if len(addrs6) == 0 { - _, err = runCommand(ctx, "nmcli", "connection", "modify", id, "ipv6.method", "link-local", "ipv6.addresses", "", "ipv6.gateway", "") - if err != nil { - errs = append(errs, fmt.Errorf("failed to set ipv6 link-local on %s: %w", id, err)) - } - } else { - gateway6S := "" - if gateway6 != nil { - gateway6S = gateway6.String() - } - _, err = runCommand(ctx, "nmcli", "connection", "modify", id, "ipv6.method", "manual", "ipv6.addresses", strings.Join(addrs6, ","), "ipv6.gateway", gateway6S) - if err != nil { - errs = append(errs, fmt.Errorf("failed to set ipv6.addresses on %s: %w", id, err)) - } + newMethod6 := nmStaticMethod6(len(addrs6) != 0) + if dhcp6 { + newMethod6 = method6 + } + gateway6S := "" + if gateway6 != nil && len(addrs6) != 0 { + gateway6S = gateway6.String() + } + _, err = runCommand(ctx, "nmcli", "connection", "modify", id, "ipv6.method", newMethod6, "ipv6.addresses", strings.Join(addrs6, ","), "ipv6.gateway", gateway6S) + if err != nil { + errs = append(errs, fmt.Errorf("failed to set ipv6.addresses on %s: %w", id, err)) } } return errors.Join(errs...) } +// Set the DHCP client state on an interface. +func (nm *networkManager) SetIfaceDHCP(ctx context.Context, iface string, dhcp4, dhcp6 bool) (err error) { + // Get connections from NM. + connections, err := nm.config.ListConnections() + if err != nil { + return err + } + + var errs []error + + // Find the connections that has the interfaces. + for _, c := range connections { + // Get the settings of the connection. + var settings gonetworkmanager.ConnectionSettings + settings, err = c.GetSettings() + if err != nil { + return err + } + + // Get the interface name. + name, ok := settings["connection"]["interface-name"].(string) + if !ok || name != iface { + continue + } + + // Get the interface id. + id, ok := settings["connection"]["id"].(string) + if !ok { + return fmt.Errorf("failed to get interface id") + } + + // Parse the connection so turning a lease off can fall back to the + // method that still serves whatever static addresses it holds. + conn, parseErr := nm.ParseConnection(settings) + if parseErr != nil { + errs = append(errs, fmt.Errorf("failed to parse connection %s: %w", id, parseErr)) + continue + } + + // Switch the IPv4 method. "auto" is DHCPv4; NetworkManager keeps + // serving any ipv4.addresses the connection carries alongside it. + method4 := "auto" + if !dhcp4 { + method4 = nmStaticMethod4(len(conn.Addresses4) != 0) + } + if _, err = runCommand(ctx, "nmcli", "connection", "modify", id, "ipv4.method", method4); err != nil { + errs = append(errs, fmt.Errorf("failed to set ipv4.method on %s: %w", id, err)) + } + + // Switch the IPv6 method. A connection already on "auto" is left there + // rather than forced to "dhcp": both run a DHCPv6 client, and "auto" + // additionally honors router advertisements, which turning it into + // "dhcp" would silently switch off. + method6 := "auto" + if dhcp6 && conn.Method6 == "dhcp" { + method6 = "dhcp" + } else if !dhcp6 { + method6 = nmStaticMethod6(len(conn.Addresses6) != 0) + } + if _, err = runCommand(ctx, "nmcli", "connection", "modify", id, "ipv6.method", method6); err != nil { + errs = append(errs, fmt.Errorf("failed to set ipv6.method on %s: %w", id, err)) + } + } + + return errors.Join(errs...) +} + +// renewDHCP has NetworkManager reapply the connection profile to the device, so +// a client it was just told to run starts and acquires a lease. `device +// reapply` changes the device in place and, unlike `connection up`, does not +// tear the link down first. +func (nm *networkManager) renewDHCP(ctx context.Context, iface string) error { + _, err := runCommand(ctx, "nmcli", "device", "reapply", iface) + return err +} + // Set static routes to interface. func (nm *networkManager) SetIfaceRoutes(ctx context.Context, iface string, routes []*Route) (err error) { // Build route slices. diff --git a/networkManager_test.go b/networkManager_test.go index 14c2a67..4c185ec 100644 --- a/networkManager_test.go +++ b/networkManager_test.go @@ -10,6 +10,8 @@ import ( "github.com/Wifx/gonetworkmanager/v3" "github.com/godbus/dbus/v5" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) type mockNMConnection struct { @@ -321,9 +323,7 @@ func TestNetworkManager(t *testing.T) { // Setup test file. tmpDir := "/tmp/networkManager-netconfig-Test" testDir, err := filepath.Abs("./tests/networkManager") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) resultsDir := filepath.Join(testDir, "results") // Update the PATH environment variable so that our nmcli is used instead of the systems. @@ -336,15 +336,11 @@ func TestNetworkManager(t *testing.T) { // Get the interfaces state. interfaces, err := n.GetInterfaces() - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Verify interfaces read from file. err = testVerifyInterfaces(interfaces, resultsDir, 1) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Test setting the IP addresses on an interface. err = n.SetIfaceAddresses(context.Background(), "test_eth0.1556", []*net.IPNet{ @@ -361,9 +357,7 @@ func TestNetworkManager(t *testing.T) { Mask: net.CIDRMask(64, 128), }, }, net.ParseIP("1.2.3.1"), net.ParseIP("fc00::1")) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Test setting routes on an interface. err = n.SetIfaceRoutes(context.Background(), "test_eth2", []*Route{ @@ -384,9 +378,7 @@ func TestNetworkManager(t *testing.T) { Metric: 100, }, }) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Test setting DNS on an interface; static DNS should disable automatic DNS. err = n.SetIfaceDNS(context.Background(), "test_eth0", []net.IP{ @@ -394,15 +386,11 @@ func TestNetworkManager(t *testing.T) { net.ParseIP("1.1.1.1"), net.ParseIP("2001:4860:4860::8888"), }, []string{"example.com"}) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Read the current file and expected state. err = testVerifyResults(resultsDir, tmpDir, 1) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Test setting the IP addresses on an interface. err = n.SetIfaceAddresses(context.Background(), "test_eth0", []*net.IPNet{ @@ -411,27 +399,19 @@ func TestNetworkManager(t *testing.T) { Mask: net.CIDRMask(24, 32), }, }, net.ParseIP("1.2.10.254"), nil) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Test setting routes on an interface. err = n.SetIfaceRoutes(context.Background(), "test_eth0.1556", []*Route{}) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Read the current file and expected state. err = testVerifyResults(resultsDir, tmpDir, 2) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Cleanup. err = os.RemoveAll(tmpDir) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) } // nmNameservers must read the modern dns-data (strings) as well as the legacy @@ -445,30 +425,21 @@ func TestNMDNSParsing(t *testing.T) { } got := nmNameservers(group) want := []net.IP{net.ParseIP("8.8.8.8"), net.ParseIP("1.1.1.1")} - if !equalIPs(got, want) { - t.Errorf("dns-data = %v, want %v", got, want) - } - if search := nmSearchDomains(group); len(search) != 2 || search[0] != "a.test" || search[1] != "b.test" { - t.Errorf("dns-search = %v, want [a.test b.test]", search) - } + assert.Truef(t, equalIPs(got, want), "dns-data = %v, want %v", got, want) + search := nmSearchDomains(group) + assert.Falsef(t, len(search) != 2 || search[0] != "a.test" || search[1] != "b.test", "dns-search = %v, want [a.test b.test]", search) // Legacy ipv4 "dns" as []uint32 (decoded via uint2IP, matching addresses). ipv4Legacy := map[string]any{"dns": []uint32{ip2Uint(net.ParseIP("1.2.3.4"))}} - if got := nmNameservers(ipv4Legacy); len(got) != 1 || !got[0].Equal(net.ParseIP("1.2.3.4")) { - t.Errorf("legacy ipv4 dns = %v, want [1.2.3.4]", got) - } + gotV4 := nmNameservers(ipv4Legacy) + assert.Falsef(t, len(gotV4) != 1 || !gotV4[0].Equal(net.ParseIP("1.2.3.4")), "legacy ipv4 dns = %v, want [1.2.3.4]", gotV4) // Legacy ipv6 "dns" as [][]byte. ipv6Legacy := map[string]any{"dns": [][]byte{net.ParseIP("2001:4860:4860::8888").To16()}} - if got := nmNameservers(ipv6Legacy); len(got) != 1 || !got[0].Equal(net.ParseIP("2001:4860:4860::8888")) { - t.Errorf("legacy ipv6 dns = %v, want [2001:4860:4860::8888]", got) - } + gotV6 := nmNameservers(ipv6Legacy) + assert.Falsef(t, len(gotV6) != 1 || !gotV6[0].Equal(net.ParseIP("2001:4860:4860::8888")), "legacy ipv6 dns = %v, want [2001:4860:4860::8888]", gotV6) // Empty group yields nothing. - if v := nmNameservers(map[string]any{}); len(v) != 0 { - t.Errorf("empty group dns = %v, want empty", v) - } - if v := nmSearchDomains(map[string]any{}); len(v) != 0 { - t.Errorf("empty group dns-search = %v, want empty", v) - } + assert.Emptyf(t, nmNameservers(map[string]any{}), "empty group dns, want empty") + assert.Emptyf(t, nmSearchDomains(map[string]any{}), "empty group dns-search, want empty") } diff --git a/networkScripts.go b/networkScripts.go index 3d9dbd0..c448d30 100644 --- a/networkScripts.go +++ b/networkScripts.go @@ -44,6 +44,43 @@ type networkScripts struct { backupRetention int } +// nsDHCPState reports whether an ifcfg file's keys enable a DHCP client for +// each family. BOOTPROTO names the IPv4 method — "dhcp" and the near-extinct +// "bootp" are the dynamic ones, and any other value, including an absent key, +// means the address is configured statically. The IPv6 client is separate and +// off unless DHCPV6C says otherwise. +func nsDHCPState(config map[string]string) (dhcp4, dhcp6 bool) { + switch strings.ToLower(config["BOOTPROTO"]) { + case "dhcp", "bootp": + dhcp4 = true + } + switch strings.ToLower(config["DHCPV6C"]) { + case "yes", "true", "1": + dhcp6 = true + } + return dhcp4, dhcp6 +} + +// nsSetDHCP writes the DHCP client state into an ifcfg config map. Disabling +// IPv4 leaves BOOTPROTO=none rather than deleting it: unlike the netplan and +// networkd schemas, an absent BOOTPROTO is read by some initscript versions as +// a prompt to guess, and "none" states the intent. PERSISTENT_DHCLIENT only has +// meaning while the IPv4 client runs, so it goes with it. +func nsSetDHCP(config map[string]string, dhcp4, dhcp6 bool) { + if dhcp4 { + config["BOOTPROTO"] = "dhcp" + } else { + config["BOOTPROTO"] = "none" + delete(config, "PERSISTENT_DHCLIENT") + } + if dhcp6 { + config["IPV6INIT"] = "yes" + config["DHCPV6C"] = "yes" + } else { + delete(config, "DHCPV6C") + } +} + // Either retreives an existing interface, or makes a new one. func (ns *networkScripts) EnsureInterface(name string) *nsInterface { // Find existing interface and return it. @@ -523,6 +560,12 @@ func (ns *networkScripts) GetInterfaces() (interfaces []*Interface, err error) { // Parse interface files. for _, file := range iface.IFFiles { + // Read the DHCP client state. Any of an interface's files enabling + // a client means the interface runs one. + dhcp4, dhcp6 := nsDHCPState(file.Config) + i.DHCP4 = i.DHCP4 || dhcp4 + i.DHCP6 = i.DHCP6 || dhcp6 + // Parse IPv4 addresses in the file. ipAddr := ns.ParseIP(file.Config, "") if ipAddr != nil { @@ -830,6 +873,42 @@ func (ns *networkScripts) SetIfaceAddresses(_ context.Context, iface string, add return } +// Set the DHCP client state on an interface. +func (ns *networkScripts) SetIfaceDHCP(_ context.Context, iface string, dhcp4, dhcp6 bool) (err error) { + // Find or create the interface. A backend must not silently no-op just + // because it has not seen this interface before (e.g. a freshly attached + // NIC), otherwise the caller gets a success with no persisted change. + ifc := ns.EnsureInterface(iface) + + // Merge configs. + config := make(map[string]string) + for _, file := range ifc.IFFiles { + err = mergo.Merge(&config, file.Config) + if err != nil { + return err + } + } + + nsSetDHCP(config, dhcp4, dhcp6) + + // Try to save. + return ifc.Save(config, ns.ConfigDir, ns.backupRetention) +} + +// renewDHCP restarts the interface through the initscripts so the DHCP client +// that was just written to its ifcfg file is started. `ifup` on an interface +// that is already up is a no-op on these distributions, so it is taken down +// first; this is the same ifdown/ifup pair the initscripts themselves use, and +// it is what acquires the lease. +func (ns *networkScripts) renewDHCP(ctx context.Context, iface string) error { + // ifdown on an interface that is already down exits non-zero. That is not a + // failure of this call — the interface being down is the state ifup wants — + // so only the ifup result decides. + _, _ = runCommand(ctx, "ifdown", iface) + _, err := runCommand(ctx, "ifup", iface) + return err +} + // Set static routes to interface. func (ns *networkScripts) SetIfaceRoutes(_ context.Context, iface string, routes []*Route) (err error) { // Build route slices. diff --git a/networkScripts_test.go b/networkScripts_test.go index d2e8db0..fb2b0c9 100644 --- a/networkScripts_test.go +++ b/networkScripts_test.go @@ -7,26 +7,22 @@ import ( "path/filepath" "strings" "testing" + + "github.com/stretchr/testify/require" ) // Validate the network scripts configuration parser/writer functions. func TestNetworkScripts(t *testing.T) { // Setup test file. tmpDir, err := os.MkdirTemp("", "") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) testDir, err := filepath.Abs("./tests/networkScripts") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) resultsDir := filepath.Join(testDir, "results") // Try to read the test directory for files to copy to our temporary directory. entries, err := os.ReadDir(testDir) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) for _, entry := range entries { if !strings.HasPrefix(entry.Name(), "ifcfg-") && !strings.HasPrefix(entry.Name(), "route-") && !strings.HasPrefix(entry.Name(), "route6-") { continue @@ -35,28 +31,20 @@ func TestNetworkScripts(t *testing.T) { configPath := filepath.Join(testDir, entry.Name()) tmpPath := filepath.Join(tmpDir, entry.Name()) err = fileCopy(configPath, tmpPath) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) } // Setup ifupdown and parse test file. ns, err := newNetworkScriptsWithConfig(tmpDir) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Get the interfaces state. interfaces, err := ns.GetInterfaces() - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Verify interfaces read from file. err = testVerifyInterfaces(interfaces, resultsDir, 1) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Test setting the IP addresses on an interface. err = ns.SetIfaceAddresses(context.Background(), "test_eth0.1556", []*net.IPNet{ @@ -73,9 +61,7 @@ func TestNetworkScripts(t *testing.T) { Mask: net.CIDRMask(64, 128), }, }, net.ParseIP("1.2.3.1"), net.ParseIP("fc00::1")) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Test setting routes on an interface. err = ns.SetIfaceRoutes(context.Background(), "test_eth2", []*Route{ @@ -96,33 +82,23 @@ func TestNetworkScripts(t *testing.T) { Metric: 100, }, }) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Verify we can re-read configurations. ns, err = newNetworkScriptsWithConfig(tmpDir) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Get the interfaces state. interfaces, err = ns.GetInterfaces() - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Verify interfaces read from file. err = testVerifyInterfaces(interfaces, resultsDir, 2) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Read the current file and expected state. err = testVerifyResults(resultsDir, tmpDir, 1) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Test setting the IP addresses on an interface. err = ns.SetIfaceAddresses(context.Background(), "test_eth0", []*net.IPNet{ @@ -131,43 +107,29 @@ func TestNetworkScripts(t *testing.T) { Mask: net.CIDRMask(24, 32), }, }, net.ParseIP("1.2.10.254"), nil) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Test setting routes on an interface. err = ns.SetIfaceRoutes(context.Background(), "test_eth0.1556", []*Route{}) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Verify we can re-read configurations. ns, err = newNetworkScriptsWithConfig(tmpDir) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Get the interfaces state. interfaces, err = ns.GetInterfaces() - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Verify interfaces read from file. err = testVerifyInterfaces(interfaces, resultsDir, 3) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Read the current file and expected state. err = testVerifyResults(resultsDir, tmpDir, 2) - if err != nil { - t.Error(err) - } + require.NoError(t, err) // Cleanup. err = os.RemoveAll(tmpDir) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) } diff --git a/networkScripts_unit_test.go b/networkScripts_unit_test.go index 68f3eaa..2bf0949 100644 --- a/networkScripts_unit_test.go +++ b/networkScripts_unit_test.go @@ -3,6 +3,9 @@ package netconfig import ( "net" "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) // parseIPRoutes is a pure transform from raw whitespace-split route fields to @@ -12,40 +15,29 @@ func TestParseIPRoutes(t *testing.T) { routes := parseIPRoutes([][]string{ {"10.0.0.0/24", "via", "10.0.0.1", "metric", "100"}, }) - if len(routes) != 1 { - t.Fatalf("got %d routes, want 1", len(routes)) - } + require.Len(t, routes, 1) r := routes[0] - if got := r.Destination.String(); got != "10.0.0.0/24" { - t.Errorf("destination = %s, want 10.0.0.0/24", got) - } - if !r.Gateway.Equal(net.ParseIP("10.0.0.1")) { - t.Errorf("gateway = %s, want 10.0.0.1", r.Gateway) - } - if r.Metric != 100 { - t.Errorf("metric = %d, want 100", r.Metric) - } + assert.Equal(t, "10.0.0.0/24", r.Destination.String()) + assert.True(t, r.Gateway.Equal(net.ParseIP("10.0.0.1"))) + assert.Equal(t, 100, r.Metric) }) t.Run("default metric when absent", func(t *testing.T) { routes := parseIPRoutes([][]string{{"10.0.0.0/24", "via", "10.0.0.1"}}) - if len(routes) != 1 || routes[0].Metric != 256 { - t.Fatalf("expected single route with default metric 256, got %+v", routes) - } + require.Len(t, routes, 1) + assert.Equal(t, 256, routes[0].Metric) }) t.Run("metric zero falls back to default", func(t *testing.T) { routes := parseIPRoutes([][]string{{"10.0.0.0/24", "metric", "0"}}) - if len(routes) != 1 || routes[0].Metric != 256 { - t.Fatalf("expected default metric 256 for metric 0, got %+v", routes) - } + require.Len(t, routes, 1) + assert.Equal(t, 256, routes[0].Metric) }) t.Run("invalid metric falls back to default", func(t *testing.T) { routes := parseIPRoutes([][]string{{"10.0.0.0/24", "metric", "notanumber"}}) - if len(routes) != 1 || routes[0].Metric != 256 { - t.Fatalf("expected default metric 256 for bad metric, got %+v", routes) - } + require.Len(t, routes, 1) + assert.Equal(t, 256, routes[0].Metric) }) t.Run("empty and invalid entries are skipped", func(t *testing.T) { @@ -55,12 +47,8 @@ func TestParseIPRoutes(t *testing.T) { {"10.0.0.0/24", "via", "nope"}, // bad gateway: skipped {"192.168.0.0/16", "via", "192.168.0.1"}, // valid }) - if len(routes) != 1 { - t.Fatalf("got %d routes, want 1", len(routes)) - } - if got := routes[0].Destination.String(); got != "192.168.0.0/16" { - t.Errorf("destination = %s, want 192.168.0.0/16", got) - } + require.Len(t, routes, 1) + assert.Equal(t, "192.168.0.0/16", routes[0].Destination.String()) }) } @@ -69,67 +57,45 @@ func TestNetworkScriptsParseIP(t *testing.T) { ns := &networkScripts{} t.Run("missing IPADDR returns nil", func(t *testing.T) { - if got := ns.ParseIP(map[string]string{}, ""); got != nil { - t.Errorf("got %v, want nil", got) - } + assert.Nil(t, ns.ParseIP(map[string]string{}, "")) }) t.Run("empty IPADDR returns nil", func(t *testing.T) { - if got := ns.ParseIP(map[string]string{"IPADDR": ""}, ""); got != nil { - t.Errorf("got %v, want nil", got) - } + assert.Nil(t, ns.ParseIP(map[string]string{"IPADDR": ""}, "")) }) t.Run("explicit prefix", func(t *testing.T) { got := ns.ParseIP(map[string]string{"IPADDR": "192.168.1.5", "PREFIX": "24"}, "") - if got == nil { - t.Fatal("got nil") - } - if !got.IP.Equal(net.ParseIP("192.168.1.5")) { - t.Errorf("IP = %s, want 192.168.1.5", got.IP) - } - if ones, _ := got.Mask.Size(); ones != 24 { - t.Errorf("prefix = %d, want 24", ones) - } + require.NotNil(t, got) + assert.True(t, got.IP.Equal(net.ParseIP("192.168.1.5"))) + ones, _ := got.Mask.Size() + assert.Equal(t, 24, ones) }) t.Run("prefix derived from netmask", func(t *testing.T) { got := ns.ParseIP(map[string]string{"IPADDR": "10.1.2.3", "NETMASK": "255.255.255.0"}, "") - if got == nil { - t.Fatal("got nil") - } - if ones, _ := got.Mask.Size(); ones != 24 { - t.Errorf("prefix = %d, want 24 (from netmask)", ones) - } + require.NotNil(t, got) + ones, _ := got.Mask.Size() + assert.Equal(t, 24, ones) }) t.Run("defaults to /16 with no prefix or netmask", func(t *testing.T) { got := ns.ParseIP(map[string]string{"IPADDR": "10.1.2.3"}, "") - if got == nil { - t.Fatal("got nil") - } - if ones, _ := got.Mask.Size(); ones != 16 { - t.Errorf("prefix = %d, want default 16", ones) - } + require.NotNil(t, got) + ones, _ := got.Mask.Size() + assert.Equal(t, 16, ones) }) t.Run("indexed keys", func(t *testing.T) { cfg := map[string]string{"IPADDR0": "172.16.0.9", "PREFIX0": "30"} got := ns.ParseIP(cfg, "0") - if got == nil { - t.Fatal("got nil") - } - if !got.IP.Equal(net.ParseIP("172.16.0.9")) { - t.Errorf("IP = %s, want 172.16.0.9", got.IP) - } - if ones, _ := got.Mask.Size(); ones != 30 { - t.Errorf("prefix = %d, want 30", ones) - } + require.NotNil(t, got) + assert.True(t, got.IP.Equal(net.ParseIP("172.16.0.9"))) + ones, _ := got.Mask.Size() + assert.Equal(t, 30, ones) }) t.Run("invalid address returns nil", func(t *testing.T) { - if got := ns.ParseIP(map[string]string{"IPADDR": "not-an-ip", "PREFIX": "24"}, ""); got != nil { - t.Errorf("got %v, want nil", got) - } + assert.Nil(t, ns.ParseIP(map[string]string{"IPADDR": "not-an-ip", "PREFIX": "24"}, "")) }) } diff --git a/networkd.go b/networkd.go index 0a38b89..e1c2aa7 100644 --- a/networkd.go +++ b/networkd.go @@ -31,6 +31,56 @@ type networkd struct { backupRetention int } +// parseNetworkdDHCP reports which families a [Network] DHCP= value enables. +// systemd.network(5) documents "yes", "no", "ipv4" and "ipv6"; the boolean +// spellings and the pre-v243 aliases "both"/"none" are still accepted by +// systemd and still appear in configs written by older tooling, so they are +// recognised here too. An unset or unrecognised value enables nothing. +func parseNetworkdDHCP(value string) (dhcp4, dhcp6 bool) { + switch strings.ToLower(strings.TrimSpace(value)) { + case "yes", "true", "on", "1", "both": + return true, true + case "ipv4", "v4": + return true, false + case "ipv6", "v6": + return false, true + } + return false, false +} + +// formatNetworkdDHCP renders the families back into a DHCP= value, returning +// false when no family is left and the key should be removed rather than +// written as "no". systemd already defaults DHCP= to no. +func formatNetworkdDHCP(dhcp4, dhcp6 bool) (string, bool) { + switch { + case dhcp4 && dhcp6: + return "yes", true + case dhcp4: + return "ipv4", true + case dhcp6: + return "ipv6", true + } + return "", false +} + +// setNetworkdDHCP rewrites the [Network] DHCP= key to request a lease for +// exactly the requested families. With neither family enabled the key is +// removed rather than written as "no": systemd already defaults DHCP= to no, +// and a config that never mentioned DHCP should not grow the key. +func setNetworkdDHCP(sec *ini.Section, dhcp4, dhcp6 bool) (err error) { + value, keep := formatNetworkdDHCP(dhcp4, dhcp6) + if !keep { + sec.DeleteKey("DHCP") + return nil + } + if key, kerr := sec.GetKey("DHCP"); kerr == nil { + key.SetValue(value) + return nil + } + _, err = sec.NewKey("DHCP", value) + return err +} + // Verify try parsing networkd configurations. func newNetworkd(backupRetention int) (n *networkd, err error) { // Try to parse the networkd configurations. /etc/systemd/network is the @@ -222,6 +272,13 @@ func (nd *networkd) GetInterfaces() (interfaces []*Interface, err error) { switch sec.Name() { // IF this is an network section, try to find addresses. case "Network": + // Read the DHCP client state before the Address lookup below, + // which bails out of this section when no address is set — a + // DHCP-only network has exactly that shape. + if dhcpKey, _ := sec.GetKey("DHCP"); dhcpKey != nil { + i.DHCP4, i.DHCP6 = parseNetworkdDHCP(dhcpKey.String()) + } + // Find the addresses in the address key. addressKey, _ := sec.GetKey("Address") if addressKey == nil { @@ -516,6 +573,45 @@ func (nd *networkd) SetIfaceAddresses(_ context.Context, iface string, addrs []* return nil } +// Set the DHCP client state on an interface. +func (nd *networkd) SetIfaceDHCP(_ context.Context, iface string, dhcp4, dhcp6 bool) (err error) { + for _, n := range nd.Networks { + if n.Name != iface { + continue + } + + // Get the network section to modify it. + sec, err := n.Config.GetSection("Network") + if err != nil { + return err + } + if err = setNetworkdDHCP(sec, dhcp4, dhcp6); err != nil { + return err + } + + // Save the network. + err = n.Save(nd.configDir, nd.backupRetention) + if err != nil { + return err + } + } + + return nil +} + +// renewDHCP has systemd-networkd re-read its configuration and reapply it to +// the interface, starting the DHCP client that was just enabled. `networkctl +// reload` picks up the file that was written; `reconfigure` is what actually +// re-runs the interface's configuration, and reload alone would leave the +// change pending until the link next changed state. +func (nd *networkd) renewDHCP(ctx context.Context, iface string) error { + if _, err := runCommand(ctx, "networkctl", "reload"); err != nil { + return err + } + _, err := runCommand(ctx, "networkctl", "reconfigure", iface) + return err +} + // Set static routes to interface. func (nd *networkd) SetIfaceRoutes(_ context.Context, iface string, routes []*Route) (err error) { for _, n := range nd.Networks { diff --git a/networkd_test.go b/networkd_test.go index b3beb32..efa4dbd 100644 --- a/networkd_test.go +++ b/networkd_test.go @@ -7,26 +7,23 @@ import ( "path/filepath" "strings" "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) // Validate the networkd configuration parser/writer functions. func TestNetworkd(t *testing.T) { // Setup test file. tmpDir, err := os.MkdirTemp("", "") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) testDir, err := filepath.Abs("./tests/networkd") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) resultsDir := filepath.Join(testDir, "results") // Try to read the test directory for files to copy to our temporary directory. entries, err := os.ReadDir(testDir) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) for _, entry := range entries { if !strings.HasSuffix(entry.Name(), ".network") { continue @@ -35,28 +32,20 @@ func TestNetworkd(t *testing.T) { configPath := filepath.Join(testDir, entry.Name()) tmpPath := filepath.Join(tmpDir, entry.Name()) err = fileCopy(configPath, tmpPath) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) } // Setup ifupdown and parse test file. n, err := readNetworkdConfigDirectory(tmpDir) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Get the interfaces state. interfaces, err := n.GetInterfaces() - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Verify interfaces read from file. err = testVerifyInterfaces(interfaces, resultsDir, 1) - if err != nil { - t.Error(err) - } + assert.NoError(t, err) // Test setting the IP addresses on an interface. err = n.SetIfaceAddresses(context.Background(), "test_eth0.1556", []*net.IPNet{ @@ -73,9 +62,7 @@ func TestNetworkd(t *testing.T) { Mask: net.CIDRMask(64, 128), }, }, net.ParseIP("1.2.3.1"), net.ParseIP("fc00::1")) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Test setting routes on an interface. err = n.SetIfaceRoutes(context.Background(), "test_eth2", []*Route{ @@ -96,33 +83,23 @@ func TestNetworkd(t *testing.T) { Metric: 100, }, }) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Verify we can re-read configurations. n, err = readNetworkdConfigDirectory(tmpDir) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Get the interfaces state. interfaces, err = n.GetInterfaces() - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Verify interfaces read from file. err = testVerifyInterfaces(interfaces, resultsDir, 2) - if err != nil { - t.Error(err) - } + assert.NoError(t, err) // Read the current file and expected state. err = testVerifyResults(resultsDir, tmpDir, 1) - if err != nil { - t.Error(err) - } + assert.NoError(t, err) // Test setting the IP addresses on an interface. err = n.SetIfaceAddresses(context.Background(), "test_eth0", []*net.IPNet{ @@ -131,43 +108,29 @@ func TestNetworkd(t *testing.T) { Mask: net.CIDRMask(24, 32), }, }, net.ParseIP("1.2.10.254"), nil) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Test setting routes on an interface. err = n.SetIfaceRoutes(context.Background(), "test_eth0.1556", []*Route{}) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Verify we can re-read configurations. n, err = readNetworkdConfigDirectory(tmpDir) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Get the interfaces state. interfaces, err = n.GetInterfaces() - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Verify interfaces read from file. err = testVerifyInterfaces(interfaces, resultsDir, 3) - if err != nil { - t.Error(err) - } + assert.NoError(t, err) // Read the current file and expected state. err = testVerifyResults(resultsDir, tmpDir, 2) - if err != nil { - t.Error(err) - } + assert.NoError(t, err) // Cleanup. err = os.RemoveAll(tmpDir) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) } diff --git a/options.go b/options.go index 8a424f4..da2e5ce 100644 --- a/options.go +++ b/options.go @@ -20,6 +20,10 @@ const ( // defaultBackupRetention is how many .bak.* copies are kept per original // configuration file. Older backups beyond this number are pruned on save. defaultBackupRetention = 5 + // defaultServiceReadyTimeout bounds how long NewConfigurator waits for a + // backend's daemon to come up and finish starting. It only elapses in full + // on a host where the service is registered to run but never arrives. + defaultServiceReadyTimeout = 60 * time.Second ) // Logger is the minimal logging surface used across the package. It is @@ -72,6 +76,14 @@ type configOptions struct { // (cPanel/Plesk/InterWorx) and only remove the address from the running // system and the network-manager configuration files. skipPanels bool + // allowNoBackends lets NewConfigurator succeed on a Linux host where no + // network configuration backend was detected, where changes would otherwise + // apply to the running system without persisting. + allowNoBackends bool + // serviceReadyTimeout bounds how long backend detection waits for a + // daemon-backed backend (NetworkManager) to become ready; <= 0 does not + // wait at all. + serviceReadyTimeout time.Duration } // defaultConfigOptions returns the options used when no Option is supplied. @@ -82,6 +94,7 @@ func defaultConfigOptions() *configOptions { pingCount: defaultPingCount, pingTimeout: defaultPingTimeout, backupRetention: defaultBackupRetention, + serviceReadyTimeout: defaultServiceReadyTimeout, } } @@ -193,3 +206,37 @@ func WithSkipPanels(skip bool) Option { o.skipPanels = skip } } + +// WithAllowNoBackends lets NewConfigurator return a configurator on a Linux +// host where backend detection found no network configuration backend. By +// default that is an error: netlink would apply an address change to the +// running system, but with nothing to write it to the change would not survive +// a reboot, and the caller would never be told. Enable this when the +// configurator is only used to read state (GetInterfaces), or when the caller +// accepts runtime-only changes. +func WithAllowNoBackends(allowed bool) Option { + return func(o *configOptions) { + o.allowNoBackends = allowed + } +} + +// WithServiceReadyTimeout bounds how long NewConfigurator waits for a backend +// whose configuration is made through a daemon rather than a file. Today that +// is NetworkManager: it is configured over D-Bus, so a configurator built +// before the daemon owns its bus name would find no connections to change. +// +// This matters at boot. A host that starts this program from a unit ordered +// alongside NetworkManager, rather than after it, reaches backend detection +// while the daemon is still starting; without the wait it would either fail to +// register the backend at all, or register one that reports an empty interface +// list. The wait ends as soon as the daemon answers and reports it has finished +// starting up, so a host where it is already running pays nothing. +// +// A timeout of zero or less does not wait: detection probes once and proceeds, +// which is the right choice for a caller that has already ordered itself after +// the daemon and would rather fail fast than block. +func WithServiceReadyTimeout(timeout time.Duration) Option { + return func(o *configOptions) { + o.serviceReadyTimeout = timeout + } +} diff --git a/options_test.go b/options_test.go index cdf912e..a1635e5 100644 --- a/options_test.go +++ b/options_test.go @@ -8,6 +8,9 @@ import ( "path/filepath" "testing" "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) // captureLogger records the last formatted message so SetLogger can be @@ -22,57 +25,48 @@ func (c *captureLogger) Println(args ...interface{}) { c.printlnCa func TestConfigOptionsDefaults(t *testing.T) { o := newConfigOptions() - if o.testAddress != defaultInternetTestAddress { - t.Errorf("default testAddress = %q, want %q", o.testAddress, defaultInternetTestAddress) - } - if o.skipConnectivityCheck { - t.Error("skipConnectivityCheck = true, want false by default") - } - if o.connectivityTimeout != defaultConnectivityTimeout { - t.Errorf("default connectivityTimeout = %v, want %v", o.connectivityTimeout, defaultConnectivityTimeout) - } - if o.pingCount != defaultPingCount { - t.Errorf("default pingCount = %d, want %d", o.pingCount, defaultPingCount) - } - if o.pingTimeout != defaultPingTimeout { - t.Errorf("default pingTimeout = %v, want %v", o.pingTimeout, defaultPingTimeout) - } - if o.backupRetention != defaultBackupRetention { - t.Errorf("default backupRetention = %d, want %d", o.backupRetention, defaultBackupRetention) - } - if o.allowPrimaryRemoval { - t.Error("allowPrimaryRemoval = true, want false by default") - } - if o.skipPanels { - t.Error("skipPanels = true, want false by default") - } + assert.Equal(t, defaultInternetTestAddress, o.testAddress) + assert.False(t, o.skipConnectivityCheck) + assert.Equal(t, defaultConnectivityTimeout, o.connectivityTimeout) + assert.Equal(t, defaultPingCount, o.pingCount) + assert.Equal(t, defaultPingTimeout, o.pingTimeout) + assert.Equal(t, defaultBackupRetention, o.backupRetention) + assert.Equal(t, defaultServiceReadyTimeout, o.serviceReadyTimeout) + assert.False(t, o.allowPrimaryRemoval) + assert.False(t, o.skipPanels) + assert.False(t, o.allowNoBackends) +} + +func TestWithServiceReadyTimeout(t *testing.T) { + o := newConfigOptions(WithServiceReadyTimeout(90 * time.Second)) + assert.Equal(t, 90*time.Second, o.serviceReadyTimeout) + + // Zero and negative both mean "do not wait", and must be preserved rather + // than falling back to the default the way WithTestAddress("") does. + o = newConfigOptions(WithServiceReadyTimeout(0)) + assert.Equal(t, time.Duration(0), o.serviceReadyTimeout) + o = newConfigOptions(WithServiceReadyTimeout(-1)) + assert.Equal(t, -1*time.Nanosecond, o.serviceReadyTimeout) } func TestWithTestAddress(t *testing.T) { o := newConfigOptions(WithTestAddress("http://canary.example/health")) - if o.testAddress != "http://canary.example/health" { - t.Errorf("testAddress = %q, want the overridden value", o.testAddress) - } + assert.Equal(t, "http://canary.example/health", o.testAddress) // An empty address must not clobber the default. o = newConfigOptions(WithTestAddress("")) - if o.testAddress != defaultInternetTestAddress { - t.Errorf("empty WithTestAddress changed default to %q", o.testAddress) - } + assert.Equal(t, defaultInternetTestAddress, o.testAddress) } func TestWithConnectivityCheck(t *testing.T) { - if o := newConfigOptions(WithConnectivityCheck(false)); !o.skipConnectivityCheck { - t.Error("WithConnectivityCheck(false) did not set skipConnectivityCheck") - } - if o := newConfigOptions(WithConnectivityCheck(true)); o.skipConnectivityCheck { - t.Error("WithConnectivityCheck(true) set skipConnectivityCheck") - } + o := newConfigOptions(WithConnectivityCheck(false)) + assert.True(t, o.skipConnectivityCheck) + o = newConfigOptions(WithConnectivityCheck(true)) + assert.False(t, o.skipConnectivityCheck) } func TestWithSkipConnectivityCheck(t *testing.T) { - if o := newConfigOptions(WithSkipConnectivityCheck()); !o.skipConnectivityCheck { - t.Error("WithSkipConnectivityCheck did not set skipConnectivityCheck") - } + o := newConfigOptions(WithSkipConnectivityCheck()) + assert.True(t, o.skipConnectivityCheck) } func TestSetLogger(t *testing.T) { @@ -81,74 +75,58 @@ func TestSetLogger(t *testing.T) { cap := &captureLogger{} SetLogger(cap) - if logger != cap { - t.Fatal("SetLogger did not replace the package logger") - } + require.Equal(t, cap, logger) logger.Printf("hi %d", 1) logger.Println("hi") - if cap.printfCalls != 1 || cap.printlnCalls != 1 { - t.Errorf("logger not used: printf=%d println=%d", cap.printfCalls, cap.printlnCalls) - } + assert.Equal(t, 1, cap.printfCalls) + assert.Equal(t, 1, cap.printlnCalls) // A nil logger must be ignored, leaving the current logger in place. SetLogger(nil) - if logger != cap { - t.Error("SetLogger(nil) replaced the logger") - } + assert.Equal(t, cap, logger) } func TestWithConnectivityTimeout(t *testing.T) { - if o := newConfigOptions(WithConnectivityTimeout(5 * time.Second)); o.connectivityTimeout != 5*time.Second { - t.Errorf("connectivityTimeout = %v, want 5s", o.connectivityTimeout) - } + o := newConfigOptions(WithConnectivityTimeout(5 * time.Second)) + assert.Equal(t, 5*time.Second, o.connectivityTimeout) // Non-positive values must not clobber the default. - if o := newConfigOptions(WithConnectivityTimeout(0)); o.connectivityTimeout != defaultConnectivityTimeout { - t.Errorf("zero WithConnectivityTimeout changed default to %v", o.connectivityTimeout) - } + o = newConfigOptions(WithConnectivityTimeout(0)) + assert.Equal(t, defaultConnectivityTimeout, o.connectivityTimeout) } func TestWithPingCount(t *testing.T) { - if o := newConfigOptions(WithPingCount(3)); o.pingCount != 3 { - t.Errorf("pingCount = %d, want 3", o.pingCount) - } + o := newConfigOptions(WithPingCount(3)) + assert.Equal(t, 3, o.pingCount) // Non-positive values must not clobber the default. - if o := newConfigOptions(WithPingCount(0)); o.pingCount != defaultPingCount { - t.Errorf("zero WithPingCount changed default to %d", o.pingCount) - } + o = newConfigOptions(WithPingCount(0)) + assert.Equal(t, defaultPingCount, o.pingCount) } func TestWithPingTimeout(t *testing.T) { - if o := newConfigOptions(WithPingTimeout(30 * time.Second)); o.pingTimeout != 30*time.Second { - t.Errorf("pingTimeout = %v, want 30s", o.pingTimeout) - } - if o := newConfigOptions(WithPingTimeout(0)); o.pingTimeout != defaultPingTimeout { - t.Errorf("zero WithPingTimeout changed default to %v", o.pingTimeout) - } + o := newConfigOptions(WithPingTimeout(30 * time.Second)) + assert.Equal(t, 30*time.Second, o.pingTimeout) + o = newConfigOptions(WithPingTimeout(0)) + assert.Equal(t, defaultPingTimeout, o.pingTimeout) } func TestWithBackupRetention(t *testing.T) { - if o := newConfigOptions(WithBackupRetention(10)); o.backupRetention != 10 { - t.Errorf("backupRetention = %d, want 10", o.backupRetention) - } + o := newConfigOptions(WithBackupRetention(10)) + assert.Equal(t, 10, o.backupRetention) // Zero/negative disables pruning (keeps all backups). - if o := newConfigOptions(WithBackupRetention(0)); o.backupRetention != 0 { - t.Errorf("backupRetention = %d, want 0", o.backupRetention) - } + o = newConfigOptions(WithBackupRetention(0)) + assert.Equal(t, 0, o.backupRetention) } func TestWithAllowPrimaryRemoval(t *testing.T) { - if o := newConfigOptions(WithAllowPrimaryRemoval(true)); !o.allowPrimaryRemoval { - t.Error("WithAllowPrimaryRemoval(true) did not set allowPrimaryRemoval") - } + o := newConfigOptions(WithAllowPrimaryRemoval(true)) + assert.True(t, o.allowPrimaryRemoval) } func TestWithSkipPanels(t *testing.T) { - if o := newConfigOptions(WithSkipPanels(true)); !o.skipPanels { - t.Error("WithSkipPanels(true) did not set skipPanels") - } - if o := newConfigOptions(WithSkipPanels(false)); o.skipPanels { - t.Error("WithSkipPanels(false) set skipPanels") - } + o := newConfigOptions(WithSkipPanels(true)) + assert.True(t, o.skipPanels) + o = newConfigOptions(WithSkipPanels(false)) + assert.False(t, o.skipPanels) } // applyIfaceDNS must invoke every backend even when an earlier one fails. @@ -160,22 +138,14 @@ func TestApplyIfaceDNS(t *testing.T) { {name: "ok", backend: ok}, } applyIfaceDNS(context.Background(), backends, "eth0", []net.IP{net.ParseIP("8.8.8.8")}, []string{"example.com"}) - if failing.dnsCalls != 1 || ok.dnsCalls != 1 { - t.Errorf("dns calls: failing=%d ok=%d, want 1 each", failing.dnsCalls, ok.dnsCalls) - } + assert.Equal(t, 1, failing.dnsCalls) + assert.Equal(t, 1, ok.dnsCalls) } func TestIPStrings(t *testing.T) { got := ipStrings([]net.IP{net.ParseIP("8.8.8.8"), nil, net.ParseIP("2001:4860:4860::8888")}) want := []string{"8.8.8.8", "2001:4860:4860::8888"} - if len(got) != len(want) { - t.Fatalf("ipStrings = %v, want %v", got, want) - } - for i := range want { - if got[i] != want[i] { - t.Errorf("ipStrings[%d] = %q, want %q", i, got[i], want[i]) - } - } + require.Equal(t, want, got) } // TestNetplanDNS round-trips DNS through the netplan backend: SetIfaceDNS writes @@ -183,29 +153,21 @@ func TestIPStrings(t *testing.T) { func TestNetplanDNS(t *testing.T) { tmpDir := t.TempDir() config := "network:\n version: 2\n ethernets:\n eth0:\n addresses: [192.0.2.10/24]\n" - if err := os.WriteFile(filepath.Join(tmpDir, "01-test.yaml"), []byte(config), 0644); err != nil { - t.Fatal(err) - } + err := os.WriteFile(filepath.Join(tmpDir, "01-test.yaml"), []byte(config), 0644) + require.NoError(t, err) np, err := readNetplanConfigDirectory(tmpDir) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) servers := []net.IP{net.ParseIP("8.8.8.8"), net.ParseIP("2001:4860:4860::8888")} - if err := np.SetIfaceDNS(context.Background(), "eth0", servers, []string{"example.com", "corp.example"}); err != nil { - t.Fatal(err) - } + err = np.SetIfaceDNS(context.Background(), "eth0", servers, []string{"example.com", "corp.example"}) + require.NoError(t, err) // Re-read from disk so we exercise the written config, not in-memory state. np, err = readNetplanConfigDirectory(tmpDir) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) ifaces, err := np.GetInterfaces() - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) var eth0 *Interface for _, i := range ifaces { @@ -213,13 +175,7 @@ func TestNetplanDNS(t *testing.T) { eth0 = i } } - if eth0 == nil { - t.Fatal("eth0 not found after SetIfaceDNS") - } - if len(eth0.DNS) != 2 || !eth0.DNS[0].Equal(servers[0]) || !eth0.DNS[1].Equal(servers[1]) { - t.Errorf("DNS = %v, want %v", eth0.DNS, servers) - } - if len(eth0.SearchDomains) != 2 || eth0.SearchDomains[0] != "example.com" || eth0.SearchDomains[1] != "corp.example" { - t.Errorf("SearchDomains = %v, want [example.com corp.example]", eth0.SearchDomains) - } + require.NotNil(t, eth0) + assert.True(t, len(eth0.DNS) == 2 && eth0.DNS[0].Equal(servers[0]) && eth0.DNS[1].Equal(servers[1])) + assert.True(t, len(eth0.SearchDomains) == 2 && eth0.SearchDomains[0] == "example.com" && eth0.SearchDomains[1] == "corp.example") } diff --git a/services.go b/services.go new file mode 100644 index 0000000..53aa28e --- /dev/null +++ b/services.go @@ -0,0 +1,78 @@ +package netconfig + +import ( + "context" + "fmt" + "time" +) + +// serviceReadyPollInterval is how often waitForServiceReady re-probes. A daemon +// coming up at boot takes seconds, so probing a few times a second costs little +// and returns promptly once it arrives. It is a variable only so tests can wait +// on the loop without sleeping for real. +var serviceReadyPollInterval = 250 * time.Millisecond + +// readyProbe reports whether a service is ready to be configured. An error is a +// probe that could not reach the service, which during boot is indistinguishable +// from a service that has not started yet; it is therefore not fatal on its own +// and is only surfaced if the wait runs out. +type readyProbe func() (bool, error) + +// waitForServiceReady blocks until probe reports the named service ready, the +// timeout elapses, or ctx is cancelled. +// +// Some backends are configured through a running daemon rather than a file, so +// a configurator constructed before that daemon is up would hold a handle that +// reports nothing and accepts no changes. This program can be started early +// enough in boot to lose that race — from a unit ordered alongside the daemon +// rather than after it — which is what the wait is for. +// +// A timeout of zero or less does not wait: probe runs once and its result is +// reported, so a caller already ordered after the daemon fails fast instead of +// blocking. A service that is already up returns on the first probe and pays no +// delay either way. +func waitForServiceReady(ctx context.Context, name string, timeout time.Duration, probe readyProbe) error { + if timeout > 0 { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, timeout) + defer cancel() + } + + ticker := time.NewTicker(serviceReadyPollInterval) + defer ticker.Stop() + + waited := false + for { + ready, probeErr := probe() + if ready { + if waited { + logger.Printf("%s: became ready", name) + } + return nil + } + + // Not waiting: report why the single probe said no. + if timeout <= 0 { + if probeErr != nil { + return fmt.Errorf("%s is not ready: %w", name, probeErr) + } + return fmt.Errorf("%s is not ready", name) + } + + if !waited { + waited = true + logger.Printf("%s: waiting up to %s for the service to become ready", name, timeout) + } + + select { + case <-ctx.Done(): + // The last probe error explains the wait better than "deadline + // exceeded" does, so prefer it when there was one. + if probeErr != nil { + return fmt.Errorf("timed out waiting for %s: %w", name, probeErr) + } + return fmt.Errorf("timed out waiting for %s: %w", name, ctx.Err()) + case <-ticker.C: + } + } +} diff --git a/services_linux.go b/services_linux.go new file mode 100644 index 0000000..59daf97 --- /dev/null +++ b/services_linux.go @@ -0,0 +1,267 @@ +//go:build linux + +package netconfig + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "strings" + + dbus "github.com/coreos/go-systemd/dbus" +) + +// rcRunlevels are the SysV runlevels a network service can be started in. +// Runlevels 0, 1, and 6 are halt, single-user, and reboot, so nothing is +// registered to start there. +var rcRunlevels = []string{"2", "3", "4", "5"} + +// systemdActive reports whether systemd is the running init (/run/systemd/system +// exists), the same marker systemctl uses to decide it can reach the manager. +// It is checked before dialing D-Bus so hosts running Upstart (Ubuntu 14.04) or +// SysVinit (CentOS 5/6) skip a connection that cannot usefully succeed. +func systemdActive() bool { + _, err := os.Stat("/run/systemd/system") + return err == nil +} + +// commandExists reports whether name resolves on PATH. +func commandExists(name string) bool { + _, err := exec.LookPath(name) + return err == nil +} + +// chkconfigOn reports whether `chkconfig --list name` shows any runlevel on, +// the RHEL-family enablement signal. Returns false when chkconfig is absent. +func chkconfigOn(ctx context.Context, name string) bool { + if !commandExists("chkconfig") { + return false + } + results, err := runCommand(ctx, "chkconfig", "--list", name) + if err != nil { + return false + } + for _, line := range results { + fields := strings.Fields(line) + if len(fields) == 0 || fields[0] != name { + continue + } + for _, f := range fields[1:] { + if _, status, found := strings.Cut(f, ":"); found && status == "on" { + return true + } + } + } + return false +} + +// rcRunlevelDirs returns the rcN.d directories under root that a start symlink +// can live in, covering both the Debian/Ubuntu layout (/etc/rcN.d) and the +// Slackware one (/etc/rc.d/rcN.d). +func rcRunlevelDirs(root string) []string { + dirs := make([]string, 0, len(rcRunlevels)*2) + for _, rl := range rcRunlevels { + dirs = append(dirs, + filepath.Join(root, "etc", "rc"+rl+".d"), + filepath.Join(root, "etc", "rc.d", "rc"+rl+".d"), + ) + } + return dirs +} + +// rcSymlinksOn reports whether an S*name start symlink exists in any rcN.d tree, +// the enablement signal for both update-rc.d (Debian/Ubuntu) and Slackware. +func rcSymlinksOn(name string) bool { + return rcSymlinksOnIn("/", name) +} + +// rcSymlinksOnIn is rcSymlinksOn rooted at root, so tests can build a runlevel +// tree in a temporary directory. +func rcSymlinksOnIn(root, name string) bool { + for _, dir := range rcRunlevelDirs(root) { + if matches, _ := filepath.Glob(filepath.Join(dir, "S*"+name)); len(matches) > 0 { + return true + } + } + return false +} + +// openrcOn reports whether name is linked into an OpenRC runlevel directory +// (default or boot), the enablement signal created by `rc-update add` on Gentoo. +func openrcOn(name string) bool { + return openrcOnIn("/", name) +} + +// openrcOnIn is openrcOn rooted at root, so tests can build a runlevel tree in a +// temporary directory. +func openrcOnIn(root, name string) bool { + for _, rl := range []string{"default", "boot"} { + if _, err := os.Lstat(filepath.Join(root, "etc", "runlevels", rl, name)); err == nil { + return true + } + } + return false +} + +// sysvServiceEnabled reports whether name is registered to start at boot under +// a SysV-family init system: chkconfig (RHEL), update-rc.d rcN.d start symlinks +// (Debian/Ubuntu/Slackware), or an OpenRC runlevel (Gentoo). name is the service +// base name without a ".service" suffix. Any one mechanism reporting it on +// counts as enabled. +func sysvServiceEnabled(ctx context.Context, name string) bool { + return chkconfigOn(ctx, name) || rcSymlinksOn(name) || openrcOn(name) +} + +// initSystemDiscoverable reports whether this host exposes any init system that +// sysvServiceEnabled or systemd can be asked about. It is false on minimal +// images and containers that have neither systemd nor chkconfig nor runlevel +// directories, where a backend's configuration file is the only evidence +// available and detection has nothing to corroborate it with. +func initSystemDiscoverable() bool { + if systemdActive() || commandExists("chkconfig") { + return true + } + for _, dir := range rcRunlevelDirs("/") { + if _, err := os.Stat(dir); err == nil { + return true + } + } + _, err := os.Stat("/etc/runlevels") + return err == nil +} + +// networkServiceUnits are the services backend detection asks systemd about. +// They are collected here so the unit-file lookup can be filtered server side +// to just these names, which is orders of magnitude cheaper than listing every +// unit file on the host. Only these names may be passed to initState.enabled +// and initState.detected; any other name silently skips the systemd answer and +// falls through to the SysV mechanisms. +var networkServiceUnits = []string{"systemd-networkd", "NetworkManager", "network", "networking"} + +// initState is a snapshot of what the host's init system reports about its +// units, taken once when the configurator is constructed. Each systemd query +// costs a D-Bus round trip, so the running units and the installed unit files +// are both fetched up front and every per-service question is answered from +// memory afterwards. +type initState struct { + // active holds the unit names ("NetworkManager.service") systemd reports as + // currently running. Empty when systemd could not be queried. + active map[string]bool + // unitFiles maps an installed unit file's base name to its UnitFileState + // ("enabled", "disabled", "generated", ...). A unit that is enabled but was + // never loaded this boot appears here and not in active, which is why both + // are read. Empty when systemd could not be queried. + unitFiles map[string]string +} + +// newInitState queries systemd for the host's running units and installed unit +// files. Every step degrades to an empty map rather than an error: a host that +// cannot answer over D-Bus is not broken, it is a host whose services must be +// discovered through the SysV mechanisms instead, which enabled falls back to. +// The queries are bound by ctx so a wedged systemd cannot stall construction. +func newInitState(ctx context.Context) *initState { + s := &initState{ + active: make(map[string]bool), + unitFiles: make(map[string]string), + } + + // Skip D-Bus entirely when systemd is not the running init. On Ubuntu 14.04 + // Upstart is PID 1 and org.freedesktop.systemd1 is served by systemd-shim, + // which accepts the connection but has no ListUnits; on CentOS 5/6 there is + // no systemd1 on the bus at all. Both would otherwise cost a failed dial and + // a failed call before reaching the SysV checks that were always going to + // answer for them. + if !systemdActive() { + return s + } + + conn, err := dbus.NewWithContext(ctx) + if err != nil { + logger.Printf("unable to connect to systemd, falling back to init-script detection: %v", err) + return s + } + defer conn.Close() + + units, err := conn.ListUnitsContext(ctx) + if err != nil { + logger.Printf("unable to list systemd units, falling back to init-script detection: %v", err) + } + for _, unit := range units { + if unit.ActiveState == "active" { + s.active[unit.Name] = true + } + } + + // The unit files report enablement, including for units that are enabled but + // have not been loaded this boot and so never appear in ListUnits. + // + // ListUnitFilesByPatterns filters server side and answers in milliseconds; + // ListUnitFiles walks every unit file on the host and can take seconds on a + // machine with a few hundred of them. Only systemd 230 and later export the + // filtered call, so fall back to the full listing for older systemd (CentOS + // 7 ships 219). Both are read into the same map keyed by unit file name. + patterns := make([]string, 0, len(networkServiceUnits)) + for _, name := range networkServiceUnits { + patterns = append(patterns, name+".service") + } + files, err := conn.ListUnitFilesByPatternsContext(ctx, nil, patterns) + if err != nil { + files, err = conn.ListUnitFilesContext(ctx) + } + if err != nil { + logger.Printf("unable to list systemd unit files, falling back to init-script detection: %v", err) + return s + } + for _, uf := range files { + s.unitFiles[filepath.Base(uf.Path)] = uf.Type + } + return s +} + +// running reports whether systemd has name's service unit active right now. +func (s *initState) running(name string) bool { + return s.active[name+".service"] +} + +// enabled reports whether name is registered to start at boot. systemd's +// UnitFileState is authoritative when it owns the unit outright; a "generated" +// state is not, because systemd-sysv-generator wraps an /etc/init.d script into +// a unit that carries no enablement of its own. RHEL 7 ships network.service +// that way whether or not chkconfig has it on, so a generated unit's real +// enablement is read from its SysV registration, as is any unit systemd has +// never heard of. +func (s *initState) enabled(ctx context.Context, name string) bool { + state, known := s.unitFiles[name+".service"] + if known && state != "generated" { + return state == "enabled" || state == "enabled-runtime" + } + return sysvServiceEnabled(ctx, name) +} + +// detected reports whether name's service manages this host's network: either +// it is running now, or it is registered to start at boot. Enablement counts +// independently of the running state because this package writes configuration +// that must survive a reboot. A manager that is enabled but not yet started — +// a host mid-provision, a chroot, an image build, a unit whose start failed — +// still owns the network on the next boot, and systemd's ListUnits does not +// report it at all. +func (s *initState) detected(ctx context.Context, name string) bool { + return s.running(name) || s.enabled(ctx, name) +} + +// networkManagerRunning reports whether NetworkManager's daemon left a runtime +// pid file. It is the running-state signal on hosts where systemd cannot be +// queried, and is unioned with the systemd signals rather than replacing them, +// so a partial answer from systemd never hides a manager this can still see. +func networkManagerRunning() bool { + for _, pidFile := range []string{ + "/var/run/NetworkManager/NetworkManager.pid", + "/run/NetworkManager/NetworkManager.pid", + } { + if _, err := os.Stat(pidFile); err == nil { + return true + } + } + return false +} diff --git a/services_linux_unit_test.go b/services_linux_unit_test.go new file mode 100644 index 0000000..c643732 --- /dev/null +++ b/services_linux_unit_test.go @@ -0,0 +1,168 @@ +//go:build linux + +package netconfig + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// absentService is a service name no host can plausibly have registered, so +// sysvServiceEnabled is guaranteed to answer false for it. It lets the systemd +// branches of initState.enabled be tested without the SysV fallback ever +// masking the result. +const absentService = "netconfig-nonexistent-service" + +// A unit file state systemd owns outright is authoritative: only "enabled" and +// "enabled-runtime" count, and the SysV fallback is never consulted. +func TestInitStateEnabledTrustsSystemdUnitFileState(t *testing.T) { + ctx := context.Background() + cases := map[string]bool{ + "enabled": true, + "enabled-runtime": true, + "disabled": false, + "static": false, + "masked": false, + } + for state, want := range cases { + s := &initState{ + active: map[string]bool{}, + unitFiles: map[string]string{absentService + ".service": state}, + } + assert.Equal(t, want, s.enabled(ctx, absentService), + "unit file state %q", state) + } +} + +// A "generated" unit is a systemd-sysv-generator wrapper around an init.d +// script. It carries no enablement of its own, so it must not be read as +// enabled — RHEL 7 ships network.service that way whether or not chkconfig has +// it on. Enablement has to come from the SysV registration, which for a service +// no host has is absent. +func TestInitStateEnabledDoesNotTrustGeneratedUnits(t *testing.T) { + s := &initState{ + active: map[string]bool{}, + unitFiles: map[string]string{absentService + ".service": "generated"}, + } + assert.False(t, s.enabled(context.Background(), absentService), + "a generated unit with no SysV registration must not read as enabled") +} + +// A service systemd has never heard of falls through to the SysV mechanisms +// rather than being reported as disabled. +func TestInitStateEnabledFallsBackForUnknownUnits(t *testing.T) { + s := &initState{active: map[string]bool{}, unitFiles: map[string]string{}} + assert.False(t, s.enabled(context.Background(), absentService)) +} + +// detected is the union of running and enabled. Enablement must count on its +// own: a manager that is enabled but has not started this boot never appears in +// systemd's ListUnits, yet it owns the network after the next reboot and its +// configuration still has to be written. Equally, a manager that is running but +// disabled owns the network right now. +func TestInitStateDetectedUnionsRunningAndEnabled(t *testing.T) { + ctx := context.Background() + unit := absentService + ".service" + + enabledNotRunning := &initState{ + active: map[string]bool{}, + unitFiles: map[string]string{unit: "enabled"}, + } + assert.True(t, enabledNotRunning.detected(ctx, absentService), + "an enabled service that is not running must be detected") + + runningNotEnabled := &initState{ + active: map[string]bool{unit: true}, + unitFiles: map[string]string{unit: "disabled"}, + } + assert.True(t, runningNotEnabled.detected(ctx, absentService), + "a running service that is not enabled must be detected") + + neither := &initState{ + active: map[string]bool{}, + unitFiles: map[string]string{unit: "disabled"}, + } + assert.False(t, neither.detected(ctx, absentService)) +} + +// running reads systemd's active-unit set, keyed by the full unit name. +func TestInitStateRunning(t *testing.T) { + s := &initState{ + active: map[string]bool{"NetworkManager.service": true}, + unitFiles: map[string]string{}, + } + assert.True(t, s.running("NetworkManager")) + assert.False(t, s.running("systemd-networkd")) +} + +// An S* start symlink in any rcN.d tree marks the service as enabled, in both +// the Debian/Ubuntu (/etc/rcN.d) and Slackware (/etc/rc.d/rcN.d) layouts. A K* +// kill symlink does not. +func TestRcSymlinksOnIn(t *testing.T) { + for _, dir := range []string{"etc/rc2.d", "etc/rc.d/rc3.d", "etc/rc5.d"} { + t.Run(dir, func(t *testing.T) { + root := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(root, dir), 0o755)) + require.NoError(t, os.WriteFile( + filepath.Join(root, dir, "S01networking"), nil, 0o644)) + assert.True(t, rcSymlinksOnIn(root, "networking")) + assert.False(t, rcSymlinksOnIn(root, "network"), + "S01networking must not match the distinct service %q", "network") + }) + } + + // A kill symlink means the service is stopped in that runlevel, not started. + root := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(root, "etc/rc2.d"), 0o755)) + require.NoError(t, os.WriteFile( + filepath.Join(root, "etc/rc2.d", "K01networking"), nil, 0o644)) + assert.False(t, rcSymlinksOnIn(root, "networking")) + + // Runlevels 0, 1, and 6 are halt, single-user, and reboot; nothing starts there. + root = t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(root, "etc/rc1.d"), 0o755)) + require.NoError(t, os.WriteFile( + filepath.Join(root, "etc/rc1.d", "S01networking"), nil, 0o644)) + assert.False(t, rcSymlinksOnIn(root, "networking")) + + assert.False(t, rcSymlinksOnIn(t.TempDir(), "networking"), + "an empty root has no enabled services") +} + +// OpenRC records enablement as a symlink into a runlevel directory. +func TestOpenrcOnIn(t *testing.T) { + for _, rl := range []string{"default", "boot"} { + t.Run(rl, func(t *testing.T) { + root := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(root, "etc/runlevels", rl), 0o755)) + require.NoError(t, os.Symlink("/etc/init.d/net.lo", + filepath.Join(root, "etc/runlevels", rl, "net.lo"))) + assert.True(t, openrcOnIn(root, "net.lo")) + assert.False(t, openrcOnIn(root, "NetworkManager")) + }) + } + + // A dangling symlink still marks the service as added to the runlevel, so + // Lstat (not Stat) is what openrcOnIn must use. + root := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(root, "etc/runlevels/default"), 0o755)) + require.NoError(t, os.Symlink("/nonexistent", + filepath.Join(root, "etc/runlevels/default/dangling"))) + assert.True(t, openrcOnIn(root, "dangling")) + + assert.False(t, openrcOnIn(t.TempDir(), "net.lo")) +} + +// Every service name detection asks systemd about must be in networkServiceUnits, +// since that list is what the unit-file lookup is filtered by. A name missing +// from it silently loses its systemd answer. +func TestNetworkServiceUnitsCoversDetectedServices(t *testing.T) { + for _, name := range []string{"systemd-networkd", "NetworkManager", "network", "networking"} { + assert.Contains(t, networkServiceUnits, name) + } +} diff --git a/services_unit_test.go b/services_unit_test.go new file mode 100644 index 0000000..2c659fb --- /dev/null +++ b/services_unit_test.go @@ -0,0 +1,122 @@ +package netconfig + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// fastPolling shrinks the readiness poll interval for the duration of a test, +// so the wait loop can be exercised without sleeping for real. +func fastPolling(t *testing.T) { + t.Helper() + original := serviceReadyPollInterval + serviceReadyPollInterval = time.Millisecond + t.Cleanup(func() { serviceReadyPollInterval = original }) +} + +// countingProbe reports ready only from the nth call onwards, and records how +// many times it was asked. +func countingProbe(readyOn int, err error) (readyProbe, *int) { + calls := 0 + probe := func() (bool, error) { + calls++ + if readyOn > 0 && calls >= readyOn { + return true, nil + } + return false, err + } + return probe, &calls +} + +func TestWaitForServiceReady(t *testing.T) { + t.Run("a service already up returns on the first probe", func(t *testing.T) { + fastPolling(t) + probe, calls := countingProbe(1, nil) + + err := waitForServiceReady(context.Background(), "svc", time.Minute, probe) + require.NoError(t, err) + assert.Equal(t, 1, *calls, "a ready service must not be polled twice") + }) + + t.Run("waits until the service arrives", func(t *testing.T) { + fastPolling(t) + probe, calls := countingProbe(4, errors.New("no bus yet")) + + err := waitForServiceReady(context.Background(), "svc", time.Minute, probe) + require.NoError(t, err) + assert.Equal(t, 4, *calls) + }) + + t.Run("a probe error while booting is not fatal", func(t *testing.T) { + fastPolling(t) + // The first probes fail with the error a host that has not started + // dbus-daemon yet would give; the service then arrives. + probe, _ := countingProbe(3, errors.New("dial unix /run/dbus: no such file")) + + assert.NoError(t, waitForServiceReady(context.Background(), "svc", time.Minute, probe)) + }) + + t.Run("times out and reports the last probe error", func(t *testing.T) { + fastPolling(t) + probeErr := errors.New("name has no owner") + probe, calls := countingProbe(0, probeErr) + + err := waitForServiceReady(context.Background(), "NetworkManager", 20*time.Millisecond, probe) + require.Error(t, err) + assert.ErrorIs(t, err, probeErr, "the probe error explains the wait better than a deadline does") + assert.ErrorContains(t, err, "timed out waiting for NetworkManager") + assert.Greater(t, *calls, 1, "the probe must be retried before giving up") + }) + + t.Run("times out and reports the deadline when the probe never errored", func(t *testing.T) { + fastPolling(t) + probe, _ := countingProbe(0, nil) + + err := waitForServiceReady(context.Background(), "svc", 20*time.Millisecond, probe) + require.Error(t, err) + assert.ErrorIs(t, err, context.DeadlineExceeded) + }) + + t.Run("a cancelled context aborts the wait", func(t *testing.T) { + fastPolling(t) + ctx, cancel := context.WithCancel(context.Background()) + cancel() + probe, _ := countingProbe(0, nil) + + err := waitForServiceReady(ctx, "svc", time.Minute, probe) + require.Error(t, err) + assert.ErrorIs(t, err, context.Canceled) + }) + + t.Run("zero timeout probes once and does not wait", func(t *testing.T) { + probe, calls := countingProbe(0, nil) + + start := time.Now() + err := waitForServiceReady(context.Background(), "svc", 0, probe) + require.Error(t, err) + assert.ErrorContains(t, err, "svc is not ready") + assert.Equal(t, 1, *calls, "a zero timeout must not retry") + assert.Less(t, time.Since(start), serviceReadyPollInterval, "a zero timeout must not sleep") + }) + + t.Run("zero timeout surfaces the probe error", func(t *testing.T) { + probeErr := errors.New("name has no owner") + probe, _ := countingProbe(0, probeErr) + + err := waitForServiceReady(context.Background(), "svc", 0, probe) + require.Error(t, err) + assert.ErrorIs(t, err, probeErr) + }) + + t.Run("zero timeout still succeeds when the service is up", func(t *testing.T) { + probe, calls := countingProbe(1, nil) + + assert.NoError(t, waitForServiceReady(context.Background(), "svc", 0, probe)) + assert.Equal(t, 1, *calls) + }) +} diff --git a/tests/networkManager/results/1/test b/tests/networkManager/results/1/test index 71f2f87..ebc2f46 100644 --- a/tests/networkManager/results/1/test +++ b/tests/networkManager/results/1/test @@ -1,5 +1,5 @@ connection modify main ipv4.method manual ipv4.addresses 1.2.3.4/24,1.2.3.43/24 ipv4.gateway 1.2.3.1 -connection modify main ipv6.method manual ipv6.addresses fc00::2/64 ipv6.gateway fc00::1 +connection modify main ipv6.method auto ipv6.addresses fc00::2/64 ipv6.gateway fc00::1 connection modify test ipv4.routes 10.253.2.0/24 203.0.113.22 100 connection modify test ipv6.routes abcd:ef12:3455:10::/64 abcd:ef12:3456:10::1 100 connection modify test_eth0 ipv4.dns 8.8.8.8,1.1.1.1 ipv4.dns-search example.com ipv4.ignore-auto-dns yes diff --git a/tests/networkManager/results/2/test b/tests/networkManager/results/2/test index 38af30a..06ead3b 100644 --- a/tests/networkManager/results/2/test +++ b/tests/networkManager/results/2/test @@ -1,10 +1,10 @@ connection modify main ipv4.method manual ipv4.addresses 1.2.3.4/24,1.2.3.43/24 ipv4.gateway 1.2.3.1 -connection modify main ipv6.method manual ipv6.addresses fc00::2/64 ipv6.gateway fc00::1 +connection modify main ipv6.method auto ipv6.addresses fc00::2/64 ipv6.gateway fc00::1 connection modify test ipv4.routes 10.253.2.0/24 203.0.113.22 100 connection modify test ipv6.routes abcd:ef12:3455:10::/64 abcd:ef12:3456:10::1 100 connection modify test_eth0 ipv4.dns 8.8.8.8,1.1.1.1 ipv4.dns-search example.com ipv4.ignore-auto-dns yes connection modify test_eth0 ipv6.dns 2001:4860:4860::8888 ipv6.dns-search example.com ipv6.ignore-auto-dns yes -connection modify test_eth0 ipv4.method manual ipv4.addresses 1.2.10.4/24 ipv4.gateway 1.2.10.254 -connection modify test_eth0 ipv6.method link-local ipv6.addresses ipv6.gateway +connection modify test_eth0 ipv4.method auto ipv4.addresses 1.2.10.4/24 ipv4.gateway 1.2.10.254 +connection modify test_eth0 ipv6.method auto ipv6.addresses ipv6.gateway connection modify main ipv4.routes connection modify main ipv6.routes diff --git a/utils.go b/utils.go index ef556d3..efe742b 100644 --- a/utils.go +++ b/utils.go @@ -111,6 +111,7 @@ func ip2Uint(ip net.IP) uint32 { // cancelled or expired context terminates it. func runCommand(ctx context.Context, command string, args ...string) (out []string, err error) { cmd := exec.CommandContext(ctx, command, args...) + hideCommandWindow(cmd) // Get output pipes. var stdout, stderr io.ReadCloser @@ -168,6 +169,48 @@ func runCommand(ctx context.Context, command string, args ...string) (out []stri return } +// naturalLess compares two strings the way a person reads them, comparing a run +// of digits by the number it spells rather than character by character, so eth2 +// sorts before eth10 and enp0s3 before enp0s10. Digit runs are compared without +// being parsed, so a name carrying a number too large for an integer still +// orders sensibly. +func naturalLess(a, b string) bool { + for a != "" && b != "" { + if isDigit(a[0]) && isDigit(b[0]) { + var aNum, bNum string + aNum, a = leadingDigits(a) + bNum, b = leadingDigits(b) + // Leading zeros change the text but not the value. + aNum = strings.TrimLeft(aNum, "0") + bNum = strings.TrimLeft(bNum, "0") + if len(aNum) != len(bNum) { + return len(aNum) < len(bNum) + } + if aNum != bNum { + return aNum < bNum + } + continue + } + if a[0] != b[0] { + return a[0] < b[0] + } + a, b = a[1:], b[1:] + } + return len(a) < len(b) +} + +// isDigit reports whether c is an ASCII digit. +func isDigit(c byte) bool { return c >= '0' && c <= '9' } + +// leadingDigits splits s into its leading run of digits and the rest. +func leadingDigits(s string) (digits, rest string) { + i := 0 + for i < len(s) && isDigit(s[i]) { + i++ + } + return s[:i], s[i:] +} + // ipStrings converts a list of IPs to their string form, skipping nil entries. func ipStrings(ips []net.IP) []string { var out []string diff --git a/utils_other.go b/utils_other.go new file mode 100644 index 0000000..b245d67 --- /dev/null +++ b/utils_other.go @@ -0,0 +1,9 @@ +//go:build !windows + +package netconfig + +import "os/exec" + +// hideCommandWindow is a no-op away from Windows, where there is no console +// window for a subprocess to raise. +func hideCommandWindow(*exec.Cmd) {} diff --git a/utils_unit_test.go b/utils_unit_test.go index d057196..f806300 100644 --- a/utils_unit_test.go +++ b/utils_unit_test.go @@ -6,10 +6,11 @@ import ( "net" "os" "path/filepath" - "reflect" "sort" "testing" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "gopkg.in/yaml.v3" ) @@ -19,17 +20,11 @@ func TestIP2UintRoundTrip(t *testing.T) { cases := []string{"0.0.0.0", "1.2.3.4", "192.168.1.255", "255.255.255.255"} for _, c := range cases { ip := net.ParseIP(c) - if ip == nil { - t.Fatalf("could not parse %q", c) - } + require.NotNil(t, ip, "could not parse %q", c) got := uint2IP(ip2Uint(ip)) - if !got.Equal(ip) { - t.Errorf("round trip of %s: got %s", c, got) - } + assert.True(t, got.Equal(ip), "round trip of %s: got %s", c, got) // uint2IP should produce the canonical four-byte representation. - if len(got) != net.IPv4len { - t.Errorf("uint2IP(%s) length = %d, want %d", c, len(got), net.IPv4len) - } + assert.Equal(t, net.IPv4len, len(got), "uint2IP(%s) length", c) } } @@ -38,13 +33,9 @@ func TestIP2UintRoundTrip(t *testing.T) { func TestIP2UintAcceptsBothRepresentations(t *testing.T) { ip16 := net.ParseIP("10.20.30.40") ip4 := ip16.To4() - if ip4 == nil { - t.Fatal("expected an IPv4 address") - } - if ip2Uint(ip16) != ip2Uint(ip4) { - t.Errorf("ip2Uint disagrees for 16-byte (%d) and 4-byte (%d) forms", - ip2Uint(ip16), ip2Uint(ip4)) - } + require.NotNil(t, ip4, "expected an IPv4 address") + assert.Equal(t, ip2Uint(ip16), ip2Uint(ip4), + "ip2Uint disagrees for 16-byte and 4-byte forms") } func TestAllFF(t *testing.T) { @@ -60,9 +51,7 @@ func TestAllFF(t *testing.T) { {[]byte{0xff, 0xff, 0xfe}, false}, } for _, c := range cases { - if got := allFF(c.in); got != c.want { - t.Errorf("allFF(%v) = %v, want %v", c.in, got, c.want) - } + assert.Equal(t, c.want, allFF(c.in), "allFF(%v)", c.in) } } @@ -79,14 +68,10 @@ func TestGetBroadcast(t *testing.T) { } for _, c := range cases { _, network, err := net.ParseCIDR(c.cidr) - if err != nil { - t.Fatalf("ParseCIDR(%q): %v", c.cidr, err) - } + require.NoError(t, err, "ParseCIDR(%q)", c.cidr) got := getBroadcast(network) want := net.ParseIP(c.want) - if !got.Equal(want) { - t.Errorf("getBroadcast(%s) = %s, want %s", c.cidr, got, c.want) - } + assert.True(t, got.Equal(want), "getBroadcast(%s) = %s, want %s", c.cidr, got, c.want) } } @@ -98,243 +83,193 @@ func TestGetBroadcastMixedFamily(t *testing.T) { Mask: net.CIDRMask(96+24, 128), // /24 expressed as a 16-byte mask. } got := getBroadcast(network) - if want := net.ParseIP("192.168.1.255"); !got.Equal(want) { - t.Errorf("getBroadcast mixed family = %s, want %s", got, want) - } + want := net.ParseIP("192.168.1.255") + assert.True(t, got.Equal(want), "getBroadcast mixed family = %s, want %s", got, want) } func TestSortableDirEntries(t *testing.T) { dir := t.TempDir() names := []string{"zebra", "alpha", "mike", "bravo"} for _, n := range names { - if err := os.WriteFile(filepath.Join(dir, n), nil, 0o644); err != nil { - t.Fatal(err) - } + require.NoError(t, os.WriteFile(filepath.Join(dir, n), nil, 0o644)) } entries, err := os.ReadDir(dir) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Shuffle into a known unsorted order before sorting. sort.Slice(entries, func(i, j int) bool { return entries[i].Name() > entries[j].Name() }) sortable := sortableDirEntries(entries) - if sortable.Len() != len(names) { - t.Errorf("Len() = %d, want %d", sortable.Len(), len(names)) - } + assert.Equal(t, len(names), sortable.Len(), "Len()") sort.Sort(sortable) want := []string{"alpha", "bravo", "mike", "zebra"} for i, w := range want { - if entries[i].Name() != w { - t.Errorf("sorted[%d] = %s, want %s", i, entries[i].Name(), w) - } + assert.Equal(t, w, entries[i].Name(), "sorted[%d]", i) } } func TestIntStringYAML(t *testing.T) { t.Run("int", func(t *testing.T) { var v intString - if err := yaml.Unmarshal([]byte("42\n"), &v); err != nil { - t.Fatal(err) - } - if v.Int == nil || *v.Int != 42 { - t.Fatalf("Int = %v, want 42", v.Int) - } - if v.String != nil { - t.Errorf("String should be nil, got %v", *v.String) - } + require.NoError(t, yaml.Unmarshal([]byte("42\n"), &v)) + require.NotNil(t, v.Int, "Int = %v, want 42", v.Int) + assert.Equal(t, 42, *v.Int) + assert.Nil(t, v.String, "String should be nil") out, err := yaml.Marshal(v) - if err != nil { - t.Fatal(err) - } - if got := string(out); got != "42\n" { - t.Errorf("marshal = %q, want %q", got, "42\n") - } + require.NoError(t, err) + assert.Equal(t, "42\n", string(out), "marshal") }) t.Run("string", func(t *testing.T) { var v intString - if err := yaml.Unmarshal([]byte("auto\n"), &v); err != nil { - t.Fatal(err) - } - if v.String == nil || *v.String != "auto" { - t.Fatalf("String = %v, want auto", v.String) - } - if v.Int != nil { - t.Errorf("Int should be nil, got %v", *v.Int) - } + require.NoError(t, yaml.Unmarshal([]byte("auto\n"), &v)) + require.NotNil(t, v.String, "String = %v, want auto", v.String) + assert.Equal(t, "auto", *v.String) + assert.Nil(t, v.Int, "Int should be nil") out, err := yaml.Marshal(v) - if err != nil { - t.Fatal(err) - } - if got := string(out); got != "auto\n" { - t.Errorf("marshal = %q, want %q", got, "auto\n") - } + require.NoError(t, err) + assert.Equal(t, "auto\n", string(out), "marshal") }) t.Run("empty marshals to null", func(t *testing.T) { out, err := yaml.Marshal(intString{}) - if err != nil { - t.Fatal(err) - } - if got := string(out); got != "null\n" { - t.Errorf("marshal of empty = %q, want %q", got, "null\n") - } + require.NoError(t, err) + assert.Equal(t, "null\n", string(out), "marshal of empty") }) } func TestIntStringJSON(t *testing.T) { t.Run("int", func(t *testing.T) { var v intString - if err := json.Unmarshal([]byte("42"), &v); err != nil { - t.Fatal(err) - } - if v.Int == nil || *v.Int != 42 { - t.Fatalf("Int = %v, want 42", v.Int) - } + require.NoError(t, json.Unmarshal([]byte("42"), &v)) + require.NotNil(t, v.Int, "Int = %v, want 42", v.Int) + assert.Equal(t, 42, *v.Int) out, err := json.Marshal(v) - if err != nil { - t.Fatal(err) - } - if got := string(out); got != "42" { - t.Errorf("marshal = %q, want %q", got, "42") - } + require.NoError(t, err) + assert.Equal(t, "42", string(out), "marshal") }) t.Run("string", func(t *testing.T) { var v intString - if err := json.Unmarshal([]byte(`"auto"`), &v); err != nil { - t.Fatal(err) - } - if v.String == nil || *v.String != "auto" { - t.Fatalf("String = %v, want auto", v.String) - } + require.NoError(t, json.Unmarshal([]byte(`"auto"`), &v)) + require.NotNil(t, v.String, "String = %v, want auto", v.String) + assert.Equal(t, "auto", *v.String) out, err := json.Marshal(v) - if err != nil { - t.Fatal(err) - } - if got := string(out); got != `"auto"` { - t.Errorf("marshal = %q, want %q", got, `"auto"`) - } + require.NoError(t, err) + assert.Equal(t, `"auto"`, string(out), "marshal") }) t.Run("invalid", func(t *testing.T) { var v intString - if err := json.Unmarshal([]byte("{}"), &v); err == nil { - t.Error("expected error for object input, got nil") - } + err := json.Unmarshal([]byte("{}"), &v) + assert.Error(t, err, "expected error for object input, got nil") }) } func TestRunCommand(t *testing.T) { t.Run("stdout", func(t *testing.T) { out, err := runCommand(context.Background(), "printf", "line1\nline2\n") - if err != nil { - t.Fatalf("unexpected error: %v", err) - } + require.NoError(t, err, "unexpected error") want := []string{"line1", "line2"} - if !reflect.DeepEqual(out, want) { - t.Errorf("out = %v, want %v", out, want) - } + assert.Equal(t, want, out, "out") }) t.Run("nonzero exit", func(t *testing.T) { _, err := runCommand(context.Background(), "false") - if err == nil { - t.Error("expected error for failing command, got nil") - } + assert.Error(t, err, "expected error for failing command, got nil") }) t.Run("missing binary", func(t *testing.T) { _, err := runCommand(context.Background(), "this-binary-should-not-exist-12345") - if err == nil { - t.Error("expected error for missing binary, got nil") - } + assert.Error(t, err, "expected error for missing binary, got nil") }) } func TestTestInternet(t *testing.T) { - if !testInternet(context.Background(), "test_success", 0) { - t.Error("test_success sentinel should report success") - } - if testInternet(context.Background(), "test_fail", 0) { - t.Error("test_fail sentinel should report failure") - } + assert.True(t, testInternet(context.Background(), "test_success", 0), + "test_success sentinel should report success") + assert.False(t, testInternet(context.Background(), "test_fail", 0), + "test_fail sentinel should report failure") } func TestFileCopyAndMove(t *testing.T) { dir := t.TempDir() src := filepath.Join(dir, "src.txt") content := []byte("hello world\n") - if err := os.WriteFile(src, content, 0o640); err != nil { - t.Fatal(err) - } + require.NoError(t, os.WriteFile(src, content, 0o640)) t.Run("fileCopy preserves contents and mode", func(t *testing.T) { dst := filepath.Join(dir, "copy.txt") - if err := fileCopy(src, dst); err != nil { - t.Fatal(err) - } + require.NoError(t, fileCopy(src, dst)) got, err := os.ReadFile(dst) - if err != nil { - t.Fatal(err) - } - if !reflect.DeepEqual(got, content) { - t.Errorf("copied contents = %q, want %q", got, content) - } + require.NoError(t, err) + assert.Equal(t, content, got, "copied contents") fi, err := os.Stat(dst) - if err != nil { - t.Fatal(err) - } - if fi.Mode().Perm() != 0o640 { - t.Errorf("copied mode = %v, want 0640", fi.Mode().Perm()) - } + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o640), fi.Mode().Perm(), "copied mode") }) t.Run("fileCopy missing source errors", func(t *testing.T) { - if err := fileCopy(filepath.Join(dir, "nope"), filepath.Join(dir, "out")); err == nil { - t.Error("expected error for missing source") - } + err := fileCopy(filepath.Join(dir, "nope"), filepath.Join(dir, "out")) + assert.Error(t, err, "expected error for missing source") }) t.Run("fileMoveCopy removes source", func(t *testing.T) { moveSrc := filepath.Join(dir, "movesrc.txt") - if err := os.WriteFile(moveSrc, content, 0o644); err != nil { - t.Fatal(err) - } + require.NoError(t, os.WriteFile(moveSrc, content, 0o644)) dst := filepath.Join(dir, "moved.txt") - if err := fileMoveCopy(moveSrc, dst); err != nil { - t.Fatal(err) - } - if _, err := os.Stat(moveSrc); !os.IsNotExist(err) { - t.Errorf("source should be gone, stat err = %v", err) - } + require.NoError(t, fileMoveCopy(moveSrc, dst)) + _, err := os.Stat(moveSrc) + assert.True(t, os.IsNotExist(err), "source should be gone, stat err = %v", err) got, err := os.ReadFile(dst) - if err != nil { - t.Fatal(err) - } - if !reflect.DeepEqual(got, content) { - t.Errorf("moved contents = %q, want %q", got, content) - } + require.NoError(t, err) + assert.Equal(t, content, got, "moved contents") }) t.Run("fileMove same device renames", func(t *testing.T) { moveSrc := filepath.Join(dir, "rename-src.txt") - if err := os.WriteFile(moveSrc, content, 0o644); err != nil { - t.Fatal(err) - } + require.NoError(t, os.WriteFile(moveSrc, content, 0o644)) dst := filepath.Join(dir, "renamed.txt") - if err := fileMove(moveSrc, dst); err != nil { - t.Fatal(err) - } - if _, err := os.Stat(moveSrc); !os.IsNotExist(err) { - t.Errorf("source should be gone after move, stat err = %v", err) - } - if _, err := os.Stat(dst); err != nil { - t.Errorf("destination missing after move: %v", err) - } + require.NoError(t, fileMove(moveSrc, dst)) + _, err := os.Stat(moveSrc) + assert.True(t, os.IsNotExist(err), "source should be gone after move, stat err = %v", err) + _, err = os.Stat(dst) + assert.NoError(t, err, "destination missing after move") }) } + +func TestNaturalLess(t *testing.T) { + cases := []struct { + a, b string + want bool + why string + }{ + {"eth2", "eth10", true, "digit runs compare by value, not character"}, + {"eth10", "eth2", false, "reversed"}, + {"eth0", "eth1", true, "same width digits"}, + {"enp0s3", "enp0s10", true, "embedded digit runs"}, + {"eth0", "eth0", false, "equal strings are not less"}, + {"eth", "eth0", true, "a prefix sorts before what extends it"}, + {"eth0", "eth", false, "reversed prefix"}, + {"eth007", "eth7", false, "leading zeros do not change the value"}, + {"eth7", "eth007", false, "reversed equal values"}, + {"eth1", "wlan0", true, "letters compare by byte"}, + {"9", "10", true, "bare numbers"}, + {"eth99999999999999999999", "eth999999999999999999999", true, "digit runs too long to parse"}, + } + for _, c := range cases { + assert.Equal(t, c.want, naturalLess(c.a, c.b), "naturalLess(%q, %q): %s", c.a, c.b, c.why) + } +} + +func TestLeadingDigits(t *testing.T) { + digits, rest := leadingDigits("10s3") + assert.Equal(t, "10", digits) + assert.Equal(t, "s3", rest) + + digits, rest = leadingDigits("eth0") + assert.Empty(t, digits) + assert.Equal(t, "eth0", rest) +} diff --git a/utils_windows.go b/utils_windows.go new file mode 100644 index 0000000..dc27a0b --- /dev/null +++ b/utils_windows.go @@ -0,0 +1,13 @@ +package netconfig + +import ( + "os/exec" + "syscall" +) + +// hideCommandWindow keeps a subprocess from flashing a console window on a +// desktop session. Every command this package runs on Windows (netsh, ipconfig) +// is a console program, and without this each one briefly steals focus. +func hideCommandWindow(cmd *exec.Cmd) { + cmd.SysProcAttr = &syscall.SysProcAttr{HideWindow: true} +}