Windows support for Flow Exporter with Flow Aggregator #2138

dreamtalen · 2021-04-29T22:28:31Z

In this commit, we fix the error when running Flow Exporter on Windows node with Flow Aggregator.
There is a limitation in DNS resolution on Windows, flow-aggregator.flow-aggregator.svc DNS name couldn't be resolved.
The reason is because on Windows the Antrea Agent runs as a process, it uses the host's default DNS setting and the DNS resolver will not be configured to talk to the CoreDNS Service for cluster local DNS queries likeflow-aggregator.flow-aggregator.svc.
So we require flowCollectorAddr could only be IP for Flow Exporter on Windows node.
For Flow Aggregator, we add IP in certicate since flow exporter on Windows node can't resolve DNS name.
Also change to use dpctl/ct-get-limits intead of dpctl/ct-get-maxconns since it returns operation not supported on Windows node.

codecov-commenter · 2021-04-29T22:34:28Z

Codecov Report

Merging #2138 (baf4ef4) into main (335e6bb) will increase coverage by 4.04%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main    #2138      +/-   ##
==========================================
+ Coverage   61.09%   65.14%   +4.04%     
==========================================
  Files         273      273              
  Lines       20644    20648       +4     
==========================================
+ Hits        12613    13451     +838     
+ Misses       6713     5818     -895     
- Partials     1318     1379      +61

Flag	Coverage Δ
e2e-tests	`56.31% <50.00%> (?)`
kind-e2e-tests	`52.05% <0.00%> (+<0.01%)`	⬆️
unit-tests	`41.01% <0.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/flowaggregator/certificate.go	`77.47% <50.00%> (+77.47%)`	⬆️
pkg/controller/networkpolicy/tier.go	`47.50% <0.00%> (-5.00%)`	⬇️
pkg/ovs/openflow/ofctrl_bridge.go	`50.36% <0.00%> (+0.36%)`	⬆️
pkg/agent/openflow/network_policy.go	`76.41% <0.00%> (+0.59%)`	⬆️
pkg/controller/networkpolicy/status_controller.go	`87.09% <0.00%> (+0.64%)`	⬆️
...ntroller/networkpolicy/networkpolicy_controller.go	`70.00% <0.00%> (+0.64%)`	⬆️
pkg/agent/cniserver/pod_configuration.go	`54.68% <0.00%> (+0.78%)`	⬆️
pkg/controller/egress/store/egressgroup.go	`1.38% <0.00%> (+1.38%)`	⬆️
pkg/ovs/openflow/ofctrl_flow.go	`47.36% <0.00%> (+1.75%)`	⬆️
pkg/agent/flowexporter/exporter/exporter.go	`77.88% <0.00%> (+2.64%)`	⬆️
... and 42 more

srikartati

LGTM.
Did you get a chance to run e2e test suite on windows and check if its passing? I see new Jenkins job for this.

srikartati · 2021-05-01T00:05:03Z

docs/network-flow-visibility.md

-name or the Cluster IP of the Service. Please note that the default values for
+name or the Cluster IP of the Service.
+
+Please note that for Antrea Agent running on a Windows node, `flowCollectorAddr` can only be IP right now because there is a DNS resolution issue in current Windows support.


Merge this line with the above paragraph. Something like..

"... with the Name and Namespace set to flow-aggregator. For Antrea Agent running on a Windows node, the user is required to change the default value of HOST in flowCollectorAddr from DNS name to the Cluster IP of Flow Aggregator service. The reason is because of the DNS resolution issue in the current Windows support of Kubernetes. In addition, if you deploy the Flow Aggregator Service with a different Name and Namespace, then either use the appropriate DNS name or the Cluster IP of the Service."

Thanks, addressed.

dreamtalen · 2021-05-03T17:35:58Z

/test-windows-e2e
/test-windows-conformance
/test-windows-networkpolicy

dreamtalen · 2021-05-03T17:51:21Z

LGTM.
Did you get a chance to run e2e test suite on windows and check if its passing? I see new Jenkins job for this.

Just triggered these Windows related Jenkins jobs, will pay attention to the result. Thanks.

antoninbas

It seems the lack of UDP support in netsh portproxy is a well-known issue, but what's the explanation for this:

However even we added --tcp-only option in Resolve-DnsName utility, it still can't resolve this DNS name.

antoninbas · 2021-05-04T00:09:12Z

pkg/agent/flowexporter/connections/conntrack_windows.go

+	connTrackOvsCtl
+}
+
+// dpctl/ct-get-maxconns returns operation not supported on Windows node, use dpctl/ct-get-limits intead.


That's not a good function-level comment. It belongs with the call to ct.ovsctlClient.RunAppctlCmd

Thanks, addressed.

antoninbas · 2021-05-04T00:09:35Z

pkg/agent/flowexporter/connections/conntrack_windows.go

+			return maxConns, nil
+		}
+	}
+	return 0, fmt.Errorf("doesn't find limit field in dpctl/ct-get-limits command output '%s'", cmdOutput)


Suggested change

return 0, fmt.Errorf("doesn't find limit field in dpctl/ct-get-limits command output '%s'", cmdOutput)

return 0, fmt.Errorf("couldn't find limit field in dpctl/ct-get-limits command output '%s'", cmdOutput)

Thanks, addressed.

not pushed yet?

pkg/agent/flowexporter/connections/conntrack_windows.go

antoninbas · 2021-05-04T00:12:49Z

docs/network-flow-visibility.md

-with the Name and Namespace set to `flow-aggregator`. If you deploy the Flow Aggregator
-Service with a different Name and Namespace, then either use the appropriate DNS
-name or the Cluster IP of the Service. Please note that the default values for
+with the Name and Namespace set to `flow-aggregator`. For Antrea Agent running on a Windows node, the user is required to change the default value of `HOST` in `flowCollectorAddr` from DNS name to the Cluster IP of Flow Aggregator service. The reason is because of the DNS resolution issue in the current Windows support of Kubernetes. In addition, if you deploy the Flow Aggregator Service with a different Name and Namespace, then either use the appropriate DNS name or the Cluster IP of the Service.


please keep line wrapped

Thanks, addressed.

dreamtalen · 2021-05-06T21:34:37Z

It seems the lack of UDP support in netsh portproxy is a well-known issue, but what's the explanation for this:

However even we added --tcp-only option in Resolve-DnsName utility, it still can't resolve this DNS name.

Sorry, we didn't probe further about "--tcp-only option" not working either, not sure if this is an existing issue of Resolve-DnsName utility on k8s clusters.

antoninbas · 2021-05-07T00:53:57Z

Sorry, we didn't probe further about "--tcp-only option" not working either, not sure if this is an existing issue of Resolve-DnsName utility on k8s clusters.

That's orthogonal to this PR, but I'd feel better knowing that this is not an issue on our side. Any chance the DNS traffic can be captured to see what happens with the DNS request?

antoninbas · 2021-05-07T00:55:44Z

docs/network-flow-visibility.md

-name or the Cluster IP of the Service. Please note that the default values for
+with the Name and Namespace set to `flow-aggregator`. For Antrea Agent running on 
+a Windows node, the user is required to change the default value of `HOST` in `flowCollectorAddr` 
+from DNS name to the Cluster IP of Flow Aggregator service. The reason is because 


of the Flow Aggregator Service

antoninbas · 2021-05-07T00:57:14Z

docs/network-flow-visibility.md

+with the Name and Namespace set to `flow-aggregator`. For Antrea Agent running on 
+a Windows node, the user is required to change the default value of `HOST` in `flowCollectorAddr` 
+from DNS name to the Cluster IP of Flow Aggregator service. The reason is because 
+of the DNS resolution issue in the current Windows support of Kubernetes. In addition, 


This is too vague. Please provide more context & point to a Github issue. The issue is specific to the userspace kube-proxy implementation on Windows, which Antrea currently depends on.

Sure, I didn't find an exisitng issue specific to the DNS resolve problem of kube-proxy on Windows, are these two issue good to cited here: #1935, #1954?

antoninbas · 2021-05-07T00:57:40Z

pkg/agent/flowexporter/connections/conntrack_windows.go

+			return maxConns, nil
+		}
+	}
+	return 0, fmt.Errorf("doesn't find limit field in dpctl/ct-get-limits command output '%s'", cmdOutput)


not pushed yet?

antoninbas · 2021-05-07T00:57:53Z

pkg/flowaggregator/certificate.go

@@ -97,6 +97,12 @@ func generateCertKey(caCert *x509.Certificate, caKey *rsa.PrivateKey, isServer b
 			cert.IPAddresses = []net.IP{ip}
 		} else {
 			cert.DNSNames = []string{flowAggregatorAddress}
+			// add IP in certicate since flow exporter on Windows node can't resolve DNS name


s/node/Node

dreamtalen · 2021-05-11T20:56:31Z

Sorry, we didn't probe further about "--tcp-only option" not working either, not sure if this is an existing issue of Resolve-DnsName utility on k8s clusters.

That's orthogonal to this PR, but I'd feel better knowing that this is not an issue on our side. Any chance the DNS traffic can be captured to see what happens with the DNS request?

I have used wireshark to capture some DNS traffic during DNS name resolving using ping or Resolve-DnsName -tcponly:

Not sure if it's relevant, we were able to use nslookup to resolve flow aggregator DNS name, here is the traffic captured:

srikartati · 2021-05-12T18:41:11Z

Not sure if it's relevant, we were able to use nslookup to resolve flow aggregator DNS name, here is the traffic captured:

As discussed offline, usingnslookup is one solution to resolve the DNS name instead of adding clusterIP in the TLS certificates. Any thoughts? @antoninbas

antoninbas · 2021-05-12T21:01:29Z

@dreamtalen @srikartati I spent some more time thinking about it. On Windows the Antrea Agent cannot be run as a Pod. It's run as a process (with possibly management Pods when the Docker container runtime is used). See https://github.com/antrea-io/antrea/blob/main/docs/windows.md. Unlike on Linux, the DNS resolver will not be configured to talk to the CoreDNS Service for cluster local DNS queries (e.g. flow-aggregator.flow-aggregator.svc). Instead the host's default DNS settings will be used. I believe that 10.72.40.1 is not the CoreDNS ClusterIP but the address of the DNS server configured for the host.

The nslookup query is not "working". I don't know what 10.195.47.241 is but it's not the IP address for the Flow Aggregator service in your cluster. It's a VMware webserver.

I think the only AI is to include the information above in the PR description / commit message.

dreamtalen · 2021-05-12T21:46:52Z

@dreamtalen @srikartati I spent some more time thinking about it. On Windows the Antrea Agent cannot be run as a Pod. It's run as a process (with possibly management Pods when the Docker container runtime is used). See https://github.com/antrea-io/antrea/blob/main/docs/windows.md. Unlike on Linux, the DNS resolver will not be configured to talk to the CoreDNS Service for cluster local DNS queries (e.g. flow-aggregator.flow-aggregator.svc). Instead the host's default DNS settings will be used. I believe that 10.72.40.1 is not the CoreDNS ClusterIP but the address of the DNS server configured for the host.

The nslookup query is not "working". I don't know what 10.195.47.241 is but it's not the IP address for the Flow Aggregator service in your cluster. It's a VMware webserver.

I think the only AI is to include the information above in the PR description / commit message.

Thanks a lot Antonin, just double checked the ClusterIP of Flow Aggregator service should be 10.98.233.117 in my cluster. Will update the PR accordingly.

srikartati

Misunderstood when Yongming mentioned that nslookup is working. Good that we could figure there is no issue on Antrea side for DNS resolution. Thanks, Antonin.

srikartati · 2021-05-14T19:07:10Z

build/yamls/windows/base/conf/antrea-agent.conf

+# stream of packets, a flow record will be exported to the collector once the elapsed
+# time since the last export event is equal to the value of this timeout.
+# Valid time units are "ns", "us" (or "µs"), "ms", "s", "m", "h".
+#activeFlowExportTimeout: "60s"


We are moving the default to 30s as part of this PR: #1949
Could you update that here to reflect the same?

Sure, updated.

srikartati

LGTM.
Please fix the failed CI tests.

srikartati · 2021-05-17T17:27:55Z

In addition, windows e2e tests are enabled now.
#2018
Could we add flow aggregator tests there?

dreamtalen · 2021-05-17T17:37:02Z

In addition, windows e2e tests are enabled now.
#2018
Could we add flow aggregator tests there?

LGTM.
Please fix the failed CI tests.

Sure, thanks.

dreamtalen · 2021-05-17T17:39:13Z

In addition, windows e2e tests are enabled now.
#2018
Could we add flow aggregator tests there?

Yes, we could do. Just got a new Windows cluster to test from zhecheng, will work on it.

srikartati

Because of windows test infra challenges, e2e support will be added in a follow-up PR.

Please rebase the PR with main for jenkins tests to pass.

In this commit, we fix the error when running Flow Exporter on Windows node with Flow Aggregator. There is a limitation in DNS resolution on Windows, flow-aggregator.flow-aggregator.svc DNS name couldn't be resolved. The reason is because on Windows the Antrea Agent runs as a process, it uses the host's default DNS setting and the DNS resolver will not be configured to talk to the CoreDNS Service for cluster local DNS queries. So we require flowCollectorAddr could only be IP for Flow Exporter on Windows node and add IP in certicate for flow aggregator. Also change to use dpctl/ct-get-limits intead of dpctl/ct-get-maxconns since it returns operation not supported on Windows node. Signed-off-by: Yongming Ding <[email protected]>

dreamtalen · 2021-05-19T21:37:17Z

/test-all

srikartati

LGTM

dreamtalen · 2021-05-20T20:16:45Z

/test-windows-conformance
/test-windows-networkpolicy
/test-e2e

dreamtalen · 2021-05-21T00:13:47Z

/test-windows-conformance
/test-windows-networkpolicy

srikartati · 2021-05-21T04:09:32Z

All the tests have passed. Merging it.
Flow Aggregator e2e test will be added to windows e2e test jenkins job.

vmwclabot added the cla-not-required label Apr 29, 2021

dreamtalen force-pushed the local-fa-win branch 2 times, most recently from b18503d to e8040c3 Compare April 29, 2021 22:32

dreamtalen force-pushed the local-fa-win branch from e8040c3 to d6a6ebd Compare April 29, 2021 23:08

srikartati reviewed May 1, 2021

View reviewed changes

dreamtalen force-pushed the local-fa-win branch from d6a6ebd to 149d987 Compare May 3, 2021 17:30

srikartati requested a review from antoninbas May 3, 2021 18:00

antoninbas reviewed May 4, 2021

View reviewed changes

dreamtalen force-pushed the local-fa-win branch from 149d987 to 2e693fc Compare May 6, 2021 21:26

antoninbas reviewed May 7, 2021

View reviewed changes

srikartati added this to the Antrea v1.1 release milestone May 12, 2021

dreamtalen force-pushed the local-fa-win branch 2 times, most recently from 1d429fc to 5059761 Compare May 13, 2021 00:07

srikartati reviewed May 14, 2021

View reviewed changes

dreamtalen force-pushed the local-fa-win branch 2 times, most recently from b013830 to baf0aed Compare May 17, 2021 17:08

srikartati previously approved these changes May 17, 2021

View reviewed changes

dreamtalen dismissed srikartati’s stale review via 639157d May 17, 2021 17:35

dreamtalen force-pushed the local-fa-win branch from baf0aed to 639157d Compare May 17, 2021 17:35

srikartati previously approved these changes May 19, 2021

View reviewed changes

srikartati mentioned this pull request May 19, 2021

Remove EnableTLSConfig from antrea agent #2193

Merged

dreamtalen dismissed srikartati’s stale review via baf4ef4 May 19, 2021 21:33

dreamtalen force-pushed the local-fa-win branch from 639157d to baf4ef4 Compare May 19, 2021 21:33

srikartati approved these changes May 20, 2021

View reviewed changes

srikartati merged commit fa54190 into antrea-io:main May 21, 2021

	return 0, fmt.Errorf("doesn't find limit field in dpctl/ct-get-limits command output '%s'", cmdOutput)
	return 0, fmt.Errorf("couldn't find limit field in dpctl/ct-get-limits command output '%s'", cmdOutput)

Windows support for Flow Exporter with Flow Aggregator #2138

Windows support for Flow Exporter with Flow Aggregator #2138

Conversation

dreamtalen commented Apr 29, 2021 • edited Loading

codecov-commenter commented Apr 29, 2021 • edited Loading

Codecov Report

srikartati left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreamtalen commented May 3, 2021

dreamtalen commented May 3, 2021

antoninbas 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

dreamtalen commented May 6, 2021

antoninbas commented May 7, 2021

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

dreamtalen commented May 11, 2021

srikartati commented May 12, 2021

antoninbas commented May 12, 2021

dreamtalen commented May 12, 2021

srikartati left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srikartati left a comment

Choose a reason for hiding this comment

srikartati commented May 17, 2021

dreamtalen commented May 17, 2021

dreamtalen commented May 17, 2021

srikartati left a comment

Choose a reason for hiding this comment

dreamtalen commented May 19, 2021

srikartati left a comment

Choose a reason for hiding this comment

dreamtalen commented May 20, 2021

dreamtalen commented May 21, 2021

srikartati commented May 21, 2021

dreamtalen commented Apr 29, 2021 •

edited

Loading

codecov-commenter commented Apr 29, 2021 •

edited

Loading

srikartati left a comment •

edited

Loading