From 366b3d3f62ddb9686b8296ba81dacfb8a283855a Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Mon, 10 May 2021 10:38:19 -0700 Subject: [PATCH 1/3] ipn{,/ipnserver}: delay JSON marshaling of ipn.Notifies If nobody is connected to the IPN bus, don't burn CPU & waste allocations (causing more GC) by encoding netmaps for nobody. This will notably help hello.ipn.dev. Updates tailscale/corp#1773 Signed-off-by: Brad Fitzpatrick --- ipn/ipnserver/server.go | 47 +++++++++++++++++++++++++++++++++++------ ipn/message.go | 17 +++++---------- ipn/message_test.go | 7 +++++- 3 files changed, 51 insertions(+), 20 deletions(-) diff --git a/ipn/ipnserver/server.go b/ipn/ipnserver/server.go index 0b323dba7..228d9f1cd 100644 --- a/ipn/ipnserver/server.go +++ b/ipn/ipnserver/server.go @@ -6,7 +6,9 @@ import ( "bufio" + "bytes" "context" + "encoding/json" "errors" "fmt" "io" @@ -251,8 +253,7 @@ func (s *server) serveConn(ctx context.Context, c net.Conn, logf logger.Logf) { return } defer c.Close() - serverToClient := func(b []byte) { ipn.WriteMsg(c, b) } - bs := ipn.NewBackendServer(logf, nil, serverToClient) + bs := ipn.NewBackendServer(logf, nil, jsonNotifier(c, s.logf)) _, occupied := err.(inUseOtherUserError) if occupied { bs.SendInUseOtherUserErrorMessage(err.Error()) @@ -567,7 +568,9 @@ func (s *server) setServerModeUserLocked() { } } -func (s *server) writeToClients(b []byte) { +var jsonEscapedZero = []byte(`\u0000`) + +func (s *server) writeToClients(n ipn.Notify) { inServerMode := s.b.InServerMode() s.mu.Lock() @@ -584,8 +587,17 @@ func (s *server) writeToClients(b []byte) { } } - for c := range s.clients { - ipn.WriteMsg(c, b) + if len(s.clients) == 0 { + // Common case (at least on busy servers): nobody + // connected (no GUI, etc), so return before + // serializing JSON. + return + } + + if b, ok := marshalNotify(n, s.logf); ok { + for c := range s.clients { + ipn.WriteMsg(c, b) + } } } @@ -671,8 +683,7 @@ func Run(ctx context.Context, logf logger.Logf, logid string, getEngine func() ( errMsg := err.Error() go func() { defer c.Close() - serverToClient := func(b []byte) { ipn.WriteMsg(c, b) } - bs := ipn.NewBackendServer(logf, nil, serverToClient) + bs := ipn.NewBackendServer(logf, nil, jsonNotifier(c, logf)) bs.SendErrorMessage(errMsg) time.Sleep(time.Second) }() @@ -962,3 +973,25 @@ func peerPid(entries []netstat.Entry, la, ra netaddr.IPPort) int { } return 0 } + +// jsonNotifier returns a notify-writer func that writes ipn.Notify +// messages to w. +func jsonNotifier(w io.Writer, logf logger.Logf) func(ipn.Notify) { + return func(n ipn.Notify) { + if b, ok := marshalNotify(n, logf); ok { + ipn.WriteMsg(w, b) + } + } +} + +func marshalNotify(n ipn.Notify, logf logger.Logf) (b []byte, ok bool) { + b, err := json.Marshal(n) + if err != nil { + logf("ipnserver: [unexpected] error serializing JSON: %v", err) + return nil, false + } + if bytes.Contains(b, jsonEscapedZero) { + logf("[unexpected] zero byte in BackendServer.send notify message: %q", b) + } + return b, true +} diff --git a/ipn/message.go b/ipn/message.go index 7f8d2be21..d8b7d1396 100644 --- a/ipn/message.go +++ b/ipn/message.go @@ -88,9 +88,9 @@ type Command struct { type BackendServer struct { logf logger.Logf - b Backend // the Backend we are serving up - sendNotifyMsg func(jsonMsg []byte) // send a notification message - GotQuit bool // a Quit command was received + b Backend // the Backend we are serving up + sendNotifyMsg func(Notify) // send a notification message + GotQuit bool // a Quit command was received } // NewBackendServer creates a new BackendServer using b. @@ -98,7 +98,7 @@ type BackendServer struct { // If sendNotifyMsg is non-nil, it additionally sets the Backend's // notification callback to call the func with ipn.Notify messages in // JSON form. If nil, it does not change the notification callback. -func NewBackendServer(logf logger.Logf, b Backend, sendNotifyMsg func(b []byte)) *BackendServer { +func NewBackendServer(logf logger.Logf, b Backend, sendNotifyMsg func(Notify)) *BackendServer { bs := &BackendServer{ logf: logf, b: b, @@ -115,14 +115,7 @@ func (bs *BackendServer) send(n Notify) { return } n.Version = version.Long - b, err := json.Marshal(n) - if err != nil { - log.Fatalf("Failed json.Marshal(notify): %v\n%#v", err, n) - } - if bytes.Contains(b, jsonEscapedZero) { - log.Printf("[unexpected] zero byte in BackendServer.send notify message: %q", b) - } - bs.sendNotifyMsg(b) + bs.sendNotifyMsg(n) } func (bs *BackendServer) SendErrorMessage(msg string) { diff --git a/ipn/message_test.go b/ipn/message_test.go index 3e2f1b8d3..79c4a76b6 100644 --- a/ipn/message_test.go +++ b/ipn/message_test.go @@ -7,6 +7,7 @@ import ( "bytes" "context" + "encoding/json" "testing" "time" @@ -74,7 +75,11 @@ func TestClientServer(t *testing.T) { bc.GotNotifyMsg(b) } }() - serverToClient := func(b []byte) { + serverToClient := func(n Notify) { + b, err := json.Marshal(n) + if err != nil { + panic(err.Error()) + } serverToClientCh <- append([]byte{}, b...) } clientToServer := func(b []byte) { From d82b28ba733f0b20a9200751ec2a91600cee5a3a Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Mon, 10 May 2021 14:41:39 -0700 Subject: [PATCH 2/3] go.mod: bump wireguard-go --- go.mod | 2 +- go.sum | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/go.mod b/go.mod index e2d9dba7b..29e42c2ed 100644 --- a/go.mod +++ b/go.mod @@ -26,7 +26,7 @@ require ( github.com/peterbourgon/ff/v2 v2.0.0 github.com/pkg/errors v0.9.1 // indirect github.com/tailscale/depaware v0.0.0-20201214215404-77d1e9757027 - github.com/tailscale/wireguard-go v0.0.0-20210510175647-030c638da3df + github.com/tailscale/wireguard-go v0.0.0-20210510192616-d1aa5623121d github.com/tcnksm/go-httpstat v0.2.0 github.com/toqueteos/webbrowser v1.2.0 go4.org/mem v0.0.0-20201119185036-c04c5a6ff174 diff --git a/go.sum b/go.sum index e0a2668e8..062e51705 100644 --- a/go.sum +++ b/go.sum @@ -129,6 +129,8 @@ github.com/tailscale/wireguard-go v0.0.0-20210429195722-6cd106ab1339 h1:OjLaZ57x github.com/tailscale/wireguard-go v0.0.0-20210429195722-6cd106ab1339/go.mod h1:ys4yUmhKncXy1jWP34qUHKipRjl322VVhxoh1Rkfo7c= github.com/tailscale/wireguard-go v0.0.0-20210510175647-030c638da3df h1:ekBw6cxmDhXf9YxTmMZh7SPwUh9rnRRnaoX7HFiGobc= github.com/tailscale/wireguard-go v0.0.0-20210510175647-030c638da3df/go.mod h1:ys4yUmhKncXy1jWP34qUHKipRjl322VVhxoh1Rkfo7c= +github.com/tailscale/wireguard-go v0.0.0-20210510192616-d1aa5623121d h1:qJSz1zlpuPLmfACtnj+tAH4g3iasJMBW8dpeFm5f4wg= +github.com/tailscale/wireguard-go v0.0.0-20210510192616-d1aa5623121d/go.mod h1:ys4yUmhKncXy1jWP34qUHKipRjl322VVhxoh1Rkfo7c= github.com/tcnksm/go-httpstat v0.2.0 h1:rP7T5e5U2HfmOBmZzGgGZjBQ5/GluWUylujl0tJ04I0= github.com/tcnksm/go-httpstat v0.2.0/go.mod h1:s3JVJFtQxtBEBC9dwcdTTXS9xFnM3SXAZwPG41aurT8= github.com/toqueteos/webbrowser v1.2.0 h1:tVP/gpK69Fx+qMJKsLE7TD8LuGWPnEV71wBN9rrstGQ= From cfde99769907b75621cabd271e4a0559659e3a21 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Mon, 10 May 2021 15:05:29 -0700 Subject: [PATCH 3/3] net/dns: don't use interfaces.Tailscale to find the tailscale interface index. interfaces.Tailscale only returns an interface if it has at least one Tailscale IP assigned to it. In the resolved DNS manager, when we're called upon to tear down DNS config, the interface no longer has IPs. Instead, look up the interface index on construction and reuse it throughout the daemon lifecycle. Fixes #1892. Signed-off-by: David Anderson --- net/dns/manager_linux.go | 6 ++--- net/dns/resolved.go | 47 +++++++++++++++------------------------- 2 files changed, 21 insertions(+), 32 deletions(-) diff --git a/net/dns/manager_linux.go b/net/dns/manager_linux.go index 0e9ee2d84..6f5bbad5a 100644 --- a/net/dns/manager_linux.go +++ b/net/dns/manager_linux.go @@ -57,12 +57,12 @@ func NewOSConfigurator(logf logger.Logf, interfaceName string) (ret OSConfigurat } if err := dbusPing("org.freedesktop.NetworkManager", "/org/freedesktop/NetworkManager/DnsManager"); err != nil { dbg("nm", "no") - return newResolvedManager(logf) + return newResolvedManager(logf, interfaceName) } dbg("nm", "yes") if err := nmIsUsingResolved(); err != nil { dbg("nm-resolved", "no") - return newResolvedManager(logf) + return newResolvedManager(logf, interfaceName) } dbg("nm-resolved", "yes") @@ -90,7 +90,7 @@ func NewOSConfigurator(logf logger.Logf, interfaceName string) (ret OSConfigurat return newNMManager(interfaceName) } dbg("nm-old", "no") - return newResolvedManager(logf) + return newResolvedManager(logf, interfaceName) case "resolvconf": dbg("rc", "resolvconf") if err := resolvconfSourceIsNM(bs); err == nil { diff --git a/net/dns/resolved.go b/net/dns/resolved.go index 27cfede02..5f8d39903 100644 --- a/net/dns/resolved.go +++ b/net/dns/resolved.go @@ -12,11 +12,11 @@ "context" "errors" "fmt" + "net" "github.com/godbus/dbus/v5" "golang.org/x/sys/unix" "inet.af/netaddr" - "tailscale.com/net/interfaces" "tailscale.com/types/logger" "tailscale.com/util/dnsname" ) @@ -85,17 +85,24 @@ func isResolvedActive() bool { // resolvedManager uses the systemd-resolved DBus API. type resolvedManager struct { logf logger.Logf + ifidx int resolved dbus.BusObject } -func newResolvedManager(logf logger.Logf) (*resolvedManager, error) { +func newResolvedManager(logf logger.Logf, interfaceName string) (*resolvedManager, error) { conn, err := dbus.SystemBus() if err != nil { return nil, err } + iface, err := net.InterfaceByName(interfaceName) + if err != nil { + return nil, err + } + return &resolvedManager{ logf: logf, + ifidx: iface.Index, resolved: conn.Object("org.freedesktop.resolve1", dbus.ObjectPath("/org/freedesktop/resolve1")), }, nil } @@ -105,16 +112,6 @@ func (m *resolvedManager) SetDNS(config OSConfig) error { ctx, cancel := context.WithTimeout(context.Background(), reconfigTimeout) defer cancel() - // In principle, we could persist this in the manager struct - // if we knew that interface indices are persistent. This does not seem to be the case. - _, iface, err := interfaces.Tailscale() - if err != nil { - return fmt.Errorf("getting interface index: %w", err) - } - if iface == nil { - return errNotReady - } - var linkNameservers = make([]resolvedLinkNameserver, len(config.Nameservers)) for i, server := range config.Nameservers { ip := server.As16() @@ -131,9 +128,9 @@ func (m *resolvedManager) SetDNS(config OSConfig) error { } } - err = m.resolved.CallWithContext( + err := m.resolved.CallWithContext( ctx, "org.freedesktop.resolve1.Manager.SetLinkDNS", 0, - iface.Index, linkNameservers, + m.ifidx, linkNameservers, ).Store() if err != nil { return fmt.Errorf("setLinkDNS: %w", err) @@ -174,13 +171,13 @@ func (m *resolvedManager) SetDNS(config OSConfig) error { err = m.resolved.CallWithContext( ctx, "org.freedesktop.resolve1.Manager.SetLinkDomains", 0, - iface.Index, linkDomains, + m.ifidx, linkDomains, ).Store() if err != nil { return fmt.Errorf("setLinkDomains: %w", err) } - if call := m.resolved.CallWithContext(ctx, "org.freedesktop.resolve1.Manager.SetLinkDefaultRoute", 0, iface.Index, len(config.MatchDomains) == 0); call.Err != nil { + if call := m.resolved.CallWithContext(ctx, "org.freedesktop.resolve1.Manager.SetLinkDefaultRoute", 0, m.ifidx, len(config.MatchDomains) == 0); call.Err != nil { return fmt.Errorf("setLinkDefaultRoute: %w", err) } @@ -189,22 +186,22 @@ func (m *resolvedManager) SetDNS(config OSConfig) error { // or something). // Disable LLMNR, we don't do multicast. - if call := m.resolved.CallWithContext(ctx, "org.freedesktop.resolve1.Manager.SetLinkLLMNR", 0, iface.Index, "no"); call.Err != nil { + if call := m.resolved.CallWithContext(ctx, "org.freedesktop.resolve1.Manager.SetLinkLLMNR", 0, m.ifidx, "no"); call.Err != nil { m.logf("[v1] failed to disable LLMNR: %v", call.Err) } // Disable mdns. - if call := m.resolved.CallWithContext(ctx, "org.freedesktop.resolve1.Manager.SetLinkMulticastDNS", 0, iface.Index, "no"); call.Err != nil { + if call := m.resolved.CallWithContext(ctx, "org.freedesktop.resolve1.Manager.SetLinkMulticastDNS", 0, m.ifidx, "no"); call.Err != nil { m.logf("[v1] failed to disable mdns: %v", call.Err) } // We don't support dnssec consistently right now, force it off to // avoid partial failures when we split DNS internally. - if call := m.resolved.CallWithContext(ctx, "org.freedesktop.resolve1.Manager.SetLinkDNSSEC", 0, iface.Index, "no"); call.Err != nil { + if call := m.resolved.CallWithContext(ctx, "org.freedesktop.resolve1.Manager.SetLinkDNSSEC", 0, m.ifidx, "no"); call.Err != nil { m.logf("[v1] failed to disable DNSSEC: %v", call.Err) } - if call := m.resolved.CallWithContext(ctx, "org.freedesktop.resolve1.Manager.SetLinkDNSOverTLS", 0, iface.Index, "no"); call.Err != nil { + if call := m.resolved.CallWithContext(ctx, "org.freedesktop.resolve1.Manager.SetLinkDNSOverTLS", 0, m.ifidx, "no"); call.Err != nil { m.logf("[v1] failed to disable DoT: %v", call.Err) } @@ -227,15 +224,7 @@ func (m *resolvedManager) Close() error { ctx, cancel := context.WithTimeout(context.Background(), reconfigTimeout) defer cancel() - _, iface, err := interfaces.Tailscale() - if err != nil { - return fmt.Errorf("getting interface index: %w", err) - } - if iface == nil { - return errNotReady - } - - if call := m.resolved.CallWithContext(ctx, "org.freedesktop.resolve1.Manager.RevertLink", 0, iface.Index); call.Err != nil { + if call := m.resolved.CallWithContext(ctx, "org.freedesktop.resolve1.Manager.RevertLink", 0, m.ifidx); call.Err != nil { return fmt.Errorf("RevertLink: %w", call.Err) }