Adding L7 visibility fields #315

tushartathgur · 2023-09-15T21:29:55Z

Adding L7 visibility fields to the Ipfix table.

l7ProtocolName: field is used to store application layer protocol name, field will be empty if application layer flow is not exported.
httpVals: The field is added to contain all the http values and store them as a string of Json similar to labels. This helps us to store multiple http Vals for a single flow which is possible in case of persistent http.

Mistakenly removed #314

codecov · 2023-09-25T17:36:14Z

Codecov Report

Merging #315 (10b82cc) into main (b8729db) will increase coverage by 0.09%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #315      +/-   ##
==========================================
+ Coverage   73.44%   73.53%   +0.09%     
==========================================
  Files          18       18              
  Lines        2790     2792       +2     
==========================================
+ Hits         2049     2053       +4     
+ Misses        574      572       -2     
  Partials      167      167

Flag	Coverage Δ
integration-tests	`51.38% <ø> (ø)`
unit-tests	`72.60% <100.00%> (+0.16%)`	⬆️

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

Files	Coverage Δ
pkg/registry/registry_antrea.go	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

dreamtalen

The L7 fields LGTM.
Could you change the PR and commit message title? It appears to be identical to the Antrea one, which is actually implementing L7 visibility support.

dreamtalen · 2023-09-25T20:19:35Z

pkg/registry/registry_antrea.go

@@ -72,8 +72,10 @@ func loadAntreaRegistry() {
 	registerInfoElement(*entities.NewInfoElement("throughputFromDestinationNode", 148, 4, 56506, 8), 56506)
 	registerInfoElement(*entities.NewInfoElement("reverseThroughputFromSourceNode", 149, 4, 56506, 8), 56506)
 	registerInfoElement(*entities.NewInfoElement("reverseThroughputFromDestinationNode", 150, 4, 56506, 8), 56506)
-	registerInfoElement(*entities.NewInfoElement("flowEndSecondsFromSourceNode", 151, 14, 56506, 4), 56506)
-	registerInfoElement(*entities.NewInfoElement("flowEndSecondsFromDestinationNode", 152, 14, 56506, 4), 56506)
+	registerInfoElement(*entities.NewInfoElement("flowEndSecondsFromSourceNode", 151, 3, 56506, 4), 56506)


Could you help take a look at this @heanlan? I can't remember the reason behind the decision to use uint32 for flowEndSecondsFromSource/DestNode instead of dateTimeSeconds.

We are following the same datatype with flowEndSeconds.

go-ipfix/pkg/registry/registry_IANA.go

Line 148 in ec54b43

registerInfoElement(*entities.NewInfoElement("flowEndSeconds", 151, 14, 0, 4), 0)

Sorry for creating some confusions here. I just noticed there are some discrepancies in both Antrea and Go-ipfix code. FlowEndSecondsFromSourceNode and FlowEndSecondsFromDestinationNode should have the same datatype with FlowEndSeconds. And the datatype is dateTimeSeconds. @tushartathgur could you revert this change and also change the following lines to dateTimeSeconds

go-ipfix/pkg/registry/registry_antrea.csv

Lines 53 to 54 in ec54b43

151,flowEndSecondsFromSourceNode,unsigned32,,current,,,,,,,,56506,

152,flowEndSecondsFromDestinationNode,unsigned32,,current,,,,,,,,56506,

Sure, Creating a separate PR for this as this is not part of L7Visibility.
Though I am confused, the registry_antrea.go should create the elements according to their datatype, however for these two fields, it should not have generated DateTimeSeconds format if the datatype was unsigned32 !

registerInfoElement(*entities.NewInfoElement("flowEndSecondsFromSourceNode", 151, 14, 56506, 4), 56506) registerInfoElement(*entities.NewInfoElement("flowEndSecondsFromDestinationNode", 152, 14, 56506, 4), 56506)

The underlying structure of DateTimeSeconds is uint32: https://www.rfc-editor.org/rfc/rfc7011.html#section-6.1.7
So actually from the code level there is no difference to use uint32 or DateTimeSeconds to my knowledge. But it can be a bit of misleading. And we are sending uint32 data from flow exporter: https://github.com/antrea-io/antrea/blob/4806be3c14e21a75dd3b8e876785fbc947439474/pkg/agent/flowexporter/exporter/exporter.go#L440

I mean the registry_antrea.go is autogenerated, it should not have changed the unsigned32 to dateTimeSeconds

Got your point. Yes, it should be auto-generated but I probably manually edited these two lines before, which causing the discrepancies/errors.

Let's keep these two lines of change, i.e. 14 -> 3, with this PR, as we want to correct the errors. As you mentioned, we can do the uint32 -> dateTimeSeconds changes in a separate PR if we would like to.

antoninbas

I still wonder if the isL7 boolean should be replaced with some "enum" instead. For example: l7FlowType: "http" (an empty of missing IE would mean that we don't have L7 data). I wonder if that could be more future-proof, if we ever want to support other protocols?

What do you think @dreamtalen @heanlan @tushartathgur ?

heanlan · 2023-09-28T19:36:50Z

I still wonder if the isL7 boolean should be replaced with some "enum" instead. For example: l7FlowType: "http" (an empty of missing IE would mean that we don't have L7 data). I wonder if that could be more future-proof, if we ever want to support other protocols?

I agree it's better to make the change now compared to do it later.

dreamtalen · 2023-09-28T19:54:25Z

I still wonder if the isL7 boolean should be replaced with some "enum" instead. For example: l7FlowType: "http" (an empty of missing IE would mean that we don't have L7 data). I wonder if that could be more future-proof, if we ever want to support other protocols?

I agree it's better to make the change now compared to do it later.

Sounds good to me as well.

tushartathgur · 2023-09-28T21:39:22Z

I still wonder if the isL7 boolean should be replaced with some "enum" instead. For example: l7FlowType: "http" (an empty of missing IE would mean that we don't have L7 data). I wonder if that could be more future-proof, if we ever want to support other protocols?

What do you think @dreamtalen @heanlan @tushartathgur ?

Sounds good to me, Adding in the next patch

antoninbas · 2023-09-29T18:08:14Z

pkg/registry/registry_antrea.csv

@@ -54,3 +54,5 @@ ElementID,Name,Abstract Data Type,Data Type Semantics,Status,Description,Units,R
 152,flowEndSecondsFromDestinationNode,unsigned32,,current,,,,,,,,56506,
 153,egressName,string,,current,,,,,,,,56506,
 154,egressIP,string,,current,,,,,,,,56506,
+155,l7FlowType,unsigned8,,current,Supported Actions(uint8 value): L7flowNotPresent(0) L7flowTypeHttp(1),,,,,,,56506,


I suggested l7FlowType as an example, but from a consistency standpoint with standard IPFIX IEs, I feel like l7ProtocolName is a better name

I also feel like using a short string ("http", and maybe later "grpc", "ssh", etc) makes more sense than using an integer value, unless these integer values are standardized somewhere (I doubt it).

I see, I got confused as it was mentioned enum, allow me to change it.

Signed-off-by: Tushar Tathgur <tathgurt@tathgurtPNQHP.vmware.com>

antoninbas

LGTM

heanlan

LGTM

* L7 Visibility support in Antrea Signed-off-by: Tushar Tathgur <tathgurt@tathgurtPNQHP.vmware.com> * Replaced isL7 bool with l7ProtocolName string Signed-off-by: Tushar Tathgur <tathgurt@tathgurtPNQHP.vmware.com> --------- Signed-off-by: Tushar Tathgur <tathgurt@tathgurtPNQHP.vmware.com> Co-authored-by: Tushar Tathgur <tathgurt@tathgurtPNQHP.vmware.com>

vmwclabot added the cla-not-required label Sep 15, 2023

tushartathgur force-pushed the l7Visibility branch 3 times, most recently from b852510 to b8d656a Compare September 16, 2023 00:26

tushartathgur closed this Sep 16, 2023

tushartathgur force-pushed the l7Visibility branch from b8d656a to 49bed1f Compare September 16, 2023 00:34

tushartathgur reopened this Sep 16, 2023

tushartathgur force-pushed the l7Visibility branch 2 times, most recently from 1bd19fc to 718a22f Compare September 16, 2023 00:56

tushartathgur closed this Sep 16, 2023

tushartathgur reopened this Sep 16, 2023

tushartathgur mentioned this pull request Sep 19, 2023

L7 Network Flow Export support in Antrea antrea-io/antrea#5218

Merged

dreamtalen reviewed Sep 25, 2023

View reviewed changes

tushartathgur force-pushed the l7Visibility branch from 8dcd5b4 to fb77988 Compare September 25, 2023 21:47

tushartathgur changed the title ~~L7 Visibility support in Antrea~~ Adding L7 visibility fields Sep 25, 2023

tushartathgur requested a review from antoninbas September 25, 2023 21:48

elton-furtado approved these changes Sep 28, 2023

View reviewed changes

antoninbas reviewed Sep 28, 2023

View reviewed changes

tushartathgur force-pushed the l7Visibility branch from 009ac28 to dff8e31 Compare September 28, 2023 21:44

tushartathgur closed this Sep 28, 2023

tushartathgur force-pushed the l7Visibility branch from dff8e31 to ec54b43 Compare September 28, 2023 21:45

tushartathgur reopened this Sep 28, 2023

tushartathgur closed this Sep 28, 2023

tushartathgur force-pushed the l7Visibility branch from dff8e31 to ec54b43 Compare September 28, 2023 21:50

tushartathgur reopened this Sep 28, 2023

antoninbas reviewed Sep 29, 2023

View reviewed changes

L7 Visibility support in Antrea

c8b02c8

Signed-off-by: Tushar Tathgur <tathgurt@tathgurtPNQHP.vmware.com>

tushartathgur force-pushed the l7Visibility branch from dff8e31 to f3ea06d Compare September 29, 2023 19:12

Replaced isL7 bool with l7ProtocolName string

10b82cc

Signed-off-by: Tushar Tathgur <tathgurt@tathgurtPNQHP.vmware.com>

tushartathgur force-pushed the l7Visibility branch from f3ea06d to 10b82cc Compare September 29, 2023 19:18

antoninbas approved these changes Sep 29, 2023

View reviewed changes

antoninbas requested review from dreamtalen and heanlan September 29, 2023 19:25

heanlan approved these changes Sep 29, 2023

View reviewed changes

tushartathgur merged commit a7df05f into vmware:main Sep 29, 2023
9 checks passed

tushartathgur mentioned this pull request Oct 2, 2023

Network Topology Dashboard Update antrea-io/theia#488

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding L7 visibility fields #315

Adding L7 visibility fields #315

tushartathgur commented Sep 15, 2023 •

edited

Loading

codecov bot commented Sep 25, 2023 •

edited

Loading

dreamtalen left a comment

dreamtalen Sep 25, 2023

heanlan Sep 25, 2023 •

edited

Loading

heanlan Sep 28, 2023 •

edited

Loading

tushartathgur Sep 28, 2023

heanlan Sep 28, 2023 •

edited

Loading

tushartathgur Sep 28, 2023

heanlan Sep 28, 2023 •

edited

Loading

heanlan Sep 28, 2023

antoninbas left a comment

heanlan commented Sep 28, 2023

dreamtalen commented Sep 28, 2023

tushartathgur commented Sep 28, 2023 •

edited

Loading

antoninbas Sep 29, 2023

tushartathgur Sep 29, 2023

antoninbas left a comment

heanlan left a comment

	151,flowEndSecondsFromSourceNode,unsigned32,,current,,,,,,,,56506,
	152,flowEndSecondsFromDestinationNode,unsigned32,,current,,,,,,,,56506,

Adding L7 visibility fields #315

Adding L7 visibility fields #315

Conversation

tushartathgur commented Sep 15, 2023 • edited Loading

codecov bot commented Sep 25, 2023 • edited Loading

Codecov Report

dreamtalen left a comment

Choose a reason for hiding this comment

dreamtalen Sep 25, 2023

Choose a reason for hiding this comment

heanlan Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

heanlan Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

tushartathgur Sep 28, 2023

Choose a reason for hiding this comment

heanlan Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

tushartathgur Sep 28, 2023

Choose a reason for hiding this comment

heanlan Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

heanlan Sep 28, 2023

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

heanlan commented Sep 28, 2023

dreamtalen commented Sep 28, 2023

tushartathgur commented Sep 28, 2023 • edited Loading

antoninbas Sep 29, 2023

Choose a reason for hiding this comment

tushartathgur Sep 29, 2023

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

heanlan left a comment

Choose a reason for hiding this comment

tushartathgur commented Sep 15, 2023 •

edited

Loading

codecov bot commented Sep 25, 2023 •

edited

Loading

heanlan Sep 25, 2023 •

edited

Loading

heanlan Sep 28, 2023 •

edited

Loading

heanlan Sep 28, 2023 •

edited

Loading

heanlan Sep 28, 2023 •

edited

Loading

tushartathgur commented Sep 28, 2023 •

edited

Loading