From f6cc4a124e8f7be884798537c9ad23071cc65cf8 Mon Sep 17 00:00:00 2001 From: Weiyi Li <1093656961@qq.com> Date: Wed, 23 Sep 2026 01:37:18 +0800 Subject: [PATCH] fix(storage): stop the memory driver from sharing stored releases The memory driver handed out pointers into its own cache, so callers could change stored releases without an Update. The persistent drivers encode on write and decode on read, so they never expose stored data. Copy releases when storing them and when returning them from Get, List and Query, so the memory driver offers the same isolation. List copies before calling the filter, matching the persistent drivers, which decode each release before filtering. Closes helm/helm#11304 Signed-off-by: Weiyi Li <1093656961@qq.com> --- internal/storage/driver/copy.go | 223 +++++++++++++++++++++++++ internal/storage/driver/copy_test.go | 114 +++++++++++++ internal/storage/driver/memory.go | 11 +- internal/storage/driver/memory_test.go | 79 +++++++++ internal/storage/driver/records.go | 4 +- pkg/storage/driver/copy.go | 223 +++++++++++++++++++++++++ pkg/storage/driver/copy_test.go | 114 +++++++++++++ pkg/storage/driver/memory.go | 11 +- pkg/storage/driver/memory_test.go | 79 +++++++++ pkg/storage/driver/records.go | 5 +- 10 files changed, 852 insertions(+), 11 deletions(-) create mode 100644 internal/storage/driver/copy.go create mode 100644 internal/storage/driver/copy_test.go create mode 100644 pkg/storage/driver/copy.go create mode 100644 pkg/storage/driver/copy_test.go diff --git a/internal/storage/driver/copy.go b/internal/storage/driver/copy.go new file mode 100644 index 000000000..1aa965be1 --- /dev/null +++ b/internal/storage/driver/copy.go @@ -0,0 +1,223 @@ +/* +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 ( + "bytes" + "maps" + "slices" + + "k8s.io/apimachinery/pkg/runtime" + + chart "helm.sh/helm/v4/internal/chart/v3" + rspb "helm.sh/helm/v4/internal/release/v2" + "helm.sh/helm/v4/pkg/chart/common" +) + +// copyRelease returns a deep copy of rls. +// +// The persistent drivers serialize releases on write and deserialize them on +// read, so callers can never reach the stored data. The memory driver keeps +// releases as Go values, and therefore has to copy them explicitly to offer the +// same isolation. +// +// Values held in the release that Helm does not model, such as custom types +// stored in Config, are copied as interface values rather than recursively. +func copyRelease(rls *rspb.Release) *rspb.Release { + if rls == nil { + return nil + } + return &rspb.Release{ + Name: rls.Name, + Info: copyInfo(rls.Info), + Chart: copyChart(rls.Chart), + Config: copyValues(rls.Config), + Manifest: rls.Manifest, + Hooks: copyHooks(rls.Hooks), + Version: rls.Version, + Namespace: rls.Namespace, + Labels: maps.Clone(rls.Labels), + ApplyMethod: rls.ApplyMethod, + } +} + +func copyInfo(info *rspb.Info) *rspb.Info { + if info == nil { + return nil + } + out := *info + if info.Resources != nil { + out.Resources = make(map[string][]runtime.Object, len(info.Resources)) + for name, objs := range info.Resources { + copied := make([]runtime.Object, len(objs)) + for i, obj := range objs { + if obj != nil { + copied[i] = obj.DeepCopyObject() + } + } + out.Resources[name] = copied + } + } + return &out +} + +func copyHooks(hooks []*rspb.Hook) []*rspb.Hook { + if hooks == nil { + return nil + } + out := make([]*rspb.Hook, len(hooks)) + for i, hook := range hooks { + if hook == nil { + continue + } + copied := *hook + copied.Events = slices.Clone(hook.Events) + copied.DeletePolicies = slices.Clone(hook.DeletePolicies) + copied.OutputLogPolicies = slices.Clone(hook.OutputLogPolicies) + out[i] = &copied + } + return out +} + +func copyChart(ch *chart.Chart) *chart.Chart { + if ch == nil { + return nil + } + out := &chart.Chart{ + Raw: copyFiles(ch.Raw), + Metadata: copyMetadata(ch.Metadata), + Lock: copyLock(ch.Lock), + Templates: copyFiles(ch.Templates), + Values: copyValues(ch.Values), + Schema: bytes.Clone(ch.Schema), + SchemaModTime: ch.SchemaModTime, + Files: copyFiles(ch.Files), + ModTime: ch.ModTime, + } + // AddDependency sets the parent of each dependency, which rebuilds the + // chart tree without reaching into the unexported fields. + for _, dep := range ch.Dependencies() { + out.AddDependency(copyChart(dep)) + } + return out +} + +func copyMetadata(md *chart.Metadata) *chart.Metadata { + if md == nil { + return nil + } + out := *md + out.Sources = slices.Clone(md.Sources) + out.Keywords = slices.Clone(md.Keywords) + out.Annotations = maps.Clone(md.Annotations) + if md.Maintainers != nil { + out.Maintainers = make([]*chart.Maintainer, len(md.Maintainers)) + for i, m := range md.Maintainers { + if m == nil { + continue + } + copied := *m + out.Maintainers[i] = &copied + } + } + out.Dependencies = copyDependencies(md.Dependencies) + return &out +} + +func copyLock(lock *chart.Lock) *chart.Lock { + if lock == nil { + return nil + } + out := *lock + out.Dependencies = copyDependencies(lock.Dependencies) + return &out +} + +func copyDependencies(deps []*chart.Dependency) []*chart.Dependency { + if deps == nil { + return nil + } + out := make([]*chart.Dependency, len(deps)) + for i, dep := range deps { + if dep == nil { + continue + } + copied := *dep + copied.Tags = slices.Clone(dep.Tags) + copied.ImportValues = copyValueSlice(dep.ImportValues) + out[i] = &copied + } + return out +} + +func copyFiles(files []*common.File) []*common.File { + if files == nil { + return nil + } + out := make([]*common.File, len(files)) + for i, file := range files { + if file == nil { + continue + } + copied := *file + copied.Data = bytes.Clone(file.Data) + out[i] = &copied + } + return out +} + +// copyValues deep copies the maps and slices that make up decoded chart values +// and release config. Other values are copied as interface values, which is +// enough for the scalars these maps normally hold. +func copyValues(values map[string]any) map[string]any { + if values == nil { + return nil + } + out := make(map[string]any, len(values)) + for key, value := range values { + out[key] = copyValue(value) + } + return out +} + +func copyValue(value any) any { + switch typed := value.(type) { + case map[string]any: + return copyValues(typed) + case map[any]any: + out := make(map[any]any, len(typed)) + for key, nested := range typed { + out[key] = copyValue(nested) + } + return out + case []any: + return copyValueSlice(typed) + default: + return value + } +} + +func copyValueSlice(values []any) []any { + if values == nil { + return nil + } + out := make([]any, len(values)) + for i, value := range values { + out[i] = copyValue(value) + } + return out +} diff --git a/internal/storage/driver/copy_test.go b/internal/storage/driver/copy_test.go new file mode 100644 index 000000000..fb4e64eac --- /dev/null +++ b/internal/storage/driver/copy_test.go @@ -0,0 +1,114 @@ +/* +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 ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + chart "helm.sh/helm/v4/internal/chart/v3" + rspb "helm.sh/helm/v4/internal/release/v2" + "helm.sh/helm/v4/pkg/chart/common" + rcommon "helm.sh/helm/v4/pkg/release/common" +) + +// chartFixture builds a chart with a dependency, raw files and nested values so +// the copy can be checked against the parts a JSON round-trip would drop. +func chartFixture() *chart.Chart { + ch := &chart.Chart{ + Metadata: &chart.Metadata{ + Name: "parent", + Annotations: map[string]string{"example.com/team": "helm"}, + Maintainers: []*chart.Maintainer{{Name: "maintainer"}}, + }, + Raw: []*common.File{{Name: "Chart.yaml", Data: []byte("name: parent")}}, + Templates: []*common.File{{Name: "templates/cm.yaml", Data: []byte("kind: ConfigMap")}}, + Values: map[string]any{"nested": map[string]any{"replicas": 1}}, + Schema: []byte("schema-bytes"), + } + ch.AddDependency(&chart.Chart{Metadata: &chart.Metadata{Name: "child"}}) + return ch +} + +func releaseFixture() *rspb.Release { + rls := releaseStub("copy-me", 1, "default", rcommon.StatusDeployed) + rls.Chart = chartFixture() + rls.Config = map[string]any{"nested": map[string]any{"enabled": true}} + rls.Hooks = []*rspb.Hook{{ + Name: "pre-install", + Events: []rspb.HookEvent{rspb.HookPreInstall}, + }} + return rls +} + +func TestCopyReleaseIsIndependent(t *testing.T) { + rls := releaseFixture() + copied := copyRelease(rls) + + require.NotSame(t, rls, copied) + assert.Equal(t, rls, copied, "a copy should hold the same data as the original") + + copied.Info.Status = rcommon.StatusFailed + copied.Labels["key1"] = "changed" + copied.Config["nested"].(map[string]any)["enabled"] = false + copied.Chart.Metadata.Name = "renamed" + copied.Chart.Metadata.Annotations["example.com/team"] = "changed" + copied.Chart.Metadata.Maintainers[0].Name = "changed" + copied.Chart.Values["nested"].(map[string]any)["replicas"] = 2 + copied.Chart.Raw[0].Data[0] = 'X' + copied.Chart.Schema[0] = 'X' + copied.Chart.Dependencies()[0].Metadata.Name = "changed" + copied.Hooks[0].Events[0] = rspb.HookPostInstall + + assert.Equal(t, rcommon.StatusDeployed, rls.Info.Status) + assert.Equal(t, "val1", rls.Labels["key1"]) + assert.Equal(t, true, rls.Config["nested"].(map[string]any)["enabled"]) + assert.Equal(t, "parent", rls.Chart.Metadata.Name) + assert.Equal(t, "helm", rls.Chart.Metadata.Annotations["example.com/team"]) + assert.Equal(t, "maintainer", rls.Chart.Metadata.Maintainers[0].Name) + assert.Equal(t, 1, rls.Chart.Values["nested"].(map[string]any)["replicas"]) + assert.Equal(t, []byte("name: parent"), rls.Chart.Raw[0].Data) + assert.Equal(t, []byte("schema-bytes"), rls.Chart.Schema) + assert.Equal(t, "child", rls.Chart.Dependencies()[0].Metadata.Name) + assert.Equal(t, rspb.HookPreInstall, rls.Hooks[0].Events[0]) +} + +// The chart tree is rebuilt through AddDependency, so the copied dependency has +// to point at the copied parent rather than the original one. +func TestCopyReleaseRebuildsChartTree(t *testing.T) { + copied := copyRelease(releaseFixture()) + + require.Len(t, copied.Chart.Dependencies(), 1) + dep := copied.Chart.Dependencies()[0] + assert.Same(t, copied.Chart, dep.Parent()) + assert.True(t, copied.Chart.IsRoot()) + assert.Equal(t, "parent.child", dep.ChartPath()) +} + +func TestCopyReleaseKeepsNilFields(t *testing.T) { + copied := copyRelease(&rspb.Release{Name: "sparse"}) + + assert.Equal(t, "sparse", copied.Name) + assert.Nil(t, copied.Info) + assert.Nil(t, copied.Chart) + assert.Nil(t, copied.Config) + assert.Nil(t, copied.Hooks) + assert.Nil(t, copied.Labels) + assert.Nil(t, copyRelease(nil)) +} diff --git a/internal/storage/driver/memory.go b/internal/storage/driver/memory.go index 7ea4a014a..4e4746acf 100644 --- a/internal/storage/driver/memory.go +++ b/internal/storage/driver/memory.go @@ -79,7 +79,7 @@ func (mem *Memory) Get(key string) (release.Releaser, error) { } if recs, ok := mem.cache[mem.namespace][name]; ok { if r := recs.Get(key); r != nil { - return r.rls, nil + return copyRelease(r.rls), nil } } return nil, ErrReleaseNotFound @@ -100,8 +100,11 @@ func (mem *Memory) List(filter func(release.Releaser) bool) ([]release.Releaser, } for _, recs := range mem.cache[namespace] { recs.Iter(func(_ int, rec *record) bool { - if filter(rec.rls) { - ls = append(ls, rec.rls) + // Copy before filtering so the callback cannot reach the + // stored release either. + rls := copyRelease(rec.rls) + if filter(rls) { + ls = append(ls, rls) } return true }) @@ -137,7 +140,7 @@ func (mem *Memory) Query(keyvals map[string]string) ([]release.Releaser, error) return false } if rec.lbs.match(lbs) { - ls = append(ls, rec.rls) + ls = append(ls, copyRelease(rec.rls)) } return true }) diff --git a/internal/storage/driver/memory_test.go b/internal/storage/driver/memory_test.go index 82f605e98..a41cd8f05 100644 --- a/internal/storage/driver/memory_test.go +++ b/internal/storage/driver/memory_test.go @@ -264,3 +264,82 @@ func TestMemoryDelete(t *testing.T) { } } } + +// A stored release should only change through Update, which is the contract the +// persistent drivers already provide by encoding on write and decoding on read. +func TestMemoryReleaseIsolation(t *testing.T) { + key := testKey("copy-me", 1) + tests := []struct { + desc string + mutate func(t *testing.T, mem *Memory, rls *rspb.Release) + }{ + { + desc: "create input", + mutate: func(_ *testing.T, _ *Memory, rls *rspb.Release) { + rls.Info.Status = common.StatusFailed + }, + }, + { + desc: "update input", + mutate: func(t *testing.T, mem *Memory, rls *rspb.Release) { + t.Helper() + require.NoError(t, mem.Update(key, rls)) + rls.Info.Status = common.StatusFailed + }, + }, + { + desc: "get result", + mutate: func(t *testing.T, mem *Memory, _ *rspb.Release) { + t.Helper() + rel, err := mem.Get(key) + require.NoError(t, err) + convertReleaserToV1(t, rel).Info.Status = common.StatusFailed + }, + }, + { + desc: "list result", + mutate: func(t *testing.T, mem *Memory, _ *rspb.Release) { + t.Helper() + ls, err := mem.List(func(release.Releaser) bool { return true }) + require.NoError(t, err) + require.Len(t, ls, 1) + convertReleaserToV1(t, ls[0]).Info.Status = common.StatusFailed + }, + }, + { + desc: "list filter argument", + mutate: func(t *testing.T, mem *Memory, _ *rspb.Release) { + t.Helper() + _, err := mem.List(func(rel release.Releaser) bool { + convertReleaserToV1(t, rel).Info.Status = common.StatusFailed + return false + }) + require.NoError(t, err) + }, + }, + { + desc: "query result", + mutate: func(t *testing.T, mem *Memory, _ *rspb.Release) { + t.Helper() + ls, err := mem.Query(map[string]string{"name": "copy-me"}) + require.NoError(t, err) + require.Len(t, ls, 1) + convertReleaserToV1(t, ls[0]).Info.Status = common.StatusFailed + }, + }, + } + + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { + mem := NewMemory() + rls := releaseFixture() + require.NoError(t, mem.Create(key, rls)) + + tt.mutate(t, mem, rls) + + stored, err := mem.Get(key) + require.NoError(t, err) + assert.Equal(t, common.StatusDeployed, convertReleaserToV1(t, stored).Info.Status) + }) + } +} diff --git a/internal/storage/driver/records.go b/internal/storage/driver/records.go index 14f13780d..0aca081a6 100644 --- a/internal/storage/driver/records.go +++ b/internal/storage/driver/records.go @@ -123,5 +123,7 @@ func newRecord(key string, rls *rspb.Release) *record { lbs.set("status", rls.Info.Status.String()) lbs.set("version", strconv.Itoa(rls.Version)) - return &record{key: key, lbs: lbs, rls: rls} + // Store a copy so later changes to the caller's release do not reach the + // record, matching the isolation the persistent drivers get from encoding. + return &record{key: key, lbs: lbs, rls: copyRelease(rls)} } diff --git a/pkg/storage/driver/copy.go b/pkg/storage/driver/copy.go new file mode 100644 index 000000000..6ee2c6514 --- /dev/null +++ b/pkg/storage/driver/copy.go @@ -0,0 +1,223 @@ +/* +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 ( + "bytes" + "maps" + "slices" + + "k8s.io/apimachinery/pkg/runtime" + + "helm.sh/helm/v4/pkg/chart/common" + chart "helm.sh/helm/v4/pkg/chart/v2" + rspb "helm.sh/helm/v4/pkg/release/v1" +) + +// copyRelease returns a deep copy of rls. +// +// The persistent drivers serialize releases on write and deserialize them on +// read, so callers can never reach the stored data. The memory driver keeps +// releases as Go values, and therefore has to copy them explicitly to offer the +// same isolation. +// +// Values held in the release that Helm does not model, such as custom types +// stored in Config, are copied as interface values rather than recursively. +func copyRelease(rls *rspb.Release) *rspb.Release { + if rls == nil { + return nil + } + return &rspb.Release{ + Name: rls.Name, + Info: copyInfo(rls.Info), + Chart: copyChart(rls.Chart), + Config: copyValues(rls.Config), + Manifest: rls.Manifest, + Hooks: copyHooks(rls.Hooks), + Version: rls.Version, + Namespace: rls.Namespace, + Labels: maps.Clone(rls.Labels), + ApplyMethod: rls.ApplyMethod, + } +} + +func copyInfo(info *rspb.Info) *rspb.Info { + if info == nil { + return nil + } + out := *info + if info.Resources != nil { + out.Resources = make(map[string][]runtime.Object, len(info.Resources)) + for name, objs := range info.Resources { + copied := make([]runtime.Object, len(objs)) + for i, obj := range objs { + if obj != nil { + copied[i] = obj.DeepCopyObject() + } + } + out.Resources[name] = copied + } + } + return &out +} + +func copyHooks(hooks []*rspb.Hook) []*rspb.Hook { + if hooks == nil { + return nil + } + out := make([]*rspb.Hook, len(hooks)) + for i, hook := range hooks { + if hook == nil { + continue + } + copied := *hook + copied.Events = slices.Clone(hook.Events) + copied.DeletePolicies = slices.Clone(hook.DeletePolicies) + copied.OutputLogPolicies = slices.Clone(hook.OutputLogPolicies) + out[i] = &copied + } + return out +} + +func copyChart(ch *chart.Chart) *chart.Chart { + if ch == nil { + return nil + } + out := &chart.Chart{ + Raw: copyFiles(ch.Raw), + Metadata: copyMetadata(ch.Metadata), + Lock: copyLock(ch.Lock), + Templates: copyFiles(ch.Templates), + Values: copyValues(ch.Values), + Schema: bytes.Clone(ch.Schema), + SchemaModTime: ch.SchemaModTime, + Files: copyFiles(ch.Files), + ModTime: ch.ModTime, + } + // AddDependency sets the parent of each dependency, which rebuilds the + // chart tree without reaching into the unexported fields. + for _, dep := range ch.Dependencies() { + out.AddDependency(copyChart(dep)) + } + return out +} + +func copyMetadata(md *chart.Metadata) *chart.Metadata { + if md == nil { + return nil + } + out := *md + out.Sources = slices.Clone(md.Sources) + out.Keywords = slices.Clone(md.Keywords) + out.Annotations = maps.Clone(md.Annotations) + if md.Maintainers != nil { + out.Maintainers = make([]*chart.Maintainer, len(md.Maintainers)) + for i, m := range md.Maintainers { + if m == nil { + continue + } + copied := *m + out.Maintainers[i] = &copied + } + } + out.Dependencies = copyDependencies(md.Dependencies) + return &out +} + +func copyLock(lock *chart.Lock) *chart.Lock { + if lock == nil { + return nil + } + out := *lock + out.Dependencies = copyDependencies(lock.Dependencies) + return &out +} + +func copyDependencies(deps []*chart.Dependency) []*chart.Dependency { + if deps == nil { + return nil + } + out := make([]*chart.Dependency, len(deps)) + for i, dep := range deps { + if dep == nil { + continue + } + copied := *dep + copied.Tags = slices.Clone(dep.Tags) + copied.ImportValues = copyValueSlice(dep.ImportValues) + out[i] = &copied + } + return out +} + +func copyFiles(files []*common.File) []*common.File { + if files == nil { + return nil + } + out := make([]*common.File, len(files)) + for i, file := range files { + if file == nil { + continue + } + copied := *file + copied.Data = bytes.Clone(file.Data) + out[i] = &copied + } + return out +} + +// copyValues deep copies the maps and slices that make up decoded chart values +// and release config. Other values are copied as interface values, which is +// enough for the scalars these maps normally hold. +func copyValues(values map[string]any) map[string]any { + if values == nil { + return nil + } + out := make(map[string]any, len(values)) + for key, value := range values { + out[key] = copyValue(value) + } + return out +} + +func copyValue(value any) any { + switch typed := value.(type) { + case map[string]any: + return copyValues(typed) + case map[any]any: + out := make(map[any]any, len(typed)) + for key, nested := range typed { + out[key] = copyValue(nested) + } + return out + case []any: + return copyValueSlice(typed) + default: + return value + } +} + +func copyValueSlice(values []any) []any { + if values == nil { + return nil + } + out := make([]any, len(values)) + for i, value := range values { + out[i] = copyValue(value) + } + return out +} diff --git a/pkg/storage/driver/copy_test.go b/pkg/storage/driver/copy_test.go new file mode 100644 index 000000000..aebaef08f --- /dev/null +++ b/pkg/storage/driver/copy_test.go @@ -0,0 +1,114 @@ +/* +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 ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "helm.sh/helm/v4/pkg/chart/common" + chart "helm.sh/helm/v4/pkg/chart/v2" + rcommon "helm.sh/helm/v4/pkg/release/common" + rspb "helm.sh/helm/v4/pkg/release/v1" +) + +// chartFixture builds a chart with a dependency, raw files and nested values so +// the copy can be checked against the parts a JSON round-trip would drop. +func chartFixture() *chart.Chart { + ch := &chart.Chart{ + Metadata: &chart.Metadata{ + Name: "parent", + Annotations: map[string]string{"example.com/team": "helm"}, + Maintainers: []*chart.Maintainer{{Name: "maintainer"}}, + }, + Raw: []*common.File{{Name: "Chart.yaml", Data: []byte("name: parent")}}, + Templates: []*common.File{{Name: "templates/cm.yaml", Data: []byte("kind: ConfigMap")}}, + Values: map[string]any{"nested": map[string]any{"replicas": 1}}, + Schema: []byte("schema-bytes"), + } + ch.AddDependency(&chart.Chart{Metadata: &chart.Metadata{Name: "child"}}) + return ch +} + +func releaseFixture() *rspb.Release { + rls := releaseStub("copy-me", 1, "default", rcommon.StatusDeployed) + rls.Chart = chartFixture() + rls.Config = map[string]any{"nested": map[string]any{"enabled": true}} + rls.Hooks = []*rspb.Hook{{ + Name: "pre-install", + Events: []rspb.HookEvent{rspb.HookPreInstall}, + }} + return rls +} + +func TestCopyReleaseIsIndependent(t *testing.T) { + rls := releaseFixture() + copied := copyRelease(rls) + + require.NotSame(t, rls, copied) + assert.Equal(t, rls, copied, "a copy should hold the same data as the original") + + copied.Info.Status = rcommon.StatusFailed + copied.Labels["key1"] = "changed" + copied.Config["nested"].(map[string]any)["enabled"] = false + copied.Chart.Metadata.Name = "renamed" + copied.Chart.Metadata.Annotations["example.com/team"] = "changed" + copied.Chart.Metadata.Maintainers[0].Name = "changed" + copied.Chart.Values["nested"].(map[string]any)["replicas"] = 2 + copied.Chart.Raw[0].Data[0] = 'X' + copied.Chart.Schema[0] = 'X' + copied.Chart.Dependencies()[0].Metadata.Name = "changed" + copied.Hooks[0].Events[0] = rspb.HookPostInstall + + assert.Equal(t, rcommon.StatusDeployed, rls.Info.Status) + assert.Equal(t, "val1", rls.Labels["key1"]) + assert.Equal(t, true, rls.Config["nested"].(map[string]any)["enabled"]) + assert.Equal(t, "parent", rls.Chart.Metadata.Name) + assert.Equal(t, "helm", rls.Chart.Metadata.Annotations["example.com/team"]) + assert.Equal(t, "maintainer", rls.Chart.Metadata.Maintainers[0].Name) + assert.Equal(t, 1, rls.Chart.Values["nested"].(map[string]any)["replicas"]) + assert.Equal(t, []byte("name: parent"), rls.Chart.Raw[0].Data) + assert.Equal(t, []byte("schema-bytes"), rls.Chart.Schema) + assert.Equal(t, "child", rls.Chart.Dependencies()[0].Metadata.Name) + assert.Equal(t, rspb.HookPreInstall, rls.Hooks[0].Events[0]) +} + +// The chart tree is rebuilt through AddDependency, so the copied dependency has +// to point at the copied parent rather than the original one. +func TestCopyReleaseRebuildsChartTree(t *testing.T) { + copied := copyRelease(releaseFixture()) + + require.Len(t, copied.Chart.Dependencies(), 1) + dep := copied.Chart.Dependencies()[0] + assert.Same(t, copied.Chart, dep.Parent()) + assert.True(t, copied.Chart.IsRoot()) + assert.Equal(t, "parent.child", dep.ChartPath()) +} + +func TestCopyReleaseKeepsNilFields(t *testing.T) { + copied := copyRelease(&rspb.Release{Name: "sparse"}) + + assert.Equal(t, "sparse", copied.Name) + assert.Nil(t, copied.Info) + assert.Nil(t, copied.Chart) + assert.Nil(t, copied.Config) + assert.Nil(t, copied.Hooks) + assert.Nil(t, copied.Labels) + assert.Nil(t, copyRelease(nil)) +} diff --git a/pkg/storage/driver/memory.go b/pkg/storage/driver/memory.go index 7ea4a014a..4e4746acf 100644 --- a/pkg/storage/driver/memory.go +++ b/pkg/storage/driver/memory.go @@ -79,7 +79,7 @@ func (mem *Memory) Get(key string) (release.Releaser, error) { } if recs, ok := mem.cache[mem.namespace][name]; ok { if r := recs.Get(key); r != nil { - return r.rls, nil + return copyRelease(r.rls), nil } } return nil, ErrReleaseNotFound @@ -100,8 +100,11 @@ func (mem *Memory) List(filter func(release.Releaser) bool) ([]release.Releaser, } for _, recs := range mem.cache[namespace] { recs.Iter(func(_ int, rec *record) bool { - if filter(rec.rls) { - ls = append(ls, rec.rls) + // Copy before filtering so the callback cannot reach the + // stored release either. + rls := copyRelease(rec.rls) + if filter(rls) { + ls = append(ls, rls) } return true }) @@ -137,7 +140,7 @@ func (mem *Memory) Query(keyvals map[string]string) ([]release.Releaser, error) return false } if rec.lbs.match(lbs) { - ls = append(ls, rec.rls) + ls = append(ls, copyRelease(rec.rls)) } return true }) diff --git a/pkg/storage/driver/memory_test.go b/pkg/storage/driver/memory_test.go index 745b12345..20cf37058 100644 --- a/pkg/storage/driver/memory_test.go +++ b/pkg/storage/driver/memory_test.go @@ -264,3 +264,82 @@ func TestMemoryDelete(t *testing.T) { } } } + +// A stored release should only change through Update, which is the contract the +// persistent drivers already provide by encoding on write and decoding on read. +func TestMemoryReleaseIsolation(t *testing.T) { + key := testKey("copy-me", 1) + tests := []struct { + desc string + mutate func(t *testing.T, mem *Memory, rls *rspb.Release) + }{ + { + desc: "create input", + mutate: func(_ *testing.T, _ *Memory, rls *rspb.Release) { + rls.Info.Status = common.StatusFailed + }, + }, + { + desc: "update input", + mutate: func(t *testing.T, mem *Memory, rls *rspb.Release) { + t.Helper() + require.NoError(t, mem.Update(key, rls)) + rls.Info.Status = common.StatusFailed + }, + }, + { + desc: "get result", + mutate: func(t *testing.T, mem *Memory, _ *rspb.Release) { + t.Helper() + rel, err := mem.Get(key) + require.NoError(t, err) + convertReleaserToV1(t, rel).Info.Status = common.StatusFailed + }, + }, + { + desc: "list result", + mutate: func(t *testing.T, mem *Memory, _ *rspb.Release) { + t.Helper() + ls, err := mem.List(func(release.Releaser) bool { return true }) + require.NoError(t, err) + require.Len(t, ls, 1) + convertReleaserToV1(t, ls[0]).Info.Status = common.StatusFailed + }, + }, + { + desc: "list filter argument", + mutate: func(t *testing.T, mem *Memory, _ *rspb.Release) { + t.Helper() + _, err := mem.List(func(rel release.Releaser) bool { + convertReleaserToV1(t, rel).Info.Status = common.StatusFailed + return false + }) + require.NoError(t, err) + }, + }, + { + desc: "query result", + mutate: func(t *testing.T, mem *Memory, _ *rspb.Release) { + t.Helper() + ls, err := mem.Query(map[string]string{"name": "copy-me"}) + require.NoError(t, err) + require.Len(t, ls, 1) + convertReleaserToV1(t, ls[0]).Info.Status = common.StatusFailed + }, + }, + } + + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { + mem := NewMemory() + rls := releaseFixture() + require.NoError(t, mem.Create(key, rls)) + + tt.mutate(t, mem, rls) + + stored, err := mem.Get(key) + require.NoError(t, err) + assert.Equal(t, common.StatusDeployed, convertReleaserToV1(t, stored).Info.Status) + }) + } +} diff --git a/pkg/storage/driver/records.go b/pkg/storage/driver/records.go index 3393bb603..8c2b0594f 100644 --- a/pkg/storage/driver/records.go +++ b/pkg/storage/driver/records.go @@ -123,6 +123,7 @@ func newRecord(key string, rls *rspb.Release) *record { lbs.set("status", rls.Info.Status.String()) lbs.set("version", strconv.Itoa(rls.Version)) - // return &record{key: key, lbs: lbs, rls: proto.Clone(rls).(*rspb.Release)} - return &record{key: key, lbs: lbs, rls: rls} + // Store a copy so later changes to the caller's release do not reach the + // record, matching the isolation the persistent drivers get from encoding. + return &record{key: key, lbs: lbs, rls: copyRelease(rls)} }