diff --git a/service/submitqueue/demo/provider/github/profiles.yaml b/service/submitqueue/demo/provider/github/profiles.yaml index 35b714877..7744d6d26 100644 --- a/service/submitqueue/demo/provider/github/profiles.yaml +++ b/service/submitqueue/demo/provider/github/profiles.yaml @@ -25,7 +25,7 @@ defaults: queues: - name: demo-queue # Reads pull request metadata from GitHub's GraphQL API. For GitHub - # Enterprise, add `github: {baseUrl: https://ghe.example.com/api/v3}`. + # Enterprise, add `github: {baseUrl: https://ghe.example.com/api/v3, host: ghe.example.com}`. changeProvider: {type: github} # Serializes batches that touch a shared directory, so changes land in diff --git a/service/submitqueue/orchestrator/server/config.go b/service/submitqueue/orchestrator/server/config.go index bf512ee69..80f0c61dd 100644 --- a/service/submitqueue/orchestrator/server/config.go +++ b/service/submitqueue/orchestrator/server/config.go @@ -205,6 +205,8 @@ type githubProviderConfig struct { TokenEnv string `yaml:"tokenEnv"` // BaseURL is the GitHub API root, for GitHub Enterprise. BaseURL string `yaml:"baseUrl"` + // Host is the GitHub instance change URIs must name. + Host string `yaml:"host"` // Timeout bounds each API call, as a Go duration ("30s"). Timeout string `yaml:"timeout"` } @@ -612,6 +614,9 @@ func (c *changeProviderConfig) ensureGitHub() { if c.GitHub.BaseURL == "" { c.GitHub.BaseURL = defaultGitHubBaseURL } + if c.GitHub.Host == "" { + c.GitHub.Host = defaultGitHubHost + } } func (c *changeProviderConfig) ensurePhabricator(where string) error { diff --git a/service/submitqueue/orchestrator/server/profiles.go b/service/submitqueue/orchestrator/server/profiles.go index 6cdbd4844..9a19dbce8 100644 --- a/service/submitqueue/orchestrator/server/profiles.go +++ b/service/submitqueue/orchestrator/server/profiles.go @@ -498,9 +498,7 @@ func (b *profileBuilder) newChangeProviderFactory(cfg changeProviderConfig, wher if makeProvider == nil { return nil, fmt.Errorf("%s: github change provider needs %s to be set", where, cfg.GitHub.TokenEnv) } - return changeProviderFunc(func(c changeprovider.Config) (changeprovider.ChangeProvider, error) { - return makeProvider(c), nil - }), nil + return changeProviderFunc(makeProvider), nil case changeProviderTypePhabricator: makeProvider, err := b.newPhabChangeProvider(*cfg.Phabricator, where) @@ -529,8 +527,9 @@ func (b *profileBuilder) newChangeProviderFactory(cfg changeProviderConfig, wher // unusable partway through a migration. func (b *profileBuilder) newRoutingChangeProviderFactory(cfg changeProviderConfig, where string) (changeprovider.Factory, error) { var ( - makeGitHub, makePhab func(changeprovider.Config) changeprovider.ChangeProvider - err error + makeGitHub func(changeprovider.Config) (changeprovider.ChangeProvider, error) + makePhab func(changeprovider.Config) changeprovider.ChangeProvider + err error ) if cfg.GitHub != nil { if makeGitHub, err = b.newGitHubChangeProvider(*cfg.GitHub, where); err != nil { @@ -552,9 +551,14 @@ func (b *profileBuilder) newRoutingChangeProviderFactory(cfg changeProviderConfi } return changeProviderFunc(func(c changeprovider.Config) (changeprovider.ChangeProvider, error) { - var gh, phab changeprovider.ChangeProvider + var ( + gh, phab changeprovider.ChangeProvider + err error + ) if makeGitHub != nil { - gh = makeGitHub(c) + if gh, err = makeGitHub(c); err != nil { + return nil, fmt.Errorf("%s: %w", where, err) + } } if makePhab != nil { phab = makePhab(c) @@ -570,7 +574,7 @@ func (b *profileBuilder) newRoutingChangeProviderFactory(cfg changeProviderConfi // newGitHubChangeProvider returns a function building the GitHub change provider // for a given queue, or nil when its token is unset. The HTTP client and metrics // scope are built once here; only the per-queue wrapper is built per call. -func (b *profileBuilder) newGitHubChangeProvider(cfg githubProviderConfig, where string) (func(changeprovider.Config) changeprovider.ChangeProvider, error) { +func (b *profileBuilder) newGitHubChangeProvider(cfg githubProviderConfig, where string) (func(changeprovider.Config) (changeprovider.ChangeProvider, error), error) { token, ok := tokenFrom(cfg.TokenEnv) if !ok { return nil, nil @@ -580,10 +584,11 @@ func (b *profileBuilder) newGitHubChangeProvider(cfg githubProviderConfig, where return nil, err } scope := b.scope.SubScope("changeprovider.github") - return func(c changeprovider.Config) changeprovider.ChangeProvider { + return func(c changeprovider.Config) (changeprovider.ChangeProvider, error) { return githubprovider.NewProvider(githubprovider.Params{ Config: c, HTTPClient: client, + Host: cfg.Host, Logger: b.logger.Sugar(), MetricsScope: scope, }) diff --git a/submitqueue/extension/changeprovider/github/provider.go b/submitqueue/extension/changeprovider/github/provider.go index 165544bad..a887509cb 100644 --- a/submitqueue/extension/changeprovider/github/provider.go +++ b/submitqueue/extension/changeprovider/github/provider.go @@ -2,6 +2,7 @@ package github import ( "context" + "errors" "fmt" "net/http" @@ -23,6 +24,9 @@ type Params struct { // HTTPClient is a pre-configured HTTP client. The caller is responsible for // configuring the base URL (via BaseURLTransport) and auth (via a transport layer). HTTPClient *http.Client + // Host is the GitHub instance change URIs must name (e.g. "github.com"): + // the one HTTPClient talks to. + Host string // Logger is the structured logger. Logger *zap.SugaredLogger // MetricsScope is the metrics scope for instrumentation. @@ -34,18 +38,24 @@ type provider struct { // cfg is the per-queue identity this provider was built for. cfg changeprovider.Config httpClient *http.Client + host string logger *zap.SugaredLogger metricsScope tally.Scope } -// NewProvider creates a new GitHub ChangeProvider. -func NewProvider(params Params) changeprovider.ChangeProvider { +// NewProvider creates a new GitHub ChangeProvider. It errors when Host is +// missing. +func NewProvider(params Params) (changeprovider.ChangeProvider, error) { + if params.Host == "" { + return nil, errors.New("github change provider: Host is required") + } return &provider{ cfg: params.Config, httpClient: params.HTTPClient, + host: params.Host, logger: params.Logger.Named("github_changeprovider"), metricsScope: params.MetricsScope.SubScope("github_changeprovider"), - } + }, nil } // Get retrieves change information from GitHub for the request's change. @@ -71,8 +81,9 @@ func (p *provider) Get(ctx context.Context, request entity.Request) (_ []entity. "uris", change.URIs, ) - // Validate stacked changes are consistent (same provider, org, and repo) - if err := validateChangeConsistency(changeIDs); err != nil { + // Checked before any API call: HTTPClient would read a PR on another host + // from this one. + if err := validateChangeConsistency(changeIDs, p.host); err != nil { return nil, err } diff --git a/submitqueue/extension/changeprovider/github/provider_test.go b/submitqueue/extension/changeprovider/github/provider_test.go index 0f703c74c..b357b61a7 100644 --- a/submitqueue/extension/changeprovider/github/provider_test.go +++ b/submitqueue/extension/changeprovider/github/provider_test.go @@ -34,11 +34,41 @@ func newTestProvider(t *testing.T, serverURL string) changeprovider.ChangeProvid t.Helper() client, err := phttp.NewClient(serverURL) require.NoError(t, err) - return NewProvider(Params{ + p, err := NewProvider(Params{ HTTPClient: client, + Host: "github.example.com", Logger: zaptest.NewLogger(t).Sugar(), MetricsScope: tally.NoopScope, }) + require.NoError(t, err) + return p +} + +func TestNewProvider(t *testing.T) { + tests := []struct { + name string + host string + wantErr bool + }{ + {name: "host set", host: "github.com"}, + {name: "host missing", wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := NewProvider(Params{ + HTTPClient: http.DefaultClient, + Host: tt.host, + Logger: zaptest.NewLogger(t).Sugar(), + MetricsScope: tally.NoopScope, + }) + if tt.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + }) + } } func servePR(t *testing.T, w http.ResponseWriter, data pullRequestData) { @@ -88,6 +118,14 @@ func TestProvider_Get(t *testing.T) { }, wantErr: true, }, + { + name: "PR on another host returns error without calling GitHub", + handler: func(w http.ResponseWriter, r *http.Request) { + t.Errorf("unexpected GitHub request %s %s", r.Method, r.URL.Path) + }, + uris: []string{"github://github.com/uber/submitqueue/pull/123/" + shaA}, + wantErr: true, + }, { name: "stale PR returns error", handler: func(w http.ResponseWriter, r *http.Request) { diff --git a/submitqueue/extension/changeprovider/github/validate.go b/submitqueue/extension/changeprovider/github/validate.go index 6c2adb19a..a05025120 100644 --- a/submitqueue/extension/changeprovider/github/validate.go +++ b/submitqueue/extension/changeprovider/github/validate.go @@ -7,15 +7,21 @@ import ( ) // validateChangeConsistency validates that all changeIDs in the stack are consistent. -// Stacked changes must have the same change provider (scheme), org, and repo. +// Stacked changes must name host and have the same change provider (scheme), org, +// and repo. func validateChangeConsistency( changeIDs []entitygithub.ChangeID, + host string, ) error { expectedScheme := changeIDs[0].Scheme expectedOrg := changeIDs[0].Org expectedRepo := changeIDs[0].Repo for _, cid := range changeIDs { + if cid.Host != host { + return fmt.Errorf("PR #%d names host %q, not %q", cid.PRNumber, cid.Host, host) + } + // Validate same change provider (scheme) if cid.Scheme != expectedScheme { return fmt.Errorf("stacked changes must use same change provider: expected %s, got %s for PR #%d", diff --git a/submitqueue/extension/changeprovider/github/validate_test.go b/submitqueue/extension/changeprovider/github/validate_test.go index 529fac1eb..5c1bd9b58 100644 --- a/submitqueue/extension/changeprovider/github/validate_test.go +++ b/submitqueue/extension/changeprovider/github/validate_test.go @@ -16,38 +16,60 @@ func TestValidateChangeConsistency(t *testing.T) { { name: "single PR", changeIDs: []entitygithub.ChangeID{ - {Scheme: "github", Org: "uber", Repo: "submitqueue", PRNumber: 1}, + {Scheme: "github", Host: "github.com", Org: "uber", Repo: "submitqueue", PRNumber: 1}, }, }, { name: "consistent stack", changeIDs: []entitygithub.ChangeID{ - {Scheme: "github", Org: "uber", Repo: "submitqueue", PRNumber: 1}, - {Scheme: "github", Org: "uber", Repo: "submitqueue", PRNumber: 2}, - {Scheme: "github", Org: "uber", Repo: "submitqueue", PRNumber: 3}, + {Scheme: "github", Host: "github.com", Org: "uber", Repo: "submitqueue", PRNumber: 1}, + {Scheme: "github", Host: "github.com", Org: "uber", Repo: "submitqueue", PRNumber: 2}, + {Scheme: "github", Host: "github.com", Org: "uber", Repo: "submitqueue", PRNumber: 3}, }, }, + { + name: "PR on another host", + changeIDs: []entitygithub.ChangeID{ + {Scheme: "github", Host: "github.example.com", Org: "uber", Repo: "submitqueue", PRNumber: 1}, + }, + wantErr: true, + }, + { + name: "stack spanning hosts", + changeIDs: []entitygithub.ChangeID{ + {Scheme: "github", Host: "github.com", Org: "uber", Repo: "submitqueue", PRNumber: 1}, + {Scheme: "github", Host: "github.example.com", Org: "uber", Repo: "submitqueue", PRNumber: 2}, + }, + wantErr: true, + }, + { + name: "host with a port it does not name", + changeIDs: []entitygithub.ChangeID{ + {Scheme: "github", Host: "github.com:8443", Org: "uber", Repo: "submitqueue", PRNumber: 1}, + }, + wantErr: true, + }, { name: "different repo", changeIDs: []entitygithub.ChangeID{ - {Scheme: "github", Org: "uber", Repo: "submitqueue", PRNumber: 1}, - {Scheme: "github", Org: "uber", Repo: "other-repo", PRNumber: 2}, + {Scheme: "github", Host: "github.com", Org: "uber", Repo: "submitqueue", PRNumber: 1}, + {Scheme: "github", Host: "github.com", Org: "uber", Repo: "other-repo", PRNumber: 2}, }, wantErr: true, }, { name: "different org", changeIDs: []entitygithub.ChangeID{ - {Scheme: "github", Org: "uber", Repo: "submitqueue", PRNumber: 1}, - {Scheme: "github", Org: "other-org", Repo: "submitqueue", PRNumber: 2}, + {Scheme: "github", Host: "github.com", Org: "uber", Repo: "submitqueue", PRNumber: 1}, + {Scheme: "github", Host: "github.com", Org: "other-org", Repo: "submitqueue", PRNumber: 2}, }, wantErr: true, }, { name: "different scheme", changeIDs: []entitygithub.ChangeID{ - {Scheme: "github", Org: "uber", Repo: "submitqueue", PRNumber: 1}, - {Scheme: "other", Org: "uber", Repo: "submitqueue", PRNumber: 2}, + {Scheme: "github", Host: "github.com", Org: "uber", Repo: "submitqueue", PRNumber: 1}, + {Scheme: "other", Host: "github.com", Org: "uber", Repo: "submitqueue", PRNumber: 2}, }, wantErr: true, }, @@ -55,7 +77,7 @@ func TestValidateChangeConsistency(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := validateChangeConsistency(tt.changeIDs) + err := validateChangeConsistency(tt.changeIDs, "github.com") if tt.wantErr { require.Error(t, err) } else {