helm / helm/helm

SQL storage driver: transaction and error-contract correctness issues

Open
#32,394 8 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
30.2k
Forks
7.8k
Avg merge
20h 58m
Merged PRs (30d)
36

Description

### What happened?

While reading `pkg/storage/driver/sql.go` I found a cluster of transaction- and error-contract defects in the SQL storage backend (`HELM_DRIVER=sql`). Filing them together since they're all in one file and some fixes overlap; happy to split if preferred. PRs incoming, smallest first.

**1. `Create`/`Delete` commit via `defer` and discard the commit error — silent data loss**

`Create` ends with `defer transaction.Commit()` and `Delete` registers the same defer *before* its DELETE statements run. Consequences:

- If COMMIT fails (connection drop, server error), `Create` returns `nil` while the release record was never persisted; `Delete` returns the release as deleted while nothing was deleted.
- In `Delete`, error paths after the defer (custom-labels lookup or delete failing) still **commit a partial deletion** while reporting failure — release row gone, labels orphaned.

**2. `ErrReleaseExists` detection can never work on PostgreSQL**

After a failed INSERT inside the transaction, `Create` runs a SELECT **on the same transaction** to decide between `ErrReleaseExists` and the raw error. PostgreSQL aborts a transaction after any statement error ("current transaction is aborted, commands ignored until end of transaction block"), so that SELECT always errors and the `ErrReleaseExists` branch is unreachable. The SQL backend therefore surfaces a raw `pq: duplicate key value…` where the Secrets/ConfigMaps drivers return Helm's "release already exists" — a storage-contract break (postgres is the only supported dialect, so this is the only path).

**3. Every read error mapped to `ErrReleaseNotFound`**

`Get` and the existence check in `Delete` map *any* DB error (connection refused, RLS permission denied, timeout) to `ErrReleaseNotFound`, with the real cause only at debug level. Callers make control-flow decisions on that sentinel — e.g. treating a transient DB outage as "release absent". Only `sql.ErrNoRows` should map to not-found.

**4. Invalid SQL in the `custom_labels` down migration**

The Down migration is `DELETE TABLE …;` — not valid SQL (should be `DROP TABLE`). Latent today since only `migrate.Up` runs, but wrong nonetheless.

**5. `Create`/`Update` mutate the driver's namespace — stateful and racy**

`s.namespace = namespace` in both write paths overwrites the *driver instance's* namespace with whichever release was last written. Any SDK program sharing one configuration across namespaces gets `Get`/`List`/`Query` scoped to the namespace of the most recent write, and it's a data race under concurrency (`List` reads `s.namespace` while `Create` writes it). The Secrets driver derives scope per call instead.

**6. `Update` never syncs custom labels**

`Update` rewrites body/name/version/status but never touches the `custom_labels_v1` table, so label changes on a stored release are silently dropped for the SQL backend (the K8s drivers re-persist labels with the whole object).

**7. `getReleaseCustomLabels` ignores the namespace it is given, so all-namespaces listing returns no custom labels**

`sql.go:694` declares `func (s *SQL) getReleaseCustomLabels(key string, _ string)` — the namespace parameter is discarded, and the query filters on `s.namespace` instead (`sql.go:698-699`). But `List` and `Query` both support all-namespaces mode (`if s.namespace != "" { ...filter... }`) and call it per row as `s.getReleaseCustomLabels(record.Key, record.Namespace)`, clearly intending per-record scoping. With `s.namespace == ""` the label lookup becomes `WHERE release_namespace = ''`, which matches nothing, so **every release listed across namespaces comes back with empty custom labels** on the SQL backend. Honouring the argument already passed at both call sites is the fix; it overlaps heavily with item 5, so the two are best done together.

### What did you expect to happen?

Commit errors surface to the caller; duplicate creates return `ErrReleaseExists` like the other drivers; only "no rows" maps to `ErrReleaseNotFound`; valid SQL in migrations; per-operation namespace scoping; label updates persisted.

### How can we reproduce it (as minimally and precisely as possible)?

Each item is visible from the code paths cited above; the commit-handling one is reproducible by killing the DB connection between the statements and COMMIT (or via sqlmock, as the regression tests in the first PR do).

### Helm version

```console
$ helm version
main @ ccb8f59
```

### Kubernetes version

Not applicable — SQL storage backend, no cluster involved.

Contributor guide

Open the contributing guide

Research direction

Start in pkg/storage/driver/sql.go, reading Create, Delete, Get, Update, the custom_labels migration, and getReleaseCustomLabels; trace the cited transaction, namespace, and label paths. Run the SQL storage tests and the sqlmock regression tests mentioned in the issue. Done means commit and database errors preserve their contracts, migrations are valid, namespace scoping is per operation, and custom labels remain correct.

Written by the indexing model from the issue text.

Assessment

Tech stack
go, sql
Domain
backend, databases
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.