Support dumping OVS flows for a Service with antctl #1877

jianjuns · 2021-02-17T04:16:28Z

antctl get of -S antrea -n kube-system

FLOW
table=41, n_packets=0, n_bytes=0, priority=200,tcp,reg4=0x10000/0x70000,nw_dst=10.108.138.236,tp_dst=443 actions=group:5
table=42, n_packets=0, n_bytes=0, priority=200,tcp,reg3=0xad85d17,reg4=0x2286d/0x7ffff actions=ct(commit,table=50,zone=65520,nat(dst=10.216.93.23:10349),exec(load:0x21->NXM_NX_CT_MARK[]))
table=106, n_packets=0, n_bytes=0, priority=200,ip,nw_src=10.216.93.23,nw_dst=10.216.93.23 actions=set_field:169.254.169.252->ip_src,load:0x1->NXM_NX_REG0[18],goto_table:110
group_id=5,type=select,bucket=bucket_id:0,weight:100,actions=load:0xad85d17->NXM_NX_REG3[],load:0x286d->NXM_NX_REG4[0..15],load:0x2->NXM_NX_REG4[16..18],load:0x1->NXM_NX_REG0[19],resubmit(,42)

docs/antctl.md

pkg/agent/apiserver/handlers/ovsflows/handler.go

pkg/agent/openflow/client.go

weiqiangt · 2021-02-22T17:49:39Z

pkg/agent/proxy/proxier.go

+// modifying the proxy.Provider interface.
+type Proxier interface {
+	// GetProxyProvider returns the real proxy Provider.
+	GetProxyProvider() k8sproxy.Provider


I think we could just extend the k8sproxy.Provider instead of having this method.

I did not know if you changed k8sproxy.Provider already or not (that is why I hope you can describe all the changes made the kube-proxy pkgs somewhere). However, I feel always better to reduce the changes to copied code to make code sync easier.

Yes, the changes are described at the top of the files in third-party.
The change I actually suggested from this comment is like

type Proxier interface { k8sproxy.Provider }

Are you suggesting:

type Proxier interface { k8sproxy.Provider GetServiceFlowKeys(serviceName, namespace string) ([]string, []binding.GroupIDType) }

I thought about it, but not sure if mockgen works or not. Do you know?

I just tried and the mock contains all defined methods.

Thanks. But I recalled the problem is for NewDualStackProxier(). How would you return an instance that implements k8sproxy.Provider then?

I think we should then use our own interface instead of the Provider.

I walked through the code and found there is no usage of Provider in the third party package.

That means we need to rewrite metaProxier too? I still hope to reduce changes to the copied code.

Make sense, I missed that part. Then current implementation looks good.

weiqiangt · 2021-02-22T17:54:28Z

pkg/agent/proxy/proxier.go


+	p.serviceEndpointsMapsMutex.Lock()


I sugges to using a function to wrap this snippet.

func() { lock() defer unlock() ... }()

Make sense. In this case, as really not much after the lock protection in this func, I just change to put "defer p.serviceEndpointsMapsMutex.Unlock()" after "p.serviceEndpointsMapsMutex.Lock()". Let me know what you think.

Sounds good.

jianjuns · 2021-02-22T18:20:43Z

pkg/agent/openflow/client.go

@@ -480,6 +488,17 @@ func (c *client) UninstallLoadBalancerServiceFromOutsideFlows(svcIP net.IP, svcP
 	return c.deleteFlows(c.serviceFlowCache, cacheKey)


@weiqiangt : BTW, I wonder why in InstallEndpointFlows() we pass the extra isIPv6 argument, but not just let net.ParseIP() return a net.IP and check it is v4 or v6 when necessary. Is it for some efficiency consideration?

Yes, for efficiency consideration, otherwise we will create a small bytes slice each time we check. cc @ksamoray.

But in this case, the IP is checked at most twice: one in endpointDNATFlow() and one in hairpinSNATFlow, and the latter is only for local Endpoint. Is it worth to optimize?

Considering parameters passing is simple I thought it is OK. But a second thought is that the cost here is not that critical, I'm fine with the change also.

As endpointDNATFlow() performs To4() anyway for v4, this argument has no importance as To4() is later applied again here
https://github.com/vmware-tanzu/antrea/blob/main/pkg/agent/openflow/pipeline.go#L1830

Thanks for the info guys! So, let us remove the argument then.

jianjuns · 2021-02-22T19:00:55Z

pkg/agent/openflow/client.go

+	return fmt.Sprintf("Service_%s_%d_%s", svcIP, svcPort, protocol)
+}
+
+func (c *client) InstallEndpointFlows(protocol binding.Protocol, endpoints []proxy.Endpoint) error {


I removed the isIPv6 argument, as I could not figure out why it is needed. Let me know if I am wrong. @weiqiangt

codecov-io · 2021-02-22T19:56:47Z

Codecov Report

❗ No coverage uploaded for pull request base (main@6ec79a7). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1877   +/-   ##
=======================================
  Coverage        ?   58.79%           
=======================================
  Files           ?      201           
  Lines           ?    17374           
  Branches        ?        0           
=======================================
  Hits            ?    10215           
  Misses          ?     6088           
  Partials        ?     1071

Flag	Coverage Δ
e2e-tests	`44.98% <0.00%> (?)`
unit-tests	`41.19% <0.00%> (?)`

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

weiqiangt

LGTM

jianjuns · 2021-02-24T22:28:53Z

/test-windows-conformance

jianjuns · 2021-02-25T03:55:38Z

/test-all

weiqiangt · 2021-02-25T03:59:51Z

/test-all

antoninbas

LGTM

antoninbas · 2021-02-26T00:46:23Z

pkg/agent/proxy/proxier.go

+	if v4Flows == nil {
+		if v6Flows == nil {
+			return nil, nil
+		}
+		// Not to return nil, even v6Flows is empty.
+		v4Flows = make([]string, 0, len(v6Flows))
+	}


I feel like a third parameter would have been clearer, i.e. the function could return (found bool, flowKeys []string, groupIDs []binding.GroupIDType)

Sure. Change to what you suggested.

antoninbas · 2021-02-26T00:47:25Z

pkg/ovs/ovsctl/ofctl.go

@@ -63,6 +64,27 @@ func (c *ovsCtlClient) DumpTableFlows(table uint8) ([]string, error) {
 	return c.DumpFlows(fmt.Sprintf("table=%d", table))
 }

+func (c *ovsCtlClient) DumpGroup(groupID int) (string, error) {
+	// There seems a bug in ovs-ofctl that dump-groups always returns all
+	// the groups when using Openflow13, even the group ID is provided. As


s/even the group ID is provided/even when the group ID is provided

antoninbas · 2021-02-26T00:47:51Z

pkg/ovs/ovsctl/ofctl.go

@@ -63,6 +64,27 @@ func (c *ovsCtlClient) DumpTableFlows(table uint8) ([]string, error) {
 	return c.DumpFlows(fmt.Sprintf("table=%d", table))
 }

+func (c *ovsCtlClient) DumpGroup(groupID int) (string, error) {
+	// There seems a bug in ovs-ofctl that dump-groups always returns all


out-of-curiosity did you report the bug?

Not yet. Will do.

antoninbas

LGTM

jianjuns · 2021-02-26T21:04:55Z

/test-all

jianjuns · 2021-02-26T22:17:50Z

/test-all-features-conformance

Extend Agent /ovsflows API handler to support returning OVS flows for a Service. Extend agent/proxy/proxier to query OVS flow keys and group IDs for a Service. Extend ovs/ovsctl/ofctl to support dumping a specific OVS group. Extend "antctl get ovsflows" command to dump OVS flows for a Service.

jianjuns · 2021-02-26T22:56:56Z

/test-all-features-conformance

jianjuns · 2021-02-27T00:34:21Z

/test-all-features-conformance

jianjuns · 2021-02-27T00:34:54Z

/test-all

antoninbas

Approving again after rebase

Extend Agent /ovsflows API handler to support returning OVS flows for a Service. Extend agent/proxy/proxier to query OVS flow keys and group IDs for a Service. Extend ovs/ovsctl/ofctl to support dumping a specific OVS group. Extend "antctl get ovsflows" command to dump OVS flows for a Service.

vmwclabot added the cla-not-required label Feb 17, 2021

jianjuns requested a review from weiqiangt February 20, 2021 00:59

weiqiangt reviewed Feb 22, 2021

View reviewed changes

jianjuns commented Feb 22, 2021

View reviewed changes

jianjuns force-pushed the service-flows branch 2 times, most recently from 36d3029 to a0e91fb Compare February 22, 2021 18:47

jianjuns commented Feb 22, 2021

View reviewed changes

jianjuns force-pushed the service-flows branch 3 times, most recently from 25d7800 to b0ea83a Compare February 22, 2021 19:52

jianjuns force-pushed the service-flows branch from b0ea83a to d6fdc09 Compare February 23, 2021 00:41

weiqiangt previously approved these changes Feb 23, 2021

View reviewed changes

weiqiangt mentioned this pull request Feb 24, 2021

Re-arrange bootstrap order of sub-components in Antrea Agent #1895

Closed

jianjuns dismissed weiqiangt’s stale review via 9464b73 February 24, 2021 19:22

jianjuns force-pushed the service-flows branch 2 times, most recently from 9464b73 to d32c258 Compare February 24, 2021 22:27

weiqiangt previously approved these changes Feb 25, 2021

View reviewed changes

antoninbas previously approved these changes Feb 26, 2021

View reviewed changes

jianjuns dismissed stale reviews from antoninbas and weiqiangt via 34808a3 February 26, 2021 06:01

jianjuns force-pushed the service-flows branch from d32c258 to 34808a3 Compare February 26, 2021 06:01

antoninbas previously approved these changes Feb 26, 2021

View reviewed changes

jianjuns dismissed antoninbas’s stale review via caf5845 February 26, 2021 22:56

jianjuns force-pushed the service-flows branch from 34808a3 to caf5845 Compare February 26, 2021 22:56

antoninbas approved these changes Feb 27, 2021

View reviewed changes

jianjuns merged commit cdf357f into antrea-io:main Feb 27, 2021

		@@ -480,6 +488,17 @@ func (c *client) UninstallLoadBalancerServiceFromOutsideFlows(svcIP net.IP, svcP
		return c.deleteFlows(c.serviceFlowCache, cacheKey)

Support dumping OVS flows for a Service with antctl #1877

Support dumping OVS flows for a Service with antctl #1877

Conversation

jianjuns commented Feb 17, 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

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

codecov-io commented Feb 22, 2021 • edited Loading

Codecov Report

weiqiangt left a comment

Choose a reason for hiding this comment

jianjuns commented Feb 24, 2021

jianjuns commented Feb 25, 2021

weiqiangt commented Feb 25, 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

antoninbas left a comment

Choose a reason for hiding this comment

jianjuns commented Feb 26, 2021

jianjuns commented Feb 26, 2021

jianjuns commented Feb 26, 2021

jianjuns commented Feb 27, 2021

jianjuns commented Feb 27, 2021

antoninbas left a comment

Choose a reason for hiding this comment

codecov-io commented Feb 22, 2021 •

edited

Loading