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

Jump to bottom

[server] Read blocked repositories from database #11036

Merged
merged 8 commits into from
Jun 30, 2022
Merged

[server] Read blocked repositories from database #11036

merged 8 commits into from
Jun 30, 2022

Conversation

andrew-farries
Copy link
Contributor

Description

Add a d_b_blocked_repository table to the application database and use it at workspace startup to check whether a workspace should be blocked. Also checks if the user should be blocked for attempting to start such a workspace and blocks the user as necessary.

Related Issue(s)

Part of #11030

How to test

  • Get a kube context for the preview environment for this branch:

    previewctl install-context

  • Port forward to the database (username and password is in the server environment):

    kubectl port-forward svc/mysql 3306:3306

  • Using mysql client or mycli (needs brew install mycli first) add an entry to the d_b_blocked_repository table.

    insert into d_b_blocked_repository (`urlRegexp`, `blockUser`, `deleted`) values ("github.com/<your github username>", true, false)

  • Attempt to start a workspace from a github repository under your user.

  • Observe that the workspace fails to start.

  • Try to open another workspace and see that you are now blocked.

  • Unblock yourself by updating your entry in the d_b_users table:

    update d_b_user set blocked = 0 where id=<id field value for the row for your username>'

  • Experiment by inserting/deleting other combinations of urlRegexp and blockUser in the d_b_blocked_repository table.

Release Notes

NONE

Werft options

  • /werft with-preview

Andrew Farries added 4 commits June 30, 2022 11:06
Add new DB entity and supporting classes
37e14a2 
Add the new BlockedRepository entity and its interface and
implementation.
Add migration to add new entity
c0b556e 
This was auto-generated by running:

`docker run --rm --name some-mysql -e MYSQL_ROOT_PASSWORD=test -e MYSQL_DATABASE=gitpod -e MYSQL_USER=gitpod -e MYSQL_PASSWORD=test -p 3306:3306 -d mysql:5.7`

`yarn typeorm migration:generate -n New` from the `gitpod-db` directory.

And then removing everything but the new table.

We should document a better way of doing this.
Add unit tests for blocked repositories
8cc499e
Check blocked repos when starting a workspace
a89a501
@andrew-farries andrew-farries requested a review from a team June 30, 2022 11:06
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Jun 30, 2022
andrew-farries
andrew-farries commented Jun 30, 2022
View reviewed changes
components/gitpod-db/src/typeorm/entity/db-blocked-repository.ts Outdated
Comment on lines 30 to 34
// TODO: not sure if we need this?
// TODO: should have a default value of false?
// This column triggers the db-sync deletion mechanism. It's not intended for public consumption.
@Column()
deleted: boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As the comment suggests, I'm not sure if we need this field on the new table.

Copy link
Member

Choose a reason for hiding this comment

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

boolean is mapped tinyint, which defaults to '0'. That's why we do not have explicit defaults throughout the code base. 👍

TODO: not sure if we need this?

If we want to be able to remove this entry from the DB cross-region then yes, we should have it. 👍

Copy link
Member

Choose a reason for hiding this comment

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

@andrew-farries For this DB table to be synced we also need an entry in tables.ts. Happy to chat about how that needs to look like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added an entry to tables.ts in c4aebe3

@andrew-farries andrew-farries mentioned this pull request Jun 30, 2022
Closed
8 tasks
@easyCZ
Copy link
Member

easyCZ commented Jun 30, 2022

To be able to land this, we also need to udpate all the runbooks and point to how to establish a DB connection so that on-call engineers can block dynamically.

To make this less one-shot-change, can we first merge the config and the DB results when checking to enable gradual migration away? This would give us time to stage the change and communicate it.

@andrew-farries
Copy link
Contributor Author

To be able to land this, we also need to udpate all the runbooks and point to how to establish a DB connection so that on-call engineers can block dynamically.

My plan (as described in #11030) was to allow the two mechanisms (reading from db and from config) to exist in parallel until we add the API and UI to interact with blocked repositories in the db. At that point we copy the blocked repos from config into the db. Until that point the blocked repository db table remains empty.

This PR isn't intended to replace the mechanism we currently use to block repos, just be a step towards that. So we don't need to update any runbooks etc.

@easyCZ
Copy link
Member

easyCZ commented Jun 30, 2022

Nice, sorry didn't click through into the parent issue.

@geropl geropl self-assigned this Jun 30, 2022
@geropl
Copy link
Member

geropl commented Jun 30, 2022

Testing...

geropl
geropl approved these changes Jun 30, 2022
View reviewed changes
Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

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

Awesome, looking forward to have this! 🚀

/hold We should remove this unused DB field.

@andrew-farries
Copy link
Contributor Author

andrew-farries commented Jun 30, 2022 •
edited by werft-gitpod-dev-com bot

/werft run with-clean-slate-deployment=true

👍 started the job as gitpod-build-af-admin-block-users.36
(with .werft/ from main)

@andrew-farries
Copy link
Contributor Author

/unhold

@roboquat roboquat merged commit 1f9ed57 into main Jun 30, 2022
@roboquat roboquat deleted the af/admin-block-users branch June 30, 2022 15:53
@andrew-farries andrew-farries mentioned this pull request Jul 1, 2022
Merged
1 task
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geropl geropl geropl approved these changes

Assignees

@geropl geropl

Labels
deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note-none size/L team: webapp Issue belongs to the WebApp team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@andrew-farries @easyCZ @geropl @roboquat