From e02a54f3ccff22db4901ebbc8b696bf8d5f7534d Mon Sep 17 00:00:00 2001 From: Mukul Date: Sat, 25 Jul 2026 15:33:43 +0530 Subject: [PATCH 1/3] Fix SQL driver error mapping for reads and duplicate creates Two related error-contract problems in the SQL storage driver: Get and Delete mapped *any* error from the release lookup to ErrReleaseNotFound. A connection failure, a permission error or a timeout was therefore indistinguishable from a release that genuinely does not exist, and callers branch on that sentinel - a transient database outage could be read as 'release absent'. Only sql.ErrNoRows now maps to ErrReleaseNotFound; every other error is wrapped and returned. Delete additionally left its transaction open on that path, so it is now rolled back. Create could never return ErrReleaseExists. After the insert fails the transaction is in an aborted state, and PostgreSQL rejects every subsequent statement on it, so the follow-up SELECT that decides between 'already exists' and a genuine error always failed and the raw driver error was surfaced instead. This diverges from the Secrets and ConfigMaps drivers, which return ErrReleaseExists. The transaction is now rolled back first and the existence check runs outside it. Refs #32394 (items 2 and 3). Signed-off-by: Mukul --- pkg/storage/driver/sql.go | 29 ++++++++-- pkg/storage/driver/sql_test.go | 102 ++++++++++++++++++++++++++++++++- 2 files changed, 123 insertions(+), 8 deletions(-) diff --git a/pkg/storage/driver/sql.go b/pkg/storage/driver/sql.go index db076da98..8dff008f1 100644 --- a/pkg/storage/driver/sql.go +++ b/pkg/storage/driver/sql.go @@ -17,6 +17,8 @@ limitations under the License. package driver import ( + "database/sql" + "errors" "fmt" "log/slog" "maps" @@ -318,10 +320,15 @@ func (s *SQL) Get(key string) (release.Releaser, error) { return nil, err } - // Get will return an error if the result is empty + // Get will return sql.ErrNoRows if the result is empty. Any other error + // (connection failure, permission denied, timeout, ...) means we do not + // know whether the release exists, so it must not be reported as missing. if err := s.db.Get(&record, query, args...); err != nil { s.Logger().Debug("got SQL error when getting release", slog.String("key", key), slog.Any("error", err)) - return nil, ErrReleaseNotFound + if errors.Is(err, sql.ErrNoRows) { + return nil, ErrReleaseNotFound + } + return nil, fmt.Errorf("failed to get release %q: %w", key, err) } release, err := decodeRelease(record.Body) @@ -519,7 +526,13 @@ func (s *SQL) Create(key string, rel release.Releaser) error { } if _, err := transaction.Exec(insertQuery, args...); err != nil { - defer transaction.Rollback() + // The failed statement leaves the transaction in an aborted state - + // PostgreSQL rejects every subsequent statement on it with "current + // transaction is aborted" - so roll it back before checking whether the + // insert failed because the release already exists. Running that check + // on the transaction always errors out, which made ErrReleaseExists + // unreachable and surfaced the raw driver error instead. + transaction.Rollback() selectQuery, args, buildErr := s.statementBuilder. Select(sqlReleaseTableKeyColumn). @@ -533,7 +546,7 @@ func (s *SQL) Create(key string, rel release.Releaser) error { } var record SQLReleaseWrapper - if err := transaction.Get(&record, selectQuery, args...); err == nil { + if getErr := s.db.Get(&record, selectQuery, args...); getErr == nil { s.Logger().Debug("release already exists", slog.String("key", key)) return ErrReleaseExists } @@ -639,8 +652,12 @@ func (s *SQL) Delete(key string) (release.Releaser, error) { var record SQLReleaseWrapper err = transaction.Get(&record, selectQuery, args...) if err != nil { - s.Logger().Debug("release not found", slog.String("key", key), slog.Any("error", err)) - return nil, ErrReleaseNotFound + s.Logger().Debug("failed to get release for deletion", slog.String("key", key), slog.Any("error", err)) + transaction.Rollback() + if errors.Is(err, sql.ErrNoRows) { + return nil, ErrReleaseNotFound + } + return nil, fmt.Errorf("failed to get release %q: %w", key, err) } release, err := decodeRelease(record.Body) diff --git a/pkg/storage/driver/sql_test.go b/pkg/storage/driver/sql_test.go index 044e9df7b..f75ec879c 100644 --- a/pkg/storage/driver/sql_test.go +++ b/pkg/storage/driver/sql_test.go @@ -14,6 +14,7 @@ limitations under the License. package driver import ( + "database/sql" "database/sql/driver" "errors" "fmt" @@ -103,6 +104,97 @@ func TestSQLGet(t *testing.T) { assert.NoErrorf(t, mock.ExpectationsWereMet(), "sql expectations weren't met") } +// A real database failure must not be reported as "release not found": callers +// branch on ErrReleaseNotFound and would treat an outage as an absent release. +func TestSQLGetDatabaseError(t *testing.T) { + vers := int(1) + name := "smug-pigeon" + namespace := "default" + key := testKey(name, vers) + + sqlDriver, mock := newTestFixtureSQL(t) + + query := fmt.Sprintf( + regexp.QuoteMeta("SELECT %s FROM %s WHERE %s = $1 AND %s = $2"), + sqlReleaseTableBodyColumn, + sqlReleaseTableName, + sqlReleaseTableKeyColumn, + sqlReleaseTableNamespaceColumn, + ) + + dbErr := errors.New("connection refused") + mock. + ExpectQuery(query). + WithArgs(key, namespace). + WillReturnError(dbErr) + + _, err := sqlDriver.Get(key) + require.Error(t, err, "expected an error when the database query fails") + require.NotErrorIs(t, err, ErrReleaseNotFound, "database failure must not be reported as ErrReleaseNotFound") + require.ErrorIs(t, err, dbErr, "expected the underlying database error to be wrapped") + assert.NoErrorf(t, mock.ExpectationsWereMet(), "sql expectations weren't met") +} + +// An empty result set is the only case that means "release not found". +func TestSQLGetNotFound(t *testing.T) { + vers := int(1) + name := "smug-pigeon" + namespace := "default" + key := testKey(name, vers) + + sqlDriver, mock := newTestFixtureSQL(t) + + query := fmt.Sprintf( + regexp.QuoteMeta("SELECT %s FROM %s WHERE %s = $1 AND %s = $2"), + sqlReleaseTableBodyColumn, + sqlReleaseTableName, + sqlReleaseTableKeyColumn, + sqlReleaseTableNamespaceColumn, + ) + + mock. + ExpectQuery(query). + WithArgs(key, namespace). + WillReturnError(sql.ErrNoRows) + + _, err := sqlDriver.Get(key) + require.ErrorIs(t, err, ErrReleaseNotFound) + assert.NoErrorf(t, mock.ExpectationsWereMet(), "sql expectations weren't met") +} + +// Same contract for Delete's existence check, which must also not leave the +// transaction open when the lookup fails. +func TestSQLDeleteDatabaseError(t *testing.T) { + vers := int(1) + name := "smug-pigeon" + namespace := "default" + key := testKey(name, vers) + + sqlDriver, mock := newTestFixtureSQL(t) + + selectQuery := fmt.Sprintf( + "SELECT %s FROM %s WHERE %s = $1 AND %s = $2", + sqlReleaseTableBodyColumn, + sqlReleaseTableName, + sqlReleaseTableKeyColumn, + sqlReleaseTableNamespaceColumn, + ) + + dbErr := errors.New("connection refused") + mock.ExpectBegin() + mock. + ExpectQuery(regexp.QuoteMeta(selectQuery)). + WithArgs(key, namespace). + WillReturnError(dbErr) + mock.ExpectRollback() + + _, err := sqlDriver.Delete(key) + require.Error(t, err, "expected an error when the database query fails") + require.NotErrorIs(t, err, ErrReleaseNotFound, "database failure must not be reported as ErrReleaseNotFound") + require.ErrorIs(t, err, dbErr, "expected the underlying database error to be wrapped") + assert.NoErrorf(t, mock.ExpectationsWereMet(), "sql expectations weren't met") +} + func TestSQLList(t *testing.T) { releases := []*rspb.Release{} releases = append(releases, @@ -269,6 +361,11 @@ func TestSqlCreateAlreadyExists(t *testing.T) { sqlReleaseTableNamespaceColumn, ) + // The failed insert aborts the transaction, so it is rolled back before the + // existence check runs - the check has to happen outside the transaction or + // PostgreSQL rejects it and ErrReleaseExists can never be returned. + mock.ExpectRollback() + // Let's check that we do make sure the error is due to a release already existing mock. ExpectQuery(selectQuery). @@ -280,9 +377,10 @@ func TestSqlCreateAlreadyExists(t *testing.T) { key, ), ).RowsWillBeClosed() - mock.ExpectRollback() - require.Errorf(t, sqlDriver.Create(key, rel), "failed to create release with key %s", key) + err := sqlDriver.Create(key, rel) + require.Errorf(t, err, "expected Create to fail for an existing release with key %s", key) + require.ErrorIs(t, err, ErrReleaseExists) assert.NoErrorf(t, mock.ExpectationsWereMet(), "sql expectations weren't met") } From 22b197580bc4ed17f6ebf4b85804f70634d949fc Mon Sep 17 00:00:00 2001 From: Mukul Date: Sat, 8 Aug 2026 09:55:19 +0530 Subject: [PATCH 2/3] Wrap create errors and cover the non-duplicate insert failure Review follow-up: the Create fall-through returned the raw insert error while Get and Delete wrap theirs, so wrap it the same way for consistent diagnostics across the driver. The 'insert failed and the release does not already exist' branch became reachable with the rollback fix but had no coverage; add a test asserting the original error is surfaced and that it is not ErrReleaseExists. Signed-off-by: Mukul --- pkg/storage/driver/sql.go | 4 +-- pkg/storage/driver/sql_test.go | 57 ++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 2 deletions(-) diff --git a/pkg/storage/driver/sql.go b/pkg/storage/driver/sql.go index 8dff008f1..ec3566851 100644 --- a/pkg/storage/driver/sql.go +++ b/pkg/storage/driver/sql.go @@ -542,7 +542,7 @@ func (s *SQL) Create(key string, rel release.Releaser) error { ToSql() if buildErr != nil { s.Logger().Debug("failed to build select query", "error", buildErr) - return err + return fmt.Errorf("failed to create release %q: %w", key, err) } var record SQLReleaseWrapper @@ -552,7 +552,7 @@ func (s *SQL) Create(key string, rel release.Releaser) error { } s.Logger().Debug("failed to store release in SQL database", slog.String("key", key), slog.Any("error", err)) - return err + return fmt.Errorf("failed to create release %q: %w", key, err) } // Filtering labels before insert cause in SQL storage driver system releases are stored in separate columns of release table diff --git a/pkg/storage/driver/sql_test.go b/pkg/storage/driver/sql_test.go index f75ec879c..7ca845485 100644 --- a/pkg/storage/driver/sql_test.go +++ b/pkg/storage/driver/sql_test.go @@ -384,6 +384,63 @@ func TestSqlCreateAlreadyExists(t *testing.T) { assert.NoErrorf(t, mock.ExpectationsWereMet(), "sql expectations weren't met") } +func TestSqlCreateInsertFailureNotAlreadyExists(t *testing.T) { + vers := 1 + name := "smug-pigeon" + namespace := "default" + key := testKey(name, vers) + rel := releaseStub(name, vers, namespace, common.StatusDeployed) + + sqlDriver, mock := newTestFixtureSQL(t) + body, _ := encodeRelease(rel) + + insertQuery := fmt.Sprintf( + "INSERT INTO %s (%s,%s,%s,%s,%s,%s,%s,%s,%s) VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9)", + sqlReleaseTableName, + sqlReleaseTableKeyColumn, + sqlReleaseTableTypeColumn, + sqlReleaseTableBodyColumn, + sqlReleaseTableNameColumn, + sqlReleaseTableNamespaceColumn, + sqlReleaseTableVersionColumn, + sqlReleaseTableStatusColumn, + sqlReleaseTableOwnerColumn, + sqlReleaseTableCreatedAtColumn, + ) + + // The insert fails for a reason unrelated to a duplicate key, e.g. the + // database is unreachable. + insertErr := errors.New("connection refused") + mock.ExpectBegin() + mock. + ExpectExec(regexp.QuoteMeta(insertQuery)). + WithArgs(key, sqlReleaseDefaultType, body, rel.Name, rel.Namespace, int(rel.Version), rel.Info.Status.String(), sqlReleaseDefaultOwner, recentUnixTimestamp()). + WillReturnError(insertErr) + + selectQuery := fmt.Sprintf( + regexp.QuoteMeta("SELECT %s FROM %s WHERE %s = $1 AND %s = $2"), + sqlReleaseTableKeyColumn, + sqlReleaseTableName, + sqlReleaseTableKeyColumn, + sqlReleaseTableNamespaceColumn, + ) + + mock.ExpectRollback() + + // No row comes back, so the release does not already exist and the original + // insert error has to be surfaced instead of ErrReleaseExists. + mock. + ExpectQuery(selectQuery). + WithArgs(key, namespace). + WillReturnError(sql.ErrNoRows) + + err := sqlDriver.Create(key, rel) + require.Errorf(t, err, "expected Create to fail when the insert fails with key %s", key) + require.NotErrorIs(t, err, ErrReleaseExists) + require.ErrorIs(t, err, insertErr) + assert.NoErrorf(t, mock.ExpectationsWereMet(), "sql expectations weren't met") +} + func TestSqlUpdate(t *testing.T) { vers := 1 name := "smug-pigeon" From e55162daaa48845e553d3df6901404a197846988 Mon Sep 17 00:00:00 2001 From: Mukul Date: Sat, 8 Aug 2026 10:18:45 +0530 Subject: [PATCH 3/3] Use ExecContext for the SQL driver's statements Importing database/sql for the sql.ErrNoRows check lets the noctx linter resolve the sqlx receivers, which surfaced the existing plain Exec calls in this file. Exec is defined as ExecContext with context.Background(), so this is behaviour-preserving and keeps the golangci-lint job green. Signed-off-by: Mukul --- pkg/storage/driver/sql.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/pkg/storage/driver/sql.go b/pkg/storage/driver/sql.go index ec3566851..93739badb 100644 --- a/pkg/storage/driver/sql.go +++ b/pkg/storage/driver/sql.go @@ -17,6 +17,7 @@ limitations under the License. package driver import ( + "context" "database/sql" "errors" "fmt" @@ -525,7 +526,7 @@ func (s *SQL) Create(key string, rel release.Releaser) error { return err } - if _, err := transaction.Exec(insertQuery, args...); err != nil { + if _, err := transaction.ExecContext(context.Background(), insertQuery, args...); err != nil { // The failed statement leaves the transaction in an aborted state - // PostgreSQL rejects every subsequent statement on it with "current // transaction is aborted" - so roll it back before checking whether the @@ -577,7 +578,7 @@ func (s *SQL) Create(key string, rel release.Releaser) error { return err } - if _, err := transaction.Exec(insertLabelsQuery, args...); err != nil { + if _, err := transaction.ExecContext(context.Background(), insertLabelsQuery, args...); err != nil { defer transaction.Rollback() s.Logger().Debug("failed to write Labels", slog.Any("error", err)) return err @@ -622,7 +623,7 @@ func (s *SQL) Update(key string, rel release.Releaser) error { return err } - if _, err := s.db.Exec(query, args...); err != nil { + if _, err := s.db.ExecContext(context.Background(), query, args...); err != nil { s.Logger().Debug("failed to update release in SQL database", slog.String("key", key), slog.Any("error", err)) return err } @@ -678,7 +679,7 @@ func (s *SQL) Delete(key string) (release.Releaser, error) { return nil, err } - _, err = transaction.Exec(deleteQuery, args...) + _, err = transaction.ExecContext(context.Background(), deleteQuery, args...) if err != nil { s.Logger().Debug("failed perform delete query", slog.Any("error", err)) return release, err @@ -702,7 +703,7 @@ func (s *SQL) Delete(key string) (release.Releaser, error) { s.Logger().Debug("failed to build delete Labels query", slog.Any("error", err)) return nil, err } - _, err = transaction.Exec(deleteCustomLabelsQuery, args...) + _, err = transaction.ExecContext(context.Background(), deleteCustomLabelsQuery, args...) return release, err }