diff --git a/docs/reference/openapi.yaml b/docs/reference/openapi.yaml index ac8b9401b..89139685f 100644 --- a/docs/reference/openapi.yaml +++ b/docs/reference/openapi.yaml @@ -1,5 +1,22 @@ components: schemas: + BasicAuthentication: + additionalProperties: false + description: User credentials for basic authentication + properties: + password: + description: Password to verify user's identity + title: Password + type: string + username: + description: Unique identifier for user + title: Username + type: string + required: + - username + - password + title: BasicAuthentication + type: object DeviceModel: additionalProperties: false description: Representation of a device @@ -224,6 +241,28 @@ components: - new_state title: StateChangeRequest type: object + StompConfig: + additionalProperties: false + description: Config for connecting to stomp broker + properties: + auth: + $ref: '#/components/schemas/BasicAuthentication' + description: Auth information for communicating with STOMP broker, if required + title: Auth + enabled: + default: false + description: True if blueapi should connect to stomp for asynchronous event + publishing + title: Enabled + type: boolean + url: + default: tcp://localhost:61613 + format: uri + minLength: 1 + title: Url + type: string + title: StompConfig + type: object Task: additionalProperties: false description: Task that will run a plan @@ -433,7 +472,7 @@ info: name: Apache 2.0 url: https://www.apache.org/licenses/LICENSE-2.0.html title: BlueAPI Control - version: 1.2.0 + version: 1.3.0 openapi: 3.1.0 paths: /config/oidc: @@ -452,6 +491,22 @@ paths: summary: Get Oidc Config tags: - Meta + /config/stomp: + get: + description: Retrieve the stomp configuration for the server. + operationId: get_stomp_config_config_stomp_get + responses: + '200': + content: + application/json: + schema: + $ref: '#/components/schemas/StompConfig' + description: Successful Response + '204': + description: No Stomp configured + summary: Get Stomp Config + tags: + - Meta /devices: get: description: Retrieve information about all available devices. diff --git a/helm/blueapi/config_schema.json b/helm/blueapi/config_schema.json index fc0a37dec..4210d7795 100644 --- a/helm/blueapi/config_schema.json +++ b/helm/blueapi/config_schema.json @@ -458,16 +458,10 @@ "type": "string" }, "auth": { - "anyOf": [ - { - "$ref": "BasicAuthentication" - }, - { - "type": "null" - } - ], + "$ref": "BasicAuthentication", "default": null, - "description": "Auth information for communicating with STOMP broker, if required" + "description": "Auth information for communicating with STOMP broker, if required", + "title": "Auth" } }, "title": "StompConfig", diff --git a/helm/blueapi/values.schema.json b/helm/blueapi/values.schema.json index e06c9bd5d..204be39e7 100644 --- a/helm/blueapi/values.schema.json +++ b/helm/blueapi/values.schema.json @@ -879,15 +879,9 @@ "type": "object", "properties": { "auth": { + "title": "Auth", "description": "Auth information for communicating with STOMP broker, if required", - "anyOf": [ - { - "$ref": "BasicAuthentication" - }, - { - "type": "null" - } - ] + "$ref": "BasicAuthentication" }, "enabled": { "title": "Enabled", diff --git a/src/blueapi/cli/cli.py b/src/blueapi/cli/cli.py index 7bafa831f..e04ad2fdc 100644 --- a/src/blueapi/cli/cli.py +++ b/src/blueapi/cli/cli.py @@ -80,11 +80,12 @@ def is_str_dict(val: Any) -> TypeGuard[TaskParameters]: invoke_without_command=True, context_settings={"auto_envvar_prefix": "BLUEAPI"} ) @click.version_option(version=__version__, prog_name="blueapi") +@click.option("-H", "--host", type=str) @click.option( "-c", "--config", type=Path, help="Path to configuration YAML file", multiple=True ) @click.pass_context -def main(ctx: click.Context, config: tuple[Path, ...]) -> None: +def main(ctx: click.Context, config: tuple[Path, ...], host: str | None): # if no command is supplied, run with the options passed # Set umask to DLS standard @@ -95,6 +96,8 @@ def main(ctx: click.Context, config: tuple[Path, ...]) -> None: config_loader.use_values_from_yaml(*config) except FileNotFoundError as fnfe: raise ClickException(f"Config file not found: {fnfe.filename}") from fnfe + if host: + config_loader.use_values({"api": {"url": host}}) loaded_config: ApplicationConfig = config_loader.load() diff --git a/src/blueapi/client/client.py b/src/blueapi/client/client.py index c5b41ff45..aa7cb471f 100644 --- a/src/blueapi/client/client.py +++ b/src/blueapi/client/client.py @@ -8,12 +8,8 @@ from pathlib import Path from typing import Self -from bluesky_stomp.messaging import MessageContext, StompClient -from bluesky_stomp.models import Broker -from observability_utils.tracing import ( - get_tracer, - start_as_current_span, -) +from bluesky_stomp.messaging import MessageContext +from observability_utils.tracing import get_tracer, start_as_current_span from blueapi.config import ( ApplicationConfig, @@ -201,7 +197,7 @@ class BlueapiClient: """Unified client for controlling blueapi""" _rest: BlueapiRestClient - _events: EventBusClient | None + _event_bus_client: EventBusClient | None _instrument_session: str | None = None _callbacks: dict[int, OnAnyEvent] _callback_id: itertools.count @@ -212,7 +208,7 @@ def __init__( events: EventBusClient | None = None, ): self._rest = rest - self._events = events + self._event_bus_client = events self._callbacks = {} self._callback_id = itertools.count() @@ -230,20 +226,8 @@ def from_config(cls, config: ApplicationConfig) -> Self: except Exception: ... # Swallow exceptions rest = BlueapiRestClient(config.api, session_manager=session_manager) - if config.stomp.enabled: - assert config.stomp.url.host is not None, "Stomp URL missing host" - assert config.stomp.url.port is not None, "Stomp URL missing port" - client = StompClient.for_broker( - broker=Broker( - host=config.stomp.url.host, - port=config.stomp.url.port, - auth=config.stomp.auth, - ) - ) - events = EventBusClient(client) - return cls(rest, events) - else: - return cls(rest) + event_bus = EventBusClient.from_stomp_config(config.stomp) + return cls(rest, event_bus) @cached_property @start_as_current_span(TRACER) @@ -460,7 +444,7 @@ def run_task( of task execution. """ - if self._events is None: + if (event_bus := self._event_bus()) is None: raise MissingStompConfigurationError( "Stomp configuration required to run plans is missing or disabled" ) @@ -504,8 +488,8 @@ def inner_on_event(event: AnyEvent, ctx: MessageContext) -> None: else: complete.set_result(event.task_status) - with self._events: - self._events.subscribe_to_all_events(inner_on_event) + with event_bus: + event_bus.subscribe_to_all_events(inner_on_event) self._rest.update_worker_task(WorkerTask(task_id=task_id)) return complete.result(timeout=timeout) @@ -744,3 +728,10 @@ def login(self, token_path: Path | None = None): auth.start_device_flow() else: print("Server is not configured to use authentication!") + + def _event_bus(self) -> EventBusClient | None: + if not self._event_bus_client: + if stomp_config := self._rest.get_stomp_config(): + self._event_bus_client = EventBusClient.from_stomp_config(stomp_config) + + return self._event_bus_client diff --git a/src/blueapi/client/event_bus.py b/src/blueapi/client/event_bus.py index cb807f24d..2f344a081 100644 --- a/src/blueapi/client/event_bus.py +++ b/src/blueapi/client/event_bus.py @@ -1,8 +1,10 @@ from collections.abc import Callable +from typing import Self -from bluesky_stomp.messaging import MessageContext, StompClient +from bluesky_stomp.messaging import Broker, MessageContext, StompClient from bluesky_stomp.models import MessageTopic +from blueapi.config import StompConfig from blueapi.core import DataEvent from blueapi.worker import ProgressEvent, WorkerEvent @@ -45,3 +47,15 @@ def subscribe_to_all_events( raise BlueskyStreamingError( "Unable to subscribe to messages from blueapi" ) from err + + @classmethod + def from_stomp_config(cls, config: StompConfig) -> Self | None: + if config.enabled: + assert config.url.host is not None, "Stomp URL missing host" + assert config.url.port is not None, "Stomp URL missing port" + client = StompClient.for_broker( + broker=Broker( + host=config.url.host, port=config.url.port, auth=config.auth + ) + ) + return cls(client) diff --git a/src/blueapi/client/rest.py b/src/blueapi/client/rest.py index 52150d36f..ac7b54669 100644 --- a/src/blueapi/client/rest.py +++ b/src/blueapi/client/rest.py @@ -10,7 +10,7 @@ ) from pydantic import BaseModel, TypeAdapter, ValidationError -from blueapi.config import RestConfig +from blueapi.config import RestConfig, StompConfig from blueapi.service.authentication import JWTAuth, SessionManager from blueapi.service.model import ( DeviceModel, @@ -233,6 +233,14 @@ def get_oidc_config(self) -> OIDCConfig | None: # Server is not using authentication return None + def get_stomp_config(self) -> StompConfig | None: + try: + return self._request_and_deserialize("/config/stomp", StompConfig) + except (NoContentError, KeyError): + # Older versions of the server may not have the endpoint implemented so + # treat 404s as no configuration. + return None + def get_python_environment( self, name: str | None = None, source: SourceInfo | None = None ) -> PythonEnvironmentResponse: diff --git a/src/blueapi/config.py b/src/blueapi/config.py index 59c6c19ab..e3c6fa758 100644 --- a/src/blueapi/config.py +++ b/src/blueapi/config.py @@ -101,7 +101,7 @@ class StompConfig(BlueapiBaseModel): default=False, ) url: TcpUrl = TcpUrl("tcp://localhost:61613") - auth: BasicAuthentication | None = Field( + auth: BasicAuthentication | SkipJsonSchema[None] = Field( description="Auth information for communicating with STOMP broker, if required", default=None, ) @@ -286,7 +286,7 @@ class ApplicationConfig(BlueapiBaseModel): """ #: API version to publish in OpenAPI schema - REST_API_VERSION: ClassVar[str] = "1.2.0" + REST_API_VERSION: ClassVar[str] = "1.3.0" LICENSE_INFO: ClassVar[dict[str, str]] = { "name": "Apache 2.0", diff --git a/src/blueapi/service/interface.py b/src/blueapi/service/interface.py index 6acc29ab7..552d93e94 100644 --- a/src/blueapi/service/interface.py +++ b/src/blueapi/service/interface.py @@ -264,6 +264,10 @@ def get_oidc_config() -> OIDCConfig | None: return config().oidc +def get_stomp_config() -> StompConfig | None: + return config().stomp + + def get_python_env( name: str | None = None, source: SourceInfo | None = None ) -> PythonEnvironmentResponse: diff --git a/src/blueapi/service/main.py b/src/blueapi/service/main.py index c79dd3df3..912e7a6f4 100644 --- a/src/blueapi/service/main.py +++ b/src/blueapi/service/main.py @@ -34,7 +34,7 @@ from starlette.responses import JSONResponse from super_state_machine.errors import TransitionError -from blueapi.config import ApplicationConfig, OIDCConfig, Tag +from blueapi.config import ApplicationConfig, OIDCConfig, StompConfig, Tag from blueapi.service import interface from blueapi.worker import TrackableTask, WorkerState from blueapi.worker.event import TaskStatusEnum @@ -226,6 +226,22 @@ def get_oidc_config( return config +@open_router.get( + "/config/stomp", + tags=[Tag.META], + responses={status.HTTP_204_NO_CONTENT: {"description": "No Stomp configured"}}, +) +@start_as_current_span(TRACER) +def get_stomp_config( + runner: Annotated[WorkerDispatcher, Depends(_runner)], +) -> StompConfig: + """Retrieve the stomp configuration for the server.""" + config = runner.run(interface.get_stomp_config) + if config is None: + raise HTTPException(status_code=status.HTTP_204_NO_CONTENT) + return config + + @secure_router.get("/plans", tags=[Tag.PLAN]) @start_as_current_span(TRACER) def get_plans(runner: Annotated[WorkerDispatcher, Depends(_runner)]) -> PlanResponse: diff --git a/tests/system_tests/test_blueapi_system.py b/tests/system_tests/test_blueapi_system.py index 1b1ac0770..131ebf3b0 100644 --- a/tests/system_tests/test_blueapi_system.py +++ b/tests/system_tests/test_blueapi_system.py @@ -169,16 +169,21 @@ def expected_devices() -> DeviceResponse: ) -@pytest.fixture -def blueapi_rest_client_get_methods() -> list[str]: - # Get a list of methods that take only one argument (self) +def authenticated_get_methods() -> list[str]: + # Get a list of methods that take only one argument (self) and require + # authentication. This will currently return + # ['get_plans', 'get_devices', 'get_state', 'get_all_tasks', + # 'get_active_task','get_environment','resume', 'stop'] return [ - name - for name, method in BlueapiRestClient.__dict__.items() - if not name.startswith("__") - and callable(method) - and len(params := inspect.signature(method).parameters) == 1 - and "self" in params + method + for method in BlueapiRestClient.__dict__ + if callable(getattr(BlueapiRestClient, method)) + and not method.startswith("__") + and len(inspect.signature(getattr(BlueapiRestClient, method)).parameters) == 1 + and "self" in inspect.signature(getattr(BlueapiRestClient, method)).parameters + # oidc_config and stomp config can be accessed without auth + and method != "get_oidc_config" + and method != "get_stomp_config" ] @@ -210,15 +215,10 @@ def reset_numtracker(): yield -def test_cannot_access_endpoints( - client_without_auth: BlueapiClient, blueapi_rest_client_get_methods: list[str] -): - blueapi_rest_client_get_methods.remove( - "get_oidc_config" - ) # get_oidc_config can be accessed without auth - for get_method in blueapi_rest_client_get_methods: - with pytest.raises(BlueskyRemoteControlError, match=r""): - getattr(client_without_auth._rest, get_method)() +@pytest.mark.parametrize("method_name", authenticated_get_methods()) +def test_cannot_access_endpoints(client_without_auth: BlueapiClient, method_name: str): + with pytest.raises(BlueskyRemoteControlError, match=r""): + getattr(client_without_auth._rest, method_name)() def test_can_get_oidc_config_without_auth(client_without_auth: BlueapiClient): diff --git a/tests/unit_tests/client/test_client.py b/tests/unit_tests/client/test_client.py index a96f428e8..fe2a1af48 100644 --- a/tests/unit_tests/client/test_client.py +++ b/tests/unit_tests/client/test_client.py @@ -20,7 +20,7 @@ ) from blueapi.client.event_bus import AnyEvent, EventBusClient from blueapi.client.rest import BlueapiRestClient, BlueskyRemoteControlError -from blueapi.config import MissingStompConfigurationError +from blueapi.config import MissingStompConfigurationError, StompConfig, TcpUrl from blueapi.core import DataEvent from blueapi.service.model import ( DeviceModel, @@ -376,7 +376,8 @@ def test_resume( ) -def test_cannot_run_task_without_message_bus(client: BlueapiClient): +def test_cannot_run_task_without_message_bus(client: BlueapiClient, mock_rest: Mock): + mock_rest.get_stomp_config.return_value = None with pytest.raises( MissingStompConfigurationError, match="Stomp configuration required to run plans is missing or disabled", @@ -384,6 +385,27 @@ def test_cannot_run_task_without_message_bus(client: BlueapiClient): client.run_task(TaskRequest(name="foo", instrument_session="cm12345-1")) +@patch("blueapi.client.client.EventBusClient") +def test_run_task_with_stomp_config_from_server( + ebc: Mock, client: BlueapiClient, mock_rest: Mock +): + mock_rest.get_stomp_config.return_value = StompConfig( + enabled=True, url=TcpUrl("tcp://localhost:9876"), auth=None + ) + mock_rest.create_task.return_value = TaskResponse(task_id="foo") + mock_rest.update_worker_task.return_value = TaskResponse(task_id="foo") + events = MagicMock(spec=EventBusClient, name="EventBusClient") + ctx = Mock(correlation_id="foo") + events.subscribe_to_all_events.side_effect = lambda on_event: on_event( + COMPLETE_EVENT, ctx + ) + ebc.from_stomp_config.return_value = events + + client.run_task(TaskRequest(name="foo", instrument_session="cm12345-1")) + + mock_rest.get_stomp_config.assert_called_once() + + def test_run_task_sets_up_control( client_with_events: BlueapiClient, mock_rest: Mock, @@ -619,8 +641,11 @@ def test_resume_span_ok( def test_cannot_run_task_span_ok( - exporter: JsonObjectSpanExporter, client: BlueapiClient + exporter: JsonObjectSpanExporter, + client: BlueapiClient, + mock_rest: Mock, ): + mock_rest.get_stomp_config.return_value = None with pytest.raises( MissingStompConfigurationError, match="Stomp configuration required to run plans is missing or disabled", diff --git a/tests/unit_tests/client/test_rest.py b/tests/unit_tests/client/test_rest.py index 2ddcdd380..ba2721f8c 100644 --- a/tests/unit_tests/client/test_rest.py +++ b/tests/unit_tests/client/test_rest.py @@ -16,7 +16,7 @@ UnknownPlanError, _create_task_exceptions, ) -from blueapi.config import OIDCConfig +from blueapi.config import OIDCConfig, TcpUrl from blueapi.service.authentication import SessionCacheManager, SessionManager from blueapi.service.model import EnvironmentResponse @@ -196,3 +196,47 @@ def test_parameter_error_other_string(): input=34, ) assert str(p1) == "Invalid value 34 for field field_one.0: error_message" + + +@responses.activate +def test_get_stomp_config(rest: BlueapiRestClient): + responses.add( + responses.GET, + "http://localhost:8000/config/stomp", + json={ + "enabled": True, + "url": "tcp://messagebus.example.com", + "auth": {"username": "foo", "password": "bar"}, + }, + status=200, + ) + stomp = rest.get_stomp_config() + assert stomp is not None + assert stomp.enabled + assert stomp.url == TcpUrl("tcp://messagebus.example.com") + assert stomp.auth is not None + assert stomp.auth.username == "foo" + assert stomp.auth.password.get_secret_value() == "bar" + + +@responses.activate +def test_get_no_stomp_config(rest: BlueapiRestClient): + responses.add( + responses.GET, + "http://localhost:8000/config/stomp", + status=204, + ) + stomp = rest.get_stomp_config() + assert stomp is None + + +@responses.activate +def test_get_stomp_config_from_old_server(rest: BlueapiRestClient): + responses.add( + responses.GET, + "http://localhost:8000/config/stomp", + json={}, # Weird default handling for 404 - See #1409 + status=404, + ) + stomp = rest.get_stomp_config() + assert stomp is None diff --git a/tests/unit_tests/service/test_interface.py b/tests/unit_tests/service/test_interface.py index a455ef72c..c18099c08 100644 --- a/tests/unit_tests/service/test_interface.py +++ b/tests/unit_tests/service/test_interface.py @@ -457,6 +457,12 @@ def test_get_oidc_config(oidc_config: OIDCConfig): assert interface.get_oidc_config() == oidc_config +def test_get_stomp_config(): + stomp_config = StompConfig(enabled=False) + interface.set_config(ApplicationConfig(stomp=stomp_config)) + assert interface.get_stomp_config() is stomp_config + + def test_stomp_config(mock_stomp_client: StompClient): with patch( "blueapi.service.interface.StompClient.for_broker", diff --git a/tests/unit_tests/service/test_rest_api.py b/tests/unit_tests/service/test_rest_api.py index c1d3b6a95..38824c9e2 100644 --- a/tests/unit_tests/service/test_rest_api.py +++ b/tests/unit_tests/service/test_rest_api.py @@ -14,7 +14,13 @@ from pydantic_core import InitErrorDetails from super_state_machine.errors import TransitionError -from blueapi.config import ApplicationConfig, CORSConfig, OIDCConfig, RestConfig +from blueapi.config import ( + ApplicationConfig, + CORSConfig, + OIDCConfig, + RestConfig, + StompConfig, +) from blueapi.core.bluesky_types import Plan from blueapi.service import main from blueapi.service.interface import ( @@ -709,6 +715,27 @@ def test_get_without_authentication(mock_runner: Mock, client: TestClient) -> No assert response.json() == {"detail": "Not authenticated"} +def test_stomp_config_not_found_when_stomp_is_disabled( + mock_runner: Mock, client: TestClient +): + mock_runner.run.return_value = None + response = client.get("/config/stomp") + assert response.status_code == status.HTTP_204_NO_CONTENT + assert response.text == "" + + +def test_get_stomp_config( + mock_runner: Mock, + mock_authn_server, + client: TestClient, +): + stomp_config = StompConfig(enabled=False) + mock_runner.run.return_value = stomp_config + response = client.get("/config/stomp") + assert response.status_code == status.HTTP_200_OK + assert response.json() == stomp_config.model_dump(mode="json") + + def test_oidc_config_not_found_when_auth_is_disabled( mock_runner: Mock, client: TestClient ): diff --git a/tests/unit_tests/test_cli.py b/tests/unit_tests/test_cli.py index 53a7a9e39..29a54dcb4 100644 --- a/tests/unit_tests/test_cli.py +++ b/tests/unit_tests/test_cli.py @@ -257,19 +257,22 @@ def test_submit_plan(runner: CliRunner): @responses.activate def test_submit_plan_without_stomp(runner: CliRunner): config_path = "tests/unit_tests/example_yaml/rest_config.yaml" - result = runner.invoke( - main, - [ - "-c", - config_path, - "controller", - "run", - "-i", - "cm12345-1", - "sleep", - '{"time": 5}', - ], - ) + with patch( + "blueapi.client.rest.BlueapiRestClient.get_stomp_config", return_value=None + ): + result = runner.invoke( + main, + [ + "-c", + config_path, + "controller", + "run", + "-i", + "cm12345-1", + "sleep", + '{"time": 5}', + ], + ) assert ( result.stderr @@ -277,7 +280,7 @@ def test_submit_plan_without_stomp(runner: CliRunner): ) -@patch("blueapi.client.client.StompClient") +@patch("blueapi.client.event_bus.StompClient") @responses.activate def test_run_plan(stomp_client: StompClient, runner: CliRunner): task_id = "abcd-1234" @@ -460,17 +463,20 @@ def test_invalid_stomp_config_for_listener(runner: CliRunner): def test_cannot_run_plans_without_stomp_config(runner: CliRunner): - result = runner.invoke( - main, - [ - "controller", - "run", - "-i", - "cm12345-1", - "sleep", - '{"time": 5}', - ], - ) + with patch( + "blueapi.client.rest.BlueapiRestClient.get_stomp_config", return_value=None + ): + result = runner.invoke( + main, + [ + "controller", + "run", + "-i", + "cm12345-1", + "sleep", + '{"time": 5}', + ], + ) assert result.exit_code == 1 assert ( result.stderr @@ -1398,3 +1404,45 @@ def test_config_schema( def test_task_parameter_type(value, result): t = ParametersType() assert t.convert(value, None, None) == result + + +@responses.activate +def test_host_option(runner: CliRunner): + response = responses.add( + responses.GET, + "http://override.example.com:5678/plans", + json={"plans": []}, + status=200, + ) + + res = runner.invoke( + main, + ["--host", "http://override.example.com:5678", "controller", "plans"], + ) + assert response.call_count == 1 + assert res.exit_code == 0 + + +@responses.activate +def test_host_overrides_config(runner: CliRunner): + config_path = "tests/unit_tests/example_yaml/rest_config.yaml" + response = responses.add( + responses.GET, + "http://override.example.com:5678/plans", + json={"plans": []}, + status=200, + ) + + res = runner.invoke( + main, + [ + "--host", + "http://override.example.com:5678", + "--config", + config_path, + "controller", + "plans", + ], + ) + assert response.call_count == 1 + assert res.exit_code == 0