Skip to content

Conversation

jpinsonneau
Copy link
Contributor

This PR skip dedupe on DNS flows.

To avoid having the request and response showing the same Direction when service is involved, we should fix the related case:
netobserv/flowlogs-pipeline#483 (comment)

@openshift-ci
Copy link

openshift-ci bot commented Sep 4, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from jpinsonneau. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jotak
Copy link
Member

jotak commented Sep 5, 2023

it doesn't looks good, as we would end up with multiple duplicates showing "Duplicate=false"

fwd := make([]*Record, 0, len(records))
for _, record := range records {
if cache.isDupe((*ebpf.BpfFlowId)(&record.Id)) {
if record.Metrics.DnsRecord.Id == 0 && cache.isDupe((*ebpf.BpfFlowId)(&record.Id)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an option we mentioned with @msherif1234 was also adding some logic:
Set it as duplicate IF:
(dnsId unset AND cache.isDupe)
OR
(ndsId unset AND DNS-tracker-feature enabled AND Source-port is dns port)

This assumes every response from a DNS port is duplicated anyway by the dns tracker so we can mark all of them as duplicates, except the one holding the DNS info.

That wouldn't be a perfect solution, it has some risks; e.g. if for any reason the DNS tracker doesn't work then all DNS flows would be marked duplicates. I think we should still try to find better .... but that could be "good enough" if we don't find better

@jpinsonneau
Copy link
Contributor Author

Closing this in favor of alternative PR

@jpinsonneau jpinsonneau closed this Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants