diff --git a/config/config.go b/config/config.go index 6033ce09..96a56530 100644 --- a/config/config.go +++ b/config/config.go @@ -16,8 +16,12 @@ package config import ( "bytes" + "errors" "fmt" + neturl "net/url" "os" + "path/filepath" + "strings" yaml "github.com/goccy/go-yaml" ) @@ -96,8 +100,8 @@ func ParseBytes(yamlBytes []byte) (*Config, error) { config.repositoryByRemote = make(map[string]*RepositoryConfig, len(config.Repository)) for i := range config.Repository { remote := config.Repository[i].Remote - if remote == "" { - return nil, fmt.Errorf("repository[%d].remote must not be empty", i) + if err := validateRepositoryRemote(remote); err != nil { + return nil, fmt.Errorf("repository[%d].remote: %w", i, err) } if _, exists := config.repositoryByRemote[remote]; exists { return nil, fmt.Errorf("duplicate repository remote %q", remote) @@ -113,3 +117,87 @@ func ParseBytes(yamlBytes []byte) (*Config, error) { } return &config, nil } + +func validateRepositoryRemote(remote string) error { + safeRemote := redactRepositoryPassword(remote) + if remote == "" { + return fmt.Errorf("remote %q must not be empty", safeRemote) + } + if strings.TrimSpace(remote) != remote { + return fmt.Errorf("remote %q must not contain leading or trailing whitespace", safeRemote) + } + if strings.ContainsAny(remote, "\x00\r\n") { + return fmt.Errorf("remote %q must not contain control characters", safeRemote) + } + if !strings.Contains(remote, "://") && !strings.HasPrefix(strings.ToLower(remote), "file:") { + if filepath.IsAbs(remote) || isSCPLikeRemote(remote) { + return nil + } + return fmt.Errorf("relative local remote %q is not allowed", safeRemote) + } + + parsed, err := neturl.Parse(remote) + if err != nil { + var urlError *neturl.Error + if errors.As(err, &urlError) { + err = urlError.Err + } + return fmt.Errorf("invalid repository URL %q: %v", safeRemote, err) + } + safeRemote = parsed.Redacted() + scheme := strings.ToLower(parsed.Scheme) + if parsed.User != nil { + _, hasPassword := parsed.User.Password() + if hasPassword { + return fmt.Errorf("repository URL %q must not contain a password", safeRemote) + } + if scheme == "http" || scheme == "https" { + return fmt.Errorf("repository URL %q must not contain HTTP(S) userinfo", safeRemote) + } + } + filePath := parsed.Path + if filePath == "" { + filePath = parsed.Opaque + } + if scheme == "file" && !filepath.IsAbs(filePath) { + return fmt.Errorf("file repository URL %q must contain an absolute path", safeRemote) + } + if parsed.RawQuery != "" { + return fmt.Errorf("repository URL %q must not contain query %q", safeRemote, parsed.RawQuery) + } + if parsed.Fragment != "" { + return fmt.Errorf("repository URL %q must not contain fragment %q", safeRemote, parsed.Fragment) + } + return nil +} + +func redactRepositoryPassword(remote string) string { + schemeEnd := strings.Index(remote, "://") + if schemeEnd < 0 { + return remote + } + authorityStart := schemeEnd + len("://") + authorityEnd := len(remote) + if offset := strings.IndexAny(remote[authorityStart:], "/?#"); offset >= 0 { + authorityEnd = authorityStart + offset + } + authority := remote[authorityStart:authorityEnd] + userinfoEnd := strings.LastIndexByte(authority, '@') + if userinfoEnd < 0 { + return remote + } + passwordStart := strings.IndexByte(authority[:userinfoEnd], ':') + if passwordStart < 0 { + return remote + } + passwordStart += authorityStart + userinfoEnd += authorityStart + return remote[:passwordStart+1] + "xxxxx" + remote[userinfoEnd:] +} + +func isSCPLikeRemote(remote string) bool { + colon := strings.IndexByte(remote, ':') + return colon > 0 && + colon < len(remote)-1 && + !strings.ContainsAny(remote[:colon], `/\`) +} diff --git a/config/config_test.go b/config/config_test.go index 580d6a17..1a32cba4 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -15,6 +15,7 @@ package config import ( + "fmt" "os" "path/filepath" "testing" @@ -208,6 +209,73 @@ service: require.Error(t, err) } +func TestParseBytes_InvalidRemoteRejected(t *testing.T) { + tests := []struct { + name string + remote string + wantError string + forbidInError string + }{ + {name: "leading whitespace", remote: " https://example.com/repo.git"}, + {name: "trailing whitespace", remote: "https://example.com/repo.git "}, + {name: "newline", remote: "https://example.com/repo.git\nother"}, + {name: "http userinfo", remote: "https://token@example.com/repo.git", wantError: "token"}, + {name: "password", remote: "ssh://git:secret@example.com/org/repo.git", wantError: "example.com/org/repo.git", forbidInError: "secret"}, + {name: "malformed URL with password", remote: "https://git:secret%zz@example.com/repo.git", wantError: "example.com/repo.git", forbidInError: "secret"}, + {name: "query", remote: "https://example.com/repo.git?token=secret", wantError: "token=secret"}, + {name: "fragment", remote: "https://example.com/repo.git#main", wantError: "main"}, + {name: "relative local path", remote: "../../outside", wantError: "../../outside"}, + {name: "relative file URL", remote: "file:../outside", wantError: "../outside"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + yamlStr := fmt.Sprintf(` +repository: + - remote: %q +service: + max_worker_pool_size: 1 + workspaces_root_path: "/tmp/tango-repo-manager" +`, tt.remote) + _, err := ParseBytes([]byte(yamlStr)) + require.Error(t, err) + if tt.wantError != "" { + assert.Contains(t, err.Error(), tt.wantError) + } + if tt.forbidInError != "" { + assert.NotContains(t, err.Error(), tt.forbidInError) + } + }) + } +} + +func TestParseBytes_ExactRemoteValuesRemainDistinct(t *testing.T) { + yamlStr := ` +repository: + - remote: "git@github.com:org/repo.git" + - remote: "ssh://git@github.com:22/org/repo" +service: + max_worker_pool_size: 1 + workspaces_root_path: "/tmp/tango-repo-manager" +` + cfg, err := ParseBytes([]byte(yamlStr)) + require.NoError(t, err) + assert.Len(t, cfg.Repository, 2) +} + +func TestParseBytes_AbsoluteLocalRemoteAccepted(t *testing.T) { + yamlStr := ` +repository: + - remote: "/var/lib/tango/repo" + - remote: "file:///var/lib/tango/other-repo" +service: + max_worker_pool_size: 1 + workspaces_root_path: "/tmp/tango-repo-manager" +` + cfg, err := ParseBytes([]byte(yamlStr)) + require.NoError(t, err) + assert.Len(t, cfg.Repository, 2) +} + func TestParseBytes_DuplicateRemoteRejected(t *testing.T) { yamlStr := ` repository: diff --git a/config/repository_config.go b/config/repository_config.go index e84e274e..5a691d93 100644 --- a/config/repository_config.go +++ b/config/repository_config.go @@ -27,7 +27,9 @@ type RepositoryConfig struct { // Remote is the URL used to `git clone` the repository. Tango clones from // this URL and uses it as the lookup key for per-repo settings. Must be // unique across all entries and match exactly what clients send in - // BuildDescription.remote. + // BuildDescription.remote. Local paths must be absolute. HTTP(S) + // credentials, URL queries, fragments, surrounding whitespace, and control + // characters are rejected. Remote string `yaml:"remote"` // TODO: FullHashRepos, ExcludedFiles, and StreamBazelLogs are not // documented in config/README.md. Delete them if they turn out to be diff --git a/controller/controller.go b/controller/controller.go index cdb73645..a8f162bf 100644 --- a/controller/controller.go +++ b/controller/controller.go @@ -20,7 +20,9 @@ import ( "github.com/uber-go/tally" "github.com/uber/tango/config" + tangoerrors "github.com/uber/tango/core/errors" "github.com/uber/tango/core/storage" + "github.com/uber/tango/internal/url" "github.com/uber/tango/observability/metrics" "github.com/uber/tango/orchestrator" pb "github.com/uber/tango/tangopb" @@ -28,15 +30,19 @@ import ( "go.uber.org/zap" ) +const unknownRepositoryMetricLabel = "unknown" + // Params are the parameters for the controller. type Params struct { fx.In Logger *zap.Logger Storage storage.Storage Orchestrator orchestrator.Orchestrator - Scope tally.Scope `optional:"true"` - MaxMessageBytes int `optional:"true"` - RepoConfig config.RepositoryConfigProvider `optional:"true"` + Scope tally.Scope `optional:"true"` + MaxMessageBytes int `optional:"true"` + // RepoConfig, when set, is the authoritative repository allowlist. RPC + // remotes must match it exactly before the controller performs cache I/O. + RepoConfig config.RepositoryConfigProvider `optional:"true"` // GraphFormat mirrors ServiceConfig.GraphFormat; empty defaults to gob. // It must match the orchestrator's configured format — both are wired // from the same ServiceConfig. @@ -47,6 +53,22 @@ type Params struct { ShadowCompare bool `optional:"true"` } +// resolveRepository verifies that a request remote exactly matches service +// configuration before the controller performs cache I/O. A nil provider is +// supported for custom orchestrators that do not use Tango repository config. +func (c *controller) resolveRepository(remote string) (string, error) { + if c.repoConfig == nil { + return url.RepositoryMetricLabel(remote), nil + } + repo, ok := c.repoConfig.GetRepositoryConfig(remote) + if !ok { + return unknownRepositoryMetricLabel, tangoerrors.NewUser( + errors.New("repository is not configured"), + ) + } + return url.RepositoryMetricLabel(repo.Remote), nil +} + type controller struct { logger *zap.Logger storage storage.Storage diff --git a/controller/getchangedtargets.go b/controller/getchangedtargets.go index cefe698e..fc7be324 100644 --- a/controller/getchangedtargets.go +++ b/controller/getchangedtargets.go @@ -32,7 +32,6 @@ import ( "github.com/uber/tango/internal/targetdiff" "github.com/uber/tango/internal/tgb" "github.com/uber/tango/internal/tgbdiff" - "github.com/uber/tango/internal/url" "github.com/uber/tango/observability/metrics" pb "github.com/uber/tango/tangopb" "go.uber.org/zap" @@ -82,12 +81,16 @@ type job struct { // client disconnects, the stream's context is cancelled and the function // returns with context.Canceled. func (c *controller) GetChangedTargets(request *pb.GetChangedTargetsRequest, stream pb.TangoServiceGetChangedTargetsYARPCServer) (retErr error) { - repo := url.ToShortRemote(request.GetFirstRevision().GetRemote()) + validationErr := validateGetChangedTargetsRequest(request) + repo := unknownRepositoryMetricLabel + var repositoryErr error + if validationErr == nil { + repo, repositoryErr = c.resolveRepository(request.GetFirstRevision().GetRemote()) + } e := c.emitter.Tagged(map[string]string{metrics.TagRepo: repo}) op := metrics.Begin(e, opGetChangedTargets, metrics.SlowDurationBuckets) logger := c.logger.WithLazy( - zap.Any("first_revision", request.GetFirstRevision()), - zap.Any("second_revision", request.GetSecondRevision()), + zap.String("repository", repo), ) defer func() { op.Complete(retErr) @@ -96,8 +99,11 @@ func (c *controller) GetChangedTargets(request *pb.GetChangedTargetsRequest, str retErr = toWireError(retErr) } }() - if err := validateGetChangedTargetsRequest(request); err != nil { - return tangoerrors.NewUser(err) + if validationErr != nil { + return tangoerrors.NewUser(validationErr) + } + if repositoryErr != nil { + return repositoryErr } ctx, cancelLink := c.linkRequestCtx(stream.Context()) defer cancelLink() diff --git a/controller/getchangedtargets_test.go b/controller/getchangedtargets_test.go index 19c74108..6237050a 100644 --- a/controller/getchangedtargets_test.go +++ b/controller/getchangedtargets_test.go @@ -309,6 +309,38 @@ func TestGetChangedTargets_ValidationError(t *testing.T) { require.Error(t, err) } +func TestGetChangedTargets_UnconfiguredRemoteRejectedBeforeCacheLookup(t *testing.T) { + ctrl := gomock.NewController(t) + stream := tangomock.NewMockTangoServiceGetChangedTargetsYARPCServer(ctrl) + store := storagemock.NewMockStorage(ctrl) + orch := orchestratormock.NewMockOrchestrator(ctrl) + configuredRemote := "git@github.com:org/repo.git" + + c := NewController(context.Background(), Params{ + Logger: zaptest.NewLogger(t), + Storage: store, + Orchestrator: orch, + RepoConfig: fakeRepoConfigProvider{ + configuredRemote: {Remote: configuredRemote}, + }, + }) + request := &pb.GetChangedTargetsRequest{ + FirstRevision: &pb.BuildDescription{ + Strategy: pb.COMPUTATION_STRATEGY_UNSET, + Remote: "ssh://git@github.com:22/org/repo", + BaseSha: "sha1", + }, + SecondRevision: &pb.BuildDescription{ + Strategy: pb.COMPUTATION_STRATEGY_UNSET, + Remote: "ssh://git@github.com:22/org/repo", + BaseSha: "sha2", + }, + } + + err := c.GetChangedTargets(request, stream) + require.Error(t, err) +} + func TestGetChangedTargets_CacheHit(t *testing.T) { ctrl := gomock.NewController(t) stream := tangomock.NewMockTangoServiceGetChangedTargetsYARPCServer(ctrl) diff --git a/controller/gettargetgraph.go b/controller/gettargetgraph.go index e59ce2ea..f8a57024 100644 --- a/controller/gettargetgraph.go +++ b/controller/gettargetgraph.go @@ -26,7 +26,6 @@ import ( tangoerrors "github.com/uber/tango/core/errors" "github.com/uber/tango/entity" "github.com/uber/tango/internal/mapper" - "github.com/uber/tango/internal/url" "github.com/uber/tango/observability/metrics" "github.com/uber/tango/core/storage" @@ -36,11 +35,16 @@ import ( // GetTargetGraph returns the target graph for a given request. func (c *controller) GetTargetGraph(request *pb.GetTargetGraphRequest, stream pb.TangoServiceGetTargetGraphYARPCServer) (retErr error) { - repo := url.ToShortRemote(request.GetBuildDescription().GetRemote()) + entityReq, mappingErr := mapper.ProtoToGetTargetGraphRequest(request) + repo := unknownRepositoryMetricLabel + var repositoryErr error + if mappingErr == nil { + repo, repositoryErr = c.resolveRepository(entityReq.Build.Remote) + } e := c.emitter.Tagged(map[string]string{metrics.TagRepo: repo}) op := metrics.Begin(e, opGetTargetGraph, metrics.SlowDurationBuckets) logger := c.logger.WithLazy( - zap.Any("build_description", request.GetBuildDescription()), + zap.String("repository", repo), ) defer func() { op.Complete(retErr) @@ -52,9 +56,11 @@ func (c *controller) GetTargetGraph(request *pb.GetTargetGraphRequest, stream pb start := time.Now() ctx, cancelLink := c.linkRequestCtx(stream.Context()) defer cancelLink() - entityReq, err := mapper.ProtoToGetTargetGraphRequest(request) - if err != nil { - return tangoerrors.NewUser(fmt.Errorf("convert get target graph request: %w", err)) + if mappingErr != nil { + return tangoerrors.NewUser(fmt.Errorf("convert get target graph request: %w", mappingErr)) + } + if repositoryErr != nil { + return repositoryErr } graphReader, err := c.getGraph(ctx, e, entityReq) if err != nil { diff --git a/controller/gettargetgraph_test.go b/controller/gettargetgraph_test.go index 1eeac959..2f6dbabf 100644 --- a/controller/gettargetgraph_test.go +++ b/controller/gettargetgraph_test.go @@ -65,6 +65,33 @@ func TestGetTargetGraph_CacheMiss_NoSend(t *testing.T) { require.NoError(t, err) } +func TestGetTargetGraph_UnconfiguredRemoteRejectedBeforeCacheLookup(t *testing.T) { + ctrl := gomock.NewController(t) + stream := tangomock.NewMockTangoServiceGetTargetGraphYARPCServer(ctrl) + stream.EXPECT().Context().Return(context.Background()) + store := storagemock.NewMockStorage(ctrl) + orch := orchestratormock.NewMockOrchestrator(ctrl) + configuredRemote := "git@github.com:org/repo.git" + + c := NewController(context.Background(), Params{ + Logger: zaptest.NewLogger(t), + Storage: store, + Orchestrator: orch, + RepoConfig: fakeRepoConfigProvider{ + configuredRemote: {Remote: configuredRemote}, + }, + }) + err := c.GetTargetGraph(&pb.GetTargetGraphRequest{ + BuildDescription: &pb.BuildDescription{ + Strategy: pb.COMPUTATION_STRATEGY_UNSET, + Remote: "ssh://git@github.com:22/org/repo", + BaseSha: "sha", + }, + }, stream) + + require.Error(t, err) +} + func TestGetTargetGraph_StorageError_Propagates(t *testing.T) { expected := errors.New("boom") ctrl := gomock.NewController(t) diff --git a/core/cachekey/cachekey.go b/core/cachekey/cachekey.go index 8004fe65..4629831e 100644 --- a/core/cachekey/cachekey.go +++ b/core/cachekey/cachekey.go @@ -34,7 +34,7 @@ import ( // excludeFilesRegex is folded into the key when non-empty (it affects // computation). Empty ⇒ legacy path unchanged. func GetGraphByTreeHash(remote, treehash string, strategy entity.ComputationStrategy, excludeFilesRegex []string) string { - path := filepath.Join(url.ToShortRemote(remote), "graphs", treehash, strategy.String()) + path := filepath.Join(url.RepositoryID(remote), "graphs", treehash, strategy.String()) if hash := hashExcludeFilesRegex(excludeFilesRegex); hash != "" { path += "_requests-options-" + hash } @@ -50,7 +50,7 @@ func GetGraphByTreeHash(remote, treehash string, strategy entity.ComputationStra // gob key rather than a child — on the disk backend a key is a file, and a // child key would need the gob file to be a directory. func GetTGBGraphByTreeHash(remote, treehash string, strategy entity.ComputationStrategy, excludeFilesRegex []string) string { - path := filepath.Join(url.ToShortRemote(remote), "graphs", treehash, strategy.String()+"-tgb") + path := filepath.Join(url.RepositoryID(remote), "graphs", treehash, strategy.String()+"-tgb") if hash := hashExcludeFilesRegex(excludeFilesRegex); hash != "" { path += "_requests-options-" + hash } @@ -62,7 +62,7 @@ func GetTGBGraphByTreeHash(remote, treehash string, strategy entity.ComputationS // requests), so neither excludeFilesRegex nor the computation strategy is // part of this key. func GetTreehashCachePath(buildDescription entity.BuildDescription) string { - path := filepath.Join(url.ToShortRemote(buildDescription.Remote), "treehashes", fmt.Sprintf("base-sha-%s", buildDescription.BaseSha)) + path := filepath.Join(url.RepositoryID(buildDescription.Remote), "treehashes", fmt.Sprintf("base-sha-%s", buildDescription.BaseSha)) if len(buildDescription.ChangeRequests) > 0 { path += "_request-urls-" + url.GetReqURLsHash(buildDescription.ChangeRequests) } @@ -75,7 +75,7 @@ func GetTreehashCachePath(buildDescription entity.BuildDescription) string { // excludeFilesRegex is folded into the key when non-empty (it affects computation). // Empty ⇒ legacy path unchanged. func GetComparedTargetsCachePath(remote, treehash1, treehash2 string, excludeFilesRegex []string) string { - path := filepath.Join(url.ToShortRemote(remote), "compared-targets", treehash1+"_"+treehash2) + path := filepath.Join(url.RepositoryID(remote), "compared-targets", treehash1+"_"+treehash2) if hash := hashExcludeFilesRegex(excludeFilesRegex); hash != "" { path += "_requests-options-" + hash } diff --git a/core/cachekey/cachekey_test.go b/core/cachekey/cachekey_test.go index 5e54c403..9f43299b 100644 --- a/core/cachekey/cachekey_test.go +++ b/core/cachekey/cachekey_test.go @@ -16,6 +16,8 @@ package cachekey import ( "path/filepath" + "regexp" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -28,10 +30,11 @@ func TestGetGraphByTreeHash(t *testing.T) { remote := "git@github:uber/tango" treehash := "abcd1234" strategy := entity.ComputationStrategyNative + repositoryID := url.RepositoryID(remote) // Nil/empty exclude list ⇒ no suffix. got := GetGraphByTreeHash(remote, treehash, strategy, nil) - assert.Equal(t, filepath.Join("uber/tango", "graphs", treehash, strategy.String()), got) + assert.Equal(t, filepath.Join(repositoryID, "graphs", treehash, strategy.String()), got) assert.Equal(t, got, GetGraphByTreeHash(remote, treehash, strategy, []string{})) // Different strategies ⇒ different keys. @@ -59,14 +62,15 @@ func TestGetTreehashCachePath(t *testing.T) { }, } got := GetTreehashCachePath(desc) - want := filepath.Join("uber/tango", "treehashes", "base-sha-deadbeef") + "_request-urls-" + url.GetReqURLsHash(desc.ChangeRequests) + want := filepath.Join(url.RepositoryID(desc.Remote), "treehashes", "base-sha-deadbeef") + "_request-urls-" + url.GetReqURLsHash(desc.ChangeRequests) assert.Equal(t, want, got) } func TestGetComparedTargetsCachePath(t *testing.T) { t.Parallel() - got := GetComparedTargetsCachePath("git@github:uber/tango", "abc", "def", nil) - assert.Equal(t, filepath.Join("uber/tango", "compared-targets", "abc_def"), got) + remote := "git@github:uber/tango" + got := GetComparedTargetsCachePath(remote, "abc", "def", nil) + assert.Equal(t, filepath.Join(url.RepositoryID(remote), "compared-targets", "abc_def"), got) // Nil/empty list ⇒ legacy path. assert.Equal(t, got, GetComparedTargetsCachePath("git@github:uber/tango", "abc", "def", []string{})) @@ -82,7 +86,7 @@ func TestGetTGBGraphByTreeHash(t *testing.T) { strategy := entity.ComputationStrategyNative got := GetTGBGraphByTreeHash(remote, treehash, strategy, nil) - assert.Equal(t, filepath.Join("uber/tango", "graphs", treehash, strategy.String()+"-tgb"), got) + assert.Equal(t, filepath.Join(url.RepositoryID(remote), "graphs", treehash, strategy.String()+"-tgb"), got) assert.Equal(t, got, GetTGBGraphByTreeHash(remote, treehash, strategy, []string{})) // Never collides with the gob key space, and stays a sibling (same @@ -97,3 +101,36 @@ func TestGetTGBGraphByTreeHash(t *testing.T) { assert.NotEqual(t, got, withRegex) assert.Contains(t, withRegex, "_requests-options-") } + +func TestRemoteNamespacesAreIsolatedAndSafe(t *testing.T) { + t.Parallel() + + githubRemote := "git@github.com:org/repo.git" + gitlabRemote := "git@gitlab.com:org/repo.git" + credentialRemote := "https://user:token@github.com/org/repo.git" + credentiallessRemote := "https://user@github.com/org/repo" + traversalRemote := "../../outside" + + assert.NotEqual(t, + GetGraphByTreeHash(githubRemote, "tree", entity.ComputationStrategyNative, nil), + GetGraphByTreeHash(gitlabRemote, "tree", entity.ComputationStrategyNative, nil), + ) + assert.NotEqual(t, + GetGraphByTreeHash(credentiallessRemote, "tree", entity.ComputationStrategyNative, nil), + GetGraphByTreeHash(credentialRemote, "tree", entity.ComputationStrategyNative, nil), + ) + + keys := []string{ + GetGraphByTreeHash(traversalRemote, "tree", entity.ComputationStrategyNative, nil), + GetTGBGraphByTreeHash(credentialRemote, "tree", entity.ComputationStrategyNative, nil), + GetTreehashCachePath(entity.BuildDescription{Remote: traversalRemote, BaseSha: "base"}), + GetComparedTargetsCachePath(credentialRemote, "first", "second", nil), + } + for _, key := range keys { + firstComponent := strings.Split(filepath.ToSlash(key), "/")[0] + assert.Regexp(t, regexp.MustCompile(`^repo-[A-Za-z0-9_-]+-[a-f0-9]{64}$`), firstComponent) + assert.False(t, filepath.IsAbs(key)) + assert.NotContains(t, key, "..") + assert.NotContains(t, key, "token") + } +} diff --git a/core/repomanager/BUILD.bazel b/core/repomanager/BUILD.bazel index 9e8e2e1c..b9eababc 100644 --- a/core/repomanager/BUILD.bazel +++ b/core/repomanager/BUILD.bazel @@ -28,6 +28,7 @@ go_test( "//core/git/gitmock", "//core/workspace", "//entity", + "//internal/url", "@com_github_stretchr_testify//assert", "@com_github_stretchr_testify//require", "@org_uber_go_mock//gomock", diff --git a/core/repomanager/repo_manager.go b/core/repomanager/repo_manager.go index a2e26c7e..60f057d7 100644 --- a/core/repomanager/repo_manager.go +++ b/core/repomanager/repo_manager.go @@ -20,6 +20,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "sync" "time" @@ -104,9 +105,13 @@ func NewRepoManager(appCtx context.Context, p Params) (RepoManager, error) { if p.PoolSize <= 0 { return nil, fmt.Errorf("pool size must be > 0, got %d", p.PoolSize) } + clonePath, err := filepath.Abs(p.RepoManagerClonePath) + if err != nil { + return nil, fmt.Errorf("resolve repo manager clone path: %w", err) + } return &repoManager{ git: p.Git, - repoManagerClonePath: p.RepoManagerClonePath, + repoManagerClonePath: clonePath, logger: p.Logger, emitter: metrics.New(p.Scope).SubScope("repo_manager"), poolSize: p.PoolSize, @@ -116,21 +121,28 @@ func NewRepoManager(appCtx context.Context, p Params) (RepoManager, error) { }, nil } -func (r *repoManager) poolFor(repo string) *workerPool { +func (r *repoManager) poolFor(repo string) (*workerPool, error) { r.mu.Lock() defer r.mu.Unlock() if pool, ok := r.pools[repo]; ok { - return pool + return pool, nil + } + originDir, err := pathUnderRoot(r.repoManagerClonePath, repo) + if err != nil { + return nil, fmt.Errorf("resolve origin directory: %w", err) + } + workersDir, err := pathUnderRoot(r.repoManagerClonePath, ".workers", repo) + if err != nil { + return nil, fmt.Errorf("resolve workers directory: %w", err) } pool := &workerPool{ - originDir: filepath.Join(r.repoManagerClonePath, repo), + originDir: originDir, avail: make(chan *workerSlot, r.poolSize), } // Pre-allocate fixed worker slots. Existing directories from a previous // run are detected and reused without re-cloning. - workersDir := filepath.Join(r.repoManagerClonePath, ".workers", repo) for i := 1; i <= r.poolSize; i++ { dir := filepath.Join(workersDir, fmt.Sprintf("worker-%d", i)) slot := &workerSlot{dir: dir} @@ -142,21 +154,24 @@ func (r *repoManager) poolFor(repo string) *workerPool { } r.pools[repo] = pool - return pool + return pool, nil } // Lease borrows a worker workspace from the pool. // If all workers are leased, it blocks until one is returned or ctx is cancelled. func (r *repoManager) Lease(ctx context.Context, desc entity.BuildDescription) (_ workspace.Workspace, retErr error) { - repo := url.ToShortRemote(desc.Remote) - e := r.emitter.Tagged(map[string]string{metrics.TagRepo: repo}) + repo := url.RepositoryID(desc.Remote) + e := r.emitter.Tagged(map[string]string{metrics.TagRepo: url.RepositoryMetricLabel(desc.Remote)}) op := metrics.Begin(e, _opLease, metrics.SlowDurationBuckets) defer func() { op.Complete(retErr) }() - pool := r.poolFor(repo) + pool, err := r.poolFor(repo) + if err != nil { + return nil, fmt.Errorf("prepare pool: %w", err) + } originStart := time.Now() - err := pool.ensureOrigin(ctx, r.git, desc.Remote) + err = pool.ensureOrigin(ctx, r.git, desc.Remote) recordStep(e, _stepEnsureOrigin, originStart, metrics.SlowDurationBuckets) if err != nil { return nil, fmt.Errorf("ensure origin: %w", err) @@ -247,6 +262,12 @@ func (p *workerPool) ensureOrigin(ctx context.Context, g git.Interface, remote s // createWorker creates a worker by cloning the origin with --local // (fast and space-efficient). func (r *repoManager) createWorker(ctx context.Context, originDir, workerDir string) error { + if err := verifyPathUnderRoot(r.repoManagerClonePath, originDir); err != nil { + return fmt.Errorf("validate origin directory: %w", err) + } + if err := verifyPathUnderRoot(r.repoManagerClonePath, workerDir); err != nil { + return fmt.Errorf("validate worker directory: %w", err) + } if err := os.RemoveAll(workerDir); err != nil { return fmt.Errorf("remove previous worker: %w", err) } @@ -255,3 +276,23 @@ func (r *repoManager) createWorker(ctx context.Context, originDir, workerDir str } return r.git.Clone(ctx, originDir, workerDir, "--local", "-c", "gc.auto=0") } + +func pathUnderRoot(root string, elements ...string) (string, error) { + candidate := filepath.Join(append([]string{root}, elements...)...) + if err := verifyPathUnderRoot(root, candidate); err != nil { + return "", err + } + return candidate, nil +} + +func verifyPathUnderRoot(root, candidate string) error { + relative, err := filepath.Rel(root, candidate) + if err != nil { + return err + } + if relative == "." || relative == ".." || filepath.IsAbs(relative) || + strings.HasPrefix(relative, ".."+string(filepath.Separator)) { + return fmt.Errorf("path escapes configured root") + } + return nil +} diff --git a/core/repomanager/repo_manager_test.go b/core/repomanager/repo_manager_test.go index f0487b70..c836502b 100644 --- a/core/repomanager/repo_manager_test.go +++ b/core/repomanager/repo_manager_test.go @@ -21,6 +21,7 @@ import ( "os" "os/exec" "path/filepath" + "strings" "testing" "time" @@ -30,6 +31,7 @@ import ( gitmock "github.com/uber/tango/core/git/gitmock" "github.com/uber/tango/core/workspace" "github.com/uber/tango/entity" + urlutil "github.com/uber/tango/internal/url" "go.uber.org/mock/gomock" "go.uber.org/zap" ) @@ -56,6 +58,14 @@ func runRepoGit(t *testing.T, directory string, args ...string) { require.NoError(t, err, "git %v: %s", args, output) } +func testOriginDir(root, remote string) string { + return filepath.Join(root, urlutil.RepositoryID(remote)) +} + +func testWorkerDir(root, remote string, worker int) string { + return filepath.Join(root, ".workers", urlutil.RepositoryID(remote), fmt.Sprintf("worker-%d", worker)) +} + func TestNewRepoManager_InvalidPoolSize(t *testing.T) { t.Parallel() _, err := NewRepoManager(context.Background(), Params{ @@ -72,8 +82,8 @@ func TestLease_ClonesOriginAndCreatesWorker(t *testing.T) { root := t.TempDir() remote := "git@github.com:org/repo" - originDir := filepath.Join(root, "org/repo") - workerDir := filepath.Join(root, ".workers", "org/repo", "worker-1") + originDir := testOriginDir(root, remote) + workerDir := testWorkerDir(root, remote, 1) g.EXPECT().Clone(gomock.Any(), remote, originDir, "-c", "gc.auto=0").Return(nil) g.EXPECT().Clone(gomock.Any(), originDir, workerDir, "--local", "-c", "gc.auto=0").Return(nil) @@ -92,8 +102,8 @@ func TestLease_SkipsOriginClone_WhenExists(t *testing.T) { root := t.TempDir() remote := "git@github.com:org/repo" - originDir := filepath.Join(root, "org/repo") - workerDir := filepath.Join(root, ".workers", "org/repo", "worker-1") + originDir := testOriginDir(root, remote) + workerDir := testWorkerDir(root, remote, 1) require.NoError(t, os.MkdirAll(filepath.Join(originDir, ".git"), 0o755)) @@ -114,8 +124,8 @@ func TestLease_ReusesWorker_AfterRelease(t *testing.T) { root := t.TempDir() remote := "git@github.com:org/repo" - originDir := filepath.Join(root, "org/repo") - workerDir := filepath.Join(root, ".workers", "org/repo", "worker-1") + originDir := testOriginDir(root, remote) + workerDir := testWorkerDir(root, remote, 1) // Exactly 1 origin + 1 worker clone total g.EXPECT().Clone(gomock.Any(), remote, originDir, "-c", "gc.auto=0").Return(nil) @@ -200,8 +210,8 @@ func TestLease_RecreatesWorker_WhenRestoreFails(t *testing.T) { root := t.TempDir() remote := "git@github.com:org/repo" - originDir := filepath.Join(root, "org/repo") - workerDir := filepath.Join(root, ".workers", "org/repo", "worker-1") + originDir := testOriginDir(root, remote) + workerDir := testWorkerDir(root, remote, 1) g.EXPECT().Clone(gomock.Any(), remote, originDir, "-c", "gc.auto=0").Return(nil) g.EXPECT().Clone(gomock.Any(), originDir, workerDir, "--local", "-c", "gc.auto=0").Return(nil).Times(2) @@ -227,11 +237,11 @@ func TestLease_CreatesMultipleWorkers(t *testing.T) { root := t.TempDir() remote := "git@github.com:org/repo" - originDir := filepath.Join(root, "org/repo") + originDir := testOriginDir(root, remote) g.EXPECT().Clone(gomock.Any(), remote, originDir, "-c", "gc.auto=0").Return(nil) for i := 1; i <= 2; i++ { - dir := filepath.Join(root, ".workers", "org/repo", fmt.Sprintf("worker-%d", i)) + dir := testWorkerDir(root, remote, i) g.EXPECT().Clone(gomock.Any(), originDir, dir, "--local", "-c", "gc.auto=0").Return(nil) } @@ -255,8 +265,8 @@ func TestLease_BlocksUntilReturn(t *testing.T) { root := t.TempDir() remote := "git@github.com:org/repo" - originDir := filepath.Join(root, "org/repo") - workerDir := filepath.Join(root, ".workers", "org/repo", "worker-1") + originDir := testOriginDir(root, remote) + workerDir := testWorkerDir(root, remote, 1) g.EXPECT().Clone(gomock.Any(), remote, originDir, "-c", "gc.auto=0").Return(nil) g.EXPECT().Clone(gomock.Any(), originDir, workerDir, "--local", "-c", "gc.auto=0").Return(nil) @@ -301,8 +311,8 @@ func TestLease_CtxCanceled(t *testing.T) { root := t.TempDir() remote := "git@github.com:org/repo" - originDir := filepath.Join(root, "org/repo") - workerDir := filepath.Join(root, ".workers", "org/repo", "worker-1") + originDir := testOriginDir(root, remote) + workerDir := testWorkerDir(root, remote, 1) g.EXPECT().Clone(gomock.Any(), remote, originDir, "-c", "gc.auto=0").Return(nil) g.EXPECT().Clone(gomock.Any(), originDir, workerDir, "--local", "-c", "gc.auto=0").Return(nil) @@ -329,8 +339,8 @@ func TestLease_CtxDeadlineExceeded(t *testing.T) { root := t.TempDir() remote := "git@github.com:org/repo" - originDir := filepath.Join(root, "org/repo") - workerDir := filepath.Join(root, ".workers", "org/repo", "worker-1") + originDir := testOriginDir(root, remote) + workerDir := testWorkerDir(root, remote, 1) g.EXPECT().Clone(gomock.Any(), remote, originDir, "-c", "gc.auto=0").Return(nil) g.EXPECT().Clone(gomock.Any(), originDir, workerDir, "--local", "-c", "gc.auto=0").Return(nil) @@ -357,7 +367,7 @@ func TestLease_OriginCloneFails(t *testing.T) { root := t.TempDir() remote := "git@github.com:org/repo" - g.EXPECT().Clone(gomock.Any(), remote, filepath.Join(root, "org/repo"), "-c", "gc.auto=0").Return(assert.AnError) + g.EXPECT().Clone(gomock.Any(), remote, testOriginDir(root, remote), "-c", "gc.auto=0").Return(assert.AnError) rm := newTestRepoManager(t, context.Background(), Params{Git: g, Logger: zap.NewNop(), RepoManagerClonePath: root, PoolSize: 1}) _, err := rm.Lease(context.Background(), entity.BuildDescription{Remote: remote}) @@ -372,8 +382,8 @@ func TestLease_WorkerCloneFails(t *testing.T) { root := t.TempDir() remote := "git@github.com:org/repo" - originDir := filepath.Join(root, "org/repo") - workerDir := filepath.Join(root, ".workers", "org/repo", "worker-1") + originDir := testOriginDir(root, remote) + workerDir := testWorkerDir(root, remote, 1) g.EXPECT().Clone(gomock.Any(), remote, originDir, "-c", "gc.auto=0").Return(nil) g.EXPECT().Clone(gomock.Any(), originDir, workerDir, "--local", "-c", "gc.auto=0").Return(assert.AnError) @@ -391,10 +401,12 @@ func TestLease_DiscoversExistingWorker(t *testing.T) { root := t.TempDir() remote := "git@github.com:org/repo" + originDir := testOriginDir(root, remote) + workerDir := testWorkerDir(root, remote, 1) // Pre-create origin and worker from a "previous run" - require.NoError(t, os.MkdirAll(filepath.Join(root, "org/repo", ".git"), 0o755)) - require.NoError(t, os.MkdirAll(filepath.Join(root, ".workers", "org/repo", "worker-1", ".git"), 0o755)) + require.NoError(t, os.MkdirAll(filepath.Join(originDir, ".git"), 0o755)) + require.NoError(t, os.MkdirAll(filepath.Join(workerDir, ".git"), 0o755)) // No Clone calls — everything already exists rm := newTestRepoManager(t, context.Background(), Params{Git: g, Logger: zap.NewNop(), RepoManagerClonePath: root, PoolSize: 1}) @@ -410,13 +422,13 @@ func TestLease_DifferentRepos_IndependentPools(t *testing.T) { g := gitmock.NewMockInterface(ctrl) root := t.TempDir() - remote1 := "git@github.com:org/repo1" - remote2 := "git@github.com:org/repo2" + remote1 := "git@github.com:org/repo" + remote2 := "git@gitlab.com:org/repo" - origin1 := filepath.Join(root, "org/repo1") - origin2 := filepath.Join(root, "org/repo2") - worker1 := filepath.Join(root, ".workers", "org/repo1", "worker-1") - worker2 := filepath.Join(root, ".workers", "org/repo2", "worker-1") + origin1 := testOriginDir(root, remote1) + origin2 := testOriginDir(root, remote2) + worker1 := testWorkerDir(root, remote1, 1) + worker2 := testWorkerDir(root, remote2, 1) g.EXPECT().Clone(gomock.Any(), remote1, origin1, "-c", "gc.auto=0").Return(nil) g.EXPECT().Clone(gomock.Any(), origin1, worker1, "--local", "-c", "gc.auto=0").Return(nil) @@ -432,8 +444,9 @@ func TestLease_DifferentRepos_IndependentPools(t *testing.T) { ws2, err := rm.Lease(ctx, entity.BuildDescription{Remote: remote2}) require.NoError(t, err) - assert.Contains(t, ws1.Path(), "repo1") - assert.Contains(t, ws2.Path(), "repo2") + assert.Equal(t, worker1, ws1.Path()) + assert.Equal(t, worker2, ws2.Path()) + assert.NotEqual(t, ws1.Path(), ws2.Path()) require.NoError(t, ws1.Release()) require.NoError(t, ws2.Release()) @@ -446,8 +459,8 @@ func TestLease_WorkerCloneFails_SlotReturnedToPool(t *testing.T) { root := t.TempDir() remote := "git@github.com:org/repo" - originDir := filepath.Join(root, "org/repo") - workerDir := filepath.Join(root, ".workers", "org/repo", "worker-1") + originDir := testOriginDir(root, remote) + workerDir := testWorkerDir(root, remote, 1) g.EXPECT().Clone(gomock.Any(), remote, originDir, "-c", "gc.auto=0").Return(nil) // First attempt fails, second succeeds @@ -468,3 +481,32 @@ func TestLease_WorkerCloneFails_SlotReturnedToPool(t *testing.T) { require.NoError(t, err) require.NoError(t, ws.Release()) } + +func TestPathUnderRoot_RejectsTraversal(t *testing.T) { + t.Parallel() + + root := t.TempDir() + _, err := pathUnderRoot(root, "..", "outside") + require.Error(t, err) +} + +func TestCreateWorker_RejectsDeletionOutsideCloneRoot(t *testing.T) { + t.Parallel() + + root := t.TempDir() + outside := t.TempDir() + sentinel := filepath.Join(outside, "sentinel") + require.NoError(t, os.WriteFile(sentinel, []byte("keep"), 0o600)) + + rm := &repoManager{repoManagerClonePath: root} + err := rm.createWorker(context.Background(), filepath.Join(root, "origin"), outside) + + require.Error(t, err) + require.FileExists(t, sentinel) +} + +func pathIsUnderRoot(root, candidate string) bool { + relative, err := filepath.Rel(root, candidate) + return err == nil && relative != ".." && !filepath.IsAbs(relative) && + !strings.HasPrefix(relative, ".."+string(filepath.Separator)) +} diff --git a/docs/observability/metrics.md b/docs/observability/metrics.md index 9d039693..66543e73 100644 --- a/docs/observability/metrics.md +++ b/docs/observability/metrics.md @@ -162,7 +162,7 @@ Each component stores the `*metrics.Emitter` it was constructed with. At the top ```go func (b *nativeOrchestrator) GetTargetGraph(ctx context.Context, req entity.GetTargetGraphRequest) (_ storage.GraphReader, retErr error) { - e := b.emitter.Tagged(map[string]string{metrics.TagRepo: url.ToShortRemote(req.Build.Remote)}) + e := b.emitter.Tagged(map[string]string{metrics.TagRepo: url.RepositoryMetricLabel(req.Build.Remote)}) op := metrics.Begin(e, _opGetTargetGraph, metrics.SlowDurationBuckets) defer func() { op.Complete(retErr) }() @@ -175,7 +175,7 @@ The controller follows the same shape; each RPC passes its own operation name an ```go func (c *controller) GetChangedTargets(req *pb.GetChangedTargetsRequest, stream pb.Tango_GetChangedTargetsServer) (retErr error) { - e := c.emitter.Tagged(map[string]string{metrics.TagRepo: url.ToShortRemote(req.GetFirstRevision().GetRemote())}) + e := c.emitter.Tagged(map[string]string{metrics.TagRepo: url.RepositoryMetricLabel(req.GetFirstRevision().GetRemote())}) op := metrics.Begin(e, opGetChangedTargets, metrics.SlowDurationBuckets) defer func() { op.Complete(retErr) }() @@ -217,4 +217,4 @@ fetch service:tango name:controller.get_changed_targets.target_count | histogram ## Request-specific tags -Each distinct tag value is a new series, so tag values must be bounded — never request IDs, commit hashes, paths, or raw repo URLs. `repo` is safe only with an explicit cardinality budget and a normalized, allow-listed value; the handlers above apply it that way (`ToShortRemote`). +Each distinct tag value is a new series, so tag values must be bounded — never request IDs, commit hashes, paths, or raw repo URLs. The bundled service validates remotes against its repository configuration before cache access and uses a fixed-length digest of the exact configured value for the `repo` tag (`RepositoryMetricLabel`). Invalid remotes use the shared `unknown` label. diff --git a/internal/url/url.go b/internal/url/url.go index 62197279..ab16fb31 100644 --- a/internal/url/url.go +++ b/internal/url/url.go @@ -16,20 +16,79 @@ package url import ( "crypto/md5" + "crypto/sha256" + "encoding/hex" "fmt" "hash" "io" + neturl "net/url" + "path" "strconv" "strings" "github.com/uber/tango/entity" ) -// ToShortRemote returns the short remote name given a git ssh remote string. -// For example, "git@github:uber/tango" will return "uber/tango". -func ToShortRemote(remote string) string { - strs := strings.Split(remote, ":") - return strs[len(strs)-1] +const ( + repositoryIDPrefix = "repo-" + repositoryNameMaxLength = 48 + repositoryNamePlaceholder = "repository" + metricDigestLength = 12 +) + +// RepositoryID returns a filesystem-safe identity containing a recognizable +// repository-name hint and a hash of the exact configured remote. +func RepositoryID(remote string) string { + return repositoryIDPrefix + repositoryName(remote) + "-" + repositoryDigest(remote) +} + +// RepositoryMetricLabel returns a fixed-length label for a configured +// repository without exposing its remote. +func RepositoryMetricLabel(remote string) string { + return repositoryIDPrefix + repositoryDigest(remote)[:metricDigestLength] +} + +func repositoryDigest(remote string) string { + sum := sha256.Sum256([]byte(remote)) + return hex.EncodeToString(sum[:]) +} + +func repositoryName(remote string) string { + candidate := remote + if strings.Contains(remote, "://") || strings.HasPrefix(strings.ToLower(remote), "file:") { + if parsed, err := neturl.Parse(remote); err == nil { + candidate = parsed.Path + if candidate == "" { + candidate = parsed.Opaque + } + } + } else if colon := strings.IndexByte(remote, ':'); colon >= 0 { + candidate = remote[colon+1:] + } + + candidate = strings.TrimSuffix(path.Base(strings.TrimRight(candidate, "/")), ".git") + var name strings.Builder + lastWasSeparator := false + for _, char := range candidate { + if char >= 'a' && char <= 'z' || + char >= 'A' && char <= 'Z' || + char >= '0' && char <= '9' || + char == '-' || char == '_' { + name.WriteRune(char) + lastWasSeparator = false + } else if name.Len() > 0 && !lastWasSeparator { + name.WriteByte('-') + lastWasSeparator = true + } + if name.Len() >= repositoryNameMaxLength { + break + } + } + safeName := strings.Trim(name.String(), "-_") + if safeName == "" { + return repositoryNamePlaceholder + } + return safeName } // GetReqURLsHash returns a fixed-length hash of the ordered change requests. diff --git a/internal/url/url_test.go b/internal/url/url_test.go index fa1e7252..959bacc7 100644 --- a/internal/url/url_test.go +++ b/internal/url/url_test.go @@ -17,40 +17,50 @@ package url import ( "crypto/md5" "fmt" + "regexp" "testing" "github.com/stretchr/testify/assert" "github.com/uber/tango/entity" ) -func TestToShortRemote(t *testing.T) { +func TestRepositoryID(t *testing.T) { t.Parallel() - tests := []struct { - name string - remote string - want string - }{ - { - name: "ssh remote with host", - remote: "git@github:uber/tango", - want: "uber/tango", - }, - { - name: "already short", - remote: "uber/tango", - want: "uber/tango", - }, - { - name: "with nested path", - remote: "git@github:org/project/sub", - want: "org/project/sub", - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.want, ToShortRemote(tt.remote)) - }) - } + + remote := "git@github.com:org/repo.git" + want := RepositoryID(remote) + assert.Regexp(t, regexp.MustCompile(`^repo-repo-[a-f0-9]{64}$`), want) + assert.Equal(t, want, RepositoryID(remote)) + assert.NotEqual(t, want, RepositoryID("ssh://git@github.com:22/org/repo")) + assert.NotEqual(t, want, RepositoryID("git@gitlab.com:org/repo.git")) + assert.NotEqual(t, RepositoryID("file:../repo-a"), RepositoryID("file:../repo-b")) + assert.Contains(t, RepositoryID("file:../repo-a"), "repo-a") + assert.Contains(t, want, "repo") + assert.NotContains(t, want, "github") + assert.NotContains(t, want, "/") + assert.NotContains(t, want, "..") +} + +func TestRepositoryID_SanitizesRepositoryName(t *testing.T) { + t.Parallel() + + id := RepositoryID("https://user:secret@example.com/org/my repo.git") + + assert.Regexp(t, regexp.MustCompile(`^repo-my-repo-[a-f0-9]{64}$`), id) + assert.NotContains(t, id, "user") + assert.NotContains(t, id, "secret") + assert.NotContains(t, id, "example") +} + +func TestRepositoryMetricLabel(t *testing.T) { + t.Parallel() + + remote := "https://github.com/org/repo.git" + label := RepositoryMetricLabel(remote) + + assert.Regexp(t, regexp.MustCompile(`^repo-[a-f0-9]{12}$`), label) + assert.NotContains(t, label, "github") + assert.Len(t, label, len(repositoryIDPrefix)+metricDigestLength) } func TestGetReqURLsHash(t *testing.T) { diff --git a/orchestrator/native_orchestrator.go b/orchestrator/native_orchestrator.go index a4cb534e..85f47a3a 100644 --- a/orchestrator/native_orchestrator.go +++ b/orchestrator/native_orchestrator.go @@ -107,7 +107,7 @@ func NewNativeOrchestrator(appCtx context.Context, p Params) (Orchestrator, erro // GetTargetGraph is used to compute the target graph locally. // It leases a workspace, checks out the base revision, applies the change requests, and computes the target graph. func (b *nativeOrchestrator) GetTargetGraph(ctx context.Context, req entity.GetTargetGraphRequest) (_ storage.GraphReader, retErr error) { - e := b.emitter.Tagged(map[string]string{metrics.TagRepo: url.ToShortRemote(req.Build.Remote)}) + e := b.emitter.Tagged(map[string]string{metrics.TagRepo: url.RepositoryMetricLabel(req.Build.Remote)}) op := metrics.Begin(e, _opGetTargetGraph, metrics.SlowDurationBuckets) defer func() { op.Complete(retErr) }() build := req.Build @@ -117,7 +117,7 @@ func (b *nativeOrchestrator) GetTargetGraph(ctx context.Context, req entity.GetT remote := build.Remote repoCfg, ok := b.config.GetRepositoryConfig(remote) if !ok { - return nil, fmt.Errorf("no repository configuration found for remote %q", remote) + return nil, fmt.Errorf("repository is not configured") } leaseStart := time.Now() ws, err := b.repoManager.Lease(ctx, build) @@ -230,7 +230,7 @@ func (b *nativeOrchestrator) GetTargetGraph(ctx context.Context, req entity.GetT GitClient: gitModule, Config: repoCfg, ExtraExcludedFiles: req.ExcludeFilesRegex, - Scope: b.scope.Tagged(map[string]string{metrics.TagRepo: url.ToShortRemote(build.Remote)}), + Scope: b.scope.Tagged(map[string]string{metrics.TagRepo: url.RepositoryMetricLabel(build.Remote)}), }) default: return nil, tangoerrors.NewUser(fmt.Errorf("unknown computation strategy: %d", build.Strategy))