Skip to content

fix: use authenticated urls for pubsub #14261

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

Merged
merged 25 commits into from
Aug 26, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 51 additions & 1 deletion coderd/database/awsiamrds/awsiamrds.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,21 @@ import (
"github.com/aws/aws-sdk-go-v2/aws"
"github.com/aws/aws-sdk-go-v2/config"
"github.com/aws/aws-sdk-go-v2/feature/rds/auth"
"github.com/lib/pq"
"golang.org/x/xerrors"

"github.com/coder/coder/v2/coderd/database"
)

type awsIamRdsDriver struct {
parent driver.Driver
cfg aws.Config
}

var _ driver.Driver = &awsIamRdsDriver{}
var (
_ driver.Driver = &awsIamRdsDriver{}
_ database.ConnectorCreator = &awsIamRdsDriver{}
)

// Register initializes and registers our aws iam rds wrapped database driver.
func Register(ctx context.Context, parentName string) (string, error) {
Expand Down Expand Up @@ -65,6 +71,16 @@ func (d *awsIamRdsDriver) Open(name string) (driver.Conn, error) {
return conn, nil
}

// Connector returns a driver.Connector that fetches a new authentication token for each connection.
func (d *awsIamRdsDriver) Connector(name string) (driver.Connector, error) {
connector := &connector{
url: name,
cfg: d.cfg,
}

return connector, nil
}

func getAuthenticatedURL(cfg aws.Config, dbURL string) (string, error) {
nURL, err := url.Parse(dbURL)
if err != nil {
Expand All @@ -82,3 +98,37 @@ func getAuthenticatedURL(cfg aws.Config, dbURL string) (string, error) {

return nURL.String(), nil
}

type connector struct {
url string
cfg aws.Config
dialer pq.Dialer
}

var _ database.DialerConnector = &connector{}

func (c *connector) Connect(ctx context.Context) (driver.Conn, error) {
nURL, err := getAuthenticatedURL(c.cfg, c.url)
if err != nil {
return nil, xerrors.Errorf("assigning authentication token to url: %w", err)
}

nc, err := pq.NewConnector(nURL)
if err != nil {
return nil, xerrors.Errorf("creating new connector: %w", err)
}

if c.dialer != nil {
nc.Dialer(c.dialer)
}

return nc.Connect(ctx)
}

func (*connector) Driver() driver.Driver {
return &pq.Driver{}
}

func (c *connector) Dialer(dialer pq.Dialer) {
c.dialer = dialer
}
28 changes: 25 additions & 3 deletions coderd/database/awsiamrds/awsiamrds_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,11 @@ import (

"github.com/stretchr/testify/require"

"cdr.dev/slog"
"cdr.dev/slog/sloggers/slogtest"

"github.com/coder/coder/v2/cli"
awsrdsiam "github.com/coder/coder/v2/coderd/database/awsiamrds"
"github.com/coder/coder/v2/coderd/database/awsiamrds"
"github.com/coder/coder/v2/coderd/database/pubsub"
"github.com/coder/coder/v2/testutil"
)

Expand All @@ -22,13 +23,15 @@ func TestDriver(t *testing.T) {
// export DBAWSIAMRDS_TEST_URL="postgres://user@host:5432/dbname";
url := os.Getenv("DBAWSIAMRDS_TEST_URL")
if url == "" {
t.Log("skipping test; no DBAWSIAMRDS_TEST_URL set")
t.Skip()
}

logger := slogtest.Make(t, nil).Leveled(slog.LevelDebug)
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort)
defer cancel()

sqlDriver, err := awsrdsiam.Register(ctx, "postgres")
sqlDriver, err := awsiamrds.Register(ctx, "postgres")
require.NoError(t, err)

db, err := cli.ConnectToPostgres(ctx, slogtest.Make(t, nil), sqlDriver, url)
Expand All @@ -47,4 +50,23 @@ func TestDriver(t *testing.T) {
var one int
require.NoError(t, i.Scan(&one))
require.Equal(t, 1, one)

ps, err := pubsub.New(ctx, logger, db, url)
require.NoError(t, err)

gotChan := make(chan struct{})
subCancel, err := ps.Subscribe("test", func(_ context.Context, _ []byte) {
close(gotChan)
})
defer subCancel()
require.NoError(t, err)

err = ps.Publish("test", []byte("hello"))
require.NoError(t, err)

select {
case <-gotChan:
case <-ctx.Done():
require.Fail(t, "timed out waiting for message")
}
}
19 changes: 19 additions & 0 deletions coderd/database/connector.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package database

import (
"context"
"database/sql/driver"

"github.com/lib/pq"
)

// ConnectorCreator can create a driver.Connector.
type ConnectorCreator interface {
Connector(name string) (driver.Connector, error)
}

// DialerConnector can create a driver.Connector and set a pq.Dialer.
type DialerConnector interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Connect(context.Context) (driver.Conn, error)
Dialer(dialer pq.Dialer)
}
36 changes: 31 additions & 5 deletions coderd/database/pubsub/pubsub.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package pubsub
import (
"context"
"database/sql"
"database/sql/driver"
"errors"
"io"
"net"
Expand All @@ -15,6 +16,8 @@ import (
"github.com/prometheus/client_golang/prometheus"
"golang.org/x/xerrors"

"github.com/coder/coder/v2/coderd/database"

"cdr.dev/slog"
)

Expand Down Expand Up @@ -432,9 +435,32 @@ func (p *PGPubsub) startListener(ctx context.Context, connectURL string) error {
// pq.defaultDialer uses a zero net.Dialer as well.
d: net.Dialer{},
}
connector driver.Connector
err error
)

// Create a custom connector if the database driver supports it.
connectorCreator, ok := p.db.Driver().(database.ConnectorCreator)
if ok {
connector, err = connectorCreator.Connector(connectURL)
Copy link
Contributor

Choose a reason for hiding this comment

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

This code doesn't get hit in the package unit tests. A good way to test it would be to create a pq Driver wrapper that we can control.

I'd like to see a test where we start pubsub with the wrapped driver, do some pub'ing and sub'ing, then kill the connection and verify that the pubsub / pq.Listener reconnects automatically. That would give a nice test of the pq changes you made as well.

if err != nil {
return xerrors.Errorf("create custom connector: %w", err)
}
} else {
// use the default pq connector otherwise
connector, err = pq.NewConnector(connectURL)
if err != nil {
return xerrors.Errorf("create pq connector: %w", err)
}
}

// Set the dialer if the connector supports it.
if dc, ok := connector.(database.DialerConnector); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to drop a CRITICAL log if this type assertion fails, since it means we've introduced a new driver that doesn't support a Dialer and our logs will be incomplete.

dc.Dialer(dialer)
}

p.pgListener = pqListenerShim{
Listener: pq.NewDialListener(dialer, connectURL, time.Second, time.Minute, func(t pq.ListenerEventType, err error) {
Listener: pq.NewConnectorListener(connector, connectURL, time.Second, time.Minute, func(t pq.ListenerEventType, err error) {
switch t {
case pq.ListenerEventConnected:
p.logger.Info(ctx, "pubsub connected to postgres")
Expand Down Expand Up @@ -583,8 +609,8 @@ func (p *PGPubsub) Collect(metrics chan<- prometheus.Metric) {
}

// New creates a new Pubsub implementation using a PostgreSQL connection.
func New(startCtx context.Context, logger slog.Logger, database *sql.DB, connectURL string) (*PGPubsub, error) {
p := newWithoutListener(logger, database)
func New(startCtx context.Context, logger slog.Logger, db *sql.DB, connectURL string) (*PGPubsub, error) {
p := newWithoutListener(logger, db)
if err := p.startListener(startCtx, connectURL); err != nil {
return nil, err
}
Expand All @@ -594,11 +620,11 @@ func New(startCtx context.Context, logger slog.Logger, database *sql.DB, connect
}

// newWithoutListener creates a new PGPubsub without creating the pqListener.
func newWithoutListener(logger slog.Logger, database *sql.DB) *PGPubsub {
func newWithoutListener(logger slog.Logger, db *sql.DB) *PGPubsub {
return &PGPubsub{
logger: logger,
listenDone: make(chan struct{}),
db: database,
db: db,
queues: make(map[string]map[uuid.UUID]*msgQueue),
latencyMeasurer: NewLatencyMeasurer(logger.Named("latency-measurer")),

Expand Down
2 changes: 1 addition & 1 deletion flake.nix
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@
name = "coder-${osArch}";
# Updated with ./scripts/update-flake.sh`.
# This should be updated whenever go.mod changes!
vendorHash = "sha256-AZ0qzh7H+UwnZNyg2iaNMSUWlGgomI/mo70T+FdF7ws=";
vendorHash = "sha256-0tsJY4/afWbsaFgq4j0F4kUSVXsWCQOwCk2VeU32vfM=";
proxyVendor = true;
src = ./.;
nativeBuildInputs = with pkgs; [ getopt openssl zstd ];
Expand Down
5 changes: 5 additions & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,11 @@ replace github.com/imulab/go-scim/pkg/v2 => github.com/coder/go-scim/pkg/v2 v2.0
// Fixes https://github.com/coder/coder/issues/6685
replace github.com/pkg/sftp => github.com/mafredri/sftp v1.13.6-0.20231212144145-8218e927edb0

// Adds support for a new Listener from a driver.Connector
// This lets us use rotating authentication tokens for passwords in connection strings
// which we use in the awsiamrds package.
replace github.com/lib/pq => github.com/coder/pq v1.10.5-0.20240813183442-0c420cb5a048

require (
cdr.dev/slog v1.6.2-0.20240126064726-20367d4aede6
cloud.google.com/go/compute/metadata v0.5.0
Expand Down
4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,8 @@ github.com/coder/go-httpstat v0.0.0-20230801153223-321c88088322 h1:m0lPZjlQ7vdVp
github.com/coder/go-httpstat v0.0.0-20230801153223-321c88088322/go.mod h1:rOLFDDVKVFiDqZFXoteXc97YXx7kFi9kYqR+2ETPkLQ=
github.com/coder/go-scim/pkg/v2 v2.0.0-20230221055123-1d63c1222136 h1:0RgB61LcNs24WOxc3PBvygSNTQurm0PYPujJjLLOzs0=
github.com/coder/go-scim/pkg/v2 v2.0.0-20230221055123-1d63c1222136/go.mod h1:VkD1P761nykiq75dz+4iFqIQIZka189tx1BQLOp0Skc=
github.com/coder/pq v1.10.5-0.20240813183442-0c420cb5a048 h1:3jzYUlGH7ZELIH4XggXhnTnP05FCYiAFeQpoN+gNR5I=
github.com/coder/pq v1.10.5-0.20240813183442-0c420cb5a048/go.mod h1:AlVN5x4E4T544tWzH6hKfbfQvm3HdbOxrmggDNAPY9o=
github.com/coder/pretty v0.0.0-20230908205945-e89ba86370e0 h1:3A0ES21Ke+FxEM8CXx9n47SZOKOpgSE1bbJzlE4qPVs=
github.com/coder/pretty v0.0.0-20230908205945-e89ba86370e0/go.mod h1:5UuS2Ts+nTToAMeOjNlnHFkPahrtDkmpydBen/3wgZc=
github.com/coder/quartz v0.1.0 h1:cLL+0g5l7xTf6ordRnUMMiZtRE8Sq5LxpghS63vEXrQ=
Expand Down Expand Up @@ -670,8 +672,6 @@ github.com/ledongthuc/pdf v0.0.0-20220302134840-0c2507a12d80/go.mod h1:imJHygn/1
github.com/leodido/go-urn v1.2.0/go.mod h1:+8+nEpDfqqsY+g338gtMEUOtuK+4dEMhiQEgxpxOKII=
github.com/leodido/go-urn v1.4.0 h1:WT9HwE9SGECu3lg4d/dIA+jxlljEa1/ffXKmRjqdmIQ=
github.com/leodido/go-urn v1.4.0/go.mod h1:bvxc+MVxLKB4z00jd1z+Dvzr47oO32F/QSNjSBOlFxI=
github.com/lib/pq v1.10.9 h1:YXG7RB+JIjhP29X+OtkiDnYaXQwpS4JEWq7dtCCRUEw=
github.com/lib/pq v1.10.9/go.mod h1:AlVN5x4E4T544tWzH6hKfbfQvm3HdbOxrmggDNAPY9o=
github.com/lucasb-eyer/go-colorful v1.2.0 h1:1nnpGOrhyZZuNyfu1QjKiUICQ74+3FNCN69Aj6K7nkY=
github.com/lucasb-eyer/go-colorful v1.2.0/go.mod h1:R4dSotOR9KMtayYi1e77YzuveK+i7ruzyGqttikkLy0=
github.com/mafredri/sftp v1.13.6-0.20231212144145-8218e927edb0 h1:lG2o/EWMEOlV/RfQrf3zYfQStjnUj0Mg2gmbcBcoxFI=
Expand Down
Loading