Skip to content

Commit d803dd4

Browse files
committed
feat(submitqueue): add a GitHub validator and select each queue's validator in the orchestrator profiles
1 parent e90d383 commit d803dd4

14 files changed

Lines changed: 1405 additions & 24 deletions

File tree

‎service/submitqueue/demo/provider/README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ Each directory here is one **provider** — a code-hosting system SubmitQueue la
44

55
| File | Selects |
66
|---|---|
7-
| `profiles.yaml` | the change provider, build runner, conflict analyzer, scorer and build budget each queue resolves to (read by the orchestrator) |
7+
| `profiles.yaml` | the change provider, validator, build runner, conflict analyzer, scorer and build budget each queue resolves to (read by the orchestrator) |
88
| `merge.yaml` | the merge target each queue lands on (read by Runway) |
99

1010
Neither holds a secret. Each integration names the *environment variable* carrying its credential, so these files stay committable and rotating a token needs no edit.

‎service/submitqueue/orchestrator/server/BUILD.bazel‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ go_library(
6868
"//submitqueue/extension/speculation/speculator/standard:go_default_library",
6969
"//submitqueue/extension/validator:go_default_library",
7070
"//submitqueue/extension/validator/fake:go_default_library",
71+
"//submitqueue/extension/validator/github:go_default_library",
7172
"//submitqueue/orchestrator:go_default_library",
7273
"//submitqueue/orchestrator/extension/storage:go_default_library",
7374
"//submitqueue/orchestrator/extension/storage/mysql:go_default_library",
@@ -144,6 +145,7 @@ go_test(
144145
"//submitqueue/extension/queuepause/fake:go_default_library",
145146
"//submitqueue/extension/speculation/scorer:go_default_library",
146147
"//submitqueue/extension/speculation/speculator:go_default_library",
148+
"//submitqueue/extension/validator:go_default_library",
147149
"//submitqueue/orchestrator/extension/storage:go_default_library",
148150
"@com_github_stretchr_testify//assert:go_default_library",
149151
"@com_github_stretchr_testify//require:go_default_library",

‎service/submitqueue/orchestrator/server/config.go‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,12 @@ const (
4040
buildRunnerTypeBuildkite = "buildkite"
4141
)
4242

43+
// Validator types selectable from configuration.
44+
const (
45+
validatorTypeFake = "fake"
46+
validatorTypeGitHub = "github"
47+
)
48+
4349
// Conflict analyzer types selectable from configuration.
4450
const (
4551
analyzerTypeAll = "all"
@@ -97,6 +103,7 @@ const (
97103
defaultGitTokenUser = "x-access-token"
98104
defaultGitHubTokenEnv = "GITHUB_TOKEN"
99105
defaultGitHubBaseURL = "https://api.github.com"
106+
defaultGitHubHost = "github.com"
100107
defaultPhabTokenEnv = "PHAB_API_TOKEN"
101108
defaultHTTPTimeout = 30 * time.Second
102109
)
@@ -122,6 +129,7 @@ type profilesConfig struct {
122129
type namedQueueProfileConfig struct {
123130
Name string `yaml:"name"`
124131
ChangeProvider *changeProviderConfig `yaml:"changeProvider"`
132+
Validator *validatorConfig `yaml:"validator"`
125133
BuildRunner *buildRunnerConfig `yaml:"buildRunner"`
126134
Analyzer *analyzerConfig `yaml:"analyzer"`
127135
Scorer *scorerConfig `yaml:"scorer"`
@@ -131,6 +139,7 @@ type namedQueueProfileConfig struct {
131139
// queueProfileConfig is the full set of extensions a queue resolves to.
132140
type queueProfileConfig struct {
133141
ChangeProvider changeProviderConfig `yaml:"changeProvider"`
142+
Validator validatorConfig `yaml:"validator"`
134143
BuildRunner buildRunnerConfig `yaml:"buildRunner"`
135144
Analyzer analyzerConfig `yaml:"analyzer"`
136145
Scorer scorerConfig `yaml:"scorer"`
@@ -191,6 +200,19 @@ type phabProviderConfig struct {
191200
Timeout string `yaml:"timeout"`
192201
}
193202

203+
// validatorConfig selects the custom validation a request passes before it is
204+
// batched.
205+
type validatorConfig struct {
206+
Type string `yaml:"type"`
207+
// TokenEnv names the environment variable holding the API token (github only).
208+
TokenEnv string `yaml:"tokenEnv"`
209+
// BaseURL is the GitHub API root, for GitHub Enterprise (github only).
210+
BaseURL string `yaml:"baseUrl"`
211+
// Host is the GitHub instance change URIs must name (github only).
212+
Host string `yaml:"host"`
213+
Timeout string `yaml:"timeout"`
214+
}
215+
194216
// buildRunnerConfig selects how a batch's build is triggered and polled.
195217
type buildRunnerConfig struct {
196218
Type string `yaml:"type"`
@@ -304,6 +326,11 @@ func (c *profilesConfig) normalizeAndValidate() error {
304326
return err
305327
}
306328
}
329+
if q.Validator != nil {
330+
if err := q.Validator.normalizeAndValidate(where); err != nil {
331+
return err
332+
}
333+
}
307334
if q.BuildRunner != nil {
308335
if err := q.BuildRunner.normalizeAndValidate(where); err != nil {
309336
return err
@@ -370,6 +397,9 @@ func (c profilesConfig) resolve(q namedQueueProfileConfig) queueProfileConfig {
370397
if q.ChangeProvider != nil {
371398
profile.ChangeProvider = *q.ChangeProvider
372399
}
400+
if q.Validator != nil {
401+
profile.Validator = *q.Validator
402+
}
373403
if q.BuildRunner != nil {
374404
profile.BuildRunner = *q.BuildRunner
375405
}
@@ -411,6 +441,9 @@ func (p *queueProfileConfig) normalizeAndValidate(where string) error {
411441
if err := p.ChangeProvider.normalizeAndValidate(where); err != nil {
412442
return err
413443
}
444+
if err := p.Validator.normalizeAndValidate(where); err != nil {
445+
return err
446+
}
414447
if err := p.BuildRunner.normalizeAndValidate(where); err != nil {
415448
return err
416449
}
@@ -511,6 +544,29 @@ func (c *changeProviderConfig) ensurePhabricator(where string) error {
511544
return nil
512545
}
513546

547+
func (v *validatorConfig) normalizeAndValidate(where string) error {
548+
if v.Type == "" {
549+
v.Type = validatorTypeFake
550+
}
551+
switch v.Type {
552+
case validatorTypeFake:
553+
return nil
554+
case validatorTypeGitHub:
555+
if v.TokenEnv == "" {
556+
v.TokenEnv = defaultGitHubTokenEnv
557+
}
558+
if v.BaseURL == "" {
559+
v.BaseURL = defaultGitHubBaseURL
560+
}
561+
if v.Host == "" {
562+
v.Host = defaultGitHubHost
563+
}
564+
return nil
565+
default:
566+
return fmt.Errorf("%s: unknown validator type %q", where, v.Type)
567+
}
568+
}
569+
514570
func (b *buildRunnerConfig) normalizeAndValidate(where string) error {
515571
if b.Type == "" {
516572
b.Type = buildRunnerTypeFake

‎service/submitqueue/orchestrator/server/config_test.go‎

Lines changed: 74 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ import (
3535
"github.com/uber/submitqueue/submitqueue/extension/changeprovider"
3636
"github.com/uber/submitqueue/submitqueue/extension/conflict"
3737
"github.com/uber/submitqueue/submitqueue/extension/speculation/speculator"
38+
"github.com/uber/submitqueue/submitqueue/extension/validator"
3839
)
3940

4041
func TestValidateProfileQueueTenants(t *testing.T) {
@@ -122,13 +123,15 @@ queues:
122123
resolved := cfg.resolve(cfg.Queues[0])
123124
assert.Equal(t, analyzerTypeNone, resolved.Analyzer.Type)
124125
assert.Equal(t, changeProviderTypeFake, resolved.ChangeProvider.Type)
126+
assert.Equal(t, validatorTypeFake, resolved.Validator.Type)
125127
assert.Equal(t, buildRunnerTypeFake, resolved.BuildRunner.Type)
126128
}
127129

128130
func TestLoadProfilesConfig_AppliesForgeDefaults(t *testing.T) {
129131
path := writeProfiles(t, `
130132
defaults:
131133
changeProvider: {type: github}
134+
validator: {type: github}
132135
buildRunner: {type: githubactions, owner: uber, repo: sq-sandbox, workflow: validate.yml}
133136
`)
134137

@@ -138,6 +141,9 @@ defaults:
138141
require.NotNil(t, cfg.Defaults.ChangeProvider.GitHub)
139142
assert.Equal(t, defaultGitHubTokenEnv, cfg.Defaults.ChangeProvider.GitHub.TokenEnv)
140143
assert.Equal(t, defaultGitHubBaseURL, cfg.Defaults.ChangeProvider.GitHub.BaseURL)
144+
assert.Equal(t, defaultGitHubTokenEnv, cfg.Defaults.Validator.TokenEnv)
145+
assert.Equal(t, defaultGitHubBaseURL, cfg.Defaults.Validator.BaseURL)
146+
assert.Equal(t, defaultGitHubHost, cfg.Defaults.Validator.Host)
141147
assert.Equal(t, defaultGitHubTokenEnv, cfg.Defaults.BuildRunner.TokenEnv)
142148
assert.Equal(t, defaultGitHubBaseURL, cfg.Defaults.BuildRunner.BaseURL)
143149
}
@@ -146,6 +152,7 @@ func TestLoadProfilesConfig_EmptyFileIsAllFakes(t *testing.T) {
146152
cfg, err := loadProfilesConfig(writeProfiles(t, ""))
147153
require.NoError(t, err)
148154
assert.Equal(t, changeProviderTypeFake, cfg.Defaults.ChangeProvider.Type)
155+
assert.Equal(t, validatorTypeFake, cfg.Defaults.Validator.Type)
149156
assert.Equal(t, buildRunnerTypeFake, cfg.Defaults.BuildRunner.Type)
150157
assert.Equal(t, analyzerTypeAll, cfg.Defaults.Analyzer.Type)
151158
}
@@ -170,6 +177,14 @@ func TestLoadProfilesConfig_Rejects(t *testing.T) {
170177
name: "unknown change provider",
171178
contents: "defaults:\n changeProvider: {type: gitlab}\n",
172179
},
180+
{
181+
name: "unknown validator",
182+
contents: "defaults:\n validator: {type: lint}\n",
183+
},
184+
{
185+
name: "unknown validator on a queue",
186+
contents: "queues:\n - name: a\n validator: {type: lint}\n",
187+
},
173188
{
174189
name: "unknown build runner",
175190
contents: "defaults:\n buildRunner: {type: jenkins}\n",
@@ -308,18 +323,70 @@ func TestNewProfiles_ResolvesPerQueueAnalyzers(t *testing.T) {
308323
func TestNewProfiles_FailsWhenAForgeTokenIsMissing(t *testing.T) {
309324
// Failing at startup beats accepting land requests and discovering the
310325
// credential gap one API call into the first merge.
311-
t.Setenv("TEST_GH_TOKEN_UNSET", "")
326+
tests := []struct {
327+
name string
328+
contents string
329+
}{
330+
{
331+
name: "github change provider",
332+
contents: "defaults:\n changeProvider:\n type: github\n github: {tokenEnv: TEST_GH_TOKEN_UNSET}\n",
333+
},
334+
{
335+
name: "github validator",
336+
contents: "queues:\n - name: gh\n validator: {type: github, tokenEnv: TEST_GH_TOKEN_UNSET}\n",
337+
},
338+
}
339+
340+
for _, tt := range tests {
341+
t.Run(tt.name, func(t *testing.T) {
342+
t.Setenv("TEST_GH_TOKEN_UNSET", "")
343+
cfg, err := loadProfilesConfig(writeProfiles(t, tt.contents))
344+
require.NoError(t, err)
345+
346+
_, err = newProfiles(context.Background(), zaptest.NewLogger(t), tally.NoopScope, nil, nil, cfg)
347+
require.Error(t, err)
348+
})
349+
}
350+
}
351+
352+
func TestNewProfiles_ResolvesPerQueueValidators(t *testing.T) {
353+
// A change URI on another GitHub host is one the GitHub validator rejects
354+
// without calling GitHub, so the two types differ observably offline.
355+
t.Setenv("TEST_GH_TOKEN", "token")
312356
path := writeProfiles(t, `
313-
defaults:
314-
changeProvider:
315-
type: github
316-
github: {tokenEnv: TEST_GH_TOKEN_UNSET}
357+
queues:
358+
- name: gh
359+
validator: {type: github, tokenEnv: TEST_GH_TOKEN}
317360
`)
318361
cfg, err := loadProfilesConfig(path)
319362
require.NoError(t, err)
363+
profiles, err := newProfiles(context.Background(), zaptest.NewLogger(t), tally.NoopScope, nil, nil, cfg)
364+
require.NoError(t, err)
320365

321-
_, err = newProfiles(context.Background(), zaptest.NewLogger(t), tally.NoopScope, nil, nil, cfg)
322-
require.Error(t, err)
366+
foreignHost := entity.Request{Change: change.Change{URIs: []string{
367+
"github://github.example.com/acme/widgets/pull/1/" + strings.Repeat("a", 40),
368+
}}}
369+
370+
tests := []struct {
371+
queue string
372+
wantReject bool
373+
}{
374+
{queue: "gh", wantReject: true},
375+
{queue: "unlisted-falls-back-to-fake"},
376+
}
377+
for _, tt := range tests {
378+
t.Run(tt.queue, func(t *testing.T) {
379+
v, err := profiles.ValidatorFactory().For(validator.Config{QueueName: tt.queue})
380+
require.NoError(t, err)
381+
382+
err = v.Validate(context.Background(), foreignHost)
383+
if tt.wantReject {
384+
assert.Error(t, err)
385+
} else {
386+
assert.NoError(t, err)
387+
}
388+
})
389+
}
323390
}
324391

325392
func TestNewProfiles_RoutingFallsBackToFakeWithoutTokens(t *testing.T) {

‎service/submitqueue/orchestrator/server/main.go‎

Lines changed: 1 addition & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -50,8 +50,6 @@ import (
5050
servicemq "github.com/uber/submitqueue/service/messagequeue"
5151
"github.com/uber/submitqueue/service/submitqueue/core"
5252
"github.com/uber/submitqueue/submitqueue/core/changeset"
53-
"github.com/uber/submitqueue/submitqueue/extension/validator"
54-
validatorfake "github.com/uber/submitqueue/submitqueue/extension/validator/fake"
5553
"github.com/uber/submitqueue/submitqueue/orchestrator"
5654
mysqlstorage "github.com/uber/submitqueue/submitqueue/orchestrator/extension/storage/mysql"
5755
"go.uber.org/zap"
@@ -220,7 +218,7 @@ func run() error {
220218
ChangeProvider: profiles.ChangeProviderFactory(),
221219
Analyzer: profiles.AnalyzerFactory(),
222220
Speculator: profiles.SpeculatorFactory(),
223-
Validator: validatorFactory{},
221+
Validator: profiles.ValidatorFactory(),
224222
Hooks: hookResolver{},
225223
}
226224

@@ -492,14 +490,3 @@ type hookResolver struct{}
492490
func (hookResolver) For(*basehook.HookEvent) []hookext.Hook {
493491
return []hookext.Hook{hooknoop.New()}
494492
}
495-
496-
// validatorFactory routes every queue to the always-passing fake validator.
497-
// Choosing an impl per queue is host policy, so the adapter lives here rather
498-
// than in the extension package. A deployment with real validation swaps this
499-
// for a routing adapter.
500-
type validatorFactory struct{}
501-
502-
// For returns the Validator bound to the queue named in config.
503-
func (validatorFactory) For(config validator.Config) (validator.Validator, error) {
504-
return validatorfake.New(config), nil
505-
}

0 commit comments

Comments
 (0)