Skip to content

Commit

Permalink
pkg/k8s: fix data race in CNP rule validation
Browse files Browse the repository at this point in the history
This change fixes the following data race:

```
==================
WARNING: DATA RACE
Write at 0x00c001cbe910 by goroutine 304:
  github.com/cilium/cilium/pkg/policy/api.(*PortProtocol).sanitize()
      /go/src/github.com/cilium/cilium/pkg/policy/api/rule_validation.go:393 +0x17a
  github.com/cilium/cilium/pkg/policy/api.(*PortRule).sanitize()
      /go/src/github.com/cilium/cilium/pkg/policy/api/rule_validation.go:345 +0xdb
  github.com/cilium/cilium/pkg/policy/api.(*IngressRule).sanitize()
      /go/src/github.com/cilium/cilium/pkg/policy/api/rule_validation.go:151 +0xbee
  github.com/cilium/cilium/pkg/policy/api.Rule.Sanitize()
      /go/src/github.com/cilium/cilium/pkg/policy/api/rule_validation.go:71 +0x1eb
  github.com/cilium/cilium/pkg/k8s/apis/cilium.io/v2.(*CiliumNetworkPolicy).Parse()
      /go/src/github.com/cilium/cilium/pkg/k8s/apis/cilium.io/v2/types.go:251 +0x663
  github.com/cilium/cilium/pkg/k8s.(*CNPStatusUpdateContext).prepareUpdate()
      /go/src/github.com/cilium/cilium/pkg/k8s/cnp.go:144 +0x8d2
  github.com/cilium/cilium/pkg/k8s.(*CNPStatusUpdateContext).UpdateStatus()
      /go/src/github.com/cilium/cilium/pkg/k8s/cnp.go:237 +0x550
  github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).updateCiliumNetworkPolicyV2AnnotationsOnly.func1()
      /go/src/github.com/cilium/cilium/pkg/k8s/watchers/cilium_network_policy.go:343 +0x7d
  github.com/cilium/cilium/pkg/controller.(*Controller).runController()
      /go/src/github.com/cilium/cilium/pkg/controller/controller.go:205 +0xc71

Previous read at 0x00c001cbe910 by goroutine 18:
  reflect.typedmemmove()
      /usr/local/go/src/runtime/mbarrier.go:177 +0x0
  reflect.packEface()
      /usr/local/go/src/reflect/value.go:119 +0x126
  reflect.valueInterface()
      /usr/local/go/src/reflect/value.go:1030 +0x1b9
  reflect.Value.Interface()
      /usr/local/go/src/reflect/value.go:1000 +0x3c27
  fmt.(*pp).printValue()
      /usr/local/go/src/fmt/print.go:726 +0x3c28
  fmt.(*pp).printValue()
      /usr/local/go/src/fmt/print.go:869 +0xfd2
  fmt.(*pp).printValue()
      /usr/local/go/src/fmt/print.go:810 +0x296e
  fmt.(*pp).printValue()
      /usr/local/go/src/fmt/print.go:869 +0xfd2
  fmt.(*pp).printValue()
      /usr/local/go/src/fmt/print.go:810 +0x296e
  fmt.(*pp).printValue()
      /usr/local/go/src/fmt/print.go:869 +0xfd2
  fmt.(*pp).printValue()
      /usr/local/go/src/fmt/print.go:810 +0x296e
  fmt.(*pp).printValue()
      /usr/local/go/src/fmt/print.go:880 +0x2709
  fmt.(*pp).printArg()
      /usr/local/go/src/fmt/print.go:716 +0x25a
  fmt.(*pp).doPrintf()
      /usr/local/go/src/fmt/print.go:1030 +0x311
  fmt.Sprintf()
      /usr/local/go/src/fmt/print.go:219 +0x73
  github.com/cilium/cilium/pkg/policy/api.Rules.String()
      /go/src/github.com/cilium/cilium/pkg/policy/api/rules.go:34 +0x13c
  github.com/cilium/cilium/daemon/cmd.(*Daemon).policyAdd()
      /go/src/github.com/cilium/cilium/daemon/cmd/policy.go:265 +0x34b0
  github.com/cilium/cilium/daemon/cmd.(*PolicyAddEvent).Handle()
      /go/src/github.com/cilium/cilium/daemon/cmd/policy.go:217 +0xc9
  github.com/cilium/cilium/pkg/eventqueue.(*EventQueue).Run.func1()
      /go/src/github.com/cilium/cilium/pkg/eventqueue/eventqueue.go:260 +0x25d
  sync.(*Once).doSlow()
      /usr/local/go/src/sync/once.go:66 +0x103
  sync.(*Once).Do()
      /usr/local/go/src/sync/once.go:57 +0x68
```

Signed-off-by: André Martins <andre@cilium.io>
  • Loading branch information
aanm committed Sep 25, 2020
1 parent efe593b commit c1814ba
Showing 1 changed file with 5 additions and 2 deletions.
7 changes: 5 additions & 2 deletions pkg/k8s/cnp.go
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,11 @@ func (c *CNPStatusUpdateContext) prepareUpdate(cnp *types.SlimCNP, scopedLog *lo
return
}

serverRule = cnp
_, err = cnp.Parse()
// Make a copy since the rule is a pointer, and any of its fields
// which are also pointers could be modified outside of this
// function or with the 'cnp.Parse()' which can also overwrite fields.
serverRule = cnp.DeepCopy()
_, err = serverRule.Parse()
if err != nil {
log.WithError(err).WithField(logfields.Object, logfields.Repr(serverRule)).
Warn("Error parsing new CiliumNetworkPolicy rule")
Expand Down

0 comments on commit c1814ba

Please sign in to comment.