msinhore commented on PR #12102:
URL: https://github.com/apache/cloudstack/pull/12102#issuecomment-4387094179
Hi @abh1sek @manojkr — really nice feature, thanks! Trying it out against a
NetworkOrchestrator extension I've been validating in lab and I think there's
one gap that prevents it from being fully usable for the network-extension
framework introduced in #13032.
## `spec.details` is parsed but not persisted
`ExtensionsImportManagerImpl.importExtensionInternal` calls:
```java
extensionsManager.createExtension(
name, description, spec.type, entrypoint.path, "Enabled",
false, Collections.emptyMap()
);
```
The last argument is the details map, and it is hard-coded to empty rather
than being sourced from `extensionConfig.spec.getDetails()`. Even though the
YAML parser already populates `Spec.details` and `ExtensionConfig` exposes
`getDetails()`, those keys never make it to `extension_details`.
For NetworkOrchestrator extensions this matters because three keys live in
`extension_details` and have no other delivery channel from the manifest:
| Key | Source |
|---|---|
| `network.services` | `ExtensionHelper.NETWORK_SERVICES_DETAIL_KEY`
(#13032) — drives auto-creation of the NetworkServiceProvider. |
| `network.service.capabilities` |
`ExtensionHelper.NETWORK_SERVICE_CAPABILITIES_DETAIL_KEY` (#13032) — fed to
`getCapabilities()` for offering selection. |
| `vif.binding` | `ExtensionHelper.VIF_BINDING_DETAIL_KEY` (follow-up:
weizhouapache/cloudstack#4) — opt-in flag so OVS-backed extensions get
`BroadcastDomainType.Lswitch` and `--nic-uuid` propagation. |
Without `spec.details` flowing through, none of these can be set at import
time and the imported extension is silently mis-configured (e.g. NSP not
generated, or NIC binding falls back to VLAN).
The fix looks like a one-liner:
```diff
- extensionsManager.createExtension(
- name, description, spec.type, entrypoint.path,
Extension.State.Enabled.name(),
- false, Collections.emptyMap());
+ Map<String, String> details = extensionConfig.spec.getDetails() != null
+ ? extensionConfig.spec.getDetails()
+ : Collections.emptyMap();
+ extensionsManager.createExtension(
+ name, description, spec.type, entrypoint.path,
Extension.State.Enabled.name(),
+ false, details);
```
## Working manifest for reference
The `ovn-extension` (real lab usage, NetworkOrchestrator-typed) manifest is
at
https://github.com/msinhore/cloudstack-ovn-network-extension/blob/main/manifest.yaml
— happy to use it as a test fixture for the network-extension path if useful.
## Optional / nice-to-have
These do not block import for our case but would tighten the schema:
1. **Validate `kind`** — currently any string works. Worth enforcing
`OrchestratorExtension` ↔ `Orchestrator` and `NetworkOrchestratorExtension` ↔
`NetworkOrchestrator` consistency between `kind` and `spec.type`.
2. **Typed `spec.networkOrchestrator` block** — parallel to
`spec.orchestrator`, holding `services`, `serviceCapabilities`, `vifBinding`
and an optional `physicalNetworkDetails` hint list. The UI could then render a
typed form for the per-physical-network details (the operator still has to run
`registerExtension` to attach to a physical network with zone-specific values
like `ovn_nb_connection`, but the manifest could declare which keys are
expected and required).
3. **`source.type: zip`** — fetch a release tarball directly instead of
`<url>/archive/refs/heads/<refs>.zip`. Useful for repos hosted outside
github/gitlab.
Happy to send a PR for #1 (the blocker) on top of yours if that helps.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
