FIX_Ethtool_filters (#63)

Ethtool feature moved to CNI where filters now assigned via the networkAttachmentDefinition.
DeviceFile functions removed.

-Ethtool unit tests removed from device plugin.
-Corrected changes from previous commit
-Amended readme section
-example network-attachment-definition.yaml updated

---------

Signed-off-by: Patrick O Grady <[email protected]>

Author: patrickog11

README.md

@@ -403,10 +403,6 @@ Below are some additional optional configurations that can be applied to pools.
 UID is an integer configuration. It is useful in scenarios where the AF_XDP pod runs as a non-zero user. This configuration can be used to inform the device plugin about the user ID of the pod. This allows that non-zero user to use the UDS without issue. If unset, then only user 0 can use the UDS.
 Note: User 0 does not imply that the pod needs to be privileged.
 
-#### EthtoolCmds
-
-EthtoolCmds is an array of strings. This is a setting that can be applied to devices in a `primary` mode pool. Here the user can provide a list of Ethtool filters to apply to the devices as they are being allocated to a pod. These strings should be formatted exactly as if setting Ethtool filters manually from the command line. Some Ethtool filters require the netdev name or the IP address and in these instances, the user can substitute these with `-device-` and `-ip-`  respectively. The plugins will apply the filters with the correct name and IP address when they become known during pod creation.
-
 #### UdsServerDisable
 
 UdsServerDisable is a Boolean configuration. If set to true, devices in this pool will not have the BPF app loaded onto the netdev. This means no UDS server is spun up when a device is allocated to a pod. By default, this is set to false.
@@ -425,13 +421,12 @@ The example below has two pools configured.
 
 The first pool:
 
-- Has the **name** `myPrimarypool`.
-- Is in `primary` **mode**, meaning no secondary devices will be created. Pods requesting `afxdp/myPrimarypool` will be allocated the full NIC port.
-- The **drivers** field for this pool has one driver, `i40e`, meaning this pool will be assigned i40e devices, where available.
-- The **uid** field for this pool is set to `1500` meaning the AF_XDP pod can run as user 1500 and use the UDS without issue.
-- The **udsTimeout** field for this pool is set to `300`, meaning the UDS server will only time out and terminate after 5 minutes of inactivity on the UDS.
-- The **RequiresUnprivilegedBpf** field is set to `true` meaning this pool will only be assigned devices from nodes where unprivileged eBPF is allowed.
-- Finally, the **ethtoolCmds** field has two filters configured. This means the filters `ethtool -X <device> equal 5 start 3` and `ethtool --config-ntuple -device- flow-type udp4 dst-ip <ip> action` will be configured on all devices as they are being attached to the AF_XDP pods. The plugins will substitute `<device>` and `<ip>` accordingly.
+ - Has the **name** `myPrimarypool`.
+ - Is in `primary` **mode**, meaning no secondary devices will be created. Pods requesting `afxdp/myPrimarypool` will be allocated the full NIC port.
+ - The **drivers** field for this pool has one driver, `i40e`, meaning this pool will be assigned i40e devices, where available.
+ - The **uid** field for this pool is set to `1500` meaning the AF_XDP pod can run as user 1500 and use the UDS without issue.
+ - The **udsTimeout** field for this pool is set to `300`, meaning the UDS server will only time out and terminate after 5 minutes of inactivity on the UDS.
+ - The **RequiresUnprivilegedBpf** field is set to `true` meaning this pool will only be assigned devices from nodes where unprivileged eBPF is allowed.
 
 The second pool:
 
@@ -444,15 +439,11 @@ The second pool:
 {
    "pools":[
       {
-         "name": "myPrimarypool",
-         "mode": "primary",
-         "uid": 1500,
-         "udsTimeout": 300,
-         "RequiresUnprivilegedBpf": true,
-         "ethtoolCmds":[
-            "-X -device- equal 5 start 3",
-            "--config-ntuple -device- flow-type udp4 dst-ip -ip- action"
-         ],
+         "name":"myPrimarypool",
+         "mode":"primary",
+         "uid":1500,
+         "udsTimeout":300,
+         "RequiresUnprivilegedBpf":true,
          "drivers":[
             {
                "name": "i40e"
@@ -533,6 +524,14 @@ The kindCluster flag is used to indicate if this is a physical cluster or a Kind
     }
 ```
 
+### Ethtool Filters
+Ethtool filters can be applied to devices during CNI runtime. This setting can only be assigned to devices in a `primary` mode pool from the networkAttachmentDefinition file.
+The EthtoolCmds field is an array of strings. These strings should be formatted exactly as if setting Ethtool filters where manually from the command line. Some Ethtool filters require the device name or the IP address and in these instances, the user can substitute these with `-device-` and `-ip-` respectively. The plugins will apply the filters with the correct name and IP address when they become known during pod creation.
+
+From the [examples/network-attachment-definition.yaml](./examples/network-attachment-definition.yaml) file: **ethtoolCmds** field has two filters configured. This means the filters `ethtool -X <device> equal 5 start 3` and `ethtool --config-ntuple -device- flow-type udp4 dst-ip <ip> action` will be configured on all devices as they are being attached to the AF_XDP pods. The plugins will substitute `<device>` and `<ip>` accordingly.
+
+*Note: When setting ethtool commands in the **ethtoolCmds** field, the 'ethtool' prefix must be removed.
+
 ## CLOC
 
 Output from CLOC (count lines of code) - github.com/AlDanial/cloc

constants/constants.go

@@ -101,11 +101,6 @@ var (
 	handshakeResponseBadRequest  = "/nak"                  // general non-acknowledgement response, usually indicates a bad request
 	handshakeResponseError       = "/error"                // general error occurred response, indicates an error occurred on the device plugin end
 
-	/*DeviceFile*/
-	name            = "device.json"    // file which enables passing of device information from device plugin to CNI in the form of device map object.
-	directory       = "/tmp/afxdp_dp/" // host location where deviceFile file is placed.
-	filePermissions = 0600             // permissions for device file.
-
 	/*EthtoolFilters*/
 	ethtoolFilterRegex = `^[a-zA-Z0-9-:.-/\s/g]+$` // regex to validate ethtool filter commands.
 )
@@ -339,12 +334,6 @@ func init() {
 		},
 	}
 
-	DeviceFile = deviceFile{
-		Name:            name,
-		FilePermissions: filePermissions,
-		Directory:       directory,
-	}
-
 	EthtoolFilter = ethtoolFilter{
 		EthtoolFilterRegex: ethtoolFilterRegex,
 	}

examples/network-attachment-definition.yaml

@@ -3,17 +3,21 @@
 apiVersion: "k8s.cni.cncf.io/v1"
 kind: NetworkAttachmentDefinition
 metadata:
-  name: afxdp-network                                        # Name of this network, pods will request this network by name
+  name: afxdp-network                                                                    # Name of this network, pods will request this network by name
   annotations:
-    k8s.v1.cni.cncf.io/resourceName: afxdp/myPool            # Needs to match the device plugin pool name / resource type
+    k8s.v1.cni.cncf.io/resourceName: afxdp/myPool                                        # Needs to match the device plugin pool name / resource type
 spec:
   config: '{
       "cniVersion": "0.3.0",
-      "type": "afxdp",                                       # CNI binary, leave as afxdp
-      "mode": "primary",                                     # CNI mode setting (required)
-      "logFile": "afxdp-cni.log",                            # CNI log file location (optional)
-      "logLevel": "debug",                                   # CNI logging level (optional)
-      "ipam": {                                              # CNI IPAM plugin and associated config (optional)
+      "type": "afxdp",                                                                   # CNI binary, leave as afxdp
+      "mode": "primary",                                                                 # CNI mode setting (required)
+      "logFile": "afxdp-cni.log",                                                        # CNI log file location (optional)
+      "logLevel": "debug",                                                               # CNI logging level (optional)
+      "ethtoolCmds" : ["-X -device- equal 5 start 3",                                    # CNI ethtool filters (optional)
+                       "--config-ntuple -device- flow-type udp4 dst-ip -ip- action"
+                      ],
+
+      "ipam": {                                                                          # CNI IPAM plugin and associated config (optional)
         "type": "host-local",
         "subnet": "192.168.1.0/24",
         "rangeStart": "192.168.1.200",

internal/cni/cni.go

@@ -29,7 +29,6 @@ import (
 	"github.com/intel/afxdp-plugins-for-kubernetes/internal/host"
 	"github.com/intel/afxdp-plugins-for-kubernetes/internal/logformats"
 	"github.com/intel/afxdp-plugins-for-kubernetes/internal/networking"
-	"github.com/intel/afxdp-plugins-for-kubernetes/internal/tools"
 	logging "github.com/sirupsen/logrus"
 	"github.com/vishvananda/netlink"
 	"os"
@@ -45,12 +44,13 @@ NetConfig holds the config passed via stdin
 */
 type NetConfig struct {
 	types.NetConf
-	Device        string `json:"deviceID"`
-	Mode          string `json:"mode"`
-	SkipUnloadBpf bool   `json:"skipUnloadBpf,omitempty"`
-	Queues        string `json:"queues,omitempty"`
-	LogFile       string `json:"logFile,omitempty"`
-	LogLevel      string `json:"logLevel,omitempty"`
+	Device        string   `json:"deviceID"`
+	Mode          string   `json:"mode"`
+	SkipUnloadBpf bool     `json:"skipUnloadBpf,omitempty"`
+	Queues        string   `json:"queues,omitempty"`
+	LogFile       string   `json:"logFile,omitempty"`
+	LogLevel      string   `json:"logLevel,omitempty"`
+	EthtoolCmds   []string `json:"ethtoolCmds,omitempty"`
 }
 
 func init() {
@@ -64,6 +64,7 @@ func (n NetConfig) Validate() error {
 	var (
 		allowedLogLevels               = constants.Logging.Levels
 		allowedModes                   = constants.Plugins.Modes
+		ethtoolRegex                   = constants.EthtoolFilter.EthtoolFilterRegex
 		logLevels        []interface{} = make([]interface{}, len(allowedLogLevels))
 		modes            []interface{} = make([]interface{}, len(allowedModes))
 	)
@@ -93,6 +94,13 @@ func (n NetConfig) Validate() error {
 			&n.Mode,
 			validation.In(modes...).Error("validate(): must be "+fmt.Sprintf("%v", modes)),
 		),
+		validation.Field(
+			&n.EthtoolCmds,
+			validation.Each(
+				validation.Required.When(len(n.EthtoolCmds) > 0).Error("Ethtool field must not be empty"),
+				validation.Match(regexp.MustCompile(ethtoolRegex)).Error("Ethtool commands must be alphanumeric or approved characters"),
+			),
+		),
 	)
 }
 
@@ -142,7 +150,6 @@ CmdAdd is called by kubelet during pod create
 func CmdAdd(args *skel.CmdArgs) error {
 	host := host.NewHandler()
 	var result *current.Result
-	var deviceDetails *networking.Device
 	netHandler := networking.NewHandler()
 
 	cfg, err := loadConf(args.StdinData)
@@ -195,45 +202,28 @@ func CmdAdd(args *skel.CmdArgs) error {
 	}
 
 	if cfg.Mode == "primary" {
-		deviceFile, err := tools.FilePathExists(constants.DeviceFile.Directory + constants.DeviceFile.Name)
-		if err != nil {
-			logging.Errorf("cmdAdd(): Failed to locate deviceFile: %v", err)
-		}
-
-		if deviceFile {
-			deviceDetails, err = netHandler.GetDeviceFromFile(cfg.Device, constants.DeviceFile.Directory+constants.DeviceFile.Name)
-			if err != nil {
-				logging.Errorf("cmdAdd():- Failed to extract device map values: %v", err)
-				return err
-			}
-
+		if cfg.EthtoolCmds != nil {
 			ethInstalled, version, err := host.HasEthtool()
 			if err != nil {
 				logging.Warningf("cmdAdd(): failed to discover ethtool on host: %v", err)
 			}
-
 			if ethInstalled {
 				logging.Debugf("cmdAdd(): ethtool found on host")
 				logging.Debugf("\t" + version)
-				if deviceDetails != nil {
-					if deviceDetails.GetEthtoolFilters() != nil {
-						logging.Infof("cmdAdd(): applying ethtool filters on device: %s", cfg.Device)
-						ethtoolCommand := deviceDetails.GetEthtoolFilters()
-						iPAddr, err := extractIP(result)
-						if err != nil {
-							logging.Errorf("cmdAdd(): Error extracting IP from result interface %v", err)
-							return err
-						}
-						err = netHandler.SetEthtool(ethtoolCommand, cfg.Device, iPAddr)
-						if err != nil {
-							logging.Errorf("cmdAdd(): unable to executed ethtool filter: %v", err)
-							return err
-						}
-					} else {
-						logging.Debugf("cmdAdd(): ethtool filters have not been specified")
-					}
+				logging.Infof("cmdAdd(): applying ethtool filters on device: %s", cfg.Device)
+				iPAddr, err := extractIP(result)
+				if err != nil {
+					logging.Errorf("cmdAdd(): Error extracting IP from result interface %v", err)
+					return err
+				}
+				err = netHandler.SetEthtool(cfg.EthtoolCmds, cfg.Device, iPAddr)
+				if err != nil {
+					logging.Errorf("cmdAdd(): unable to executed ethtool filter: %v", err)
+					return err
 				}
 			}
+		} else {
+			logging.Debugf("cmdAdd(): ethtool filters have not been specified")
 		}
 	}
 
@@ -354,19 +344,20 @@ func CmdDel(args *skel.CmdArgs) error {
 			return err
 		}
 	}
-
 	if cfg.Mode == "primary" {
-		logging.Debugf("cmdDel: checking host for Ethtool")
-		ethInstalled, _, err := host.HasEthtool()
-		if err != nil {
-			logging.Errorf("cmdDel(): error checking if Ethtool is present on host: %v", err)
-			return err
-		}
-		if ethInstalled {
-			logging.Infof("cmdDel(): Removing ethtool filters on device: %s", cfg.Device)
-			err := netHandler.DeleteEthtool(cfg.Device)
+		if cfg.EthtoolCmds != nil {
+			logging.Debugf("cmdDel: checking host for Ethtool")
+			ethInstalled, _, err := host.HasEthtool()
 			if err != nil {
-				logging.Warningf("cmdDel(): failed to remove ethtool filter: %v", err)
+				logging.Errorf("cmdDel(): error checking if Ethtool is present on host: %v", err)
+				return err
+			}
+			if ethInstalled {
+				logging.Infof("cmdDel(): Removing ethtool filters on device: %s", cfg.Device)
+				err := netHandler.DeleteEthtool(cfg.Device)
+				if err != nil {
+					logging.Warningf("cmdDel(): failed to remove ethtool filter: %v", err)
+				}
 			}
 		}
 	}

internal/deviceplugin/config.go

@@ -55,13 +55,12 @@ type PoolConfig struct {
 	Name                    string                        // the name of the pool, used for logging and advertising resource to K8s. Pods will request this resource
 	Mode                    string                        // the mode that this pool operates in
 	Devices                 map[string]*networking.Device // a map of devices that the pool will manage
-	Filters                 []string                      // the ethtool/rss filters we apply to devices from this pool
 	UdsServerDisable        bool                          // a boolean to say if pods in this pool require BPF loading the UDS server
 	UdsTimeout              int                           // timeout value in seconds for the UDS sockets, user provided or defaults to value from constants package
 	UdsFuzz                 bool                          // a boolean to turn on fuzz testing within the UDS server, has no use outside of development and testing
 	RequiresUnprivilegedBpf bool                          // a boolean to say if this pool requires unprivileged BPF
 	UID                     int                           // the id of the pod user, we give this user ACL access to the UDS socket
-	EthtoolCmds             []string                      // list of ethtool filters to apply to the netdev
+
 }
 
 /*
@@ -252,7 +251,6 @@ func GetPoolConfigs(configFile string, net networking.Handler, host host.Handler
 				UdsFuzz:                 pool.UdsFuzz,
 				RequiresUnprivilegedBpf: pool.RequiresUnprivilegedBpf,
 				UID:                     pool.UID,
-				EthtoolCmds:             pool.EthtoolCmds,
 			})
 		}
 

internal/deviceplugin/configFile.go

@@ -55,8 +55,6 @@ const (
 	poolUdsTimeoutError   = "UDS socket timeout must be -1, 0, or between 30 and 300 seconds"
 	poolModeRequiredError = "Plugin must have a mode"
 	poolModeMustBeError   = "Plugin mode must be one of "
-	poolEthtoolNotEmpty   = "Ethtool commands cannot be empty"
-	poolEthtoolCharacters = "Ethtool commands must be alphanumeric or contain only approved charaters"
 
 	// logging errors
 	filenameValidError = "must be a valid .log or .txt filename"
@@ -94,7 +92,6 @@ type configFile_Pool struct {
 	UdsFuzz                 bool                 `json:"UdsFuzz"`
 	RequiresUnprivilegedBpf bool                 `json:"RequiresUnprivilegedBpf"`
 	UID                     int                  `json:"uid"`
-	EthtoolCmds             []string             `json:"ethtoolCmds"`
 }
 
 type configFile struct {
@@ -229,13 +226,6 @@ func (c configFile_Pool) Validate() error {
 			validation.When(!(c.UID == 0), validation.Max(constants.UID.Maximum)),
 			validation.When(!(c.UID == 0), validation.Min(constants.UID.Minimum)),
 		),
-		validation.Field(
-			&c.EthtoolCmds,
-			validation.Each(
-				validation.Required.When(len(c.EthtoolCmds) > 0).Error(poolEthtoolNotEmpty),
-				validation.Match(regexp.MustCompile(constants.EthtoolFilter.EthtoolFilterRegex)).Error(poolEthtoolCharacters),
-			),
-		),
 	)
 }