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
Prev Previous commit
Next Next commit
fix(network): add id field to get security group (#85)
  • Loading branch information
platanus-kr committed Oct 16, 2025
commit bc330543919693afad4424f5db58d48f9cd9b3ee
16 changes: 6 additions & 10 deletions src/openstack_mcp_server/tools/network_tools.py
Original file line number Diff line number Diff line change
Expand Up @@ -1175,12 +1175,14 @@ def get_security_groups(
self,
project_id: str | None = None,
name: str | None = None,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

이미 훌륭한 코드에 한 가지 제 생각을 덧붙여보자면, id필드가 있으면 더 좋지 않을까 생각이 듭니다.
id가 필요없을까? 생각에서 뻗어나갔는데요,
security group은 name을 중복값으로 생성할 수 있을텐데,
만약 조금 복잡하게 질문해서

  • 전제: 중복된 security name이 많음
  • 프롬프트: "instance중에 tag가 prod인 인스턴스들을 조회하고, 그 인스턴스들이 어떤 security group을 쓰는지 조회해줘"
    라고 했을 때,
    기존 프로젝트의 compute에서는 return은 id와 name 모두를 반환하므로 get_security_group_detail 함수에 id를 통해 정확히 조회해준다면 아름답겠지만, 자칫 get_security_groups의 name으로 조회하게된다면, 결과를 보장하기 힘들 것 같습니다.
    그런데 id필드가 있으면 적어도 이 케이스에 대해서는 안전할 것 같습니다

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

그러네요 ID 필드 추가하도록 하겠습니다!

id: str | None = None,
) -> list[SecurityGroup]:
"""
Get the list of Security Groups with optional filtering.

:param project_id: Filter by project ID
:param name: Filter by security group name
:param id: Filter by security group ID
:return: List of SecurityGroup objects
"""
conn = get_openstack_conn()
Expand All @@ -1189,6 +1191,8 @@ def get_security_groups(
filters["project_id"] = project_id
if name:
filters["name"] = name
if id:
filters["id"] = id
security_groups = conn.network.security_groups(**filters)
return [
self._convert_to_security_group_model(sg) for sg in security_groups
Expand Down Expand Up @@ -1282,7 +1286,8 @@ def _convert_to_security_group_model(self, openstack_sg) -> SecurityGroup:
rules = getattr(openstack_sg, "security_group_rules", None)
if rules is not None:
dto_rules = [
self._convert_to_security_group_rule_model(r) for r in rules
SecurityGroupRule.model_validate(r, from_attributes=True)
for r in rules
]
rule_ids = [str(r.id) for r in dto_rules if getattr(r, "id", None)]

Expand All @@ -1294,12 +1299,3 @@ def _convert_to_security_group_model(self, openstack_sg) -> SecurityGroup:
project_id=getattr(openstack_sg, "project_id", None),
security_group_rule_ids=rule_ids,
)

def _convert_to_security_group_rule_model(self, rule) -> SecurityGroupRule:
"""
Convert an OpenStack Security Group Rule to a SecurityGroupRule DTO.

:param rule: OpenStack security group rule object or dict
:return: Pydantic SecurityGroupRule model
"""
return SecurityGroupRule.model_validate(rule, from_attributes=True)
43 changes: 40 additions & 3 deletions tests/tools/test_network_tools.py
Original file line number Diff line number Diff line change
Expand Up @@ -1400,6 +1400,37 @@ def test_get_security_groups_filters(self, mock_openstack_connect_network):
project_id="proj-1", name="default"
)

def test_get_security_groups_filter_by_id(
self, mock_openstack_connect_network
):
mock_conn = mock_openstack_connect_network

sg = Mock()
sg.id = "sg-1"
sg.name = "default"
sg.status = None
sg.description = "desc"
sg.project_id = "proj-1"
sg.security_group_rules = [
{"id": "r-1"},
{"id": "r-2"},
]
mock_conn.network.security_groups.return_value = [sg]

tools = self.get_network_tools()
res = tools.get_security_groups(id="sg-1")
assert res == [
SecurityGroup(
id="sg-1",
name="default",
status=None,
description="desc",
project_id="proj-1",
security_group_rule_ids=["r-1", "r-2"],
)
]
mock_conn.network.security_groups.assert_called_once_with(id="sg-1")

def test_create_security_group(self, mock_openstack_connect_network):
mock_conn = mock_openstack_connect_network
sg = Mock()
Expand Down Expand Up @@ -1463,7 +1494,10 @@ def test_update_security_group(self, mock_openstack_connect_network):
"sg-4", name="new-name", description="new-desc"
)

# No fields -> returns current
def test_update_security_group_no_fields_returns_current(
self, mock_openstack_connect_network
):
mock_conn = mock_openstack_connect_network
current = Mock()
current.id = "sg-5"
current.name = "cur"
Expand All @@ -1472,8 +1506,11 @@ def test_update_security_group(self, mock_openstack_connect_network):
current.project_id = None
current.security_group_rules = None
mock_conn.network.get_security_group.return_value = current
res2 = tools.update_security_group("sg-5")
assert res2.id == "sg-5"

tools = self.get_network_tools()
res = tools.update_security_group("sg-5")
assert res.id == "sg-5"
mock_conn.network.get_security_group.assert_called_once_with("sg-5")

def test_delete_security_group(self, mock_openstack_connect_network):
mock_conn = mock_openstack_connect_network
Expand Down