ipn/ipnlocal: add validations when setting serve config (#17950)

These validations were previously performed in the CLI frontend. There
are two motivations for moving these to the local backend:
1. The backend controls synchronization around the relevant state, so
   only the backend can guarantee many of these validations.
2. Doing these validations in the back-end avoids the need to repeat
   them across every frontend (e.g. the CLI and tsnet).

Updates tailscale/corp#27200

Signed-off-by: Harry Harpham <harry@tailscale.com>

cmd/tailscale/cli/serve_v2.go

@@ -478,11 +478,6 @@ func (e *serveEnv) runServeCombined(subcmd serveMode) execFunc {
 		}
 		wantFg := !e.bg.Value && !turnOff
 		if wantFg {
-			// validate the config before creating a WatchIPNBus session
-			if err := e.validateConfig(parentSC, srvPort, srvType, svcName); err != nil {
-				return err
-			}
-
 			// if foreground mode, create a WatchIPNBus session
 			// and use the nested config for all following operations
 			// TODO(marwan-at-work): nested-config validations should happen here or previous to this point.
@@ -508,9 +503,6 @@ func (e *serveEnv) runServeCombined(subcmd serveMode) execFunc {
 			// only unset serve when trying to unset with type and port flags.
 			err = e.unsetServe(sc, dnsName, srvType, srvPort, mount, magicDNSSuffix)
 		} else {
-			if err := e.validateConfig(parentSC, srvPort, srvType, svcName); err != nil {
-				return err
-			}
 			if forService {
 				e.addServiceToPrefs(ctx, svcName)
 			}
@@ -907,66 +899,6 @@ func (e *serveEnv) runServeSetConfig(ctx context.Context, args []string) (err er
 	return e.lc.SetServeConfig(ctx, sc)
 }
 
-const backgroundExistsMsg = "background configuration already exists, use `tailscale %s --%s=%d off` to remove the existing configuration"
-
-// validateConfig checks if the serve config is valid to serve the type wanted on the port.
-// dnsName is a FQDN or a serviceName (with `svc:` prefix).
-func (e *serveEnv) validateConfig(sc *ipn.ServeConfig, port uint16, wantServe serveType, svcName tailcfg.ServiceName) error {
-	var tcpHandlerForPort *ipn.TCPPortHandler
-	if svcName != noService {
-		svc := sc.Services[svcName]
-		if svc == nil {
-			return nil
-		}
-		if wantServe == serveTypeTUN && (svc.TCP != nil || svc.Web != nil) {
-			return errors.New("service already has a TCP or Web handler, cannot serve in TUN mode")
-		}
-		if svc.Tun && wantServe != serveTypeTUN {
-			return errors.New("service is already being served in TUN mode")
-		}
-		if svc.TCP[port] == nil {
-			return nil
-		}
-		tcpHandlerForPort = svc.TCP[port]
-	} else {
-		sc, isFg := sc.FindConfig(port)
-		if sc == nil {
-			return nil
-		}
-		if isFg {
-			return errors.New("foreground already exists under this port")
-		}
-		if !e.bg.Value {
-			return fmt.Errorf(backgroundExistsMsg, infoMap[e.subcmd].Name, wantServe.String(), port)
-		}
-		tcpHandlerForPort = sc.TCP[port]
-	}
-	existingServe := serveFromPortHandler(tcpHandlerForPort)
-	if wantServe != existingServe {
-		target := svcName
-		if target == noService {
-			target = "machine"
-		}
-		return fmt.Errorf("want to serve %q but port is already serving %q for %q", wantServe, existingServe, target)
-	}
-	return nil
-}
-
-func serveFromPortHandler(tcp *ipn.TCPPortHandler) serveType {
-	switch {
-	case tcp.HTTP:
-		return serveTypeHTTP
-	case tcp.HTTPS:
-		return serveTypeHTTPS
-	case tcp.TerminateTLS != "":
-		return serveTypeTLSTerminatedTCP
-	case tcp.TCPForward != "":
-		return serveTypeTCP
-	default:
-		return -1
-	}
-}
-
 func (e *serveEnv) setServe(sc *ipn.ServeConfig, dnsName string, srvType serveType, srvPort uint16, mount string, target string, allowFunnel bool, mds string, caps []tailcfg.PeerCapability, proxyProtocol int) error {
 	// update serve config based on the type
 	switch srvType {

cmd/tailscale/cli/serve_v2_test.go

@@ -819,26 +819,6 @@ func TestServeDevConfigMutations(t *testing.T) {
 				},
 			},
 		},
-		{
-			name: "forground_with_bg_conflict",
-			steps: []step{
-				{
-					command: cmd("serve --bg --http=3000  localhost:3000"),
-					want: &ipn.ServeConfig{
-						TCP: map[uint16]*ipn.TCPPortHandler{3000: {HTTP: true}},
-						Web: map[ipn.HostPort]*ipn.WebServerConfig{
-							"foo.test.ts.net:3000": {Handlers: map[string]*ipn.HTTPHandler{
-								"/": {Proxy: "http://localhost:3000"},
-							}},
-						},
-					},
-				},
-				{
-					command: cmd("serve --http=3000 localhost:3000"),
-					wantErr: exactErrMsg(fmt.Errorf(backgroundExistsMsg, "serve", "http", 3000)),
-				},
-			},
-		},
 		{
 			name: "advertise_service",
 			initialState: fakeLocalServeClient{
@@ -1067,190 +1047,6 @@ func TestServeDevConfigMutations(t *testing.T) {
 	}
 }
 
-func TestValidateConfig(t *testing.T) {
-	tests := [...]struct {
-		name      string
-		desc      string
-		cfg       *ipn.ServeConfig
-		svc       tailcfg.ServiceName
-		servePort uint16
-		serveType serveType
-		bg        bgBoolFlag
-		wantErr   bool
-	}{
-		{
-			name:      "nil_config",
-			desc:      "when config is nil, all requests valid",
-			cfg:       nil,
-			servePort: 3000,
-			serveType: serveTypeHTTPS,
-		},
-		{
-			name: "new_bg_tcp",
-			desc: "no error when config exists but we're adding a new bg tcp port",
-			cfg: &ipn.ServeConfig{
-				TCP: map[uint16]*ipn.TCPPortHandler{
-					443: {HTTPS: true},
-				},
-			},
-			bg:        bgBoolFlag{true, false},
-			servePort: 10000,
-			serveType: serveTypeHTTPS,
-		},
-		{
-			name: "override_bg_tcp",
-			desc: "no error when overwriting previous port under the same serve type",
-			cfg: &ipn.ServeConfig{
-				TCP: map[uint16]*ipn.TCPPortHandler{
-					443: {TCPForward: "http://localhost:4545"},
-				},
-			},
-			bg:        bgBoolFlag{true, false},
-			servePort: 443,
-			serveType: serveTypeTCP,
-		},
-		{
-			name: "override_bg_tcp",
-			desc: "error when overwriting previous port under a different serve type",
-			cfg: &ipn.ServeConfig{
-				TCP: map[uint16]*ipn.TCPPortHandler{
-					443: {HTTPS: true},
-				},
-			},
-			bg:        bgBoolFlag{true, false},
-			servePort: 443,
-			serveType: serveTypeHTTP,
-			wantErr:   true,
-		},
-		{
-			name: "new_fg_port",
-			desc: "no error when serving a new foreground port",
-			cfg: &ipn.ServeConfig{
-				TCP: map[uint16]*ipn.TCPPortHandler{
-					443: {HTTPS: true},
-				},
-				Foreground: map[string]*ipn.ServeConfig{
-					"abc123": {
-						TCP: map[uint16]*ipn.TCPPortHandler{
-							3000: {HTTPS: true},
-						},
-					},
-				},
-			},
-			servePort: 4040,
-			serveType: serveTypeTCP,
-		},
-		{
-			name: "same_fg_port",
-			desc: "error when overwriting a previous fg port",
-			cfg: &ipn.ServeConfig{
-				Foreground: map[string]*ipn.ServeConfig{
-					"abc123": {
-						TCP: map[uint16]*ipn.TCPPortHandler{
-							3000: {HTTPS: true},
-						},
-					},
-				},
-			},
-			servePort: 3000,
-			serveType: serveTypeTCP,
-			wantErr:   true,
-		},
-		{
-			name: "new_service_tcp",
-			desc: "no error when adding a new service port",
-			cfg: &ipn.ServeConfig{
-				Services: map[tailcfg.ServiceName]*ipn.ServiceConfig{
-					"svc:foo": {
-						TCP: map[uint16]*ipn.TCPPortHandler{80: {HTTP: true}},
-					},
-				},
-			},
-			svc:       "svc:foo",
-			servePort: 8080,
-			serveType: serveTypeTCP,
-		},
-		{
-			name: "override_service_tcp",
-			desc: "no error when overwriting a previous service port",
-			cfg: &ipn.ServeConfig{
-				Services: map[tailcfg.ServiceName]*ipn.ServiceConfig{
-					"svc:foo": {
-						TCP: map[uint16]*ipn.TCPPortHandler{
-							443: {TCPForward: "http://localhost:4545"},
-						},
-					},
-				},
-			},
-			svc:       "svc:foo",
-			servePort: 443,
-			serveType: serveTypeTCP,
-		},
-		{
-			name: "override_service_tcp",
-			desc: "error when overwriting a previous service port with a different serve type",
-			cfg: &ipn.ServeConfig{
-				Services: map[tailcfg.ServiceName]*ipn.ServiceConfig{
-					"svc:foo": {
-						TCP: map[uint16]*ipn.TCPPortHandler{
-							443: {HTTPS: true},
-						},
-					},
-				},
-			},
-			svc:       "svc:foo",
-			servePort: 443,
-			serveType: serveTypeHTTP,
-			wantErr:   true,
-		},
-		{
-			name: "override_service_tcp",
-			desc: "error when setting previous tcp service to tun mode",
-			cfg: &ipn.ServeConfig{
-				Services: map[tailcfg.ServiceName]*ipn.ServiceConfig{
-					"svc:foo": {
-						TCP: map[uint16]*ipn.TCPPortHandler{
-							443: {TCPForward: "http://localhost:4545"},
-						},
-					},
-				},
-			},
-			svc:       "svc:foo",
-			serveType: serveTypeTUN,
-			wantErr:   true,
-		},
-		{
-			name: "override_service_tun",
-			desc: "error when setting previous tun service to tcp forwarder",
-			cfg: &ipn.ServeConfig{
-				Services: map[tailcfg.ServiceName]*ipn.ServiceConfig{
-					"svc:foo": {
-						Tun: true,
-					},
-				},
-			},
-			svc:       "svc:foo",
-			serveType: serveTypeTCP,
-			servePort: 443,
-			wantErr:   true,
-		},
-	}
-
-	for _, tc := range tests {
-		t.Run(tc.name, func(t *testing.T) {
-			se := serveEnv{bg: tc.bg}
-			err := se.validateConfig(tc.cfg, tc.servePort, tc.serveType, tc.svc)
-			if err == nil && tc.wantErr {
-				t.Fatal("expected an error but got nil")
-			}
-			if err != nil && !tc.wantErr {
-				t.Fatalf("expected no error but got: %v", err)
-			}
-		})
-	}
-
-}
-
 func TestSrcTypeFromFlags(t *testing.T) {
 	tests := []struct {
 		name         string

ipn/ipnlocal/serve.go

@@ -292,6 +292,10 @@ func (b *LocalBackend) updateServeTCPPortNetMapAddrListenersLocked(ports []uint1
 // SetServeConfig establishes or replaces the current serve config.
 // ETag is an optional parameter to enforce Optimistic Concurrency Control.
 // If it is an empty string, then the config will be overwritten.
+//
+// New foreground config cannot override existing listeners--neither existing
+// foreground listeners nor existing background listeners. Background config can
+// change as long as the serve type (e.g. HTTP, TCP, etc.) remains the same.
 func (b *LocalBackend) SetServeConfig(config *ipn.ServeConfig, etag string) error {
 	b.mu.Lock()
 	defer b.mu.Unlock()
@@ -307,12 +311,6 @@ func (b *LocalBackend) setServeConfigLocked(config *ipn.ServeConfig, etag string
 		return errors.New("can't reconfigure tailscaled when using a config file; config file is locked")
 	}
 
-	if config != nil {
-		if err := config.CheckValidServicesConfig(); err != nil {
-			return err
-		}
-	}
-
 	nm := b.NetMap()
 	if nm == nil {
 		return errors.New("netMap is nil")
@@ -340,6 +338,10 @@ func (b *LocalBackend) setServeConfigLocked(config *ipn.ServeConfig, etag string
 		}
 	}
 
+	if err := validateServeConfigUpdate(prevConfig, config.View()); err != nil {
+		return err
+	}
+
 	var bs []byte
 	if config != nil {
 		j, err := json.Marshal(config)
@@ -1566,3 +1568,144 @@ func vipServiceHash(logf logger.Logf, services []*tailcfg.VIPService) string {
 	h.Sum(buf[:0])
 	return hex.EncodeToString(buf[:])
 }
+
+// validateServeConfigUpdate validates changes proposed by incoming serve
+// configuration.
+func validateServeConfigUpdate(existing, incoming ipn.ServeConfigView) error {
+	// Error messages returned by this function may be presented to end-users by
+	// frontends like the CLI. Thus these error messages should provide enough
+	// information for end-users to diagnose and resolve conflicts.
+
+	if !incoming.Valid() {
+		return nil
+	}
+
+	// For Services, TUN mode is mutually exclusive with L4 or L7 handlers.
+	for svcName, svcCfg := range incoming.Services().All() {
+		hasTCP := svcCfg.TCP().Len() > 0
+		hasWeb := svcCfg.Web().Len() > 0
+		if svcCfg.Tun() && (hasTCP || hasWeb) {
+			return fmt.Errorf("cannot configure TUN mode in combination with TCP or web handlers for %s", svcName)
+		}
+	}
+
+	if !existing.Valid() {
+		return nil
+	}
+
+	// New foreground listeners must be on open ports.
+	for sessionID, incomingFg := range incoming.Foreground().All() {
+		if !existing.Foreground().Has(sessionID) {
+			// This is a new session.
+			for port := range incomingFg.TCPs() {
+				if _, exists := existing.FindTCP(port); exists {
+					return fmt.Errorf("listener already exists for port %d", port)
+				}
+			}
+		}
+	}
+
+	// New background listeners cannot overwrite existing foreground listeners.
+	for port := range incoming.TCP().All() {
+		if _, exists := existing.FindForegroundTCP(port); exists {
+			return fmt.Errorf("foreground listener already exists for port %d", port)
+		}
+	}
+
+	// Incoming configuration cannot change the serve type in use by a port.
+	for port, incomingHandler := range incoming.TCP().All() {
+		existingHandler, exists := existing.FindTCP(port)
+		if !exists {
+			continue
+		}
+
+		existingServeType := serveTypeFromPortHandler(existingHandler)
+		incomingServeType := serveTypeFromPortHandler(incomingHandler)
+		if incomingServeType != existingServeType {
+			return fmt.Errorf("want to serve %q, but port %d is already serving %q", incomingServeType, port, existingServeType)
+		}
+	}
+
+	// Validations for Tailscale Services.
+	for svcName, incomingSvcCfg := range incoming.Services().All() {
+		existingSvcCfg, exists := existing.Services().GetOk(svcName)
+		if !exists {
+			continue
+		}
+
+		// Incoming configuration cannot change the serve type in use by a port.
+		for port, incomingHandler := range incomingSvcCfg.TCP().All() {
+			existingHandler, exists := existingSvcCfg.TCP().GetOk(port)
+			if !exists {
+				continue
+			}
+
+			existingServeType := serveTypeFromPortHandler(existingHandler)
+			incomingServeType := serveTypeFromPortHandler(incomingHandler)
+			if incomingServeType != existingServeType {
+				return fmt.Errorf("want to serve %q, but port %d is already serving %q for %s", incomingServeType, port, existingServeType, svcName)
+			}
+		}
+
+		existingHasTCP := existingSvcCfg.TCP().Len() > 0
+		existingHasWeb := existingSvcCfg.Web().Len() > 0
+
+		// A Service cannot turn on TUN mode if TCP or web handlers exist.
+		if incomingSvcCfg.Tun() && (existingHasTCP || existingHasWeb) {
+			return fmt.Errorf("cannot turn on TUN mode with existing TCP or web handlers for %s", svcName)
+		}
+
+		incomingHasTCP := incomingSvcCfg.TCP().Len() > 0
+		incomingHasWeb := incomingSvcCfg.Web().Len() > 0
+
+		// A Service cannot add TCP or web handlers if TUN mode is enabled.
+		if (incomingHasTCP || incomingHasWeb) && existingSvcCfg.Tun() {
+			return fmt.Errorf("cannot add TCP or web handlers as TUN mode is enabled for %s", svcName)
+		}
+	}
+
+	return nil
+}
+
+// serveType is a high-level descriptor of the kind of serve performed by a TCP
+// port handler.
+type serveType int
+
+const (
+	serveTypeHTTPS serveType = iota
+	serveTypeHTTP
+	serveTypeTCP
+	serveTypeTLSTerminatedTCP
+)
+
+func (s serveType) String() string {
+	switch s {
+	case serveTypeHTTP:
+		return "http"
+	case serveTypeHTTPS:
+		return "https"
+	case serveTypeTCP:
+		return "tcp"
+	case serveTypeTLSTerminatedTCP:
+		return "tls-terminated-tcp"
+	default:
+		return "unknownServeType"
+	}
+}
+
+// serveTypeFromPortHandler is used to get a high-level descriptor of the kind
+// of serve being performed by a port handler.
+func serveTypeFromPortHandler(ph ipn.TCPPortHandlerView) serveType {
+	switch {
+	case ph.HTTP():
+		return serveTypeHTTP
+	case ph.HTTPS():
+		return serveTypeHTTPS
+	case ph.TerminateTLS() != "":
+		return serveTypeTLSTerminatedTCP
+	case ph.TCPForward() != "":
+		return serveTypeTCP
+	default:
+		return -1
+	}
+}

ipn/ipnlocal/serve_test.go

@@ -388,7 +388,7 @@ func TestServeConfigServices(t *testing.T) {
 	tests := []struct {
 		name              string
 		conf              *ipn.ServeConfig
-		expectedErr       error
+		errExpected       bool
 		packetDstAddrPort []netip.AddrPort
 		intercepted       bool
 	}{
@@ -412,7 +412,7 @@ func TestServeConfigServices(t *testing.T) {
 					},
 				},
 			},
-			expectedErr: ipn.ErrServiceConfigHasBothTCPAndTun,
+			errExpected: true,
 		},
 		{
 			// one correctly configured service with packet should be intercepted
@@ -519,13 +519,13 @@ func TestServeConfigServices(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			err := b.SetServeConfig(tt.conf, "")
-			if err != nil && tt.expectedErr != nil {
-				if !errors.Is(err, tt.expectedErr) {
-					t.Fatalf("expected error %v,\n got %v", tt.expectedErr, err)
-				}
-				return
+			if err == nil && tt.errExpected {
+				t.Fatal("expected error")
 			}
 			if err != nil {
+				if tt.errExpected {
+					return
+				}
 				t.Fatal(err)
 			}
 			for _, addrPort := range tt.packetDstAddrPort {
@@ -1454,3 +1454,315 @@ func TestServeHTTPRedirect(t *testing.T) {
 		})
 	}
 }
+
+func TestValidateServeConfigUpdate(t *testing.T) {
+	tests := []struct {
+		name, description  string
+		existing, incoming *ipn.ServeConfig
+		wantError          bool
+	}{
+		{
+			name:        "empty existing config",
+			description: "should be able to update with empty existing config",
+			existing:    &ipn.ServeConfig{},
+			incoming: &ipn.ServeConfig{
+				TCP: map[uint16]*ipn.TCPPortHandler{
+					8080: {},
+				},
+			},
+			wantError: false,
+		},
+		{
+			name:        "no existing config",
+			description: "should be able to update with no existing config",
+			existing:    nil,
+			incoming: &ipn.ServeConfig{
+				TCP: map[uint16]*ipn.TCPPortHandler{
+					8080: {},
+				},
+			},
+			wantError: false,
+		},
+		{
+			name:        "empty incoming config",
+			description: "wiping config should work",
+			existing: &ipn.ServeConfig{
+				TCP: map[uint16]*ipn.TCPPortHandler{
+					80: {},
+				},
+			},
+			incoming:  &ipn.ServeConfig{},
+			wantError: false,
+		},
+		{
+			name:        "no incoming config",
+			description: "missing incoming config should not result in an error",
+			existing: &ipn.ServeConfig{
+				TCP: map[uint16]*ipn.TCPPortHandler{
+					80: {},
+				},
+			},
+			incoming:  nil,
+			wantError: false,
+		},
+		{
+			name:        "non-overlapping update",
+			description: "non-overlapping update should work",
+			existing: &ipn.ServeConfig{
+				TCP: map[uint16]*ipn.TCPPortHandler{
+					80: {},
+				},
+			},
+			incoming: &ipn.ServeConfig{
+				TCP: map[uint16]*ipn.TCPPortHandler{
+					8080: {},
+				},
+			},
+			wantError: false,
+		},
+		{
+			name:        "overwriting background port",
+			description: "should be able to overwrite a background port",
+			existing: &ipn.ServeConfig{
+				TCP: map[uint16]*ipn.TCPPortHandler{
+					80: {
+						TCPForward: "localhost:8080",
+					},
+				},
+			},
+			incoming: &ipn.ServeConfig{
+				TCP: map[uint16]*ipn.TCPPortHandler{
+					80: {
+						TCPForward: "localhost:9999",
+					},
+				},
+			},
+			wantError: false,
+		},
+		{
+			name:        "broken existing config",
+			description: "broken existing config should not prevent new config updates",
+			existing: &ipn.ServeConfig{
+				TCP: map[uint16]*ipn.TCPPortHandler{
+					// Broken because HTTPS and TCPForward are mutually exclusive.
+					9000: {
+						HTTPS:      true,
+						TCPForward: "127.0.0.1:9000",
+					},
+					// Broken because foreground and background handlers cannot coexist.
+					443: {},
+				},
+				Foreground: map[string]*ipn.ServeConfig{
+					"12345": {
+						TCP: map[uint16]*ipn.TCPPortHandler{
+							// Broken because foreground and background handlers cannot coexist.
+							443: {},
+						},
+					},
+				},
+				// Broken because Services cannot specify TUN mode and a TCP handler.
+				Services: map[tailcfg.ServiceName]*ipn.ServiceConfig{
+					"svc:foo": {
+						TCP: map[uint16]*ipn.TCPPortHandler{
+							6060: {},
+						},
+						Tun: true,
+					},
+				},
+			},
+			incoming: &ipn.ServeConfig{
+				TCP: map[uint16]*ipn.TCPPortHandler{
+					80: {},
+				},
+			},
+			wantError: false,
+		},
+		{
+			name:        "services same port as background",
+			description: "services should be able to use the same port as background listeners",
+			existing: &ipn.ServeConfig{
+				TCP: map[uint16]*ipn.TCPPortHandler{
+					80: {},
+				},
+			},
+			incoming: &ipn.ServeConfig{
+				Services: map[tailcfg.ServiceName]*ipn.ServiceConfig{
+					"svc:foo": {
+						TCP: map[uint16]*ipn.TCPPortHandler{
+							80: {},
+						},
+					},
+				},
+			},
+			wantError: false,
+		},
+		{
+			name:        "services tun mode",
+			description: "TUN mode should be mutually exclusive with TCP or web handlers for new Services",
+			existing:    &ipn.ServeConfig{},
+			incoming: &ipn.ServeConfig{
+				Services: map[tailcfg.ServiceName]*ipn.ServiceConfig{
+					"svc:foo": {
+						TCP: map[uint16]*ipn.TCPPortHandler{
+							6060: {},
+						},
+						Tun: true,
+					},
+				},
+			},
+			wantError: true,
+		},
+		{
+			name:        "new foreground listener",
+			description: "new foreground listeners must be on open ports",
+			existing: &ipn.ServeConfig{
+				TCP: map[uint16]*ipn.TCPPortHandler{
+					80: {},
+				},
+			},
+			incoming: &ipn.ServeConfig{
+				Foreground: map[string]*ipn.ServeConfig{
+					"12345": {
+						TCP: map[uint16]*ipn.TCPPortHandler{
+							80: {},
+						},
+					},
+				},
+			},
+			wantError: true,
+		},
+		{
+			name:        "new background listener",
+			description: "new background listers cannot overwrite foreground listeners",
+			existing: &ipn.ServeConfig{
+				Foreground: map[string]*ipn.ServeConfig{
+					"12345": {
+						TCP: map[uint16]*ipn.TCPPortHandler{
+							80: {},
+						},
+					},
+				},
+			},
+			incoming: &ipn.ServeConfig{
+				TCP: map[uint16]*ipn.TCPPortHandler{
+					80: {},
+				},
+			},
+			wantError: true,
+		},
+		{
+			name:        "serve type overwrite",
+			description: "incoming configuration cannot change the serve type in use by a port",
+			existing: &ipn.ServeConfig{
+				TCP: map[uint16]*ipn.TCPPortHandler{
+					80: {
+						HTTP: true,
+					},
+				},
+			},
+			incoming: &ipn.ServeConfig{
+				TCP: map[uint16]*ipn.TCPPortHandler{
+					80: {
+						TCPForward: "localhost:8080",
+					},
+				},
+			},
+			wantError: true,
+		},
+		{
+			name:        "serve type overwrite services",
+			description: "incoming Services configuration cannot change the serve type in use by a port",
+			existing: &ipn.ServeConfig{
+				Services: map[tailcfg.ServiceName]*ipn.ServiceConfig{
+					"svc:foo": {
+						TCP: map[uint16]*ipn.TCPPortHandler{
+							80: {
+								HTTP: true,
+							},
+						},
+					},
+				},
+			},
+			incoming: &ipn.ServeConfig{
+				Services: map[tailcfg.ServiceName]*ipn.ServiceConfig{
+					"svc:foo": {
+						TCP: map[uint16]*ipn.TCPPortHandler{
+							80: {
+								TCPForward: "localhost:8080",
+							},
+						},
+					},
+				},
+			},
+			wantError: true,
+		},
+		{
+			name:        "tun mode with handlers",
+			description: "Services cannot enable TUN mode if L4 or L7 handlers already exist",
+			existing: &ipn.ServeConfig{
+				Services: map[tailcfg.ServiceName]*ipn.ServiceConfig{
+					"svc:foo": {
+						TCP: map[uint16]*ipn.TCPPortHandler{
+							443: {
+								HTTPS: true,
+							},
+						},
+						Web: map[ipn.HostPort]*ipn.WebServerConfig{
+							"127.0.0.1:443": {
+								Handlers: map[string]*ipn.HTTPHandler{},
+							},
+						},
+					},
+				},
+			},
+			incoming: &ipn.ServeConfig{
+				Services: map[tailcfg.ServiceName]*ipn.ServiceConfig{
+					"svc:foo": {
+						Tun: true,
+					},
+				},
+			},
+			wantError: true,
+		},
+		{
+			name:        "handlers with tun mode",
+			description: "Services cannot add L4 or L7 handlers if TUN mode is already enabled",
+			existing: &ipn.ServeConfig{
+				Services: map[tailcfg.ServiceName]*ipn.ServiceConfig{
+					"svc:foo": {
+						Tun: true,
+					},
+				},
+			},
+			incoming: &ipn.ServeConfig{
+				Services: map[tailcfg.ServiceName]*ipn.ServiceConfig{
+					"svc:foo": {
+						TCP: map[uint16]*ipn.TCPPortHandler{
+							443: {
+								HTTPS: true,
+							},
+						},
+						Web: map[ipn.HostPort]*ipn.WebServerConfig{
+							"127.0.0.1:443": {
+								Handlers: map[string]*ipn.HTTPHandler{},
+							},
+						},
+					},
+				},
+			},
+			wantError: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := validateServeConfigUpdate(tt.existing.View(), tt.incoming.View())
+			if err != nil && !tt.wantError {
+				t.Error("unexpected error:", err)
+			}
+			if err == nil && tt.wantError {
+				t.Error("expected error, got nil;", tt.description)
+			}
+		})
+	}
+}

ipn/serve.go

@@ -802,6 +802,7 @@ func (v ServeConfigView) FindServiceTCP(svcName tailcfg.ServiceName, port uint16
 	return svcCfg.TCP().GetOk(port)
 }
 
+// FindServiceWeb returns the web handler for the service's host-port.
 func (v ServeConfigView) FindServiceWeb(svcName tailcfg.ServiceName, hp HostPort) (res WebServerConfigView, ok bool) {
 	if svcCfg, ok := v.Services().GetOk(svcName); ok {
 		if res, ok := svcCfg.Web().GetOk(hp); ok {
@@ -815,10 +816,9 @@ func (v ServeConfigView) FindServiceWeb(svcName tailcfg.ServiceName, hp HostPort
 // prefers a foreground match first followed by a background search if none
 // existed.
 func (v ServeConfigView) FindTCP(port uint16) (res TCPPortHandlerView, ok bool) {
-	for _, conf := range v.Foreground().All() {
-		if res, ok := conf.TCP().GetOk(port); ok {
-			return res, ok
-		}
+	res, ok = v.FindForegroundTCP(port)
+	if ok {
+		return res, ok
 	}
 	return v.TCP().GetOk(port)
 }
@@ -835,6 +835,17 @@ func (v ServeConfigView) FindWeb(hp HostPort) (res WebServerConfigView, ok bool)
 	return v.Web().GetOk(hp)
 }
 
+// FindForegroundTCP returns the first foreground TCP handler matching the input
+// port.
+func (v ServeConfigView) FindForegroundTCP(port uint16) (res TCPPortHandlerView, ok bool) {
+	for _, conf := range v.Foreground().All() {
+		if res, ok := conf.TCP().GetOk(port); ok {
+			return res, ok
+		}
+	}
+	return res, false
+}
+
 // HasAllowFunnel returns whether this config has at least one AllowFunnel
 // set in the background or foreground configs.
 func (v ServeConfigView) HasAllowFunnel() bool {
@@ -863,17 +874,6 @@ func (v ServeConfigView) HasFunnelForTarget(target HostPort) bool {
 	return false
 }
 
-// CheckValidServicesConfig reports whether the ServeConfig has
-// invalid service configurations.
-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)
-		}
-	}
-	return nil
-}
-
 // ServicePortRange returns the list of tailcfg.ProtoPortRange that represents
 // the proto/ports pairs that are being served by the service.
 //
@@ -911,17 +911,3 @@ func (v ServiceConfigView) ServicePortRange() []tailcfg.ProtoPortRange {
 	}
 	return ranges
 }
-
-// ErrServiceConfigHasBothTCPAndTun signals that a service
-// in Tun mode cannot also has TCP or Web handlers set.
-var ErrServiceConfigHasBothTCPAndTun = errors.New("the VIP Service configuration can not set TUN at the same time as TCP or Web")
-
-// checkValidConfig checks if the service configuration is valid.
-// Currently, the only invalid configuration is when the service is in Tun mode
-// and has TCP or Web handlers.
-func (v *ServiceConfig) checkValidConfig() error {
-	if v.Tun && (len(v.TCP) > 0 || len(v.Web) > 0) {
-		return ErrServiceConfigHasBothTCPAndTun
-	}
-	return nil
-}