core: services: cable_guy: api: manager: Remove subnet from deleted IP

[patrickelectric]

Delete the orphaned route on IP removal and scrub it from saved settings for good.

Summary by Sourcery

Clean up orphaned subnet routes when removing IP addresses and avoid persisting kernel-managed connected routes.

Bug Fixes:

• Remove subnet routes that outlive their deleted IP addresses and could hijack traffic.
• Ensure kernel-managed connected routes are not adopted as persistent routes in saved interface settings.

[sourcery-ai]

Hey - I've left some high level feedback:

• The new _remove_orphaned_subnet_route helper hardcodes a /24 prefix, which may not match the actual prefix of the removed IP; consider deriving the subnet from the interface’s configured prefix instead of assuming /24.
• In _remove_orphaned_subnet_route, you use IPv4Address and the string literal "0.0.0.0"; if weak_is_ip_address can return true for IPv6 addresses, this path will fail—add an explicit IPv4-only guard or handle IPv6 routes separately.
• When filtering out kernel-maintained routes in get_routes, you directly index raw_route["proto"]; for robustness against unexpected netlink messages, consider using raw_route.get("proto") and handling missing or unknown values gracefully.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The new `_remove_orphaned_subnet_route` helper hardcodes a /24 prefix, which may not match the actual prefix of the removed IP; consider deriving the subnet from the interface’s configured prefix instead of assuming /24.
- In `_remove_orphaned_subnet_route`, you use `IPv4Address` and the string literal `"0.0.0.0"`; if `weak_is_ip_address` can return true for IPv6 addresses, this path will fail—add an explicit IPv4-only guard or handle IPv6 routes separately.
- When filtering out kernel-maintained routes in `get_routes`, you directly index `raw_route["proto"]`; for robustness against unexpected netlink messages, consider using `raw_route.get("proto")` and handling missing or unknown values gracefully.

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[github-actions]

Automated PR Review

0. Summary

• Verdict: DOES NOT SAIL 🪨
• Critical items to address: 1.1

Adds _remove_orphaned_subnet_route to cable_guy's remove_ip flow so the /24 connected subnet is explicitly torn down (and scrubbed from persisted settings) when its IP is removed, and filters kernel-managed (RTPROT_KERNEL) routes out of get_routes so they're no longer adopted as managed routes.

1. Correctness & Implementation Bugs

• 1.1 [major] core/services/cable_guy/api/manager.py (new _remove_orphaned_subnet_route) — the settings scrub is unreachable in the common case, which defeats the PR's stated goal ("scrub it from saved settings for good"). Kernel-connected /24 routes are auto-removed by the kernel the moment their IP is deleted; by the time we reach this helper, self.ipr.route("del", dst=subnet, ...) will raise NetlinkError(ESRCH/ENOENT). That's caught by the broad except Exception, which logs an error and returns, so saved_interface.routes is not filtered. Any legacy setting that already had the /24 recorded as managed=True therefore survives, and on the next set_configuration _set_routes_configuration will re-add it via add_route — the exact hijack this PR is trying to prevent. Suggested shape:

  try:
      self.ipr.route("del", dst=str(subnet), oif=self._get_interface_index(interface_name))
      logger.info(f"Removed orphaned route {subnet} from interface {interface_name}.")
  except NetlinkError as error:
      if error.code not in (errno.ESRCH, errno.ENOENT):
          logger.error(f"Failed to remove orphaned route {subnet} on {interface_name}: {error}")
          return
  saved_interface.routes = [route for route in saved_interface.routes if route.destination != str(subnet)]

  Also lift the get_interface_by_name(...) / "any remaining IP in subnet" check out of the same try — bundling them with the netlink call makes a ValueError from a missing interface look like a route-delete failure in the logs.
• 1.2 [minor] Same helper — the broad except Exception will trigger a red logger.error(...) on every normal IP removal (route already gone by the kernel), producing false-alarm log noise on a well-trodden path. Fixing 1.1 as above also silences this.

6. Code Quality & Style

• 6.1 [minor] core/services/cable_guy/api/manager.py — line beginning # A subnet route previously adopted will outlives its IP …: grammar ("will outlives"). Use outlives or will outlive.
• 6.2 [nit] The /24 prefix is hard-coded here and in networksetup.py:53,214,360. That invariant is worth pulling into a single module-level constant (e.g. STATIC_IP_PREFIXLEN = 24) so the assumption is stated once and easy to change; not a blocker for this PR.

7. Tests

• 7.1 [minor] No tests added for the new helper or for the RTPROT_KERNEL skip in get_routes. core/services/cable_guy/ currently has no test module, so this isn't a regression, but the logic here (subnet-membership guard, saved-settings scrub, kernel-route filter) is exactly the kind of thing a small pytest around _remove_orphaned_subnet_route with a mocked ipr would pin down — especially given finding 1.1 slipped through review-by-eye.

8. Documentation

• 8.1 [nit] core/services/cable_guy/api/manager.py (new comment block above the RTPROT_KERNEL skip in get_routes) — good, non-parrot comment; keep. The one-liner above the /24 subnet computation ("mirrors the prefix used when adding/removing static IPs") is also worth keeping since the constant is repeated across files (see 6.2).

Generated by PR Review Bot. This is advisory, a human reviewer must still approve.

[joaoantoniocardoso]

I remember seeing a forever-growing list in some scenarios, and I think this must be it.

[joaoantoniocardoso]

Some small changes to keep the IPv6 compatibility on the routes API.

[joaoantoniocardoso]

Suggested change

	if not self.weak_is_ip_address(ip_address) or ip_address == "0.0.0.0":
	return
	# parse
	try:
	addr = ip_address(ip_address)
	except ValueError:
	return
	# check
	if addr.is_unspecified:
	return

[joaoantoniocardoso]

Suggested change

	subnet = ip_network(f"{ip_address}/24", strict=False)
	prefixlen = 24 if addr.version == 4 else 64
	subnet = ip_network((addr, prefixlen), strict=False)

[joaoantoniocardoso]

Suggested change

	if any(self.weak_is_ip_address(address.ip) and IPv4Address(address.ip) in subnet for address in remaining):
	return
	for address in remaining:
	# parse
	try:
	other = ipaddress.ip_address(address.ip)
	except ValueError:
	continue
	# check
	if other.version == addr.version and other in subnet:
	return