diff --git a/appc/conn25.go b/appc/conn25.go index fd1748fa6..9b44eb88c 100644 --- a/appc/conn25.go +++ b/appc/conn25.go @@ -16,7 +16,7 @@ import ( const AppConnectorsExperimentalAttrName = "tailscale.com/app-connectors-experimental" -func isEligibleConnector(peer tailcfg.NodeView) bool { +func isPeerEligibleConnector(peer tailcfg.NodeView) bool { if !peer.Valid() || !peer.Hostinfo().Valid() { return false } @@ -39,7 +39,7 @@ func sortByPreference(ns []tailcfg.NodeView) { func PickConnector(nb ipnext.NodeBackend, app appctype.Conn25Attr) []tailcfg.NodeView { appTagsSet := set.SetOf(app.Connectors) matches := nb.AppendMatchingPeers(nil, func(n tailcfg.NodeView) bool { - if !isEligibleConnector(n) { + if !isPeerEligibleConnector(n) { return false } for _, t := range n.Tags().All() { @@ -55,7 +55,7 @@ func PickConnector(nb ipnext.NodeBackend, app appctype.Conn25Attr) []tailcfg.Nod // PickSplitDNSPeers looks at the netmap peers capabilities and finds which peers // want to be connectors for which domains. -func PickSplitDNSPeers(hasCap func(c tailcfg.NodeCapability) bool, self tailcfg.NodeView, peers map[tailcfg.NodeID]tailcfg.NodeView) map[string][]tailcfg.NodeView { +func PickSplitDNSPeers(hasCap func(c tailcfg.NodeCapability) bool, self tailcfg.NodeView, peers map[tailcfg.NodeID]tailcfg.NodeView, isSelfEligibleConnector bool) map[string][]tailcfg.NodeView { var m map[string][]tailcfg.NodeView if !hasCap(AppConnectorsExperimentalAttrName) { return m @@ -65,21 +65,34 @@ func PickSplitDNSPeers(hasCap func(c tailcfg.NodeCapability) bool, self tailcfg. return m } tagToDomain := make(map[string][]string) + selfTags := set.SetOf(self.Tags().AsSlice()) + selfRoutedDomains := set.Set[string]{} for _, app := range apps { for _, tag := range app.Connectors { - tagToDomain[tag] = append(tagToDomain[tag], app.Domains...) + domains := tagToDomain[tag] + domains = slices.Grow(domains, len(app.Domains)) + for _, d := range app.Domains { + if isSelfEligibleConnector && selfTags.Contains(tag) { + selfRoutedDomains.Add(d) + } + domains = append(domains, d) + } + tagToDomain[tag] = domains } } // NodeIDs are Comparable, and we have a map of NodeID to NodeView anyway, so // use a Set of NodeIDs to deduplicate, and populate into a []NodeView later. var work map[string]set.Set[tailcfg.NodeID] for _, peer := range peers { - if !isEligibleConnector(peer) { + if !isPeerEligibleConnector(peer) { continue } for _, t := range peer.Tags().All() { domains := tagToDomain[t] for _, domain := range domains { + if selfRoutedDomains.Contains(domain) { + continue + } if work[domain] == nil { mak.Set(&work, domain, set.Set[tailcfg.NodeID]{}) } diff --git a/appc/conn25_test.go b/appc/conn25_test.go index fc14caf36..ae5d59d63 100644 --- a/appc/conn25_test.go +++ b/appc/conn25_test.go @@ -47,10 +47,12 @@ func TestPickSplitDNSPeers(t *testing.T) { nvp4 := makeNodeView(4, "p4", []string{"tag:two", "tag:three2", "tag:four2"}) for _, tt := range []struct { - name string - want map[string][]tailcfg.NodeView - peers []tailcfg.NodeView - config []tailcfg.RawMessage + name string + peers []tailcfg.NodeView + config []tailcfg.RawMessage + isEligibleConnector bool + selfTags []string + want map[string][]tailcfg.NodeView }{ { name: "empty", @@ -111,6 +113,85 @@ func TestPickSplitDNSPeers(t *testing.T) { "c.example.com": {nvp2, nvp4}, }, }, + { + name: "self-connector-exclude-self-domains", + config: []tailcfg.RawMessage{ + tailcfg.RawMessage(appOneBytes), + tailcfg.RawMessage(appTwoBytes), + tailcfg.RawMessage(appThreeBytes), + tailcfg.RawMessage(appFourBytes), + }, + peers: []tailcfg.NodeView{ + nvp1, + nvp2, + nvp3, + nvp4, + }, + isEligibleConnector: true, + selfTags: []string{"tag:three1"}, + want: map[string][]tailcfg.NodeView{ + // woo.b.example.com and hoo.b.example.com are covered + // by tag:three1, and so is this self-node. + // So those domains should not be routed to peers. + // woo.b.example.com is also covered by another tag, + // but still not included since this connector can route to it. + "example.com": {nvp1}, + "a.example.com": {nvp3, nvp4}, + "c.example.com": {nvp2, nvp4}, + }, + }, + { + name: "self-eligible-connector-no-matching-tag-include-all-domains", + config: []tailcfg.RawMessage{ + tailcfg.RawMessage(appOneBytes), + tailcfg.RawMessage(appTwoBytes), + tailcfg.RawMessage(appThreeBytes), + tailcfg.RawMessage(appFourBytes), + }, + peers: []tailcfg.NodeView{ + nvp1, + nvp2, + nvp3, + nvp4, + }, + isEligibleConnector: true, + selfTags: []string{"tag:unrelated"}, + want: map[string][]tailcfg.NodeView{ + // Self has prefs set but no tags matching any app, + // so no domains are self-routed and all appear. + "example.com": {nvp1}, + "a.example.com": {nvp3, nvp4}, + "woo.b.example.com": {nvp2, nvp3, nvp4}, + "hoo.b.example.com": {nvp3, nvp4}, + "c.example.com": {nvp2, nvp4}, + }, + }, + { + name: "self-not-eligible-connector-but-tagged-include-all-domains", + config: []tailcfg.RawMessage{ + tailcfg.RawMessage(appOneBytes), + tailcfg.RawMessage(appTwoBytes), + tailcfg.RawMessage(appThreeBytes), + tailcfg.RawMessage(appFourBytes), + }, + peers: []tailcfg.NodeView{ + nvp1, + nvp2, + nvp3, + nvp4, + }, + selfTags: []string{"tag:three1"}, + want: map[string][]tailcfg.NodeView{ + // Even though this self node has a tag for an app + // the prefs don't advertise as connector, so + // should still route through other connectors. + "example.com": {nvp1}, + "a.example.com": {nvp3, nvp4}, + "woo.b.example.com": {nvp2, nvp3, nvp4}, + "hoo.b.example.com": {nvp3, nvp4}, + "c.example.com": {nvp2, nvp4}, + }, + }, } { t.Run(tt.name, func(t *testing.T) { selfNode := &tailcfg.Node{} @@ -119,6 +200,7 @@ func TestPickSplitDNSPeers(t *testing.T) { tailcfg.NodeCapability(AppConnectorsExperimentalAttrName): tt.config, } } + selfNode.Tags = append(selfNode.Tags, tt.selfTags...) selfView := selfNode.View() peers := map[tailcfg.NodeID]tailcfg.NodeView{} for _, p := range tt.peers { @@ -126,7 +208,8 @@ func TestPickSplitDNSPeers(t *testing.T) { } got := PickSplitDNSPeers(func(_ tailcfg.NodeCapability) bool { return true - }, selfView, peers) + }, selfView, peers, tt.isEligibleConnector) + if !reflect.DeepEqual(got, tt.want) { t.Fatalf("got %v, want %v", got, tt.want) } diff --git a/feature/conn25/conn25.go b/feature/conn25/conn25.go index e716c09d0..e5db9619b 100644 --- a/feature/conn25/conn25.go +++ b/feature/conn25/conn25.go @@ -25,6 +25,7 @@ import ( "tailscale.com/appc" "tailscale.com/envknob" "tailscale.com/feature" + "tailscale.com/ipn" "tailscale.com/ipn/ipnext" "tailscale.com/ipn/ipnlocal" "tailscale.com/net/packet" @@ -124,6 +125,7 @@ func (e *extension) Init(host ipnext.Host) error { if err := e.installHooks(dph); err != nil { return err } + e.seedPrefsConfig() ctx, cancel := context.WithCancelCause(context.Background()) e.ctxCancel = cancel @@ -167,10 +169,13 @@ func (e *extension) installHooks(dph *datapathHandler) error { } // Manage how we react to changes to the current node, - // including property changes (e.g. HostInfo, Capabilities, CapMap) - // and profile switches. + // including property changes (e.g. HostInfo, Capabilities, CapMap). e.host.Hooks().OnSelfChange.Add(e.onSelfChange) + // Manage how we react profile state changes, which include + // prefs changes. + e.host.Hooks().ProfileStateChange.Add(e.profileStateChange) + // Allow the client to send packets with Transit IP destinations // in the link-local space. e.host.Hooks().Filter.LinkLocalAllowHooks.Add(func(p packet.Parsed) (bool, string) { @@ -217,6 +222,15 @@ func (e *extension) installHooks(dph *datapathHandler) error { return nil } +// seedPrefsConfig provides an initial prefs config before +// any hooks fire. The OnSelfChange hook needs to fire +// for Conn25 to be fully configured and ready to use. +func (e *extension) seedPrefsConfig() { + var cfg config + cfg.prefs = configFromPrefs(e.host.Profiles().CurrentPrefs()) + e.conn25.reconfig(cfg) +} + // ClientTransitIPForMagicIP implements [IPMapper]. func (c *Conn25) ClientTransitIPForMagicIP(m netip.Addr) (netip.Addr, error) { return c.client.transitIPForMagicIP(m) @@ -229,7 +243,7 @@ func (c *Conn25) ConnectorRealIPForTransitIPConnection(src, transit netip.Addr) func (e *extension) getMagicRange() views.Slice[netip.Prefix] { cfg := e.conn25.client.getConfig() - return views.SliceOf(slices.Concat(cfg.v4MagicIPSet.Prefixes(), cfg.v6MagicIPSet.Prefixes())) + return views.SliceOf(slices.Concat(cfg.nv.v4MagicIPSet.Prefixes(), cfg.nv.v6MagicIPSet.Prefixes())) } // Shutdown implements [ipnlocal.Extension]. @@ -264,12 +278,30 @@ func (e *extension) handleConnectorTransitIP(h ipnlocal.PeerAPIHandler, w http.R w.Write(bs) } +// onSelfChange implements the [ipnext.Hooks.OnSelfChange] hook. +// It reads then modifies the current config. We expect that OnSelfChange +// is not called concurrently with itself or with ProfileStateChange to +// prevent TOCTOU errors. func (e *extension) onSelfChange(selfNode tailcfg.NodeView) { - err := e.conn25.reconfig(selfNode) + cfg := e.conn25.client.getConfig() + nvCfg, err := configFromNodeView(selfNode) if err != nil { - e.conn25.client.logf("error during Reconfig onSelfChange: %v", err) + e.conn25.client.logf("error generating config from self node view: %v", err) return } + cfg.nv = nvCfg + e.conn25.reconfig(cfg) +} + +// profileStateChange implements the [ipnext.Hooks.ProfileStateChange] hook. +// It reads then modifies the current config. We expect that ProfileStateChange +// is not called concurrently with itself or with OnSelfChange to +// prevent TOCTOU errors. +func (e *extension) profileStateChange(loginProfile ipn.LoginProfileView, prefs ipn.PrefsView, sameNode bool) { + // TODO(mzb): Handle node changes. Wipe out all config? + cfg := e.conn25.client.getConfig() + cfg.prefs = configFromPrefs(prefs) + e.conn25.reconfig(cfg) } func (e *extension) extraWireGuardAllowedIPs(k key.NodePublic) views.Slice[netip.Prefix] { @@ -283,6 +315,7 @@ type appAddr struct { // Conn25 holds state for routing traffic for a domain via a connector. type Conn25 struct { + mu sync.Mutex // mu protects reconfiguration of client and connector client *client connector *connector } @@ -310,18 +343,12 @@ func ipSetFromIPRanges(rs []netipx.IPRange) (*netipx.IPSet, error) { return b.IPSet() } -func (c *Conn25) reconfig(selfNode tailcfg.NodeView) error { - cfg, err := configFromNodeView(selfNode) - if err != nil { - return err - } - if err := c.client.reconfig(cfg); err != nil { - return err - } - if err := c.connector.reconfig(cfg); err != nil { - return err - } - return nil +func (c *Conn25) reconfig(cfg config) { + c.mu.Lock() + defer c.mu.Unlock() + + c.client.reconfig(cfg) + c.connector.reconfig(cfg) } // mapDNSResponse parses and inspects the DNS response, and uses the @@ -397,7 +424,7 @@ func (c *connector) handleTransitIPRequest(n tailcfg.NodeView, peerV4 netip.Addr c.mu.Lock() defer c.mu.Unlock() - if _, ok := c.config.appsByName[tipr.App]; !ok { + if _, ok := c.config.nv.appsByName[tipr.App]; !ok { c.logf("[Unexpected] peer attempt to map a transit IP referenced unknown app: node: %s, app: %q", n.StableID(), tipr.App) return TransitIPResponse{Code: UnknownAppName, Message: unknownAppNameMessage} @@ -485,69 +512,71 @@ type ConnectorTransitIPResponse struct { const AppConnectorsExperimentalAttrName = "tailscale.com/app-connectors-experimental" -// config holds the config from the policy and lookups derived from that. -// config is not safe for concurrent use. -type config struct { - isConfigured bool - apps []appctype.Conn25Attr - appsByName map[string]appctype.Conn25Attr - appNamesByDomain map[dnsname.FQDN][]string - selfRoutedDomains set.Set[dnsname.FQDN] - v4TransitIPSet netipx.IPSet - v4MagicIPSet netipx.IPSet - v6TransitIPSet netipx.IPSet - v6MagicIPSet netipx.IPSet +// nodeViewConfig holds the config derived from the self node view, +// which includes the policy. +// nodeViewConfig is not safe for concurrent use. +type nodeViewConfig struct { + isConfigured bool + apps []appctype.Conn25Attr + appsByName map[string]appctype.Conn25Attr + appNamesByDomain map[dnsname.FQDN][]string + selfDomains set.Set[dnsname.FQDN] + v4TransitIPSet netipx.IPSet + v4MagicIPSet netipx.IPSet + v6TransitIPSet netipx.IPSet + v6MagicIPSet netipx.IPSet } -func configFromNodeView(n tailcfg.NodeView) (config, error) { +func configFromNodeView(n tailcfg.NodeView) (nodeViewConfig, error) { apps, err := tailcfg.UnmarshalNodeCapViewJSON[appctype.Conn25Attr](n.CapMap(), AppConnectorsExperimentalAttrName) if err != nil { - return config{}, err + return nodeViewConfig{}, err } if len(apps) == 0 { - return config{}, nil + return nodeViewConfig{}, nil } selfTags := set.SetOf(n.Tags().AsSlice()) - cfg := config{ - isConfigured: true, - apps: apps, - appsByName: map[string]appctype.Conn25Attr{}, - appNamesByDomain: map[dnsname.FQDN][]string{}, - selfRoutedDomains: set.Set[dnsname.FQDN]{}, + cfg := nodeViewConfig{ + isConfigured: true, + apps: apps, + appsByName: map[string]appctype.Conn25Attr{}, + appNamesByDomain: map[dnsname.FQDN][]string{}, + selfDomains: set.Set[dnsname.FQDN]{}, } for _, app := range apps { selfMatchesTags := slices.ContainsFunc(app.Connectors, selfTags.Contains) for _, d := range app.Domains { fqdn, err := normalizeDNSName(d) if err != nil { - return config{}, err + return nodeViewConfig{}, err } mak.Set(&cfg.appNamesByDomain, fqdn, append(cfg.appNamesByDomain[fqdn], app.Name)) if selfMatchesTags { - cfg.selfRoutedDomains.Add(fqdn) + cfg.selfDomains.Add(fqdn) } } mak.Set(&cfg.appsByName, app.Name, app) } + // TODO(fran) 2026-03-18 we don't yet have a proper way to communicate the // global IP pool config. For now just take it from the first app. if len(apps) != 0 { app := apps[0] v4Mipp, err := ipSetFromIPRanges(app.V4MagicIPPool) if err != nil { - return config{}, err + return nodeViewConfig{}, err } v4Tipp, err := ipSetFromIPRanges(app.V4TransitIPPool) if err != nil { - return config{}, err + return nodeViewConfig{}, err } v6Mipp, err := ipSetFromIPRanges(app.V6MagicIPPool) if err != nil { - return config{}, err + return nodeViewConfig{}, err } v6Tipp, err := ipSetFromIPRanges(app.V6TransitIPPool) if err != nil { - return config{}, err + return nodeViewConfig{}, err } cfg.v4MagicIPSet = *v4Mipp cfg.v4TransitIPSet = *v4Tipp @@ -557,6 +586,33 @@ func configFromNodeView(n tailcfg.NodeView) (config, error) { return cfg, nil } +// prefsConfig holds the config derived from the current prefs. +// prefsConfig is not safe for concurrent use. +type prefsConfig struct { + isConfigured bool + isEligibleConnector bool +} + +func configFromPrefs(prefs ipn.PrefsView) prefsConfig { + return prefsConfig{ + isConfigured: true, + isEligibleConnector: prefs.AppConnector().Advertise, + } +} + +// config wraps the config from disparate sources. +// config is not safe for concurrent use. +type config struct { + nv nodeViewConfig + prefs prefsConfig +} + +// isSelfRoutedDomain reports whether the self node is currently +// acting as a connector for the given domain. +func (c config) isSelfRoutedDomain(d dnsname.FQDN) bool { + return c.prefs.isEligibleConnector && c.nv.selfDomains.Contains(d) +} + // client performs the conn25 functionality for clients of connectors // It allocates magic and transit IP addresses and communicates them with // connectors. @@ -589,7 +645,7 @@ func (c *client) transitIPForMagicIP(magicIP netip.Addr) (netip.Addr, error) { if ok { return v.transit, nil } - if !c.config.v4MagicIPSet.Contains(magicIP) && !c.config.v6MagicIPSet.Contains(magicIP) { + if !c.config.nv.v4MagicIPSet.Contains(magicIP) && !c.config.nv.v6MagicIPSet.Contains(magicIP) { return netip.Addr{}, nil } return netip.Addr{}, ErrUnmappedMagicIP @@ -615,29 +671,40 @@ func (c *client) isKnownTransitIP(tip netip.Addr) bool { return ok } +// isConfigured reports whether the client has received both a node view +// config (from the netmap/policy) and a prefs config. Both are required +// before the feature is active. func (c *client) isConfigured() bool { c.mu.Lock() defer c.mu.Unlock() - return c.config.isConfigured + return c.config.prefs.isConfigured && c.config.nv.isConfigured } -func (c *client) reconfig(newCfg config) error { +func (c *client) reconfig(newCfg config) { c.mu.Lock() defer c.mu.Unlock() c.config = newCfg - c.v4MagicIPPool = newIPPool(&(newCfg.v4MagicIPSet)) - c.v4TransitIPPool = newIPPool(&(newCfg.v4TransitIPSet)) - c.v6MagicIPPool = newIPPool(&(newCfg.v6MagicIPSet)) - c.v6TransitIPPool = newIPPool(&(newCfg.v6TransitIPSet)) - return nil + c.v4MagicIPPool = newIPPool(&(newCfg.nv.v4MagicIPSet)) + c.v4TransitIPPool = newIPPool(&(newCfg.nv.v4TransitIPSet)) + c.v6MagicIPPool = newIPPool(&(newCfg.nv.v6MagicIPSet)) + c.v6TransitIPPool = newIPPool(&(newCfg.nv.v6TransitIPSet)) } +// isConnectorDomain returns true if the domain is expected +// to be routed through a peer connector, but returns false +// if the self node is a connector responsible for routing the +// domain, and false in all other cases. func (c *client) isConnectorDomain(domain dnsname.FQDN) bool { c.mu.Lock() defer c.mu.Unlock() - appNames, ok := c.config.appNamesByDomain[domain] + + if c.config.isSelfRoutedDomain(domain) { + return false + } + + appNames, ok := c.config.nv.appNamesByDomain[domain] return ok && len(appNames) > 0 } @@ -654,7 +721,7 @@ func (c *client) reserveAddresses(domain dnsname.FQDN, dst netip.Addr) (addrs, e if existing, ok := c.assignments.lookupByDomainDst(domain, dst); ok { return existing, nil } - appNames, _ := c.config.appNamesByDomain[domain] + appNames, _ := c.config.nv.appNamesByDomain[domain] if len(appNames) == 0 { return addrs{}, fmt.Errorf("no app names found for domain %q", domain) } @@ -802,7 +869,7 @@ func makePeerAPIReq(ctx context.Context, httpClient *http.Client, urlBase string } func (e *extension) sendAddressAssignment(ctx context.Context, as addrs) (tailcfg.NodeView, error) { - app, ok := e.conn25.client.getConfig().appsByName[as.app] + app, ok := e.conn25.client.getConfig().nv.appsByName[as.app] if !ok { e.conn25.client.logf("App not found for app: %s (domain: %s)", as.app, as.domain) return tailcfg.NodeView{}, errors.New("app not found") @@ -1056,7 +1123,7 @@ func (c *connector) realIPForTransitIPConnection(srcIP netip.Addr, transitIP net if ok { return v.addr, nil } - if !c.config.v4TransitIPSet.Contains(transitIP) && !c.config.v6TransitIPSet.Contains(transitIP) { + if !c.config.nv.v4TransitIPSet.Contains(transitIP) && !c.config.nv.v6TransitIPSet.Contains(transitIP) { return netip.Addr{}, nil } return netip.Addr{}, ErrUnmappedSrcAndTransitIP @@ -1085,11 +1152,10 @@ func (c *connector) lookupBySrcIPAndTransitIP(srcIP, transitIP netip.Addr) (appA return v, ok } -func (c *connector) reconfig(newCfg config) error { +func (c *connector) reconfig(newCfg config) { c.mu.Lock() defer c.mu.Unlock() c.config = newCfg - return nil } type addrs struct { diff --git a/feature/conn25/conn25_test.go b/feature/conn25/conn25_test.go index 5f136c556..8e829724b 100644 --- a/feature/conn25/conn25_test.go +++ b/feature/conn25/conn25_test.go @@ -8,6 +8,7 @@ import ( "net/http" "net/http/httptest" "net/netip" + "reflect" "slices" "testing" "time" @@ -17,6 +18,7 @@ import ( "go4.org/mem" "go4.org/netipx" "golang.org/x/net/dns/dnsmessage" + "tailscale.com/ipn" "tailscale.com/ipn/ipnext" "tailscale.com/net/dns" "tailscale.com/net/packet" @@ -339,7 +341,9 @@ func TestHandleConnectorTransitIPRequest(t *testing.T) { // Use the same Conn25 for each request in the test and seed it with a test app name. c := newConn25(logger.Discard) c.connector.config = config{ - appsByName: map[string]appctype.Conn25Attr{appName: {}}, + nv: nodeViewConfig{ + appsByName: map[string]appctype.Conn25Attr{appName: {}}, + }, } for i, peer := range tt.ctipReqPeers { @@ -398,7 +402,7 @@ func TestReserveIPs(t *testing.T) { domainStr := "example.com." mbd := map[dnsname.FQDN][]string{} mbd["example.com."] = []string{app} - c.client.config.appNamesByDomain = mbd + c.client.config.nv.appNamesByDomain = mbd domain := must.Get(dnsname.ToFQDN(domainStr)) for _, tt := range []struct { @@ -453,29 +457,87 @@ func TestReconfig(t *testing.T) { } c := newConn25(logger.Discard) + if c.isConfigured() { + t.Fatal("expected Conn25 to report unconfigured before reconfig") + } + sn := (&tailcfg.Node{ CapMap: capMap, }).View() + cfg := mustConfig(t, sn, testPrefsIsConnector) + c.reconfig(cfg) - err := c.reconfig(sn) - if err != nil { - t.Fatal(err) + if !c.isConfigured() { + t.Fatal("expected Conn25 to report configured after reconfig") } - if len(c.client.config.apps) != 1 || c.client.config.apps[0].Name != "app1" { - t.Fatalf("want apps to have one entry 'app1', got %v", c.client.config.apps) + if !reflect.DeepEqual(c.client.config, c.connector.config) { + t.Fatal("client and connector have different configs") + } + + if len(c.client.config.nv.apps) != 1 || c.client.config.nv.apps[0].Name != "app1" { + t.Fatalf("want apps to have one entry 'app1', got %v", c.client.config.nv.apps) + } + + if c.client.config.prefs.isEligibleConnector != true { + t.Fatal("want prefs config to have isEligibleConnector=true") + } + + c.reconfig(config{ + prefs: prefsConfig{isConfigured: true}, + }) + if c.isConfigured() { + t.Fatal("expected Conn25 to report unconfigured with only prefs config") + } + + c.reconfig(config{ + nv: nodeViewConfig{isConfigured: true}, + }) + if c.isConfigured() { + t.Fatal("expected Conn25 to report unconfigured with only nodeview config") } } -func TestConfigReconfig(t *testing.T) { +func TestConfigFromPrefs(t *testing.T) { for _, tt := range []struct { - name string - rawCfg string - cfg []appctype.Conn25Attr - tags []string - wantErr bool - wantAppsByDomain map[dnsname.FQDN][]string - wantSelfRoutedDomains set.Set[dnsname.FQDN] + name string + prefs ipn.PrefsView + expected prefsConfig + }{ + { + name: "is-eligible-connector", + prefs: testPrefsIsConnector, + expected: prefsConfig{ + isConfigured: true, + isEligibleConnector: true, + }, + }, + { + name: "not-eligible-connector", + prefs: testPrefsNotConnector, + expected: prefsConfig{ + isConfigured: true, + isEligibleConnector: false, + }, + }, + } { + t.Run(tt.name, func(t *testing.T) { + if diff := cmp.Diff(tt.expected, configFromPrefs(tt.prefs), cmp.AllowUnexported(prefsConfig{})); diff != "" { + t.Errorf("unexpected prefsConfig (-want,+got): %s", diff) + } + }) + } +} + +func TestConfigFromNodeView(t *testing.T) { + for _, tt := range []struct { + name string + rawCfg string + cfg []appctype.Conn25Attr + tags []string + wantErr bool + wantAppsByDomain map[dnsname.FQDN][]string + wantSelfDomains set.Set[dnsname.FQDN] }{ { name: "bad-config", @@ -493,10 +555,10 @@ func TestConfigReconfig(t *testing.T) { "a.example.com.": {"one"}, "b.example.com.": {"two"}, }, - wantSelfRoutedDomains: set.SetOf([]dnsname.FQDN{"a.example.com."}), + wantSelfDomains: set.SetOf([]dnsname.FQDN{"a.example.com."}), }, { - name: "more-complex", + name: "more-complex-with-connector-self-domains", cfg: []appctype.Conn25Attr{ {Name: "one", Domains: []string{"1.a.example.com", "1.b.example.com"}, Connectors: []string{"tag:one", "tag:onea"}}, {Name: "two", Domains: []string{"2.b.example.com", "2.c.example.com"}, Connectors: []string{"tag:two", "tag:twoa"}}, @@ -513,7 +575,19 @@ func TestConfigReconfig(t *testing.T) { "4.b.example.com.": {"four"}, "4.d.example.com.": {"four"}, }, - wantSelfRoutedDomains: set.SetOf([]dnsname.FQDN{"1.a.example.com.", "1.b.example.com.", "4.b.example.com.", "4.d.example.com."}), + wantSelfDomains: set.SetOf([]dnsname.FQDN{"1.a.example.com.", "1.b.example.com.", "4.b.example.com.", "4.d.example.com."}), + }, + { + name: "eligible-connector-no-matching-tag-no-self-domains", + cfg: []appctype.Conn25Attr{ + {Name: "one", Domains: []string{"a.example.com"}, Connectors: []string{"tag:one"}}, + {Name: "two", Domains: []string{"b.example.com"}, Connectors: []string{"tag:two"}}, + }, + tags: []string{"tag:unrelated"}, + wantAppsByDomain: map[dnsname.FQDN][]string{ + "a.example.com.": {"one"}, + "b.example.com.": {"two"}, + }, }, } { t.Run(tt.name, func(t *testing.T) { @@ -535,6 +609,7 @@ func TestConfigReconfig(t *testing.T) { CapMap: capMap, Tags: tt.tags, }).View() + c, err := configFromNodeView(sn) if (err != nil) != tt.wantErr { t.Fatalf("wantErr: %t, err: %v", tt.wantErr, err) @@ -542,8 +617,8 @@ func TestConfigReconfig(t *testing.T) { if diff := cmp.Diff(tt.wantAppsByDomain, c.appNamesByDomain); diff != "" { t.Errorf("appsByDomain diff (-want, +got):\n%s", diff) } - if diff := cmp.Diff(tt.wantSelfRoutedDomains, c.selfRoutedDomains); diff != "" { - t.Errorf("selfRoutedDomains diff (-want, +got):\n%s", diff) + if diff := cmp.Diff(tt.wantSelfDomains, c.selfDomains); diff != "" { + t.Errorf("selfDomains diff (-want, +got):\n%s", diff) } }) } @@ -562,12 +637,33 @@ func makeSelfNode(t *testing.T, attrs []appctype.Conn25Attr, tags []string) tail capMap := tailcfg.NodeCapMap{ tailcfg.NodeCapability(AppConnectorsExperimentalAttrName): cfg, } + return (&tailcfg.Node{ CapMap: capMap, Tags: tags, }).View() } +var ( + testPrefsIsConnector = (&ipn.Prefs{AppConnector: ipn.AppConnectorPrefs{Advertise: true}}).View() + testPrefsNotConnector = (&ipn.Prefs{AppConnector: ipn.AppConnectorPrefs{Advertise: false}}).View() +) + +func mustConfig(t *testing.T, selfNode tailcfg.NodeView, prefs ipn.PrefsView) config { + t.Helper() + nvCfg, err := configFromNodeView(selfNode) + if err != nil { + t.Fatal(err) + } + + prefsCfg := configFromPrefs(prefs) + + return config{ + nv: nvCfg, + prefs: prefsCfg, + } +} + func v4RangeFrom(from, to string) netipx.IPRange { return netipx.IPRangeFrom( netip.MustParseAddr("100.64.0."+from), @@ -706,11 +802,13 @@ func makeDNSResponseForSections(t *testing.T, questions []dnsmessage.Question, a func TestMapDNSResponseAssignsAddrs(t *testing.T) { for _, tt := range []struct { - name string - domain string - v4Addrs []*dnsmessage.AResource - v6Addrs []*dnsmessage.AAAAResource - wantByMagicIP map[netip.Addr]addrs + name string + domain string + v4Addrs []*dnsmessage.AResource + v6Addrs []*dnsmessage.AAAAResource + selfTags []string + isEligibleConnector bool + wantByMagicIP map[netip.Addr]addrs }{ { name: "one-ip-matches", @@ -783,6 +881,48 @@ func TestMapDNSResponseAssignsAddrs(t *testing.T) { {A: [4]byte{2, 0, 0, 0}}, }, }, + { + name: "no-rewrite-self-routed-domain", + domain: "example.com.", + v4Addrs: []*dnsmessage.AResource{{A: [4]byte{1, 0, 0, 0}}}, + selfTags: []string{"tag:woo"}, + isEligibleConnector: true, + }, + { + name: "rewrite-tagged-but-not-eligible-connector", + domain: "example.com.", + v4Addrs: []*dnsmessage.AResource{{A: [4]byte{1, 0, 0, 0}}}, + selfTags: []string{"tag:woo"}, + // isEligibleConnector is false: tag matches but prefs not set, + // so DNS response should be rewritten normally. + wantByMagicIP: map[netip.Addr]addrs{ + netip.MustParseAddr("100.64.0.0"): { + domain: "example.com.", + dst: netip.MustParseAddr("1.0.0.0"), + magic: netip.MustParseAddr("100.64.0.0"), + transit: netip.MustParseAddr("100.64.0.40"), + app: "app1", + }, + }, + }, + { + name: "rewrite-eligible-connector-no-matching-tag", + domain: "example.com.", + v4Addrs: []*dnsmessage.AResource{{A: [4]byte{1, 0, 0, 0}}}, + selfTags: []string{"tag:unrelated"}, + isEligibleConnector: true, + // isEligibleConnector is true but tag doesn't match the app, + // so DNS response should be rewritten normally. + wantByMagicIP: map[netip.Addr]addrs{ + netip.MustParseAddr("100.64.0.0"): { + domain: "example.com.", + dst: netip.MustParseAddr("1.0.0.0"), + magic: netip.MustParseAddr("100.64.0.0"), + transit: netip.MustParseAddr("100.64.0.40"), + app: "app1", + }, + }, + }, } { t.Run(tt.name, func(t *testing.T) { var dnsResp []byte @@ -799,9 +939,15 @@ func TestMapDNSResponseAssignsAddrs(t *testing.T) { V6MagicIPPool: []netipx.IPRange{v6RangeFrom("0", "10"), v6RangeFrom("20", "30")}, V4TransitIPPool: []netipx.IPRange{v4RangeFrom("40", "50")}, V6TransitIPPool: []netipx.IPRange{v6RangeFrom("40", "50")}, - }}, []string{}) + }}, tt.selfTags) + prefs := testPrefsNotConnector + if tt.isEligibleConnector { + prefs = testPrefsIsConnector + } + c := newConn25(logger.Discard) - c.reconfig(sn) + cfg := mustConfig(t, sn, prefs) + c.reconfig(cfg) c.mapDNSResponse(dnsResp) if diff := cmp.Diff(tt.wantByMagicIP, c.client.assignments.byMagicIP, cmpopts.EquateComparable(addrs{}, netip.Addr{})); diff != "" { @@ -831,7 +977,7 @@ func TestReserveAddressesDeduplicated(t *testing.T) { c.client.v6MagicIPPool = newIPPool(mustIPSetFromPrefix("fd7a:115c:a1e0:a99c:0100::/80")) c.client.v4TransitIPPool = newIPPool(mustIPSetFromPrefix("169.254.0.0/24")) c.client.v6TransitIPPool = newIPPool(mustIPSetFromPrefix("fd7a:115c:a1e0:a99c:0200::/80")) - c.client.config.appNamesByDomain = map[dnsname.FQDN][]string{"example.com.": {"a"}} + c.client.config.nv.appNamesByDomain = map[dnsname.FQDN][]string{"example.com.": {"a"}} first, err := c.client.reserveAddresses("example.com.", tt.dst) if err != nil { @@ -880,16 +1026,25 @@ func (nb *testNodeBackend) PeerAPIBase(p tailcfg.NodeView) string { return nb.peerAPIURL } +type testProfileServices struct { + ipnext.ProfileServices + prefs ipn.PrefsView +} + +func (p *testProfileServices) CurrentPrefs() ipn.PrefsView { return p.prefs } + type testHost struct { ipnext.Host nb ipnext.NodeBackend hooks ipnext.Hooks + prefs ipn.PrefsView authReconfigAsync func() } -func (h *testHost) NodeBackend() ipnext.NodeBackend { return h.nb } -func (h *testHost) Hooks() *ipnext.Hooks { return &h.hooks } -func (h *testHost) AuthReconfigAsync() { h.authReconfigAsync() } +func (h *testHost) NodeBackend() ipnext.NodeBackend { return h.nb } +func (h *testHost) Hooks() *ipnext.Hooks { return &h.hooks } +func (h *testHost) Profiles() ipnext.ProfileServices { return &testProfileServices{prefs: h.prefs} } +func (h *testHost) AuthReconfigAsync() { h.authReconfigAsync() } type testSafeBackend struct { ipnext.SafeBackend @@ -950,6 +1105,7 @@ func TestAddressAssignmentIsHandled(t *testing.T) { peers: []tailcfg.NodeView{connectorPeer}, peerAPIURL: peersAPI.URL, }, + prefs: testPrefsNotConnector, authReconfigAsync: func() { authReconfigAsyncCalled <- struct{}{} }, @@ -963,10 +1119,9 @@ func TestAddressAssignmentIsHandled(t *testing.T) { Connectors: []string{"tag:woo"}, Domains: []string{"example.com"}, }}, []string{}) - err := ext.conn25.reconfig(sn) - if err != nil { - t.Fatal(err) - } + + cfg := mustConfig(t, sn, testPrefsNotConnector) + ext.conn25.reconfig(cfg) as := addrs{ dst: netip.MustParseAddr("1.2.3.4"), @@ -1046,6 +1201,8 @@ func TestMapDNSResponseRewritesResponses(t *testing.T) { V6TransitIPPool: []netipx.IPRange{netipx.IPRangeFrom(netip.MustParseAddr("2606:4700::6813:100"), netip.MustParseAddr("2606:4700::6813:1ff"))}, }}, []string{}) + cfg := mustConfig(t, sn, testPrefsNotConnector) + compareToRecords := func(t *testing.T, resources []dnsmessage.Resource, want []netip.Addr) { t.Helper() var got []netip.Addr @@ -1323,9 +1480,7 @@ func TestMapDNSResponseRewritesResponses(t *testing.T) { } { t.Run(tt.name, func(t *testing.T) { c := newConn25(logger.Discard) - if err := c.reconfig(sn); err != nil { - t.Fatal(err) - } + c.reconfig(cfg) bs := c.mapDNSResponse(tt.toMap) tt.assertFx(t, bs) }) @@ -1376,6 +1531,7 @@ func TestHandleAddressAssignmentStoresTransitIPs(t *testing.T) { peers: connectorPeers, peerAPIURL: peersAPI.URL, }, + prefs: testPrefsNotConnector, authReconfigAsync: func() { authReconfigAsyncCalled <- struct{}{} }, @@ -1396,10 +1552,9 @@ func TestHandleAddressAssignmentStoresTransitIPs(t *testing.T) { Domains: []string{"hoo.example.com"}, }, }, []string{}) - err := ext.conn25.reconfig(sn) - if err != nil { - t.Fatal(err) - } + + cfg := mustConfig(t, sn, testPrefsNotConnector) + ext.conn25.reconfig(cfg) type lookup struct { connKey key.NodePublic @@ -1576,6 +1731,7 @@ func TestClientTransitIPForMagicIP(t *testing.T) { V4MagicIPPool: []netipx.IPRange{v4RangeFrom("0", "10")}, // 100.64.0.0 - 100.64.0.10 V6MagicIPPool: []netipx.IPRange{v6RangeFrom("0", "10")}, }}, []string{}) + cfg := mustConfig(t, sn, testPrefsNotConnector) mappedMip := netip.MustParseAddr("100.64.0.0") mappedTip := netip.MustParseAddr("169.0.0.0") @@ -1634,9 +1790,8 @@ func TestClientTransitIPForMagicIP(t *testing.T) { } { t.Run(tt.name, func(t *testing.T) { c := newConn25(t.Logf) - if err := c.reconfig(sn); err != nil { - t.Fatal(err) - } + c.reconfig(cfg) + if err := c.client.assignments.insert(addrs{ magic: mappedMip, transit: mappedTip, @@ -1666,6 +1821,8 @@ func TestConnectorRealIPForTransitIPConnection(t *testing.T) { sn := makeSelfNode(t, []appctype.Conn25Attr{{ V4TransitIPPool: []netipx.IPRange{v4RangeFrom("40", "50")}, // 100.64.0.40 - 100.64.0.50 }}, []string{}) + cfg := mustConfig(t, sn, testPrefsIsConnector) + mappedSrc := netip.MustParseAddr("100.0.0.1") unmappedSrc := netip.MustParseAddr("100.0.0.2") mappedTip := netip.MustParseAddr("100.64.0.41") @@ -1717,9 +1874,7 @@ func TestConnectorRealIPForTransitIPConnection(t *testing.T) { } { t.Run(tt.name, func(t *testing.T) { c := newConn25(t.Logf) - if err := c.reconfig(sn); err != nil { - t.Fatal(err) - } + c.reconfig(cfg) c.connector.transitIPs = map[netip.Addr]map[netip.Addr]appAddr{} c.connector.transitIPs[mappedSrc] = map[netip.Addr]appAddr{} c.connector.transitIPs[mappedSrc][mappedTip] = appAddr{addr: mappedMip} @@ -1812,10 +1967,9 @@ func TestGetMagicRange(t *testing.T) { V4MagicIPPool: []netipx.IPRange{netipx.IPRangeFrom(netip.MustParseAddr("0.0.0.1"), netip.MustParseAddr("0.0.0.3"))}, V6MagicIPPool: []netipx.IPRange{netipx.IPRangeFrom(netip.MustParseAddr("::1"), netip.MustParseAddr("::3"))}, }}, []string{}) + cfg := mustConfig(t, sn, testPrefsNotConnector) c := newConn25(t.Logf) - if err := c.reconfig(sn); err != nil { - t.Fatal(err) - } + c.reconfig(cfg) ext := &extension{ conn25: c, } diff --git a/ipn/ipnlocal/node_backend.go b/ipn/ipnlocal/node_backend.go index 75550b3d5..6c5db0e0d 100644 --- a/ipn/ipnlocal/node_backend.go +++ b/ipn/ipnlocal/node_backend.go @@ -864,7 +864,7 @@ func dnsConfigForNetmap(nm *netmap.NetworkMap, peers map[tailcfg.NodeID]tailcfg. addSplitDNSRoutes(nm.DNS.Routes) // Add split DNS routes for conn25 - conn25DNSTargets := appc.PickSplitDNSPeers(nm.HasCap, nm.SelfNode, peers) + conn25DNSTargets := appc.PickSplitDNSPeers(nm.HasCap, nm.SelfNode, peers, prefs.AppConnector().Advertise) if conn25DNSTargets != nil { var m map[string][]*dnstype.Resolver for domain, candidateSplitDNSPeers := range conn25DNSTargets {