Repository: kumahq/kuma
Pull request #5372 (2022-11-24 15:21:46)

fix(kuma-cp): memory store keeps children after owner update
Memory store was not keeping children after owner update.
Production is not affected, because memory store is not used on prod, only in tests and local development.
This Postgres and K8S is NOT affected.

That was the real cause of flaky update policy on KDS.
Default mesh resources created by tr-test mesh (TrafficRoute suite) were there after the test was done. 
Then Global CP was syncing them down to Zone CP and Zone CP was rejecting all TrafficRoute resources because mesh was absent.
```
2022-11-24T13:30:04.268Z	INFO	kds-zone	creating a new resource from upstream	{"type": "CircuitBreaker", "name": "circuit-breaker-all-tr-test", "mesh": "tr-test"}
2022-11-24T13:30:04.270Z	INFO	kds-zone	error during callback received, sending NACK	{"peer-id": "global", "err": "failed to create k8s resource: admission webhook \"owner-reference.kuma-admission.kuma.io\" denied the request: Mesh.kuma.io \"tr-test\" not found", "errVerbose": "admission webhook \"owner-reference.kuma-admission.kuma.io\" denied the request: Mesh.kuma.io \"tr-test\" not found\nfailed to create k8s resource\ngithub.com/kumahq/kuma/pkg/plugins/resources/k8s.(*KubernetesStore).Create\n\t/home/circleci/project/pkg/plugins/resources/k8s/store.go:75\ngithub.com/kumahq/kuma/pkg/core/resources/store.(*paginationStore).Create\n\t/home/circleci/project/pkg/core/resources/store/pagination_store.go:30\ngithub.com/kumahq/kuma/pkg/metrics/store.(*MeteredStore).Create\n\t/home/circleci/project/pkg/metrics/store/store.go:38\ngithub.com/kumahq/kuma/pkg/core/resources/store.(*customizableResourceStore).Create\n\t/home/circleci/project/pkg/core/resources/store/customizable_store.go:30\ngithub.com/kumahq/kuma/pkg/kds/store.(*syncResourceStore).Sync\n\t/home/circleci/project/pkg/kds/store/sync.go:152\ngithub.com/kumahq/kuma/pkg/kds/zone.Callbacks.func1\n\t/home/circleci/project/pkg/kds/zone/components.go:145\ngithub.com/kumahq/kuma/pkg/kds/client.(*kdsSink).Receive\n\t/home/circleci/project/pkg/kds/client/sink.go:64\ngithub.com/kumahq/kuma/pkg/kds/zone.Setup.func1.2\n\t/home/circleci/project/pkg/kds/zone/components.go:76\nruntime.goexit\n\t/home/circleci/go/src/runtime/asm_amd64.s:1571"}
```

### Checklist prior to review

<!--
Each of these sections need to be filled by the author when opening the PR.

If something doesn't apply please check the box and add a justification after the `--`
-->

- [X] Link to docs PR or issue --
- [X] Link to UI issue or PR --
- [X] Is the [issue worked on linked][1]? --
- [X] The PR does not hardcode values that might break projects that depend on kuma (e.g. "kumahq" as a image registry) --
- [X] The PR will work for both Linux and Windows, system specific functions like `syscall.Mkfifo` have equivalent implementation on the other OS --
- [X] Unit Tests --
- [X] E2E Tests --
- [X] Manual Universal Tests --
- [X] Manual Kubernetes Tests --
- [X] Do you need to update [`UPGRADE.md`](../blob/master/UPGRADE.md)? --
- [X] Does it need to be backported according to the [backporting policy](../blob/master/CONTRIBUTING.md#backporting)? --
- [X] Do you need to explicitly set a [`> Changelog:` entry here](../blob/master/CONTRIBUTING.md#submitting-a-patch) or add a `ci/` label to run fewer/more tests?

[1]: https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword



Patch:
diff --git a/pkg/plugins/resources/memory/store.go b/pkg/plugins/resources/memory/store.go
index 672cab421..30e5bb7c0 100644
--- a/pkg/plugins/resources/memory/store.go
+++ b/pkg/plugins/resources/memory/store.go
@@ -161,25 +161,22 @@ func (c *memoryStore) Update(_ context.Context, r core_model.Resource, fs ...sto
 
 	// Name must be provided via r.GetMeta()
 	mesh := r.GetMeta().GetMesh()
-	idx, record := c.findRecord(string(r.Descriptor().Name), r.GetMeta().GetName(), mesh)
+	_, record := c.findRecord(string(r.Descriptor().Name), r.GetMeta().GetName(), mesh)
 	if record == nil || meta.Version != record.Version {
 		return store.ErrorResourceConflict(r.Descriptor().Name, r.GetMeta().GetName(), r.GetMeta().GetMesh())
 	}
 	meta.Version = meta.Version.Next()
 	meta.ModificationTime = opts.ModificationTime
+	r.SetMeta(meta)
 
-	record, err := c.marshalRecord(
-		string(r.Descriptor().Name),
-		meta,
-		r.GetSpec())
+	record.Version = meta.Version
+	record.ModificationTime = meta.ModificationTime
+	content, err := core_model.ToJSON(r.GetSpec())
 	if err != nil {
 		return err
 	}
+	record.Spec = string(content)
 
-	// persist
-	c.records[idx] = record
-
-	r.SetMeta(meta)
 	if c.eventWriter != nil {
 		go func() {
 			c.eventWriter.Send(events.ResourceChangedEvent{


Test patch:
diff --git a/pkg/test/store/owner_test_template.go b/pkg/test/store/owner_test_template.go
index 7156ffda7..5937723e2 100644
--- a/pkg/test/store/owner_test_template.go
+++ b/pkg/test/store/owner_test_template.go
@@ -61,6 +61,41 @@ func ExecuteOwnerTests(
 		Expect(store.IsResourceNotFound(err)).To(BeTrue())
 	})
 
+	It("should delete resource when its owner is deleted after owner update", func() {
+		// setup
+		meshRes := core_mesh.NewMeshResource()
+		err := s.Create(context.Background(), meshRes, store.CreateByKey(mesh, model.NoMesh))
+		Expect(err).ToNot(HaveOccurred())
+
+		name := "resource-1"
+		trRes := core_mesh.TrafficRouteResource{
+			Spec: &v1alpha1.TrafficRoute{
+				Conf: &v1alpha1.TrafficRoute_Conf{
+					Destination: map[string]string{
+						"path": "demo",
+					},
+				},
+			},
+		}
+		err = s.Create(context.Background(), &trRes,
+			store.CreateByKey(name, mesh),
+			store.CreatedAt(time.Now()),
+			store.CreateWithOwner(meshRes))
+		Expect(err).ToNot(HaveOccurred())
+
+		// when owner is updated
+		Expect(s.Update(context.Background(), meshRes)).To(Succeed())
+
+		// and only then deleted
+		err = s.Delete(context.Background(), meshRes, store.DeleteByKey(mesh, model.NoMesh))
+		Expect(err).ToNot(HaveOccurred())
+
+		// then
+		actual := core_mesh.NewTrafficRouteResource()
+		err = s.Get(context.Background(), actual, store.GetByKey(name, mesh))
+		Expect(store.IsResourceNotFound(err)).To(BeTrue())
+	})
+
 	It("should delete several resources when their owner is deleted", func() {
 		// setup
 		meshRes := core_mesh.NewMeshResource()
diff --git a/test/e2e_env/multizone/sync/sync.go b/test/e2e_env/multizone/sync/sync.go
index 05ae07d30..ec74ce638 100644
--- a/test/e2e_env/multizone/sync/sync.go
+++ b/test/e2e_env/multizone/sync/sync.go
@@ -131,7 +131,7 @@ spec:
 			policySyncedToZones(name)
 		})
 
-		PIt("should sync policy update", func() {
+		It("should sync policy update", func() {
 			// given
 			name := "tr-update"
 			Expect(env.Global.Install(YamlUniversal(universalPolicyNamed(name, 100)))).To(Succeed())