Copilot commented on code in PR #13032:
URL: https://github.com/apache/cloudstack/pull/13032#discussion_r3100743358
##########
framework/extensions/src/main/java/org/apache/cloudstack/framework/extensions/manager/ExtensionsManagerImpl.java:
##########
@@ -370,16 +443,55 @@ protected Extension
getExtensionFromResource(ExtensionCustomAction.ResourceType
VirtualMachine vm = (VirtualMachine) object;
Pair<Long, Long> clusterHostId =
virtualMachineManager.findClusterAndHostIdForVm(vm, false);
clusterId = clusterHostId.first();
- }
- if (clusterId == null) {
- return null;
- }
- ExtensionResourceMapVO mapVO =
- extensionResourceMapDao.findByResourceIdAndType(clusterId,
ExtensionResourceMap.ResourceType.Cluster);
- if (mapVO == null) {
+ if (clusterId == null) {
+ return null;
+ }
+ ExtensionResourceMapVO mapVO =
+ extensionResourceMapDao.findByResourceIdAndType(clusterId,
ExtensionResourceMap.ResourceType.Cluster);
+ if (mapVO == null) {
+ return null;
+ }
+ return extensionDao.findById(mapVO.getExtensionId());
+ } else if (resourceType == ExtensionCustomAction.ResourceType.Network)
{
+ Network network = (Network) object;
+ Long physicalNetworkId = network.getPhysicalNetworkId();
+ if (physicalNetworkId == null) {
+ return null;
+ }
+ // Use provider-based lookup: match the network's service-map
providers
+ // against extension names registered on the physical network.
+ // This correctly handles multiple different extensions on the
same physical network.
+ List<String> providers =
networkServiceMapDao.getDistinctProviders(network.getId());
+ if (CollectionUtils.isNotEmpty(providers)) {
+ for (String providerName : providers) {
+ Extension ext =
getExtensionForPhysicalNetworkAndProvider(physicalNetworkId, providerName);
+ if (ext != null) {
+ return ext;
+ }
+ }
+ }
+ // Fallback: return the first extension registered on the physical
network
+ List<ExtensionResourceMapVO> maps =
extensionResourceMapDao.listByResourceIdAndType(
+ physicalNetworkId,
ExtensionResourceMap.ResourceType.PhysicalNetwork);
+ if (CollectionUtils.isEmpty(maps)) {
+ return null;
+ }
+ return extensionDao.findById(maps.get(0).getExtensionId());
Review Comment:
`getExtensionFromResource` falls back to returning the *first* extension
registered on the physical network when none of the network's service-map
providers match an extension name. This can route custom actions to the wrong
extension (and is nondeterministic if multiple extensions are registered), and
it can also incorrectly return an extension for networks that aren't actually
managed by any extension provider. Consider removing this fallback, or only
using it when there's exactly one registered extension and the network is
confirmed to use an extension-backed provider.
```suggestion
return null;
```
##########
server/src/main/java/com/cloud/network/NetworkServiceImpl.java:
##########
@@ -400,6 +401,8 @@ public class NetworkServiceImpl extends ManagerBase
implements NetworkService, C
@Inject
NetworkDetailsDao _networkDetailsDao;
@Inject
+ ExtensionsManager extensionsManager;
+ @Inject
Review Comment:
`ExtensionsManager extensionsManager` is injected but not referenced
anywhere in this class. If it isn't needed, please remove the injection/import
to avoid confusion; if it is intended for upcoming logic, consider adding the
usage in this PR or a TODO explaining why it's required.
##########
framework/extensions/src/main/java/org/apache/cloudstack/framework/extensions/manager/ExtensionsManagerImpl.java:
##########
@@ -934,6 +1173,246 @@ 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 savedExtensionMap;
+ });
+ }
+
+ /**
+ * Resolves the set of network service names declared in the extension's
+ * {@code network.services} detail. Falls back to an empty set if not
present
+ */
+ private Set<String> resolveExtensionServices(Extension extension) {
+ Map<String, String> extDetails =
extensionDetailsDao.listDetailsKeyPairs(extension.getId());
+ Set<String> parsed = parseNetworkServicesFromDetailKeys(extDetails);
+ if (!parsed.isEmpty()) {
+ return parsed;
+ }
+ // Default: the full set of services NetworkExtensionElement supports
+ return new HashSet<>();
Review Comment:
`resolveExtensionServices` says it defaults to “the full set of services
NetworkExtensionElement supports”, but currently returns an empty set. When
`network.services` is not provided, this causes the auto-created
`physical_network_service_providers` entry to have all `*_service_provided`
flags set to false, which can prevent the extension provider from showing up in
service/provider selection flows. Either enforce `network.services` as required
for NetworkOrchestrator extensions, or actually default this to the supported
service set (and set the NSP flags accordingly).
##########
ui/src/views/infra/network/ServiceProvidersTab.vue:
##########
@@ -170,10 +192,14 @@ export default {
actionLoading: false,
showFormAction: false,
currentAction: {},
- tabKey: 'BaremetalDhcpProvider'
+ tabKey: 'BaremetalDhcpProvider',
+ registeredExtensions: []
}
},
computed: {
+ isExtensionTab () {
+ return this.registeredExtensions.some(ext => ext.name === this.tabKey)
+ },
Review Comment:
The computed property `isExtensionTab` is currently unused (no references in
template or methods). Consider removing it or wiring it into the logic that
differentiates hardcoded vs extension-backed tabs to avoid dead code.
--
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]
