Skip to content

Commit

Permalink
pkg/k8s: use a deep copy of CNP in UpdateStatus to avoid race condition
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
aanm committed Oct 10, 2023
1 parent 9dcc5d1 commit 3de85a1
Showing 1 changed file with 6 additions and 2 deletions.
8 changes: 6 additions & 2 deletions pkg/k8s/watchers/cilium_network_policy.go
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,9 @@ func (k *K8sWatcher) addCiliumNetworkPolicyV2(ciliumNPClient clientset.Interface
controller.ControllerParams{
Group: syncCNPStatusControllerGroup,
DoFunc: func(ctx context.Context) error {
return updateContext.UpdateStatus(ctx, cnp, rev, policyImportErr)
// We need to deep-copy the CNP object because UpdateStatus modifies it
// to clear the LastAppliedConfiguration key from annotations map.
return updateContext.UpdateStatus(ctx, cnp.DeepCopy(), rev, policyImportErr)
},
},
)
Expand Down Expand Up @@ -481,7 +483,9 @@ func (k *K8sWatcher) updateCiliumNetworkPolicyV2AnnotationsOnly(ciliumNPClient c
controller.ControllerParams{
Group: syncCNPStatusControllerGroup,
DoFunc: func(ctx context.Context) error {
return updateContext.UpdateStatus(ctx, cnp, meta.revision, meta.policyImportError)
// We need to deep-copy the CNP object because UpdateStatus modifies it
// to clear the LastAppliedConfiguration key from annotations map.
return updateContext.UpdateStatus(ctx, cnp.DeepCopy(), meta.revision, meta.policyImportError)
},
})

Expand Down

0 comments on commit 3de85a1

Please sign in to comment.