diff --git a/controller/controller.go b/controller/controller.go index ee7f28e2..4fdd20fb 100644 --- a/controller/controller.go +++ b/controller/controller.go @@ -17,11 +17,9 @@ 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/entity" "github.com/uber/tango/observability/metrics" @@ -31,49 +29,23 @@ import ( "go.uber.org/zap" ) -// Duplicated in the handler. Temporary: the controller drops this copy when it -// takes the resolved repository from the handler. -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 + Scope tally.Scope `optional:"true"` + MaxMessageBytes int `optional:"true"` GraphConfig config.GraphConfigProvider `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. -// -// Duplicated in the handler. Temporary: the controller drops this copy when it -// takes the resolved repository from the handler. -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 orchestrator orchestrator.Orchestrator emitter *metrics.Emitter maxMessageBytes int - repoConfig config.RepositoryConfigProvider graphConfig config.GraphConfigProvider // appCtx is the application lifetime; cancel it on process shutdown. @@ -103,7 +75,6 @@ func NewController(appCtx context.Context, p Params) Controller { orchestrator: p.Orchestrator, emitter: emitter, maxMessageBytes: maxMessageBytes, - repoConfig: p.RepoConfig, graphConfig: p.GraphConfig, appCtx: appCtx, } diff --git a/controller/controller_test.go b/controller/controller_test.go index 07fad1fc..2914172b 100644 --- a/controller/controller_test.go +++ b/controller/controller_test.go @@ -19,38 +19,12 @@ 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) { @@ -59,7 +33,6 @@ 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_test.go b/controller/getchangedtargets_test.go index 9670a108..fd2ce7a3 100644 --- a/controller/getchangedtargets_test.go +++ b/controller/getchangedtargets_test.go @@ -269,7 +269,6 @@ 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), @@ -298,7 +297,6 @@ 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), @@ -382,7 +380,6 @@ func TestGetChangedTargets_StreamSendError(t *testing.T) { }) c := NewController(context.Background(), Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), Storage: storagemock, Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), @@ -475,7 +472,6 @@ func TestGetChangedTargets_streamChunks(t *testing.T) { }) c := NewController(context.Background(), Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), Storage: storagemock, Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), @@ -567,7 +563,6 @@ 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), @@ -1060,7 +1055,6 @@ 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), diff --git a/controller/getchangedtargets_tgb_test.go b/controller/getchangedtargets_tgb_test.go index 919c11b8..8a201eba 100644 --- a/controller/getchangedtargets_tgb_test.go +++ b/controller/getchangedtargets_tgb_test.go @@ -124,7 +124,6 @@ func TestGetChangedTargets_TGBNativePath(t *testing.T) { 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 @@ -234,7 +233,6 @@ func TestGetChangedTargets_TGBAllTargetsTrigger(t *testing.T) { scope := tally.NewTestScope("", nil) c := NewController(context.Background(), Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), Storage: st, Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), @@ -279,7 +277,6 @@ func TestGetChangedTargets_TGBAllTargetsTriggerPreservesMembershipChanges(t *tes scope := tally.NewTestScope("", nil) c := NewController(context.Background(), Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), Storage: st, Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), @@ -357,7 +354,6 @@ func TestGetChangedTargets_TGBAllTargetsNoTrigger(t *testing.T) { scope := tally.NewTestScope("", nil) c := NewController(context.Background(), Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), Storage: st, Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), @@ -403,7 +399,6 @@ func TestGetChangedTargets_TGBMixedFormatFallsBack(t *testing.T) { 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_test.go b/controller/gettargetgraph_test.go index 40152a89..cd4b3e73 100644 --- a/controller/gettargetgraph_test.go +++ b/controller/gettargetgraph_test.go @@ -47,9 +47,8 @@ func TestGetTargetGraph_CacheMiss_NoSend(t *testing.T) { Return(storage.DownloadResponse{ReadCloser: newMockReadCloser([]byte{})}, nil), ) c := NewController(context.Background(), Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, - Logger: zaptest.NewLogger(t), - Storage: store, + Logger: zaptest.NewLogger(t), + Storage: store, }) req := entity.GetTargetGraphRequest{ Build: entity.BuildDescription{Remote: "repo:go-code", BaseSha: "sha", @@ -71,9 +70,8 @@ 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{ - RepoConfig: allowAnyRepositoryConfigProvider{}, - Logger: zaptest.NewLogger(t), - Storage: storagemock, + Logger: zaptest.NewLogger(t), + Storage: storagemock, }) err := c.GetTargetGraph(entity.GetTargetGraphRequest{ Build: entity.BuildDescription{Remote: "repo:go-code", BaseSha: "sha", @@ -96,9 +94,8 @@ 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{ - RepoConfig: allowAnyRepositoryConfigProvider{}, - Logger: zaptest.NewLogger(t), - Storage: storagemock, + Logger: zaptest.NewLogger(t), + Storage: storagemock, }) err := c.GetTargetGraph(entity.GetTargetGraphRequest{ Build: entity.BuildDescription{Remote: "repo:go-code", BaseSha: "sha", @@ -125,9 +122,8 @@ 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{ - RepoConfig: allowAnyRepositoryConfigProvider{}, - Logger: zaptest.NewLogger(t), - Storage: store, + Logger: zaptest.NewLogger(t), + Storage: store, }) err := c.GetTargetGraph(entity.GetTargetGraphRequest{ Build: entity.BuildDescription{Remote: "repo:go-code", BaseSha: "sha", @@ -153,7 +149,6 @@ 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, @@ -172,9 +167,8 @@ 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{ - RepoConfig: allowAnyRepositoryConfigProvider{}, - Logger: zaptest.NewLogger(t), - Storage: store, + Logger: zaptest.NewLogger(t), + Storage: store, }) err := c.GetTargetGraph(entity.GetTargetGraphRequest{ Build: entity.BuildDescription{Remote: "repo:go-code", BaseSha: "sha"}, @@ -193,9 +187,8 @@ 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{ - RepoConfig: allowAnyRepositoryConfigProvider{}, - Logger: zaptest.NewLogger(t), - Storage: store, + Logger: zaptest.NewLogger(t), + Storage: store, }) err := c.GetTargetGraph(entity.GetTargetGraphRequest{ Build: entity.BuildDescription{Remote: "repo:go-code", BaseSha: "sha"}, @@ -214,9 +207,8 @@ 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{ - RepoConfig: allowAnyRepositoryConfigProvider{}, - Logger: zaptest.NewLogger(t), - Storage: store, + Logger: zaptest.NewLogger(t), + Storage: store, }) err := c.GetTargetGraph(entity.GetTargetGraphRequest{ Build: entity.BuildDescription{Remote: "repo:go-code", BaseSha: "sha"}, @@ -240,9 +232,8 @@ 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{ - RepoConfig: allowAnyRepositoryConfigProvider{}, - Logger: zaptest.NewLogger(t), - Storage: storagemock, + Logger: zaptest.NewLogger(t), + Storage: storagemock, }) err := c.GetTargetGraph(entity.GetTargetGraphRequest{ Build: entity.BuildDescription{Remote: "repo:go-code", BaseSha: "sha"}, @@ -265,7 +256,6 @@ 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, @@ -288,9 +278,8 @@ 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{ - RepoConfig: allowAnyRepositoryConfigProvider{}, - Logger: zaptest.NewLogger(t), - Storage: store, + Logger: zaptest.NewLogger(t), + Storage: store, }) err := c.GetTargetGraph(entity.GetTargetGraphRequest{ Build: entity.BuildDescription{Remote: "repo:go-code", BaseSha: "sha"}, @@ -309,7 +298,6 @@ 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 34d16b0a..384c8691 100644 --- a/controller/testhelper_test.go +++ b/controller/testhelper_test.go @@ -26,15 +26,6 @@ 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" } @@ -44,7 +35,6 @@ func newTestController(logger *zap.Logger) *controller { logger: logger, emitter: metrics.Nop(), maxMessageBytes: config.DefaultMaxMessageBytes, - repoConfig: allowAnyRepositoryConfigProvider{}, appCtx: context.Background(), } } diff --git a/example/main.go b/example/main.go index e3977c0e..b57f1db8 100644 --- a/example/main.go +++ b/example/main.go @@ -105,7 +105,6 @@ func run() error { Storage: store, Orchestrator: orch, MaxMessageBytes: cfg.Service.MaxMessageBytes, - RepoConfig: cfg, GraphConfig: cfg, }) diff --git a/handler/handler.go b/handler/handler.go index a186cdc6..64ed3eea 100644 --- a/handler/handler.go +++ b/handler/handler.go @@ -27,8 +27,6 @@ import ( "go.uber.org/zap" ) -// Duplicate of the constant in the controller. Temporary: the controller -// drops its copy when it takes the resolved repository from the handler. const unknownRepositoryMetricLabel = "unknown" type Params struct { @@ -59,9 +57,6 @@ func New(p Params) pb.TangoYARPCServer { // 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. -// -// Duplicate of controller.resolveRequestRepository. Temporary: the controller -// drops its copy when it takes the resolved repository from the handler. func (h *handler) resolveRequestRepository(remote string, requestErr error) (config.RepositoryConfig, string, error) { if requestErr != nil { return config.RepositoryConfig{}, unknownRepositoryMetricLabel, requestErr diff --git a/handler/wireerror_test.go b/handler/wireerror_test.go index eaf661c5..166ea3e1 100644 --- a/handler/wireerror_test.go +++ b/handler/wireerror_test.go @@ -89,8 +89,7 @@ func TestGetTargetGraph_ValidationError_WiresTangoError(t *testing.T) { stream := tangomock.NewMockTangoServiceGetTargetGraphYARPCServer(ctrl) c := New(Params{Logger: zaptest.NewLogger(t), RepoConfig: allowAnyRepositoryConfigProvider{}, Controller: controller.NewController(context.Background(), controller.Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, - Logger: zaptest.NewLogger(t), + Logger: zaptest.NewLogger(t), })}) // Missing BaseSha triggers a validation error classified as ERROR_USER. @@ -118,8 +117,7 @@ func TestGetChangedTargets_ValidationError_WiresTangoError(t *testing.T) { stream.EXPECT().Context().Return(context.Background()).AnyTimes() c := New(Params{Logger: zaptest.NewLogger(t), RepoConfig: allowAnyRepositoryConfigProvider{}, Controller: controller.NewController(context.Background(), controller.Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, - Logger: zaptest.NewLogger(t), + Logger: zaptest.NewLogger(t), })}) // Missing first revision triggers validation error classified as ERROR_USER. @@ -147,9 +145,8 @@ func TestGetTargetGraph_InfraError_WiresTangoError(t *testing.T) { store.EXPECT().Get(gomock.Any(), gomock.Any()).Return(storage.DownloadResponse{}, errors.New("disk on fire")) c := New(Params{Logger: zaptest.NewLogger(t), RepoConfig: allowAnyRepositoryConfigProvider{}, Controller: controller.NewController(context.Background(), controller.Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, - Logger: zaptest.NewLogger(t), - Storage: store, + Logger: zaptest.NewLogger(t), + Storage: store, })}) err := c.GetTargetGraph(&pb.GetTargetGraphRequest{ diff --git a/integration/integration_test.go b/integration/integration_test.go index eaf805f7..2bae81a1 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -152,7 +152,6 @@ func startServerWithLogger(t testing.TB, remote string, zl *zap.Logger) string { Logger: zl, Storage: store, Orchestrator: orch, - RepoConfig: cfg, GraphConfig: cfg, })