fix(neutron-understack): lift vlan trunk restrictions (no. 2)

[stevekeay]

The check was added to prevent the user from claiming a segmentation ID
that happens to be the same as the parent port of this vlan. We don't
want to allow that situation because the switch (stupidly) rejects that
configuration.

However, the way the check was implemented was too restrictive - it
banned a range of thousands of VLAN IDs. It did successfully prevent
all theoretical clashes present and future, but it also prevented
legitimate configurations and caused an upset customer.

We update this check so that it now compares the VLAN ID of the actual
native VLAN on this trunk interface, and only raises an error if the
user is configuring a number that would currently clash.

The theoretical valid range is 1-4094 according to 802.1Q.

However we would avoid VLAN 1, and we must avoid our nxos "reserved" vlan range 3872-3999.

On Nexus 9000 NX-OS, Cisco treats VLANs 4048-4093 as their own. These
might work, but even if they work today, they might randomly stop
working at some future point.

So this puts the realistic useable VLAN range at 2-3871 inclusive.

There was an existing config setting that seemed to do the right thing so I
piggy-backed on that rather than create another that does almost exactly
the same thing.

Comments from the LLM:

1. [P1] Bind validation depends on optional trunk_details.

   python/neutron-understack/neutron_understack/understack_mech.py:386 calls configure_trunk() only when context.current["trunk_details"] is present. During production VIF attach it may be absent, so a trunk created while unbound can skip both collision validation and subport configuration.

   This exact problem was already diagnosed and fixed on sibling commit 9b8e101: it fetched the trunk by parent port from the database and called configure_trunk() unconditionally. That fix is not in this branch. The new scenario passes because the test harness supplies the extension; it does not cover its absence.

   Restore the DB lookup and the earlier test_without_trunk_details_still_checks_for_trunk regression test.

2. [P1] Detecting a collision during binding still does not prevent the bind.

   The scenario explicitly documents and xfails this at python/neutron-understack/neutron_understack/tests/scenarios/test_trunk_operations.py:262. The Understack mechanism driver raises, but ML2 catches mechanism-driver exceptions, tries the next driver, and the port binds anyway.

   Therefore a user can:
   • create an unbound parent;
   • create the trunk and colliding subport;
   • have Ironic bind the parent to a native VLAN matching that subport.

   The final state is inconsistent despite the driver noticing it. This test must pass without xfail against the Neutron version being shipped.

3. [P2] Bound-parent checks may inspect an INACTIVE binding.

   Both python/neutron-understack/neutron_understack/trunk.py:137 and python/neutron-understack/neutron_understack/utils.py:475 use port.bindings[0]. Ports can have ACTIVE and INACTIVE bindings; ordering is not authoritative. The check can consequently:
   • treat a bound parent as unbound and skip validation;
   • check the physnet/native VLAN of the wrong binding.

   Sibling PR 2464 already fixes this by selecting the ACTIVE binding. This branch should depend on or incorporate that fix.

4. [P2] Concurrent subport-add and parent-bind operations can evade both checks.

   Trunk/subport creation validates in the callbacks at python/neutron-understack/neutron_understack/trunk.py:69, while binding validates separately at python/neutron-understack/neutron_understack/understack_mech.py:386.

   A possible interleaving is:

   1. Binding reads the trunk before the new subport commits.
   2. Subport creation reads the parent before binding commits and sees it as unbound.
   3. Both validations pass.
   4. Both operations commit.

   There is no shared lock or generation/revision recheck. This also allows a subport added during binding to miss its binding-level configuration. A concurrency regression test and a serialization/retry strategy are needed.

5. [P3, admin-only] Multiple segments can make the bound-parent check use the wrong native VLAN.

   The Segments API is enabled at components/neutron/values.yaml:93, and an administrator can create multiple segments for a network/physnet. Yet python/neutron-understack/neutron_understack/utils.py:436 deliberately returns an arbitrary first segment.

   For an already-bound parent, collision validation should obtain the segment from the ACTIVE port binding level, not query by (network, physnet) and select the first result.

  ## Lifecycle coverage

   Operation                                                     Coverage
  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
   Create trunk with subports on an already-bound parent         Covered
  ────────────────────────────────────────────────────────────  ──────────────────────────────────────────────────────
   Add a subport to an already-bound trunk                       Covered
  ────────────────────────────────────────────────────────────  ──────────────────────────────────────────────────────
   Remove and re-add with a different tag                        Covered on re-add
  ────────────────────────────────────────────────────────────  ──────────────────────────────────────────────────────
   Update a subport’s segmentation ID in place                   Not supported by Neutron; remove/add is required
  ────────────────────────────────────────────────────────────  ──────────────────────────────────────────────────────
   Change the trunk’s parent port in place                       Not supported
  ────────────────────────────────────────────────────────────  ──────────────────────────────────────────────────────
   Create trunk/subports before first parent bind                Intended, but currently defeated by findings 1 and 2
  ────────────────────────────────────────────────────────────  ──────────────────────────────────────────────────────
   Detach and rebind parent, possibly to another physnet/VLAN    Must be covered at bind; currently has the same gaps
  ────────────────────────────────────────────────────────────  ──────────────────────────────────────────────────────
   Add a subport concurrently with binding                       Not covered
  ────────────────────────────────────────────────────────────  ──────────────────────────────────────────────────────
   Multiple ACTIVE/INACTIVE port bindings                        Not covered
  ────────────────────────────────────────────────────────────  ──────────────────────────────────────────────────────
   Admin-created multiple segments on one physnet                Not safely covered

The premise that a trunk parent “cannot be updated” is only partly correct. Ordinary project users cannot normally update binding:profile, but service users such as Ironic can change binding host/profile and detach or reattach ports under Neutron’s default policy (OpenStack port policy (https://opendev.org/openstack/neutron/src/branch/master/neutron/conf/policies/port.py)). Those
changes trigger unbind/rebind, so removing the separate precommit-update check is reasonable only once bind-time validation is dependable.

Current verification: 428 passed, 6 xfailed; the native-VLAN bind rejection is one of the xfails. git diff --check 8dcfc4c^..HEAD is clean.

[github-actions]

Python test results

0 tests   0 ✅  0s ⏱️
0 suites  0 💤
1 files    0 ❌
1 errors

For more details on these parsing errors, see this check.

Results for commit b7bdc29.

♻️ This comment has been updated with latest results.