From 6c9d495d9646950b35b5d9f4b4879706b630be38 Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Wed, 9 Aug 2023 08:21:46 +0000 Subject: [PATCH 1/2] fix: remove stun nodes from workspace proxy regions --- enterprise/coderd/coderd.go | 37 ++++++++++++++++-------------- enterprise/wsproxy/wsproxy_test.go | 26 +++++++++++---------- 2 files changed, 34 insertions(+), 29 deletions(-) diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index 71d975e3ef1d6..838a462c28a73 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -754,25 +754,28 @@ func derpMapper(logger slog.Logger, cfg *codersdk.DeploymentValues, proxyHealth } var stunNodes []*tailcfg.DERPNode - 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() + // 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{} } - 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), diff --git a/enterprise/wsproxy/wsproxy_test.go b/enterprise/wsproxy/wsproxy_test.go index afcb3d1f16143..d9d4e1a96bc8d 100644 --- a/enterprise/wsproxy/wsproxy_test.go +++ b/enterprise/wsproxy/wsproxy_test.go @@ -244,24 +244,24 @@ resourceLoop: require.Equal(t, "coder_best-proxy", proxy1Region.RegionCode) require.Equal(t, 10001, proxy1Region.RegionID) require.False(t, proxy1Region.EmbeddedRelay) - require.Len(t, proxy1Region.Nodes, 2) // proxy + stun - require.Equal(t, "10001a", proxy1Region.Nodes[1].Name) - require.Equal(t, 10001, proxy1Region.Nodes[1].RegionID) - require.Equal(t, proxyAPI1.Options.AccessURL.Hostname(), proxy1Region.Nodes[1].HostName) - require.Equal(t, proxyAPI1.Options.AccessURL.Port(), fmt.Sprint(proxy1Region.Nodes[1].DERPPort)) - require.Equal(t, proxyAPI1.Options.AccessURL.Scheme == "http", proxy1Region.Nodes[1].ForceHTTP) + require.Len(t, proxy1Region.Nodes, 1) + require.Equal(t, "10001a", proxy1Region.Nodes[0].Name) + require.Equal(t, 10001, proxy1Region.Nodes[0].RegionID) + require.Equal(t, proxyAPI1.Options.AccessURL.Hostname(), proxy1Region.Nodes[0].HostName) + require.Equal(t, proxyAPI1.Options.AccessURL.Port(), fmt.Sprint(proxy1Region.Nodes[0].DERPPort)) + require.Equal(t, proxyAPI1.Options.AccessURL.Scheme == "http", proxy1Region.Nodes[0].ForceHTTP) // The second proxy region: require.Equal(t, "worst-proxy", proxy2Region.RegionName) require.Equal(t, "coder_worst-proxy", proxy2Region.RegionCode) require.Equal(t, 10002, proxy2Region.RegionID) require.False(t, proxy2Region.EmbeddedRelay) - require.Len(t, proxy2Region.Nodes, 2) // proxy + stun - require.Equal(t, "10002a", proxy2Region.Nodes[1].Name) - require.Equal(t, 10002, proxy2Region.Nodes[1].RegionID) - require.Equal(t, proxyAPI2.Options.AccessURL.Hostname(), proxy2Region.Nodes[1].HostName) - require.Equal(t, proxyAPI2.Options.AccessURL.Port(), fmt.Sprint(proxy2Region.Nodes[1].DERPPort)) - require.Equal(t, proxyAPI2.Options.AccessURL.Scheme == "http", proxy2Region.Nodes[1].ForceHTTP) + require.Len(t, proxy2Region.Nodes, 1) + require.Equal(t, "10002a", proxy2Region.Nodes[0].Name) + require.Equal(t, 10002, proxy2Region.Nodes[0].RegionID) + require.Equal(t, proxyAPI2.Options.AccessURL.Hostname(), proxy2Region.Nodes[0].HostName) + require.Equal(t, proxyAPI2.Options.AccessURL.Port(), fmt.Sprint(proxy2Region.Nodes[0].DERPPort)) + require.Equal(t, proxyAPI2.Options.AccessURL.Scheme == "http", proxy2Region.Nodes[0].ForceHTTP) }) t.Run("ConnectDERP", func(t *testing.T) { @@ -313,6 +313,8 @@ 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{ From 259a788849955475a4fd14e649a04900b96771e6 Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Wed, 9 Aug 2023 08:48:31 +0000 Subject: [PATCH 2/2] fixup! fix: remove stun nodes from workspace proxy regions --- enterprise/coderd/coderd.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index 838a462c28a73..1679165750b16 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -659,7 +659,7 @@ var ( lastDerpConflictLog time.Time ) -func derpMapper(logger slog.Logger, cfg *codersdk.DeploymentValues, proxyHealth *proxyhealth.ProxyHealth) func(*tailcfg.DERPMap) *tailcfg.DERPMap { +func derpMapper(logger slog.Logger, _ *codersdk.DeploymentValues, proxyHealth *proxyhealth.ProxyHealth) func(*tailcfg.DERPMap) *tailcfg.DERPMap { return func(derpMap *tailcfg.DERPMap) *tailcfg.DERPMap { derpMap = derpMap.Clone()