From ea0e36b456646d87e88b3b966aeacdd48e71affe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 17 Dec 2020 15:39:25 +0100 Subject: [PATCH 1/2] updater: optionally fake the oldvalue for testing This API allows us to easily inject fake oldvalues into update-ref, the purpose of which is to easily test ref race conditions without carefully needing to orchestrate race conditions. Patrick suggested I could use something like testhelper.CaptureHookEnv() for this. For update-ref we'd need to have that more complex since it would need to munge stdin, but more specifically we'd need to get some out-of-bounds message to it (via a file) to ask it to map key->value. Not such a big deal now, we could write this in $PATH: #!/bin/sh sed 's/EXPECTED/CHANGED/' | git update-ref "$@" But once we're going to tread update-ref like "cat-file --batch" (via the transactions it supports) that becomes painful. I.e. a follow-up to 154dbc1a2 (updateref: Safeguard against accidentally committing updates, 2020-12-15) with the new interface in Git 2.30. So I think doing it this way sucks the least, it's obvious from the variable name that you shouldn't use it for testing, and we could die on some AreWeTesting() I don't know about in fakeOldValueForTesting(). The performance hit of checking a hash map for whether it has any keys is trivial. --- internal/git/updateref/updateref.go | 29 ++++++++++++- internal/git/updateref/updateref_test.go | 54 ++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 2 deletions(-) diff --git a/internal/git/updateref/updateref.go b/internal/git/updateref/updateref.go index 6880429254e..4ce9d0c8a83 100644 --- a/internal/git/updateref/updateref.go +++ b/internal/git/updateref/updateref.go @@ -77,10 +77,35 @@ func (u *Updater) Create(ref, value string) error { return err } +var mungeMapForTesting = make(map[string]string) + +// MungeMapForTestingAdd is a test-only interface to the private +// mungeMapForTesting. Use it for ad-hoc mapping the Update() +// "oldvalue" to some fake value. The munged value will only be used +// once, it's delete()-ed on retrieval. +func MungeMapForTestingAdd(key, value string) { + mungeMapForTesting[key] = value +} + +func fakeOldValueForTesting(oldValue string) string { + fakeOldValue := mungeMapForTesting[oldValue] + if len(fakeOldValue) != 0 { + // Don't persist the value, it's one-use. If we had a + // Del() interface and the test died and forgot to + // cleanup later tests might die at a distance. + delete(mungeMapForTesting, oldValue) + return fakeOldValue + } + return oldValue +} + // Update commands the reference to be updated to point at the sha specified in // newvalue -func (u *Updater) Update(ref, newvalue, oldvalue string) error { - _, err := fmt.Fprintf(u.cmd, "update %s\x00%s\x00%s\x00", ref, newvalue, oldvalue) +func (u *Updater) Update(ref, newvalue, oldValue string) error { + if len(mungeMapForTesting) != 0 { + oldValue = fakeOldValueForTesting(oldValue) + } + _, err := fmt.Fprintf(u.cmd, "update %s\x00%s\x00%s\x00", ref, newvalue, oldValue) return err } diff --git a/internal/git/updateref/updateref_test.go b/internal/git/updateref/updateref_test.go index 905b9e47004..1586b1ece34 100644 --- a/internal/git/updateref/updateref_test.go +++ b/internal/git/updateref/updateref_test.go @@ -99,6 +99,60 @@ func TestUpdate(t *testing.T) { require.NotEqual(t, commit.Id, parentCommit.Id, "reference was updated when it shouldn't have been") } +func TestUpdateFake(t *testing.T) { + ctx, testRepo, _, teardown := setup(t) + defer teardown() + + locator := config.NewLocator(config.Config) + masterCommit, err := log.GetCommit(ctx, locator, testRepo, "refs/heads/master") + require.NoError(t, err) + + updater, err := New(ctx, testRepo) + require.NoError(t, err) + + ref := "refs/heads/fix" + + // Sanity check: ensure the ref exists before we start + fixCommit, logErr := log.GetCommit(ctx, locator, testRepo, ref) + require.NoError(t, logErr) + require.NotEqual(t, masterCommit.Id, fixCommit.Id, "%s points to %s in the test repository", ref, fixCommit.Id) + + // Updating it from fix->master with a bad oldvalue fails + updateErr := updater.Update(ref, masterCommit.Id, masterCommit.Id) + require.NoError(t, updateErr) + require.Error(t, updater.Wait()) // the error is deferred + updater, err = New(ctx, testRepo) // .Wait() closes it, create a new one + require.NoError(t, err) + + // check that the ref was not updated + fixCommitTwo, logErr := log.GetCommit(ctx, locator, testRepo, ref) + require.NoError(t, logErr) + require.Equal(t, fixCommit.Id, fixCommitTwo.Id) + + // Now fake up the oldvalue + MungeMapForTestingAdd(masterCommit.Id, fixCommit.Id) + + // Updating it from fix->master with a bad oldvalue works, + // since we're lying via MungeMapForTesting + updateErr = updater.Update(ref, masterCommit.Id, masterCommit.Id) + require.NoError(t, updateErr) + require.NoError(t, updater.Wait()) // the error is deferred + updater, err = New(ctx, testRepo) // .Wait() closes it, create a new one + require.NoError(t, err) + + // check that the ref was updated + fixCommitThree, logErr := log.GetCommit(ctx, locator, testRepo, ref) + require.NoError(t, logErr) + require.Equal(t, masterCommit.Id, fixCommitThree.Id) + + // Updating it with masterCommit.Id does not error, since the + // MungeMapForTesting is one-use (it deleted the key after it + // was used) (it would if Del() wasn't called) + updateErr = updater.Update(ref, fixCommit.Id, masterCommit.Id) + require.NoError(t, updateErr) + require.NoError(t, updater.Wait()) // the error is deferred +} + func TestDelete(t *testing.T) { ctx, testRepo, _, teardown := setup(t) defer teardown() -- GitLab From b999292653176613f76afbe66e829a2f3341b5a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 17 Dec 2020 16:44:32 +0100 Subject: [PATCH 2/2] UserCreateTag tests: use new update-ref mock to test update-ref error This fixes a TODO item in be093ba33 (User{Branch,Tag,Submodule}: ferry update-ref errors upwards, 2020-12-14) for UserDeleteTag, we now know what response we return for update-ref failures. I'm also adding tests for the new UserCreateTag. This is a Go-only test, since in the Ruby codepath we're using libgit2. I don't think we're going to realistically have both Go & Ruby tests for features using this deep-gutsy fakery, but at least here we can see what we'd do in this scenario. --- .../gitaly/service/operations/tags_test.go | 78 +++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/internal/gitaly/service/operations/tags_test.go b/internal/gitaly/service/operations/tags_test.go index 9202af69192..fe9da56bfa0 100644 --- a/internal/gitaly/service/operations/tags_test.go +++ b/internal/gitaly/service/operations/tags_test.go @@ -9,7 +9,9 @@ import ( "testing" "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/git/log" + "gitlab.com/gitlab-org/gitaly/internal/git/updateref" "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/internal/helper/text" "gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag" @@ -1253,6 +1255,82 @@ func testFailedUserCreateTagRequestDueToValidation(t *testing.T, ctx context.Con } } +//nolint: errcheck +func TestFailedUserDeleteTagRequestDueToUpdateRef(t *testing.T) { + ctx, cancel := testhelper.Context() + defer cancel() + + ctx = featureflag.OutgoingCtxWithFeatureFlagValue(ctx, featureflag.GoUserDeleteTag, "true") + + serverSocketPath, stop := runOperationServiceServer(t) + defer stop() + + client, conn := newOperationClient(t, serverSocketPath) + defer conn.Close() + + testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t) + defer cleanupFn() + + tagName := "my-new-tag" + targetRevision := "c7fbe50c7c7419d9701eebe64b1fdacc3df5b9dd" + testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "tag", tagName, targetRevision) + + tagID := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "rev-parse", tagName) + require.Equal(t, targetRevision, text.ChompBytes(tagID)) + + updateref.MungeMapForTestingAdd(targetRevision, "612036fac47c5d31c212b17268e2f3ba807bce1e") + request := &gitalypb.UserDeleteTagRequest{ + Repository: testRepo, + TagName: []byte(tagName), + User: testhelper.TestUser, + } + response, err := client.UserDeleteTag(ctx, request) + require.Equal(t, status.Errorf(codes.FailedPrecondition, "Could not update refs/tags/%s. Please refresh and try again.", tagName), err) + require.Nil(t, response) + + tags := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "for-each-ref", "--", "refs/tags/"+tagName) + require.Contains(t, string(tags), targetRevision) +} + +//nolint: errcheck +func TestFailedUserCreateTagRequestDueToUpdateRef(t *testing.T) { + ctx, cancel := testhelper.Context() + defer cancel() + + ctx = featureflag.OutgoingCtxWithFeatureFlagValue(ctx, featureflag.GoUserCreateTag, "true") + + serverSocketPath, stop := runOperationServiceServer(t) + defer stop() + + client, conn := newOperationClient(t, serverSocketPath) + defer conn.Close() + + testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t) + defer cleanupFn() + + targetRevision := "c7fbe50c7c7419d9701eebe64b1fdacc3df5b9dd" + updateref.MungeMapForTestingAdd(git.NullSHA, "612036fac47c5d31c212b17268e2f3ba807bce1e") + + tagName := "my-new-tag" + request := &gitalypb.UserCreateTagRequest{ + Repository: testRepo, + TagName: []byte(tagName), + TargetRevision: []byte(targetRevision), + User: testhelper.TestUser, + } + response, err := client.UserCreateTag(ctx, request) + require.NoError(t, err) + require.Empty(t, response.PreReceiveError) + responseOk := &gitalypb.UserCreateTagResponse{ + Tag: nil, + Exists: true, + } + require.Equal(t, responseOk, response) + + tags := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "for-each-ref", "--", "refs/tags/"+tagName) + require.Equal(t, string(tags), "") +} + func TestTagHookOutput(t *testing.T) { testhelper.NewFeatureSets([]featureflag.FeatureFlag{ featureflag.GoUserDeleteTag, -- GitLab