Fishy config log warning

4 files changed, 99 insertions, 25 deletions

diff --git a/changelogs/unreleased/po-praefect-fishy-config.yml b/changelogs/unreleased/po-praefect-fishy-config.yml
new file mode 100644
index 000000000..7fff372d8
--- /dev/null
+++ b/changelogs/unreleased/po-praefect-fishy-config.yml
@@ -0,0 +1,5 @@
+---
+title: Fishy config log warning
+merge_request: 1570
+author:
+type: added
diff --git a/internal/praefect/helper_test.go b/internal/praefect/helper_test.go
index 0e4dc2ffe..ae55ade51 100644
--- a/internal/praefect/helper_test.go
+++ b/internal/praefect/helper_test.go
@@ -8,6 +8,7 @@ import (
 	"time"
 
 	"github.com/golang/protobuf/protoc-gen-go/descriptor"
+	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/client"
 	internalauth "gitlab.com/gitlab-org/gitaly/internal/auth"
@@ -34,8 +35,8 @@ func waitUntil(t *testing.T, ch <-chan struct{}, timeout time.Duration) {
 	}
 }
 
-// generates a praefect configuration with the specified
-// number of backend nodes
+// generates a praefect configuration with the specified number of backend
+// nodes
 func testConfig(backends int) config.Config {
 	cfg := config.Config{
 		VirtualStorageName: "praefect",
@@ -62,17 +63,47 @@ func testConfig(backends int) config.Config {
 	return cfg
 }
 
+// setupServer wires all praefect dependencies together via dependency
+// injection
+func setupServer(t testing.TB, conf config.Config, l *logrus.Entry, fds []*descriptor.FileDescriptorProto) (*MemoryDatastore, *conn.ClientConnections, *Server) {
+	var (
+		datastore   = NewMemoryDatastore(conf)
+		clientCC    = conn.NewClientConnections()
+		coordinator = NewCoordinator(l, datastore, clientCC, conf, fds...)
+	)
+
+	var defaultNode *models.Node
+	for _, n := range conf.Nodes {
+		if n.DefaultPrimary {
+			defaultNode = n
+		}
+	}
+	require.NotNil(t, defaultNode)
+
+	replmgr := NewReplMgr(
+		defaultNode.Storage,
+		l,
+		datastore,
+		clientCC,
+	)
+	server := NewServer(
+		coordinator,
+		replmgr,
+		nil,
+		l,
+		clientCC,
+		conf,
+	)
+
+	return datastore, clientCC, server
+}
+
 // runPraefectServer runs a praefect server with the provided mock servers.
 // Each mock server is keyed by the corresponding index of the node in the
 // config.Nodes. There must be a 1-to-1 mapping between backend server and
 // configured storage node.
 func runPraefectServerWithMock(t *testing.T, conf config.Config, backends map[int]mock.SimpleServiceServer) (mock.SimpleServiceClient, *Server, func()) {
-	var (
-		datastore   = NewMemoryDatastore(conf)
-		logEntry    = log.Default()
-		clientCC    = conn.NewClientConnections()
-		coordinator = NewCoordinator(logEntry, datastore, clientCC, conf, mustLoadProtoReg(t))
-	)
+	datastore, clientCC, prf := setupServer(t, conf, log.Default(), []*descriptor.FileDescriptorProto{mustLoadProtoReg(t)})
 
 	require.Equal(t, len(backends), len(conf.Nodes),
 		"mock server count doesn't match config nodes")
@@ -91,21 +122,6 @@ func runPraefectServerWithMock(t *testing.T, conf config.Config, backends map[in
 		datastore.storageNodes.m[id] = nodeStorage
 	}
 
-	replmgr := NewReplMgr(
-		"default",
-		logEntry,
-		datastore,
-		clientCC,
-	)
-	prf := NewServer(
-		coordinator,
-		replmgr,
-		nil,
-		logEntry,
-		clientCC,
-		conf,
-	)
-
 	listener, port := listenAvailPort(t)
 	t.Logf("praefect listening on port %d", port)
 
@@ -200,7 +216,7 @@ func runInternalGitalyServer(t *testing.T, token string) (*grpc.Server, string)
 	return server, "unix://" + serverSocketPath
 }
 
-func mustLoadProtoReg(t *testing.T) *descriptor.FileDescriptorProto {
+func mustLoadProtoReg(t testing.TB) *descriptor.FileDescriptorProto {
 	gz, _ := (*mock.SimpleRequest)(nil).Descriptor()
 	fd, err := protoregistry.ExtractFileDescriptor(gz)
 	require.NoError(t, err)
diff --git a/internal/praefect/server.go b/internal/praefect/server.go
index bf6f4d3de..c689bec4d 100644
--- a/internal/praefect/server.go
+++ b/internal/praefect/server.go
@@ -31,6 +31,23 @@ type Server struct {
 	repl              ReplMgr
 	s                 *grpc.Server
 	conf              config.Config
+	l                 *logrus.Entry
+}
+
+func (srv *Server) warnDupeAddrs(c config.Config) {
+	addrSet := map[string]struct{}{}
+	fishy := false
+	for _, n := range c.Nodes {
+		_, ok := addrSet[n.Address]
+		if ok {
+			srv.l.Warnf("more than one backend node is hosted at %s", n.Address)
+			fishy = true
+		}
+		addrSet[n.Address] = struct{}{}
+	}
+	if fishy {
+		srv.l.Warnf("your Praefect configuration may not offer actual redundancy")
+	}
 }
 
 // NewServer returns an initialized praefect gPRC proxy server configured
@@ -61,12 +78,17 @@ func NewServer(c *Coordinator, repl ReplMgr, grpcOpts []grpc.ServerOption, l *lo
 		)),
 	}...)
 
-	return &Server{
+	s := &Server{
 		s:                 grpc.NewServer(grpcOpts...),
 		repl:              repl,
 		clientConnections: clientConnections,
 		conf:              conf,
+		l:                 l,
 	}
+
+	s.warnDupeAddrs(conf)
+
+	return s
 }
 
 func proxyRequiredOpts(director proxy.StreamDirector) []grpc.ServerOption {
diff --git a/internal/praefect/server_test.go b/internal/praefect/server_test.go
index e7a6fdcb2..dfc28f04b 100644
--- a/internal/praefect/server_test.go
+++ b/internal/praefect/server_test.go
@@ -3,10 +3,13 @@ package praefect
 import (
 	"context"
 	"fmt"
+	"strings"
 	"testing"
 	"time"
 
 	"github.com/golang/protobuf/proto"
+	"github.com/sirupsen/logrus"
+	"github.com/sirupsen/logrus/hooks/test"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
@@ -144,3 +147,31 @@ func TestRejectBadStorage(t *testing.T) {
 	testhelper.RequireGrpcError(t, err, codes.InvalidArgument)
 	require.Equal(t, fmt.Sprintf("only messages for %s are allowed", conf.VirtualStorageName), status.Convert(err).Message())
 }
+
+func TestWarnDuplicateAddrs(t *testing.T) {
+	conf := config.Config{
+		VirtualStorageName: "praefect",
+		Nodes: []*models.Node{
+			&models.Node{
+				DefaultPrimary: true,
+				Storage:        "praefect-internal-0",
+				Address:        "tcp::/samesies",
+			},
+			&models.Node{
+				Storage: "praefect-internal-1",
+				Address: "tcp::/samesies",
+			},
+		},
+	}
+
+	tLogger, hook := test.NewNullLogger()
+
+	setupServer(t, conf, logrus.NewEntry(tLogger), nil) // instantiates a praefect server and triggers warning
+
+	for _, entry := range hook.Entries {
+		if strings.Contains(entry.Message, "more than one backend node") {
+			return // pass!
+		}
+	}
+	t.Fatal("could not find expected log message")
+}