diff options
author | Sami Hiltunen <shiltunen@gitlab.com> | 2023-09-27 09:06:32 +0300 |
---|---|---|
committer | Sami Hiltunen <shiltunen@gitlab.com> | 2023-09-27 09:06:32 +0300 |
commit | b7074c08d5e57806fda21f77dec30185db6043a1 (patch) | |
tree | 6902e854f840241c71a9eac4cd831afb83d18b9f | |
parent | 0d02e2dfdd429b7c5cda79c8f2af1132566ade65 (diff) | |
parent | a56e600ead177e500c82514759291902638c7df1 (diff) |
Merge branch 'smh-support-multiple-ref-updates' into 'master'
Capture reference changes into a transaction in reference transaction hook
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6383
Merged-by: Sami Hiltunen <shiltunen@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Reviewed-by: Sami Hiltunen <shiltunen@gitlab.com>
Reviewed-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Reviewed-by: karthik nayak <knayak@gitlab.com>
29 files changed, 918 insertions, 167 deletions
diff --git a/cmd/gitaly-hooks/hooks_test.go b/cmd/gitaly-hooks/hooks_test.go index 1503e7077..c8886caac 100644 --- a/cmd/gitaly-hooks/hooks_test.go +++ b/cmd/gitaly-hooks/hooks_test.go @@ -22,6 +22,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config/prometheus" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/service" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/service/hook" + "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/transaction" "gitlab.com/gitlab-org/gitaly/v16/internal/gitlab" "gitlab.com/gitlab-org/gitaly/v16/internal/grpc/metadata" @@ -79,7 +80,11 @@ func envForHooks(tb testing.TB, ctx context.Context, cfg config.Cfg, repo *gital Username: glHookValues.GLUsername, Protocol: glHookValues.GLProtocol, RemoteIP: glHookValues.RemoteIP, - }, git.AllHooks, featureFlags(ctx)).Env() + }, + git.AllHooks, + featureFlags(ctx), + storage.ExtractTransactionID(ctx), + ).Env() require.NoError(tb, err) env := append(command.AllowedEnvironment(os.Environ()), []string{ @@ -453,6 +458,7 @@ func TestHooksPostReceiveFailed(t *testing.T) { }, git.PostReceiveHook, featureFlags(ctx), + storage.ExtractTransactionID(ctx), ).Env() require.NoError(t, err) @@ -557,6 +563,7 @@ func TestRequestedHooks(t *testing.T) { nil, git.AllHooks&^hook, nil, + 0, ).Env() require.NoError(t, err) @@ -579,6 +586,7 @@ func TestRequestedHooks(t *testing.T) { nil, hook, nil, + 0, ).Env() require.NoError(t, err) diff --git a/internal/cli/gitaly/serve.go b/internal/cli/gitaly/serve.go index 81f332883..67a34acf1 100644 --- a/internal/cli/gitaly/serve.go +++ b/internal/cli/gitaly/serve.go @@ -31,6 +31,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/service/setup" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage/counter" + "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage/storagemgr" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/transaction" "gitlab.com/gitlab-org/gitaly/v16/internal/gitlab" "gitlab.com/gitlab-org/gitaly/v16/internal/grpc/backchannel" @@ -263,7 +264,7 @@ func run(cfg config.Cfg, logger log.Logger) error { } prometheus.MustRegister(gitlabClient) - hm := hook.NewManager(cfg, locator, gitCmdFactory, transactionManager, gitlabClient) + hm := hook.NewManager(cfg, locator, gitCmdFactory, transactionManager, gitlabClient, hook.NewTransactionRegistry(storagemgr.NewTransactionRegistry())) hookManager = hm } diff --git a/internal/cli/gitaly/subcmd_check.go b/internal/cli/gitaly/subcmd_check.go index 36385cc2b..a8362c656 100644 --- a/internal/cli/gitaly/subcmd_check.go +++ b/internal/cli/gitaly/subcmd_check.go @@ -9,6 +9,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config/prometheus" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/hook" + "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage/storagemgr" "gitlab.com/gitlab-org/gitaly/v16/internal/gitlab" "gitlab.com/gitlab-org/gitaly/v16/internal/log" ) @@ -73,5 +74,5 @@ func checkAPI(cfg config.Cfg, logger log.Logger) (*gitlab.CheckInfo, error) { } defer cleanup() - return hook.NewManager(cfg, config.NewLocator(cfg), gitCmdFactory, nil, gitlabAPI).Check(context.Background()) + return hook.NewManager(cfg, config.NewLocator(cfg), gitCmdFactory, nil, gitlabAPI, hook.NewTransactionRegistry(storagemgr.NewTransactionRegistry())).Check(context.Background()) } diff --git a/internal/git/hooks_options.go b/internal/git/hooks_options.go index 43991e765..f0c0bb32f 100644 --- a/internal/git/hooks_options.go +++ b/internal/git/hooks_options.go @@ -152,7 +152,9 @@ func (cc *cmdCfg) configureHooks( transaction, userDetails, requestedHooks, - featureflag.FromContext(ctx)).Env() + featureflag.FromContext(ctx), + storage.ExtractTransactionID(ctx), + ).Env() if err != nil { return err } diff --git a/internal/git/hooks_payload.go b/internal/git/hooks_payload.go index 2dfd8f15d..c1528d0f6 100644 --- a/internal/git/hooks_payload.go +++ b/internal/git/hooks_payload.go @@ -9,6 +9,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/internal/featureflag" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config" + "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" "gitlab.com/gitlab-org/gitaly/v16/internal/transaction/txinfo" "gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb" "google.golang.org/protobuf/encoding/protojson" @@ -86,6 +87,10 @@ type HooksPayload struct { // UserDetails contains information required when executing // git-receive-pack or git-upload-pack UserDetails *UserDetails `json:"user_details"` + + // TransactionID identifies the storage transaction this hooks call runs in. It's + // used to access the transaction in the hook manager. + TransactionID storage.TransactionID `json:"transaction_id,omitempty"` } // UserDetails contains all information which is required for hooks @@ -121,6 +126,7 @@ func NewHooksPayload( userDetails *UserDetails, requestedHooks Hook, featureFlagsWithValue map[featureflag.FeatureFlag]bool, + transactionID storage.TransactionID, ) HooksPayload { flags := make([]FeatureFlagWithValue, 0, len(featureFlagsWithValue)) for flag, enabled := range featureFlagsWithValue { @@ -140,6 +146,7 @@ func NewHooksPayload( UserDetails: userDetails, RequestedHooks: requestedHooks, FeatureFlagsWithValue: flags, + TransactionID: transactionID, } } diff --git a/internal/git/hooks_payload_test.go b/internal/git/hooks_payload_test.go index e73e755a6..dfe5bea6a 100644 --- a/internal/git/hooks_payload_test.go +++ b/internal/git/hooks_payload_test.go @@ -38,6 +38,7 @@ func TestHooksPayload(t *testing.T) { nil, git.AllHooks, nil, + 0, ).Env() require.NoError(t, err) require.True(t, strings.HasPrefix(env, git.EnvHooksPayload+"=")) @@ -53,7 +54,9 @@ func TestHooksPayload(t *testing.T) { git.PreReceiveHook, map[featureflag.FeatureFlag]bool{ {Name: "flag_key"}: true, - }).Env() + }, + 1, + ).Env() require.NoError(t, err) payload, err := git.HooksPayloadFromEnv([]string{ @@ -76,6 +79,7 @@ func TestHooksPayload(t *testing.T) { Enabled: true, }, }, + TransactionID: 1, }, payload) }) @@ -88,6 +92,7 @@ func TestHooksPayload(t *testing.T) { nil, git.UpdateHook, nil, + 0, ).Env() require.NoError(t, err) @@ -125,7 +130,7 @@ func TestHooksPayload(t *testing.T) { UserID: "1234", Username: "user", Protocol: "ssh", - }, git.PostReceiveHook, nil).Env() + }, git.PostReceiveHook, nil, 0).Env() require.NoError(t, err) payload, err := git.HooksPayloadFromEnv([]string{ diff --git a/internal/gitaly/hook/manager.go b/internal/gitaly/hook/manager.go index b021d9909..d093412db 100644 --- a/internal/gitaly/hook/manager.go +++ b/internal/gitaly/hook/manager.go @@ -7,6 +7,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" + "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage/storagemgr" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/transaction" "gitlab.com/gitlab-org/gitaly/v16/internal/gitlab" "gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb" @@ -47,6 +48,32 @@ type Manager interface { ReferenceTransactionHook(ctx context.Context, state ReferenceTransactionState, env []string, stdin io.Reader) error } +// Transaction is the interface of storagemgr.Transaction. It's used for mocking in the tests. +type Transaction interface { + RecordInitialReferenceValues(context.Context, map[git.ReferenceName]git.ObjectID) error + UpdateReferences(storagemgr.ReferenceUpdates) +} + +// TransactionRegistry is the interface of storagemgr.TransactionRegistry. It's used for mocking +// in the tests. +type TransactionRegistry interface { + Get(storage.TransactionID) (Transaction, error) +} + +type transactionRegistry struct { + registry *storagemgr.TransactionRegistry +} + +func (r *transactionRegistry) Get(id storage.TransactionID) (Transaction, error) { + return r.registry.Get(id) +} + +// NewTransactionRegistry wraps a storagemgr.TransactionRegistry to adapt it to the interface +// used by the manager. +func NewTransactionRegistry(txRegistry *storagemgr.TransactionRegistry) TransactionRegistry { + return &transactionRegistry{registry: txRegistry} +} + // GitLabHookManager is a hook manager containing Git hook business logic. It // uses the GitLab API to authenticate and track ongoing hook calls. type GitLabHookManager struct { @@ -55,6 +82,7 @@ type GitLabHookManager struct { gitCmdFactory git.CommandFactory txManager transaction.Manager gitlabClient gitlab.Client + txRegistry TransactionRegistry } // NewManager returns a new hook manager @@ -64,6 +92,7 @@ func NewManager( gitCmdFactory git.CommandFactory, txManager transaction.Manager, gitlabClient gitlab.Client, + txRegistry TransactionRegistry, ) *GitLabHookManager { return &GitLabHookManager{ cfg: cfg, @@ -71,5 +100,6 @@ func NewManager( gitCmdFactory: gitCmdFactory, txManager: txManager, gitlabClient: gitlabClient, + txRegistry: txRegistry, } } diff --git a/internal/gitaly/hook/postreceive_test.go b/internal/gitaly/hook/postreceive_test.go index 9d8227979..096279196 100644 --- a/internal/gitaly/hook/postreceive_test.go +++ b/internal/gitaly/hook/postreceive_test.go @@ -16,6 +16,8 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo" "gitlab.com/gitlab-org/gitaly/v16/internal/git/quarantine" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config" + "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" + "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage/storagemgr" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/transaction" "gitlab.com/gitlab-org/gitaly/v16/internal/gitlab" "gitlab.com/gitlab-org/gitaly/v16/internal/grpc/backchannel" @@ -82,7 +84,7 @@ func TestPostReceive_customHook(t *testing.T) { txManager := transaction.NewTrackingManager() hookManager := NewManager(cfg, locator, gitCmdFactory, txManager, gitlab.NewMockClient( t, gitlab.MockAllowed, gitlab.MockPreReceive, gitlab.MockPostReceive, - )) + ), NewTransactionRegistry(storagemgr.NewTransactionRegistry())) receiveHooksPayload := &git.UserDetails{ UserID: "1234", @@ -98,6 +100,7 @@ func TestPostReceive_customHook(t *testing.T) { receiveHooksPayload, git.PostReceiveHook, featureflag.FromContext(ctx), + storage.ExtractTransactionID(ctx), ).Env() require.NoError(t, err) @@ -111,6 +114,7 @@ func TestPostReceive_customHook(t *testing.T) { receiveHooksPayload, git.PostReceiveHook, featureflag.FromContext(ctx), + storage.ExtractTransactionID(ctx), ).Env() require.NoError(t, err) @@ -124,6 +128,7 @@ func TestPostReceive_customHook(t *testing.T) { receiveHooksPayload, git.PostReceiveHook, featureflag.FromContext(ctx), + storage.ExtractTransactionID(ctx), ).Env() require.NoError(t, err) @@ -287,7 +292,7 @@ func TestPostReceive_gitlab(t *testing.T) { UserID: "1234", Username: "user", Protocol: "web", - }, git.PostReceiveHook, nil).Env() + }, git.PostReceiveHook, nil, storage.ExtractTransactionID(ctx)).Env() require.NoError(t, err) standardEnv := []string{payload} @@ -376,7 +381,7 @@ func TestPostReceive_gitlab(t *testing.T) { }, } - hookManager := NewManager(cfg, config.NewLocator(cfg), gittest.NewCommandFactory(t, cfg), transaction.NewManager(cfg, backchannel.NewRegistry()), &gitlabAPI) + hookManager := NewManager(cfg, config.NewLocator(cfg), gittest.NewCommandFactory(t, cfg), transaction.NewManager(cfg, backchannel.NewRegistry()), &gitlabAPI, NewTransactionRegistry(storagemgr.NewTransactionRegistry())) gittest.WriteCustomHook(t, repoPath, "post-receive", []byte("#!/bin/sh\necho hook called\n")) @@ -414,7 +419,7 @@ func TestPostReceive_quarantine(t *testing.T) { hookManager := NewManager(cfg, config.NewLocator(cfg), gittest.NewCommandFactory(t, cfg), nil, gitlab.NewMockClient( t, gitlab.MockAllowed, gitlab.MockPreReceive, gitlab.MockPostReceive, - )) + ), NewTransactionRegistry(storagemgr.NewTransactionRegistry())) gittest.WriteCustomHook(t, repoPath, "post-receive", []byte(fmt.Sprintf( `#!/bin/sh @@ -438,6 +443,7 @@ func TestPostReceive_quarantine(t *testing.T) { }, git.PreReceiveHook, featureflag.FromContext(ctx), + storage.ExtractTransactionID(ctx), ).Env() require.NoError(t, err) diff --git a/internal/gitaly/hook/prereceive_test.go b/internal/gitaly/hook/prereceive_test.go index 1525e096b..9da48e9da 100644 --- a/internal/gitaly/hook/prereceive_test.go +++ b/internal/gitaly/hook/prereceive_test.go @@ -15,6 +15,8 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo" "gitlab.com/gitlab-org/gitaly/v16/internal/git/quarantine" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config" + "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" + "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage/storagemgr" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/transaction" "gitlab.com/gitlab-org/gitaly/v16/internal/gitlab" "gitlab.com/gitlab-org/gitaly/v16/internal/grpc/backchannel" @@ -41,7 +43,7 @@ func TestPrereceive_customHooks(t *testing.T) { txManager := transaction.NewTrackingManager() hookManager := NewManager(cfg, locator, gitCmdFactory, txManager, gitlab.NewMockClient( t, gitlab.MockAllowed, gitlab.MockPreReceive, gitlab.MockPostReceive, - )) + ), NewTransactionRegistry(storagemgr.NewTransactionRegistry())) receiveHooksPayload := &git.UserDetails{ UserID: "1234", @@ -57,6 +59,7 @@ func TestPrereceive_customHooks(t *testing.T) { receiveHooksPayload, git.PreReceiveHook, featureflag.FromContext(ctx), + storage.ExtractTransactionID(ctx), ).Env() require.NoError(t, err) @@ -70,6 +73,7 @@ func TestPrereceive_customHooks(t *testing.T) { receiveHooksPayload, git.PreReceiveHook, featureflag.FromContext(ctx), + storage.ExtractTransactionID(ctx), ).Env() require.NoError(t, err) @@ -83,6 +87,7 @@ func TestPrereceive_customHooks(t *testing.T) { receiveHooksPayload, git.PreReceiveHook, featureflag.FromContext(ctx), + storage.ExtractTransactionID(ctx), ).Env() require.NoError(t, err) @@ -223,7 +228,7 @@ func TestPrereceive_quarantine(t *testing.T) { hookManager := NewManager(cfg, config.NewLocator(cfg), gittest.NewCommandFactory(t, cfg), nil, gitlab.NewMockClient( t, gitlab.MockAllowed, gitlab.MockPreReceive, gitlab.MockPostReceive, - )) + ), NewTransactionRegistry(storagemgr.NewTransactionRegistry())) //nolint:gitaly-linters gittest.WriteCustomHook(t, repoPath, "pre-receive", []byte(fmt.Sprintf( @@ -248,6 +253,7 @@ func TestPrereceive_quarantine(t *testing.T) { }, git.PreReceiveHook, featureflag.FromContext(ctx), + storage.ExtractTransactionID(ctx), ).Env() require.NoError(t, err) @@ -309,7 +315,7 @@ func TestPrereceive_gitlab(t *testing.T) { UserID: "1234", Username: "user", Protocol: "web", - }, git.PreReceiveHook, nil).Env() + }, git.PreReceiveHook, nil, storage.ExtractTransactionID(ctx)).Env() require.NoError(t, err) standardEnv := []string{payload} @@ -412,7 +418,7 @@ func TestPrereceive_gitlab(t *testing.T) { }, } - hookManager := NewManager(cfg, config.NewLocator(cfg), gittest.NewCommandFactory(t, cfg), transaction.NewManager(cfg, backchannel.NewRegistry()), &gitlabAPI) + hookManager := NewManager(cfg, config.NewLocator(cfg), gittest.NewCommandFactory(t, cfg), transaction.NewManager(cfg, backchannel.NewRegistry()), &gitlabAPI, NewTransactionRegistry(storagemgr.NewTransactionRegistry())) gittest.WriteCustomHook(t, repoPath, "pre-receive", []byte("#!/bin/sh\necho called\n")) diff --git a/internal/gitaly/hook/referencetransaction.go b/internal/gitaly/hook/referencetransaction.go index cf376e389..bd90023f0 100644 --- a/internal/gitaly/hook/referencetransaction.go +++ b/internal/gitaly/hook/referencetransaction.go @@ -7,8 +7,10 @@ import ( "crypto/sha1" "fmt" "io" + "strings" "gitlab.com/gitlab-org/gitaly/v16/internal/git" + "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage/storagemgr" "gitlab.com/gitlab-org/gitaly/v16/internal/transaction/voting" ) @@ -29,18 +31,53 @@ func (m *GitLabHookManager) ReferenceTransactionHook(ctx context.Context, state return fmt.Errorf("reading stdin from request: %w", err) } + var tx Transaction + if payload.TransactionID > 0 { + tx, err = m.txRegistry.Get(payload.TransactionID) + if err != nil { + return fmt.Errorf("get transaction: %w", err) + } + } + var phase voting.Phase switch state { // We're voting in prepared state as this is the only stage in Git's reference transaction // which allows us to abort the transaction. case ReferenceTransactionPrepared: phase = voting.Prepared + + if tx != nil { + updates, err := parseChanges(ctx, objectHash, bytes.NewReader(changes)) + if err != nil { + return fmt.Errorf("parse changes: %w", err) + } + + initialValues := map[git.ReferenceName]git.ObjectID{} + for reference, update := range updates { + initialValues[reference] = update.OldOID + } + + // Only record the initial values of the reference in the prepare step as this + // change hasn't yet been committed. + if err := tx.RecordInitialReferenceValues(ctx, initialValues); err != nil { + return fmt.Errorf("record initial reference value: %w", err) + } + } // We're also voting in committed state to tell Praefect we've actually persisted the // changes. This is necessary as some RPCs fail return errors in the response body rather // than as an error code. Praefect can't tell if these RPCs have failed. Voting on committed // ensure Praefect sees either a missing vote or that the RPC did commit the changes. case ReferenceTransactionCommitted: phase = voting.Committed + + if tx != nil { + updates, err := parseChanges(ctx, objectHash, bytes.NewReader(changes)) + if err != nil { + return fmt.Errorf("parse changes: %w", err) + } + + tx.UpdateReferences(updates) + } default: return nil } @@ -76,6 +113,44 @@ func (m *GitLabHookManager) ReferenceTransactionHook(ctx context.Context, state return nil } +// parseChanges parses the changes from the reader. All updates to references lacking a 'refs/' prefix are ignored. These +// are the various pseudo reference like ORIG_HEAD but also HEAD. See the documentation of the reference-transaction hook +// for details on the format: https://git-scm.com/docs/githooks#_reference_transaction +func parseChanges(ctx context.Context, objectHash git.ObjectHash, changes io.Reader) (storagemgr.ReferenceUpdates, error) { + scanner := bufio.NewScanner(changes) + + updates := storagemgr.ReferenceUpdates{} + for scanner.Scan() { + line := scanner.Text() + components := strings.Split(line, " ") + if len(components) != 3 { + return nil, fmt.Errorf("unexpected change line: %q", line) + } + + reference := git.ReferenceName(components[2]) + if !strings.HasPrefix(reference.String(), "refs/") { + continue + } + + update := storagemgr.ReferenceUpdate{} + + var err error + update.OldOID, err = objectHash.FromHex(components[0]) + if err != nil { + return nil, fmt.Errorf("parse old: %w", err) + } + + update.NewOID, err = objectHash.FromHex(components[1]) + if err != nil { + return nil, fmt.Errorf("parse new: %w", err) + } + + updates[reference] = update + } + + return updates, nil +} + // isForceDeletionsOnly determines whether the given changes only consist of force-deletions. func isForceDeletionsOnly(objectHash git.ObjectHash, changes io.Reader) bool { // forceDeletionPrefix is the prefix of a queued reference transaction which deletes a diff --git a/internal/gitaly/hook/transactions_test.go b/internal/gitaly/hook/transactions_test.go index bf708b15e..ce829848c 100644 --- a/internal/gitaly/hook/transactions_test.go +++ b/internal/gitaly/hook/transactions_test.go @@ -13,6 +13,8 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config" + "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" + "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage/storagemgr" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/transaction" "gitlab.com/gitlab-org/gitaly/v16/internal/gitlab" "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper" @@ -36,7 +38,7 @@ func TestHookManager_stopCalled(t *testing.T) { var mockTxMgr transaction.MockManager hookManager := NewManager(cfg, config.NewLocator(cfg), gittest.NewCommandFactory(t, cfg), &mockTxMgr, gitlab.NewMockClient( t, gitlab.MockAllowed, gitlab.MockPreReceive, gitlab.MockPostReceive, - )) + ), NewTransactionRegistry(storagemgr.NewTransactionRegistry())) hooksPayload, err := git.NewHooksPayload( cfg, @@ -50,6 +52,7 @@ func TestHookManager_stopCalled(t *testing.T) { }, git.ReferenceTransactionHook, featureflag.FromContext(ctx), + storage.ExtractTransactionID(ctx), ).Env() require.NoError(t, err) @@ -141,7 +144,7 @@ func TestHookManager_contextCancellationCancelsVote(t *testing.T) { hookManager := NewManager(cfg, config.NewLocator(cfg), gittest.NewCommandFactory(t, cfg), &mockTxMgr, gitlab.NewMockClient( t, gitlab.MockAllowed, gitlab.MockPreReceive, gitlab.MockPostReceive, - )) + ), NewTransactionRegistry(storagemgr.NewTransactionRegistry())) hooksPayload, err := git.NewHooksPayload( cfg, @@ -153,6 +156,7 @@ func TestHookManager_contextCancellationCancelsVote(t *testing.T) { nil, git.ReferenceTransactionHook, nil, + storage.ExtractTransactionID(ctx), ).Env() require.NoError(t, err) diff --git a/internal/gitaly/hook/update_test.go b/internal/gitaly/hook/update_test.go index 898317fe9..45dec2ca1 100644 --- a/internal/gitaly/hook/update_test.go +++ b/internal/gitaly/hook/update_test.go @@ -13,6 +13,8 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo" "gitlab.com/gitlab-org/gitaly/v16/internal/git/quarantine" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config" + "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" + "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage/storagemgr" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/transaction" "gitlab.com/gitlab-org/gitaly/v16/internal/gitlab" "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper" @@ -39,7 +41,7 @@ func TestUpdate_customHooks(t *testing.T) { txManager := transaction.NewTrackingManager() hookManager := NewManager(cfg, locator, gitCmdFactory, txManager, gitlab.NewMockClient( t, gitlab.MockAllowed, gitlab.MockPreReceive, gitlab.MockPostReceive, - )) + ), NewTransactionRegistry(storagemgr.NewTransactionRegistry())) receiveHooksPayload := &git.UserDetails{ UserID: "1234", @@ -55,6 +57,7 @@ func TestUpdate_customHooks(t *testing.T) { receiveHooksPayload, git.UpdateHook, featureflag.FromContext(ctx), + storage.ExtractTransactionID(ctx), ).Env() require.NoError(t, err) @@ -68,6 +71,7 @@ func TestUpdate_customHooks(t *testing.T) { receiveHooksPayload, git.UpdateHook, featureflag.FromContext(ctx), + storage.ExtractTransactionID(ctx), ).Env() require.NoError(t, err) @@ -81,6 +85,7 @@ func TestUpdate_customHooks(t *testing.T) { receiveHooksPayload, git.UpdateHook, featureflag.FromContext(ctx), + storage.ExtractTransactionID(ctx), ).Env() require.NoError(t, err) @@ -253,7 +258,7 @@ func TestUpdate_quarantine(t *testing.T) { hookManager := NewManager(cfg, config.NewLocator(cfg), gittest.NewCommandFactory(t, cfg), nil, gitlab.NewMockClient( t, gitlab.MockAllowed, gitlab.MockPreReceive, gitlab.MockPostReceive, - )) + ), NewTransactionRegistry(storagemgr.NewTransactionRegistry())) //nolint:gitaly-linters gittest.WriteCustomHook(t, repoPath, "update", []byte(fmt.Sprintf( @@ -278,6 +283,7 @@ func TestUpdate_quarantine(t *testing.T) { }, git.PreReceiveHook, featureflag.FromContext(ctx), + storage.ExtractTransactionID(ctx), ).Env() require.NoError(t, err) diff --git a/internal/gitaly/hook/updateref/update_with_hooks.go b/internal/gitaly/hook/updateref/update_with_hooks.go index a1fb7175a..d2dbf7f33 100644 --- a/internal/gitaly/hook/updateref/update_with_hooks.go +++ b/internal/gitaly/hook/updateref/update_with_hooks.go @@ -202,7 +202,7 @@ func (u *UpdaterWithHooks) UpdateReference( quarantinedRepo = quarantineDir.QuarantinedRepo() } - hooksPayload, err := git.NewHooksPayload(u.cfg, quarantinedRepo, objectHash, transaction, &receiveHooksPayload, git.ReceivePackHooks, featureflag.FromContext(ctx)).Env() + hooksPayload, err := git.NewHooksPayload(u.cfg, quarantinedRepo, objectHash, transaction, &receiveHooksPayload, git.ReceivePackHooks, featureflag.FromContext(ctx), storage.ExtractTransactionID(ctx)).Env() if err != nil { return fmt.Errorf("constructing hooks payload: %w", err) } @@ -224,7 +224,7 @@ func (u *UpdaterWithHooks) UpdateReference( // We only need to update the hooks payload to the unquarantined repo in case we // had a quarantine environment. Otherwise, the initial hooks payload is for the // real repository anyway. - hooksPayload, err = git.NewHooksPayload(u.cfg, repoProto, objectHash, transaction, &receiveHooksPayload, git.ReceivePackHooks, featureflag.FromContext(ctx)).Env() + hooksPayload, err = git.NewHooksPayload(u.cfg, repoProto, objectHash, transaction, &receiveHooksPayload, git.ReceivePackHooks, featureflag.FromContext(ctx), storage.ExtractTransactionID(ctx)).Env() if err != nil { return fmt.Errorf("constructing quarantined hooks payload: %w", err) } diff --git a/internal/gitaly/hook/updateref/update_with_hooks_test.go b/internal/gitaly/hook/updateref/update_with_hooks_test.go index 45306cde1..13786f2cd 100644 --- a/internal/gitaly/hook/updateref/update_with_hooks_test.go +++ b/internal/gitaly/hook/updateref/update_with_hooks_test.go @@ -19,6 +19,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/hook/updateref" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/service" hookservice "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/service/hook" + "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper/testcfg" "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper/testserver" @@ -118,7 +119,11 @@ func TestUpdaterWithHooks_UpdateReference(t *testing.T) { UserID: gittest.TestUser.GlId, Username: gittest.TestUser.GlUsername, Protocol: "web", - }, git.ReceivePackHooks, featureflag.FromContext(ctx)) + }, + git.ReceivePackHooks, + featureflag.FromContext(ctx), + storage.ExtractTransactionID(ctx), + ) actualPayload, err := git.HooksPayloadFromEnv(env) require.NoError(t, err) diff --git a/internal/gitaly/server/auth_test.go b/internal/gitaly/server/auth_test.go index 4ca5b2584..7ab5aad0e 100644 --- a/internal/gitaly/server/auth_test.go +++ b/internal/gitaly/server/auth_test.go @@ -22,6 +22,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/hook/updateref" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/service" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/service/setup" + "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage/storagemgr" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/transaction" "gitlab.com/gitlab-org/gitaly/v16/internal/gitlab" "gitlab.com/gitlab-org/gitaly/v16/internal/grpc/backchannel" @@ -194,7 +195,7 @@ func runServer(t *testing.T, cfg config.Cfg) string { gitCmdFactory := gittest.NewCommandFactory(t, cfg) hookManager := hook.NewManager(cfg, locator, gitCmdFactory, txManager, gitlab.NewMockClient( t, gitlab.MockAllowed, gitlab.MockPreReceive, gitlab.MockPostReceive, - )) + ), hook.NewTransactionRegistry(storagemgr.NewTransactionRegistry())) catfileCache := catfile.NewCache(cfg) t.Cleanup(catfileCache.Stop) diskCache := cache.New(cfg, locator, logger) diff --git a/internal/gitaly/service/dependencies.go b/internal/gitaly/service/dependencies.go index e260a1465..923dcddbd 100644 --- a/internal/gitaly/service/dependencies.go +++ b/internal/gitaly/service/dependencies.go @@ -39,6 +39,7 @@ type Dependencies struct { RepositoryCounter *counter.RepositoryCounter UpdaterWithHooks *updateref.UpdaterWithHooks HousekeepingManager housekeeping.Manager + TransactionRegistry *storagemgr.TransactionRegistry PartitionManager *storagemgr.PartitionManager BackupSink backup.Sink BackupLocator backup.Locator @@ -124,6 +125,11 @@ func (dc *Dependencies) GetPackObjectsLimiter() limiter.Limiter { return dc.PackObjectsLimiter } +// GetTransactionRegistry returns the TransactionRegistry. +func (dc *Dependencies) GetTransactionRegistry() *storagemgr.TransactionRegistry { + return dc.TransactionRegistry +} + // GetPartitionManager returns the PartitionManager. func (dc *Dependencies) GetPartitionManager() *storagemgr.PartitionManager { return dc.PartitionManager diff --git a/internal/gitaly/service/hook/post_receive_test.go b/internal/gitaly/service/hook/post_receive_test.go index d84a0f3b8..5d2af9eba 100644 --- a/internal/gitaly/service/hook/post_receive_test.go +++ b/internal/gitaly/service/hook/post_receive_test.go @@ -125,6 +125,7 @@ func TestHooksMissingStdin(t *testing.T) { }, git.PostReceiveHook, featureflag.FromContext(ctx), + storage.ExtractTransactionID(ctx), ).Env() require.NoError(t, err) @@ -259,6 +260,7 @@ To create a merge request for okay, visit: }, git.PostReceiveHook, featureflag.FromContext(ctx), + storage.ExtractTransactionID(ctx), ).Env() require.NoError(t, err) diff --git a/internal/gitaly/service/hook/pre_receive_test.go b/internal/gitaly/service/hook/pre_receive_test.go index 6131e909f..921e19c50 100644 --- a/internal/gitaly/service/hook/pre_receive_test.go +++ b/internal/gitaly/service/hook/pre_receive_test.go @@ -157,6 +157,7 @@ func TestPreReceiveHook_GitlabAPIAccess(t *testing.T) { }, git.PreReceiveHook, featureflag.FromContext(ctx), + storage.ExtractTransactionID(ctx), ).Env() require.NoError(t, err) @@ -278,6 +279,7 @@ func TestPreReceive_APIErrors(t *testing.T) { }, git.PreReceiveHook, featureflag.FromContext(ctx), + storage.ExtractTransactionID(ctx), ).Env() require.NoError(t, err) @@ -352,6 +354,7 @@ exit %d }, git.PreReceiveHook, featureflag.FromContext(ctx), + storage.ExtractTransactionID(ctx), ).Env() require.NoError(t, err) @@ -505,6 +508,7 @@ func TestPreReceiveHook_Primary(t *testing.T) { }, git.PreReceiveHook, featureflag.FromContext(ctx), + storage.ExtractTransactionID(ctx), ).Env() require.NoError(t, err) diff --git a/internal/gitaly/service/hook/reference_transaction_test.go b/internal/gitaly/service/hook/reference_transaction_test.go index 6cf47050f..3dfeb9153 100644 --- a/internal/gitaly/service/hook/reference_transaction_test.go +++ b/internal/gitaly/service/hook/reference_transaction_test.go @@ -3,6 +3,7 @@ package hook import ( "context" "crypto/sha1" + "fmt" "net" "testing" @@ -10,7 +11,9 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/internal/featureflag" "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest" + "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/hook" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" + "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage/storagemgr" "gitlab.com/gitlab-org/gitaly/v16/internal/grpc/backchannel" "gitlab.com/gitlab-org/gitaly/v16/internal/structerr" "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper" @@ -22,6 +25,27 @@ import ( "google.golang.org/grpc/credentials/insecure" ) +type mockTransactionRegistry struct { + getFunc func(storage.TransactionID) (hook.Transaction, error) +} + +func (m mockTransactionRegistry) Get(id storage.TransactionID) (hook.Transaction, error) { + return m.getFunc(id) +} + +type mockTransaction struct { + updateReferencesFunc func(storagemgr.ReferenceUpdates) + recordInitialReferenceValues func(context.Context, map[git.ReferenceName]git.ObjectID) error +} + +func (m mockTransaction) UpdateReferences(updates storagemgr.ReferenceUpdates) { + m.updateReferencesFunc(updates) +} + +func (m mockTransaction) RecordInitialReferenceValues(ctx context.Context, initialValues map[git.ReferenceName]git.ObjectID) error { + return m.recordInitialReferenceValues(ctx, initialValues) +} + type testTransactionServer struct { gitalypb.UnimplementedRefTransactionServer handler func(in *gitalypb.VoteTransactionRequest) (*gitalypb.VoteTransactionResponse, error) @@ -51,29 +75,45 @@ func TestReferenceTransactionHookInvalidArgument(t *testing.T) { } func TestReferenceTransactionHook(t *testing.T) { + stdin := []byte(fmt.Sprintf( + `%[1]s %[2]s refs/heads/branch-1 +%[2]s %[1]s refs/heads/branch-2 +%[1]s %[2]s HEAD +`, + gittest.DefaultObjectHash.ZeroOID, + gittest.DefaultObjectHash.EmptyTreeOID, + )) + testCases := []struct { - desc string - stdin []byte - state gitalypb.ReferenceTransactionHookRequest_State - voteResponse gitalypb.VoteTransactionResponse_TransactionState - expectedErr error - expectedResponse *gitalypb.ReferenceTransactionHookResponse - expectedReftxHash []byte + desc string + stdin []byte + state gitalypb.ReferenceTransactionHookRequest_State + voteResponse gitalypb.VoteTransactionResponse_TransactionState + noTransaction bool + expectedErr error + expectedResponse *gitalypb.ReferenceTransactionHookResponse + expectedReftxHash []byte + expectedReferenceUpdates storagemgr.ReferenceUpdates + expectedInitialValues map[git.ReferenceName]git.ObjectID }{ { desc: "hook triggers transaction with default state", - stdin: []byte("foobar"), + stdin: stdin, voteResponse: gitalypb.VoteTransactionResponse_COMMIT, expectedResponse: &gitalypb.ReferenceTransactionHookResponse{ ExitStatus: &gitalypb.ExitStatus{ Value: 0, }, }, - expectedReftxHash: []byte("foobar"), + expectedReftxHash: stdin, + expectedInitialValues: map[git.ReferenceName]git.ObjectID{ + "refs/heads/branch-1": gittest.DefaultObjectHash.ZeroOID, + "refs/heads/branch-2": gittest.DefaultObjectHash.EmptyTreeOID, + }, }, { desc: "hook triggers transaction with explicit prepared state", - stdin: []byte("foobar"), + stdin: stdin, state: gitalypb.ReferenceTransactionHookRequest_PREPARED, voteResponse: gitalypb.VoteTransactionResponse_COMMIT, expectedResponse: &gitalypb.ReferenceTransactionHookResponse{ @@ -81,11 +121,28 @@ func TestReferenceTransactionHook(t *testing.T) { Value: 0, }, }, - expectedReftxHash: []byte("foobar"), + expectedReftxHash: stdin, + expectedInitialValues: map[git.ReferenceName]git.ObjectID{ + "refs/heads/branch-1": gittest.DefaultObjectHash.ZeroOID, + "refs/heads/branch-2": gittest.DefaultObjectHash.EmptyTreeOID, + }, + }, + { + desc: "hook triggers transaction with explicit prepared state without transaction", + stdin: stdin, + state: gitalypb.ReferenceTransactionHookRequest_PREPARED, + voteResponse: gitalypb.VoteTransactionResponse_COMMIT, + noTransaction: true, + expectedResponse: &gitalypb.ReferenceTransactionHookResponse{ + ExitStatus: &gitalypb.ExitStatus{ + Value: 0, + }, + }, + expectedReftxHash: stdin, }, { desc: "hook does not trigger transaction with aborted state", - stdin: []byte("foobar"), + stdin: stdin, state: gitalypb.ReferenceTransactionHookRequest_ABORTED, expectedResponse: &gitalypb.ReferenceTransactionHookResponse{ ExitStatus: &gitalypb.ExitStatus{ @@ -95,28 +152,76 @@ func TestReferenceTransactionHook(t *testing.T) { }, { desc: "hook triggers transaction with committed state", - stdin: []byte("foobar"), + stdin: stdin, state: gitalypb.ReferenceTransactionHookRequest_COMMITTED, expectedResponse: &gitalypb.ReferenceTransactionHookResponse{ ExitStatus: &gitalypb.ExitStatus{ Value: 0, }, }, - expectedReftxHash: []byte("foobar"), + expectedReftxHash: stdin, + expectedReferenceUpdates: storagemgr.ReferenceUpdates{ + "refs/heads/branch-1": { + OldOID: gittest.DefaultObjectHash.ZeroOID, + NewOID: gittest.DefaultObjectHash.EmptyTreeOID, + }, + "refs/heads/branch-2": { + OldOID: gittest.DefaultObjectHash.EmptyTreeOID, + NewOID: gittest.DefaultObjectHash.ZeroOID, + }, + }, + }, + { + desc: "hook triggers transaction with committed state without transaction", + stdin: stdin, + state: gitalypb.ReferenceTransactionHookRequest_COMMITTED, + noTransaction: true, + expectedResponse: &gitalypb.ReferenceTransactionHookResponse{ + ExitStatus: &gitalypb.ExitStatus{ + Value: 0, + }, + }, + expectedReftxHash: stdin, }, { desc: "hook fails with failed vote", - stdin: []byte("foobar"), + stdin: stdin, voteResponse: gitalypb.VoteTransactionResponse_ABORT, expectedErr: structerr.NewAborted("reference-transaction hook: error voting on transaction: transaction was aborted"), - expectedReftxHash: []byte("foobar"), + expectedReftxHash: stdin, + expectedInitialValues: map[git.ReferenceName]git.ObjectID{ + "refs/heads/branch-1": gittest.DefaultObjectHash.ZeroOID, + "refs/heads/branch-2": gittest.DefaultObjectHash.EmptyTreeOID, + }, }, { desc: "hook fails with stopped vote", - stdin: []byte("foobar"), + stdin: stdin, voteResponse: gitalypb.VoteTransactionResponse_STOP, expectedErr: structerr.NewFailedPrecondition("reference-transaction hook: error voting on transaction: transaction was stopped"), - expectedReftxHash: []byte("foobar"), + expectedReftxHash: stdin, + expectedInitialValues: map[git.ReferenceName]git.ObjectID{ + "refs/heads/branch-1": gittest.DefaultObjectHash.ZeroOID, + "refs/heads/branch-2": gittest.DefaultObjectHash.EmptyTreeOID, + }, + }, + { + desc: "invalid change line", + stdin: []byte("invalid change_line"), + voteResponse: gitalypb.VoteTransactionResponse_STOP, + expectedErr: structerr.NewInternal(`reference-transaction hook: parse changes: unexpected change line: "invalid change_line"`), + }, + { + desc: "invalid old oid", + stdin: []byte(fmt.Sprintf("invalid %s refs/heads/main", gittest.DefaultObjectHash.EmptyTreeOID)), + voteResponse: gitalypb.VoteTransactionResponse_STOP, + expectedErr: structerr.NewInternal(`reference-transaction hook: parse changes: parse old: invalid object ID: "invalid", expected length %d, got 7`, gittest.DefaultObjectHash.EncodedLen()), + }, + { + desc: "invalid new oid", + stdin: []byte(fmt.Sprintf("%s invalid refs/heads/main", gittest.DefaultObjectHash.EmptyTreeOID)), + voteResponse: gitalypb.VoteTransactionResponse_STOP, + expectedErr: structerr.NewInternal(`reference-transaction hook: parse changes: parse new: invalid object ID: "invalid", expected length %d, got 7`, gittest.DefaultObjectHash.EncodedLen()), }, } @@ -155,11 +260,31 @@ func TestReferenceTransactionHook(t *testing.T) { }, nil } - cfg.SocketPath = runHooksServer(t, cfg, nil, testserver.WithBackchannelRegistry(registry)) + var actualReferenceUpdates storagemgr.ReferenceUpdates + var actualInitialValues map[git.ReferenceName]git.ObjectID + txRegistry := mockTransactionRegistry{ + getFunc: func(storage.TransactionID) (hook.Transaction, error) { + return mockTransaction{ + updateReferencesFunc: func(updates storagemgr.ReferenceUpdates) { + actualReferenceUpdates = updates + }, + recordInitialReferenceValues: func(_ context.Context, initialValues map[git.ReferenceName]git.ObjectID) error { + actualInitialValues = initialValues + return nil + }, + }, nil + }, + } + + cfg.SocketPath = runHooksServerWithTransactionRegistry(t, cfg, nil, txRegistry, testserver.WithBackchannelRegistry(registry)) ctx := testhelper.Context(t) repo, _ := gittest.CreateRepository(t, ctx, cfg) + transactionID := storage.TransactionID(1) + if tc.noTransaction { + transactionID = 0 + } hooksPayload, err := git.NewHooksPayload( cfg, repo, @@ -172,6 +297,7 @@ func TestReferenceTransactionHook(t *testing.T) { nil, git.ReferenceTransactionHook, featureflag.FromContext(ctx), + transactionID, ).Env() require.NoError(t, err) @@ -200,10 +326,13 @@ func TestReferenceTransactionHook(t *testing.T) { var expectedReftxHash []byte if tc.expectedReftxHash != nil { - hash := sha1.Sum(tc.expectedReftxHash) + hash := sha1.Sum(tc.stdin) expectedReftxHash = hash[:] } require.Equal(t, expectedReftxHash[:], reftxHash) + + require.Equal(t, tc.expectedReferenceUpdates, actualReferenceUpdates) + require.Equal(t, tc.expectedInitialValues, actualInitialValues) }) } } diff --git a/internal/gitaly/service/hook/testhelper_test.go b/internal/gitaly/service/hook/testhelper_test.go index f55cbea57..d3f541193 100644 --- a/internal/gitaly/service/hook/testhelper_test.go +++ b/internal/gitaly/service/hook/testhelper_test.go @@ -4,6 +4,7 @@ import ( "testing" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config" + gitalyhook "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/hook" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/service" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/service/repository" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/transaction" @@ -48,11 +49,19 @@ func newHooksClient(tb testing.TB, serverSocketPath string) (gitalypb.HookServic type serverOption func(*server) func runHooksServer(tb testing.TB, cfg config.Cfg, opts []serverOption, serverOpts ...testserver.GitalyServerOpt) string { + return runHooksServerWithTransactionRegistry(tb, cfg, opts, nil, serverOpts...) +} + +func runHooksServerWithTransactionRegistry(tb testing.TB, cfg config.Cfg, opts []serverOption, txRegistry gitalyhook.TransactionRegistry, serverOpts ...testserver.GitalyServerOpt) string { tb.Helper() serverOpts = append(serverOpts, testserver.WithDisablePraefect()) return testserver.RunGitalyServer(tb, cfg, func(srv *grpc.Server, deps *service.Dependencies) { + if txRegistry != nil { + deps.GitalyHookManager = gitalyhook.NewManager(deps.GetCfg(), deps.GetLocator(), deps.GetGitCmdFactory(), deps.GetTxManager(), deps.GetGitlabClient(), txRegistry) + } + hookServer := NewServer(deps) for _, opt := range opts { opt(hookServer.(*server)) diff --git a/internal/gitaly/service/hook/update_test.go b/internal/gitaly/service/hook/update_test.go index 0be292b15..2eb7bfb3e 100644 --- a/internal/gitaly/service/hook/update_test.go +++ b/internal/gitaly/service/hook/update_test.go @@ -53,6 +53,7 @@ func TestUpdate_CustomHooks(t *testing.T) { }, git.UpdateHook, featureflag.FromContext(ctx), + storage.ExtractTransactionID(ctx), ).Env() require.NoError(t, err) diff --git a/internal/gitaly/service/operations/merge_branch_test.go b/internal/gitaly/service/operations/merge_branch_test.go index a74b2fafd..6bf9d04c6 100644 --- a/internal/gitaly/service/operations/merge_branch_test.go +++ b/internal/gitaly/service/operations/merge_branch_test.go @@ -20,6 +20,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/hook" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" + "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage/storagemgr" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/transaction" "gitlab.com/gitlab-org/gitaly/v16/internal/gitlab" "gitlab.com/gitlab-org/gitaly/v16/internal/grpc/backchannel" @@ -1117,7 +1118,7 @@ func testUserMergeBranchAllowed(t *testing.T, ctx context.Context) { }, gitlab.MockPreReceive, gitlab.MockPostReceive, - )) + ), hook.NewTransactionRegistry(storagemgr.NewTransactionRegistry())) ctx, cfg, client := setupOperationsServiceWithCfg( t, ctx, cfg, diff --git a/internal/gitaly/storage/storagemgr/transaction_manager.go b/internal/gitaly/storage/storagemgr/transaction_manager.go index af0174b1f..5de67ab82 100644 --- a/internal/gitaly/storage/storagemgr/transaction_manager.go +++ b/internal/gitaly/storage/storagemgr/transaction_manager.go @@ -90,9 +90,6 @@ func (index LogIndex) String() string { // ReferenceUpdate describes the state of a reference's old and new tip in an update. type ReferenceUpdate struct { - // Force indicates this is a forced reference update. If set, the reference is pointed - // to the new value regardless of the old value. - Force bool // OldOID is the old OID the reference is expected to point to prior to updating it. // If the reference does not point to the old value, the reference verification fails. OldOID git.ObjectID @@ -182,6 +179,7 @@ type Transaction struct { snapshot Snapshot skipVerificationFailures bool + initialReferenceValues map[git.ReferenceName]git.ObjectID referenceUpdates ReferenceUpdates defaultBranchUpdate *DefaultBranchUpdate customHooksUpdate *CustomHooksUpdate @@ -403,10 +401,75 @@ func (txn *Transaction) SkipVerificationFailures() { txn.skipVerificationFailures = true } -// UpdateReferences updates the given references as part of the transaction. If UpdateReferences is called -// multiple times, only the changes from the latest invocation take place. +// RecordInitialReferenceValues records the initial values of the reference if they haven't yet been recorded. If oid is +// not a zero OID, it's used as the initial value. If oid is a zero value, the reference's actual value is resolved. +// +// The reference's first recorded value is used as its old OID in the final committed update. RecordInitialReferenceValues +// can be used to record the value without staging an update in the transaction. This is useful for example generally recording +// the initial value in the 'prepare' phase of the reference transaction hook before any changes are made without staging +// any updates before the 'committed' phase is reached. +func (txn *Transaction) RecordInitialReferenceValues(ctx context.Context, initialValues map[git.ReferenceName]git.ObjectID) error { + if txn.initialReferenceValues == nil { + txn.initialReferenceValues = make(map[git.ReferenceName]git.ObjectID, len(initialValues)) + } + + for reference, oid := range initialValues { + if _, ok := txn.initialReferenceValues[reference]; ok { + // If the reference's starting value has already been recorded, we don't have to record it again. + continue + } + + objectHash, err := txn.stagingRepository.ObjectHash(ctx) + if err != nil { + return fmt.Errorf("object hash: %w", err) + } + + if objectHash.IsZeroOID(oid) { + // If this is a zero OID, resolve the value to see if this is a force update or the + // reference doesn't exist. + if current, err := txn.stagingRepository.ResolveRevision(ctx, reference.Revision()); err != nil { + if !errors.Is(err, git.ErrReferenceNotFound) { + return fmt.Errorf("resolve revision: %w", err) + } + + // The reference doesn't exist, leave the value as zero oid. + } else { + oid = current + } + } + + txn.initialReferenceValues[reference] = oid + } + + return nil +} + +// UpdateReferences updates the given references as part of the transaction. +// +// If a reference is updated multiple times during a transaction, its first recorded old OID is kept +// and the new OID is updated. This means updates like 'oid-1 -> oid-2 -> oid-3' will ultimately be +// committed as 'oid-1 -> oid-3'. The intermediate states are not relevant when committing the write +// to the actual repository. func (txn *Transaction) UpdateReferences(updates ReferenceUpdates) { - txn.referenceUpdates = updates + if txn.referenceUpdates == nil { + txn.referenceUpdates = ReferenceUpdates{} + } + + for reference, update := range updates { + oldOID := update.OldOID + if initialValue, ok := txn.initialReferenceValues[reference]; ok { + oldOID = initialValue + } + + if previousUpdate, ok := txn.referenceUpdates[reference]; ok { + oldOID = previousUpdate.OldOID + } + + txn.referenceUpdates[reference] = ReferenceUpdate{ + OldOID: oldOID, + NewOID: update.NewOID, + } + } } // DeleteRepository deletes the repository when the transaction is committed. @@ -455,9 +518,6 @@ func (txn *Transaction) walFilesPath() string { // - The reference verification failures can be ignored instead of aborting the entire transaction. // If done, the references that failed verification are dropped from the transaction but the updates // that passed verification are still performed. -// - The reference verification may also be skipped if the write is force updating references. If -// done, the current state of the references is ignored and they are directly updated to point -// to the new tips. // 2. The transaction is appended to the write-ahead log. Once the write has been logged, it is effectively // committed and will be applied to the repository even after restarting. // 3. The transaction is applied from the write-ahead log to the repository by actually performing the reference @@ -1222,29 +1282,27 @@ func (mgr *TransactionManager) verifyReferences(ctx context.Context, transaction return nil, InvalidReferenceFormatError{ReferenceName: referenceName} } - if !update.Force { - actualOldTip, err := transaction.stagingRepository.ResolveRevision(ctx, referenceName.Revision()) - if errors.Is(err, git.ErrReferenceNotFound) { - objectHash, err := transaction.stagingRepository.ObjectHash(ctx) - if err != nil { - return nil, fmt.Errorf("object hash: %w", err) - } - - actualOldTip = objectHash.ZeroOID - } else if err != nil { - return nil, fmt.Errorf("resolve revision: %w", err) + actualOldTip, err := transaction.stagingRepository.ResolveRevision(ctx, referenceName.Revision()) + if errors.Is(err, git.ErrReferenceNotFound) { + objectHash, err := transaction.stagingRepository.ObjectHash(ctx) + if err != nil { + return nil, fmt.Errorf("object hash: %w", err) } - if update.OldOID != actualOldTip { - if transaction.skipVerificationFailures { - continue - } + actualOldTip = objectHash.ZeroOID + } else if err != nil { + return nil, fmt.Errorf("resolve revision: %w", err) + } - return nil, ReferenceVerificationError{ - ReferenceName: referenceName, - ExpectedOID: update.OldOID, - ActualOID: actualOldTip, - } + if update.OldOID != actualOldTip { + if transaction.skipVerificationFailures { + continue + } + + return nil, ReferenceVerificationError{ + ReferenceName: referenceName, + ExpectedOID: update.OldOID, + ActualOID: actualOldTip, } } diff --git a/internal/gitaly/storage/storagemgr/transaction_manager_test.go b/internal/gitaly/storage/storagemgr/transaction_manager_test.go index d06af7e2c..7575861c6 100644 --- a/internal/gitaly/storage/storagemgr/transaction_manager_test.go +++ b/internal/gitaly/storage/storagemgr/transaction_manager_test.go @@ -352,6 +352,22 @@ func TestTransactionManager(t *testing.T) { ExpectedError error } + // RecordInitialReferenceValues calls RecordInitialReferenceValues on a transaction. + type RecordInitialReferenceValues struct { + // TransactionID identifies the transaction to prepare the reference updates on. + TransactionID int + // InitialValues are the initial values to record. + InitialValues map[git.ReferenceName]git.ObjectID + } + + // UpdateReferences calls UpdateReferences on a transaction. + type UpdateReferences struct { + // TransactionID identifies the transaction to update references on. + TransactionID int + // ReferenceUpdates are the reference updates to make. + ReferenceUpdates ReferenceUpdates + } + // Rollback calls Rollback on a transaction. type Rollback struct { // TransactionID identifies the transaction to rollback. @@ -1379,6 +1395,318 @@ func TestTransactionManager(t *testing.T) { }, }, { + desc: "update reference multiple times successfully in a transaction", + steps: steps{ + StartManager{}, + Begin{}, + UpdateReferences{ + ReferenceUpdates: ReferenceUpdates{ + "refs/heads/main": {OldOID: setup.ObjectHash.ZeroOID, NewOID: setup.Commits.First.OID}, + }, + }, + UpdateReferences{ + ReferenceUpdates: ReferenceUpdates{ + "refs/heads/main": {OldOID: setup.Commits.First.OID, NewOID: setup.Commits.Second.OID}, + }, + }, + Commit{}, + }, + expectedState: StateAssertion{ + Database: DatabaseState{ + string(keyAppliedLogIndex(relativePath)): LogIndex(1).toProto(), + }, + Repositories: RepositoryStates{ + relativePath: { + References: []git.Reference{{Name: "refs/heads/main", Target: setup.Commits.Second.OID.String()}}, + }, + }, + }, + }, + { + desc: "update reference multiple times fails due to wrong initial value", + steps: steps{ + StartManager{}, + Begin{}, + UpdateReferences{ + ReferenceUpdates: ReferenceUpdates{ + "refs/heads/main": {OldOID: setup.Commits.First.OID, NewOID: setup.Commits.Second.OID}, + }, + }, + UpdateReferences{ + ReferenceUpdates: ReferenceUpdates{ + // The old oid should be ignored since there's already a recorded initial value for the + // reference. + "refs/heads/main": {NewOID: setup.Commits.Third.OID}, + }, + }, + Commit{ + ExpectedError: ReferenceVerificationError{ + ReferenceName: "refs/heads/main", + ExpectedOID: setup.Commits.First.OID, + ActualOID: setup.ObjectHash.ZeroOID, + }, + }, + }, + }, + { + desc: "recording initial value of a reference stages no updates", + steps: steps{ + StartManager{}, + Begin{}, + RecordInitialReferenceValues{ + InitialValues: map[git.ReferenceName]git.ObjectID{ + "refs/heads/main": setup.Commits.First.OID, + }, + }, + Commit{}, + }, + expectedState: StateAssertion{ + Database: DatabaseState{ + string(keyAppliedLogIndex(relativePath)): LogIndex(1).toProto(), + }, + }, + }, + { + desc: "update reference with non-existent initial value", + steps: steps{ + StartManager{}, + Begin{}, + RecordInitialReferenceValues{ + InitialValues: map[git.ReferenceName]git.ObjectID{ + "refs/heads/main": setup.ObjectHash.ZeroOID, + }, + }, + UpdateReferences{ + // The old oid is ignored as the references old value was already recorded. + ReferenceUpdates: ReferenceUpdates{ + "refs/heads/main": {NewOID: setup.Commits.First.OID}, + }, + }, + Commit{}, + }, + expectedState: StateAssertion{ + Database: DatabaseState{ + string(keyAppliedLogIndex(relativePath)): LogIndex(1).toProto(), + }, + Repositories: RepositoryStates{ + relativePath: { + References: []git.Reference{{Name: "refs/heads/main", Target: setup.Commits.First.OID.String()}}, + }, + }, + }, + }, + { + desc: "update reference with the zero oid initial value", + steps: steps{ + StartManager{}, + Begin{ + TransactionID: 1, + }, + Commit{ + TransactionID: 1, + ReferenceUpdates: ReferenceUpdates{ + "refs/heads/main": {OldOID: setup.ObjectHash.ZeroOID, NewOID: setup.Commits.First.OID}, + }, + }, + Begin{ + TransactionID: 2, + ExpectedSnapshot: Snapshot{ + ReadIndex: 1, + }, + }, + RecordInitialReferenceValues{ + TransactionID: 2, + InitialValues: map[git.ReferenceName]git.ObjectID{ + "refs/heads/main": setup.ObjectHash.ZeroOID, + }, + }, + UpdateReferences{ + TransactionID: 2, + // The old oid is ignored as the references old value was already recorded. + ReferenceUpdates: ReferenceUpdates{ + "refs/heads/main": {NewOID: setup.Commits.Second.OID}, + }, + }, + Commit{ + TransactionID: 2, + }, + }, + expectedState: StateAssertion{ + Database: DatabaseState{ + string(keyAppliedLogIndex(relativePath)): LogIndex(2).toProto(), + }, + Repositories: RepositoryStates{ + relativePath: { + References: []git.Reference{{Name: "refs/heads/main", Target: setup.Commits.Second.OID.String()}}, + }, + }, + }, + }, + { + desc: "update reference with the correct initial value", + steps: steps{ + StartManager{}, + Begin{ + TransactionID: 1, + }, + Commit{ + TransactionID: 1, + ReferenceUpdates: ReferenceUpdates{ + "refs/heads/main": {OldOID: setup.ObjectHash.ZeroOID, NewOID: setup.Commits.First.OID}, + }, + }, + Begin{ + TransactionID: 2, + ExpectedSnapshot: Snapshot{ + ReadIndex: 1, + }, + }, + RecordInitialReferenceValues{ + TransactionID: 2, + InitialValues: map[git.ReferenceName]git.ObjectID{ + "refs/heads/main": setup.Commits.First.OID, + }, + }, + UpdateReferences{ + TransactionID: 2, + // The old oid is ignored as the references old value was already recorded. + ReferenceUpdates: ReferenceUpdates{ + "refs/heads/main": {NewOID: setup.Commits.Second.OID}, + }, + }, + Commit{ + TransactionID: 2, + }, + }, + expectedState: StateAssertion{ + Database: DatabaseState{ + string(keyAppliedLogIndex(relativePath)): LogIndex(2).toProto(), + }, + Repositories: RepositoryStates{ + relativePath: { + References: []git.Reference{{Name: "refs/heads/main", Target: setup.Commits.Second.OID.String()}}, + }, + }, + }, + }, + { + desc: "update reference with the incorrect initial value", + steps: steps{ + StartManager{}, + Begin{ + TransactionID: 1, + }, + Commit{ + TransactionID: 1, + ReferenceUpdates: ReferenceUpdates{ + "refs/heads/main": {OldOID: setup.ObjectHash.ZeroOID, NewOID: setup.Commits.First.OID}, + }, + }, + Begin{ + TransactionID: 2, + ExpectedSnapshot: Snapshot{ + ReadIndex: 1, + }, + }, + RecordInitialReferenceValues{ + TransactionID: 2, + InitialValues: map[git.ReferenceName]git.ObjectID{ + "refs/heads/main": setup.Commits.Third.OID, + }, + }, + UpdateReferences{ + TransactionID: 2, + // The old oid is ignored as the references old value was already recorded. + ReferenceUpdates: ReferenceUpdates{ + "refs/heads/main": {NewOID: setup.Commits.Second.OID}, + }, + }, + Commit{ + TransactionID: 2, + ExpectedError: ReferenceVerificationError{ + ReferenceName: "refs/heads/main", + ExpectedOID: setup.Commits.Third.OID, + ActualOID: setup.Commits.First.OID, + }, + }, + }, + expectedState: StateAssertion{ + Database: DatabaseState{ + string(keyAppliedLogIndex(relativePath)): LogIndex(1).toProto(), + }, + Repositories: RepositoryStates{ + relativePath: { + References: []git.Reference{{Name: "refs/heads/main", Target: setup.Commits.First.OID.String()}}, + }, + }, + }, + }, + { + desc: "initial value is only recorded on the first time", + steps: steps{ + StartManager{}, + Begin{}, + RecordInitialReferenceValues{ + InitialValues: map[git.ReferenceName]git.ObjectID{ + "refs/heads/main": setup.ObjectHash.ZeroOID, + }, + }, + RecordInitialReferenceValues{ + InitialValues: map[git.ReferenceName]git.ObjectID{ + "refs/heads/main": setup.Commits.Third.OID, + "refs/heads/branch-2": setup.ObjectHash.ZeroOID, + }, + }, + Commit{ + ReferenceUpdates: ReferenceUpdates{ + "refs/heads/main": {NewOID: setup.Commits.First.OID}, + "refs/heads/branch-2": {NewOID: setup.Commits.First.OID}, + }, + }, + }, + expectedState: StateAssertion{ + Database: DatabaseState{ + string(keyAppliedLogIndex(relativePath)): LogIndex(1).toProto(), + }, + Repositories: RepositoryStates{ + relativePath: { + References: []git.Reference{ + {Name: "refs/heads/branch-2", Target: setup.Commits.First.OID.String()}, + {Name: "refs/heads/main", Target: setup.Commits.First.OID.String()}, + }, + }, + }, + }, + }, + { + desc: "initial value is set on the first update", + steps: steps{ + StartManager{}, + Begin{}, + UpdateReferences{ + ReferenceUpdates: ReferenceUpdates{ + "refs/heads/main": {OldOID: setup.ObjectHash.ZeroOID, NewOID: setup.Commits.First.OID}, + }, + }, + RecordInitialReferenceValues{ + InitialValues: map[git.ReferenceName]git.ObjectID{ + "refs/heads/main": setup.Commits.Third.OID, + }, + }, + Commit{}, + }, + expectedState: StateAssertion{ + Database: DatabaseState{ + string(keyAppliedLogIndex(relativePath)): LogIndex(1).toProto(), + }, + Repositories: RepositoryStates{ + relativePath: { + References: []git.Reference{{Name: "refs/heads/main", Target: setup.Commits.First.OID.String()}}, + }, + }, + }, + }, + { desc: "set custom hooks successfully", steps: steps{ StartManager{}, @@ -3445,99 +3773,6 @@ func TestTransactionManager(t *testing.T) { }, }, { - desc: "forced reference creation succeeds", - steps: steps{ - StartManager{}, - Begin{}, - Commit{ - ReferenceUpdates: ReferenceUpdates{ - "refs/heads/main": {Force: true, NewOID: setup.Commits.First.OID}, - }, - }, - }, - expectedState: StateAssertion{ - Database: DatabaseState{ - string(keyAppliedLogIndex(relativePath)): LogIndex(1).toProto(), - }, - Repositories: RepositoryStates{ - relativePath: { - DefaultBranch: "refs/heads/main", - References: []git.Reference{{Name: "refs/heads/main", Target: setup.Commits.First.OID.String()}}, - }, - }, - }, - }, - { - desc: "forced reference update succeeds", - steps: steps{ - StartManager{}, - Begin{ - TransactionID: 1, - }, - Commit{ - TransactionID: 1, - ReferenceUpdates: ReferenceUpdates{ - "refs/heads/main": {OldOID: setup.ObjectHash.ZeroOID, NewOID: setup.Commits.First.OID}, - }, - }, - Begin{ - TransactionID: 2, - ExpectedSnapshot: Snapshot{ - ReadIndex: 1, - }, - }, - Commit{ - TransactionID: 2, - ReferenceUpdates: ReferenceUpdates{ - "refs/heads/main": {Force: true, NewOID: setup.Commits.Second.OID}, - }, - }, - }, - expectedState: StateAssertion{ - Database: DatabaseState{ - string(keyAppliedLogIndex(relativePath)): LogIndex(2).toProto(), - }, - Repositories: RepositoryStates{ - relativePath: { - DefaultBranch: "refs/heads/main", - References: []git.Reference{{Name: "refs/heads/main", Target: setup.Commits.Second.OID.String()}}, - }, - }, - }, - }, - { - desc: "forced reference deletion succeeds", - steps: steps{ - StartManager{}, - Begin{ - TransactionID: 1, - }, - Commit{ - TransactionID: 1, - ReferenceUpdates: ReferenceUpdates{ - "refs/heads/main": {OldOID: setup.ObjectHash.ZeroOID, NewOID: setup.Commits.First.OID}, - }, - }, - Begin{ - TransactionID: 2, - ExpectedSnapshot: Snapshot{ - ReadIndex: 1, - }, - }, - Commit{ - TransactionID: 2, - ReferenceUpdates: ReferenceUpdates{ - "refs/heads/main": {Force: true, NewOID: setup.ObjectHash.ZeroOID}, - }, - }, - }, - expectedState: StateAssertion{ - Database: DatabaseState{ - string(keyAppliedLogIndex(relativePath)): LogIndex(2).toProto(), - }, - }, - }, - { desc: "transaction rollbacked after already being rollbacked", steps: steps{ StartManager{}, @@ -3699,7 +3934,7 @@ func TestTransactionManager(t *testing.T) { Begin{}, Commit{ ReferenceUpdates: ReferenceUpdates{ - tc.referenceName: {Force: true, NewOID: setup.Commits.First.OID}, + tc.referenceName: {OldOID: setup.ObjectHash.ZeroOID, NewOID: setup.Commits.First.OID}, }, ExpectedError: InvalidReferenceFormatError{ReferenceName: tc.referenceName}, }, @@ -3989,6 +4224,16 @@ func TestTransactionManager(t *testing.T) { // determine that the deletion has actually been admitted, and is waiting for application to ensure the commit order is always // as expected by the test. <-transaction.admitted + case RecordInitialReferenceValues: + require.Contains(t, openTransactions, step.TransactionID, "test error: record initial reference value on transaction before beginning it") + + transaction := openTransactions[step.TransactionID] + require.NoError(t, transaction.RecordInitialReferenceValues(ctx, step.InitialValues)) + case UpdateReferences: + require.Contains(t, openTransactions, step.TransactionID, "test error: reference updates aborted on committed before beginning it") + + transaction := openTransactions[step.TransactionID] + transaction.UpdateReferences(step.ReferenceUpdates) case Rollback: require.Contains(t, openTransactions, step.TransactionID, "test error: transaction rollbacked before beginning it") require.Equal(t, step.ExpectedError, openTransactions[step.TransactionID].Rollback()) diff --git a/internal/gitaly/storage/storagemgr/transaction_registry.go b/internal/gitaly/storage/storagemgr/transaction_registry.go new file mode 100644 index 000000000..b11508acc --- /dev/null +++ b/internal/gitaly/storage/storagemgr/transaction_registry.go @@ -0,0 +1,51 @@ +package storagemgr + +import ( + "errors" + "sync" + + "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" +) + +var errTransactionNotFound = errors.New("transaction not found") + +// TransactionRegistry stores references to transactions by their ID. +type TransactionRegistry struct { + m sync.RWMutex + idSequence storage.TransactionID + transactions map[storage.TransactionID]*Transaction +} + +// NewTransactionRegistry returns a new TransactionRegistry. +func NewTransactionRegistry() *TransactionRegistry { + return &TransactionRegistry{ + transactions: make(map[storage.TransactionID]*Transaction), + } +} + +func (r *TransactionRegistry) register(tx *Transaction) storage.TransactionID { + r.m.Lock() + defer r.m.Unlock() + + r.idSequence++ + r.transactions[r.idSequence] = tx + return r.idSequence +} + +func (r *TransactionRegistry) unregister(id storage.TransactionID) { + r.m.Lock() + defer r.m.Unlock() + delete(r.transactions, id) +} + +// Get retrieves a transaction by its ID. An error when a transaction is not found. +func (r *TransactionRegistry) Get(id storage.TransactionID) (*Transaction, error) { + r.m.RLock() + defer r.m.RUnlock() + tx, ok := r.transactions[id] + if !ok { + return nil, errTransactionNotFound + } + + return tx, nil +} diff --git a/internal/gitaly/storage/storagemgr/transaction_registry_test.go b/internal/gitaly/storage/storagemgr/transaction_registry_test.go new file mode 100644 index 000000000..f1f0dddaa --- /dev/null +++ b/internal/gitaly/storage/storagemgr/transaction_registry_test.go @@ -0,0 +1,33 @@ +package storagemgr + +import ( + "testing" + + "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" +) + +func TestTransactionRegistry(t *testing.T) { + registry := NewTransactionRegistry() + + expectedTX1 := &Transaction{} + txID1 := registry.register(expectedTX1) + require.Equal(t, txID1, storage.TransactionID(1)) + + expectedTX2 := &Transaction{} + txID2 := registry.register(expectedTX2) + require.Equal(t, txID2, storage.TransactionID(2)) + + actualTX, err := registry.Get(txID1) + require.NoError(t, err) + require.Same(t, expectedTX1, actualTX) + + registry.unregister(txID1) + actualTX, err = registry.Get(txID1) + require.Equal(t, errTransactionNotFound, err) + require.Nil(t, actualTX) + + actualTX, err = registry.Get(txID2) + require.NoError(t, err) + require.Same(t, expectedTX2, actualTX) +} diff --git a/internal/gitaly/storage/transaction.go b/internal/gitaly/storage/transaction.go new file mode 100644 index 000000000..689b349e5 --- /dev/null +++ b/internal/gitaly/storage/transaction.go @@ -0,0 +1,27 @@ +package storage + +import ( + "context" +) + +// TransactionID is an ID that uniquely identifies a Transaction. +type TransactionID uint64 + +// keyTransactionID is the context key storing a TransactionID. +type keyTransactionID struct{} + +// ContextWithTransactionID stores the transaction id in the context. +func ContextWithTransactionID(ctx context.Context, id TransactionID) context.Context { + return context.WithValue(ctx, keyTransactionID{}, id) +} + +// ExtractTransactionID extracts the transaction ID from the context. The returned ID is zero +// if there was no transaction ID in the context. +func ExtractTransactionID(ctx context.Context) TransactionID { + value := ctx.Value(keyTransactionID{}) + if value == nil { + return 0 + } + + return value.(TransactionID) +} diff --git a/internal/gitaly/storage/transaction_test.go b/internal/gitaly/storage/transaction_test.go new file mode 100644 index 000000000..9d3c5963c --- /dev/null +++ b/internal/gitaly/storage/transaction_test.go @@ -0,0 +1,26 @@ +package storage + +import ( + "testing" + + "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper" +) + +func TestContextWithTransactionID(t *testing.T) { + t.Run("no transaction id in context", func(t *testing.T) { + require.Equal(t, + TransactionID(0), + ExtractTransactionID(testhelper.Context(t)), + ) + }) + + t.Run("transaction id in context", func(t *testing.T) { + require.Equal(t, + TransactionID(1), + ExtractTransactionID( + ContextWithTransactionID(testhelper.Context(t), 1), + ), + ) + }) +} diff --git a/internal/testhelper/testserver/gitaly.go b/internal/testhelper/testserver/gitaly.go index 2875d2e1d..e7b2659e7 100644 --- a/internal/testhelper/testserver/gitaly.go +++ b/internal/testhelper/testserver/gitaly.go @@ -303,8 +303,9 @@ func (gsd *gitalyServerDeps) createDependencies(tb testing.TB, cfg config.Cfg) * gsd.gitCmdFactory = gittest.NewCommandFactory(tb, cfg) } + transactionRegistry := storagemgr.NewTransactionRegistry() if gsd.hookMgr == nil { - gsd.hookMgr = hook.NewManager(cfg, gsd.locator, gsd.gitCmdFactory, gsd.txMgr, gsd.gitlabClient) + gsd.hookMgr = hook.NewManager(cfg, gsd.locator, gsd.gitCmdFactory, gsd.txMgr, gsd.gitlabClient, hook.NewTransactionRegistry(transactionRegistry)) } if gsd.catfileCache == nil { @@ -382,6 +383,7 @@ func (gsd *gitalyServerDeps) createDependencies(tb testing.TB, cfg config.Cfg) * RepositoryCounter: gsd.repositoryCounter, UpdaterWithHooks: gsd.updaterWithHooks, HousekeepingManager: gsd.housekeepingManager, + TransactionRegistry: transactionRegistry, PartitionManager: partitionManager, BackupSink: gsd.backupSink, BackupLocator: gsd.backupLocator, |