msb-public
/
helm
mirror of https://github.com/helm/helm
1
0
Code Issues Projects Releases Wiki Activity

fix(storage/sql): sync custom labels on Update

Browse Source 
Update rewrote body/name/version/status but never touched
custom_labels_v1, so any label added, changed, or removed after the
initial Create was silently dropped on every subsequent Update.

Update now wraps its work in a transaction: after updating the release
row, it clears existing custom_labels_v1 rows for that release and
re-inserts its current labels, mirroring how Create seeds them.
Handles label add/change/remove in one pass, with explicit commit-error
checking rather than the old defer transaction.Commit() pattern.

Fixes item 6 of #32394

Signed-off-by: ahmedfawzy21 <ahmed.fawzy21@gmail.com>
pull/32692/head
ahmedfawzy21 5 days ago
parent a8ab76e86f
commit 484884cb1a
2 changed files with 281 additions and 1 deletions
Show Stats Download Patch File Download Diff File

63
pkg/storage/driver/sql.go
Unescape View File

@ -593,6 +593,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).
@ -605,15 +611,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
}

219
pkg/storage/driver/sql_test.go
Unescape View File

@ -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{

Write Preview
Loading…
Cancel
Save