Skip to content
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

packetbeat/sniffer: allow multiple interface devices to be followed #32933

Merged
merged 1 commit into from
Sep 20, 2022

Conversation

efd6
Copy link
Contributor

@efd6 efd6 commented Aug 31, 2022

What does this PR do?

This runs multiple sniffers for each Sniffer, each handling default route
polling as needed independently. All sniffers terminate if any
individual terminates.

Why is it important?

This allows multiple network devices to be followed.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

@efd6 efd6 self-assigned this Aug 31, 2022
@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Aug 31, 2022
@elasticmachine
Copy link
Collaborator

elasticmachine commented Aug 31, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-09-20T01:27:43.050+0000

  • Duration: 50 min 34 sec

Test stats 🧪

Test Results
Failed 0
Passed 1709
Skipped 19
Total 1728

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@mergify
Copy link
Contributor

mergify bot commented Sep 13, 2022

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b network_traffic_multiple_sniffers upstream/network_traffic_multiple_sniffers
git merge upstream/main
git push upstream network_traffic_multiple_sniffers

@efd6 efd6 force-pushed the network_traffic_multiple_sniffers branch from b38350d to ccf510d Compare September 16, 2022 06:23
@efd6 efd6 marked this pull request as ready for review September 16, 2022 07:25
@efd6 efd6 requested a review from a team as a code owner September 16, 2022 07:25
@elasticmachine
Copy link
Collaborator

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

@efd6 efd6 force-pushed the network_traffic_multiple_sniffers branch 2 times, most recently from e5824c4 to 4d7dab6 Compare September 16, 2022 07:37
Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

LGTM (I reviewed the last commit 4d7dab6e8213a68ffc507dfe9c66b4b568b5ef48. I left other comments relating to the parent commits in #32732).

}
if defaultRoute == nil {
return s.sniffStatic(s.device)
g, ctx := errgroup.WithContext(context.Background())
Copy link
Member

Choose a reason for hiding this comment

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

With the sniffer also utilizing the done channel as a shutdown signal, I would consider a future refactoring to combine the shutdown signalling into a single context.Context. Like having a ctx that comes down from the beater package that gets cancelled on shutdown and then pass that into this errgroup as its parent.

@efd6
Copy link
Contributor Author

efd6 commented Sep 19, 2022

If this is OK to merge, please also approve the parent PR; I'd like to keep these separate in the history.

@mergify
Copy link
Contributor

mergify bot commented Sep 20, 2022

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b network_traffic_multiple_sniffers upstream/network_traffic_multiple_sniffers
git merge upstream/main
git push upstream network_traffic_multiple_sniffers

This runs multiple sniffers for each Sniffer, each handling default route
polling as needed independently. All sniffers terminate if any individual
terminates.
@efd6 efd6 force-pushed the network_traffic_multiple_sniffers branch from 4d7dab6 to dda0ba3 Compare September 20, 2022 01:27
@efd6 efd6 merged commit 0f272d9 into elastic:main Sep 20, 2022
chrisberkhout pushed a commit that referenced this pull request Jun 1, 2023
…32933)

This runs multiple sniffers for each Sniffer, each handling default route
polling as needed independently. All sniffers terminate if any individual
terminates.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.5 candidate backport-skip Skip notification from the automated backport with mergify enhancement Packetbeat
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants