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 | |
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.
-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) + }) + } +} |