Merge branch 'po-remove-cache-invalidator-ff' into 'master'

Remove cache invalidator feature flag

Closes #2092

See merge request gitlab-org/gitaly!1549

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 {