Fix bug in patronictl query command (#2859)

Previous to this commit `patronictl query` was working only if `-r` argument was provided to the command. Otherwise it would face issues:

* If neither `-r` nor `-m` were provided:

```
 PGPASSWORD=zalando patronictl -c postgres0.yml query -U postgres -c "SHOW PORT"
2023-09-12 17:45:38	No connection to role=None is available
```

* If only `-m` was provided:

```
$ PGPASSWORD=zalando patronictl -c postgres0.yml query -U postgres -c "SHOW PORT" -m postgresql0
2023-09-12 17:46:15	No connection to member postgresql0 is available
```

This issue was a regression introduced by `4c3e0b9382820524239d2aa4d6b95379ef1291db` through PR #2687.

Through that PR we decided to move the common logic used to check mutually exclusiveness of `--role` and `--member` arguments to `get_any_member` function.

However, previous to that change `role` variable would assume the default value of `any` in `query` method, before `get_any_member` was called, which was not the case after the change.

This commit fixes that issue by adding a handler in `get_cursor` function to `role=None`. As `role` defaulting to `any` is handled in a sub-call to `get_any_member`, we are apparently safe in `get_cursor` to return the cursor if `role=None`.

Unit tests were updated accordingly.

References: PAT-204.

patroni/ctl.py

@@ -561,9 +561,10 @@ def get_cursor(obj: Dict[str, Any], cluster: Cluster, group: Optional[int], conn
     from . import psycopg
     conn = psycopg.connect(**params)
     cursor = conn.cursor()
-    # If we want ``any`` node we are fine to return the cursor
+    # If we want ``any`` node we are fine to return the cursor. ``None`` is similar to ``any`` at this point, as it's
+    # been dealt with through :func:`get_any_member`.
     # If we want the Patroni leader node, :func:`get_any_member` already checks that for us
-    if role in ('any', 'leader'):
+    if role in (None, 'any', 'leader'):
         return cursor
 
     # If we want something other than ``any`` or ``leader``, then we do not rely only on the DCS information about
@@ -857,9 +858,11 @@ def query_member(obj: Dict[str, Any], cluster: Cluster, group: Optional[int],
 
         if cursor is None:
             if member is not None:
-                message = 'No connection to member {0} is available'.format(member)
+                message = f'No connection to member {member} is available'
+            elif role is not None:
+                message = f'No connection to role {role} is available'
             else:
-                message = 'No connection to role={0} is available'.format(role)
+                message = 'No connection is available'
             logging.debug(message)
             return [[timestamp(0), message]], None
 

tests/test_ctl.py

@@ -75,6 +75,21 @@ class TestCtl(unittest.TestCase):
 
         self.assertIsNotNone(get_cursor({}, get_cluster_initialized_with_leader(), None, {'dbname': 'foo'}, role='any'))
 
+        # Mutually exclusive options
+        with self.assertRaises(PatroniCtlException) as e:
+            get_cursor({}, get_cluster_initialized_with_leader(), None, {'dbname': 'foo'}, member_name='other',
+                       role='replica')
+
+        self.assertEqual(str(e.exception), '--role and --member are mutually exclusive options')
+
+        # Invalid member provided
+        self.assertIsNone(get_cursor({}, get_cluster_initialized_with_leader(), None, {'dbname': 'foo'},
+                                     member_name='invalid'))
+
+        # Valid member provided
+        self.assertIsNotNone(get_cursor({}, get_cluster_initialized_with_leader(), None, {'dbname': 'foo'},
+                                        member_name='other'))
+
     def test_parse_dcs(self):
         assert parse_dcs(None) is None
         assert parse_dcs('localhost') == {'etcd': {'host': 'localhost:2379'}}
@@ -278,11 +293,17 @@ class TestCtl(unittest.TestCase):
                 rows = query_member({}, None, None, None, None, 'replica', 'SELECT pg_catalog.pg_is_in_recovery()', {})
 
         with patch('patroni.ctl.get_cursor', Mock(return_value=None)):
+            # No role nor member given -- generic message
             rows = query_member({}, None, None, None, None, None, 'SELECT pg_catalog.pg_is_in_recovery()', {})
-            self.assertTrue('No connection to' in str(rows))
+            self.assertTrue('No connection is available' in str(rows))
 
-            rows = query_member({}, None, None, None, 'foo', 'replica', 'SELECT pg_catalog.pg_is_in_recovery()', {})
-            self.assertTrue('No connection to' in str(rows))
+            # Member given -- message pointing to member
+            rows = query_member({}, None, None, None, 'foo', None, 'SELECT pg_catalog.pg_is_in_recovery()', {})
+            self.assertTrue('No connection to member foo' in str(rows))
+
+            # Role given -- message pointing to role
+            rows = query_member({}, None, None, None, None, 'replica', 'SELECT pg_catalog.pg_is_in_recovery()', {})
+            self.assertTrue('No connection to role replica' in str(rows))
 
         with patch('patroni.ctl.get_cursor', Mock(side_effect=OperationalError('bla'))):
             rows = query_member({}, None, None, None, None, 'replica', 'SELECT pg_catalog.pg_is_in_recovery()', {})