From b21eec7621bd0a20809684adba9b3c42245424d2 Mon Sep 17 00:00:00 2001 From: Irbe Krumina Date: Sun, 16 Feb 2025 09:38:02 +0000 Subject: [PATCH] ipn/ipnlocal,tailcfg: don't send WireIngress if IngressEnabled already true (#14960) Hostinfo.WireIngress is used as a hint that the node intends to use funnel. We now send another field, IngressEnabled, in cases where funnel is explicitly enabled, and the logic control-side has been changed to look at IngressEnabled as well as WireIngress in all cases where previously the hint was used - so we can now stop sending WireIngress when IngressEnabled is true to save some bandwidth. Updates tailscale/tailscale#11572 Updates tailscale/corp#25931 Signed-off-by: Irbe Krumina --- ipn/ipnlocal/local.go | 38 +++++++++++++++++++++----------------- ipn/ipnlocal/local_test.go | 8 +++----- tailcfg/tailcfg.go | 25 ++++++++++++++++--------- 3 files changed, 40 insertions(+), 31 deletions(-) diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 8b30484d6..43d82c900 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -4372,6 +4372,12 @@ func (b *LocalBackend) hasIngressEnabledLocked() bool { return b.serveConfig.Valid() && b.serveConfig.IsFunnelOn() } +// shouldWireInactiveIngressLocked reports whether the node is in a state where funnel is not actively enabled, but it +// seems that it is intended to be used with funnel. +func (b *LocalBackend) shouldWireInactiveIngressLocked() bool { + return b.serveConfig.Valid() && !b.hasIngressEnabledLocked() && b.wantIngressLocked() +} + // setPrefsLockedOnEntry requires b.mu be held to call it, but it // unlocks b.mu when done. newp ownership passes to this function. // It returns a read-only copy of the new prefs. @@ -5479,18 +5485,18 @@ func (b *LocalBackend) applyPrefsToHostinfoLocked(hi *tailcfg.Hostinfo, prefs ip hi.ServicesHash = b.vipServiceHash(b.vipServicesFromPrefsLocked(prefs)) - // The Hostinfo.WantIngress field tells control whether this node wants to - // be wired up for ingress connections. If harmless if it's accidentally - // true; the actual policy is controlled in tailscaled by ServeConfig. But - // if this is accidentally false, then control may not configure DNS - // properly. This exists as an optimization to control to program fewer DNS - // records that have ingress enabled but are not actually being used. - // TODO(irbekrm): once control knows that if hostinfo.IngressEnabled is true, - // then wireIngress can be considered true, don't send wireIngress in that case. - hi.WireIngress = b.wantIngressLocked() // The Hostinfo.IngressEnabled field is used to communicate to control whether - // the funnel is actually enabled. + // the node has funnel enabled. hi.IngressEnabled = b.hasIngressEnabledLocked() + // The Hostinfo.WantIngress field tells control whether the user intends + // to use funnel with this node even though it is not currently enabled. + // This is an optimization to control- Funnel requires creation of DNS + // records and because DNS propagation can take time, we want to ensure + // that the records exist for any node that intends to use funnel even + // if it's not enabled. If hi.IngressEnabled is true, control knows that + // DNS records are needed, so we can save bandwidth and not send + // WireIngress. + hi.WireIngress = b.shouldWireInactiveIngressLocked() hi.AppConnector.Set(prefs.AppConnector().Advertise) } @@ -6404,8 +6410,6 @@ func (b *LocalBackend) setTCPPortsInterceptedFromNetmapAndPrefsLocked(prefs ipn. // updateIngressLocked updates the hostinfo.WireIngress and hostinfo.IngressEnabled fields and kicks off a Hostinfo // update if the values have changed. -// TODO(irbekrm): once control knows that if hostinfo.IngressEnabled is true, then wireIngress can be considered true, -// we can stop sending hostinfo.WireIngress in that case. // // b.mu must be held. func (b *LocalBackend) updateIngressLocked() { @@ -6413,16 +6417,16 @@ func (b *LocalBackend) updateIngressLocked() { return } hostInfoChanged := false - if wire := b.wantIngressLocked(); b.hostinfo.WireIngress != wire { - b.logf("Hostinfo.WireIngress changed to %v", wire) - b.hostinfo.WireIngress = wire - hostInfoChanged = true - } if ie := b.hasIngressEnabledLocked(); b.hostinfo.IngressEnabled != ie { b.logf("Hostinfo.IngressEnabled changed to %v", ie) b.hostinfo.IngressEnabled = ie hostInfoChanged = true } + if wire := b.shouldWireInactiveIngressLocked(); b.hostinfo.WireIngress != wire { + b.logf("Hostinfo.WireIngress changed to %v", wire) + b.hostinfo.WireIngress = wire + 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 f0c712777..35977e679 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -5084,7 +5084,7 @@ func TestUpdateIngressLocked(t *testing.T) { }, }, wantIngress: true, - wantWireIngress: true, + wantWireIngress: false, // implied by wantIngress wantControlUpdate: true, }, { @@ -5111,7 +5111,6 @@ func TestUpdateIngressLocked(t *testing.T) { name: "funnel_enabled_no_change", hi: &tailcfg.Hostinfo{ IngressEnabled: true, - WireIngress: true, }, sc: &ipn.ServeConfig{ AllowFunnel: map[ipn.HostPort]bool{ @@ -5119,7 +5118,7 @@ func TestUpdateIngressLocked(t *testing.T) { }, }, wantIngress: true, - wantWireIngress: true, + wantWireIngress: false, // implied by wantIngress }, { name: "funnel_disabled_no_change", @@ -5137,7 +5136,6 @@ func TestUpdateIngressLocked(t *testing.T) { name: "funnel_changes_to_disabled", hi: &tailcfg.Hostinfo{ IngressEnabled: true, - WireIngress: true, }, sc: &ipn.ServeConfig{ AllowFunnel: map[ipn.HostPort]bool{ @@ -5157,8 +5155,8 @@ func TestUpdateIngressLocked(t *testing.T) { "tailnet.xyz:443": true, }, }, - wantWireIngress: true, wantIngress: true, + wantWireIngress: false, // implied by wantIngress wantControlUpdate: true, }, } diff --git a/tailcfg/tailcfg.go b/tailcfg/tailcfg.go index 4d30f6501..f82c6eb81 100644 --- a/tailcfg/tailcfg.go +++ b/tailcfg/tailcfg.go @@ -835,15 +835,22 @@ type Hostinfo struct { // App is used to disambiguate Tailscale clients that run using tsnet. App string `json:",omitempty"` // "k8s-operator", "golinks", ... - Desktop opt.Bool `json:",omitempty"` // if a desktop was detected on Linux - Package string `json:",omitempty"` // Tailscale package to disambiguate ("choco", "appstore", etc; "" for unknown) - DeviceModel string `json:",omitempty"` // mobile phone model ("Pixel 3a", "iPhone12,3") - PushDeviceToken string `json:",omitempty"` // macOS/iOS APNs device token for notifications (and Android in the future) - Hostname string `json:",omitempty"` // name of the host the client runs on - ShieldsUp bool `json:",omitempty"` // indicates whether the host is blocking incoming connections - ShareeNode bool `json:",omitempty"` // indicates this node exists in netmap because it's owned by a shared-to user - NoLogsNoSupport bool `json:",omitempty"` // indicates that the user has opted out of sending logs and support - WireIngress bool `json:",omitempty"` // indicates that the node wants the option to receive ingress connections + Desktop opt.Bool `json:",omitempty"` // if a desktop was detected on Linux + Package string `json:",omitempty"` // Tailscale package to disambiguate ("choco", "appstore", etc; "" for unknown) + DeviceModel string `json:",omitempty"` // mobile phone model ("Pixel 3a", "iPhone12,3") + PushDeviceToken string `json:",omitempty"` // macOS/iOS APNs device token for notifications (and Android in the future) + Hostname string `json:",omitempty"` // name of the host the client runs on + ShieldsUp bool `json:",omitempty"` // indicates whether the host is blocking incoming connections + ShareeNode bool `json:",omitempty"` // indicates this node exists in netmap because it's owned by a shared-to user + NoLogsNoSupport bool `json:",omitempty"` // indicates that the user has opted out of sending logs and support + // WireIngress indicates that the node would like to be wired up server-side + // (DNS, etc) to be able to use Tailscale Funnel, even if it's not currently + // enabled. For example, the user might only use it for intermittent + // foreground CLI serve sessions, for which they'd like it to work right + // away, even if it's disabled most of the time. As an optimization, this is + // only sent if IngressEnabled is false, as IngressEnabled implies that this + // option is true. + WireIngress bool `json:",omitempty"` IngressEnabled bool `json:",omitempty"` // if the node has any funnel endpoint enabled AllowsUpdate bool `json:",omitempty"` // indicates that the node has opted-in to admin-console-drive remote updates Machine string `json:",omitempty"` // the current host's machine type (uname -m)