Skip to content

Commit

Permalink
ec2/mock: Fix data races
Browse files Browse the repository at this point in the history 
Data updates are passed in as pointers, deep copies must be made for
data passed in and for data passed out.

Fixes:
```
WARNING: DATA RACE
Write at 0x00c000276970 by goroutine 31:
  github.com/cilium/cilium/pkg/aws/ec2/mock.(*API).AssignPrivateIpAddresses()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/aws/ec2/mock/mock.go:296 +0x4f7
  github.com/cilium/cilium/pkg/aws/eni.(*Node).AllocateIPs()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/aws/eni/node.go:253 +0x102
  github.com/cilium/cilium/pkg/ipam.(*Node).maintainIPPool()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/ipam/node.go:502 +0xa0d
  github.com/cilium/cilium/pkg/ipam.(*Node).MaintainIPPool()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/ipam/node.go:532 +0x90
  github.com/cilium/cilium/pkg/ipam.(*NodeManager).Update.func1()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/ipam/node_manager.go:249 +0x8b
  github.com/cilium/cilium/pkg/trigger.(*Trigger).waiter()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/trigger/trigger.go:206 +0x4b9
```

```
WARNING: DATA RACE
Read at 0x0000048bffa8 by goroutine 25:
  github.com/cilium/cilium/pkg/aws/ec2/mock.(*API).CreateNetworkInterface()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/aws/ec2/mock/mock.go:199 +0x5fd
  github.com/cilium/cilium/pkg/aws/eni.(*Node).CreateInterface()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/aws/eni/node.go:387 +0x927
  github.com/cilium/cilium/pkg/ipam.(*Node).createInterface()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/ipam/node.go:330 +0x2d4
  github.com/cilium/cilium/pkg/ipam.(*Node).maintainIPPool()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/ipam/node.go:516 +0x85d
  github.com/cilium/cilium/pkg/ipam.(*Node).MaintainIPPool()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/ipam/node.go:532 +0x90
  github.com/cilium/cilium/pkg/ipam.(*NodeManager).Update.func1()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/ipam/node_manager.go:249 +0x8b
  github.com/cilium/cilium/pkg/trigger.(*Trigger).waiter()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/trigger/trigger.go:206 +0x4b9
```

```
WARNING: DATA RACE
Read at 0x00c0006aa9f0 by goroutine 30:
  github.com/cilium/cilium/pkg/aws/eni/types.(*ENI).DeepCopyInto()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/aws/eni/types/zz_generated.deepcopy.go:60 +0x65
  github.com/cilium/cilium/pkg/aws/eni/types.(*ENI).DeepCopy()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/aws/eni/types/zz_generated.deepcopy.go:82 +0x476
  github.com/cilium/cilium/pkg/aws/ec2/mock.(*API).GetInstances()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/aws/ec2/mock/mock.go:382 +0x1b8
  github.com/cilium/cilium/pkg/aws/eni.(*InstancesManager).Resync()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/aws/eni/instances.go:171 +0x851
  github.com/cilium/cilium/pkg/aws/eni.(*ENISuite).TestNodeManagerManyNodes()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/aws/eni/node_manager_test.go:570 +0xaf4
  runtime.call32()
      /usr/local/go/src/runtime/asm_amd64.s:539 +0x3a
  reflect.Value.Call()
      /usr/local/go/src/reflect/value.go:321 +0xd3
  gopkg.in/check%2ev1.(*suiteRunner).forkTest.func1()
      /home/vagrant/go/src/github.com/cilium/cilium/vendor/gopkg.in/check.v1/check.go:781 +0xa0a
  gopkg.in/check%2ev1.(*suiteRunner).forkCall.func1()
      /home/vagrant/go/src/github.com/cilium/cilium/vendor/gopkg.in/check.v1/check.go:675 +0xd9
```

Signed-off-by: Thomas Graf <thomas@cilium.io>
  • Loading branch information
@tgraf @aanm
tgraf authored and aanm committed Mar 17, 2020
1 parent cd4520d commit 4829aa7
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 8 deletions.
24 changes: 16 additions & 8 deletions pkg/aws/ec2/mock/mock.go
View file
Expand Up @@ -95,25 +95,33 @@ func NewAPI(subnets []*ipamTypes.Subnet, vpcs []*ipamTypes.VirtualNetwork, secur
// UpdateSubnets replaces the subents which the mock API will return
func (e *API) UpdateSubnets(subnets []*ipamTypes.Subnet) {
e.mutex.Lock()
e.subnets = map[string]*ipamTypes.Subnet{}
for _, s := range subnets {
e.subnets[s.ID] = s
e.subnets[s.ID] = s.DeepCopy()
}
e.mutex.Unlock()
}

// UpdateSecurityGroups replaces the security groups which the mock API will return
func (e *API) UpdateSecurityGroups(securityGroups []*types.SecurityGroup) {
e.mutex.Lock()
e.securityGroups = map[string]*types.SecurityGroup{}
for _, sg := range securityGroups {
e.securityGroups[sg.ID] = sg
e.securityGroups[sg.ID] = sg.DeepCopy()
}
e.mutex.Unlock()
}

// UpdateENIs replaces the ENIs which the mock API will return
func (e *API) UpdateENIs(enis map[string]ENIMap) {
e.mutex.Lock()
e.enis = enis
e.enis = map[string]ENIMap{}
for instanceID, m := range enis {
e.enis[instanceID] = ENIMap{}
for eniID, eni := range m {
e.enis[instanceID][eniID] = eni.DeepCopy()
}
}
e.mutex.Unlock()
}

Expand Down Expand Up @@ -199,7 +207,7 @@ func (e *API) CreateNetworkInterface(ctx context.Context, toAllocate int64, subn
subnet.AvailableAddresses -= int(toAllocate)

e.unattached[eniID] = eni
return eniID, eni, nil
return eniID, eni.DeepCopy(), nil
}

func (e *API) DeleteNetworkInterface(ctx context.Context, eniID string) error {
Expand Down Expand Up @@ -371,7 +379,7 @@ func (e *API) GetInstances(ctx context.Context, vpcs ipamTypes.VirtualNetworkMap
}
}

instances.Add(instanceID, eni)
instances.Add(instanceID, eni.DeepCopy())
}
}

Expand All @@ -385,7 +393,7 @@ func (e *API) GetVpcs(ctx context.Context) (ipamTypes.VirtualNetworkMap, error)
defer e.mutex.RUnlock()

for _, v := range e.vpcs {
vpcs[v.ID] = v
vpcs[v.ID] = v.DeepCopy()
}
return vpcs, nil
}
Expand All @@ -397,7 +405,7 @@ func (e *API) GetSubnets(ctx context.Context) (ipamTypes.SubnetMap, error) {
defer e.mutex.RUnlock()

for _, s := range e.subnets {
subnets[s.ID] = s
subnets[s.ID] = s.DeepCopy()
}
return subnets, nil
}
Expand All @@ -423,7 +431,7 @@ func (e *API) GetSecurityGroups(ctx context.Context) (types.SecurityGroupMap, er
defer e.mutex.RUnlock()

for _, sg := range e.securityGroups {
securityGroups[sg.ID] = sg
securityGroups[sg.ID] = sg.DeepCopy()
}
return securityGroups, nil
}
17 changes: 17 additions & 0 deletions pkg/aws/ec2/mock/mock_test.go
View file
Expand Up @@ -22,6 +22,7 @@ import (
"testing"

"github.com/cilium/cilium/pkg/aws/types"
"github.com/cilium/cilium/pkg/checker"
ipamTypes "github.com/cilium/cilium/pkg/ipam/types"

"gopkg.in/check.v1"
Expand Down Expand Up @@ -72,6 +73,22 @@ func (e *MockSuite) TestMock(c *check.C) {
c.Assert(ok, check.Equals, false)
_, ok = api.enis["i-1"][eniID2]
c.Assert(ok, check.Equals, false)

sg1 := &types.SecurityGroup{
ID: "sg1",
VpcID: "vpc-1",
Tags: map[string]string{"k1": "v1"},
}
sg2 := &types.SecurityGroup{
ID: "sg2",
VpcID: "vpc-1",
Tags: map[string]string{"k1": "v1"},
}
api.UpdateSecurityGroups([]*types.SecurityGroup{sg1, sg2})

sgMap, err := api.GetSecurityGroups(context.TODO())
c.Assert(err, check.IsNil)
c.Assert(sgMap, checker.DeepEquals, types.SecurityGroupMap{"sg1": sg1, "sg2": sg2})
}

func (e *MockSuite) TestSetMockError(c *check.C) {
Expand Down

0 comments on commit 4829aa7

Please sign in to comment.