Skip to content

Commit 2219f5e

Browse files
mathur07mathur07
committed
KONFLUX-5929: Improve GitHub app installation ID retrieval
- Add eventEmitter parameter to NewInstallation for better error visibility - Use formatting.GetRepoOwnerSplitted for consistent URL parsing - Add GitHub-specific validation for owner/repo format - Emit events when GitHub App token generation fails - Add comprehensive test coverage for various scenarios - Handle token generation failures gracefully while returning installation ID
1 parent 8cdbaff commit 2219f5e

File tree

2 files changed

+84
-26
lines changed

2 files changed

+84
-26
lines changed

‎pkg/provider/github/app/token.go‎

Lines changed: 32 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -10,29 +10,34 @@ import (
1010

1111
"github.com/golang-jwt/jwt/v4"
1212
"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
13+
"github.com/openshift-pipelines/pipelines-as-code/pkg/events"
14+
"github.com/openshift-pipelines/pipelines-as-code/pkg/formatting"
1315
"github.com/openshift-pipelines/pipelines-as-code/pkg/params"
1416
"github.com/openshift-pipelines/pipelines-as-code/pkg/provider/github"
17+
"go.uber.org/zap"
1518
"knative.dev/pkg/logging"
1619
)
1720

1821
type Install struct {
19-
request *http.Request
20-
run *params.Run
21-
repo *v1alpha1.Repository
22-
ghClient *github.Provider
23-
namespace string
22+
request *http.Request
23+
run *params.Run
24+
repo *v1alpha1.Repository
25+
ghClient *github.Provider
26+
namespace string
27+
eventEmitter *events.EventEmitter
2428
}
2529

26-
func NewInstallation(req *http.Request, run *params.Run, repo *v1alpha1.Repository, gh *github.Provider, namespace string) *Install {
30+
func NewInstallation(req *http.Request, run *params.Run, repo *v1alpha1.Repository, gh *github.Provider, namespace string, eventEmitter *events.EventEmitter) *Install {
2731
if req == nil {
2832
req = &http.Request{}
2933
}
3034
return &Install{
31-
request: req,
32-
run: run,
33-
repo: repo,
34-
ghClient: gh,
35-
namespace: namespace,
35+
request: req,
36+
run: run,
37+
repo: repo,
38+
ghClient: gh,
39+
namespace: namespace,
40+
eventEmitter: eventEmitter,
3641
}
3742
}
3843

@@ -48,19 +53,20 @@ func (ip *Install) GetAndUpdateInstallationID(ctx context.Context) (string, stri
4853
return "", "", 0, err
4954
}
5055

51-
// Get owner and repo from the repository URL
56+
// Get owner and repo from the repository URL using the standard formatting function
57+
owner, repoName, err := formatting.GetRepoOwnerSplitted(ip.repo.Spec.URL)
58+
if err != nil {
59+
return "", "", 0, fmt.Errorf("failed to parse repository URL: %w", err)
60+
}
61+
62+
// GitHub-specific validation: ensure we have exactly owner/repo format (not group/subgroup/repo like GitLab)
5263
repoURL, err := url.Parse(ip.repo.Spec.URL)
5364
if err != nil {
5465
return "", "", 0, fmt.Errorf("failed to parse repository URL: %w", err)
5566
}
5667
pathParts := strings.Split(strings.Trim(repoURL.Path, "/"), "/")
5768
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")
69+
return "", "", 0, fmt.Errorf("invalid GitHub repository URL path: expected '<organization>/<repository>', got: %s", repoURL.Path)
6470
}
6571

6672
if ip.ghClient.APIURL == nil {
@@ -95,7 +101,14 @@ func (ip *Install) GetAndUpdateInstallationID(ctx context.Context) (string, stri
95101
installationID := *installation.ID
96102
token, err := ip.ghClient.GetAppToken(ctx, ip.run.Clients.Kube, enterpriseHost, installationID, ip.namespace)
97103
if err != nil {
98-
logger.Warnf("Could not get a token for installation ID %d: %v", installationID, err)
104+
msg := fmt.Sprintf("Could not get a token for installation ID %d: %v", installationID, err)
105+
logger.Warnf(msg)
106+
107+
// Emit an event for better visibility of GitHub App token issues
108+
if ip.eventEmitter != nil {
109+
ip.eventEmitter.EmitMessage(ip.repo, zap.WarnLevel, "GitHubAppTokenGenerationFailed", msg)
110+
}
111+
99112
// Return with the installation ID even if token generation fails,
100113
// as some operations might only need the ID.
101114
return enterpriseHost, "", installationID, nil

‎pkg/provider/github/app/token_test.go‎

Lines changed: 52 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,7 @@ func Test_GenerateJWT(t *testing.T) {
144144
},
145145
}
146146

147-
ip := NewInstallation(httptest.NewRequest(http.MethodGet, "http://localhost", strings.NewReader("")), run, &v1alpha1.Repository{}, &github.Provider{}, tt.namespace.GetName())
147+
ip := NewInstallation(httptest.NewRequest(http.MethodGet, "http://localhost", strings.NewReader("")), run, &v1alpha1.Repository{}, &github.Provider{}, tt.namespace.GetName(), nil)
148148
token, err := ip.GenerateJWT(ctx)
149149
if tt.wantErr {
150150
assert.Assert(t, err != nil)
@@ -206,7 +206,7 @@ func Test_GetAndUpdateInstallationID(t *testing.T) {
206206
ctx = info.StoreCurrentControllerName(ctx, "default")
207207
ctx = info.StoreNS(ctx, testNamespace.GetName())
208208

209-
ip := NewInstallation(httptest.NewRequest(http.MethodGet, "http://localhost", strings.NewReader("")), run, &v1alpha1.Repository{}, &github.Provider{}, testNamespace.GetName())
209+
ip := NewInstallation(httptest.NewRequest(http.MethodGet, "http://localhost", strings.NewReader("")), run, &v1alpha1.Repository{}, &github.Provider{}, testNamespace.GetName(), nil)
210210
jwtToken, err := ip.GenerateJWT(ctx)
211211
assert.NilError(t, err)
212212
req := httptest.NewRequest(http.MethodGet, "http://localhost", strings.NewReader(""))
@@ -256,7 +256,7 @@ func Test_GetAndUpdateInstallationID(t *testing.T) {
256256
`{"total_count": 2,"repositories": [{"id":1,"html_url": "https://matched/%s/incoming"},{"id":2,"html_url": "https://anotherrepo/that/would/failit"}]}`,
257257
orgName)
258258
})
259-
ip = NewInstallation(req, run, repo, gprovider, testNamespace.GetName())
259+
ip = NewInstallation(req, run, repo, gprovider, testNamespace.GetName(), nil)
260260
_, token, installationID, err := ip.GetAndUpdateInstallationID(ctx)
261261
assert.NilError(t, err)
262262
assert.Equal(t, installationID, int64(wantID))
@@ -294,6 +294,24 @@ func TestGetAndUpdateInstallationID_Fallbacks(t *testing.T) {
294294
skip bool
295295
expectedErrorString string
296296
}{
297+
{
298+
name: "repo installation succeeds directly",
299+
repoURL: fmt.Sprintf("https://matched/%s/%s", orgName, repoName),
300+
setupMux: func(mux *http.ServeMux, jwtToken string) {
301+
mux.HandleFunc(fmt.Sprintf("/repos/%s/%s/installation", orgName, repoName), func(w http.ResponseWriter, _ *http.Request) {
302+
_, _ = fmt.Fprintf(w, `{"id": %d}`, orgID)
303+
})
304+
mux.HandleFunc(fmt.Sprintf("/app/installations/%d/access_tokens", orgID), func(w http.ResponseWriter, r *http.Request) {
305+
testMethod(t, r)
306+
w.Header().Set("Authorization", "Bearer "+jwtToken)
307+
w.Header().Set("Accept", "application/vnd.github+json")
308+
_, _ = fmt.Fprintf(w, `{"token": "%s"}`, wantToken)
309+
})
310+
},
311+
wantErr: false,
312+
wantInstallationID: orgID,
313+
wantToken: wantToken,
314+
},
297315
{
298316
name: "repo installation fails, org installation succeeds",
299317
repoURL: fmt.Sprintf("https://matched/%s/%s", orgName, repoName),
@@ -357,12 +375,39 @@ func TestGetAndUpdateInstallationID_Fallbacks(t *testing.T) {
357375
expectedErrorString: "could not find repository, organization or user installation",
358376
},
359377
{
360-
name: "invalid repo url",
378+
name: "repo installation succeeds but token generation fails",
379+
repoURL: fmt.Sprintf("https://matched/%s/%s", orgName, repoName),
380+
setupMux: func(mux *http.ServeMux, jwtToken string) {
381+
mux.HandleFunc(fmt.Sprintf("/repos/%s/%s/installation", orgName, repoName), func(w http.ResponseWriter, _ *http.Request) {
382+
_, _ = fmt.Fprintf(w, `{"id": %d}`, orgID)
383+
})
384+
mux.HandleFunc(fmt.Sprintf("/app/installations/%d/access_tokens", orgID), func(w http.ResponseWriter, r *http.Request) {
385+
testMethod(t, r)
386+
w.Header().Set("Authorization", "Bearer "+jwtToken)
387+
w.Header().Set("Accept", "application/vnd.github+json")
388+
w.WriteHeader(http.StatusForbidden)
389+
_, _ = fmt.Fprintf(w, `{"message": "Forbidden"}`)
390+
})
391+
},
392+
wantErr: false, // Should not error, but return empty token
393+
wantInstallationID: orgID,
394+
wantToken: "", // Empty token when generation fails
395+
},
396+
{
397+
name: "invalid repo url - too few path segments",
361398
repoURL: "https://invalid/url",
362399
setupMux: func(_ *http.ServeMux, _ string) {
363400
},
364401
wantErr: true,
365-
expectedErrorString: "invalid repository URL path",
402+
expectedErrorString: "invalid repo url at least a organization/project and a repo needs to be specified",
403+
},
404+
{
405+
name: "invalid GitHub repo url - too many path segments",
406+
repoURL: "https://github.com/group/subgroup/repo",
407+
setupMux: func(_ *http.ServeMux, _ string) {
408+
},
409+
wantErr: true,
410+
expectedErrorString: "invalid GitHub repository URL path: expected '<organization>/<repository>', got: /group/subgroup/repo",
366411
},
367412
}
368413

@@ -393,7 +438,7 @@ func TestGetAndUpdateInstallationID_Fallbacks(t *testing.T) {
393438
ctx = info.StoreCurrentControllerName(ctx, "default")
394439
ctx = info.StoreNS(ctx, testNamespace.GetName())
395440

396-
ip := NewInstallation(httptest.NewRequest(http.MethodGet, "http://localhost", strings.NewReader("")), run, &v1alpha1.Repository{}, &github.Provider{}, testNamespace.GetName())
441+
ip := NewInstallation(httptest.NewRequest(http.MethodGet, "http://localhost", strings.NewReader("")), run, &v1alpha1.Repository{}, &github.Provider{}, testNamespace.GetName(), nil)
397442
jwtToken, err := ip.GenerateJWT(ctx)
398443
assert.NilError(t, err)
399444

@@ -412,7 +457,7 @@ func TestGetAndUpdateInstallationID_Fallbacks(t *testing.T) {
412457
gprovider.SetGithubClient(fakeghclient)
413458
t.Setenv("PAC_GIT_PROVIDER_TOKEN_APIURL", serverURL)
414459

415-
ip = NewInstallation(httptest.NewRequest(http.MethodGet, "http://localhost", strings.NewReader("")), run, repo, gprovider, testNamespace.GetName())
460+
ip = NewInstallation(httptest.NewRequest(http.MethodGet, "http://localhost", strings.NewReader("")), run, repo, gprovider, testNamespace.GetName(), nil)
416461
_, token, installationID, err := ip.GetAndUpdateInstallationID(ctx)
417462

418463
if tt.wantErr {

0 commit comments

Comments
 (0)