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

Author: chmouel

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,91 +37,60 @@ 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)
 	if err != nil {
 		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]
+
 	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"
 	}
 
-	logger := logging.FromContext(ctx)
-	opt := &gt.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
+			return "", "", 0, fmt.Errorf("could not find repository or organization installation for %s/%s: %w", owner, repoName, err)
 		}
-		installationData = append(installationData, installationSet...)
-		if resp.NextPage == 0 {
-			break
-		}
-		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 installation.ID == nil {
+		return "", "", 0, fmt.Errorf("installation ID is nil")
 	}
-	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)
+	installationID := *installation.ID
+	token, err := ip.ghClient.GetAppToken(ctx, ip.run.Clients.Kube, enterpriseHost, installationID, ip.namespace)
 	if err != nil {
-		return false, err
-	}
-	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
-		}
+		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.

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"},
@@ -235,6 +236,10 @@ func Test_GetAndUpdateInstallationID(t *testing.T) {
 		_, _ = 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")
 		w.Header().Set("Authorization", "Bearer "+jwtToken)
@@ -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)
@@ -263,44 +270,3 @@ func testMethod(t *testing.T, r *http.Request, want string) {
 		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"}]}`)
-	})
-
-	repo := &v1alpha1.Repository{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: "repo",
-		},
-		Spec: v1alpha1.RepositorySpec{
-			URL: "https://matched/by/incoming",
-			Incomings: &[]v1alpha1.Incoming{
-				{
-					Targets: []string{"main"},
-					Secret: v1alpha1.Secret{
-						Name: "secret",
-					},
-				},
-			},
-		},
-	}
-
-	ctx, _ := rtesting.SetupFakeContext(t)
-	gprovider := &github.Provider{}
-	gprovider.SetGithubClient(fakeclient)
-	ip := NewInstallation(httptest.NewRequest(http.MethodGet, "http://localhost", strings.NewReader("")),
-		&params.Run{}, repo, gprovider, testNamespace.GetName())
-	exist, err := ip.matchRepos(ctx)
-	assert.NilError(t, err)
-	assert.Equal(t, exist, true)
-}