targetRepo can be nil if the client didn't set it

When the client didn't set the target repository, the cache middleware
will panic right now. This change handles it, and lets the RPC handle
it.

The middleware could also handle the error, though that would intervene
with the 'usual' way and might catch a developer off guard.

5 files changed, 25 insertions, 19 deletions

diff --git a/internal/middleware/cache/cache.go b/internal/middleware/cache/cache.go
index 3400f9b00..da5cd07c8 100644
--- a/internal/middleware/cache/cache.go
+++ b/internal/middleware/cache/cache.go
@@ -9,7 +9,6 @@ import (
 	"github.com/golang/protobuf/proto"
 	"github.com/sirupsen/logrus"
 	diskcache "gitlab.com/gitlab-org/gitaly/internal/cache"
-	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/protoregistry"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"google.golang.org/grpc"
@@ -23,9 +22,6 @@ type Invalidator interface {
 	StartLease(repo *gitalypb.Repository) (diskcache.LeaseEnder, error)
 }
 
-// FeatureFlag enables the cache invalidator
-const FeatureFlag = "cache-invalidator"
-
 func methodErrLogger(method string) func(error) {
 	return func(err error) {
 		countMethodErr(method)
@@ -41,8 +37,7 @@ func shouldIgnore(fullMethod string) bool {
 // repository in a gRPC stream based RPC
 func StreamInvalidator(ci Invalidator, reg *protoregistry.Registry) grpc.StreamServerInterceptor {
 	return func(srv interface{}, ss grpc.ServerStream, info *grpc.StreamServerInfo, handler grpc.StreamHandler) error {
-		if !featureflag.IsEnabled(ss.Context(), FeatureFlag) ||
-			shouldIgnore(info.FullMethod) {
+		if shouldIgnore(info.FullMethod) {
 			return handler(srv, ss)
 		}
 
@@ -69,8 +64,7 @@ func StreamInvalidator(ci Invalidator, reg *protoregistry.Registry) grpc.StreamS
 // repository in a gRPC unary RPC
 func UnaryInvalidator(ci Invalidator, reg *protoregistry.Registry) grpc.UnaryServerInterceptor {
 	return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (resp interface{}, err error) {
-		if !featureflag.IsEnabled(ctx, FeatureFlag) ||
-			shouldIgnore(info.FullMethod) {
+		if shouldIgnore(info.FullMethod) {
 			return handler(ctx, req)
 		}
 
diff --git a/internal/middleware/cache/cache_test.go b/internal/middleware/cache/cache_test.go
index 3f5f4065f..fa3aef4a4 100644
--- a/internal/middleware/cache/cache_test.go
+++ b/internal/middleware/cache/cache_test.go
@@ -13,7 +13,6 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	diskcache "gitlab.com/gitlab-org/gitaly/internal/cache"
-	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/internal/middleware/cache"
 	"gitlab.com/gitlab-org/gitaly/internal/middleware/cache/testdata"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/protoregistry"
@@ -21,7 +20,6 @@ import (
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/health"
 	"google.golang.org/grpc/health/grpc_health_v1"
-	"google.golang.org/grpc/metadata"
 )
 
 //go:generate make testdata/stream.pb.go
@@ -47,8 +45,6 @@ func TestInvalidators(t *testing.T) {
 	ctx, cancel := context.WithTimeout(context.Background(), time.Second)
 	defer cancel()
 
-	ctx = enableCache(ctx)
-
 	svc := &testSvc{}
 
 	cli, cc, cleanup := newTestSvc(t, ctx, srvr, svc)
@@ -131,12 +127,6 @@ func TestInvalidators(t *testing.T) {
 	require.Equal(t, 3, mCache.(*mockCache).endedLeases.count)
 }
 
-func enableCache(ctx context.Context) context.Context {
-	return metadata.NewOutgoingContext(ctx, metadata.New(map[string]string{
-		featureflag.HeaderKey(cache.FeatureFlag): "true",
-	}))
-}
-
 // mockCache allows us to relay back via channel which repos are being
 // invalidated in the cache
 type mockCache struct {
diff --git a/internal/praefect/protoregistry/protoregistry.go b/internal/praefect/protoregistry/protoregistry.go
index 94fd464be..d4cf34c69 100644
--- a/internal/praefect/protoregistry/protoregistry.go
+++ b/internal/praefect/protoregistry/protoregistry.go
@@ -112,7 +112,18 @@ func (mi MethodInfo) getRepo(msg proto.Message, targetOid []int) (*gitalypb.Repo
 		)
 	}
 
-	return reflectFindRepoTarget(msg, targetOid)
+	repo, err := reflectFindRepoTarget(msg, targetOid)
+	switch {
+	case err != nil:
+		return nil, err
+	case repo == nil:
+		// it is possible for the target repo to not be set (especially in our unit
+		// tests designed to fail and this should return an error to prevent nil
+		// pointer dereferencing
+		return nil, ErrTargetRepoMissing
+	default:
+		return repo, nil
+	}
 }
 
 // UnmarshalRequestProto will unmarshal the bytes into the method's request
diff --git a/internal/praefect/protoregistry/targetrepo.go b/internal/praefect/protoregistry/targetrepo.go
index 16afe395c..ebca49fee 100644
--- a/internal/praefect/protoregistry/targetrepo.go
+++ b/internal/praefect/protoregistry/targetrepo.go
@@ -1,6 +1,7 @@
 package protoregistry
 
 import (
+	"errors"
 	"fmt"
 	"reflect"
 	"regexp"
@@ -15,6 +16,9 @@ const (
 	protobufOneOfTag = "protobuf_oneof"
 )
 
+// ErrTargetRepoMissing indicates that the target repo is missing or not set
+var ErrTargetRepoMissing = errors.New("target repo is not set")
+
 // reflectFindRepoTarget finds the target repository by using the OID to
 // navigate the struct tags
 // Warning: this reflection filled function is full of forbidden dark elf magic
diff --git a/internal/praefect/protoregistry/targetrepo_test.go b/internal/praefect/protoregistry/targetrepo_test.go
index 23708b53b..1982b5d64 100644
--- a/internal/praefect/protoregistry/targetrepo_test.go
+++ b/internal/praefect/protoregistry/targetrepo_test.go
@@ -84,6 +84,13 @@ func TestProtoRegistryTargetRepo(t *testing.T) {
 			expectRepo:           testRepos[1],
 			expectAdditionalRepo: testRepos[0],
 		},
+		{
+			desc:      "target repo is nil",
+			svc:       "RepositoryService",
+			method:    "RepackIncremental",
+			pbMsg:     &gitalypb.RepackIncrementalRequest{Repository: nil},
+			expectErr: protoregistry.ErrTargetRepoMissing,
+		},
 	}
 
 	for _, tc := range testcases {