ensure AddressResolver supports localhost even if ipv6 is disabled in sysctl but not /etc/hosts

[rmannibucau]

if ::1 is mapped to localhost in /etc/hosts then java stack and dns resolver of polaris can pick it up (0:0:...:1) but if sysctl disabled ipv6 then it will still be able to picj the ipv6 using /etc/hosts - can depend network manager but on ubuntu it does by default.

this is bothering cause LOCAL_ADDRESS set was wrong but still everything was functional

just fixed it for localhost since it hits in tests but can be a more general issue even if less likely to be used for other cases/outside tests since address would be explicitly passed IMHO

Checklist

• 🛡️ Don't disclose security issues! (contact [email protected])
• 🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #
• 🧪 Added/updated tests with good coverage, or manually tested (and explained how - not really possible without playing with OS tools, likely undesired)
• 💡 Added comments for complex logic
• 🧾 Updated CHANGELOG.md (if needed - not needed)
• 📚 Updated documentation in site/content/in-dev/unreleased (if needed - not needed)

[snazy]

Thanks for this!
Generally, the change LGTM, but I think the Stream.concat can be moved outside of the networkInterfaces().flatMap() to concatenate the network interfaces addresses and the local one.

[jbonofre]

It looks good to me.

[rmannibucau]

@snazy so we add localhost even if it is not an active network interface? (can be weird but it can be dropped/put down if desired)?

[snazy]

@snazy so we add localhost even if it is not an active network interface? (can be weird but it can be dropped/put down if desired)?

Ah, sorry, we're good here. I missed the if...localhost.

My comment's about that findLocalhostAddresses() is called for every network interface, not only once.

Hm, wonder if we need to check for NetworkInterface.isLoopback(). (I recall checking for such interfaces isn't that straightforward/unambiguous on all OS's.) That's probably (I'm jet-lagged) where my first comment came from.
I suspect you're running on Linux?
However, if the change's fine there, we could also just go ahead and fix-forward if other changes are neede.

[rmannibucau]

@snazy yep linux (debian derivative but doesnt change the world ;)).
On testing isLoopback it can be more vicious if we start to dig, personally i'm not sure why it takes all local interfaces, seems the opposite of what we do want (if I have multiple interfaces i dont want to enable them all for all apps in general) plus you need to handle links as well (a bit like tomcat does even if not 1-1 with polaris need: https://github.com/apache/tomcat/blob/cbaa713f3fa82667755cd67fa626a5b7bf07be93/java/org/apache/tomcat/util/net/AbstractEndpoint.java#L1504).
So I think we can include this change (reworked or not) and then potentially rework totally this logic.
Only usage (except test) is https://github.com/apache/polaris/blob/main/persistence/nosql/persistence/cdi/quarkus-distcache/src/main/java/org/apache/polaris/persistence/nosql/quarkus/distcache/CacheInvalidationSender.java#L145 and this makes it impossible to run two instances locally (which is a valid use case IMHO).

Hope it makes sense, starts to be late there :D

[snazy]

On testing isLoopback it can be more vicious if we start to dig, personally i'm not sure why it takes all local interfaces

Yea, the amount of interfaces and the reported-capabilities complexity increased quite a bit in the last years ;)

[snazy]

It's a one-off thing run during initialization, so that's fine IMO and the PR's good to go.
If needed, we can improve it later.