diff options
author | Justin Tobler <jtobler@gitlab.com> | 2023-02-01 00:14:11 +0300 |
---|---|---|
committer | Justin Tobler <jtobler@gitlab.com> | 2023-02-17 13:51:28 +0300 |
commit | 0c28b4a7b1fccea5c05679fd46f28bbab96a3e79 (patch) | |
tree | 9a13d7524af7b764704b0a6fbfd196d08d08abf7 /internal | |
parent | 6858a88debb67d3d190898497cd098ca956982e8 (diff) |
repository: Generalize setting custom hooks
Currently the `SetCustomHooks` RPC is set up in such a way that requires
a tar archive to be provided as input. In a future commit the
`ReplicateRepository` RPC will be updated to replicate hooks and
leverage the logic from `SetCustomHooks` to set hooks for the
repository. Since some repositories may not contain any custom hooks,
the logic needs to be updated to support this additional use case.
This change updates `extractHooks()` to be able to support receiving an
empty `io.Reader` as input without producing an error. Since the version
of tar used at runtime is system dependent, the code is generalized to
support both GNU and BSD tar.
Diffstat (limited to 'internal')
-rw-r--r-- | internal/gitaly/service/repository/set_custom_hooks.go | 36 | ||||
-rw-r--r-- | internal/gitaly/service/repository/set_custom_hooks_test.go | 106 |
2 files changed, 134 insertions, 8 deletions
diff --git a/internal/gitaly/service/repository/set_custom_hooks.go b/internal/gitaly/service/repository/set_custom_hooks.go index 5e231a6f9..6c8d47444 100644 --- a/internal/gitaly/service/repository/set_custom_hooks.go +++ b/internal/gitaly/service/repository/set_custom_hooks.go @@ -1,6 +1,7 @@ package repository import ( + "bufio" "context" "encoding/binary" "errors" @@ -9,6 +10,7 @@ import ( "io/fs" "os" "path/filepath" + "strings" "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" "gitlab.com/gitlab-org/gitaly/v15/internal/command" @@ -34,12 +36,12 @@ func (s *server) SetCustomHooks(stream gitalypb.RepositoryService_SetCustomHooks firstRequest, err := stream.Recv() if err != nil { - return structerr.NewInternal("first request failed %w", err) + return structerr.NewInternal("getting first request: %w", err) } repo := firstRequest.GetRepository() if err := service.ValidateRepository(repo); err != nil { - return structerr.NewInvalidArgument("%w", err) + return structerr.NewInvalidArgument("validating repo: %w", err) } reader := streamio.NewReader(func() ([]byte, error) { @@ -68,12 +70,12 @@ func (s *server) RestoreCustomHooks(stream gitalypb.RepositoryService_RestoreCus firstRequest, err := stream.Recv() if err != nil { - return structerr.NewInternal("first request failed %w", err) + return structerr.NewInternal("getting first request: %w", err) } repo := firstRequest.GetRepository() if err := service.ValidateRepository(repo); err != nil { - return structerr.NewInvalidArgument("%w", err) + return structerr.NewInvalidArgument("validating repo: %w", err) } reader := streamio.NewReader(func() ([]byte, error) { @@ -97,7 +99,7 @@ func (s *server) RestoreCustomHooks(stream gitalypb.RepositoryService_RestoreCus func (s *server) setCustomHooks(ctx context.Context, reader io.Reader, repo repository.GitRepo) error { if featureflag.TransactionalRestoreCustomHooks.IsEnabled(ctx) { if err := s.setCustomHooksTransaction(ctx, reader, repo); err != nil { - return fmt.Errorf("writing custom hooks: %w", err) + return fmt.Errorf("setting custom hooks: %w", err) } return nil @@ -105,7 +107,7 @@ func (s *server) setCustomHooks(ctx context.Context, reader io.Reader, repo repo repoPath, err := s.locator.GetPath(repo) if err != nil { - return fmt.Errorf("getting repo path failed %w", err) + return fmt.Errorf("getting repo path: %w", err) } if err := extractHooks(ctx, reader, repoPath); err != nil { @@ -291,14 +293,34 @@ func voteCustomHooks( // extractHooks unpacks a tar file containing custom hooks into a `custom_hooks` // directory at the specified path. func extractHooks(ctx context.Context, reader io.Reader, path string) error { + // GNU tar does not accept an empty file as a valid tar archive and produces + // an error. Since an empty hooks tar is symbolic of a repository having no + // hooks, the reader is peeked to check if there is any data present. + buf := bufio.NewReader(reader) + if _, err := buf.Peek(1); err == io.EOF { + return nil + } + cmdArgs := []string{"-xf", "-", "-C", path, customHooksDir} - cmd, err := command.New(ctx, append([]string{"tar"}, cmdArgs...), command.WithStdin(reader)) + var stderrBuilder strings.Builder + cmd, err := command.New(ctx, append([]string{"tar"}, cmdArgs...), + command.WithStdin(buf), + command.WithStderr(&stderrBuilder)) if err != nil { return fmt.Errorf("executing tar command: %w", err) } if err := cmd.Wait(); err != nil { + // GNU and BSD tar versions have differing errors when attempting to + // extract specified members from a valid tar archive. If the tar + // archive is valid the errors for GNU and BSD tar should have the + // same prefix, which can be checked to validate whether the expected + // content is present in the archive for extraction. + if strings.HasPrefix(stderrBuilder.String(), "tar: custom_hooks: Not found in archive") { + return nil + } + return fmt.Errorf("waiting for tar command completion: %w", err) } diff --git a/internal/gitaly/service/repository/set_custom_hooks_test.go b/internal/gitaly/service/repository/set_custom_hooks_test.go index 6302f2fdb..040906765 100644 --- a/internal/gitaly/service/repository/set_custom_hooks_test.go +++ b/internal/gitaly/service/repository/set_custom_hooks_test.go @@ -5,6 +5,7 @@ package repository import ( "context" "io" + "io/fs" "os" "path/filepath" "syscall" @@ -192,7 +193,7 @@ func testFailedSetCustomHooksDueToValidations(t *testing.T, ctx context.Context) err := tc.streamSender(t, ctx, client) testhelper.RequireGrpcError(t, err, status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect( - "empty Repository", + "validating repo: empty Repository", "repo scoped: empty Repository", ))) }) @@ -398,3 +399,106 @@ func mustCreateCorruptHooksArchive(t *testing.T) string { return archivePath } + +func TestExtractHooks(t *testing.T) { + t.Parallel() + + ctx := testhelper.Context(t) + cfg := testcfg.Build(t) + + hooksFiles := []testFile{ + {name: "pre-commit.sample", content: "foo", mode: 0o755}, + {name: "pre-push.sample", content: "bar", mode: 0o755}, + } + + tmpDir := testhelper.TempDir(t) + emptyTar := filepath.Join(tmpDir, "empty_hooks.tar") + err := os.WriteFile(emptyTar, nil, 0o644) + require.NoError(t, err) + + for _, tc := range []struct { + desc string + path string + expectedFiles []testFile + expectedErr string + }{ + { + desc: "custom hooks tar", + path: mustCreateCustomHooksArchive(t, ctx, hooksFiles, customHooksDir), + expectedFiles: []testFile{ + {name: "custom_hooks"}, + {name: "custom_hooks/pre-commit.sample", content: "foo"}, + {name: "custom_hooks/pre-push.sample", content: "bar"}, + }, + }, + { + desc: "empty custom hooks tar", + path: mustCreateCustomHooksArchive(t, ctx, []testFile{}, customHooksDir), + expectedFiles: []testFile{{name: "custom_hooks"}}, + }, + { + desc: "no hooks tar", + path: mustCreateCustomHooksArchive(t, ctx, hooksFiles, "no_hooks"), + }, + { + desc: "corrupt tar", + path: mustCreateCorruptHooksArchive(t), + expectedErr: "waiting for tar command completion: exit status", + }, + { + desc: "empty tar", + path: emptyTar, + }, + } { + tc := tc + + t.Run(tc.desc, func(t *testing.T) { + t.Parallel() + + _, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{ + SkipCreationViaService: true, + }) + + tarball, err := os.Open(tc.path) + require.NoError(t, err) + defer tarball.Close() + + err = extractHooks(ctx, tarball, repoPath) + if tc.expectedErr == "" { + require.NoError(t, err) + } else { + require.ErrorContains(t, err, tc.expectedErr) + } + + if len(tc.expectedFiles) == 0 { + require.NoDirExists(t, filepath.Join(repoPath, customHooksDir)) + return + } + + var extractedFiles []testFile + + hooksPath := filepath.Join(repoPath, customHooksDir) + err = filepath.WalkDir(hooksPath, func(path string, d fs.DirEntry, err error) error { + require.NoError(t, err) + + relPath, err := filepath.Rel(repoPath, path) + require.NoError(t, err) + + var hookData []byte + if !d.IsDir() { + hookData, err = os.ReadFile(path) + require.NoError(t, err) + } + + extractedFiles = append(extractedFiles, testFile{ + name: relPath, + content: string(hookData), + }) + + return nil + }) + require.NoError(t, err) + require.Equal(t, tc.expectedFiles, extractedFiles) + }) + } +} |