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

Feat/traceroute #113

Merged
merged 21 commits into from
Mar 1, 2024
Merged

Feat/traceroute #113

merged 21 commits into from
Mar 1, 2024

Conversation

niklastreml
Copy link
Contributor

@niklastreml niklastreml commented Feb 21, 2024

Motivation

This pr implements a basic traceroute check.

Relates to #111, #112

Currently its using "github.com/aeden/traceroute" to implement the actual tracerouting logic, but I intend to to change this in the near future (more on this in #112).

In case you're testing this locally

The check is extremely slow, since it creates a lot of network requests. I tried to improve by concurrently running the check for every target, but the internals of the library do everything synchronously so there isn't much we can actually improve there outside of #112. While developing I used the below config, with retries set to 0, and it took a minute or so per run of the check.

Config

traceroute:
  interval: 5s
  timeout: 3s
  retries: 3
  maxHops: 8
  targets:
    - addr: 8.8.8.8

Debugging

Since this check requires extra permissions to open the required sockets, debugging is a bit complicated. I have added a script in scripts/debug-elevated.sh which compiles sparrow as an unoptimized binary and launches it with a headless debugger and elevated permissions. You can then use the Connect to server target in the .vscode/launch.json to connect to the debugger and debug as usual. This is only necessary, when you actually need to debug the traceroute check, you can debug the others as usual.

Changes

For additional information look at the commits.

Tests done

TODO

  • I've assigned this PR to myself
  • I've labeled this PR correctly

@niklastreml niklastreml added feature Introduces a new feature area/checks Issues/PRs related to Checks labels Feb 21, 2024
@niklastreml niklastreml self-assigned this Feb 21, 2024
Copy link
Member

@lvlcn-t lvlcn-t left a comment

Choose a reason for hiding this comment

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

First review

pkg/checks/runtime/config.go Show resolved Hide resolved
pkg/checks/traceroute/traceroute.go Outdated Show resolved Hide resolved
pkg/checks/traceroute/traceroute.go Outdated Show resolved Hide resolved
pkg/checks/traceroute/traceroute.go Outdated Show resolved Hide resolved
pkg/checks/traceroute/traceroute.go Outdated Show resolved Hide resolved
pkg/checks/traceroute/traceroute.go Outdated Show resolved Hide resolved
pkg/checks/traceroute/traceroute.go Show resolved Hide resolved
pkg/checks/traceroute/traceroute.go Outdated Show resolved Hide resolved
pkg/checks/traceroute/traceroute_test.go Outdated Show resolved Hide resolved
pkg/checks/traceroute/traceroute.go Outdated Show resolved Hide resolved
@lvlcn-t lvlcn-t linked an issue Feb 21, 2024 that may be closed by this pull request
1 task
Copy link
Member

@puffitos puffitos left a comment

Choose a reason for hiding this comment

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

Some remarks:

  • please document at least the exposed functions
  • Please use a consistent name for the receiver of the Traceroute functions (d and c are used, tr maybe be a good candidate)

pkg/checks/traceroute/traceroute.go Outdated Show resolved Hide resolved
pkg/checks/traceroute/traceroute.go Outdated Show resolved Hide resolved
pkg/checks/traceroute/traceroute.go Outdated Show resolved Hide resolved
pkg/checks/traceroute/traceroute.go Outdated Show resolved Hide resolved
pkg/checks/traceroute/config.go Show resolved Hide resolved
pkg/checks/traceroute/config.go Outdated Show resolved Hide resolved
pkg/checks/traceroute/traceroute.go Outdated Show resolved Hide resolved
pkg/checks/traceroute/traceroute.go Outdated Show resolved Hide resolved
pkg/checks/traceroute/config.go Show resolved Hide resolved
pkg/checks/traceroute/traceroute.go Show resolved Hide resolved
pkg/checks/traceroute/traceroute_test.go Outdated Show resolved Hide resolved
@lvlcn-t
Copy link
Member

lvlcn-t commented Feb 28, 2024

@niklastreml Please also document the check in a new section of the README.

Copy link
Member

@lvlcn-t lvlcn-t left a comment

Choose a reason for hiding this comment

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

Only some minor things, I think after that it'll LGTM.

README.md Show resolved Hide resolved
pkg/checks/traceroute/traceroute.go Outdated Show resolved Hide resolved
pkg/checks/traceroute/traceroute.go Outdated Show resolved Hide resolved
Copy link
Member

@lvlcn-t lvlcn-t left a comment

Choose a reason for hiding this comment

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

LGTM

@niklastreml niklastreml merged commit ec1fc82 into main Mar 1, 2024
11 checks passed
@lvlcn-t lvlcn-t deleted the feat/traceroute branch March 1, 2024 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/checks Issues/PRs related to Checks feature Introduces a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Implement Traceroute Check
3 participants