Repository: zalando/patroni
Pull request #2231 (2022-02-28 14:38:09)

consul: Allow setting TLSServerName on consul service checks
See also https://www.consul.io/api-docs/agent/check#tlsservername
Useful in case checks are done by IP and the consul `node_name`
is not an FQDN.

grembo: @CyberDem0n I checked the [the reload_config method](https://github.com/zalando/patroni/blob/c824ef1e68951af9b97d1b0936c53ea620d8c2a9/patroni/dcs/consul.py#L251), but I'm not 100% clear about its semantics - does it actually apply changes (e.g., new value for "dc"), and what would that mean for changing the new value I added (and potentially other values like the check interval)?

In an ideal world, I would expect those changes to catch on immediately (so imagine, patroni is running, but health checks are failing, since the tls name doesn't match - I would like to change patroni's config to override TLSServerName and then do a reload, which should change the existing health check in consul). Not sure if the current logic would work like that (only in this case it would make sense to have "hot" reload, otherwise I think it would be more confusing then helpful).

Also, if config reload actually applied the changes to an already running health check, setting TLSServerName to an empty string (what this patch does right now) will actually be important, so one could switch back the configuration setting to default (e.g., set it to a server name in patroni.yml, do a reload, it gets changed in the health check, realize that not what you wanted, remove it from patroni.yml, do another reload, it gets set to an empty string (=unset)).

FxKu: :+1: 

Patch:
diff --git a/docs/ENVIRONMENT.rst b/docs/ENVIRONMENT.rst
index dd78d35e..bb04bd83 100644
--- a/docs/ENVIRONMENT.rst
+++ b/docs/ENVIRONMENT.rst
@@ -49,6 +49,7 @@ Consul
 -  **PATRONI\_CONSUL\_CHECKS**: (optional) list of Consul health checks used for the session. By default an empty list is used.
 -  **PATRONI\_CONSUL\_REGISTER\_SERVICE**: (optional) whether or not to register a service with the name defined by the scope parameter and the tag master, replica or standby-leader depending on the node's role. Defaults to **false**
 -  **PATRONI\_CONSUL\_SERVICE\_CHECK\_INTERVAL**: (optional) how often to perform health check against registered url
+-  **PATRONI\_CONSUL\_SERVICE\_CHECK\_TLS\_SERVER\_NAME**: (optional) overide SNI host when connecting via TLS, see also `consul agent check API reference <https://www.consul.io/api-docs/agent/check#tlsservername>`__.
 
 Etcd
 ----
diff --git a/docs/SETTINGS.rst b/docs/SETTINGS.rst
index 7433d438..63e7f1c5 100644
--- a/docs/SETTINGS.rst
+++ b/docs/SETTINGS.rst
@@ -132,6 +132,7 @@ Most of the parameters are optional, but you have to specify one of the **host**
 -  **register\_service**: (optional) whether or not to register a service with the name defined by the scope parameter and the tag master, replica or standby-leader depending on the node's role. Defaults to **false**.
 -  **service\_tags**: (optional) additional static tags to add to the Consul service apart from the role (``master``/``replica``/``standby-leader``).  By default an empty list is used.
 -  **service\_check\_interval**: (optional) how often to perform health check against registered url.
+-  **service\_check\_tls\_server\_name**: (optional) overide SNI host when connecting via TLS, see also `consul agent check API reference <https://www.consul.io/api-docs/agent/check#tlsservername>`__.
 
 The ``token`` needs to have the following ACL permissions:
 
diff --git a/patroni/config.py b/patroni/config.py
index 8633d447..5996f68e 100644
--- a/patroni/config.py
+++ b/patroni/config.py
@@ -350,9 +350,9 @@ class Config(object):
                 name, suffix = (param[8:].split('_', 1) + [''])[:2]
                 if suffix in ('HOST', 'HOSTS', 'PORT', 'USE_PROXIES', 'PROTOCOL', 'SRV', 'SRV_SUFFIX', 'URL', 'PROXY',
                               'CACERT', 'CERT', 'KEY', 'VERIFY', 'TOKEN', 'CHECKS', 'DC', 'CONSISTENCY',
-                              'REGISTER_SERVICE', 'SERVICE_CHECK_INTERVAL', 'NAMESPACE', 'CONTEXT',
-                              'USE_ENDPOINTS', 'SCOPE_LABEL', 'ROLE_LABEL', 'POD_IP', 'PORTS', 'LABELS',
-                              'BYPASS_API_SERVICE', 'KEY_PASSWORD', 'USE_SSL', 'SET_ACLS') and name:
+                              'REGISTER_SERVICE', 'SERVICE_CHECK_INTERVAL', 'SERVICE_CHECK_TLS_SERVER_NAME',
+                              'NAMESPACE', 'CONTEXT', 'USE_ENDPOINTS', 'SCOPE_LABEL', 'ROLE_LABEL', 'POD_IP',
+                              'PORTS', 'LABELS', 'BYPASS_API_SERVICE', 'KEY_PASSWORD', 'USE_SSL', 'SET_ACLS') and name:
                     value = os.environ.pop(param)
                     if suffix == 'PORT':
                         value = value and parse_int(value)
diff --git a/patroni/dcs/consul.py b/patroni/dcs/consul.py
index 8b4c25bc..10886c6e 100644
--- a/patroni/dcs/consul.py
+++ b/patroni/dcs/consul.py
@@ -233,6 +233,7 @@ class Consul(AbstractDCS):
         if self._register_service:
             self._set_service_name()
         self._service_check_interval = config.get('service_check_interval', '5s')
+        self._service_check_tls_server_name = config.get('service_check_tls_server_name', None)
         if not self._ctl:
             self.create_session()
 
@@ -458,6 +459,8 @@ class Consul(AbstractDCS):
         conn_parts = urlparse(data['conn_url'])
         check = base.Check.http(api_parts.geturl(), self._service_check_interval,
                                 deregister='{0}s'.format(self._client.http.ttl * 10))
+        if self._service_check_tls_server_name is not None:
+            check['TLSServerName'] = self._service_check_tls_server_name
         tags = self._service_tags[:]
         tags.append(role)
         self._previous_loop_service_tags = self._service_tags
diff --git a/patroni/ha.py b/patroni/ha.py
index 9e0f9b84..d5f47bd6 100644
--- a/patroni/ha.py
+++ b/patroni/ha.py
@@ -293,12 +293,25 @@ class Ha(object):
 
         return result
 
+    def _handle_crash_recovery(self):
+        if not self._crash_recovery_executed and (self.cluster.is_unlocked() or self._rewind.can_rewind):
+            self._crash_recovery_executed = True
+            self._crash_recovery_started = time.time()
+            msg = 'doing crash recovery in a single user mode'
+            return self._async_executor.try_run_async(msg, self._rewind.ensure_clean_shutdown) or msg
+
     def _handle_rewind_or_reinitialize(self):
         leader = self.get_remote_master() if self.is_standby_cluster() else self.cluster.leader
         if not self._rewind.rewind_or_reinitialize_needed_and_possible(leader):
             return None
 
         if self._rewind.can_rewind:
+            # rewind is required, but postgres wasn't shut down cleanly.
+            if self.state_handler.controldata().get('Database cluster state') == 'in archive recovery':
+                msg = self._handle_crash_recovery()
+                if msg:
+                    return msg
+
             msg = 'running pg_rewind from ' + leader.name
             return self._async_executor.try_run_async(msg, self._rewind.execute, args=(leader,)) or msg
 
@@ -325,13 +338,10 @@ class Ha(object):
 
         data = self.state_handler.controldata()
         logger.info('pg_controldata:\n%s\n', '\n'.join('  {0}: {1}'.format(k, v) for k, v in data.items()))
-        if data.get('Database cluster state') in ('in production', 'shutting down', 'in crash recovery') \
-                and not self._crash_recovery_executed and \
-                (self.cluster.is_unlocked() or self._rewind.can_rewind):
-            self._crash_recovery_executed = True
-            self._crash_recovery_started = time.time()
-            msg = 'doing crash recovery in a single user mode'
-            return self._async_executor.try_run_async(msg, self._rewind.ensure_clean_shutdown) or msg
+        if data.get('Database cluster state') in ('in production', 'shutting down', 'in crash recovery'):
+            msg = self._handle_crash_recovery()
+            if msg:
+                return msg
 
         self.load_cluster_from_dcs()
 
diff --git a/patroni/postgresql/__init__.py b/patroni/postgresql/__init__.py
index aeadc3e9..3b9f2507 100644
--- a/patroni/postgresql/__init__.py
+++ b/patroni/postgresql/__init__.py
@@ -798,7 +798,8 @@ class Postgresql(object):
         return True
 
     def get_guc_value(self, name):
-        cmd = [self.pgcommand('postgres'), '-D', self._data_dir, '-C', name]
+        cmd = [self.pgcommand('postgres'), '-D', self._data_dir, '-C', name,
+               '--config-file={}'.format(self.config.postgresql_conf)]
         try:
             data = subprocess.check_output(cmd)
             if data:
diff --git a/patroni/postgresql/rewind.py b/patroni/postgresql/rewind.py
index ba6e4532..5a2e4a39 100644
--- a/patroni/postgresql/rewind.py
+++ b/patroni/postgresql/rewind.py
@@ -112,7 +112,7 @@ class Rewind(object):
         in_recovery = timeline = lsn = None
         data = self._postgresql.controldata()
         try:
-            if data.get('Database cluster state') == 'shut down in recovery':
+            if data.get('Database cluster state') in ('shut down in recovery', 'in archive recovery'):
                 in_recovery = True
                 lsn = data.get('Minimum recovery ending location')
                 timeline = int(data.get("Min recovery ending loc's timeline"))
@@ -311,8 +311,14 @@ class Rewind(object):
         restore_command = self._postgresql.config.get('recovery_conf', {}).get('restore_command') \
             if self._postgresql.major_version < 120000 else self._postgresql.get_guc_value('restore_command')
 
+        # currently, pg_rewind expects postgresql.conf to be inside $PGDATA, which is not the case on e.g. Debian
+        # Fix this logic if e.g. PG15 receives an update for pg_rewind:
+        pg_rewind_can_restore = self._postgresql.major_version >= 130000 \
+            and restore_command \
+            and self._postgresql.config._config_dir == self._postgresql.data_dir
+
         cmd = [self._postgresql.pgcommand('pg_rewind')]
-        if self._postgresql.major_version >= 130000 and restore_command:
+        if pg_rewind_can_restore:
             cmd.append('--restore-target-wal')
         cmd.extend(['-D', self._postgresql.data_dir, '--source-server', dsn])
 
@@ -329,7 +335,7 @@ class Rewind(object):
             if ret == 0:
                 return True
 
-            if not restore_command or self._postgresql.major_version >= 130000:
+            if not restore_command or pg_rewind_can_restore:
                 return False
 
             missing_wal = self._find_missing_wal(results['stderr']) or self._find_missing_wal(results['stdout'])


Test patch:
diff --git a/tests/test_ha.py b/tests/test_ha.py
index 90467b20..e2ef7b6f 100644
--- a/tests/test_ha.py
+++ b/tests/test_ha.py
@@ -284,6 +284,18 @@ class TestHa(PostgresInit):
             self.ha.patroni.config.set_dynamic_configuration({'maximum_lag_on_failover': 10})
             self.assertEqual(self.ha.run_cycle(), 'terminated crash recovery because of startup timeout')
 
+    @patch.object(Rewind, 'ensure_clean_shutdown', Mock())
+    @patch.object(Rewind, 'rewind_or_reinitialize_needed_and_possible', Mock(return_value=True))
+    @patch.object(Rewind, 'can_rewind', PropertyMock(return_value=True))
+    def test_crash_recovery_before_rewind(self):
+        self.p.is_leader = false
+        self.p.is_running = false
+        self.p.controldata = lambda: {'Database cluster state': 'in archive recovery',
+                                      'Database system identifier': SYSID}
+        self.ha._rewind.trigger_check_diverged_lsn()
+        self.ha.cluster = get_cluster_initialized_with_leader()
+        self.assertEqual(self.ha.run_cycle(), 'doing crash recovery in a single user mode')
+
     @patch.object(Rewind, 'rewind_or_reinitialize_needed_and_possible', Mock(return_value=True))
     @patch.object(Rewind, 'can_rewind', PropertyMock(return_value=True))
     def test_recover_with_rewind(self):