From 98219c42cc12e34a6a7995c6c04235aca28f2d80 Mon Sep 17 00:00:00 2001 From: Weiyi Li <1093656961@qq.com> Date: Mon, 21 Sep 2026 13:31:47 +0000 Subject: [PATCH] fix(storage): return ErrReleaseNotFound when updating missing releases Map Kubernetes NotFound errors and detect zero-row SQL updates in both storage driver implementations. Preserve other failures and add regression coverage for the missing-release contract and update errors. Closes helm/helm#32673 Signed-off-by: Weiyi Li <1093656961@qq.com> --- internal/storage/driver/cfgmaps.go | 8 ++- internal/storage/driver/driver_test.go | 83 ++++++++++++++++++++++++++ internal/storage/driver/mock_test.go | 12 +++- internal/storage/driver/secrets.go | 8 ++- internal/storage/driver/sql.go | 11 +++- internal/storage/driver/sql_test.go | 58 +++++++++++++----- pkg/storage/driver/cfgmaps.go | 8 ++- pkg/storage/driver/driver_test.go | 83 ++++++++++++++++++++++++++ pkg/storage/driver/mock_test.go | 12 +++- pkg/storage/driver/secrets.go | 8 ++- pkg/storage/driver/sql.go | 11 +++- pkg/storage/driver/sql_test.go | 58 +++++++++++++----- 12 files changed, 314 insertions(+), 46 deletions(-) create mode 100644 internal/storage/driver/driver_test.go create mode 100644 pkg/storage/driver/driver_test.go diff --git a/internal/storage/driver/cfgmaps.go b/internal/storage/driver/cfgmaps.go index 73076fdfe..55beed976 100644 --- a/internal/storage/driver/cfgmaps.go +++ b/internal/storage/driver/cfgmaps.go @@ -196,8 +196,8 @@ func (cfgmaps *ConfigMaps) Create(key string, rls release.Releaser) error { return nil } -// Update updates the ConfigMap holding the release. If not found -// the ConfigMap is created to hold the release. +// Update updates the ConfigMap holding the release. If not found, +// ErrReleaseNotFound is returned. func (cfgmaps *ConfigMaps) Update(key string, rel release.Releaser) error { // set labels for configmaps object meta data var lbs labels @@ -224,6 +224,10 @@ func (cfgmaps *ConfigMaps) Update(key string, rel release.Releaser) error { // push the configmap object out into the kubiverse _, err = cfgmaps.impl.Update(context.Background(), obj, metav1.UpdateOptions{}) if err != nil { + if apierrors.IsNotFound(err) { + return ErrReleaseNotFound + } + cfgmaps.Logger().Debug("failed to update release", slog.Any("error", err)) return err } diff --git a/internal/storage/driver/driver_test.go b/internal/storage/driver/driver_test.go new file mode 100644 index 000000000..48c3112e2 --- /dev/null +++ b/internal/storage/driver/driver_test.go @@ -0,0 +1,83 @@ +/* +Copyright The Helm Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package driver + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/require" + v1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + + "helm.sh/helm/v4/pkg/release/common" +) + +func TestUpdateMissingRelease(t *testing.T) { + tests := []struct { + name string + newDriver func(*testing.T) Driver + }{ + {"memory", func(*testing.T) Driver { return NewMemory() }}, + {"configmaps", func(t *testing.T) Driver { + t.Helper() + return newTestFixtureCfgMaps(t) + }}, + {"secrets", func(t *testing.T) Driver { + t.Helper() + return newTestFixtureSecrets(t) + }}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + d := tt.newDriver(t) + rel := releaseStub("missing", 1, "default", common.StatusDeployed) + key := testKey(rel.Name, rel.Version) + require.ErrorIs(t, d.Update(key, rel), ErrReleaseNotFound) + // An update must not create the missing release. + _, err := d.Get(key) + require.ErrorIs(t, err, ErrReleaseNotFound) + }) + } +} + +func TestUpdateKubernetesErrors(t *testing.T) { + tests := []struct { + name string + err error + }{ + {"forbidden", apierrors.NewForbidden(v1.Resource("tests"), "test", errors.New("access denied"))}, + {"conflict", apierrors.NewConflict(v1.Resource("tests"), "test", errors.New("stale resource version"))}, + {"connection", errors.New("connection failed")}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + drivers := []Driver{ + NewConfigMaps(&MockConfigMapsInterface{updateError: tt.err}), + NewSecrets(&MockSecretsInterface{updateError: tt.err}), + } + for _, d := range drivers { + t.Run(d.Name(), func(t *testing.T) { + rel := releaseStub("test", 1, "default", common.StatusDeployed) + err := d.Update(testKey(rel.Name, rel.Version), rel) + require.ErrorIs(t, err, tt.err) + require.NotErrorIs(t, err, ErrReleaseNotFound) + }) + } + }) + } +} diff --git a/internal/storage/driver/mock_test.go b/internal/storage/driver/mock_test.go index a5eff0fd1..dcc3c51aa 100644 --- a/internal/storage/driver/mock_test.go +++ b/internal/storage/driver/mock_test.go @@ -94,7 +94,8 @@ func newTestFixtureCfgMaps(t *testing.T, releases ...*rspb.Release) *ConfigMaps type MockConfigMapsInterface struct { corev1.ConfigMapInterface - objects map[string]*v1.ConfigMap + objects map[string]*v1.ConfigMap + updateError error } // Init initializes the MockConfigMapsInterface with the set of releases. @@ -149,6 +150,9 @@ func (mock *MockConfigMapsInterface) Create(_ context.Context, cfgmap *v1.Config // Update updates a ConfigMap. func (mock *MockConfigMapsInterface) Update(_ context.Context, cfgmap *v1.ConfigMap, _ metav1.UpdateOptions) (*v1.ConfigMap, error) { + if mock.updateError != nil { + return nil, mock.updateError + } name := cfgmap.Name if _, ok := mock.objects[name]; !ok { return nil, apierrors.NewNotFound(v1.Resource("tests"), name) @@ -180,7 +184,8 @@ func newTestFixtureSecrets(t *testing.T, releases ...*rspb.Release) *Secrets { type MockSecretsInterface struct { corev1.SecretInterface - objects map[string]*v1.Secret + objects map[string]*v1.Secret + updateError error } // Init initializes the MockSecretsInterface with the set of releases. @@ -235,6 +240,9 @@ func (mock *MockSecretsInterface) Create(_ context.Context, secret *v1.Secret, _ // Update updates a Secret. func (mock *MockSecretsInterface) Update(_ context.Context, secret *v1.Secret, _ metav1.UpdateOptions) (*v1.Secret, error) { + if mock.updateError != nil { + return nil, mock.updateError + } name := secret.Name if _, ok := mock.objects[name]; !ok { return nil, apierrors.NewNotFound(v1.Resource("tests"), name) diff --git a/internal/storage/driver/secrets.go b/internal/storage/driver/secrets.go index e12aa2c1d..60f22361d 100644 --- a/internal/storage/driver/secrets.go +++ b/internal/storage/driver/secrets.go @@ -189,8 +189,8 @@ func (secrets *Secrets) Create(key string, rel release.Releaser) error { return nil } -// Update updates the Secret holding the release. If not found -// the Secret is created to hold the release. +// Update updates the Secret holding the release. If not found, +// ErrReleaseNotFound is returned. func (secrets *Secrets) Update(key string, rel release.Releaser) error { // set labels for secrets object meta data var lbs labels @@ -212,6 +212,10 @@ func (secrets *Secrets) Update(key string, rel release.Releaser) error { // push the secret object out into the kubiverse _, err = secrets.impl.Update(context.Background(), obj, metav1.UpdateOptions{}) if err != nil { + if apierrors.IsNotFound(err) { + return ErrReleaseNotFound + } + return fmt.Errorf("update: failed to update: %w", err) } return nil diff --git a/internal/storage/driver/sql.go b/internal/storage/driver/sql.go index 653507ba9..80c5acb05 100644 --- a/internal/storage/driver/sql.go +++ b/internal/storage/driver/sql.go @@ -624,11 +624,20 @@ func (s *SQL) Update(key string, rel release.Releaser) error { return err } - if _, err := s.db.Exec(query, args...); err != nil { + result, err := s.db.Exec(query, args...) + if err != nil { s.Logger().Debug("failed to update release in SQL database", slog.String("key", key), slog.Any("error", err)) return err } + rows, err := result.RowsAffected() + if err != nil { + return err + } + if rows == 0 { + return ErrReleaseNotFound + } + return nil } diff --git a/internal/storage/driver/sql_test.go b/internal/storage/driver/sql_test.go index 99c323680..05d44d2e1 100644 --- a/internal/storage/driver/sql_test.go +++ b/internal/storage/driver/sql_test.go @@ -287,15 +287,19 @@ func TestSqlCreateAlreadyExists(t *testing.T) { } func TestSqlUpdate(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) - + execErr := errors.New("database unavailable") + rowsErr := errors.New("rows affected unavailable") + tests := []struct { + name string + result driver.Result + execErr error + wantErr error + }{ + {"updated", sqlmock.NewResult(0, 1), nil, nil}, + {"missing", sqlmock.NewResult(0, 0), nil, ErrReleaseNotFound}, + {"execution error", nil, execErr, execErr}, + {"rows affected error", sqlmock.NewErrorResult(rowsErr), nil, rowsErr}, + } query := fmt.Sprintf( "UPDATE %s SET %s = $1, %s = $2, %s = $3, %s = $4, %s = $5, %s = $6, %s = $7 WHERE %s = $8 AND %s = $9", sqlReleaseTableName, @@ -310,13 +314,35 @@ func TestSqlUpdate(t *testing.T) { sqlReleaseTableNamespaceColumn, ) - mock. - ExpectExec(regexp.QuoteMeta(query)). - WithArgs(body, rel.Name, int(rel.Version), rel.Info.Status.String(), sqlReleaseDefaultOwner, sqlReleaseDefaultType, recentUnixTimestamp(), key, namespace). - 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") + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + name := "smug-pigeon" + namespace := "default" + key := testKey(name, 1) + rel := releaseStub(name, 1, namespace, common.StatusDeployed) + sqlDriver, mock := newTestFixtureSQL(t) + body, err := encodeRelease(rel) + require.NoError(t, err) + + expectation := mock.ExpectExec(regexp.QuoteMeta(query)).WithArgs(body, rel.Name, int(rel.Version), rel.Info.Status.String(), sqlReleaseDefaultOwner, sqlReleaseDefaultType, recentUnixTimestamp(), key, namespace) + if tt.execErr != nil { + expectation.WillReturnError(tt.execErr) + } else { + expectation.WillReturnResult(tt.result) + } + + err = sqlDriver.Update(key, rel) + if tt.wantErr != nil { + require.ErrorIs(t, err, tt.wantErr) + if !errors.Is(tt.wantErr, ErrReleaseNotFound) { + require.NotErrorIs(t, err, ErrReleaseNotFound) + } + } else { + require.NoError(t, err) + } + require.NoError(t, mock.ExpectationsWereMet()) + }) + } } func TestSqlQuery(t *testing.T) { diff --git a/pkg/storage/driver/cfgmaps.go b/pkg/storage/driver/cfgmaps.go index f71ce44f1..f460926b9 100644 --- a/pkg/storage/driver/cfgmaps.go +++ b/pkg/storage/driver/cfgmaps.go @@ -196,8 +196,8 @@ func (cfgmaps *ConfigMaps) Create(key string, rls release.Releaser) error { return nil } -// Update updates the ConfigMap holding the release. If not found -// the ConfigMap is created to hold the release. +// Update updates the ConfigMap holding the release. If not found, +// ErrReleaseNotFound is returned. func (cfgmaps *ConfigMaps) Update(key string, rel release.Releaser) error { // set labels for configmaps object meta data var lbs labels @@ -224,6 +224,10 @@ func (cfgmaps *ConfigMaps) Update(key string, rel release.Releaser) error { // push the configmap object out into the kubiverse _, err = cfgmaps.impl.Update(context.Background(), obj, metav1.UpdateOptions{}) if err != nil { + if apierrors.IsNotFound(err) { + return ErrReleaseNotFound + } + cfgmaps.Logger().Debug("failed to update release", slog.Any("error", err)) return err } diff --git a/pkg/storage/driver/driver_test.go b/pkg/storage/driver/driver_test.go new file mode 100644 index 000000000..48c3112e2 --- /dev/null +++ b/pkg/storage/driver/driver_test.go @@ -0,0 +1,83 @@ +/* +Copyright The Helm Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package driver + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/require" + v1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + + "helm.sh/helm/v4/pkg/release/common" +) + +func TestUpdateMissingRelease(t *testing.T) { + tests := []struct { + name string + newDriver func(*testing.T) Driver + }{ + {"memory", func(*testing.T) Driver { return NewMemory() }}, + {"configmaps", func(t *testing.T) Driver { + t.Helper() + return newTestFixtureCfgMaps(t) + }}, + {"secrets", func(t *testing.T) Driver { + t.Helper() + return newTestFixtureSecrets(t) + }}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + d := tt.newDriver(t) + rel := releaseStub("missing", 1, "default", common.StatusDeployed) + key := testKey(rel.Name, rel.Version) + require.ErrorIs(t, d.Update(key, rel), ErrReleaseNotFound) + // An update must not create the missing release. + _, err := d.Get(key) + require.ErrorIs(t, err, ErrReleaseNotFound) + }) + } +} + +func TestUpdateKubernetesErrors(t *testing.T) { + tests := []struct { + name string + err error + }{ + {"forbidden", apierrors.NewForbidden(v1.Resource("tests"), "test", errors.New("access denied"))}, + {"conflict", apierrors.NewConflict(v1.Resource("tests"), "test", errors.New("stale resource version"))}, + {"connection", errors.New("connection failed")}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + drivers := []Driver{ + NewConfigMaps(&MockConfigMapsInterface{updateError: tt.err}), + NewSecrets(&MockSecretsInterface{updateError: tt.err}), + } + for _, d := range drivers { + t.Run(d.Name(), func(t *testing.T) { + rel := releaseStub("test", 1, "default", common.StatusDeployed) + err := d.Update(testKey(rel.Name, rel.Version), rel) + require.ErrorIs(t, err, tt.err) + require.NotErrorIs(t, err, ErrReleaseNotFound) + }) + } + }) + } +} diff --git a/pkg/storage/driver/mock_test.go b/pkg/storage/driver/mock_test.go index c366d106d..d6676552d 100644 --- a/pkg/storage/driver/mock_test.go +++ b/pkg/storage/driver/mock_test.go @@ -94,7 +94,8 @@ func newTestFixtureCfgMaps(t *testing.T, releases ...*rspb.Release) *ConfigMaps type MockConfigMapsInterface struct { corev1.ConfigMapInterface - objects map[string]*v1.ConfigMap + objects map[string]*v1.ConfigMap + updateError error } // Init initializes the MockConfigMapsInterface with the set of releases. @@ -149,6 +150,9 @@ func (mock *MockConfigMapsInterface) Create(_ context.Context, cfgmap *v1.Config // Update updates a ConfigMap. func (mock *MockConfigMapsInterface) Update(_ context.Context, cfgmap *v1.ConfigMap, _ metav1.UpdateOptions) (*v1.ConfigMap, error) { + if mock.updateError != nil { + return nil, mock.updateError + } name := cfgmap.Name if _, ok := mock.objects[name]; !ok { return nil, apierrors.NewNotFound(v1.Resource("tests"), name) @@ -180,7 +184,8 @@ func newTestFixtureSecrets(t *testing.T, releases ...*rspb.Release) *Secrets { type MockSecretsInterface struct { corev1.SecretInterface - objects map[string]*v1.Secret + objects map[string]*v1.Secret + updateError error } // Init initializes the MockSecretsInterface with the set of releases. @@ -235,6 +240,9 @@ func (mock *MockSecretsInterface) Create(_ context.Context, secret *v1.Secret, _ // Update updates a Secret. func (mock *MockSecretsInterface) Update(_ context.Context, secret *v1.Secret, _ metav1.UpdateOptions) (*v1.Secret, error) { + if mock.updateError != nil { + return nil, mock.updateError + } name := secret.Name if _, ok := mock.objects[name]; !ok { return nil, apierrors.NewNotFound(v1.Resource("tests"), name) diff --git a/pkg/storage/driver/secrets.go b/pkg/storage/driver/secrets.go index a1f3e94fc..3f67da701 100644 --- a/pkg/storage/driver/secrets.go +++ b/pkg/storage/driver/secrets.go @@ -189,8 +189,8 @@ func (secrets *Secrets) Create(key string, rel release.Releaser) error { return nil } -// Update updates the Secret holding the release. If not found -// the Secret is created to hold the release. +// Update updates the Secret holding the release. If not found, +// ErrReleaseNotFound is returned. func (secrets *Secrets) Update(key string, rel release.Releaser) error { // set labels for secrets object meta data var lbs labels @@ -212,6 +212,10 @@ func (secrets *Secrets) Update(key string, rel release.Releaser) error { // push the secret object out into the kubiverse _, err = secrets.impl.Update(context.Background(), obj, metav1.UpdateOptions{}) if err != nil { + if apierrors.IsNotFound(err) { + return ErrReleaseNotFound + } + return fmt.Errorf("update: failed to update: %w", err) } return nil diff --git a/pkg/storage/driver/sql.go b/pkg/storage/driver/sql.go index 2b278f7cb..ceee7274f 100644 --- a/pkg/storage/driver/sql.go +++ b/pkg/storage/driver/sql.go @@ -623,11 +623,20 @@ func (s *SQL) Update(key string, rel release.Releaser) error { return err } - if _, err := s.db.Exec(query, args...); err != nil { + result, err := s.db.Exec(query, args...) + if err != nil { s.Logger().Debug("failed to update release in SQL database", slog.String("key", key), slog.Any("error", err)) return err } + rows, err := result.RowsAffected() + if err != nil { + return err + } + if rows == 0 { + return ErrReleaseNotFound + } + return nil } diff --git a/pkg/storage/driver/sql_test.go b/pkg/storage/driver/sql_test.go index e5fde405b..171163f3f 100644 --- a/pkg/storage/driver/sql_test.go +++ b/pkg/storage/driver/sql_test.go @@ -287,15 +287,19 @@ func TestSqlCreateAlreadyExists(t *testing.T) { } func TestSqlUpdate(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) - + execErr := errors.New("database unavailable") + rowsErr := errors.New("rows affected unavailable") + tests := []struct { + name string + result driver.Result + execErr error + wantErr error + }{ + {"updated", sqlmock.NewResult(0, 1), nil, nil}, + {"missing", sqlmock.NewResult(0, 0), nil, ErrReleaseNotFound}, + {"execution error", nil, execErr, execErr}, + {"rows affected error", sqlmock.NewErrorResult(rowsErr), nil, rowsErr}, + } 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, @@ -309,13 +313,35 @@ func TestSqlUpdate(t *testing.T) { sqlReleaseTableNamespaceColumn, ) - 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)) - - 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") + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + name := "smug-pigeon" + namespace := "default" + key := testKey(name, 1) + rel := releaseStub(name, 1, namespace, common.StatusDeployed) + sqlDriver, mock := newTestFixtureSQL(t) + body, err := encodeRelease(rel) + require.NoError(t, err) + + expectation := mock.ExpectExec(regexp.QuoteMeta(query)).WithArgs(body, rel.Name, int(rel.Version), rel.Info.Status.String(), sqlReleaseDefaultOwner, recentUnixTimestamp(), key, namespace) + if tt.execErr != nil { + expectation.WillReturnError(tt.execErr) + } else { + expectation.WillReturnResult(tt.result) + } + + err = sqlDriver.Update(key, rel) + if tt.wantErr != nil { + require.ErrorIs(t, err, tt.wantErr) + if !errors.Is(tt.wantErr, ErrReleaseNotFound) { + require.NotErrorIs(t, err, ErrReleaseNotFound) + } + } else { + require.NoError(t, err) + } + require.NoError(t, mock.ExpectationsWereMet()) + }) + } } func TestSqlQuery(t *testing.T) {