diff options
author | Zeger-Jan van de Weg <git@zjvandeweg.nl> | 2021-01-18 15:02:09 +0300 |
---|---|---|
committer | Zeger-Jan van de Weg <git@zjvandeweg.nl> | 2021-01-18 15:02:09 +0300 |
commit | cebd5cc82779b53d67e528bf1f31816389475a88 (patch) | |
tree | 2506015f0bfe80c09bcd383ebd58599bc9d0b9a4 | |
parent | 6dd2027292f0842be9accd7722ae8df53f5232e6 (diff) | |
parent | adfa1692fd9dfde07b89609a05b8801a40c18fd2 (diff) |
Merge branch 'master' into 'pks-revert-go-resolve-conflicts-default'
Conflicts:
internal/metadata/featureflag/feature_flags.go
20 files changed, 231 insertions, 502 deletions
diff --git a/changelogs/unreleased/ps-cleanup-ruby-fetch-remote.yml b/changelogs/unreleased/ps-cleanup-ruby-fetch-remote.yml new file mode 100644 index 000000000..e7a590c6f --- /dev/null +++ b/changelogs/unreleased/ps-cleanup-ruby-fetch-remote.yml @@ -0,0 +1,5 @@ +--- +title: Removal of ruby implementation of the FetchRemote +merge_request: 2967 +author: +type: removed diff --git a/internal/gitaly/service/conflicts/resolve_conflicts.go b/internal/gitaly/service/conflicts/resolve_conflicts.go index 0d7d495a7..024e2df1a 100644 --- a/internal/gitaly/service/conflicts/resolve_conflicts.go +++ b/internal/gitaly/service/conflicts/resolve_conflicts.go @@ -279,7 +279,7 @@ func (s *server) repoWithBranchCommit(ctx context.Context, srcRepo, targetRepo * return nil } - env, err := gitalyssh.UploadPackEnv(ctx, &gitalypb.SSHUploadPackRequest{Repository: targetRepo}) + env, err := gitalyssh.UploadPackEnv(ctx, s.cfg, &gitalypb.SSHUploadPackRequest{Repository: targetRepo}) if err != nil { return err } diff --git a/internal/gitaly/service/operations/commit_files.go b/internal/gitaly/service/operations/commit_files.go index ee9922272..df2f90024 100644 --- a/internal/gitaly/service/operations/commit_files.go +++ b/internal/gitaly/service/operations/commit_files.go @@ -429,7 +429,7 @@ func (s *Server) fetchMissingCommit(ctx context.Context, local, remote *gitalypb } func (s *Server) fetchRemoteObject(ctx context.Context, local, remote *gitalypb.Repository, sha string) error { - env, err := gitalyssh.UploadPackEnv(ctx, &gitalypb.SSHUploadPackRequest{ + env, err := gitalyssh.UploadPackEnv(ctx, s.cfg, &gitalypb.SSHUploadPackRequest{ Repository: remote, GitConfigOptions: []string{"uploadpack.allowAnySHA1InWant=true"}, }) diff --git a/internal/gitaly/service/register.go b/internal/gitaly/service/register.go index f7365d3fe..83f24ab49 100644 --- a/internal/gitaly/service/register.go +++ b/internal/gitaly/service/register.go @@ -87,7 +87,7 @@ func RegisterAll(grpcServer *grpc.Server, cfg config.Cfg, rubyServer *rubyserver )) gitalypb.RegisterWikiServiceServer(grpcServer, wiki.NewServer(rubyServer, locator)) gitalypb.RegisterConflictsServiceServer(grpcServer, conflicts.NewServer(rubyServer, cfg, locator)) - gitalypb.RegisterRemoteServiceServer(grpcServer, remote.NewServer(rubyServer, locator)) + gitalypb.RegisterRemoteServiceServer(grpcServer, remote.NewServer(cfg, rubyServer, locator)) gitalypb.RegisterServerServiceServer(grpcServer, server.NewServer(cfg.Storages)) gitalypb.RegisterObjectPoolServiceServer(grpcServer, objectpool.NewServer(cfg, locator)) gitalypb.RegisterHookServiceServer(grpcServer, hook.NewServer(cfg, hookManager)) diff --git a/internal/gitaly/service/remote/fetch_internal_remote.go b/internal/gitaly/service/remote/fetch_internal_remote.go index 72fe4765a..321be53b0 100644 --- a/internal/gitaly/service/remote/fetch_internal_remote.go +++ b/internal/gitaly/service/remote/fetch_internal_remote.go @@ -27,7 +27,7 @@ func (s *server) FetchInternalRemote(ctx context.Context, req *gitalypb.FetchInt return nil, status.Errorf(codes.InvalidArgument, "FetchInternalRemote: %v", err) } - env, err := gitalyssh.UploadPackEnv(ctx, &gitalypb.SSHUploadPackRequest{Repository: req.RemoteRepository}) + env, err := gitalyssh.UploadPackEnv(ctx, s.cfg, &gitalypb.SSHUploadPackRequest{Repository: req.RemoteRepository}) if err != nil { return nil, fmt.Errorf("upload pack environment: %w", err) } diff --git a/internal/gitaly/service/remote/server.go b/internal/gitaly/service/remote/server.go index 7da72b02d..ef48bbf9a 100644 --- a/internal/gitaly/service/remote/server.go +++ b/internal/gitaly/service/remote/server.go @@ -2,12 +2,14 @@ package remote import ( "gitlab.com/gitlab-org/gitaly/client" + "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/internal/gitaly/rubyserver" "gitlab.com/gitlab-org/gitaly/internal/storage" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" ) type server struct { + cfg config.Cfg ruby *rubyserver.Server locator storage.Locator @@ -15,8 +17,9 @@ type server struct { } // NewServer creates a new instance of a grpc RemoteServiceServer -func NewServer(rs *rubyserver.Server, locator storage.Locator) gitalypb.RemoteServiceServer { +func NewServer(cfg config.Cfg, rs *rubyserver.Server, locator storage.Locator) gitalypb.RemoteServiceServer { return &server{ + cfg: cfg, ruby: rs, locator: locator, conns: client.NewPoolWithOptions( diff --git a/internal/gitaly/service/remote/testhelper_test.go b/internal/gitaly/service/remote/testhelper_test.go index 44cd2bdaa..c90669e73 100644 --- a/internal/gitaly/service/remote/testhelper_test.go +++ b/internal/gitaly/service/remote/testhelper_test.go @@ -38,7 +38,7 @@ func testMain(m *testing.M) int { func RunRemoteServiceServer(t *testing.T, opts ...testhelper.TestServerOpt) (string, func()) { srv := testhelper.NewServer(t, nil, nil, opts...) - gitalypb.RegisterRemoteServiceServer(srv.GrpcServer(), NewServer(RubyServer, config.NewLocator(config.Config))) + gitalypb.RegisterRemoteServiceServer(srv.GrpcServer(), NewServer(config.Config, RubyServer, config.NewLocator(config.Config))) reflection.Register(srv.GrpcServer()) srv.Start(t) diff --git a/internal/gitaly/service/repository/fetch.go b/internal/gitaly/service/repository/fetch.go index 417e1f2f8..3d7d357a7 100644 --- a/internal/gitaly/service/repository/fetch.go +++ b/internal/gitaly/service/repository/fetch.go @@ -76,7 +76,7 @@ func (s *server) FetchSourceBranch(ctx context.Context, req *gitalypb.FetchSourc // There's no need to perform the fetch if we already have the object // available. if !containsObject { - env, err := gitalyssh.UploadPackEnv(ctx, &gitalypb.SSHUploadPackRequest{Repository: req.SourceRepository}) + env, err := gitalyssh.UploadPackEnv(ctx, s.cfg, &gitalypb.SSHUploadPackRequest{Repository: req.SourceRepository}) if err != nil { return nil, err } diff --git a/internal/gitaly/service/repository/fetch_remote.go b/internal/gitaly/service/repository/fetch_remote.go index 486feb106..fc08616c4 100644 --- a/internal/gitaly/service/repository/fetch_remote.go +++ b/internal/gitaly/service/repository/fetch_remote.go @@ -13,51 +13,17 @@ import ( "time" "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" - "github.com/prometheus/client_golang/prometheus" "github.com/sirupsen/logrus" "gitlab.com/gitlab-org/gitaly/internal/command" "gitlab.com/gitlab-org/gitaly/internal/errors" "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/gitaly/rubyserver" "gitlab.com/gitlab-org/gitaly/internal/helper" - "gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" "google.golang.org/grpc/status" ) -var ( - fetchRemoteImplCounter = prometheus.NewCounterVec( - prometheus.CounterOpts{ - Name: "gitaly_fetch_remote_counter", - Help: "Number of calls to FetchRemote rpc for each implementation (ruby/go)", - }, - []string{"impl"}, - ) -) - -func init() { - prometheus.MustRegister(fetchRemoteImplCounter) -} - func (s *server) FetchRemote(ctx context.Context, req *gitalypb.FetchRemoteRequest) (*gitalypb.FetchRemoteResponse, error) { - if featureflag.IsDisabled(ctx, featureflag.GoFetchRemote) { - fetchRemoteImplCounter.WithLabelValues("ruby").Inc() - - client, err := s.ruby.RepositoryServiceClient(ctx) - if err != nil { - return nil, err - } - - clientCtx, err := rubyserver.SetHeaders(ctx, s.locator, req.GetRepository()) - if err != nil { - return nil, err - } - - return client.FetchRemote(clientCtx, req) - } - - fetchRemoteImplCounter.WithLabelValues("go").Inc() - if err := s.validateFetchRemoteRequest(req); err != nil { return nil, err } diff --git a/internal/gitaly/service/repository/fetch_remote_test.go b/internal/gitaly/service/repository/fetch_remote_test.go index 1006fd59c..c0577df6a 100644 --- a/internal/gitaly/service/repository/fetch_remote_test.go +++ b/internal/gitaly/service/repository/fetch_remote_test.go @@ -1,7 +1,6 @@ package repository import ( - "context" "fmt" "io/ioutil" "net/http" @@ -16,12 +15,10 @@ import ( "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/internal/helper/text" - "gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/internal/storage" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" "google.golang.org/grpc/codes" - "google.golang.org/grpc/metadata" ) func copyRepoWithNewRemote(t *testing.T, repo *gitalypb.Repository, locator storage.Locator, remote string) (*gitalypb.Repository, string) { @@ -48,40 +45,32 @@ func TestFetchRemoteSuccess(t *testing.T) { client, conn := newRepositoryClient(t, serverSocketPath) defer conn.Close() - testhelper.NewFeatureSets([]featureflag.FeatureFlag{ - featureflag.GoFetchRemote, - }).Run(t, func(t *testing.T, ctx context.Context) { - testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t) - defer cleanupFn() - - cloneRepo, cloneRepoPath := copyRepoWithNewRemote(t, testRepo, locator, "my-remote") - defer func() { - require.NoError(t, os.RemoveAll(cloneRepoPath)) - }() - - // Ensure there's a new tag to fetch - testhelper.CreateTag(t, testRepoPath, "testtag", "master", nil) - - // On the first run, TagsChanged will be true for both implementations - req := &gitalypb.FetchRemoteRequest{Repository: cloneRepo, Remote: "my-remote", Timeout: 120, CheckTagsChanged: true} - resp, err := client.FetchRemote(ctx, req) - require.NoError(t, err) - require.NotNil(t, resp) - require.Equal(t, resp.TagsChanged, true) - - // On the second run, TagsChanged will be false for the Go implementation only - resp, err = client.FetchRemote(ctx, req) - require.NoError(t, err) - require.NotNil(t, resp) - require.Equal(t, resp.TagsChanged, !isFeatureEnabled(ctx, featureflag.GoFetchRemote)) - - // Finally, ensure that it returns true if we're asked not to check - req.CheckTagsChanged = false - resp, err = client.FetchRemote(ctx, req) - require.NoError(t, err) - require.NotNil(t, resp) - require.Equal(t, resp.TagsChanged, true) - }) + ctx, cancel := testhelper.Context() + defer cancel() + + testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t) + defer cleanupFn() + + cloneRepo, cloneRepoPath := copyRepoWithNewRemote(t, testRepo, locator, "my-remote") + defer func() { + require.NoError(t, os.RemoveAll(cloneRepoPath)) + }() + + // Ensure there's a new tag to fetch + testhelper.CreateTag(t, testRepoPath, "testtag", "master", nil) + + req := &gitalypb.FetchRemoteRequest{Repository: cloneRepo, Remote: "my-remote", Timeout: 120, CheckTagsChanged: true} + resp, err := client.FetchRemote(ctx, req) + require.NoError(t, err) + require.NotNil(t, resp) + require.Equal(t, resp.TagsChanged, true) + + // Ensure that it returns true if we're asked not to check + req.CheckTagsChanged = false + resp, err = client.FetchRemote(ctx, req) + require.NoError(t, err) + require.NotNil(t, resp) + require.Equal(t, resp.TagsChanged, true) } func TestFetchRemoteFailure(t *testing.T) { @@ -98,139 +87,117 @@ func TestFetchRemoteFailure(t *testing.T) { client, conn := newRepositoryClient(t, serverSocketPath) defer conn.Close() - testhelper.NewFeatureSets([]featureflag.FeatureFlag{ - featureflag.GoFetchRemote, - }).Run(t, func(t *testing.T, ctx context.Context) { - tests := []struct { - desc string - req *gitalypb.FetchRemoteRequest - goCode codes.Code - goErrMsg string - rubyCode codes.Code - rubyErrMsg string - }{ - { - desc: "no repository", - req: &gitalypb.FetchRemoteRequest{ - Repository: nil, - Remote: remoteName, - Timeout: 1000, - }, - goCode: codes.InvalidArgument, - goErrMsg: "empty Repository", - rubyCode: codes.InvalidArgument, - rubyErrMsg: "", + ctx, cancel := testhelper.Context() + defer cancel() + + tests := []struct { + desc string + req *gitalypb.FetchRemoteRequest + code codes.Code + errMsg string + }{ + { + desc: "no repository", + req: &gitalypb.FetchRemoteRequest{ + Repository: nil, + Remote: remoteName, + Timeout: 1000, }, - { - desc: "invalid storage", - req: &gitalypb.FetchRemoteRequest{ - Repository: &gitalypb.Repository{ - StorageName: "invalid", - RelativePath: "foobar.git", - }, - Remote: remoteName, - Timeout: 1000, + code: codes.InvalidArgument, + errMsg: "empty Repository", + }, + { + desc: "invalid storage", + req: &gitalypb.FetchRemoteRequest{ + Repository: &gitalypb.Repository{ + StorageName: "invalid", + RelativePath: "foobar.git", }, - // the error text is shortened to only a single word as requests to gitaly done via praefect returns different error messages - goCode: codes.InvalidArgument, - goErrMsg: "invalid", - rubyCode: codes.InvalidArgument, - rubyErrMsg: "invalid", + Remote: remoteName, + Timeout: 1000, }, - { - desc: "invalid remote", - req: &gitalypb.FetchRemoteRequest{ - Repository: repo, - Remote: "", - Timeout: 1000, - }, - goCode: codes.InvalidArgument, - goErrMsg: `blank or empty "remote"`, - rubyCode: codes.Unknown, - rubyErrMsg: `fatal: no path specified`, + // the error text is shortened to only a single word as requests to gitaly done via praefect returns different error messages + code: codes.InvalidArgument, + errMsg: "invalid", + }, + { + desc: "invalid remote", + req: &gitalypb.FetchRemoteRequest{ + Repository: repo, + Remote: "", + Timeout: 1000, }, - { - desc: "invalid remote url: bad format", - req: &gitalypb.FetchRemoteRequest{ - Repository: repo, - RemoteParams: &gitalypb.Remote{ - Url: "not a url", - Name: remoteName, - HttpAuthorizationHeader: httpToken, - }, - Timeout: 1000, + code: codes.InvalidArgument, + errMsg: `blank or empty "remote"`, + }, + { + desc: "invalid remote url: bad format", + req: &gitalypb.FetchRemoteRequest{ + Repository: repo, + RemoteParams: &gitalypb.Remote{ + Url: "not a url", + Name: remoteName, + HttpAuthorizationHeader: httpToken, }, - goCode: codes.InvalidArgument, - goErrMsg: `invalid "remote_params.url"`, - rubyCode: codes.InvalidArgument, - rubyErrMsg: "invalid remote url", + Timeout: 1000, }, - { - desc: "invalid remote url: no host", - req: &gitalypb.FetchRemoteRequest{ - Repository: repo, - RemoteParams: &gitalypb.Remote{ - Url: "/not/a/url", - Name: remoteName, - HttpAuthorizationHeader: httpToken, - }, - Timeout: 1000, + code: codes.InvalidArgument, + errMsg: `invalid "remote_params.url"`, + }, + { + desc: "invalid remote url: no host", + req: &gitalypb.FetchRemoteRequest{ + Repository: repo, + RemoteParams: &gitalypb.Remote{ + Url: "/not/a/url", + Name: remoteName, + HttpAuthorizationHeader: httpToken, }, - goCode: codes.InvalidArgument, - goErrMsg: `invalid "remote_params.url"`, - rubyCode: codes.Unknown, - rubyErrMsg: "fatal: '/not/a/url' does not appear to be a git repository", + Timeout: 1000, }, - { - desc: "no name", - req: &gitalypb.FetchRemoteRequest{ - Repository: repo, - RemoteParams: &gitalypb.Remote{ - Name: "", - Url: url, - HttpAuthorizationHeader: httpToken, - }, - Timeout: 1000, + code: codes.InvalidArgument, + errMsg: `invalid "remote_params.url"`, + }, + { + desc: "no name", + req: &gitalypb.FetchRemoteRequest{ + Repository: repo, + RemoteParams: &gitalypb.Remote{ + Name: "", + Url: url, + HttpAuthorizationHeader: httpToken, }, - goCode: codes.InvalidArgument, - goErrMsg: `blank or empty "remote_params.name"`, - rubyCode: codes.Unknown, - rubyErrMsg: "Rugged::ConfigError: '' is not a valid remote name", + Timeout: 1000, }, - { - desc: "not existing repo via http", - req: &gitalypb.FetchRemoteRequest{ - Repository: repo, - RemoteParams: &gitalypb.Remote{ - Url: httpSrv.URL + "/invalid/repo/path.git", - Name: remoteName, - HttpAuthorizationHeader: httpToken, - MirrorRefmaps: []string{"all_refs"}, - }, - Timeout: 1000, + code: codes.InvalidArgument, + errMsg: `blank or empty "remote_params.name"`, + }, + { + desc: "not existing repo via http", + req: &gitalypb.FetchRemoteRequest{ + Repository: repo, + RemoteParams: &gitalypb.Remote{ + Url: httpSrv.URL + "/invalid/repo/path.git", + Name: remoteName, + HttpAuthorizationHeader: httpToken, + MirrorRefmaps: []string{"all_refs"}, }, - goCode: codes.Unknown, - goErrMsg: "invalid/repo/path.git/' not found", - rubyCode: codes.Unknown, - rubyErrMsg: "/invalid/repo/path.git/' not found", + Timeout: 1000, }, - } - for _, tc := range tests { - t.Run(tc.desc, func(t *testing.T) { - resp, err := client.FetchRemote(ctx, tc.req) - require.Error(t, err) - require.Nil(t, resp) - - if isFeatureEnabled(ctx, featureflag.GoFetchRemote) { - require.Contains(t, err.Error(), tc.goErrMsg) - testhelper.RequireGrpcError(t, err, tc.goCode) - } else { - require.Contains(t, err.Error(), tc.rubyErrMsg) - testhelper.RequireGrpcError(t, err, tc.rubyCode) - } - }) - } - }) + code: codes.Unknown, + errMsg: "invalid/repo/path.git/' not found", + }, + } + for _, tc := range tests { + t.Run(tc.desc, func(t *testing.T) { + resp, err := client.FetchRemote(ctx, tc.req) + require.Error(t, err) + require.Nil(t, resp) + + require.Contains(t, err.Error(), tc.errMsg) + testhelper.RequireGrpcError(t, err, tc.code) + }) + } } const ( @@ -275,59 +242,58 @@ func TestFetchRemoteOverHTTP(t *testing.T) { client, conn := newRepositoryClient(t, serverSocketPath) defer conn.Close() - testhelper.NewFeatureSets([]featureflag.FeatureFlag{ - featureflag.GoFetchRemote, - }).Run(t, func(t *testing.T, ctx context.Context) { - testCases := []struct { - description string - httpToken string - remoteURL string - }{ - { - description: "with http token", - httpToken: httpToken, - }, - { - description: "without http token", - httpToken: "", - }, - } - - for _, tc := range testCases { - t.Run(tc.description, func(t *testing.T) { - forkedRepo, forkedRepoPath, forkedRepoCleanup := testhelper.NewTestRepo(t) - defer forkedRepoCleanup() - - s, remoteURL := remoteHTTPServer(t, "my-repo", tc.httpToken) - defer s.Close() - - req := &gitalypb.FetchRemoteRequest{ - Repository: forkedRepo, - RemoteParams: &gitalypb.Remote{ - Url: remoteURL, - Name: "geo", - HttpAuthorizationHeader: tc.httpToken, - MirrorRefmaps: []string{"all_refs"}, - }, - Timeout: 1000, - } - if tc.remoteURL != "" { - req.RemoteParams.Url = s.URL + tc.remoteURL - } - - refs := getRefnames(t, forkedRepoPath) - require.True(t, len(refs) > 1, "the advertisement.txt should have deleted all refs except for master") - - _, err := client.FetchRemote(ctx, req) - require.NoError(t, err) - - refs = getRefnames(t, forkedRepoPath) - - require.Len(t, refs, 1) - assert.Equal(t, "master", refs[0]) - }) - } - }) + ctx, cancel := testhelper.Context() + defer cancel() + + testCases := []struct { + description string + httpToken string + remoteURL string + }{ + { + description: "with http token", + httpToken: httpToken, + }, + { + description: "without http token", + httpToken: "", + }, + } + + for _, tc := range testCases { + t.Run(tc.description, func(t *testing.T) { + forkedRepo, forkedRepoPath, forkedRepoCleanup := testhelper.NewTestRepo(t) + defer forkedRepoCleanup() + + s, remoteURL := remoteHTTPServer(t, "my-repo", tc.httpToken) + defer s.Close() + + req := &gitalypb.FetchRemoteRequest{ + Repository: forkedRepo, + RemoteParams: &gitalypb.Remote{ + Url: remoteURL, + Name: "geo", + HttpAuthorizationHeader: tc.httpToken, + MirrorRefmaps: []string{"all_refs"}, + }, + Timeout: 1000, + } + if tc.remoteURL != "" { + req.RemoteParams.Url = s.URL + tc.remoteURL + } + + refs := getRefnames(t, forkedRepoPath) + require.True(t, len(refs) > 1, "the advertisement.txt should have deleted all refs except for master") + + _, err := client.FetchRemote(ctx, req) + require.NoError(t, err) + + refs = getRefnames(t, forkedRepoPath) + + require.Len(t, refs, 1) + assert.Equal(t, "master", refs[0]) + }) + } } func TestFetchRemoteOverHTTPWithRedirect(t *testing.T) { @@ -345,22 +311,21 @@ func TestFetchRemoteOverHTTPWithRedirect(t *testing.T) { ) defer s.Close() - testhelper.NewFeatureSets([]featureflag.FeatureFlag{ - featureflag.GoFetchRemote, - }).Run(t, func(t *testing.T, ctx context.Context) { - testRepo, _, cleanup := testhelper.NewTestRepo(t) - defer cleanup() - - req := &gitalypb.FetchRemoteRequest{ - Repository: testRepo, - RemoteParams: &gitalypb.Remote{Url: s.URL, Name: "geo"}, - Timeout: 1000, - } - - _, err := client.FetchRemote(ctx, req) - require.Error(t, err) - require.Contains(t, err.Error(), "The requested URL returned error: 303") - }) + ctx, cancel := testhelper.Context() + defer cancel() + + testRepo, _, cleanup := testhelper.NewTestRepo(t) + defer cleanup() + + req := &gitalypb.FetchRemoteRequest{ + Repository: testRepo, + RemoteParams: &gitalypb.Remote{Url: s.URL, Name: "geo"}, + Timeout: 1000, + } + + _, err := client.FetchRemote(ctx, req) + require.Error(t, err) + require.Contains(t, err.Error(), "The requested URL returned error: 303") } func TestFetchRemoteOverHTTPWithTimeout(t *testing.T) { @@ -379,30 +344,20 @@ func TestFetchRemoteOverHTTPWithTimeout(t *testing.T) { ) defer s.Close() - testhelper.NewFeatureSets([]featureflag.FeatureFlag{ - featureflag.GoFetchRemote, - }).Run(t, func(t *testing.T, ctx context.Context) { - testRepo, _, cleanup := testhelper.NewTestRepo(t) - defer cleanup() - - req := &gitalypb.FetchRemoteRequest{ - Repository: testRepo, - RemoteParams: &gitalypb.Remote{Url: s.URL, Name: "geo"}, - Timeout: 1, - } - - _, err := client.FetchRemote(ctx, req) - require.Error(t, err) - - if isFeatureEnabled(ctx, featureflag.GoFetchRemote) { - require.Contains(t, err.Error(), "fetch remote: signal: terminated") - } else { - require.Contains(t, err.Error(), "failed: Timed out") - } - }) -} + ctx, cancel := testhelper.Context() + defer cancel() + + testRepo, _, cleanup := testhelper.NewTestRepo(t) + defer cleanup() + + req := &gitalypb.FetchRemoteRequest{ + Repository: testRepo, + RemoteParams: &gitalypb.Remote{Url: s.URL, Name: "geo"}, + Timeout: 1, + } + + _, err := client.FetchRemote(ctx, req) + require.Error(t, err) -func isFeatureEnabled(ctx context.Context, flag featureflag.FeatureFlag) bool { - md, _ := metadata.FromOutgoingContext(ctx) - return featureflag.IsEnabled(metadata.NewIncomingContext(ctx, md), flag) + require.Contains(t, err.Error(), "fetch remote: signal: terminated") } diff --git a/internal/gitaly/service/repository/fork.go b/internal/gitaly/service/repository/fork.go index 721b07c2f..46c96b7fb 100644 --- a/internal/gitaly/service/repository/fork.go +++ b/internal/gitaly/service/repository/fork.go @@ -48,7 +48,7 @@ func (s *server) CreateFork(ctx context.Context, req *gitalypb.CreateForkRequest return nil, status.Errorf(codes.Internal, "CreateFork: create dest dir: %v", err) } - env, err := gitalyssh.UploadPackEnv(ctx, &gitalypb.SSHUploadPackRequest{Repository: sourceRepository}) + env, err := gitalyssh.UploadPackEnv(ctx, s.cfg, &gitalypb.SSHUploadPackRequest{Repository: sourceRepository}) if err != nil { return nil, err } diff --git a/internal/gitalyssh/gitalyssh.go b/internal/gitalyssh/gitalyssh.go index 4b57f6168..26ccaad9f 100644 --- a/internal/gitalyssh/gitalyssh.go +++ b/internal/gitalyssh/gitalyssh.go @@ -30,15 +30,16 @@ var ( envInjector = tracing.NewEnvInjector() ) -func UploadPackEnv(ctx context.Context, req *gitalypb.SSHUploadPackRequest) ([]string, error) { - env, err := commandEnv(ctx, req.Repository.StorageName, "upload-pack", req) +// UploadPackEnv returns a list of the key=val pairs required to set proper configuration options for upload-pack command. +func UploadPackEnv(ctx context.Context, cfg config.Cfg, req *gitalypb.SSHUploadPackRequest) ([]string, error) { + env, err := commandEnv(ctx, cfg, req.Repository.StorageName, "upload-pack", req) if err != nil { return nil, err } return envInjector(ctx, env), nil } -func commandEnv(ctx context.Context, storageName, command string, message proto.Message) ([]string, error) { +func commandEnv(ctx context.Context, cfg config.Cfg, storageName, command string, message proto.Message) ([]string, error) { var pbMarshaler jsonpb.Marshaler payload, err := pbMarshaler.MarshalToString(message) if err != nil { @@ -63,7 +64,7 @@ func commandEnv(ctx context.Context, storageName, command string, message proto. return []string{ fmt.Sprintf("GITALY_PAYLOAD=%s", payload), - fmt.Sprintf("GIT_SSH_COMMAND=%s %s", gitalySSHPath(), command), + fmt.Sprintf("GIT_SSH_COMMAND=%s %s", filepath.Join(cfg.BinDir, "gitaly-ssh"), command), fmt.Sprintf("GITALY_ADDRESS=%s", storageInfo.Address), fmt.Sprintf("GITALY_TOKEN=%s", storageInfo.Token), fmt.Sprintf("GITALY_FEATUREFLAGS=%s", strings.Join(featureFlagPairs, ",")), @@ -76,7 +77,3 @@ func commandEnv(ctx context.Context, storageName, command string, message proto. fmt.Sprintf("%s=%s", gitaly_x509.SSLCertFile, os.Getenv(gitaly_x509.SSLCertFile)), }, nil } - -func gitalySSHPath() string { - return filepath.Join(config.Config.BinDir, "gitaly-ssh") -} diff --git a/internal/gitalyssh/gitalyssh_test.go b/internal/gitalyssh/gitalyssh_test.go index 7c7fe9dc9..17173bdaa 100644 --- a/internal/gitalyssh/gitalyssh_test.go +++ b/internal/gitalyssh/gitalyssh_test.go @@ -3,7 +3,6 @@ package gitalyssh import ( "encoding/base64" "fmt" - "path/filepath" "testing" "github.com/golang/protobuf/jsonpb" @@ -34,11 +33,11 @@ func TestUploadPackEnv(t *testing.T) { expectedPayload, err := pbMarshaler.MarshalToString(&req) require.NoError(t, err) - env, err := UploadPackEnv(ctx, &req) + env, err := UploadPackEnv(ctx, config.Cfg{BinDir: "/path/bin"}, &req) require.NoError(t, err) require.Subset(t, env, []string{ - fmt.Sprintf("GIT_SSH_COMMAND=%s upload-pack", filepath.Join(config.Config.BinDir, "gitaly-ssh")), + "GIT_SSH_COMMAND=/path/bin/gitaly-ssh upload-pack", fmt.Sprintf("GITALY_PAYLOAD=%s", expectedPayload), "CORRELATION_ID=correlation-id-1", "GIT_SSH_VARIANT=simple", diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go index 18a987a6b..3b002ad35 100644 --- a/internal/metadata/featureflag/feature_flags.go +++ b/internal/metadata/featureflag/feature_flags.go @@ -117,7 +117,6 @@ var All = []FeatureFlag{ GoUserSquash, GoUserCommitFiles, GoResolveConflicts, - GoFetchRemote, GoUserDeleteTag, GoUserCreateTag, GoUserRevert, diff --git a/internal/praefect/replicator_test.go b/internal/praefect/replicator_test.go index 121a2142e..6bab2c2a4 100644 --- a/internal/praefect/replicator_test.go +++ b/internal/praefect/replicator_test.go @@ -1034,7 +1034,7 @@ func newReplicationService(tb testing.TB) (*grpc.Server, string) { locator := gitaly_config.NewLocator(gitaly_config.Config) gitalypb.RegisterRepositoryServiceServer(svr, repository.NewServer(gitaly_config.Config, RubyServer, locator)) gitalypb.RegisterObjectPoolServiceServer(svr, objectpoolservice.NewServer(gitaly_config.Config, locator)) - gitalypb.RegisterRemoteServiceServer(svr, remote.NewServer(RubyServer, locator)) + gitalypb.RegisterRemoteServiceServer(svr, remote.NewServer(gitaly_config.Config, RubyServer, locator)) gitalypb.RegisterSSHServiceServer(svr, ssh.NewServer(locator)) gitalypb.RegisterRefServiceServer(svr, ref.NewServer(locator)) healthpb.RegisterHealthServer(svr, health.NewServer()) diff --git a/ruby/lib/gitaly_server/repository_service.rb b/ruby/lib/gitaly_server/repository_service.rb index 80f9098d1..9707862b6 100644 --- a/ruby/lib/gitaly_server/repository_service.rb +++ b/ruby/lib/gitaly_server/repository_service.rb @@ -12,47 +12,6 @@ module GitalyServer Gitaly::FetchSourceBranchResponse.new(result: result) end - def fetch_remote(request, call) - gitlab_projects = Gitlab::Git::GitlabProjects.from_gitaly(request.repository, call) - - remote_name = request.remote - - repository = nil - - if request.remote_params - params = request.remote_params - repository = Gitlab::Git::Repository.from_gitaly(request.repository, call) - - mirror_refmaps = parse_refmaps(params.mirror_refmaps) - - repository.add_remote(params.name, params.url, mirror_refmap: mirror_refmaps) - remote_name = params.name - end - - success = Gitlab::Git::SshAuth.from_gitaly(request).setup do |env| - Gitlab::Git::HttpAuth.from_gitaly(request, call) do - gitlab_projects.fetch_remote( - remote_name, - request.timeout, - force: request.force, - tags: !request.no_tags, - env: env - ) - end - end - - raise GRPC::Unknown.new("Fetching remote #{request.remote} failed: #{gitlab_projects.output}") unless success - - # This implementation is being replaced with a Go implementation, which - # correctly implements the tags_changed parameter. Pretending the tags - # have always changed here retains the semantics of this implementation, - # which will be removed once the rollout is complete: - # https://gitlab.com/gitlab-org/gitaly/-/issues/3307 - Gitaly::FetchRemoteResponse.new(tags_changed: true) - ensure - repository&.remove_remote(remote_name) - end - def set_config(request, call) repo = Gitlab::Git::Repository.from_gitaly(request.repository, call) diff --git a/ruby/lib/gitlab/git/gitlab_projects.rb b/ruby/lib/gitlab/git/gitlab_projects.rb index a23cdfa91..9c11e4815 100644 --- a/ruby/lib/gitlab/git/gitlab_projects.rb +++ b/ruby/lib/gitlab/git/gitlab_projects.rb @@ -59,15 +59,6 @@ module Gitlab end end - def fetch_remote(name, timeout, force:, tags:, env: {}, prune: true) - logger.info "Fetching remote #{name} for repository #{repository_absolute_path}." - cmd = fetch_remote_command(name, tags, prune, force) - - run_with_timeout(cmd, timeout, repository_absolute_path, env).tap do |success| - logger.error "Fetching remote #{name} for repository #{repository_absolute_path} failed." unless success - end - end - def push_branches(remote_name, timeout, force, branch_names, env: {}) branch_names.each_slice(BRANCHES_PER_PUSH) do |branches| logger.info "Pushing #{branches.count} branches from #{repository_absolute_path} to remote #{remote_name}" @@ -125,16 +116,6 @@ module Gitlab cmd = %W(#{Gitlab.config.git.bin_path} remote rm origin) run(cmd, repository_absolute_path) end - - private - - def fetch_remote_command(name, tags, prune, force) - %W(#{Gitlab.config.git.bin_path} -c http.followRedirects=false fetch #{name} --quiet).tap do |cmd| - cmd << '--prune' if prune - cmd << '--force' if force - cmd << (tags ? '--tags' : '--no-tags') - end - end end end end diff --git a/ruby/lib/gitlab/git/http_auth.rb b/ruby/lib/gitlab/git/http_auth.rb deleted file mode 100644 index de4e7287e..000000000 --- a/ruby/lib/gitlab/git/http_auth.rb +++ /dev/null @@ -1,33 +0,0 @@ -module Gitlab - module Git - class HttpAuth - def self.from_gitaly(request, call) - params = request.remote_params - - return yield unless params.present? - return yield unless params.http_authorization_header.present? - - repo = Gitlab::Git::Repository.from_gitaly(request.repository, call) - - validate_remote_params(params) - - key = "http.#{params.url}.extraHeader" - repo.rugged.config[key] = "Authorization: #{params.http_authorization_header}" - - begin - yield - ensure - repo.rugged.config.delete(key) - end - end - - def self.validate_remote_params(remote_params) - begin - URI.parse(remote_params.url) - rescue URI::Error - raise GRPC::InvalidArgument, 'invalid remote url' - end - end - end - end -end diff --git a/ruby/spec/gitaly/repository_service_spec.rb b/ruby/spec/gitaly/repository_service_spec.rb index e4c9bcf95..5c41e9780 100644 --- a/ruby/spec/gitaly/repository_service_spec.rb +++ b/ruby/spec/gitaly/repository_service_spec.rb @@ -19,45 +19,4 @@ describe Gitaly::RepositoryService do expect(response.exists).to eq(true) end end - - describe 'FetchRemote' do - let(:call) { double(metadata: { 'gitaly-storage-path' => '/path/to/storage' }) } - let(:repo) { gitaly_repo('default', 'foobar.git') } - let(:remote) { 'my-remote' } - - let(:gl_projects) { double('Gitlab::Git::GitlabProjects') } - - before do - allow(Gitlab::Git::GitlabProjects).to receive(:from_gitaly).and_return(gl_projects) - end - - context 'request does not have ssh_key and known_hosts set' do - it 'calls GitlabProjects#fetch_remote with an empty environment' do - request = Gitaly::FetchRemoteRequest.new(repository: repo, remote: remote) - - expect(gl_projects).to receive(:fetch_remote) - .with(remote, 0, force: false, tags: true, env: {}) - .and_return(true) - - GitalyServer::RepositoryService.new.fetch_remote(request, call) - end - end - - context 'request has ssh_key and known_hosts set' do - it 'calls GitlabProjects#fetch_remote with a custom GIT_SSH_COMMAND' do - request = Gitaly::FetchRemoteRequest.new( - repository: repo, - remote: remote, - ssh_key: 'SSH KEY', - known_hosts: 'KNOWN HOSTS' - ) - - expect(gl_projects).to receive(:fetch_remote) - .with(remote, 0, force: false, tags: true, env: { 'GIT_SSH_COMMAND' => /ssh/ }) - .and_return(true) - - GitalyServer::RepositoryService.new.fetch_remote(request, call) - end - end - end end diff --git a/ruby/spec/lib/gitlab/git/gitlab_projects_spec.rb b/ruby/spec/lib/gitlab/git/gitlab_projects_spec.rb index cd1a6c2e4..ac1ae4d67 100644 --- a/ruby/spec/lib/gitlab/git/gitlab_projects_spec.rb +++ b/ruby/spec/lib/gitlab/git/gitlab_projects_spec.rb @@ -95,67 +95,6 @@ describe Gitlab::Git::GitlabProjects do end end - describe '#fetch_remote' do - let(:remote_name) { 'remote-name' } - let(:branch_name) { 'master' } - let(:force) { false } - let(:tags) { true } - let(:env) { { 'GIT_SSH_COMMAND' => 'foo-command bar' } } - let(:prune) { true } - let(:follow_redirects) { false } - let(:args) { { force: force, tags: tags, env: env, prune: prune } } - let(:cmd) { %W(#{Gitlab.config.git.bin_path} -c http.followRedirects=false fetch #{remote_name} --quiet --prune --tags) } - - subject { gl_projects.fetch_remote(remote_name, 600, **args) } - - context 'with default args' do - it 'executes the command' do - stub_spawn(cmd, 600, tmp_repo_path, env, success: true) - - is_expected.to be_truthy - end - - it 'returns false if the command fails' do - stub_spawn(cmd, 600, tmp_repo_path, env, success: false) - - is_expected.to be_falsy - end - end - - context 'with --force' do - let(:force) { true } - let(:cmd) { %W(#{Gitlab.config.git.bin_path} -c http.followRedirects=false fetch #{remote_name} --quiet --prune --force --tags) } - - it 'executes the command with forced option' do - stub_spawn(cmd, 600, tmp_repo_path, env, success: true) - - is_expected.to be_truthy - end - end - - context 'with --no-tags' do - let(:tags) { false } - let(:cmd) { %W(#{Gitlab.config.git.bin_path} -c http.followRedirects=false fetch #{remote_name} --quiet --prune --no-tags) } - - it 'executes the command' do - stub_spawn(cmd, 600, tmp_repo_path, env, success: true) - - is_expected.to be_truthy - end - end - - context 'with no prune' do - let(:prune) { false } - let(:cmd) { %W(#{Gitlab.config.git.bin_path} -c http.followRedirects=false fetch #{remote_name} --quiet --tags) } - - it 'executes the command' do - stub_spawn(cmd, 600, tmp_repo_path, env, success: true) - - is_expected.to be_truthy - end - end - end - describe '#delete_remote_branches' do let(:remote_name) { 'remote-name' } let(:branch_names) { 20.times.map { |i| "branch#{i}" } } |