From c0833b2dc026c7b9c9d2ec8cbf953e09d521b389 Mon Sep 17 00:00:00 2001 From: Chmouel Boudjnah Date: Tue, 1 Jul 2025 15:26:38 +0200 Subject: [PATCH] refactor: optimize GHApp installation ID retrieval - Removed listing of all installations. - Fetched the installation directly using the repository URL. - Added fallback to find the organization installation if the repository installation is not found. - Enhanced error handling for token retrieval. - Updated tests to reflect the changes. - Eliminated the matchRepos method from Install struct as it was unused refactor: Improve GitHub App installation ID retrieval Signed-off-by: Chmouel Boudjnah --- pkg/provider/github/app/token.go | 113 ++++++--------- pkg/provider/github/app/token_test.go | 201 +++++++++++++++++++++----- 2 files changed, 209 insertions(+), 105 deletions(-) diff --git a/pkg/provider/github/app/token.go b/pkg/provider/github/app/token.go index 51323edb6..949b4eecd 100644 --- a/pkg/provider/github/app/token.go +++ b/pkg/provider/github/app/token.go @@ -4,10 +4,11 @@ import ( "context" "fmt" "net/http" + "net/url" + "strings" "time" "github.com/golang-jwt/jwt/v4" - gt "github.com/google/go-github/v71/github" "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1" "github.com/openshift-pipelines/pipelines-as-code/pkg/params" "github.com/openshift-pipelines/pipelines-as-code/pkg/provider/github" @@ -20,8 +21,6 @@ type Install struct { repo *v1alpha1.Repository ghClient *github.Provider namespace string - - repoList []string } func NewInstallation(req *http.Request, run *params.Run, repo *v1alpha1.Repository, gh *github.Provider, namespace string) *Install { @@ -38,13 +37,10 @@ func NewInstallation(req *http.Request, run *params.Run, repo *v1alpha1.Reposito } // GetAndUpdateInstallationID retrieves and updates the installation ID for the GitHub App. -// It generates a JWT token, lists all installations, and matches repositories to their installation IDs. -// If a matching repository is found, it returns the enterprise host, token, and installation ID. +// It generates a JWT token, and directly fetches the installation for the +// repository. func (ip *Install) GetAndUpdateInstallationID(ctx context.Context) (string, string, int64, error) { - var ( - enterpriseHost, token string - installationID int64 - ) + logger := logging.FromContext(ctx) // Generate a JWT token for authentication jwtToken, err := ip.GenerateJWT(ctx) @@ -52,77 +48,60 @@ func (ip *Install) GetAndUpdateInstallationID(ctx context.Context) (string, stri return "", "", 0, err } + // Get owner and repo from the repository URL + repoURL, err := url.Parse(ip.repo.Spec.URL) + if err != nil { + return "", "", 0, fmt.Errorf("failed to parse repository URL: %w", err) + } + pathParts := strings.Split(strings.Trim(repoURL.Path, "/"), "/") + if len(pathParts) != 2 { + return "", "", 0, fmt.Errorf("invalid repository URL path: %s", repoURL.Path) + } + owner := pathParts[0] + repoName := pathParts[1] + if owner == "" || repoName == "" { + return "", "", 0, fmt.Errorf("invalid repository URL: owner or repo name is empty") + } + + if ip.ghClient.APIURL == nil { + return "", "", 0, fmt.Errorf("github client APIURL is nil") + } apiURL := *ip.ghClient.APIURL - enterpriseHost = ip.request.Header.Get("X-GitHub-Enterprise-Host") + enterpriseHost := ip.request.Header.Get("X-GitHub-Enterprise-Host") if enterpriseHost != "" { - // NOTE: Hopefully this works even when the GHE URL is on another host than the API URL - apiURL = "https://" + enterpriseHost + "/api/v3" + apiURL = fmt.Sprintf("https://%s/api/v3", strings.TrimSuffix(enterpriseHost, "/")) } - logger := logging.FromContext(ctx) - opt := >.ListOptions{PerPage: ip.ghClient.PaginedNumber} client, _, _ := github.MakeClient(ctx, apiURL, jwtToken) - installationData := []*gt.Installation{} - - // List all installations - for { - installationSet, resp, err := client.Apps.ListInstallations(ctx, opt) + // Directly get the installation for the repository + installation, _, err := client.Apps.FindRepositoryInstallation(ctx, owner, repoName) + if err != nil { + // Fallback to finding organization installation if repository installation is not found + installation, _, err = client.Apps.FindOrganizationInstallation(ctx, owner) if err != nil { - return "", "", 0, err - } - installationData = append(installationData, installationSet...) - if resp.NextPage == 0 { - break + // Fallback to finding user installation if organization installation is not found + installation, _, err = client.Apps.FindUserInstallation(ctx, owner) } - opt.Page = resp.NextPage } - // Iterate through each installation to find a matching repository - for i := range installationData { - if installationData[i].ID == nil { - return "", "", 0, fmt.Errorf("installation ID is nil") - } - if *installationData[i].ID != 0 { - token, err = ip.ghClient.GetAppToken(ctx, ip.run.Clients.Kube, enterpriseHost, *installationData[i].ID, ip.namespace) - // While looping on the list of installations, there could be cases where we can't - // obtain a token for installation. In a test I did for GitHub App with ~400 - // installations, there were 3 failing consistently with: - // "could not refresh installation id XXX's token: received non 2xx response status "403 Forbidden". - // If there is a matching installation after the failure, we miss it. So instead of - // failing, we just log the error and continue. Token is "". - if err != nil { - logger.Warn(err) - continue - } - } - exist, err := ip.matchRepos(ctx) - if err != nil { - return "", "", 0, err - } - if exist { - installationID = *installationData[i].ID - break - } + if err != nil { + return "", "", 0, fmt.Errorf("could not find repository, organization or user installation for %s/%s: %w", owner, repoName, err) } - return enterpriseHost, token, installationID, nil -} -// matchRepos matches GitHub repositories to their installation IDs. -// It lists all repositories accessible to the app installation and checks if -// any match the repository URL in the spec. -func (ip *Install) matchRepos(ctx context.Context) (bool, error) { - installationRepoList, err := github.ListRepos(ctx, ip.ghClient) - if err != nil { - return false, err + if installation.ID == nil { + return "", "", 0, fmt.Errorf("github App installation found but contained no ID. This is likely a bug") } - ip.repoList = append(ip.repoList, installationRepoList...) - for i := range installationRepoList { - // If URL matches with repo spec URL then we can break the loop - if installationRepoList[i] == ip.repo.Spec.URL { - return true, nil - } + + installationID := *installation.ID + token, err := ip.ghClient.GetAppToken(ctx, ip.run.Clients.Kube, enterpriseHost, installationID, ip.namespace) + if err != nil { + logger.Warnf("Could not get a token for installation ID %d: %v", installationID, err) + // Return with the installation ID even if token generation fails, + // as some operations might only need the ID. + return enterpriseHost, "", installationID, nil } - return false, nil + + return enterpriseHost, token, installationID, nil } // JWTClaim represents the JWT claims for the GitHub App. diff --git a/pkg/provider/github/app/token_test.go b/pkg/provider/github/app/token_test.go index 4c0a51671..8bea5e84d 100644 --- a/pkg/provider/github/app/token_test.go +++ b/pkg/provider/github/app/token_test.go @@ -170,6 +170,7 @@ func Test_GetAndUpdateInstallationID(t *testing.T) { badToken := "BADTOKEN" badID := 666 missingID := 111 + orgName := "org" fakeghclient, mux, serverURL, teardown := ghtesthelper.SetupGH() defer teardown() @@ -214,7 +215,7 @@ func Test_GetAndUpdateInstallationID(t *testing.T) { Name: "repo", }, Spec: v1alpha1.RepositorySpec{ - URL: "https://matched/by/incoming", + URL: fmt.Sprintf("https://matched/%s/incoming", orgName), Incomings: &[]v1alpha1.Incoming{ { Targets: []string{"main"}, @@ -229,14 +230,18 @@ func Test_GetAndUpdateInstallationID(t *testing.T) { gprovider := &github.Provider{APIURL: &serverURL, Run: run} gprovider.SetGithubClient(fakeghclient) mux.HandleFunc(fmt.Sprintf("/app/installations/%d/access_tokens", wantID), func(w http.ResponseWriter, r *http.Request) { - testMethod(t, r, "POST") + testMethod(t, r) w.Header().Set("Authorization", "Bearer "+jwtToken) w.Header().Set("Accept", "application/vnd.github+json") _, _ = fmt.Fprintf(w, `{"token": "%s"}`, wantToken) }) + mux.HandleFunc(fmt.Sprintf("/orgs/%s/installation", orgName), func(w http.ResponseWriter, _ *http.Request) { + _, _ = fmt.Fprintf(w, `{"id": %d}`, wantID) + }) + mux.HandleFunc(fmt.Sprintf("/app/installations/%d/access_tokens", badID), func(w http.ResponseWriter, r *http.Request) { - testMethod(t, r, "POST") + testMethod(t, r) w.Header().Set("Authorization", "Bearer "+jwtToken) w.Header().Set("Accept", "application/vnd.github+json") _, _ = fmt.Fprintf(w, `{"token": "%s"}`, badToken) @@ -247,7 +252,9 @@ func Test_GetAndUpdateInstallationID(t *testing.T) { mux.HandleFunc("/installation/repositories", func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Authorization", "Bearer 12345") w.Header().Set("Accept", "application/vnd.github+json") - _, _ = fmt.Fprint(w, `{"total_count": 2,"repositories": [{"id":1,"html_url": "https://matched/by/incoming"},{"id":2,"html_url": "https://anotherrepo/that/would/failit"}]}`) + _, _ = fmt.Fprintf(w, + `{"total_count": 2,"repositories": [{"id":1,"html_url": "https://matched/%s/incoming"},{"id":2,"html_url": "https://anotherrepo/that/would/failit"}]}`, + orgName) }) ip = NewInstallation(req, run, repo, gprovider, testNamespace.GetName()) _, token, installationID, err := ip.GetAndUpdateInstallationID(ctx) @@ -257,50 +264,168 @@ func Test_GetAndUpdateInstallationID(t *testing.T) { assert.Equal(t, token, wantToken) } -func testMethod(t *testing.T, r *http.Request, want string) { +func testMethod(t *testing.T, r *http.Request) { + want := "POST" t.Helper() if got := r.Method; got != want { t.Errorf("Request method: %v, want %v", got, want) } } -func Test_ListRepos(t *testing.T) { - fakeclient, mux, _, teardown := ghtesthelper.SetupGH() - defer teardown() - - mux.HandleFunc("user/installations/1/repositories/2", func(rw http.ResponseWriter, _ *http.Request) { - fmt.Fprint(rw) - }) - - mux.HandleFunc("/installation/repositories", func(w http.ResponseWriter, _ *http.Request) { - w.Header().Set("Authorization", "Bearer 12345") - w.Header().Set("Accept", "application/vnd.github+json") - _, _ = fmt.Fprint(w, `{"total_count": 1,"repositories": [{"id":1,"html_url": "https://matched/by/incoming"}]}`) - }) +func TestGetAndUpdateInstallationID_Fallbacks(t *testing.T) { + tdata := testclient.Data{ + Namespaces: []*corev1.Namespace{testNamespace}, + Secret: []*corev1.Secret{validSecret}, + } + wantToken := "GOODTOKEN" + orgName := "org" + repoName := "repo" + userName := "user" + orgID := int64(120) + userID := int64(121) - repo := &v1alpha1.Repository{ - ObjectMeta: metav1.ObjectMeta{ - Name: "repo", + tests := []struct { + name string + repoURL string + setupMux func(mux *http.ServeMux, jwtToken string) + wantErr bool + wantInstallationID int64 + wantToken string + skip bool + expectedErrorString string + }{ + { + name: "repo installation fails, org installation succeeds", + repoURL: fmt.Sprintf("https://matched/%s/%s", orgName, repoName), + setupMux: func(mux *http.ServeMux, jwtToken string) { + mux.HandleFunc(fmt.Sprintf("/repos/%s/%s/installation", orgName, repoName), func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + }) + mux.HandleFunc(fmt.Sprintf("/orgs/%s/installation", orgName), func(w http.ResponseWriter, _ *http.Request) { + _, _ = fmt.Fprintf(w, `{"id": %d}`, orgID) + }) + mux.HandleFunc(fmt.Sprintf("/app/installations/%d/access_tokens", orgID), func(w http.ResponseWriter, r *http.Request) { + testMethod(t, r) + w.Header().Set("Authorization", "Bearer "+jwtToken) + w.Header().Set("Accept", "application/vnd.github+json") + _, _ = fmt.Fprintf(w, `{"token": "%s"}`, wantToken) + }) + }, + wantErr: false, + wantInstallationID: orgID, + wantToken: wantToken, }, - Spec: v1alpha1.RepositorySpec{ - URL: "https://matched/by/incoming", - Incomings: &[]v1alpha1.Incoming{ - { - Targets: []string{"main"}, - Secret: v1alpha1.Secret{ - Name: "secret", - }, - }, + { + name: "repo and org installation fail, user installation succeeds", + repoURL: fmt.Sprintf("https://matched/%s/%s", userName, repoName), + setupMux: func(mux *http.ServeMux, jwtToken string) { + mux.HandleFunc(fmt.Sprintf("/repos/%s/%s/installation", userName, repoName), func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + }) + mux.HandleFunc(fmt.Sprintf("/orgs/%s/installation", userName), func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + }) + mux.HandleFunc(fmt.Sprintf("/users/%s/installation", userName), func(w http.ResponseWriter, _ *http.Request) { + _, _ = fmt.Fprintf(w, `{"id": %d}`, userID) + }) + mux.HandleFunc(fmt.Sprintf("/app/installations/%d/access_tokens", userID), func(w http.ResponseWriter, r *http.Request) { + testMethod(t, r) + w.Header().Set("Authorization", "Bearer "+jwtToken) + w.Header().Set("Accept", "application/vnd.github+json") + _, _ = fmt.Fprintf(w, `{"token": "%s"}`, wantToken) + }) + }, + wantErr: false, + wantInstallationID: userID, + wantToken: wantToken, + }, + { + name: "all installations fail", + repoURL: fmt.Sprintf("https://matched/%s/%s", orgName, repoName), + setupMux: func(mux *http.ServeMux, _ string) { + mux.HandleFunc(fmt.Sprintf("/repos/%s/%s/installation", orgName, repoName), func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + }) + mux.HandleFunc(fmt.Sprintf("/orgs/%s/installation", orgName), func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + }) + mux.HandleFunc(fmt.Sprintf("/users/%s/installation", orgName), func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + }) + }, + wantErr: true, + expectedErrorString: "could not find repository, organization or user installation", + }, + { + name: "invalid repo url", + repoURL: "https://invalid/url", + setupMux: func(_ *http.ServeMux, _ string) { }, + wantErr: true, + expectedErrorString: "invalid repository URL path", }, } - ctx, _ := rtesting.SetupFakeContext(t) - gprovider := &github.Provider{} - gprovider.SetGithubClient(fakeclient) - ip := NewInstallation(httptest.NewRequest(http.MethodGet, "http://localhost", strings.NewReader("")), - ¶ms.Run{}, repo, gprovider, testNamespace.GetName()) - exist, err := ip.matchRepos(ctx) - assert.NilError(t, err) - assert.Equal(t, exist, true) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.skip { + t.Skip() + } + fakeghclient, mux, serverURL, teardown := ghtesthelper.SetupGH() + defer teardown() + + ctx, _ := rtesting.SetupFakeContext(t) + stdata, _ := testclient.SeedTestData(t, ctx, tdata) + logger, _ := logger.GetLogger() + run := ¶ms.Run{ + Clients: clients.Clients{ + Log: logger, + PipelineAsCode: stdata.PipelineAsCode, + Kube: stdata.Kube, + }, + Info: info.Info{ + Pac: &info.PacOpts{ + Settings: settings.Settings{}, + }, + Controller: &info.ControllerInfo{Secret: validSecret.GetName()}, + }, + } + ctx = info.StoreCurrentControllerName(ctx, "default") + ctx = info.StoreNS(ctx, testNamespace.GetName()) + + ip := NewInstallation(httptest.NewRequest(http.MethodGet, "http://localhost", strings.NewReader("")), run, &v1alpha1.Repository{}, &github.Provider{}, testNamespace.GetName()) + jwtToken, err := ip.GenerateJWT(ctx) + assert.NilError(t, err) + + tt.setupMux(mux, jwtToken) + + repo := &v1alpha1.Repository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "repo", + }, + Spec: v1alpha1.RepositorySpec{ + URL: tt.repoURL, + }, + } + + gprovider := &github.Provider{APIURL: &serverURL, Run: run} + gprovider.SetGithubClient(fakeghclient) + t.Setenv("PAC_GIT_PROVIDER_TOKEN_APIURL", serverURL) + + ip = NewInstallation(httptest.NewRequest(http.MethodGet, "http://localhost", strings.NewReader("")), run, repo, gprovider, testNamespace.GetName()) + _, token, installationID, err := ip.GetAndUpdateInstallationID(ctx) + + if tt.wantErr { + assert.Assert(t, err != nil) + if tt.expectedErrorString != "" { + assert.Assert(t, strings.Contains(err.Error(), tt.expectedErrorString), "expected error string '%s' not found in '%s'", tt.expectedErrorString, err.Error()) + } + return + } + + assert.NilError(t, err) + assert.Equal(t, installationID, tt.wantInstallationID) + assert.Equal(t, token, tt.wantToken) + }) + } }