Skip to content

Add additional external connectivity Checks to kconmon#2

Open
MaxRink wants to merge 9 commits intoStono:masterfrom
MaxRink:master
Open

Add additional external connectivity Checks to kconmon#2
MaxRink wants to merge 9 commits intoStono:masterfrom
MaxRink:master

Conversation

@MaxRink
Copy link

@MaxRink MaxRink commented Jul 9, 2020

We have the Use-Case of also monitoring some of our external connections as some of our Networks are a bit flaky sometimes and we want be able to detect that.
This PR impliments external ICMP and HTTP connection checks to for example monitor your rough network latency (routing changes e.g.) and status of connection to for example your registry,

@Stono
Copy link
Owner

Stono commented Jul 14, 2020

Hey,
Sorry for the delay in reviewing this, had a busy few days. Firstly, thanks a lot for the contribution, I think the custom http checks is a great addition.

I'm on the fence about the ping implementation however.

The module you're importing, ping simply shells out to the os ping, rather than natively crafting an ICMP packets such as node-net-ping. I'm against this for a couple of reasons:

  1. We bring into play a dependency on the native OS implementation of ping (in this case provided by iputils), as the module is simply parsing stdout. That feels very flakey and something that could bite us later.
  2. I'm concerned about the cpu and memory overhead of frequently shelling out, it's quite an expensive operation when compared to crafting an icmp packet natively.

@MaxRink
Copy link
Author

MaxRink commented Jul 20, 2020

Ok, ill see if i can restructure it

@MaxRink
Copy link
Author

MaxRink commented Jul 29, 2020

Hi @Stono
we've did some reworking but are now running into the issue that crafting raw ICMP packets requires root rights.

{"timestamp":"2020-07-21T14:25:21.121Z","level":"error","module":"tester","message":"icmp test for 8.8.8.8 failed","error":{"name":"Error","message":"Operation not permitted","stack":"at new Socket (/app/node_modules/raw-socket/index.js:47:14)"}}
{"timestamp":"2020-07-21T14:25:21.121Z","level":"error","module":"tester","message":"icmp test for 8.8.4.4 failed","error":{"name":"Error","message":"Operation not permitted","stack":"at new Socket (/app/node_modules/raw-socket/index.js:47:14)"}}

Imo it would be a bad idea to run tasks as root, so im not sure how to proceed here. Tbh, im propably just lacking unterstanding for Node.js and how to accomplish this root-less with node.

@Stono
Copy link
Owner

Stono commented Jul 29, 2020

Interesting, if you share your branch with me I can try and take a look tomorrow.

You might not need root, just a certain netcap

@MaxRink
Copy link
Author

MaxRink commented Jul 29, 2020

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