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.

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

[db] Make hand-written transformers more robust against odd DB values #6775

Merged
merged 1 commit into from
Nov 24, 2021

Conversation

geropl
Copy link
Member

@geropl geropl commented Nov 18, 2021

Description

Make hand-written transformers more robust against odd DB values

Related Issue(s)

Fixes #6642

How to test

  1. Add an entry to d_b_workspace_cluster and set admissionPreferences/admissionConstraints to odd values like: [, ] or NULL
    1. one terminal: kubectl port-forward mysl-0 3306
    2. another terminal: mysql -u gitpod -h 127.0.0.1 -p
    3. insert: INSERT INTO d_b_workspace_cluster SET name = "test", url = "https://example.dev", state = "available", score = 100, maxScore = 100, govern = TRUE, tls = "{", admissionConstraints = "[", admissionPreferences = "]";
  2. see that ws-manager-bridge logs errors (and failing re-tries), but you're still able to start a workspace
    1. grep for: error parsing value ws-cluster from DB

Release Notes

make DB layer more robust against odd DB values

Documentation

Copy link
Contributor

@jankeromnes jankeromnes left a comment

Choose a reason for hiding this comment

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

  1. Add an entry to d_b_workspace_cluster and set admissionPreferences/admissionConstraints to odd values like: [, ] or NULL

Sorry, I may have taken this a bit too far 😅

mysql> insert into d_b_workspace_cluster (name, url, tls, state, score, maxScore, govern, admissionConstraints, admissionPreferences) values ('hyper2', 'https://gitpod-staging.com', '-----BEGIN RSA PRIVATE KEY-----\n1234', 'available', 100, 100, 1, '"YOLO"', "3.1415926535");

No longer able to start workspaces:

Screenshot 2021-11-19 at 16 33 38

But I guess this is not what you had in mind right?

@@ -27,6 +27,7 @@ export class DBPrebuildInfo {
const obj = JSON.parse(value);
return PrebuildInfo.is(obj) ? obj : undefined;
} catch (error) {
return undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. Why was it necessary to make this explicit?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why was it necessary to make this explicit?

Not necessary (empty functions return undefined), did not even notice making this change. Just pleasing my sense of order I guess. 😉

Copy link
Contributor

@JanKoehnlein JanKoehnlein left a comment

Choose a reason for hiding this comment

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

Why wouldn't we harden and re-use Transformer#Simple_JSON? Unless there is something very specific about these cases, re-use seems to be the less error-prone fix to me here.

@geropl
Copy link
Member Author

geropl commented Nov 22, 2021

@JanKoehnlein The error here actually was triggered in simple-json, not our code. 🙂 Also, we have some requirements ("" | NULL => undefined) that is does not handle.

@geropl
Copy link
Member Author

geropl commented Nov 22, 2021

@jankeromnes That's an interesting case, albeit a different one: Injecting valid JSON of a different type.

IMO we should not take it too far here. The root cause here is that Transformer.simpleJSON does not handle unvalid values (NULL) well, and we experienced one outage for bc of this.

@JanKoehnlein
Copy link
Contributor

/lgtm

@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: 6156820224663b5063c160d9a221963e16ba2824

@roboquat
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JanKoehnlein

Associated issue: #6642

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@roboquat roboquat merged commit 3be9435 into main Nov 24, 2021
@roboquat roboquat deleted the gpl/6642-transformers branch November 24, 2021 12:57
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved deployed: webapp Meta team change is running in production deployed Change is completely running in production groundwork: in review release-note size/M team: webapp Issue belongs to the WebApp team
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

[db] Revise hand-written TypeORM transformers
4 participants