diff --git a/pkg/storage/driver/mock_test.go b/pkg/storage/driver/mock_test.go index c366d106d..dc520264a 100644 --- a/pkg/storage/driver/mock_test.go +++ b/pkg/storage/driver/mock_test.go @@ -254,6 +254,13 @@ func (mock *MockSecretsInterface) Delete(_ context.Context, name string, _ metav // newTestFixtureSQL mocks the SQL database (for testing purposes) func newTestFixtureSQL(t *testing.T, _ ...*rspb.Release) (*SQL, sqlmock.Sqlmock) { + t.Helper() + return newTestFixtureSQLWithNamespace(t, "default") +} + +// newTestFixtureSQLWithNamespace is like newTestFixtureSQL but lets the caller configure the +// driver's namespace - in particular, pass "" to simulate all-namespaces mode. +func newTestFixtureSQLWithNamespace(t *testing.T, namespace string) (*SQL, sqlmock.Sqlmock) { t.Helper() sqlDB, mock, err := sqlmock.New() require.NoError(t, err, "error when opening stub database connection") @@ -261,7 +268,7 @@ func newTestFixtureSQL(t *testing.T, _ ...*rspb.Release) (*SQL, sqlmock.Sqlmock) sqlxDB := sqlx.NewDb(sqlDB, "sqlmock") return &SQL{ db: sqlxDB, - namespace: "default", + namespace: namespace, statementBuilder: sq.StatementBuilder.PlaceholderFormat(sq.Dollar), }, mock } diff --git a/pkg/storage/driver/sql.go b/pkg/storage/driver/sql.go index db076da98..4595f6342 100644 --- a/pkg/storage/driver/sql.go +++ b/pkg/storage/driver/sql.go @@ -475,7 +475,6 @@ func (s *SQL) Create(key string, rel release.Releaser) error { if namespace == "" { namespace = defaultNamespace } - s.namespace = namespace body, err := encodeRelease(rls) if err != nil { @@ -525,7 +524,7 @@ func (s *SQL) Create(key string, rel release.Releaser) error { Select(sqlReleaseTableKeyColumn). From(sqlReleaseTableName). Where(sq.Eq{sqlReleaseTableKeyColumn: key}). - Where(sq.Eq{sqlReleaseTableNamespaceColumn: s.namespace}). + Where(sq.Eq{sqlReleaseTableNamespaceColumn: namespace}). ToSql() if buildErr != nil { s.Logger().Debug("failed to build select query", "error", buildErr) @@ -585,7 +584,6 @@ func (s *SQL) Update(key string, rel release.Releaser) error { if namespace == "" { namespace = defaultNamespace } - s.namespace = namespace body, err := encodeRelease(rls) if err != nil { @@ -690,13 +688,13 @@ func (s *SQL) Delete(key string) (release.Releaser, error) { } // Get release custom labels from database -func (s *SQL) getReleaseCustomLabels(key string, _ string) (map[string]string, error) { +func (s *SQL) getReleaseCustomLabels(key string, namespace string) (map[string]string, error) { query, args, err := s.statementBuilder. Select(sqlCustomLabelsTableKeyColumn, sqlCustomLabelsTableValueColumn). From(sqlCustomLabelsTableName). Where(sq.Eq{ sqlCustomLabelsTableReleaseKeyColumn: key, - sqlCustomLabelsTableReleaseNamespaceColumn: s.namespace, + sqlCustomLabelsTableReleaseNamespaceColumn: namespace, }). ToSql() if err != nil { diff --git a/pkg/storage/driver/sql_test.go b/pkg/storage/driver/sql_test.go index 044e9df7b..b5634c4d6 100644 --- a/pkg/storage/driver/sql_test.go +++ b/pkg/storage/driver/sql_test.go @@ -128,11 +128,13 @@ func TestSQLList(t *testing.T) { ) rows := mock.NewRows([]string{ + sqlReleaseTableKeyColumn, + sqlReleaseTableNamespaceColumn, sqlReleaseTableBodyColumn, }) for _, r := range releases { body, _ := encodeRelease(r) - rows.AddRow(body) + rows.AddRow(testKey(r.Name, r.Version), r.Namespace, body) } mock. ExpectQuery(regexp.QuoteMeta(query)). @@ -140,7 +142,7 @@ func TestSQLList(t *testing.T) { WillReturnRows(rows).RowsWillBeClosed() for _, r := range releases { - mockGetReleaseCustomLabels(mock, "", r.Namespace, r.Labels) + mockGetReleaseCustomLabels(mock, testKey(r.Name, r.Version), r.Namespace, r.Labels) } } @@ -178,6 +180,53 @@ func TestSQLList(t *testing.T) { require.Contains(t, rls.Labels, "key1", "Expected 'key1' label in results, actual %v", rls.Labels) } +// TestSqlListAllNamespacesReturnsCustomLabels is a regression test for helm/helm#32394 (item 7): +// getReleaseCustomLabels ignored the namespace argument passed to it by List/Query and used +// s.namespace instead. In all-namespaces mode (s.namespace == ""), this meant the custom-labels +// query filtered on an empty namespace and matched nothing, so every release listed across +// namespaces silently came back with empty custom labels. +func TestSqlListAllNamespacesReturnsCustomLabels(t *testing.T) { + sqlDriver, mock := newTestFixtureSQLWithNamespace(t, "") + + rel := releaseStub("smug-pigeon", 1, "team-a", common.StatusDeployed) + key := testKey(rel.Name, 1) + body, _ := encodeRelease(rel) + + listQuery := fmt.Sprintf( + "SELECT %s, %s, %s FROM %s WHERE %s = $1", + sqlReleaseTableKeyColumn, + sqlReleaseTableNamespaceColumn, + sqlReleaseTableBodyColumn, + sqlReleaseTableName, + sqlReleaseTableOwnerColumn, + ) + mock. + ExpectQuery(regexp.QuoteMeta(listQuery)). + WithArgs(sqlReleaseDefaultOwner). + WillReturnRows( + mock.NewRows([]string{ + sqlReleaseTableKeyColumn, + sqlReleaseTableNamespaceColumn, + sqlReleaseTableBodyColumn, + }).AddRow(key, rel.Namespace, body), + ) + + // The custom-labels lookup must be scoped to this release's own namespace ("team-a"), + // not the driver's all-namespaces configuration (""). mockGetReleaseCustomLabels asserts + // the query args match exactly, so this expectation fails if the fix regresses. + mockGetReleaseCustomLabels(mock, key, rel.Namespace, rel.Labels) + + releases, err := sqlDriver.List(func(release.Releaser) bool { return true }) + require.NoError(t, err) + require.Len(t, releases, 1) + + got := convertReleaserToV1(t, releases[0]) + for k, v := range filterSystemLabels(rel.Labels) { + assert.Equal(t, v, got.Labels[k], "custom label %q should be present when listing across all namespaces", k) + } + assert.NoErrorf(t, mock.ExpectationsWereMet(), "sql expectations weren't met") +} + func TestSqlCreate(t *testing.T) { vers := 1 name := "smug-pigeon" @@ -230,6 +279,147 @@ func TestSqlCreate(t *testing.T) { assert.NoErrorf(t, mock.ExpectationsWereMet(), "sql expectations weren't met") } +// TestSqlCreateDoesNotMutateDriverNamespace is a regression test for helm/helm#32394 (item 5): +// Create used to overwrite the driver's own namespace field with the namespace of whichever +// release it last wrote, so any later Get/List/Query call on the same driver instance would be +// silently scoped to that release's namespace instead of the namespace the driver was configured +// with. This is also a data race, since List reads s.namespace while Create writes it +// concurrently. The fix keeps the release's namespace as a local variable used only for that +// call's own queries, leaving the driver's configured namespace untouched. +func TestSqlCreateDoesNotMutateDriverNamespace(t *testing.T) { + vers := 1 + name := "smug-pigeon" + // The driver fixture below is configured for "default", but this release lives in a + // different namespace - simulating an SDK caller that shares one driver instance across + // namespaces. + otherNamespace := "other-ns" + key := testKey(name, vers) + rel := releaseStub(name, vers, otherNamespace, common.StatusDeployed) + + sqlDriver, mock := newTestFixtureSQL(t) + require.Equal(t, "default", sqlDriver.namespace, "fixture should be configured for the default namespace") + + 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, + ) + + mock.ExpectBegin() + mock. + ExpectExec(regexp.QuoteMeta(insertQuery)). + WithArgs(key, sqlReleaseDefaultType, body, rel.Name, otherNamespace, int(rel.Version), rel.Info.Status.String(), sqlReleaseDefaultOwner, recentUnixTimestamp()). + WillReturnResult(sqlmock.NewResult(1, 1)) + + labelsQuery := fmt.Sprintf( + "INSERT INTO %s (%s,%s,%s,%s) VALUES ($1,$2,$3,$4)", + sqlCustomLabelsTableName, + sqlCustomLabelsTableReleaseKeyColumn, + sqlCustomLabelsTableReleaseNamespaceColumn, + sqlCustomLabelsTableKeyColumn, + sqlCustomLabelsTableValueColumn, + ) + + mock.MatchExpectationsInOrder(false) + for k, v := range filterSystemLabels(rel.Labels) { + mock. + ExpectExec(regexp.QuoteMeta(labelsQuery)). + WithArgs(key, otherNamespace, k, v). + WillReturnResult(sqlmock.NewResult(1, 1)) + } + mock.ExpectCommit() + + require.NoErrorf(t, sqlDriver.Create(key, rel), "failed to create release with key %s", key) + assert.NoErrorf(t, mock.ExpectationsWereMet(), "sql expectations weren't met") + + assert.Equal(t, "default", sqlDriver.namespace, + "Create must not mutate the driver's configured namespace based on the release being created") +} + +// TestSqlCreateThenListUsesConfiguredNamespace is a black-box regression test for +// helm/helm#32394 (item 5): after creating a release in a namespace other than the driver's +// configured one, a subsequent List must still be scoped to the driver's configured namespace - +// not the namespace of whichever release was last created. Before the fix, Create overwrote +// s.namespace, so this List would have queried using "other-ns" instead of "default". +func TestSqlCreateThenListUsesConfiguredNamespace(t *testing.T) { + vers := 1 + name := "smug-pigeon" + otherNamespace := "other-ns" + key := testKey(name, vers) + rel := releaseStub(name, vers, otherNamespace, 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, + ) + mock.ExpectBegin() + mock. + ExpectExec(regexp.QuoteMeta(insertQuery)). + WithArgs(key, sqlReleaseDefaultType, body, rel.Name, otherNamespace, int(rel.Version), rel.Info.Status.String(), sqlReleaseDefaultOwner, recentUnixTimestamp()). + WillReturnResult(sqlmock.NewResult(1, 1)) + + labelsQuery := fmt.Sprintf( + "INSERT INTO %s (%s,%s,%s,%s) VALUES ($1,$2,$3,$4)", + sqlCustomLabelsTableName, + sqlCustomLabelsTableReleaseKeyColumn, + sqlCustomLabelsTableReleaseNamespaceColumn, + sqlCustomLabelsTableKeyColumn, + sqlCustomLabelsTableValueColumn, + ) + mock.MatchExpectationsInOrder(false) + for k, v := range filterSystemLabels(rel.Labels) { + mock. + ExpectExec(regexp.QuoteMeta(labelsQuery)). + WithArgs(key, otherNamespace, k, v). + WillReturnResult(sqlmock.NewResult(1, 1)) + } + mock.ExpectCommit() + + require.NoErrorf(t, sqlDriver.Create(key, rel), "failed to create release with key %s", key) + + // Now List - this must be scoped to "default" (the driver's configured namespace), not + // "other-ns" (the namespace of the release we just created). + listQuery := fmt.Sprintf( + "SELECT %s, %s, %s FROM %s WHERE %s = $1 AND %s = $2", + sqlReleaseTableKeyColumn, + sqlReleaseTableNamespaceColumn, + sqlReleaseTableBodyColumn, + sqlReleaseTableName, + sqlReleaseTableOwnerColumn, + sqlReleaseTableNamespaceColumn, + ) + mock. + ExpectQuery(regexp.QuoteMeta(listQuery)). + WithArgs(sqlReleaseDefaultOwner, "default"). + WillReturnRows(mock.NewRows([]string{sqlReleaseTableBodyColumn})) + + _, err := sqlDriver.List(func(release.Releaser) bool { return true }) + require.NoError(t, err, "List must query using the driver's configured namespace, not the last-created release's namespace") + assert.NoErrorf(t, mock.ExpectationsWereMet(), "sql expectations weren't met - List likely queried using the wrong namespace") +} + func TestSqlCreateAlreadyExists(t *testing.T) { vers := 1 name := "smug-pigeon" @@ -318,6 +508,45 @@ func TestSqlUpdate(t *testing.T) { assert.NoErrorf(t, mock.ExpectationsWereMet(), "sql expectations weren't met") } +// TestSqlUpdateDoesNotMutateDriverNamespace is a regression test for helm/helm#32394 (item 5): +// see TestSqlCreateDoesNotMutateDriverNamespace for the full rationale. Update had the same bug. +func TestSqlUpdateDoesNotMutateDriverNamespace(t *testing.T) { + vers := 1 + name := "smug-pigeon" + otherNamespace := "other-ns" + key := testKey(name, vers) + rel := releaseStub(name, vers, otherNamespace, common.StatusDeployed) + + sqlDriver, mock := newTestFixtureSQL(t) + require.Equal(t, "default", sqlDriver.namespace, "fixture should be configured for the default namespace") + + 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, + ) + + mock. + ExpectExec(regexp.QuoteMeta(query)). + WithArgs(body, rel.Name, int(rel.Version), rel.Info.Status.String(), sqlReleaseDefaultOwner, recentUnixTimestamp(), key, otherNamespace). + WillReturnResult(sqlmock.NewResult(0, 1)) + + 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") + + assert.Equal(t, "default", sqlDriver.namespace, + "Update must not mutate the driver's configured namespace based on the release being updated") +} + func TestSqlQuery(t *testing.T) { // Reflect actual use cases in ../storage.go labelSetUnknown := map[string]string{ @@ -360,6 +589,8 @@ func TestSqlQuery(t *testing.T) { WithArgs("smug-pigeon", sqlReleaseDefaultOwner, "unknown", "default"). WillReturnRows( mock.NewRows([]string{ + sqlReleaseTableKeyColumn, + sqlReleaseTableNamespaceColumn, sqlReleaseTableBodyColumn, }), ).RowsWillBeClosed() @@ -369,13 +600,15 @@ func TestSqlQuery(t *testing.T) { WithArgs("smug-pigeon", sqlReleaseDefaultOwner, "deployed", "default"). WillReturnRows( mock.NewRows([]string{ + sqlReleaseTableKeyColumn, + sqlReleaseTableNamespaceColumn, sqlReleaseTableBodyColumn, }).AddRow( - deployedReleaseBody, + testKey(deployedRelease.Name, deployedRelease.Version), deployedRelease.Namespace, deployedReleaseBody, ), ).RowsWillBeClosed() - mockGetReleaseCustomLabels(mock, "", deployedRelease.Namespace, deployedRelease.Labels) + mockGetReleaseCustomLabels(mock, testKey(deployedRelease.Name, deployedRelease.Version), deployedRelease.Namespace, deployedRelease.Labels) query = fmt.Sprintf( "SELECT %s, %s, %s FROM %s WHERE %s = $1 AND %s = $2 AND %s = $3", @@ -393,16 +626,18 @@ func TestSqlQuery(t *testing.T) { WithArgs("smug-pigeon", sqlReleaseDefaultOwner, "default"). WillReturnRows( mock.NewRows([]string{ + sqlReleaseTableKeyColumn, + sqlReleaseTableNamespaceColumn, sqlReleaseTableBodyColumn, }).AddRow( - supersededReleaseBody, + testKey(supersededRelease.Name, supersededRelease.Version), supersededRelease.Namespace, supersededReleaseBody, ).AddRow( - deployedReleaseBody, + testKey(deployedRelease.Name, deployedRelease.Version), deployedRelease.Namespace, deployedReleaseBody, ), ).RowsWillBeClosed() - mockGetReleaseCustomLabels(mock, "", supersededRelease.Namespace, supersededRelease.Labels) - mockGetReleaseCustomLabels(mock, "", deployedRelease.Namespace, deployedRelease.Labels) + mockGetReleaseCustomLabels(mock, testKey(supersededRelease.Name, supersededRelease.Version), supersededRelease.Namespace, supersededRelease.Labels) + mockGetReleaseCustomLabels(mock, testKey(deployedRelease.Name, deployedRelease.Version), deployedRelease.Namespace, deployedRelease.Labels) _, err := sqlDriver.Query(labelSetUnknown) require.Errorf(t, err, "Expected error {%v}, got nil", ErrReleaseNotFound)