diff options
author | Patrick Steinhardt <psteinhardt@gitlab.com> | 2021-09-22 09:24:40 +0300 |
---|---|---|
committer | Patrick Steinhardt <psteinhardt@gitlab.com> | 2021-09-22 09:24:40 +0300 |
commit | b7178c04005efe8e9350057a44a8ae9aba813689 (patch) | |
tree | d8f257b91ab5594bf3ce34aca30ae58c5e2b1912 /internal | |
parent | f308b90f8eb1ccfdafdbff49639d3487b8a14387 (diff) | |
parent | 4a0061039867b4e8747828aa27a0ca7f3ed0daa2 (diff) |
Merge branch 'pks-ff-drop-tx-file-locking' into 'master'
repository: Always lock gitconfig and gitattributes on write
Closes #3764
See merge request gitlab-org/gitaly!3891
Diffstat (limited to 'internal')
-rw-r--r-- | internal/git/housekeeping/housekeeping_test.go | 6 | ||||
-rw-r--r-- | internal/git/localrepo/config.go | 31 | ||||
-rw-r--r-- | internal/git/localrepo/config_test.go | 60 | ||||
-rw-r--r-- | internal/git/localrepo/remote_extra_test.go | 2 | ||||
-rw-r--r-- | internal/gitaly/service/repository/apply_gitattributes.go | 54 | ||||
-rw-r--r-- | internal/gitaly/service/repository/apply_gitattributes_test.go | 57 | ||||
-rw-r--r-- | internal/gitaly/service/repository/config.go | 40 | ||||
-rw-r--r-- | internal/gitaly/service/repository/fullpath.go | 21 | ||||
-rw-r--r-- | internal/gitaly/service/repository/fullpath_test.go | 15 | ||||
-rw-r--r-- | internal/metadata/featureflag/ff_tx_file_locking.go | 5 |
10 files changed, 34 insertions, 257 deletions
diff --git a/internal/git/housekeeping/housekeeping_test.go b/internal/git/housekeeping/housekeeping_test.go index 09b46599d..e62c5de6f 100644 --- a/internal/git/housekeeping/housekeeping_test.go +++ b/internal/git/housekeeping/housekeeping_test.go @@ -666,7 +666,7 @@ func TestPerformRepoDoesNotExist(t *testing.T) { } func TestPerform_UnsetConfiguration(t *testing.T) { - cfg, repoProto, _ := testcfg.BuildWithRepo(t) + cfg, repoProto, repoPath := testcfg.BuildWithRepo(t) repo := localrepo.NewTestRepo(t, cfg, repoProto) ctx, cancel := testhelper.Context() @@ -679,7 +679,7 @@ func TestPerform_UnsetConfiguration(t *testing.T) { "http.something.else": "untouched", "totally.unrelated": "untouched", } { - require.NoError(t, repo.Config().Set(ctx, key, value)) + gittest.Exec(t, cfg, "-C", repoPath, "config", key, value) } opts, err := repo.Config().GetRegexp(ctx, ".*", git.ConfigGetRegexpOpts{}) @@ -711,7 +711,7 @@ func testPerformUnsetConfigurationTransactional(t *testing.T, ctx context.Contex repoProto, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0]) repo := localrepo.NewTestRepo(t, cfg, repoProto) - require.NoError(t, repo.Config().Set(ctx, "http.some.extraHeader", "value")) + gittest.Exec(t, cfg, "-C", repoPath, "config", "http.some.extraHeader", "value") votes := 0 txManager := &transaction.MockManager{ diff --git a/internal/git/localrepo/config.go b/internal/git/localrepo/config.go index 97f4de2e4..49faa6cda 100644 --- a/internal/git/localrepo/config.go +++ b/internal/git/localrepo/config.go @@ -21,37 +21,6 @@ type Config struct { repo *Repo } -// Set will set a configuration value. Any preexisting values will be overwritten with the new -// value. -func (cfg Config) Set(ctx context.Context, name, value string) error { - if err := validateNotBlank(name, "name"); err != nil { - return err - } - - if err := cfg.repo.ExecAndWait(ctx, git.SubCmd{ - Name: "config", - Flags: []git.Option{ - git.Flag{Name: "--replace-all"}, - }, - Args: []string{name, value}, - }); err != nil { - // Please refer to https://git-scm.com/docs/git-config#_description - // on return codes. - switch { - case isExitWithCode(err, 1): - // section or key is invalid - return fmt.Errorf("%w: bad section or name", git.ErrInvalidArg) - case isExitWithCode(err, 2): - // no section or name was provided - return fmt.Errorf("%w: missing section or name", git.ErrInvalidArg) - } - - return err - } - - return nil -} - // GetRegexp gets all config entries which whose keys match the given regexp. func (cfg Config) GetRegexp(ctx context.Context, nameRegexp string, opts git.ConfigGetRegexpOpts) ([]git.ConfigPair, error) { if err := validateNotBlank(nameRegexp, "nameRegexp"); err != nil { diff --git a/internal/git/localrepo/config_test.go b/internal/git/localrepo/config_test.go index f09e525a5..15c440524 100644 --- a/internal/git/localrepo/config_test.go +++ b/internal/git/localrepo/config_test.go @@ -36,66 +36,6 @@ func setupRepoConfig(t *testing.T) (Config, string) { return repo.Config(), repoPath } -func TestConfig_Set(t *testing.T) { - ctx, cancel := testhelper.Context() - defer cancel() - - repoConfig, repoPath := setupRepoConfig(t) - - t.Run("setting a new value", func(t *testing.T) { - require.NoError(t, repoConfig.Set(ctx, "key.one", "1")) - - actual := text.ChompBytes(gittest.Exec(t, repoConfig.repo.cfg, "-C", repoPath, "config", "key.one")) - require.Equal(t, "1", actual) - }) - - t.Run("overwriting an old value", func(t *testing.T) { - require.NoError(t, repoConfig.Set(ctx, "key.two", "2")) - require.NoError(t, repoConfig.Set(ctx, "key.two", "3")) - - actual := text.ChompBytes(gittest.Exec(t, repoConfig.repo.cfg, "-C", repoPath, "config", "--get-all", "key.two")) - require.Equal(t, "3", actual) - }) - - t.Run("invalid argument", func(t *testing.T) { - for _, tc := range []struct { - desc string - name string - expErr error - expMsg string - }{ - { - desc: "empty name", - name: "", - expErr: git.ErrInvalidArg, - expMsg: `"name" is blank or empty`, - }, - { - desc: "invalid name", - name: "`.\n", - expErr: git.ErrInvalidArg, - expMsg: "bad section or name", - }, - { - desc: "no section or name", - name: "missing", - expErr: git.ErrInvalidArg, - expMsg: "missing section or name", - }, - } { - t.Run(tc.desc, func(t *testing.T) { - ctx, cancel := testhelper.Context() - defer cancel() - - err := repoConfig.Set(ctx, tc.name, "some") - require.Error(t, err) - require.True(t, errors.Is(err, tc.expErr), err.Error()) - require.Contains(t, err.Error(), tc.expMsg) - }) - } - }) -} - func TestBuildConfigGetRegexpOptsFlags(t *testing.T) { for _, tc := range []struct { desc string diff --git a/internal/git/localrepo/remote_extra_test.go b/internal/git/localrepo/remote_extra_test.go index f80be863b..8dc1a3cc5 100644 --- a/internal/git/localrepo/remote_extra_test.go +++ b/internal/git/localrepo/remote_extra_test.go @@ -60,7 +60,7 @@ func TestRepo_FetchInternal(t *testing.T) { repoProto, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0]) repo := localrepo.NewTestRepo(t, cfg, repoProto) - require.NoError(t, repo.Config().Set(ctx, "fetch.writeCommitGraph", "true")) + gittest.Exec(t, cfg, "-C", repoPath, "config", "fetch.writeCommitGraph", "true") require.NoError(t, repo.FetchInternal( ctx, remoteRepoProto, []string{"refs/heads/master:refs/heads/master"}, diff --git a/internal/gitaly/service/repository/apply_gitattributes.go b/internal/gitaly/service/repository/apply_gitattributes.go index 38b517837..00d3c2bf7 100644 --- a/internal/gitaly/service/repository/apply_gitattributes.go +++ b/internal/gitaly/service/repository/apply_gitattributes.go @@ -8,12 +8,10 @@ import ( "os" "path/filepath" - "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" "gitlab.com/gitlab-org/gitaly/v14/internal/git" "gitlab.com/gitlab-org/gitaly/v14/internal/git/catfile" "gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction" "gitlab.com/gitlab-org/gitaly/v14/internal/helper" - "gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/v14/internal/safe" "gitlab.com/gitlab-org/gitaly/v14/internal/transaction/txinfo" "gitlab.com/gitlab-org/gitaly/v14/internal/transaction/voting" @@ -65,57 +63,23 @@ func (s *server) applyGitattributes(ctx context.Context, c catfile.Batch, repoPa return err } - if featureflag.TxFileLocking.IsEnabled(ctx) { - writer, err := safe.NewLockingFileWriter(attributesPath, safe.LockingFileWriterConfig{ - FileWriterConfig: safe.FileWriterConfig{FileMode: attributesFileMode}, - }) - if err != nil { - return fmt.Errorf("creating gitattributes writer: %w", err) - } - defer writer.Close() - - if _, err := io.CopyN(writer, blobObj.Reader, blobInfo.Size); err != nil { - return err - } - - if err := transaction.CommitLockedFile(ctx, s.txManager, writer); err != nil { - return fmt.Errorf("committing gitattributes: %w", err) - } - - return nil - } - - tempFile, err := os.CreateTemp(infoPath, "attributes") + writer, err := safe.NewLockingFileWriter(attributesPath, safe.LockingFileWriterConfig{ + FileWriterConfig: safe.FileWriterConfig{FileMode: attributesFileMode}, + }) if err != nil { - return helper.ErrInternalf("creating temporary gitattributes file: %w", err) + return fmt.Errorf("creating gitattributes writer: %w", err) } - defer func() { - if err := os.Remove(tempFile.Name()); err != nil && !errors.Is(err, os.ErrNotExist) { - ctxlogrus.Extract(ctx).WithError(err).Errorf("failed to remove tmp file %q", tempFile.Name()) - } - }() + defer writer.Close() - // Write attributes to temp file - if _, err := io.CopyN(tempFile, blobObj.Reader, blobInfo.Size); err != nil { + if _, err := io.CopyN(writer, blobObj.Reader, blobInfo.Size); err != nil { return err } - if err := tempFile.Close(); err != nil { - return err + if err := transaction.CommitLockedFile(ctx, s.txManager, writer); err != nil { + return fmt.Errorf("committing gitattributes: %w", err) } - // Change the permission of tempFile as the permission of file attributesPath - if err := os.Chmod(tempFile.Name(), attributesFileMode); err != nil { - return err - } - - // Vote on the contents of the newly written gitattributes file. - if err := s.vote(ctx, blobInfo.Oid); err != nil { - return fmt.Errorf("could not commit gitattributes: %w", err) - } - - // Rename temp file and return the result - return os.Rename(tempFile.Name(), attributesPath) + return nil } func (s *server) vote(ctx context.Context, oid git.ObjectID) error { diff --git a/internal/gitaly/service/repository/apply_gitattributes_test.go b/internal/gitaly/service/repository/apply_gitattributes_test.go index b83c2fd09..f85b6f456 100644 --- a/internal/gitaly/service/repository/apply_gitattributes_test.go +++ b/internal/gitaly/service/repository/apply_gitattributes_test.go @@ -12,10 +12,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/v14/internal/backchannel" - "gitlab.com/gitlab-org/gitaly/v14/internal/git" "gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/service" "gitlab.com/gitlab-org/gitaly/v14/internal/metadata" - "gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/v14/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testassert" "gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testcfg" @@ -29,13 +27,11 @@ import ( ) func TestApplyGitattributesSuccess(t *testing.T) { - testhelper.NewFeatureSets([]featureflag.FeatureFlag{ - featureflag.TxFileLocking, - }).Run(t, testApplyGitattributesSuccess) -} - -func testApplyGitattributesSuccess(t *testing.T, ctx context.Context) { t.Parallel() + + ctx, cancel := testhelper.Context() + defer cancel() + cfg, repo, _, client := setupRepositoryService(t) infoPath := filepath.Join(cfg.Storages[0].Path, repo.GetRelativePath(), "info") @@ -93,13 +89,11 @@ func (s *testTransactionServer) VoteTransaction(ctx context.Context, in *gitalyp } func TestApplyGitattributesWithTransaction(t *testing.T) { - testhelper.NewFeatureSets([]featureflag.FeatureFlag{ - featureflag.TxFileLocking, - }).Run(t, testApplyGitattributesWithTransaction) -} - -func testApplyGitattributesWithTransaction(t *testing.T, ctx context.Context) { t.Parallel() + + ctx, cancel := testhelper.Context() + defer cancel() + cfg, repo, repoPath := testcfg.BuildWithRepo(t) transactionServer := &testTransactionServer{} @@ -138,16 +132,8 @@ func testApplyGitattributesWithTransaction(t *testing.T, ctx context.Context) { desc: "successful vote writes gitattributes", revision: []byte("e63f41fe459e62e1228fcef60d7189127aeba95a"), voteFn: func(t *testing.T, request *gitalypb.VoteTransactionRequest) (*gitalypb.VoteTransactionResponse, error) { - var expectedHash []byte - if featureflag.TxFileLocking.IsEnabled(ctx) { - vote := voting.VoteFromData([]byte("/custom-highlighting/*.gitlab-custom gitlab-language=ruby\n")) - expectedHash = vote.Bytes() - } else { - oid, err := git.NewObjectIDFromHex("36814a3da051159a1683479e7a1487120309db8f") - require.NoError(t, err) - expectedHash, err = oid.Bytes() - require.NoError(t, err) - } + vote := voting.VoteFromData([]byte("/custom-highlighting/*.gitlab-custom gitlab-language=ruby\n")) + expectedHash := vote.Bytes() require.Equal(t, expectedHash, request.ReferenceUpdatesHash) return &gitalypb.VoteTransactionResponse{ @@ -166,11 +152,7 @@ func testApplyGitattributesWithTransaction(t *testing.T, ctx context.Context) { }, shouldExist: false, expectedErr: func() error { - if featureflag.TxFileLocking.IsEnabled(ctx) { - return status.Error(codes.Unknown, "committing gitattributes: voting on locked file: preimage vote: transaction was aborted") - } - - return status.Error(codes.Unknown, "could not commit gitattributes: vote failed: transaction was aborted") + return status.Error(codes.Unknown, "committing gitattributes: voting on locked file: preimage vote: transaction was aborted") }(), }, { @@ -180,13 +162,8 @@ func testApplyGitattributesWithTransaction(t *testing.T, ctx context.Context) { return nil, errors.New("foobar") }, shouldExist: false, - expectedErr: func() error { - if featureflag.TxFileLocking.IsEnabled(ctx) { - return status.Error(codes.Unknown, "committing gitattributes: voting on locked file: preimage vote: rpc error: code = Unknown desc = foobar") - } - - return status.Error(codes.Unknown, "could not commit gitattributes: vote failed: rpc error: code = Unknown desc = foobar") + return status.Error(codes.Unknown, "committing gitattributes: voting on locked file: preimage vote: rpc error: code = Unknown desc = foobar") }(), }, { @@ -232,13 +209,11 @@ func testApplyGitattributesWithTransaction(t *testing.T, ctx context.Context) { } func TestApplyGitattributesFailure(t *testing.T) { - testhelper.NewFeatureSets([]featureflag.FeatureFlag{ - featureflag.TxFileLocking, - }).Run(t, testApplyGitattributesFailure) -} - -func testApplyGitattributesFailure(t *testing.T, ctx context.Context) { t.Parallel() + + ctx, cancel := testhelper.Context() + defer cancel() + _, repo, _, client := setupRepositoryService(t) tests := []struct { diff --git a/internal/gitaly/service/repository/config.go b/internal/gitaly/service/repository/config.go index c650bc65b..a0a7f420c 100644 --- a/internal/gitaly/service/repository/config.go +++ b/internal/gitaly/service/repository/config.go @@ -1,16 +1,11 @@ package repository import ( - "context" - "fmt" "io" "os" "path/filepath" - "gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction" "gitlab.com/gitlab-org/gitaly/v14/internal/helper" - "gitlab.com/gitlab-org/gitaly/v14/internal/transaction/txinfo" - "gitlab.com/gitlab-org/gitaly/v14/internal/transaction/voting" "gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/v14/streamio" "google.golang.org/grpc/codes" @@ -49,38 +44,3 @@ func (s *server) GetConfig( return nil } - -func (s *server) voteOnConfig(ctx context.Context, repo *gitalypb.Repository) error { - return transaction.RunOnContext(ctx, func(tx txinfo.Transaction) error { - repoPath, err := s.locator.GetPath(repo) - if err != nil { - return fmt.Errorf("get repo path: %w", err) - } - - var vote voting.Vote - - config, err := os.Open(filepath.Join(repoPath, "config")) - switch { - case err == nil: - hash := voting.NewVoteHash() - if _, err := io.Copy(hash, config); err != nil { - return fmt.Errorf("seeding vote: %w", err) - } - - vote, err = hash.Vote() - if err != nil { - return fmt.Errorf("computing vote: %w", err) - } - case os.IsNotExist(err): - vote = voting.VoteFromData([]byte("notfound")) - default: - return fmt.Errorf("open repo config: %w", err) - } - - if err := s.txManager.Vote(ctx, tx, vote); err != nil { - return fmt.Errorf("casting vote: %w", err) - } - - return nil - }) -} diff --git a/internal/gitaly/service/repository/fullpath.go b/internal/gitaly/service/repository/fullpath.go index 0fbe27878..d2fdbe86d 100644 --- a/internal/gitaly/service/repository/fullpath.go +++ b/internal/gitaly/service/repository/fullpath.go @@ -4,7 +4,6 @@ import ( "context" "gitlab.com/gitlab-org/gitaly/v14/internal/helper" - "gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb" ) @@ -26,24 +25,8 @@ func (s *server) SetFullPath( repo := s.localrepo(request.GetRepository()) - if featureflag.TxFileLocking.IsEnabled(ctx) { - if err := repo.SetConfig(ctx, fullPathKey, request.GetPath(), s.txManager); err != nil { - return nil, helper.ErrInternalf("setting config: %w", err) - } - - return &gitalypb.SetFullPathResponse{}, nil - } - - if err := s.voteOnConfig(ctx, request.GetRepository()); err != nil { - return nil, helper.ErrInternalf("preimage vote on config: %w", err) - } - - if err := repo.Config().Set(ctx, fullPathKey, request.GetPath()); err != nil { - return nil, helper.ErrInternalf("writing config: %w", err) - } - - if err := s.voteOnConfig(ctx, request.GetRepository()); err != nil { - return nil, helper.ErrInternalf("postimage vote on config: %w", err) + if err := repo.SetConfig(ctx, fullPathKey, request.GetPath(), s.txManager); err != nil { + return nil, helper.ErrInternalf("setting config: %w", err) } return &gitalypb.SetFullPathResponse{}, nil diff --git a/internal/gitaly/service/repository/fullpath_test.go b/internal/gitaly/service/repository/fullpath_test.go index 1224e30fd..dea6d131e 100644 --- a/internal/gitaly/service/repository/fullpath_test.go +++ b/internal/gitaly/service/repository/fullpath_test.go @@ -1,7 +1,6 @@ package repository import ( - "context" "fmt" "os" "path/filepath" @@ -12,7 +11,6 @@ import ( "gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/v14/internal/helper" "gitlab.com/gitlab-org/gitaly/v14/internal/helper/text" - "gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/v14/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testassert" "gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb" @@ -21,12 +19,9 @@ import ( func TestSetFullPath(t *testing.T) { t.Parallel() - testhelper.NewFeatureSets([]featureflag.FeatureFlag{ - featureflag.TxFileLocking, - }).Run(t, testSetFullPath) -} + ctx, cancel := testhelper.Context() + defer cancel() -func testSetFullPath(t *testing.T, ctx context.Context) { cfg, client := setupRepositoryServiceWithoutRepo(t) t.Run("missing repository", func(t *testing.T) { @@ -79,12 +74,8 @@ func testSetFullPath(t *testing.T, ctx context.Context) { require.Nil(t, response) - expectedErr := fmt.Sprintf("rpc error: code = NotFound desc = writing config: rpc "+ + expectedErr := fmt.Sprintf("rpc error: code = NotFound desc = setting config: rpc "+ "error: code = NotFound desc = GetRepoPath: not a git repository: %q", repoPath) - if featureflag.TxFileLocking.IsEnabled(ctx) { - expectedErr = fmt.Sprintf("rpc error: code = NotFound desc = setting config: rpc "+ - "error: code = NotFound desc = GetRepoPath: not a git repository: %q", repoPath) - } require.EqualError(t, err, expectedErr) }) diff --git a/internal/metadata/featureflag/ff_tx_file_locking.go b/internal/metadata/featureflag/ff_tx_file_locking.go deleted file mode 100644 index 08f1c9f6c..000000000 --- a/internal/metadata/featureflag/ff_tx_file_locking.go +++ /dev/null @@ -1,5 +0,0 @@ -package featureflag - -// TxFileLocking enables two-phase voting on files with proper locking semantics such that no races -// can exist anymore. -var TxFileLocking = NewFeatureFlag("tx_file_locking", false) |