Changes based on feedback

psiphon/server/discovery.go

@@ -20,7 +20,6 @@
 package server
 
 import (
-	"context"
 	"net"
 	"sync"
 
@@ -40,23 +39,25 @@ type Discovery struct {
 	support         *SupportServices
 	currentStrategy string
 	discovery       *discovery.Discovery
-	cancelFunc      context.CancelFunc
 
 	sync.RWMutex
 }
 
-func newDiscovery(support *SupportServices) (*Discovery, error) {
-
-	d := &Discovery{
+func makeDiscovery(support *SupportServices) *Discovery {
+	return &Discovery{
 		support: support,
 	}
+}
+
+// Start starts discovery.
+func (d *Discovery) Start() error {
 
 	err := d.reload(false)
 	if err != nil {
-		return nil, errors.Trace(err)
+		return errors.Trace(err)
 	}
 
-	return d, nil
+	return nil
 }
 
 // reload reinitializes the underlying discovery component. If reloadedTactics
@@ -64,7 +65,8 @@ func newDiscovery(support *SupportServices) (*Discovery, error) {
 // underlying discovery component is not reinitialized.
 func (d *Discovery) reload(reloadedTactics bool) error {
 
-	// Determine which discovery strategy to use
+	// Determine which discovery strategy to use. Assumes no GeoIP targeting
+	// for the ServerDiscoveryStrategy tactic.
 
 	p, err := d.support.ServerTacticsParametersCache.Get(NewGeoIPData())
 	if err != nil {
@@ -76,8 +78,8 @@ func (d *Discovery) reload(reloadedTactics bool) error {
 		strategy = p.String(parameters.ServerDiscoveryStrategy)
 	}
 	if strategy == "" {
-		// No tactics are configured; default to classic discovery.
-		strategy = DISCOVERY_STRATEGY_CLASSIC
+		// No tactics are configured; default to consistent discovery.
+		strategy = DISCOVERY_STRATEGY_CONSISTENT
 	}
 
 	// Do not reinitialize underlying discovery component if only tactics have
@@ -97,8 +99,7 @@ func (d *Discovery) reload(reloadedTactics bool) error {
 		}
 	} else if strategy == DISCOVERY_STRATEGY_CLASSIC {
 		discoveryStrategy, err = discovery.NewClassicDiscovery(
-			d.support.Config.DiscoveryValueHMACKey,
-			discovery.RealClock{})
+			d.support.Config.DiscoveryValueHMACKey)
 		if err != nil {
 			return errors.Trace(err)
 		}
@@ -109,42 +110,42 @@ func (d *Discovery) reload(reloadedTactics bool) error {
 	// Initialize and set underlying discovery component. Replaces old
 	// component if discovery is already initialized.
 
-	ctx, cancelFunc := context.WithCancel(context.Background())
+	oldDiscovery := d.discovery
 
-	discovery, err := discovery.NewDiscovery(
-		ctx,
-		discovery.RealClock{},
+	discovery := discovery.MakeDiscovery(
 		d.support.PsinetDatabase.GetDiscoveryServers(),
 		discoveryStrategy)
-	if err != nil {
-		cancelFunc()
-		return errors.Trace(err)
-	}
+
+	discovery.Start()
 
 	d.Lock()
 
+	d.discovery = discovery
+	d.currentStrategy = strategy
+
+	d.Unlock()
+
 	// Ensure resources used by previous underlying discovery component are
 	// cleaned up.
-	// Note: a more efficient impementation would not reinitialize the underlying
-	// discovery entirely if the discovery strategy has not changed, but
+	// Note: a more efficient impementation would not recreate the underlying
+	// discovery instance if the discovery strategy has not changed, but
 	// instead would update the underlying set of discovery servers if the set
 	// of discovery servers has changed.
-	if d.cancelFunc != nil {
-		d.cancelFunc()
+	if oldDiscovery != nil {
+		oldDiscovery.Stop()
 	}
 
-	d.discovery = discovery
-	d.cancelFunc = cancelFunc
-	d.currentStrategy = strategy
-
-	d.Unlock()
-
 	log.WithTraceFields(
-		LogFields{"event_name": "discovery_strategy"}).Infof("using %s discovery", strategy)
+		LogFields{"discovery_strategy": strategy}).Infof("reloaded discovery")
 
 	return nil
 }
 
+// Stop stops discovery and cleans up underlying resources.
+func (d *Discovery) Stop() {
+	d.discovery.Stop()
+}
+
 // DiscoverServers selects new encoded server entries to be "discovered" by
 // the client, using the client's IP address as the input into the discovery
 // algorithm.

psiphon/server/discovery/classic.go

@@ -25,6 +25,7 @@ import (
 	"math"
 	"net"
 	"sync"
+	"time"
 
 	"github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon/server/psinet"
 )
@@ -37,7 +38,11 @@ type classicDiscovery struct {
 	sync.RWMutex
 }
 
-func NewClassicDiscovery(discoveryValueHMACKey string, clk clock) (*classicDiscovery, error) {
+func NewClassicDiscovery(discoveryValueHMACKey string) (*classicDiscovery, error) {
+	return newClassicDiscovery(discoveryValueHMACKey, realClock{})
+}
+
+func newClassicDiscovery(discoveryValueHMACKey string, clk clock) (*classicDiscovery, error) {
 	return &classicDiscovery{
 		clk:                   clk,
 		discoveryValueHMACKey: discoveryValueHMACKey,
@@ -91,7 +96,7 @@ func (c *classicDiscovery) discoverServers(discoveryValue int) []*psinet.Discove
 	}
 
 	timeInSeconds := int(discoveryDate.Unix())
-	servers := selectServers(buckets, timeInSeconds, discoveryValue)
+	servers := selectServers(buckets, timeInSeconds, discoveryValue, discoveryDate)
 
 	return servers
 }
@@ -112,7 +117,10 @@ func (c *classicDiscovery) discoverServers(discoveryValue int) []*psinet.Discove
 // priority: if there are only a couple of servers, for example, IP address alone
 // determines the outcome.
 func selectServers(
-	buckets [][]*psinet.DiscoveryServer, timeInSeconds, discoveryValue int) []*psinet.DiscoveryServer {
+	buckets [][]*psinet.DiscoveryServer,
+	timeInSeconds,
+	discoveryValue int,
+	discoveryDate time.Time) []*psinet.DiscoveryServer {
 
 	TIME_GRANULARITY := 3600
 
@@ -131,10 +139,14 @@ func selectServers(
 	if len(bucket) == 0 {
 		return nil
 	}
-
-	// TODO: consider checking that server is in its discover window
 	server := bucket[timeStrategyValue%len(bucket)]
 
+	// Double check that server is discoverable at this time.
+	if discoveryDate.Before(server.DiscoveryDateRange[0]) ||
+		!discoveryDate.Before(server.DiscoveryDateRange[1]) {
+		return nil
+	}
+
 	serverList := make([]*psinet.DiscoveryServer, 1)
 	serverList[0] = server
 

psiphon/server/discovery/classic_test.go

@@ -60,7 +60,10 @@ func TestDiscoveryBuckets(t *testing.T) {
 
 	servers := make([]*psinet.DiscoveryServer, 0)
 	for i := 0; i < 105; i++ {
-		servers = append(servers, &psinet.DiscoveryServer{EncodedServerEntry: strconv.Itoa(i)})
+		servers = append(servers, &psinet.DiscoveryServer{
+			EncodedServerEntry: strconv.Itoa(i),
+			DiscoveryDateRange: []time.Time{time.Time{}, time.Now()},
+		})
 	}
 
 	t.Run("5 servers, 5 buckets", func(t *testing.T) {
@@ -113,7 +116,7 @@ func TestDiscoveryBuckets(t *testing.T) {
 
 			buckets := bucketizeServerList(servers, calculateBucketCount(len(servers)))
 
-			for _, server := range selectServers(buckets, i*int(time.Hour/time.Second), discoveryValue) {
+			for _, server := range selectServers(buckets, i*int(time.Hour/time.Second), discoveryValue, time.Time{}) {
 				discoveredServers[server.EncodedServerEntry] = true
 			}
 		}

psiphon/server/discovery/consistent.go

@@ -36,6 +36,7 @@ func (h hasher) Sum64(data []byte) uint64 {
 }
 
 type consistentHashingDiscovery struct {
+	clk    clock
 	config *consistent.Config
 	ring   *consistent.Consistent
 
@@ -43,7 +44,12 @@ type consistentHashingDiscovery struct {
 }
 
 func NewConsistentHashingDiscovery() (*consistentHashingDiscovery, error) {
+	return newConsistentHashingDiscovery(realClock{})
+}
+
+func newConsistentHashingDiscovery(clk clock) (*consistentHashingDiscovery, error) {
 	return &consistentHashingDiscovery{
+		clk: clk,
 		config: &consistent.Config{
 			PartitionCount:    0, // set in serversChanged
 			ReplicationFactor: 1, // ensure all servers are discoverable
@@ -68,8 +74,8 @@ func (c *consistentHashingDiscovery) serversChanged(newServers []*psinet.Discove
 		// Note: requires full reinitialization because we cannot change
 		// PartitionCount on the fly. Add/Remove do not update PartitionCount
 		// and updating ParitionCount is required to ensure that there is not
-		// a panic in the consistent package and that all servers are
-		// discoverable.
+		// a panic in the Psiphon-Labs/consistent package and that all servers
+		// are discoverable.
 		c.config.PartitionCount = len(newServers)
 
 		c.RWMutex.Lock()
@@ -88,12 +94,21 @@ func (c *consistentHashingDiscovery) selectServers(clientIP net.IP) []*psinet.Di
 		return nil
 	}
 
-	server := c.ring.LocateKey(clientIP)
-	if server == nil {
+	member := c.ring.LocateKey(clientIP)
+	if member == nil {
 		// Should never happen.
 		return nil
 	}
 
-	// TODO: consider checking that server is in its discover window
-	return []*psinet.DiscoveryServer{server.(*psinet.DiscoveryServer)}
+	server := member.(*psinet.DiscoveryServer)
+
+	discoveryDate := c.clk.Now()
+
+	// Double check that server is discoverable at this time.
+	if discoveryDate.Before(server.DiscoveryDateRange[0]) ||
+		!discoveryDate.Before(server.DiscoveryDateRange[1]) {
+		return nil
+	}
+
+	return []*psinet.DiscoveryServer{server}
 }

psiphon/server/discovery/discovery.go

@@ -23,6 +23,7 @@ package discovery
 import (
 	"context"
 	"net"
+	"sync"
 	"time"
 
 	"github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon/server/psinet"
@@ -39,16 +40,16 @@ type clock interface {
 	NewTimer(d time.Duration) timer
 }
 
-// RealClock implements clock using the time package in the Go standard library.
-type RealClock struct{}
+// realClock implements clock using the time package in the Go standard library.
+type realClock struct{}
 
-func (RealClock) Now() time.Time { return time.Now() }
+func (realClock) Now() time.Time { return time.Now() }
 
-func (RealClock) Until(t time.Time) time.Duration { return time.Until(t) }
+func (realClock) Until(t time.Time) time.Duration { return time.Until(t) }
 
-func (RealClock) After(d time.Duration) <-chan time.Time { return time.After(d) }
+func (realClock) After(d time.Duration) <-chan time.Time { return time.After(d) }
 
-func (RealClock) NewTimer(d time.Duration) timer { return &realTimer{t: time.NewTimer(d)} }
+func (realClock) NewTimer(d time.Duration) timer { return &realTimer{t: time.NewTimer(d)} }
 
 // timer is an interface matching what Timer in the time package provides in
 // the Go standard library, which enables using implementations in tests that
@@ -93,42 +94,57 @@ type DiscoveryStrategy interface {
 // Discovery is the combination of a discovery strategy with a set of discovery
 // servers. It's safe for concurrent usage.
 type Discovery struct {
-	all      []*psinet.DiscoveryServer
-	strategy DiscoveryStrategy
+	clk        clock
+	all        []*psinet.DiscoveryServer
+	strategy   DiscoveryStrategy
+	cancelFunc context.CancelFunc
+	wg         *sync.WaitGroup
 }
 
-// NewDiscovery creates a new Discovery instance, which uses the specified
-// strategy with the given discovery servers. Servers are discoverable when the
-// current time falls within their discovery window, i.e.
-// DiscoveryDateRange[0] <= clk.Now() < DiscoveryDateRange[1].
-//
-// Cancelling ctx stops the underlying go routines and, therefore, ctx should
-// be cancelled as soon as Discovery is no longer needed. Discovery should not
-// be used after this because the set of discoverable servers will no longer be
-// updated so it may contain servers that are no longer in their discover
-// window and exclude servers that are.
-func NewDiscovery(
-	ctx context.Context,
-	clk clock,
+// MakeDiscovery creates a new Discovery instance, which uses the specified
+// strategy with the given discovery servers.
+func MakeDiscovery(
 	servers []*psinet.DiscoveryServer,
-	strategy DiscoveryStrategy) (*Discovery, error) {
+	strategy DiscoveryStrategy) *Discovery {
 
-	current, nextUpdate := discoverableServers(servers, clk)
+	return makeDiscovery(realClock{}, servers, strategy)
+}
 
-	strategy.serversChanged(current)
+func makeDiscovery(
+	clk clock,
+	servers []*psinet.DiscoveryServer,
+	strategy DiscoveryStrategy) *Discovery {
 
 	d := Discovery{
+		clk:      clk,
 		all:      servers,
 		strategy: strategy,
+		wg:       new(sync.WaitGroup),
 	}
 
+	return &d
+}
+
+// Start starts discovery. Servers are discoverable when the current time
+// falls within their discovery date range, i.e. DiscoveryDateRange[0] <=
+// clk.Now() < DiscoveryDateRange[1].
+func (d *Discovery) Start() {
+
+	current, nextUpdate := discoverableServers(d.all, d.clk)
+
+	d.strategy.serversChanged(current)
+
+	ctx, cancelFunc := context.WithCancel(context.Background())
+	d.cancelFunc = cancelFunc
+	d.wg.Add(1)
+
 	// Update the set of discovery servers used by the chosen discovery
 	// algorithm, and therefore discoverable with SelectServers, everytime a
-	// server enters, or exits, its discovery window.
+	// server enters, or exits, its discovery date range.
 	go func() {
 		for ctx.Err() == nil {
 			// Wait until the next time a server enters, or exits, its
-			// discovery window.
+			// discovery date range.
 			//
 			// Warning: NewTimer uses the monotonic clock but discovery uses
 			// the wall clock. If there is wall clock drift, then it is
@@ -138,7 +154,7 @@ func NewDiscovery(
 			// not handled. One solution would be to periodically check if set
 			// of discoverable servers has changed in conjunction with using a
 			// timer.
-			t := clk.NewTimer(clk.Until(nextUpdate))
+			t := d.clk.NewTimer(d.clk.Until(nextUpdate))
 
 			select {
 			case <-t.C():
@@ -148,18 +164,18 @@ func NewDiscovery(
 			}
 			t.Stop()
 
-			// Note: servers with a discovery window in the past are not
-			// removed from d.all incase the wall clock has drifted; otherwise,
-			// we risk removing them prematurely.
-			servers, nextUpdate = discoverableServers(d.all, clk)
+			// Note: servers with a discovery date range in the past are not
+			// removed from d.all in case the wall clock has drifted;
+			// otherwise, we risk removing them prematurely.
+			servers, nextUpdate := discoverableServers(d.all, d.clk)
 
 			// Update the set of discoverable servers.
-			strategy.serversChanged(servers)
+			d.strategy.serversChanged(servers)
 
 			if nextUpdate == (time.Time{}) {
-				// The discovery windows of all candidate discovery servers are
-				// in the past. No more serversChanged calls willl be made to
-				// DiscoveryStrategy.
+				// The discovery date range of all candidate discovery servers
+				// are in the past. No more serversChanged calls will be made
+				// to DiscoveryStrategy.
 				//
 				// Warning: at this point if the wall clock has drifted but
 				// will correct itself in the future such that the set of
@@ -172,9 +188,18 @@ func NewDiscovery(
 				break
 			}
 		}
+		d.wg.Done()
 	}()
+}
 
-	return &d, nil
+// Stop stops discovery and cleans up underlying resources. Stop should be
+// invoked as soon as Discovery is no longer needed. Discovery should not be
+// used after this because the set of discoverable servers will no longer be
+// updated, so it may contain servers that are no longer discoverable and
+// exclude servers that are.
+func (d *Discovery) Stop() {
+	d.cancelFunc()
+	d.wg.Wait()
 }
 
 // SelectServers selects new server entries to be "discovered" by the client,
@@ -186,7 +211,7 @@ func (d *Discovery) SelectServers(clientIP net.IP) []*psinet.DiscoveryServer {
 
 // discoverableServers returns all servers in discoveryServers that are currently
 // eligible for discovery along with the next time that a server in
-// discoveryServers will enter, or exit, its discovery window.
+// discoveryServers will enter, or exit, its discovery date range.
 func discoverableServers(
 	discoveryServers []*psinet.DiscoveryServer,
 	clk clock) (discoverableServers []*psinet.DiscoveryServer, nextUpdate time.Time) {
@@ -198,29 +223,27 @@ func discoverableServers(
 	var nextServerRemove time.Time
 
 	for _, server := range discoveryServers {
-		// All servers that are discoverable on this day are eligible for discovery
 		if len(server.DiscoveryDateRange) == 2 {
-			if !now.Before(server.DiscoveryDateRange[0]) &&
-				now.Before(server.DiscoveryDateRange[1]) {
-
+			if now.Before(server.DiscoveryDateRange[0]) {
+				// Next server that will enter its discovery date range.
+				if nextServerAdd == (time.Time{}) || server.DiscoveryDateRange[0].Before(nextServerAdd) {
+					nextServerAdd = server.DiscoveryDateRange[0]
+				}
+			} else if now.Before(server.DiscoveryDateRange[1]) {
 				discoverableServers = append(discoverableServers, server)
 
-				// Next server that will exit its discovery window
+				// Next server that will exit its discovery date range.
 				if nextServerRemove == (time.Time{}) || server.DiscoveryDateRange[1].Before(nextServerRemove) {
 					nextServerRemove = server.DiscoveryDateRange[1]
 				}
-			} else if now.Before(server.DiscoveryDateRange[0]) {
-				// Next server that will enter its discovery window
-				if nextServerAdd == (time.Time{}) || server.DiscoveryDateRange[0].Before(nextServerAdd) {
-					nextServerAdd = server.DiscoveryDateRange[0]
-				}
 			}
 		}
 	}
 
 	// The next time the set of servers eligible for discovery changes is
 	// whichever occurs first: the next time a server enters its discovery
-	// window or the next time a server exits its discovery window.
+	// discovery date range or the next time a server exits its discovery
+	// date range.
 	nextUpdate = nextServerAdd
 	if nextServerAdd == (time.Time{}) ||
 		(nextServerRemove.Before(nextServerAdd) && nextServerRemove != (time.Time{})) {

psiphon/server/discovery/discovery_test.go

@@ -20,7 +20,6 @@
 package discovery
 
 import (
-	"context"
 	"math/rand"
 	"net"
 	"sync"
@@ -145,13 +144,9 @@ func runDiscoveryTest(tt *discoveryTest, now time.Time) error {
 		return errors.Trace(err)
 	}
 
-	ctx, cancelFunc := context.WithCancel(context.Background())
-	defer cancelFunc()
+	discovery := makeDiscovery(&clk, tt.servers, strategy)
 
-	discovery, err := NewDiscovery(ctx, &clk, tt.servers, strategy)
-	if err != nil {
-		return errors.Trace(err)
-	}
+	discovery.Start()
 
 	for _, check := range tt.checks {
 		time.Sleep(1 * time.Second) // let async code complete
@@ -184,6 +179,8 @@ func runDiscoveryTest(tt *discoveryTest, now time.Time) error {
 		}
 	}
 
+	discovery.Stop()
+
 	return nil
 }
 
@@ -225,7 +222,7 @@ func TestDiscoveryTestClock(t *testing.T) {
 		{
 			name: "classic",
 			newDiscoveryStrategy: func(clk clock) (DiscoveryStrategy, error) {
-				return NewClassicDiscovery("discoveryValueHMACKey", clk)
+				return newClassicDiscovery("discoveryValueHMACKey", clk)
 			},
 			servers: []*psinet.DiscoveryServer{
 				server1,
@@ -266,7 +263,7 @@ func TestDiscoveryTestClock(t *testing.T) {
 		{
 			name: "consistent",
 			newDiscoveryStrategy: func(clk clock) (DiscoveryStrategy, error) {
-				return NewConsistentHashingDiscovery()
+				return newConsistentHashingDiscovery(clk)
 			},
 			servers: []*psinet.DiscoveryServer{
 				server1,

psiphon/server/psinet/psinet.go

@@ -40,9 +40,11 @@ const (
 	MAX_DATABASE_AGE_FOR_SERVER_ENTRY_VALIDITY = 48 * time.Hour
 )
 
-// Database serves Psiphon API data requests. It's safe for
-// concurrent usage. The Reload function supports hot reloading
-// of Psiphon network data while the server is running.
+// Database serves Psiphon API data requests. The Reload function supports hot
+// reloading of Psiphon network data while the server is running.
+//
+// All of the methods on Database are thread-safe, but callers must not mutate
+// any returned data. The struct may be safely shared across goroutines.
 type Database struct {
 	common.ReloadableFile
 

psiphon/server/server_test.go

@@ -816,7 +816,7 @@ func runServer(t *testing.T, runConfig *runServerConfig) {
 			runConfig.doDestinationBytes,
 			runConfig.applyPrefix,
 			runConfig.forceFragmenting,
-			"consistent",
+			"classic",
 		)
 	}
 
@@ -902,6 +902,12 @@ func runServer(t *testing.T, runConfig *runServerConfig) {
 		}
 
 		if logFields["event_name"] == nil {
+			if logFields["discovery_strategy"] != nil {
+				select {
+				case discoveryLog <- logFields:
+				default:
+				}
+			}
 			return
 		}
 
@@ -921,11 +927,6 @@ func runServer(t *testing.T, runConfig *runServerConfig) {
 			case serverTunnelLog <- logFields:
 			default:
 			}
-		case "discovery_strategy":
-			select {
-			case discoveryLog <- logFields:
-			default:
-			}
 		}
 	})
 
@@ -1040,7 +1041,7 @@ func runServer(t *testing.T, runConfig *runServerConfig) {
 				runConfig.doDestinationBytes,
 				runConfig.applyPrefix,
 				runConfig.forceFragmenting,
-				"classic",
+				"consistent",
 			)
 		}
 
@@ -1430,7 +1431,7 @@ func runServer(t *testing.T, runConfig *runServerConfig) {
 			runConfig.doBurstMonitor,
 			false,
 			false, false,
-			"classic")
+			"consistent")
 
 		p, _ := os.FindProcess(os.Getpid())
 		p.Signal(syscall.SIGUSR1)
@@ -1641,33 +1642,33 @@ func runServer(t *testing.T, runConfig *runServerConfig) {
 
 	// Check logs emitted by discovery.
 
-	var expectedDiscoveryLogs []string
+	var expectedDiscoveryStrategy []string
 
 	// Discovery emits 1 log on startup.
 	if doServerTactics {
-		expectedDiscoveryLogs = append(expectedDiscoveryLogs, "consistent")
+		expectedDiscoveryStrategy = append(expectedDiscoveryStrategy, "classic")
 	} else {
-		expectedDiscoveryLogs = append(expectedDiscoveryLogs, "classic")
+		expectedDiscoveryStrategy = append(expectedDiscoveryStrategy, "consistent")
 	}
 	if runConfig.doHotReload {
 		if doServerTactics {
 			// Discovery emits 1 log when tactics are reloaded, which happens
 			// before the psinet database is reloaded.
-			expectedDiscoveryLogs = append(expectedDiscoveryLogs, "consistent")
+			expectedDiscoveryStrategy = append(expectedDiscoveryStrategy, "classic")
 		}
 		// Discovery emits 1 when the psinet database is reloaded.
-		expectedDiscoveryLogs = append(expectedDiscoveryLogs, "classic")
+		expectedDiscoveryStrategy = append(expectedDiscoveryStrategy, "consistent")
 	}
 
-	for _, expectedLog := range expectedDiscoveryLogs {
+	for _, expectedStrategy := range expectedDiscoveryStrategy {
 		select {
 		case logFields := <-discoveryLog:
-			if v, ok := logFields["msg"].(string); ok {
-				if !strings.Contains(v, expectedLog) {
-					t.Fatalf("expected discovery log to contain \"%s\"", expectedLog)
+			if strategy, ok := logFields["discovery_strategy"].(string); ok {
+				if strategy != expectedStrategy {
+					t.Fatalf("expected discovery strategy \"%s\"", expectedStrategy)
 				}
 			} else {
-				t.Fatalf("missing msg field")
+				t.Fatalf("missing discovery_strategy field")
 			}
 		default:
 			t.Fatalf("missing discovery log")

psiphon/server/services.go

@@ -126,12 +126,7 @@ func RunServices(configJSON []byte) (retErr error) {
 		support.PacketManipulator = packetManipulator
 	}
 
-	discovery, err := newDiscovery(support)
-	if err != nil {
-		return errors.Trace(err)
-	}
-
-	support.discovery = discovery
+	support.discovery = makeDiscovery(support)
 
 	// After this point, errors should be delivered to the errors channel and
 	// orderly shutdown should flow through to the end of the function to ensure
@@ -164,6 +159,21 @@ func RunServices(configJSON []byte) (retErr error) {
 		}
 	}
 
+	err = support.discovery.Start()
+	if err != nil {
+		select {
+		case errorChannel <- err:
+		default:
+		}
+	} else {
+		waitGroup.Add(1)
+		go func() {
+			defer waitGroup.Done()
+			<-shutdownBroadcast
+			support.discovery.Stop()
+		}()
+	}
+
 	if config.RunLoadMonitor() {
 		waitGroup.Add(1)
 		go func() {
@@ -486,6 +496,8 @@ func logIrregularTunnel(
 // components, which allows these data components to be refreshed
 // without restarting the server process.
 type SupportServices struct {
+	// TODO: make all fields non-exported, none are accessed outside
+	// of this package.
 	Config                       *Config
 	TrafficRulesSet              *TrafficRulesSet
 	OSLConfig                    *osl.Config