Skip to content

feat(BA-5506): Add vfolder host permission filter#10683

Open
seedspirit wants to merge 6 commits intomainfrom
feat/BA-5506
Open

feat(BA-5506): Add vfolder host permission filter#10683
seedspirit wants to merge 6 commits intomainfrom
feat/BA-5506

Conversation

@seedspirit
Copy link
Copy Markdown
Contributor

@seedspirit seedspirit commented Mar 31, 2026

resolve #10672 (BA-5506)

Checklist: (if applicable)

  • Milestone metadata specifying the target backport version
  • Mention to the original issue
  • Installer updates including:
    • Fixtures for db schema changes
    • New mandatory config options
  • Update of end-to-end CLI integration tests in ai.backend.test
  • API server-client counterparts (e.g., manager API -> client SDK)
  • Test case(s) to:
    • Demonstrate the difference of before/after
    • Demonstrate the flow of abstract/conceptual models with a concrete implementation
  • Documentation
    • Contents in the docs directory
    • docstrings in public interfaces and type annotations

📚 Documentation preview 📚: https://sorna--10683.org.readthedocs.build/en/10683/


📚 Documentation preview 📚: https://sorna-ko--10683.org.readthedocs.build/ko/10683/

@github-actions github-actions bot added size:XL 500~ LoC comp:manager Related to Manager component comp:common Related to Common component area:docs Documentations labels Mar 31, 2026
seedspirit added a commit that referenced this pull request Mar 31, 2026
@seedspirit seedspirit requested a review from a team March 31, 2026 07:02
@seedspirit seedspirit marked this pull request as ready for review March 31, 2026 07:02
Copilot AI review requested due to automatic review settings March 31, 2026 07:02
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a new host_permission filter to the VFolder “my_search” query path so callers can filter vfolders by whether the requesting user has specific permissions on the underlying storage host (based on allowed host permissions aggregated from domain/group/keypair-policy sources).

Changes:

  • Implemented VFolderConditions.by_host_permission() to build a DB-side subquery that derives permitted hosts from allowed_vfolder_hosts JSONB sources.
  • Wired host_permission into DTOs and GraphQL vfolder filter inputs/enums.
  • Added unit tests covering domain isolation, group membership differences, shared vfolders, pagination, and combinations with other filters.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
src/ai/backend/manager/models/vfolder/conditions.py Adds SQL condition factory for host-permission-based filtering.
src/ai/backend/manager/api/adapters/vfolder.py Passes requester context into filter conversion and emits host-permission query conditions for my_search.
src/ai/backend/common/dto/manager/v2/vfolder/types.py Introduces DTO enum + HostPermissionFilter model.
src/ai/backend/common/dto/manager/v2/vfolder/request.py Extends VFolderFilter DTO with host_permission.
src/ai/backend/manager/api/gql/vfolder_v2/types/filters.py Adds HostPermissionFilter GQL input and extends VFolderFilter GQL input.
src/ai/backend/manager/api/gql/vfolder_v2/types/enum.py Adds a new GraphQL enum for host permissions.
src/ai/backend/manager/api/gql/vfolder_v2/types/__init__.py Re-exports newly added GQL types.
src/ai/backend/manager/api/gql/vfolder_v2/__init__.py Re-exports newly added GQL types at package level.
tests/unit/manager/repositories/vfolder/test_vfolder_host_permission_filter.py Adds unit tests for the new host-permission filtering behavior.
docs/manager/graphql-reference/v2-schema.graphql Updates rendered schema docs for new filter + enum.
docs/manager/graphql-reference/supergraph.graphql Updates rendered supergraph schema docs for new filter + enum.
changes/10683.feature.md Adds changelog entry for the new filter.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +65 to +90
@gql_enum(
BackendAIGQLMeta(
added_version=NEXT_RELEASE_VERSION,
description=(
"Host-level permission for virtual folder storage hosts. "
"CREATE: Create new vfolders on the host. "
"MODIFY: Rename or update vfolder options. "
"DELETE: Delete vfolders from the host. "
"MOUNT_IN_SESSION: Mount vfolders in compute sessions. "
"UPLOAD_FILE: Upload files to vfolders. "
"DOWNLOAD_FILE: Download files from vfolders. "
"INVITE_OTHERS: Invite other users to user-type vfolders. "
"SET_USER_PERM: Override permission of group-type vfolders."
),
),
name="VFolderHostPermission",
)
class VFolderHostPermissionGQL(StrEnum):
CREATE = "create-vfolder"
MODIFY = "modify-vfolder"
DELETE = "delete-vfolder"
MOUNT_IN_SESSION = "mount-in-session"
UPLOAD_FILE = "upload-file"
DOWNLOAD_FILE = "download-file"
INVITE_OTHERS = "invite-others"
SET_USER_PERM = "set-user-specific-permission"
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This introduces a new GraphQL enum VFolderHostPermission even though an equivalent enum already exists as VFolderHostPermissionV2 (VFolderHostPermissionEnum in src/ai/backend/manager/api/gql/common_types.py:148-166) with the same underlying string values. Having two enums for the same permission set can confuse API consumers and increases maintenance cost; consider reusing the existing enum (or renaming/alising it) instead of defining a parallel one here unless there is a strong compatibility reason.

Copilot uses AI. Check for mistakes.
Comment on lines +274 to +287
# Use literal_column to embed the array directly in SQL, avoiding
# asyncpg parameter binding issues with array/JSONB type coercion.
perm_array_literal: sa.ColumnElement[list[str]] = sa.literal_column(
"ARRAY[" + ",".join(f"'{v}'" for v in perm_values) + "]::text[]"
)
hosts_with_perms = (
sa.select(all_pairs.c[0].label("host_key"))
.group_by(all_pairs.c[0])
.having(
sa.type_coerce(
sa.func.array_agg(sa.distinct(all_pairs.c[1])),
ARRAY(sa.Text),
).contains(perm_array_literal)
)
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The permission array is constructed via manual string concatenation into literal_column("ARRAY['...']::text[]"). Even if current values come from enums, this is brittle (quoting/escaping) and makes the query harder to reason about. Prefer building a typed SQLAlchemy array expression (e.g., sa.cast(sa.array(perm_values), ARRAY(sa.Text)) / bindparams) so SQLAlchemy can handle escaping and typing reliably, and only fall back to raw SQL if there is a demonstrated driver limitation.

Copilot uses AI. Check for mistakes.
max_concurrent_sessions=10,
max_concurrent_sftp_sessions=5,
max_containers_per_session=1,
idle_timeout=3600,
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The module docstring says this test suite verifies host permissions from domains, groups, and keypair_resource_policies, but the fixture creates KeyPairResourcePolicyRow without setting any allowed_vfolder_hosts and none of the cases assert behavior coming from the keypair resource policy source. Add at least one non-empty keypair policy host entry + vfolder on that host to ensure the keypair-resource-policy branch of by_host_permission() is actually covered.

Suggested change
idle_timeout=3600,
idle_timeout=3600,
allowed_vfolder_hosts={
"host-shared": ["create-vfolder", "mount-in-session"],
},

Copilot uses AI. Check for mistakes.
Co-authored-by: octodog <mu001@lablup.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:docs Documentations comp:common Related to Common component comp:manager Related to Manager component size:XL 500~ LoC

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add host_permission filter to VFolder my_search

2 participants