From 48f28e5fcaf180ed1a17131b5ac2856e9be8a2a2 Mon Sep 17 00:00:00 2001 From: Carlos Date: Wed, 24 Dec 2025 13:27:39 +0100 Subject: [PATCH 1/2] fix: Show user-friendly messages for GCP resource exhaustion errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #901 When GCP fails to create a VM (e.g., due to zone resource exhaustion), the error was logged but the test status showed a raw error dict that was not user-friendly. Changes: - Added parse_gcp_error() helper function to extract meaningful messages from GCP API error responses - Added GCP_ERROR_MESSAGES dict mapping known error codes to user-friendly messages (ZONE_RESOURCE_POOL_EXHAUSTED, QUOTA_EXCEEDED, TIMEOUT, etc.) - Updated start_test() to use parse_gcp_error() when VM creation fails - For unknown errors, shows the error code and a truncated message Now users see messages like: "GCP resources temporarily unavailable in the configured zone. The test will be retried automatically when resources become available." Instead of raw dicts like: "{'errors': [{'code': 'ZONE_RESOURCE_POOL_EXHAUSTED', ...}]}" 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- mod_ci/controllers.py | 65 ++++++++++++++- tests/test_ci/test_controllers.py | 130 ++++++++++++++++++++++++++++++ 2 files changed, 193 insertions(+), 2 deletions(-) diff --git a/mod_ci/controllers.py b/mod_ci/controllers.py index e60c0e0d..ef366b6b 100755 --- a/mod_ci/controllers.py +++ b/mod_ci/controllers.py @@ -122,6 +122,67 @@ def safe_db_commit(db, operation_description: str = "database operation") -> boo return False +# User-friendly messages for known GCP error codes +GCP_ERROR_MESSAGES = { + 'ZONE_RESOURCE_POOL_EXHAUSTED': ( + "GCP resources temporarily unavailable in the configured zone. " + "The test will be retried automatically when resources become available." + ), + 'QUOTA_EXCEEDED': ( + "GCP quota limit reached. Please wait for other tests to complete " + "or contact the administrator." + ), + 'RESOURCE_NOT_FOUND': "Required GCP resource not found. Please contact the administrator.", + 'RESOURCE_ALREADY_EXISTS': "A VM with this name already exists. Please contact the administrator.", + 'TIMEOUT': "GCP operation timed out. The test will be retried automatically.", +} + + +def parse_gcp_error(result: Dict) -> str: + """ + Parse a GCP API error response and return a user-friendly message. + + GCP errors have the structure: + { + 'error': { + 'errors': [{'code': 'ERROR_CODE', 'message': '...'}] + } + } + + :param result: The GCP API response dictionary + :return: A user-friendly error message + """ + if not isinstance(result, dict): + return f"Unknown error: {result}" + + error = result.get('error') + if error is None: + return "Unknown error (no error details provided)" + + if not isinstance(error, dict): + return f"Unknown error: {error}" + + errors = error.get('errors', []) + if not errors: + return f"Unknown error: {error}" + + # Get the first error (usually the most relevant) + first_error = errors[0] if isinstance(errors, list) and len(errors) > 0 else {} + error_code = first_error.get('code', 'UNKNOWN') + error_message = first_error.get('message', 'No details provided') + + # Check if we have a user-friendly message for this error code + if error_code in GCP_ERROR_MESSAGES: + return GCP_ERROR_MESSAGES[error_code] + + # For unknown errors, return a cleaned-up version of the original message + # Truncate very long messages + if len(error_message) > 200: + error_message = error_message[:200] + "..." + + return f"{error_code}: {error_message}" + + mod_ci = Blueprint('ci', __name__) @@ -782,9 +843,9 @@ def start_test(compute, app, db, repository: Repository.Repository, test, bot_to if not safe_db_commit(db, f"recording GCP instance for test {test.id}"): log.error(f"Failed to record GCP instance for test {test.id}, but VM was created") else: - error_msg = result.get('error', 'Unknown error') if isinstance(result, dict) else str(result) + error_msg = parse_gcp_error(result) log.error(f"Error creating test instance for test {test.id}, result: {result}") - mark_test_failed(db, test, repository, f"Failed to create VM: {error_msg}") + mark_test_failed(db, test, repository, error_msg) def create_instance(compute, project, zone, test, reportURL) -> Dict: diff --git a/tests/test_ci/test_controllers.py b/tests/test_ci/test_controllers.py index d9ed6152..c4a8647c 100644 --- a/tests/test_ci/test_controllers.py +++ b/tests/test_ci/test_controllers.py @@ -1,4 +1,5 @@ import json +import unittest from importlib import reload from unittest import mock from unittest.mock import MagicMock @@ -3092,3 +3093,132 @@ def test_mark_test_failed_includes_target_url(self, mock_progress, mock_update_g call_args = mock_update_github.call_args self.assertEqual(len(call_args[0]), 5) # 5 positional args including target_url self.assertIn("456", call_args[0][4]) # target_url contains test ID + + +class TestParseGcpError(unittest.TestCase): + """Tests for the parse_gcp_error helper function.""" + + def test_parse_gcp_error_zone_resource_exhausted(self): + """Test that ZONE_RESOURCE_POOL_EXHAUSTED returns user-friendly message.""" + from mod_ci.controllers import parse_gcp_error + + result = { + 'status': 'DONE', + 'error': { + 'errors': [{ + 'code': 'ZONE_RESOURCE_POOL_EXHAUSTED', + 'message': "The zone 'projects/test/zones/us-central1-a' does not " + "have enough resources available to fulfill the request." + }] + } + } + + error_msg = parse_gcp_error(result) + self.assertIn("GCP resources temporarily unavailable", error_msg) + self.assertIn("retried automatically", error_msg) + # Should NOT contain raw technical details + self.assertNotIn("us-central1-a", error_msg) + + def test_parse_gcp_error_quota_exceeded(self): + """Test that QUOTA_EXCEEDED returns user-friendly message.""" + from mod_ci.controllers import parse_gcp_error + + result = { + 'error': { + 'errors': [{ + 'code': 'QUOTA_EXCEEDED', + 'message': 'Quota exceeded for resource.' + }] + } + } + + error_msg = parse_gcp_error(result) + self.assertIn("quota limit reached", error_msg) + + def test_parse_gcp_error_timeout(self): + """Test that TIMEOUT returns user-friendly message.""" + from mod_ci.controllers import parse_gcp_error + + result = { + 'status': 'TIMEOUT', + 'error': { + 'errors': [{ + 'code': 'TIMEOUT', + 'message': 'Operation timed out after 1800 seconds' + }] + } + } + + error_msg = parse_gcp_error(result) + self.assertIn("timed out", error_msg) + self.assertIn("retried automatically", error_msg) + + def test_parse_gcp_error_unknown_code(self): + """Test that unknown error codes return the code and message.""" + from mod_ci.controllers import parse_gcp_error + + result = { + 'error': { + 'errors': [{ + 'code': 'SOME_NEW_ERROR', + 'message': 'Something unexpected happened.' + }] + } + } + + error_msg = parse_gcp_error(result) + self.assertIn("SOME_NEW_ERROR", error_msg) + self.assertIn("Something unexpected happened", error_msg) + + def test_parse_gcp_error_long_message_truncated(self): + """Test that very long error messages are truncated.""" + from mod_ci.controllers import parse_gcp_error + + long_message = "A" * 300 # 300 characters + result = { + 'error': { + 'errors': [{ + 'code': 'UNKNOWN_ERROR', + 'message': long_message + }] + } + } + + error_msg = parse_gcp_error(result) + self.assertLess(len(error_msg), 250) # Should be truncated + self.assertIn("...", error_msg) + + def test_parse_gcp_error_no_error_key(self): + """Test handling when 'error' key is missing.""" + from mod_ci.controllers import parse_gcp_error + + result = {'status': 'DONE'} + + error_msg = parse_gcp_error(result) + self.assertIn("Unknown error", error_msg) + + def test_parse_gcp_error_empty_errors_list(self): + """Test handling when 'errors' list is empty.""" + from mod_ci.controllers import parse_gcp_error + + result = {'error': {'errors': []}} + + error_msg = parse_gcp_error(result) + self.assertIn("Unknown error", error_msg) + + def test_parse_gcp_error_not_a_dict(self): + """Test handling when result is not a dictionary.""" + from mod_ci.controllers import parse_gcp_error + + error_msg = parse_gcp_error("some string error") + self.assertIn("Unknown error", error_msg) + self.assertIn("some string error", error_msg) + + def test_parse_gcp_error_error_not_a_dict(self): + """Test handling when 'error' value is not a dictionary.""" + from mod_ci.controllers import parse_gcp_error + + result = {'error': 'just a string'} + + error_msg = parse_gcp_error(result) + self.assertIn("Unknown error", error_msg) From e7a213734ce5a016cbf17ba42aa32965ad50d210 Mon Sep 17 00:00:00 2001 From: Carlos Date: Wed, 24 Dec 2025 13:50:29 +0100 Subject: [PATCH 2/2] fix: Log GCP error details server-side, return generic message to users MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review feedback: For unknown GCP error codes, the raw error message could contain sensitive info (project names, zones, etc). Changes: - Log full error details server-side for debugging - Return generic "VM creation failed" message to users for unknown errors - Known error codes (ZONE_RESOURCE_POOL_EXHAUSTED, etc.) still return user-friendly messages - Added optional `log` parameter for testability - Updated tests to verify sensitive info is logged but not returned 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- mod_ci/controllers.py | 33 ++++++++++----- tests/test_ci/test_controllers.py | 70 +++++++++++++++++++++---------- 2 files changed, 70 insertions(+), 33 deletions(-) diff --git a/mod_ci/controllers.py b/mod_ci/controllers.py index ef366b6b..58e142f4 100755 --- a/mod_ci/controllers.py +++ b/mod_ci/controllers.py @@ -138,7 +138,7 @@ def safe_db_commit(db, operation_description: str = "database operation") -> boo } -def parse_gcp_error(result: Dict) -> str: +def parse_gcp_error(result: Dict, log=None) -> str: """ Parse a GCP API error response and return a user-friendly message. @@ -149,22 +149,35 @@ def parse_gcp_error(result: Dict) -> str: } } + For known error codes, returns a user-friendly message. + For unknown errors, logs the details server-side and returns a generic message + to avoid exposing potentially sensitive information. + :param result: The GCP API response dictionary + :param log: Optional logger instance. If not provided, uses module logger. :return: A user-friendly error message """ + import logging + if log is None: + log = logging.getLogger('Platform') + if not isinstance(result, dict): - return f"Unknown error: {result}" + log.error(f"GCP error (non-dict): {result}") + return "VM creation failed. Please contact the administrator." error = result.get('error') if error is None: - return "Unknown error (no error details provided)" + log.error(f"GCP error (no error key): {result}") + return "VM creation failed. Please contact the administrator." if not isinstance(error, dict): - return f"Unknown error: {error}" + log.error(f"GCP error (error not dict): {error}") + return "VM creation failed. Please contact the administrator." errors = error.get('errors', []) if not errors: - return f"Unknown error: {error}" + log.error(f"GCP error (empty errors list): {error}") + return "VM creation failed. Please contact the administrator." # Get the first error (usually the most relevant) first_error = errors[0] if isinstance(errors, list) and len(errors) > 0 else {} @@ -175,12 +188,10 @@ def parse_gcp_error(result: Dict) -> str: if error_code in GCP_ERROR_MESSAGES: return GCP_ERROR_MESSAGES[error_code] - # For unknown errors, return a cleaned-up version of the original message - # Truncate very long messages - if len(error_message) > 200: - error_message = error_message[:200] + "..." - - return f"{error_code}: {error_message}" + # For unknown errors, log full details server-side but return generic message + # to avoid exposing potentially sensitive information (project names, zones, etc.) + log.error(f"GCP error ({error_code}): {error_message}") + return f"VM creation failed ({error_code}). Please contact the administrator." mod_ci = Blueprint('ci', __name__) diff --git a/tests/test_ci/test_controllers.py b/tests/test_ci/test_controllers.py index c4a8647c..59db5bc2 100644 --- a/tests/test_ci/test_controllers.py +++ b/tests/test_ci/test_controllers.py @@ -3102,6 +3102,7 @@ def test_parse_gcp_error_zone_resource_exhausted(self): """Test that ZONE_RESOURCE_POOL_EXHAUSTED returns user-friendly message.""" from mod_ci.controllers import parse_gcp_error + mock_log = MagicMock() result = { 'status': 'DONE', 'error': { @@ -3113,7 +3114,7 @@ def test_parse_gcp_error_zone_resource_exhausted(self): } } - error_msg = parse_gcp_error(result) + error_msg = parse_gcp_error(result, log=mock_log) self.assertIn("GCP resources temporarily unavailable", error_msg) self.assertIn("retried automatically", error_msg) # Should NOT contain raw technical details @@ -3123,6 +3124,7 @@ def test_parse_gcp_error_quota_exceeded(self): """Test that QUOTA_EXCEEDED returns user-friendly message.""" from mod_ci.controllers import parse_gcp_error + mock_log = MagicMock() result = { 'error': { 'errors': [{ @@ -3132,13 +3134,14 @@ def test_parse_gcp_error_quota_exceeded(self): } } - error_msg = parse_gcp_error(result) + error_msg = parse_gcp_error(result, log=mock_log) self.assertIn("quota limit reached", error_msg) def test_parse_gcp_error_timeout(self): """Test that TIMEOUT returns user-friendly message.""" from mod_ci.controllers import parse_gcp_error + mock_log = MagicMock() result = { 'status': 'TIMEOUT', 'error': { @@ -3149,14 +3152,15 @@ def test_parse_gcp_error_timeout(self): } } - error_msg = parse_gcp_error(result) + error_msg = parse_gcp_error(result, log=mock_log) self.assertIn("timed out", error_msg) self.assertIn("retried automatically", error_msg) def test_parse_gcp_error_unknown_code(self): - """Test that unknown error codes return the code and message.""" + """Test that unknown error codes return generic message and log details.""" from mod_ci.controllers import parse_gcp_error + mock_log = MagicMock() result = { 'error': { 'errors': [{ @@ -3166,59 +3170,81 @@ def test_parse_gcp_error_unknown_code(self): } } - error_msg = parse_gcp_error(result) + error_msg = parse_gcp_error(result, log=mock_log) + # Should include error code but not the full message (security) self.assertIn("SOME_NEW_ERROR", error_msg) - self.assertIn("Something unexpected happened", error_msg) + self.assertIn("contact the administrator", error_msg) + # Should NOT expose the raw error message + self.assertNotIn("Something unexpected happened", error_msg) + # Should log the full details server-side + mock_log.error.assert_called_once() + self.assertIn("SOME_NEW_ERROR", mock_log.error.call_args[0][0]) + self.assertIn("Something unexpected happened", mock_log.error.call_args[0][0]) - def test_parse_gcp_error_long_message_truncated(self): - """Test that very long error messages are truncated.""" + def test_parse_gcp_error_logs_sensitive_info(self): + """Test that sensitive info is logged but not returned to user.""" from mod_ci.controllers import parse_gcp_error - long_message = "A" * 300 # 300 characters + mock_log = MagicMock() result = { 'error': { 'errors': [{ 'code': 'UNKNOWN_ERROR', - 'message': long_message + 'message': 'Error in project my-secret-project zone us-central1-a' }] } } - error_msg = parse_gcp_error(result) - self.assertLess(len(error_msg), 250) # Should be truncated - self.assertIn("...", error_msg) + error_msg = parse_gcp_error(result, log=mock_log) + # Should NOT expose project/zone names + self.assertNotIn("my-secret-project", error_msg) + self.assertNotIn("us-central1-a", error_msg) + # But should log them server-side + mock_log.error.assert_called_once() + self.assertIn("my-secret-project", mock_log.error.call_args[0][0]) def test_parse_gcp_error_no_error_key(self): """Test handling when 'error' key is missing.""" from mod_ci.controllers import parse_gcp_error + mock_log = MagicMock() result = {'status': 'DONE'} - error_msg = parse_gcp_error(result) - self.assertIn("Unknown error", error_msg) + error_msg = parse_gcp_error(result, log=mock_log) + self.assertIn("VM creation failed", error_msg) + mock_log.error.assert_called_once() def test_parse_gcp_error_empty_errors_list(self): """Test handling when 'errors' list is empty.""" from mod_ci.controllers import parse_gcp_error + mock_log = MagicMock() result = {'error': {'errors': []}} - error_msg = parse_gcp_error(result) - self.assertIn("Unknown error", error_msg) + error_msg = parse_gcp_error(result, log=mock_log) + self.assertIn("VM creation failed", error_msg) + mock_log.error.assert_called_once() def test_parse_gcp_error_not_a_dict(self): """Test handling when result is not a dictionary.""" from mod_ci.controllers import parse_gcp_error - error_msg = parse_gcp_error("some string error") - self.assertIn("Unknown error", error_msg) - self.assertIn("some string error", error_msg) + mock_log = MagicMock() + error_msg = parse_gcp_error("some string error", log=mock_log) + self.assertIn("VM creation failed", error_msg) + # Should NOT expose the raw input + self.assertNotIn("some string error", error_msg) + # But should log it + mock_log.error.assert_called_once() + self.assertIn("some string error", mock_log.error.call_args[0][0]) def test_parse_gcp_error_error_not_a_dict(self): """Test handling when 'error' value is not a dictionary.""" from mod_ci.controllers import parse_gcp_error + mock_log = MagicMock() result = {'error': 'just a string'} - error_msg = parse_gcp_error(result) - self.assertIn("Unknown error", error_msg) + error_msg = parse_gcp_error(result, log=mock_log) + self.assertIn("VM creation failed", error_msg) + mock_log.error.assert_called_once()