Windows CNI for overlay (vxlan) and host-gw (l2bridge) modes

[rakelkar]

This is an early PR to get feedback on a resolution to #80. Proposes changes to add CNI plugins for windows into the CNI repository. The plugins use the Windows Kernel Host Networking Service (HNS) APIs via the HCSSHIM package to configure an IP address on the windows network stack. Depends on #77.

TODO:

• Add Unit Tests
• Update Deps
• Merge with dup logic in Support delegating to Windows CNI from Flannel CNI #76
• Add Integration Tests

[squeed]

This seems pretty reasonable. One important thing - modules in pkg should have docblocks on all functions.

[gkudra-msft]

I like this. A couple of points:

• To be clear, plugin-specific parameters are part of the top-level configuration (like ipMasq and endpointMacPrefix)?

• The existing (private) wincni.exe binary would no longer exist. Instead, the unified code would be a part of this pkg/hns/* package. This is where code interpreting additionalArgs into HNS policy would go. Is the structure of the additionalArgs different than raw HNS policy, and if so, should there be docs for that?

• A feature (or "capability") doesn't seem like it belongs in either of these. The Linux model for capabilities such as port mapping seems to have separate plugins. For Windows, it makes sense (to me) to put them into the hns package. Will they be implicitly applied when the plugin passes them along? My gut (and analysis of the code) says no; how, then, do we do that cleanly?

[rakelkar]

@gkudra-msft

1. endpointMacPrefix is not part of top-level configuration since l2bridge mode doesn't need it.
2. The current code already supports merging additionalArgs - the structure is identical.
3. I think portmapping is a chained plugin to enable all Linux plugins to use it. For windows it depends on whether policies can be set after the endpoint is created. If yes, makes sense to have a windows verson of portmap if no, then the right thing to do it to put helpers into pkg/hns and use while creating endpoints

[madhanrm]

will update the PR with review comments

[madhanrm]

Sample build output:
madhanm@NetAppLinux:~/repo/gopath/src/github.com/containernetworking/plugins$ bash build.sh
Building plugins
flannel
portmap
tuning
bridge
host-device
ipvlan
loopback
macvlan
ptp
vlan
l2bridge.exe
overlay.exe
dhcp
host-local
sample

[madhanrm]

Fixed the Windows CI test.

[madhanrm]

@squeed Can you look into this and get this merged? Is there any open items here?

[rakelkar]

@madhanrm see TODO in PR description. CNI maintainers asked for integration tests like other plugins.

[madhanrm]

@rakelkar Yes, that has to be done in a separate PR. This PR is already huge.

[rakelkar]

Maybe you want to do the vendor update in a separate PR... I am guessing that contributes most of the hugeness?

…

On Dec 21, 2017 1:26 PM, "Madhan Raj Mookkandy" ***@***.***> wrote: @rakelkar <https://github.com/rakelkar> Yes, that has to be done in a separate PR. This PR is already huge. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#85 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADQphYFCtFfIBXGx1TxVTMuWkfTIwBCPks5tCs0ggaJpZM4QOUvB> .

[madhanrm]

Vendor changes are required to get the current windows Appveyor test running and passing. We should be adding the integration test separately, once we have the dependencies merged.

[madhanrm]

ping @rosenhouse , @squeed

[rosenhouse]

Sorry for the delay, most of us were/still-are on holiday.

Please squash and follow guidelines for commit message.

[PatrickLang]

@madhanrm - can you fix the travis break?

Is this just waiting on review or are there other issues blocking this from moving forward?

[benmoss]

@thxCode opened a PR with fixes here: #193

[daschott]

@rosenhouse @thxCode thank you for the insight and help for this PR. Are there any outstanding issues or actions needed to merge this initial PR in?

[dcbw]

Merged #193 as a continuation of this one.