From 0c096d5f07c7bcff3cfbd7c716203ddd4db5e64d Mon Sep 17 00:00:00 2001 From: Cuong Manh Le Date: Fri, 11 Aug 2023 16:11:04 +0000 Subject: [PATCH] internal/router: make router.Cleanup idempotent On routers where we want to wait for NTP by checking nvram key. Before waiting, we clean up the router to ensure it's restored to original state. However, router.Cleanup is not idempotent, causing dnsmasq restarted. On tomato/ddwrt, restarting have no delay, and spawning new dnsmasq process immediately. On merlin, somehow it takes time to spawn new dnsmasq process, causing ctrld wrongly think there's no one listening on port 53. Fixing this by ensuring router.Cleanup is idempotent. While at it, also adding "ntp_done" to nvram key, which is now using on latest ddwrt. --- cmd/ctrld/os_linux.go | 10 ++++++++++ internal/router/ddwrt/ddwrt.go | 14 ++++++++------ internal/router/merlin/merlin.go | 19 ++++++++++++++----- internal/router/ntp/ntp.go | 15 +++++++++------ internal/router/tomato/tomato.go | 14 ++++++++------ 5 files changed, 49 insertions(+), 23 deletions(-) diff --git a/cmd/ctrld/os_linux.go b/cmd/ctrld/os_linux.go index e26e396..7a4efae 100644 --- a/cmd/ctrld/os_linux.go +++ b/cmd/ctrld/os_linux.go @@ -24,6 +24,8 @@ import ( "github.com/Control-D-Inc/ctrld/internal/resolvconffile" ) +const resolvConfBackupFailedMsg = "open /etc/resolv.pre-ctrld-backup.conf: read-only file system" + // allocate loopback ip // sudo ip a add 127.0.0.2/24 dev lo func allocateIP(ip string) error { @@ -73,6 +75,14 @@ func setDNS(iface *net.Interface, nameservers []string) error { trySystemdResolve = true break } + // This error happens on read-only file system, which causes ctrld failed to create backup + // for /etc/resolv.conf file. It is ok, because the DNS is still set anyway, and restore + // DNS will fallback to use DHCP if there's no backup /etc/resolv.conf file. + // The error format is controlled by us, so checking for error string is fine. + // See: ../../internal/dns/direct.go:L278 + if r.Mode() == "direct" && strings.Contains(err.Error(), resolvConfBackupFailedMsg) { + return nil + } return err } currentNS := currentDNS(iface) diff --git a/internal/router/ddwrt/ddwrt.go b/internal/router/ddwrt/ddwrt.go index 9f2dc20..edd7e6b 100644 --- a/internal/router/ddwrt/ddwrt.go +++ b/internal/router/ddwrt/ddwrt.go @@ -87,12 +87,14 @@ func (d *Ddwrt) Cleanup() error { if d.cfg.FirstListener().IsDirectDnsListener() { return nil } - if val, _ := nvram.Run("get", nvram.CtrldSetupKey); val == "1" { - nvramKvMap["dnsmasq_options"] = "" - // Restore old configs. - if err := nvram.Restore(nvramKvMap, nvram.CtrldSetupKey); err != nil { - return err - } + if val, _ := nvram.Run("get", nvram.CtrldSetupKey); val != "1" { + return nil // was restored, nothing to do. + } + + nvramKvMap["dnsmasq_options"] = "" + // Restore old configs. + if err := nvram.Restore(nvramKvMap, nvram.CtrldSetupKey); err != nil { + return err } // Restart dnsmasq service. diff --git a/internal/router/merlin/merlin.go b/internal/router/merlin/merlin.go index 5abf8f1..84ebd1c 100644 --- a/internal/router/merlin/merlin.go +++ b/internal/router/merlin/merlin.go @@ -52,6 +52,13 @@ func (m *Merlin) Setup() error { if m.cfg.FirstListener().IsDirectDnsListener() { return nil } + // Already setup. + if val, _ := nvram.Run("get", nvram.CtrldSetupKey); val == "1" { + return nil + } + if _, err := nvram.Run("set", nvram.CtrldSetupKey+"=1"); err != nil { + return err + } buf, err := os.ReadFile(dnsmasq.MerlinPostConfPath) // Already setup. if bytes.Contains(buf, []byte(dnsmasq.MerlinPostConfMarker)) { @@ -92,11 +99,13 @@ func (m *Merlin) Cleanup() error { if m.cfg.FirstListener().IsDirectDnsListener() { return nil } - if val, _ := nvram.Run("get", nvram.CtrldSetupKey); val == "1" { - // Restore old configs. - if err := nvram.Restore(nvramKvMap, nvram.CtrldSetupKey); err != nil { - return err - } + if val, _ := nvram.Run("get", nvram.CtrldSetupKey); val != "1" { + return nil // was restored, nothing to do. + } + + // Restore old configs. + if err := nvram.Restore(nvramKvMap, nvram.CtrldSetupKey); err != nil { + return err } buf, err := os.ReadFile(dnsmasq.MerlinPostConfPath) diff --git a/internal/router/ntp/ntp.go b/internal/router/ntp/ntp.go index a9a9409..5c04a36 100644 --- a/internal/router/ntp/ntp.go +++ b/internal/router/ntp/ntp.go @@ -18,12 +18,15 @@ func WaitNvram() error { // Wait until `ntp_ready=1` set. b := backoff.NewBackoff("ntp.Wait", func(format string, args ...any) {}, 10*time.Second) for { - out, err := nvram.Run("get", "ntp_ready") - if err != nil { - return fmt.Errorf("PreStart: nvram: %w", err) - } - if out == "1" { - return nil + // ddwrt use "ntp_done": https://github.com/mirror/dd-wrt/blob/a08c693527ab3204bf7bebd408a7c9a83b6ede47/src/router/rc/ntp.c#L100 + for _, key := range []string{"ntp_ready", "ntp_done"} { + out, err := nvram.Run("get", key) + if err != nil { + return fmt.Errorf("PreStart: nvram: %w", err) + } + if out == "1" { + return nil + } } b.BackOff(context.Background(), errors.New("ntp not ready")) } diff --git a/internal/router/tomato/tomato.go b/internal/router/tomato/tomato.go index cd8df70..ee5f09b 100644 --- a/internal/router/tomato/tomato.go +++ b/internal/router/tomato/tomato.go @@ -89,12 +89,14 @@ func (f *FreshTomato) Cleanup() error { if f.cfg.FirstListener().IsDirectDnsListener() { return nil } - if val, _ := nvram.Run("get", nvram.CtrldSetupKey); val == "1" { - nvramKvMap["dnsmasq_custom"] = "" - // Restore old configs. - if err := nvram.Restore(nvramKvMap, nvram.CtrldSetupKey); err != nil { - return err - } + if val, _ := nvram.Run("get", nvram.CtrldSetupKey); val != "1" { + return nil // was restored, nothing to do. + } + + nvramKvMap["dnsmasq_custom"] = "" + // Restore old configs. + if err := nvram.Restore(nvramKvMap, nvram.CtrldSetupKey); err != nil { + return err } // Restart dnscrypt-proxy service.