ipn/ipnlocal: Support TCP and Web VIP services #14508

KevinLiang10 · 2025-01-02T20:28:07Z

This PR intend to provide support for TCP and Web VIP services and also allow user to use Tun for VIP services if they want to.

The first commit includes:
1.Setting TCP intercept function for VIP Services.
2.Update netstack to send packet written from WG to netStack handler for VIP service.
3.Return correct TCP hander for VIP services when netstack acceptTCP.

This commit also includes unit tests for if the local backend setServeConfig would set correct TCP intercept
function and test if a hander gets returned when getting TCPHandlerForDst. The shouldProcessInbound
check is not unit tested since the test result just depends on mocked functions. There should be an integration
test to cover shouldProcessInbound and if the returned TCP handler actually does what the serveConfig says.

Updates https://github.com/tailscale/corp/issues/24604

Signed-off-by: KevinLiang10 37811973+KevinLiang10@users.noreply.github.com

This commit intend to provide support for TCP and Web VIP services and also allow user to use Tun for VIP services if they want to. The commit includes: 1.Setting TCP intercept function for VIP Services. 2.Update netstack to send packet written from WG to netStack handler for VIP service. 3.Return correct TCP hander for VIP services when netstack acceptTCP. This commit also includes unit tests for if the local backend setServeConfig would set correct TCP intercept function and test if a hander gets returned when getting TCPHandlerForDst. The shouldProcessInbound check is not unit tested since the test result just depends on mocked functions. There should be an integration test to cover shouldProcessInbound and if the returned TCP handler actually does what the serveConfig says. Updates tailscale/corp#24604 Signed-off-by: KevinLiang10 <37811973+KevinLiang10@users.noreply.github.com>

ipn/ipnlocal/serve_test.go

bradfitz · 2025-01-20T17:44:29Z

ipn/serve.go

@@ -662,6 +682,17 @@ func (v ServeConfigView) HasFunnelForTarget(target HostPort) bool {
 	return false
 }

+// HasValidServicesConfig reports whether if ServeConfig has at least


just "whether" not "whether if"

but "whether" wording is normally used only for bool results. This returns an error.

So also the "at least one" wording is weird with an error result.

what about
"CheckValidServicesConfig reports whether the ServeConfig has invalid service configurations."

ipn/serve.go

types/netmap/netmap.go

ipn/serve.go

bradfitz · 2025-01-20T17:51:34Z

ipn/serve.go

-	// Services maps from service name to a ServiceConfig. Which describes the
-	// L3, L4, and L7 forwarding information for the service.
+	// Services maps from service name (in the form "svc:dns_label") to a ServiceConfig.
+	// Which describes the L3, L4, and L7 forwarding information for the service.


What L3 stuff do we do?

We don't do anything. If Tun is set to true, we just let the packets pass to Tun and user can do their own customization to route then to their wanted destination.

Signed-off-by: KevinLiang10 <37811973+KevinLiang10@users.noreply.github.com>

sailorfrag · 2025-01-20T21:19:15Z

ipn/ipnlocal/local.go

+	if vipServiceIPMap == nil {
+		b.logf("can't set intercept function for Service TCP Ports, VIPServiceAddrInfo is nil")
+		return
+	}


Suggested change

if vipServiceIPMap == nil {

b.logf("can't set intercept function for Service TCP Ports, VIPServiceAddrInfo is nil")

return

}

if len(vipServiceIPMap) == 0 {

// No approved VIP Services

return

}

sailorfrag · 2025-01-20T21:20:18Z

ipn/ipnlocal/local.go

+		return
+	}
+
+	svcAddrPorts := map[netip.Addr]func(uint16) bool{}


I believe the convention is usually to make a map so that it has some empty space allocated.

Suggested change

svcAddrPorts := map[netip.Addr]func(uint16) bool{}

svcAddrPorts := make(map[netip.Addr]func(uint16) bool)

sailorfrag · 2025-01-20T21:21:50Z

ipn/ipnlocal/local.go

+
+	svcAddrPorts := map[netip.Addr]func(uint16) bool{}
+	// Only set the intercept function if the service has been assigned a VIP.
+	for svcName, ports := range svcPorts {


This is fine, but I feel like it might make more sense to range on vipServiceIPMap since I'd expect that to be the smaller of the two maps.

sailorfrag · 2025-01-20T21:22:30Z

ipn/ipnlocal/local.go

+	for svcName, ports := range svcPorts {
+		if addrs, ok := vipServiceIPMap[svcName]; ok {
+			for _, addr := range addrs {
+				svcAddrPorts[addr] = generateInterceptTCPPortFunc(ports)


I'd recommend it generate the func once outside the loop

sailorfrag · 2025-01-20T21:26:54Z

ipn/ipnlocal/local.go

@@ -6013,10 +6095,11 @@ func (b *LocalBackend) setTCPPortsInterceptedFromNetmapAndPrefsLocked(prefs ipn.
 	if wire := b.wantIngressLocked(); b.hostinfo != nil && b.hostinfo.WireIngress != wire {
 		b.logf("Hostinfo.WireIngress changed to %v", wire)
 		b.hostinfo.WireIngress = wire
-		b.goTracker.Go(b.doSetHostinfoFilterServices)
+		go b.doSetHostinfoFilterServices()


Switch this back to using goTracker

sailorfrag · 2025-01-20T21:58:09Z

ipn/serve.go

+func (sc *ServeConfig) CheckValidServicesConfig() error {
+	for svcName, service := range sc.Services {
+		if err := service.checkValidConfig(); err != nil {
+			return fmt.Errorf("invalid service configuration for \"%q\": %w", svcName, err)


%q already puts quotes around the contents, you don't need to add them manually

sailorfrag · 2025-01-20T22:00:23Z

types/netmap/netmap.go

+		return nil
+	}
+
+	ipMaps, err := tailcfg.UnmarshalNodeCapJSON[tailcfg.ServiceIPMappings](nm.SelfNode.AsStruct().CapMap, tailcfg.NodeAttrServiceHost)


There should be a way to avoid running this through AsStruct since that makes an expensive deep copy.

sailorfrag · 2025-01-20T22:03:42Z

types/netmap/netmap.go

+// GetVIPServiceIPMap returns a map of service names to the slice of
+// VIP addresses that correspond to the service. The service names are
+// with the prefix "svc:".
+func (nm *NetworkMap) GetVIPServiceIPMap() tailcfg.ServiceIPMappings {


It seems like it would be good for this to cache the unmarshaled details rather than JSON decoding multiple times when a new netmap is received.

sailorfrag · 2025-01-20T22:08:28Z

wgengine/netstack/netstack.go

+		ns.atomicIsVIPServiceIPFunc.Store(func(ip netip.Addr) bool {
+			_, ok := serviceAddrSet[ip]
+			return ok
+		})


Does this work?

Suggested change

ns.atomicIsVIPServiceIPFunc.Store(func(ip netip.Addr) bool {

_, ok := serviceAddrSet[ip]

return ok

})

ns.atomicIsVIPServiceIPFunc.Store(serviceAddrSet.Contains)

sailorfrag · 2025-01-20T22:10:00Z

wgengine/netstack/netstack.go

@@ -990,6 +1011,13 @@ func (ns *Impl) shouldProcessInbound(p *packet.Parsed, t *tstun.Wrapper) bool {
 			return true
 		}
 	}
+	if ns.lb != nil && p.IPProto == ipproto.TCP && isService {
+		// An assumption holds for this to work: when tun mode is on for a service,
+		// it's tcp and web are not set. It's enforced in b.setServeConfigLocked.


Suggested change

// it's tcp and web are not set. It's enforced in b.setServeConfigLocked.

// its tcp and web are not set. This is enforced in b.setServeConfigLocked.

KevinLiang10 force-pushed the kevin/serve_support_TCP_and_Web_VIP_Services branch 2 times, most recently from 7cbfe27 to 36c0813 Compare January 17, 2025 03:10

KevinLiang10 force-pushed the kevin/serve_support_TCP_and_Web_VIP_Services branch from 45fa3ba to 579d75d Compare January 20, 2025 17:03

KevinLiang10 changed the title ~~temp~~ ipn/ipnlocal: Support TCP and Web VIP services Jan 20, 2025

KevinLiang10 marked this pull request as ready for review January 20, 2025 17:25

KevinLiang10 requested a review from a team as a code owner January 20, 2025 17:25

KevinLiang10 requested a review from sailorfrag January 20, 2025 17:44

bradfitz reviewed Jan 20, 2025

View reviewed changes

KevinLiang10 added 3 commits January 20, 2025 12:52

add function descriptions

a7bdf91

Signed-off-by: KevinLiang10 <37811973+KevinLiang10@users.noreply.github.com>

fix comments and namings

9705335

Signed-off-by: KevinLiang10 <37811973+KevinLiang10@users.noreply.github.com>

specify where is IPServiceMappings used

f00b246

Signed-off-by: KevinLiang10 <37811973+KevinLiang10@users.noreply.github.com>

sailorfrag reviewed Jan 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ipn/ipnlocal: Support TCP and Web VIP services #14508

ipn/ipnlocal: Support TCP and Web VIP services #14508

KevinLiang10 commented Jan 2, 2025 •

edited

Loading

bradfitz Jan 20, 2025

KevinLiang10 Jan 20, 2025

bradfitz Jan 20, 2025

KevinLiang10 Jan 20, 2025

sailorfrag Jan 20, 2025

sailorfrag Jan 20, 2025

sailorfrag Jan 20, 2025

sailorfrag Jan 20, 2025

sailorfrag Jan 20, 2025

sailorfrag Jan 20, 2025

sailorfrag Jan 20, 2025

sailorfrag Jan 20, 2025

sailorfrag Jan 20, 2025

sailorfrag Jan 20, 2025

	svcAddrPorts := map[netip.Addr]func(uint16) bool{}
	svcAddrPorts := make(map[netip.Addr]func(uint16) bool)

	// it's tcp and web are not set. It's enforced in b.setServeConfigLocked.
	// its tcp and web are not set. This is enforced in b.setServeConfigLocked.

ipn/ipnlocal: Support TCP and Web VIP services #14508

Are you sure you want to change the base?

ipn/ipnlocal: Support TCP and Web VIP services #14508

Conversation

KevinLiang10 commented Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinLiang10 commented Jan 2, 2025 •

edited

Loading