BeforeConnect Hook #1875

justcompile · 2021-04-20T19:34:44Z

As per discussions in #1848

This PR adds a new hook into Options to facilitate the use of secret providers / dynamic / one-time passwords. It has been added as BeforeConnect in order for the usage to be generic; whether it is for secret providers or simply logging when database connections are established.

Example

some error handling removed for readability

package main

imports (
    "context"
    "github.com/go-pg/pg/v10"

    "github.com/fake-user/supersecrets"
)

func main() {
    opts, _ := pg.ParseURL("postgresql://dbUser@dbhost/accounts")
   
    opts.BeforeConnect = func(ctx context.Context, o *pg.Options) error {
        token, err := supersecrets.GetToken("accounts-db-password")
        if err != nil {
            return err
        }

        o.Password = token

        return nil
    }

    db := pg.Connect(opts)

    // business logic
}

…like

.gitignore

vmihailenco · 2021-04-21T13:03:55Z

base.go

@@ -115,6 +115,12 @@ func (db *baseDB) initConn(ctx context.Context, cn *pool.Conn) error {
 		}
 	}

+	if db.opt.BeforeConnect != nil {
+		if err := db.opt.BeforeConnect(ctx, db.opt); err != nil {


I believe we should copy the options to avoid data race (since this hooks is called concurrently):

opt := db.opt.Clone() // use the cloned opt

and then reassign to db.opt?

if db.opt.BeforeConnect != nil { opts := db.opt.Clone() if err := opt.BeforeConnect(ctx, opt); err != nil { return err } db.opts = opts }

If you re assign to db.opt then you need to make that re assignment atomic (*thread safe?). What if this code is being executed in multiple go routines?

Also if we replace the db.opt here this can lead to a few interesting issues depending on how the user is using the library. What if they change the DB address entirely? what if they add TLS? Beyond simply changing the password or the user that a connection is issued with; changing other variables available in the options object could have a lot of unintended consequences that are currently unknown.

If at all possible we might just want to limit being able to change authentication parameters if we do update the original db.opts. Maybe limiting it to changing the user, password and TLS config.

If they for some reason changed something like the connection pool settings, how would that affect the pool that is already running?

@justcompile I don't think you need to replace db.opt. Just clone it (when needed for BeforeConnect) and use below in startup:

opt := db.opt.Clone() opt.BeforeConnect(ctx, opt) db.startup(ctx, cn, opt.User, opt.Password, opt.Database, opt.ApplicationName)

That will not cover all options but is good enough for this particular use case. We can improve it later if needed.

if db.opt.BeforeConnect != nil { db.opt.mux.Lock() updateOpts := &BeforeConnectOptions{ User: db.opt.User, Password: db.opt.Password, TLSConfig: db.opt.TLSConfig, } if err := db.opt.BeforeConnect(ctx, updateOpts); err != nil { db.opt.mux.Unlock() return err } db.opt.applyUpdatedConnectionOptions(updateOpts) db.opt.mux.Unlock() } // options.go func (opt *Options) applyUpdatedConnectionOptions(bcOpts *BeforeConnectOptions) { opt.User = bcOpts.User opt.Password = bcOpts.Password if bcOpts.TLSConfig != nil { opt.TLSConfig = bcOpts.TLSConfig.Clone() } }

Based on the concurrency issue mentioned & the points around unintended side effects of a user changing the Addr or Pool options, I was looking at something like the above.

But if that's overkill, I can go with the opts.Clone approach.

I'm also locking the mux here rather than within applyUpdatedConnectionOptions so that if multiple go routines are calling BeforeConnect it's a bit more deterministic

Let's keep as is BeforeConnect(ctx context.Context, o *pg.Options) error. It will not work for all options but that is fine. It is an advanced feature. Add add a warning to the comment.

elliotcourant · 2021-04-21T13:50:24Z

db_test.go

@@ -108,6 +108,30 @@ func TestDBConnectWithStartupNotice(t *testing.T) {
 	require.NoError(t, db.Ping(context.Background()), "must successfully ping database with long application name")
 }

+func TestBeforeConnect(t *testing.T) {


I would add another test that tests the before connect with multiple go routines to make sure that a race condition is not triggered by accident. A race condition in a before connection hook for an ORM could cripple an application.

I think a good way to do this would be to set the options to have a max pool of 1. And then try to send multiple queries concurrently in a few go routines.

This should trigger a concurrent call of the BeforeConnect method; and given the -race flag should be a sufficient enough smoke test for most use cases.

justcompile · 2021-04-21T14:30:39Z

Thanks for the feedback, I'll work the suggestions into this PR over the coming days

elliotcourant · 2021-09-28T19:11:30Z

@vmihailenco any way I'd be able to take this over? Still very interested in it

vmihailenco · 2021-09-29T07:00:56Z

@elliotcourant sure, go ahead 👍

justcompile · 2021-10-29T12:27:22Z

@vmihailenco sorry for the horrendous delay in this. Before I get around to resolve the testing points from @elliotcourant, could I confirm whether the current changes align with your expectations around cloning Options?

elliotcourant · 2021-10-29T16:23:55Z

@justcompile I will take a look at the diff again this weekend and double check it!

elliotcourant

Still looks good to me, added some more info about how the before connect thing could be tested.

elliotcourant · 2021-10-30T15:28:25Z

options.go

+		Network: opt.Network,
+		Addr: opt.Addr,
+		Dialer: opt.Dialer,
+		BeforeConnect: opt.BeforeConnect,
+		OnConnect: opt.OnConnect,
+		User: opt.User,
+		Password: opt.Password,
+		Database: opt.Database,
+		ApplicationName: opt.ApplicationName,
+		TLSConfig: opt.TLSConfig.Clone(),
+		DialTimeout: opt.DialTimeout,
+		ReadTimeout: opt.ReadTimeout,
+		WriteTimeout: opt.WriteTimeout,
+		MaxRetries: opt.MaxRetries,
+		RetryStatementTimeout: opt.RetryStatementTimeout,
+		MinRetryBackoff: opt.MinRetryBackoff,
+		MaxRetryBackoff: opt.MaxRetryBackoff,
+		PoolSize: opt.PoolSize,
+		MinIdleConns: opt.MinIdleConns,
+		MaxConnAge: opt.MaxConnAge,
+		PoolTimeout: opt.PoolTimeout,
+		IdleTimeout: opt.IdleTimeout,
+		IdleCheckFrequency: opt.IdleCheckFrequency,


Do these need to be go fmted?

elliotcourant · 2021-10-30T15:30:03Z

db_test.go

@@ -108,6 +108,30 @@ func TestDBConnectWithStartupNotice(t *testing.T) {
 	require.NoError(t, db.Ping(context.Background()), "must successfully ping database with long application name")
 }

+func TestBeforeConnect(t *testing.T) {


I think a good way to do this would be to set the options to have a max pool of 1. And then try to send multiple queries concurrently in a few go routines.

This should trigger a concurrent call of the BeforeConnect method; and given the -race flag should be a sufficient enough smoke test for most use cases.

added new hook to Options for catering for dynamic passwords and the …

68e8bb8

…like

vmihailenco reviewed Apr 21, 2021

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

vmihailenco reviewed Apr 21, 2021

View reviewed changes

removed .gitignore

7b867af

elliotcourant reviewed Apr 21, 2021

View reviewed changes

vmihailenco force-pushed the v10 branch 2 times, most recently from 99258be to 4d138f1 Compare October 25, 2021 07:29

justcompile and others added 4 commits October 29, 2021 12:58

Merge branch 'go-pg:v10' into v10

98f3ff4

implemented options clone approach

6186baf

Merge branch 'v10' of github.com:go-pg/pg into v10

22a4c8d

Merge branch 'v10' of github.com:justcompile/pg into v10

5a91137

elliotcourant requested changes Oct 30, 2021

View reviewed changes

vmihailenco force-pushed the v10 branch 3 times, most recently from 8925177 to 49ebb57 Compare April 5, 2022 06:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BeforeConnect Hook #1875

BeforeConnect Hook #1875

justcompile commented Apr 20, 2021

vmihailenco Apr 21, 2021 •

edited

Loading

justcompile Apr 21, 2021 •

edited

Loading

elliotcourant Apr 21, 2021 •

edited

Loading

elliotcourant Apr 21, 2021

vmihailenco Apr 23, 2021 •

edited

Loading

justcompile Apr 23, 2021 •

edited

Loading

vmihailenco Apr 23, 2021

elliotcourant Apr 21, 2021 •

edited

Loading

elliotcourant Oct 30, 2021

justcompile commented Apr 21, 2021

elliotcourant commented Sep 28, 2021

vmihailenco commented Sep 29, 2021

justcompile commented Oct 29, 2021

elliotcourant commented Oct 29, 2021

elliotcourant left a comment

elliotcourant Oct 30, 2021

elliotcourant Oct 30, 2021

BeforeConnect Hook #1875

Are you sure you want to change the base?

BeforeConnect Hook #1875

Conversation

justcompile commented Apr 20, 2021

Example

vmihailenco Apr 21, 2021 • edited Loading

Choose a reason for hiding this comment

justcompile Apr 21, 2021 • edited Loading

Choose a reason for hiding this comment

elliotcourant Apr 21, 2021 • edited Loading

Choose a reason for hiding this comment

elliotcourant Apr 21, 2021

Choose a reason for hiding this comment

vmihailenco Apr 23, 2021 • edited Loading

Choose a reason for hiding this comment

justcompile Apr 23, 2021 • edited Loading

Choose a reason for hiding this comment

vmihailenco Apr 23, 2021

Choose a reason for hiding this comment

elliotcourant Apr 21, 2021 • edited Loading

Choose a reason for hiding this comment

elliotcourant Oct 30, 2021

Choose a reason for hiding this comment

justcompile commented Apr 21, 2021

elliotcourant commented Sep 28, 2021

vmihailenco commented Sep 29, 2021

justcompile commented Oct 29, 2021

elliotcourant commented Oct 29, 2021

elliotcourant left a comment

Choose a reason for hiding this comment

elliotcourant Oct 30, 2021

Choose a reason for hiding this comment

elliotcourant Oct 30, 2021

Choose a reason for hiding this comment

vmihailenco Apr 21, 2021 •

edited

Loading

justcompile Apr 21, 2021 •

edited

Loading

elliotcourant Apr 21, 2021 •

edited

Loading

vmihailenco Apr 23, 2021 •

edited

Loading

justcompile Apr 23, 2021 •

edited

Loading

elliotcourant Apr 21, 2021 •

edited

Loading