From aa6d61477ec4d2a0ec13fcfae08c7c96d64c45b6 Mon Sep 17 00:00:00 2001 From: Christopher Swenson Date: Tue, 26 Apr 2022 12:47:06 -0700 Subject: [PATCH] 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. --- builtin/logical/database/rotation_test.go | 2 +- builtin/logical/mssql/path_creds_create.go | 2 +- builtin/logical/mssql/secret_creds.go | 2 +- builtin/logical/mysql/path_role_create.go | 2 +- .../logical/postgresql/path_role_create.go | 2 +- builtin/logical/postgresql/secret_creds.go | 4 +-- changelog/15166.txt | 3 +++ plugins/database/hana/hana.go | 8 +++--- plugins/database/mssql/mssql.go | 8 +++--- plugins/database/mssql/mssql_test.go | 2 +- plugins/database/postgresql/postgresql.go | 12 ++++----- plugins/database/redshift/redshift.go | 10 +++---- plugins/database/redshift/redshift_test.go | 2 +- sdk/helper/dbtxn/dbtxn.go | 27 +++++++++++++++++-- 14 files changed, 56 insertions(+), 30 deletions(-) create mode 100644 changelog/15166.txt diff --git a/builtin/logical/database/rotation_test.go b/builtin/logical/database/rotation_test.go index 3ebc50ca5..6aa22a257 100644 --- a/builtin/logical/database/rotation_test.go +++ b/builtin/logical/database/rotation_test.go @@ -444,7 +444,7 @@ func createTestPGUser(t *testing.T, connURL string, username, password, query st "name": username, "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) } // Commit the transaction diff --git a/builtin/logical/mssql/path_creds_create.go b/builtin/logical/mssql/path_creds_create.go index 7982e630b..42e8642c0 100644 --- a/builtin/logical/mssql/path_creds_create.go +++ b/builtin/logical/mssql/path_creds_create.go @@ -95,7 +95,7 @@ func (b *backend) pathCredsCreateRead(ctx context.Context, req *logical.Request, "name": username, "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 } } diff --git a/builtin/logical/mssql/secret_creds.go b/builtin/logical/mssql/secret_creds.go index a1a550b6d..9fc52f9a7 100644 --- a/builtin/logical/mssql/secret_creds.go +++ b/builtin/logical/mssql/secret_creds.go @@ -131,7 +131,7 @@ func (b *backend) secretCredsRevoke(ctx context.Context, req *logical.Request, d // many permissions as possible right now var lastStmtError error 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 continue } diff --git a/builtin/logical/mysql/path_role_create.go b/builtin/logical/mysql/path_role_create.go index a553fc0c2..34c08102d 100644 --- a/builtin/logical/mysql/path_role_create.go +++ b/builtin/logical/mysql/path_role_create.go @@ -108,7 +108,7 @@ func (b *backend) pathRoleCreateRead(ctx context.Context, req *logical.Request, "name": username, "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 } } diff --git a/builtin/logical/postgresql/path_role_create.go b/builtin/logical/postgresql/path_role_create.go index 2a0cde0b7..c050a7ed3 100644 --- a/builtin/logical/postgresql/path_role_create.go +++ b/builtin/logical/postgresql/path_role_create.go @@ -113,7 +113,7 @@ func (b *backend) pathRoleCreateRead(ctx context.Context, req *logical.Request, "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 } } diff --git a/builtin/logical/postgresql/secret_creds.go b/builtin/logical/postgresql/secret_creds.go index a96c27145..2b2afaceb 100644 --- a/builtin/logical/postgresql/secret_creds.go +++ b/builtin/logical/postgresql/secret_creds.go @@ -211,7 +211,7 @@ func (b *backend) secretCredsRevoke(ctx context.Context, req *logical.Request, d // many permissions as possible right now var lastStmtError error 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 } } @@ -254,7 +254,7 @@ func (b *backend) secretCredsRevoke(ctx context.Context, req *logical.Request, d m := map[string]string{ "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 } } diff --git a/changelog/15166.txt b/changelog/15166.txt new file mode 100644 index 000000000..bf73aef93 --- /dev/null +++ b/changelog/15166.txt @@ -0,0 +1,3 @@ +```release-note:improvement +core: Add new DB methods that do not prepare statements. +``` diff --git a/plugins/database/hana/hana.go b/plugins/database/hana/hana.go index 7802192ad..7462a3506 100644 --- a/plugins/database/hana/hana.go +++ b/plugins/database/hana/hana.go @@ -134,7 +134,7 @@ func (h *HANA) NewUser(ctx context.Context, req dbplugin.NewUserRequest) (respon "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 } } @@ -223,7 +223,7 @@ func (h *HANA) updateUserPassword(ctx context.Context, tx *sql.Tx, username stri "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) } } @@ -259,7 +259,7 @@ func (h *HANA) updateUserExpiration(ctx context.Context, tx *sql.Tx, username st "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) } } @@ -302,7 +302,7 @@ func (h *HANA) DeleteUser(ctx context.Context, req dbplugin.DeleteUserRequest) ( m := map[string]string{ "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 } } diff --git a/plugins/database/mssql/mssql.go b/plugins/database/mssql/mssql.go index 00f1773a1..545049473 100644 --- a/plugins/database/mssql/mssql.go +++ b/plugins/database/mssql/mssql.go @@ -154,7 +154,7 @@ func (m *MSSQL) NewUser(ctx context.Context, req dbplugin.NewUserRequest) (dbplu "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 } } @@ -198,7 +198,7 @@ func (m *MSSQL) DeleteUser(ctx context.Context, req dbplugin.DeleteUserRequest) m := map[string]string{ "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) } } @@ -303,7 +303,7 @@ func (m *MSSQL) revokeUserDefault(ctx context.Context, username string) error { // many permissions as possible right now var lastStmtError error 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 } } @@ -394,7 +394,7 @@ func (m *MSSQL) updateUserPass(ctx context.Context, username string, changePass "username": username, "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) } } diff --git a/plugins/database/mssql/mssql_test.go b/plugins/database/mssql/mssql_test.go index 8f2dac0db..2292490d8 100644 --- a/plugins/database/mssql/mssql_test.go +++ b/plugins/database/mssql/mssql_test.go @@ -552,7 +552,7 @@ func createTestMSSQLUser(connURL string, username, password, query string) error "name": username, "password": password, } - if err := dbtxn.ExecuteTxQuery(ctx, tx, m, query); err != nil { + if err := dbtxn.ExecuteTxQueryDirect(ctx, tx, m, query); err != nil { return err } // Commit the transaction diff --git a/plugins/database/postgresql/postgresql.go b/plugins/database/postgresql/postgresql.go index a3826d3a8..908519ec7 100644 --- a/plugins/database/postgresql/postgresql.go +++ b/plugins/database/postgresql/postgresql.go @@ -180,7 +180,7 @@ func (p *PostgreSQL) changeUserPassword(ctx context.Context, username string, ch "username": username, "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) } } @@ -229,7 +229,7 @@ func (p *PostgreSQL) changeUserExpiration(ctx context.Context, username string, "username": username, "expiration": expirationStr, } - if err := dbtxn.ExecuteTxQuery(ctx, tx, m, query); err != nil { + if err := dbtxn.ExecuteTxQueryDirect(ctx, tx, m, query); err != nil { return err } } @@ -273,7 +273,7 @@ func (p *PostgreSQL) NewUser(ctx context.Context, req dbplugin.NewUserRequest) ( "password": req.Password, "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) } continue @@ -291,7 +291,7 @@ func (p *PostgreSQL) NewUser(ctx context.Context, req dbplugin.NewUserRequest) ( "password": req.Password, "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) } } @@ -343,7 +343,7 @@ func (p *PostgreSQL) customDeleteUser(ctx context.Context, username string, revo "name": 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 } } @@ -436,7 +436,7 @@ func (p *PostgreSQL) defaultDeleteUser(ctx context.Context, username string) err // many permissions as possible right now var lastStmtError error 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 } } diff --git a/plugins/database/redshift/redshift.go b/plugins/database/redshift/redshift.go index 86e3fc33e..3b4cf8b39 100644 --- a/plugins/database/redshift/redshift.go +++ b/plugins/database/redshift/redshift.go @@ -164,7 +164,7 @@ func (r *RedShift) NewUser(ctx context.Context, req dbplugin.NewUserRequest) (db "password": password, "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 } } @@ -246,7 +246,7 @@ func updateUserExpiration(ctx context.Context, req dbplugin.UpdateUserRequest, t "username": req.Username, "expiration": expirationStr, } - if err := dbtxn.ExecuteTxQuery(ctx, tx, m, query); err != nil { + if err := dbtxn.ExecuteTxQueryDirect(ctx, tx, m, query); err != nil { return err } } @@ -293,7 +293,7 @@ func updateUserPassword(ctx context.Context, req dbplugin.UpdateUserRequest, tx "username": username, "password": password, } - if err := dbtxn.ExecuteTxQuery(ctx, tx, m, query); err != nil { + if err := dbtxn.ExecuteTxQueryDirect(ctx, tx, m, query); err != nil { return err } } @@ -341,7 +341,7 @@ func (r *RedShift) customDeleteUser(ctx context.Context, req dbplugin.DeleteUser "name": 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 } } @@ -452,7 +452,7 @@ $$;`) // many permissions as possible right now var lastStmtError *multierror.Error // error 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) } } diff --git a/plugins/database/redshift/redshift_test.go b/plugins/database/redshift/redshift_test.go index b4a107a92..02b082bc3 100644 --- a/plugins/database/redshift/redshift_test.go +++ b/plugins/database/redshift/redshift_test.go @@ -537,7 +537,7 @@ func createTestPGUser(t *testing.T, connURL string, username, password, query st "name": username, "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) } // Commit the transaction diff --git a/sdk/helper/dbtxn/dbtxn.go b/sdk/helper/dbtxn/dbtxn.go index fab9e942d..a5c8bf961 100644 --- a/sdk/helper/dbtxn/dbtxn.go +++ b/sdk/helper/dbtxn/dbtxn.go @@ -7,7 +7,7 @@ import ( "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 // - db: Required // - 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) } -// 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 // - tx: Required // - 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) } +// 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 { if _, err := stmt.ExecContext(ctx); err != nil { return err