Copilot commented on code in PR #13032:
URL: https://github.com/apache/cloudstack/pull/13032#discussion_r3093229558
##########
server/src/main/java/com/cloud/network/vpc/VpcManagerImpl.java:
##########
@@ -2027,9 +2043,12 @@ private boolean checkAndUpdateRouterSourceNatIp(Vpc vpc,
String sourceNatIp) {
if (! userIps.isEmpty()) {
try {
_ipAddrMgr.updateSourceNatIpAddress(requestedIp, userIps);
- if (isVpcForProvider(Provider.Nsx, vpc) ||
isVpcForProvider(Provider.Netris, vpc)) {
+ if (isVpcForProvider(Provider.Nsx, vpc) ||
isVpcForProvider(Provider.Netris, vpc)
+ || isVpcForProvider(Provider.NetworkExtension, vpc)) {
boolean isForNsx =
_vpcOffSvcMapDao.isProviderForVpcOffering(Provider.Nsx, vpc.getVpcOfferingId());
- String providerName = isForNsx ? Provider.Nsx.getName() :
Provider.Netris.getName();
+ boolean isForNetris =
_vpcOffSvcMapDao.isProviderForVpcOffering(Provider.Netris,
vpc.getVpcOfferingId());
+ String providerName = isForNsx ? Provider.Nsx.getName()
+ : (isForNetris ? Provider.Netris.getName() :
Provider.NetworkExtension.getName());
VpcProvider providerElement = (VpcProvider)
_ntwkModel.getElementImplementingProvider(providerName);
Review Comment:
`checkAndUpdateRouterSourceNatIp` tries to detect extension-backed VPCs via
`isVpcForProvider(Provider.NetworkExtension, vpc)` and then dispatches to
provider name `NetworkExtension`. However extension-backed VPC offerings store
the provider as the *extension name* (dynamic provider), so this condition will
never match and dispatching via `NetworkExtension` won’t reach the right
`VpcProvider`. Consider detecting the actual provider name(s) configured on the
VPC offering (e.g., from the VPC offering service map) and, if it’s an
extension-backed provider, dispatch using that provider name (or
`extensionHelper.isNetworkExtensionProvider(providerName)`).
##########
framework/extensions/src/main/java/org/apache/cloudstack/framework/extensions/api/ListExtensionsCmd.java:
##########
@@ -70,6 +70,17 @@ public class ListExtensionsCmd extends BaseListCmd {
+ " When no parameters are passed, all the details are
returned.")
private List<String> details;
+ @Parameter(name = ApiConstants.TYPE, type = CommandType.STRING,
description = "Type of the extension (e.g. Orchestrator, NetworkOrchestrator).
Default is Orchestrator if not set")
+ private String type;
+
+ @Parameter(name = ApiConstants.RESOURCE_ID, type = CommandType.STRING,
+ description = "ID of the resource to list registered extensions
for (e.g. cluster UUID, physical network UUID)")
+ private String resourceId;
+
+ @Parameter(name = ApiConstants.RESOURCE_TYPE, type = CommandType.STRING,
+ description = "Type of the resource (e.g. Cluster,
PhysicalNetwork). Default is Cluster if not set")
+ private String resourceType;
Review Comment:
The `type` parameter description says “Default is Orchestrator if not set”,
but the implementation does not default `type` and will return all extension
types when `type` is null. Update the parameter description (or implement the
default) so API behavior and docs match.
##########
ui/src/views/offering/AddNetworkOffering.vue:
##########
@@ -732,6 +753,30 @@ export default {
this.fetchServiceOfferingData()
this.fetchIpv6NetworkOfferingConfiguration()
this.fetchRoutedNetworkConfiguration()
+ this.fetchExtensionProviders()
+ },
+ fetchExtensionProviders () {
+ // Load NetworkOrchestrator extensions that are registered to at least
one
+ // physical network (i.e. have a corresponding NetworkServiceProvider
entry).
+ // Only these can be selected as a provider when creating a network
offering.
+ getAPI('listExtensions', { type: 'NetworkOrchestrator', state: 'Enabled'
}).then(json => {
+ const allExts = (json.listextensionsresponse &&
json.listextensionsresponse.extension) || []
+ if (allExts.length === 0) {
+ this.availableExtensionProviders = []
+ return
+ }
+ // Filter to those which have at least one matching NSP (nsp name ==
extension name)
+ getAPI('listNetworkServiceProviders', {}).then(nspJson => {
+ const nsps = (nspJson.listnetworkserviceprovidersresponse &&
nspJson.listnetworkserviceprovidersresponse.networkserviceprovider) || []
+ const nspNames = new Set(nsps.map(n => n.name))
+ this.availableExtensionProviders = allExts.filter(e =>
nspNames.has(e.name))
+ }).catch(() => {
+ // Fallback: show all enabled extensions
+ this.availableExtensionProviders = allExts
+ })
+ }).catch(() => {
+ this.availableExtensionProviders = []
+ })
Review Comment:
`fetchExtensionProviders()` is async; if the user selects an
extension-backed provider before `availableExtensionProviders` is populated,
`isExternalNetworkProvider` will be false during `handleProviderChange`, so the
external service map won’t be built and won’t self-correct when the extension
list finishes loading. Consider re-evaluating the selected provider after
`availableExtensionProviders` is set (e.g., if the current `provider` matches
an extension, rebuild `externalNetworkSupportedServicesMap` / call
`handleProviderChange(provider)`), or make the external/provider initialization
independent of the timing of this fetch.
##########
framework/extensions/src/main/java/org/apache/cloudstack/framework/extensions/manager/ExtensionsManagerImpl.java:
##########
@@ -880,19 +1017,108 @@ 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));
+ }
+ if (physicalNetwork == null) {
+ throw new InvalidParameterValueException("Invalid physical
network ID specified");
+ }
+ ExtensionResourceMap extensionResourceMap =
registerExtensionWithPhysicalNetwork(physicalNetwork, extension,
cmd.getDetails());
+ return
extensionDao.findById(extensionResourceMap.getExtensionId());
+ }
ClusterVO clusterVO = clusterDao.findByUuid(resourceId);
if (clusterVO == null) {
throw new InvalidParameterValueException("Invalid cluster ID
specified");
}
+ ExtensionResourceMap extensionResourceMap =
registerExtensionWithCluster(clusterVO, extension, cmd.getDetails());
+ return extensionDao.findById(extensionResourceMap.getExtensionId());
+ }
+
+ @Override
+ @ActionEvent(eventType = EventTypes.EVENT_EXTENSION_RESOURCE_UPDATE,
eventDescription = "updating extension resource")
+ public Extension
updateRegisteredExtensionWithResource(UpdateRegisteredExtensionCmd cmd) {
+ final String resourceId = cmd.getResourceId();
+ final Long extensionId = cmd.getExtensionId();
+ final String resourceType = cmd.getResourceType();
+ final Map<String, String> details = cmd.getDetails();
+ final Boolean cleanupDetails = cmd.isCleanupDetails();
+
+ if (!EnumUtils.isValidEnum(ExtensionResourceMap.ResourceType.class,
resourceType)) {
+ throw new InvalidParameterValueException(
+ String.format("Currently only [%s] can be used to update
an extension registration",
+
EnumSet.allOf(ExtensionResourceMap.ResourceType.class)));
+ }
ExtensionVO extension = extensionDao.findById(extensionId);
if (extension == null) {
throw new InvalidParameterValueException("Invalid extension
specified");
}
- ExtensionResourceMap extensionResourceMap =
registerExtensionWithCluster(clusterVO, extension, cmd.getDetails());
- return extensionDao.findById(extensionResourceMap.getExtensionId());
+
+ ExtensionResourceMap.ResourceType resType =
ExtensionResourceMap.ResourceType.valueOf(resourceType);
+ long resolvedResourceId;
+ if (ExtensionResourceMap.ResourceType.PhysicalNetwork.equals(resType))
{
+ PhysicalNetworkVO physicalNetwork =
physicalNetworkDao.findByUuid(resourceId);
+ if (physicalNetwork == null) {
+ try {
+ physicalNetwork =
physicalNetworkDao.findById(Long.parseLong(resourceId));
+ } catch (NumberFormatException ignored) {
+ }
+ }
+ if (physicalNetwork == null) {
+ throw new InvalidParameterValueException("Invalid physical
network ID specified");
+ }
+ resolvedResourceId = physicalNetwork.getId();
+ } else {
+ ClusterVO clusterVO = clusterDao.findByUuid(resourceId);
+ if (clusterVO == null) {
+ throw new InvalidParameterValueException("Invalid cluster ID
specified");
+ }
+ resolvedResourceId = clusterVO.getId();
+ }
+
+ List<ExtensionResourceMapVO> mappings =
extensionResourceMapDao.listByResourceIdAndType(resolvedResourceId, resType);
+ ExtensionResourceMapVO targetMapping = null;
+ if (CollectionUtils.isNotEmpty(mappings)) {
+ for (ExtensionResourceMapVO mapping : mappings) {
+ if (mapping.getExtensionId() == extensionId) {
+ targetMapping = mapping;
+ break;
+ }
+ }
+ }
+ if (targetMapping == null) {
+ throw new InvalidParameterValueException(String.format(
+ "Extension '%s' is not registered with resource %s (%s)",
+ extension.getName(), resourceId, resourceType));
+ }
+
+ if (Boolean.TRUE.equals(cleanupDetails)) {
+
extensionResourceMapDetailsDao.removeDetails(targetMapping.getId());
+ } else {
+ List<ExtensionResourceMapDetailsVO> detailsList =
buildExtensionResourceDetailsArray(targetMapping.getId(), details);
+ if (CollectionUtils.isNotEmpty(detailsList)) {
+ appendHiddenExtensionResourceDetails(targetMapping.getId(),
detailsList);
+ }
+ detailsList = detailsList.stream()
+ .filter(detail -> detail.getValue() != null)
+ .collect(Collectors.toList());
+ if (CollectionUtils.isNotEmpty(detailsList)) {
+ extensionResourceMapDetailsDao.saveDetails(detailsList);
+ } else {
+
extensionResourceMapDetailsDao.removeDetails(targetMapping.getId());
+ }
Review Comment:
When `cleanupDetails` is false and the request omits the `details` map (so
`cmd.getDetails()` becomes empty), the current logic falls through to
`removeDetails(...)`, effectively wiping all existing registration details.
That’s surprising for an “update” API. Consider treating an empty `details` map
as a no-op (keep existing details) unless `cleanupDetails=true` is explicitly
set, or introduce an explicit flag to indicate full replacement vs partial
update.
##########
ui/src/config/section/network.js:
##########
@@ -202,6 +202,20 @@ export default {
}
}
},
+ {
+ api: 'runCustomAction',
+ icon: 'thunderbolt-outlined',
+ label: 'label.run.action',
+ dataView: true,
+ show: (record) => {
+ return 'runCustomAction' in store.getters.apis &&
+ 'listCustomActions' in store.getters.apis &&
+ record.service && record.service.some(s =>
+ s.provider && s.provider.some(p => p.name === 'ExternalNetwork'))
+ },
+ popup: true,
+ component: shallowRef(defineAsyncComponent(() =>
import('@/views/extension/RunCustomAction.vue')))
+ },
Review Comment:
The “Run Action” button visibility check hard-codes provider name
`ExternalNetwork`, but the provider introduced/used for extension-backed
networking appears to be either the extension name (dynamic) or
`NetworkExtension` (core provider). As written, the action will likely never be
shown for extension-backed networks. Consider basing the condition on the
presence of custom actions for the network/extension (e.g., via
`listCustomActions` result) or checking for the actual provider name(s)
returned by the API.
##########
ui/src/views/offering/AddVpcOffering.vue:
##########
@@ -384,6 +406,25 @@ export default {
this.fetchSupportedServiceData()
this.fetchIpv6NetworkOfferingConfiguration()
this.fetchRoutedNetworkConfiguration()
+ this.fetchExtensionProviders()
+ },
+ fetchExtensionProviders () {
+ getAPI('listExtensions', { type: 'NetworkOrchestrator', state: 'Enabled'
}).then(json => {
+ const allExts = (json.listextensionsresponse &&
json.listextensionsresponse.extension) || []
+ if (allExts.length === 0) {
+ this.availableExtensionProviders = []
+ return
+ }
+ getAPI('listNetworkServiceProviders', {}).then(nspJson => {
+ const nsps = (nspJson.listnetworkserviceprovidersresponse &&
nspJson.listnetworkserviceprovidersresponse.networkserviceprovider) || []
+ const nspNames = new Set(nsps.map(n => n.name))
+ this.availableExtensionProviders = allExts.filter(e =>
nspNames.has(e.name))
+ }).catch(() => {
+ this.availableExtensionProviders = allExts
+ })
+ }).catch(() => {
+ this.availableExtensionProviders = []
+ })
Review Comment:
Same async timing issue as in AddNetworkOffering: `handleProviderChange`
relies on `isExternalNetworkProvider`, which depends on
`availableExtensionProviders` being loaded. If the provider is selected before
`fetchExtensionProviders()` completes, the external service map won’t be built
and the UI won’t update automatically once the extension list arrives. Consider
re-running provider initialization once `availableExtensionProviders` is
populated (or making external-provider detection not depend on this fetch
timing).
##########
framework/extensions/src/main/java/org/apache/cloudstack/framework/extensions/manager/ExtensionsManagerImpl.java:
##########
@@ -934,6 +1161,245 @@ public ExtensionResourceMap
registerExtensionWithCluster(Cluster cluster, Extens
return result;
}
+ protected ExtensionResourceMap
registerExtensionWithPhysicalNetwork(PhysicalNetworkVO physicalNetwork,
+ Extension extension, Map<String, String> details) {
+ // Only NetworkOrchestrator extensions can be registered with physical
networks
+ if (!Extension.Type.NetworkOrchestrator.equals(extension.getType())) {
+ throw new InvalidParameterValueException(String.format(
+ "Only extensions of type %s can be registered with a
physical network. "
+ + "Extension '%s' is of type %s.",
+ Extension.Type.NetworkOrchestrator.name(),
+ extension.getName(), extension.getType().name()));
+ }
+
+ // Block registering the exact same extension twice on the same
physical network
+ final ExtensionResourceMap.ResourceType resourceType =
ExtensionResourceMap.ResourceType.PhysicalNetwork;
+ List<ExtensionResourceMapVO> existing =
extensionResourceMapDao.listByResourceIdAndType(
+ physicalNetwork.getId(), resourceType);
+ if (existing != null) {
+ for (ExtensionResourceMapVO ex : existing) {
+ if (ex.getExtensionId() == extension.getId()) {
+ throw new CloudRuntimeException(String.format(
+ "Extension '%s' is already registered with
physical network %s",
+ extension.getName(), physicalNetwork.getId()));
+ }
+ }
+ }
+
+ // Resolve which services this extension provides from its
network.services detail
+ Set<String> services = resolveExtensionServices(extension);
+
+ return
Transaction.execute((TransactionCallbackWithException<ExtensionResourceMap,
CloudRuntimeException>) status -> {
+ // 1. Persist the extension<->physical-network mapping
+ ExtensionResourceMapVO extensionMap = new
ExtensionResourceMapVO(extension.getId(),
+ physicalNetwork.getId(), resourceType);
+ ExtensionResourceMapVO savedExtensionMap =
extensionResourceMapDao.persist(extensionMap);
+
+ // 2. Persist device credentials / details
+ List<ExtensionResourceMapDetailsVO> detailsVOList = new
ArrayList<>();
+ if (MapUtils.isNotEmpty(details)) {
+ for (Map.Entry<String, String> entry : details.entrySet()) {
+ boolean display =
!SENSITIVE_DETAIL_KEYS.contains(entry.getKey().toLowerCase());
+ detailsVOList.add(new
ExtensionResourceMapDetailsVO(savedExtensionMap.getId(),
+ entry.getKey(), entry.getValue(), display));
+ }
+ extensionResourceMapDetailsDao.saveDetails(detailsVOList);
+ }
+
+ // 3. Auto-create the NetworkServiceProvider entry for this
extension so that
+ // the services are visible in the UI and in
listSupportedNetworkServices.
+ // The NSP name equals the extension name; state is Enabled by
default.
+ PhysicalNetworkServiceProviderVO existingNsp =
+ physicalNetworkServiceProviderDao.findByServiceProvider(
+ physicalNetwork.getId(), extension.getName());
+ if (existingNsp == null) {
+ PhysicalNetworkServiceProviderVO nsp =
+ new
PhysicalNetworkServiceProviderVO(physicalNetwork.getId(), extension.getName());
+ applyServicesToNsp(nsp, services);
+ nsp.setState(PhysicalNetworkServiceProvider.State.Enabled);
+ physicalNetworkServiceProviderDao.persist(nsp);
+ logger.info("Auto-created NetworkServiceProvider '{}'
(Enabled) for physical network {} "
+ + "with services {}", extension.getName(),
physicalNetwork.getId(), services);
+ }
+
+ return extensionMap;
Review Comment:
`registerExtensionWithPhysicalNetwork` persists `savedExtensionMap` but
returns `extensionMap` (the pre-persist VO). If callers ever rely on fields
populated during `persist()` (notably the DB id), this will be
incorrect/inconsistent with `registerExtensionWithCluster`. Consider returning
`savedExtensionMap` instead.
```suggestion
return savedExtensionMap;
```
--
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]
