WIP: Add support to fallback to all results of getaddrinfo() (ipv6/ ipv4 dual-stack, DNS-RR)

[chr4]

This pull request introduces support to store hostnames in the sentinelAddr struct instead of resolved IP adresses.

This fixes the issue of:

1. Changing IP adresses (when a DNS hostname is given in the configuration file)
2. Dual-Stack setups where one ip version might be not available (e.g. an AAAA record is set, but the connection can only be established via ipv4 (or vice-versa)), or other networking problems.

It seems to me that after getaddrinfo() the correct behaviour should be to iterate over all addrinfo structs using ai_next until a connection succeeds. This behaviour is mostly already implemented, but overridden by some (apparently conciously chosen) decisions.

The comments mention performance reasons and an reduced amount of DNS lookups, so this might introduce some overhead. At least in our setup, an additional DNS request upon a (re)-connect seems to be less of an issue than problems with the dual-stack setup.

This fixes (or at least should fix):

• Fallback from IPv6 to IPv4 not supported #6358
• Sentinel does not properly handle a combination of v4, v6 addresses in bind, monitor #6345

Test are green, but this wasn't extensively tested in production yet.

Opinions welcome!

NOTE: This PR is against the 4.0 branch, as this is the currently shipped version with Ubuntu 18.04 LTS, but can be easily rebased upon request on unstable or 5.0.

NOTE: This patch includes some changes to hiredis. I've included them here for now, but it might be wise to add an additional pull request to https://github.com/redis/hiredis if this turns out to be a good idea.

[antirez]

Thanks @chr4, I'll evaluate this change with Redis 6 as target, with the possibility to back port to Redis 5. Right now I don't have the full picture to really evaluate if it's ok to merge this: in the past the code have assumed that instances could be identified by their IP address, especially in Sentinel (for Redis Cluster things are simpler because of the Node ID), so I need to check if every safety assumption remains the same after merging this change.

[chr4]

Thanks!

There's an issue addressing another one of the identification-by-IP-address cases in redis-server I'm currently working on. I've added "WIP" to the PR title to reflect this.

Will push more changes next week after some testing.
Let me know if you encounter other issues or thoughts on this, should you find the time!

[chr4]

Pushed some more changes.

We're running this in our staging environment, so far without any issues (with one caveat: The hostnames in the Redis config shouldn't point to localhost (e.g. in /etc/hosts).

As I changed some buffers to use hostnames instead of ip adresses, I'm not quite sure about the buffer length. Naturally _POSIX_HOST_NAME_MAX would be a good fit for the length of the new hostname buffers, but I'm afraid that somewhere in the code this might lead to an overflow if a buffer is copied to a buffer only NET_IP_STR_LEN big. For security reasons, I left the buffer sizes at NET_IP_STR_LEN for now, even though that restricts hostnames to 46 characters.

Also, there's the potential of exchanging a lot of code to use hostnames instead of IPs (e.g. in the ip and myip variables used for announcing, etc.). My knowledge of Redis is not deep enough to be able to assess this so far. I'm currently trying to patch Sentinel to also use hostnames when broadcasting information about servers, but this seems a little more complex. Also, I'm not sure how much work I should put into Sentinel, as far as I understood it might be superseeded by Cluster in the future?

[hwware]

This looks like a conflict work with #8282, pinging @yossigo , maybe we should close this?

[yossigo]

@hwware It overlaps with the hostname support, but #8282 probably doesn't really address dual stack scenarios where we need to maintain and use different addresses.

I am not sure this is such a common problem, and addressing it is going to be pretty complex as Sentinel already makes too many assumptions about addresses uniquely identifying instances.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ chr4
❌ vin01
_{You have signed the CLA already but the status is still pending? Let us recheck it.}