Add tools for static network configuration#91
Add tools for static network configuration#91keitwb wants to merge 1 commit intoopenshift-assisted:masterfrom
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: keitwb The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
WalkthroughAdds static network configuration support: a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Server as MCP Tool (server.py)
participant SN as static_net helpers
participant IC as InventoryClient / API
rect rgba(230,245,255,0.6)
note over Client,Server: Add or update host static network config
Client->>Server: update_static_network_config_for_host(cluster_id,dns,mac,ip,prefix,gw)
Server->>IC: _get_cluster_infra_env_id(cluster_id) / get_infra_env(id)
IC-->>Server: infra_env (includes static_network_config)
Server->>SN: add_or_update_static_host_config(existing_json, dns, mac, ip, prefix, gw)
SN-->>Server: updated_configs
Server->>IC: update_infra_env(id, static_network_config=updated_configs)
IC-->>Server: result
Server-->>Client: result (string)
end
rect rgba(240,255,240,0.6)
note over Client,Server: Delete host static network config
Client->>Server: delete_static_network_config_for_host(cluster_id, mac)
Server->>IC: _get_cluster_infra_env_id(cluster_id) / get_infra_env(id)
IC-->>Server: infra_env
Server->>SN: remove_static_host_config(existing_json, mac)
SN-->>Server: updated_configs
Server->>IC: update_infra_env(id, static_network_config=updated_configs)
IC-->>Server: result
Server-->>Client: result (string)
end
rect rgba(255,250,230,0.6)
note over Client,Server: List infra-envs for a cluster
Client->>Server: list_infra_envs(cluster_id)
Server->>IC: list_infra_envs(cluster_id)
IC-->>Server: infra_envs[]
Server-->>Client: JSON-encoded list
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
Bot
added
the
size/XL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
service_client/static_net.py (2)
78-106: Prefer structured YAML emission to avoid injection/format fragility.F-strings embed raw user values; use a dict + yaml.safe_dump for robustness (optional if you don’t want a PyYAML dep).
+from textwrap import dedent +# Optional: +# import yaml @@ -def generate_basic_nmstate_yaml( +def generate_basic_nmstate_yaml( dns_server: str, ip_address: str, subnet_prefix_len: int, gateway_address: str, ) -> str: """Generate a basic NMState config with the given parameters.""" - return f""" + return f""" interfaces: - name: eth0 @@ table-id: 254 """Optional structured approach (outside selected range for context):
# Optional replacement: def generate_basic_nmstate_yaml(...): data = { "interfaces": [{ "name": "eth0", "type": "ethernet", "state": "up", "ipv4": { "address": [{"ip": ip_address, "prefix-length": subnet_prefix_len}], "enabled": True, "dhcp": False, }, }], "dns-resolver": {"config": {"server": [str(dns_server)]}}, "routes": {"config": [{ "destination": "0.0.0.0/0", "next-hop-address": gateway_address, "next-hop-interface": "eth0", "table-id": 254, }]}, } # return yaml.safe_dump(data, sort_keys=False) from json import dumps # if avoiding PyYAML, at least validate via dict first return dedent( f"""{dumps(data)}""" ) # or keep current YAML but build 'data' first to validate inputs
21-28: Add basic input validation for IP/prefix (fail fast).Validate IPs and prefix length to avoid pushing invalid configs downstream.
+from ipaddress import ip_address as _ip, IPv4Address @@ def add_or_update_static_host_config( @@ ) -> list[HostStaticNetworkConfig]: @@ + # Basic sanity checks (IPv4-only template) + if not isinstance(subnet_prefix_len, int) or not (0 <= subnet_prefix_len <= 32): + raise ValueError("subnet_prefix_len must be 0..32") + if not isinstance(dns_server, str) or not isinstance(ip_address, str) or not isinstance(gateway_address, str): + raise ValueError("dns_server, ip_address, and gateway_address must be strings") + for v in (dns_server, ip_address, gateway_address): + if not isinstance(_ip(v), IPv4Address): + raise ValueError("Only IPv4 addresses are supported by the basic template")tests/test_static_net.py (3)
1-1: Remove unnecessary type ignore.Not needed for tests; can hide genuine issues.
-# type: ignore
204-240: Add remove() test with list input.@@ class TestRemoveStaticHostConfig: def test_remove_existing_host_config(self): @@ assert result[0]["network_yaml"] == "yaml2" + + def test_remove_with_existing_list_input(self): + existing_list = [ + {"mac_interface_map": [{"logical_nic_name": "eth0", "mac_address": "00:11:22:33:44:55"}], "network_yaml": "yaml1"}, + {"mac_interface_map": [{"logical_nic_name": "eth0", "mac_address": "aa:bb:cc:dd:ee:ff"}], "network_yaml": "yaml2"}, + ] + result = remove_static_host_config(existing_static_network_config=existing_list, mac_address="00:11:22:33:44:55") + assert len(result) == 1 + assert result[0]["mac_interface_map"][0]["mac_address"] == "aa:bb:cc:dd:ee:ff"
63-88: Add case-insensitive MAC matching test.@@ class TestFindHostConfigIndexForMac: def test_find_mac_in_empty_list(self): """Test searching in an empty configuration list.""" index = find_host_config_index_for_mac("00:11:22:33:44:55", []) assert index is None + + def test_find_mac_case_insensitive(self): + configs = [ + { + "mac_interface_map": [{"logical_nic_name": "eth0", "mac_address": "AA:BB:CC:DD:EE:FF"}], + "network_yaml": "yaml", + } + ] + assert find_host_config_index_for_mac("aa:bb:cc:dd:ee:ff", configs) == 0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server.py(2 hunks)service_client/static_net.py(1 hunks)tests/test_static_net.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/test_static_net.py (1)
service_client/static_net.py (4)
add_or_update_static_host_config(21-60)find_host_config_index_for_mac(63-75)generate_basic_nmstate_yaml(78-106)remove_static_host_config(109-127)
server.py (3)
metrics/metrics.py (3)
metrics(69-71)track_tool_usage(51-65)initiate_metrics(44-48)service_client/assisted_service_api.py (3)
get_infra_env(203-218)update_infra_env(306-327)list_infra_envs(221-239)service_client/static_net.py (2)
add_or_update_static_host_config(21-60)remove_static_host_config(109-127)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Red Hat Konflux / assisted-service-mcp-saas-main-on-pull-request
🔇 Additional comments (2)
server.py (2)
18-24: LGTM on metrics/static_net imports.Imports are consistent with usage below.
458-476: LGTM: list_infra_envs tool.Straight-through listing with JSON response and helpful log.
Right now it is pretty basic with just the ability to add basic network config with a predefined NMState template. The API handling is a bit awkward given the MAC to IP mapping but all of that complexity is hidden from the LLM.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
server.py (1)
432-456: Delete path looks correct with current helper types.This now passes the string/None
infra_env.static_network_configtoremove_static_host_config(str|None), which aligns with the helpers and the API response behavior. The earlier concern about list inputs is addressed by the updated helper signatures.
🧹 Nitpick comments (3)
server.py (3)
18-24: Alias the metrics route to avoid shadowing and verify package re-exports.Importing
metricsfrom themetricspackage may shadow the package name locally and relies on__init__.pyre-exportingmetrics,track_tool_usage, andinitiate_metrics. Alias the route for clarity and confirm the re-exports exist; otherwise import frommetrics.metrics.-from metrics import metrics, track_tool_usage, initiate_metrics +from metrics import metrics as metrics_endpoint, track_tool_usage, initiate_metricsApply outside this hunk:
- app.add_route("/metrics", metrics) + app.add_route("/metrics", metrics_endpoint)To verify exports:
- Ensure metrics/init.py re-exports:
from .metrics import track_tool_usage, initiate_metricsand exposesmetrics.
405-424: Add basic IP and prefix validation to fail fast with clear errors.Validate IP formats, family consistency, and prefix length before generating nmstate YAML to prevent opaque backend 4xx errors.
client = InventoryClient(get_access_token()) infra_env_id = await _get_cluster_infra_env_id(client, cluster_id) infra_env = await client.get_infra_env(infra_env_id) + # Validate addressing inputs early for clearer errors + import ipaddress + try: + ip = ipaddress.ip_address(ip_address) + gw = ipaddress.ip_address(gateway_address) + dns = ipaddress.ip_address(dns_server) + except ValueError as e: + raise ValueError(f"Invalid IP address input: {e}") from e + if ip.version != gw.version or ip.version != dns.version: + raise ValueError("IP, gateway, and DNS server must be the same IP family (IPv4 or IPv6).") + if ip.version == 4 and not (0 <= subnet_prefix_len <= 32): + raise ValueError("subnet_prefix_len must be between 0 and 32 for IPv4.") + if ip.version == 6 and not (0 <= subnet_prefix_len <= 128): + raise ValueError("subnet_prefix_len must be between 0 and 128 for IPv6.") + static_network_config = add_or_update_static_host_config( existing_static_network_config=infra_env.static_network_config, dns_server=dns_server, mac_address=mac_address, ip_address=ip_address, gateway_address=gateway_address, subnet_prefix_len=subnet_prefix_len, )
458-476: LGTM; consider response shaping if output becomes noisy.Returning raw infra-env dicts is fine. If this proves too verbose in the UI, optionally filter to key fields (e.g., id, name, static_network_config) or pretty-print with an indent for readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server.py(2 hunks)service_client/static_net.py(1 hunks)tests/test_static_net.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_static_net.py
- service_client/static_net.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-09T18:51:46.556Z
Learnt from: keitwb
PR: openshift-assisted/assisted-service-mcp#91
File: service_client/static_net.py:21-36
Timestamp: 2025-09-09T18:51:46.556Z
Learning: In the assisted-service API, the static_network_config field is typed as a list when input to the API but comes back out as a string in responses. Functions processing this field from API responses should handle string inputs only.
Applied to files:
server.py
🧬 Code graph analysis (1)
server.py (3)
metrics/metrics.py (3)
metrics(69-71)track_tool_usage(51-65)initiate_metrics(44-48)service_client/assisted_service_api.py (3)
get_infra_env(203-218)update_infra_env(306-327)list_infra_envs(221-239)service_client/static_net.py (2)
add_or_update_static_host_config(21-60)remove_static_host_config(109-127)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Red Hat Konflux / assisted-service-mcp-saas-main-on-pull-request
🔇 Additional comments (1)
server.py (1)
417-424: Good: Handles API’s string-encoded static_network_config correctly.Passing
infra_env.static_network_config(string/None) intoadd_or_update_static_host_configthat acceptsstr | Nonematches the Assisted Service response quirk noted in learnings. The helper returns a list suitable forupdate_infra_env.
|
@keitwb: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
eranco74
left a comment
eranco74
left a comment
There was a problem hiding this comment.
Looks good,
Unsure about list_infra_envs tool.
|
|
||
| @mcp.tool() | ||
| @track_tool_usage() | ||
| async def list_infra_envs(cluster_id: str) -> str: |
There was a problem hiding this comment.
Why do we need this tool?
There was a problem hiding this comment.
So that you can ask the bot what the current static networking config is for a given cluster.
|
Replaced by #96 |
2 participants
Right now it is pretty basic with just the ability to add basic network config with a predefined NMState template.
The API handling is a bit awkward given the MAC to IP mapping but all of that complexity is hidden from the LLM.
Summary by CodeRabbit
New Features
Tests