feat(network): improve feedback (#87)

Author: platanus-kr

src/openstack_mcp_server/tools/network_tools.py

@@ -44,7 +44,7 @@ def register_tools(self, mcp: FastMCP):
         mcp.tool()(self.delete_port)
         mcp.tool()(self.get_port_allowed_address_pairs)
         mcp.tool()(self.set_port_binding)
-        mcp.tool()(self.add_security_group_to_port)
+        mcp.tool()(self.add_port_to_security_group)
         mcp.tool()(self.remove_security_group_from_port)
         mcp.tool()(self.get_floating_ips)
         mcp.tool()(self.create_floating_ip)
@@ -517,7 +517,7 @@ def set_port_binding(
         updated = conn.network.update_port(port_id, **update_args)
         return self._convert_to_port_model(updated)
 
-    def add_security_group_to_port(
+    def add_port_to_security_group(
         self,
         port_id: str,
         security_group_id: str,
@@ -534,7 +534,7 @@ def add_security_group_to_port(
         """
         conn = get_openstack_conn()
         current = conn.network.get_port(port_id)
-        current_group_ids: list[str] = list(current.security_group_ids or [])
+        current_group_ids: list[str] = list(current.security_group_ids)
         if security_group_id in current_group_ids:
             return self._convert_to_port_model(current)
 
@@ -561,7 +561,7 @@ def remove_security_group_from_port(
         """
         conn = get_openstack_conn()
         current = conn.network.get_port(port_id)
-        current_group_ids: list[str] = list(current.security_group_ids or [])
+        current_group_ids: list[str] = list(current.security_group_ids)
         if security_group_id not in current_group_ids:
             return self._convert_to_port_model(current)
 
@@ -1233,6 +1233,25 @@ def _sanitize_server_filters(self, filters: dict) -> dict:
         attrs.pop("status", None)
         return attrs
 
+    def _coerce_port(self, value) -> int | None:
+        """
+        Coerce a port value that may arrive as a string (e.g., "80") into int.
+
+        This relaxes tool input validation issues from UIs that serialize numbers
+        as strings. Only base-10 digit strings are converted; otherwise returns
+        None so the field can be omitted.
+
+        :param value: Port as int, str, or None
+        :return: int port or None
+        """
+        if isinstance(value, int):
+            return value
+        if isinstance(value, str):
+            stripped = value.strip()
+            if stripped.isdigit():
+                return int(stripped)
+        return None
+
     def get_security_groups(
         self,
         project_id: str | None = None,
@@ -1344,19 +1363,12 @@ def _convert_to_security_group_model(self, openstack_sg) -> SecurityGroup:
         :param openstack_sg: OpenStack security group object
         :return: Pydantic SecurityGroup model
         """
-        rule_ids: list[str] | None = None
         rules = getattr(openstack_sg, "security_group_rules", None)
-        if rules is not None:
-            extracted: list[str] = []
-            for r in rules:
-                rid = (
-                    r.get("id")
-                    if isinstance(r, dict)
-                    else getattr(r, "id", None)
-                )
-                if rid:
-                    extracted.append(str(rid))
-            rule_ids = extracted
+        rule_ids: list[str] | None = (
+            [r.get("id") for r in rules if r.get("id")]
+            if rules is not None
+            else None
+        )
 
         return SecurityGroup(
             id=openstack_sg.id,
@@ -1373,8 +1385,8 @@ def create_security_group_rule(
         direction: str = "ingress",
         ethertype: str = "IPv4",
         protocol: str | None = None,
-        port_range_min: int | None = None,
-        port_range_max: int | None = None,
+        port_range_min: int | str | None = None,
+        port_range_max: int | str | None = None,
         remote_ip_prefix: str | None = None,
         remote_group_id: str | None = None,
         description: str | None = None,
@@ -1396,20 +1408,31 @@ def create_security_group_rule(
         :return: Created SecurityGroupRule
         """
         conn = get_openstack_conn()
-        args: dict = {
-            "security_group_id": security_group_id,
-            "direction": direction,
-            "ethertype": ethertype,
-        }
-        args["protocol"] = protocol
-        args["port_range_min"] = port_range_min
-        args["port_range_max"] = port_range_max
-        args["remote_ip_prefix"] = remote_ip_prefix
-        args["remote_group_id"] = remote_group_id
-        args["description"] = description
-        args["project_id"] = project_id
-        rule = conn.network.create_security_group_rule(**args)
-        return self._convert_to_security_group_rule_model(rule)
+        create_args: dict = {}
+        if protocol is not None:
+            create_args["protocol"] = protocol
+            coerced_min = self._coerce_port(port_range_min)
+            coerced_max = self._coerce_port(port_range_max)
+            if coerced_min is not None:
+                create_args["port_range_min"] = coerced_min
+            if coerced_max is not None:
+                create_args["port_range_max"] = coerced_max
+        if remote_ip_prefix is not None:
+            create_args["remote_ip_prefix"] = remote_ip_prefix
+        if remote_group_id is not None:
+            create_args["remote_group_id"] = remote_group_id
+        if description is not None:
+            create_args["description"] = description
+        if project_id is not None:
+            create_args["project_id"] = project_id
+
+        rule = conn.network.create_security_group_rule(
+            security_group_id=security_group_id,
+            direction=direction,
+            ethertype=ethertype,
+            **create_args,
+        )
+        return SecurityGroupRule(**rule)
 
     def get_security_group_rule_detail(
         self, rule_id: str
@@ -1422,7 +1445,7 @@ def get_security_group_rule_detail(
         """
         conn = get_openstack_conn()
         rule = conn.network.get_security_group_rule(rule_id)
-        return self._convert_to_security_group_rule_model(rule)
+        return SecurityGroupRule(**rule)
 
     def delete_security_group_rule(self, rule_id: str) -> None:
         """
@@ -1450,32 +1473,25 @@ def create_security_group_rules_bulk(
         :return: List of created SecurityGroupRule models
         """
         conn = get_openstack_conn()
-        created = conn.network.create_security_group_rules(rules=rules)
-        return [self._convert_to_security_group_rule_model(r) for r in created]
 
-    def _convert_to_security_group_rule_model(
-        self, openstack_rule
-    ) -> SecurityGroupRule:
-        """
-        Convert an OpenStack Security Group Rule object to a pydantic model.
-
-        :param openstack_rule: OpenStack rule object
-        :return: SecurityGroupRule model
-        """
-        return SecurityGroupRule(
-            id=getattr(openstack_rule, "id"),
-            name=getattr(openstack_rule, "name", None),
-            status=getattr(openstack_rule, "status", None),
-            description=getattr(openstack_rule, "description", None),
-            project_id=getattr(openstack_rule, "project_id", None),
-            direction=getattr(openstack_rule, "direction", None),
-            ethertype=getattr(openstack_rule, "ethertype", None),
-            protocol=getattr(openstack_rule, "protocol", None),
-            port_range_min=getattr(openstack_rule, "port_range_min", None),
-            port_range_max=getattr(openstack_rule, "port_range_max", None),
-            remote_ip_prefix=getattr(openstack_rule, "remote_ip_prefix", None),
-            remote_group_id=getattr(openstack_rule, "remote_group_id", None),
-            security_group_id=getattr(
-                openstack_rule, "security_group_id", None
-            ),
-        )
+        def _clean_rule_payload(raw: dict) -> dict:
+            payload = dict(raw)
+            # Remove port ranges when protocol is absent (Neutron requirement)
+            if not payload.get("protocol"):
+                payload.pop("port_range_min", None)
+                payload.pop("port_range_max", None)
+            else:
+                # Coerce possible string ports to integers
+                for key in ("port_range_min", "port_range_max"):
+                    if key in payload:
+                        coerced = self._coerce_port(payload.get(key))
+                        if coerced is None:
+                            payload.pop(key, None)
+                        else:
+                            payload[key] = coerced
+            # Drop keys explicitly set to None
+            return {k: v for k, v in payload.items() if v is not None}
+
+        cleaned_rules = [_clean_rule_payload(r) for r in rules]
+        created = conn.network.create_security_group_rules(rules=cleaned_rules)
+        return [SecurityGroupRule(**r) for r in created]

tests/tools/test_network_tools.py

@@ -909,15 +909,15 @@ def test_add_remove_security_group_on_port(
         mock_conn.network.update_port.return_value = updated_add
 
         tools = self.get_network_tools()
-        res_add = tools.add_security_group_to_port("port-1", "sg-9")
+        res_add = tools.add_port_to_security_group("port-1", "sg-9")
         assert isinstance(res_add, Port)
         mock_conn.network.update_port.assert_called_with(
             "port-1", security_groups=["sg-1", "sg-2", "sg-9"]
         )
 
         # idempotent add when sg already present
         mock_conn.network.get_port.return_value = updated_add
-        res_add_again = tools.add_security_group_to_port("port-1", "sg-9")
+        res_add_again = tools.add_port_to_security_group("port-1", "sg-9")
         assert isinstance(res_add_again, Port)
 
         # updated after remove
@@ -1460,10 +1460,7 @@ def test_get_security_groups_filters(self, mock_openstack_connect_network):
         sg.status = None
         sg.description = "desc"
         sg.project_id = "proj-1"
-        sg.security_group_rules = [
-            {"id": "r-1"},
-            {"id": "r-2"},
-        ]
+        sg.security_group_rules = [{"id": "r-1"}, {"id": "r-2"}]
 
         expected_sg = SecurityGroup(
             id="sg-1",
@@ -1852,20 +1849,21 @@ def test_add_get_remove_router_interface_by_port(
 
     def test_create_security_group_rule(self, mock_openstack_connect_network):
         mock_conn = mock_openstack_connect_network
-        rule = Mock()
-        rule.id = "r-1"
-        rule.name = None
-        rule.status = None
-        rule.description = "allow 22"
-        rule.project_id = "proj-1"
-        rule.direction = "ingress"
-        rule.ethertype = "IPv4"
-        rule.protocol = "tcp"
-        rule.port_range_min = 22
-        rule.port_range_max = 22
-        rule.remote_ip_prefix = "0.0.0.0/0"
-        rule.remote_group_id = None
-        rule.security_group_id = "sg-1"
+        rule = {
+            "id": "r-1",
+            "name": None,
+            "status": None,
+            "description": "allow 22",
+            "project_id": "proj-1",
+            "direction": "ingress",
+            "ethertype": "IPv4",
+            "protocol": "tcp",
+            "port_range_min": 22,
+            "port_range_max": 22,
+            "remote_ip_prefix": "0.0.0.0/0",
+            "remote_group_id": None,
+            "security_group_id": "sg-1",
+        }
         mock_conn.network.create_security_group_rule.return_value = rule
 
         tools = self.get_network_tools()
@@ -1887,20 +1885,21 @@ def test_get_security_group_rule_detail(
         self, mock_openstack_connect_network
     ):
         mock_conn = mock_openstack_connect_network
-        rule = Mock()
-        rule.id = "r-2"
-        rule.name = None
-        rule.status = None
-        rule.description = None
-        rule.project_id = None
-        rule.direction = "egress"
-        rule.ethertype = "IPv4"
-        rule.protocol = None
-        rule.port_range_min = None
-        rule.port_range_max = None
-        rule.remote_ip_prefix = None
-        rule.remote_group_id = None
-        rule.security_group_id = "sg-1"
+        rule = {
+            "id": "r-2",
+            "name": None,
+            "status": None,
+            "description": None,
+            "project_id": None,
+            "direction": "egress",
+            "ethertype": "IPv4",
+            "protocol": None,
+            "port_range_min": None,
+            "port_range_max": None,
+            "remote_ip_prefix": None,
+            "remote_group_id": None,
+            "security_group_id": "sg-1",
+        }
         mock_conn.network.get_security_group_rule.return_value = rule
 
         tools = self.get_network_tools()
@@ -1925,35 +1924,37 @@ def test_create_security_group_rules_bulk(
         self, mock_openstack_connect_network
     ):
         mock_conn = mock_openstack_connect_network
-        r1 = Mock()
-        r1.id = "r-10"
-        r1.name = None
-        r1.status = None
-        r1.description = None
-        r1.project_id = None
-        r1.security_group_id = "sg-1"
-        r1.direction = "ingress"
-        r1.ethertype = "IPv4"
-        r1.protocol = "tcp"
-        r1.port_range_min = 80
-        r1.port_range_max = 80
-        r1.remote_ip_prefix = "0.0.0.0/0"
-        r1.remote_group_id = None
-
-        r2 = Mock()
-        r2.id = "r-11"
-        r2.name = None
-        r2.status = None
-        r2.description = None
-        r2.project_id = None
-        r2.security_group_id = "sg-1"
-        r2.direction = "ingress"
-        r2.ethertype = "IPv4"
-        r2.protocol = "tcp"
-        r2.port_range_min = 443
-        r2.port_range_max = 443
-        r2.remote_ip_prefix = "0.0.0.0/0"
-        r2.remote_group_id = None
+        r1 = {
+            "id": "r-10",
+            "name": None,
+            "status": None,
+            "description": None,
+            "project_id": None,
+            "security_group_id": "sg-1",
+            "direction": "ingress",
+            "ethertype": "IPv4",
+            "protocol": "tcp",
+            "port_range_min": 80,
+            "port_range_max": 80,
+            "remote_ip_prefix": "0.0.0.0/0",
+            "remote_group_id": None,
+        }
+
+        r2 = {
+            "id": "r-11",
+            "name": None,
+            "status": None,
+            "description": None,
+            "project_id": None,
+            "security_group_id": "sg-1",
+            "direction": "ingress",
+            "ethertype": "IPv4",
+            "protocol": "tcp",
+            "port_range_min": 443,
+            "port_range_max": 443,
+            "remote_ip_prefix": "0.0.0.0/0",
+            "remote_group_id": None,
+        }
 
         mock_conn.network.create_security_group_rules.return_value = [r1, r2]