Fixup as per comments

Signed-off-by: Benjamin Leggett <[email protected]>

Author: bleggett

Diff for: SPEC.md

@@ -111,7 +111,8 @@ A network configuration consists of a JSON object with the following keys:
 - `cniVersions` (string list): List of all CNI versions which this configuration supports. See [version selection](#version-selection) below.
 - `name` (string): Network name. This should be unique across all network configurations on a host (or other administrative domain).  Must start with an alphanumeric character, optionally followed by any combination of one or more alphanumeric characters, underscore, dot (.) or hyphen (-). Must not contain characters disallowed in file paths. A path segment (such as a filesystem directory) with the same name as the network name, containing one or more plugin configuration JSON objects for that network, should exist at the same level as the network configuration object itself.
 - `disableCheck` (boolean): Either `true` or `false`.  If `disableCheck` is `true`, runtimes must not call `CHECK` for this network configuration list.  This allows an administrator to prevent `CHECK`ing where a combination of plugins is known to return spurious errors.
-<!-- - `plugins` (list): A list of CNI plugins and their configuration, which is a list of plugin configuration objects. -->
+- `loadPluginsFromFolder` (boolean): Either `true` or `false`. If `true`, indicates [plugin configuration objects](#plugin-configuration-objects) should be loaded from a sibling folder with the same name as the network `name` field. These sibling folders should exist at the same path as the network configuration itself. Any valid plugin configuration objects within the sibling folder will be appended to the final list of plugins for that network. If `plugins` is not present in the network configuration, `loadPluginsFromFolder` must be present, and set to true.
+- `plugins` (list): A list of inlined [plugin configuration objects](#plugin-configuration-objects). If this key is populated with inlined plugin objects, and `loadPluginsFromFolder` is true, the final set of plugins for a network must consist of all the plugin objects in this list and the all the plugins loaded from the sibling folder with the same name as the network.
 
 #### Plugin configuration objects:
 All plugin configuration objects present in a directory with the same name as a valid network configuration must be parsed, and each plugin with a parsable configuration object must be invoked.
@@ -147,13 +148,14 @@ Plugins that consume any of these configuration keys should respect their intend
 Plugins may define additional fields that they accept and may generate an error if called with unknown fields. Runtimes must preserve unknown fields in plugin configuration objects when transforming for execution.
 
 #### Example configuration
-
+Network configuration with no inlined plugin confs, and two loaded plugin confs:
 `/etc/cni/net.d/10-dbnet.conf`:
 ```jsonc
 {
   "cniVersion": "1.1.0",
   "cniVersions": ["0.3.1", "0.4.0", "1.0.0", "1.1.0"],
   "name": "dbnet",
+  "loadPluginsFromFolder": true,
 }
 ```
 
@@ -177,7 +179,7 @@ Plugins may define additional fields that they accept and may generate an error
   "dns": {
     "nameservers": [ "10.1.0.1" ]
   }
-},
+}
 ```
 
 `/etc/cni/net.d/dbnet/10-tuning.conf`:
@@ -190,16 +192,83 @@ Plugins may define additional fields that they accept and may generate an error
   "sysctl": {
     "net.core.somaxconn": "500"
   }
-},
+}
 ```
 
-`/etc/cni/net.d/dbnet/15-portmap.conf`:
+Network configuration with one inlined plugin conf, and one loaded plugin conf:
+`/etc/cni/net.d/10-dbnet.conf`:
 ```jsonc
 {
-    "type": "portmap",
-    "capabilities": {"portMappings": true}
+  "cniVersion": "1.1.0",
+  "cniVersions": ["0.3.1", "0.4.0", "1.0.0", "1.1.0"],
+  "name": "dbnet",
+  "loadPluginsFromFolder": true,
+  plugins: [
+    {
+      "type": "bridge",
+      // plugin specific parameters
+      "bridge": "cni0",
+      "keyA": ["some more", "plugin specific", "configuration"],
+
+      "ipam": {
+        "type": "host-local",
+        // ipam specific
+        "subnet": "10.1.0.0/16",
+        "gateway": "10.1.0.1",
+        "routes": [
+            {"dst": "0.0.0.0/0"}
+        ]
+      },
+      "dns": {
+        "nameservers": [ "10.1.0.1" ]
+      }
+    }
+  ]
+}
+```
+
+`/etc/cni/net.d/dbnet/10-tuning.conf`:
+```jsonc
+{
+  "type": "tuning",
+  "capabilities": {
+    "mac": true
+  },
+  "sysctl": {
+    "net.core.somaxconn": "500"
+  }
 }
+```
 
+Network configuration with one inlined plugin conf, and no loaded plugin conf:
+`/etc/cni/net.d/10-dbnet.conf`:
+```jsonc
+{
+  "cniVersion": "1.1.0",
+  "cniVersions": ["0.3.1", "0.4.0", "1.0.0", "1.1.0"],
+  "name": "dbnet",
+  "plugins": [
+    {
+      "type": "bridge",
+      // plugin specific parameters
+      "bridge": "cni0",
+      "keyA": ["some more", "plugin specific", "configuration"],
+
+      "ipam": {
+        "type": "host-local",
+        // ipam specific
+        "subnet": "10.1.0.0/16",
+        "gateway": "10.1.0.1",
+        "routes": [
+            {"dst": "0.0.0.0/0"}
+        ]
+      },
+      "dns": {
+        "nameservers": [ "10.1.0.1" ]
+      }
+    }
+  ]
+}
 ```
 
 ### Version considerations

Diff for: cnitool/cnitool.go

@@ -66,7 +66,7 @@ func main() {
 	if netdir == "" {
 		netdir = DefaultNetDir
 	}
-	netconf, err := libcni.LoadConfList(netdir, os.Args[2])
+	netconf, err := libcni.LoadNetworkConf(netdir, os.Args[2])
 	if err != nil {
 		exit(err)
 	}

Diff for: libcni/api.go

@@ -76,11 +76,12 @@ type PluginConfig struct {
 }
 
 type NetworkConfigList struct {
-	Name         string
-	CNIVersion   string
-	DisableCheck bool
-	Plugins      []*PluginConfig
-	Bytes        []byte
+	Name                  string
+	CNIVersion            string
+	DisableCheck          bool
+	LoadPluginsFromFolder bool
+	Plugins               []*PluginConfig
+	Bytes                 []byte
 }
 
 type NetworkAttachment struct {

Diff for: libcni/conf.go

@@ -67,17 +67,20 @@ func NetworkPluginConfFromBytes(pluginConfBytes []byte) (*PluginConfig, error) {
 // Given a path to a directory containing a network configuration, and the name of a network,
 // loads all plugin definitions found at path `networkConfPath/networkName/*.conf`
 func NetworkPluginConfsFromFiles(networkConfPath, networkName string) ([]*PluginConfig, error) {
-	pluginConfFiles, err := ConfFiles(filepath.Join(networkConfPath, networkName), []string{".conf"})
+	var pConfs []*PluginConfig
+
+	pluginConfPath := filepath.Join(networkConfPath, networkName)
+
+	pluginConfFiles, err := ConfFiles(pluginConfPath, []string{".conf"})
 	switch {
 	case err != nil:
-		return nil, fmt.Errorf("failed to read plugin config file: %w", err)
+		return nil, fmt.Errorf("failed to read plugin config files in %s: %w", pluginConfPath, err)
 	case len(pluginConfFiles) == 0:
 		// Having 0 plugins for a given network is not necessarily a problem,
-		// but return as error for caller to decide.
-		return nil, fmt.Errorf("no plugin config found in %s: %w", networkConfPath, err)
+		// but return as error for caller to decide, since they tried to load
+		return nil, fmt.Errorf("no plugin config found in %s", pluginConfPath)
 	}
 
-	var pConfs []*PluginConfig
 	for _, pluginConfFile := range pluginConfFiles {
 		pluginConfBytes, err := os.ReadFile(pluginConfFile)
 		if err != nil {
@@ -176,15 +179,52 @@ func NetworkConfFromBytes(confBytes []byte) (*NetworkConfigList, error) {
 		}
 	}
 
+	loadPluginsFromFolder := false
+	if rawLoadCheck, ok := rawList["loadPluginsFromFolder"]; ok {
+		loadPluginsFromFolder, ok = rawLoadCheck.(bool)
+		if !ok {
+			return nil, fmt.Errorf("error parsing configuration list: invalid loadPluginsFromFolder type %T", rawLoadCheck)
+		}
+	}
+
 	list := &NetworkConfigList{
-		Name:         name,
-		DisableCheck: disableCheck,
-		CNIVersion:   cniVersion,
-		Bytes:        confBytes,
+		Name:                  name,
+		DisableCheck:          disableCheck,
+		LoadPluginsFromFolder: loadPluginsFromFolder,
+		CNIVersion:            cniVersion,
+		Bytes:                 confBytes,
+	}
+
+	var plugins []interface{}
+	plug, ok := rawList["plugins"]
+	// We can have a `plugins` list key in the main conf,
+	// We can also have `loadPluginsFromFolder == true`
+	// They can both be present in the same config.
+	// But if one of them is NOT present/false, the other *must* be there.
+	if !ok && !loadPluginsFromFolder {
+		return nil, fmt.Errorf("error parsing configuration list: `loadPluginsFromFolder` not true, and no 'plugins' key")
+	} else if !ok && loadPluginsFromFolder {
+		return list, nil
 	}
 
-	if _, ok := rawList["plugins"]; ok {
-		return nil, fmt.Errorf("error parsing configuration list: no 'plugins' key allowed, cniVersion %s must load plugins from subdirectories", version.Current())
+	plugins, ok = plug.([]interface{})
+	if !ok {
+		return nil, fmt.Errorf("error parsing configuration list: invalid 'plugins' type %T", plug)
+	}
+	if len(plugins) == 0 {
+		return nil, fmt.Errorf("error parsing configuration list: no plugins in list")
+	}
+
+	for i, conf := range plugins {
+		newBytes, err := json.Marshal(conf)
+		if err != nil {
+			return nil, fmt.Errorf("failed to marshal plugin config %d: %w", i, err)
+		}
+		netConf, err := ConfFromBytes(newBytes)
+		if err != nil {
+			return nil, fmt.Errorf("failed to parse plugin config %d: %w", i, err)
+		}
+		list.Plugins = append(list.Plugins, netConf)
 	}
 	return list, nil
 }
@@ -195,31 +235,27 @@ func NetworkConfFromFile(filename string) (*NetworkConfigList, error) {
 		return nil, fmt.Errorf("error reading %s: %w", filename, err)
 	}
 
-	// TODO
 	conf, err := NetworkConfFromBytes(bytes)
 	if err != nil {
 		return nil, err
 	}
 
-	plugins, err := NetworkPluginConfsFromFiles(filepath.Dir(filename), conf.Name)
-	if err != nil {
-		return nil, err
+	if conf.LoadPluginsFromFolder {
+		plugins, err := NetworkPluginConfsFromFiles(filepath.Dir(filename), conf.Name)
+		if err != nil {
+			return nil, err
+		}
+		conf.Plugins = append(conf.Plugins, plugins...)
 	}
 
-	conf.Plugins = plugins
-
 	return conf, nil
 }
 
 // Deprecated: This file format is no longer supported, use NetworkConfXXX and NetworkPluginXXX functions
-func ConfFromBytes(bytes []byte) (*PluginConfig, error) {
+func ConfFromBytes(bytes []byte) (*NetworkConfig, error) {
 	return NetworkPluginConfFromBytes(bytes)
 }
 
-// TODO Are we ok, at this point, cutting a new major version of libCNI and dropping the non-list CNI config?
-//
-// The non-list format was dropped in 1.0 and so I feel we should.
-//
 // Deprecated: This file format is no longer supported, use NetworkConfXXX and NetworkPluginXXX functions
 func ConfFromFile(filename string) (*NetworkConfig, error) {
 	bytes, err := os.ReadFile(filename)
@@ -292,14 +328,23 @@ func LoadConf(dir, name string) (*NetworkConfig, error) {
 
 // Deprecated: Use NetworkConfXXX and NetworkPluginXXX functions
 func LoadConfList(dir, name string) (*NetworkConfigList, error) {
+	return LoadNetworkConf(dir, name)
+}
+
+// LoadNetworkConf looks at all the network configs in a given dir,
+// loads and parses them all, and returns the first one with an extension of `.conf`
+// that matches the provided network name predicate.
+func LoadNetworkConf(dir, name string) (*NetworkConfigList, error) {
+	// TODO this .conflist/.conf extension thing is confusing and inexact
+	// for implementors. We should pick one extension for everything and stick with it.
 	files, err := ConfFiles(dir, []string{".conflist"})
 	if err != nil {
 		return nil, err
 	}
 	sort.Strings(files)
 
 	for _, confFile := range files {
-		conf, err := ConfListFromFile(confFile)
+		conf, err := NetworkConfFromFile(confFile)
 		if err != nil {
 			return nil, err
 		}
@@ -308,7 +353,7 @@ func LoadConfList(dir, name string) (*NetworkConfigList, error) {
 		}
 	}
 
-	// Try and load a network configuration file (instead of list)
+	// Deprecated: Try and load a network configuration file (instead of list)
 	// from the same name, then upconvert.
 	singleConf, err := LoadConf(dir, name)
 	if err != nil {
@@ -324,28 +369,6 @@ func LoadConfList(dir, name string) (*NetworkConfigList, error) {
 	return ConfListFromConf(singleConf)
 }
 
-// LoadNetworkConf looks at all the network configs in a given dir,
-// loads and parses them all, and returns the first one that matches the provided
-// network name predicate.
-func LoadNetworkConf(dir, name string) (*NetworkConfigList, error) {
-	files, err := ConfFiles(dir, []string{".conf"})
-	if err != nil {
-		return nil, err
-	}
-	sort.Strings(files)
-
-	for _, confFile := range files {
-		conf, err := NetworkConfFromFile(confFile)
-		if err != nil {
-			return nil, err
-		}
-		if conf.Name == name {
-			return conf, nil
-		}
-	}
-	return nil, fmt.Errorf("no netconfig found in %q with name %s", dir, name)
-}
-
 // InjectConf takes a PluginConfig and inserts additional values into it, ensuring the result is serializable.
 func InjectConf(original *PluginConfig, newValues map[string]interface{}) (*PluginConfig, error) {
 	config := make(map[string]interface{})