-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Stop running prebuilds for inactive projects (10+ weeks) #9219
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
/** | ||
* Copyright (c) 2022 Gitpod GmbH. All rights reserved. | ||
* Licensed under the Gitpod Enterprise Source Code License, | ||
* See License.enterprise.txt in the project root folder. | ||
*/ | ||
|
||
import { Entity, Column, PrimaryColumn } from "typeorm"; | ||
|
||
import { TypeORM } from "../../typeorm/typeorm"; | ||
|
||
@Entity() | ||
// on DB but not Typeorm: @Index("ind_dbsync", ["_lastModified"]) // DBSync | ||
export class DBProjectUsage { | ||
@PrimaryColumn(TypeORM.UUID_COLUMN_TYPE) | ||
projectId: string; | ||
|
||
@Column("varchar") | ||
lastWebhookReceived: string; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this field strictly needed for our first version? We do set it, but we're not using it in read paths. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch, it's not strictly needed here indeed. However, since I modified very related code, I thought this small drive-by change could be a good first step toward fixing #7010 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll leave it up to you. For surfacing which webhook we've received, I'd expose it as a first class-table with webhooks (and webhook payloads), such that we can show what we've received in the webhook as well (would definitely be useful for our debugging). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea. I'll leave this as is, and we can decide later whether we want a full-fledged table of webhook events, or whether showing a simple timestamp of the last event is good enough. 🛹 |
||
|
||
@Column("varchar") | ||
lastWorkspaceStart: string; | ||
|
||
// This column triggers the db-sync deletion mechanism. It's not intended for public consumption. | ||
@Column() | ||
deleted: boolean; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
/** | ||
* Copyright (c) 2022 Gitpod GmbH. All rights reserved. | ||
* Licensed under the GNU Affero General Public License (AGPL). | ||
* See License-AGPL.txt in the project root for license information. | ||
*/ | ||
|
||
import { MigrationInterface, QueryRunner } from "typeorm"; | ||
|
||
export class ProjectUsage1649667202321 implements MigrationInterface { | ||
public async up(queryRunner: QueryRunner): Promise<void> { | ||
await queryRunner.query( | ||
"CREATE TABLE IF NOT EXISTS `d_b_project_usage` (`projectId` char(36) NOT NULL, `lastWebhookReceived` varchar(255) NOT NULL DEFAULT '', `lastWorkspaceStart` varchar(255) NOT NULL DEFAULT '', `deleted` tinyint(4) NOT NULL DEFAULT '0', `_lastModified` timestamp(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6) ON UPDATE CURRENT_TIMESTAMP(6), PRIMARY KEY (`projectId`), KEY `ind_dbsync` (`_lastModified`)) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4;", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here's we're setting a limit on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not too familiar with the intricacies of our ORM model, so I wouldn't know the pros and cons of doing this. Briefly looking through our code, it seems that we typically use |
||
); | ||
} | ||
|
||
public async down(queryRunner: QueryRunner): Promise<void> {} | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -116,6 +116,15 @@ export class GitHubEnterpriseApp { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
): Promise<StartPrebuildResult | undefined> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const span = TraceContext.startSpan("GitHubEnterpriseApp.handlePushHook", ctx); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const cloneURL = payload.repository.clone_url; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const projectAndOwner = await this.findProjectAndOwner(cloneURL, user); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (projectAndOwner.project) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At the point of this call, how do we know that the project specified in the payload in fact belongs to the correct user? Is it possible to poison our data across projects here? Mostly lacking context, so trying to understand better how the payload from the webhook is authhorized There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That's a good question. I don't know about webhook events from the GitHub App, but for GitLab, we look at the provided (user + project) token in order to make sure that the project specified in the payload actually belongs to the user who installed this webhook: gitpod/components/server/ee/src/prebuilds/gitlab-app.ts Lines 61 to 97 in e8fa6d2
We do something similar for GitHub Enterprise webhooks, where the secret token is not sent as part of the webhook payload, but is used to sign the webhook payload (which we can verify against the user's tokens): gitpod/components/server/ee/src/prebuilds/github-enterprise-app.ts Lines 89 to 106 in e8fa6d2
I don't think it is possible to send forged webhook events to Gitpod. However, your question does remind me of an issue where Pull Requests can come from forks, but trigger a Prebuild for the main repository, which can be problematic in some cases: https://github.com/gitpod-io/security/issues/26 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/* tslint:disable-next-line */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/** no await */ this.projectDB.updateProjectUsage(projectAndOwner.project.id, { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
lastWebhookReceived: new Date().toISOString(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const contextURL = this.createContextUrl(payload); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
span.setTag("contextURL", contextURL); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const context = (await this.contextParser.handle({ span }, user, contextURL)) as CommitContext; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -127,15 +136,13 @@ export class GitHubEnterpriseApp { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
log.debug("GitHub Enterprise push event: Starting prebuild.", { contextURL }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const cloneURL = payload.repository.clone_url; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const projectAndOwner = await this.findProjectAndOwner(cloneURL, user); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const commitInfo = await this.getCommitInfo(user, payload.repository.url, payload.after); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const ws = await this.prebuildManager.startPrebuild( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ span }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
context, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
user: projectAndOwner.user, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
project: projectAndOwner?.project, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
project: projectAndOwner.project, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
commitInfo, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's historically the reason for using varchar to store timestamps? Is it possible to use a DATETIME?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question -- I don't have a strong opinion here, and was just following established practice 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I ask is that when prototyping an ORM for Golang, Varchar timestamps make it much more complicated - this is further coupled with the somewhat non-standard usage of ISO8601 in javascript to represent a timestamp, which isn't quite RFC3339 compatible.
See https://github.com/gitpod-io/gitpod/pull/8967/files#diff-64ef712fa293d2472e903c3b14ffbaceb31a8b65b79e0f7a31316dbdcbe0c702R18
Definitely not trying to block this PR on this, mostly looking to understand historical reasoning for the varchar choice. Feel free to ignore this comment to land the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think your concerns are valid, and there is definitely room for improvement in our DB types. 👍
Thanks! I would love to, but first I need a review. 😇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still reviewing :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's awesome, thanks a bunch 🙏
I'll try to practice our new review process by assigning this Pull Request to you then (but please feel free to un-assign yourself again if that didn't make sense!)