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

'v6' option is not properly implemented/guaranteed on Linux #95

Closed
nahanil opened this issue Feb 28, 2019 · 2 comments
Closed

'v6' option is not properly implemented/guaranteed on Linux #95

nahanil opened this issue Feb 28, 2019 · 2 comments

Comments

@nahanil
Copy link
Contributor

nahanil commented Feb 28, 2019

Excluding the v6: true option or setting it to false in a "Linux" environment is no guarantee that an IPv4 ping check will occur or work at all.

As kinda mentioned in #79, there are a few different variants of the ping command floating around, some that accept the -4 or -6 flag to explicitly set which IP version to use (as done with many other command line utilities), and other distributions/implementations that require you use the ping6 command.

The way that the code tries to find the ping6 command in lib/builder/factory.js#L78 is not ideal. None of my Debian systems have /sbin/ping6 but it's found at /bin/ping6. So passing v6: true would probably just explode unless the code checks for pingX programs in a few common locations or calling something like which pingX.

I also found /bin/ping4 which seems to be an IPv4 only ping, or equivalent to ping -4 on modern systems which might be able to be used if v6: false.

As a side note, if the v6: option was false meaning "always v4 ping" and true meaning "always v6 ping", how would one specify that either is fine?

A ping response not specifically saying you expect v4 ping can do a v6 check

ping one.one.one.one
PING one.one.one.one(one.one.one.one (2606:4700:4700::1001)) 56 data bytes
64 bytes from one.one.one.one (2606:4700:4700::1001): icmp_seq=1 ttl=61 time=2.00 ms
64 bytes from one.one.one.one (2606:4700:4700::1001): icmp_seq=2 ttl=61 time=1.59 ms
64 bytes from one.one.one.one (2606:4700:4700::1001): icmp_seq=3 ttl=61 time=1.58 ms
--- one.one.one.one ping statistics ---
3 packets transmitted, 3 received, 0% packet loss, time 2003ms
rtt min/avg/max/mdev = 1.589/1.727/2.002/0.194 ms

Explicit v4 check with 'modern' ping command. Should be the same as ping4 one.one.one.one

ping -4 one.one.one.one
PING one.one.one.one (1.1.1.1) 56(84) bytes of data.
64 bytes from one.one.one.one (1.1.1.1): icmp_seq=1 ttl=60 time=1.79 ms
64 bytes from one.one.one.one (1.1.1.1): icmp_seq=2 ttl=60 time=1.43 ms
64 bytes from one.one.one.one (1.1.1.1): icmp_seq=3 ttl=60 time=1.43 ms
--- one.one.one.one ping statistics ---
3 packets transmitted, 3 received, 0% packet loss, time 2000ms
rtt min/avg/max/mdev = 1.432/1.555/1.795/0.169 ms
@mondwan
Copy link
Collaborator

mondwan commented Mar 9, 2019

Umm, as I don't have linux machines at hand, can you try to extend this library in following way?

  • getExecutablePath returns ping or ping6 directly instead of returning the full path of a binary for linux platform
  • Extends ArgumentBuilder such that it has a method for building an object for options in child_process.spawn
    • For linux platform, such option should enable shell
    • For rest of the platforms, it returns an empty object

The idea is we delegating the binary finding part to the shell instead of finding by our own on linux platform. It should work as long as such command is installed properly.

@mondwan
Copy link
Collaborator

mondwan commented Jun 7, 2019

@texh Please check whether the implementation works on your platform or not. I will merge this MR soon if there are no more feedback. Thanks

@mondwan mondwan closed this as completed Dec 21, 2019
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

No branches or pull requests

2 participants