Skip to content

Commit

Permalink
Remove URL-escaping for coordinator password and username (#1546)
Browse files Browse the repository at this point in the history
## Description of changes

*Summarize the changes made by this PR.*
 - Improvements & Bug fixes
- Don't url-escape username and password. The library does this
automatically and query-escaping is a non-idempotent operation, so this
leads to errors

Originally we weren't running the coordinator using `/bin/sh` in
kubernetes, which defaults to using Linux's `exec` environment. Because
of this, the `$PASSWORD` flag wasn't being correctly populated -- it was
being passed in as the literal string `"$PASSWORD"`. The resulting
error, `invalid characters in username or password` (or something like
that) made me think we needed to query-escape the password when in
reality, the fix was to use `/bin/sh` to execute the coordinator command
and properly hydrate the `--password` flag. Now that we're using
`/bin/sh` and properly hydrating the `--password` flag, our
special-character containing password is going through two layers of
query escaping: once in our application logic and once in the underlying
postgres library. Query escaping is non-idempotent because of the `%`
character so this gives us authentication failures trying to connect to
our RDS instance.

## Test plan
*How are these changes tested?*

- [x] Built and deployed a Coordinator image with these changes. Spun it
up in staging, ssh'd in, manually started the coordinator, passing in
the raw un-escaped password. It worked.

## Documentation Changes
*Are all docstrings for user-facing APIs updated if required? Do we need
to make documentation changes in the [docs
repository](https://github.com/chroma-core/docs)?*
  • Loading branch information
beggers authored Dec 18, 2023
1 parent ae641fd commit 889569c
Showing 1 changed file with 2 additions and 3 deletions.
5 changes: 2 additions & 3 deletions go/coordinator/internal/metastore/db/dbcore/core.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package dbcore
import (
"context"
"fmt"
"net/url"
"reflect"

"github.com/chroma/chroma-coordinator/internal/common"
Expand Down Expand Up @@ -32,8 +31,8 @@ type DBConfig struct {
}

func Connect(cfg DBConfig) (*gorm.DB, error) {
dsn := fmt.Sprintf("host=%s user=%s password=%s dbname=%s port=%d",
cfg.Address, url.QueryEscape(cfg.Username), url.QueryEscape(cfg.Password), cfg.DBName, cfg.Port)
dsn := fmt.Sprintf("host=%s user=%s password=%s dbname=%s port=%d sslmode=require",
cfg.Address, cfg.Username, cfg.Password, cfg.DBName, cfg.Port)

ormLogger := logger.Default
ormLogger.LogMode(logger.Info)
Expand Down

0 comments on commit 889569c

Please sign in to comment.