plugins/ecs-bridge/engine: fix race conditions.

When multiple awsvpc tasks launched at the same time, creating and configuring the bridge might fail because of race conditions. Fixing by:
(1) When creating the bridge, check whether the bridge exists after failing to create it.
(2) When assigning ip address to the bridge, check whether the ip address is already assigned after failing to assign it.

Makefile

@@ -71,6 +71,9 @@ unit-test: $(SOURCES)
 integration-test: $(SOURCE)
 	go test -v -tags integration -race -timeout 10s ./pkg/... ./plugins/...
 
+sudo-integration-test: $(SOURCE)
+	sudo -E ${GO_EXECUTABLE} test -v -tags "sudo integration" -race -timeout 10s ./plugins/...
+
 e2e-test:  $(SOURCE) plugins
 	sudo -E CNI_PATH=${ROOT}/bin/plugins ${GO_EXECUTABLE} test -v -tags e2e -race -timeout 120s ./plugins/...
 

plugins/ecs-bridge/commands/commands.go

@@ -27,21 +27,28 @@ import (
 // connect container's namespace with the bridge
 func Add(args *skel.CmdArgs) error {
 	defer log.Flush()
-	return add(args, engine.New())
+	err := add(args, engine.New())
+	if err != nil {
+		log.Errorf("Error executing ADD command: %v", err)
+	}
+
+	return err
 }
 
 // Del invokes the command to tear down the bridge and the veth pair
 func Del(args *skel.CmdArgs) error {
 	defer log.Flush()
-	return del(args, engine.New())
+	err := del(args, engine.New())
+	if err != nil {
+		log.Errorf("Error executing DEL command: %v", err)
+	}
+
+	return err
 }
 
 func add(args *skel.CmdArgs, engine engine.Engine) error {
 	conf, err := types.NewConf(args)
 	if err != nil {
-		// TODO: We log and return errors throughout this function.
-		// Either should be sufficient.
-		log.Errorf("Error loading config from args: %v", err)
 		return err
 	}
 
@@ -108,9 +115,6 @@ func add(args *skel.CmdArgs, engine engine.Engine) error {
 func del(args *skel.CmdArgs, engine engine.Engine) error {
 	conf, err := types.NewConf(args)
 	if err != nil {
-		// TODO: We log and return errors throughout this function.
-		// Either should be sufficient.
-		log.Errorf("Error loading config from args: %v", err)
 		return err
 	}
 

plugins/ecs-bridge/engine/engine.go

@@ -15,6 +15,7 @@ package engine
 
 import (
 	"net"
+	"strings"
 	"syscall"
 
 	"github.com/aws/amazon-ecs-cni-plugins/pkg/cniipamwrapper"
@@ -26,10 +27,14 @@ import (
 	"github.com/vishvananda/netlink"
 )
 
-// zeroLengthIPString is what we expect net.IP.String() to return if the
-// ip has length 0. We use this to determing if an IP is empty.
-// Refer https://golang.org/pkg/net/#IP.String
-const zeroLengthIPString = "<nil>"
+const  (
+	// zeroLengthIPString is what we expect net.IP.String() to return if the
+	// ip has length 0. We use this to determing if an IP is empty.
+	// Refer https://golang.org/pkg/net/#IP.String
+	zeroLengthIPString = "<nil>"
+
+	fileExistsErrMsg = "file exists"
+)
 
 // Engine represents the execution engine for the ECS Bridge plugin.
 // It defines all the operations performed during the execution of the
@@ -73,7 +78,12 @@ func (engine *engine) CreateBridge(bridgeName string, mtu int) (*netlink.Bridge,
 	if bridge == nil {
 		err = engine.createBridge(bridgeName, mtu)
 		if err != nil {
-			return nil, err
+			if !strings.Contains(err.Error(), fileExistsErrMsg) {
+				return nil, err
+			}
+			// If the error returned by createBridge is that the bridge already exists, proceed to
+			// lookupBridge because that means the bridge was created by someone else right before
+			// we tried creating it, which is fine
 		}
 
 		// We need to lookup the bridge link again because LinkAdd
@@ -241,14 +251,21 @@ func (engine *engine) ConfigureBridge(result *current.Result, bridge *netlink.Br
 	bridgeAddr := &netlink.Addr{
 		IPNet: resultBridgeNetwork,
 	}
-	err = engine.netLink.AddrAdd(bridge, bridgeAddr)
-	if err != nil {
-		return errors.Wrapf(err,
-			"bridge configure: unable to assign ip address to bridge %s",
-			bridge.Attrs().Name)
+
+	addrAddErr := engine.netLink.AddrAdd(bridge, bridgeAddr)
+	if addrAddErr == nil {
+		return nil
 	}
 
-	return nil
+	if strings.Contains(addrAddErr.Error(), fileExistsErrMsg) {
+		// if we fail to assign ip address because of a file exist error, the error is probably caused by someone else
+		// assigned the address right before we assigned it, which is fine
+		return nil
+	}
+
+	return errors.Wrapf(addrAddErr,
+		"bridge configure: unable to assign ip address to bridge %s",
+		bridge.Attrs().Name)
 }
 
 // GetInterfaceIPV4Address gets the ipv4 address of a given interface

plugins/ecs-bridge/engine/engine_integ_test.go

@@ -0,0 +1,104 @@
+// +build sudo,integration
+
+// Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License"). You may
+// not use this file except in compliance with the License. A copy of the
+// License is located at
+//
+//     http://aws.amazon.com/apache2.0/
+//
+// or in the "license" file accompanying this file. This file is distributed
+// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
+// express or implied. See the License for the specific language governing
+// permissions and limitations under the License.
+
+package engine
+
+import (
+	"github.com/aws/amazon-ecs-cni-plugins/pkg/netlinkwrapper"
+	"github.com/containernetworking/cni/pkg/types/current"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"github.com/vishvananda/netlink"
+	"net"
+	"os"
+	"testing"
+)
+
+const (
+	testBridgeName    = "test-ecs-bridge"
+	testMTU           = 1500
+	testGatewayIPCIDR = "172.31.0.1"
+	testdb            = "/tmp/__boltdb_test"
+)
+
+func cleanup(t *testing.T) {
+	_, err := os.Stat(testdb)
+	if err != nil {
+		require.True(t, os.IsNotExist(err), "if it's not file not exist error, then there should be a problem: %v", err)
+	} else {
+		err = os.Remove(testdb)
+		require.NoError(t, err, "Remove the existed db should not cause error")
+	}
+
+	// clean up the test bridge, if it's created
+	testBridge, err := netlink.LinkByName(testBridgeName)
+	if err == nil {
+		err = netlink.LinkDel(testBridge)
+		assert.NoError(t, err)
+	}
+}
+
+func TestCreateBridgeAlreadyExists(t *testing.T) {
+	defer cleanup(t)
+
+	testEngine := &engine{
+		netLink: netlinkwrapper.NewNetLink(),
+	}
+	err := testEngine.createBridge(testBridgeName, testMTU)
+	require.NoError(t, err)
+
+	// try creating the bridge again, expect getting a "file exists" error
+	err = testEngine.createBridge(testBridgeName, testMTU)
+	assert.Error(t, err)
+	assert.Contains(t, err.Error(), fileExistsErrMsg)
+}
+
+func TestConfigureBridgeNetworkAlreadyExists(t *testing.T) {
+	defer cleanup(t)
+
+	gatewayIPAddr := net.ParseIP(testGatewayIPCIDR)
+
+	testEngine := &engine{
+		netLink: netlinkwrapper.NewNetLink(),
+	}
+
+	ipConfig := &current.IPConfig{
+		Address: net.IPNet{
+			Mask: net.CIDRMask(31, 32),
+		},
+		Gateway: gatewayIPAddr,
+	}
+
+	result := &current.Result{
+		IPs: []*current.IPConfig{ipConfig},
+	}
+
+	// create and configure a bridge
+	testBridge, err := testEngine.CreateBridge(testBridgeName, testMTU)
+	require.NoError(t, err)
+
+	err = testEngine.ConfigureBridge(result, testBridge)
+	require.NoError(t, err)
+
+	// check that we get a "file exists" error when trying to assign same address to the bridge
+	err = netlink.AddrAdd(testBridge, &netlink.Addr{
+		IPNet: &net.IPNet{
+			IP:   result.IPs[0].Gateway,
+			Mask: result.IPs[0].Address.Mask,
+		},
+	})
+	assert.Error(t, err)
+	assert.Contains(t, err.Error(), fileExistsErrMsg)
+}

plugins/ecs-bridge/engine/engine_test.go

@@ -133,7 +133,23 @@ func TestCreateBridgeLookupBridgeError(t *testing.T) {
 	assert.Error(t, err)
 }
 
-func TestCreateBridgeLinkAddError(t *testing.T) {
+func TestCreateBridgeLinkAddExistErrorLinkSetUpSuccess(t *testing.T) {
+	ctrl, _, mockNetLink, _, _, _ := setup(t)
+	defer ctrl.Finish()
+
+	bridgeLink := &netlink.Bridge{}
+	gomock.InOrder(
+		mockNetLink.EXPECT().LinkByName(bridgeName).Return(nil, netlink.LinkNotFoundError{}),
+		mockNetLink.EXPECT().LinkAdd(gomock.Any()).Return(errors.New("file exists")),
+		mockNetLink.EXPECT().LinkByName(bridgeName).Return(bridgeLink, nil),
+		mockNetLink.EXPECT().LinkSetUp(bridgeLink).Return(nil),
+	)
+	engine := &engine{netLink: mockNetLink}
+	_, err := engine.CreateBridge(bridgeName, mtu)
+	assert.NoError(t, err)
+}
+
+func TestCreateBridgeLinkAddOtherError(t *testing.T) {
 	ctrl, _, mockNetLink, _, _, _ := setup(t)
 	defer ctrl.Finish()
 
@@ -640,7 +656,39 @@ func TestConfigureBridgeAddrListWhenNotFound(t *testing.T) {
 	assert.Error(t, err)
 }
 
-func TestConfigureBridgeAddrAddError(t *testing.T) {
+func TestConfigureBridgeAddrAddFileExistsError(t *testing.T) {
+	ctrl, _, mockNetLink, _, _, _ := setup(t)
+	defer ctrl.Finish()
+
+	bridgeLink := &netlink.Bridge{}
+	gatewayIPAddr := net.ParseIP(gatewayIPCIDR)
+	ipConfig := &current.IPConfig{
+		Address: net.IPNet{
+			Mask: net.CIDRMask(31, 32),
+		},
+		Gateway: gatewayIPAddr,
+	}
+
+	result := &current.Result{
+		IPs: []*current.IPConfig{ipConfig},
+	}
+
+	bridgeAddr := &netlink.Addr{
+		IPNet: &net.IPNet{
+			IP:   gatewayIPAddr,
+			Mask: net.CIDRMask(31, 32),
+		},
+	}
+	gomock.InOrder(
+		mockNetLink.EXPECT().AddrList(bridgeLink, syscall.AF_INET).Return(nil, nil),
+		mockNetLink.EXPECT().AddrAdd(bridgeLink, bridgeAddr).Return(errors.New("file exists")),
+	)
+	engine := &engine{netLink: mockNetLink}
+	err := engine.ConfigureBridge(result, bridgeLink)
+	assert.NoError(t, err)
+}
+
+func TestConfigureBridgeAddrAddOtherError(t *testing.T) {
 	ctrl, _, mockNetLink, _, _, _ := setup(t)
 	defer ctrl.Finish()