ipn/ipnlocal: add validations when setting serve config (tailscale#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 <[email protected]>

Author: hwh33

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