-
Notifications
You must be signed in to change notification settings - Fork 2
אישור שליחת הודעת אדמין #3071
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
אישור שליחת הודעת אדמין #3071
Changes from all commits
1cc1202
4ce8bf2
9c19b0a
5ea7363
b663ed5
aff9cd9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -756,6 +756,190 @@ async def notify_admins(context: ContextTypes.DEFAULT_TYPE, text: str) -> bool: | |
| return False | ||
|
|
||
|
|
||
| def _get_alertmanager_api_alerts_url() -> str | None: | ||
| """בונה URL ל-Alertmanager API עבור POST /api/v2/alerts. | ||
|
|
||
| תומך ב-2 מצבים: | ||
| - ALERTMANAGER_API_URL: URL בסיס מלא (כולל scheme), לדוגמה: https://code-keeper-alertmanager.onrender.com | ||
| - ALERTMANAGER_TARGET: host:port (כמו בפריסת Prometheus), לדוגמה: code-keeper-alertmanager.onrender.com:443 | ||
| """ | ||
| raw = (os.getenv("ALERTMANAGER_API_URL") or os.getenv("ALERTMANAGER_TARGET") or "").strip() | ||
| if not raw: | ||
| return None | ||
|
|
||
| base = raw | ||
| if "://" not in base: | ||
| # ניחוש scheme: 443 => https, אחרת נוטים ל-http (למשל docker-compose: alertmanager:9093) | ||
| scheme = "https" | ||
| try: | ||
| low = base.lower() | ||
| if low.endswith(":443"): | ||
| scheme = "https" | ||
| elif low.endswith(":9093") or low.endswith(":80") or low.startswith("localhost") or low.startswith("127.") or low.startswith("alertmanager"): | ||
| scheme = "http" | ||
| else: | ||
| # ברירת מחדל סבירה לסביבות ענן | ||
| scheme = "https" | ||
| except Exception: | ||
| scheme = "https" | ||
| base = f"{scheme}://{base}" | ||
|
|
||
| base = base.rstrip("/") | ||
| return f"{base}/api/v2/alerts" | ||
|
|
||
|
|
||
| async def _send_admin_report_via_alertmanager( | ||
| report_text: str, | ||
| *, | ||
| user_id: Any = None, | ||
| username: Any = None, | ||
| display: Any = None, | ||
| ) -> bool: | ||
| """שולח דיווח ל-Alertmanager ומחזיר True רק אם התקבלה תשובת 2xx. | ||
|
|
||
| חשוב: זה "אישור קבלה" מצד Alertmanager (accepted), לא הבטחה שההודעה הגיעה ליעד הסופי. | ||
| """ | ||
| url = _get_alertmanager_api_alerts_url() | ||
| if not url: | ||
| return False | ||
|
|
||
| if not report_text: | ||
| return False | ||
|
|
||
| def _truncate(s: Any, limit: int = 400) -> str: | ||
| try: | ||
| text = str(s or "") | ||
| except Exception: | ||
| text = "" | ||
| text = text.strip() | ||
| if limit and len(text) > limit: | ||
| return text[: max(0, limit - 1)] + "…" | ||
| return text | ||
|
|
||
| # Alertmanager expects a list of alerts | ||
| now = datetime.now(timezone.utc).isoformat() | ||
| alert = { | ||
| "labels": { | ||
| "alertname": "AdminUserReport", | ||
| "severity": "info", | ||
| "component": "bot", | ||
| }, | ||
| "annotations": { | ||
| "summary": _truncate(report_text, 1800), | ||
| "user_id": _truncate(user_id, 80), | ||
| "username": _truncate(username, 120), | ||
| "user_display": _truncate(display, 200), | ||
| }, | ||
| "startsAt": now, | ||
| } | ||
|
|
||
| headers: dict[str, str] = {"Content-Type": "application/json"} | ||
| token = (os.getenv("ALERTMANAGER_API_TOKEN") or "").strip() | ||
| if token: | ||
| headers["Authorization"] = f"Bearer {token}" | ||
|
|
||
| last_err: str | None = None | ||
|
|
||
| # Prefer the project's resilient async HTTP helper (aiohttp + retries/circuit breaker). | ||
| try: | ||
| try: | ||
| from http_async import request as http_request # type: ignore | ||
| except Exception: | ||
| http_request = None # type: ignore | ||
|
|
||
| # aiohttp דורש ClientTimeout, לא int. אם aiohttp לא זמין, לא נעביר timeout כלל. | ||
| timeout_obj = None | ||
| try: | ||
| import aiohttp # type: ignore | ||
|
|
||
| timeout_obj = aiohttp.ClientTimeout(total=6) # type: ignore[attr-defined] | ||
| except Exception: | ||
| timeout_obj = None | ||
|
|
||
| if http_request is not None: | ||
| req_kwargs = { | ||
| "json": [alert], | ||
| "headers": headers, | ||
| "service": "alertmanager", | ||
| "endpoint": "/api/v2/alerts", | ||
| "max_attempts": 2, | ||
| } | ||
| if timeout_obj is not None: | ||
| req_kwargs["timeout"] = timeout_obj | ||
|
|
||
| async with http_request("POST", url, **req_kwargs) as resp: | ||
| try: | ||
| status = int(getattr(resp, "status", 0) or 0) | ||
| except Exception: | ||
| status = 0 | ||
|
|
||
| if 200 <= status < 300: | ||
| return True | ||
|
|
||
| # לא מציגים למשתמש; רק לוג דיבוג קצר | ||
| body_preview = "" | ||
| try: | ||
| body_preview = _truncate(await resp.text(), 300) # type: ignore[func-returns-value] | ||
| except Exception: | ||
| body_preview = "" | ||
| try: | ||
| logger.warning( | ||
| "alertmanager_admin_report_failed status=%s body=%s", | ||
| status, | ||
| body_preview or "—", | ||
| ) | ||
| except Exception: | ||
| pass | ||
|
cursor[bot] marked this conversation as resolved.
|
||
| # אם קיבלנו תשובה מהשרת (גם אם דחייה), לא ננסה לשלוח שוב ב-sync fallback | ||
| # כדי לא ליצור כפילות/latency מיותרת. | ||
| return False | ||
| except Exception as e: | ||
| last_err = str(e) | ||
| try: | ||
| logger.warning("alertmanager_admin_report_async_exception error=%s", last_err) | ||
| except Exception: | ||
| pass | ||
|
|
||
| # Fallback: synchronous request in a thread (do not skip if async path failed) | ||
| try: | ||
| try: | ||
| from http_sync import request as http_sync_request # type: ignore | ||
| except Exception: | ||
| http_sync_request = None # type: ignore | ||
|
|
||
| if http_sync_request is None: | ||
| return False | ||
|
|
||
| def _do_sync() -> bool: | ||
| try: | ||
| resp = http_sync_request( | ||
| "POST", | ||
| url, | ||
| json=[alert], | ||
| headers=headers, | ||
| timeout=6, | ||
| service="alertmanager", | ||
| endpoint="/api/v2/alerts", | ||
| max_attempts=2, | ||
| ) | ||
| code = int(getattr(resp, "status_code", 0) or 0) | ||
| return 200 <= code < 300 | ||
| except Exception: | ||
| return False | ||
|
|
||
| return bool(await asyncio.to_thread(_do_sync)) | ||
| except Exception as e: | ||
| try: | ||
| logger.warning( | ||
| "alertmanager_admin_report_sync_exception error=%s prev=%s", | ||
| str(e), | ||
| last_err or "", | ||
| ) | ||
| except Exception: | ||
| pass | ||
| return False | ||
|
|
||
|
|
||
| async def _send_direct_admins(context: ContextTypes.DEFAULT_TYPE, text: str) -> bool: | ||
| """Fallback לשליחת הודעה ישירה לאדמינים בטלגרם.""" | ||
| try: | ||
|
|
@@ -809,11 +993,21 @@ async def admin_report_command(update: Update, context: ContextTypes.DEFAULT_TYP | |
| f"• user_id: {user_id}\n" | ||
| f"• הודעה: {args_text}" | ||
| ) | ||
| sent = await notify_admins(context, report) | ||
| if not sent: | ||
| sent = await _send_direct_admins(context, report) | ||
| if sent: | ||
| # חשוב: נציג "נשלח" רק אם Alertmanager קיבל את ההודעה בהצלחה (2xx). | ||
| # אם Alertmanager לא זמין, ננסה fallback ישיר לאדמינים – אבל ננסח זאת במפורש כדי לא להטעות. | ||
| sent_via_am = await _send_admin_report_via_alertmanager( | ||
| report, | ||
| user_id=user_id, | ||
| username=username, | ||
| display=display, | ||
| ) | ||
| if sent_via_am: | ||
| await message.reply_text("תודה! הדיווח נשלח לאדמין.") | ||
| return | ||
|
|
||
| sent_direct = await _send_direct_admins(context, report) | ||
| if sent_direct: | ||
| await message.reply_text("כרגע Alertmanager לא זמין, אבל שלחתי את הדיווח ישירות לאדמין.") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fallback bypasses Rule Engine and internal alertsHigh Severity The new |
||
| else: | ||
| await message.reply_text("לא הצלחתי לשלוח את הדיווח כרגע.") | ||
| except Exception: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,100 @@ | ||
| import types | ||
| import pytest | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_admin_report_ack_only_on_alertmanager_success(monkeypatch): | ||
| import main as main_mod | ||
|
|
||
| class Msg: | ||
| def __init__(self): | ||
| self.replies = [] | ||
|
|
||
| async def reply_text(self, text): | ||
| self.replies.append(text) | ||
|
|
||
| msg = Msg() | ||
| upd = types.SimpleNamespace( | ||
| message=msg, | ||
| effective_message=msg, | ||
| effective_user=types.SimpleNamespace(id=123, username="alice", full_name="Alice"), | ||
| ) | ||
| ctx = types.SimpleNamespace(args=["יש", "בעיה"]) | ||
|
|
||
| async def _am_ok(*_a, **_k): | ||
| return True | ||
|
|
||
| async def _direct_should_not_run(*_a, **_k): | ||
| raise AssertionError("_send_direct_admins should not be called when Alertmanager succeeds") | ||
|
|
||
| monkeypatch.setattr(main_mod, "_send_admin_report_via_alertmanager", _am_ok) | ||
| monkeypatch.setattr(main_mod, "_send_direct_admins", _direct_should_not_run) | ||
|
|
||
| await main_mod.admin_report_command(upd, ctx) | ||
| assert msg.replies and msg.replies[-1] == "תודה! הדיווח נשלח לאדמין." | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_admin_report_fallback_message_when_alertmanager_fails(monkeypatch): | ||
| import main as main_mod | ||
|
|
||
| class Msg: | ||
| def __init__(self): | ||
| self.replies = [] | ||
|
|
||
| async def reply_text(self, text): | ||
| self.replies.append(text) | ||
|
|
||
| msg = Msg() | ||
| upd = types.SimpleNamespace( | ||
| message=msg, | ||
| effective_message=msg, | ||
| effective_user=types.SimpleNamespace(id=7, username=None, full_name="Bob"), | ||
| ) | ||
| ctx = types.SimpleNamespace(args=["הודעה"]) | ||
|
|
||
| async def _am_fail(*_a, **_k): | ||
| return False | ||
|
|
||
| async def _direct_ok(*_a, **_k): | ||
| return True | ||
|
|
||
| monkeypatch.setattr(main_mod, "_send_admin_report_via_alertmanager", _am_fail) | ||
| monkeypatch.setattr(main_mod, "_send_direct_admins", _direct_ok) | ||
|
|
||
| await main_mod.admin_report_command(upd, ctx) | ||
| assert msg.replies | ||
| assert "Alertmanager לא זמין" in msg.replies[-1] | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_admin_report_failure_when_both_paths_fail(monkeypatch): | ||
| import main as main_mod | ||
|
|
||
| class Msg: | ||
| def __init__(self): | ||
| self.replies = [] | ||
|
|
||
| async def reply_text(self, text): | ||
| self.replies.append(text) | ||
|
|
||
| msg = Msg() | ||
| upd = types.SimpleNamespace( | ||
| message=msg, | ||
| effective_message=msg, | ||
| effective_user=types.SimpleNamespace(id=7, username="u", full_name="U"), | ||
| ) | ||
| ctx = types.SimpleNamespace(args=["הודעה"]) | ||
|
|
||
| async def _am_fail(*_a, **_k): | ||
| return False | ||
|
|
||
| async def _direct_fail(*_a, **_k): | ||
| return False | ||
|
|
||
| monkeypatch.setattr(main_mod, "_send_admin_report_via_alertmanager", _am_fail) | ||
| monkeypatch.setattr(main_mod, "_send_direct_admins", _direct_fail) | ||
|
|
||
| await main_mod.admin_report_command(upd, ctx) | ||
| assert msg.replies and msg.replies[-1] == "לא הצלחתי לשלוח את הדיווח כרגע." | ||
|
|


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scheme guessing fails for non-standard HTTPS ports
Low Severity
The scheme guessing logic checks if the hostname starts with
alertmanagerand defaults tohttp, which incorrectly handles URLs likealertmanager.prod.example.com:8443oralertmanager-api.example.com:9443where HTTPS is running on non-standard ports. The check forstartswith("alertmanager")on line 778 takes precedence over port-based detection for any port other than 443, causing HTTPS requests to fail. Users can work around this by usingALERTMANAGER_API_URLwith the full scheme.