-
Notifications
You must be signed in to change notification settings - Fork 0
Add NewPostgresqlConnectorFromDSN() to pgutils #18
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
base: main
Are you sure you want to change the base?
Conversation
jonahb
left a comment
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
PR Description
To simplify handling both RDS IAM auth and conventional password auth postgres connections, NewPostgresqlConnectorFromDSN() adds the concept of a custom DSN postgres+rds-iam://@[:]/, which will instruct the function to do down the IAM auth path in setting up a connector. Alternatively, if this function is passed a normal postgres DSN, then a conventional password auth is used.
Tests:
Updated the DSO Metrics Migration method to use NewPostgresqlConnectorFromDSN() and ran it against a test database using both conventional and custom DSN
PR Checklist
Examples:
To provide feedback on this template, visit https://docs.google.com/document/d/1YfTv7Amyop5G_8w1c2GJ_Mu-70L0KkZHhm9f9umDi3U/edit