Untie the HCS NetworkType from the network name

[TBBle]

Currently, the network.NetworkInfo.Type value passed through to HCS (which must be one of network.NetworkType, i.e. hcn.NetworkType values) is simply the network name. This is pretty surprising, and also breaks the CNI spec if you want multiple networks of the same type, as each plugin or plugin-list must have a host-unique name.

It also means none of the example configs work, as they will all complain with something like the below:

hcnCreateNetwork failed in Win32: Invalid JSON document string. (0x803b001b) {"Success":false,"Error":"Invalid JSON document string. {{Type,UnknownEnumValue}}","ErrorCode":2151350299}

This appears to be deliberate, but it's not very user-friendly.

The obvious approach seems to me to have each of the executables specify their own NetworkType when they call into core.Core, since the network type used appears to be the only intended distinction between the executables. I started trying to plumb this approach through, but got lost in the abstraction layers, trying to work out how to carry that value without needing to put a hard-coded [string]NetworkType map somewhere in the system, or pull a bunch of stuff into the core package.

Another option would be to expose the network type as an additional parameter, and just produce one executable that covers all the use-cases, which is really what we have now anyway, but be intentional about it.

Another option is to have the executables named to match the NetworkType, and then we can just use config.Type instead of config.Name, and there's little-to-no surprise, as long as no one renames their own executable. Again, this is really just one executable compiled three times, so not really a great approach, I feel.

So I'm interested in guidance and suggestions. This has come up while working on porting BuildKit to Windows, as the only network layer it supports natively is CNI, so I needed minimally-functional CNI with NAT, ala the nat CNI plugin I found here. I used the lattermost option as my workaround, but without renaming binaries it only works for nat, not sdnoverlay or sdnbridge, as those are not NetworkType values.

[nagiesek]

I'm working on other projects but I'll try and slip this in this week.

[daschott]

@Keith-Mange FYI

[aznashwan]

Another option is to have the executables named to match the NetworkType, and then we can just use config.Type instead of config.Name, and there's little-to-no surprise, as long as no one renames their own executable. Again, this is really just one executable compiled three times, so not really a great approach, I feel.

FWIW this exact approach was marged in #94 as seen here.

Scratch that, that was an unrelated issue which doesn't fix the actual underlying problem, and the failure to map the binary names to the HCN types still occurs.

Will propose some sort of solution (or at least a proper workaround) soon.

[TBBle]

I wrote this part before the above comment was modified. Looks like it was updated with the same conclusion.

Interesting. Poking around, it doesn't seem that the issue was resolved that the Makefile produces binaries named sdnbridge and sdnoverlay, but those are not documented NetworkMode values. The examples seem to mostly expect binaries named l2bridge and l2tunnel (which aren't output here), but the flannel and dual-stack examples still reference the sdn* names, which I expect will still fail with the error in my original example.

Unless I'm overlooking some point where the sdn* names are translated into the HCN expected enum values... I note that the in-tree test code is not using the executable names, but passing the expected NetworkType values directly into util.MakeTestStruct, so the executable-name part isn't being covered by the test suite.

That said, since we've already gone down this path, perhaps simply changing the Makefile (and whatever else is lying around like the examples) to output executables to match the NetworkType enum is the shortest-path workaround.

Thinking further about it, and also in relation to this comment:

// getOrCreateNetwork
// TODO: Require network to be created beforehand and make it an error of the network is not found.
// Once that is done, remove this function.

perhaps the long-term solution is to complete this TODO, and then unifiy the plugins into a single type wincni. AFAICT from GitHub-browsing the source, if the network already exists, the type from the CNI config file is not actually used, we trust the type of the existing HCN network.

The only place I can see accessing the CNI config's Type is GetHostComputeNetworkConfig, itself only called during CreateNetwork.

I'm not sure about the history of this comment or the decision to require pre-creating networks (That seems like CNI's role to my limited understanding) so this might not be a viable path. I also admit I don't know off-hand how I would pre-create the NAT network, for example.

---

Which suggests a different interesting issue: If you pre-create an Overlay network, you can trivially use that network's name in your CNI config, and a network type of nat (or NAT, or nAt, but that's a different issue), and it'll process it as an Overlay network based on the details retrieved from HCN. I suspect this is how this issue has slipped under the radar, if the general users of this repo are pre-creating their networks, then there's no failure with calling the sdnoverlay executable by CNI Config Type sdnoverlay but referencing an Overlay (or L2Bridge, or NAT) network.