diff --git a/modules/dnsmasq/cmd/dnsmasq-tools/templates_test.go b/modules/dnsmasq/cmd/dnsmasq-tools/templates_test.go index 7d0718b5..2536a2e1 100644 --- a/modules/dnsmasq/cmd/dnsmasq-tools/templates_test.go +++ b/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") + } +} diff --git a/modules/dnsmasq/files/mesh.conf b/modules/dnsmasq/files/mesh.conf index 96d8d7d6..7f86d122 100644 --- a/modules/dnsmasq/files/mesh.conf +++ b/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 -# missing is tried again every ten seconds rather than left failed. +# machine's private address is there: a start that finds it missing fails after at most two seconds and +# 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