From 5a70a72988658f9de925c68ccc0b55ede6bed606 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Fri, 5 Jun 2026 14:23:23 +0000 Subject: [PATCH] types: lock tailcfg DNS config access for extra-records updates The watcher write raced per-client map builds cloning ExtraRecords. --- hscontrol/app.go | 4 +- .../mapper/dns_extrarecords_race_test.go | 60 +++++++++++++++++++ hscontrol/mapper/mapper.go | 5 +- hscontrol/types/config.go | 31 ++++++++++ 4 files changed, 95 insertions(+), 5 deletions(-) create mode 100644 hscontrol/mapper/dns_extrarecords_race_test.go diff --git a/hscontrol/app.go b/hscontrol/app.go index c93a6a2e..c183c3bb 100644 --- a/hscontrol/app.go +++ b/hscontrol/app.go @@ -377,7 +377,7 @@ func (h *Headscale) scheduledTasks(ctx context.Context) { continue } - h.cfg.TailcfgDNSConfig.ExtraRecords = records + h.cfg.SetExtraRecords(records) h.Change(change.ExtraRecords()) @@ -667,7 +667,7 @@ func (h *Headscale) Serve() error { return fmt.Errorf("setting up extrarecord manager: %w", err) } - h.cfg.TailcfgDNSConfig.ExtraRecords = h.extraRecordMan.Records() + h.cfg.SetExtraRecords(h.extraRecordMan.Records()) go h.extraRecordMan.Run() defer h.extraRecordMan.Close() diff --git a/hscontrol/mapper/dns_extrarecords_race_test.go b/hscontrol/mapper/dns_extrarecords_race_test.go new file mode 100644 index 00000000..424f7fde --- /dev/null +++ b/hscontrol/mapper/dns_extrarecords_race_test.go @@ -0,0 +1,60 @@ +package mapper + +import ( + "sync" + "testing" + + "github.com/juanfont/headscale/hscontrol/types" + "tailscale.com/tailcfg" +) + +// TestExtraRecordsConcurrentUpdateNoRace exercises the extra-records watcher +// write path (Config.SetExtraRecords) concurrently with per-client DNS config +// builds (generateDNSConfig -> Config.CloneTailcfgDNSConfig). Both must go +// through the shared lock so the run is race-free under -race. +func TestExtraRecordsConcurrentUpdateNoRace(t *testing.T) { + uid := uint(1) + cfg := &types.Config{ + TailcfgDNSConfig: &tailcfg.DNSConfig{ + ExtraRecords: []tailcfg.DNSRecord{ + {Name: "initial.example.com", Type: "A", Value: "100.64.0.1"}, + }, + }, + } + + node := (&types.Node{ + Hostname: "race-node", + UserID: &uid, + User: &types.User{Name: "racer"}, + }).View() + + const iterations = 2000 + + var wg sync.WaitGroup + + // Writer: the extra-records update path. + wg.Go(func() { + for i := range iterations { + recs := []tailcfg.DNSRecord{{Name: "a.example.com", Type: "A", Value: "100.64.0.2"}} + if i%2 == 0 { + recs = append(recs, tailcfg.DNSRecord{Name: "b.example.com", Type: "A", Value: "100.64.0.3"}) + } + + cfg.SetExtraRecords(recs) + } + }) + + // Readers: the per-client map build path. + const readers = 8 + for range readers { + wg.Go(func() { + for range iterations { + if d := generateDNSConfig(cfg, node, nil); d != nil { + _ = len(d.ExtraRecords) + } + } + }) + } + + wg.Wait() +} diff --git a/hscontrol/mapper/mapper.go b/hscontrol/mapper/mapper.go index 8e99467e..a499800c 100644 --- a/hscontrol/mapper/mapper.go +++ b/hscontrol/mapper/mapper.go @@ -135,12 +135,11 @@ func generateDNSConfig( node types.NodeView, capMap tailcfg.NodeCapMap, ) *tailcfg.DNSConfig { - if cfg.TailcfgDNSConfig == nil { + dnsConfig := cfg.CloneTailcfgDNSConfig() + if dnsConfig == nil { return nil } - dnsConfig := cfg.TailcfgDNSConfig.Clone() - profile := nextDNSProfileFromCapMap(capMap) if profile != "" { applyNextDNSProfile(dnsConfig.Resolvers, profile) diff --git a/hscontrol/types/config.go b/hscontrol/types/config.go index 8295cd05..a2cace06 100644 --- a/hscontrol/types/config.go +++ b/hscontrol/types/config.go @@ -8,6 +8,7 @@ import ( "net/url" "os" "strings" + "sync" "time" "github.com/coreos/go-oidc/v3/oidc" @@ -1482,3 +1483,33 @@ func (d *deprecator) Log() { log.Warn().Msg("\n" + d.String()) } } + +// tailcfgDNSMu guards concurrent access to the mutable ExtraRecords of +// [Config.TailcfgDNSConfig] between the extra-records file watcher (writer) +// and the per-client map builds that clone it (readers). It is a package-level +// lock so [Config] stays freely copyable during construction. +var tailcfgDNSMu sync.RWMutex + +// CloneTailcfgDNSConfig returns a deep copy of [Config.TailcfgDNSConfig], or +// nil if none is set. Safe for concurrent use with [Config.SetExtraRecords]. +func (c *Config) CloneTailcfgDNSConfig() *tailcfg.DNSConfig { + tailcfgDNSMu.RLock() + defer tailcfgDNSMu.RUnlock() + + if c.TailcfgDNSConfig == nil { + return nil + } + + return c.TailcfgDNSConfig.Clone() +} + +// SetExtraRecords replaces the ExtraRecords of [Config.TailcfgDNSConfig]. Safe +// for concurrent use with [Config.CloneTailcfgDNSConfig]. +func (c *Config) SetExtraRecords(records []tailcfg.DNSRecord) { + tailcfgDNSMu.Lock() + defer tailcfgDNSMu.Unlock() + + if c.TailcfgDNSConfig != nil { + c.TailcfgDNSConfig.ExtraRecords = records + } +}