diff --git a/README.md b/README.md index 7eeb875d..ffbb5116 100644 --- a/README.md +++ b/README.md @@ -290,7 +290,8 @@ github-app { } ``` -Installations can also be discovered dynamically via the GitHub API. +Each App lists its installations via `GET /app/installations` at startup and every five minutes. Orgs matching no +installation (case-insensitively) use the `fallback-org` token without further API calls. ## CLI @@ -336,7 +337,7 @@ metrics { Admin endpoints: `/_liveness`, `/_readiness`, `PUT /admin/log/level`, `/admin/pprof/`. GitHub App installations are checked at startup and every minute via `/rate_limit`. -The poller includes dynamically discovered installations and the configured fallback organization. +The poller includes every listed installation and the configured fallback organization. | Metric | Type | Labels | | --- | --- | --- | @@ -346,9 +347,9 @@ The poller includes dynamically discovered installations and the configured fall | `github.api_calls` | Counter | `owner`, `operation` | `app` is the configured App ID. `owner` identifies the installation actually used, -including fallback credentials. Installation discovery uses `owner=unknown` because -it authenticates as the App before resolving an installation. API calls for release -metadata/assets, installation discovery, token creation and quota polling are counted. Responses with less than +including fallback credentials. Installation listing uses `owner=unknown` because +it authenticates as the App rather than an installation. API calls for release +metadata/assets, installation listing, token creation and quota polling are counted. Responses with less than 10% of their API quota remaining produce a warning. Static release tokens receive API call counts and warnings, but are not polled for quota gauges. diff --git a/cmd/cachewd/main.go b/cmd/cachewd/main.go index 561c186b..49b2a142 100644 --- a/cmd/cachewd/main.go +++ b/cmd/cachewd/main.go @@ -393,6 +393,9 @@ func startMetrics(ctx context.Context, client *metrics.Client, provider githubap return errors.Wrap(err, "create GitHub App token manager") } if manager != nil { + go manager.RunInstallationRefresh(ctx, func(err error) { + logging.FromContext(ctx).WarnContext(ctx, "Failed to refresh GitHub App installations", "error", err) + }) go githubapp.NewRateLimitPoller(manager, 0).Run(ctx, func(err error) { logging.FromContext(ctx).WarnContext(ctx, "Failed to poll GitHub rate limit", "error", err) }) diff --git a/internal/githubapp/poller_external_test.go b/internal/githubapp/poller_external_test.go index 0bfc043c..108d65a2 100644 --- a/internal/githubapp/poller_external_test.go +++ b/internal/githubapp/poller_external_test.go @@ -18,8 +18,8 @@ import ( func TestRateLimitPollerBootstrapsFallback(t *testing.T) { polled := make(chan struct{}) mux := http.NewServeMux() - mux.HandleFunc("GET /orgs/squareup/installation", func(w http.ResponseWriter, _ *http.Request) { - _, _ = fmt.Fprint(w, `{"id":123}`) + mux.HandleFunc("GET /app/installations", func(w http.ResponseWriter, _ *http.Request) { + _, _ = fmt.Fprint(w, `[{"id":123,"account":{"login":"block"}}]`) }) mux.HandleFunc("POST /app/installations/123/access_tokens", func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusCreated) @@ -33,7 +33,7 @@ func TestRateLimitPollerBootstrapsFallback(t *testing.T) { server := httptest.NewServer(mux) defer server.Close() manager := githubapp.NewTokenManagerForTest(t, []githubapp.Config{{}, { - AppID: "111", PrivateKeyPath: generateTestKey(t), FallbackOrg: "squareup", + AppID: "111", PrivateKeyPath: generateTestKey(t), FallbackOrg: "block", }}, slog.Default(), server.URL, server.Client()) ctx, cancel := context.WithCancel(logging.ContextWithLogger(t.Context(), slog.Default())) defer cancel() diff --git a/internal/githubapp/ratelimit_test.go b/internal/githubapp/ratelimit_test.go index f66ab522..9d940048 100644 --- a/internal/githubapp/ratelimit_test.go +++ b/internal/githubapp/ratelimit_test.go @@ -60,11 +60,10 @@ func collectMetrics(t *testing.T, reader *sdkmetric.ManualReader) map[string][]m func cachedTestApp(server *httptest.Server, appID, owner, installationID string) *appState { app := &appState{ appID: appID, apiBase: server.URL, httpClient: NewAPIClient(server.Client()), - cacheConfig: DefaultTokenCacheConfig(), installationCache: make(map[string]string), - installationOwners: make(map[string]string), - tokens: map[string]*cachedToken{installationID: {token: "token-" + appID, expiresAt: time.Now().Add(time.Hour)}}, + cacheConfig: DefaultTokenCacheConfig(), + tokens: map[string]*cachedToken{installationID: {token: "token-" + appID, expiresAt: time.Now().Add(time.Hour)}}, } - app.rememberInstallation(owner, installationID, owner) + app.setInstallations(map[string]string{installationID: owner}) return app } @@ -80,10 +79,9 @@ func TestRateLimitPollerSamplesInstallations(t *testing.T) { _, _ = fmt.Fprint(w, `{"resources":{"core":{"remaining":4900,"limit":5000,"used":100},"search":{"remaining":30,"limit":30,"used":0}}}`) })) defer server.Close() - app := cachedTestApp(server, "111", "squareup", "123") - app.rememberInstallation("alias", "123", "alias") - otherApp := cachedTestApp(server, "222", "block", "456") - manager := &TokenManager{apps: []*appState{app, otherApp}, fallbackApp: app, fallbackOrg: "squareup"} + app := cachedTestApp(server, "111", "block", "123") + otherApp := cachedTestApp(server, "222", "example-org", "456") + manager := &TokenManager{apps: []*appState{app, otherApp}, fallbackApp: app, fallbackOrg: "block"} assert.NoError(t, NewRateLimitPoller(manager, 0).poll(pollerContext(t))) assert.Equal(t, int64(2), calls.Load()) data := collectMetrics(t, reader) @@ -101,11 +99,11 @@ func TestRateLimitPollerSamplesInstallations(t *testing.T) { owner, _ := point.Attributes.Value("owner") if appID.AsString() == "111" { assert.Equal(t, "123", installation.AsString()) - assert.Equal(t, "squareup", owner.AsString()) + assert.Equal(t, "block", owner.AsString()) } else { assert.Equal(t, "222", appID.AsString()) assert.Equal(t, "456", installation.AsString()) - assert.Equal(t, "block", owner.AsString()) + assert.Equal(t, "example-org", owner.AsString()) } } } @@ -117,11 +115,11 @@ func TestRateLimitPollerFindsNewInstallations(t *testing.T) { _, _ = fmt.Fprint(w, `{"resources":{"core":{"remaining":50,"limit":100,"used":50}}}`) })) defer server.Close() - app := cachedTestApp(server, "111", "squareup", "123") + app := cachedTestApp(server, "111", "block", "123") poller := NewRateLimitPoller(&TokenManager{apps: []*appState{app}}, 0) assert.NoError(t, poller.poll(pollerContext(t))) assert.Equal(t, 1, len(collectMetrics(t, reader)["github.ratelimit_remaining"])) - app.rememberInstallation("block", "456", "block") + app.setInstallations(map[string]string{"123": "block", "456": "example-org"}) app.mu.Lock() app.tokens["456"] = &cachedToken{token: "later-token", expiresAt: time.Now().Add(time.Hour)} app.mu.Unlock() @@ -139,7 +137,7 @@ func TestRateLimitPrometheusExport(t *testing.T) { _, _ = fmt.Fprint(w, `{"resources":{"core":{"remaining":42,"limit":100,"used":58}}}`) })) defer server.Close() - app := cachedTestApp(server, "111", "squareup", "123") + app := cachedTestApp(server, "111", "block", "123") assert.NoError(t, NewRateLimitPoller(&TokenManager{apps: []*appState{app}}, 0).poll(pollerContext(t))) response := httptest.NewRecorder() client.Handler().ServeHTTP(response, httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/metrics", nil)) @@ -147,33 +145,10 @@ func TestRateLimitPrometheusExport(t *testing.T) { assert.Contains(t, response.Body.String(), `github_ratelimit_remaining{`) assert.Contains(t, response.Body.String(), `app="111"`) assert.Contains(t, response.Body.String(), `installation_id="123"`) - assert.Contains(t, response.Body.String(), `owner="squareup"`) + assert.Contains(t, response.Body.String(), `owner="block"`) assert.Contains(t, response.Body.String(), `resource="core"`) } -func TestRateLimitPollerRetainsOwnerAcrossAliases(t *testing.T) { - reader := newMetricReader(t) - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - _, _ = fmt.Fprint(w, `{"resources":{"core":{"remaining":42,"limit":100,"used":58}}}`) - })) - defer server.Close() - app := cachedTestApp(server, "111", "example-org", "123") - poller := NewRateLimitPoller(&TokenManager{apps: []*appState{app}}, 0) - for _, alias := range []string{"Example-Org", "old-name", "renamed-org"} { - app.rememberInstallation(alias, "123", alias) - token, installation, err := app.getToken(pollerContext(t), alias) - assert.NoError(t, err) - assert.Equal(t, "token-111", token) - assert.Equal(t, Installation{ID: "123", Owner: "example-org"}, installation) - assert.Equal(t, map[string]string{"123": "example-org"}, app.knownInstallations()) - assert.NoError(t, poller.poll(pollerContext(t))) - points := collectMetrics(t, reader)["github.ratelimit_remaining"] - assert.Equal(t, 1, len(points)) - assert.Equal(t, attribute.NewSet(attribute.String("app", "111"), attribute.String("owner", "example-org"), - attribute.String("installation_id", "123"), attribute.String("resource", "core")), points[0].Attributes) - } -} - func TestRateLimitPollerErrorsDoNotEmitZeroes(t *testing.T) { for _, tc := range []struct { name string @@ -190,7 +165,7 @@ func TestRateLimitPollerErrorsDoNotEmitZeroes(t *testing.T) { _, _ = fmt.Fprint(w, tc.body) })) defer server.Close() - app := cachedTestApp(server, "111", "squareup", "123") + app := cachedTestApp(server, "111", "block", "123") poller := NewRateLimitPoller(&TokenManager{apps: []*appState{app}}, 0) assert.Error(t, poller.poll(pollerContext(t))) assert.Zero(t, len(collectMetrics(t, reader)["github.ratelimit_remaining"])) @@ -209,13 +184,13 @@ func TestRateLimitPollerContinuesAfterFailure(t *testing.T) { })) defer server.Close() manager := &TokenManager{apps: []*appState{ - cachedTestApp(server, "111", "squareup", "123"), cachedTestApp(server, "222", "block", "456"), + cachedTestApp(server, "111", "block", "123"), cachedTestApp(server, "222", "example-org", "456"), }} assert.Error(t, NewRateLimitPoller(manager, 0).poll(pollerContext(t))) points := collectMetrics(t, reader)["github.ratelimit_remaining"] assert.Equal(t, 1, len(points)) assert.Equal(t, int64(42), points[0].Value) - assert.Equal(t, attribute.NewSet(attribute.String("app", "222"), attribute.String("owner", "block"), + assert.Equal(t, attribute.NewSet(attribute.String("app", "222"), attribute.String("owner", "example-org"), attribute.String("installation_id", "456"), attribute.String("resource", "core")), points[0].Attributes) } @@ -232,7 +207,7 @@ func TestRateLimitPollerRun(t *testing.T) { defer server.Close() ctx, cancel := context.WithTimeout(pollerContext(t), time.Second) defer cancel() - poller := NewRateLimitPoller(&TokenManager{apps: []*appState{cachedTestApp(server, "111", "squareup", "123")}}, time.Millisecond) + poller := NewRateLimitPoller(&TokenManager{apps: []*appState{cachedTestApp(server, "111", "block", "123")}}, time.Millisecond) done := make(chan struct{}) go func() { poller.Run(ctx, func(err error) { t.Error(err) }) @@ -307,7 +282,7 @@ func TestRateLimitPollerCancelsRequest(t *testing.T) { defer server.Close() ctx, cancel := context.WithCancel(pollerContext(t)) defer cancel() - poller := NewRateLimitPoller(&TokenManager{apps: []*appState{cachedTestApp(server, "111", "squareup", "123")}}, 0) + poller := NewRateLimitPoller(&TokenManager{apps: []*appState{cachedTestApp(server, "111", "block", "123")}}, 0) done := make(chan struct{}) go func() { poller.Run(ctx, func(err error) { t.Error(err) }) diff --git a/internal/githubapp/tokens.go b/internal/githubapp/tokens.go index e284c0cb..fa3e37aa 100644 --- a/internal/githubapp/tokens.go +++ b/internal/githubapp/tokens.go @@ -38,9 +38,10 @@ type appState struct { httpClient *http.Client apiBase string + listMu sync.Mutex // serialises installation listings installationMu sync.RWMutex - installationCache map[string]string // org -> installation ID (dynamically discovered) - installationOwners map[string]string // installation ID -> stable owner + installations map[string]string // lowercased account login -> installation ID, nil until listed + installationOwners map[string]string // installation ID -> account login mu sync.RWMutex tokens map[string]*cachedToken @@ -53,16 +54,13 @@ type cachedToken struct { // TokenManager manages GitHub App installation tokens across one or more apps. type TokenManager struct { - mu sync.RWMutex - orgToApp map[string]*appState - - apps []*appState // all configured apps, for dynamic installation discovery + apps []*appState fallbackApp *appState fallbackOrg string } func newTokenManager(configs []Config, logger *slog.Logger, apiBase string, httpClient *http.Client) (*TokenManager, error) { - tm := &TokenManager{orgToApp: make(map[string]*appState)} + tm := &TokenManager{} for _, config := range configs { hasAny := config.AppID != "" || config.PrivateKeyPath != "" @@ -81,14 +79,12 @@ func newTokenManager(configs []Config, logger *slog.Logger, apiBase string, http } app := &appState{ - appID: config.AppID, - jwtGenerator: jwtGen, - cacheConfig: cacheConfig, - httpClient: NewAPIClient(httpClient), - apiBase: apiBase, - installationCache: make(map[string]string), - installationOwners: make(map[string]string), - tokens: make(map[string]*cachedToken), + appID: config.AppID, + jwtGenerator: jwtGen, + cacheConfig: cacheConfig, + httpClient: NewAPIClient(httpClient), + apiBase: apiBase, + tokens: make(map[string]*cachedToken), } tm.apps = append(tm.apps, app) @@ -107,9 +103,8 @@ func newTokenManager(configs []Config, logger *slog.Logger, apiBase string, http } // GetTokenForOrg returns an installation token for the given GitHub organization. -// It dynamically discovers the installation ID via the GitHub API on first use, -// caches the result, and falls back to the fallback org's token for orgs where -// the app is not installed. +// Installations are listed via the GitHub API on first use and refreshed by +// RunInstallationRefresh. Orgs without an installation get the fallback org's token. func (tm *TokenManager) GetTokenForOrg(ctx context.Context, org string) (string, error) { token, _, err := tm.GetTokenForOrgWithInstallation(ctx, org) return token, err @@ -129,44 +124,63 @@ func (tm *TokenManager) GetTokenForOrgWithInstallation(ctx context.Context, org } logger := logging.FromContext(ctx) - - // Check cached discovery first - tm.mu.RLock() - app, ok := tm.orgToApp[org] - tm.mu.RUnlock() - if ok { - return app.getToken(ctx, org) - } - - // Discover installation via GitHub API for _, app := range tm.apps { installationID, err := app.lookupInstallationID(ctx, org) if err != nil { - logger.DebugContext(ctx, "Dynamic installation lookup failed", "org", org, "app_id", app.appID, "error", err) + logger.DebugContext(ctx, "GitHub App installation lookup failed", "org", org, "app_id", app.appID, "error", err) continue } - - logger.InfoContext(ctx, "Dynamically discovered GitHub App installation", "org", org, "app_id", app.appID, "installation_id", installationID) - - // Cache the mapping for future requests - tm.mu.Lock() - tm.orgToApp[org] = app - tm.mu.Unlock() - return app.getToken(ctx, org) + return app.getInstallationToken(ctx, installationID) } - // Fall back to fallback org if tm.fallbackApp != nil { logger.InfoContext(ctx, "Using fallback org token", "requested_org", org, "fallback_org", tm.fallbackOrg) - if _, err := tm.fallbackApp.lookupInstallationID(ctx, tm.fallbackOrg); err != nil { + installationID, err := tm.fallbackApp.lookupInstallationID(ctx, tm.fallbackOrg) + if err != nil { return "", Installation{}, errors.Wrap(err, "resolve fallback installation") } - return tm.fallbackApp.getToken(ctx, tm.fallbackOrg) + return tm.fallbackApp.getInstallationToken(ctx, installationID) } return "", Installation{}, errors.Errorf("no GitHub App installation found for org: %s", org) } +// RefreshInstallations relists the installations of every configured app. An app +// whose listing fails keeps its previously known installations. +func (tm *TokenManager) RefreshInstallations(ctx context.Context) error { + var errs []error + for _, app := range tm.apps { + if err := app.refreshInstallations(ctx); err != nil { + errs = append(errs, errors.Wrapf(err, "app %s", app.appID)) + } + } + return errors.Join(errs...) +} + +const installationRefreshInterval = 5 * time.Minute +const installationRefreshTimeout = 30 * time.Second + +// RunInstallationRefresh calls RefreshInstallations now and every five minutes until +// ctx is cancelled, so installations added later are found. It passes failures to +// onError, which must be non-nil. +func (tm *TokenManager) RunInstallationRefresh(ctx context.Context, onError func(error)) { + ticker := time.NewTicker(installationRefreshInterval) + defer ticker.Stop() + for { + refreshCtx, cancel := context.WithTimeout(ctx, installationRefreshTimeout) + err := tm.RefreshInstallations(refreshCtx) + cancel() + if err != nil && ctx.Err() == nil { + onError(err) + } + select { + case <-ctx.Done(): + return + case <-ticker.C: + } + } +} + // GetTokenForURL extracts the org from a GitHub URL and returns an installation token. func (tm *TokenManager) GetTokenForURL(ctx context.Context, url string) (string, error) { if tm == nil { @@ -180,16 +194,6 @@ func (tm *TokenManager) GetTokenForURL(ctx context.Context, url string) (string, return tm.GetTokenForOrg(ctx, org) } -func (a *appState) getToken(ctx context.Context, org string) (string, Installation, error) { - a.installationMu.RLock() - installationID := a.installationCache[org] - a.installationMu.RUnlock() - if installationID == "" { - return "", Installation{}, errors.Errorf("no installation ID for org: %s", org) - } - return a.getInstallationToken(ctx, installationID) -} - func (a *appState) getInstallationToken(ctx context.Context, installationID string) (string, Installation, error) { a.installationMu.RLock() owner := a.installationOwners[installationID] @@ -266,85 +270,117 @@ func (a *appState) fetchInstallationToken(ctx context.Context, org, installation return result.Token, result.ExpiresAt, nil } -// lookupInstallationID queries the GitHub API to find the installation ID for the -// given org. It tries /orgs/{org}/installation first, then /users/{user}/installation. -// Results are cached in installationCache. +// lookupInstallationID returns the app's installation ID for the given org or user +// from the listed installations, listing them first if they have never been listed. func (a *appState) lookupInstallationID(ctx context.Context, org string) (string, error) { - // Check cache first - a.installationMu.RLock() - if id, ok := a.installationCache[org]; ok { - a.installationMu.RUnlock() - return id, nil + if err := a.ensureInstallations(ctx); err != nil { + return "", err } + a.installationMu.RLock() + id, ok := a.installations[strings.ToLower(org)] a.installationMu.RUnlock() + if !ok { + return "", errors.Errorf("no GitHub App installation found for %s", org) + } + return id, nil +} - jwt, err := a.jwtGenerator.GenerateJWT() - if err != nil { - return "", errors.Wrap(err, "generate JWT") +func (a *appState) ensureInstallations(ctx context.Context) error { + if a.listed() { + return nil + } + a.listMu.Lock() + defer a.listMu.Unlock() + if a.listed() { + return nil } + return a.listInstallations(ctx) +} - // Try org endpoint first, then user endpoint - for _, endpoint := range []string{ - fmt.Sprintf("%s/orgs/%s/installation", a.apiBase, org), - fmt.Sprintf("%s/users/%s/installation", a.apiBase, org), - } { - req, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint, nil) - if err != nil { - return "", errors.Wrap(err, "create request") - } +func (a *appState) listed() bool { + a.installationMu.RLock() + defer a.installationMu.RUnlock() + return a.installations != nil +} - req.Header.Set("Accept", "application/vnd.github+json") - req.Header.Set("Authorization", "Bearer "+jwt) - req = req.WithContext(httputil.WithCooldownScope(req.Context(), "github-app:"+a.appID)) - req.Header.Set("X-Github-Api-Version", "2022-11-28") +// refreshInstallations replaces the known installations with the app's current +// installations, keeping the previous set if listing fails. +func (a *appState) refreshInstallations(ctx context.Context) error { + a.listMu.Lock() + defer a.listMu.Unlock() + return a.listInstallations(ctx) +} - resp, err := a.httpClient.Do(WithAPIMetrics(req, "unknown", "lookup_installation")) +func (a *appState) listInstallations(ctx context.Context) error { + jwt, err := a.jwtGenerator.GenerateJWT() + if err != nil { + return errors.Wrap(err, "generate JWT") + } + owners := map[string]string{} + for page := 1; ; page++ { + installations, err := a.listInstallationsPage(ctx, jwt, page) if err != nil { - return "", errors.Wrap(err, "execute request") - } - defer resp.Body.Close() - - if resp.StatusCode == http.StatusNotFound { - continue + return err } - - if resp.StatusCode != http.StatusOK { - return "", errors.Errorf("GitHub API returned status %d for %s", resp.StatusCode, endpoint) - } - - var result struct { - ID int64 `json:"id"` - Account struct { - Login string `json:"login"` - } `json:"account"` + for _, installation := range installations { + if installation.Account.Login != "" { + owners[strconv.FormatInt(installation.ID, 10)] = installation.Account.Login + } } - if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { - return "", errors.Wrap(err, "decode response") + if len(installations) < installationsPerPage { + break } + } + a.setInstallations(owners) + return nil +} - installationID := strconv.FormatInt(result.ID, 10) +const installationsPerPage = 100 - a.rememberInstallation(org, installationID, result.Account.Login) +type listedInstallation struct { + ID int64 `json:"id"` + Account struct { + Login string `json:"login"` + } `json:"account"` +} - return installationID, nil +func (a *appState) listInstallationsPage(ctx context.Context, jwt string, page int) ([]listedInstallation, error) { + url := fmt.Sprintf("%s/app/installations?per_page=%d&page=%d", a.apiBase, installationsPerPage, page) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) + if err != nil { + return nil, errors.Wrap(err, "create request") } + req.Header.Set("Accept", "application/vnd.github+json") + req.Header.Set("Authorization", "Bearer "+jwt) + req = req.WithContext(httputil.WithCooldownScope(req.Context(), "github-app:"+a.appID)) + req.Header.Set("X-Github-Api-Version", "2022-11-28") - return "", errors.Errorf("no GitHub App installation found for %s", org) + resp, err := a.httpClient.Do(WithAPIMetrics(req, "unknown", "list_installations")) + if err != nil { + return nil, errors.Wrap(err, "list installations") + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + return nil, errors.Errorf("GitHub API returned status %d for %s", resp.StatusCode, url) + } + var installations []listedInstallation + if err := json.NewDecoder(resp.Body).Decode(&installations); err != nil { + return nil, errors.Wrap(err, "decode installations") + } + return installations, nil } -func (a *appState) rememberInstallation(org, installationID, owner string) { - if owner == "" { - owner = org +// setInstallations replaces the known installations, keyed by installation ID with +// the account login as the owner. +func (a *appState) setInstallations(owners map[string]string) { + installations := make(map[string]string, len(owners)) + for id, owner := range owners { + installations[strings.ToLower(owner)] = id } a.installationMu.Lock() defer a.installationMu.Unlock() - if canonical, ok := a.installationOwners[installationID]; ok { - owner = canonical - } else { - a.installationOwners[installationID] = owner - } - a.installationCache[org] = installationID - a.installationCache[owner] = installationID + a.installations = installations + a.installationOwners = owners } func extractOrgFromURL(url string) (string, error) { diff --git a/internal/githubapp/tokens_test.go b/internal/githubapp/tokens_test.go index 02f6dad9..7b71e57c 100644 --- a/internal/githubapp/tokens_test.go +++ b/internal/githubapp/tokens_test.go @@ -106,26 +106,46 @@ func TestNewTokenManagerProvider(t *testing.T) { }) } -func TestGetTokenForOrgDynamicDiscovery(t *testing.T) { - // Mock GitHub API that returns installation IDs for known orgs - mux := http.NewServeMux() - mux.HandleFunc("GET /orgs/squareup/installation", func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusOK) - json.NewEncoder(w).Encode(map[string]any{"id": 12345}) //nolint:errcheck - }) - mux.HandleFunc("GET /orgs/AfterpayTouch/installation", func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusOK) - json.NewEncoder(w).Encode(map[string]any{"id": 67890}) //nolint:errcheck - }) - mux.HandleFunc("GET /orgs/unknown-org/installation", func(w http.ResponseWriter, _ *http.Request) { +func installationsJSON(t *testing.T, owners map[int]string) string { + t.Helper() + installations := []map[string]any{} + for id, login := range owners { + installations = append(installations, map[string]any{"id": id, "account": map[string]string{"login": login}}) + } + body, err := json.Marshal(installations) + assert.NoError(t, err) + return string(body) +} + +func rejectInstallationLookups(t *testing.T, mux *http.ServeMux) { + t.Helper() + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + t.Errorf("unexpected request %s %s", r.Method, r.URL.Path) w.WriteHeader(http.StatusNotFound) }) - mux.HandleFunc("GET /users/unknown-org/installation", func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusNotFound) +} + +func TestGetTokenForOrgDynamicDiscovery(t *testing.T) { + mux := http.NewServeMux() + rejectInstallationLookups(t, mux) + mux.HandleFunc("GET /app/installations", func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "100", r.URL.Query().Get("per_page")) + switch r.URL.Query().Get("page") { + case "1": + owners := map[int]string{12345: "block"} + for i := range 99 { + owners[i+1] = fmt.Sprintf("filler-%d", i) + } + _, _ = fmt.Fprint(w, installationsJSON(t, owners)) + case "2": + _, _ = fmt.Fprint(w, installationsJSON(t, map[int]string{67890: "AfterpayTouch"})) + default: + t.Errorf("unexpected page %q", r.URL.Query().Get("page")) + } }) mux.HandleFunc("POST /app/installations/12345/access_tokens", func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusCreated) - json.NewEncoder(w).Encode(map[string]any{"token": "ghs_squareup_token", "expires_at": "2099-01-01T00:00:00Z"}) //nolint:errcheck + json.NewEncoder(w).Encode(map[string]any{"token": "ghs_block_token", "expires_at": "2099-01-01T00:00:00Z"}) //nolint:errcheck }) mux.HandleFunc("POST /app/installations/67890/access_tokens", func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusCreated) @@ -142,7 +162,7 @@ func TestGetTokenForOrgDynamicDiscovery(t *testing.T) { { AppID: "111", PrivateKeyPath: keyPath, - FallbackOrg: "squareup", + FallbackOrg: "block", }, }, logger, server.URL, server.Client()) @@ -151,23 +171,23 @@ func TestGetTokenForOrgDynamicDiscovery(t *testing.T) { t.Run("FallbackBeforeDiscovery", func(t *testing.T) { token, installation, err := tm.GetTokenForOrgWithInstallation(ctx, "unknown-org") assert.NoError(t, err) - assert.Equal(t, "ghs_squareup_token", token) - assert.Equal(t, githubapp.Installation{ID: "12345", Owner: "squareup"}, installation) + assert.Equal(t, "ghs_block_token", token) + assert.Equal(t, githubapp.Installation{ID: "12345", Owner: "block"}, installation) }) t.Run("DiscoverAndCacheInstallation", func(t *testing.T) { - token, installation, err := tm.GetTokenForOrgWithInstallation(ctx, "squareup") + token, installation, err := tm.GetTokenForOrgWithInstallation(ctx, "block") assert.NoError(t, err) - assert.Equal(t, "ghs_squareup_token", token) - assert.Equal(t, githubapp.Installation{ID: "12345", Owner: "squareup"}, installation) + assert.Equal(t, "ghs_block_token", token) + assert.Equal(t, githubapp.Installation{ID: "12345", Owner: "block"}, installation) // Second call should use cache - token, err = tm.GetTokenForOrg(ctx, "squareup") + token, err = tm.GetTokenForOrg(ctx, "block") assert.NoError(t, err) - assert.Equal(t, "ghs_squareup_token", token) + assert.Equal(t, "ghs_block_token", token) }) - t.Run("DiscoverNewOrg", func(t *testing.T) { + t.Run("DiscoverInstallationOnLaterPage", func(t *testing.T) { token, installation, err := tm.GetTokenForOrgWithInstallation(ctx, "AfterpayTouch") assert.NoError(t, err) assert.Equal(t, "ghs_afterpay_token", token) @@ -177,17 +197,15 @@ func TestGetTokenForOrgDynamicDiscovery(t *testing.T) { t.Run("FallbackForUnknownOrg", func(t *testing.T) { token, err := tm.GetTokenForOrg(ctx, "unknown-org") assert.NoError(t, err) - assert.Equal(t, "ghs_squareup_token", token) + assert.Equal(t, "ghs_block_token", token) }) } func TestGetTokenForOrgNoFallback(t *testing.T) { mux := http.NewServeMux() - mux.HandleFunc("GET /orgs/unknown-org/installation", func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusNotFound) - }) - mux.HandleFunc("GET /users/unknown-org/installation", func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusNotFound) + rejectInstallationLookups(t, mux) + mux.HandleFunc("GET /app/installations", func(w http.ResponseWriter, _ *http.Request) { + _, _ = fmt.Fprint(w, `[]`) }) server := httptest.NewServer(mux) defer server.Close() @@ -209,15 +227,12 @@ func TestGetTokenForOrgNoFallback(t *testing.T) { assert.Contains(t, err.Error(), "no GitHub App installation found for org") } -func TestGetTokenForOrgRetainsInstallationOwner(t *testing.T) { +func TestGetTokenForOrgMatchesOwnerCaseInsensitively(t *testing.T) { var mints atomic.Int64 mux := http.NewServeMux() - mux.HandleFunc("GET /orgs/{org}/installation", func(w http.ResponseWriter, r *http.Request) { - owner := "example-org" - if r.PathValue("org") == "renamed-org" { - owner = "renamed-org" - } - assert.NoError(t, json.NewEncoder(w).Encode(map[string]any{"id": 123, "account": map[string]string{"login": owner}})) + rejectInstallationLookups(t, mux) + mux.HandleFunc("GET /app/installations", func(w http.ResponseWriter, _ *http.Request) { + _, _ = fmt.Fprint(w, installationsJSON(t, map[int]string{123: "Example-Org"})) }) mux.HandleFunc("POST /app/installations/123/access_tokens", func(w http.ResponseWriter, _ *http.Request) { mints.Add(1) @@ -229,15 +244,62 @@ func TestGetTokenForOrgRetainsInstallationOwner(t *testing.T) { manager := githubapp.NewTokenManagerForTest(t, []githubapp.Config{{AppID: "111", PrivateKeyPath: generateTestKey(t)}}, slog.Default(), server.URL, server.Client()) ctx := logging.ContextWithLogger(t.Context(), slog.Default()) - for _, alias := range []string{"Example-Org", "example-org", "renamed-org"} { + for _, alias := range []string{"example-org", "EXAMPLE-ORG", "Example-Org"} { token, installation, err := manager.GetTokenForOrgWithInstallation(ctx, alias) assert.NoError(t, err) assert.Equal(t, "test-token", token) - assert.Equal(t, githubapp.Installation{ID: "123", Owner: "example-org"}, installation) + assert.Equal(t, githubapp.Installation{ID: "123", Owner: "Example-Org"}, installation) } assert.Equal(t, int64(1), mints.Load()) } +func TestRefreshInstallationsFindsNewInstallations(t *testing.T) { + var listings atomic.Int64 + var listingStatus atomic.Int64 + listingStatus.Store(http.StatusOK) + var listing atomic.Value + listing.Store(installationsJSON(t, map[int]string{1: "block"})) + mux := http.NewServeMux() + rejectInstallationLookups(t, mux) + mux.HandleFunc("GET /app/installations", func(w http.ResponseWriter, _ *http.Request) { + listings.Add(1) + w.WriteHeader(int(listingStatus.Load())) + _, _ = fmt.Fprint(w, listing.Load()) + }) + mux.HandleFunc("POST /app/installations/{id}/access_tokens", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusCreated) + _, _ = fmt.Fprintf(w, `{"token":"token-%s","expires_at":"2099-01-01T00:00:00Z"}`, r.PathValue("id")) + }) + server := httptest.NewServer(mux) + defer server.Close() + manager := githubapp.NewTokenManagerForTest(t, []githubapp.Config{{ + AppID: "111", PrivateKeyPath: generateTestKey(t), FallbackOrg: "block", + }}, slog.Default(), server.URL, server.Client()) + ctx := logging.ContextWithLogger(t.Context(), slog.Default()) + + for range 3 { + token, installation, err := manager.GetTokenForOrgWithInstallation(ctx, "cashapp") + assert.NoError(t, err) + assert.Equal(t, "token-1", token) + assert.Equal(t, githubapp.Installation{ID: "1", Owner: "block"}, installation) + } + assert.Equal(t, int64(1), listings.Load()) + + listing.Store(installationsJSON(t, map[int]string{1: "block", 2: "CashApp"})) + assert.NoError(t, manager.RefreshInstallations(ctx)) + token, installation, err := manager.GetTokenForOrgWithInstallation(ctx, "cashapp") + assert.NoError(t, err) + assert.Equal(t, "token-2", token) + assert.Equal(t, githubapp.Installation{ID: "2", Owner: "CashApp"}, installation) + + listingStatus.Store(http.StatusInternalServerError) + assert.Error(t, manager.RefreshInstallations(ctx)) + token, err = manager.GetTokenForOrg(ctx, "cashapp") + assert.NoError(t, err) + assert.Equal(t, "token-2", token) + assert.Equal(t, int64(3), listings.Load()) +} + func TestGetTokenForOrgNilManager(t *testing.T) { var tm *githubapp.TokenManager _, err := tm.GetTokenForOrg(t.Context(), "any") @@ -245,7 +307,7 @@ func TestGetTokenForOrgNilManager(t *testing.T) { assert.Contains(t, err.Error(), "not initialized") } -func TestInstallationLookupMetricsUseBoundedOwner(t *testing.T) { +func TestMissingInstallationsMakeNoPerOrgCalls(t *testing.T) { previous := otel.GetMeterProvider() reader := sdkmetric.NewManualReader() provider := sdkmetric.NewMeterProvider(sdkmetric.WithReader(reader)) @@ -254,13 +316,18 @@ func TestInstallationLookupMetricsUseBoundedOwner(t *testing.T) { otel.SetMeterProvider(previous) assert.NoError(t, provider.Shutdown(context.Background())) }) - server := httptest.NewServer(http.NotFoundHandler()) + mux := http.NewServeMux() + rejectInstallationLookups(t, mux) + mux.HandleFunc("GET /app/installations", func(w http.ResponseWriter, _ *http.Request) { + _, _ = fmt.Fprint(w, `[]`) + }) + server := httptest.NewServer(mux) defer server.Close() manager := githubapp.NewTokenManagerForTest(t, []githubapp.Config{{AppID: "111", PrivateKeyPath: generateTestKey(t)}}, slog.Default(), server.URL, server.Client()) ctx := logging.ContextWithLogger(t.Context(), slog.Default()) for i := range 100 { - _, err := manager.GetTokenForOrg(ctx, fmt.Sprintf("nonexistent-org-%d", i)) + _, err := manager.GetTokenForOrg(ctx, fmt.Sprintf("nonexistent-org-%d", i%10)) assert.Error(t, err) } var data metricdata.ResourceMetrics @@ -277,9 +344,9 @@ func TestInstallationLookupMetricsUseBoundedOwner(t *testing.T) { } } assert.Equal(t, 1, len(points)) - assert.Equal(t, int64(200), points[0].Value) + assert.Equal(t, int64(1), points[0].Value) assert.Equal(t, attribute.NewSet(attribute.String("owner", "unknown"), - attribute.String("operation", "lookup_installation")), points[0].Attributes) + attribute.String("operation", "list_installations")), points[0].Attributes) } func TestGetTokenForURLNilManager(t *testing.T) {