fix(cubelet): name sandbox TAPs after IPv4 and reject oversize names

[FakeLearne]

The z+IPv4 scheme produced 16-byte names (e.g. z192.168.195.183). vishvananda/netlink LinkAdd copies into ifr_name[:15] and truncates silently, so create could succeed while later open/query used a different string than the kernel device.

New creates use the dotted IPv4. Recover still accepts leftover z-prefixed names when the decoded address is in the configured sandbox CIDR. Create fails before LinkAdd if the name is >= IFNAMSIZ.

[cubesandboxbot]

The new two-candidate loop only treats LinkNotFoundError as "try the next name"; every other GetByName error aborts. But getTapByName returns a hard error for a device that exists under the candidate name yet is not a Cube TAP — e.g. "%s is not tap" when the link is a non-Tuntap netdev, or "not cube tap: %s" when the name fails parseCubeTapIP. So a host with an unrelated netdev named exactly like an allocated sandbox IP (a dummy/bridge/VXLAN given a dotted-quad name — the same kind of device your new TestListCubeTapsAcceptsBothNamesAndFiltersCIDR fixture includes as a netlink.Device named 192.168.0.12) will make the first iteration return "<ip> is not tap" and abort before ever consulting the legacy z<ip> candidate that this loop exists to find. The create then fails with a misleading error, and a genuinely stale legacy TAP at z<ip> is never destroyed.

listCubeTaps already tolerates such devices (it skips non-TUNTAP_MODE_TAP links), so getTapByName/cleanupConflictingTap should be consistent: treat "device exists but is not a Cube TAP" as "no conflicting tap under this name — continue", and only propagate real netlink errors.

[FakeLearne]

That makes sense.

[cubesandboxbot]

Review: fix(cubelet): name sandbox TAPs after IPv4 and reject oversize names (#1552)

AI-generated review — not human-approved.

Overview

The root cause is real and the fix is sound. vishvananda/netlink's LinkAdd copies the requested name into a fixed IFNAMSIZ (16) buffer without checking the length, so a 16-byte z192.168.195.183 was silently truncated to z192.168.195.18 on the kernel side while the runtime kept the full string in memory — create "succeeded" but subsequent by-name lookups (open, restore, destroy) used a string that doesn't match the actual device. Renaming to the bare dotted IPv4 (max 15 bytes) makes the name always fit, and the backward-compatible recovery path (legacy z-prefixed candidates, CIDR-scoped lookup) handles in-flight/leftover devices from older runtimes. Tests for the new parsing, naming, candidateTapNames ordering, and listCubeTaps CIDR filtering are clear and well-targeted.

I verified the diff compiles against the base tree (all callers of the changed listCubeTaps/getTapByName/newRealTapDeviceAdapter signatures are updated; the remaining z<ipv4> strings in test fixtures are standalone mock data, not derived from tapName()), and traced the common.sh awk escaping through die/echo — the new ^(z)?[0-9]+\.…$ pattern reaches awk as intended (matches bare IPv4 and legacy z+IPv4).

Findings

1. Missing explicit IFNAMSIZ guard — PR description overstates the code (low)

The description says "Create fails before LinkAdd if the name is >= IFNAMSIZ", but no such guard exists: newTap calls netlink.LinkAdd directly and tapName returns the input verbatim. The invariant holds today only because a dotted-quad IPv4 is ≤15 bytes. TapDeviceAdapter.Create accepts any net.IP, so an IPv6 (or any 16+ char name) reaching newTap in the future would silently truncate again — the exact bug this PR fixes. Recommend an explicit len(name) >= unix.IFNAMSIZ check in newTap. (Inline comment posted on tapName.)

2. Bare-IPv4 names drop the z namespace marker — broader claim of foreign devices (design tradeoff, worth confirming)

Previously the z prefix was an ownership marker: only z<ipv4>-named TAPs were treated as cube-managed. Now any host TAP (or, for cleanupConflictingTap, any netdev) whose name decodes to an IPv4 inside the configured sandbox CIDR is claimed:

• listCubeTaps adopts it during recovery and keys it by IP;
• cleanupConflictingTap (controller.go:826) destroys it when the allocator hands out that IP on a pool-miss create;
• deploy/one-click/lib/common.sh is_cube_tap_netdev/awk now classify bare-IP-named devices as cube "deployment residue" and delete them on CIDR change.

A pre-existing foreign TAP named e.g. 10.0.0.5 in a 10.0.0.0/8 sandbox would be adopted and, on IP collision, destroyed. This is inherent to the naming constraint (≤15 chars, IP-derived), and the CIDR filter meaningfully narrows it, but it is a genuine widening of blast radius versus the z prefix. Worth an explicit note in the changelog/upgrade docs so operators aren't surprised when a non-cube TAP named like an IP in the sandbox range disappears.

3. List() vs GetByName() nil-CIDR asymmetry (minor)

With an unparseable/empty CIDR (sandboxCIDR == nil), listCubeTaps claims nothing (sandboxCIDR == nil || !Contains → continue), while getTapByName filters nothing (sandboxCIDR != nil && !Contains). The two surfaces disagree about what is a cube TAP in the nil case. Unreachable in production — newProductionControllerDeps constructs the allocator from cfg.CIDR before the adapter and newIPAllocator rejects invalid CIDRs, so net.ParseCIDR in newRealTapDeviceAdapter will always succeed there — but it's a latent inconsistency worth a comment or a shared helper.

4. Test coverage gaps (minor)

The new tests cover listCubeTaps CIDR filtering, but not getTapByName with the CIDR filter, and there is no test exercising the new two-candidate cleanupConflictingTap loop (current-name miss → legacy z-name hit) in startup_recover.go. The latter is the critical upgrade-path behavior (recovering/cleaning leftover z-prefixed devices after rollout) and would be cheap to cover with a fake adapter.

Minor nits

• parseCubeTapIP strips exactly one leading z via TrimPrefix; a device named zz10.0.0.1 is correctly rejected, but the intent ("a leftover z prefix") could be stated explicitly in the name (e.g. strings.CutPrefix) for clarity — behavior is correct as-is.

Verification notes

• The silent-truncation premise is consistent with how LinkAdd fills the fixed-size interface-name buffer; the bare-IPv4 rename (≤15 chars incl. NUL) is sufficient to avoid it for the IPv4-only allocator.
• Production cfg.CIDR is always valid (validated by newIPAllocator before newRealTapDeviceAdapter is constructed), so the nil-CIDR branches are purely defensive.
• Legacy state files (host_tap_name: z<ipv4>) still reconcile: restoreTap looks up the stored name directly, and listCubeTaps/getTapByName accept the z prefix when the decoded IP is in the sandbox CIDR.

[chenhengqi]

What does this check mean?

[FakeLearne]

done

[chenhengqi]

This seems unnecessary.

[FakeLearne]

done

[chenhengqi]

Always trim prefix and parse IP?

[FakeLearne]

done

[cubesandboxbot]

The PR description states "Create fails before LinkAdd if the name is >= IFNAMSIZ", but no such guard exists in this diff. newTap still calls netlink.LinkAdd(tapConfig) directly with name := tapName(ip.String()) (tap_device.go:130) — and tapName here just returns the input verbatim.

The fix works only because a dotted-quad IPv4 string is at most 15 bytes (e.g. 255.255.255.255), so the 16-byte z<ipv4> overflow can no longer occur. But TapDeviceAdapter.Create accepts any net.IP, and nothing prevents an IPv6 address (or any other 16+ char input) from reaching newTap in the future — in which case netlink.LinkAdd will silently truncate the name again and reproduce the exact bug this PR fixes.

Suggest making the described invariant explicit, e.g. in newTap:

name := tapName(ip.String())
if len(name) >= unix.IFNAMSIZ {
    return nil, fmt.Errorf("tap name %q exceeds IFNAMSIZ", name)
}