Enable Calico-Felix for Windows #1638

nwoodmsft · 2017-11-30T06:00:18Z

Description

This change enables Calico-Felix to be compiled and run on Windows (policy-only). There are three main work items being covered in this change:

Makefile is updated to support cross-compiling a Calico-Felix binary for Windows (bin/calico-felix.exe)
To enable compilation to succeed for Windows, there is some minor refactoring of functions/blocks (mainly from felix.go) into seperate source files with build constraints.
A Windows dataplane driver has been added. This provides the ability to program policies into the Windows dataplane via the Host Network Service (HNS).

Todos

Unit tests (full coverage)
Integration tests (delete as appropriate) In plan
Documentation (Windows deployment guidance is in-review)
~~Backport~~
Release note

Release Note

Enabled Calico-Felix to be compiled and run on Windows (policy-only)

CLAassistant · 2017-11-30T06:00:24Z

All committers have signed the CLA.

fasaxc

Thanks @nwoodmsft. The code generally looks in good shape and it's easy to follow. Most of the issues I spotted were to do with changes that have gone into master since you took your fork:

we now have a named port IP set type; for this PR, I propose that you add that to your list of match types to log and skip
we added health check support to Felix so it'd be good to plumb that through into the windows driver

I think I spotted a bug in the rule conversion when there are multiple IP sets to match.

The rest are suggested cleanups; I think it should be pretty trivial to avoid duplicating code in logutils for example. I'd also like to get to the bottom of the set.go issue but that doesn't need to block the merge if it's unreasonably tricky.

I wonder if we should update the README to mention the Windows support and make sure people understand what is and isn't supported.

fasaxc · 2017-11-30T09:49:45Z

dataplane-drivers/windataplane/endpoint_mgr.go

+	case *proto.WorkloadEndpointRemove:
+		log.WithField("workloadEndpointId", msg.Id).Info("Processing WorkloadEndpointRemove")
+		m.pendingWlEpUpdates[*msg.Id] = nil
+	case *proto.IPSetDeltaUpdate:


I think you might need to process IPSetUpdate here too. I think it might be possible in some corner cases for the calc graph to send an IPSetUpdate then a second IPSetUpdate rather than sending deltas.

Thanks. I hadn't seen a case (in testing) where updates came through as a IPSetUpdate instead of a IPSetDeltaUpdate, so I thought it was safe to have endpoint manager just react to the Deltas. If there are cases where IPSetUpdate might be generated instead, then we definitely need to handle this here. Added.

fasaxc · 2017-11-30T10:12:00Z

dataplane-drivers/windataplane/endpoint_mgr.go

+	// endpoint id for a given endpoint ip address.
+	addressToEndpointId map[string]string
+	// read-write lock to protect access to the addressToEndpointId map during cache updates.
+	endpointLock sync.RWMutex


I don't think you need this lock, unless I missed somethingOnUpdate and CompleteDeferredWork are both called from the same goroutine (the dataplane driver's main loop).

Agreed, will remove. I think there was a case in an earlier iteration of the code where we thought we needed this to be on the safe side, but it doesn't look like it's needed now.

fasaxc · 2017-11-30T10:15:45Z

dataplane-drivers/windataplane/endpoint_mgr.go

+			activePolicyNames = append(activePolicyNames, workload.Tiers[0].EgressPolicies...)
+		} else {
+			if len(workload.ProfileIds) > 0 {
+				activePolicyNames = workload.ProfileIds


You should namespace policy/profile names; you could get a profile and a policy with the same name.

Good catch, i'll take a look.

fasaxc · 2017-11-30T10:20:19Z

dataplane-drivers/windataplane/endpoint_mgr.go

+	var rules []*hns.ACLPolicy
+	if (len(policyNames) > 0) {
+		rules = m.policysetsDataplane.GetPolicySetRules(policyNames)
+		for _, rule := range rules {


If there could be many rules here, might be worth wrapping this in a conditional: if log.GetLevel() >= log.DebugLevel {...

fasaxc · 2017-11-30T10:25:24Z

dataplane-drivers/windataplane/policy_mgr.go

+	switch msg := msg.(type) {
+	case *proto.ActivePolicyUpdate:
+		log.WithField("policyID", msg.Id).Info("Processing ActivePolicyUpdate")
+		m.policysetsDataplane.AddOrReplacePolicySet(msg.Id.Name, msg.Policy)


Think you may need to namespace the profiles/policies by type to avoid clashes.

fasaxc · 2017-11-30T11:19:25Z

dataplane-drivers/windataplane/win_dataplane.go

+			log.Debug("Reschedule kick received")
+			d.dataplaneNeedsSync = true
+			d.reschedC = nil
+		}


Please can you port across the health reporting function. Think we added that after you took your fork. Should just be a matter of starting an extra ticker and calling HealthAggregator.Report(...). Felix now has a health endpoint for Kubernetes to poll.

fasaxc · 2017-11-30T11:26:30Z

logutils/logutils.go

@@ -1,3 +1,5 @@
+// +build !windows


Please can you pull out the shared function into a shared file? I think that'd be pretty easy to do for this module.

I think you could:

pull out a function configureSyslog that is implemented on Linux, but stubbed on Windows

pull out a function openLogFile that is implemented differently on each

share everything else

Ok will take a look

fasaxc · 2017-11-30T11:28:18Z

glide.yaml

@@ -59,3 +59,5 @@ import:
  version: 2e7ea655c10e4d4d73365f0f073b81b39cb08ee1
  subpackages:
  - net
+- package: github.com/Microsoft/go-winio


Please can you pin these to a version or a specific revision? We've found that glide can resolve floating dependencies differently depending on the state of its cache configuration so we've taken to pinning all our immediate dependencies.

Yes, will do.

fasaxc · 2017-11-30T11:45:54Z

dataplane-drivers/windataplane/policysets/policysets.go

+	if len(pRule.NotSrcIpSetIds) > 0 || len(pRule.NotDstIpSetIds) > 0 {
+		return true
+	}
+	if pRule.NotProtocol != nil {


master has some new negated matches to consider for named ports.

Ok for now I've added NotSrcNamedPortIpSetIds and NotDstNamedPortIpSetIds to this list.

fasaxc · 2017-11-30T11:49:02Z

dataplane-drivers/windataplane/win_dataplane.go

+
+// msgStringer wraps an API message to customise how we stringify it.  For example, it truncates
+// the lists of members in the (potentially very large) IPSetsUpdate messages.
+type msgStringer struct {


Please can you move this to a utility package (maybe put it in the proto package along with the protobuf definitions?) That'd let us share it between the two implementations.

Yes, makes sense to share this code. Done.

fasaxc · 2017-12-04T13:23:40Z

@nwoodmsft Please can you resolve conflicts? To resolve the glide.lock, it should just be a matter of merging glide.yaml and then re-running make update-vendor.

Resolving the glide conflict should fix the Typha pin issue picked up by Semaphore.

fasaxc

LGTM

fasaxc added kind/enhancement release-note-required labels Nov 30, 2017

fasaxc self-assigned this Nov 30, 2017

fasaxc requested changes Nov 30, 2017

View reviewed changes

nwoodmsft added 6 commits December 4, 2017 16:50

Enable Calico-Felix for Windows

a066229

Adding copyright headers to new source files

fa9500f

Remove monotime dependency

0a096f4

go-fmt updates and fixed windows dataplane unit test

ac4d05e

Addressing PR feedback

f51feb1

Addressing merge issues for the PR

5ef6386

nwoodmsft force-pushed the master branch from 69c7eb5 to 5ef6386 Compare December 5, 2017 00:54

nwoodmsft added 2 commits December 4, 2017 17:13

Fixing felix.go merge

0bf0190

Fixing felix.go goimport

149c7f7

fasaxc approved these changes Dec 7, 2017

View reviewed changes

fasaxc merged commit 661afe2 into projectcalico:master Dec 7, 2017

fasaxc added this to the Calico v3.0.0 milestone Dec 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Calico-Felix for Windows #1638

Enable Calico-Felix for Windows #1638

nwoodmsft commented Nov 30, 2017 •

edited by fasaxc

Loading

CLAassistant commented Nov 30, 2017 •

edited

Loading

fasaxc left a comment

fasaxc Nov 30, 2017

nwoodmsft Dec 1, 2017

fasaxc Nov 30, 2017

nwoodmsft Dec 1, 2017

fasaxc Nov 30, 2017

nwoodmsft Dec 1, 2017

fasaxc Nov 30, 2017

nwoodmsft Dec 1, 2017

fasaxc Nov 30, 2017

fasaxc Nov 30, 2017

nwoodmsft Dec 2, 2017

fasaxc Nov 30, 2017

nwoodmsft Dec 1, 2017

fasaxc Nov 30, 2017

nwoodmsft Dec 1, 2017

fasaxc Nov 30, 2017

nwoodmsft Dec 2, 2017

fasaxc Nov 30, 2017

nwoodmsft Dec 1, 2017

fasaxc commented Dec 4, 2017 •

edited

Loading

fasaxc left a comment

Enable Calico-Felix for Windows #1638

Enable Calico-Felix for Windows #1638

Conversation

nwoodmsft commented Nov 30, 2017 • edited by fasaxc Loading

Description

Todos

Release Note

CLAassistant commented Nov 30, 2017 • edited Loading

fasaxc 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

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

fasaxc commented Dec 4, 2017 • edited Loading

fasaxc left a comment

Choose a reason for hiding this comment

nwoodmsft commented Nov 30, 2017 •

edited by fasaxc

Loading

CLAassistant commented Nov 30, 2017 •

edited

Loading

fasaxc commented Dec 4, 2017 •

edited

Loading