diff --git a/pkg/storage/driver/sql.go b/pkg/storage/driver/sql.go index 2b278f7cb..ac990b97b 100644 --- a/pkg/storage/driver/sql.go +++ b/pkg/storage/driver/sql.go @@ -607,6 +607,12 @@ func (s *SQL) Update(key string, rel release.Releaser) error { return err } + transaction, err := s.db.Beginx() + if err != nil { + s.Logger().Debug("failed to start SQL transaction", slog.Any("error", err)) + return fmt.Errorf("error beginning transaction: %w", err) + } + query, args, err := s.statementBuilder. Update(sqlReleaseTableName). Set(sqlReleaseTableBodyColumn, body). @@ -619,15 +625,70 @@ func (s *SQL) Update(key string, rel release.Releaser) error { Where(sq.Eq{sqlReleaseTableNamespaceColumn: namespace}). ToSql() if err != nil { + transaction.Rollback() s.Logger().Debug("failed to build update query", slog.Any("error", err)) return err } - if _, err := s.db.Exec(query, args...); err != nil { + if _, err := transaction.Exec(query, args...); err != nil { + transaction.Rollback() s.Logger().Debug("failed to update release in SQL database", slog.String("key", key), slog.Any("error", err)) return err } + // Custom labels aren't part of the release body update above, so they must be + // reconciled separately: clear out whatever was stored for this release and + // re-seed it from the current label set, mirroring how Create seeds labels. + // Without this, labels added, changed, or removed after the initial Create are + // silently dropped on every subsequent Update. + deleteLabelsQuery, args, err := s.statementBuilder. + Delete(sqlCustomLabelsTableName). + Where(sq.Eq{ + sqlCustomLabelsTableReleaseKeyColumn: key, + sqlCustomLabelsTableReleaseNamespaceColumn: namespace, + }). + ToSql() + if err != nil { + transaction.Rollback() + s.Logger().Debug("failed to build delete labels query", slog.Any("error", err)) + return err + } + + if _, err := transaction.Exec(deleteLabelsQuery, args...); err != nil { + transaction.Rollback() + s.Logger().Debug("failed to clear existing labels", slog.Any("error", err)) + return err + } + + for k, v := range filterSystemLabels(rls.Labels) { + insertLabelsQuery, args, err := s.statementBuilder. + Insert(sqlCustomLabelsTableName). + Columns( + sqlCustomLabelsTableReleaseKeyColumn, + sqlCustomLabelsTableReleaseNamespaceColumn, + sqlCustomLabelsTableKeyColumn, + sqlCustomLabelsTableValueColumn, + ). + Values(key, namespace, k, v). + ToSql() + if err != nil { + transaction.Rollback() + s.Logger().Debug("failed to build insert labels query", slog.Any("error", err)) + return err + } + + if _, err := transaction.Exec(insertLabelsQuery, args...); err != nil { + transaction.Rollback() + s.Logger().Debug("failed to write updated labels", slog.Any("error", err)) + return err + } + } + + if err := transaction.Commit(); err != nil { + s.Logger().Debug("failed to commit transaction", slog.Any("error", err)) + return fmt.Errorf("error committing transaction: %w", err) + } + return nil } diff --git a/pkg/storage/driver/sql_test.go b/pkg/storage/driver/sql_test.go index e5fde405b..260046163 100644 --- a/pkg/storage/driver/sql_test.go +++ b/pkg/storage/driver/sql_test.go @@ -308,16 +308,235 @@ func TestSqlUpdate(t *testing.T) { sqlReleaseTableKeyColumn, sqlReleaseTableNamespaceColumn, ) + deleteLabelsQuery := fmt.Sprintf( + "DELETE FROM %s WHERE %s = $1 AND %s = $2", + sqlCustomLabelsTableName, + sqlCustomLabelsTableReleaseKeyColumn, + sqlCustomLabelsTableReleaseNamespaceColumn, + ) + insertLabelsQuery := fmt.Sprintf( + "INSERT INTO %s (%s,%s,%s,%s) VALUES ($1,$2,$3,$4)", + sqlCustomLabelsTableName, + sqlCustomLabelsTableReleaseKeyColumn, + sqlCustomLabelsTableReleaseNamespaceColumn, + sqlCustomLabelsTableKeyColumn, + sqlCustomLabelsTableValueColumn, + ) + mock.ExpectBegin() mock. ExpectExec(regexp.QuoteMeta(query)). WithArgs(body, rel.Name, int(rel.Version), rel.Info.Status.String(), sqlReleaseDefaultOwner, recentUnixTimestamp(), key, namespace). WillReturnResult(sqlmock.NewResult(0, 1)) + mock. + ExpectExec(regexp.QuoteMeta(deleteLabelsQuery)). + WithArgs(key, namespace). + WillReturnResult(sqlmock.NewResult(0, 1)) + + mock.MatchExpectationsInOrder(false) + for k, v := range filterSystemLabels(rel.Labels) { + mock. + ExpectExec(regexp.QuoteMeta(insertLabelsQuery)). + WithArgs(key, namespace, k, v). + WillReturnResult(sqlmock.NewResult(1, 1)) + } + mock.ExpectCommit() require.NoErrorf(t, sqlDriver.Update(key, rel), "failed to update release with key %s", key) assert.NoErrorf(t, mock.ExpectationsWereMet(), "sql expectations weren't met") } +// TestSqlUpdateSyncsAddedLabel is a regression test for helm/helm#32394 (item 6): Update +// rewrote body/name/version/status but never touched custom_labels_v1, so a label added after +// the initial Create was silently dropped on every subsequent Update. Update now reconciles +// custom_labels_v1 by clearing existing rows for the release and re-inserting its current labels. +func TestSqlUpdateSyncsAddedLabel(t *testing.T) { + vers := 1 + name := "smug-pigeon" + namespace := "default" + key := testKey(name, vers) + rel := releaseStub(name, vers, namespace, common.StatusDeployed) + rel.Labels["environment"] = "prod" // label added since the release was created + + sqlDriver, mock := newTestFixtureSQL(t) + body, _ := encodeRelease(rel) + + query := fmt.Sprintf( + "UPDATE %s SET %s = $1, %s = $2, %s = $3, %s = $4, %s = $5, %s = $6 WHERE %s = $7 AND %s = $8", + sqlReleaseTableName, + sqlReleaseTableBodyColumn, + sqlReleaseTableNameColumn, + sqlReleaseTableVersionColumn, + sqlReleaseTableStatusColumn, + sqlReleaseTableOwnerColumn, + sqlReleaseTableModifiedAtColumn, + sqlReleaseTableKeyColumn, + sqlReleaseTableNamespaceColumn, + ) + deleteLabelsQuery := fmt.Sprintf( + "DELETE FROM %s WHERE %s = $1 AND %s = $2", + sqlCustomLabelsTableName, + sqlCustomLabelsTableReleaseKeyColumn, + sqlCustomLabelsTableReleaseNamespaceColumn, + ) + insertLabelsQuery := fmt.Sprintf( + "INSERT INTO %s (%s,%s,%s,%s) VALUES ($1,$2,$3,$4)", + sqlCustomLabelsTableName, + sqlCustomLabelsTableReleaseKeyColumn, + sqlCustomLabelsTableReleaseNamespaceColumn, + sqlCustomLabelsTableKeyColumn, + sqlCustomLabelsTableValueColumn, + ) + + mock.ExpectBegin() + mock. + ExpectExec(regexp.QuoteMeta(query)). + WithArgs(body, rel.Name, int(rel.Version), rel.Info.Status.String(), sqlReleaseDefaultOwner, recentUnixTimestamp(), key, namespace). + WillReturnResult(sqlmock.NewResult(0, 1)) + mock. + ExpectExec(regexp.QuoteMeta(deleteLabelsQuery)). + WithArgs(key, namespace). + WillReturnResult(sqlmock.NewResult(0, 1)) + + mock.MatchExpectationsInOrder(false) + for k, v := range filterSystemLabels(rel.Labels) { + mock. + ExpectExec(regexp.QuoteMeta(insertLabelsQuery)). + WithArgs(key, namespace, k, v). + WillReturnResult(sqlmock.NewResult(1, 1)) + } + mock.ExpectCommit() + + require.NoErrorf(t, sqlDriver.Update(key, rel), "failed to update release with key %s", key) + assert.NoErrorf(t, mock.ExpectationsWereMet(), "sql expectations weren't met - the newly added label was not written") +} + +// TestSqlUpdateSyncsChangedLabelValue is a regression test for helm/helm#32394 (item 6): a +// label's value changed on Update must be reflected in custom_labels_v1, not left at its +// stale, originally-created value. +func TestSqlUpdateSyncsChangedLabelValue(t *testing.T) { + vers := 1 + name := "smug-pigeon" + namespace := "default" + key := testKey(name, vers) + rel := releaseStub(name, vers, namespace, common.StatusDeployed) + rel.Labels["key1"] = "val1-updated" // value changed since the release was created + + sqlDriver, mock := newTestFixtureSQL(t) + body, _ := encodeRelease(rel) + + query := fmt.Sprintf( + "UPDATE %s SET %s = $1, %s = $2, %s = $3, %s = $4, %s = $5, %s = $6 WHERE %s = $7 AND %s = $8", + sqlReleaseTableName, + sqlReleaseTableBodyColumn, + sqlReleaseTableNameColumn, + sqlReleaseTableVersionColumn, + sqlReleaseTableStatusColumn, + sqlReleaseTableOwnerColumn, + sqlReleaseTableModifiedAtColumn, + sqlReleaseTableKeyColumn, + sqlReleaseTableNamespaceColumn, + ) + deleteLabelsQuery := fmt.Sprintf( + "DELETE FROM %s WHERE %s = $1 AND %s = $2", + sqlCustomLabelsTableName, + sqlCustomLabelsTableReleaseKeyColumn, + sqlCustomLabelsTableReleaseNamespaceColumn, + ) + insertLabelsQuery := fmt.Sprintf( + "INSERT INTO %s (%s,%s,%s,%s) VALUES ($1,$2,$3,$4)", + sqlCustomLabelsTableName, + sqlCustomLabelsTableReleaseKeyColumn, + sqlCustomLabelsTableReleaseNamespaceColumn, + sqlCustomLabelsTableKeyColumn, + sqlCustomLabelsTableValueColumn, + ) + + mock.ExpectBegin() + mock. + ExpectExec(regexp.QuoteMeta(query)). + WithArgs(body, rel.Name, int(rel.Version), rel.Info.Status.String(), sqlReleaseDefaultOwner, recentUnixTimestamp(), key, namespace). + WillReturnResult(sqlmock.NewResult(0, 1)) + mock. + ExpectExec(regexp.QuoteMeta(deleteLabelsQuery)). + WithArgs(key, namespace). + WillReturnResult(sqlmock.NewResult(0, 1)) + + mock.MatchExpectationsInOrder(false) + for k, v := range filterSystemLabels(rel.Labels) { + mock. + ExpectExec(regexp.QuoteMeta(insertLabelsQuery)). + WithArgs(key, namespace, k, v). + WillReturnResult(sqlmock.NewResult(1, 1)) + } + mock.ExpectCommit() + + require.NoErrorf(t, sqlDriver.Update(key, rel), "failed to update release with key %s", key) + assert.NoErrorf(t, mock.ExpectationsWereMet(), "sql expectations weren't met - the changed label value was not written") +} + +// TestSqlUpdateRemovesDroppedLabel is a regression test for helm/helm#32394 (item 6): a label +// removed on Update must be deleted from custom_labels_v1, not left orphaned from the release's +// original Create. +func TestSqlUpdateRemovesDroppedLabel(t *testing.T) { + vers := 1 + name := "smug-pigeon" + namespace := "default" + key := testKey(name, vers) + rel := releaseStub(name, vers, namespace, common.StatusDeployed) + delete(rel.Labels, "key2") // label removed since the release was created + + sqlDriver, mock := newTestFixtureSQL(t) + body, _ := encodeRelease(rel) + + query := fmt.Sprintf( + "UPDATE %s SET %s = $1, %s = $2, %s = $3, %s = $4, %s = $5, %s = $6 WHERE %s = $7 AND %s = $8", + sqlReleaseTableName, + sqlReleaseTableBodyColumn, + sqlReleaseTableNameColumn, + sqlReleaseTableVersionColumn, + sqlReleaseTableStatusColumn, + sqlReleaseTableOwnerColumn, + sqlReleaseTableModifiedAtColumn, + sqlReleaseTableKeyColumn, + sqlReleaseTableNamespaceColumn, + ) + deleteLabelsQuery := fmt.Sprintf( + "DELETE FROM %s WHERE %s = $1 AND %s = $2", + sqlCustomLabelsTableName, + sqlCustomLabelsTableReleaseKeyColumn, + sqlCustomLabelsTableReleaseNamespaceColumn, + ) + insertLabelsQuery := fmt.Sprintf( + "INSERT INTO %s (%s,%s,%s,%s) VALUES ($1,$2,$3,$4)", + sqlCustomLabelsTableName, + sqlCustomLabelsTableReleaseKeyColumn, + sqlCustomLabelsTableReleaseNamespaceColumn, + sqlCustomLabelsTableKeyColumn, + sqlCustomLabelsTableValueColumn, + ) + + mock.ExpectBegin() + mock. + ExpectExec(regexp.QuoteMeta(query)). + WithArgs(body, rel.Name, int(rel.Version), rel.Info.Status.String(), sqlReleaseDefaultOwner, recentUnixTimestamp(), key, namespace). + WillReturnResult(sqlmock.NewResult(0, 1)) + mock. + ExpectExec(regexp.QuoteMeta(deleteLabelsQuery)). + WithArgs(key, namespace). + WillReturnResult(sqlmock.NewResult(0, 1)) + + // Only key1 should be re-inserted - key2 was removed and must not be written back. + mock. + ExpectExec(regexp.QuoteMeta(insertLabelsQuery)). + WithArgs(key, namespace, "key1", "val1"). + WillReturnResult(sqlmock.NewResult(1, 1)) + mock.ExpectCommit() + + require.NoErrorf(t, sqlDriver.Update(key, rel), "failed to update release with key %s", key) + assert.NoErrorf(t, mock.ExpectationsWereMet(), "sql expectations weren't met - dropped label key2 should not have been re-inserted, and the DELETE should have cleared it") +} + func TestSqlQuery(t *testing.T) { // Reflect actual use cases in ../storage.go labelSetUnknown := map[string]string{