Skip to content

[CMCSMACD-6133] Support search_path in postgres+rds-iam scheme#21

Merged
hundt-corbalt merged 2 commits into
mainfrom
hundt-search-path
Apr 20, 2026
Merged

[CMCSMACD-6133] Support search_path in postgres+rds-iam scheme#21
hundt-corbalt merged 2 commits into
mainfrom
hundt-search-path

Conversation

@hundt-corbalt
Copy link
Copy Markdown
Contributor

Also, use "config" param when adding a search path to a regular postgres DSN because that is supported by both psql and the Go pq library, whereas search_path is only supported by the latter.

Testing: successfully

  • Used cmd/rds-iam-to-dsn with a search path to generate a DSN with a token and a search path that is valid for use with psql
  • Used pgutils.WithSchemaSearchPath to add a search path and successfully connected with pgutils.MustConnectDB and pgutils.ToConnector and executed a query.

PR Checklist

  • New automated tests have been written to the extent possible.
  • The code has been checked for structural/syntactic validity.
    • AMI/application: a build was performed
    • terraform changes: "terraform plan" checked on every affected environment
  • (If applicable) the code has been manually tested on our infrastructure.
    • AMI/application: deployed an a test or dev environment
    • terraform changes: applied to test or dev environment
    • script: run against test or dev environment
  • Likely failure points and new functionality have been identified and tested manually.
    Examples:
    • Application manually run in a way that triggers any new branches
    • AMI logged into and changes verified from login shell
  • Pull request description includes a description of all the manual steps performed to accomplish the above.

To provide feedback on this template, visit https://docs.google.com/document/d/1YfTv7Amyop5G_8w1c2GJ_Mu-70L0KkZHhm9f9umDi3U/edit

Also, use "config" param when adding a search path to a regular
postgres DSN because that is supported by both psql and the Go
pq library, whereas search_path is only supported by the latter.
Copy link
Copy Markdown
Contributor

@andrew-corbalt andrew-corbalt left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Copy Markdown
Contributor

@leslie-corbalt leslie-corbalt left a comment

Choose a reason for hiding this comment

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

I had a few comments.

Comment thread pgutils/connector.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This comment is stale. Maybe:
// addSearchPathToURL returns a copy of rawURL with search_path set through
// the PostgreSQL options connection parameter. This produces a URL accepted by
// libpq/psql, unlike adding search_path as a direct query parameter.
// It returns an error if search_path is already present.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I updated the comment. What do you think?

Comment thread pgutils/connector.go Outdated
@@ -143,7 +143,10 @@ func addSearchPathToURL(rawURL string, searchPath string) (string, error) {
if v := q.Get("search_path"); v != "" {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If search_path is present we want to fail.
For ?search_path=, q.Get("search_path") returns an empty string, so the check does not fire.
After options is set to search_path, we could get something we don't want
postgres://user:pass@host/db?options=-csearch_path%3Dmy_schema&search_path=
Better to check for search_path using a map. Ditto for q.Get("options")
if _, ok := q["search_path"]; ok {
return "", fmt.Errorf("search_path already set")
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point! Done

Comment thread pgutils/connector.go Outdated
}, nil
}

if searchPath := q.Get("search_path"); searchPath != "" {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If search_path is present, require exactly one non-empty value before applying it WithSchemaSearchPath

values, ok := q["search_path"]
Then you can check:
if !ok {
// not configured
}
if len(values) != 1 || values[0] == "" {
// invalid
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Did something like this but didn't check against empty string. Don't see any reason we need to disallow it at this level.

Copy link
Copy Markdown
Contributor

@leslie-corbalt leslie-corbalt left a comment

Choose a reason for hiding this comment

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

LGTM

@hundt-corbalt hundt-corbalt merged commit f65664e into main Apr 20, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants