[Bug Fix] Ensure port mapping is not done in isolation of host mapping

The existing remapping logic looks at host remapping and port remapping in
isolation, which leads to failure in some cases where all from_ports are same
like the following example.
```
startup_nodes = [
	{"host": "127.0.0.1", "port": "17000"},
	{"host": "127.0.0.1", "port": "17001"},
	{"host": "127.0.0.1", "port": "17002"},
	{"host": "127.0.0.1", "port": "17003"},
	{"host": "127.0.0.1", "port": "17004"},
	{"host": "127.0.0.1", "port": "17005"}
]

host_port_remap=[
    {'from_host': '41.1.3.1', 'from_port': 6379, 'to_host': '127.0.0.1', 'to_port': 17000},
    {'from_host': '41.1.3.5', 'from_port': 6379, 'to_host': '127.0.0.1', 'to_port': 17001},
    {'from_host': '41.1.4.2', 'from_port': 6379, 'to_host': '127.0.0.1', 'to_port': 17002},
    {'from_host': '50.0.1.7', 'from_port': 6379, 'to_host': '127.0.0.1', 'to_port': 17003},
    {'from_host': '50.0.7.3', 'from_port': 6379, 'to_host': '127.0.0.1', 'to_port': 17004},
    {'from_host': '32.0.1.1', 'from_port': 6379, 'to_host': '127.0.0.1', 'to_port': 17005}
]
```
This is becasue when just ports get remapped, it ends up producing overlapping node names
and ends up corrupting both the `nodes_cache` and the `slots` dictionaries in nodemanager

Changes:
- This change modifies the remapping logic to use both host and port info(if avaiable) to decide if remapping should happen
- Adds more cases in the host_port_remap list validation logic so that no unknown keys are allowed and only valid ips are allowed
- Added a more explicit ip comparison logic(comparing octets indiviidually)
- Also fixed an unrelated failing testcase `test_blocked_commands` in `test_cluster_obj.py`
- Updated the docs/clients.rst with details about the new mapping logic and added an example

Test Plan:
- Updated Unit tests to catch this is issue
- Tested the changes a real AWS Elasticache Redis cluster fronted by stunnel
  and tested both get, mget and set command with differrent keys

Author: Appurv Jain

.gitignore

@@ -13,3 +13,4 @@ build
 .cache
 docs/_build
 docs/_build_html
+.idea

docs/authors.rst

@@ -27,3 +27,4 @@ Authors who contributed code or testing:
  - astrohsy - https://github.com/astrohsy
  - Artur Stawiarski - https://github.com/astawiarski
  - Matthew Anderson - https://github.com/mc3ander
+ - Appurv Jain - https://github.com/appurvj

docs/client.rst

@@ -12,13 +12,45 @@ host_port_remap
 
 This option exists to enable the client to fix a problem where the redis-server internally tracks a different ip:port compared to what your clients would like to connect to.
 
-The simples example to describe this problem is if you start a redis cluster through docker on your local machine. If we assume that you start the docker image grokzen/redis-cluster, when the redis cluster is initialized it will track the docker network IP for each node in the cluster.
+A simple example to describe this problem is if you start a redis cluster through docker on your local machine. If we assume that you start the docker image grokzen/redis-cluster,
+when the redis cluster is initialized it will track the docker network IP for each node in the cluster.
 
-For example this could be 172.18.0.2. The problem is that a client that runs outside on your local machine will recieve from the redis cluster that each node is reachable on the ip 172.18.0.2. But in some cases this IP is not available on your host system and to solve this we need a remapping table where we can tell this client that if you get back from your cluster 172.18.0.2 then your should remap it to localhost instead. When the client does this it can now connect and reach all nodes in your cluster.
+For example this could be 172.18.0.2. The problem is that a client that runs outside on your local machine will receive from the redis cluster that each node is reachable on the ip 172.18.0.2.
+But in some cases this IP is not available on your host system.To solve this we need a remapping table where we can tell this client that if you get back from your cluster 172.18.0.2 then your should remap it to localhost instead.
+When the client does this it can now connect and reach all nodes in your cluster.
 
-It is also possible to remap the port for each node as well.
 
-Example script
+Remapping works off a rules list. Each rule is a dictionary of the form shown below
+
+.. code-block::
+
+    {
+        'from_host': <Host name on redis server side>, # String
+        'from_port': <Port on the redis server side>, # Integer
+        'to_host': <Host name that the client needs to map to>, # String
+        'to_port': <Port that the client needs to map to> # Integer
+    }
+
+
+Remapping properties:
+
+- This host_port_remap feature will not work on the startup_nodes so you still need to put in a valid and reachable set of startup nodes.
+- The remapping logic treats host_port_remap list as a "rules list" and only the first matching remapping entry will be applied
+- A remapping rule may contain just host or just port mapping, but both sides of the maping( i.e. from_host and to_host or from_port and to_port) are required for either
+- If both from_host and from_port are specified, then both will be used to decide if a remapping rule applies
+
+Examples of valid rules:
+
+.. code-block:: python
+
+    {'from_host': "1.2.3.4", 'from_port': 1000, 'to_host': "2.2.2.2", 'to_port': 2000}
+
+    {'from_host': "1.1.1.1", 'to_host': "127.0.0.1"}
+
+    {'from_port': 1000, 'to_port': 2000}
+
+
+Example scripts:
 
 .. code-block:: python
 
@@ -52,5 +84,43 @@ Example script
     ## Test the client that it can still send and recieve data from the nodes after the remap has been done
     print(rc.set('foo', 'bar'))
 
+This feature is also useful in cases such as when one is trying to access AWS ElastiCache cluster secured by Stunnel (https://www.stunnel.org/)
+
+.. code-block:: python
+
+    from rediscluster import RedisCluster
+
+    startup_nodes = [
+        {"host": "127.0.0.1", "port": "17000"},
+        {"host": "127.0.0.1", "port": "17001"},
+        {"host": "127.0.0.1", "port": "17002"},
+        {"host": "127.0.0.1", "port": "17003"},
+        {"host": "127.0.0.1", "port": "17004"},
+        {"host": "127.0.0.1", "port": "17005"}
+    ]
+
+    host_port_remap=[
+        {'from_host': '41.1.3.1', 'from_port': 6379, 'to_host': '127.0.0.1', 'to_port': 17000},
+        {'from_host': '41.1.3.5', 'from_port': 6379, 'to_host': '127.0.0.1', 'to_port': 17001},
+        {'from_host': '41.1.4.2', 'from_port': 6379, 'to_host': '127.0.0.1', 'to_port': 17002},
+        {'from_host': '50.0.1.7', 'from_port': 6379, 'to_host': '127.0.0.1', 'to_port': 17003},
+        {'from_host': '50.0.7.3', 'from_port': 6379, 'to_host': '127.0.0.1', 'to_port': 17004},
+        {'from_host': '32.0.1.1', 'from_port': 6379, 'to_host': '127.0.0.1', 'to_port': 17005}
+    ]
+
+
+    # Note: decode_responses must be set to True when used with python3
+    rc = RedisCluster(
+        startup_nodes=startup_nodes,
+        host_port_remap=host_port_remap,
+        decode_responses=True,
+        ssl=True,
+        ssl_cert_reqs=None,
+        # Needed for Elasticache Clusters
+        skip_full_coverage_check=True)
 
-Pleaes note that this host_port_remap feature will not work on the startup_nodes so you still need to put in a valid and reachable set of startup nodes.
+
+    print(rc.connection_pool.nodes.nodes)
+    print(rc.ping())
+    print(rc.set('foo', 'bar'))
+    print(rc.get('foo'))

docs/release-notes.rst

@@ -26,7 +26,7 @@ Release Notes
     * Implement new connection pool ClusterBlockingConnectionPool (#347)
     * Nodemanager initiailize should now handle usernames properly (#365)
     * PubSub tests has been all been disabled
-    * New feature, host_port_remap. Send in a remapping configuration to RedisCluster instance where the nodes configuration recieved from the redis cluster can be altered to allow for connection in certain circumstances. See new section in clients.rst in docs/ for usage example.
+    * New feature, host_port_remap. Send in a remapping configuration to RedisCluster instance where the nodes configuration recieved from the redis cluster can be altered to allow for connection in certain circumstances. See new section in client.rst in docs/ for usage example.
     * When a slot is not covered by the cluster, it will not raise SlotNotCoveredError instead of the old generic RedisClusterException. The client will not attempt to rebuild the cluster layout a few times before giving up and raising that exception to the user. (#350)
     * CLIENT SETNAME is now possible to use from the client instance. For setting the name for all connections from the client by default, see issue #802 in redis-py repo for the change that was implemented in redis-py 3.4.0.
 

rediscluster/nodemanager.py

@@ -4,14 +4,14 @@
 import json
 import logging
 import random
+import socket
 
 # rediscluster imports
 from .crc import crc16
 from .exceptions import RedisClusterException, RedisClusterConfigError
 
 # 3rd party imports
 from redis import Redis
-from redis._compat import unicode, long, basestring
 from redis.connection import Encoder
 from redis import ConnectionError, TimeoutError, ResponseError
 
@@ -71,12 +71,30 @@ def _validate_host_port_remap(self, host_port_remap):
             if not isinstance(item, dict):
                 raise RedisClusterConfigError("items inside host_port_remap list must be of dict type")
 
+            if len(set(item.keys()) - {'from_host', 'from_port', 'to_host', 'to_port'}) != 0:
+                raise RedisClusterConfigError("Invalid keys provided in host_port_remap rule")
+
             # If we have from_host, we must have a to_host option to allow for translation to work
             if ('from_host' in item and 'to_host' not in item) or ('from_host' not in item and 'to_host' in item):
-                raise RedisClusterConfigError("Both from_host and to_host must be present in remap item if either is defined")
+                raise RedisClusterConfigError("Both from_host and to_host must be present in host_port_remap rule if either is defined")
+
+            if ('from_port' in item and 'to_port' not in item) or ('from_port' not in item and 'to_port' in item):
+                raise RedisClusterConfigError("Both from_port and to_port must be present in host_port_remap rule if either is defined")
+
+            try:
+                socket.inet_aton(item.get('from_host', '0.0.0.0').strip())
+                socket.inet_aton(item.get('to_host', '0.0.0.0').strip())
+            except socket.error:
+                raise RedisClusterConfigError("Both from_host and to_host in host_port_remap rule must be a valid ip address")
+            if len(item.get('from_host', '0.0.0.0').split('.')) < 4 or len(item.get('to_host', '0.0.0.0').split('.')) < 4 :
+                raise RedisClusterConfigError(
+                    "Both from_host and to_host in host_port_remap rule must must have all octets specified")
 
-            if ('from_port' in item and 'to_port' not in item) or  ('from_port' not in item and 'to_port' in item):
-                raise RedisClusterConfigError("Both from_port and to_port must be present in remap item")
+            try:
+                int(item.get('from_port', 0))
+                int(item.get('to_port', 0))
+            except ValueError:
+                raise RedisClusterConfigError("Both from_port and to_port in host_port_remap rule must be integers")
 
     def keyslot(self, key):
         """
@@ -305,9 +323,30 @@ def remap_internal_node_object(self, node_obj):
             if 'from_port' in remap_rule and 'to_port' in remap_rule:
                 if remap_rule['from_port'] == node_obj[1]:
                     node_obj[1] = remap_rule['to_port']
+                # At this point remapping has occurred, so no further rules should be processed
+                break
 
         return node_obj
 
+    def _remap_rule_applies(self, remap_rule, node_obj):
+        # Double check to make sure that the relevant host and/or port fields are present
+        if not (('from_host' in remap_rule and 'to_host' in remap_rule) or ('from_port' in remap_rule and 'to_port' in remap_rule)):
+            return False
+        if 'from_host' in remap_rule and not self._ips_equal(remap_rule['from_host'], node_obj[0]):
+            return False
+        if 'from_port' in remap_rule and remap_rule['from_port'] != node_obj[1]:
+            return False
+        # If the previous conditions are not met then this is a valid match.
+        return True
+
+    def _ips_equal(self, ip1, ip2):
+        split_ip1 = ip1.strip().split(".")
+        split_ip2 = ip2.strip().split(".")
+        for i, octet in enumerate(split_ip1):
+            if int(octet) != int(split_ip2[i]):
+                return False
+        return True
+
     def increment_reinitialize_counter(self, ct=1, count=1):
         for i in range(min(ct, self.reinitialize_steps)):
             self.reinitialize_counter += count

tests/test_cluster_node_manager.py

@@ -495,13 +495,32 @@ def test_host_port_remap():
             startup_nodes=[{"host": "127.0.0.1", "port": 7000}],
             host_port_remap=[{'to_port': ''}],
         )
+    # Invalid keys in the rules should also raise exception
+    with pytest.raises(RedisClusterConfigError) as excp:
+        n = NodeManager(
+            startup_nodes=[{"host": "127.0.0.1", "port": 7000}],
+            host_port_remap=[{'invalid_key': ''}],
+        )
+
+    # Invalid ips in the rules should raise exception
+    with pytest.raises(RedisClusterConfigError) as excp:
+        n = NodeManager(
+            startup_nodes=[{"host": "127.0.0.1", "port": 7000}],
+            host_port_remap=[{'from_host': '127.2.x.w', 'to_host': '127.0.0.1'}],
+        )
+    # Incomplete ips in the rules should raise exception
+    with pytest.raises(RedisClusterConfigError) as excp:
+        n = NodeManager(
+            startup_nodes=[{"host": "127.0.0.1", "port": 7000}],
+            host_port_remap=[{'from_host': '127.2', 'to_host': '127.0.0.1'}],
+        )
 
     # Creating a valid config with multiple entries
     n = NodeManager(
         startup_nodes=[{"host": "127.0.0.1", "port": 7000}],
         host_port_remap=[
-            {'from_host': '127.0.0.1', 'to_host': 'localhost', 'from_port': 7000, 'to_port': 70001},
-            {'from_host': '172.1.0.1', 'to_host': 'localhost', 'from_port': 7000, 'to_port': 70001},
+            {'from_host': '127.0.0.1', 'to_host': '127.0.0.1', 'from_port': 7000, 'to_port': 70001},
+            {'from_host': '172.1.0.1', 'to_host': '127.0.0.1', 'from_port': 7000, 'to_port': 70001},
         ],
     )
 
@@ -516,10 +535,56 @@ def test_host_port_remap():
 
     # Test that modifying both host and port works
     n = NodeManager(
-        host_port_remap=[{'from_host': '127.0.0.1', 'to_host': 'localhost', 'from_port': 7000, 'to_port': 7001}],
+        host_port_remap=[{'from_host': '127.1.1.1', 'to_host': '128.0.0.1', 'from_port': 7000, 'to_port': 7001},
+                         {'from_host': '127.2.2.2', 'to_host': '128.0.0.1', 'from_port': 7000, 'to_port': 7005}],
+        startup_nodes=[{"host": "128.0.0.1", "port": 7000}]
+    )
+    initial_node_obj = ['127.1.1.1', 7000, 'xyz']
+    remapped_obj = n.remap_internal_node_object(initial_node_obj)
+    assert remapped_obj[0] == '128.0.0.1'
+    assert remapped_obj[1] == 7001
+
+    # Validate that ports are NOT remapped in isolation if hosts are also present
+    n = NodeManager(
+        host_port_remap=[{'from_host': '127.2.2.2', 'to_host': '127.0.0.1', 'from_port': 7000, 'to_port': 7001},
+                         {'from_host': '127.3.3.3', 'to_host': '127.0.0.1', 'from_port': 7000, 'to_port': 7005}],
         startup_nodes=[{"host": "127.0.0.1", "port": 7000}]
     )
     initial_node_obj = ['127.0.0.1', 7000, 'xyz']
     remapped_obj = n.remap_internal_node_object(initial_node_obj)
-    assert remapped_obj[0] == 'localhost'
+    assert remapped_obj[0] == '127.0.0.1'
+    assert remapped_obj[1] == 7000
+
+    # Validate that first applicable rule is applied
+    n = NodeManager(
+        host_port_remap=[{'from_host': '127.2.2.2', 'to_host': '127.0.0.1', 'from_port': 7000, 'to_port': 7001},
+                         {'from_host': '127.3.3.3', 'to_host': '127.0.0.1', 'from_port': 7000, 'to_port': 7005},
+                         {'from_host': '127.2.2.2', 'to_host': '127.0.0.1', 'from_port': 7000, 'to_port': 7006}],
+        startup_nodes=[{"host": "127.0.0.1", "port": 7000}]
+    )
+    initial_node_obj = ['127.2.2.2', 7000, 'xyz']
+    remapped_obj = n.remap_internal_node_object(initial_node_obj)
+    assert remapped_obj[0] == '127.0.0.1'
     assert remapped_obj[1] == 7001
+
+    # Validate just port mapping works
+    n = NodeManager(
+        host_port_remap=[{'from_port': 7000, 'to_port': 7001},
+                         {'from_port': 7002, 'to_port': 7005}],
+        startup_nodes=[{"host": "127.0.0.1", "port": 7000}]
+    )
+    initial_node_obj = ['127.0.0.1', 7000, 'xyz']
+    remapped_obj = n.remap_internal_node_object(initial_node_obj)
+    assert remapped_obj[0] == '127.0.0.1'
+    assert remapped_obj[1] == 7001
+
+    # Validate just host mapping works
+    n = NodeManager(
+        host_port_remap=[{'from_host': '127.2.2.2', 'to_host': '127.0.0.1'},
+                         {'from_host': '127.3.3.3', 'to_host': '127.0.0.2'}],
+        startup_nodes=[{"host": "127.0.0.1", "port": 7000}]
+    )
+    initial_node_obj = ['127.3.3.3', 7000, 'xyz']
+    remapped_obj = n.remap_internal_node_object(initial_node_obj)
+    assert remapped_obj[0] == '127.0.0.2'
+    assert remapped_obj[1] == 7000