From 6730c4f1c1b0c2ccc89dab0157fe9397052c96e4 Mon Sep 17 00:00:00 2001 From: Charly Molter Date: Tue, 12 Nov 2024 12:38:30 +0100 Subject: [PATCH 1/2] fix(kuma-cp): avoid concurrent access on resource meta (#11997) There were some places where labels on meta weren't not cloned when duplicating the meta. Walked the code for odd usages of `GetLabels()`. See also https://github.com/kumahq/kuma/issues/11886 Signed-off-by: Charly Molter Signed-off-by: Mike Beaumont Co-authored-by: Mike Beaumont --- pkg/kds/util/meta.go | 8 -------- pkg/kds/util/resource.go | 16 +++++++++++++++- pkg/plugins/resources/memory/store.go | 5 +++-- pkg/plugins/resources/remote/store.go | 5 +++-- pkg/plugins/secrets/k8s/store.go | 1 + 5 files changed, 22 insertions(+), 13 deletions(-) diff --git a/pkg/kds/util/meta.go b/pkg/kds/util/meta.go index 8ba29d994ebe..fb703424a322 100644 --- a/pkg/kds/util/meta.go +++ b/pkg/kds/util/meta.go @@ -53,14 +53,6 @@ func CloneResourceMeta(m model.ResourceMeta, fs ...CloneResourceMetaOpt) model.R return meta } -func kumaResourceMetaToResourceMeta(meta *mesh_proto.KumaResource_Meta) model.ResourceMeta { - return &resourceMeta{ - name: meta.Name, - mesh: meta.Mesh, - labels: meta.GetLabels(), - } -} - func (r *resourceMeta) GetName() string { return r.name } diff --git a/pkg/kds/util/resource.go b/pkg/kds/util/resource.go index b26c22f3da6b..1f5f0933d2b4 100644 --- a/pkg/kds/util/resource.go +++ b/pkg/kds/util/resource.go @@ -2,6 +2,7 @@ package util import ( "fmt" + "maps" "strings" envoy_sd "github.com/envoyproxy/go-control-plane/envoy/service/discovery/v3" @@ -56,7 +57,7 @@ func ToEnvoyResources(rlist model.ResourceList) ([]envoy_types.Resource, error) Meta: &mesh_proto.KumaResource_Meta{ Name: r.GetMeta().GetName(), Mesh: r.GetMeta().GetMesh(), - Labels: r.GetMeta().GetLabels(), + Labels: maps.Clone(r.GetMeta().GetLabels()), Version: "", }, Spec: pbany, @@ -136,7 +137,20 @@ func toResources(resourceType model.ResourceType, krs []*mesh_proto.KumaResource if err = model.FromAny(kr.Spec, obj.GetSpec()); err != nil { return nil, err } +<<<<<<< HEAD obj.SetMeta(kumaResourceMetaToResourceMeta(kr.Meta)) +======= + if obj.Descriptor().HasStatus && kr.Status != nil { + if err = model.FromAny(kr.Status, obj.GetStatus()); err != nil { + return nil, err + } + } + obj.SetMeta(&resourceMeta{ + name: kr.GetMeta().GetName(), + mesh: kr.GetMeta().GetMesh(), + labels: maps.Clone(kr.GetMeta().GetLabels()), + }) +>>>>>>> c3d7187c7 (fix(kuma-cp): avoid concurrent access on resource meta (#11997)) if err := list.AddItem(obj); err != nil { return nil, err } diff --git a/pkg/plugins/resources/memory/store.go b/pkg/plugins/resources/memory/store.go index f45057ff1f66..7e858cf62bfa 100644 --- a/pkg/plugins/resources/memory/store.go +++ b/pkg/plugins/resources/memory/store.go @@ -3,6 +3,7 @@ package memory import ( "context" "fmt" + "maps" "strconv" "strings" "sync" @@ -123,7 +124,7 @@ func (c *memoryStore) Create(_ context.Context, r core_model.Resource, fs ...sto Version: initialVersion(), CreationTime: opts.CreationTime, ModificationTime: opts.CreationTime, - Labels: opts.Labels, + Labels: maps.Clone(opts.Labels), } // fill the meta @@ -180,7 +181,7 @@ func (c *memoryStore) Update(_ context.Context, r core_model.Resource, fs ...sto meta.Version = meta.Version.Next() meta.ModificationTime = opts.ModificationTime if opts.ModifyLabels { - meta.Labels = opts.Labels + meta.Labels = maps.Clone(opts.Labels) } r.SetMeta(meta) diff --git a/pkg/plugins/resources/remote/store.go b/pkg/plugins/resources/remote/store.go index 09b0079af978..5eeb6cc62d82 100644 --- a/pkg/plugins/resources/remote/store.go +++ b/pkg/plugins/resources/remote/store.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "io" + "maps" "net/http" "strconv" @@ -41,7 +42,7 @@ func (s *remoteStore) Create(ctx context.Context, res model.Resource, fs ...stor Type: string(res.Descriptor().Name), Name: opts.Name, Mesh: opts.Mesh, - Labels: opts.Labels, + Labels: maps.Clone(opts.Labels), } if err := s.upsert(ctx, res, meta); err != nil { return err @@ -55,7 +56,7 @@ func (s *remoteStore) Update(ctx context.Context, res model.Resource, fs ...stor Type: string(res.Descriptor().Name), Name: res.GetMeta().GetName(), Mesh: res.GetMeta().GetMesh(), - Labels: opts.Labels, + Labels: maps.Clone(opts.Labels), } if err := s.upsert(ctx, res, meta); err != nil { return err diff --git a/pkg/plugins/secrets/k8s/store.go b/pkg/plugins/secrets/k8s/store.go index fd30e075b72f..a4c423ae8b8a 100644 --- a/pkg/plugins/secrets/k8s/store.go +++ b/pkg/plugins/secrets/k8s/store.go @@ -113,6 +113,7 @@ func (s *KubernetesStore) Update(ctx context.Context, r core_model.Resource, fs } func setLabelsAnnotationsAndMesh(s *kube_core.Secret, mesh string, labels map[string]string) { + labels = maps.Clone(labels) if labels == nil { labels = map[string]string{} } From ba440d574aa8b1fc195694d325c5718303331347 Mon Sep 17 00:00:00 2001 From: Mike Beaumont Date: Tue, 12 Nov 2024 14:14:17 +0100 Subject: [PATCH 2/2] fix: conflict Signed-off-by: Mike Beaumont --- pkg/kds/util/resource.go | 9 --------- 1 file changed, 9 deletions(-) diff --git a/pkg/kds/util/resource.go b/pkg/kds/util/resource.go index 1f5f0933d2b4..1e3298414a53 100644 --- a/pkg/kds/util/resource.go +++ b/pkg/kds/util/resource.go @@ -137,20 +137,11 @@ func toResources(resourceType model.ResourceType, krs []*mesh_proto.KumaResource if err = model.FromAny(kr.Spec, obj.GetSpec()); err != nil { return nil, err } -<<<<<<< HEAD - obj.SetMeta(kumaResourceMetaToResourceMeta(kr.Meta)) -======= - if obj.Descriptor().HasStatus && kr.Status != nil { - if err = model.FromAny(kr.Status, obj.GetStatus()); err != nil { - return nil, err - } - } obj.SetMeta(&resourceMeta{ name: kr.GetMeta().GetName(), mesh: kr.GetMeta().GetMesh(), labels: maps.Clone(kr.GetMeta().GetLabels()), }) ->>>>>>> c3d7187c7 (fix(kuma-cp): avoid concurrent access on resource meta (#11997)) if err := list.AddItem(obj); err != nil { return nil, err }