Replace PostgresqlConnector with ConnectionStringProvider#18
Replace PostgresqlConnector with ConnectionStringProvider#18andrew-corbalt wants to merge 12 commits intomainfrom
Conversation
jonahb
left a comment
jonahb
left a comment
There was a problem hiding this comment.
This looks good. I like the new approach. I guess it'd also be worth thinking about whether we want to tweak things per Slack thread and how this change might fit into that.
pgutils/connector.go
Outdated
| // - assume_role_session_name: only used when assume_role_arn is set; defaults to "pgutils-rds-iam" if omitted. | ||
| // | ||
| // IAM example 2: postgres+rds-iam://<user>@<host>[:<port>]/<dbname>?assume_role_arn=...&assume_role_session_name=... | ||
| func NewPostgresqlConnectorFromDSN(ctx context.Context, dsn string) (*PostgresqlConnector, error) { |
There was a problem hiding this comment.
This function only accepts URLs (not any DSN / connection string), so I think we should make that clear, e.g.:
func NewPostgresqlConnectorFromURL(ctx context.Context, u *url.URL) (*PostgresqlConnector, error)
We could also provide a helper that parses the URL string (NewPostgresqlConnectorFromURLString).
pgutils/connector.go
Outdated
| cfg.AssumeRoleSessionName = q.Get("assume_role_session_name") | ||
| } | ||
|
|
||
| return NewPostgresqlConnectorWithIAMAuth(ctx, cfg) |
There was a problem hiding this comment.
This seems good while clients are changing to connection strings (DSNs), but per engineering sync, the plan is eventually to remove the IAM and connection string-specific constructors.
pgutils/connector.go
Outdated
| return db | ||
| } | ||
|
|
||
| func parseURL(rawURL string) (*url.URL, error) { |
There was a problem hiding this comment.
Does this wrapper do anything useful?
There was a problem hiding this comment.
Its only used in 2 places right now. We could just remove the function and do the parsing checks in each function. What do you think?
pgutils/pgutils.go
Outdated
| return str | ||
| } | ||
|
|
||
| func CensorDSN(dsn string) (string, error) { |
There was a problem hiding this comment.
For usability, what do you think about not returning an error (only string) and if the URL is unparseable you can just return "<cannot parse URL to censor>" or something like that? Then callers can avoid the hassle of checking the return value and can use this return value directly as an argument to a logging call. (In the extremely unlikely event that they pass an invalid URL, all that will happen is the logs will be less useful.) Maybe rename to CensorDSNForLogs to make clear that you should not try to use this string for anything else.
Thoughts?
There was a problem hiding this comment.
I almost did exactly that, but since the original version was already approved in a PR I decided to go that direction. Also I like the idea of renaming it to CensorDSNForLogs() which I think makes it much more appropriate to return an error string. Lets change to this suggestion!
andrew-corbalt
changed the title
hundt-corbalt
left a comment
hundt-corbalt
left a comment
There was a problem hiding this comment.
Code LGTM! But PR description still refers to an older version of the code. Can you update that?
done |
andrew-corbalt
dismissed
jonahb’s stale review
Code has been rewritten since the requested change was submitted. Feel free to re-review the new version that was made with Chris' input!
3 participants
PR Description
To simplify handling both RDS IAM auth and conventional password auth postgres connections, PostgresqlConnector is replaced with ConnectionStringProvider which adds the concept of a custom DSN
postgres+rds-iam://<user>@<host>[:<port>]/<dbname>for IAM auth. Alternatively, a normal postgres DSN can be used with conventional password auth.Tests:
Updated the ProfessorMAC app and microservices to use ConnectionStringProvider and verified function
PR Checklist
Examples:
To provide feedback on this template, visit https://docs.google.com/document/d/1YfTv7Amyop5G_8w1c2GJ_Mu-70L0KkZHhm9f9umDi3U/edit