apache / apache/cloudstack-extensions

Improvements to the network-wrapper script

Đang mở
#5 1 bình luận 0 reaction 1 người được giao Được @weizhouapache nhận Xem trên GitHub
Ngôn ngữ chính
Không có dữ liệu ngôn ngữ
Star
4
Fork
4
Merge trung bình
1 giờ
Pull request đã merge (30 ngày)
3

Mô tả

Multiple bugs in `Network-Namespace/network-namespace-wrapper.sh` prevent it from working out of the box

## Summary

Following the setup instructions in the CloudStack `NetworkExtension` protocol doc
(`framework/extensions/.../network/README.md` in apache/cloudstack) to register this
reference implementation as a `NetworkOrchestrator` extension (CloudStack 4.23,
KVM, physical network registered with `guest.network.device`/`public.network.device`),
I hit six distinct issues before a VM could successfully deploy onto a network backed
by this script. Filing them together since they were found in one debugging pass and
several are related.

**Environment:** Apache CloudStack 4.23.0, KVM hosts on Ubuntu 24.04, `network-namespace`
branch, script fetched fresh via the documented raw.githubusercontent.com URL.

---

## 1. `ensure_host_bridge()`'s return-via-stdout leaks into the extension's real stdout

`ensure_host_bridge()` returns its result the usual bash way — `echo "${br}"` — meant to
be captured with `br=$(ensure_host_bridge ...)`. But at **four call sites** it's invoked
as a bare statement, with the actual bridge name recomputed separately on the next line
via `host_bridge_name()`:

- `cmd_implement_network()` (guest bridge): `ensure_host_bridge "${GUEST_ETH}" "${VLAN}"`
- `cmd_assign_ip()` (public bridge): `ensure_host_bridge "${PUB_ETH}" "${PUBLIC_VLAN}"`
- two more identical public-bridge call sites in the VPC source-NAT / source-NAT-IP-update paths

Since CloudStack's `NetworkExtensionElement` treats the command's stdout as the payload
to parse, this leaked bridge name (e.g. `breth0-600`, or later `cloudbr0` once VLAN was
`untagged`) corrupts the real output and gets logged as `Ignoring non-object script output: ...`
or worse, treated as the entire (invalid) response.

**Fix:** redirect all four calls to `/dev/null`:
```bash
ensure_host_bridge "${GUEST_ETH}" "${VLAN}" >/dev/null
...
ensure_host_bridge "${PUB_ETH}" "${PUBLIC_VLAN}" >/dev/null
```

---

## 2. `implement-network` never emits the required `network.broadcast_uri` / `network.broadcast_domain_type` JSON

Per the protocol doc, whenever the extension has the detail
`network.isolation.method=NetworkExtension` set (which selects
`NetworkExtensionGuestNetworkGuru` in CloudStack core), `implement-network`'s stdout
**must** be a JSON object containing `network.broadcast_domain_type` and
`network.broadcast_uri` — enforced unconditionally by
`NetworkExtensionElement.applyNetworkUpdateFromScriptOutput()` in CloudStack core.

`cmd_implement_network()` never emits these keys anywhere in the script, so every
`implement-network` call fails with:

```
Script output is missing required network properties 'network.broadcast_uri' and
'network.broadcast_domain_type' for network ...
```

**Fix:** emit the JSON at the end of `cmd_implement_network()`, e.g.:
```bash
if [ -n "${VLAN}" ]; then
printf '{"network.broadcast_domain_type":"Vlan","network.broadcast_uri":"vlan://%s"}\n' "${VLAN}"
fi
```

---

## 3. No handling for an `untagged` public VLAN

When a zone's public IP range has no VLAN tag, CloudStack passes the literal string
`"untagged"` as `payload.public_vlan`. `ensure_host_bridge()`/`host_bridge_name()` pass
this straight through to:
```bash
ip link add link "${eth}" name "${vif}" type vlan id "${vlan}"
```
which fails immediately since VLAN IDs must be numeric:
```
Error: argument "untagged" is wrong: id is invalid
```
This is a completely normal, common CloudStack zone configuration (public network
without 802.1q tagging), not an edge case.

**Fix:** special-case `vlan == "untagged"` in both functions to reuse the NIC's
*existing* bridge (e.g. `cloudbr0`, detected via `ip -o link show | grep -o 'master [^ ]*'`)
instead of trying to create a fresh VLAN sub-interface + bridge.

---

## 4. Related to #3 — `teardown_host_bridge_if_unused()` would delete the host's shared bridge

Once #3 is fixed so that `host_bridge_name()` can return an *existing*, host-critical
bridge like `cloudbr0` for the untagged case, `teardown_host_bridge_if_unused()` — which
runs `ip link del "${br}"` whenever it thinks the bridge has no other members — becomes
dangerous: it did not create `cloudbr0` and has no business deleting it, but nothing
in the current code prevents it from trying once that bridge is "unused" by this
script's own veth accounting.

**Fix:** short-circuit `teardown_host_bridge_if_unused()` to a no-op whenever
`vlan == "untagged"`.

---

## 5. `pub_veth_host_name()` / `pub_veth_ns_name()` don't apply the existing 15-char shortening logic

The **guest**-side equivalents (`veth_host_name()`, `veth_ns_name()`) already shorten
the generated interface name via `shorten_id()` when `vh--` /
`vn--` would exceed the Linux `IFNAMSIZ` limit (15 chars). The **public**-side
functions never got the same treatment — they just do `echo "vph-${pvlan}-${id}"` /
`echo "vpn-${pvlan}-${id}"` unconditionally.

This is latent for short numeric VLANs but fails as soon as `pvlan` is `"untagged"`
(8 chars) combined with even a short numeric id, e.g. network id `222`:
`vpn-untagged-222` = 17 characters →
```
Error: argument "vpn-untagged-222" is wrong: "name" not a valid ifname
```

**Fix:** apply the same three-tier shortening (`shorten_id()`, then hard-truncate to 15)
already used in `veth_host_name()`/`veth_ns_name()`.

---

## 6. Design gap: the script requires the caller to pre-supply a VLAN; it never self-allocates one

`NetworkExtensionGuestNetworkGuru` deliberately skips CloudStack's normal auto-VLAN
pool — the protocol is designed for the extension's `implement-network` handler to
generate **its own** segment identifier and report it back (the doc's own example uses
`{"network.broadcast_domain_type":"Lswitch","network.broadcast_uri":"ovn://cs-net-42"}`,
no VLAN involved at all).

This script instead just echoes back whatever `payload.vlan` it was given and never
allocates anything itself, so a plain auto-VLAN Isolated network offering (no
`specifyvlan`) always reaches `implement-network` with an empty VLAN, and the script's
device-naming logic (`vn--`) then fails outright (`Cannot find device "vn--"`).
In practice this forces every offering built on this extension to use
`specifyvlan=true` plus an explicit `vlan=` on every `createNetwork` call, which isn't
mentioned anywhere in the README's "CloudStack Setup Steps".

**Suggested fix:** either (a) have `implement-network` fall back to self-generating a
stable identifier (e.g. derived from the network/VPC id) when `payload.vlan` is empty,
consistent with the protocol's intent, or (b) at minimum, document the
`specifyvlan=true` + explicit `vlan=` requirement explicitly in the README's setup
steps.

---

## 7. (Minor / docs) Missing host packages fail silently as far as CloudStack is concerned

`cmd_restore_network()` hard-requires `dnsmasq`, `apache2`/`httpd`, and `haproxy` on the
KVM host via `_require_binary`, but `die()` in this script only logs to the wrapper's
own local log file (`/tmp/cloudstack-extensions/.log` on the KVM host) — nothing
reaches stdout, so CloudStack (and the mgmt-server SSH proxy's exit-code-3 convention
for "remote script returned non-zero") only ever sees an **empty** error message. This
makes a missing-package issue on a KVM host very hard to diagnose from the CloudStack
side alone.

**Suggested fix:** either have `_require_binary`'s `die()` path also print to stdout
(so it surfaces in the CloudStack-side log), or list `dnsmasq`, `apache2`, `haproxy` as
explicit prerequisites in the README's KVM host setup section.

Hướng dẫn đóng góp

Chưa lập chỉ mục được hướng dẫn đóng góp cho kho mã nguồn này

Đánh giá

Issue này chưa được đánh giá.

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.