[Windows] Implement Windows host configuration

[wenyingd]

1. Create Transarent HNS Network in agent initialization.
2. Use netroute to configure routing rules when new Node join the
   cluster on Windows.
3. Extract OS specific functions under pkg/agent to seperate files.
4. Move iptables configuraitons to prepare host networking stage,
   and Windows creates HNS Network at the same stage.

[antrea-bot]

Thanks for your PR.
Unit tests and code linters are run automatically every time the PR is updated.
E2e, conformance and network policy tests can only be triggered by a member of the vmware-tanzu organization. Regular contributors to the project should join the org.

The following commands are available:

• /test-e2e: to trigger e2e tests.
• /skip-e2e: to skip e2e tests.
• /test-conformance: to trigger conformance tests.
• /skip-conformance: to skip conformance tests.
• /test-networkpolicy: to trigger networkpolicy tests.
• /skip-networkpolicy: to skip networkpolicy tests.
• /test-all: to trigger all tests.
• /skip-all: to skip all tests.

These commands can only be run by members of the vmware-tanzu organization.

[wenyingd]

/test-all

[jianjuns]

We merged the noencap PR today. I guess you need to rebase your branch and adjust the files for your two PRs.

[jianjuns]

Do you think it makes sense to create a separate interface for OS specific funcs which have different implementations for different OSes, rather than reusing Initializer?
The benefit is reader can easily figure out the OS specific funcs. I am not very sure if it is right way in Go or not.
@tnqn

[tnqn]

So far I see a lot of such cases in K8s are done via _OS.go approach, the reason in my mind is:

• clean code, no need to have branch/options to switch implementation

• bit size saving (even in interface way, we shouldn't and can't compile windows impl in linux bit, so there's going to be _OS.go anyway, then why it needs to have an option to select OS based implementation?)

• it can do more and has less requirement (the different part can be a function, a variable, a struct) while interface way requires well-designed interfaces and strongly consistent code on interface consumer side.

But I haven't experienced this before, I will take a look at more Go projects and suggest.

[jianjuns]

Not sure if I got all your mean. But even we have an interface, we can still achieve what you said (no extra branch/option, the interface implementation can still be in _OS.go)? If the interface implementation is in the same package then almost no difference from convenience perspective?

[tnqn]

Yes, but having this interface or not is not related to OS implementation but whether we want to make it abstract. I'm not against abstracting interface, just prefer not calling platform specific functions with platform specific names on main path.
For example, having a generic function setupExternalNetworking to call i.iptablesClient.Initialize in host_configuration_linux.go looks better to me than calling i.iptablesClient.Initialize directly with a dummy implementation on main path.

[jianjuns]

Agreed on what you said, we should define generic interface/funcs rather than OS specific ones like iptablesXYZ.

[wenyingd]

/test-all

[wenyingd]

/test-all

[wenyingd]

/test-all

[tnqn]

Overall LGTM, some comments inline

[wenyingd]

/test-all

[wenyingd]

/test-all

[wenyingd]

/test-all

[tnqn]

This is the single remaining IPTables logic in main path, could we replace it with a generic function name like others

[jianjuns]

+1.

To make the code simpler, do you think we can keep the loop for Windows as well, but just change c.iptablesClient.AddPeerCIDR to allowTrafficToPeerCIDR? And then we could even remove node_route_controller_os.go and put allowTrafficToPeerCIDR to host_configuration_os.go?

[wenyingd]

Actually, the two functions in node_route_controller_os are both to call iptables. If we only keep a single file without any OS suffix, the code would be simpler(it is also the previous version of this PR), but iptables functions will be invoked on Windows. Althought it would be no error returned since I have implemented a dummy implementation on Windows, but it still needs an extra invocation. So I think it should be a trade-off. What's your opinion, @jianjuns @tnqn ?

[jianjuns]

+1.

To make the code simpler, do you think we can keep the loop for Windows as well, but just change c.iptablesClient.AddPeerCIDR to allowTrafficToPeerCIDR? And then we could even remove node_route_controller_os.go and put allowTrafficToPeerCIDR to host_configuration_os.go?

[jianjuns]

How about name it allowNodeToPeerCIDRTraffic?

Another question - do you think we should change Linux to SNAT in OVS too, so to unify Windows and Linux? @tnqn

[wenyingd]

I will rename the function.

[jianjuns]

I would agree with what @tnqn said - do not create dummy funcs if they are not needed for an OS.

[jianjuns]

I now feel we should create an interface like TrafficFilterClient:

• put the New() func in agent_os.go
• move allowNodeToPeerCIDRTraffic to this interface.
• let iptablesClient implements the interface.

Then we can remove iptables_windows.go (change it to traffic_filter_windows.go).

What you think?

[wenyingd]

@jianjuns I don't think we could remove iptables_windows.go(or traffic_filter_windows.go) even if we define an interface. Since "iptables" is a Linux specific library, all functions calling this library should be placed in file with _linux suffix, otherwise it should break compiling on Windows. If so, function "allowNodeToPeerCIDRTraffic" must have separate implementations with different OS type, and the WIndows implementation might be an empty function. So do other functions we plan to in the interface.

[jianjuns]

No, I am not saying we can remove traffic_filter_windows.go. I am just saying we should not call it iptables_windows.go, and if we have an interface, it just includes a single func of allowNodeToPeerCIDRTraffic().

[jianjuns]

traffic_filter_windows.go should include the implementation of allowNodeToPeerCIDRTraffic().

[wenyingd]

I could rename the file as 'traffic_filter_windows.go'.

allowNodeToPeerCIDRTraffic() should not be the single method of the interface. These methods are required outside package "iptables" on Linux Node: 1) NewClient(), 2) Initialize, 3) Reconcile, 4) AllowNodeToPeerCIDRTraffic. If we define an interface and return it in function "NewClient", all above functions should be defined in the interface, and the OS specific implementations must be privoded.

We have no such functions in current _windows.go file is because we returned the struct directly, and no other functions except for "NewClient" are called outside "iptables" on Windows .

[jianjuns]

Yes, you need Initialize() at least. NewClient() should not be defined in the interface, but could be in agent_os.go.

For Reconcile() I was thinking you might convert the trafficFilter interface to iptablesClient in node_route_controller_windows.go, and call iptablesClient.reconcile() directly. But if you think that is strange, and not many other funcs, we might add Reconcile() to the interface.

[jianjuns]

Do you think can be merged into prepareHostNetworking()?

[wenyingd]

It can't be merged into prepareHostNetworking() if we planned to do SNAT with OpenFlow, for the OVS is not ready when preparing Host Networking on Windows.

But if we could change to use Windows NAT utilities to provide SNAT, it would be OK to merge them.

[jianjuns]

Ok. Do you think it can be a good idea to make SNAT by OVS a separate option if it works on Linux as well? But I guess let us keep your current implementation first.

[jianjuns]

@tnqn: do you think we can mvoe files in util/ethtool/ and net_os.go to a single pkg util/net/?

[tnqn]

Yes, they sounds close.

[jianjuns]

I now feel stronger we should put net_os.go to util/net, and maybe move ethtool files there too. We added more and more funcs here.

[tnqn]

Yes, I plan to move them after this PR and policy only PR is checked in, don't want to make them have to solve conflicts again.

[jianjuns]

Sounds good.

[ruicao93]

/test-all

[tnqn]

A minor comment

[jianjuns]

I am not sure this one and enableHNSOnOVS() should be put inside package util. They sound like some application logics but not utilities?
Should we still have the agent_windows.go? @tnqn

[wenyingd]

According to the offline disscussion we had last week, I remove the call of function PrepareHostNetworking from agent.go, and plan to move it to Antrea-Service, that's why I move the function to utils(If it is still not proper, I think a choice is to move it to Antrea-Service). Then the only function differentiates OS in agent should be SNAT. Quan has moved iptables-SNAT to routeClient initialization. Windows is using OpenFlow to provide this feature, and I didn't want to add OFClient as a property of routeClient. So agent_window might be added back in "SNAT" PR.

[jianjuns]

Sorry, you mean you want to move PrepareHostNetworking() to Service later, or keep it in Agent? If we keep it Agent, I feel it should not be in util, and agent_windows.go is better.

[wenyingd]

It is planned to move to Service.

[jianjuns]

Do we have other similar funcs in service code?
In dev env, if we do not have a service, how could we prepare the network before Agent runs?

[jianjuns]

Got it. I now feel from convenience perspective, seems the logic in agent is more convenient. So, I wonder do we have similar cases in service now (I remember no).
If we anyway will have agent_windows.go for other funcs like NAT, we might put this one there too?

[wenyingd]

Currently, we only have three functions planned to be in Service: 1) Start/Monitor Agent, 2) Prepare environment including HNS Network; 3) Monitoring ovs-vswitchd service, and recover host network when OVS is down.

No other funcs like PrepareHostNetworking are in Service.

A bad effect for placing PrepareHostNetworking in Agent is, it will make Agent behavior on Windows different from Linux.

[jianjuns]

I assume 2) and 3) (recover host network when OVS is down) are not really relevant right, or 3) is to recover what are changed in 2)?

[wenyingd]

In my opinion, the latter is a correct relation between 2) and 3), as OVS takes over Windows host network is because of 2), and when OVS is down, the host network should be controlled by Windows system.

[jianjuns]

Ok. Then it makes to move them to service.

[jianjuns]

I now feel stronger we should put net_os.go to util/net, and maybe move ethtool files there too. We added more and more funcs here.

[wenyingd]

/test-all