Add IP version selection

[lucasnz]

⚠️⚠️⚠️ Since we do not accept all types of pull requests and do not want to waste your time. Please be sure that you have read pull request rules:
https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#can-i-create-a-pull-request-for-uptime-kuma

Tick the checkbox if you understand [x]:

• I have read and understand the pull request rules.

Description

This PR introduces the ability to force a monitor to use IPv4 or IPv6. The existing behavior is left as is if automatic is set (which is the default in the PR). I.e. end-users would need to specifically select IPv4 or IPv6.

This addresses (at least in part) the following issues:
Resolves #3542
Resolves #1242
Resolves #1025

The option is currently only available for the following monitors: ping, tcp, dns.

I looked into adding it for http monitors, but it is not possible without modifying axios, see:

• The axios always try ipv6 first when send request to http://localhost:8800，even if I set family to 4 axios/axios#5785
• How can I set axios to prefer IPv6? axios/axios#590

Type of change

Please delete any options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• User interface (UI)
• New feature (non-breaking change which adds functionality)

Checklist

• My code follows the style guidelines of this project
• I ran ESLint and other linters for modified files
• I have performed a self-review of my own code and tested it
• I have commented my code, particularly in hard-to-understand areas
  (including JSDoc for methods)
• My changes generate no new warnings
• My code needed automated testing. I have added them (this is optional task)

Screenshots (if any)

[lucasnz]

Could you add Resolves #ISSUE-NUMBER to the issues this PR resolves?

As I described in the description, this PR supports ping, tcp, and dns. Which means it resolves the problem for these monitors. Notably, I can't fix http which requires a change to axios. Therefore while things are improved for these issues, they are not resolved if you are using an http monitor.

[lucasnz]

#3542 and #1025 can be considered fixed since they specifically mention the Ping and TCP Port monitor types.

Yes, I agree it Resolves #1025

Without digging into #3542 it's hard to know exactly what their issue is. This PR will add the option they're asking for to force IPv4. Do we consider that resolved? Even if their issue is something else. They have not given detail on their monitor, so it's unclear what is generating the error.

[louislam]

Just have a quick look of code changes, haven't checkout yet.

• We don't provide new features for 1.23.X. You have to rebase to master branch.

• Have to use knex migration now since we will support MariaDB in upcoming version: https://github.com/louislam/uptime-kuma/tree/master/db/knex_migrations

• I saw lines like this this.hostname = address. Is that the original hostname is being overwritten by the resolved IP address? If yes, this approach is not ok.

• For DNS Monitor, this option could be ambiguous, since users could wrongly think it is A record vs AAAA record.

[lucasnz]

Thanks for the comments. I'm happy to make adjustments. I have a few questions.

• We don't provide new features for 1.23.X. You have to rebase to master branch.

Furthermore, I have merged with master already, but happy to rebase. Do you want me to rebase to master, or is the merge OK?

• Have to use knex migration now since we will support MariaDB in upcoming version: https://github.com/louislam/uptime-kuma/tree/master/db/knex_migrations

I'll need to look into what this means.

• I saw lines like this this.hostname = address. Is that the original hostname is being overwritten by the resolved IP address? If yes, this approach is not ok.

I need to understand a bit more to resolve this. Would it be acceptable, to define a new variable in monitor.js, and then pass this new variable into the various monitors in place of the hostname? Or is there something else I have to consider?

• For DNS Monitor, this option could be ambiguous, since users could wrongly think it is A record vs AAAA record.

This is mainly a UI comment.
Putting the option far down under advanced settings I think addresses this, but I'm open to ideas.
Do we need a brief description in the UI?

[chakflying]

Maybe we should rename the variable to ipByHostname or similar to prevent confusion, since it's definitely not a hostname anymore.

[lucasnz]

Maybe we should rename the variable to ipByHostname or similar to prevent confusion, since it's definitely not a hostname anymore.

Happy to change the variable name. It stays as hostname to retain the existing behaviour if IPv4 or IPv6 is not selected. Would HostnameOrIp be more appropriate?

[chakflying]

Ah I see how it works now, I'm okay with this.

I'm also slightly concerned that you're setting the value to null on error. Would continuing to execute the monitor with the null value give weird results? Should we revert to the original hostname, or throw an error to mark the heartbeat as DOWN?

[lucasnz]

Ah I see how it works now, I'm okay with this.

I'm also slightly concerned that you're setting the value to null on error. Would continuing to execute the monitor with the null value give weird results? Should we revert to the original hostname, or throw an error to mark the heartbeat as DOWN?

This is intended to generate an error in the monitor. We don't want to retain the original value, otherwise the monitor might succeed (e.g. it might be set to IPv6 and have no AAAA record, and then pass with IPv4 - this would have the IPv6 monitor showing up). It might be better to throw an error and mark the monitor down straight away. I'll look into that...

[lucasnz]

@chakflying updated to throw instead of setting to null. This is definitely an improvement. Thanks.

[lucasnz]

Any more comments or are we good to merge?

[peterablehmann]

Can we get this PR merged?