feat: add is_self_host function

[DoobleD]

Used in https://github.com/haraka/Haraka/pull/3166.

[msimerson]

Considering that is_local_host is used in exactly one spot in Haraka outbound, wouldn't it be simpler and cleaner to just add a call to get_public_ip in is_local_host and append any results to the ips array?

[DoobleD]

Considering that is_local_host is used in exactly one spot in Haraka outbound, wouldn't it be simpler and cleaner to just add a call to get_public_ip in is_local_host and append any results to the ips array?

That would work as well, assuming we additionaly use ip_in_list in is_local_host. To me "local" doesn't suggest checking (self) public ip so separating in 2 functions is clearer IMO, but if you prefer 1 function that's fine with me.

If we keep 1 function I'd say at least call it is_self_host instead of is_local_host.

[msimerson]

To me "local" doesn't suggest checking (self) public ip

I think its too late to litigate the past naming choices. That's the function that's doing the job and changing its name is a breaking API change. The least invasive change is updating the function to also include the results of get_public_ip.

[DoobleD]

To me "local" doesn't suggest checking (self) public ip

I think its too late to litigate the past naming choices. That's the function that's doing the job and changing its name is a breaking API change. The least invasive change is updating the function to also include the results of get_public_ip.

I pushed the change, though I'm not sure that's what you were thinking to?

[msimerson]

Much better. But I find complex ternaries harder to reason about. How about simplifying the changes to something like this?

diff --git a/index.js b/index.js
index 3f6a40a..d95d179 100644
--- a/index.js
+++ b/index.js
@@ -141,7 +141,12 @@ exports.on_local_interface = function (ip) {

 exports.is_local_host = async function (host) {

-  if (net.isIP(host)) return this.is_local_ip(host);
+  if (net.isIP(host)) {
+    const public_ip = await this.get_public_ip()
+    if (public_ip && public_ip === host) return true
+
+    return this.is_local_ip(host);
+  }

   try {
     const ips = await this.get_ips_by_host(host)

[DoobleD]

Thank you for the feedback @msimerson. I don't particularly agree that these code changes are an improvement, but I'm not against it either of course, so it's done. :)

[msimerson]

I don't particularly agree that these code changes are an improvement

Heh heh, we agree. I made a couple more mods which I think is an improvement. Please have a look and provide feedback. Also, the tests were failing in ubuntu node v20 and is_local_host('self hostname') was failing on all versions of Windows. I just disabled that check if the platform is windows. I can't tell if it's broken in Windows in general, or just broken on GitHub Windows test runners.

[msimerson]

Please have a look at #79. I've included your tests and I think all your code changes, just formatted and arranged differently. You had the right idea with improved variable names and I took it a little further, to make the code more self documenting. Let me know what you think.

[DoobleD]

Thank you for diving into this @msimerson, much appreciated. https://github.com/haraka/haraka-net-utils/pull/79 looks good to me, it should indeed do the same thing and the dst_host/dest_ips naming is more explicit.

[DoobleD]

Closing in favor of https://github.com/haraka/haraka-net-utils/pull/79.