From fb803176953c4fe013485f7f7dc73eb0f201db26 Mon Sep 17 00:00:00 2001 From: sergeyb Date: Wed, 26 Aug 2026 18:14:19 +0000 Subject: [PATCH] fix(repository): use configured repository IDs (BUG-001) --- config/config.go | 3 + config/config_test.go | 27 ++++++ config/repository_config.go | 4 + controller/BUILD.bazel | 1 - controller/controller.go | 29 ++++++- controller/controller_test.go | 27 ++++++ controller/getchangedtargets.go | 55 ++++++------ controller/getchangedtargets_test.go | 56 ++++++------- controller/getchangedtargets_tgb_test.go | 22 +++-- controller/gettargetgraph.go | 31 +++---- controller/gettargetgraph_test.go | 58 ++++++++----- controller/testhelper_test.go | 14 ++++ controller/wireerror_test.go | 11 ++- core/cachekey/cachekey.go | 18 ++-- core/cachekey/cachekey_test.go | 44 +++++----- core/cachekey/exclude_regex_rapid_test.go | 17 ++-- core/cachekey/treehash_rapid_test.go | 6 +- core/repomanager/BUILD.bazel | 3 +- core/repomanager/repo_manager.go | 21 ++++- core/repomanager/repo_manager_test.go | 92 ++++++++++++++------- docs/observability/metrics.md | 14 +++- example/README.md | 1 + example/main.go | 1 + example/tango-config.yaml | 2 + integration/integration_test.go | 8 +- integration/testdata/tango-config.yaml.tmpl | 1 + orchestrator/BUILD.bazel | 1 - orchestrator/native_orchestrator.go | 34 ++++---- orchestrator/native_orchestrator_test.go | 4 +- orchestrator/testdata/config.yaml | 1 + 30 files changed, 395 insertions(+), 211 deletions(-) diff --git a/config/config.go b/config/config.go index 6033ce09..9c9535c7 100644 --- a/config/config.go +++ b/config/config.go @@ -99,6 +99,9 @@ func ParseBytes(yamlBytes []byte) (*Config, error) { if remote == "" { return nil, fmt.Errorf("repository[%d].remote must not be empty", i) } + if config.Repository[i].RepositoryID == "" { + return nil, fmt.Errorf("repository[%d].repository_id must not be empty", i) + } if _, exists := config.repositoryByRemote[remote]; exists { return nil, fmt.Errorf("duplicate repository remote %q", remote) } diff --git a/config/config_test.go b/config/config_test.go index 580d6a17..1c2ff5c9 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -28,6 +28,7 @@ func minimal() string { return ` repository: - remote: "https://example.com/repo.git" + repository_id: "test-repository" service: max_worker_pool_size: 2 workspaces_root_path: "/tmp/tango-repo-manager" @@ -49,6 +50,7 @@ func TestParseBytes_ExplicitValues(t *testing.T) { yamlStr := ` repository: - remote: "https://example.com/repo.git" + repository_id: "test-repository" query_timeout_seconds: 60 bzlmod_enabled: false full_hash_repos: ["//"] @@ -88,6 +90,7 @@ storage: type: "memory" repository: - remote: "https://example.com/r.git" + repository_id: "test-repository" service: max_worker_pool_size: 1 workspaces_root_path: "/tmp/tango-repo-manager" @@ -98,6 +101,7 @@ service: yaml: ` repository: - remote: "https://example.com/r.git" + repository_id: "test-repository" service: max_worker_pool_size: 1 workspaces_root_path: "/tmp/tango-repo-manager" @@ -112,6 +116,7 @@ storage: root_path: "/tmp/store" repository: - remote: "https://example.com/r.git" + repository_id: "test-repository" service: max_worker_pool_size: 1 workspaces_root_path: "/tmp/tango-repo-manager" @@ -125,6 +130,7 @@ storage: type: "disk" repository: - remote: "https://example.com/r.git" + repository_id: "test-repository" service: max_worker_pool_size: 1 workspaces_root_path: "/tmp/tango-repo-manager" @@ -140,6 +146,7 @@ storage: root_path: "" repository: - remote: "https://example.com/r.git" + repository_id: "test-repository" service: max_worker_pool_size: 1 workspaces_root_path: "/tmp/tango-repo-manager" @@ -153,6 +160,7 @@ storage: type: "s3" repository: - remote: "https://example.com/r.git" + repository_id: "test-repository" service: max_worker_pool_size: 1 workspaces_root_path: "/tmp/tango-repo-manager" @@ -175,6 +183,7 @@ func TestParseBytes_UnknownFieldsRejected(t *testing.T) { yamlStr := ` repository: - remote: "https://example.com/repo.git" + repository_id: "test-repository" service: max_worker_pool_size: 1 workspaces_root_path: "/tmp/tango-repo-manager" @@ -188,6 +197,7 @@ func TestParseBytes_WorkerPoolSizeRequired(t *testing.T) { yamlStr := ` repository: - remote: "https://example.com/repo.git" + repository_id: "test-repository" service: max_worker_pool_size: 0 workspaces_root_path: "/tmp/tango-repo-manager" @@ -200,6 +210,19 @@ func TestParseBytes_EmptyRemoteRejected(t *testing.T) { yamlStr := ` repository: - remote: "" + repository_id: "test-repository" +service: + max_worker_pool_size: 1 + workspaces_root_path: "/tmp/tango-repo-manager" +` + _, err := ParseBytes([]byte(yamlStr)) + require.Error(t, err) +} + +func TestParseBytes_RepositoryIDRequired(t *testing.T) { + yamlStr := ` +repository: + - remote: "https://example.com/repo.git" service: max_worker_pool_size: 1 workspaces_root_path: "/tmp/tango-repo-manager" @@ -212,7 +235,9 @@ func TestParseBytes_DuplicateRemoteRejected(t *testing.T) { yamlStr := ` repository: - remote: "https://example.com/repo.git" + repository_id: "test-repository" - remote: "https://example.com/repo.git" + repository_id: "test-repository" service: max_worker_pool_size: 1 workspaces_root_path: "/tmp/tango-repo-manager" @@ -225,6 +250,7 @@ func TestParseBytes_WorkspacesRootPathRequired(t *testing.T) { yamlStr := ` repository: - remote: "https://example.com/repo.git" + repository_id: "test-repository" service: max_worker_pool_size: 1 ` @@ -254,6 +280,7 @@ func TestGetRepositoryConfig(t *testing.T) { repo, ok := cfg.GetRepositoryConfig("https://example.com/repo.git") assert.True(t, ok) assert.Equal(t, "https://example.com/repo.git", repo.Remote) + assert.Equal(t, "test-repository", repo.RepositoryID) _, ok = cfg.GetRepositoryConfig("https://missing.com/repo.git") assert.False(t, ok) diff --git a/config/repository_config.go b/config/repository_config.go index e84e274e..da4c8c39 100644 --- a/config/repository_config.go +++ b/config/repository_config.go @@ -29,6 +29,10 @@ type RepositoryConfig struct { // unique across all entries and match exactly what clients send in // BuildDescription.remote. Remote string `yaml:"remote"` + // RepositoryID is the required operator-provided name used for metrics, + // repository workspaces, and cache keys. It must be safe for all three and + // uniquely identify this repository across Tango installations. + RepositoryID string `yaml:"repository_id"` // TODO: FullHashRepos, ExcludedFiles, and StreamBazelLogs are not // documented in config/README.md. Delete them if they turn out to be // unneeded, otherwise document them there. diff --git a/controller/BUILD.bazel b/controller/BUILD.bazel index f4fc9cb6..8fe1f823 100644 --- a/controller/BUILD.bazel +++ b/controller/BUILD.bazel @@ -26,7 +26,6 @@ go_library( "//internal/targetdiff", "//internal/tgb", "//internal/tgbdiff", - "//internal/url", "//observability/metrics", "//orchestrator", "//tangopb", diff --git a/controller/controller.go b/controller/controller.go index cdb73645..e916f64b 100644 --- a/controller/controller.go +++ b/controller/controller.go @@ -17,9 +17,11 @@ package controller import ( "context" "errors" + "fmt" "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/observability/metrics" "github.com/uber/tango/orchestrator" @@ -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 is the authoritative repository allowlist. RPC remotes must + // match it exactly before the controller performs cache I/O. + RepoConfig config.RepositoryConfigProvider // 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,23 @@ type Params struct { ShadowCompare bool `optional:"true"` } +// resolveRequestRepository returns the configured repository and metric label +// for a validated request. Invalid requests retain the common unknown label and +// their existing error; valid requests must exactly match the configured +// repository allowlist before controller cache I/O. +func (c *controller) resolveRequestRepository(remote string, requestErr error) (config.RepositoryConfig, string, error) { + if requestErr != nil { + return config.RepositoryConfig{}, unknownRepositoryMetricLabel, requestErr + } + repo, ok := c.repoConfig.GetRepositoryConfig(remote) + if !ok { + return config.RepositoryConfig{}, unknownRepositoryMetricLabel, tangoerrors.NewUser( + fmt.Errorf("repository remote %q is not configured", remote), + ) + } + return repo, repo.RepositoryID, nil +} + type controller struct { logger *zap.Logger storage storage.Storage diff --git a/controller/controller_test.go b/controller/controller_test.go index 2914172b..07fad1fc 100644 --- a/controller/controller_test.go +++ b/controller/controller_test.go @@ -19,12 +19,38 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/uber/tango/config" tangoerrors "github.com/uber/tango/core/errors" orchestratormock "github.com/uber/tango/orchestrator/orchestratormock" "go.uber.org/mock/gomock" "go.uber.org/zap" ) +type rejectAllRepositoryConfigProvider struct{} + +func (rejectAllRepositoryConfigProvider) GetRepositoryConfig(string) (config.RepositoryConfig, bool) { + return config.RepositoryConfig{}, false +} + +func TestResolveRequestRepository(t *testing.T) { + c := &controller{repoConfig: rejectAllRepositoryConfigProvider{}} + + t.Run("request error uses unknown repository", func(t *testing.T) { + repo, label, err := c.resolveRequestRepository("ignored", assert.AnError) + assert.Empty(t, repo) + assert.Equal(t, unknownRepositoryMetricLabel, label) + assert.ErrorIs(t, err, assert.AnError) + }) + + t.Run("plain remote", func(t *testing.T) { + _, label, err := c.resolveRequestRepository("git@github.com:other/repo.git", nil) + require.Error(t, err) + assert.Equal(t, unknownRepositoryMetricLabel, label) + assert.Equal(t, tangoerrors.ErrorUser, tangoerrors.GetErrorCode(err)) + }) +} + // TestNewController_StoresAppContext verifies the caller-supplied context is // retained and is the one observed by background goroutines. func TestNewController_StoresAppContext(t *testing.T) { @@ -33,6 +59,7 @@ func TestNewController_StoresAppContext(t *testing.T) { defer cancel() c := NewController(appCtx, Params{ + RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zap.NewNop(), Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), }).(*controller) diff --git a/controller/getchangedtargets.go b/controller/getchangedtargets.go index cefe698e..7aa27c03 100644 --- a/controller/getchangedtargets.go +++ b/controller/getchangedtargets.go @@ -22,6 +22,7 @@ import ( "maps" "time" + "github.com/uber/tango/config" "github.com/uber/tango/core/cachekey" tangoerrors "github.com/uber/tango/core/errors" "github.com/uber/tango/core/storage" @@ -32,7 +33,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 +82,15 @@ 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) + if validationErr != nil { + validationErr = tangoerrors.NewUser(validationErr) + } + repoCfg, repo, repositoryErr := c.resolveRequestRepository(request.GetFirstRevision().GetRemote(), validationErr) 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,8 @@ func (c *controller) GetChangedTargets(request *pb.GetChangedTargetsRequest, str retErr = toWireError(retErr) } }() - if err := validateGetChangedTargetsRequest(request); err != nil { - return tangoerrors.NewUser(err) + if repositoryErr != nil { + return repositoryErr } ctx, cancelLink := c.linkRequestCtx(stream.Context()) defer cancelLink() @@ -115,7 +118,7 @@ func (c *controller) GetChangedTargets(request *pb.GetChangedTargetsRequest, str // Fast path: stream a previously computed result straight from cache. if !request.GetBypassCache() { - served, err := c.serveChangedTargetsFromCache(ctx, e, logger, request, stream, maxDist, start) + served, err := c.serveChangedTargetsFromCache(ctx, e, logger, request, stream, repoCfg.RepositoryID, maxDist, start) if err != nil { return fmt.Errorf("serve from cache: %w", err) } @@ -125,12 +128,12 @@ func (c *controller) GetChangedTargets(request *pb.GetChangedTargetsRequest, str } // Fetch both revisions' target graphs concurrently. - firstGraph, secondGraph, err := c.fetchTargetGraphs(ctx, e, logger, request) + firstGraph, secondGraph, err := c.fetchTargetGraphs(ctx, e, logger, request, repoCfg.RepositoryID) if err != nil { return fmt.Errorf("fetch target graphs: %w", err) } - changedTargetsResponses, err := c.compareFetchedGraphs(ctx, e, logger, firstGraph, secondGraph, c.seedAttributesFor(request.GetFirstRevision().GetRemote())) + changedTargetsResponses, err := c.compareFetchedGraphs(ctx, e, logger, firstGraph, secondGraph, seedAttributesFor(repoCfg)) // Allow GC of raw graph data while the caching goroutine runs. firstGraph = fetchedGraph{} secondGraph = fetchedGraph{} @@ -142,7 +145,7 @@ func (c *controller) GetChangedTargets(request *pb.GetChangedTargetsRequest, str } // Cache the computed result concurrently so it doesn't block the stream send. - c.cacheComparedTargets(logger, request, changedTargetsResponses) + c.cacheComparedTargets(logger, request, repoCfg.RepositoryID, changedTargetsResponses) sendStart := time.Now() if err := sendTrimmedChangedTargets(stream, changedTargetsResponses, maxDist, request.GetOutputConfig()); err != nil { @@ -168,9 +171,9 @@ func (c *controller) GetChangedTargets(request *pb.GetChangedTargetsRequest, str // real storage error surfaces here so an infra failure that disables the cache // (e.g. a missing-deadline "missing TTL" reject) becomes a visible request failure // rather than silent degradation. -func (c *controller) serveChangedTargetsFromCache(ctx context.Context, e *metrics.Emitter, logger *zap.Logger, request *pb.GetChangedTargetsRequest, stream pb.TangoServiceGetChangedTargetsYARPCServer, maxDist int32, start time.Time) (bool, error) { +func (c *controller) serveChangedTargetsFromCache(ctx context.Context, e *metrics.Emitter, logger *zap.Logger, request *pb.GetChangedTargetsRequest, stream pb.TangoServiceGetChangedTargetsYARPCServer, repositoryID string, maxDist int32, start time.Time) (bool, error) { cacheStart := time.Now() - treehash1, treehash2, err := readTreehashParallel(ctx, c.storage, request.GetFirstRevision(), request.GetSecondRevision(), e, opGetChangedTargets) + treehash1, treehash2, err := readTreehashParallel(ctx, c.storage, request.GetFirstRevision(), request.GetSecondRevision(), repositoryID, e, opGetChangedTargets) if err != nil { return false, fmt.Errorf("read revision treehash: %w", err) } @@ -178,7 +181,7 @@ func (c *controller) serveChangedTargetsFromCache(ctx context.Context, e *metric return false, nil } - cacheKey := cachekey.GetComparedTargetsCachePath(request.GetFirstRevision().GetRemote(), treehash1, treehash2, request.GetRequestOptions().GetExtraExcludeFilesRegex()) + cacheKey := cachekey.GetComparedTargetsCachePath(repositoryID, treehash1, treehash2, request.GetRequestOptions().GetExtraExcludeFilesRegex()) cachedReader, cacheErr := storage.NewChangedTargetsReader(ctx, c.storage, cacheKey) if cacheErr != nil && !storage.IsNotFound(cacheErr) { logger.Warn("GetChangedTargets: Failed to read from cache, proceeding to compute", zap.Error(cacheErr)) @@ -241,7 +244,7 @@ func (c *controller) serveChangedTargetsFromCache(ctx context.Context, e *metric // original failure is returned. A client disconnect surfaces as a user-cancelled // error. A graph stored as a TGB blob comes back as its undrained reader; a // gob-era graph is drained into chunks here, inside the concurrent fetch. -func (c *controller) fetchTargetGraphs(ctx context.Context, e *metrics.Emitter, logger *zap.Logger, request *pb.GetChangedTargetsRequest) (fetchedGraph, fetchedGraph, error) { +func (c *controller) fetchTargetGraphs(ctx context.Context, e *metrics.Emitter, logger *zap.Logger, request *pb.GetChangedTargetsRequest, repositoryID string) (fetchedGraph, fetchedGraph, error) { jobs := make([]*job, 2) for i := 0; i < 2; i++ { // create independent contexts for each job; if one of the jobs fails, the other one should be cancelled to save resources and improve latency @@ -284,7 +287,7 @@ func (c *controller) fetchTargetGraphs(ctx context.Context, e *metrics.Emitter, ExcludeFilesRegex: request.GetRequestOptions().GetExtraExcludeFilesRegex(), BypassCache: request.GetBypassCache(), } - graphReader, err := c.getGraph(jobs[idx].ctx, e, entityReq) + graphReader, err := c.getGraph(jobs[idx].ctx, e, entityReq, repositoryID) if err != nil || graphReader == nil { results <- graphResult{order: idx, err: err} return @@ -376,7 +379,7 @@ func (c *controller) fetchTargetGraphs(ctx context.Context, e *metrics.Emitter, // a fire-and-forget goroutine so it does not block the stream send. The responses // is only read (never mutated) by the goroutine and the foreground send, so // concurrent access is safe; the caller must not mutate it. This is best effort. -func (c *controller) cacheComparedTargets(logger *zap.Logger, request *pb.GetChangedTargetsRequest, responses []entity.GetChangedTargetsResponse) { +func (c *controller) cacheComparedTargets(logger *zap.Logger, request *pb.GetChangedTargetsRequest, repositoryID string, responses []entity.GetChangedTargetsResponse) { go func() { // Use c.appCtx directly: the cache write is fire-and-forget and must // outlive the request (so a client disconnect doesn't abort it) but @@ -388,7 +391,7 @@ func (c *controller) cacheComparedTargets(logger *zap.Logger, request *pb.GetCha // The treehash reads here are for building the write key, not a cache // serve attempt, so they pass a no-op emitter to avoid skewing the // treehash cache hit rate. - treehash1, treehash2, err := readTreehashParallel(c.appCtx, c.storage, request.GetFirstRevision(), request.GetSecondRevision(), metrics.Nop(), opGetChangedTargets) + treehash1, treehash2, err := readTreehashParallel(c.appCtx, c.storage, request.GetFirstRevision(), request.GetSecondRevision(), repositoryID, metrics.Nop(), opGetChangedTargets) if err != nil { // Goroutine outlives the handler so we can't return; log loudly and // abandon the cache write. Surfacing infra failures matters more than @@ -397,7 +400,7 @@ func (c *controller) cacheComparedTargets(logger *zap.Logger, request *pb.GetCha return } if treehash1 != "" && treehash2 != "" { - cacheKey := cachekey.GetComparedTargetsCachePath(request.GetFirstRevision().GetRemote(), treehash1, treehash2, request.GetRequestOptions().GetExtraExcludeFilesRegex()) + cacheKey := cachekey.GetComparedTargetsCachePath(repositoryID, treehash1, treehash2, request.GetRequestOptions().GetExtraExcludeFilesRegex()) if writeErr := storage.WriteChangedTargetsStream(c.appCtx, c.storage, cacheKey, responses); writeErr != nil { logger.Warn("GetChangedTargets: Failed to cache result", zap.Error(writeErr)) } @@ -749,12 +752,8 @@ func (c *controller) allTargetsChangedFromTGB(ctx context.Context, r *tgb.Reader // RepositoryConfig.SeedAttributes for the full rationale, and // attributesChanged in internal/targetdiff/compare.go for how the allowlist // is applied. -func (c *controller) seedAttributesFor(remote string) map[string]bool { - if c.repoConfig == nil { - return nil - } - cfg, ok := c.repoConfig.GetRepositoryConfig(remote) - if !ok || len(cfg.SeedAttributes) == 0 { +func seedAttributesFor(cfg config.RepositoryConfig) map[string]bool { + if len(cfg.SeedAttributes) == 0 { return nil } set := make(map[string]bool, len(cfg.SeedAttributes)) @@ -994,7 +993,7 @@ func validateGetChangedTargetsRequest(request *pb.GetChangedTargetsRequest) erro // wasting work on a result that will be discarded anyway. The cancelled sibling's error // is dropped — only the original failure is returned, so a self-inflicted // context.Canceled never masks the real reason the lookup failed. -func readTreehashParallel(ctx context.Context, st storage.Storage, first, second *pb.BuildDescription, e *metrics.Emitter, op string) (string, string, error) { +func readTreehashParallel(ctx context.Context, st storage.Storage, first, second *pb.BuildDescription, repositoryID string, e *metrics.Emitter, op string) (string, string, error) { ctx, cancel := context.WithCancel(ctx) defer cancel() @@ -1007,7 +1006,7 @@ func readTreehashParallel(ctx context.Context, st storage.Storage, first, second results := make(chan result, len(descs)) for i, desc := range descs { go func(idx int, d *pb.BuildDescription) { - hash, err := readTreehash(ctx, st, d, e, op) + hash, err := readTreehash(ctx, st, d, repositoryID, e, op) results <- result{idx: idx, hash: hash, err: err} }(i, desc) } @@ -1034,12 +1033,12 @@ func readTreehashParallel(ctx context.Context, st storage.Storage, first, second // Returns ("", nil) on a cache miss (not-found is the normal "not yet computed" state). // Returns ("", err) on any other storage or read failure so callers can decide whether to // surface the error or fall back. Returns (treehash, nil) on a successful read. -func readTreehash(ctx context.Context, st storage.Storage, buildDescription *pb.BuildDescription, e *metrics.Emitter, op string) (string, error) { +func readTreehash(ctx context.Context, st storage.Storage, buildDescription *pb.BuildDescription, repositoryID string, e *metrics.Emitter, op string) (string, error) { entityBuild, err := mapper.ProtoToBuildDescription(buildDescription) if err != nil { return "", err } - key := cachekey.GetTreehashCachePath(entityBuild) + key := cachekey.GetTreehashCachePath(repositoryID, entityBuild) resp, err := st.Get(ctx, storage.DownloadRequest{Key: key}) metrics.RecordCacheLookup(e, op, metrics.TreehashCacheLookup, err) if err != nil { diff --git a/controller/getchangedtargets_test.go b/controller/getchangedtargets_test.go index 19c74108..8347c992 100644 --- a/controller/getchangedtargets_test.go +++ b/controller/getchangedtargets_test.go @@ -303,7 +303,7 @@ func TestGetChangedTargets_ValidationError(t *testing.T) { ctrl := gomock.NewController(t) stream := tangomock.NewMockTangoServiceGetChangedTargetsYARPCServer(ctrl) - c := NewController(context.Background(), Params{Logger: zap.NewNop(), Orchestrator: orchestratormock.NewMockOrchestrator(ctrl)}) + c := NewController(context.Background(), Params{RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zap.NewNop(), Orchestrator: orchestratormock.NewMockOrchestrator(ctrl)}) err := c.GetChangedTargets(nil, stream) require.Error(t, err) @@ -336,6 +336,7 @@ func TestGetChangedTargets_CacheHit(t *testing.T) { stream.EXPECT().Send(gomock.Any()).Return(nil).Times(2) c := NewController(context.Background(), Params{ + RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), Storage: storagemock, Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), @@ -367,6 +368,7 @@ func TestGetChangedTargets_TreehashReadError(t *testing.T) { Return(storage.DownloadResponse{}, injected).Times(2) c := NewController(context.Background(), Params{ + RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zap.NewNop(), Storage: storagemock, Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), @@ -392,7 +394,7 @@ func TestReadTreehash(t *testing.T) { st.EXPECT().Get(gomock.Any(), gomock.Any()). Return(storage.DownloadResponse{}, storage.NewNotFoundError("missing")) - val, err := readTreehash(t.Context(), st, bd, metrics.Nop(), opGetChangedTargets) + val, err := readTreehash(t.Context(), st, bd, testRepositoryID(bd.GetRemote()), metrics.Nop(), opGetChangedTargets) require.NoError(t, err) assert.Empty(t, val) }) @@ -404,7 +406,7 @@ func TestReadTreehash(t *testing.T) { st.EXPECT().Get(gomock.Any(), gomock.Any()). Return(storage.DownloadResponse{}, injected) - val, err := readTreehash(t.Context(), st, bd, metrics.Nop(), opGetChangedTargets) + val, err := readTreehash(t.Context(), st, bd, testRepositoryID(bd.GetRemote()), metrics.Nop(), opGetChangedTargets) require.Error(t, err) assert.ErrorIs(t, err, injected) assert.Empty(t, val) @@ -416,7 +418,7 @@ func TestReadTreehash(t *testing.T) { st.EXPECT().Get(gomock.Any(), gomock.Any()). Return(storage.DownloadResponse{ReadCloser: io.NopCloser(strings.NewReader("deadbeef"))}, nil) - val, err := readTreehash(t.Context(), st, bd, metrics.Nop(), opGetChangedTargets) + val, err := readTreehash(t.Context(), st, bd, testRepositoryID(bd.GetRemote()), metrics.Nop(), opGetChangedTargets) require.NoError(t, err) assert.Equal(t, "deadbeef", val) }) @@ -453,6 +455,7 @@ func TestGetChangedTargets_StreamSendError(t *testing.T) { }) c := NewController(context.Background(), Params{ + RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), Storage: storagemock, Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), @@ -548,6 +551,7 @@ func TestGetChangedTargets_streamChunks(t *testing.T) { }) c := NewController(context.Background(), Params{ + RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), Storage: storagemock, Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), @@ -642,6 +646,7 @@ func TestGetChangedTargets_CacheWriteUsesAppCtx(t *testing.T) { appCtx, cancelApp := context.WithCancel(context.Background()) defer cancelApp() c := NewController(appCtx, Params{ + RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), Storage: storagemock, Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), @@ -1135,6 +1140,7 @@ func TestGetChangedTargets_CacheHitWithDistanceFilter(t *testing.T) { }).Times(2) c := NewController(context.Background(), Params{ + RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), Storage: storagemock, Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), @@ -1408,7 +1414,7 @@ func TestServeChangedTargetsFromCache(t *testing.T) { c.storage = st stream := tangomock.NewMockTangoServiceGetChangedTargetsYARPCServer(ctrl) - served, err := c.serveChangedTargetsFromCache(t.Context(), c.emitter, c.logger, changedTargetsRequest(), stream, -1, time.Now()) + served, err := c.serveChangedTargetsFromCache(t.Context(), c.emitter, c.logger, changedTargetsRequest(), stream, testRepositoryID("repo:go-code"), -1, time.Now()) require.NoError(t, err) assert.False(t, served, "a cache miss must not be served") }) @@ -1447,7 +1453,7 @@ func TestServeChangedTargetsFromCache(t *testing.T) { stream := tangomock.NewMockTangoServiceGetChangedTargetsYARPCServer(ctrl) // No Send expectation: a corrupt blob must not send anything to the client. - served, err := c.serveChangedTargetsFromCache(t.Context(), c.emitter, c.logger, changedTargetsRequest(), stream, -1, time.Now()) + served, err := c.serveChangedTargetsFromCache(t.Context(), c.emitter, c.logger, changedTargetsRequest(), stream, testRepositoryID("repo:go-code"), -1, time.Now()) require.NoError(t, err) assert.False(t, served, "a corrupt blob must trigger recompute, not a partial send") }) @@ -1481,7 +1487,7 @@ func TestServeChangedTargetsFromCache(t *testing.T) { stream := tangomock.NewMockTangoServiceGetChangedTargetsYARPCServer(ctrl) stream.EXPECT().Send(gomock.Any()).Return(nil).Times(2) - served, err := c.serveChangedTargetsFromCache(t.Context(), c.emitter, c.logger, changedTargetsRequest(), stream, -1, time.Now()) + served, err := c.serveChangedTargetsFromCache(t.Context(), c.emitter, c.logger, changedTargetsRequest(), stream, testRepositoryID("repo:go-code"), -1, time.Now()) require.NoError(t, err) assert.True(t, served, "a clean cache hit must be served") }) @@ -1508,7 +1514,7 @@ func TestFetchTargetGraphs(t *testing.T) { c := newTestController(zaptest.NewLogger(t)) c.orchestrator = orch - first, second, err := c.fetchTargetGraphs(t.Context(), c.emitter, c.logger, bypassRequest()) + first, second, err := c.fetchTargetGraphs(t.Context(), c.emitter, c.logger, bypassRequest(), testRepositoryID("repo:go-code")) require.NoError(t, err) require.Len(t, first.chunks, 1) require.Len(t, second.chunks, 1) @@ -1532,7 +1538,7 @@ func TestFetchTargetGraphs(t *testing.T) { c := newTestController(zaptest.NewLogger(t)) c.orchestrator = orch - first, second, err := c.fetchTargetGraphs(t.Context(), c.emitter, c.logger, bypassRequest()) + first, second, err := c.fetchTargetGraphs(t.Context(), c.emitter, c.logger, bypassRequest(), testRepositoryID("repo:go-code")) require.Error(t, err) assert.ErrorIs(t, err, injected) assert.Zero(t, first) @@ -1553,7 +1559,7 @@ func TestFetchTargetGraphs(t *testing.T) { c := newTestController(zaptest.NewLogger(t)) c.orchestrator = orch - _, _, err := c.fetchTargetGraphs(t.Context(), c.emitter, c.logger, bypassRequest()) + _, _, err := c.fetchTargetGraphs(t.Context(), c.emitter, c.logger, bypassRequest(), testRepositoryID("repo:go-code")) require.Error(t, err) }) @@ -1568,7 +1574,7 @@ func TestFetchTargetGraphs(t *testing.T) { c := newTestController(zaptest.NewLogger(t)) c.orchestrator = orch - _, _, err := c.fetchTargetGraphs(t.Context(), c.emitter, c.logger, bypassRequest()) + _, _, err := c.fetchTargetGraphs(t.Context(), c.emitter, c.logger, bypassRequest(), testRepositoryID("repo:go-code")) require.Error(t, err) }) } @@ -1651,37 +1657,29 @@ type fakeRepoConfigProvider map[string]config.RepositoryConfig func (f fakeRepoConfigProvider) GetRepositoryConfig(remote string) (config.RepositoryConfig, bool) { cfg, ok := f[remote] + if ok && cfg.RepositoryID == "" { + cfg.RepositoryID = "test-repository" + } return cfg, ok } func TestSeedAttributesFor(t *testing.T) { t.Run("no repo config provider means no filtering", func(t *testing.T) { - c := newTestController(zaptest.NewLogger(t)) - assert.Nil(t, c.seedAttributesFor("some-remote")) + assert.Nil(t, seedAttributesFor(config.RepositoryConfig{})) }) t.Run("remote not found means no filtering", func(t *testing.T) { - c := newTestController(zaptest.NewLogger(t)) - c.repoConfig = fakeRepoConfigProvider{} - assert.Nil(t, c.seedAttributesFor("some-remote")) + assert.Nil(t, seedAttributesFor(config.RepositoryConfig{})) }) t.Run("configured remote with no seed_attributes means no filtering", func(t *testing.T) { - c := newTestController(zaptest.NewLogger(t)) - c.repoConfig = fakeRepoConfigProvider{ - "some-remote": config.RepositoryConfig{Remote: "some-remote"}, - } - assert.Nil(t, c.seedAttributesFor("some-remote")) + assert.Nil(t, seedAttributesFor(config.RepositoryConfig{Remote: "some-remote"})) }) t.Run("configured remote with seed_attributes returns allowlist", func(t *testing.T) { - c := newTestController(zaptest.NewLogger(t)) - c.repoConfig = fakeRepoConfigProvider{ - "some-remote": config.RepositoryConfig{ - Remote: "some-remote", - SeedAttributes: []string{"size", "timeout"}, - }, - } - assert.Equal(t, map[string]bool{"size": true, "timeout": true}, c.seedAttributesFor("some-remote")) + assert.Equal(t, map[string]bool{"size": true, "timeout": true}, seedAttributesFor(config.RepositoryConfig{ + Remote: "some-remote", + SeedAttributes: []string{"size", "timeout"}, + })) }) } diff --git a/controller/getchangedtargets_tgb_test.go b/controller/getchangedtargets_tgb_test.go index b8df732b..bda81e40 100644 --- a/controller/getchangedtargets_tgb_test.go +++ b/controller/getchangedtargets_tgb_test.go @@ -62,7 +62,7 @@ func tgbTestGraphChunks(hash2 string) []entity.GetTargetGraphResponse { // seedTreehash stores the sha→treehash mapping the request resolution reads. func seedTreehash(t *testing.T, st storage.Storage, baseSha, treehash string) { t.Helper() - key := cachekey.GetTreehashCachePath(entity.BuildDescription{Remote: "repo:go-code", BaseSha: baseSha}) + key := cachekey.GetTreehashCachePath(testRepositoryID("repo:go-code"), entity.BuildDescription{Remote: "repo:go-code", BaseSha: baseSha}) require.NoError(t, st.Put(t.Context(), storage.UploadRequest{Key: key, Reader: bytes.NewReader([]byte(treehash))})) } @@ -116,14 +116,15 @@ func TestGetChangedTargets_TGBNativePath(t *testing.T) { seedTreehash(t, st, "sha1", "treehash1") seedTreehash(t, st, "sha2", "treehash2") require.NoError(t, storage.WriteTGBGraph(t.Context(), st, - cachekey.GetTGBGraphByTreeHash("repo:go-code", "treehash1", entity.ComputationStrategyUnset, nil), + cachekey.GetTGBGraphByTreeHash(testRepositoryID("repo:go-code"), "treehash1", entity.ComputationStrategyUnset, nil), tgbTestGraphChunks(tgbHash2Old))) require.NoError(t, storage.WriteTGBGraph(t.Context(), st, - cachekey.GetTGBGraphByTreeHash("repo:go-code", "treehash2", entity.ComputationStrategyUnset, nil), + cachekey.GetTGBGraphByTreeHash(testRepositoryID("repo:go-code"), "treehash2", entity.ComputationStrategyUnset, nil), tgbTestGraphChunks(tgbHash2New))) scope := tally.NewTestScope("", nil) c := NewController(context.Background(), Params{ + RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), Storage: st, Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), // no calls expected: both graphs are cached @@ -188,14 +189,15 @@ func TestGetChangedTargets_TGBAllTargetsTrigger(t *testing.T) { seedTreehash(t, st, "sha1", "treehash1") seedTreehash(t, st, "sha2", "treehash2") require.NoError(t, storage.WriteTGBGraph(t.Context(), st, - cachekey.GetTGBGraphByTreeHash("repo:go-code", "treehash1", entity.ComputationStrategyUnset, nil), + cachekey.GetTGBGraphByTreeHash(testRepositoryID("repo:go-code"), "treehash1", entity.ComputationStrategyUnset, nil), tgbTestGraphChunksWithATFH(tgbHash2Old, map[string]string{".bazelrc": "old-hash"}))) require.NoError(t, storage.WriteTGBGraph(t.Context(), st, - cachekey.GetTGBGraphByTreeHash("repo:go-code", "treehash2", entity.ComputationStrategyUnset, nil), + cachekey.GetTGBGraphByTreeHash(testRepositoryID("repo:go-code"), "treehash2", entity.ComputationStrategyUnset, nil), tgbTestGraphChunksWithATFH(tgbHash2Old, map[string]string{".bazelrc": "new-hash"}))) scope := tally.NewTestScope("", nil) c := NewController(context.Background(), Params{ + RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), Storage: st, Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), @@ -234,14 +236,15 @@ func TestGetChangedTargets_TGBAllTargetsNoTrigger(t *testing.T) { seedTreehash(t, st, "sha1", "treehash1") seedTreehash(t, st, "sha2", "treehash2") require.NoError(t, storage.WriteTGBGraph(t.Context(), st, - cachekey.GetTGBGraphByTreeHash("repo:go-code", "treehash1", entity.ComputationStrategyUnset, nil), + cachekey.GetTGBGraphByTreeHash(testRepositoryID("repo:go-code"), "treehash1", entity.ComputationStrategyUnset, nil), tgbTestGraphChunksWithATFH(tgbHash2Old, map[string]string{".bazelrc": "same-hash"}))) require.NoError(t, storage.WriteTGBGraph(t.Context(), st, - cachekey.GetTGBGraphByTreeHash("repo:go-code", "treehash2", entity.ComputationStrategyUnset, nil), + cachekey.GetTGBGraphByTreeHash(testRepositoryID("repo:go-code"), "treehash2", entity.ComputationStrategyUnset, nil), tgbTestGraphChunksWithATFH(tgbHash2New, map[string]string{".bazelrc": "same-hash"}))) scope := tally.NewTestScope("", nil) c := NewController(context.Background(), Params{ + RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), Storage: st, Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), @@ -279,14 +282,15 @@ func TestGetChangedTargets_TGBMixedFormatFallsBack(t *testing.T) { seedTreehash(t, st, "sha2", "treehash2") // First revision predates the flip: gob only, at the gob key. require.NoError(t, storage.WriteGraphStream(t.Context(), st, - cachekey.GetGraphByTreeHash("repo:go-code", "treehash1", entity.ComputationStrategyUnset, nil), + cachekey.GetGraphByTreeHash(testRepositoryID("repo:go-code"), "treehash1", entity.ComputationStrategyUnset, nil), tgbTestGraphChunks(tgbHash2Old))) require.NoError(t, storage.WriteTGBGraph(t.Context(), st, - cachekey.GetTGBGraphByTreeHash("repo:go-code", "treehash2", entity.ComputationStrategyUnset, nil), + cachekey.GetTGBGraphByTreeHash(testRepositoryID("repo:go-code"), "treehash2", entity.ComputationStrategyUnset, nil), tgbTestGraphChunks(tgbHash2New))) scope := tally.NewTestScope("", nil) c := NewController(context.Background(), Params{ + RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), Storage: st, Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), diff --git a/controller/gettargetgraph.go b/controller/gettargetgraph.go index e59ce2ea..cbe494f2 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,15 @@ 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) + if mappingErr != nil { + mappingErr = tangoerrors.NewUser(fmt.Errorf("convert get target graph request: %w", mappingErr)) + } + repoCfg, repo, repositoryErr := c.resolveRequestRepository(entityReq.Build.Remote, mappingErr) 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,11 +55,10 @@ 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 repositoryErr != nil { + return repositoryErr } - graphReader, err := c.getGraph(ctx, e, entityReq) + graphReader, err := c.getGraph(ctx, e, entityReq, repoCfg.RepositoryID) if err != nil { return fmt.Errorf("get graph: %w", err) } @@ -96,14 +98,15 @@ func (c *controller) GetTargetGraph(request *pb.GetTargetGraphRequest, stream pb // entries store the full payload and stripping happens at send time, so // letting an orchestrator see it could poison the shared cache with // stripped graphs. -func (c *controller) getGraph(ctx context.Context, e *metrics.Emitter, req entity.GetTargetGraphRequest) (storage.GraphReader, error) { +func (c *controller) getGraph(ctx context.Context, e *metrics.Emitter, req entity.GetTargetGraphRequest, repositoryID string) (storage.GraphReader, error) { start := time.Now() logger := c.logger.With( - zap.Any("build_description", req.Build), + zap.String("base_sha", req.Build.BaseSha), + zap.Stringer("strategy", req.Build.Strategy), ) if !req.BypassCache { // Look up the the git treehash based on cache path - treehashCachePath := cachekey.GetTreehashCachePath(req.Build) + treehashCachePath := cachekey.GetTreehashCachePath(repositoryID, req.Build) treehashResponse, err := c.storage.Get(ctx, storage.DownloadRequest{Key: treehashCachePath}) metrics.RecordCacheLookup(e, opGetTargetGraph, metrics.TreehashCacheLookup, err) if err != nil { @@ -123,7 +126,7 @@ func (c *controller) getGraph(ctx context.Context, e *metrics.Emitter, req entit logger.Info("getGraph: treehash found") // Download the target graph based on treehash. storageStart := time.Now() - graphReader, err := c.readCachedGraph(ctx, logger, req.Build.Remote, string(treehashBytes), req.Build.Strategy, req.ExcludeFilesRegex) + graphReader, err := c.readCachedGraph(ctx, logger, repositoryID, string(treehashBytes), req.Build.Strategy, req.ExcludeFilesRegex) if ctx.Err() != nil { err = context.Cause(ctx) } @@ -166,9 +169,9 @@ func (c *controller) getGraph(ctx context.Context, e *metrics.Emitter, req entit // exists but fails validation is treated as a miss (the orchestrator will // recompute and overwrite it), not an infra failure. Returns a not-found // error when neither format is present. -func (c *controller) readCachedGraph(ctx context.Context, logger *zap.Logger, remote, treehash string, strategy entity.ComputationStrategy, excludeFilesRegex []string) (storage.GraphReader, error) { +func (c *controller) readCachedGraph(ctx context.Context, logger *zap.Logger, repositoryID, treehash string, strategy entity.ComputationStrategy, excludeFilesRegex []string) (storage.GraphReader, error) { if c.graphFormat == config.GraphFormatTGB { - tgbPath := cachekey.GetTGBGraphByTreeHash(remote, treehash, strategy, excludeFilesRegex) + tgbPath := cachekey.GetTGBGraphByTreeHash(repositoryID, treehash, strategy, excludeFilesRegex) graphReader, err := storage.NewTGBGraphReader(ctx, c.storage, tgbPath, c.maxMessageBytes) if err == nil { return graphReader, nil @@ -179,6 +182,6 @@ func (c *controller) readCachedGraph(ctx context.Context, logger *zap.Logger, re return nil, err } } - gobPath := cachekey.GetGraphByTreeHash(remote, treehash, strategy, excludeFilesRegex) + gobPath := cachekey.GetGraphByTreeHash(repositoryID, treehash, strategy, excludeFilesRegex) return storage.NewGraphReader(ctx, c.storage, gobPath) } diff --git a/controller/gettargetgraph_test.go b/controller/gettargetgraph_test.go index 1eeac959..260268a2 100644 --- a/controller/gettargetgraph_test.go +++ b/controller/gettargetgraph_test.go @@ -47,8 +47,9 @@ func TestGetTargetGraph_CacheMiss_NoSend(t *testing.T) { Return(storage.DownloadResponse{ReadCloser: newMockReadCloser([]byte{})}, nil), ) c := NewController(context.Background(), Params{ - Logger: zaptest.NewLogger(t), - Storage: store, + RepoConfig: allowAnyRepositoryConfigProvider{}, + Logger: zaptest.NewLogger(t), + Storage: store, }) req := &pb.GetTargetGraphRequest{ BuildDescription: &pb.BuildDescription{ @@ -73,8 +74,9 @@ func TestGetTargetGraph_StorageError_Propagates(t *testing.T) { storagemock := storagemock.NewMockStorage(ctrl) storagemock.EXPECT().Get(gomock.Any(), gomock.Any()).Return(storage.DownloadResponse{}, expected) c := NewController(context.Background(), Params{ - Logger: zaptest.NewLogger(t), - Storage: storagemock, + RepoConfig: allowAnyRepositoryConfigProvider{}, + Logger: zaptest.NewLogger(t), + Storage: storagemock, }) err := c.GetTargetGraph(&pb.GetTargetGraphRequest{ BuildDescription: &pb.BuildDescription{ @@ -100,8 +102,9 @@ func TestGetTargetGraph_DecodeError_ReturnsError(t *testing.T) { storagemock.EXPECT().Get(gomock.Any(), gomock.Any()).Return(storage.DownloadResponse{ReadCloser: newMockReadCloser([]byte("bad-bytes"))}, nil), ) c := NewController(context.Background(), Params{ - Logger: zaptest.NewLogger(t), - Storage: storagemock, + RepoConfig: allowAnyRepositoryConfigProvider{}, + Logger: zaptest.NewLogger(t), + Storage: storagemock, }) err := c.GetTargetGraph(&pb.GetTargetGraphRequest{ BuildDescription: &pb.BuildDescription{ @@ -131,8 +134,9 @@ func TestGetTargetGraph_SendsWhenItemPresent(t *testing.T) { store.EXPECT().Get(gomock.Any(), gomock.Any()).Return(storage.DownloadResponse{ReadCloser: newMockReadCloser(buf.Bytes())}, nil), ) c := NewController(context.Background(), Params{ - Logger: zaptest.NewLogger(t), - Storage: store, + RepoConfig: allowAnyRepositoryConfigProvider{}, + Logger: zaptest.NewLogger(t), + Storage: store, }) err := c.GetTargetGraph(&pb.GetTargetGraphRequest{ BuildDescription: &pb.BuildDescription{ @@ -154,8 +158,9 @@ func TestGetTargetGraph_BuildDescriptionMissingRequiredFields_ReturnsError(t *te stream.EXPECT().Context().Return(context.Background()) store := storagemock.NewMockStorage(ctrl) c := NewController(context.Background(), Params{ - Logger: zaptest.NewLogger(t), - Storage: store, + RepoConfig: allowAnyRepositoryConfigProvider{}, + Logger: zaptest.NewLogger(t), + Storage: store, }) err := c.GetTargetGraph(&pb.GetTargetGraphRequest{ BuildDescription: &pb.BuildDescription{ @@ -176,8 +181,9 @@ func TestGetTargetGraph_MissingBuildDescription_ReturnsError(t *testing.T) { stream.EXPECT().Context().Return(context.Background()) store := storagemock.NewMockStorage(ctrl) c := NewController(context.Background(), Params{ - Logger: zaptest.NewLogger(t), - Storage: store, + RepoConfig: allowAnyRepositoryConfigProvider{}, + Logger: zaptest.NewLogger(t), + Storage: store, }) err := c.GetTargetGraph(&pb.GetTargetGraphRequest{}, stream) assert.Error(t, err) @@ -196,6 +202,7 @@ func TestGetTargetGraph_TreehashNotFound_NoError(t *testing.T) { graphReader := newGraphReader(t, entity.GetTargetGraphResponse{Targets: []entity.OptimizedTarget{}}) orchestrator.EXPECT().GetTargetGraph(gomock.Any(), gomock.Any()).Return(graphReader, nil) c := NewController(context.Background(), Params{ + RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), Storage: store, Orchestrator: orchestrator, @@ -214,8 +221,9 @@ func TestGetTargetGraph_TreehashReadError(t *testing.T) { store := storagemock.NewMockStorage(ctrl) store.EXPECT().Get(gomock.Any(), gomock.Any()).Return(storage.DownloadResponse{ReadCloser: &errReadCloser{err: errors.New("readfail")}}, nil) c := NewController(context.Background(), Params{ - Logger: zaptest.NewLogger(t), - Storage: store, + RepoConfig: allowAnyRepositoryConfigProvider{}, + Logger: zaptest.NewLogger(t), + Storage: store, }) err := c.GetTargetGraph(&pb.GetTargetGraphRequest{ BuildDescription: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha"}, @@ -234,8 +242,9 @@ func TestGetTargetGraph_GraphFetchError(t *testing.T) { store.EXPECT().Get(gomock.Any(), gomock.Any()).Return(storage.DownloadResponse{}, errors.New("graph error")), ) c := NewController(context.Background(), Params{ - Logger: zaptest.NewLogger(t), - Storage: store, + RepoConfig: allowAnyRepositoryConfigProvider{}, + Logger: zaptest.NewLogger(t), + Storage: store, }) err := c.GetTargetGraph(&pb.GetTargetGraphRequest{ BuildDescription: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha"}, @@ -254,8 +263,9 @@ func TestGetTargetGraph_GraphReadError(t *testing.T) { store.EXPECT().Get(gomock.Any(), gomock.Any()).Return(storage.DownloadResponse{ReadCloser: &errReadCloser{err: errors.New("readfail")}}, nil), ) c := NewController(context.Background(), Params{ - Logger: zaptest.NewLogger(t), - Storage: store, + RepoConfig: allowAnyRepositoryConfigProvider{}, + Logger: zaptest.NewLogger(t), + Storage: store, }) err := c.GetTargetGraph(&pb.GetTargetGraphRequest{ BuildDescription: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha"}, @@ -279,8 +289,9 @@ func TestGetTargetGraph_StreamSendError(t *testing.T) { storagemock.EXPECT().Get(gomock.Any(), gomock.Any()).Return(storage.DownloadResponse{ReadCloser: newMockReadCloser(buf.Bytes())}, nil), ) c := NewController(context.Background(), Params{ - Logger: zaptest.NewLogger(t), - Storage: storagemock, + RepoConfig: allowAnyRepositoryConfigProvider{}, + Logger: zaptest.NewLogger(t), + Storage: storagemock, }) err := c.GetTargetGraph(&pb.GetTargetGraphRequest{ BuildDescription: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha"}, @@ -303,6 +314,7 @@ func TestGetTargetGraph_GraphNotFound_FallsThrough(t *testing.T) { graphReader := newGraphReader(t, entity.GetTargetGraphResponse{Targets: []entity.OptimizedTarget{}}) orch.EXPECT().GetTargetGraph(gomock.Any(), gomock.Any()).Return(graphReader, nil) c := NewController(context.Background(), Params{ + RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), Storage: store, Orchestrator: orch, @@ -325,8 +337,9 @@ func TestGetTargetGraph_GraphReadCancelled(t *testing.T) { store.EXPECT().Get(gomock.Any(), gomock.Any()).Return(storage.DownloadResponse{}, errors.New("context canceled")), ) c := NewController(context.Background(), Params{ - Logger: zaptest.NewLogger(t), - Storage: store, + RepoConfig: allowAnyRepositoryConfigProvider{}, + Logger: zaptest.NewLogger(t), + Storage: store, }) err := c.GetTargetGraph(&pb.GetTargetGraphRequest{ BuildDescription: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha"}, @@ -345,6 +358,7 @@ func TestGetTargetGraph_OrchestratorCancelled(t *testing.T) { orch := orchestratormock.NewMockOrchestrator(ctrl) orch.EXPECT().GetTargetGraph(gomock.Any(), gomock.Any()).Return(nil, errors.New("context canceled")) c := NewController(context.Background(), Params{ + RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), Storage: store, Orchestrator: orch, diff --git a/controller/testhelper_test.go b/controller/testhelper_test.go index d44b9dc6..71b59f66 100644 --- a/controller/testhelper_test.go +++ b/controller/testhelper_test.go @@ -26,11 +26,25 @@ import ( "go.uber.org/zap" ) +type allowAnyRepositoryConfigProvider struct{} + +func (allowAnyRepositoryConfigProvider) GetRepositoryConfig(remote string) (config.RepositoryConfig, bool) { + return config.RepositoryConfig{ + Remote: remote, + RepositoryID: "test-repository", + }, true +} + +func testRepositoryID(string) string { + return "test-repository" +} + func newTestController(logger *zap.Logger) *controller { return &controller{ logger: logger, emitter: metrics.Nop(), maxMessageBytes: config.DefaultMaxMessageBytes, + repoConfig: allowAnyRepositoryConfigProvider{}, appCtx: context.Background(), } } diff --git a/controller/wireerror_test.go b/controller/wireerror_test.go index 8bee7809..ae42f33c 100644 --- a/controller/wireerror_test.go +++ b/controller/wireerror_test.go @@ -89,7 +89,8 @@ func TestGetTargetGraph_ValidationError_WiresTangoError(t *testing.T) { stream.EXPECT().Context().Return(context.Background()) c := NewController(context.Background(), Params{ - Logger: zaptest.NewLogger(t), + RepoConfig: allowAnyRepositoryConfigProvider{}, + Logger: zaptest.NewLogger(t), }) // Missing BaseSha triggers a validation error classified as ERROR_USER. @@ -117,7 +118,8 @@ func TestGetChangedTargets_ValidationError_WiresTangoError(t *testing.T) { stream.EXPECT().Context().Return(context.Background()).AnyTimes() c := NewController(context.Background(), Params{ - Logger: zaptest.NewLogger(t), + RepoConfig: allowAnyRepositoryConfigProvider{}, + Logger: zaptest.NewLogger(t), }) // Missing first revision triggers validation error classified as ERROR_USER. @@ -145,8 +147,9 @@ func TestGetTargetGraph_InfraError_WiresTangoError(t *testing.T) { store.EXPECT().Get(gomock.Any(), gomock.Any()).Return(storage.DownloadResponse{}, errors.New("disk on fire")) c := NewController(context.Background(), Params{ - Logger: zaptest.NewLogger(t), - Storage: store, + RepoConfig: allowAnyRepositoryConfigProvider{}, + Logger: zaptest.NewLogger(t), + Storage: store, }) err := c.GetTargetGraph(&pb.GetTargetGraphRequest{ diff --git a/core/cachekey/cachekey.go b/core/cachekey/cachekey.go index 8004fe65..f2cbbfd9 100644 --- a/core/cachekey/cachekey.go +++ b/core/cachekey/cachekey.go @@ -33,8 +33,8 @@ import ( // SHELL vs NATIVE) can produce different graphs from the same tree state. // 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()) +func GetGraphByTreeHash(repositoryID, treehash string, strategy entity.ComputationStrategy, excludeFilesRegex []string) string { + path := filepath.Join(repositoryID, "graphs", treehash, strategy.String()) if hash := hashExcludeFilesRegex(excludeFilesRegex); hash != "" { path += "_requests-options-" + hash } @@ -49,8 +49,8 @@ func GetGraphByTreeHash(remote, treehash string, strategy entity.ComputationStra // existing blob is interpreted. The suffix keeps the key a sibling of the // 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") +func GetTGBGraphByTreeHash(repositoryID, treehash string, strategy entity.ComputationStrategy, excludeFilesRegex []string) string { + path := filepath.Join(repositoryID, "graphs", treehash, strategy.String()+"-tgb") if hash := hashExcludeFilesRegex(excludeFilesRegex); hash != "" { path += "_requests-options-" + hash } @@ -61,8 +61,8 @@ func GetTGBGraphByTreeHash(remote, treehash string, strategy entity.ComputationS // The git treehash is purely a function of git state (base SHA + applied // 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)) +func GetTreehashCachePath(repositoryID string, buildDescription entity.BuildDescription) string { + path := filepath.Join(repositoryID, "treehashes", fmt.Sprintf("base-sha-%s", buildDescription.BaseSha)) if len(buildDescription.ChangeRequests) > 0 { path += "_request-urls-" + url.GetReqURLsHash(buildDescription.ChangeRequests) } @@ -71,11 +71,11 @@ func GetTreehashCachePath(buildDescription entity.BuildDescription) string { // GetComparedTargetsCachePath returns the cache path for a compared target graph result. // treehash1 and treehash2 are the resolved treehashes of the first and second revisions. -// remote is the shared git remote for both revisions. +// repositoryID is the configured namespace shared by both revisions. // 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) +func GetComparedTargetsCachePath(repositoryID, treehash1, treehash2 string, excludeFilesRegex []string) string { + path := filepath.Join(repositoryID, "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..fcabbc44 100644 --- a/core/cachekey/cachekey_test.go +++ b/core/cachekey/cachekey_test.go @@ -25,26 +25,26 @@ import ( func TestGetGraphByTreeHash(t *testing.T) { t.Parallel() - remote := "git@github:uber/tango" treehash := "abcd1234" strategy := entity.ComputationStrategyNative + repositoryID := "test-repository" // 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, got, GetGraphByTreeHash(remote, treehash, strategy, []string{})) + got := GetGraphByTreeHash(repositoryID, treehash, strategy, nil) + assert.Equal(t, filepath.Join(repositoryID, "graphs", treehash, strategy.String()), got) + assert.Equal(t, got, GetGraphByTreeHash(repositoryID, treehash, strategy, []string{})) // Different strategies ⇒ different keys. - assert.NotEqual(t, got, GetGraphByTreeHash(remote, treehash, entity.ComputationStrategyShell, nil)) + assert.NotEqual(t, got, GetGraphByTreeHash(repositoryID, treehash, entity.ComputationStrategyShell, nil)) // Non-empty list ⇒ suffix appended; different lists ⇒ different keys. - withFoo := GetGraphByTreeHash(remote, treehash, strategy, []string{"foo.*"}) + withFoo := GetGraphByTreeHash(repositoryID, treehash, strategy, []string{"foo.*"}) assert.NotEqual(t, got, withFoo) - assert.NotEqual(t, withFoo, GetGraphByTreeHash(remote, treehash, strategy, []string{"bar.*"})) + assert.NotEqual(t, withFoo, GetGraphByTreeHash(repositoryID, treehash, strategy, []string{"bar.*"})) // Order-independence: sort before hashing. assert.Equal(t, - GetGraphByTreeHash(remote, treehash, strategy, []string{"a", "b"}), - GetGraphByTreeHash(remote, treehash, strategy, []string{"b", "a"}), + GetGraphByTreeHash(repositoryID, treehash, strategy, []string{"a", "b"}), + GetGraphByTreeHash(repositoryID, treehash, strategy, []string{"b", "a"}), ) } @@ -58,42 +58,44 @@ func TestGetTreehashCachePath(t *testing.T) { {URL: "github://github.com/org/repo/pull/2/2222222222222222222222222222222222222222"}, }, } - got := GetTreehashCachePath(desc) - want := filepath.Join("uber/tango", "treehashes", "base-sha-deadbeef") + "_request-urls-" + url.GetReqURLsHash(desc.ChangeRequests) + repositoryID := "test-repository" + got := GetTreehashCachePath(repositoryID, desc) + want := filepath.Join(repositoryID, "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) + repositoryID := "test-repository" + got := GetComparedTargetsCachePath(repositoryID, "abc", "def", nil) + assert.Equal(t, filepath.Join(repositoryID, "compared-targets", "abc_def"), got) // Nil/empty list ⇒ legacy path. - assert.Equal(t, got, GetComparedTargetsCachePath("git@github:uber/tango", "abc", "def", []string{})) + assert.Equal(t, got, GetComparedTargetsCachePath(repositoryID, "abc", "def", []string{})) // Different exclude lists ⇒ different keys. - assert.NotEqual(t, got, GetComparedTargetsCachePath("git@github:uber/tango", "abc", "def", []string{"foo.*"})) + assert.NotEqual(t, got, GetComparedTargetsCachePath(repositoryID, "abc", "def", []string{"foo.*"})) } func TestGetTGBGraphByTreeHash(t *testing.T) { t.Parallel() - remote := "git@github:uber/tango" treehash := "abcd1234" strategy := entity.ComputationStrategyNative + repositoryID := "test-repository" - got := GetTGBGraphByTreeHash(remote, treehash, strategy, nil) - assert.Equal(t, filepath.Join("uber/tango", "graphs", treehash, strategy.String()+"-tgb"), got) - assert.Equal(t, got, GetTGBGraphByTreeHash(remote, treehash, strategy, []string{})) + got := GetTGBGraphByTreeHash(repositoryID, treehash, strategy, nil) + assert.Equal(t, filepath.Join(repositoryID, "graphs", treehash, strategy.String()+"-tgb"), got) + assert.Equal(t, got, GetTGBGraphByTreeHash(repositoryID, treehash, strategy, []string{})) // Never collides with the gob key space, and stays a sibling (same // directory) rather than a child of the gob key — on the disk backend a // key is a file, so a child key could not coexist with the gob blob. - gob := GetGraphByTreeHash(remote, treehash, strategy, nil) + gob := GetGraphByTreeHash(repositoryID, treehash, strategy, nil) assert.NotEqual(t, gob, got) assert.Equal(t, filepath.Dir(gob), filepath.Dir(got)) // Exclude-files regex folds into the key the same way as the gob variant. - withRegex := GetTGBGraphByTreeHash(remote, treehash, strategy, []string{"^docs/"}) + withRegex := GetTGBGraphByTreeHash(repositoryID, treehash, strategy, []string{"^docs/"}) assert.NotEqual(t, got, withRegex) assert.Contains(t, withRegex, "_requests-options-") } diff --git a/core/cachekey/exclude_regex_rapid_test.go b/core/cachekey/exclude_regex_rapid_test.go index b089fd96..423f7078 100644 --- a/core/cachekey/exclude_regex_rapid_test.go +++ b/core/cachekey/exclude_regex_rapid_test.go @@ -33,16 +33,17 @@ func TestExcludeFilesRegex_keyProperties(t *testing.T) { rightBefore := append([]string(nil), right...) leftPermuted := []string{left[1], left[0]} rightPermuted := []string{right[1], right[0]} + repositoryID := "test-repository" - leftGraph := GetGraphByTreeHash("git@github:uber/tango", "tree", entity.ComputationStrategyNative, left) - rightGraph := GetGraphByTreeHash("git@github:uber/tango", "tree", entity.ComputationStrategyNative, right) - require.Equal(t, leftGraph, GetGraphByTreeHash("git@github:uber/tango", "tree", entity.ComputationStrategyNative, leftPermuted)) + leftGraph := GetGraphByTreeHash(repositoryID, "tree", entity.ComputationStrategyNative, left) + rightGraph := GetGraphByTreeHash(repositoryID, "tree", entity.ComputationStrategyNative, right) + require.Equal(t, leftGraph, GetGraphByTreeHash(repositoryID, "tree", entity.ComputationStrategyNative, leftPermuted)) - leftCompared := GetComparedTargetsCachePath("git@github:uber/tango", "before", "after", left) - rightCompared := GetComparedTargetsCachePath("git@github:uber/tango", "before", "after", right) - require.Equal(t, leftCompared, GetComparedTargetsCachePath("git@github:uber/tango", "before", "after", leftPermuted)) - require.Equal(t, rightGraph, GetGraphByTreeHash("git@github:uber/tango", "tree", entity.ComputationStrategyNative, rightPermuted)) - require.Equal(t, rightCompared, GetComparedTargetsCachePath("git@github:uber/tango", "before", "after", rightPermuted)) + leftCompared := GetComparedTargetsCachePath(repositoryID, "before", "after", left) + rightCompared := GetComparedTargetsCachePath(repositoryID, "before", "after", right) + require.Equal(t, leftCompared, GetComparedTargetsCachePath(repositoryID, "before", "after", leftPermuted)) + require.Equal(t, rightGraph, GetGraphByTreeHash(repositoryID, "tree", entity.ComputationStrategyNative, rightPermuted)) + require.Equal(t, rightCompared, GetComparedTargetsCachePath(repositoryID, "before", "after", rightPermuted)) require.Equal(t, leftBefore, left) require.Equal(t, rightBefore, right) require.NotEqual(t, leftGraph, rightGraph) diff --git a/core/cachekey/treehash_rapid_test.go b/core/cachekey/treehash_rapid_test.go index 0eed349d..b84e5f96 100644 --- a/core/cachekey/treehash_rapid_test.go +++ b/core/cachekey/treehash_rapid_test.go @@ -38,7 +38,8 @@ func TestGetTreehashCachePath_headSHAAffectsKey(t *testing.T) { {URL: "github://github.com/uber/tango/pull/1/" + otherSHA}, } - require.NotEqual(t, GetTreehashCachePath(build), GetTreehashCachePath(changed)) + repositoryID := "test-repository" + require.NotEqual(t, GetTreehashCachePath(repositoryID, build), GetTreehashCachePath(repositoryID, changed)) }) } @@ -60,6 +61,7 @@ func TestGetTreehashCachePath_changeRequestOrderAffectsKey(t *testing.T) { build.ChangeRequests[0], } - require.NotEqual(t, GetTreehashCachePath(build), GetTreehashCachePath(swapped)) + repositoryID := "test-repository" + require.NotEqual(t, GetTreehashCachePath(repositoryID, build), GetTreehashCachePath(repositoryID, swapped)) }) } diff --git a/core/repomanager/BUILD.bazel b/core/repomanager/BUILD.bazel index 9e8e2e1c..ee2f73c2 100644 --- a/core/repomanager/BUILD.bazel +++ b/core/repomanager/BUILD.bazel @@ -9,10 +9,10 @@ go_library( importpath = "github.com/uber/tango/core/repomanager", visibility = ["//visibility:public"], deps = [ + "//config", "//core/git", "//core/workspace", "//entity", - "//internal/url", "//observability/metrics", "@com_github_uber_go_tally//:tally", "@org_uber_go_zap//:zap", @@ -24,6 +24,7 @@ go_test( srcs = ["repo_manager_test.go"], embed = [":repomanager"], deps = [ + "//config", "//core/git", "//core/git/gitmock", "//core/workspace", diff --git a/core/repomanager/repo_manager.go b/core/repomanager/repo_manager.go index a2e26c7e..c16dca83 100644 --- a/core/repomanager/repo_manager.go +++ b/core/repomanager/repo_manager.go @@ -24,10 +24,10 @@ import ( "time" "github.com/uber-go/tally" + "github.com/uber/tango/config" "github.com/uber/tango/core/git" "github.com/uber/tango/core/workspace" "github.com/uber/tango/entity" - "github.com/uber/tango/internal/url" "github.com/uber/tango/observability/metrics" "go.uber.org/zap" ) @@ -49,6 +49,7 @@ type repoManager struct { repoManagerClonePath string logger *zap.Logger emitter *metrics.Emitter + repoConfig config.RepositoryConfigProvider poolSize int restoreWorker func(context.Context, string) error @@ -90,6 +91,9 @@ type Params struct { Logger *zap.Logger RepoManagerClonePath string PoolSize int + // RepoConfig is the authoritative repository allowlist and supplies the + // configured safe repository ID. + RepoConfig config.RepositoryConfigProvider // Scope is the tally scope for metrics. A nil scope disables metrics // (defaults to a no-op). Metrics nest under .repo_manager.*. Scope tally.Scope @@ -104,11 +108,16 @@ 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"), + repoConfig: p.RepoConfig, poolSize: p.PoolSize, restoreWorker: git.RestoreWorktree, pools: make(map[string]*workerPool), @@ -148,8 +157,12 @@ func (r *repoManager) poolFor(repo string) *workerPool { // 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}) + repoCfg, ok := r.repoConfig.GetRepositoryConfig(desc.Remote) + if !ok { + return nil, fmt.Errorf("repository remote %q is not configured", desc.Remote) + } + repo := repoCfg.RepositoryID + e := r.emitter.Tagged(map[string]string{metrics.TagRepo: repoCfg.RepositoryID}) op := metrics.Begin(e, _opLease, metrics.SlowDurationBuckets) defer func() { op.Complete(retErr) }() diff --git a/core/repomanager/repo_manager_test.go b/core/repomanager/repo_manager_test.go index f0487b70..887f8879 100644 --- a/core/repomanager/repo_manager_test.go +++ b/core/repomanager/repo_manager_test.go @@ -16,6 +16,7 @@ package repomanager import ( "context" + "crypto/sha256" "errors" "fmt" "os" @@ -26,6 +27,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/uber/tango/config" tangogit "github.com/uber/tango/core/git" gitmock "github.com/uber/tango/core/git/gitmock" "github.com/uber/tango/core/workspace" @@ -36,12 +38,24 @@ import ( type requestFunc func(context.Context) error +type testRepositoryConfigProvider struct{} + +func (testRepositoryConfigProvider) GetRepositoryConfig(remote string) (config.RepositoryConfig, bool) { + return config.RepositoryConfig{ + Remote: remote, + RepositoryID: testRepositoryID(remote), + }, true +} + func (f requestFunc) Apply(ctx context.Context) error { return f(ctx) } func newTestRepoManager(t *testing.T, appCtx context.Context, p Params) RepoManager { t.Helper() + if p.RepoConfig == nil { + p.RepoConfig = testRepositoryConfigProvider{} + } rm, err := NewRepoManager(appCtx, p) require.NoError(t, err) rm.(*repoManager).restoreWorker = func(context.Context, string) error { return nil } @@ -56,6 +70,18 @@ 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, testRepositoryID(remote)) +} + +func testWorkerDir(root, remote string, worker int) string { + return filepath.Join(root, ".workers", testRepositoryID(remote), fmt.Sprintf("worker-%d", worker)) +} + +func testRepositoryID(remote string) string { + return fmt.Sprintf("repository-%x", sha256.Sum256([]byte(remote))) +} + func TestNewRepoManager_InvalidPoolSize(t *testing.T) { t.Parallel() _, err := NewRepoManager(context.Background(), Params{ @@ -72,8 +98,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 +118,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 +140,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) @@ -153,6 +179,7 @@ func TestLease_RestoresWorkerAfterFailedMaterialization(t *testing.T) { Logger: zap.NewNop(), RepoManagerClonePath: root, PoolSize: 1, + RepoConfig: testRepositoryConfigProvider{}, }) require.NoError(t, err) @@ -200,8 +227,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 +254,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 +282,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 +328,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 +356,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 +384,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 +399,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 +418,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 +439,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 +461,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 +476,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 diff --git a/docs/observability/metrics.md b/docs/observability/metrics.md index 9d039693..f071ec2d 100644 --- a/docs/observability/metrics.md +++ b/docs/observability/metrics.md @@ -162,7 +162,11 @@ 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)}) + repoCfg, ok := b.config.GetRepositoryConfig(req.Build.Remote) + if !ok { + return nil, fmt.Errorf("repository remote %q is not configured", req.Build.Remote) + } + e := b.emitter.Tagged(map[string]string{metrics.TagRepo: repoCfg.RepositoryID}) op := metrics.Begin(e, _opGetTargetGraph, metrics.SlowDurationBuckets) defer func() { op.Complete(retErr) }() @@ -175,7 +179,11 @@ 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())}) + repoCfg, err := c.resolveRepository(req.GetFirstRevision().GetRemote()) + if err != nil { + return err + } + e := c.emitter.Tagged(map[string]string{metrics.TagRepo: repoCfg.RepositoryID}) op := metrics.Begin(e, opGetChangedTargets, metrics.SlowDurationBuckets) defer func() { op.Complete(retErr) }() @@ -217,4 +225,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 come from a bounded set — never request IDs, commit hashes, paths, or raw repo URLs. Every repository configuration supplies a mandatory `repository_id` that operators guarantee is safe for metric labels, filesystem paths, and cache keys. Request handlers require an exact configured-remote match before cache or workspace access, then reuse that configured ID; unconfigured requests use the shared `unknown` label. diff --git a/example/README.md b/example/README.md index 893ac1b3..af39ac7c 100644 --- a/example/README.md +++ b/example/README.md @@ -20,6 +20,7 @@ The server reads [`tango-config.yaml`](tango-config.yaml). Unknown fields are re | Field | Required/default | Description | |---|---|---| | `repository[].remote` | Required | URL Tango clones and uses to look up this entry. | +| `repository[].repository_id` | Required | Operator-provided safe name used for metrics, workspace directories, and cache-key namespaces. It must uniquely identify the repository and be unique across Tango installations. | | `repository[].full_hash_repos` | Optional; defaults to `[]` | External repositories whose individual files should be hashed instead of sharing the repository-rule hash. The main repository is always fully hashed. | | `repository[].excluded_files` | Optional; defaults to `[]` | Regular expressions for target labels to exclude from the hashed graph. | | `repository[].bzlmod_enabled` | Optional; defaults to `true` | Whether the repository uses Bzlmod. Set to `false` for legacy WORKSPACE dependency resolution. | diff --git a/example/main.go b/example/main.go index 98bcc22f..9c2cef58 100644 --- a/example/main.go +++ b/example/main.go @@ -81,6 +81,7 @@ func run() error { Logger: logger, RepoManagerClonePath: repoManagerClonePath, PoolSize: cfg.Service.MaxWorkerPoolSize, + RepoConfig: cfg, }) if err != nil { return fmt.Errorf("failed to create repo manager: %w", err) diff --git a/example/tango-config.yaml b/example/tango-config.yaml index 702520c1..3fb8f82b 100644 --- a/example/tango-config.yaml +++ b/example/tango-config.yaml @@ -9,6 +9,8 @@ storage: # Repository configuration repository: - remote: "https://github.com/uber/tango.git" + # Required safe name that uniquely identifies this repository across Tango installations. + repository_id: "tango" # External repositories to hash file-by-file. The main repository is always fully hashed. full_hash_repos: [] # Regular expressions matched against target labels. diff --git a/integration/integration_test.go b/integration/integration_test.go index 2d6891ad..f43b6435 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -126,17 +126,18 @@ func startServerWithLogger(t testing.TB, remote string, zl *zap.Logger) string { store := storage.NewMemoryStorage() + cfg, err := config.Parse(configPath) + require.NoError(t, err, "failed to parse config") + rm, err := repomanager.NewRepoManager(appCtx, repomanager.Params{ Git: git.New(clonePath, zl), Logger: zl, RepoManagerClonePath: clonePath, PoolSize: 2, + RepoConfig: cfg, }) require.NoError(t, err, "failed to create repo manager") - cfg, err := config.Parse(configPath) - require.NoError(t, err, "failed to parse config") - orch, err := orchestrator.NewNativeOrchestrator(appCtx, orchestrator.Params{ Storage: store, RepoManager: rm, @@ -150,6 +151,7 @@ func startServerWithLogger(t testing.TB, remote string, zl *zap.Logger) string { Logger: zl, Storage: store, Orchestrator: orch, + RepoConfig: cfg, }) grpcTransport := yarpcgrpc.NewTransport() diff --git a/integration/testdata/tango-config.yaml.tmpl b/integration/testdata/tango-config.yaml.tmpl index 20a38f9f..c38032ee 100644 --- a/integration/testdata/tango-config.yaml.tmpl +++ b/integration/testdata/tango-config.yaml.tmpl @@ -3,6 +3,7 @@ storage: repository: - remote: {{.Remote}} + repository_id: "integration-repository" {{- if .BazelCommand}} bazel_command_path: {{.BazelCommand}} {{- end}} diff --git a/orchestrator/BUILD.bazel b/orchestrator/BUILD.bazel index ea2a0f61..7fce1191 100644 --- a/orchestrator/BUILD.bazel +++ b/orchestrator/BUILD.bazel @@ -21,7 +21,6 @@ go_library( "//core/workspace", "//entity", "//graphrunner", - "//internal/url", "//mapper", "//observability/metrics", "@com_github_uber_go_tally//:tally", diff --git a/orchestrator/native_orchestrator.go b/orchestrator/native_orchestrator.go index a4cb534e..7a486188 100644 --- a/orchestrator/native_orchestrator.go +++ b/orchestrator/native_orchestrator.go @@ -34,7 +34,6 @@ import ( "github.com/uber/tango/core/workspace" "github.com/uber/tango/entity" "github.com/uber/tango/graphrunner" - "github.com/uber/tango/internal/url" "github.com/uber/tango/mapper" "github.com/uber/tango/observability/metrics" "go.uber.org/zap" @@ -107,18 +106,21 @@ 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)}) + build := req.Build + repoCfg, ok := b.config.GetRepositoryConfig(build.Remote) + if !ok { + return nil, fmt.Errorf("repository remote %q is not configured", build.Remote) + } + e := b.emitter.Tagged(map[string]string{metrics.TagRepo: repoCfg.RepositoryID}) op := metrics.Begin(e, _opGetTargetGraph, metrics.SlowDurationBuckets) defer func() { op.Complete(retErr) }() - build := req.Build - logger := b.logger.With(zap.Any("build_description", build)) + logger := b.logger.With( + zap.String("repository", repoCfg.RepositoryID), + zap.String("base_sha", build.BaseSha), + zap.Stringer("strategy", build.Strategy), + ) logger.Info("GetTargetGraph: Processing request") - remote := build.Remote - repoCfg, ok := b.config.GetRepositoryConfig(remote) - if !ok { - return nil, fmt.Errorf("no repository configuration found for remote %q", remote) - } leaseStart := time.Now() ws, err := b.repoManager.Lease(ctx, build) recordStep(e, "lease_duration", leaseStart, metrics.FastDurationBuckets) @@ -138,7 +140,7 @@ func (b *nativeOrchestrator) GetTargetGraph(ctx context.Context, req entity.GetT err = ws.Checkout(ctx, build.Remote, build.BaseSha) recordStep(e, "checkout_duration", checkoutStart, metrics.FastDurationBuckets) if err != nil { - return nil, classifyGitError(fmt.Errorf("checkout %s@%s: %w", build.Remote, build.BaseSha, err)) + return nil, classifyGitError(fmt.Errorf("checkout repository %s at %s: %w", repoCfg.RepositoryID, build.BaseSha, err)) } logger.Info("GetTargetGraph: Checked out base revision") @@ -160,18 +162,18 @@ func (b *nativeOrchestrator) GetTargetGraph(ctx context.Context, req entity.GetT err = ws.ApplyRequests(ctx, requests) recordStep(e, "apply_requests_duration", applyStart, metrics.FastDurationBuckets) if err != nil { - return nil, classifyGitError(fmt.Errorf("apply requests for %s@%s: %w", build.Remote, build.BaseSha, err)) + return nil, classifyGitError(fmt.Errorf("apply requests for repository %s at %s: %w", repoCfg.RepositoryID, build.BaseSha, err)) } logger.Info("GetTargetGraph: Applied requests", zap.Int("request_count", len(requests))) // Compute the treehash and download the target graph from storage if exists. treehash, err := gitModule.RevParse(ctx, "HEAD^{tree}") if err != nil { - return nil, classifyGitError(fmt.Errorf("compute treehash for %s@%s: %w", build.Remote, build.BaseSha, err)) + return nil, classifyGitError(fmt.Errorf("compute treehash for repository %s at %s: %w", repoCfg.RepositoryID, build.BaseSha, err)) } - treehashPath := cachekey.GetGraphByTreeHash(build.Remote, treehash, build.Strategy, req.ExcludeFilesRegex) + treehashPath := cachekey.GetGraphByTreeHash(repoCfg.RepositoryID, treehash, build.Strategy, req.ExcludeFilesRegex) useTGB := b.config.Service.GraphFormat == config.GraphFormatTGB - tgbPath := cachekey.GetTGBGraphByTreeHash(build.Remote, treehash, build.Strategy, req.ExcludeFilesRegex) + tgbPath := cachekey.GetTGBGraphByTreeHash(repoCfg.RepositoryID, treehash, build.Strategy, req.ExcludeFilesRegex) if !req.BypassCache { cacheReadStart := time.Now() graphReader, err := b.readCachedGraph(ctx, logger, useTGB, tgbPath, treehashPath) @@ -193,7 +195,7 @@ func (b *nativeOrchestrator) GetTargetGraph(ctx context.Context, req entity.GetT // resolve it without waiting for the graph to finish. go func() { bgOp := metrics.Begin(e, _opTreehashCacheWrite, metrics.FastDurationBuckets) - thCachePath := cachekey.GetTreehashCachePath(build) + thCachePath := cachekey.GetTreehashCachePath(repoCfg.RepositoryID, build) putErr := b.storage.Put(b.appCtx, storage.UploadRequest{ Key: thCachePath, Reader: bytes.NewReader([]byte(treehash)), @@ -230,7 +232,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: repoCfg.RepositoryID}), }) default: return nil, tangoerrors.NewUser(fmt.Errorf("unknown computation strategy: %d", build.Strategy)) diff --git a/orchestrator/native_orchestrator_test.go b/orchestrator/native_orchestrator_test.go index 8f3d9101..1304b3dc 100644 --- a/orchestrator/native_orchestrator_test.go +++ b/orchestrator/native_orchestrator_test.go @@ -421,10 +421,10 @@ func TestNative_GetTargetGraph_TGBFormat(t *testing.T) { assert.True(t, sawTarget, "chunk stream should carry the computed target") // The blob landed under the tgb key only; nothing was written to the gob key. - tgbKey := cachekey.GetTGBGraphByTreeHash(build.Remote, "th", build.Strategy, nil) + tgbKey := cachekey.GetTGBGraphByTreeHash("tango", "th", build.Strategy, nil) _, err = storage.NewTGBGraphReader(context.Background(), st, tgbKey, config.DefaultMaxMessageBytes) require.NoError(t, err) - gobKey := cachekey.GetGraphByTreeHash(build.Remote, "th", build.Strategy, nil) + gobKey := cachekey.GetGraphByTreeHash("tango", "th", build.Strategy, nil) _, err = storage.NewGraphReader(context.Background(), st, gobKey) require.Error(t, err) assert.True(t, storage.IsNotFound(err), "gob key must stay empty under graph_format=tgb") diff --git a/orchestrator/testdata/config.yaml b/orchestrator/testdata/config.yaml index a1d84539..3cfe5267 100644 --- a/orchestrator/testdata/config.yaml +++ b/orchestrator/testdata/config.yaml @@ -1,5 +1,6 @@ repository: - remote: "git@github:uber/tango" + repository_id: "tango" full_hash_repos: - "//" - "//external"