Skip to content
This repository has been archived by the owner on Jun 20, 2024. It is now read-only.

NPC: avoid logging dropped packets that were not actually dropped #3852

Merged
merged 1 commit into from
Aug 27, 2020

Conversation

bboreham
Copy link
Contributor

@bboreham bboreham commented Aug 26, 2020

Move egress logging rule beside drop rule, otherwise we might log a blocked connection even though the packet was not dropped.

Noticed while investigating #3829

Otherwise we might log a blocked connection even though the packet
was not dropped.
@bboreham bboreham changed the base branch from master to 2.7 August 27, 2020 10:33
@bboreham bboreham added this to the 2.7.1 milestone Aug 27, 2020
Copy link

@saada saada left a comment

Choose a reason for hiding this comment

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

Well done! 👏
Had to go up to see what defaultEgressDrop was intended for... So we're just ensuring setting flag to true after drop rules are actually completed.

@bboreham
Copy link
Contributor Author

Yes, instead of setting it true before attempting then setting it back to false on error, we just wait till both commands have succeeded before setting it true.

The whole function has a lock on npc so we can't get another thread trying to start the operation at the same time.

@bboreham bboreham merged commit fb7fc7d into 2.7 Aug 27, 2020
@bboreham bboreham deleted the spurious-egress-drop-logs branch August 27, 2020 15:12
@bboreham bboreham changed the title Move egress logging rule beside drop rule Avoid logging dropped packets that were not actually dropped Jan 18, 2021
@bboreham bboreham changed the title Avoid logging dropped packets that were not actually dropped NPC: avoid logging dropped packets that were not actually dropped Jan 18, 2021
@bboreham bboreham modified the milestones: 2.7.1, 2.8.0 Jan 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants