From 88df3007d836b503158bd2b921e2d49a153997fc Mon Sep 17 00:00:00 2001 From: Irbe Krumina Date: Thu, 26 Jun 2025 17:19:25 +0100 Subject: [PATCH] ipn/ipnlocal: Update hostinfo to control on service config change (#16146) (#16358) This commit fixes the bug that c2n requests are skiped when updating vipServices in serveConfig. This then resulted netmap update being skipped which caused inaccuracy of Capmap info on client side. After this fix, client always inform control about it's vipServices config changes. Fixes tailscale/corp#29219 (cherry picked from commit 7b06532ea108bd7d12785125c2423a1a4e9987b3) Signed-off-by: KevinLiang10 <37811973+KevinLiang10@users.noreply.github.com> Co-authored-by: KevinLiang10 <37811973+KevinLiang10@users.noreply.github.com> --- ipn/ipnlocal/local.go | 15 ++++++--- ipn/ipnlocal/local_test.go | 69 ++++++++++++++++++++++++++++++++++++-- 2 files changed, 76 insertions(+), 8 deletions(-) diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 468fd72eb..ea043aa93 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -6137,17 +6137,17 @@ func (b *LocalBackend) setTCPPortsInterceptedFromNetmapAndPrefsLocked(prefs ipn. } } - // Update funnel info in hostinfo and kick off control update if needed. - b.updateIngressLocked() + // Update funnel and service hash info in hostinfo and kick off control update if needed. + b.updateIngressAndServiceHashLocked(prefs) b.setTCPPortsIntercepted(handlePorts) b.setVIPServicesTCPPortsInterceptedLocked(vipServicesPorts) } -// updateIngressLocked updates the hostinfo.WireIngress and hostinfo.IngressEnabled fields and kicks off a Hostinfo -// update if the values have changed. +// updateIngressAndServiceHashLocked updates the hostinfo.ServicesHash, hostinfo.WireIngress and +// hostinfo.IngressEnabled fields and kicks off a Hostinfo update if the values have changed. // // b.mu must be held. -func (b *LocalBackend) updateIngressLocked() { +func (b *LocalBackend) updateIngressAndServiceHashLocked(prefs ipn.PrefsView) { if b.hostinfo == nil { return } @@ -6162,6 +6162,11 @@ func (b *LocalBackend) updateIngressLocked() { b.hostinfo.WireIngress = wire hostInfoChanged = true } + latestHash := b.vipServiceHash(b.vipServicesFromPrefsLocked(prefs)) + if b.hostinfo.ServicesHash != latestHash { + b.hostinfo.ServicesHash = latestHash + hostInfoChanged = true + } // Kick off a Hostinfo update to control if ingress status has changed. if hostInfoChanged { b.goTracker.Go(b.doSetHostinfoFilterServices) diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index 19cfd9195..1def6d98a 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -5134,10 +5134,17 @@ func TestUpdatePrefsOnSysPolicyChange(t *testing.T) { } } -func TestUpdateIngressLocked(t *testing.T) { +func TestUpdateIngressAndServiceHashLocked(t *testing.T) { + prefs := ipn.NewPrefs().View() + previousSC := &ipn.ServeConfig{ + Services: map[tailcfg.ServiceName]*ipn.ServiceConfig{ + "svc:abc": {Tun: true}, + }, + } tests := []struct { name string hi *tailcfg.Hostinfo + hasPreviousSC bool // whether to overwrite the ServeConfig hash in the Hostinfo using previousSC sc *ipn.ServeConfig wantIngress bool wantWireIngress bool @@ -5163,6 +5170,16 @@ func TestUpdateIngressLocked(t *testing.T) { wantWireIngress: false, // implied by wantIngress wantControlUpdate: true, }, + { + name: "empty_hostinfo_service_configured", + hi: &tailcfg.Hostinfo{}, + sc: &ipn.ServeConfig{ + Services: map[tailcfg.ServiceName]*ipn.ServiceConfig{ + "svc:abc": {Tun: true}, + }, + }, + wantControlUpdate: true, + }, { name: "empty_hostinfo_funnel_disabled", hi: &tailcfg.Hostinfo{}, @@ -5175,7 +5192,7 @@ func TestUpdateIngressLocked(t *testing.T) { wantControlUpdate: true, }, { - name: "empty_hostinfo_no_funnel", + name: "empty_hostinfo_no_funnel_no_service", hi: &tailcfg.Hostinfo{}, sc: &ipn.ServeConfig{ TCP: map[uint16]*ipn.TCPPortHandler{ @@ -5196,6 +5213,16 @@ func TestUpdateIngressLocked(t *testing.T) { wantIngress: true, wantWireIngress: false, // implied by wantIngress }, + { + name: "service_hash_no_change", + hi: &tailcfg.Hostinfo{}, + hasPreviousSC: true, + sc: &ipn.ServeConfig{ + Services: map[tailcfg.ServiceName]*ipn.ServiceConfig{ + "svc:abc": {Tun: true}, + }, + }, + }, { name: "funnel_disabled_no_change", hi: &tailcfg.Hostinfo{ @@ -5208,6 +5235,13 @@ func TestUpdateIngressLocked(t *testing.T) { }, wantWireIngress: true, // true if there is any AllowFunnel block }, + { + name: "service_got_removed", + hi: &tailcfg.Hostinfo{}, + hasPreviousSC: true, + sc: &ipn.ServeConfig{}, + wantControlUpdate: true, + }, { name: "funnel_changes_to_disabled", hi: &tailcfg.Hostinfo{ @@ -5235,12 +5269,35 @@ func TestUpdateIngressLocked(t *testing.T) { wantWireIngress: false, // implied by wantIngress wantControlUpdate: true, }, + { + name: "both_funnel_and_service_changes", + hi: &tailcfg.Hostinfo{ + IngressEnabled: true, + }, + sc: &ipn.ServeConfig{ + AllowFunnel: map[ipn.HostPort]bool{ + "tailnet.xyz:443": false, + }, + Services: map[tailcfg.ServiceName]*ipn.ServiceConfig{ + "svc:abc": {Tun: true}, + }, + }, + wantWireIngress: true, // true if there is any AllowFunnel block + wantControlUpdate: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Parallel() b := newTestLocalBackend(t) b.hostinfo = tt.hi + if tt.hasPreviousSC { + b.mu.Lock() + b.serveConfig = previousSC.View() + b.hostinfo.ServicesHash = b.vipServiceHash(b.vipServicesFromPrefsLocked(prefs)) + b.mu.Unlock() + } b.serveConfig = tt.sc.View() allDone := make(chan bool, 1) defer b.goTracker.AddDoneCallback(func() { @@ -5256,7 +5313,7 @@ func TestUpdateIngressLocked(t *testing.T) { })() was := b.goTracker.StartedGoroutines() - b.updateIngressLocked() + b.updateIngressAndServiceHashLocked(prefs) if tt.hi != nil { if tt.hi.IngressEnabled != tt.wantIngress { @@ -5265,6 +5322,12 @@ func TestUpdateIngressLocked(t *testing.T) { if tt.hi.WireIngress != tt.wantWireIngress { t.Errorf("WireIngress = %v, want %v", tt.hi.WireIngress, tt.wantWireIngress) } + b.mu.Lock() + svcHash := b.vipServiceHash(b.vipServicesFromPrefsLocked(prefs)) + b.mu.Unlock() + if tt.hi.ServicesHash != svcHash { + t.Errorf("ServicesHash = %v, want %v", tt.hi.ServicesHash, svcHash) + } } startedGoroutine := b.goTracker.StartedGoroutines() != was