From d45d49867296bb5833a17260df82eecdb1b50257 Mon Sep 17 00:00:00 2001 From: John Chilton Date: Mon, 22 Apr 2024 10:22:50 -0400 Subject: [PATCH] Refine object store templates backend. --- client/src/api/schema/schema.ts | 4 +- lib/galaxy/app.py | 1 + lib/galaxy/config/schemas/config_schema.yml | 13 +++ lib/galaxy/managers/object_store_instances.py | 84 ++++++++++++------- lib/galaxy/model/__init__.py | 19 ++--- ...3c93d66a_add_user_defined_object_stores.py | 16 ++-- lib/galaxy/objectstore/__init__.py | 18 ++-- lib/galaxy/webapps/galaxy/api/object_store.py | 4 +- .../test_from_configuration_object.py | 1 + 9 files changed, 104 insertions(+), 56 deletions(-) diff --git a/client/src/api/schema/schema.ts b/client/src/api/schema/schema.ts index fcc0839f7b37..cadaa01f31ab 100644 --- a/client/src/api/schema/schema.ts +++ b/client/src/api/schema/schema.ts @@ -20669,7 +20669,7 @@ export interface operations { header?: { "run-as"?: string | null; }; - /** @description The model ID for a persisted UserObjectStore object. */ + /** @description The identifier used to index a persisted UserObjectStore object. */ path: { user_object_store_id: string; }; @@ -20696,7 +20696,7 @@ export interface operations { header?: { "run-as"?: string | null; }; - /** @description The model ID for a persisted UserObjectStore object. */ + /** @description The identifier used to index a persisted UserObjectStore object. */ path: { user_object_store_id: string; }; diff --git a/lib/galaxy/app.py b/lib/galaxy/app.py index 0e871e5deeab..682b85fc751b 100644 --- a/lib/galaxy/app.py +++ b/lib/galaxy/app.py @@ -418,6 +418,7 @@ def _configure_object_store(self, **kwds): umask=self.config.umask, object_store_cache_size=self.config.object_store_cache_size, object_store_cache_path=self.config.object_store_cache_path, + user_object_store_index_by=self.config.user_object_store_index_by, ) self._register_singleton(UserObjectStoresAppConfig, app_config) user_object_store_resolver = self._register_abstract_singleton( diff --git a/lib/galaxy/config/schemas/config_schema.yml b/lib/galaxy/config/schemas/config_schema.yml index b13dd58fef5c..1f5c79c1ffbd 100644 --- a/lib/galaxy/config/schemas/config_schema.yml +++ b/lib/galaxy/config/schemas/config_schema.yml @@ -566,6 +566,19 @@ mapping: desc: | Configured Object Store templates embedded into Galaxy's config. + user_object_store_index_by: + type: str + default: 'uuid' + required: false + enum: ['uuid', 'id'] + desc: | + Configure URIs for user object stores to use either the object ID ('id') + or UUIDs ('uuid'). Either is fine really, Galaxy doesn't typically expose + database objects by 'id' but there isn't any obvious disadvantage to doing + it in this case and it keeps user exposed URIs much smaller. The default of + UUID feels a little more like a typical way to do this within Galaxy though. + Do not change this value once user object stores have been created. + enable_mulled_containers: type: bool default: true diff --git a/lib/galaxy/managers/object_store_instances.py b/lib/galaxy/managers/object_store_instances.py index 893adef27302..d5a51b905e3d 100644 --- a/lib/galaxy/managers/object_store_instances.py +++ b/lib/galaxy/managers/object_store_instances.py @@ -9,11 +9,13 @@ import logging from typing import ( + Any, Dict, List, Optional, Union, ) +from uuid import uuid4 from pydantic import BaseModel @@ -92,16 +94,19 @@ class UserConcreteObjectStoreModel(ConcreteObjectStoreModel): class ObjectStoreInstancesManager: _catalog: ConfiguredObjectStoreTemplates _sa_session: galaxy_scoped_session + _app_config: UserObjectStoresAppConfig def __init__( self, catalog: ConfiguredObjectStoreTemplates, sa_session: galaxy_scoped_session, vault: Vault, + app_config: UserObjectStoresAppConfig, ): self._catalog = catalog self._sa_session = sa_session self._app_vault = vault + self._app_config = app_config @property def summaries(self) -> ObjectStoreTemplateSummaries: @@ -123,10 +128,10 @@ def _upgrade_instance( ) -> UserConcreteObjectStoreModel: persisted_object_store = self._get(trans, id) catalog = self._catalog - template = catalog.find_template_by(persisted_object_store.object_store_template_id, payload.template_version) - persisted_object_store.object_store_template_version = template.version - persisted_object_store.object_store_template_definition = template.model_dump() - old_variables = persisted_object_store.object_store_template_variables or {} + template = catalog.find_template_by(persisted_object_store.template_id, payload.template_version) + persisted_object_store.template_version = template.version + persisted_object_store.template_definition = template.model_dump() + old_variables = persisted_object_store.template_variables or {} updated_variables = payload.variables actual_variables: OBJECT_STORE_TEMPLATE_CONFIGURATION_VARIABLES_TYPE = {} for variable in template.variables or []: @@ -136,11 +141,11 @@ def _upgrade_instance( if updated_value: actual_variables[variable_name] = updated_value - persisted_object_store.object_store_template_variables = actual_variables - old_secrets = persisted_object_store.object_store_template_secrets or [] + persisted_object_store.template_variables = actual_variables + old_secrets = persisted_object_store.template_secrets or [] new_secrets = payload.secrets - recorded_secrets = persisted_object_store.object_store_template_secrets or [] + recorded_secrets = persisted_object_store.template_secrets or [] user_vault = trans.user_vault upgraded_template_secrets = [] @@ -154,7 +159,7 @@ def _upgrade_instance( continue secret_value = new_secrets[secret_name] - key = user_vault_key(persisted_object_store, secret_name) + key = user_vault_key(persisted_object_store, secret_name, self._app_config) user_vault.write_secret(key, secret_value) if secret_name not in recorded_secrets: recorded_secrets.append(secret_name) @@ -162,7 +167,7 @@ def _upgrade_instance( secrets_to_delete: List[str] = [] for recorded_secret in recorded_secrets: if recorded_secret not in upgraded_template_secrets: - key = user_vault_key(persisted_object_store, recorded_secret) + key = user_vault_key(persisted_object_store, recorded_secret, self._app_config) log.info(f"deleting {key} from user vault") user_vault.delete_secret(key) secrets_to_delete.append(recorded_secret) @@ -170,7 +175,7 @@ def _upgrade_instance( for secret_to_delete in secrets_to_delete: recorded_secrets.remove(secret_to_delete) - persisted_object_store.object_store_template_secrets = recorded_secrets + persisted_object_store.template_secrets = recorded_secrets self._save(persisted_object_store) rval = self._to_model(trans, persisted_object_store) return rval @@ -188,7 +193,7 @@ def _update_instance( persisted_object_store.description = payload.description if payload.variables is not None: # maybe just record the valid variables according to template like in upgrade - persisted_object_store.object_store_template_variables = payload.variables + persisted_object_store.template_variables = payload.variables self._save(persisted_object_store) return self._to_model(trans, persisted_object_store) @@ -197,7 +202,7 @@ def _update_instance_secret( ) -> UserConcreteObjectStoreModel: persisted_object_store = self._get(trans, id) user_vault = trans.user_vault - key = user_vault_key(persisted_object_store, payload.secret_name) + key = user_vault_key(persisted_object_store, payload.secret_name, self._app_config) user_vault.write_secret(key, payload.secret_value) return self._to_model(trans, persisted_object_store) @@ -212,10 +217,11 @@ def create_instance( persisted_object_store = UserObjectStore() persisted_object_store.user_id = trans.user.id assert persisted_object_store.user_id - persisted_object_store.object_store_template_definition = template.model_dump() - persisted_object_store.object_store_template_id = template.id - persisted_object_store.object_store_template_version = template.version - persisted_object_store.object_store_template_variables = payload.variables + persisted_object_store.uuid = uuid4().hex + persisted_object_store.template_definition = template.model_dump() + persisted_object_store.template_id = template.id + persisted_object_store.template_version = template.version + persisted_object_store.template_variables = payload.variables persisted_object_store.name = payload.name persisted_object_store.description = payload.description self._save(persisted_object_store) @@ -231,13 +237,13 @@ def create_instance( recorded_secrets = [] try: for secret, value in payload.secrets.items(): - key = user_vault_key(persisted_object_store, secret) + key = user_vault_key(persisted_object_store, secret, self._app_config) user_vault.write_secret(key, value) recorded_secrets.append(secret) except Exception: self._sa_session.delete(persisted_object_store) raise - persisted_object_store.object_store_template_secrets = recorded_secrets + persisted_object_store.template_secrets = recorded_secrets self._save(persisted_object_store) return self._to_model(trans, persisted_object_store) @@ -274,32 +280,43 @@ def _to_model(self, trans, persisted_object_store: UserObjectStore) -> UserConcr object_store_type in ["azure_blob", "s3"], ) # These shouldn't be null but sometimes can be? - secrets = persisted_object_store.object_store_template_secrets or [] + secrets = persisted_object_store.template_secrets or [] + uos_id: str + if self._app_config.user_object_store_index_by == "id": + uos_id = str(persisted_object_store.id) + else: + uos_id = str(persisted_object_store.uuid) + object_store_id = f"user_objects://{uos_id}" return UserConcreteObjectStoreModel( id=persisted_object_store.id, type=object_store_type, - template_id=persisted_object_store.object_store_template_id, - template_version=persisted_object_store.object_store_template_version, - variables=persisted_object_store.object_store_template_variables, + template_id=persisted_object_store.template_id, + template_version=persisted_object_store.template_version, + variables=persisted_object_store.template_variables, secrets=secrets, name=persisted_object_store.name, description=persisted_object_store.description, - object_store_id=f"user_objects://{persisted_object_store.id}", + object_store_id=object_store_id, private=True, quota=quota, badges=badges, ) -def user_vault_key(user_object_store: UserObjectStore, secret: str) -> str: - uos_id = user_object_store.id +def user_vault_key(user_object_store: UserObjectStore, secret: str, app_config: UserObjectStoresAppConfig) -> str: + if app_config.user_object_store_index_by == "id": + uos_id = str(user_object_store.id) + else: + uos_id = str(user_object_store.uuid) assert uos_id user_vault_id_prefix = f"object_store_config/{uos_id}" key = f"{user_vault_id_prefix}/{secret}" return key -def recover_secrets(user_object_store: UserObjectStore, vault: Vault) -> Dict[str, str]: +def recover_secrets( + user_object_store: UserObjectStore, vault: Vault, app_config: UserObjectStoresAppConfig +) -> Dict[str, str]: user: User = user_object_store.user user_vault = UserVaultWrapper(vault, user) secrets: Dict[str, str] = {} @@ -307,9 +324,9 @@ def recover_secrets(user_object_store: UserObjectStore, vault: Vault) -> Dict[st # ones recorded as written in the persisted object, the ones # expected in the catalog, or the ones expected in the definition # persisted. - persisted_secret_names = user_object_store.object_store_template_secrets or [] + persisted_secret_names = user_object_store.template_secrets or [] for secret in persisted_secret_names: - vault_key = user_vault_key(user_object_store, secret) + vault_key = user_vault_key(user_object_store, secret, app_config) secret_value = user_vault.read_secret(vault_key) # assert secret_value if secret_value is not None: @@ -325,8 +342,13 @@ def __init__(self, sa_session: galaxy_scoped_session, vault: Vault, app_config: def resolve_object_store_uri_config(self, uri: str) -> ObjectStoreConfiguration: user_object_store_id = uri.split("://", 1)[1] - id_filter = UserObjectStore.__table__.c.id == user_object_store_id - user_object_store: UserObjectStore = self._sa_session.query(UserObjectStore).filter(id_filter).one() - secrets = recover_secrets(user_object_store, self._vault) + index_by = self._app_config.user_object_store_index_by + index_filter: Any + if index_by == "id": + index_filter = UserObjectStore.__table__.c.id == user_object_store_id + else: + index_filter = UserObjectStore.__table__.c.uuid == user_object_store_id + user_object_store: UserObjectStore = self._sa_session.query(UserObjectStore).filter(index_filter).one() + secrets = recover_secrets(user_object_store, self._vault, self._app_config) object_store_configuration = user_object_store.object_store_configuration(secrets=secrets) return object_store_configuration diff --git a/lib/galaxy/model/__init__.py b/lib/galaxy/model/__init__.py index 3c3167141796..9a4f39a743af 100644 --- a/lib/galaxy/model/__init__.py +++ b/lib/galaxy/model/__init__.py @@ -10889,6 +10889,7 @@ class UserObjectStore(Base, RepresentById): id: Mapped[int] = mapped_column(Integer, primary_key=True) user_id: Mapped[Optional[int]] = mapped_column(Integer, ForeignKey("galaxy_user.id"), index=True) + uuid: Mapped[Union[UUID, str]] = mapped_column(UUIDType(), index=True) create_time: Mapped[datetime] = mapped_column(DateTime, default=now) update_time: Mapped[datetime] = mapped_column(DateTime, default=now, onupdate=now, index=True) # user specified name of the instance they've created @@ -10896,30 +10897,26 @@ class UserObjectStore(Base, RepresentById): # user specified description of the instance they've created description: Mapped[Optional[str]] = mapped_column(Text) # the template store id - object_store_template_id: Mapped[str] = mapped_column(String(255), index=True) + template_id: Mapped[str] = mapped_column(String(255), index=True) # the template store version (0, 1, ...) - object_store_template_version: Mapped[int] = mapped_column(Integer, index=True) + template_version: Mapped[int] = mapped_column(Integer, index=True) # Full template from object_store_templates.yml catalog. # For tools we just store references, so here we could easily just use # the id/version and not record the definition... as the templates change # over time this choice has some big consequences despite being easy to swap # implementations. - object_store_template_definition: Mapped[Optional[OBJECT_STORE_TEMPLATE_DEFINITION_TYPE]] = mapped_column(JSONType) + template_definition: Mapped[Optional[OBJECT_STORE_TEMPLATE_DEFINITION_TYPE]] = mapped_column(JSONType) # Big JSON blob of the variable name -> value mapping defined for the store's # variables by the user. - object_store_template_variables: Mapped[Optional[OBJECT_STORE_TEMPLATE_CONFIGURATION_VARIABLES_TYPE]] = ( - mapped_column(JSONType) - ) + template_variables: Mapped[Optional[OBJECT_STORE_TEMPLATE_CONFIGURATION_VARIABLES_TYPE]] = mapped_column(JSONType) # Track a list of secrets that were defined for this object store at creation - object_store_template_secrets: Mapped[Optional[OBJECT_STORE_TEMPLATE_CONFIGURATION_SECRET_NAMES_TYPE]] = ( - mapped_column(JSONType) - ) + template_secrets: Mapped[Optional[OBJECT_STORE_TEMPLATE_CONFIGURATION_SECRET_NAMES_TYPE]] = mapped_column(JSONType) user = relationship("User", back_populates="object_stores") @property def template(self) -> ObjectStoreTemplate: - return ObjectStoreTemplate(**self.object_store_template_definition or {}) + return ObjectStoreTemplate(**self.template_definition or {}) def object_store_configuration(self, secrets: Dict[str, Any]) -> ObjectStoreConfiguration: user = self.user @@ -10928,7 +10925,7 @@ def object_store_configuration(self, secrets: Dict[str, Any]) -> ObjectStoreConf "email": user.email, "id": user.id, } - variables: OBJECT_STORE_TEMPLATE_CONFIGURATION_VARIABLES_TYPE = self.object_store_template_variables or {} + variables: OBJECT_STORE_TEMPLATE_CONFIGURATION_VARIABLES_TYPE = self.template_variables or {} return template_to_configuration( self.template, variables=variables, diff --git a/lib/galaxy/model/migrations/alembic/versions_gxy/c14a3c93d66a_add_user_defined_object_stores.py b/lib/galaxy/model/migrations/alembic/versions_gxy/c14a3c93d66a_add_user_defined_object_stores.py index 47ef643059f7..09d296af5a74 100644 --- a/lib/galaxy/model/migrations/alembic/versions_gxy/c14a3c93d66a_add_user_defined_object_stores.py +++ b/lib/galaxy/model/migrations/alembic/versions_gxy/c14a3c93d66a_add_user_defined_object_stores.py @@ -15,7 +15,10 @@ Text, ) -from galaxy.model.custom_types import JSONType +from galaxy.model.custom_types import ( + JSONType, + UUIDType, +) from galaxy.model.migrations.util import ( create_table, drop_table, @@ -37,15 +40,16 @@ def upgrade(): table_name, Column("id", Integer, primary_key=True), Column("user_id", Integer, ForeignKey("galaxy_user.id"), nullable=False, index=True), + Column("uuid", UUIDType, nullable=False, index=True), Column("name", String(255), index=True), Column("description", Text, index=True), Column("create_time", DateTime), Column("update_time", DateTime), - Column("object_store_template_id", String(255), index=True), - Column("object_store_template_version", Integer, index=True), - Column("object_store_template_definition", JSONType), - Column("object_store_template_variables", JSONType), - Column("object_store_template_secrets", JSONType), + Column("template_id", String(255), index=True), + Column("template_version", Integer, index=True), + Column("template_definition", JSONType), + Column("template_variables", JSONType), + Column("template_secrets", JSONType), ) diff --git a/lib/galaxy/objectstore/__init__.py b/lib/galaxy/objectstore/__init__.py index 205859833014..f4bc39b4d899 100644 --- a/lib/galaxy/objectstore/__init__.py +++ b/lib/galaxy/objectstore/__init__.py @@ -26,7 +26,7 @@ import yaml from pydantic import BaseModel -from typing_extensions import Protocol +from typing_extensions import Literal, Protocol from galaxy.exceptions import ( ConfigDoesNotAllowException, @@ -1360,10 +1360,17 @@ def validate_selected_object_store_id(self, user, object_store_id: Optional[str] if not user: return "Supplied object store id is not accessible" rest_of_uri = object_store_id.split("://", 1)[1] - user_object_store_id = int(rest_of_uri) - for user_object_store in user.object_stores: - if user_object_store.id == user_object_store_id: - return None + index_by = self.config.user_object_store_index_by + if index_by == "id": + user_object_store_id = int(rest_of_uri) + for user_object_store in user.object_stores: + if user_object_store.id == user_object_store_id: + return None + else: + user_object_store_uuid = rest_of_uri + for user_object_store in user.object_stores: + if str(user_object_store.uuid) == user_object_store_uuid: + return None return "Supplied object store id was not found" if object_store_id not in self.object_store_ids_allowing_selection(): return "Supplied object store id is not an allowed object store selection" @@ -1624,6 +1631,7 @@ def build_object_store_from_config( class UserObjectStoresAppConfig(BaseModel): object_store_cache_path: str object_store_cache_size: int + user_object_store_index_by: Literal["uuid", "id"] jobs_directory: str new_file_path: str umask: int diff --git a/lib/galaxy/webapps/galaxy/api/object_store.py b/lib/galaxy/webapps/galaxy/api/object_store.py index 42851c940e1f..905318604462 100644 --- a/lib/galaxy/webapps/galaxy/api/object_store.py +++ b/lib/galaxy/webapps/galaxy/api/object_store.py @@ -42,7 +42,9 @@ ) UserObjectStoreIdPathParam: str = Path( - ..., title="User Object Store ID", description="The model ID for a persisted UserObjectStore object." + ..., + title="User Object Store Identifier", + description="The identifier used to index a persisted UserObjectStore object.", ) SelectableQueryParam: bool = Query( diff --git a/test/unit/objectstore/test_from_configuration_object.py b/test/unit/objectstore/test_from_configuration_object.py index e1e9dac6edee..3829bd64fd7b 100644 --- a/test/unit/objectstore/test_from_configuration_object.py +++ b/test/unit/objectstore/test_from_configuration_object.py @@ -41,5 +41,6 @@ def app_config(tmpdir) -> UserObjectStoresAppConfig: umask=0o077, object_store_cache_path=str(tmpdir / "cache"), object_store_cache_size=1, + user_object_store_index_by="uuid", ) return app_config