Conversation
_get_controller() checked `CONF.unifi.controller not in self._controllers` before every single controller-API call, but config.py never registers a `controller` option in the [unifi] group -- only host, port, apikey, username, password, site, verify_ssl, cafile, and the feature-flag options. Every real call raised `oslo_config.cfg.NoSuchOptError` unconditionally. initialize()'s own call to _get_controller() (line 92) is wrapped in a broad try/except that only logs and continues, which is why this never surfaced at driver startup. create_network_postcommit and friends have no such guard, so a real `openstack network create` fails outright and Neutron's ML2 manager rolls back by deleting the network it just created. The fix is the option name actually used as the dict key two lines later (`self._controllers[CONF.unifi.host]`), not a new option -- `self.controller = None` in __init__ (dead, unused elsewhere) suggests `self.controller` was likely the original intent and got typo'd into `CONF.unifi.controller` at some point. Added a regression test that exercises real oslo_config registration directly (TestUnifiMechDriver's existing setUp replaces cfg.CONF with a MagicMock entirely, so register_opts/set_override there never validate anything real -- which is exactly how this shipped unnoticed). Verified against a real Python 3.10.12 + neutron/neutron-lib/oslo_config environment (not just unit-test mocks): the new test passes, and the existing suite's 10 pre-existing failures in TestUnifiMechDriver are unchanged before/after this fix -- confirmed by running both against the same environment, so they're pre-existing (stale mocks for an older aiounifi API shape), not something this change caused.
Renames the PyPI distribution (not the import name or entry point -- still unifi_ml2_driver / unifi, so this stays a drop-in replacement) since publishing under upstream's own unifi-ml2-driver name isn't possible without owning that PyPI project. requires-python drops from upstream's >=3.12.0 to >=3.10.12, matching what the code actually needs rather than upstream's policy bump. Verified live: the fixed driver plus its aiohttp-unifi>=88 dependency both import and run correctly under real Python 3.10.12 (the actual target platform this fork exists for), given a small sitecustomize.py that back-ports the handful of Python 3.11+-only typing/enum symbols aiohttp-unifi>=86 uses (Self, NotRequired, Required, StrEnum) from typing_extensions/backports.strenum -- neither package's own code needs 3.12 syntax, confirmed by a direct py_compile pass plus a real import/pytest run inside a Python 3.10.12 environment matching the target Neutron image exactly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Poetry's own dependency solver enforces overlapping Python-version ranges strictly and refuses to lock or install a graph where this fork's own requires-python (>=3.10.12) doesn't fully contain a dependency's declared range (aiohttp-unifi's >=3.12.0) -- confirmed: `poetry lock` fails outright with "version solving failed" on this exact combination. pip's --ignore-requires-python (what neutron-ml2-guardian's own install/download commands use) has no poetry equivalent. `poetry build` needs neither a lock file nor an installed environment, only pyproject.toml, so removing the poetry.lock this repo can no longer validly maintain and dropping `poetry install` from the publish workflow doesn't affect building/publishing at all. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
It derived a nonsensical "1.0.9.dev3" even from a perfectly clean tree with the release tag exactly at HEAD (git describe agreed: "v1.0.10", no distance suffix) -- and worse, it turns out to hook poetry's build process directly, silently overriding a manual `poetry version` call regardless of what value is set. Simpler and more reliable to just derive the version from the git ref directly and set it with a plain `poetry version`, matching the pattern neutron-ml2-guardian's own CI already uses for its Helm chart version. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Failed outright: "No file ... matched to [**/poetry.lock]". Same root cause as the previous two commits -- this repo can't maintain a valid poetry.lock given its relaxed requires-python, by design. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Found live via a real openstack network create: every HTTPS request to a real UniFi controller failed outright with "Cannot create a client socket with a PROTOCOL_TLS_SERVER context" -- ssl.Purpose. CLIENT_AUTH configures a context for a server verifying incoming client certificates, the opposite of what's needed to make an outgoing request as a client verifying the remote server's cert (ssl.Purpose.SERVER_AUTH). This context was never usable for its actual purpose at all with verify_ssl left at its default (true). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Found live via a real openstack network create against a real UDM-SE:
the request succeeded in reaching the controller (both prior fixes --
_get_controller's NoSuchOptError and the SSL Purpose bug -- confirmed
working), but the server rejected it with api.err.VlanUsed reporting
vlan: 1, "Default Management Network" -- not the vlan we actually
requested (1999).
Root cause: aiounifi's Network.vlan property only returns the raw
"vlan" value when vlan_enabled is also true (defaults false):
def vlan(self) -> Optional[int]:
if self.vlan_enabled:
return self.raw.get("vlan", None)
vlan_data never set vlan_enabled at all, so the server treated the
submission as untagged/VLAN-disabled -- which collides with the
Default Management Network's own untagged (effectively "vlan 1")
configuration, hence the conflict reported against vlan 1 rather than
whatever we asked for.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The five subnet hooks (create/update/delete _precommit/_postcommit) were literal no-op stubs, both upstream and in this fork. Networks got VLAN-level sync via create_network_postcommit, but subnet-level info (CIDR, gateway, DHCP pool, DNS servers) never reached the matching UniFi networkconf's ip_subnet/dhcpd_*/dhcpdv6_* fields. Postcommit hooks locate the UniFi network by VLAN ID and push the Neutron subnet's cidr/gateway_ip/enable_dhcp/allocation_pools/ dns_nameservers onto it via NetworkUpdateRequest, following the same pattern already proven for network create/update. IPv4 and IPv6 (including DHCPv6-stateful) are both handled, since this environment already has an IPv6 self-service subnet. Delete disables DHCP/gateway on the UniFi side rather than clearing ip_subnet/ipv6_subnet outright, to avoid leaving the network config in an invalid state. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Firewall zone membership in UniFi's Zone-Based Firewall lives on the zone object's own network_ids list, not on the network's networkconf -- there's no per-network "zone" field to set directly. Adds CONF.unifi.default_firewall_zone (unset by default, matching every other optional feature in this driver) and two helpers, _assign_network_to_default_zone/_unassign_network_from_default_zone, that look up the configured zone by name via the Network Integration API's /firewall/zone endpoints and add/remove the network's ID from it. Wired into create_network_postcommit, update_network_postcommit (after the VLAN is (re)created), and delete_network_postcommit (before the network itself is deleted). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
create_network_postcommit/update_network_postcommit's zone-assignment calls passed the Neutron network UUID to _assign_network_to_default_zone, but UniFi assigns its own _id (a Mongo ObjectId) on create and ignores the requested "_id" -- confirmed live against a real UDM-SE: a test network's real _id (6aa7f6d7742467ea9dfa2f6d) had nothing to do with its Neutron network UUID. Zone membership (network_ids) needs that real id. Both postcommit hooks now look the network up by VLAN via _unifi_network_for_vlan (same helper the subnet hooks already use) and pass its .id instead. delete_network_postcommit already did this correctly via target_network.id -- only the create/update paths had the bug. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Two bugs found live against a real UDM-SE while verifying ubiquiti-community#22: 1. Round-tripping the full zone.raw dict (as returned by GET) into the PUT body includes "attr_no_edit", which the UniFi API rejects outright: "JSON parse error: Unrecognized field \"attr_no_edit\" ... not marked as ignorable". Strip it before sending. 2. That rejection was fatal: create_network_postcommit's exception handler re-raises, which Neutron treats as a MechanismDriverError and reacts to by deleting the network it just created -- so a firewall-zone hiccup was taking down the core VLAN/subnet sync that had already succeeded. Both zone helpers now catch their own exceptions and log-and-continue instead of propagating, matching the non-raising convention delete_network_postcommit already uses for the same reason (a secondary feature must never block or unwind the primary one). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Stripping attr_no_edit alone (from ubiquiti-community#23) wasn't enough. Confirmed live by manually replicating the PUT: the real UDM-SE zone object also carries cloud_template, default_zone, external_id, and zone_key, and the /firewall/zone/{id} endpoint rejects every one of them as an "unrecognized field" on write -- only _id, name, and network_ids are accepted. Round-tripping GET's full payload into PUT was never going to work here; build a minimal update object instead of filtering the GET response field-by-field. Verified against the real UDM-SE: a minimal {_id, name, network_ids} PUT succeeds and the zone's network_ids reflects the change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #23 -- stripping
attr_no_editalone wasn't enough. Confirmed live by manually replicating the PUT against a real UDM-SE: the zone object also carriescloud_template,default_zone,external_id, andzone_key, and/firewall/zone/{id}rejects every one of them as an "unrecognized field" on write. Only_id,name, andnetwork_idsare accepted. Builds a minimal update object instead of filtering the GET response field-by-field.Test plan
python3 -m py_compile unifi_ml2_driver/unifi_mech.py{_id, name, network_ids}PUT succeeds and the zone'snetwork_idsreflects the change.🤖 Generated with Claude Code