Skip to content

Commit 9a655a1

Browse files
mihaipmihaip
committed
net/dnsfallback: more explicitly pass through logf function
Redoes the approach from tailscale#5550 and tailscale#7539 to explicitly pass in the logf function, instead of having global state that can be overridden. Signed-off-by: Mihai Parparita <mihai@tailscale.com>
1 parent 28cb122 commit 9a655a1

File tree

8 files changed

+20
-66
lines changed

8 files changed

+20
-66
lines changed

‎cmd/tailscaled/tailscaled.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -534,7 +534,7 @@ func getLocalBackend(ctx context.Context, logf logger.Logf, logID logid.PublicID
534534
lb.SetLogFlusher(logPol.Logtail.StartFlush)
535535
}
536536
if root := lb.TailscaleVarRoot(); root != "" {
537-
dnsfallback.SetCachePath(filepath.Join(root, "derpmap.cached.json"))
537+
dnsfallback.SetCachePath(filepath.Join(root, "derpmap.cached.json"), logf)
538538
}
539539
lb.SetDecompressor(func() (controlclient.Decompressor, error) {
540540
return smallzstd.NewDecoder(nil)

‎control/controlclient/direct.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -211,7 +211,7 @@ func NewDirect(opts Options) (*Direct, error) {
211211
dnsCache := &dnscache.Resolver{
212212
Forward: dnscache.Get().Forward, // use default cache's forwarder
213213
UseLastGood: true,
214-
LookupIPFallback: dnsfallback.Lookup,
214+
LookupIPFallback: dnsfallback.Lookup(opts.Logf),
215215
Logf: opts.Logf,
216216
}
217217
tr := http.DefaultTransport.(*http.Transport).Clone()

‎control/controlhttp/client.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -393,7 +393,7 @@ func (a *Dialer) tryURLUpgrade(ctx context.Context, u *url.URL, addr netip.Addr,
393393
} else {
394394
dns = &dnscache.Resolver{
395395
Forward: dnscache.Get().Forward,
396-
LookupIPFallback: dnsfallback.Lookup,
396+
LookupIPFallback: dnsfallback.Lookup(a.logf),
397397
UseLastGood: true,
398398
Logf: a.Logf, // not a.logf method; we want to propagate nil-ness
399399
}

‎ipn/ipnlocal/local.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1077,7 +1077,7 @@ func (b *LocalBackend) setClientStatus(st controlclient.Status) {
10771077
b.e.SetDERPMap(st.NetMap.DERPMap)
10781078

10791079
// Update our cached DERP map
1080-
dnsfallback.UpdateCache(st.NetMap.DERPMap)
1080+
dnsfallback.UpdateCache(st.NetMap.DERPMap, b.logf)
10811081

10821082
b.send(ipn.Notify{NetMap: st.NetMap})
10831083
}

‎logpolicy/logpolicy.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -711,7 +711,7 @@ func DialContext(ctx context.Context, netw, addr string) (net.Conn, error) {
711711
dnsCache := &dnscache.Resolver{
712712
Forward: dnscache.Get().Forward, // use default cache's forwarder
713713
UseLastGood: true,
714-
LookupIPFallback: dnsfallback.Lookup,
714+
LookupIPFallback: dnsfallback.Lookup(log.Printf),
715715
}
716716
dialer := dnscache.Dialer(nd.DialContext, dnsCache)
717717
c, err = dialer(ctx, netw, addr)

‎net/dnsfallback/dnsfallback.go

Lines changed: 11 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ import (
1313
"encoding/json"
1414
"errors"
1515
"fmt"
16-
"log"
1716
"net"
1817
"net/http"
1918
"net/netip"
@@ -27,13 +26,18 @@ import (
2726
"tailscale.com/net/netns"
2827
"tailscale.com/net/tlsdial"
2928
"tailscale.com/net/tshttpproxy"
30-
"tailscale.com/syncs"
3129
"tailscale.com/tailcfg"
3230
"tailscale.com/types/logger"
3331
"tailscale.com/util/slicesx"
3432
)
3533

36-
func Lookup(ctx context.Context, host string) ([]netip.Addr, error) {
34+
func Lookup(logf logger.Logf) func(ctx context.Context, host string) ([]netip.Addr, error) {
35+
return func(ctx context.Context, host string) ([]netip.Addr, error) {
36+
return lookup(ctx, host, logf)
37+
}
38+
}
39+
40+
func lookup(ctx context.Context, host string, logf logger.Logf) ([]netip.Addr, error) {
3741
if ip, err := netip.ParseAddr(host); err == nil && ip.IsValid() {
3842
return []netip.Addr{ip}, nil
3943
}
@@ -81,7 +85,7 @@ func Lookup(ctx context.Context, host string) ([]netip.Addr, error) {
8185
logf("trying bootstrapDNS(%q, %q) for %q ...", cand.dnsName, cand.ip, host)
8286
ctx, cancel := context.WithTimeout(ctx, 3*time.Second)
8387
defer cancel()
84-
dm, err := bootstrapDNSMap(ctx, cand.dnsName, cand.ip, host)
88+
dm, err := bootstrapDNSMap(ctx, cand.dnsName, cand.ip, host, logf)
8589
if err != nil {
8690
logf("bootstrapDNS(%q, %q) for %q error: %v", cand.dnsName, cand.ip, host, err)
8791
continue
@@ -100,7 +104,7 @@ func Lookup(ctx context.Context, host string) ([]netip.Addr, error) {
100104

101105
// serverName and serverIP of are, say, "derpN.tailscale.com".
102106
// queryName is the name being sought (e.g. "controlplane.tailscale.com"), passed as hint.
103-
func bootstrapDNSMap(ctx context.Context, serverName string, serverIP netip.Addr, queryName string) (dnsMap, error) {
107+
func bootstrapDNSMap(ctx context.Context, serverName string, serverIP netip.Addr, queryName string, logf logger.Logf) (dnsMap, error) {
104108
dialer := netns.NewDialer(logf)
105109
tr := http.DefaultTransport.(*http.Transport).Clone()
106110
tr.Proxy = tshttpproxy.ProxyFromEnvironment
@@ -194,7 +198,7 @@ var cachePath string
194198
// UpdateCache stores the DERP map cache back to disk.
195199
//
196200
// The caller must not mutate 'c' after calling this function.
197-
func UpdateCache(c *tailcfg.DERPMap) {
201+
func UpdateCache(c *tailcfg.DERPMap, logf logger.Logf) {
198202
// Don't do anything if nothing changed.
199203
curr := cachedDERPMap.Load()
200204
if reflect.DeepEqual(curr, c) {
@@ -227,7 +231,7 @@ func UpdateCache(c *tailcfg.DERPMap) {
227231
//
228232
// This function should be called before any calls to UpdateCache, as it is not
229233
// concurrency-safe.
230-
func SetCachePath(path string) {
234+
func SetCachePath(path string, logf logger.Logf) {
231235
cachePath = path
232236

233237
f, err := os.Open(path)
@@ -246,23 +250,3 @@ func SetCachePath(path string) {
246250
cachedDERPMap.Store(dm)
247251
logf("[v2] dnsfallback: SetCachePath loaded cached DERP map")
248252
}
249-
250-
// logfunc stores the logging function to use for this package.
251-
var logfunc syncs.AtomicValue[logger.Logf]
252-
253-
// SetLogger sets the logging function that this package will use, and returns
254-
// the old value (which may be nil).
255-
//
256-
// If this function is never called, or if this function is called with a nil
257-
// value, 'log.Printf' will be used to print logs.
258-
func SetLogger(log logger.Logf) (old logger.Logf) {
259-
return logfunc.Swap(log)
260-
}
261-
262-
func logf(format string, args ...any) {
263-
if lf := logfunc.Load(); lf != nil {
264-
lf(format, args...)
265-
} else {
266-
log.Printf(format, args...)
267-
}
268-
}

‎net/dnsfallback/dnsfallback_test.go

Lines changed: 4 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -24,11 +24,6 @@ func TestGetDERPMap(t *testing.T) {
2424
}
2525

2626
func TestCache(t *testing.T) {
27-
oldlog := logfunc.Load()
28-
SetLogger(t.Logf)
29-
t.Cleanup(func() {
30-
SetLogger(oldlog)
31-
})
3227
cacheFile := filepath.Join(t.TempDir(), "cache.json")
3328

3429
// Write initial cache value
@@ -73,7 +68,7 @@ func TestCache(t *testing.T) {
7368
cachedDERPMap.Store(nil)
7469

7570
// Load the cache
76-
SetCachePath(cacheFile)
71+
SetCachePath(cacheFile, t.Logf)
7772
if cm := cachedDERPMap.Load(); !reflect.DeepEqual(initialCache, cm) {
7873
t.Fatalf("cached map was %+v; want %+v", cm, initialCache)
7974
}
@@ -105,11 +100,6 @@ func TestCache(t *testing.T) {
105100
}
106101

107102
func TestCacheUnchanged(t *testing.T) {
108-
oldlog := logfunc.Load()
109-
SetLogger(t.Logf)
110-
t.Cleanup(func() {
111-
SetLogger(oldlog)
112-
})
113103
cacheFile := filepath.Join(t.TempDir(), "cache.json")
114104

115105
// Write initial cache value
@@ -140,7 +130,7 @@ func TestCacheUnchanged(t *testing.T) {
140130
cachedDERPMap.Store(nil)
141131

142132
// Load the cache
143-
SetCachePath(cacheFile)
133+
SetCachePath(cacheFile, t.Logf)
144134
if cm := cachedDERPMap.Load(); !reflect.DeepEqual(initialCache, cm) {
145135
t.Fatalf("cached map was %+v; want %+v", cm, initialCache)
146136
}
@@ -152,7 +142,7 @@ func TestCacheUnchanged(t *testing.T) {
152142
t.Fatal(err)
153143
}
154144

155-
UpdateCache(initialCache)
145+
UpdateCache(initialCache, t.Logf)
156146
if _, err := os.Stat(cacheFile); !os.IsNotExist(err) {
157147
t.Fatalf("got err=%v; expected to not find cache file", err)
158148
}
@@ -173,7 +163,7 @@ func TestCacheUnchanged(t *testing.T) {
173163
clonedNode.IPv4 = "1.2.3.5"
174164
updatedCache.Regions[99].Nodes = append(updatedCache.Regions[99].Nodes, &clonedNode)
175165

176-
UpdateCache(updatedCache)
166+
UpdateCache(updatedCache, t.Logf)
177167
if st, err := os.Stat(cacheFile); err != nil {
178168
t.Fatalf("could not stat cache file; err=%v", err)
179169
} else if !st.Mode().IsRegular() || st.Size() == 0 {

‎tsnet/tsnet.go

Lines changed: 0 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,6 @@ import (
4141
"tailscale.com/logpolicy"
4242
"tailscale.com/logtail"
4343
"tailscale.com/logtail/filch"
44-
"tailscale.com/net/dnsfallback"
4544
"tailscale.com/net/memnet"
4645
"tailscale.com/net/proxymux"
4746
"tailscale.com/net/socks5"
@@ -651,25 +650,6 @@ func (s *Server) logf(format string, a ...interface{}) {
651650
log.Printf(format, a...)
652651
}
653652

654-
// ReplaceGlobalLoggers will replace any Tailscale-specific package-global
655-
// loggers with this Server's logger. It returns a function that, when called,
656-
// will undo any changes made.
657-
//
658-
// Note that calling this function from multiple Servers will result in the
659-
// last call taking all logs; logs are not duplicated.
660-
func (s *Server) ReplaceGlobalLoggers() (undo func()) {
661-
var undos []func()
662-
663-
oldDnsFallback := dnsfallback.SetLogger(s.logf)
664-
undos = append(undos, func() { dnsfallback.SetLogger(oldDnsFallback) })
665-
666-
return func() {
667-
for _, fn := range undos {
668-
fn()
669-
}
670-
}
671-
}
672-
673653
// printAuthURLLoop loops once every few seconds while the server is still running and
674654
// is in NeedsLogin state, printing out the auth URL.
675655
func (s *Server) printAuthURLLoop() {

0 commit comments

Comments
 (0)