From 5bb604d6b9faad8ecf79641c8dff2af8e31c3288 Mon Sep 17 00:00:00 2001 From: Volker Theile Date: Tue, 14 Nov 2023 15:12:23 +0100 Subject: [PATCH] Instance Identification Additional changes: - Relocate logging initialization. This way logger calls done by our code will be logged as well. Fixes: https://github.com/aquarist-labs/s3gw/issues/765 Signed-off-by: Volker Theile --- src/backend/api/config.py | 9 +- src/backend/api/objects.py | 4 +- src/backend/config.py | 166 ++++++++++++++---- src/backend/tests/unit/api/test_api_config.py | 2 +- src/backend/tests/unit/test_app.py | 35 +++- src/backend/tests/unit/test_config.py | 50 ++++-- src/frontend/src/app/app.module.ts | 7 + .../shared/login-page/login-page.component.ts | 18 +- .../components/top-bar/top-bar.component.html | 8 + .../components/top-bar/top-bar.component.ts | 2 + .../src/app/shared/forms/validators.ts | 12 +- .../app/shared/pipes/basename.pipe.spec.ts | 10 +- .../src/app/shared/pipes/basename.pipe.ts | 6 +- .../shared/services/api/s3-bucket.service.ts | 6 +- .../services/api/s3gw-api.service.spec.ts | 4 +- .../shared/services/api/s3gw-api.service.ts | 12 +- ...pec.ts => app-main-config.service.spec.ts} | 16 +- .../services/app-main-config.service.ts | 57 ++++++ .../shared/services/s3gw-config.service.ts | 20 --- .../vendor/longhorn/assets/app.config.json | 2 +- src/s3gw_ui_backend.py | 29 ++- 21 files changed, 364 insertions(+), 111 deletions(-) rename src/frontend/src/app/shared/services/{s3gw-config.service.spec.ts => app-main-config.service.spec.ts} (50%) create mode 100644 src/frontend/src/app/shared/services/app-main-config.service.ts delete mode 100644 src/frontend/src/app/shared/services/s3gw-config.service.ts diff --git a/src/backend/api/config.py b/src/backend/api/config.py index 86b1cf3d..ce58f0a2 100644 --- a/src/backend/api/config.py +++ b/src/backend/api/config.py @@ -20,7 +20,10 @@ class ConfigResponse(BaseModel): - endpoint: str + Endpoint: str + InstanceId: str + Delimiter: str + ApiPath: str router = APIRouter(prefix="/config", tags=["config"]) @@ -31,5 +34,5 @@ class ConfigResponse(BaseModel): response_model=ConfigResponse, ) async def get_config(req: Request) -> ConfigResponse: - cfg: Config = req.app.state.config - return ConfigResponse(endpoint=cfg.s3gw_addr) + config: Config = req.app.state.config + return ConfigResponse.parse_obj(config.to_dict()) diff --git a/src/backend/api/objects.py b/src/backend/api/objects.py index 74b9e05b..9be11820 100644 --- a/src/backend/api/objects.py +++ b/src/backend/api/objects.py @@ -93,7 +93,9 @@ class ObjectBodyStreamingResponse(StreamingResponse): Helper class to stream the object body. """ - def __init__(self, conn: S3GWClientDep, bucket: str, params: ObjectRequest): + def __init__( + self, conn: S3GWClientDep, bucket: str, params: ObjectRequest + ): # noqa # Note, do not call the parent class constructor which does some # initializations that we don't want at the moment. These are # done at a later stage, e.g. in the `stream_response` method. diff --git a/src/backend/config.py b/src/backend/config.py index d8ac2d3f..18b792f8 100644 --- a/src/backend/config.py +++ b/src/backend/config.py @@ -12,10 +12,11 @@ # See the License for the specific language governing permissions and # limitations under the License. +import json import os import re from enum import Enum, EnumMeta -from typing import Any +from typing import Any, Callable, Dict, Optional, Type, TypeVar from fastapi.logger import logger @@ -31,31 +32,104 @@ class S3AddressingStyle(Enum, metaclass=S3AddressingStyleEnumMeta): PATH = "path" -def get_s3gw_address() -> str: - """Obtain s3gw service address from environment, and validate format.""" +class EnvironMalformedError(Exception): + def __init__(self, key: str) -> None: + super().__init__( + f"The value of the environment variable {key} is malformed" + ) + + +class EnvironMissingError(Exception): + def __init__(self, key: str) -> None: + super().__init__(f"The environment variable {key} is not set") + + +ET = TypeVar("ET", bound=Enum) # Enum type. + + +def get_environ_str( + key: str, + default: Optional[str] = None, + cb: Optional[Callable[[str, str | None], str]] = None, +) -> str: + """ + Helper function to obtain a string value from an environment variable. + :param key: The name of the environment variable. + :param default: The default value if the variable does not exist. + Defaults to an empty string. + :param cb: An optional callback function that is used to post-process + the value from the environment variable. The callback function + must return a string. + :return: The content of the specified environment variable as string. + If the environment variable is not set, an empty string is returned. + """ + value: str | None = os.environ.get(key, default) + if cb: + value = cb(key, value) + else: + value = "" if value is None else value + logger.info(f"Using {key}={value}") + return value - url = os.environ.get("S3GW_SERVICE_URL") - if url is None: - logger.error("S3GW_SERVICE_URL env variable not set!") - raise Exception("S3GW_SERVICE_URL env variable not set") - m = re.fullmatch(r"https?://[\w.-]+(?:\.[\w]+)?(?::\d+)?/?", url) - if m is None: - logger.error(f"Malformed s3gw URL: {url}") - raise Exception("Malformed URL") +def get_environ_enum(enum_cls: Type[ET], key: str, default: Any) -> ET: + """ + Helper function to obtain an enum value from an environment variable. + :param enum_cls: The enum class to be used. + Note for Python < 3.12: Make sure the `Enum` metaclass overrides + the __contains__ dunder method to do not throw an exception if + the checked value does not exist. + :param key: The name of the environment variable. + :param default: The default value if the variable does not exist. + Defaults to an empty string. + :return: The content of the specified environment variable as enum. + """ + value: Any = os.environ.get(key, default) + if value not in enum_cls: + value = default + logger.info(f"Using {key}={value}") + return enum_cls(value) + + +def get_s3gw_address() -> str: + """ + Obtain s3gw service address from environment, and validate format. + """ + + def post_process(key: str, value: str | None) -> str: + if value is None: + logger.error(f"The environment variable {key} is not set") + raise EnvironMissingError(key) + m = re.fullmatch(r"https?://[\w.-]+(?:\.[\w]+)?(?::\d+)?/?", value) + if m is None: + logger.error( + f"Malformed value in environment variable {key}: {value}" + ) + raise EnvironMalformedError(key) + return value + + url = get_environ_str("S3GW_SERVICE_URL", cb=post_process) return url def get_ui_path() -> str: - """Obtain the path under which the UI should be served, e.g. /ui""" - path = os.environ.get("S3GW_UI_PATH") - if path is None: - return "/" - match = re.fullmatch(r"/?[\w.-/]+(?:[\w]+)/?", path) - if match is None: - logger.error(f"Malformed path for UI: {path}") - raise Exception("Malformed UI path") - return path if path.startswith("/") else f"/{path}" + """ + Obtain the path under which the UI should be served, e.g. `/ui`. + """ + + def post_process(key: str, value: str | None) -> str: + if value is None: + return "/" + match = re.fullmatch(r"/?[\w.-/]+(?:[\w]+)/?", value) + if match is None: + logger.error( + f"The value of the environment variable {key} is malformed: {value}" # noqa: E501 + ) + raise EnvironMalformedError(key) + return value if value.startswith("/") else f"/{value}" + + path = get_environ_str("S3GW_UI_PATH", cb=post_process) + return path def get_api_path(ui_path: str) -> str: @@ -67,34 +141,30 @@ def get_api_path(ui_path: str) -> str: return f"{ui_path.rstrip('/')}/api" -def get_s3_addressing_style() -> S3AddressingStyle: +class Config: """ - Obtain the S3 addressing style. Defaults to `auto`. + Keeps config relevant for the backend's operation. """ - addressing_style: str = os.environ.get( - "S3GW_S3_ADDRESSING_STYLE", "auto" - ).lower() - if addressing_style not in S3AddressingStyle: - addressing_style = S3AddressingStyle.AUTO.value - logger.info(f"Using '{addressing_style}' S3 addressing style") - return S3AddressingStyle(addressing_style) - - -class Config: - """Keeps config relevant for the backend's operation.""" # Address for the s3gw instance we're servicing. _s3gw_addr: str _s3_addressing_style: S3AddressingStyle + _s3_prefix_delimiter: str _ui_path: str _api_path: str + _instance_id: str def __init__(self) -> None: self._s3gw_addr = get_s3gw_address() - self._s3_addressing_style = get_s3_addressing_style() + self._s3_addressing_style = get_environ_enum( + S3AddressingStyle, "S3GW_S3_ADDRESSING_STYLE", "auto" + ) + self._s3_prefix_delimiter = get_environ_str( + "S3GW_S3_PREFIX_DELIMITER", "/" + ) self._ui_path = get_ui_path() self._api_path = get_api_path(self._ui_path) - logger.info(f"Servicing s3gw at {self._s3gw_addr}") + self._instance_id = get_environ_str("S3GW_INSTANCE_ID") @property def s3gw_addr(self) -> str: @@ -117,3 +187,29 @@ def s3_addressing_style(self) -> S3AddressingStyle: Obtain the S3 addressing style. """ return self._s3_addressing_style + + @property + def s3_prefix_delimiter(self) -> str: + """ + The prefix delimiter. Defaults to `/`. See + https://docs.aws.amazon.com/AmazonS3/latest/userguide/using-prefixes.html + """ + return self._s3_prefix_delimiter + + @property + def instance_id(self) -> str: + """ + Obtain the instance identifier. Defaults to an empty string. + """ + return self._instance_id + + def to_dict(self) -> Dict[str, Any]: + return { + "ApiPath": self.api_path, + "Delimiter": self.s3_prefix_delimiter, + "Endpoint": self.s3gw_addr, + "InstanceId": self.instance_id, + } + + def to_json(self) -> str: + return json.dumps(self.to_dict()) diff --git a/src/backend/tests/unit/api/test_api_config.py b/src/backend/tests/unit/api/test_api_config.py index c7291c40..9e5690fb 100644 --- a/src/backend/tests/unit/api/test_api_config.py +++ b/src/backend/tests/unit/api/test_api_config.py @@ -38,4 +38,4 @@ class MockApp: req.app.state.config = test_config res = await api_config.get_config(req) assert isinstance(res, api_config.ConfigResponse) - assert res.endpoint == "http://foo.bar:123" + assert res.Endpoint == "http://foo.bar:123" diff --git a/src/backend/tests/unit/test_app.py b/src/backend/tests/unit/test_app.py index ce6e8249..4cf3f68f 100644 --- a/src/backend/tests/unit/test_app.py +++ b/src/backend/tests/unit/test_app.py @@ -12,14 +12,21 @@ # See the License for the specific language governing permissions and # limitations under the License. +import logging import os from pathlib import Path, PosixPath import pytest from fastapi import FastAPI from fastapi.testclient import TestClient +from pytest_mock import MockerFixture -from s3gw_ui_backend import NoCacheStaticFiles, app_factory +from s3gw_ui_backend import ( + NoCacheStaticFiles, + app_factory, + lifespan, + s3gw_factory, +) @pytest.fixture @@ -79,3 +86,29 @@ def test_config_init() -> None: assert ui_resp.status_code == 200 api_resp = client.get("/s3gwui/api/buckets/") assert api_resp.status_code == 422 + + +def test_config_init_failure(caplog: pytest.LogCaptureFixture) -> None: + caplog.set_level(logging.ERROR) + os.environ.pop("S3GW_SERVICE_URL", None) + with pytest.raises(SystemExit) as e: + s3gw_factory("/foo") + assert e.type == SystemExit + assert e.value.code == 1 + assert ( + "The environment variable S3GW_SERVICE_URL is not set" + in caplog.messages[0] + ) + assert "Unable to init config -- exit!" in caplog.messages[-1] + + +@pytest.mark.anyio +async def test_lifespan( + caplog: pytest.LogCaptureFixture, mocker: MockerFixture +) -> None: + caplog.set_level(logging.INFO) + app = FastAPI() + async with lifespan(app): + pass + assert "Starting s3gw-ui backend" in caplog.messages[0] + assert "Shutting down s3gw-ui backend" in caplog.messages[-1] diff --git a/src/backend/tests/unit/test_config.py b/src/backend/tests/unit/test_config.py index 6a130025..50089913 100644 --- a/src/backend/tests/unit/test_config.py +++ b/src/backend/tests/unit/test_config.py @@ -22,7 +22,8 @@ from backend.config import ( Config, S3AddressingStyle, - get_s3_addressing_style, + get_environ_enum, + get_environ_str, get_s3gw_address, get_ui_path, ) @@ -41,7 +42,10 @@ def test_s3gw_malformed_address() -> None: os.environ["S3GW_SERVICE_URL"] = url with pytest.raises(Exception) as e: get_s3gw_address() - assert str(e.value) == "Malformed URL" + assert ( + str(e.value) + == "The value of the environment variable S3GW_SERVICE_URL is malformed" # noqa: E501 + ) def test_s3gw_good_address() -> None: @@ -58,7 +62,9 @@ def test_s3gw_missing_address() -> None: os.environ.pop("S3GW_SERVICE_URL") with pytest.raises(Exception) as e: get_s3gw_address() - assert str(e.value) == "S3GW_SERVICE_URL env variable not set" + assert ( + str(e.value) == "The environment variable S3GW_SERVICE_URL is not set" + ) def test_config_1() -> None: @@ -97,7 +103,10 @@ def test_malformed_ui_path() -> None: os.environ["S3GW_UI_PATH"] = loc with pytest.raises(Exception) as e: get_ui_path() - assert str(e.value) == "Malformed UI path" + assert ( + str(e.value) + == "The value of the environment variable S3GW_UI_PATH is malformed" + ) def test_good_ui_path() -> None: @@ -139,16 +148,37 @@ def test_api_path_with_trailing_slash() -> None: pytest.fail(str(e)) -def test_get_s3_addressing_style_1() -> None: +def test_get_environ_enum_1() -> None: os.environ["S3GW_S3_ADDRESSING_STYLE"] = "foo" - assert S3AddressingStyle.AUTO == get_s3_addressing_style() + assert S3AddressingStyle.AUTO == get_environ_enum( + S3AddressingStyle, "S3GW_S3_ADDRESSING_STYLE", "auto" + ) -def test_get_s3_addressing_style_2() -> None: +def test_get_environ_enum_2() -> None: os.environ["S3GW_S3_ADDRESSING_STYLE"] = "VIRTUAL" - assert S3AddressingStyle.VIRTUAL == get_s3_addressing_style() + assert S3AddressingStyle.AUTO == get_environ_enum( + S3AddressingStyle, "S3GW_S3_ADDRESSING_STYLE", "auto" + ) -def test_get_s3_addressing_style_3() -> None: +def test_get_environ_enum_3() -> None: os.environ.pop("S3GW_S3_ADDRESSING_STYLE", None) - assert S3AddressingStyle.AUTO == get_s3_addressing_style() + assert S3AddressingStyle.AUTO == get_environ_enum( + S3AddressingStyle, "S3GW_S3_ADDRESSING_STYLE", "auto" + ) + + +def test_get_environ_str_1() -> None: + os.environ["S3GW_S3_PREFIX_DELIMITER"] = "|" + assert "|" == get_environ_str("S3GW_S3_PREFIX_DELIMITER") + + +def test_get_environ_str_2() -> None: + os.environ.pop("S3GW_S3_PREFIX_DELIMITER", None) + assert "&" == get_environ_str("S3GW_S3_PREFIX_DELIMITER", "&") + + +def test_get_environ_str_3() -> None: + os.environ.pop("S3GW_INSTANCE_ID", None) + assert "" == get_environ_str("S3GW_INSTANCE_ID") diff --git a/src/frontend/src/app/app.module.ts b/src/frontend/src/app/app.module.ts index 8fca103b..df466626 100644 --- a/src/frontend/src/app/app.module.ts +++ b/src/frontend/src/app/app.module.ts @@ -10,6 +10,7 @@ import { AppRoutingModule } from '~/app/app-routing.module'; import { getCurrentLanguage, setTranslationService } from '~/app/i18n.helper'; import { PagesModule } from '~/app/pages/pages.module'; import { AppConfigService } from '~/app/shared/services/app-config.service'; +import { AppMainConfigService } from '~/app/shared/services/app-main-config.service'; import { HttpErrorInterceptorService } from '~/app/shared/services/http-error-interceptor.service'; import { SharedModule } from '~/app/shared/shared.module'; import { TranslocoRootModule } from '~/app/transloco-root.module'; @@ -37,6 +38,12 @@ import { TranslocoRootModule } from '~/app/transloco-root.module'; multi: true, deps: [AppConfigService] }, + { + provide: APP_INITIALIZER, + useFactory: (appMainConfigService: AppMainConfigService) => () => appMainConfigService.load(), + multi: true, + deps: [AppMainConfigService] + }, { provide: APP_INITIALIZER, useFactory: (translocoService: TranslocoService) => () => { diff --git a/src/frontend/src/app/pages/shared/login-page/login-page.component.ts b/src/frontend/src/app/pages/shared/login-page/login-page.component.ts index f2d5ae32..922bc4e4 100644 --- a/src/frontend/src/app/pages/shared/login-page/login-page.component.ts +++ b/src/frontend/src/app/pages/shared/login-page/login-page.component.ts @@ -9,9 +9,9 @@ import { DeclarativeFormComponent } from '~/app/shared/components/declarative-fo import { DeclarativeFormConfig } from '~/app/shared/models/declarative-form-config.type'; import { AuthResponse, AuthService } from '~/app/shared/services/api/auth.service'; import { AppConfigService } from '~/app/shared/services/app-config.service'; +import { AppMainConfigService } from '~/app/shared/services/app-main-config.service'; import { BlockUiService } from '~/app/shared/services/block-ui.service'; import { DialogService } from '~/app/shared/services/dialog.service'; -import { NotificationService } from '~/app/shared/services/notification.service'; @Component({ selector: 's3gw-login-page', @@ -57,19 +57,31 @@ export class LoginPageComponent implements OnInit { constructor( private appConfigService: AppConfigService, + private appMainConfigService: AppMainConfigService, private authService: AuthService, private blockUiService: BlockUiService, private dialogService: DialogService, - private notificationService: NotificationService, private router: Router ) { - this.welcomeMessage = translate(TEXT('Welcome to {{ name }}'), this.appConfigService.config); + this.welcomeMessage = translate(TEXT('Welcome to {{ title }}'), this.appConfigService.config); } ngOnInit(): void { this.blockUiService.resetGlobal(); // Ensure all open modal dialogs are closed. this.dialogService.dismissAll(); + // Add the additional `Instance` field to the form if the + // `instanceId` is available. + if (this.appMainConfigService.config.InstanceId) { + this.config.fields.unshift({ + name: 'instanceId', + type: 'text', + label: TEXT('Instance'), + value: this.appMainConfigService.config.InstanceId, + readonly: true, + submitValue: false + }); + } } onLogin(): void { diff --git a/src/frontend/src/app/shared/components/top-bar/top-bar.component.html b/src/frontend/src/app/shared/components/top-bar/top-bar.component.html index 6680b2e3..49a43561 100644 --- a/src/frontend/src/app/shared/components/top-bar/top-bar.component.html +++ b/src/frontend/src/app/shared/components/top-bar/top-bar.component.html @@ -11,7 +11,9 @@ alt="logo"> +
+
@@ -29,6 +31,12 @@
+ + +
{{ appMainConfigService.config.InstanceId }}
+
+ +