-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding L7 visibility fields #315
Conversation
b852510
to
b8d656a
Compare
b8d656a
to
49bed1f
Compare
1bd19fc
to
718a22f
Compare
Codecov Report
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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
pkg/registry/registry_antrea.go
Outdated
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean the registry_antrea.go is autogenerated, it should not have changed the unsigned32 to dateTimeSeconds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got your point. Yes, it should be auto-generated but I probably manually edited these two lines before, which causing the discrepancies/errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
8dcd5b4
to
fb77988
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
I agree it's better to make the change now compared to do it later. |
Sounds good to me as well. |
Sounds good to me, Adding in the next patch |
009ac28
to
dff8e31
Compare
dff8e31
to
ec54b43
Compare
dff8e31
to
ec54b43
Compare
pkg/registry/registry_antrea.csv
Outdated
@@ -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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, I got confused as it was mentioned enum, allow me to change it.
Signed-off-by: Tushar Tathgur <tathgurt@tathgurtPNQHP.vmware.com>
dff8e31
to
f3ea06d
Compare
Signed-off-by: Tushar Tathgur <tathgurt@tathgurtPNQHP.vmware.com>
f3ea06d
to
10b82cc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
* 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>
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