Add network policy info to IPFIX flow records as part of Flow Exporter #1268

srikartati · 2020-09-18T05:22:01Z

Network policy info (name and namespace), both ingress and egress
policies are added.

Async rule cache in idAllocator is configured with asyncDeleteInterval that is maximum of either 5s or configured flowPollInterval in Antrea Agent.

antrea-bot · 2020-09-18T05:22:15Z

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).

codecov-commenter · 2020-09-18T05:26:14Z

Codecov Report

Merging #1268 into master will decrease coverage by 10.20%.
The diff coverage is 9.09%.

@@             Coverage Diff             @@
##           master    #1268       +/-   ##
===========================================
- Coverage   54.98%   44.78%   -10.21%     
===========================================
  Files         110       73       -37     
  Lines       10573     5564     -5009     
===========================================
- Hits         5814     2492     -3322     
+ Misses       4185     2791     -1394     
+ Partials      574      281      -293

Flag	Coverage Δ
#integration-tests	`44.78% <9.09%> (-0.18%)`	⬇️
#unit-tests	`?`

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

Impacted Files	Coverage Δ
.../agent/flowexporter/connections/conntrack_linux.go	`0.00% <0.00%> (-8.20%)`	⬇️
pkg/agent/flowexporter/connections/connections.go	`35.89% <10.00%> (-33.18%)`	⬇️
pkg/agent/proxy/service.go	`0.00% <0.00%> (-100.00%)`	⬇️
pkg/ovs/ovsconfig/default.go	`0.00% <0.00%> (-100.00%)`	⬇️
pkg/agent/util/iptables/lock.go	`0.00% <0.00%> (-81.82%)`	⬇️
pkg/agent/proxy/endpoints.go	`0.00% <0.00%> (-81.43%)`	⬇️
pkg/cni/client.go	`0.00% <0.00%> (-75.52%)`	⬇️
...kg/agent/flowexporter/connections/conntrack_ovs.go	`0.00% <0.00%> (-74.51%)`	⬇️
pkg/agent/flowexporter/connections/conntrack.go	`0.00% <0.00%> (-68.43%)`	⬇️
pkg/agent/querier/querier.go	`0.00% <0.00%> (-66.67%)`	⬇️
... and 54 more

codecov-io · 2020-10-31T12:45:09Z

Codecov Report

Merging #1268 (e8c6dda) into master (9d3d10b) will decrease coverage by 4.39%.
The diff coverage is 49.65%.

@@            Coverage Diff             @@
##           master    #1268      +/-   ##
==========================================
- Coverage   63.31%   58.92%   -4.40%     
==========================================
  Files         170      179       +9     
  Lines       14250    15022     +772     
==========================================
- Hits         9023     8852     -171     
- Misses       4292     5199     +907     
- Partials      935      971      +36

Flag	Coverage Δ
e2e-tests	`21.89% <10.95%> (?)`
kind-e2e-tests	`45.76% <45.22%> (-9.63%)`	⬇️
unit-tests	`40.96% <17.02%> (-0.32%)`	⬇️

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

Impacted Files	Coverage Δ
cmd/antrea-agent/agent.go	`0.00% <0.00%> (ø)`
.../agent/flowexporter/connections/conntrack_linux.go	`28.37% <0.00%> (-0.79%)`	⬇️
pkg/agent/stats/collector.go	`92.04% <ø> (-5.69%)`	⬇️
pkg/ovs/openflow/ofctrl_builder.go	`59.48% <0.00%> (-2.69%)`	⬇️
pkg/ovs/openflow/ofctrl_group.go	`48.57% <0.00%> (-4.56%)`	⬇️
pkg/ovs/openflow/ofctrl_action.go	`53.36% <5.71%> (-8.00%)`	⬇️
pkg/agent/agent.go	`48.32% <33.33%> (-0.39%)`	⬇️
pkg/agent/proxy/types/types.go	`60.00% <33.33%> (-24.62%)`	⬇️
pkg/agent/controller/networkpolicy/reconciler.go	`69.39% <50.00%> (-1.93%)`	⬇️
pkg/agent/openflow/pipeline.go	`63.07% <53.96%> (-6.70%)`	⬇️
... and 55 more

jianjuns

This PR LGTM. I know you need to wait for the other one merged first.

jianjuns · 2020-11-03T01:50:55Z

pkg/agent/flowexporter/connections/connections.go

@@ -145,6 +155,39 @@ func (cs *ConnectionStore) addOrUpdateConn(conn *flowexporter.Connection) {
 				}
 			}
 		}
+
+		// Add network policy name and namespace from Connection label


network policy -> NetworkPolicy

jianjuns · 2020-11-03T01:52:11Z

pkg/agent/flowexporter/connections/connections.go

+			ingressOfID := binary.LittleEndian.Uint32(conn.Labels[:4])
+			egressOfID := binary.LittleEndian.Uint32(conn.Labels[4:8])
+			// TODO: There's a chance that the ingressOfID is released and reused by another
+			//  NetworkPolicy rule could be modified in-between by the time the conntrack


Guess you need to rephrase the sentence.

Thanks for pointing this. This is not applicable after PR #1411 is merged, so removed it. I will be changing asyncDeleteInterval to pollInterval of Flow Exporter.
Rephrased the below comment.

jianjuns · 2020-11-03T01:53:06Z

pkg/agent/flowexporter/connections/connections.go

+			//  metrics. We probably need a different solution.
+			if ingressOfID != 0 {
+				policy := cs.ofClient.GetPolicyFromConjunction(ingressOfID)
+				// Same as above, this may be because the NetworkPolicy is removed right after the metrics are fetched.


It is "metrics" or "flow record"?

antrea-bot · 2020-11-06T09:13:30Z

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-all-features-conformance: to trigger conformance tests with all alpha features enabled.
/skip-all-features-conformance: to skip conformance tests with all alpha features enabled.
/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-11-10T18:28:19Z

This PR LGTM. I know you need to wait for the other one merged first.

I updated the patch with modifications after asyncRuleCache PR. Could you please review the patch again?

jianjuns · 2020-11-11T04:49:21Z

pkg/agent/flowexporter/connections/connections.go

@@ -145,6 +156,36 @@ func (cs *ConnectionStore) addOrUpdateConn(conn *flowexporter.Connection) {
 				}
 			}
 		}
+
+		// Add NetworkPolicy name and namespace from Connection label


Nit: namespace -> Namespace
Add "." at EOL.

srikartati · 2020-11-11T05:42:26Z

Thanks for the review. Will wait for IPv6 PR to merge: #1518
And then do the rebase.

antoninbas

couple nits, otherwise LGTM. Is there no plan to test that feature?

antoninbas · 2020-11-11T05:46:51Z

pkg/agent/controller/networkpolicy/allocator.go

+func newIDAllocator(flowPollInterval time.Duration, allocatedIDs ...uint32) *idAllocator {
 	allocator := &idAllocator{
 		availableSet:   make(map[uint32]struct{}),
 		asyncRuleCache: cache.NewStore(asyncRuleCacheKeyFunc),
 		deleteQueue:    workqueue.NewNamedDelayingQueue("async_delete_networkpolicyrule"),
 	}

+	// Set the asyncDeleteInterval based on flowPollInterval value.
+	if asyncDeleteInterval < flowPollInterval {
+		asyncDeleteInterval = flowPollInterval
+	}
+


I feel like flowPollInterval does not really belong in this code. Maybe the parameter should be named asyncDeleteInterval instead and the current asyncDeleteInterval variable should be renamed to minAsyncDeleteInterval

Yes, I was not comfortable too. This is a good suggestion. Changed other parts as well to keep flow poll interval separate.

antoninbas · 2020-11-11T05:49:36Z

pkg/agent/flowexporter/connections/connections.go

@@ -145,6 +156,35 @@ func (cs *ConnectionStore) addOrUpdateConn(conn *flowexporter.Connection) {
 				}
 			}
 		}
+
+		// Add NetworkPolicy Name and Namespace from the connection label.


Suggested change

// Add NetworkPolicy Name and Namespace from the connection label.

// Retrieve NetworkPolicy Name and Namespace by using the ID stored in the connection label (for both ingress and egress directions)

zyiou

Tested exporter with multiple ingress and egress network policy using elk flow collector. LGTM.

srikartati

Added more unit tests and enhanced e2e test to test network policy support in Flow Exporter.

srikartati · 2020-11-11T17:37:44Z

pkg/agent/controller/networkpolicy/allocator.go

+func newIDAllocator(flowPollInterval time.Duration, allocatedIDs ...uint32) *idAllocator {
 	allocator := &idAllocator{
 		availableSet:   make(map[uint32]struct{}),
 		asyncRuleCache: cache.NewStore(asyncRuleCacheKeyFunc),
 		deleteQueue:    workqueue.NewNamedDelayingQueue("async_delete_networkpolicyrule"),
 	}

+	// Set the asyncDeleteInterval based on flowPollInterval value.
+	if asyncDeleteInterval < flowPollInterval {
+		asyncDeleteInterval = flowPollInterval
+	}
+


Yes, I was not comfortable too. This is a good suggestion. Changed other parts as well to keep flow poll interval separate.

antoninbas · 2020-11-11T20:12:18Z

cmd/antrea-agent/agent.go

@@ -145,13 +145,19 @@ func run(o *Options) error {
 	// notifying NetworkPolicyController to reconcile rules related to the
 	// updated Pods.
 	podUpdates := make(chan v1beta2.PodReference, 100)
+	// We set flow poll interval as the time interval for rule deletion in the async
+	// rule cache, which is implemented as the part of idAllocator. This is to preserve


s/as the part of/as part of

antoninbas · 2020-11-11T20:15:02Z

cmd/antrea-agent/agent.go

@@ -145,13 +145,19 @@ func run(o *Options) error {
 	// notifying NetworkPolicyController to reconcile rules related to the
 	// updated Pods.
 	podUpdates := make(chan v1beta2.PodReference, 100)
+	// We set flow poll interval as the time interval for rule deletion in the async
+	// rule cache, which is implemented as the part of idAllocator. This is to preserve
+	// the rule info for mapping in the flow records of Flow Exporter even after


the rule info for populating NetworkPolicy fields in the Flow Exporter even after rule deletion

antoninbas · 2020-11-11T20:16:26Z

pkg/agent/controller/networkpolicy/allocator.go

@@ -29,7 +29,8 @@ import (
 )

 var (
-	asyncDeleteInterval = time.Second * 5
+	minAsyncDeleteInterval = time.Second * 5
+	asyncDeleteInterval    = time.Second * 0


that should really be a parameter of newIDAllocator

antoninbas

LGTM. I think in the future we may want to investigate breaking up TestFlowExporter into several test cases

antoninbas · 2020-11-11T21:38:44Z

test/e2e/flowexporter_test.go

+				t.Fatalf("Records with PodAIP and PodBIP does not have Pod Namespace")
+			}
+			// Check if records have both ingress and egress network policies.
+			if !strings.Contains(record, "octetDeltaCount: 0") {


that's a weird condition, could you add a comment?

antoninbas · 2020-11-11T21:41:48Z

pkg/agent/flowexporter/connections/connections.go

@@ -118,13 +128,6 @@ func (cs *ConnectionStore) addOrUpdateConn(conn *flowexporter.Connection) {
 			conn.DestinationPodName = dIface.ContainerInterfaceConfig.PodName
 			conn.DestinationPodNamespace = dIface.ContainerInterfaceConfig.PodNamespace
 		}
-		// Do not export flow records of connections whose destination is local Pod and source is remote Pod.


so now that we generate 2 "copies", is there any confusion when we consume the flow records in ELK?

Yes, that's possible because of two copies of flows. Need to work with @zyiou to look into that.

I was in two minds whether to enable flow records from the destination node or disable because of flow collector implications. Need some more time. I am ok with continuing the old format of not to export to avoid confusion and take this up in a following PR that can be included in a patch release. What do you think?

If we don't export from the destination Node, then we will never have ingress network policy information unless the source & destination nodes are the same. However, unless I am mistaken this is already the case today for other fields (e.g. destination Pod name)

That wouldn't really be a candidate for a patch release, based on the impact on record consumers and the fact that this (FlowExporter) is an Alpha feature. But we could resolve this for 0.12.

I'll leave it up to you. Let's chat offline about the impact on the consumers of flow records, and especially on our reference ELK stack.

antoninbas · 2020-11-11T21:43:35Z

test/e2e/flowexporter_test.go

@@ -51,6 +52,56 @@ func TestFlowExporter(t *testing.T) {
 	if err != nil {
 		t.Fatalf("Error when getting the perftest server Pod's IP: %v", err)
 	}
+


maybe it's worth adding a comment somewhere, that because both source & destination Pods run on the same Node, we do not receive duplicate records from 2 different Nodes and records include "complete" information (e.g. both ingress & egress policy information)

srikartati

There is an issue to enhance e2e tests Issue #1438. It is a good idea to break this up. Thanks.

srikartati · 2020-11-12T03:53:21Z

pkg/agent/flowexporter/connections/connections.go

@@ -118,13 +128,6 @@ func (cs *ConnectionStore) addOrUpdateConn(conn *flowexporter.Connection) {
 			conn.DestinationPodName = dIface.ContainerInterfaceConfig.PodName
 			conn.DestinationPodNamespace = dIface.ContainerInterfaceConfig.PodNamespace
 		}
-		// Do not export flow records of connections whose destination is local Pod and source is remote Pod.


Yes, that's possible because of two copies of flows. Need to work with @zyiou to look into that.

I was in two minds whether to enable flow records from the destination node or disable because of flow collector implications. Need some more time. I am ok with continuing the old format of not to export to avoid confusion and take this up in a following PR that can be included in a patch release. What do you think?

srikartati · 2020-11-12T03:54:23Z

test/e2e/flowexporter_test.go

+				t.Fatalf("Records with PodAIP and PodBIP does not have Pod Namespace")
+			}
+			// Check if records have both ingress and egress network policies.
+			if !strings.Contains(record, "octetDeltaCount: 0") {


antoninbas

LGTM

srikartati · 2020-11-12T05:02:54Z

/test-all

srikartati · 2020-11-12T06:07:19Z

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

srikartati · 2020-11-12T07:00:02Z

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

srikartati · 2020-11-12T11:56:55Z

/test-conformance

Network policy info (name and namespace), both ingress and egress policies are added. As part of this commit, we support exporting both flow records, for the case of inter-Node flows. One from the source Node, where the flow originates from and other from the destination Node of the flow, where the destination Pod resides. The reason is that they contain different NetworkPolicy info.

srikartati · 2020-11-12T18:19:26Z

/test-all

srikartati · 2020-11-12T19:15:31Z

As there are no IPv6 related changes in this patch. Merging it without testing ipv6 tests.

vmwclabot added the cla-not-required label Sep 18, 2020

srikartati mentioned this pull request Sep 18, 2020

Issue in querying policyCache given ruleID for adding netpol info in IPFIX flow records #1270

Closed

srikartati force-pushed the flowexp_np branch from cafd4cd to dc114c7 Compare September 21, 2020 20:10

srikartati force-pushed the flowexp_np branch from dc114c7 to 92b97e0 Compare October 31, 2020 12:41

srikartati force-pushed the flowexp_np branch from 92b97e0 to 34cc9ca Compare November 2, 2020 18:19

srikartati changed the title ~~[WIP]Add network policy info to IPFIX flow records as part of Flow Exporter~~ Add network policy info to IPFIX flow records as part of Flow Exporter Nov 2, 2020

srikartati added this to the Antrea v0.11.0 release milestone Nov 2, 2020

jianjuns reviewed Nov 3, 2020

View reviewed changes

srikartati force-pushed the flowexp_np branch from 34cc9ca to 2cd0128 Compare November 6, 2020 09:13

zyiou mentioned this pull request Nov 9, 2020

Kibana and docs update for network policy fields in Flow Exporter #1513

Merged

srikartati force-pushed the flowexp_np branch from 2cd0128 to bc0369f Compare November 10, 2020 18:20

srikartati force-pushed the flowexp_np branch 3 times, most recently from 453fc3f to 3198f96 Compare November 11, 2020 04:03

srikartati requested review from zyiou, antoninbas, tnqn and jianjuns November 11, 2020 04:05

jianjuns previously approved these changes Nov 11, 2020

View reviewed changes

srikartati dismissed jianjuns’s stale review via ae923cc November 11, 2020 05:38

srikartati force-pushed the flowexp_np branch from 3198f96 to ae923cc Compare November 11, 2020 05:38

antoninbas reviewed Nov 11, 2020

View reviewed changes

zyiou reviewed Nov 11, 2020

View reviewed changes

srikartati commented Nov 11, 2020

View reviewed changes

srikartati force-pushed the flowexp_np branch 4 times, most recently from b9afb1e to 3e80300 Compare November 11, 2020 20:10

antoninbas reviewed Nov 11, 2020

View reviewed changes

srikartati force-pushed the flowexp_np branch from 3e80300 to 1a7e4a9 Compare November 11, 2020 20:56

srikartati requested a review from antoninbas November 11, 2020 21:29

srikartati force-pushed the flowexp_np branch from 1a7e4a9 to ab70af1 Compare November 11, 2020 21:36

antoninbas reviewed Nov 11, 2020

View reviewed changes

srikartati force-pushed the flowexp_np branch from ab70af1 to aa17922 Compare November 12, 2020 03:45

srikartati commented Nov 12, 2020

View reviewed changes

srikartati force-pushed the flowexp_np branch 2 times, most recently from 11e1cc4 to 360d0b5 Compare November 12, 2020 04:43

antoninbas previously approved these changes Nov 12, 2020

View reviewed changes

srikartati dismissed antoninbas’s stale review via 3b02387 November 12, 2020 05:00

srikartati force-pushed the flowexp_np branch from 360d0b5 to 3b02387 Compare November 12, 2020 05:00

srikartati force-pushed the flowexp_np branch from 3b02387 to e8c6dda Compare November 12, 2020 18:18

antoninbas approved these changes Nov 12, 2020

View reviewed changes

srikartati merged commit 8a3ee6b into antrea-io:master Nov 12, 2020

srikartati deleted the flowexp_np branch November 12, 2020 19:17

dreamtalen mentioned this pull request Dec 17, 2020

[Flow-Aggregator] Add e2e tests for flow aggregator #1628

Merged

	// Add NetworkPolicy Name and Namespace from the connection label.
	// Retrieve NetworkPolicy Name and Namespace by using the ID stored in the connection label (for both ingress and egress directions)

Add network policy info to IPFIX flow records as part of Flow Exporter #1268

Add network policy info to IPFIX flow records as part of Flow Exporter #1268

Conversation

srikartati commented Sep 18, 2020 • edited Loading

antrea-bot commented Sep 18, 2020

codecov-commenter commented Sep 18, 2020 • edited Loading

Codecov Report

codecov-io commented Oct 31, 2020 • edited Loading

Codecov Report

jianjuns 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

antrea-bot commented Nov 6, 2020

srikartati commented Nov 10, 2020 • edited Loading

Choose a reason for hiding this comment

srikartati commented Nov 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

zyiou left a comment

Choose a reason for hiding this comment

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

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

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 Nov 12, 2020 • edited Loading

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

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

srikartati commented Nov 12, 2020

srikartati commented Nov 12, 2020

srikartati commented Nov 12, 2020

srikartati commented Nov 12, 2020

srikartati commented Nov 12, 2020

srikartati commented Nov 12, 2020

srikartati commented Sep 18, 2020 •

edited

Loading

codecov-commenter commented Sep 18, 2020 •

edited

Loading

codecov-io commented Oct 31, 2020 •

edited

Loading

srikartati commented Nov 10, 2020 •

edited

Loading

antoninbas Nov 12, 2020 •

edited

Loading