istio / api

API definitions for the Istio project
Apache License 2.0
460 stars 556 forks source link

DeepCopy for generated API types is not safe for concurrent use #3019

Open nrfox opened 11 months ago

nrfox commented 11 months ago

Bug description When calling obj.DeepCopy() on one of the generated API types concurrently, the go race detector throws some warnings about a data race.

Affected product area (please put an X in all that apply)

[ ] Configuration Infrastructure [ ] Docs [ ] Installation [ ] Networking [ ] Performance and Scalability [ ] Policies and Telemetry [ ] Security [ ] Test and Release [X] User Experience

Expected behavior Expected to call obj.DeepCopy() concurrently without any data race issues. This works with k8s.io/api types.

Steps to reproduce the bug Here's a small reproducer. Run this with the -race flag: go run -race main.go

package main

import (
    networking_v1beta1 "istio.io/client-go/pkg/apis/networking/v1beta1"
    // core_v1 "k8s.io/api/core/v1"
)

func main() {
    const iterations = 10000

    done := make(chan struct{})
    obj := &networking_v1beta1.VirtualService{}
    // obj := &core_v1.Service{}
    go func() {
        for i := 0; i < iterations; i++ {
            obj.DeepCopy()
        }
        done <- struct{}{}
    }()
    go func() {
        for i := 0; i < iterations; i++ {
            obj.DeepCopy()
        }
        done <- struct{}{}
    }()
    <-done
    <-done
}

output:

==================
WARNING: DATA RACE
Read at 0x00c0001c5e48 by goroutine 7:
  istio.io/client-go/pkg/apis/networking/v1beta1.(*VirtualService).DeepCopyInto()
      /home/nrfox/go/pkg/mod/istio.io/client-go@v1.20.0/pkg/apis/networking/v1beta1/zz_generated.deepcopy.gen.go:353 +0x44
  istio.io/client-go/pkg/apis/networking/v1beta1.(*VirtualService).DeepCopy()
      /home/nrfox/go/pkg/mod/istio.io/client-go@v1.20.0/pkg/apis/networking/v1beta1/zz_generated.deepcopy.gen.go:367 +0xa4
  main.main.func1()
      /home/nrfox/Developer/redhat/kiali/testingmain/main.go:16 +0xaf

Previous write at 0x00c0001c5e48 by goroutine 8:
  sync/atomic.StoreInt64()
      /usr/local/go/src/runtime/race_amd64.s:237 +0xb
  sync/atomic.StorePointer()
      /usr/local/go/src/runtime/atomic_pointer.go:72 +0x44
  istio.io/api/networking/v1beta1.(*VirtualService).ProtoReflect()
      /home/nrfox/go/pkg/mod/istio.io/api@v1.20.0-beta.0.0.20231031143729-871b2914253f/networking/v1beta1/virtual_service.pb.go:374 +0x7c
  google.golang.org/protobuf/proto.Clone()
      /home/nrfox/go/pkg/mod/google.golang.org/protobuf@v1.31.0/proto/merge.go:53 +0x44
  istio.io/api/networking/v1beta1.(*VirtualService).DeepCopyInto()
      /home/nrfox/go/pkg/mod/istio.io/api@v1.20.0-beta.0.0.20231031143729-871b2914253f/networking/v1beta1/virtual_service_deepcopy.gen.go:10 +0x164
  istio.io/client-go/pkg/apis/networking/v1beta1.(*VirtualService).DeepCopyInto()
      /home/nrfox/go/pkg/mod/istio.io/client-go@v1.20.0/pkg/apis/networking/v1beta1/zz_generated.deepcopy.gen.go:356 +0x145
  istio.io/client-go/pkg/apis/networking/v1beta1.(*VirtualService).DeepCopy()
      /home/nrfox/go/pkg/mod/istio.io/client-go@v1.20.0/pkg/apis/networking/v1beta1/zz_generated.deepcopy.gen.go:367 +0xa4
  main.main.func2()
      /home/nrfox/Developer/redhat/kiali/testingmain/main.go:22 +0xaf

Goroutine 7 (running) created at:
  main.main()
      /home/nrfox/Developer/redhat/kiali/testingmain/main.go:14 +0x136

Goroutine 8 (running) created at:
  main.main()
      /home/nrfox/Developer/redhat/kiali/testingmain/main.go:20 +0x1de
==================
Found 1 data race(s)
exit status 66

You can comment out the VirtualService obj and uncomment the Service object to see the behavior with a k8s.io/api type. There are no data race errors.

Version (include the output of istioctl version --remote and kubectl version) From my go.mod

    istio.io/api v1.20.0-beta.0.0.20231031143729-871b2914253f
    istio.io/client-go v1.20.0

How was Istio installed? N/A

Environment where bug was observed (cloud vendor, OS, etc) N/A

howardjohn commented 11 months ago

I think removal of *out = *in fixes it, but we should understand why that is done before make any changes