control/controlclient: start simplifying netmap fetch APIs

bradfitz · bradfitz · commit 526b0b6890d7 · 2022-06-19T15:43:12.000-07:00
Step 1 of many, cleaning up the direct/auto client & restarting map requests that leads to all the unnecessary map requests. Updates tailscale/corp#5761 Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
diff --git a/control/controlclient/auto.go b/control/controlclient/auto.go
@@ -41,6 +41,8 @@ func (g *LoginGoal) sendLogoutError(err error) {
 	}
 }
 
+var _ Client = (*Auto)(nil)
+
 // Auto connects to a tailcontrol server for a node.
 // It's a concrete implementation of the Client interface.
 type Auto struct {
@@ -462,7 +464,7 @@ func (c *Auto) mapRoutine() {
 			c.mu.Unlock()
 			health.SetInPollNetMap(false)
 
-			err := c.direct.PollNetMap(ctx, -1, func(nm *netmap.NetworkMap) {
+			err := c.direct.PollNetMap(ctx, func(nm *netmap.NetworkMap) {
 				health.SetInPollNetMap(true)
 				c.mu.Lock()
 
diff --git a/control/controlclient/direct.go b/control/controlclient/direct.go
@@ -620,18 +620,27 @@ func inTest() bool { return flag.Lookup("test.v") != nil }
 
 // PollNetMap makes a /map request to download the network map, calling cb with
 // each new netmap.
-//
-// maxPolls is how many network maps to download; common values are 1
-// or -1 (to keep a long-poll query open to the server).
-func (c *Direct) PollNetMap(ctx context.Context, maxPolls int, cb func(*netmap.NetworkMap)) error {
-	return c.sendMapRequest(ctx, maxPolls, cb)
+func (c *Direct) PollNetMap(ctx context.Context, cb func(*netmap.NetworkMap)) error {
+	return c.sendMapRequest(ctx, -1, false, cb)
+}
+
+// FetchNetMap fetches the netmap once.
+func (c *Direct) FetchNetMap(ctx context.Context) (*netmap.NetworkMap, error) {
+	var ret *netmap.NetworkMap
+	err := c.sendMapRequest(ctx, 1, false, func(nm *netmap.NetworkMap) {
+		ret = nm
+	})
+	if err == nil && ret == nil {
+		return nil, errors.New("[unexpected] sendMapRequest success without callback")
+	}
+	return ret, err
 }
 
 // SendLiteMapUpdate makes a /map request to update the server of our latest state,
 // but does not fetch anything. It returns an error if the server did not return a
 // successful 200 OK response.
 func (c *Direct) SendLiteMapUpdate(ctx context.Context) error {
-	return c.sendMapRequest(ctx, 1, nil)
+	return c.sendMapRequest(ctx, 1, false, nil)
 }
 
 // If we go more than pollTimeout without hearing from the server,
@@ -640,7 +649,7 @@ func (c *Direct) SendLiteMapUpdate(ctx context.Context) error {
 const pollTimeout = 120 * time.Second
 
 // cb nil means to omit peers.
-func (c *Direct) sendMapRequest(ctx context.Context, maxPolls int, cb func(*netmap.NetworkMap)) error {
+func (c *Direct) sendMapRequest(ctx context.Context, maxPolls int, readOnly bool, cb func(*netmap.NetworkMap)) error {
 	metricMapRequests.Add(1)
 	metricMapRequestsActive.Add(1)
 	defer metricMapRequestsActive.Add(-1)
@@ -703,6 +712,16 @@ func (c *Direct) sendMapRequest(ctx context.Context, maxPolls int, cb func(*netm
 		Hostinfo:      hi,
 		DebugFlags:    c.debugFlags,
 		OmitPeers:     cb == nil,
+
+		// On initial startup before we know our endpoints, set the ReadOnly flag
+		// to tell the control server not to distribute out our (empty) endpoints to peers.
+		// Presumably we'll learn our endpoints in a half second and do another post
+		// with useful results. The first POST just gets us the DERP map which we
+		// need to do the STUN queries to discover our endpoints.
+		// TODO(bradfitz): we skip this optimization in tests, though,
+		// because the e2e tests are currently hyperspecific about the
+		// ordering of things. The e2e tests need love.
+		ReadOnly: readOnly || (len(epStrs) == 0 && !everEndpoints && !inTest()),
 	}
 	var extraDebugFlags []string
 	if hi != nil && c.linkMon != nil && !c.skipIPForwardingCheck &&
@@ -725,17 +744,6 @@ func (c *Direct) sendMapRequest(ctx context.Context, maxPolls int, cb func(*netm
 	if c.newDecompressor != nil {
 		request.Compress = "zstd"
 	}
-	// On initial startup before we know our endpoints, set the ReadOnly flag
-	// to tell the control server not to distribute out our (empty) endpoints to peers.
-	// Presumably we'll learn our endpoints in a half second and do another post
-	// with useful results. The first POST just gets us the DERP map which we
-	// need to do the STUN queries to discover our endpoints.
-	// TODO(bradfitz): we skip this optimization in tests, though,
-	// because the e2e tests are currently hyperspecific about the
-	// ordering of things. The e2e tests need love.
-	if len(epStrs) == 0 && !everEndpoints && !inTest() {
-		request.ReadOnly = true
-	}
 
 	bodyData, err := encode(request, serverKey, serverNoiseKey, machinePrivKey)
 	if err != nil {

Original file line number	Diff line number	Diff line change
`@@ -41,6 +41,8 @@ func (g *LoginGoal) sendLogoutError(err error) {`
`41`	`41`	`}`
`42`	`42`	`}`
`43`	`43`
	`44`	`+var _ Client = (*Auto)(nil)`
	`45`	`+`
`44`	`46`	`// Auto connects to a tailcontrol server for a node.`
`45`	`47`	`// It's a concrete implementation of the Client interface.`
`46`	`48`	`type Auto struct {`
`@@ -462,7 +464,7 @@ func (c *Auto) mapRoutine() {`
`462`	`464`	`c.mu.Unlock()`
`463`	`465`	`health.SetInPollNetMap(false)`
`464`	`466`
`465`		`- err := c.direct.PollNetMap(ctx, -1, func(nm *netmap.NetworkMap) {`
	`467`	`+ err := c.direct.PollNetMap(ctx, func(nm *netmap.NetworkMap) {`
`466`	`468`	`health.SetInPollNetMap(true)`
`467`	`469`	`c.mu.Lock()`
`468`	`470`