diff --git a/internal/gitaly/service/remote/fetch_internal_remote.go b/internal/gitaly/service/remote/fetch_internal_remote.go index 5c72264c1943185eb5f4e1580a5fda43e595a097..eee66218ef808828dc0bd5dad60e0ec626ba8de5 100644 --- a/internal/gitaly/service/remote/fetch_internal_remote.go +++ b/internal/gitaly/service/remote/fetch_internal_remote.go @@ -107,7 +107,7 @@ func (s *server) FetchInternalRemote(ctx context.Context, req *gitalypb.FetchInt if err := FetchInternalRemote(ctx, s.cfg, s.conns, repo, req.RemoteRepository); err != nil { var fetchErr fetchFailedError - if featureflag.IsDisabled(ctx, featureflag.FetchInternalRemoteErrors) && errors.As(err, &fetchErr) { + if s.isFetchInternalRemoteErrorsDisabled(ctx) && errors.As(err, &fetchErr) { // Design quirk: if the fetch fails, this RPC returns Result: false, but no error. ctxlogrus.Extract(ctx).WithError(fetchErr.err).WithField("stderr", fetchErr.stderr).Warn("git fetch failed") return &gitalypb.FetchInternalRemoteResponse{Result: false}, nil @@ -119,6 +119,13 @@ func (s *server) FetchInternalRemote(ctx context.Context, req *gitalypb.FetchInt return &gitalypb.FetchInternalRemoteResponse{Result: true}, nil } +func (s *server) isFetchInternalRemoteErrorsDisabled(ctx context.Context) bool { + // Since feature flags can only be turned on through requests from + // gitlab-rails we need a way to explicitly disable the feature without + // triggering a new release. + return featureflag.IsDisabled(ctx, featureflag.FetchInternalRemoteErrors) || s.disableFetchInternalRemoteErrors +} + // getRemoteDefaultBranch gets the default branch of a repository hosted on another Gitaly node. func getRemoteDefaultBranch(ctx context.Context, repo *gitalypb.Repository, conns *client.Pool) ([]byte, error) { serverInfo, err := helper.ExtractGitalyServer(ctx, repo.StorageName) diff --git a/internal/gitaly/service/remote/fetch_internal_remote_test.go b/internal/gitaly/service/remote/fetch_internal_remote_test.go index d593ca7223c01dfe310f7b5d60c1c5cee8da37e3..db784800021d706ba89f5912b1d66fccdb20afa7 100644 --- a/internal/gitaly/service/remote/fetch_internal_remote_test.go +++ b/internal/gitaly/service/remote/fetch_internal_remote_test.go @@ -260,6 +260,27 @@ func TestFailedFetchInternalRemote(t *testing.T) { require.NoError(t, err, "FetchInternalRemote is not supposed to return an error when 'git fetch' fails") require.False(t, c.GetResult()) }) + + t.Run("GITALY_DISABLE_FETCH_INTERNAL_REMOTE_ERRORS set", func(t *testing.T) { + cleanup := testhelper.ModifyEnvironment(t, "GITALY_DISABLE_FETCH_INTERNAL_REMOTE_ERRORS", "1") + t.Cleanup(cleanup) + + cfg, repo, _, client := setupRemoteService(t) + + ctx := testhelper.MergeOutgoingMetadata(ctx, testhelper.GitalyServersMetadataFromCfg(t, cfg)) + + // Non-existing remote repo + remoteRepo := &gitalypb.Repository{StorageName: repo.GetStorageName(), RelativePath: "fake.git"} + + request := &gitalypb.FetchInternalRemoteRequest{ + Repository: repo, + RemoteRepository: remoteRepo, + } + + c, err := client.FetchInternalRemote(ctx, request) + require.NoError(t, err, "FetchInternalRemote is not supposed to return an error when 'git fetch' fails") + require.False(t, c.GetResult()) + }) } func TestFailedFetchInternalRemoteDueToValidations(t *testing.T) { diff --git a/internal/gitaly/service/remote/server.go b/internal/gitaly/service/remote/server.go index 2d0bd3dafe0e208fd1b5283d129a40250fc781a8..c7ca4fadf7b567a0b853876456e8e04094b9550a 100644 --- a/internal/gitaly/service/remote/server.go +++ b/internal/gitaly/service/remote/server.go @@ -9,6 +9,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/rubyserver" "gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction" + "gitlab.com/gitlab-org/gitaly/v14/internal/helper/env" "gitlab.com/gitlab-org/gitaly/v14/internal/storage" "gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb" ) @@ -23,6 +24,8 @@ type server struct { txManager transaction.Manager conns *client.Pool + + disableFetchInternalRemoteErrors bool } // NewServer creates a new instance of a grpc RemoteServiceServer @@ -34,6 +37,11 @@ func NewServer( catfileCache catfile.Cache, txManager transaction.Manager, ) gitalypb.RemoteServiceServer { + disableFetchInternalRemoteErrors, err := env.GetBool("GITALY_DISABLE_FETCH_INTERNAL_REMOTE_ERRORS", false) + if err != nil { + panic(err) + } + return &server{ cfg: cfg, ruby: rs, @@ -45,6 +53,7 @@ func NewServer( client.WithDialer(client.HealthCheckDialer(client.DialContext)), client.WithDialOptions(client.FailOnNonTempDialError()...), ), + disableFetchInternalRemoteErrors: disableFetchInternalRemoteErrors, } } diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go index 4b8d05bf671a87dcf3881a6ca63cd97bdc6c2380..1d2767d4d80d1fd266846d8f4bc1d241bb777c97 100644 --- a/internal/metadata/featureflag/feature_flags.go +++ b/internal/metadata/featureflag/feature_flags.go @@ -12,7 +12,7 @@ var ( // GoUpdateRemoteMirror enables the Go implementation of UpdateRemoteMirror GoUpdateRemoteMirror = FeatureFlag{Name: "go_update_remote_mirror", OnByDefault: false} // FetchInternalRemoteErrors makes FetchInternalRemote return actual errors instead of a boolean - FetchInternalRemoteErrors = FeatureFlag{Name: "fetch_internal_remote_errors", OnByDefault: false} + FetchInternalRemoteErrors = FeatureFlag{Name: "fetch_internal_remote_errors", OnByDefault: true} // TxConfig enables transactional voting for SetConfig and DeleteConfig RPCs. TxConfig = FeatureFlag{Name: "tx_config", OnByDefault: true} // TxRemote enables transactional voting for AddRemote and DeleteRemote.