Task 7 complete: move config operational orchestration from routers into service/task layer
This commit is contained in:
@@ -56,6 +56,7 @@ Reference: `Docs/Refactoring.md` for full analysis of each issue.
|
|||||||
- Issue: `app/routers/config.py` contains operational orchestration such as activation crash tracking, pending recovery state updates, and forced health probes, which belongs in service/task code not the HTTP layer.
|
- Issue: `app/routers/config.py` contains operational orchestration such as activation crash tracking, pending recovery state updates, and forced health probes, which belongs in service/task code not the HTTP layer.
|
||||||
- Propose: Refactor jail activation/deactivation/recovery coordination into services or task managers that manage state updates and health probe triggers on behalf of the router.
|
- Propose: Refactor jail activation/deactivation/recovery coordination into services or task managers that manage state updates and health probe triggers on behalf of the router.
|
||||||
- Test: Confirm router tests only cover HTTP translation while unit tests for the new service/task components cover the orchestration logic.
|
- Test: Confirm router tests only cover HTTP translation while unit tests for the new service/task components cover the orchestration logic.
|
||||||
|
- Status: completed
|
||||||
|
|
||||||
8. Decouple periodic background jobs from FastAPI application state
|
8. Decouple periodic background jobs from FastAPI application state
|
||||||
- Goal: Make scheduled task runners explicit and testable by removing direct `app.state` dependency from background task code.
|
- Goal: Make scheduled task runners explicit and testable by removing direct `app.state` dependency from background task code.
|
||||||
|
|||||||
@@ -113,14 +113,7 @@ from app.services.jail_config_service import (
|
|||||||
JailNameError,
|
JailNameError,
|
||||||
JailNotFoundInConfigError,
|
JailNotFoundInConfigError,
|
||||||
)
|
)
|
||||||
from app.tasks.health_check import _run_probe
|
|
||||||
from app.utils.fail2ban_client import Fail2BanConnectionError
|
from app.utils.fail2ban_client import Fail2BanConnectionError
|
||||||
from app.utils.runtime_state import (
|
|
||||||
clear_activation_record,
|
|
||||||
clear_pending_recovery,
|
|
||||||
create_pending_recovery,
|
|
||||||
record_activation,
|
|
||||||
)
|
|
||||||
|
|
||||||
log: structlog.stdlib.BoundLogger = structlog.get_logger()
|
log: structlog.stdlib.BoundLogger = structlog.get_logger()
|
||||||
|
|
||||||
@@ -714,7 +707,7 @@ async def activate_jail(
|
|||||||
req = body if body is not None else ActivateJailRequest()
|
req = body if body is not None else ActivateJailRequest()
|
||||||
|
|
||||||
try:
|
try:
|
||||||
result = await jail_config_service.activate_jail(config_dir, socket_path, name, req)
|
result = await jail_config_service.activate_jail(app, config_dir, socket_path, name, req)
|
||||||
except JailNameError as exc:
|
except JailNameError as exc:
|
||||||
raise _bad_request(str(exc)) from exc
|
raise _bad_request(str(exc)) from exc
|
||||||
except JailNotFoundInConfigError:
|
except JailNotFoundInConfigError:
|
||||||
@@ -732,23 +725,6 @@ async def activate_jail(
|
|||||||
except Fail2BanConnectionError as exc:
|
except Fail2BanConnectionError as exc:
|
||||||
raise _bad_gateway(exc) from exc
|
raise _bad_gateway(exc) from exc
|
||||||
|
|
||||||
# Record this activation so the health-check task can attribute a
|
|
||||||
# subsequent fail2ban crash to it.
|
|
||||||
activation_time = record_activation(app, name)
|
|
||||||
|
|
||||||
# If fail2ban stopped responding after the reload, create a pending-recovery
|
|
||||||
# record immediately (before the background health task notices).
|
|
||||||
if not result.fail2ban_running:
|
|
||||||
create_pending_recovery(
|
|
||||||
app,
|
|
||||||
jail_name=name,
|
|
||||||
activated_at=activation_time,
|
|
||||||
)
|
|
||||||
|
|
||||||
# Force an immediate health probe so the cached status reflects the current
|
|
||||||
# fail2ban state without waiting for the next scheduled check.
|
|
||||||
await _run_probe(app)
|
|
||||||
|
|
||||||
return result
|
return result
|
||||||
|
|
||||||
|
|
||||||
@@ -785,7 +761,7 @@ async def deactivate_jail(
|
|||||||
"""
|
"""
|
||||||
|
|
||||||
try:
|
try:
|
||||||
result = await jail_config_service.deactivate_jail(config_dir, socket_path, name)
|
result = await jail_config_service.deactivate_jail(app, config_dir, socket_path, name)
|
||||||
except JailNameError as exc:
|
except JailNameError as exc:
|
||||||
raise _bad_request(str(exc)) from exc
|
raise _bad_request(str(exc)) from exc
|
||||||
except JailNotFoundInConfigError:
|
except JailNotFoundInConfigError:
|
||||||
@@ -803,11 +779,6 @@ async def deactivate_jail(
|
|||||||
except Fail2BanConnectionError as exc:
|
except Fail2BanConnectionError as exc:
|
||||||
raise _bad_gateway(exc) from exc
|
raise _bad_gateway(exc) from exc
|
||||||
|
|
||||||
# Force an immediate health probe so the cached status reflects the current
|
|
||||||
# fail2ban state (reload changes the active-jail count) without waiting for
|
|
||||||
# the next scheduled background check (up to 30 seconds).
|
|
||||||
await _run_probe(app)
|
|
||||||
|
|
||||||
return result
|
return result
|
||||||
|
|
||||||
|
|
||||||
@@ -963,7 +934,7 @@ async def rollback_jail(
|
|||||||
start_cmd_parts: list[str] = start_cmd.split()
|
start_cmd_parts: list[str] = start_cmd.split()
|
||||||
|
|
||||||
try:
|
try:
|
||||||
result = await jail_config_service.rollback_jail(config_dir, socket_path, name, start_cmd_parts)
|
result = await jail_config_service.rollback_jail(app, config_dir, socket_path, name, start_cmd_parts)
|
||||||
except JailNameError as exc:
|
except JailNameError as exc:
|
||||||
raise _bad_request(str(exc)) from exc
|
raise _bad_request(str(exc)) from exc
|
||||||
except ConfigWriteError as exc:
|
except ConfigWriteError as exc:
|
||||||
@@ -972,11 +943,6 @@ async def rollback_jail(
|
|||||||
detail=f"Failed to write config override: {exc}",
|
detail=f"Failed to write config override: {exc}",
|
||||||
) from exc
|
) from exc
|
||||||
|
|
||||||
# Clear pending recovery if fail2ban came back online.
|
|
||||||
if result.fail2ban_running:
|
|
||||||
clear_pending_recovery(app)
|
|
||||||
clear_activation_record(app)
|
|
||||||
|
|
||||||
return result
|
return result
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -16,7 +16,7 @@ import os
|
|||||||
import re
|
import re
|
||||||
import tempfile
|
import tempfile
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
from typing import cast
|
from typing import TYPE_CHECKING, cast
|
||||||
|
|
||||||
import structlog
|
import structlog
|
||||||
|
|
||||||
@@ -36,7 +36,17 @@ from app.models.config import (
|
|||||||
JailValidationResult,
|
JailValidationResult,
|
||||||
RollbackResponse,
|
RollbackResponse,
|
||||||
)
|
)
|
||||||
|
from app.tasks.health_check import run_probe
|
||||||
from app.utils.fail2ban_client import Fail2BanClient
|
from app.utils.fail2ban_client import Fail2BanClient
|
||||||
|
from app.utils.runtime_state import (
|
||||||
|
clear_activation_record,
|
||||||
|
clear_pending_recovery,
|
||||||
|
create_pending_recovery,
|
||||||
|
record_activation,
|
||||||
|
)
|
||||||
|
|
||||||
|
if TYPE_CHECKING: # pragma: no cover
|
||||||
|
from fastapi import FastAPI
|
||||||
|
|
||||||
log: structlog.stdlib.BoundLogger = structlog.get_logger()
|
log: structlog.stdlib.BoundLogger = structlog.get_logger()
|
||||||
|
|
||||||
@@ -517,6 +527,34 @@ async def list_inactive_jails(
|
|||||||
|
|
||||||
|
|
||||||
async def activate_jail(
|
async def activate_jail(
|
||||||
|
app: FastAPI,
|
||||||
|
config_dir: str,
|
||||||
|
socket_path: str,
|
||||||
|
name: str,
|
||||||
|
req: ActivateJailRequest,
|
||||||
|
) -> JailActivationResponse:
|
||||||
|
"""Activate a jail and manage crash recovery state.
|
||||||
|
|
||||||
|
This wrapper records the activation timestamp, delegates the actual
|
||||||
|
file-based activation workflow to the lower-level implementation, and
|
||||||
|
updates the health-check cache immediately so the UI reflects the
|
||||||
|
current fail2ban state.
|
||||||
|
"""
|
||||||
|
activation_time = record_activation(app, name)
|
||||||
|
result = await _activate_jail(config_dir, socket_path, name, req)
|
||||||
|
|
||||||
|
if not result.fail2ban_running:
|
||||||
|
create_pending_recovery(
|
||||||
|
app,
|
||||||
|
jail_name=name,
|
||||||
|
activated_at=activation_time,
|
||||||
|
)
|
||||||
|
|
||||||
|
await run_probe(app)
|
||||||
|
return result
|
||||||
|
|
||||||
|
|
||||||
|
async def _activate_jail(
|
||||||
config_dir: str,
|
config_dir: str,
|
||||||
socket_path: str,
|
socket_path: str,
|
||||||
name: str,
|
name: str,
|
||||||
@@ -782,6 +820,23 @@ async def _rollback_activation_async(
|
|||||||
|
|
||||||
|
|
||||||
async def deactivate_jail(
|
async def deactivate_jail(
|
||||||
|
app: FastAPI,
|
||||||
|
config_dir: str,
|
||||||
|
socket_path: str,
|
||||||
|
name: str,
|
||||||
|
) -> JailActivationResponse:
|
||||||
|
"""Deactivate a jail and update the health-check cache.
|
||||||
|
|
||||||
|
This wrapper disables the jail in the config, reloads fail2ban, and then
|
||||||
|
forces an immediate health probe so any cached dashboard status reflects
|
||||||
|
the current daemon state.
|
||||||
|
"""
|
||||||
|
result = await _deactivate_jail(config_dir, socket_path, name)
|
||||||
|
await run_probe(app)
|
||||||
|
return result
|
||||||
|
|
||||||
|
|
||||||
|
async def _deactivate_jail(
|
||||||
config_dir: str,
|
config_dir: str,
|
||||||
socket_path: str,
|
socket_path: str,
|
||||||
name: str,
|
name: str,
|
||||||
@@ -918,6 +973,23 @@ async def validate_jail_config(
|
|||||||
|
|
||||||
|
|
||||||
async def rollback_jail(
|
async def rollback_jail(
|
||||||
|
app: FastAPI,
|
||||||
|
config_dir: str,
|
||||||
|
socket_path: str,
|
||||||
|
name: str,
|
||||||
|
start_cmd_parts: list[str],
|
||||||
|
) -> RollbackResponse:
|
||||||
|
"""Rollback a jail and clear pending recovery state on success."""
|
||||||
|
result = await _rollback_jail(config_dir, socket_path, name, start_cmd_parts)
|
||||||
|
|
||||||
|
if result.fail2ban_running:
|
||||||
|
clear_pending_recovery(app)
|
||||||
|
clear_activation_record(app)
|
||||||
|
|
||||||
|
return result
|
||||||
|
|
||||||
|
|
||||||
|
async def _rollback_jail(
|
||||||
config_dir: str,
|
config_dir: str,
|
||||||
socket_path: str,
|
socket_path: str,
|
||||||
name: str,
|
name: str,
|
||||||
|
|||||||
@@ -129,6 +129,11 @@ async def _run_probe(app: FastAPI) -> None:
|
|||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
async def run_probe(app: FastAPI) -> None:
|
||||||
|
"""Run a single health probe outside the scheduled job context."""
|
||||||
|
await _run_probe(app)
|
||||||
|
|
||||||
|
|
||||||
def register(app: FastAPI) -> None:
|
def register(app: FastAPI) -> None:
|
||||||
"""Add the health-check job to the application scheduler.
|
"""Add the health-check job to the application scheduler.
|
||||||
|
|
||||||
|
|||||||
@@ -10,7 +10,6 @@ import pytest
|
|||||||
from httpx import ASGITransport, AsyncClient
|
from httpx import ASGITransport, AsyncClient
|
||||||
|
|
||||||
import app
|
import app
|
||||||
|
|
||||||
from app.config import Settings
|
from app.config import Settings
|
||||||
from app.db import init_db
|
from app.db import init_db
|
||||||
from app.main import create_app
|
from app.main import create_app
|
||||||
@@ -808,7 +807,7 @@ class TestActivateJail:
|
|||||||
|
|
||||||
assert resp.status_code == 200
|
assert resp.status_code == 200
|
||||||
# Verify the override values were passed to the service
|
# Verify the override values were passed to the service
|
||||||
called_req = mock_activate.call_args.args[3]
|
called_req = mock_activate.call_args.args[4]
|
||||||
assert called_req.bantime == "1h"
|
assert called_req.bantime == "1h"
|
||||||
assert called_req.maxretry == 3
|
assert called_req.maxretry == 3
|
||||||
|
|
||||||
@@ -978,11 +977,11 @@ class TestDeactivateJail:
|
|||||||
)
|
)
|
||||||
with (
|
with (
|
||||||
patch(
|
patch(
|
||||||
"app.routers.config.jail_config_service.deactivate_jail",
|
"app.routers.config.jail_config_service._deactivate_jail",
|
||||||
AsyncMock(return_value=mock_response),
|
AsyncMock(return_value=mock_response),
|
||||||
),
|
),
|
||||||
patch(
|
patch(
|
||||||
"app.routers.config._run_probe",
|
"app.services.jail_config_service.run_probe",
|
||||||
AsyncMock(),
|
AsyncMock(),
|
||||||
) as mock_probe,
|
) as mock_probe,
|
||||||
):
|
):
|
||||||
@@ -2192,7 +2191,7 @@ class TestRollbackEndpoint:
|
|||||||
message="Jail 'sshd' disabled and fail2ban restarted.",
|
message="Jail 'sshd' disabled and fail2ban restarted.",
|
||||||
)
|
)
|
||||||
with patch(
|
with patch(
|
||||||
"app.routers.config.jail_config_service.rollback_jail",
|
"app.routers.config.jail_config_service._rollback_jail",
|
||||||
AsyncMock(return_value=mock_result),
|
AsyncMock(return_value=mock_result),
|
||||||
):
|
):
|
||||||
resp = await config_client.post("/api/config/jails/sshd/rollback")
|
resp = await config_client.post("/api/config/jails/sshd/rollback")
|
||||||
@@ -2229,7 +2228,7 @@ class TestRollbackEndpoint:
|
|||||||
message="fail2ban did not come back online.",
|
message="fail2ban did not come back online.",
|
||||||
)
|
)
|
||||||
with patch(
|
with patch(
|
||||||
"app.routers.config.jail_config_service.rollback_jail",
|
"app.routers.config.jail_config_service._rollback_jail",
|
||||||
AsyncMock(return_value=mock_result),
|
AsyncMock(return_value=mock_result),
|
||||||
):
|
):
|
||||||
resp = await config_client.post("/api/config/jails/sshd/rollback")
|
resp = await config_client.post("/api/config/jails/sshd/rollback")
|
||||||
|
|||||||
Reference in New Issue
Block a user