Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkg/k8s: use a deep copy of CNP in UpdateStatus to avoid race condition #28364

Merged
merged 1 commit into from Oct 10, 2023

Conversation

aanm
Copy link
Member

@aanm aanm commented Oct 2, 2023

We modified the UpdateStatus function to ensure that the CNP object is
deep-copied before passing it as an argument. This change was necessary
because the UpdateStatus function was modifying the CNP object, specifically
clearing the LastAppliedConfiguration key from the annotations map. By
deep-copying the CNP object, we ensure that the original object remains
unmodified which fixes the following race condition:

Write at 0x00c002a98510 by goroutine 119834:
  runtime.mapassign_faststr()
      /usr/local/go/src/runtime/map_faststr.go:203 +0x0
  github.com/cilium/cilium/pkg/k8s.(*CNPStatusUpdateContext).updateViaAPIServer.func1()
      ./pkg/k8s/cnp.go:215 +0x53
  runtime.deferreturn()
      /usr/local/go/src/runtime/panic.go:477 +0x30
  github.com/cilium/cilium/pkg/k8s.(*CNPStatusUpdateContext).updateStatus()
      ./pkg/k8s/cnp.go:78 +0x2c7
  github.com/cilium/cilium/pkg/k8s.(*CNPStatusUpdateContext).UpdateStatus()
      ./pkg/k8s/cnp.go:146 +0x786
  github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).addCiliumNetworkPolicyV2.func1()
      ./pkg/k8s/watchers/cilium_network_policy.go:352 +0x86
  github.com/cilium/cilium/pkg/controller.(*controller).runController()
      ./pkg/controller/controller.go:251 +0x171
  github.com/cilium/cilium/pkg/controller.(*Manager).createControllerLocked.func1()
      ./pkg/controller/manager.go:111 +0xa4

Previous read at 0x00c002a98510 by goroutine 1205:
  runtime.mapiterinit()
      /usr/local/go/src/runtime/map.go:816 +0x0
  github.com/cilium/cilium/pkg/comparator.MapStringEqualsIgnoreKeys()
      ./pkg/comparator/comparator.go:82 +0xb1
  github.com/cilium/cilium/pkg/k8s/apis/cilium.io/v2.objectMetaDeepEqual()
      ./pkg/k8s/apis/cilium.io/v2/cnp_types.go:65 +0xb0
  github.com/cilium/cilium/pkg/k8s/apis/cilium.io/v2.(*CiliumNetworkPolicy).DeepEqual()
      ./pkg/k8s/apis/cilium.io/v2/cnp_types.go:54 +0x177
  github.com/cilium/cilium/pkg/k8s/types.(*SlimCNP).DeepEqual()
      ./pkg/k8s/types/zz_generated.deepequal.go:82 +0xbd
  github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).onUpsert()
      ./pkg/k8s/watchers/cilium_network_policy.go:238 +0x170
  github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).ciliumNetworkPoliciesInit.func1()
      ./pkg/k8s/watchers/cilium_network_policy.go:175 +0xc64

Goroutine 119834 (running) created at:
  github.com/cilium/cilium/pkg/controller.(*Manager).createControllerLocked()
      ./pkg/controller/manager.go:111 +0x757
  github.com/cilium/cilium/pkg/controller.(*Manager).updateController()
      ./pkg/controller/manager.go:84 +0x44f
  github.com/cilium/cilium/pkg/controller.(*Manager).UpdateController()
      ./pkg/controller/manager.go:52 +0xe6f
  github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).addCiliumNetworkPolicyV2()
      ./pkg/k8s/watchers/cilium_network_policy.go:348 +0xc75
  github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).onUpsert()
      ./pkg/k8s/watchers/cilium_network_policy.go:271 +0x744
  github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).ciliumNetworkPoliciesInit.func1()
      ./pkg/k8s/watchers/cilium_network_policy.go:175 +0xc64

Goroutine 1205 (running) created at:
  github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).ciliumNetworkPoliciesInit()
      ./pkg/k8s/watchers/cilium_network_policy.go:91 +0x27c
  github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).enableK8sWatchers.func1()
      ./pkg/k8s/watchers/watcher.go:578 +0x59
  sync.(*Once).doSlow()
      /usr/local/go/src/sync/once.go:74 +0xf0
  sync.(*Once).Do()
      /usr/local/go/src/sync/once.go:65 +0x44
  github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).enableK8sWatchers()
      ./pkg/k8s/watchers/watcher.go:578 +0xa24
  github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).InitK8sSubsystem()
      ./pkg/k8s/watchers/watcher.go:508 +0x104
  github.com/cilium/cilium/daemon/cmd.newDaemon()
      ./daemon/cmd/daemon.go:1001 +0x9070
  github.com/cilium/cilium/daemon/cmd.newDaemonPromise.func1()
      ./daemon/cmd/daemon_main.go:1687 +0xa4
  github.com/cilium/cilium/pkg/hive.Hook.Start()
      ./pkg/hive/lifecycle.go:34 +0x70
  github.com/cilium/cilium/pkg/hive.(*Hook).Start()
      <autogenerated>:1 +0x1f
  github.com/cilium/cilium/pkg/hive.(*DefaultLifecycle).Start()
      ./pkg/hive/lifecycle.go:103 +0x3f1
  github.com/cilium/cilium/pkg/hive.(*Hive).Start()
      ./pkg/hive/hive.go:291 +0x152
  github.com/cilium/cilium/pkg/hive.(*Hive).Run()
      ./pkg/hive/hive.go:191 +0xc4
  github.com/cilium/cilium/daemon/cmd.NewAgentCmd.func1()
      ./daemon/cmd/root.go:39 +0x264
  github.com/spf13/cobra.(*Command).execute()
      ./vendor/github.com/spf13/cobra/command.go:944 +0xcb8
  github.com/spf13/cobra.(*Command).ExecuteC()
      ./vendor/github.com/spf13/cobra/command.go:1068 +0x5c4
  github.com/spf13/cobra.(*Command).Execute()
      ./vendor/github.com/spf13/cobra/command.go:992 +0x2e
  github.com/cilium/cilium/daemon/cmd.Execute()
      ./daemon/cmd/root.go:79 +0x2f
  main.main()
      ./daemon/main.go:14 +0xa9

@aanm aanm added kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium. needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Oct 2, 2023
@aanm aanm requested a review from a team as a code owner October 2, 2023 22:28
@aanm aanm requested a review from christarazi October 2, 2023 22:28
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.3 Oct 2, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.13.8 Oct 2, 2023
@aanm aanm added affects/v1.11 This issue affects v1.11 branch affects/v1.12 This issue affects v1.12 branch affects/v1.13 This issue affects v1.13 branch affects/v1.14 This issue affects v1.14 branch labels Oct 2, 2023
@aanm
Copy link
Member Author

aanm commented Oct 2, 2023

/test

@aanm
Copy link
Member Author

aanm commented Oct 5, 2023

/test

We modified the UpdateStatus function to ensure that the CNP object is
deep-copied before passing it as an argument. This change was necessary
because the UpdateStatus function was modifying the CNP object, specifically
clearing the LastAppliedConfiguration key from the annotations map. By
deep-copying the CNP object, we ensure that the original object remains
unmodified which fixes the following race condition:

```
Write at 0x00c002a98510 by goroutine 119834:
  runtime.mapassign_faststr()
      /usr/local/go/src/runtime/map_faststr.go:203 +0x0
  github.com/cilium/cilium/pkg/k8s.(*CNPStatusUpdateContext).updateViaAPIServer.func1()
      ./pkg/k8s/cnp.go:215 +0x53
  runtime.deferreturn()
      /usr/local/go/src/runtime/panic.go:477 +0x30
  github.com/cilium/cilium/pkg/k8s.(*CNPStatusUpdateContext).updateStatus()
      ./pkg/k8s/cnp.go:78 +0x2c7
  github.com/cilium/cilium/pkg/k8s.(*CNPStatusUpdateContext).UpdateStatus()
      ./pkg/k8s/cnp.go:146 +0x786
  github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).addCiliumNetworkPolicyV2.func1()
      ./pkg/k8s/watchers/cilium_network_policy.go:352 +0x86
  github.com/cilium/cilium/pkg/controller.(*controller).runController()
      ./pkg/controller/controller.go:251 +0x171
  github.com/cilium/cilium/pkg/controller.(*Manager).createControllerLocked.func1()
      ./pkg/controller/manager.go:111 +0xa4

Previous read at 0x00c002a98510 by goroutine 1205:
  runtime.mapiterinit()
      /usr/local/go/src/runtime/map.go:816 +0x0
  github.com/cilium/cilium/pkg/comparator.MapStringEqualsIgnoreKeys()
      ./pkg/comparator/comparator.go:82 +0xb1
  github.com/cilium/cilium/pkg/k8s/apis/cilium.io/v2.objectMetaDeepEqual()
      ./pkg/k8s/apis/cilium.io/v2/cnp_types.go:65 +0xb0
  github.com/cilium/cilium/pkg/k8s/apis/cilium.io/v2.(*CiliumNetworkPolicy).DeepEqual()
      ./pkg/k8s/apis/cilium.io/v2/cnp_types.go:54 +0x177
  github.com/cilium/cilium/pkg/k8s/types.(*SlimCNP).DeepEqual()
      ./pkg/k8s/types/zz_generated.deepequal.go:82 +0xbd
  github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).onUpsert()
      ./pkg/k8s/watchers/cilium_network_policy.go:238 +0x170
  github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).ciliumNetworkPoliciesInit.func1()
      ./pkg/k8s/watchers/cilium_network_policy.go:175 +0xc64

Goroutine 119834 (running) created at:
  github.com/cilium/cilium/pkg/controller.(*Manager).createControllerLocked()
      ./pkg/controller/manager.go:111 +0x757
  github.com/cilium/cilium/pkg/controller.(*Manager).updateController()
      ./pkg/controller/manager.go:84 +0x44f
  github.com/cilium/cilium/pkg/controller.(*Manager).UpdateController()
      ./pkg/controller/manager.go:52 +0xe6f
  github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).addCiliumNetworkPolicyV2()
      ./pkg/k8s/watchers/cilium_network_policy.go:348 +0xc75
  github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).onUpsert()
      ./pkg/k8s/watchers/cilium_network_policy.go:271 +0x744
  github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).ciliumNetworkPoliciesInit.func1()
      ./pkg/k8s/watchers/cilium_network_policy.go:175 +0xc64

Goroutine 1205 (running) created at:
  github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).ciliumNetworkPoliciesInit()
      ./pkg/k8s/watchers/cilium_network_policy.go:91 +0x27c
  github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).enableK8sWatchers.func1()
      ./pkg/k8s/watchers/watcher.go:578 +0x59
  sync.(*Once).doSlow()
      /usr/local/go/src/sync/once.go:74 +0xf0
  sync.(*Once).Do()
      /usr/local/go/src/sync/once.go:65 +0x44
  github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).enableK8sWatchers()
      ./pkg/k8s/watchers/watcher.go:578 +0xa24
  github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).InitK8sSubsystem()
      ./pkg/k8s/watchers/watcher.go:508 +0x104
  github.com/cilium/cilium/daemon/cmd.newDaemon()
      ./daemon/cmd/daemon.go:1001 +0x9070
  github.com/cilium/cilium/daemon/cmd.newDaemonPromise.func1()
      ./daemon/cmd/daemon_main.go:1687 +0xa4
  github.com/cilium/cilium/pkg/hive.Hook.Start()
      ./pkg/hive/lifecycle.go:34 +0x70
  github.com/cilium/cilium/pkg/hive.(*Hook).Start()
      <autogenerated>:1 +0x1f
  github.com/cilium/cilium/pkg/hive.(*DefaultLifecycle).Start()
      ./pkg/hive/lifecycle.go:103 +0x3f1
  github.com/cilium/cilium/pkg/hive.(*Hive).Start()
      ./pkg/hive/hive.go:291 +0x152
  github.com/cilium/cilium/pkg/hive.(*Hive).Run()
      ./pkg/hive/hive.go:191 +0xc4
  github.com/cilium/cilium/daemon/cmd.NewAgentCmd.func1()
      ./daemon/cmd/root.go:39 +0x264
  github.com/spf13/cobra.(*Command).execute()
      ./vendor/github.com/spf13/cobra/command.go:944 +0xcb8
  github.com/spf13/cobra.(*Command).ExecuteC()
      ./vendor/github.com/spf13/cobra/command.go:1068 +0x5c4
  github.com/spf13/cobra.(*Command).Execute()
      ./vendor/github.com/spf13/cobra/command.go:992 +0x2e
  github.com/cilium/cilium/daemon/cmd.Execute()
      ./daemon/cmd/root.go:79 +0x2f
  main.main()
      ./daemon/main.go:14 +0xa9
```

Signed-off-by: André Martins <andre@cilium.io>
@aanm
Copy link
Member Author

aanm commented Oct 6, 2023

/test

@aanm aanm changed the title pkg/k8s: store the original CNP in CNP cache pkg/k8s: use a deep copy of CNP in UpdateStatus to avoid race condition Oct 6, 2023
Copy link
Member

@pippolo84 pippolo84 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! 🚀

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 9, 2023
@aanm aanm merged commit 3de85a1 into cilium:main Oct 10, 2023
60 of 61 checks passed
@joamaki joamaki mentioned this pull request Oct 10, 2023
4 tasks
@joamaki joamaki added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Oct 10, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.14 in 1.14.3 Oct 10, 2023
@joamaki joamaki mentioned this pull request Oct 11, 2023
3 tasks
@joamaki joamaki added backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. and removed needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Oct 11, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.13 in 1.13.8 Oct 11, 2023
@github-actions github-actions bot added backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. and removed backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. labels Oct 12, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed this from Backport pending to v1.14 in 1.14.3 Oct 12, 2023
@github-actions github-actions bot added backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. labels Oct 12, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Backport done to v1.14 in 1.14.3 Oct 12, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed this from Backport pending to v1.13 in 1.13.8 Oct 12, 2023
@aanm aanm deleted the pr/fix-store-controller branch November 8, 2023 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects/v1.11 This issue affects v1.11 branch affects/v1.12 This issue affects v1.12 branch affects/v1.13 This issue affects v1.13 branch affects/v1.14 This issue affects v1.14 branch backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. kind/bug This is a bug in the Cilium logic. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
No open projects
1.14.3
Backport done to v1.14
Development

Successfully merging this pull request may close these issues.

None yet

4 participants