plugins/eni/engine: replace scanning sysfs tree with netlink for devi…

…ce search

Use netlink.LinkList instead of browsing the sysfs subtree to determine the
device name of the ENI. This is much cleaner than the earlier implementation,
which involved reading directories and files.

Resolves #10

plugins/eni/engine/engine.go

@@ -31,8 +31,6 @@ import (
 const (
 	metadataNetworkInterfacesPath               = "network/interfaces/macs/"
 	metadataNetworkInterfaceIDPathSuffix        = "interface-id"
-	sysfsPathForNetworkDevices                  = "/sys/class/net/"
-	sysfsPathForNetworkDeviceAddressSuffix      = "/address"
 	metadataNetworkInterfaceIPV4CIDRPathSuffix  = "/subnet-ipv4-cidr-block"
 	metadataNetworkInterfaceIPV4AddressesSuffix = "/local-ipv4s"
 )
@@ -121,29 +119,18 @@ func (engine *engine) GetMACAddressOfENI(macAddresses []string, eniID string) (s
 
 // GetInterfaceDeviceName gets the device name on the host, given a mac address
 func (engine *engine) GetInterfaceDeviceName(macAddress string) (string, error) {
-	// TODO: Use LinkList for this
-	files, err := engine.ioutil.ReadDir(sysfsPathForNetworkDevices)
+	hardwareAddr, err := net.ParseMAC(macAddress)
 	if err != nil {
-		return "", errors.Wrap(err,
-			"getInterfaceDeviceName engine: error listing network devices from sys fs")
+		return "", errors.Wrapf(err, "getInterfaceDeviceName engine: malformatted mac address specified")
 	}
-	for _, file := range files {
-		// Read the 'address' file in sys for the device. An example here is: if reading for device
-		// 'eth1', read the '/sys/class/net/eth1/address' file to get the address of the device
-		// TODO Use fmt.Sprintf and wrap that in a method
-		addressFile := sysfsPathForNetworkDevices + file.Name() + sysfsPathForNetworkDeviceAddressSuffix
-		contents, err := engine.ioutil.ReadFile(addressFile)
-		if err != nil {
-			log.Warnf("Error reading contents of the address file for device '%s': %v", file.Name(), err)
-			continue
-		}
-		if strings.Contains(string(contents), macAddress) {
-			return file.Name(), nil
-		}
+
+	link, err := getLinkByHardwareAddress(engine.netLink, hardwareAddr)
+	if err != nil {
+		return "", errors.Wrapf(err,
+			"getInterfaceDeviceName engine: unable to get device with hardware address '%s'", macAddress)
 	}
 
-	return "", newUnmappedDeviceNameError("getInterfaceDeviceName", "engine",
-		fmt.Sprintf("network device name not found for mac address '%s'", macAddress))
+	return link.Attrs().Name, nil
 }
 
 // GetIPV4GatewayNetmask gets the ipv4 gateway and the netmask from the instance

plugins/eni/engine/engine_test.go

@@ -16,14 +16,12 @@ package engine
 import (
 	"errors"
 	"net"
-	"os"
 	"testing"
 
 	"github.com/aws/amazon-ecs-cni-plugins/pkg/cninswrapper/mocks"
 	"github.com/aws/amazon-ecs-cni-plugins/pkg/cninswrapper/mocks_netns"
 	"github.com/aws/amazon-ecs-cni-plugins/pkg/ec2metadata/mocks"
 	"github.com/aws/amazon-ecs-cni-plugins/pkg/execwrapper/mocks"
-	"github.com/aws/amazon-ecs-cni-plugins/pkg/ioutilwrapper/mocks_fileinfo"
 	"github.com/aws/amazon-ecs-cni-plugins/pkg/ioutilwrapper/mocks_ioutilwrapper"
 	"github.com/aws/amazon-ecs-cni-plugins/pkg/netlinkwrapper/mocks"
 	"github.com/aws/amazon-ecs-cni-plugins/pkg/netlinkwrapper/mocks_link"
@@ -166,78 +164,76 @@ func TestGetMACAddressOfENI(t *testing.T) {
 	assert.Equal(t, addr, secondMACAddressSanitized)
 }
 
-func TestGetInterfaceDeviceNameReturnsErrorOnReadDirError(t *testing.T) {
-	ctrl, _, mockIOUtil, _, _, _, _ := setup(t)
+func TestGetInterfaceDeviceNameReturnsErrorOnInvalidMACAddress(t *testing.T) {
+	ctrl, _, _, _, _, _, _ := setup(t)
 	defer ctrl.Finish()
 
-	mockIOUtil.EXPECT().ReadDir(sysfsPathForNetworkDevices).Return(nil, errors.New("error"))
-	engine := &engine{ioutil: mockIOUtil}
+	engine := &engine{}
 
-	_, err := engine.GetInterfaceDeviceName(firstMACAddressSanitized)
+	_, err := engine.GetInterfaceDeviceName("")
 	assert.Error(t, err)
-	_, ok := err.(*unmappedDeviceNameError)
-	assert.False(t, ok)
 }
 
-func TestGetInterfaceDeviceNameReturnsErrorOnReadFileError(t *testing.T) {
-	ctrl, _, mockIOUtil, _, _, _, _ := setup(t)
+func TestGetInterfaceDeviceNameReturnsErrorOnLinkListErrort(t *testing.T) {
+	ctrl, _, _, _, mockNetLink, _, _ := setup(t)
 	defer ctrl.Finish()
 
-	mockFileInfo := mock_os.NewMockFileInfo(ctrl)
-	gomock.InOrder(
-		mockIOUtil.EXPECT().ReadDir(sysfsPathForNetworkDevices).Return([]os.FileInfo{mockFileInfo}, nil),
-		mockFileInfo.EXPECT().Name().Return(deviceName),
-		mockIOUtil.EXPECT().ReadFile(sysfsPathForNetworkDevices+deviceName+sysfsPathForNetworkDeviceAddressSuffix).Return(nil, errors.New("error")),
-		mockFileInfo.EXPECT().Name().Return(deviceName),
-	)
-	engine := &engine{ioutil: mockIOUtil}
+	mockNetLink.EXPECT().LinkList().Return(nil, errors.New("error"))
+	engine := &engine{netLink: mockNetLink}
 
-	_, err := engine.GetInterfaceDeviceName(firstMACAddressSanitized)
+	_, err := engine.GetInterfaceDeviceName(eniMACAddress)
 	assert.Error(t, err)
-	_, ok := err.(*unmappedDeviceNameError)
-	assert.True(t, ok)
 }
 
 func TestGetInterfaceDeviceNameReturnsErrorWhenDeviceNotFound(t *testing.T) {
-	ctrl, _, mockIOUtil, _, _, _, _ := setup(t)
+	ctrl, _, _, _, mockNetLink, _, _ := setup(t)
 	defer ctrl.Finish()
 
-	mockFileInfo := mock_os.NewMockFileInfo(ctrl)
+	lo := mock_netlink.NewMockLink(ctrl)
+	eth1 := mock_netlink.NewMockLink(ctrl)
+
+	loAddress, err := net.ParseMAC(loMACAddress)
+	assert.NoError(t, err)
+	eth1Address, err := net.ParseMAC(eniMACAddress)
+	assert.NoError(t, err)
+
 	gomock.InOrder(
-		mockIOUtil.EXPECT().ReadDir(sysfsPathForNetworkDevices).Return([]os.FileInfo{mockFileInfo}, nil),
-		mockFileInfo.EXPECT().Name().Return(deviceName),
-		mockIOUtil.EXPECT().ReadFile(
-			sysfsPathForNetworkDevices+deviceName+sysfsPathForNetworkDeviceAddressSuffix).Return([]byte(secondMACAddressSanitized), nil),
+		mockNetLink.EXPECT().LinkList().Return([]netlink.Link{lo, eth1}, nil),
+		lo.EXPECT().Attrs().Return(&netlink.LinkAttrs{HardwareAddr: loAddress}),
+		eth1.EXPECT().Attrs().Return(&netlink.LinkAttrs{HardwareAddr: eth1Address}),
 	)
-	engine := &engine{ioutil: mockIOUtil}
 
-	_, err := engine.GetInterfaceDeviceName(firstMACAddressSanitized)
+	engine := &engine{netLink: mockNetLink}
+	_, err = engine.GetInterfaceDeviceName(unknownMACAddress)
 	assert.Error(t, err)
-	_, ok := err.(*unmappedDeviceNameError)
-	assert.True(t, ok)
 }
 
 func TestGetInterfaceDeviceNameReturnsDeviceWhenFound(t *testing.T) {
-	ctrl, _, mockIOUtil, _, _, _, _ := setup(t)
+	ctrl, _, _, _, mockNetLink, _, _ := setup(t)
 	defer ctrl.Finish()
 
-	mockFileInfoEth1 := mock_os.NewMockFileInfo(ctrl)
-	mockFileInfoEth2 := mock_os.NewMockFileInfo(ctrl)
+	lo := mock_netlink.NewMockLink(ctrl)
+	eth1 := mock_netlink.NewMockLink(ctrl)
+
+	loAddress, err := net.ParseMAC(loMACAddress)
+	assert.NoError(t, err)
+	eth1Address, err := net.ParseMAC(eniMACAddress)
+	assert.NoError(t, err)
+
 	gomock.InOrder(
-		mockIOUtil.EXPECT().ReadDir(sysfsPathForNetworkDevices).Return([]os.FileInfo{mockFileInfoEth1, mockFileInfoEth2}, nil),
-		mockFileInfoEth1.EXPECT().Name().Return(deviceName),
-		mockIOUtil.EXPECT().ReadFile(
-			sysfsPathForNetworkDevices+deviceName+sysfsPathForNetworkDeviceAddressSuffix).Return([]byte(firstMACAddressSanitized), nil),
-		mockFileInfoEth2.EXPECT().Name().Return("eth2"),
-		mockIOUtil.EXPECT().ReadFile(
-			sysfsPathForNetworkDevices+"eth2"+sysfsPathForNetworkDeviceAddressSuffix).Return([]byte(secondMACAddressSanitized), nil),
-		mockFileInfoEth2.EXPECT().Name().Return("eth2"),
+		mockNetLink.EXPECT().LinkList().Return([]netlink.Link{lo, eth1}, nil),
+		lo.EXPECT().Attrs().Return(&netlink.LinkAttrs{HardwareAddr: loAddress}),
+		eth1.EXPECT().Attrs().Return(&netlink.LinkAttrs{HardwareAddr: eth1Address}),
+		eth1.EXPECT().Attrs().Return(&netlink.LinkAttrs{
+			HardwareAddr: eth1Address,
+			Name:         deviceName,
+		}),
 	)
-	engine := &engine{ioutil: mockIOUtil}
 
-	deviceName, err := engine.GetInterfaceDeviceName(secondMACAddressSanitized)
+	engine := &engine{netLink: mockNetLink}
+	eniDeviceName, err := engine.GetInterfaceDeviceName(eniMACAddress)
 	assert.NoError(t, err)
-	assert.Equal(t, deviceName, "eth2")
+	assert.Equal(t, eniDeviceName, deviceName)
 }
 
 func TestGetIPV4GatewayNetMaskLocalReturnsErrorOnMalformedCIDR(t *testing.T) {

plugins/eni/engine/error.go

@@ -46,26 +46,6 @@ func newUnmappedMACAddressError(operation string, origin string, message string)
 	}
 }
 
-// unmappedDeviceNameError is used to indicate that the the MAC address of the
-// ENI cannot be mapped to any of the network devices available on the host
-type unmappedDeviceNameError struct {
-	err *_error
-}
-
-func (devNameErr *unmappedDeviceNameError) Error() string {
-	return devNameErr.err.Error()
-}
-
-func newUnmappedDeviceNameError(operation string, origin string, message string) error {
-	return &unmappedDeviceNameError{
-		err: &_error{
-			operation: operation,
-			origin:    origin,
-			message:   message,
-		},
-	}
-}
-
 // parseIPV4GatewayNetmaskError is used to indicate any error with parsing the
 // IPV4 address and the netmask of the ENI
 type parseIPV4GatewayNetmaskError struct {

plugins/eni/engine/nsclosure.go

@@ -152,7 +152,8 @@ func constructDHClientLeasePIDFilePath(deviceName string) string {
 func (closure *teardownNamespaceClosure) run(_ ns.NetNS) error {
 	link, err := getLinkByHardwareAddress(closure.netLink, closure.hardwareAddr)
 	if err != nil {
-		return err
+		return errors.Wrapf(err,
+			"teardownNamespaceClosure engine: unable to get device with hardware address '%s'", closure.hardwareAddr.String())
 	}
 
 	deviceName := link.Attrs().Name
@@ -190,8 +191,7 @@ func (closure *teardownNamespaceClosure) run(_ ns.NetNS) error {
 func getLinkByHardwareAddress(netLink netlinkwrapper.NetLink, hardwareAddr net.HardwareAddr) (netlink.Link, error) {
 	links, err := netLink.LinkList()
 	if err != nil {
-		errors.Wrapf(err,
-			"teardownNamespaceClosure engine: unable to list device and attributes")
+		return nil, err
 	}
 
 	for _, link := range links {