From f8e907e0deda09a4b4dd7b56d28309ae45de3e75 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 3ebc50ca5e..6aa22a2575 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 7982e630bc..42e8642c03 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 a1a550b6d3..9fc52f9a77 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 a553fc0c22..34c08102d8 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 2a0cde0b71..c050a7ed34 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 a96c271451..2b2afaceba 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 0000000000..bf73aef93c --- /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 7802192ad7..7462a35063 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 00f1773a1e..5450494731 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 8f2dac0dbd..2292490d88 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 a3826d3a83..908519ec70 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 86e3fc33e0..3b4cf8b39d 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 b4a107a92e..02b082bc35 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 fab9e942d7..a5c8bf961e 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