-
Notifications
You must be signed in to change notification settings - Fork 23
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
NETOBSERV-1233: Remove L3 fields for ARP ethtype packets and update unit-test #471
NETOBSERV-1233: Remove L3 fields for ARP ethtype packets and update unit-test #471
Conversation
msherif1234
commented
Aug 10, 2023
•
edited
Loading
edited
- remove l3 fields fom FLP storgae for layer2 EthTypes protocol like ARP
- extend unit-test to test various protocols and L2 protocols
- update conntrack to skip l2 flows
Signed-off-by: Mohamed Mahmoud <mmahmoud@redhat.com>
Signed-off-by: Mohamed Mahmoud <mmahmoud@redhat.com>
038696f
to
e0eef85
Compare
Codecov Report
@@ Coverage Diff @@
## main #471 +/- ##
==========================================
- Coverage 66.08% 66.03% -0.06%
==========================================
Files 94 94
Lines 6877 6921 +44
==========================================
+ Hits 4545 4570 +25
- Misses 2071 2092 +21
+ Partials 261 259 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
/ok-to-test |
New image: It will expire after two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=4e8b12e make set-flp-image |
Signed-off-by: Mohamed Mahmoud <mmahmoud@redhat.com>
/ok-to-test |
New image: It will expire after two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=dcf47f6 make set-flp-image |
@@ -61,6 +61,11 @@ func (ct *conntrackImpl) Extract(flowLogs []config.GenericMap) []config.GenericM | |||
|
|||
var outputRecords []config.GenericMap | |||
for _, fl := range flowLogs { | |||
if !fl.IsValidProtocol() { | |||
log.Debugf("skipping layer2 protocols flow log %v", fl) | |||
ct.metrics.inputRecords.WithLabelValues("discarded").Inc() |
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.
ct.metrics.inputRecords.WithLabelValues("discarded").Inc() | |
ct.metrics.inputRecords.WithLabelValues("rejected").Inc() |
Any reason not using the same metric as below ? Else maybe we should make the difference clear for the user
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 felt rejected because of any error but in this case there is no error just l2 protocol that we aren't supporting but I am up for better suggestion ?
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.
What about a note in the README explaining the diffs then ?
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.
Hi @jpinsonneau I checked readme it doesn't really explain any of the metrics ? is there another place where this is already doc ?
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.
nope @msherif1234 it's something we should add around the line I mentionned I guess
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 have docs about the operational metrics in
https://github.com/netobserv/flowlogs-pipeline/blob/5912edfd278091cd07c693baf27bfbdc8f8408dd/docs/operational-metrics.md
but it is auto-generated and doesn't include the details about label values.
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.
Small comment, otherwise looks good ! thanks @msherif1234 !
/ok-to-test |
New image: It will expire after two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=9a62d90 make set-flp-image |
@msherif1234: This pull request references NETOBSERV-1233 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
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 thanks @msherif1234
New image: It will expire after two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=c26bbca make set-flp-image |
206b646
to
9983b83
Compare
9983b83
to
eacc980
Compare
pkg/config/generic_map.go
Outdated
return false | ||
} | ||
|
||
func (m GenericMap) FilterFLowLog() bool { |
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.
func (m GenericMap) FilterFLowLog() bool { | |
func (m GenericMap) FilterFlowLog() bool { |
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 think FilterFlowLog()
should be a method conntrackImpl
and get a flowLog as an argument. Sorry for not making it clear in my previous comment.
const duplicateFieldName = "Duplicate" | ||
const ( | ||
duplicateFieldName = "Duplicate" | ||
protoFieldName = "Proto" |
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.
My point is:
What if we get flow logs from a source that gives the protocol field a name different from Proto
, for example Protocol
?
eacc980
to
4f0be98
Compare
/ok-to-test |
@Amoghrd can u pls check out this fix and add the required tags if changes are ok ? |
New image: It will expire after two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=6236dec make set-flp-image |
/label qe-approved |
Signed-off-by: Mohamed Mahmoud <mmahmoud@redhat.com>
Signed-off-by: Mohamed Mahmoud <mmahmoud@redhat.com>
4f0be98
to
fa5ecbf
Compare
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: msherif1234 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |