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)} }