fix: move STUN servers into their own regions (#9030)

enterprise/cli/testdata/coder_server_--help.golden

@@ -175,18 +175,15 @@ backed by Tailscale and WireGuard.
       --derp-server-enable bool, $CODER_DERP_SERVER_ENABLE (default: true)
           Whether to enable or disable the embedded DERP relay server.
 
-      --derp-server-region-code string, $CODER_DERP_SERVER_REGION_CODE (default: coder)
-          Region code to use for the embedded DERP server.
-
-      --derp-server-region-id int, $CODER_DERP_SERVER_REGION_ID (default: 999)
-          Region ID to use for the embedded DERP server.
-
       --derp-server-region-name string, $CODER_DERP_SERVER_REGION_NAME (default: Coder Embedded Relay)
           Region name that for the embedded DERP server.
 
-      --derp-server-stun-addresses string-array, $CODER_DERP_SERVER_STUN_ADDRESSES (default: stun.l.google.com:19302)
-          Addresses for STUN servers to establish P2P connections. Use special
-          value 'disable' to turn off STUN.
+      --derp-server-stun-addresses string-array, $CODER_DERP_SERVER_STUN_ADDRESSES (default: stun.l.google.com:19302,stun1.l.google.com:19302,stun2.l.google.com:19302,stun3.l.google.com:19302,stun4.l.google.com:19302)
+          Addresses for STUN servers to establish P2P connections. It's
+          recommended to have at least two STUN servers to give users the best
+          chance of connecting P2P to workspaces. Each STUN server will get it's
+          own DERP region, with region IDs starting at `--derp-server-region-id
+          + 1`. Use special value 'disable' to turn off STUN completely.
 
 Networking / HTTP Options 
       --disable-password-auth bool, $CODER_DISABLE_PASSWORD_AUTH

enterprise/coderd/coderd.go

@@ -594,7 +594,7 @@ func (api *API) updateEntitlements(ctx context.Context) error {
 
 	if initial, changed, enabled := featureChanged(codersdk.FeatureWorkspaceProxy); shouldUpdate(initial, changed, enabled) {
 		if enabled {
-			fn := derpMapper(api.Logger, api.DeploymentValues, api.ProxyHealth)
+			fn := derpMapper(api.Logger, api.ProxyHealth)
 			api.AGPL.DERPMapper.Store(&fn)
 		} else {
 			api.AGPL.DERPMapper.Store(nil)
@@ -659,7 +659,7 @@ var (
 	lastDerpConflictLog   time.Time
 )
 
-func derpMapper(logger slog.Logger, _ *codersdk.DeploymentValues, proxyHealth *proxyhealth.ProxyHealth) func(*tailcfg.DERPMap) *tailcfg.DERPMap {
+func derpMapper(logger slog.Logger, proxyHealth *proxyhealth.ProxyHealth) func(*tailcfg.DERPMap) *tailcfg.DERPMap {
 	return func(derpMap *tailcfg.DERPMap) *tailcfg.DERPMap {
 		derpMap = derpMap.Clone()
 
@@ -753,46 +753,22 @@ func derpMapper(logger slog.Logger, _ *codersdk.DeploymentValues, proxyHealth *p
 				}
 			}
 
-			var stunNodes []*tailcfg.DERPNode
-			// TODO(@dean): potentially re-enable this depending on impact
-			/*
-				if !cfg.DERP.Config.BlockDirect.Value() {
-					stunNodes, err = agpltailnet.STUNNodes(regionID, cfg.DERP.Server.STUNAddresses)
-					if err != nil {
-						// Log a warning if we haven't logged one in the last
-						// minute.
-						lastDerpConflictMutex.Lock()
-						shouldLog := lastDerpConflictLog.IsZero() || time.Since(lastDerpConflictLog) > time.Minute
-						if shouldLog {
-							lastDerpConflictLog = time.Now()
-						}
-						lastDerpConflictMutex.Unlock()
-						if shouldLog {
-							logger.Error(context.Background(), "failed to calculate STUN nodes", slog.Error(err))
-						}
-
-						// No continue because we can keep going.
-						stunNodes = []*tailcfg.DERPNode{}
-					}
-				}
-			*/
-
-			nodes := append(stunNodes, &tailcfg.DERPNode{
-				Name:      fmt.Sprintf("%da", regionID),
-				RegionID:  regionID,
-				HostName:  u.Hostname(),
-				DERPPort:  portInt,
-				STUNPort:  -1,
-				ForceHTTP: u.Scheme == "http",
-			})
-
 			derpMap.Regions[regionID] = &tailcfg.DERPRegion{
 				// EmbeddedRelay ONLY applies to the primary.
 				EmbeddedRelay: false,
 				RegionID:      regionID,
 				RegionCode:    regionCode,
 				RegionName:    regionName,
-				Nodes:         nodes,
+				Nodes: []*tailcfg.DERPNode{
+					{
+						Name:      fmt.Sprintf("%da", regionID),
+						RegionID:  regionID,
+						HostName:  u.Hostname(),
+						DERPPort:  portInt,
+						STUNPort:  -1,
+						ForceHTTP: u.Scheme == "http",
+					},
+				},
 			}
 		}
 

enterprise/wsproxy/wsproxy_test.go

@@ -27,7 +27,6 @@ import (
 	"github.com/coder/coder/enterprise/coderd/coderdenttest"
 	"github.com/coder/coder/enterprise/coderd/license"
 	"github.com/coder/coder/provisioner/echo"
-	"github.com/coder/coder/tailnet"
 	"github.com/coder/coder/testutil"
 )
 
@@ -203,10 +202,10 @@ resourceLoop:
 		connInfo, err := client.WorkspaceAgentConnectionInfo(ctx, agentID)
 		require.NoError(t, err)
 
-		// There should be three DERP servers in the map: the primary, and each
-		// of the two running proxies.
+		// There should be three DERP regions in the map: the primary, and each
+		// of the two running proxies. Also the STUN-only regions.
 		require.NotNil(t, connInfo.DERPMap)
-		require.Len(t, connInfo.DERPMap.Regions, 3)
+		require.Len(t, connInfo.DERPMap.Regions, 3+len(api.DeploymentValues.DERP.Server.STUNAddresses.Value()))
 
 		var (
 			primaryRegion *tailcfg.DERPRegion
@@ -230,6 +229,11 @@ resourceLoop:
 			// The last region is never started, which means it's never healthy,
 			// which means it's never added to the DERP map.
 
+			if len(r.Nodes) == 1 && r.Nodes[0].STUNOnly {
+				// Skip STUN-only regions.
+				continue
+			}
+
 			t.Fatalf("unexpected region: %+v", r)
 		}
 
@@ -270,11 +274,15 @@ resourceLoop:
 		connInfo, err := client.WorkspaceAgentConnectionInfo(testutil.Context(t, testutil.WaitLong), agentID)
 		require.NoError(t, err)
 		require.NotNil(t, connInfo.DERPMap)
-		require.Len(t, connInfo.DERPMap.Regions, 3)
+		require.Len(t, connInfo.DERPMap.Regions, 3+len(api.DeploymentValues.DERP.Server.STUNAddresses.Value()))
 
 		// Connect to each region.
 		for _, r := range connInfo.DERPMap.Regions {
 			r := r
+			if len(r.Nodes) == 1 && r.Nodes[0].STUNOnly {
+				// Skip STUN-only regions.
+				continue
+			}
 
 			t.Run(r.RegionName, func(t *testing.T) {
 				t.Parallel()
@@ -311,134 +319,6 @@ resourceLoop:
 	})
 }
 
-func TestDERPMapStunNodes(t *testing.T) {
-	t.Parallel()
-	// See: enterprise/coderd/coderd.go
-	t.Skip("STUN nodes are removed from proxy regions in the DERP map for now")
-
-	deploymentValues := coderdtest.DeploymentValues(t)
-	deploymentValues.Experiments = []string{
-		string(codersdk.ExperimentMoons),
-		"*",
-	}
-	stunAddresses := []string{
-		"stun.l.google.com:19302",
-		"stun1.l.google.com:19302",
-		"stun2.l.google.com:19302",
-		"stun3.l.google.com:19302",
-		"stun4.l.google.com:19302",
-	}
-	deploymentValues.DERP.Server.STUNAddresses = stunAddresses
-
-	client, closer, api, _ := coderdenttest.NewWithAPI(t, &coderdenttest.Options{
-		Options: &coderdtest.Options{
-			DeploymentValues:         deploymentValues,
-			AppHostname:              "*.primary.test.coder.com",
-			IncludeProvisionerDaemon: true,
-			RealIPConfig: &httpmw.RealIPConfig{
-				TrustedOrigins: []*net.IPNet{{
-					IP:   net.ParseIP("127.0.0.1"),
-					Mask: net.CIDRMask(8, 32),
-				}},
-				TrustedHeaders: []string{
-					"CF-Connecting-IP",
-				},
-			},
-		},
-		LicenseOptions: &coderdenttest.LicenseOptions{
-			Features: license.Features{
-				codersdk.FeatureWorkspaceProxy: 1,
-			},
-		},
-	})
-	t.Cleanup(func() {
-		_ = closer.Close()
-	})
-
-	// Create a running external proxy.
-	_ = coderdenttest.NewWorkspaceProxy(t, api, client, &coderdenttest.ProxyOptions{
-		Name: "cool-proxy",
-	})
-
-	// Wait for both running proxies to become healthy.
-	require.Eventually(t, func() bool {
-		healthCtx := testutil.Context(t, testutil.WaitLong)
-		err := api.ProxyHealth.ForceUpdate(healthCtx)
-		if !assert.NoError(t, err) {
-			return false
-		}
-
-		regions, err := client.Regions(healthCtx)
-		if !assert.NoError(t, err) {
-			return false
-		}
-		if !assert.Len(t, regions, 2) {
-			return false
-		}
-
-		// All regions should be healthy.
-		for _, r := range regions {
-			if !r.Healthy {
-				return false
-			}
-		}
-		return true
-	}, testutil.WaitLong, testutil.IntervalMedium)
-
-	// Get the DERP map and ensure that the built-in region and the proxy region
-	// both have the STUN nodes.
-	ctx := testutil.Context(t, testutil.WaitLong)
-	connInfo, err := client.WorkspaceAgentConnectionInfoGeneric(ctx)
-	require.NoError(t, err)
-
-	// There should be two DERP servers in the map: the primary and the
-	// proxy.
-	require.NotNil(t, connInfo.DERPMap)
-	require.Len(t, connInfo.DERPMap.Regions, 2)
-
-	var (
-		primaryRegion *tailcfg.DERPRegion
-		proxyRegion   *tailcfg.DERPRegion
-	)
-	for _, r := range connInfo.DERPMap.Regions {
-		if r.EmbeddedRelay {
-			primaryRegion = r
-			continue
-		}
-		if r.RegionName == "cool-proxy" {
-			proxyRegion = r
-			continue
-		}
-
-		t.Fatalf("unexpected region: %+v", r)
-	}
-
-	// The primary region:
-	require.Equal(t, "Coder Embedded Relay", primaryRegion.RegionName)
-	require.Equal(t, "coder", primaryRegion.RegionCode)
-	require.Equal(t, 999, primaryRegion.RegionID)
-	require.True(t, primaryRegion.EmbeddedRelay)
-	require.Len(t, primaryRegion.Nodes, len(stunAddresses)+1)
-
-	// The proxy region:
-	require.Equal(t, "cool-proxy", proxyRegion.RegionName)
-	require.Equal(t, "coder_cool-proxy", proxyRegion.RegionCode)
-	require.Equal(t, 10001, proxyRegion.RegionID)
-	require.False(t, proxyRegion.EmbeddedRelay)
-	require.Len(t, proxyRegion.Nodes, len(stunAddresses)+1)
-
-	for _, region := range []*tailcfg.DERPRegion{primaryRegion, proxyRegion} {
-		stunNodes, err := tailnet.STUNNodes(region.RegionID, stunAddresses)
-		require.NoError(t, err)
-		require.Len(t, stunNodes, len(stunAddresses))
-
-		require.Equal(t, stunNodes, region.Nodes[:len(stunNodes)])
-
-		// The last node should be the Coder server.
-		require.NotZero(t, region.Nodes[len(region.Nodes)-1].DERPPort)
-	}
-}
-
 func TestDERPEndToEnd(t *testing.T) {
 	t.Parallel()