endpoint: allow empty Maps to disable Zoekt (#46280)

The only service discovery we support is via k8s. We want to ensure that
is working, so we have the invariant that if its empty we make all Map
lookups fail. However, this means that even if we explicitly wanted an
empty Map those would also fail (once communicated via config's
ServiceConnections).

This moves the enforcement of the invariant from the discovery loop to
just the k8s discovery code. Now our ServiceConnections based discovery
can return an empty list.

Additionally this allows us to simplify the condition around how we take
discovered endpoints/errors and update the map. This is because in all
functions on Map if err is non-nil we return the error.

This commit also allows an empty value for zoekt address. Explicitly setting
an empty value disables zoekt.

Test Plan: go test and manual testing with zoekt disabled.

cmd/frontend/internal/cli/config.go

@@ -587,6 +587,9 @@ func computeIndexedEndpoints() *endpoint.Map {
 	indexedEndpointsOnce.Do(func() {
 		if addr := zoektAddr(os.Environ()); addr != "" {
 			indexedEndpoints = endpoint.New(addr)
+		} else {
+			// It is OK to have no indexed search endpoints.
+			indexedEndpoints = endpoint.Static()
 		}
 	})
 	return indexedEndpoints

internal/endpoint/endpoint.go

@@ -13,9 +13,17 @@ import (
 
 	"github.com/sourcegraph/sourcegraph/internal/conf"
 	"github.com/sourcegraph/sourcegraph/internal/conf/conftypes"
-	"github.com/sourcegraph/sourcegraph/lib/errors"
 )
 
+// EmptyError is returned when looking up an endpoint on an empty map.
+type EmptyError struct {
+	URLSpec string
+}
+
+func (e *EmptyError) Error() string {
+	return fmt.Sprintf("endpoint.Map(%s) is empty", e.URLSpec)
+}
+
 // Map is a consistent hash map to URLs. It uses the kubernetes API to
 // watch the endpoints for a service and update the map when they change. It
 // can also fallback to static URLs if not configured for kubernetes.
@@ -106,7 +114,12 @@ func (m *Map) Get(key string) (string, error) {
 		return "", m.err
 	}
 
-	return m.hm.Lookup(key), nil
+	v := m.hm.Lookup(key)
+	if v == "" {
+		return "", &EmptyError{URLSpec: m.urlspec}
+	}
+
+	return v, nil
 }
 
 // GetN gets the n closest URLs in the hash to the provided key.
@@ -120,6 +133,16 @@ func (m *Map) GetN(key string, n int) ([]string, error) {
 		return nil, m.err
 	}
 
+	// LookupN can fail if n > len(nodes), but the client code will have a
+	// race. So double check while we hold the lock.
+	nodes := len(m.hm.Nodes())
+	if nodes == 0 {
+		return nil, &EmptyError{URLSpec: m.urlspec}
+	}
+	if n > nodes {
+		n = nodes
+	}
+
 	return m.hm.LookupN(key, n), nil
 }
 
@@ -137,6 +160,11 @@ func (m *Map) GetMany(keys ...string) ([]string, error) {
 		return nil, m.err
 	}
 
+	// If we are doing a lookup ensure we are not empty.
+	if len(keys) > 0 && len(m.hm.Nodes()) == 0 {
+		return nil, &EmptyError{URLSpec: m.urlspec}
+	}
+
 	vals := make([]string, len(keys))
 	for i := range keys {
 		vals[i] = m.hm.Lookup(keys[i])
@@ -186,29 +214,18 @@ func (m *Map) sync(ch chan endpoints, ready chan struct{}) {
 			log.Error(eps.Error),
 		)
 
-		switch {
-		case eps.Error != nil:
-			m.mu.Lock()
-			m.err = eps.Error
-			m.mu.Unlock()
-		case len(eps.Endpoints) > 0:
-			metricEndpointSize.WithLabelValues(eps.Service).Set(float64(len(eps.Endpoints)))
+		metricEndpointSize.WithLabelValues(eps.Service).Set(float64(len(eps.Endpoints)))
 
-			hm := newConsistentHash(eps.Endpoints)
-			m.mu.Lock()
-			m.hm = hm
-			m.err = nil
-			m.mu.Unlock()
-		default:
-			m.mu.Lock()
-			m.err = errors.Errorf(
-				"no %s endpoints could be found (this may indicate more %s replicas are needed, contact support@sourcegraph.com for assistance)",
-				eps.Service,
-				eps.Service,
-			)
-			m.mu.Unlock()
+		var hm *rendezvous.Rendezvous
+		if eps.Error == nil {
+			hm = newConsistentHash(eps.Endpoints)
 		}
 
+		m.mu.Lock()
+		m.hm = hm
+		m.err = eps.Error
+		m.mu.Unlock()
+
 		select {
 		case <-ready:
 		default:

internal/endpoint/endpoint_test.go

@@ -6,6 +6,8 @@ import (
 	"testing"
 	"time"
 
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/sourcegraph/sourcegraph/lib/errors"
 )
 
@@ -25,6 +27,35 @@ func TestStatic(t *testing.T) {
 	expectEndpoints(t, m, "http://test-1", "http://test-2")
 }
 
+func TestStatic_empty(t *testing.T) {
+	m := Static()
+	expectEndpoints(t, m)
+
+	// Empty maps should fail on Get but not on Endpoints
+	_, err := m.Get("foo")
+	if _, ok := err.(*EmptyError); !ok {
+		t.Fatal("Get should return EmptyError")
+	}
+
+	_, err = m.GetN("foo", 5)
+	if _, ok := err.(*EmptyError); !ok {
+		t.Fatal("GetN should return EmptyError")
+	}
+
+	_, err = m.GetMany("foo")
+	if _, ok := err.(*EmptyError); !ok {
+		t.Fatal("GetMany should return EmptyError")
+	}
+
+	eps, err := m.Endpoints()
+	if err != nil {
+		t.Fatal("Endpoints should not return an error")
+	}
+	if len(eps) != 0 {
+		t.Fatal("Endpoints should be empty")
+	}
+}
+
 func TestGetN(t *testing.T) {
 	endpoints := []string{"http://test-1", "http://test-2", "http://test-3", "http://test-4"}
 	m := Static(endpoints...)
@@ -84,8 +115,8 @@ func expectEndpoints(t *testing.T, m *Map, endpoints ...string) {
 	}
 	if got, err := m.GetMany(keys...); err != nil {
 		t.Fatalf("GetMany failed: %v", err)
-	} else if !reflect.DeepEqual(got, vals) {
-		t.Fatalf("GetMany(%v) unexpected response:\ngot  %v\nwant %v", keys, got, vals)
+	} else if diff := cmp.Diff(vals, got, cmpopts.EquateEmpty()); diff != "" {
+		t.Fatalf("GetMany(%v) unexpected response (-want, +got):\n%s", keys, diff)
 	}
 }
 

internal/endpoint/k8s.go

@@ -73,6 +73,16 @@ func k8sDiscovery(logger log.Logger, urlspec, ns string, clientFactory func() (*
 				log.Strings("endpoints", eps),
 			)
 
+			if len(eps) == 0 {
+				err := errors.Errorf(
+					"no %s endpoints could be found (this may indicate more %s replicas are needed, contact support@sourcegraph.com for assistance)",
+					u.Service,
+					u.Service,
+				)
+				disco <- endpoints{Service: u.Service, Error: err}
+				return
+			}
+
 			disco <- endpoints{Service: u.Service, Endpoints: eps}
 		}