luxolus
/
open-vault
2
0
Fork 0
Code Issues 1 Pull Requests Releases Activity

VAULT-5827 Don't prepare SQL queries before executing them (#15166) 

VAULT-5827 Don't prepare SQL queries before executing them

We don't support proper prepared statements, i.e., preparing once and
executing many times since we do our own templating. So preparing our
queries does not really accomplish anything, and can have severe
performance impacts (see
https://github.com/hashicorp/vault-plugin-database-snowflake/issues/13
for example).

This behavior seems to have been copy-pasted for many years but not for
any particular reason that we have been able to find. First use was in
https://github.com/hashicorp/vault/pull/15

So here we switch to new methods suffixed with `Direct` to indicate
that they don't `Prepare` before running `Exec`, and switch everything
here to use those. We maintain the older methods with the existing
behavior (with `Prepare`) for backwards compatibility.
This commit is contained in:
Christopher Swenson 2022-04-26 12:47:06 -07:00 committed by GitHub
parent 9eaea7bc14
commit aa6d61477e
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
14 changed files with 56 additions and 30 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
builtin/logical/database/rotation_test.go
View File

@ -444,7 +444,7 @@ func createTestPGUser(t *testing.T, connURL string, username, password, query st
"name": username, "name": username,
"password": password, "password": password,
} }
if err := dbtxn.ExecuteTxQuery(ctx, tx, m, query); err != nil { if err := dbtxn.ExecuteTxQueryDirect(ctx, tx, m, query); err != nil {
t.Fatal(err) t.Fatal(err)
} }
// Commit the transaction // Commit the transaction

2
builtin/logical/mssql/path_creds_create.go
View File

@ -95,7 +95,7 @@ func (b *backend) pathCredsCreateRead(ctx context.Context, req *logical.Request,
"name": username, "name": username,
"password": password, "password": password,
} }
if err := dbtxn.ExecuteTxQuery(ctx, tx, m, query); err != nil { if err := dbtxn.ExecuteTxQueryDirect(ctx, tx, m, query); err != nil {
return nil, err return nil, err
} }
} }

2
builtin/logical/mssql/secret_creds.go
View File

@ -131,7 +131,7 @@ func (b *backend) secretCredsRevoke(ctx context.Context, req *logical.Request, d
// many permissions as possible right now // many permissions as possible right now
var lastStmtError error var lastStmtError error
for _, query := range revokeStmts { for _, query := range revokeStmts {
if err := dbtxn.ExecuteDBQuery(ctx, db, nil, query); err != nil { if err := dbtxn.ExecuteDBQueryDirect(ctx, db, nil, query); err != nil {
lastStmtError = err lastStmtError = err
continue continue
} }

2
builtin/logical/mysql/path_role_create.go
View File

@ -108,7 +108,7 @@ func (b *backend) pathRoleCreateRead(ctx context.Context, req *logical.Request,
"name": username, "name": username,
"password": password, "password": password,
} }
if err := dbtxn.ExecuteTxQuery(ctx, tx, m, query); err != nil { if err := dbtxn.ExecuteTxQueryDirect(ctx, tx, m, query); err != nil {
return nil, err return nil, err
} }
} }

2
builtin/logical/postgresql/path_role_create.go
View File

@ -113,7 +113,7 @@ func (b *backend) pathRoleCreateRead(ctx context.Context, req *logical.Request,
"expiration": expiration, "expiration": expiration,
} }
if err := dbtxn.ExecuteTxQuery(ctx, tx, m, query); err != nil { if err := dbtxn.ExecuteTxQueryDirect(ctx, tx, m, query); err != nil {
return nil, err return nil, err
} }
} }

4
builtin/logical/postgresql/secret_creds.go
View File

@ -211,7 +211,7 @@ func (b *backend) secretCredsRevoke(ctx context.Context, req *logical.Request, d
// many permissions as possible right now // many permissions as possible right now
var lastStmtError error var lastStmtError error
for _, query := range revocationStmts { for _, query := range revocationStmts {
if err := dbtxn.ExecuteDBQuery(ctx, db, nil, query); err != nil { if err := dbtxn.ExecuteDBQueryDirect(ctx, db, nil, query); err != nil {
lastStmtError = err lastStmtError = err
} }
} }
@ -254,7 +254,7 @@ func (b *backend) secretCredsRevoke(ctx context.Context, req *logical.Request, d
m := map[string]string{ m := map[string]string{
"name": username, "name": username,
} }
if err := dbtxn.ExecuteTxQuery(ctx, tx, m, query); err != nil { if err := dbtxn.ExecuteTxQueryDirect(ctx, tx, m, query); err != nil {
return nil, err return nil, err
} }
} }

3
changelog/15166.txt Normal file
View File

@ -0,0 +1,3 @@
```release-note:improvement
core: Add new DB methods that do not prepare statements.
```

8
plugins/database/hana/hana.go
View File

@ -134,7 +134,7 @@ func (h *HANA) NewUser(ctx context.Context, req dbplugin.NewUserRequest) (respon
"expiration": expirationStr, "expiration": expirationStr,
} }
if err := dbtxn.ExecuteTxQuery(ctx, tx, m, query); err != nil { if err := dbtxn.ExecuteTxQueryDirect(ctx, tx, m, query); err != nil {
return dbplugin.NewUserResponse{}, err return dbplugin.NewUserResponse{}, err
} }
} }
@ -223,7 +223,7 @@ func (h *HANA) updateUserPassword(ctx context.Context, tx *sql.Tx, username stri
"password": password, "password": password,
} }
if err := dbtxn.ExecuteTxQuery(ctx, tx, m, query); err != nil { if err := dbtxn.ExecuteTxQueryDirect(ctx, tx, m, query); err != nil {
return fmt.Errorf("failed to execute query: %w", err) return fmt.Errorf("failed to execute query: %w", err)
} }
} }
@ -259,7 +259,7 @@ func (h *HANA) updateUserExpiration(ctx context.Context, tx *sql.Tx, username st
"expiration": expirationStr, "expiration": expirationStr,
} }
if err := dbtxn.ExecuteTxQuery(ctx, tx, m, query); err != nil { if err := dbtxn.ExecuteTxQueryDirect(ctx, tx, m, query); err != nil {
return fmt.Errorf("failed to execute query: %w", err) return fmt.Errorf("failed to execute query: %w", err)
} }
} }
@ -302,7 +302,7 @@ func (h *HANA) DeleteUser(ctx context.Context, req dbplugin.DeleteUserRequest) (
m := map[string]string{ m := map[string]string{
"name": req.Username, "name": req.Username,
} }
if err := dbtxn.ExecuteTxQuery(ctx, tx, m, query); err != nil { if err := dbtxn.ExecuteTxQueryDirect(ctx, tx, m, query); err != nil {
return dbplugin.DeleteUserResponse{}, err return dbplugin.DeleteUserResponse{}, err
} }
} }

8
plugins/database/mssql/mssql.go
View File

@ -154,7 +154,7 @@ func (m *MSSQL) NewUser(ctx context.Context, req dbplugin.NewUserRequest) (dbplu
"expiration": expirationStr, "expiration": expirationStr,
} }
if err := dbtxn.ExecuteTxQuery(ctx, tx, m, query); err != nil { if err := dbtxn.ExecuteTxQueryDirect(ctx, tx, m, query); err != nil {
return dbplugin.NewUserResponse{}, err return dbplugin.NewUserResponse{}, err
} }
} }
@ -198,7 +198,7 @@ func (m *MSSQL) DeleteUser(ctx context.Context, req dbplugin.DeleteUserRequest)
m := map[string]string{ m := map[string]string{
"name": req.Username, "name": req.Username,
} }
if err := dbtxn.ExecuteDBQuery(ctx, db, m, query); err != nil { if err := dbtxn.ExecuteDBQueryDirect(ctx, db, m, query); err != nil {
merr = multierror.Append(merr, err) merr = multierror.Append(merr, err)
} }
} }
@ -303,7 +303,7 @@ func (m *MSSQL) revokeUserDefault(ctx context.Context, username string) error {
// many permissions as possible right now // many permissions as possible right now
var lastStmtError error var lastStmtError error
for _, query := range revokeStmts { for _, query := range revokeStmts {
if err := dbtxn.ExecuteDBQuery(ctx, db, nil, query); err != nil { if err := dbtxn.ExecuteDBQueryDirect(ctx, db, nil, query); err != nil {
lastStmtError = err lastStmtError = err
} }
} }
@ -394,7 +394,7 @@ func (m *MSSQL) updateUserPass(ctx context.Context, username string, changePass
"username": username, "username": username,
"password": password, "password": password,
} }
if err := dbtxn.ExecuteTxQuery(ctx, tx, m, query); err != nil { if err := dbtxn.ExecuteTxQueryDirect(ctx, tx, m, query); err != nil {
return fmt.Errorf("failed to execute query: %w", err) return fmt.Errorf("failed to execute query: %w", err)
} }
} }

2
plugins/database/mssql/mssql_test.go
View File

@ -552,7 +552,7 @@ func createTestMSSQLUser(connURL string, username, password, query string) error
"name": username, "name": username,
"password": password, "password": password,
} }
if err := dbtxn.ExecuteTxQuery(ctx, tx, m, query); err != nil { if err := dbtxn.ExecuteTxQueryDirect(ctx, tx, m, query); err != nil {
return err return err
} }
// Commit the transaction // Commit the transaction

12
plugins/database/postgresql/postgresql.go
View File

@ -180,7 +180,7 @@ func (p *PostgreSQL) changeUserPassword(ctx context.Context, username string, ch
"username": username, "username": username,
"password": password, "password": password,
} }
if err := dbtxn.ExecuteTxQuery(ctx, tx, m, query); err != nil { if err := dbtxn.ExecuteTxQueryDirect(ctx, tx, m, query); err != nil {
return fmt.Errorf("failed to execute query: %w", err) return fmt.Errorf("failed to execute query: %w", err)
} }
} }
@ -229,7 +229,7 @@ func (p *PostgreSQL) changeUserExpiration(ctx context.Context, username string,
"username": username, "username": username,
"expiration": expirationStr, "expiration": expirationStr,
} }
if err := dbtxn.ExecuteTxQuery(ctx, tx, m, query); err != nil { if err := dbtxn.ExecuteTxQueryDirect(ctx, tx, m, query); err != nil {
return err return err
} }
} }
@ -273,7 +273,7 @@ func (p *PostgreSQL) NewUser(ctx context.Context, req dbplugin.NewUserRequest) (
"password": req.Password, "password": req.Password,
"expiration": expirationStr, "expiration": expirationStr,
} }
if err := dbtxn.ExecuteTxQuery(ctx, tx, m, stmt); err != nil { if err := dbtxn.ExecuteTxQueryDirect(ctx, tx, m, stmt); err != nil {
return dbplugin.NewUserResponse{}, fmt.Errorf("failed to execute query: %w", err) return dbplugin.NewUserResponse{}, fmt.Errorf("failed to execute query: %w", err)
} }
continue continue
@ -291,7 +291,7 @@ func (p *PostgreSQL) NewUser(ctx context.Context, req dbplugin.NewUserRequest) (
"password": req.Password, "password": req.Password,
"expiration": expirationStr, "expiration": expirationStr,
} }
if err := dbtxn.ExecuteTxQuery(ctx, tx, m, query); err != nil { if err := dbtxn.ExecuteTxQueryDirect(ctx, tx, m, query); err != nil {
return dbplugin.NewUserResponse{}, fmt.Errorf("failed to execute query: %w", err) return dbplugin.NewUserResponse{}, fmt.Errorf("failed to execute query: %w", err)
} }
} }
@ -343,7 +343,7 @@ func (p *PostgreSQL) customDeleteUser(ctx context.Context, username string, revo
"name": username, "name": username,
"username": username, "username": username,
} }
if err := dbtxn.ExecuteTxQuery(ctx, tx, m, query); err != nil { if err := dbtxn.ExecuteTxQueryDirect(ctx, tx, m, query); err != nil {
return err return err
} }
} }
@ -436,7 +436,7 @@ func (p *PostgreSQL) defaultDeleteUser(ctx context.Context, username string) err
// many permissions as possible right now // many permissions as possible right now
var lastStmtError error var lastStmtError error
for _, query := range revocationStmts { for _, query := range revocationStmts {
if err := dbtxn.ExecuteDBQuery(ctx, db, nil, query); err != nil { if err := dbtxn.ExecuteDBQueryDirect(ctx, db, nil, query); err != nil {
lastStmtError = err lastStmtError = err
} }
} }

10
plugins/database/redshift/redshift.go
View File

@ -164,7 +164,7 @@ func (r *RedShift) NewUser(ctx context.Context, req dbplugin.NewUserRequest) (db
"password": password, "password": password,
"expiration": expirationStr, "expiration": expirationStr,
} }
if err := dbtxn.ExecuteTxQuery(ctx, tx, m, query); err != nil { if err := dbtxn.ExecuteTxQueryDirect(ctx, tx, m, query); err != nil {
return dbplugin.NewUserResponse{}, err return dbplugin.NewUserResponse{}, err
} }
} }
@ -246,7 +246,7 @@ func updateUserExpiration(ctx context.Context, req dbplugin.UpdateUserRequest, t
"username": req.Username, "username": req.Username,
"expiration": expirationStr, "expiration": expirationStr,
} }
if err := dbtxn.ExecuteTxQuery(ctx, tx, m, query); err != nil { if err := dbtxn.ExecuteTxQueryDirect(ctx, tx, m, query); err != nil {
return err return err
} }
} }
@ -293,7 +293,7 @@ func updateUserPassword(ctx context.Context, req dbplugin.UpdateUserRequest, tx
"username": username, "username": username,
"password": password, "password": password,
} }
if err := dbtxn.ExecuteTxQuery(ctx, tx, m, query); err != nil { if err := dbtxn.ExecuteTxQueryDirect(ctx, tx, m, query); err != nil {
return err return err
} }
} }
@ -341,7 +341,7 @@ func (r *RedShift) customDeleteUser(ctx context.Context, req dbplugin.DeleteUser
"name": req.Username, "name": req.Username,
"username": req.Username, "username": req.Username,
} }
if err := dbtxn.ExecuteTxQuery(ctx, tx, m, query); err != nil { if err := dbtxn.ExecuteTxQueryDirect(ctx, tx, m, query); err != nil {
return dbplugin.DeleteUserResponse{}, err return dbplugin.DeleteUserResponse{}, err
} }
} }
@ -452,7 +452,7 @@ $$;`)
// many permissions as possible right now // many permissions as possible right now
var lastStmtError *multierror.Error // error var lastStmtError *multierror.Error // error
for _, query := range revocationStmts { for _, query := range revocationStmts {
if err := dbtxn.ExecuteDBQuery(ctx, db, nil, query); err != nil { if err := dbtxn.ExecuteDBQueryDirect(ctx, db, nil, query); err != nil {
lastStmtError = multierror.Append(lastStmtError, err) lastStmtError = multierror.Append(lastStmtError, err)
} }
} }

2
plugins/database/redshift/redshift_test.go
View File

@ -537,7 +537,7 @@ func createTestPGUser(t *testing.T, connURL string, username, password, query st
"name": username, "name": username,
"password": password, "password": password,
} }
if err := dbtxn.ExecuteTxQuery(ctx, tx, m, query); err != nil { if err := dbtxn.ExecuteTxQueryDirect(ctx, tx, m, query); err != nil {
t.Fatal(err) t.Fatal(err)
} }
// Commit the transaction // Commit the transaction

27
sdk/helper/dbtxn/dbtxn.go
View File

@ -7,7 +7,7 @@ import (
"strings" "strings"
) )
// ExecuteDBQuery handles executing one single statement, while properly releasing its resources. // ExecuteDBQuery handles executing one single statement while properly releasing its resources.
// - ctx: Required // - ctx: Required
// - db: Required // - db: Required
// - config: Optional, may be nil // - config: Optional, may be nil
@ -24,7 +24,19 @@ func ExecuteDBQuery(ctx context.Context, db *sql.DB, params map[string]string, q
return execute(ctx, stmt) return execute(ctx, stmt)
} }
// ExecuteTxQuery handles executing one single statement, while properly releasing its resources. // ExecuteDBQueryDirect handles executing one single statement without preparing the query
// before executing it, which can be more efficient.
// - ctx: Required
// - db: Required
// - config: Optional, may be nil
// - query: Required
func ExecuteDBQueryDirect(ctx context.Context, db *sql.DB, params map[string]string, query string) error {
parsedQuery := parseQuery(params, query)
_, err := db.ExecContext(ctx, parsedQuery)
return err
}
// ExecuteTxQuery handles executing one single statement while properly releasing its resources.
// - ctx: Required // - ctx: Required
// - tx: Required // - tx: Required
// - config: Optional, may be nil // - config: Optional, may be nil
@ -41,6 +53,17 @@ func ExecuteTxQuery(ctx context.Context, tx *sql.Tx, params map[string]string, q
return execute(ctx, stmt) return execute(ctx, stmt)
} }
// ExecuteTxQueryDirect handles executing one single statement.
// - ctx: Required
// - tx: Required
// - config: Optional, may be nil
// - query: Required
func ExecuteTxQueryDirect(ctx context.Context, tx *sql.Tx, params map[string]string, query string) error {
parsedQuery := parseQuery(params, query)
_, err := tx.ExecContext(ctx, parsedQuery)
return err
}
func execute(ctx context.Context, stmt *sql.Stmt) error { func execute(ctx context.Context, stmt *sql.Stmt) error {
if _, err := stmt.ExecContext(ctx); err != nil { if _, err := stmt.ExecContext(ctx); err != nil {
return err return err