Support Pod-to-Service flows with Flow Exporter when Antrea Proxy enabled #984

srikartati · 2020-07-27T17:16:19Z

This is based on PR #825. The actual commit for this PR is 714eb8f.

Flow exporter can send flow records of Pod-To-Service flows
with the service name, when Antrea proxy is enabled. Flow records from
the source host (local to the pod) contain the service name.
For this purpose, added a map in proxier:
ServicePort(clusterIP:Port/Proto) to ServicePortName

antrea-bot · 2020-07-27T17:16:33Z

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-whole-conformance: to trigger all conformance tests on linux.
/skip-whole-conformance: to skip all conformance tests on linux.
/test-networkpolicy: to trigger networkpolicy tests.
/skip-networkpolicy: to skip networkpolicy tests.
/test-windows-conformance: to trigger windows conformance tests.
/skip-windows-conformance: to skip windows conformance tests.
/test-windows-networkpolicy: to trigger windows networkpolicy tests.
/skip-windows-networkpolicy: to skip windows networkpolicy tests.
/test-all: to trigger all tests (except whole conformance).
/skip-all: to skip all tests (except whole conformance).

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

antrea-bot · 2020-07-29T11:25:32Z

Can one of the admins verify this patch?

antrea-bot · 2020-08-04T19:38:54Z

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-whole-conformance: to trigger all conformance tests on linux.
/skip-whole-conformance: to skip all conformance tests on linux.
/test-networkpolicy: to trigger networkpolicy tests.
/skip-networkpolicy: to skip networkpolicy tests.
/test-windows-conformance: to trigger windows conformance tests.
/skip-windows-conformance: to skip windows conformance tests.
/test-windows-networkpolicy: to trigger windows networkpolicy tests.
/skip-windows-networkpolicy: to skip windows networkpolicy tests.
/test-hw-offload: to trigger ovs hardware offload test.
/skip-hw-offload: to skip ovs hardware offload test.
/test-all: to trigger all tests (except whole conformance).
/skip-all: to skip all tests (except whole conformance).

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

antoninbas

Reviewed the latest commit. LGTM, only some minor comments.

antoninbas · 2020-08-07T19:14:16Z

pkg/agent/flowexporter/connections/connections.go

@@ -124,6 +130,26 @@ func (cs *ConnectionStore) addOrUpdateConn(conn *flowexporter.Connection) {
 		if !srcFound && dstFound {
 			conn.DoExport = false
 		}
+
+		// Pod-to-Service flows w/ antrea-proxy:Antrea proxy is enabled.


Suggested change

// Pod-to-Service flows w/ antrea-proxy:Antrea proxy is enabled.

// Process Pod-to-Service flows when Antrea proxy is enabled

antoninbas · 2020-08-07T19:16:12Z

pkg/util/ip/ip.go

@@ -167,3 +175,12 @@ func LookupProtocolMap(name string) (uint8, error) {
 	}
 	return proto, nil
 }
+
+// LookupServiceProtocol return service protocol string given protocol identifier
+func LookupServiceProtocol(protoID uint8) (corev1.Protocol, error) {


I don't think there is much value in having this function in this package, especially with the dependency on K8s. It is clear that you also only support protocols supported by K8s Services. I think you can just move it to pkg/agent/flowexporter/connections/connections.go

Only one file is using right now. Ok to move there and acknowledge the point on k8s dependency.

antoninbas · 2020-08-07T19:17:08Z

pkg/agent/flowexporter/connections/connections.go

+				svcPort := conn.TupleOrig.DestinationPort
+				protocol, err := ip.LookupServiceProtocol(conn.TupleOrig.Protocol)
+				if err != nil {
+					klog.Warningf("Could not retrieve service protocol: %v", err)


s/service/Service in log messages

I don't think this was addressed actually?

I changed in the other line. Missed this. Done.

antoninbas · 2020-08-07T19:42:34Z

pkg/agent/proxy/proxier.go

+func (p *proxier) addServiceByIP(serviceStr string, servicePortName k8sproxy.ServicePortName) {
+	p.serviceStringMapMutex.Lock()
+	defer p.serviceStringMapMutex.Unlock()
+	klog.V(2).Infof("Added service with key: %v value: %v", serviceStr, servicePortName)


Not convinced we need a log message just for updating this map? At least, maybe you can reduce the verbosity and add a symmetric log message for service deletion.

Used it for initial debugging. Function is straightforward; do not see much use as well.

antoninbas · 2020-08-07T19:42:57Z

pkg/agent/flowexporter/exporter/exporter_test.go

@@ -140,6 +141,7 @@ func TestFlowExporter_sendDataRecord(t *testing.T) {
 	for i, ie := range AntreaInfoElements {
 		elemList[i+len(IANAInfoElements)+len(IANAReverseInfoElements)] = ipfixentities.NewInfoElement(ie, 0, 0, 0, 0)
 	}
+	// Define mocks and flowExporter


not a very useful comment I think

removed it.

antoninbas · 2020-08-07T19:59:20Z

pkg/agent/flowexporter/exporter/exporter.go

@@ -61,6 +61,8 @@ var (
 		"destinationPodName",
 		"destinationPodNamespace",
 		"destinationNodeName",
+		"destinationClusterIP",
+		"destinationServiceName",


this includes the port as well right? should the name be destinationServicePortName?

Agreed. Changed the name here and in Antrea IPFIX registry

antrea-bot · 2020-08-11T14:27:32Z

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-whole-conformance: to trigger all conformance tests on linux.
/skip-whole-conformance: to skip all conformance tests on linux.
/test-networkpolicy: to trigger networkpolicy tests.
/skip-networkpolicy: to skip networkpolicy tests.
/test-windows-conformance: to trigger windows conformance tests.
/skip-windows-conformance: to skip windows conformance tests.
/test-windows-networkpolicy: to trigger windows networkpolicy tests.
/skip-windows-networkpolicy: to skip windows networkpolicy tests.
/test-hw-offload: to trigger ovs hardware offload test.
/skip-hw-offload: to skip ovs hardware offload test.
/test-all: to trigger all tests (except whole conformance).
/skip-all: to skip all tests (except whole conformance).

srikartati · 2020-08-11T16:07:32Z

/test-all

antoninbas

a couple of nits, otherwise LGTM

antoninbas · 2020-08-11T17:34:16Z

pkg/agent/flowexporter/connections/connections_test.go

+		TupleOrig:  *tuple3,
+		TupleReply: *revTuple3,


If you always end up dereferencing the tuples, I think it may make more sense to just return values (and not pointers) in makeTuple.

Sure.. Any insight on the copy of return value from a function VS copy from dereferencing? I believe the first one is little efficient because of return registers.

I think performance is not an issue here, since the struct is small, the function is not called many times, and this is test code. It's also not always faster to return a pointer. So I would favor readability. If you were embedding a point in flowexporter.Connection, I would recommend using a pointer instead.

You are also using the tuple as an immutable value in a way, so I think passing it around by value works well.

antoninbas · 2020-08-11T17:37:06Z

pkg/agent/flowexporter/exporter/exporter.go

+				// Sending dummy IP as IPFIX collector expects constant length of data for IP field.
+				// We should probably think of better approach as this involves customization of IPFIX collector to ignore
+				// this dummy IP address.
+				_, err = dataRec.AddInfoElement(ie, net.IP{0, 0, 0, 0})


newbie question: is it not possible to simply omit the information element in this case? Or will the collector complain if some elements are missing?

Yes collector checks that if number of elements in a data record matches to originally sent template record or not.

antoninbas · 2020-08-11T17:40:29Z

pkg/agent/flowexporter/connections/connections.go

@@ -184,3 +218,12 @@ func (cs *ConnectionStore) DeleteConnectionByKey(connKey flowexporter.Connection

 	return nil
 }
+
+// LookupServiceProtocol return service protocol string given protocol identifier


Suggested change

// LookupServiceProtocol return service protocol string given protocol identifier

// LookupServiceProtocol returns the corresponding Service protocol string given a protocol identifier.

srikartati · 2020-08-11T19:03:04Z

/test-all

antoninbas

LGTM

srikartati · 2020-08-11T20:49:14Z

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

srikartati · 2020-08-11T21:16:00Z

/test-e2e

srikartati · 2020-08-11T21:39:53Z

/test-e2e

srikartati · 2020-08-11T21:57:54Z

/test-e2e

srikartati · 2020-08-11T22:56:43Z

/test-e2e

antoninbas · 2020-08-11T23:01:02Z

@srikartati if there is an issue with the Jenkins e2e testbed, we should let @lzhecheng. In some cases, re-triggering the job too much actually makes things worse :(

srikartati · 2020-08-11T23:05:02Z

@srikartati if there is an issue with the Jenkins e2e testbed, we should let @lzhecheng. In some cases, re-triggering the job too much actually makes things worse :(

Ok. I see the following error:

=== Get kubeconfig (try for 1m) ===
Error from server (NotFound): secrets "antrea-e2e-for-pull-request-1022-kubeconfig" not found
=== Get kubeconfig (try for 1m) ===
Error from server (NotFound): secrets "antrea-e2e-for-pull-request-1022-kubeconfig" not found

I will check with @lzhecheng

srikartati · 2020-08-12T05:34:01Z

/test-all

srikartati · 2020-08-12T14:57:33Z

/test-windows-conformance
/test-hw-offload

srikartati · 2020-08-12T17:31:20Z

Hi @antoninbas: Can I skip hw-offload test for this PR too? This was in pending state for few hours and I could not see the logs as well. I see it is being done for some recent PRs.

antoninbas · 2020-08-12T17:38:49Z

/skip-hw-offload

Flow exporter can send flow records of Pod-To-Service flows with service name, when Antrea proxy is enabled. Flow records from source host (local to pod) contains service name. For this purpose, added a map in proxier: ServicePort(clusterIP:Port/Proto) to ServicePortName

srikartati · 2020-08-12T18:25:19Z

/test-all

srikartati · 2020-08-12T19:55:49Z

/test-network-policy
/skip-hw-offload

srikartati · 2020-08-12T19:56:16Z

/test-windows-networkpolicy

srikartati · 2020-08-12T20:56:10Z

/skip-hw-offload

antoninbas

Approving again

…bled (antrea-io#984) * Flow Exporter: Support Pod-To-Service flows with Antrea Proxy Flow exporter can send flow records of Pod-To-Service flows with the service name, when Antrea proxy is enabled. Flow records from the source host (local to the sender pod) contain a service name. For this purpose, added a map in proxier: ServicePort(clusterIP:Port/Proto) to ServicePortName

vmwclabot added the cla-not-required label Jul 27, 2020

srikartati changed the title ~~WIP: Support Pod-to-Service flows with Flow Exporter with Antrea Proxy~~ WIP: Support Pod-to-Service flows with Flow Exporter when Antrea Proxy enabled Jul 28, 2020

srikartati force-pushed the service_info branch from e641ee6 to c3fe824 Compare August 4, 2020 19:38

srikartati force-pushed the service_info branch 3 times, most recently from 519a36b to 714eb8f Compare August 5, 2020 05:48

srikartati changed the title ~~WIP: Support Pod-to-Service flows with Flow Exporter when Antrea Proxy enabled~~ Support Pod-to-Service flows with Flow Exporter when Antrea Proxy enabled Aug 7, 2020

antoninbas reviewed Aug 7, 2020

View reviewed changes

srikartati force-pushed the service_info branch 4 times, most recently from 15db4fc to 5039410 Compare August 11, 2020 14:27

srikartati added this to the Antrea v0.9.0 release milestone Aug 11, 2020

srikartati force-pushed the service_info branch from 5039410 to ab73f37 Compare August 11, 2020 14:44

srikartati force-pushed the service_info branch from ab73f37 to ad77568 Compare August 11, 2020 16:42

srikartati mentioned this pull request Aug 11, 2020

Support sending IPFIX flow records for Antrea flow exporter #825

Merged

srikartati force-pushed the service_info branch 3 times, most recently from b90fed8 to 736cf16 Compare August 11, 2020 17:31

antoninbas reviewed Aug 11, 2020

View reviewed changes

srikartati force-pushed the service_info branch 2 times, most recently from bd8f387 to c4c535b Compare August 11, 2020 18:58

antoninbas previously approved these changes Aug 11, 2020

View reviewed changes

MatthewHinton56 mentioned this pull request Aug 12, 2020

Support self-signed certificate rotation #1024

Merged

srikartati dismissed antoninbas’s stale review via 16e71e3 August 12, 2020 03:13

srikartati force-pushed the service_info branch 2 times, most recently from 16e71e3 to 6233fc7 Compare August 12, 2020 03:33

srikartati added 2 commits August 12, 2020 11:00

Addressed the comments

431ff8f

srikartati force-pushed the service_info branch from 98b1429 to 431ff8f Compare August 12, 2020 18:09

antoninbas approved these changes Aug 12, 2020

View reviewed changes

srikartati merged commit 14a248d into antrea-io:master Aug 12, 2020

	// Pod-to-Service flows w/ antrea-proxy:Antrea proxy is enabled.
	// Process Pod-to-Service flows when Antrea proxy is enabled

	// LookupServiceProtocol return service protocol string given protocol identifier
	// LookupServiceProtocol returns the corresponding Service protocol string given a protocol identifier.

Support Pod-to-Service flows with Flow Exporter when Antrea Proxy enabled #984

Support Pod-to-Service flows with Flow Exporter when Antrea Proxy enabled #984

Conversation

srikartati commented Jul 27, 2020 • edited Loading

antrea-bot commented Jul 27, 2020

antrea-bot commented Jul 29, 2020

antrea-bot commented Aug 4, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srikartati Aug 8, 2020 • edited Loading

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

antrea-bot commented Aug 11, 2020

srikartati commented Aug 11, 2020

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

srikartati commented Aug 11, 2020

antoninbas left a comment

Choose a reason for hiding this comment

srikartati commented Aug 11, 2020

srikartati commented Aug 11, 2020

srikartati commented Aug 11, 2020

srikartati commented Aug 11, 2020

srikartati commented Aug 11, 2020

antoninbas commented Aug 11, 2020 • edited Loading

srikartati commented Aug 11, 2020

srikartati commented Aug 12, 2020

srikartati commented Aug 12, 2020

srikartati commented Aug 12, 2020

antoninbas commented Aug 12, 2020

srikartati commented Aug 12, 2020

srikartati commented Aug 12, 2020

srikartati commented Aug 12, 2020

srikartati commented Aug 12, 2020

antoninbas left a comment

Choose a reason for hiding this comment

srikartati commented Jul 27, 2020 •

edited

Loading

srikartati Aug 8, 2020 •

edited

Loading

antoninbas commented Aug 11, 2020 •

edited

Loading