feat(validator): add a GitHub validator and select each queue's validator in the orchestrator profiles

service/submitqueue/demo/provider/README.md

@@ -4,7 +4,7 @@ Each directory here is one **provider** — a code-hosting system SubmitQueue la
 
 | File | Selects |
 |---|---|
-| `profiles.yaml` | the change provider, build runner, conflict analyzer, scorer and build budget each queue resolves to (read by the orchestrator) |
+| `profiles.yaml` | the change provider, validator, build runner, conflict analyzer, scorer and build budget each queue resolves to (read by the orchestrator) |
 | `merge.yaml` | the merge target each queue lands on (read by Runway) |
 
 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

@@ -68,6 +68,7 @@ go_library(
         "//submitqueue/extension/speculation/speculator/standard:go_default_library",
         "//submitqueue/extension/validator:go_default_library",
         "//submitqueue/extension/validator/fake:go_default_library",
+        "//submitqueue/extension/validator/github:go_default_library",
         "//submitqueue/orchestrator:go_default_library",
         "//submitqueue/orchestrator/extension/storage:go_default_library",
         "//submitqueue/orchestrator/extension/storage/mysql:go_default_library",
@@ -144,6 +145,7 @@ go_test(
         "//submitqueue/extension/queuepause/fake:go_default_library",
         "//submitqueue/extension/speculation/scorer:go_default_library",
         "//submitqueue/extension/speculation/speculator:go_default_library",
+        "//submitqueue/extension/validator:go_default_library",
         "//submitqueue/orchestrator/extension/storage:go_default_library",
         "@com_github_stretchr_testify//assert:go_default_library",
         "@com_github_stretchr_testify//require:go_default_library",

service/submitqueue/orchestrator/server/config.go

@@ -40,6 +40,12 @@ const (
 	buildRunnerTypeBuildkite     = "buildkite"
 )
 
+// Validator types selectable from configuration.
+const (
+	validatorTypeFake   = "fake"
+	validatorTypeGitHub = "github"
+)
+
 // Conflict analyzer types selectable from configuration.
 const (
 	analyzerTypeAll         = "all"
@@ -97,6 +103,7 @@ const (
 	defaultGitTokenUser   = "x-access-token"
 	defaultGitHubTokenEnv = "GITHUB_TOKEN"
 	defaultGitHubBaseURL  = "https://api.github.com"
+	defaultGitHubHost     = "github.com"
 	defaultPhabTokenEnv   = "PHAB_API_TOKEN"
 	defaultHTTPTimeout    = 30 * time.Second
 )
@@ -122,6 +129,7 @@ type profilesConfig struct {
 type namedQueueProfileConfig struct {
 	Name           string                `yaml:"name"`
 	ChangeProvider *changeProviderConfig `yaml:"changeProvider"`
+	Validator      *validatorConfig      `yaml:"validator"`
 	BuildRunner    *buildRunnerConfig    `yaml:"buildRunner"`
 	Analyzer       *analyzerConfig       `yaml:"analyzer"`
 	Scorer         *scorerConfig         `yaml:"scorer"`
@@ -131,6 +139,7 @@ type namedQueueProfileConfig struct {
 // queueProfileConfig is the full set of extensions a queue resolves to.
 type queueProfileConfig struct {
 	ChangeProvider changeProviderConfig `yaml:"changeProvider"`
+	Validator      validatorConfig      `yaml:"validator"`
 	BuildRunner    buildRunnerConfig    `yaml:"buildRunner"`
 	Analyzer       analyzerConfig       `yaml:"analyzer"`
 	Scorer         scorerConfig         `yaml:"scorer"`
@@ -191,6 +200,19 @@ type phabProviderConfig struct {
 	Timeout  string `yaml:"timeout"`
 }
 
+// validatorConfig selects the custom validation a request passes before it is
+// batched.
+type validatorConfig struct {
+	Type string `yaml:"type"`
+	// TokenEnv names the environment variable holding the API token (github only).
+	TokenEnv string `yaml:"tokenEnv"`
+	// BaseURL is the GitHub API root, for GitHub Enterprise (github only).
+	BaseURL string `yaml:"baseUrl"`
+	// Host is the GitHub instance change URIs must name (github only).
+	Host    string `yaml:"host"`
+	Timeout string `yaml:"timeout"`
+}
+
 // buildRunnerConfig selects how a batch's build is triggered and polled.
 type buildRunnerConfig struct {
 	Type string `yaml:"type"`
@@ -304,6 +326,11 @@ func (c *profilesConfig) normalizeAndValidate() error {
 				return err
 			}
 		}
+		if q.Validator != nil {
+			if err := q.Validator.normalizeAndValidate(where); err != nil {
+				return err
+			}
+		}
 		if q.BuildRunner != nil {
 			if err := q.BuildRunner.normalizeAndValidate(where); err != nil {
 				return err
@@ -370,6 +397,9 @@ func (c profilesConfig) resolve(q namedQueueProfileConfig) queueProfileConfig {
 	if q.ChangeProvider != nil {
 		profile.ChangeProvider = *q.ChangeProvider
 	}
+	if q.Validator != nil {
+		profile.Validator = *q.Validator
+	}
 	if q.BuildRunner != nil {
 		profile.BuildRunner = *q.BuildRunner
 	}
@@ -411,6 +441,9 @@ func (p *queueProfileConfig) normalizeAndValidate(where string) error {
 	if err := p.ChangeProvider.normalizeAndValidate(where); err != nil {
 		return err
 	}
+	if err := p.Validator.normalizeAndValidate(where); err != nil {
+		return err
+	}
 	if err := p.BuildRunner.normalizeAndValidate(where); err != nil {
 		return err
 	}
@@ -511,6 +544,29 @@ func (c *changeProviderConfig) ensurePhabricator(where string) error {
 	return nil
 }
 
+func (v *validatorConfig) normalizeAndValidate(where string) error {
+	if v.Type == "" {
+		v.Type = validatorTypeFake
+	}
+	switch v.Type {
+	case validatorTypeFake:
+		return nil
+	case validatorTypeGitHub:
+		if v.TokenEnv == "" {
+			v.TokenEnv = defaultGitHubTokenEnv
+		}
+		if v.BaseURL == "" {
+			v.BaseURL = defaultGitHubBaseURL
+		}
+		if v.Host == "" {
+			v.Host = defaultGitHubHost
+		}
+		return nil
+	default:
+		return fmt.Errorf("%s: unknown validator type %q", where, v.Type)
+	}
+}
+
 func (b *buildRunnerConfig) normalizeAndValidate(where string) error {
 	if b.Type == "" {
 		b.Type = buildRunnerTypeFake

service/submitqueue/orchestrator/server/config_test.go

@@ -35,6 +35,7 @@ import (
 	"github.com/uber/submitqueue/submitqueue/extension/changeprovider"
 	"github.com/uber/submitqueue/submitqueue/extension/conflict"
 	"github.com/uber/submitqueue/submitqueue/extension/speculation/speculator"
+	"github.com/uber/submitqueue/submitqueue/extension/validator"
 )
 
 func TestValidateProfileQueueTenants(t *testing.T) {
@@ -122,13 +123,15 @@ queues:
 	resolved := cfg.resolve(cfg.Queues[0])
 	assert.Equal(t, analyzerTypeNone, resolved.Analyzer.Type)
 	assert.Equal(t, changeProviderTypeFake, resolved.ChangeProvider.Type)
+	assert.Equal(t, validatorTypeFake, resolved.Validator.Type)
 	assert.Equal(t, buildRunnerTypeFake, resolved.BuildRunner.Type)
 }
 
 func TestLoadProfilesConfig_AppliesForgeDefaults(t *testing.T) {
 	path := writeProfiles(t, `
 defaults:
   changeProvider: {type: github}
+  validator: {type: github}
   buildRunner: {type: githubactions, owner: uber, repo: sq-sandbox, workflow: validate.yml}
 `)
 
@@ -138,6 +141,9 @@ defaults:
 	require.NotNil(t, cfg.Defaults.ChangeProvider.GitHub)
 	assert.Equal(t, defaultGitHubTokenEnv, cfg.Defaults.ChangeProvider.GitHub.TokenEnv)
 	assert.Equal(t, defaultGitHubBaseURL, cfg.Defaults.ChangeProvider.GitHub.BaseURL)
+	assert.Equal(t, defaultGitHubTokenEnv, cfg.Defaults.Validator.TokenEnv)
+	assert.Equal(t, defaultGitHubBaseURL, cfg.Defaults.Validator.BaseURL)
+	assert.Equal(t, defaultGitHubHost, cfg.Defaults.Validator.Host)
 	assert.Equal(t, defaultGitHubTokenEnv, cfg.Defaults.BuildRunner.TokenEnv)
 	assert.Equal(t, defaultGitHubBaseURL, cfg.Defaults.BuildRunner.BaseURL)
 }
@@ -146,6 +152,7 @@ func TestLoadProfilesConfig_EmptyFileIsAllFakes(t *testing.T) {
 	cfg, err := loadProfilesConfig(writeProfiles(t, ""))
 	require.NoError(t, err)
 	assert.Equal(t, changeProviderTypeFake, cfg.Defaults.ChangeProvider.Type)
+	assert.Equal(t, validatorTypeFake, cfg.Defaults.Validator.Type)
 	assert.Equal(t, buildRunnerTypeFake, cfg.Defaults.BuildRunner.Type)
 	assert.Equal(t, analyzerTypeAll, cfg.Defaults.Analyzer.Type)
 }
@@ -170,6 +177,14 @@ func TestLoadProfilesConfig_Rejects(t *testing.T) {
 			name:     "unknown change provider",
 			contents: "defaults:\n  changeProvider: {type: gitlab}\n",
 		},
+		{
+			name:     "unknown validator",
+			contents: "defaults:\n  validator: {type: lint}\n",
+		},
+		{
+			name:     "unknown validator on a queue",
+			contents: "queues:\n  - name: a\n    validator: {type: lint}\n",
+		},
 		{
 			name:     "unknown build runner",
 			contents: "defaults:\n  buildRunner: {type: jenkins}\n",
@@ -308,18 +323,70 @@ func TestNewProfiles_ResolvesPerQueueAnalyzers(t *testing.T) {
 func TestNewProfiles_FailsWhenAForgeTokenIsMissing(t *testing.T) {
 	// Failing at startup beats accepting land requests and discovering the
 	// credential gap one API call into the first merge.
-	t.Setenv("TEST_GH_TOKEN_UNSET", "")
+	tests := []struct {
+		name     string
+		contents string
+	}{
+		{
+			name:     "github change provider",
+			contents: "defaults:\n  changeProvider:\n    type: github\n    github: {tokenEnv: TEST_GH_TOKEN_UNSET}\n",
+		},
+		{
+			name:     "github validator",
+			contents: "queues:\n  - name: gh\n    validator: {type: github, tokenEnv: TEST_GH_TOKEN_UNSET}\n",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Setenv("TEST_GH_TOKEN_UNSET", "")
+			cfg, err := loadProfilesConfig(writeProfiles(t, tt.contents))
+			require.NoError(t, err)
+
+			_, err = newProfiles(context.Background(), zaptest.NewLogger(t), tally.NoopScope, nil, nil, cfg)
+			require.Error(t, err)
+		})
+	}
+}
+
+func TestNewProfiles_ResolvesPerQueueValidators(t *testing.T) {
+	// A change URI on another GitHub host is one the GitHub validator rejects
+	// without calling GitHub, so the two types differ observably offline.
+	t.Setenv("TEST_GH_TOKEN", "token")
 	path := writeProfiles(t, `
-defaults:
-  changeProvider:
-    type: github
-    github: {tokenEnv: TEST_GH_TOKEN_UNSET}
+queues:
+  - name: gh
+    validator: {type: github, tokenEnv: TEST_GH_TOKEN}
 `)
 	cfg, err := loadProfilesConfig(path)
 	require.NoError(t, err)
+	profiles, err := newProfiles(context.Background(), zaptest.NewLogger(t), tally.NoopScope, nil, nil, cfg)
+	require.NoError(t, err)
 
-	_, err = newProfiles(context.Background(), zaptest.NewLogger(t), tally.NoopScope, nil, nil, cfg)
-	require.Error(t, err)
+	foreignHost := entity.Request{Change: change.Change{URIs: []string{
+		"github://github.example.com/acme/widgets/pull/1/" + strings.Repeat("a", 40),
+	}}}
+
+	tests := []struct {
+		queue      string
+		wantReject bool
+	}{
+		{queue: "gh", wantReject: true},
+		{queue: "unlisted-falls-back-to-fake"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.queue, func(t *testing.T) {
+			v, err := profiles.ValidatorFactory().For(validator.Config{QueueName: tt.queue})
+			require.NoError(t, err)
+
+			err = v.Validate(context.Background(), foreignHost)
+			if tt.wantReject {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+			}
+		})
+	}
 }
 
 func TestNewProfiles_RoutingFallsBackToFakeWithoutTokens(t *testing.T) {

service/submitqueue/orchestrator/server/main.go

@@ -50,8 +50,6 @@ import (
 	servicemq "github.com/uber/submitqueue/service/messagequeue"
 	"github.com/uber/submitqueue/service/submitqueue/core"
 	"github.com/uber/submitqueue/submitqueue/core/changeset"
-	"github.com/uber/submitqueue/submitqueue/extension/validator"
-	validatorfake "github.com/uber/submitqueue/submitqueue/extension/validator/fake"
 	"github.com/uber/submitqueue/submitqueue/orchestrator"
 	mysqlstorage "github.com/uber/submitqueue/submitqueue/orchestrator/extension/storage/mysql"
 	"go.uber.org/zap"
@@ -220,7 +218,7 @@ func run() error {
 		ChangeProvider: profiles.ChangeProviderFactory(),
 		Analyzer:       profiles.AnalyzerFactory(),
 		Speculator:     profiles.SpeculatorFactory(),
-		Validator:      validatorFactory{},
+		Validator:      profiles.ValidatorFactory(),
 		Hooks:          hookResolver{},
 	}
 
@@ -492,14 +490,3 @@ type hookResolver struct{}
 func (hookResolver) For(*basehook.HookEvent) []hookext.Hook {
 	return []hookext.Hook{hooknoop.New()}
 }
-
-// validatorFactory routes every queue to the always-passing fake validator.
-// Choosing an impl per queue is host policy, so the adapter lives here rather
-// than in the extension package. A deployment with real validation swaps this
-// for a routing adapter.
-type validatorFactory struct{}
-
-// For returns the Validator bound to the queue named in config.
-func (validatorFactory) For(config validator.Config) (validator.Validator, error) {
-	return validatorfake.New(config), nil
-}