diff --git a/cli/netcheck_test.go b/cli/netcheck_test.go index 890260c1a704e..be0b50e1a5bac 100644 --- a/cli/netcheck_test.go +++ b/cli/netcheck_test.go @@ -31,7 +31,7 @@ func TestNetcheck(t *testing.T) { require.NoError(t, json.Unmarshal(b, &report)) assert.True(t, report.Healthy) - require.Len(t, report.Regions, 1) + require.Len(t, report.Regions, 1+5) // 1 built-in region + 5 STUN regions by default for _, v := range report.Regions { require.Len(t, v.NodeReports, len(v.Region.Nodes)) } diff --git a/cli/testdata/coder_server_--help.golden b/cli/testdata/coder_server_--help.golden index 4e1274bb7ad20..cd8fd309f1154 100644 --- a/cli/testdata/coder_server_--help.golden +++ b/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 diff --git a/cli/testdata/server-config.yaml.golden b/cli/testdata/server-config.yaml.golden index f3149b31eec7f..27aeb41a61b6a 100644 --- a/cli/testdata/server-config.yaml.golden +++ b/cli/testdata/server-config.yaml.golden @@ -111,11 +111,20 @@ networking: # Region name that for the embedded DERP server. # (default: Coder Embedded Relay, type: string) regionName: Coder Embedded Relay - # Addresses for STUN servers to establish P2P connections. Use special value - # 'disable' to turn off STUN. - # (default: stun.l.google.com:19302, type: string-array) + # 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. + # (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, + # type: string-array) stunAddresses: - 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 # An HTTP URL that is accessible by other replicas to relay DERP traffic. Required # for high availability. # (default: , type: url) diff --git a/codersdk/deployment.go b/codersdk/deployment.go index 96cb9d19516f3..a7dd2a5af50a0 100644 --- a/codersdk/deployment.go +++ b/codersdk/deployment.go @@ -733,6 +733,7 @@ when required by your organization's security policy.`, Value: &c.DERP.Server.RegionID, Group: &deploymentGroupNetworkingDERP, YAML: "regionID", + Hidden: true, // Does not apply to external proxies as this value is generated. }, { @@ -744,6 +745,7 @@ when required by your organization's security policy.`, Value: &c.DERP.Server.RegionCode, Group: &deploymentGroupNetworkingDERP, YAML: "regionCode", + Hidden: true, // Does not apply to external proxies as we use the proxy name. }, { @@ -759,10 +761,10 @@ when required by your organization's security policy.`, }, { Name: "DERP Server STUN Addresses", - Description: "Addresses for STUN servers to establish P2P connections. Use special value 'disable' to turn off STUN.", + Description: "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.", Flag: "derp-server-stun-addresses", Env: "CODER_DERP_SERVER_STUN_ADDRESSES", - Default: "stun.l.google.com:19302", + 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", Value: &c.DERP.Server.STUNAddresses, Group: &deploymentGroupNetworkingDERP, YAML: "stunAddresses", diff --git a/docs/cli/server.md b/docs/cli/server.md index 27658ee0d16e8..9c7a90893ac9e 100644 --- a/docs/cli/server.md +++ b/docs/cli/server.md @@ -129,28 +129,6 @@ URL to fetch a DERP mapping on startup. See: https://tailscale.com/kb/1118/custo Whether to enable or disable the embedded DERP relay server. -### --derp-server-region-code - -| | | -| ----------- | ------------------------------------------- | -| Type | string | -| Environment | $CODER_DERP_SERVER_REGION_CODE | -| YAML | networking.derp.regionCode | -| Default | coder | - -Region code to use for the embedded DERP server. - -### --derp-server-region-id - -| | | -| ----------- | ----------------------------------------- | -| Type | int | -| Environment | $CODER_DERP_SERVER_REGION_ID | -| YAML | networking.derp.regionID | -| Default | 999 | - -Region ID to use for the embedded DERP server. - ### --derp-server-region-name | | | @@ -174,14 +152,14 @@ An HTTP URL that is accessible by other replicas to relay DERP traffic. Required ### --derp-server-stun-addresses -| | | -| ----------- | ---------------------------------------------- | -| Type | string-array | -| Environment | $CODER_DERP_SERVER_STUN_ADDRESSES | -| YAML | networking.derp.stunAddresses | -| Default | stun.l.google.com:19302 | +| | | +| ----------- | ---------------------------------------------------------------------------------------------------------------------------------------- | +| Type | string-array | +| Environment | $CODER_DERP_SERVER_STUN_ADDRESSES | +| YAML | networking.derp.stunAddresses | +| 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. Use special value 'disable' to turn off STUN. +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. ### --default-quiet-hours-schedule diff --git a/enterprise/cli/testdata/coder_server_--help.golden b/enterprise/cli/testdata/coder_server_--help.golden index 4e1274bb7ad20..cd8fd309f1154 100644 --- a/enterprise/cli/testdata/coder_server_--help.golden +++ b/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 diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index 1679165750b16..de385209e44f4 100644 --- a/enterprise/coderd/coderd.go +++ b/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", + }, + }, } } diff --git a/enterprise/wsproxy/wsproxy_test.go b/enterprise/wsproxy/wsproxy_test.go index d9d4e1a96bc8d..3ba8463331d6a 100644 --- a/enterprise/wsproxy/wsproxy_test.go +++ b/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() diff --git a/tailnet/derpmap.go b/tailnet/derpmap.go index aa79300801d71..f35e48156b768 100644 --- a/tailnet/derpmap.go +++ b/tailnet/derpmap.go @@ -13,11 +13,11 @@ import ( "tailscale.com/tailcfg" ) -func STUNNodes(regionID int, stunAddrs []string) ([]*tailcfg.DERPNode, error) { - nodes := []*tailcfg.DERPNode{} +func STUNRegions(baseRegionID int, stunAddrs []string) ([]*tailcfg.DERPRegion, error) { + regions := make([]*tailcfg.DERPRegion, 0, len(stunAddrs)) for index, stunAddr := range stunAddrs { if stunAddr == "disable" { - return []*tailcfg.DERPNode{}, nil + return []*tailcfg.DERPRegion{}, nil } host, rawPort, err := net.SplitHostPort(stunAddr) @@ -28,16 +28,24 @@ func STUNNodes(regionID int, stunAddrs []string) ([]*tailcfg.DERPNode, error) { if err != nil { return nil, xerrors.Errorf("parse port for %q: %w", stunAddr, err) } - nodes = append([]*tailcfg.DERPNode{{ - Name: fmt.Sprintf("%dstun%d", regionID, index), - RegionID: regionID, - HostName: host, - STUNOnly: true, - STUNPort: port, - }}, nodes...) + + regionID := baseRegionID + index + 1 + regions = append(regions, &tailcfg.DERPRegion{ + EmbeddedRelay: false, + RegionID: regionID, + RegionCode: fmt.Sprintf("coder_stun_%d", regionID), + RegionName: fmt.Sprintf("Coder STUN %d", regionID), + Nodes: []*tailcfg.DERPNode{{ + Name: fmt.Sprintf("%dstun0", regionID), + RegionID: regionID, + HostName: host, + STUNOnly: true, + STUNPort: port, + }}, + }) } - return nodes, nil + return regions, nil } // NewDERPMap constructs a DERPMap from a set of STUN addresses and optionally a remote @@ -52,13 +60,17 @@ func NewDERPMap(ctx context.Context, region *tailcfg.DERPRegion, stunAddrs []str stunAddrs = nil } + // stunAddrs only applies when a default region is set. Each STUN node gets + // it's own region ID because netcheck will only try a single STUN server in + // each region before canceling the region's STUN check. + addRegions := []*tailcfg.DERPRegion{} if region != nil { - stunNodes, err := STUNNodes(region.RegionID, stunAddrs) + addRegions = append(addRegions, region) + stunRegions, err := STUNRegions(region.RegionID, stunAddrs) if err != nil { - return nil, xerrors.Errorf("construct stun nodes: %w", err) + return nil, xerrors.Errorf("create stun regions: %w", err) } - - region.Nodes = append(stunNodes, region.Nodes...) + addRegions = append(addRegions, stunRegions...) } derpMap := &tailcfg.DERPMap{ @@ -89,13 +101,18 @@ func NewDERPMap(ctx context.Context, region *tailcfg.DERPRegion, stunAddrs []str return nil, xerrors.Errorf("unmarshal derpmap: %w", err) } } - if region != nil { - _, conflicts := derpMap.Regions[region.RegionID] - if conflicts { - return nil, xerrors.Errorf("the default region ID conflicts with a remote region from %q", remoteURL) + + // Add our custom regions to the DERP map. + if len(addRegions) > 0 { + for _, region := range addRegions { + _, conflicts := derpMap.Regions[region.RegionID] + if conflicts { + return nil, xerrors.Errorf("a default region ID %d (%s - %q) conflicts with a remote region from %q", region.RegionID, region.RegionCode, region.RegionName, remoteURL) + } + derpMap.Regions[region.RegionID] = region } - derpMap.Regions[region.RegionID] = region } + // Remove all STUNPorts from DERPy nodes, and fully remove all STUNOnly // nodes. if disableSTUN { diff --git a/tailnet/derpmap_test.go b/tailnet/derpmap_test.go index bc5205cc45cf4..07d1b11255c93 100644 --- a/tailnet/derpmap_test.go +++ b/tailnet/derpmap_test.go @@ -24,7 +24,9 @@ func TestNewDERPMap(t *testing.T) { Nodes: []*tailcfg.DERPNode{{}}, }, []string{"stun.google.com:2345"}, "", "", false) require.NoError(t, err) - require.Len(t, derpMap.Regions[1].Nodes, 2) + require.Len(t, derpMap.Regions, 2) + require.Len(t, derpMap.Regions[1].Nodes, 1) + require.Len(t, derpMap.Regions[2].Nodes, 1) }) t.Run("RemoteURL", func(t *testing.T) { t.Parallel()