From f20f3fb1b08091d985d54439251d85dbea3dc79f Mon Sep 17 00:00:00 2001 From: sergeyb Date: Thu, 30 Jul 2026 01:15:07 +0000 Subject: [PATCH 1/2] fix(config): validate storage and unknown fields in Parse, wire max_message_bytes (audit #12) Move storage type/root_path validation from example/main.go into config.Parse so all embedders get it. Switch to strict YAML decoding (DisallowUnknownField) to reject typos. Set query_timeout default (900s) in Parse instead of letting zero leak to core/bazel. Wire MaxMessageBytes into controller construction in example/main.go. Document field defaults and constraints. Add config_test.go covering all defaults and rejection branches. Update example/README.md. Co-Authored-By: Claude Fable 5 --- config/BUILD.bazel | 12 +- config/config.go | 32 ++++- config/config_test.go | 243 ++++++++++++++++++++++++++++++++++++ config/repository_config.go | 9 +- config/service_config.go | 13 +- example/README.md | 24 +--- example/main.go | 18 ++- 7 files changed, 313 insertions(+), 38 deletions(-) create mode 100644 config/config_test.go diff --git a/config/BUILD.bazel b/config/BUILD.bazel index 0c95044a..bf0b6118 100644 --- a/config/BUILD.bazel +++ b/config/BUILD.bazel @@ -1,4 +1,4 @@ -load("@rules_go//go:def.bzl", "go_library") +load("@rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "config", @@ -12,3 +12,13 @@ go_library( visibility = ["//visibility:public"], deps = ["@com_github_goccy_go_yaml//:go-yaml"], ) + +go_test( + name = "config_test", + srcs = ["config_test.go"], + embed = [":config"], + deps = [ + "@com_github_stretchr_testify//assert", + "@com_github_stretchr_testify//require", + ], +) diff --git a/config/config.go b/config/config.go index 083695dc..d9a15eef 100644 --- a/config/config.go +++ b/config/config.go @@ -15,6 +15,7 @@ package config import ( + "bytes" "fmt" "os" "path/filepath" @@ -50,17 +51,35 @@ func Parse(configFilePath string) (*Config, error) { if err != nil { return nil, err } + return ParseBytes(yamlBytes) +} + +// ParseBytes parses the full configuration from raw YAML bytes. +// Unknown YAML fields are rejected. +func ParseBytes(yamlBytes []byte) (*Config, error) { var config Config - if err := yaml.Unmarshal(yamlBytes, &config); err != nil { + dec := yaml.NewDecoder(bytes.NewReader(yamlBytes), yaml.DisallowUnknownField()) + if err := dec.Decode(&config); err != nil { return nil, err } - // Default to memory storage if not specified - if config.Storage.Type == "" { + + // --- storage validation --- + switch config.Storage.Type { + case StorageTypeMemory, "": config.Storage.Type = StorageTypeMemory + case StorageTypeDisk: + if config.Storage.Disk == nil || config.Storage.Disk.RootPath == "" { + return nil, fmt.Errorf("storage.disk.root_path must be set when storage type is %q", StorageTypeDisk) + } + default: + return nil, fmt.Errorf("unsupported storage type: %q (supported: %q, %q)", config.Storage.Type, StorageTypeMemory, StorageTypeDisk) } + + // --- service validation and defaults --- if config.Service.WorkerRootPath != "" && config.Service.RepoManagerClonePath == "" { return nil, fmt.Errorf("service.repo_manager_clone_path must be set when worker_root_path is specified") } + // Default: os.TempDir()/tango-repo-manager. if config.Service.RepoManagerClonePath == "" { config.Service.RepoManagerClonePath = filepath.Join(os.TempDir(), "tango-repo-manager") } @@ -73,6 +92,8 @@ func Parse(configFilePath string) (*Config, error) { if config.Service.MaxMessageBytes <= 0 { config.Service.MaxMessageBytes = DefaultMaxMessageBytes } + + // --- repository validation and defaults --- config.repositoryByRemote = make(map[string]*RepositoryConfig, len(config.Repository)) for i := range config.Repository { remote := config.Repository[i].Remote @@ -82,6 +103,11 @@ func Parse(configFilePath string) (*Config, error) { if _, exists := config.repositoryByRemote[remote]; exists { return nil, fmt.Errorf("duplicate repository remote %q", remote) } + // Default query_timeout: 900 seconds (15 minutes), matching the + // core/bazel package's _queryTimeout constant. + if config.Repository[i].QueryTimeout <= 0 { + config.Repository[i].QueryTimeout = DefaultQueryTimeoutSeconds + } config.repositoryByRemote[remote] = &config.Repository[i] } return &config, nil diff --git a/config/config_test.go b/config/config_test.go new file mode 100644 index 00000000..c332b4d0 --- /dev/null +++ b/config/config_test.go @@ -0,0 +1,243 @@ +// Copyright (c) 2025 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package config + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// minimal returns a valid minimal YAML config string. +func minimal() string { + return ` +repository: + - remote: "https://example.com/repo.git" +service: + worker_pool_size: 2 +` +} + +func TestParseBytes_Defaults(t *testing.T) { + cfg, err := ParseBytes([]byte(minimal())) + require.NoError(t, err) + + assert.Equal(t, StorageTypeMemory, cfg.Storage.Type, "storage type should default to memory") + assert.Equal(t, DefaultMaxMessageBytes, cfg.Service.MaxMessageBytes, "max_message_bytes should default") + assert.Contains(t, cfg.Service.RepoManagerClonePath, "tango-repo-manager", "clone path should default to temp dir") + assert.Contains(t, cfg.Service.WorkerRootPath, ".workers", "worker root should default under clone path") + assert.Equal(t, DefaultQueryTimeoutSeconds, cfg.Repository[0].QueryTimeout, "query_timeout should default to 900") +} + +func TestParseBytes_ExplicitValues(t *testing.T) { + yamlStr := ` +repository: + - remote: "https://example.com/repo.git" + query_timeout: 60 +storage: + type: "memory" +service: + worker_pool_size: 4 + repo_manager_clone_path: "/custom/clone" + worker_root_path: "/custom/workers" + max_message_bytes: 1000000 +` + cfg, err := ParseBytes([]byte(yamlStr)) + require.NoError(t, err) + + assert.Equal(t, StorageTypeMemory, cfg.Storage.Type) + assert.Equal(t, int64(60), cfg.Repository[0].QueryTimeout) + assert.Equal(t, "/custom/clone", cfg.Service.RepoManagerClonePath) + assert.Equal(t, "/custom/workers", cfg.Service.WorkerRootPath) + assert.Equal(t, 1000000, cfg.Service.MaxMessageBytes) +} + +func TestParseBytes_StorageValidation(t *testing.T) { + tests := []struct { + name string + yaml string + wantErr bool + }{ + { + name: "memory explicit", + yaml: ` +storage: + type: "memory" +repository: + - remote: "https://example.com/r.git" +service: + worker_pool_size: 1 +`, + }, + { + name: "empty defaults to memory", + yaml: ` +repository: + - remote: "https://example.com/r.git" +service: + worker_pool_size: 1 +`, + }, + { + name: "disk with root_path", + yaml: ` +storage: + type: "disk" + disk: + root_path: "/tmp/store" +repository: + - remote: "https://example.com/r.git" +service: + worker_pool_size: 1 +`, + }, + { + name: "disk without root_path", + wantErr: true, + yaml: ` +storage: + type: "disk" +repository: + - remote: "https://example.com/r.git" +service: + worker_pool_size: 1 +`, + }, + { + name: "disk with empty root_path", + wantErr: true, + yaml: ` +storage: + type: "disk" + disk: + root_path: "" +repository: + - remote: "https://example.com/r.git" +service: + worker_pool_size: 1 +`, + }, + { + name: "unknown storage type", + wantErr: true, + yaml: ` +storage: + type: "s3" +repository: + - remote: "https://example.com/r.git" +service: + worker_pool_size: 1 +`, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := ParseBytes([]byte(tt.yaml)) + if tt.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + }) + } +} + +func TestParseBytes_UnknownFieldsRejected(t *testing.T) { + yamlStr := ` +repository: + - remote: "https://example.com/repo.git" +service: + worker_pool_size: 1 + totally_bogus_field: true +` + _, err := ParseBytes([]byte(yamlStr)) + require.Error(t, err, "unknown YAML fields should be rejected") +} + +func TestParseBytes_WorkerPoolSizeRequired(t *testing.T) { + yamlStr := ` +repository: + - remote: "https://example.com/repo.git" +service: + worker_pool_size: 0 +` + _, err := ParseBytes([]byte(yamlStr)) + require.Error(t, err) +} + +func TestParseBytes_EmptyRemoteRejected(t *testing.T) { + yamlStr := ` +repository: + - remote: "" +service: + worker_pool_size: 1 +` + _, err := ParseBytes([]byte(yamlStr)) + require.Error(t, err) +} + +func TestParseBytes_DuplicateRemoteRejected(t *testing.T) { + yamlStr := ` +repository: + - remote: "https://example.com/repo.git" + - remote: "https://example.com/repo.git" +service: + worker_pool_size: 1 +` + _, err := ParseBytes([]byte(yamlStr)) + require.Error(t, err) +} + +func TestParseBytes_WorkerRootPathRequiresClonePath(t *testing.T) { + yamlStr := ` +repository: + - remote: "https://example.com/repo.git" +service: + worker_pool_size: 1 + worker_root_path: "/some/path" +` + _, err := ParseBytes([]byte(yamlStr)) + require.Error(t, err) +} + +func TestParse_FileNotFound(t *testing.T) { + _, err := Parse("/nonexistent/path/config.yaml") + require.Error(t, err) +} + +func TestParse_FromFile(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "config.yaml") + require.NoError(t, os.WriteFile(path, []byte(minimal()), 0o644)) + + cfg, err := Parse(path) + require.NoError(t, err) + assert.Equal(t, StorageTypeMemory, cfg.Storage.Type) +} + +func TestGetRepositoryConfig(t *testing.T) { + cfg, err := ParseBytes([]byte(minimal())) + require.NoError(t, err) + + repo, ok := cfg.GetRepositoryConfig("https://example.com/repo.git") + assert.True(t, ok) + assert.Equal(t, "https://example.com/repo.git", repo.Remote) + + _, 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 46f52f7b..57f55926 100644 --- a/config/repository_config.go +++ b/config/repository_config.go @@ -14,6 +14,10 @@ package config +// DefaultQueryTimeoutSeconds is the default query_timeout applied when the +// field is unset or zero (15 minutes), matching core/bazel's _queryTimeout. +const DefaultQueryTimeoutSeconds int64 = 900 + // RepositoryConfig holds configuration for a single repository. type RepositoryConfig struct { Remote string `yaml:"remote"` @@ -22,8 +26,9 @@ type RepositoryConfig struct { ExcludeExternalTargets bool `yaml:"exclude_external_targets"` BzlmodEnabled bool `yaml:"bzlmod_enabled"` BazelCommand string `yaml:"bazel_command"` - QueryTimeout int64 `yaml:"query_timeout"` // in seconds - BazelExtraArgs []string `yaml:"bazel_extra_args"` + // QueryTimeout is the Bazel query timeout in seconds. Defaults to DefaultQueryTimeoutSeconds (900, i.e. 15 minutes). + QueryTimeout int64 `yaml:"query_timeout"` + BazelExtraArgs []string `yaml:"bazel_extra_args"` // BazelStartupOptions are Bazel startup flags placed before the `query` subcommand (e.g. "--batch"); empty by default. BazelStartupOptions []string `yaml:"bazel_startup_options"` StreamBazelLogs bool `yaml:"stream_bazel_logs"` diff --git a/config/service_config.go b/config/service_config.go index 9b06e84f..48293784 100644 --- a/config/service_config.go +++ b/config/service_config.go @@ -16,10 +16,15 @@ package config // ServiceConfig holds operational configuration for the Tango service. type ServiceConfig struct { - WorkerPoolSize int `yaml:"worker_pool_size"` // number of worker workspaces per repo - RepoManagerClonePath string `yaml:"repo_manager_clone_path"` // root directory for origin repo clones - WorkerRootPath string `yaml:"worker_root_path"` // root directory for worker workspace checkouts; defaults to repo_manager_clone_path/.workers - MaxMessageBytes int `yaml:"max_message_bytes"` // max serialized bytes per streamed gRPC message; 0 → DefaultMaxMessageBytes + WorkerPoolSize int `yaml:"worker_pool_size"` // number of worker workspaces per repo + // RepoManagerClonePath is the root directory for origin repo clones. + // Defaults to os.TempDir()/tango-repo-manager. + // Must be set explicitly when worker_root_path is specified. + RepoManagerClonePath string `yaml:"repo_manager_clone_path"` + // WorkerRootPath is the root directory for worker workspace checkouts. + // Defaults to repo_manager_clone_path/.workers. + WorkerRootPath string `yaml:"worker_root_path"` + MaxMessageBytes int `yaml:"max_message_bytes"` // max serialized bytes per streamed gRPC message; 0 → DefaultMaxMessageBytes } // DefaultMaxMessageBytes is the fallback max serialized size per streamed diff --git a/example/README.md b/example/README.md index 369313d2..a33c04a7 100644 --- a/example/README.md +++ b/example/README.md @@ -1,21 +1,14 @@ # Example -A demonstration server that shows how to run Tango end-to-end. It boots a -YARPC/gRPC server on `127.0.0.1:8081`, wiring together config parsing, -storage, the repo manager, the orchestrator, and the controller. A companion -CLI client calls the server's streaming RPCs, and a query-bench tool exercises -the underlying Bazel query and target-hashing path without bringing up the -server. +A demonstration server that shows how to run Tango end-to-end. It boots a YARPC/gRPC server on `127.0.0.1:8081`, wiring together config parsing, storage, the repo manager, the orchestrator, and the controller. A companion CLI client calls the server's streaming RPCs, and a query-bench tool exercises the underlying Bazel query and target-hashing path without bringing up the server. ## Configuration The server reads `tango-config.yaml`. Top-level sections: -- `storage` — `type: memory` (default) or `type: disk` with a `root_path`. -- `repository` — remotes Tango is allowed to operate on, with default branch, - excluded files, external-target handling, bzlmod, and per-query timeout. -- `service` — worker pool size, origin clone path, and per-worker checkout - path. Both directories are created on start and removed on clean shutdown. +- `storage` — `type: memory` (default) or `type: disk` with a `disk.root_path`. Unknown storage types are rejected at parse time. +- `repository` — a list of remotes Tango is allowed to operate on. Each entry supports `remote` (required), `full_hash_repos` (list of query-scope prefixes for full hashing), `excluded_files` (regexes of files to skip), `exclude_external_targets`, `bzlmod_enabled`, `bazel_command` (path to the Bazel binary), `bazel_extra_args`, `bazel_startup_options`, `stream_bazel_logs`, and `query_timeout` (seconds; defaults to 900, i.e. 15 minutes). +- `service` — `worker_pool_size` (required, > 0), `repo_manager_clone_path` (root for origin clones; defaults to `$TMPDIR/tango-repo-manager`), `worker_root_path` (root for per-worker checkouts; defaults to `repo_manager_clone_path/.workers`), and `max_message_bytes` (max serialized bytes per streamed gRPC message; defaults to ~4.25 MB). Both directories are created on start and removed on clean shutdown. ## Running @@ -42,18 +35,13 @@ make run-client-changed-targets \ NEW_BASE_SHA=872881fd ``` -The client supports two methods (`get-target-graph`, `get-changed-targets`) -and flags for limiting changed-target distance (`-max-distance`), cache bypass -(`-bypass-cache`), output detail (`-include-hashes`, `-include-tags`, -`-include-attributes`), and request URLs (`-request-urls`, -`-new-request-urls`). Run with `-h` for the full list. +The client supports two methods (`get-target-graph`, `get-changed-targets`) and flags for limiting changed-target distance (`-max-distance`), cache bypass (`-bypass-cache`), output detail (`-include-hashes`, `-include-tags`, `-include-attributes`), and request URLs (`-request-urls`, `-new-request-urls`). Run with `-h` for the full list. Change requests are identified by canonical change URIs of the form `github://{host[:port]}/{org}/{repo}/pull/{pr}/{head_sha}`, per the [change-URI RFC](https://github.com/uber/submitqueue/blob/main/doc/rfc/change-uri.md) — for example `github://github.com/uber/tango/pull/123/c3a4b5d6e7f80912a3b4c5d6e7f80912a3b4c5d6`. The head SHA is the PR's head commit at submission time; it pins the exact code state applied on top of the base revision and forms the cache identity. The bundled native orchestrator rejects non-canonical spellings (uppercase host, abbreviated SHA, missing host); custom Orchestrator implementations may accept formats of their own. ## Benchmarking -The query-bench tool times the standard Tango query against a real Bazel -workspace and reports per-stage timings (query, hashing, response conversion): +The query-bench tool times the standard Tango query against a real Bazel workspace and reports per-stage timings (query, hashing, response conversion): ```bash bazel run //example/cmd/query-bench -- --workspace /path/to/repo --runs 3 diff --git a/example/main.go b/example/main.go index 54ee079d..aaa10c75 100644 --- a/example/main.go +++ b/example/main.go @@ -106,9 +106,10 @@ func run() error { // Controller (YARPC server implementation). appCtx is forwarded so the // controller's background goroutines are tied to process lifetime. ctrl := controller.NewController(appCtx, controller.Params{ - Logger: zl, - Storage: store, - Orchestrator: orch, + Logger: zl, + Storage: store, + Orchestrator: orch, + MaxMessageBytes: cfg.Service.MaxMessageBytes, }) // YARPC transports and dispatcher @@ -142,19 +143,16 @@ func run() error { } // newStorage creates a Storage implementation based on the provided configuration. +// Storage type and disk.root_path are validated by config.Parse, so the switch +// only needs to handle the known types. func newStorage(cfg config.StorageConfig) (storage.Storage, error) { switch cfg.Type { - case config.StorageTypeMemory, "": + case config.StorageTypeMemory: return storage.NewMemoryStorage(), nil case config.StorageTypeDisk: - if cfg.Disk == nil { - return nil, fmt.Errorf("disk storage requires 'disk' configuration") - } - if cfg.Disk.RootPath == "" { - return nil, fmt.Errorf("disk storage requires 'root_path' to be set") - } return disk.New(cfg.Disk.RootPath) default: + // Unreachable after config.Parse validation, but kept for safety. return nil, fmt.Errorf("unsupported storage type: %q", cfg.Type) } } From 345579b1ae6f14d81b4e572e79fa2d4cb8264af0 Mon Sep 17 00:00:00 2001 From: sergeyb Date: Wed, 5 Aug 2026 00:15:55 +0000 Subject: [PATCH 2/2] fix(integration): remove stale default_branch config --- integration/testdata/tango-config.yaml.tmpl | 1 - 1 file changed, 1 deletion(-) diff --git a/integration/testdata/tango-config.yaml.tmpl b/integration/testdata/tango-config.yaml.tmpl index 417da807..16e6a653 100644 --- a/integration/testdata/tango-config.yaml.tmpl +++ b/integration/testdata/tango-config.yaml.tmpl @@ -3,7 +3,6 @@ storage: repository: - remote: {{.Remote}} - default_branch: "main" {{- if .BazelCommand}} bazel_command: {{.BazelCommand}} {{- end}}