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

Bail if upstream URL is malformed #82

Open
amartin96 opened this issue Jun 27, 2023 · 7 comments
Open

Bail if upstream URL is malformed #82

amartin96 opened this issue Jun 27, 2023 · 7 comments
Assignees
Labels
first-issue Created by Linear-GitHub Sync High priority Created by Linear-GitHub Sync

Comments

@amartin96
Copy link
Contributor

In at least one case (postgres missing db name), a malformed upstream URL results in errors that could be cleaner (indefinitely repeating replicator errors).

We should make this case a permanent failure with a user-friendly message, and investigate/fix possible others.

From SyncLinear.com | REA-2917

@amartin96 amartin96 self-assigned this Jun 27, 2023
@amartin96 amartin96 added the High priority Created by Linear-GitHub Sync label Jun 27, 2023
@amartin96
Copy link
Contributor Author

Postgres can be configured to allow passwordless login as well. It does, however, always require a username, as far as I've read.

@amartin96 amartin96 added the first-issue Created by Linear-GitHub Sync label Jun 27, 2023
@amartin96
Copy link
Contributor Author

In MySQL, you can create a superuser with no username or password:

CREATE USER ''@'%' IDENTIFIED BY '';
GRANT ALL PRIVILEGES ON *.* TO ''@'%';

When such a user exists, ReadySet will connect and replicate from a URL of the form:

mysql://<host>:<port>/<db>

So we don't want to require username + password for MySQL deployments.

@amartin96
Copy link
Contributor Author

Postgres username missing:

Error { kind: Db, cause: Some(DbError { severity: "FATAL", parsed_severity: Some(Fatal), code: SqlState(E28000), message: "no PostgreSQL user name specified in startup packet", detail: None, hint: None, position: None, where_: None, schema: None, table: None, column: None, datatype: None, constraint: None, file: Some("postmaster.c"), line: Some(2268), routine: Some("ProcessStartupPacket") }) }

Postgres password missing:

Error { kind: Config, cause: Some("password missing") }

Postgres bad username/password:

Error { kind: Db, cause: Some(DbError { severity: "FATAL", parsed_severity: Some(Fatal), code: SqlState(E28P01), message: "password authentication failed for user \"postgress\"", detail: None, hint: None, position: None, where_: None, schema: None, table: None, column: None, datatype: None, constraint: None, file: Some("auth.c"), line: Some(335), routine: Some("auth_failed") }) }

Conclusions:

It might be possible to differentiate between recoverable and unrecoverable errors based on this information, but it would involve matching on strings and/or opaque error codes, which is brittle. We should just check for these fields manually.

@amartin96
Copy link
Contributor Author

Blocked because I think the most robust way to handle these errors is to treat them as unrecoverable where they are now, rather than add logic elsewhere to try and catch them early.

@amartin96
Copy link
Contributor Author

Omitting the username or password also leads to a replicator failure loop.

@amartin96
Copy link
Contributor Author

@amartin96
Copy link
Contributor Author

Some discussion on whether readyset might ever want to deal with postgres upstream URLs that don't contain a database name:

https://readyset-workspace.slack.com/archives/C01HF1WTP6Y/p1686944393015599

@zoready zoready changed the title [REA-2917] Bail if upstream URL is malformed Bail if upstream URL is malformed Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
first-issue Created by Linear-GitHub Sync High priority Created by Linear-GitHub Sync
Projects
None yet
Development

No branches or pull requests

1 participant