Fix all monitoring hooks to warn if the server returns an HTTP 4xx error (#554).

NEWS

@@ -1,6 +1,8 @@
 1.6.5.dev0
  * #553: Fix logging to include the full traceback when Borg experiences an internal error, not just
    the first few lines.
+ * #554: Fix all monitoring hooks to warn if the server returns an HTTP 4xx error. This can happen
+   with Healthchecks, for instance, when using an invalid ping URL.
 
 1.6.4
  * #546, #382: Keep your repository passphrases and database passwords outside of borgmatic's

borgmatic/hooks/cronhub.py

@@ -43,7 +43,9 @@ def ping_monitor(hook_config, config_filename, state, monitoring_log_level, dry_
     if not dry_run:
         logging.getLogger('urllib3').setLevel(logging.ERROR)
         try:
-            requests.get(ping_url)
+            response = requests.get(ping_url)
+            if not response.ok:
+                response.raise_for_status()
         except requests.exceptions.RequestException as error:
             logger.warning(f'{config_filename}: Cronhub error: {error}')
 

borgmatic/hooks/cronitor.py

@@ -38,7 +38,9 @@ def ping_monitor(hook_config, config_filename, state, monitoring_log_level, dry_
     if not dry_run:
         logging.getLogger('urllib3').setLevel(logging.ERROR)
         try:
-            requests.get(ping_url)
+            response = requests.get(ping_url)
+            if not response.ok:
+                response.raise_for_status()
         except requests.exceptions.RequestException as error:
             logger.warning(f'{config_filename}: Cronitor error: {error}')
 

borgmatic/hooks/healthchecks.py

@@ -125,7 +125,9 @@ def ping_monitor(hook_config, config_filename, state, monitoring_log_level, dry_
     if not dry_run:
         logging.getLogger('urllib3').setLevel(logging.ERROR)
         try:
-            requests.post(ping_url, data=payload.encode('utf-8'))
+            response = requests.post(ping_url, data=payload.encode('utf-8'))
+            if not response.ok:
+                response.raise_for_status()
         except requests.exceptions.RequestException as error:
             logger.warning(f'{config_filename}: Healthchecks error: {error}')
 

borgmatic/hooks/ntfy.py

@@ -59,7 +59,9 @@ def ping_monitor(hook_config, config_filename, state, monitoring_log_level, dry_
         if not dry_run:
             logging.getLogger('urllib3').setLevel(logging.ERROR)
             try:
-                requests.post(f'{base_url}/{topic}', headers=headers)
+                response = requests.post(f'{base_url}/{topic}', headers=headers)
+                if not response.ok:
+                    response.raise_for_status()
             except requests.exceptions.RequestException as error:
                 logger.warning(f'{config_filename}: Ntfy error: {error}')
 

borgmatic/hooks/pagerduty.py

@@ -69,7 +69,9 @@ def ping_monitor(hook_config, config_filename, state, monitoring_log_level, dry_
 
     logging.getLogger('urllib3').setLevel(logging.ERROR)
     try:
-        requests.post(EVENTS_API_URL, data=payload.encode('utf-8'))
+        response = requests.post(EVENTS_API_URL, data=payload.encode('utf-8'))
+        if not response.ok:
+            response.raise_for_status()
     except requests.exceptions.RequestException as error:
         logger.warning(f'{config_filename}: PagerDuty error: {error}')
 

tests/unit/hooks/test_cronhub.py

@@ -5,7 +5,9 @@ from borgmatic.hooks import cronhub as module
 
 def test_ping_monitor_rewrites_ping_url_for_start_state():
     hook_config = {'ping_url': 'https://example.com/start/abcdef'}
-    flexmock(module.requests).should_receive('get').with_args('https://example.com/start/abcdef')
+    flexmock(module.requests).should_receive('get').with_args(
+        'https://example.com/start/abcdef'
+    ).and_return(flexmock(ok=True))
 
     module.ping_monitor(
         hook_config,
@@ -18,7 +20,9 @@ def test_ping_monitor_rewrites_ping_url_for_start_state():
 
 def test_ping_monitor_rewrites_ping_url_and_state_for_start_state():
     hook_config = {'ping_url': 'https://example.com/ping/abcdef'}
-    flexmock(module.requests).should_receive('get').with_args('https://example.com/start/abcdef')
+    flexmock(module.requests).should_receive('get').with_args(
+        'https://example.com/start/abcdef'
+    ).and_return(flexmock(ok=True))
 
     module.ping_monitor(
         hook_config,
@@ -31,7 +35,9 @@ def test_ping_monitor_rewrites_ping_url_and_state_for_start_state():
 
 def test_ping_monitor_rewrites_ping_url_for_finish_state():
     hook_config = {'ping_url': 'https://example.com/start/abcdef'}
-    flexmock(module.requests).should_receive('get').with_args('https://example.com/finish/abcdef')
+    flexmock(module.requests).should_receive('get').with_args(
+        'https://example.com/finish/abcdef'
+    ).and_return(flexmock(ok=True))
 
     module.ping_monitor(
         hook_config,
@@ -44,7 +50,9 @@ def test_ping_monitor_rewrites_ping_url_for_finish_state():
 
 def test_ping_monitor_rewrites_ping_url_for_fail_state():
     hook_config = {'ping_url': 'https://example.com/start/abcdef'}
-    flexmock(module.requests).should_receive('get').with_args('https://example.com/fail/abcdef')
+    flexmock(module.requests).should_receive('get').with_args(
+        'https://example.com/fail/abcdef'
+    ).and_return(flexmock(ok=True))
 
     module.ping_monitor(
         hook_config, 'config.yaml', module.monitor.State.FAIL, monitoring_log_level=1, dry_run=False
@@ -60,11 +68,32 @@ def test_ping_monitor_dry_run_does_not_hit_ping_url():
     )
 
 
-def test_ping_monitor_with_connection_error_does_not_raise():
+def test_ping_monitor_with_connection_error_logs_warning():
     hook_config = {'ping_url': 'https://example.com/start/abcdef'}
     flexmock(module.requests).should_receive('get').and_raise(
         module.requests.exceptions.ConnectionError
     )
+    flexmock(module.logger).should_receive('warning').once()
+
+    module.ping_monitor(
+        hook_config,
+        'config.yaml',
+        module.monitor.State.START,
+        monitoring_log_level=1,
+        dry_run=False,
+    )
+
+
+def test_ping_monitor_with_other_error_logs_warning():
+    hook_config = {'ping_url': 'https://example.com/start/abcdef'}
+    response = flexmock(ok=False)
+    response.should_receive('raise_for_status').and_raise(
+        module.requests.exceptions.RequestException
+    )
+    flexmock(module.requests).should_receive('get').with_args(
+        'https://example.com/start/abcdef'
+    ).and_return(response)
+    flexmock(module.logger).should_receive('warning').once()
 
     module.ping_monitor(
         hook_config,

tests/unit/hooks/test_cronitor.py

@@ -5,7 +5,9 @@ from borgmatic.hooks import cronitor as module
 
 def test_ping_monitor_hits_ping_url_for_start_state():
     hook_config = {'ping_url': 'https://example.com'}
-    flexmock(module.requests).should_receive('get').with_args('https://example.com/run')
+    flexmock(module.requests).should_receive('get').with_args('https://example.com/run').and_return(
+        flexmock(ok=True)
+    )
 
     module.ping_monitor(
         hook_config,
@@ -18,7 +20,9 @@ def test_ping_monitor_hits_ping_url_for_start_state():
 
 def test_ping_monitor_hits_ping_url_for_finish_state():
     hook_config = {'ping_url': 'https://example.com'}
-    flexmock(module.requests).should_receive('get').with_args('https://example.com/complete')
+    flexmock(module.requests).should_receive('get').with_args(
+        'https://example.com/complete'
+    ).and_return(flexmock(ok=True))
 
     module.ping_monitor(
         hook_config,
@@ -31,7 +35,9 @@ def test_ping_monitor_hits_ping_url_for_finish_state():
 
 def test_ping_monitor_hits_ping_url_for_fail_state():
     hook_config = {'ping_url': 'https://example.com'}
-    flexmock(module.requests).should_receive('get').with_args('https://example.com/fail')
+    flexmock(module.requests).should_receive('get').with_args(
+        'https://example.com/fail'
+    ).and_return(flexmock(ok=True))
 
     module.ping_monitor(
         hook_config, 'config.yaml', module.monitor.State.FAIL, monitoring_log_level=1, dry_run=False
@@ -47,11 +53,32 @@ def test_ping_monitor_dry_run_does_not_hit_ping_url():
     )
 
 
-def test_ping_monitor_with_connection_error_does_not_raise():
+def test_ping_monitor_with_connection_error_logs_warning():
     hook_config = {'ping_url': 'https://example.com'}
     flexmock(module.requests).should_receive('get').and_raise(
         module.requests.exceptions.ConnectionError
     )
+    flexmock(module.logger).should_receive('warning').once()
+
+    module.ping_monitor(
+        hook_config,
+        'config.yaml',
+        module.monitor.State.START,
+        monitoring_log_level=1,
+        dry_run=False,
+    )
+
+
+def test_ping_monitor_with_other_error_logs_warning():
+    hook_config = {'ping_url': 'https://example.com'}
+    response = flexmock(ok=False)
+    response.should_receive('raise_for_status').and_raise(
+        module.requests.exceptions.RequestException
+    )
+    flexmock(module.requests).should_receive('get').with_args('https://example.com/run').and_return(
+        response
+    )
+    flexmock(module.logger).should_receive('warning').once()
 
     module.ping_monitor(
         hook_config,

tests/unit/hooks/test_healthchecks.py

@@ -139,7 +139,7 @@ def test_ping_monitor_hits_ping_url_for_start_state():
     hook_config = {'ping_url': 'https://example.com'}
     flexmock(module.requests).should_receive('post').with_args(
         'https://example.com/start', data=''.encode('utf-8')
-    )
+    ).and_return(flexmock(ok=True))
 
     module.ping_monitor(
         hook_config,
@@ -156,7 +156,7 @@ def test_ping_monitor_hits_ping_url_for_finish_state():
     flexmock(module).should_receive('format_buffered_logs_for_payload').and_return(payload)
     flexmock(module.requests).should_receive('post').with_args(
         'https://example.com', data=payload.encode('utf-8')
-    )
+    ).and_return(flexmock(ok=True))
 
     module.ping_monitor(
         hook_config,
@@ -173,7 +173,7 @@ def test_ping_monitor_hits_ping_url_for_fail_state():
     flexmock(module).should_receive('format_buffered_logs_for_payload').and_return(payload)
     flexmock(module.requests).should_receive('post').with_args(
         'https://example.com/fail', data=payload.encode('utf')
-    )
+    ).and_return(flexmock(ok=True))
 
     module.ping_monitor(
         hook_config,
@@ -190,7 +190,7 @@ def test_ping_monitor_with_ping_uuid_hits_corresponding_url():
     flexmock(module).should_receive('format_buffered_logs_for_payload').and_return(payload)
     flexmock(module.requests).should_receive('post').with_args(
         'https://hc-ping.com/{}'.format(hook_config['ping_url']), data=payload.encode('utf-8')
-    )
+    ).and_return(flexmock(ok=True))
 
     module.ping_monitor(
         hook_config,
@@ -234,7 +234,7 @@ def test_ping_monitor_hits_ping_url_when_states_matching():
     hook_config = {'ping_url': 'https://example.com', 'states': ['start', 'finish']}
     flexmock(module.requests).should_receive('post').with_args(
         'https://example.com/start', data=''.encode('utf-8')
-    )
+    ).and_return(flexmock(ok=True))
 
     module.ping_monitor(
         hook_config,
@@ -245,13 +245,34 @@ def test_ping_monitor_hits_ping_url_when_states_matching():
     )
 
 
-def test_ping_monitor_with_connection_error_does_not_raise():
+def test_ping_monitor_with_connection_error_logs_warning():
     flexmock(module).should_receive('Forgetful_buffering_handler')
-    flexmock(module.logger).should_receive('warning')
     hook_config = {'ping_url': 'https://example.com'}
     flexmock(module.requests).should_receive('post').with_args(
         'https://example.com/start', data=''.encode('utf-8')
     ).and_raise(module.requests.exceptions.ConnectionError)
+    flexmock(module.logger).should_receive('warning').once()
+
+    module.ping_monitor(
+        hook_config,
+        'config.yaml',
+        state=module.monitor.State.START,
+        monitoring_log_level=1,
+        dry_run=False,
+    )
+
+
+def test_ping_monitor_with_other_error_logs_warning():
+    flexmock(module).should_receive('Forgetful_buffering_handler')
+    hook_config = {'ping_url': 'https://example.com'}
+    response = flexmock(ok=False)
+    response.should_receive('raise_for_status').and_raise(
+        module.requests.exceptions.RequestException
+    )
+    flexmock(module.requests).should_receive('post').with_args(
+        'https://example.com/start', data=''.encode('utf-8')
+    ).and_return(response)
+    flexmock(module.logger).should_receive('warning').once()
 
     module.ping_monitor(
         hook_config,

tests/unit/hooks/test_ntfy.py

@@ -38,7 +38,7 @@ def test_ping_monitor_minimal_config_hits_hosted_ntfy_on_fail():
     flexmock(module.requests).should_receive('post').with_args(
         f'{default_base_url}/{topic}',
         headers=return_default_message_headers(module.monitor.State.FAIL),
-    ).once()
+    ).and_return(flexmock(ok=True)).once()
 
     module.ping_monitor(
         hook_config, 'config.yaml', module.monitor.State.FAIL, monitoring_log_level=1, dry_run=False
@@ -76,7 +76,7 @@ def test_ping_monitor_minimal_config_hits_selfhosted_ntfy_on_fail():
     flexmock(module.requests).should_receive('post').with_args(
         f'{custom_base_url}/{topic}',
         headers=return_default_message_headers(module.monitor.State.FAIL),
-    ).once()
+    ).and_return(flexmock(ok=True)).once()
 
     module.ping_monitor(
         hook_config, 'config.yaml', module.monitor.State.FAIL, monitoring_log_level=1, dry_run=False
@@ -96,7 +96,7 @@ def test_ping_monitor_custom_message_hits_hosted_ntfy_on_fail():
     hook_config = {'topic': topic, 'fail': custom_message_config}
     flexmock(module.requests).should_receive('post').with_args(
         f'{default_base_url}/{topic}', headers=custom_message_headers,
-    ).once()
+    ).and_return(flexmock(ok=True)).once()
 
     module.ping_monitor(
         hook_config, 'config.yaml', module.monitor.State.FAIL, monitoring_log_level=1, dry_run=False
@@ -108,7 +108,7 @@ def test_ping_monitor_custom_state_hits_hosted_ntfy_on_start():
     flexmock(module.requests).should_receive('post').with_args(
         f'{default_base_url}/{topic}',
         headers=return_default_message_headers(module.monitor.State.START),
-    ).once()
+    ).and_return(flexmock(ok=True)).once()
 
     module.ping_monitor(
         hook_config,
@@ -119,12 +119,34 @@ def test_ping_monitor_custom_state_hits_hosted_ntfy_on_start():
     )
 
 
-def test_ping_monitor_with_connection_error_does_not_raise():
+def test_ping_monitor_with_connection_error_logs_warning():
     hook_config = {'topic': topic}
     flexmock(module.requests).should_receive('post').with_args(
         f'{default_base_url}/{topic}',
         headers=return_default_message_headers(module.monitor.State.FAIL),
     ).and_raise(module.requests.exceptions.ConnectionError)
+    flexmock(module.logger).should_receive('warning').once()
+
+    module.ping_monitor(
+        hook_config,
+        'config.yaml',
+        module.monitor.State.FAIL,
+        monitoring_log_level=1,
+        dry_run=False,
+    )
+
+
+def test_ping_monitor_with_other_error_logs_warning():
+    hook_config = {'topic': topic}
+    response = flexmock(ok=False)
+    response.should_receive('raise_for_status').and_raise(
+        module.requests.exceptions.RequestException
+    )
+    flexmock(module.requests).should_receive('post').with_args(
+        f'{default_base_url}/{topic}',
+        headers=return_default_message_headers(module.monitor.State.FAIL),
+    ).and_return(response)
+    flexmock(module.logger).should_receive('warning').once()
 
     module.ping_monitor(
         hook_config,

tests/unit/hooks/test_pagerduty.py

@@ -28,7 +28,7 @@ def test_ping_monitor_ignores_finish_state():
 
 
 def test_ping_monitor_calls_api_for_fail_state():
-    flexmock(module.requests).should_receive('post')
+    flexmock(module.requests).should_receive('post').and_return(flexmock(ok=True))
 
     module.ping_monitor(
         {'integration_key': 'abc123'},
@@ -51,10 +51,27 @@ def test_ping_monitor_dry_run_does_not_call_api():
     )
 
 
-def test_ping_monitor_with_connection_error_does_not_raise():
+def test_ping_monitor_with_connection_error_logs_warning():
     flexmock(module.requests).should_receive('post').and_raise(
         module.requests.exceptions.ConnectionError
     )
+    flexmock(module.logger).should_receive('warning').once()
+
+    module.ping_monitor(
+        {'integration_key': 'abc123'},
+        'config.yaml',
+        module.monitor.State.FAIL,
+        monitoring_log_level=1,
+        dry_run=False,
+    )
+
+
+def test_ping_monitor_with_other_error_logs_warning():
+    response = flexmock(ok=False)
+    response.should_receive('raise_for_status').and_raise(
+        module.requests.exceptions.RequestException
+    )
+    flexmock(module.requests).should_receive('post').and_return(response)
     flexmock(module.logger).should_receive('warning')
 
     module.ping_monitor(