From 10723efc68edacb97f9c9bba0fa77c35387b5545 Mon Sep 17 00:00:00 2001 From: Dan Helfman Date: Wed, 29 Jun 2022 21:19:40 -0700 Subject: [PATCH] Fix all monitoring hooks to warn if the server returns an HTTP 4xx error (#554). --- NEWS | 2 ++ borgmatic/hooks/cronhub.py | 4 ++- borgmatic/hooks/cronitor.py | 4 ++- borgmatic/hooks/healthchecks.py | 4 ++- borgmatic/hooks/ntfy.py | 4 ++- borgmatic/hooks/pagerduty.py | 4 ++- tests/unit/hooks/test_cronhub.py | 39 +++++++++++++++++++++++---- tests/unit/hooks/test_cronitor.py | 35 +++++++++++++++++++++--- tests/unit/hooks/test_healthchecks.py | 35 +++++++++++++++++++----- tests/unit/hooks/test_ntfy.py | 32 ++++++++++++++++++---- tests/unit/hooks/test_pagerduty.py | 21 +++++++++++++-- 11 files changed, 156 insertions(+), 28 deletions(-) diff --git a/NEWS b/NEWS index 8c2dc6e2..58df9bbd 100644 --- a/NEWS +++ b/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 diff --git a/borgmatic/hooks/cronhub.py b/borgmatic/hooks/cronhub.py index 48323471..b93788e4 100644 --- a/borgmatic/hooks/cronhub.py +++ b/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}') diff --git a/borgmatic/hooks/cronitor.py b/borgmatic/hooks/cronitor.py index 43f17792..8866a6ac 100644 --- a/borgmatic/hooks/cronitor.py +++ b/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}') diff --git a/borgmatic/hooks/healthchecks.py b/borgmatic/hooks/healthchecks.py index 7ac04e28..c801f182 100644 --- a/borgmatic/hooks/healthchecks.py +++ b/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}') diff --git a/borgmatic/hooks/ntfy.py b/borgmatic/hooks/ntfy.py index 54e89f05..c62b5110 100644 --- a/borgmatic/hooks/ntfy.py +++ b/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}') diff --git a/borgmatic/hooks/pagerduty.py b/borgmatic/hooks/pagerduty.py index 20fc771e..fbb67fbf 100644 --- a/borgmatic/hooks/pagerduty.py +++ b/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}') diff --git a/tests/unit/hooks/test_cronhub.py b/tests/unit/hooks/test_cronhub.py index 54bd3db9..14e8eb23 100644 --- a/tests/unit/hooks/test_cronhub.py +++ b/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, diff --git a/tests/unit/hooks/test_cronitor.py b/tests/unit/hooks/test_cronitor.py index 5c258ae2..4b762d85 100644 --- a/tests/unit/hooks/test_cronitor.py +++ b/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, diff --git a/tests/unit/hooks/test_healthchecks.py b/tests/unit/hooks/test_healthchecks.py index 9e7cda62..65c5613d 100644 --- a/tests/unit/hooks/test_healthchecks.py +++ b/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, diff --git a/tests/unit/hooks/test_ntfy.py b/tests/unit/hooks/test_ntfy.py index ec891368..3867cee1 100644 --- a/tests/unit/hooks/test_ntfy.py +++ b/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, diff --git a/tests/unit/hooks/test_pagerduty.py b/tests/unit/hooks/test_pagerduty.py index 3d8589f0..0fccae00 100644 --- a/tests/unit/hooks/test_pagerduty.py +++ b/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(