Copilot commented on code in PR #13032:
URL: https://github.com/apache/cloudstack/pull/13032#discussion_r3168255207
##########
server/src/main/java/com/cloud/network/NetworkModelImpl.java:
##########
@@ -1201,9 +1310,76 @@ public List<? extends Provider>
listSupportedNetworkServiceProviders(String serv
}
}
+ // Also include extension-backed NetworkExtension providers registered
in
+ // physical_network_service_providers whose provider name matches a
registered
+ // NetworkOrchestrator extension (detected via
extensionHelper.isNetworkExtensionProvider).
+ //
+ // We use _pNSPDao.listBy(physNetId) to enumerate all NSP entries,
then check
+ // each provider name against the extension registry. This avoids a
separate
+ // pass over all physical-network/extension combinations.
+ // resolveProvider() creates a transient Provider (not added to the
static list)
+ // for extension names that are not in the built-in registry.
+ try {
+ List<PhysicalNetworkVO> physNets = _physicalNetworkDao.listAll();
+ if (physNets != null) {
+ // Use a set to avoid adding the same provider name twice
(multiple phys-nets)
+ Set<String> addedExtProviders = new HashSet<>();
+ for (PhysicalNetworkVO physNet : physNets) {
+
List<com.cloud.network.dao.PhysicalNetworkServiceProviderVO> nsps =
+ _pNSPDao.listBy(physNet.getId());
+ if (nsps == null) continue;
Review Comment:
`listSupportedNetworkServiceProviders` now does
`_physicalNetworkDao.listAll()` and then calls `_pNSPDao.listBy(...)` for each
physical network. This creates an N+1 query pattern and can make
`listSupportedNetworkServices` expensive in large environments. Consider adding
a DAO method to fetch all `PhysicalNetworkServiceProviderVO` rows in one query
(or a join), then filter/dedupe in-memory (or cache extension provider names).
##########
framework/extensions/src/main/java/org/apache/cloudstack/framework/extensions/manager/ExtensionsManagerImpl.java:
##########
@@ -880,19 +1022,102 @@ public Extension
registerExtensionWithResource(RegisterExtensionCmd cmd) {
String resourceType = cmd.getResourceType();
if (!EnumUtils.isValidEnum(ExtensionResourceMap.ResourceType.class,
resourceType)) {
throw new InvalidParameterValueException(
- String.format("Currently only [%s] can be used to register
an extension of type Orchestrator",
+ String.format("Currently only [%s] can be used to register
an extension",
EnumSet.allOf(ExtensionResourceMap.ResourceType.class)));
}
+ ExtensionVO extension = extensionDao.findById(extensionId);
+ if (extension == null) {
+ throw new InvalidParameterValueException("Invalid extension
specified");
+ }
+ ExtensionResourceMap.ResourceType resType =
ExtensionResourceMap.ResourceType.valueOf(resourceType);
+ if (ExtensionResourceMap.ResourceType.PhysicalNetwork.equals(resType))
{
+ PhysicalNetworkVO physicalNetwork =
physicalNetworkDao.findByUuid(resourceId);
+ if (physicalNetwork == null) {
+ physicalNetwork =
physicalNetworkDao.findById(Long.parseLong(resourceId));
Review Comment:
When registering an extension to a PhysicalNetwork, if
`physicalNetworkDao.findByUuid(resourceId)` returns null and `resourceId` is
not numeric, `Long.parseLong(resourceId)` will throw `NumberFormatException`
and surface as a 500. Consider wrapping the parse in a try/catch and throwing
an `InvalidParameterValueException` with a clear message instead.
```suggestion
try {
physicalNetwork =
physicalNetworkDao.findById(Long.parseLong(resourceId));
} catch (NumberFormatException e) {
throw new InvalidParameterValueException("Invalid
physical network ID specified");
}
```
##########
ui/src/views/offering/AddVpcOffering.vue:
##########
@@ -431,6 +431,13 @@ export default {
this.zoneLoading = false
})
},
+ isVpcCoreProvider (providerName) {
+ return ['VpcVirtualRouter', 'Netscaler', 'BigSwitchBcf',
'ConfigDrive'].includes(providerName)
+ },
+ isDynamicExtensionProvider (providerName) {
+ const knownProviders = ['VirtualRouter', 'VpcVirtualRouter',
'InternalLbVm', 'Netscaler', 'BigSwitchBcf', 'ConfigDrive', 'Nsx', 'Netris']
+ return !knownProviders.includes(providerName)
+ },
Review Comment:
`isDynamicExtensionProvider()` currently treats any provider not in
`knownProviders` as a “dynamic extension provider”. This will also match
built-in providers like `NiciraNvp`, `Ovs`, and `JuniperContrailVpcRouter`,
causing them to be auto-enabled as if they were extensions. Consider using an
explicit “is extension-backed provider” signal (e.g., API-provided flag /
listExtensions lookup) or at least expanding `knownProviders` so only true
extension providers are classified/enabled dynamically.
##########
ui/src/views/offering/AddNetworkOffering.vue:
##########
@@ -917,11 +924,13 @@ export default {
var providers = svc.provider
providers.forEach(function (provider, providerIndex) {
if (self.forVpc) { // *** vpc ***
- var enabledProviders = ['VpcVirtualRouter', 'Netscaler',
'BigSwitchBcf', 'ConfigDrive']
- if (self.lbType === 'internalLb') {
- enabledProviders.push('InternalLbVm')
+ // Keep the known VPC-safe providers allowlisted and only
additionally
+ // enable dynamically discovered extension providers.
+ if (provider.name === 'InternalLbVm') {
+ provider.enabled = self.lbType === 'internalLb' && svc.name
=== 'Lb'
+ } else {
+ provider.enabled = self.isVpcCoreProvider(provider.name) ||
self.isDynamicExtensionProvider(provider.name)
}
Review Comment:
In VPC mode, the new enablement logic uses `isDynamicExtensionProvider()` to
enable “unknown” providers by default. Because the known-provider list doesn’t
include all built-in providers (e.g. `NiciraNvp`, `Ovs`,
`JuniperContrailVpcRouter`), those will also be enabled as if they were
extension-backed providers. This changes the previous VPC allowlist behavior;
consider keeping the strict allowlist for built-ins and only enabling providers
that are confirmed extension-backed.
##########
framework/extensions/src/main/java/org/apache/cloudstack/framework/extensions/manager/ExtensionsManagerImpl.java:
##########
@@ -694,25 +793,68 @@ public List<ExtensionResponse>
listExtensions(ListExtensionsCmd cmd) {
Long id = cmd.getExtensionId();
String name = cmd.getName();
String keyword = cmd.getKeyword();
+ String typeStr = cmd.getType();
+ String resourceIdStr = cmd.getResourceId();
+ String resourceTypeStr = cmd.getResourceType();
+
+ // If resourceId + resourceType are specified, return only extensions
registered to that resource
+ if (StringUtils.isNotBlank(resourceIdStr) &&
StringUtils.isNotBlank(resourceTypeStr)) {
+ if
(!EnumUtils.isValidEnum(ExtensionResourceMap.ResourceType.class,
resourceTypeStr)) {
+ throw new InvalidParameterValueException("Invalid
resourcetype: " + resourceTypeStr);
+ }
+ ExtensionResourceMap.ResourceType resType =
ExtensionResourceMap.ResourceType.valueOf(resourceTypeStr);
+ // Resolve resourceId to a DB id
+ long resolvedResourceId;
+ if
(ExtensionResourceMap.ResourceType.PhysicalNetwork.equals(resType)) {
+ PhysicalNetworkVO pn =
physicalNetworkDao.findByUuid(resourceIdStr);
+ if (pn == null) {
+ try { pn =
physicalNetworkDao.findById(Long.parseLong(resourceIdStr)); } catch
(NumberFormatException ignored) {}
+ }
+ if (pn == null) throw new
InvalidParameterValueException("Invalid physical network ID: " + resourceIdStr);
+ resolvedResourceId = pn.getId();
+ } else {
+ try { resolvedResourceId = Long.parseLong(resourceIdStr); }
catch (NumberFormatException e) {
+ throw new InvalidParameterValueException("Invalid resource
ID: " + resourceIdStr);
+ }
+ }
Review Comment:
`listExtensions` resource filtering parses `resourceId` as a numeric ID for
all resource types except `PhysicalNetwork`. For `Cluster`, the API/UI commonly
uses UUIDs (and the param description mentions cluster UUID), so this branch
will reject a valid UUID with `Invalid resource ID`. Consider resolving Cluster
IDs via `clusterDao.findByUuid(...)` (and optionally falling back to numeric)
similar to the PhysicalNetwork handling.
--
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]
