Windows CNI support #193

thxCode · 2018-08-23T06:37:36Z

A patch PR for #85

thxCode · 2018-08-23T06:38:25Z

@rosenhouse

rebase to master
update deps

build script: GOARCH=amd64 ./build.sh

benmoss · 2018-08-28T13:59:25Z

LGTM 👍

rosenhouse · 2018-08-29T05:52:08Z

ok, so this isn't so much a patch on top of #85, just a replacement for it. we'd just merge this one and close that one.

When we merge, we can fix-up the middle commit to include the original authors (@rakelkar and @madhanrm) as Co-authored-by: lines. Sound good to everyone?

cc @dineshgovindasamy

Edit: Here's a branch with the same contents as this PR, but with commit messages slightly fixed up, giving credit to original authors: master...rosenhouse:gabe-windows-cleanup

feiskyer · 2018-08-29T06:15:59Z

LGTM

rosenhouse · 2018-08-29T15:10:20Z

Good suggestion by @squeed: we could add an authors or maintainers file here listing the Microsoft folks.

rakelkar · 2018-08-29T15:23:13Z

LGTM So exciting to see this getting merged finally!

…

-r

On Wed, Aug 29, 2018, 8:12 AM Gabe Rosenhouse ***@***.***> wrote: Good suggestion by @squeed <https://github.com/squeed>: we could add an authors or maintainers file here listing the Microsoft folks. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#193 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADQpheSZPow9Yy9OSgHJJbp8Bsj6dAHeks5uVq90gaJpZM4WI7iv> .

thxCode · 2018-08-30T01:23:51Z

@rosenhouse, fix a mistake from #85 (comment). FYI, @benmoss .

dineshgovindasamy · 2018-08-30T22:21:00Z

LGTM

dineshgovindasamy · 2018-09-05T17:49:45Z

@rosenhouse @benmoss

We have subsequent PR's waiting on this merge. Can we please get this in as all our remaining work is gates on this PR?

rosenhouse · 2018-09-05T17:58:27Z

Will need a second approval from one of the @containernetworking/cni-maintainers before we can merge.

benmoss · 2018-09-05T21:16:40Z

@squeed any chance you can take a look?

JMesser81 · 2018-09-08T23:08:57Z

@dcbw @squeed @bboreham

Can you please take a look and LGTM so we can get this merged? It's been about a year since we first started this effort and many partner teams are taking a dependency here. We have unit tests added now as well. Thanks!

daschott · 2018-09-10T17:37:48Z

Thank you @rosenhouse for the support and assistance in this PR and your review 🥇
Also @matthewdupre @squaremo @steveej @tomdee can one of you kindly take a look and LGTM or comment? We'd love to contribute more but our subsequent work depends on this PR being merged first :)

plugins/main/windows/l2bridge/l2bridge_windows.go

plugins/main/windows/overlay/sample.conf

plugins/main/windows/l2bridge/sample.conf

plugins/main/windows/overlay/overlay_windows.go

dcbw

There are a number of review comments that we'd like addressed, but looks like this is on the right track. Thanks!

dcbw · 2018-09-05T15:25:43Z

pkg/hns/netconf.go

+// NetConf is the CNI spec
+type NetConf struct {
+	types.NetConf
+	AdditionalArgs []policyArgument `json:"AdditionalArgs,omitempty"`


Need to fix the JSON capitalization:

json:"additionalArgs,omitempty"

dcbw · 2018-09-12T15:07:32Z

plugins/main/windows/l2bridge/l2bridge_windows.go

+	types.CommonArgs
+	K8S_POD_NAMESPACE          types.UnmarshallableString `json:"K8S_POD_NAMESPACE,omitempty"`
+	K8S_POD_NAME               types.UnmarshallableString `json:"K8S_POD_NAME,omitempty"`
+	K8S_POD_INFRA_CONTAINER_ID types.UnmarshallableString `json:"K8S_POD_INFRA_CONTAINER_ID,omitempty"`


INFRA_CONTAINER_ID isn't used anywhere, so it should be removed.

Same for K8S_POD_NAME

dcbw · 2018-09-12T15:07:59Z

plugins/main/windows/overlay/overlay_windows.go

+	types.CommonArgs
+	K8S_POD_NAMESPACE          types.UnmarshallableString `json:"K8S_POD_NAMESPACE,omitempty"`
+	K8S_POD_NAME               types.UnmarshallableString `json:"K8S_POD_NAME,omitempty"`
+	K8S_POD_INFRA_CONTAINER_ID types.UnmarshallableString `json:"K8S_POD_INFRA_CONTAINER_ID,omitempty"`


INFRA_POD_CONTAINER_ID isn't used anywhere so it should be removed.

Same for K8S_POD_NAME

Actually per below, let's remove all the CNI Args stuff for now.

dcbw · 2018-09-12T15:29:29Z

plugins/main/windows/overlay/overlay_windows.go

+
+		ipAddr := result.IPs[0].Address.IP.To4()
+		// conjure a MAC based on the IP for Overlay
+		macAddr := fmt.Sprintf("%v-%02x-%02x-%02x-%02x", n.endpointMacPrefix, ipAddr[0], ipAddr[1], ipAddr[2], ipAddr[3])


This plugin isn't IPv6-ready, is it? That could be a blocker, since all the rest of the CNI Plugins work with IPv6.

dcbw · 2018-09-12T15:30:42Z

plugins/main/windows/overlay/overlay_windows.go

+	cniargs, err := parseCniArgs(args.Args)
+	k8sNamespace := "default"
+	if err == nil {
+		k8sNamespace = string(cniargs.K8S_POD_NAMESPACE)


This is Kubernetes specific, which isn't something we want in CNI since CNI is not Kubernetes-specific and works with other runtimes too. For Kube I believe there are some other solutions that we can explore that don't hardcode Kube-ness into the CNI plugin.

For now, let's remove all the parseCniArgs() and K8S_POD_NAMESPACE stuff and fix that in a follow-up patch.

dcbw · 2018-09-12T15:40:06Z

plugins/main/windows/l2bridge/README.md

+```
+{
+	"name": "mynet",
+	"type": "win-l2bridge",


Need to make the type consistent with the actual plugin name, eg when renamed to "win-bridge" need to make this "type": "win-bridge",

dcbw · 2018-09-12T15:40:25Z

plugins/main/windows/l2bridge/README.md

+## Network configuration reference
+
+* `name` (string, required): the name of the network.
+* `type` (string, required): "win-l2bridge".


Need to keep the plugin name consistent with the binary name.

dcbw · 2018-09-12T15:40:39Z

plugins/meta/flannel/README.md

+
+The following fields must be set in the delegated plugin configuration:
+* `name` (string, required): the name of the network (must match the name in Flannel config / name of the HNS network)
+* `type` (string, optional): set to win-l2bridge by default. Can be set to win-overlay or other custom windows CNI


Need to keep the plugin name consistent with the binary name.

dcbw · 2018-09-12T15:41:07Z

plugins/main/windows/overlay/README.md

@@ -0,0 +1,27 @@
+# L2Bridge  plugin (Windows)


This is for the Overlay plugin, right? Not L2Bridge.

dcbw · 2018-09-12T15:43:02Z

plugins/main/windows/overlay/README.md

+```
+{
+	"name": "mynet",
+	"type": "win-overlay",


Need to keep the type consistent with the binary name, and to match win-bridge, let's rename the binary to "win-overlay".

thxCode · 2018-09-13T09:08:17Z

@dcbw, @squeed , please review again.

bboreham

I think this is ok to go in; the main eyebrow-raising points have been addressed.

plugins/main/windows/win-bridge/win-bridge_windows.go

plugins/main/windows/win-overlay/win-overlay_windows.go

plugins/meta/flannel/flannel_windows.go

thxCode · 2018-09-19T02:10:08Z

@rosenhouse , @dcbw , @squeed , @bboreham , is there any new questions on this PR?

squeed · 2018-09-19T15:38:42Z

Changes look good to me, thanks!

One last request - could you squash your commits in to one for /vendor and one for everything else?

plugins/main/windows/win-bridge/README.md

(*) github.com/Microsoft/hcsshim (*) golang.org/x/sys (*) github.com/x/cyrpto (*) github.com/sirupsen/logrus (*) github.com/Microsoft/go-winio (*) github.com/juju/errors (*) github.com/buger/jsonparser

Patch for containernetworking#85 + Windows cni plugins are added (*) win-bridge (hostgw) (*) win-overlay (vxlan) + Windows netconf unit test + Fix appveyor config to run the test + Build release support for windows plugins Address comments From: - containernetworking#85 - rakelkar@0049c64

dcbw · 2018-09-20T20:15:33Z

LGTM, I think the remaining issues can be addressed in follow-up PRs

thxCode force-pushed the windows_cni branch 7 times, most recently from 7dc47ba to 578b32b Compare August 23, 2018 08:58

benmoss mentioned this pull request Aug 28, 2018

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

Closed

4 tasks

pablodav mentioned this pull request Aug 28, 2018

what's the status for flannel on windows? pablodav/kubernetes-for-windows#10

Closed

thxCode force-pushed the windows_cni branch 3 times, most recently from d82e0a3 to ea2c3b6 Compare August 30, 2018 01:42

rosenhouse approved these changes Sep 5, 2018

View reviewed changes

squeed reviewed Sep 12, 2018

View reviewed changes

plugins/main/windows/l2bridge/l2bridge_windows.go Outdated Show resolved Hide resolved

squeed reviewed Sep 12, 2018

View reviewed changes

plugins/main/windows/overlay/sample.conf Outdated Show resolved Hide resolved

squeed requested changes Sep 12, 2018

View reviewed changes

plugins/main/windows/l2bridge/sample.conf Outdated Show resolved Hide resolved

plugins/main/windows/overlay/overlay_windows.go Outdated Show resolved Hide resolved

dcbw requested changes Sep 12, 2018

View reviewed changes

bboreham approved these changes Sep 13, 2018

View reviewed changes

fasaxc reviewed Sep 13, 2018

View reviewed changes

plugins/main/windows/win-bridge/win-bridge_windows.go Outdated Show resolved Hide resolved

fasaxc reviewed Sep 13, 2018

View reviewed changes

plugins/main/windows/win-overlay/win-overlay_windows.go Outdated Show resolved Hide resolved

thxCode force-pushed the windows_cni branch from 098544b to dac2261 Compare September 13, 2018 15:00

benmoss reviewed Sep 13, 2018

View reviewed changes

plugins/meta/flannel/flannel_windows.go Show resolved Hide resolved

thxCode force-pushed the windows_cni branch from 3b64a83 to cfc87a0 Compare September 14, 2018 05:42

squeed approved these changes Sep 19, 2018

View reviewed changes

dcbw reviewed Sep 19, 2018

View reviewed changes

plugins/main/windows/win-bridge/README.md Outdated Show resolved Hide resolved

thxCode force-pushed the windows_cni branch 8 times, most recently from 1125448 to 915374c Compare September 20, 2018 16:33

thxCode added 2 commits September 21, 2018 00:34

thxCode force-pushed the windows_cni branch from 915374c to b56ca2f Compare September 20, 2018 16:34

dcbw approved these changes Sep 20, 2018

View reviewed changes

dcbw merged commit a8ad12d into containernetworking:master Sep 20, 2018

thxCode deleted the windows_cni branch September 21, 2018 01:20

ptylenda mentioned this pull request Sep 21, 2018

Discussion roadmap v1.0 pablodav/kubernetes-for-windows#13

Closed

This was referenced Nov 20, 2018

Windows Binaries in Releases #232

Closed

Add a CNI for Windows #80

Closed

loganhz mentioned this pull request Mar 4, 2019

Windows GA in Rancher rancher/rancher#16460

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows CNI support #193

Windows CNI support #193

thxCode commented Aug 23, 2018

thxCode commented Aug 23, 2018 •

edited

Loading

benmoss commented Aug 28, 2018

rosenhouse commented Aug 29, 2018 •

edited

Loading

feiskyer commented Aug 29, 2018

rosenhouse commented Aug 29, 2018

rakelkar commented Aug 29, 2018 via email

thxCode commented Aug 30, 2018

dineshgovindasamy commented Aug 30, 2018

dineshgovindasamy commented Sep 5, 2018

rosenhouse commented Sep 5, 2018

benmoss commented Sep 5, 2018

JMesser81 commented Sep 8, 2018

daschott commented Sep 10, 2018

dcbw left a comment

dcbw Sep 5, 2018

dcbw Sep 12, 2018

dcbw Sep 12, 2018

dcbw Sep 12, 2018

dcbw Sep 12, 2018

dcbw Sep 12, 2018

dcbw Sep 12, 2018

dcbw Sep 12, 2018

dcbw Sep 12, 2018

dcbw Sep 12, 2018

dcbw Sep 12, 2018

dcbw Sep 12, 2018

dcbw Sep 12, 2018

thxCode commented Sep 13, 2018

bboreham left a comment

thxCode commented Sep 19, 2018

squeed commented Sep 19, 2018

dcbw commented Sep 20, 2018

Windows CNI support #193

Windows CNI support #193

Conversation

thxCode commented Aug 23, 2018

thxCode commented Aug 23, 2018 • edited Loading

benmoss commented Aug 28, 2018

rosenhouse commented Aug 29, 2018 • edited Loading

feiskyer commented Aug 29, 2018

rosenhouse commented Aug 29, 2018

rakelkar commented Aug 29, 2018 via email

thxCode commented Aug 30, 2018

dineshgovindasamy commented Aug 30, 2018

dineshgovindasamy commented Sep 5, 2018

rosenhouse commented Sep 5, 2018

benmoss commented Sep 5, 2018

JMesser81 commented Sep 8, 2018

daschott commented Sep 10, 2018

dcbw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thxCode commented Sep 13, 2018

bboreham left a comment

Choose a reason for hiding this comment

thxCode commented Sep 19, 2018

squeed commented Sep 19, 2018

dcbw commented Sep 20, 2018

thxCode commented Aug 23, 2018 •

edited

Loading

rosenhouse commented Aug 29, 2018 •

edited

Loading