diff options
author | Patrick Steinhardt <psteinhardt@gitlab.com> | 2022-07-13 10:01:34 +0300 |
---|---|---|
committer | Patrick Steinhardt <psteinhardt@gitlab.com> | 2022-07-13 14:41:16 +0300 |
commit | 827d9ec8fcfc4282995d809efe28047bae237db9 (patch) | |
tree | 9d618161e84a0ddb6dd1b7fdaf75e7a9b50d73c1 | |
parent | e97000ef789ac963308f2e61b5c4d14c2f556be7 (diff) |
housekeeping: Convert function to check for pool repos to be typesafe
The `IsPoolPath()` function can be used to check whether a given path
belongs to a pool repository or not. It is not quite clear though what
kind of parameter the function expects: is it a relative path, or is it
the full path to the object pool? This is easy to get wrong and may have
fatal consequences.
Change the interface to instead accept a `repository.GitRepo` to clarify
this.
-rw-r--r-- | internal/git/housekeeping/object_pool.go | 14 | ||||
-rw-r--r-- | internal/git/housekeeping/object_pool_test.go | 51 | ||||
-rw-r--r-- | internal/git/housekeeping/optimize_repository.go | 2 | ||||
-rw-r--r-- | internal/git/objectpool/pool.go | 8 | ||||
-rw-r--r-- | internal/praefect/delete_object_pool.go | 2 | ||||
-rw-r--r-- | internal/praefect/praefectutil/replica_path.go | 8 | ||||
-rw-r--r-- | internal/praefect/praefectutil/replica_path_test.go | 42 | ||||
-rw-r--r-- | internal/praefect/router_per_repository.go | 6 |
8 files changed, 86 insertions, 47 deletions
diff --git a/internal/git/housekeeping/object_pool.go b/internal/git/housekeeping/object_pool.go index 6af1a9973..732045e09 100644 --- a/internal/git/housekeeping/object_pool.go +++ b/internal/git/housekeeping/object_pool.go @@ -4,15 +4,16 @@ import ( "regexp" "strings" + "gitlab.com/gitlab-org/gitaly/v15/internal/git/repository" "gitlab.com/gitlab-org/gitaly/v15/internal/praefect/praefectutil" ) // railsPoolDirRegexp is used to validate object pool directory structure and name as generated by Rails. var railsPoolDirRegexp = regexp.MustCompile(`^@pools/([0-9a-f]{2})/([0-9a-f]{2})/([0-9a-f]{64})\.git$`) -// IsRailsPoolPath returns whether the relative path indicates this is a pool path generated by Rails. -func IsRailsPoolPath(relativePath string) bool { - matches := railsPoolDirRegexp.FindStringSubmatch(relativePath) +// IsRailsPoolRepository returns whether the repository is a pool repository generated by Rails. +func IsRailsPoolRepository(repo repository.GitRepo) bool { + matches := railsPoolDirRegexp.FindStringSubmatch(repo.GetRelativePath()) if matches == nil || !strings.HasPrefix(matches[3], matches[1]+matches[2]) { return false } @@ -20,8 +21,7 @@ func IsRailsPoolPath(relativePath string) bool { return true } -// IsPoolPath returns whether the relative path indicates the repository is an object -// pool. -func IsPoolPath(relativePath string) bool { - return IsRailsPoolPath(relativePath) || praefectutil.IsPoolPath(relativePath) +// IsPoolRepository returns whether the repository is an object pool. +func IsPoolRepository(repo repository.GitRepo) bool { + return IsRailsPoolRepository(repo) || praefectutil.IsPoolRepository(repo) } diff --git a/internal/git/housekeeping/object_pool_test.go b/internal/git/housekeeping/object_pool_test.go index 7c0bf5422..daf134505 100644 --- a/internal/git/housekeeping/object_pool_test.go +++ b/internal/git/housekeeping/object_pool_test.go @@ -6,42 +6,57 @@ import ( "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest" "gitlab.com/gitlab-org/gitaly/v15/internal/praefect/praefectutil" + "gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb" ) -func TestIsPoolPath(t *testing.T) { +func TestIsPoolRepository(t *testing.T) { for _, tc := range []struct { - desc string - relativePath string - isPoolPath bool + desc string + repo *gitalypb.Repository + isPoolPath bool }{ { - desc: "rails pool directory", - relativePath: gittest.NewObjectPoolName(t), - isPoolPath: true, + desc: "rails pool directory", + repo: &gitalypb.Repository{ + RelativePath: gittest.NewObjectPoolName(t), + }, + isPoolPath: true, }, { - desc: "praefect pool path", - relativePath: praefectutil.DerivePoolPath(1), - isPoolPath: true, + desc: "praefect pool path", + repo: &gitalypb.Repository{ + RelativePath: praefectutil.DerivePoolPath(1), + }, + isPoolPath: true, }, { - desc: "praefect replica path", - relativePath: praefectutil.DeriveReplicaPath(1), + desc: "praefect replica path", + repo: &gitalypb.Repository{ + RelativePath: praefectutil.DeriveReplicaPath(1), + }, }, { - desc: "empty string", + desc: "missing repository", }, { - desc: "rails path first to subdirs dont match full hash", - relativePath: "@pools/aa/bb/ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff.git", + desc: "empty repository", + repo: &gitalypb.Repository{}, }, { - desc: "normal repos dont match", - relativePath: "@hashed/" + gittest.NewRepositoryName(t, true), + desc: "rails path first to subdirs dont match full hash", + repo: &gitalypb.Repository{ + RelativePath: "@pools/aa/bb/ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff.git", + }, + }, + { + desc: "normal repos dont match", + repo: &gitalypb.Repository{ + RelativePath: "@hashed/" + gittest.NewRepositoryName(t, true), + }, }, } { t.Run(tc.desc, func(t *testing.T) { - require.Equal(t, tc.isPoolPath, IsPoolPath(tc.relativePath)) + require.Equal(t, tc.isPoolPath, IsPoolRepository(tc.repo)) }) } } diff --git a/internal/git/housekeeping/optimize_repository.go b/internal/git/housekeeping/optimize_repository.go index 0d79db70e..eedc1571f 100644 --- a/internal/git/housekeeping/optimize_repository.go +++ b/internal/git/housekeeping/optimize_repository.go @@ -359,7 +359,7 @@ func estimateLooseObjectCount(repo *localrepo.Repo, cutoffDate time.Time) (int64 func pruneIfNeeded(ctx context.Context, repo *localrepo.Repo) (bool, error) { // Pool repositories must never prune any objects, or otherwise we may corrupt members of // that pool if they still refer to that object. - if IsPoolPath(repo.GetRelativePath()) { + if IsPoolRepository(repo) { return false, nil } diff --git a/internal/git/objectpool/pool.go b/internal/git/objectpool/pool.go index 1a454e67e..d1676c2af 100644 --- a/internal/git/objectpool/pool.go +++ b/internal/git/objectpool/pool.go @@ -57,10 +57,6 @@ func NewObjectPool( return nil, err } - if !housekeeping.IsPoolPath(relativePath) { - return nil, ErrInvalidPoolDir - } - pool := &ObjectPool{ gitCmdFactory: gitCmdFactory, txManager: txManager, @@ -71,6 +67,10 @@ func NewObjectPool( } pool.Repo = localrepo.New(locator, gitCmdFactory, catfileCache, pool) + if !housekeeping.IsPoolRepository(pool) { + return nil, ErrInvalidPoolDir + } + return pool, nil } diff --git a/internal/praefect/delete_object_pool.go b/internal/praefect/delete_object_pool.go index 83d8ab9dd..c10449d42 100644 --- a/internal/praefect/delete_object_pool.go +++ b/internal/praefect/delete_object_pool.go @@ -29,7 +29,7 @@ func DeleteObjectPoolHandler(rs datastore.RepositoryStore, conns Connections) gr return nil, err } - if !housekeeping.IsRailsPoolPath(repo.GetRelativePath()) { + if !housekeeping.IsRailsPoolRepository(repo) { return nil, helper.ErrInvalidArgument(objectpool.ErrInvalidPoolDir) } diff --git a/internal/praefect/praefectutil/replica_path.go b/internal/praefect/praefectutil/replica_path.go index 576762d22..1bf34b33b 100644 --- a/internal/praefect/praefectutil/replica_path.go +++ b/internal/praefect/praefectutil/replica_path.go @@ -6,14 +6,16 @@ import ( "path/filepath" "strconv" "strings" + + "gitlab.com/gitlab-org/gitaly/v15/internal/git/repository" ) // poolPathPrefix is the prefix directory where Praefect places object pools. const poolPathPrefix = "@cluster/pools/" -// IsPoolPath returns whether the relative path indicates this is a Praefect generated object pool path. -func IsPoolPath(relativePath string) bool { - return strings.HasPrefix(relativePath, poolPathPrefix) +// IsPoolRepository returns whether the repository is a Praefect generated object pool repository. +func IsPoolRepository(repo repository.GitRepo) bool { + return strings.HasPrefix(repo.GetRelativePath(), poolPathPrefix) } // DeriveReplicaPath derives a repository's disk storage path from its repository ID. The repository ID diff --git a/internal/praefect/praefectutil/replica_path_test.go b/internal/praefect/praefectutil/replica_path_test.go index be9f42e24..d168778c1 100644 --- a/internal/praefect/praefectutil/replica_path_test.go +++ b/internal/praefect/praefectutil/replica_path_test.go @@ -5,6 +5,7 @@ import ( "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest" + "gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb" ) func TestDeriveReplicaPath(t *testing.T) { @@ -17,28 +18,45 @@ func TestDerivePoolPath(t *testing.T) { require.Equal(t, "@cluster/pools/d4/73/2", DerivePoolPath(2)) } -func TestIsPoolPath(t *testing.T) { +func TestIsPoolRepository(t *testing.T) { for _, tc := range []struct { - desc string - relativePath string - isPoolPath bool + desc string + repo *gitalypb.Repository + isPoolPath bool }{ { - desc: "praefect pool path", - relativePath: DerivePoolPath(1), - isPoolPath: true, + desc: "missing repository", + isPoolPath: false, }, { - desc: "praefect replica path", - relativePath: DeriveReplicaPath(1), + desc: "empty string", + repo: &gitalypb.Repository{ + RelativePath: "", + }, + isPoolPath: false, }, { - desc: "rails pool path", - relativePath: gittest.NewObjectPoolName(t), + desc: "praefect pool path", + repo: &gitalypb.Repository{ + RelativePath: DerivePoolPath(1), + }, + isPoolPath: true, + }, + { + desc: "praefect replica path", + repo: &gitalypb.Repository{ + RelativePath: DeriveReplicaPath(1), + }, + }, + { + desc: "rails pool path", + repo: &gitalypb.Repository{ + RelativePath: gittest.NewObjectPoolName(t), + }, }, } { t.Run(tc.desc, func(t *testing.T) { - require.Equal(t, tc.isPoolPath, IsPoolPath(tc.relativePath)) + require.Equal(t, tc.isPoolPath, IsPoolRepository(tc.repo)) }) } } diff --git a/internal/praefect/router_per_repository.go b/internal/praefect/router_per_repository.go index ea3d60118..287d8d33b 100644 --- a/internal/praefect/router_per_repository.go +++ b/internal/praefect/router_per_repository.go @@ -10,6 +10,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/praefect/datastore" "gitlab.com/gitlab-org/gitaly/v15/internal/praefect/nodes" "gitlab.com/gitlab-org/gitaly/v15/internal/praefect/praefectutil" + "gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb" "google.golang.org/grpc" ) @@ -313,7 +314,10 @@ func (r *PerRepositoryRouter) RouteRepositoryCreation(ctx context.Context, virtu replicaPath := relativePath if featureflag.PraefectGeneratedReplicaPaths.IsEnabled(ctx) { replicaPath = praefectutil.DeriveReplicaPath(id) - if housekeeping.IsRailsPoolPath(relativePath) { + if housekeeping.IsRailsPoolRepository(&gitalypb.Repository{ + StorageName: virtualStorage, + RelativePath: relativePath, + }) { replicaPath = praefectutil.DerivePoolPath(id) } } |