Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[server][github] record "pull" webhook events #13292

Merged
merged 1 commit into from
Sep 26, 2022
Merged

[server][github] record "pull" webhook events #13292

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 43 additions & 9 deletions components/server/ee/src/prebuilds/github-app.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,12 +181,17 @@ export class GithubApp {
options.getRouter("/reconfigure").get(
"/",
asyncHandler(async (req: express.Request, res: express.Response) => {
const gh = await app.auth();
const data = await gh.apps.getAuthenticated();
const slug = data.data.slug;

const state = req.query.state;
res.redirect(`https://github.com/apps/${slug}/installations/new?state=${state}`);
try {
const gh = await app.auth();
const data = await gh.apps.getAuthenticated();
const slug = data.data.slug;

const state = req.query.state;
res.redirect(`https://github.com/apps/${slug}/installations/new?state=${state}`);
} catch (error) {
console.error(error, { error });
res.status(500).send("GitHub App is not configured.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are all errors 500 errors? Can it happen that const data = await gh.apps.getAuthenticated(); gives us a Not Found?

I'm trying to understand if all errors are in fact Server Errors, or if there are modes which fail due to user errors etc. Always returning a 500 has the obvious impact to our SLOs and may not always communicate the underlying problem to the user well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can it happen that const data = await gh.apps.getAuthenticated(); gives us a Not Found?

Don't think so. If auth succeed, it should be an in scope of an authenticated app, thus getAuthenticated should return its details.
https://docs.github.com/en/rest/apps/apps#get-the-authenticated-app

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to understand if all errors are in fact Server Errors

👍🏻
In that case we're navigating to /reconfigure, but there is an issue with GH App settings. Example for this is in preview enviroments, or self-hosted installations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about if auth doesn't succeed? Does it still make sense to return a 500, even if the reason is that the user doesn't exist or it has an invalid access token?

I'd expect here that we only return a 500 if we fail for a reason that's not expected - Users not being found or not existing are expected reasons.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, there was a GH issue to do exactly this. I've hard times to find it, though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about if auth doesn't succeed? Does it still make sense to return a 500, even if the reason is that the user doesn't exist or it has an invalid access token?

@easyCZ, I see. app.auth is the request to use the configured GH App secrets to do the app authentication. Here it's the GH App of Gitpod which needs to do the handshake, in order to be auth'd for operations. That's not related to users. More on this: https://docs.github.com/en/developers/apps/building-github-apps/authenticating-with-github-apps#authenticating-as-a-github-app

If app authentication is failing, reason is most likely the app secrets aren't set up properly, or the app is not configured. Otherwise this endpoint should not be navigable from UI.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To sum up: the code 500 isn't new here.
The effect of this change:

  1. readable error message to users, instead of error stack trace
  2. logging of the actual error/reason in logs, instead of swallowing it

}
}),
);
options.getRouter &&
Expand Down Expand Up @@ -398,6 +403,12 @@ export class GithubApp {
const span = TraceContext.startSpan("GithubApp.handlePullRequest", {});
span.setTag("request", ctx.id);

const event = await this.webhookEvents.createEvent({
type: ctx.name,
status: "received",
rawEvent: JSON.stringify(ctx.payload),
});

try {
const installationId = ctx.payload.installation?.id;
const cloneURL = ctx.payload.repository.clone_url;
Expand All @@ -423,14 +434,37 @@ export class GithubApp {
user = r.user;
project = r.project;

const prebuildStartPromise = await this.onPrStartPrebuild({ span }, ctx, config, context, user, project);
if (prebuildStartPromise) {
await this.onPrAddCheck({ span }, config, ctx, prebuildStartPromise);
await this.webhookEvents.updateEvent(event.id, {
authorizedUserId: user.id,
projectId: project?.id,
cloneUrl: context.repository.cloneUrl,
branch: context.ref,
commit: context.revision,
});

const prebuildStartResult = await this.onPrStartPrebuild({ span }, ctx, config, context, user, project);
if (prebuildStartResult) {
await this.webhookEvents.updateEvent(event.id, {
prebuildStatus: "prebuild_triggered",
status: "processed",
prebuildId: prebuildStartResult.prebuildId,
});

await this.onPrAddCheck({ span }, config, ctx, prebuildStartResult);
this.onPrAddBadge(config, ctx);
await this.onPrAddComment(config, ctx);
} else {
await this.webhookEvents.updateEvent(event.id, {
prebuildStatus: "ignored_unconfigured",
status: "processed",
});
}
} catch (e) {
TraceContext.setError({ span }, e);
await this.webhookEvents.updateEvent(event.id, {
prebuildStatus: "prebuild_trigger_failed",
status: "processed",
easyCZ marked this conversation as resolved.
Show resolved Hide resolved
});
throw e;
} finally {
span.finish();
Expand Down