Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 43 additions & 1 deletion src/openstack_mcp_server/tools/block_storage_tools.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

from .base import get_openstack_conn
from .response.block_storage import (
Attachment,
ConnectionInfo,
Volume,
VolumeAttachment,
)
Expand All @@ -22,6 +24,8 @@ def register_tools(self, mcp: FastMCP):
mcp.tool()(self.delete_volume)
mcp.tool()(self.extend_volume)

mcp.tool()(self.get_attachment_details)

def get_volumes(self) -> list[Volume]:
"""
Get the list of Block Storage volumes.
Expand All @@ -39,7 +43,7 @@ def get_volumes(self) -> list[Volume]:
VolumeAttachment(
server_id=attachment.get("server_id"),
device=attachment.get("device"),
attachment_id=attachment.get("id"),
attachment_id=attachment.get("attachment_id"),
),
)

Expand Down Expand Up @@ -183,3 +187,41 @@ def extend_volume(self, volume_id: str, new_size: int) -> None:
conn = get_openstack_conn()

conn.block_storage.extend_volume(volume_id, new_size)

def get_attachment_details(self, attachment_id: str) -> Attachment:
"""
Get detailed information about a specific attachment.

:param attachment_id: The ID of the attachment to get details for
:return: An Attachment object with detailed information
"""
conn = get_openstack_conn()

attachment = conn.block_storage.get_attachment(attachment_id)

# NOTE: We exclude the auth_* fields for security reasons
Copy link
Collaborator

Choose a reason for hiding this comment

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

auth 관련 필드정보가 필터링 되어야하는 이유가 있을까요?

Copy link
Collaborator Author

@S0okJu S0okJu Oct 2, 2025

Choose a reason for hiding this comment

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

@halucinor
auth_* 부분( auth_mehtod, auth_username, auth_password)을 필터링한 이유는 cinder에 접근하기 위한 중요한 인증 정보이기 때문입니다.

auth_method=CHAP인 경우를 예로 들자면 Cinder에서 볼륨을 attach할때 컴퓨트 노드는 해당 계정을 활용하여 target에 대한 로그인을 수행하고, target은 이를 검증하여 허용된 initiator만 세션을 주고받게 됩니다. (참고)

하지만 이러한 인증 정보가 노출되면 cinder에 대한 무단 접근이 가능해집니다. 이는 데이터 탈취와 같은 심각한 보안 문제가 발생할 수 있습니다.

개인적인 이유로, mcp서버가 관리자 등 다양한 사용자를 대상으로 한다고 할지라도 인증 정보 필터링등 최소한의 안전 장치는 마련해야 한다고 생각합니다.

connection_info = attachment.connection_info
filtered_connection_info = ConnectionInfo(
access_mode=connection_info.get("access_mode"),
cacheable=connection_info.get("cacheable"),
driver_volume_type=connection_info.get("driver_volume_type"),
encrypted=connection_info.get("encrypted"),
qos_specs=connection_info.get("qos_specs"),
target_discovered=connection_info.get("target_discovered"),
target_iqn=connection_info.get("target_iqn"),
target_lun=connection_info.get("target_lun"),
target_portal=connection_info.get("target_portal"),
)

params = {
"id": attachment.id,
"instance": attachment.instance,
"volume_id": attachment.volume_id,
"attached_at": attachment.attached_at,
"detached_at": attachment.detached_at,
"attach_mode": attachment.attach_mode,
"connection_info": filtered_connection_info,
"connector": attachment.connector,
}

return Attachment(**params)
23 changes: 23 additions & 0 deletions src/openstack_mcp_server/tools/response/block_storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,26 @@ class Volume(BaseModel):
is_encrypted: bool | None = None
description: str | None = None
attachments: list[VolumeAttachment] = []


class ConnectionInfo(BaseModel):
access_mode: str | None = None
cacheable: bool | None = None
driver_volume_type: str | None = None
encrypted: bool | None = None
qos_specs: str | None = None
target_discovered: bool | None = None
target_iqn: str | None = None
target_lun: int | None = None
target_portal: str | None = None


class Attachment(BaseModel):
id: str
instance: str
volume_id: str
attached_at: str | None = None
detached_at: str | None = None
attach_mode: str | None = None
connection_info: ConnectionInfo | None = None
connector: str | None = None
63 changes: 62 additions & 1 deletion tests/tools/test_block_storage_tools.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

from openstack_mcp_server.tools.block_storage_tools import BlockStorageTools
from openstack_mcp_server.tools.response.block_storage import (
Attachment,
ConnectionInfo,
Volume,
VolumeAttachment,
)
Expand Down Expand Up @@ -614,7 +616,7 @@ def test_register_tools(self):
block_storage_tools.register_tools(mock_mcp)

# Verify mcp.tool() was called for each method
assert mock_mcp.tool.call_count == 5
assert mock_mcp.tool.call_count == 6
Copy link
Collaborator

@paikend paikend Oct 2, 2025

Choose a reason for hiding this comment

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

mock_mcp.tool.call_count 갯수 확인이 어떤 의미를 주는지 와닿지 않는 상태로 말씀드립니다.

도구가 추가되거나 삭제되면 테스트 케이스를 매번 수정해줘야할 것 같아 유지보수 부담을 줄 수 있을 것 같네요

이런 테스트 케이스는 삭제하면 어떨까요?

요고 리뷰에서 논의되서 지우는 방향으로가면 제가 나머지 테스트 함수들 확인해보겠습니다.

(오늘 멘토링 내용중에 '의미있는 테스트를 작성하는가'에 대한 이야기가 지나가듯이 나와서 영감을 받아 코멘트남깁니다.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

위의 코드에 대해서 비슷한 느낌을 받았습니다.
해당 테스트 케이스를 삭제하겠습니다!

Copy link
Collaborator

Choose a reason for hiding this comment

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

#77
관련해서 디스커션 생성해봤습니다! (디스커션 기능 처음 써 봄)
1~2 팀원분들 추가 의견들어볼게욥! ㅋㅋ


# Verify all methods were registered
registered_methods = [
Expand Down Expand Up @@ -683,3 +685,62 @@ def test_all_block_storage_methods_have_docstrings(self):
assert len(docstring.strip()) > 0, (
f"{method_name} docstring should not be empty"
)

def test_get_attachment_details(
self, mock_get_openstack_conn_block_storage
):
"""Test getting attachment details."""

# Set up the attachment mock object
mock_attachment = Mock()
mock_attachment.id = "attach-123"
mock_attachment.instance = "server-123"
mock_attachment.volume_id = "vol-123"
mock_attachment.attached_at = "2024-01-01T12:00:00Z"
mock_attachment.detached_at = None
mock_attachment.attach_mode = "attach"
mock_attachment.connection_info = {
"access_mode": "rw",
"cacheable": True,
"driver_volume_type": "iscsi",
"encrypted": False,
"qos_specs": None,
"target_discovered": True,
"target_iqn": "iqn.2024-01-01.com.example:volume-123",
"target_lun": 0,
"target_portal": "192.168.1.100:3260",
}
mock_attachment.connector = "connector-123"

# Configure the mock block_storage.get_attachment()
mock_conn = mock_get_openstack_conn_block_storage
mock_conn.block_storage.get_attachment.return_value = mock_attachment

block_storage_tools = BlockStorageTools()
result = block_storage_tools.get_attachment_details("attach-123")

# Verify the result
assert isinstance(result, Attachment)
assert result.id == "attach-123"
assert result.instance == "server-123"
assert result.attached_at == "2024-01-01T12:00:00Z"
assert result.detached_at is None
assert result.attach_mode == "attach"
assert result.connection_info == ConnectionInfo(
access_mode="rw",
cacheable=True,
driver_volume_type="iscsi",
encrypted=False,
qos_specs=None,
target_discovered=True,
target_iqn="iqn.2024-01-01.com.example:volume-123",
target_lun=0,
target_portal="192.168.1.100:3260",
)
assert result.connector == "connector-123"
assert result.volume_id == "vol-123"

# Verify the mock calls
mock_conn.block_storage.get_attachment.assert_called_once_with(
"attach-123"
)