Skip to content

Commit 75709dc

Browse files
committed
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 <[email protected]>
1 parent 542f1a0 commit 75709dc

File tree

2 files changed

+209
-105
lines changed

2 files changed

+209
-105
lines changed

pkg/provider/github/app/token.go

Lines changed: 45 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,11 @@ import (
44
"context"
55
"fmt"
66
"net/http"
7+
"net/url"
8+
"strings"
79
"time"
810

911
"github.com/golang-jwt/jwt/v4"
10-
gt "github.com/google/go-github/v71/github"
1112
"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
1213
"github.com/openshift-pipelines/pipelines-as-code/pkg/params"
1314
"github.com/openshift-pipelines/pipelines-as-code/pkg/provider/github"
@@ -20,8 +21,6 @@ type Install struct {
2021
repo *v1alpha1.Repository
2122
ghClient *github.Provider
2223
namespace string
23-
24-
repoList []string
2524
}
2625

2726
func NewInstallation(req *http.Request, run *params.Run, repo *v1alpha1.Repository, gh *github.Provider, namespace string) *Install {
@@ -38,91 +37,70 @@ func NewInstallation(req *http.Request, run *params.Run, repo *v1alpha1.Reposito
3837
}
3938

4039
// GetAndUpdateInstallationID retrieves and updates the installation ID for the GitHub App.
41-
// It generates a JWT token, lists all installations, and matches repositories to their installation IDs.
42-
// If a matching repository is found, it returns the enterprise host, token, and installation ID.
40+
// It generates a JWT token, and directly fetches the installation for the
41+
// repository.
4342
func (ip *Install) GetAndUpdateInstallationID(ctx context.Context) (string, string, int64, error) {
44-
var (
45-
enterpriseHost, token string
46-
installationID int64
47-
)
43+
logger := logging.FromContext(ctx)
4844

4945
// Generate a JWT token for authentication
5046
jwtToken, err := ip.GenerateJWT(ctx)
5147
if err != nil {
5248
return "", "", 0, err
5349
}
5450

51+
// Get owner and repo from the repository URL
52+
repoURL, err := url.Parse(ip.repo.Spec.URL)
53+
if err != nil {
54+
return "", "", 0, fmt.Errorf("failed to parse repository URL: %w", err)
55+
}
56+
pathParts := strings.Split(strings.Trim(repoURL.Path, "/"), "/")
57+
if len(pathParts) != 2 {
58+
return "", "", 0, fmt.Errorf("invalid repository URL path: %s", repoURL.Path)
59+
}
60+
owner := pathParts[0]
61+
repoName := pathParts[1]
62+
if owner == "" || repoName == "" {
63+
return "", "", 0, fmt.Errorf("invalid repository URL: owner or repo name is empty")
64+
}
65+
66+
if ip.ghClient.APIURL == nil {
67+
return "", "", 0, fmt.Errorf("github client APIURL is nil")
68+
}
5569
apiURL := *ip.ghClient.APIURL
56-
enterpriseHost = ip.request.Header.Get("X-GitHub-Enterprise-Host")
70+
enterpriseHost := ip.request.Header.Get("X-GitHub-Enterprise-Host")
5771
if enterpriseHost != "" {
58-
// NOTE: Hopefully this works even when the GHE URL is on another host than the API URL
59-
apiURL = "https://" + enterpriseHost + "/api/v3"
72+
apiURL = fmt.Sprintf("https://%s/api/v3", strings.TrimSuffix(enterpriseHost, "/"))
6073
}
6174

62-
logger := logging.FromContext(ctx)
63-
opt := &gt.ListOptions{PerPage: ip.ghClient.PaginedNumber}
6475
client, _, _ := github.MakeClient(ctx, apiURL, jwtToken)
65-
installationData := []*gt.Installation{}
66-
67-
// List all installations
68-
for {
69-
installationSet, resp, err := client.Apps.ListInstallations(ctx, opt)
76+
// Directly get the installation for the repository
77+
installation, _, err := client.Apps.FindRepositoryInstallation(ctx, owner, repoName)
78+
if err != nil {
79+
// Fallback to finding organization installation if repository installation is not found
80+
installation, _, err = client.Apps.FindOrganizationInstallation(ctx, owner)
7081
if err != nil {
71-
return "", "", 0, err
72-
}
73-
installationData = append(installationData, installationSet...)
74-
if resp.NextPage == 0 {
75-
break
76-
}
77-
opt.Page = resp.NextPage
78-
}
79-
80-
// Iterate through each installation to find a matching repository
81-
for i := range installationData {
82-
if installationData[i].ID == nil {
83-
return "", "", 0, fmt.Errorf("installation ID is nil")
84-
}
85-
if *installationData[i].ID != 0 {
86-
token, err = ip.ghClient.GetAppToken(ctx, ip.run.Clients.Kube, enterpriseHost, *installationData[i].ID, ip.namespace)
87-
// While looping on the list of installations, there could be cases where we can't
88-
// obtain a token for installation. In a test I did for GitHub App with ~400
89-
// installations, there were 3 failing consistently with:
90-
// "could not refresh installation id XXX's token: received non 2xx response status "403 Forbidden".
91-
// If there is a matching installation after the failure, we miss it. So instead of
92-
// failing, we just log the error and continue. Token is "".
82+
// Fallback to finding user installation if organization installation is not found
83+
installation, _, err = client.Apps.FindUserInstallation(ctx, owner)
9384
if err != nil {
94-
logger.Warn(err)
95-
continue
85+
return "", "", 0, fmt.Errorf("could not find repository, organization or user installation for %s/%s: %w", owner, repoName, err)
9686
}
9787
}
98-
exist, err := ip.matchRepos(ctx)
99-
if err != nil {
100-
return "", "", 0, err
101-
}
102-
if exist {
103-
installationID = *installationData[i].ID
104-
break
105-
}
10688
}
107-
return enterpriseHost, token, installationID, nil
108-
}
10989

110-
// matchRepos matches GitHub repositories to their installation IDs.
111-
// It lists all repositories accessible to the app installation and checks if
112-
// any match the repository URL in the spec.
113-
func (ip *Install) matchRepos(ctx context.Context) (bool, error) {
114-
installationRepoList, err := github.ListRepos(ctx, ip.ghClient)
115-
if err != nil {
116-
return false, err
90+
if installation.ID == nil {
91+
return "", "", 0, fmt.Errorf("github App installation found but contained no ID. This is likely a bug")
11792
}
118-
ip.repoList = append(ip.repoList, installationRepoList...)
119-
for i := range installationRepoList {
120-
// If URL matches with repo spec URL then we can break the loop
121-
if installationRepoList[i] == ip.repo.Spec.URL {
122-
return true, nil
123-
}
93+
94+
installationID := *installation.ID
95+
token, err := ip.ghClient.GetAppToken(ctx, ip.run.Clients.Kube, enterpriseHost, installationID, ip.namespace)
96+
if err != nil {
97+
logger.Warnf("Could not get a token for installation ID %d: %v", installationID, err)
98+
// Return with the installation ID even if token generation fails,
99+
// as some operations might only need the ID.
100+
return enterpriseHost, "", installationID, nil
124101
}
125-
return false, nil
102+
103+
return enterpriseHost, token, installationID, nil
126104
}
127105

128106
// JWTClaim represents the JWT claims for the GitHub App.

0 commit comments

Comments
 (0)