From f31b22f2e74189f75882d29ce50a43a0e624e57e Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Fri, 10 Apr 2026 12:39:36 +0800 Subject: [PATCH 1/2] mgr/dashboard: return 400 for all tracker API errors in feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The feedback create endpoint previously classified most tracker API errors as 500 (Internal Server Error) and relied on fragile string matching to detect the single "Invalid issue tracker API key" case for a 400 response. This caused intermittent test failures (and misleading errors for users) whenever tracker.ceph.com returned something other than a plain 401 — e.g., 5xx, forbidden, or a network error. Because the error message would not match the magic string and the code would fall through to a hardcoded 500. Any failure from an upstream API is by definition not an internal server error. Surface all RuntimeError from the feedback module as a 400 DashboardException, letting the caller see the underlying tracker error message. Signed-off-by: Kefu Chai --- src/pybind/mgr/dashboard/controllers/feedback.py | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/pybind/mgr/dashboard/controllers/feedback.py b/src/pybind/mgr/dashboard/controllers/feedback.py index c75ffa94a89..c1b51dca3f8 100644 --- a/src/pybind/mgr/dashboard/controllers/feedback.py +++ b/src/pybind/mgr/dashboard/controllers/feedback.py @@ -38,14 +38,11 @@ class FeedbackController(RESTController): response = mgr.remote('feedback', 'validate_and_create_issue', project, tracker, subject, description, api_key) except RuntimeError as error: - if "Invalid issue tracker API key" in str(error): - raise DashboardException(msg='Error in creating tracker issue: Invalid API key', - component='feedback') - if "KeyError" in str(error): - raise DashboardException(msg=f'Error in creating tracker issue: {error}', - component='feedback') - raise DashboardException(msg=f'{error}', - http_status_code=500, + # Any failure from the tracker API (invalid key, network error, + # upstream 5xx, etc.) is a client/upstream error, not an internal + # server error. Surface it as 400 via DashboardException's default + # status code. + raise DashboardException(msg=f'Error in creating tracker issue: {error}', component='feedback') return response From 69eff2d74b4219540fc8877e103b662f4e974374 Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Fri, 10 Apr 2026 12:41:00 +0800 Subject: [PATCH 2/2] mgr/feedback: make tracker URL configurable and stop hitting live tracker in tests The feedback module hard-coded tracker.ceph.com as the Ceph issue tracker hostname. This meant the dashboard test test_issue_tracker_create_with_invalid_key actually performed an HTTPS POST against the live production tracker on every CI run. Whenever tracker.ceph.com returned an unexpected response (5xx, gateway error, transient failure), the test would fail on completely unrelated pull requests. Add a runtime module option 'mgr/feedback/tracker_url' that controls which host the CephTrackerClient talks to. The default keeps the existing behaviour (tracker.ceph.com). The dashboard test now sets this option to an unreachable hostname in setUpClass so create_issue fails fast with a ConnectionError instead of depending on tracker.ceph.com being reachable. Combined with the prior commit's error-handling fix, the test now deterministically verifies that any tracker failure is surfaced as HTTP 400, without requiring network access. Signed-off-by: Kefu Chai --- qa/tasks/mgr/dashboard/test_feedback.py | 8 ++++++++ src/pybind/mgr/feedback/module.py | 19 ++++++++++++++----- src/pybind/mgr/feedback/service.py | 12 +++++++----- 3 files changed, 29 insertions(+), 10 deletions(-) diff --git a/qa/tasks/mgr/dashboard/test_feedback.py b/qa/tasks/mgr/dashboard/test_feedback.py index 1a5a66154e1..a395adf4865 100644 --- a/qa/tasks/mgr/dashboard/test_feedback.py +++ b/qa/tasks/mgr/dashboard/test_feedback.py @@ -7,6 +7,14 @@ class FeedbackTest(MgrModuleTestCase): def setUpClass(cls): super().setUpClass() cls._ceph_cmd(['mgr', 'module', 'enable', 'feedback'], wait=3) + # Point the feedback module at an unreachable host so the test + # does not depend on tracker.ceph.com being available. Any + # create_issue call will fail fast with a ConnectionError + # (a RequestException subclass) which the dashboard controller + # is expected to surface as HTTP 400. + cls._ceph_cmd(['config', 'set', 'mgr', + 'mgr/feedback/tracker_url', + 'invalid.example.invalid']) cls._get( '/api/mgr/module', retries=5, diff --git a/src/pybind/mgr/feedback/module.py b/src/pybind/mgr/feedback/module.py index 6c76e0d5a92..9d1e0ae66f1 100644 --- a/src/pybind/mgr/feedback/module.py +++ b/src/pybind/mgr/feedback/module.py @@ -9,7 +9,7 @@ from requests.exceptions import RequestException from .cli import FeedbackCLICommand -from mgr_module import HandleCommandResult, MgrModule +from mgr_module import HandleCommandResult, MgrModule, Option import errno from .service import CephTrackerClient @@ -19,6 +19,15 @@ from .model import Feedback class FeedbackModule(MgrModule): CLICommand = FeedbackCLICommand + MODULE_OPTIONS = [ + Option( + name='tracker_url', + type='str', + default='tracker.ceph.com', + desc='Hostname of the Ceph issue tracker (Redmine) instance', + runtime=True), + ] + # there are CLI commands we implement @FeedbackCLICommand.Read('feedback set api-key') def _cmd_feedback_set_api_key(self, key: str) -> HandleCommandResult: @@ -60,7 +69,7 @@ class FeedbackModule(MgrModule): """ Fetch issue list """ - tracker_client = CephTrackerClient() + tracker_client = CephTrackerClient(self.get_module_option('tracker_url')) try: response = tracker_client.list_issues() except Exception: @@ -83,7 +92,7 @@ class FeedbackModule(MgrModule): return HandleCommandResult(stderr='Issue tracker key is not set. Set key with `ceph set issue_key `') except Exception as error: return HandleCommandResult(stderr=f'Error in retreiving issue tracker API key: {error}') - tracker_client = CephTrackerClient() + tracker_client = CephTrackerClient(self.get_module_option('tracker_url')) try: response = tracker_client.create_issue(feedback, current_api_key) except RequestException as error: @@ -121,13 +130,13 @@ class FeedbackModule(MgrModule): return 'Successfully deleted API key' def get_issues(self): - tracker_client = CephTrackerClient() + tracker_client = CephTrackerClient(self.get_module_option('tracker_url')) return tracker_client.list_issues() def validate_and_create_issue(self, project: str, tracker: str, subject: str, description: str, api_key=None): feedback = Feedback(Feedback.Project[project].value, Feedback.TrackerType[tracker].value, subject, description) - tracker_client = CephTrackerClient() + tracker_client = CephTrackerClient(self.get_module_option('tracker_url')) stored_api_key = self.get_store('api_key') try: if api_key: diff --git a/src/pybind/mgr/feedback/service.py b/src/pybind/mgr/feedback/service.py index dc8c6b64a6d..75db80f8a3d 100644 --- a/src/pybind/mgr/feedback/service.py +++ b/src/pybind/mgr/feedback/service.py @@ -6,12 +6,14 @@ from requests.exceptions import RequestException from .model import Feedback -class config: - url = 'tracker.ceph.com' - port = 443 +DEFAULT_TRACKER_URL = 'tracker.ceph.com' + class CephTrackerClient(): + def __init__(self, tracker_url: str = DEFAULT_TRACKER_URL): + self.tracker_url = tracker_url + def list_issues(self): ''' Fetch an issue from the Ceph Issue tracker @@ -20,7 +22,7 @@ class CephTrackerClient(): 'Content-Type': 'application/json', } response = requests.get( - f'https://{config.url}/issues.json', headers=headers) + f'https://{self.tracker_url}/issues.json', headers=headers) if not response.ok: if response.status_code == 404: raise FileNotFoundError @@ -40,7 +42,7 @@ class CephTrackerClient(): raise Exception("Ceph Tracker API Key not set") data = json.dumps(feedback.as_dict()) response = requests.post( - f'https://{config.url}/projects/{feedback.project_id}/issues.json', + f'https://{self.tracker_url}/projects/{feedback.project_id}/issues.json', headers=headers, data=data) if not response.ok: if response.status_code == 401: