[server][github] fix selection of user for prebuilds #8132

AlexTugarev · 2022-02-10T08:48:58Z

Description

Related to #8093 this PR fixes a bug in the selection of the user account to ran a prebuild.

Before it would select any of the team owners.
Now it should first try with the GitHub App installer first as long as that user is still member of the team. Just as a fallback it might try to pick a team member with a github.com connection, perhaps this account has permissions to access the repository.

Related Issue(s)

Fixes #8128

Release Notes

[GitHub] Fix the user account picked for a prebuild.

codecov · 2022-02-10T08:49:58Z

Codecov Report

Merging #8132 (49758c6) into main (ededf2c) will decrease coverage by 17.09%.
The diff coverage is n/a.

❗ Current head 49758c6 differs from pull request most recent head c8ef534. Consider uploading reports for the commit c8ef534 to get more accurate results

@@             Coverage Diff             @@
##             main    #8132       +/-   ##
===========================================
- Coverage   27.92%   10.82%   -17.10%     
===========================================
  Files          41       18       -23     
  Lines        3510     1025     -2485     
===========================================
- Hits          980      111      -869     
+ Misses       2475      912     -1563     
+ Partials       55        2       -53

Flag	Coverage Δ
components-gitpod-cli-app	`10.82% <ø> (ø)`
components-local-app-app-darwin-amd64	`?`
components-local-app-app-darwin-arm64	`?`
components-local-app-app-linux-amd64	`?`
components-local-app-app-linux-arm64	`?`
components-local-app-app-windows-386	`?`
components-local-app-app-windows-amd64	`?`
components-local-app-app-windows-arm64	`?`
components-ws-proxy-app	`?`
installer-raw-app	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
components/ws-proxy/pkg/proxy/workspacerouter.go
components/ws-proxy/pkg/proxy/config.go
components/ws-proxy/pkg/proxy/routes.go
installer/pkg/components/ws-manager/rolebinding.go
installer/pkg/components/ws-manager/configmap.go
installer/pkg/components/ws-manager/tlssecret.go
components/local-app/pkg/auth/pkce.go
components/ws-proxy/pkg/proxy/cookies.go
...components/ws-manager/unpriviledged-rolebinding.go
installer/pkg/common/display.go
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ededf2c...c8ef534. Read the comment docs.

svenefftinge · 2022-02-10T12:51:33Z

components/server/ee/src/prebuilds/github-app.ts

-            const owner = installationId && (await this.findProjectOwner(cloneURL) || (await this.findInstallationOwner(installationId)));
-            if (!owner) {
-                log.info(`Did not find user for installation. Probably an incomplete app installation.`, { repo: ctx.payload.repository, installationId });
+            const installationOwner = installationId ? await this.findInstallationOwner(installationId) : undefined;


I looked at the DB and it seems like we don't have completed installations anymore with projects. So this would not return a user.

good point! checking...

There seem to be a couple of installations per day:

SELECT * FROM d_b_app_installation AS ai WHERE ai.state = 'installed' ORDER by ai.creationTime DESC LIMIT 200;

So I'd expect this to work as expected.

There are a lot of entry in claimed.platform state, which mark uncomplete registrations. I don't think that is problem; except that we might want to think hope to increase conversion (out of scope here).

The current code will happily do prebuilds for those incomplete registrations. With this change all of them will silently no longer see prebuilds.

@svenefftinge, thanks for revealing this! it's indeed a bigger issue. now that we're not encouraging users to install the Gitpod app from the GitHub marketplace, but rather go through /new there is need to adjust the "find app installer" logic.

I just pushed a change which alters findInstallation in a way that can be used as intended in this PR, i.e. it will return the latest recorded installation (with a state != uninstalled) which allows to find the installer of a particular app installation. given that we should be able to fix the wrong user account selection.

For completeness, that still isn't our final destination. We want to select any team {member, owner} with proper access to that repository to be able to ready repository details necessary to create a prebuild.

svenefftinge · 2022-02-10T12:53:47Z

The installations db is based on getting two way connection. One from GH one from gitpod and only if the two entries exist we will add a third 'installed' entry. It seems like with the new project flow we usually only get one of the signals but not the other.

geropl · 2022-02-10T13:25:10Z

components/server/ee/src/prebuilds/github-app.ts

+    /**
+     * Finds the relevant user account to create a prebuild with.
+     *
+     * First it tries to find the installer of the GitHub App installation


I agree with the logic here. But will double check with Svens comment above...

geropl · 2022-02-10T13:40:56Z

/lgtm
based on this discussion: #8132 (comment)
Approving to unblock.

/hold
Holding in case I'm missing something. 🙃

jldec · 2022-02-10T17:55:10Z

Thanks for working on this @AlexTugarev

My main worry with the implementation is that we are still failing silently when something is not right during the prebuild handler e.g. when fetchConfig throws for some reason. Do you think we could log such errors (in addition to tracing like we do now) and include the project/repo/branch/commit context of the push notification. Logging would allow us to capture metrics also.

For the future, we should also report webhook errors back to users e.g. via the dashboard or commit status.

jldec

I have tested this on a preview environment with the GitHub app installed.
Unfortunately the behavior described in #8128 is not fixed, in fact it seems worse.
Prebuilds now stop working as soon as another member who does not have repo access joins the team. The member does not have to be promoted to owner.

components/gitpod-db/src/typeorm/app-installation-db-impl.ts

geropl · 2022-02-11T13:04:57Z

components/gitpod-db/src/typeorm/app-installation-db-impl.ts

-            return finishedInstallation;
-        }
-
-        // maybe we need to finish an existing/ongoing installation


nit: I'd love to have some reference here as it's the only piece of code that makes the table schema understandable.
Alternatively, an issue for cleaning it/the whole registration process up would be nice.

geropl · 2022-02-11T13:07:48Z

@AlexTugarev New code LGTM 👍

Waiting for @jldec to approve as well.
Comments are nits.

jldec · 2022-02-11T13:57:20Z

I have verified that for projects under a team, adding additional team members or owners who do not have access to the project repo, does not block prebuilds for that repo as reported in #8128.

I see a new problem with prebuilds when the project is NOT under a team, but lives under a personal account. In that case no prebuilds happen for projects for repos from an org. (Personal repos are ok). I verified that the app was installed on the org in question.

jldec

see comment - doesn't work for projects under personal accounts.

components/server/ee/src/prebuilds/github-app.ts

jldec

lgtm

to return any non-uninstalled record which may now be used to identify the installer of an GitHub App installation.

AlexTugarev · 2022-02-11T16:42:46Z

Just squashed the commits and need a new approval, please!

jldec · 2022-02-11T16:49:08Z

/unhold

AlexTugarev requested review from jankeromnes, jldec, geropl and a team February 10, 2022 08:48

roboquat added the release-note label Feb 10, 2022

github-actions bot added the team: webapp Issue belongs to the WebApp team label Feb 10, 2022

roboquat added the size/M label Feb 10, 2022

github-actions bot removed the size/M label Feb 10, 2022

svenefftinge reviewed Feb 10, 2022

View reviewed changes

geropl reviewed Feb 10, 2022

View reviewed changes

roboquat added the do-not-merge/hold label Feb 10, 2022

jldec suggested changes Feb 10, 2022

View reviewed changes

roboquat added the size/L label Feb 11, 2022

AlexTugarev requested a review from jldec February 11, 2022 12:43

geropl reviewed Feb 11, 2022

View reviewed changes

components/gitpod-db/src/typeorm/app-installation-db-impl.ts Show resolved Hide resolved

geropl reviewed Feb 11, 2022

View reviewed changes

jldec suggested changes Feb 11, 2022

View reviewed changes

components/server/ee/src/prebuilds/github-app.ts Show resolved Hide resolved

jldec previously approved these changes Feb 11, 2022

View reviewed changes

AlexTugarev added 2 commits February 11, 2022 16:41

[server][github] fix selection of user for prebuilds

1863b69

[github-app] change findInstallation semantics

c8ef534

to return any non-uninstalled record which may now be used to identify the installer of an GitHub App installation.

AlexTugarev dismissed jldec’s stale review via c8ef534 February 11, 2022 16:41

AlexTugarev force-pushed the at/gh-prebuilds branch from 49758c6 to c8ef534 Compare February 11, 2022 16:41

jldec approved these changes Feb 11, 2022

View reviewed changes

roboquat removed the do-not-merge/hold label Feb 11, 2022

roboquat merged commit ebf351e into main Feb 11, 2022

roboquat deleted the at/gh-prebuilds branch February 11, 2022 16:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[server][github] fix selection of user for prebuilds #8132

[server][github] fix selection of user for prebuilds #8132

AlexTugarev commented Feb 10, 2022

codecov bot commented Feb 10, 2022 •

edited

Loading

svenefftinge Feb 10, 2022

AlexTugarev Feb 10, 2022

geropl Feb 10, 2022

geropl Feb 10, 2022

svenefftinge Feb 10, 2022

AlexTugarev Feb 11, 2022

AlexTugarev Feb 11, 2022

svenefftinge commented Feb 10, 2022

geropl Feb 10, 2022

geropl commented Feb 10, 2022

jldec commented Feb 10, 2022

jldec left a comment

geropl Feb 11, 2022

AlexTugarev Feb 11, 2022

geropl commented Feb 11, 2022

jldec commented Feb 11, 2022

jldec left a comment •

edited

Loading

jldec left a comment

AlexTugarev commented Feb 11, 2022

jldec commented Feb 11, 2022

[server][github] fix selection of user for prebuilds #8132

[server][github] fix selection of user for prebuilds #8132

Conversation

AlexTugarev commented Feb 10, 2022

Description

Related Issue(s)

Release Notes

codecov bot commented Feb 10, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svenefftinge commented Feb 10, 2022

Choose a reason for hiding this comment

geropl commented Feb 10, 2022

jldec commented Feb 10, 2022

jldec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geropl commented Feb 11, 2022

jldec commented Feb 11, 2022

jldec left a comment • edited Loading

Choose a reason for hiding this comment

jldec left a comment

Choose a reason for hiding this comment

AlexTugarev commented Feb 11, 2022

jldec commented Feb 11, 2022

codecov bot commented Feb 10, 2022 •

edited

Loading

jldec left a comment •

edited

Loading