Merge pull request 'dnsmasq starts at boot: its drop-in no longer orders after a target the package orders it before (issue 1126)' (#550) from fix/dnsmasq-cycle into main

modules/dnsmasq/cmd/dnsmasq-tools/templates_test.go

@@ -4,6 +4,9 @@ import (
 	"encoding/json"
 	"os"
 	"path/filepath"
+	"regexp"
+	"strconv"
+	"strings"
 	"testing"
 )
 
@@ -62,3 +65,60 @@ func TestEveryFileTakesItsTextFromATemplate(t *testing.T) {
 		t.Errorf("the build packs files/ into %d archives, not one", packs)
 	}
 }
+
+// The packaged dnsmasq.service orders itself Before= network-online.target and nss-lookup.target. A drop-in
+// that orders it After= either of them makes an ordering cycle, and systemd drops the start job: the mesh
+// resolver stayed down for 90 minutes at a boot (novox/hq issue 1126). Wants=, Requires= and BindsTo= are
+// refused too: they pull the target in, and with the packaged ordering they are the same mistake. Because the
+// tunnel is ordered after those targets, the drop-in cannot wait for the address either: a start that finds it
+// missing fails at once and Restart=on-failure brings it up (see the next test).
+func TestDropInDoesNotOrderAfterATargetThePackagedUnitOrdersBefore(t *testing.T) {
+	raw, err := os.ReadFile(filepath.Join(moduleDir, "files", "mesh.conf"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	forbidden := []string{"network-online.target", "nss-lookup.target"}
+	for n, line := range strings.Split(string(raw), "\n") {
+		line = strings.TrimSpace(line)
+		if strings.HasPrefix(line, "#") {
+			continue
+		}
+		key, val, ok := strings.Cut(line, "=")
+		key, val = strings.TrimSpace(key), strings.TrimSpace(val)
+		if !ok || (key != "After" && key != "Requires" && key != "Wants" && key != "BindsTo") {
+			continue
+		}
+		for _, target := range forbidden {
+			if strings.Contains(val, target) {
+				t.Errorf("mesh.conf line %d: %s=%s orders dnsmasq after a target its packaged unit orders itself before: a cycle", n+1, key, val)
+			}
+		}
+	}
+}
+
+// The wait for the address in ExecStartPre is at most two seconds: the tunnel that brings the address is ordered
+// after network-online.target, which the resolver is ordered before, so a longer wait would hold the tunnel back.
+func TestAddressWaitIsAtMostTwoSeconds(t *testing.T) {
+	raw, err := os.ReadFile(filepath.Join(moduleDir, "files", "mesh.conf"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	found := false
+	for _, line := range strings.Split(string(raw), "\n") {
+		line = strings.TrimSpace(line)
+		if !strings.HasPrefix(line, "ExecStartPre=") {
+			continue
+		}
+		m := regexp.MustCompile(`seq 1 (\d+)`).FindStringSubmatch(line)
+		if m == nil {
+			t.Fatalf("ExecStartPre has no bounded wait (seq 1 N): %s", line)
+		}
+		found = true
+		if n, _ := strconv.Atoi(m[1]); n > 2 {
+			t.Errorf("ExecStartPre waits up to %d s for the address, more than 2 s", n)
+		}
+	}
+	if !found {
+		t.Fatal("mesh.conf has no ExecStartPre")
+	}
+}

modules/dnsmasq/files/mesh.conf

@@ -1,14 +1,15 @@
 # Written by the mesh (module dnsmasq, novox/hq issue 348). Replaced on every push.
 #
 # The resolver binds its addresses once, at start (bind-interfaces), so it starts only once this
-# machine's private address is there: it waits up to 80 seconds for it, and a start that found it
+# machine's private address is there: a start that finds it missing fails after at most two seconds and
-# missing is tried again every ten seconds rather than left failed.
+# is tried again every ten seconds (Restart=on-failure, no start limit) until the tunnel has brought it.
+# It does not order itself After= or Wants= network-online.target, and its wait is short: the packaged
+# unit is Before= network-online.target and nss-lookup.target, and the tunnel is ordered after them, so
+# a long wait would hold the tunnel back and a drop-in ordering would be a cycle (issue 1126).
 [Unit]
-After=network-online.target
-Wants=network-online.target
 StartLimitIntervalSec=0
 
 [Service]
-ExecStartPre=/usr/bin/sh -c 'for i in $(seq 1 80); do ip -4 -o addr show to ${machine:address} | grep -q . && exit 0; sleep 1; done; echo "${machine:address} is not on this machine" >&2; exit 1'
+ExecStartPre=/usr/bin/sh -c 'for i in $(seq 1 2); do ip -4 -o addr show to ${machine:address} | grep -q . && exit 0; sleep 1; done; echo "${machine:address} is not on this machine" >&2; exit 1'
 Restart=on-failure
 RestartSec=10s