Copilot commented on code in PR #13032:
URL: https://github.com/apache/cloudstack/pull/13032#discussion_r3086931699
##########
server/src/main/java/com/cloud/network/vpc/NetworkACLManagerImpl.java:
##########
@@ -442,12 +446,23 @@ public boolean applyACLItemsToNetwork(final long
networkId, final List<NetworkAC
logger.debug("Applying NetworkACL for network: {} with Network ACL
service provider", network);
handled = element.applyNetworkACLs(network, rules);
if (handled) {
- // publish message on message bus, so that network elements
implementing distributed routing
- // capability can act on the event
- _messageBus.publish(_name, "Network_ACL_Replaced",
PublishScope.LOCAL, network);
break;
}
}
+ if (!foundProvider) {
+ // Get provider name and get the element by provider name (it
could be an external provider)
+ String aclProviderName =
networkServiceMapDao.getProviderForServiceInNetwork(network.getId(),
Service.NetworkACL);
+ if (aclProviderName != null) {
+ foundProvider = true;
+ NetworkElement element =
_networkModel.getElementImplementingProvider(aclProviderName);
+ handled = ((NetworkACLServiceProvider)
element).applyNetworkACLs(network, rules);
+ }
Review Comment:
The external-provider fallback path casts `element` without checking for
null or interface compatibility. If `getElementImplementingProvider` returns
null (or a NetworkElement that doesn't implement `NetworkACLServiceProvider`),
this will throw at runtime. Add null/`instanceof` checks and return `false` (or
throw a controlled exception) when the provider can't handle NetworkACL.
##########
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java:
##########
@@ -4438,6 +4482,8 @@ public Map<String, String>
finalizeServicesAndProvidersForNetwork(final NetworkO
if (provider == null) {
provider =
_networkModel.getDefaultUniqueProviderForService(service).getName();
+ } else {
+ provider = _networkModel.resolveProvider(provider).getName();
Review Comment:
Possible NullPointerException: `_networkModel.resolveProvider(provider)` may
return null, but the code immediately calls `.getName()`. Guard against null
(and return a clear Unsupported/Invalid provider error) to avoid failures when
an invalid provider string is stored in the offering service map.
```suggestion
final Provider resolvedProvider =
_networkModel.resolveProvider(provider);
if (resolvedProvider == null) {
throw new InvalidParameterValueException("Invalid
provider " + provider + " configured for service " + service);
}
provider = resolvedProvider.getName();
```
##########
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'))
+ },
Review Comment:
The `runCustomAction` action visibility check hard-codes provider name
`ExternalNetwork`, but extension-backed providers are selected by extension/NSP
name (and the server-side constant introduced is `NetworkExtension`). As
written, the action will likely never appear for extension-backed networks.
Update the predicate to detect extension-backed providers (e.g., via provider
name `NetworkExtension` or by matching any provider name that is a
NetworkOrchestrator extension).
##########
ui/src/views/offering/AddNetworkOffering.vue:
##########
@@ -1029,9 +1078,46 @@ export default {
...(this.forVpc && { NetworkACL: this.Netris }),
...(!this.forVpc && { Firewall: this.Netris })
}
+ } else if (this.isExternalNetworkProvider) {
+ // Extension-backed provider: services come from the extension's
network.capabilities.
+ // this.provider is the extension name (= NSP name)
+ const extProviderObj = {
+ name: this.provider,
+ description: this.provider,
+ enabled: true
+ }
+ const svcMap = { Dhcp: this.VR, Dns: this.VR, UserData: this.VR }
+ // Infer services from the selected extension's network.capabilities
detail
+ const extDef = this.availableExtensionProviders.find(e => e.name ===
this.provider)
+ const services = this._getExtensionServices(extDef)
+ if (services.length > 0) {
+ services.forEach(svc => {
+ if (!['Dhcp', 'Dns', 'UserData'].includes(svc)) {
+ svcMap[svc] = extProviderObj
+ }
+ })
+ } else {
+ // Default services if no capabilities declared
+ svcMap.SourceNat = extProviderObj
+ svcMap.StaticNat = extProviderObj
+ svcMap.PortForwarding = extProviderObj
+ svcMap.Firewall = extProviderObj
+ svcMap.Gateway = extProviderObj
+ }
+ this.externalNetworkSupportedServicesMap = svcMap
+ this.externalNetworkProviderObj = extProviderObj
}
this.fetchSupportedServiceData()
},
+ _getExtensionServices (extDef) {
+ if (!extDef || !extDef.details ||
!extDef.details['network.capabilities']) return []
+ try {
+ const caps = JSON.parse(extDef.details['network.capabilities'])
+ return (caps && caps.services) ? caps.services : []
Review Comment:
`_getExtensionServices` only reads `extDef.details['network.capabilities']`.
Elsewhere (e.g. AddVpcOffering) there is a fallback to `network.services` CSV;
without that fallback, extensions that only set `network.services` won't have
their services reflected in the offering UI. Consider supporting both formats
here for consistency.
##########
server/src/main/java/com/cloud/network/NetworkModelImpl.java:
##########
@@ -1161,7 +1266,7 @@ public Map<Service, Set<Provider>>
getNetworkOfferingServiceProvidersMap(long ne
if (providers == null) {
providers = new HashSet<Provider>();
}
- providers.add(Provider.getProvider(instance.getProvider()));
+ providers.add(resolveProvider(instance.getProvider()));
serviceProviderMap.put(Service.getService(service), providers);
Review Comment:
Possible NPE/data corruption: `resolveProvider(instance.getProvider())` can
return null, but the null value is added to the providers set. This can later
trigger NPEs when code assumes every provider in the set is non-null. Skip null
providers (and/or throw an exception if the DB contains an invalid provider
name).
##########
framework/extensions/src/main/java/org/apache/cloudstack/framework/extensions/manager/ExtensionsManagerImpl.java:
##########
@@ -934,6 +1161,247 @@ 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;
+ });
+ }
+
+ /**
+ * 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 = parseServicesFromDetailKeys(extDetails);
+ if (!parsed.isEmpty()) {
+ return parsed;
+ }
+ // Default: the full set of services NetworkExtensionElement supports
+ return new HashSet<>();
+ }
+
+ /**
+ * Resolves the set of service names from the extension detail map.
+ * From {@code network.services} comma-separated key.
+ */
+ @SuppressWarnings("deprecation")
+ private Set<String> parseServicesFromDetailKeys(Map<String, String>
extDetails) {
+ if (extDetails == null) {
+ return Collections.emptySet();
+ }
+ // New format: "network.services" = "SourceNat,StaticNat,..."
+ if
(extDetails.containsKey(ExtensionHelper.NETWORK_SERVICES_DETAIL_KEY)) {
+ String value =
extDetails.get(ExtensionHelper.NETWORK_SERVICES_DETAIL_KEY);
+ if (StringUtils.isNotBlank(value)) {
+ Set<String> services = new HashSet<>();
+ for (String s : value.split(",")) {
+ String trimmed = s.trim();
+ if (!trimmed.isEmpty()) {
+ services.add(trimmed);
+ }
+ }
+ if (!services.isEmpty()) {
+ return services;
+ }
+ }
+ }
+
+ return Collections.emptySet();
+ }
+
+ /**
+ * Builds a full {@code Map<Service, Map<Capability, String>>} from the
+ * extension detail map. From the split keys
+ * {@code network.services} + {@code network.service.capabilities}.
+ */
+ @SuppressWarnings("deprecation")
+ private Map<Service, Map<Capability, String>>
buildCapabilitiesFromDetailKeys(
+ Map<String, String> extDetails) {
+ if (extDetails == null) {
+ return new HashMap<>();
+ }
+ // New split format
+ if
(extDetails.containsKey(ExtensionHelper.NETWORK_SERVICES_DETAIL_KEY)) {
+ Set<String> serviceNames = parseServicesFromDetailKeys(extDetails);
+ if (!serviceNames.isEmpty()) {
+ JsonObject capsObj = null;
+ if
(extDetails.containsKey(ExtensionHelper.NETWORK_SERVICE_CAPABILITIES_DETAIL_KEY))
{
+ try {
+ capsObj = JsonParser.parseString(
+
extDetails.get(ExtensionHelper.NETWORK_SERVICE_CAPABILITIES_DETAIL_KEY))
+ .getAsJsonObject();
+ } catch (Exception e) {
+ logger.warn("Failed to parse
network.service.capabilities JSON: {}", e.getMessage());
+ }
+ }
Review Comment:
The code only parses `network.services` / `network.service.capabilities`,
but both the UI (e.g. offering creation) and `NetworkExtensionElement`
documentation refer to a single `network.capabilities` JSON detail. If an
extension only provides `network.capabilities`, service flags and capabilities
will be empty and the auto-created NSP will not reflect the extension’s
declared services. Consider adding parsing support for `network.capabilities`
(or updating all callers/docs to consistently use the split keys).
##########
server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java:
##########
@@ -2049,6 +2050,14 @@ protected boolean applyLbRules(Network network,
List<LoadBalancingRule> rules) t
if (handled)
break;
}
+ if (!handled) {
+ // Get provider name and get the element by provider name (it
could be an external provider)
+ String lbProviderName =
_ntwkSrvcDao.getProviderForServiceInNetwork(network.getId(), Service.Lb);
+ if (lbProviderName != null) {
+ NetworkElement element =
_networkModel.getElementImplementingProvider(lbProviderName);
+ handled = ((LoadBalancingServiceProvider)
element).applyLBRules(network, rules);
+ }
Review Comment:
The external-provider fallback path casts `element` without checking for
null or `instanceof LoadBalancingServiceProvider`. If the provider name is
present in the service map but no element implements it (or it doesn't support
LB), this will throw a ClassCastException/NPE. Add null/`instanceof` checks and
handle the unsupported case gracefully.
##########
server/src/main/java/com/cloud/network/firewall/FirewallManagerImpl.java:
##########
@@ -700,18 +718,34 @@ public boolean applyRules(Network network, Purpose
purpose, List<? extends Firew
if (handled)
break;
}
+ if (!handled) {
+ // Get provider name and get the element by provider name (it
could be an external provider)
+ String fwProviderName =
networkServiceMapDao.getProviderForServiceInNetwork(network.getId(),
Service.Firewall);
+ if (fwProviderName != null) {
+ NetworkElement element =
_networkModel.getElementImplementingProvider(fwProviderName);
+ handled = ((FirewallServiceProvider)
element).applyFWRules(network, rules);
+ }
+ }
break;
case PortForwarding:
- for (PortForwardingServiceProvider element : _pfElements) {
+ for (PortForwardingServiceProvider element : _pfElements) {
Network.Provider provider = element.getProvider();
boolean isPfProvider =
_networkModel.isProviderSupportServiceInNetwork(network.getId(),
Service.PortForwarding, provider);
if (!isPfProvider) {
continue;
}
- handled = element.applyPFRules(network,
(List<PortForwardingRule>)rules);
+ handled = element.applyPFRules(network,
(List<PortForwardingRule>)rules);
if (handled)
break;
}
+ if (!handled) {
+ // Get provider name and get the element by provider name (it
could be an external provider)
+ String pfProviderName =
networkServiceMapDao.getProviderForServiceInNetwork(network.getId(),
Service.PortForwarding);
+ if (pfProviderName != null) {
+ NetworkElement element =
_networkModel.getElementImplementingProvider(pfProviderName);
+ handled = ((PortForwardingServiceProvider)
element).applyPFRules(network, (List<PortForwardingRule>) rules);
+ }
Review Comment:
The extension-provider fallback casts `element` to
`PortForwardingServiceProvider` without checking for null or `instanceof`. If
the provider name isn't backed by a PF-capable element, this will throw at
runtime. Add null/`instanceof` checks and handle the unsupported case
gracefully.
##########
server/src/main/java/com/cloud/network/firewall/FirewallManagerImpl.java:
##########
@@ -700,18 +718,34 @@ public boolean applyRules(Network network, Purpose
purpose, List<? extends Firew
if (handled)
break;
}
+ if (!handled) {
+ // Get provider name and get the element by provider name (it
could be an external provider)
+ String fwProviderName =
networkServiceMapDao.getProviderForServiceInNetwork(network.getId(),
Service.Firewall);
+ if (fwProviderName != null) {
+ NetworkElement element =
_networkModel.getElementImplementingProvider(fwProviderName);
+ handled = ((FirewallServiceProvider)
element).applyFWRules(network, rules);
+ }
Review Comment:
The extension-provider fallback casts `element` to `FirewallServiceProvider`
without checking for null or `instanceof`. If `networkServiceMapDao` returns a
provider name with no matching element (or one that doesn't implement
FirewallServiceProvider), this will throw at runtime. Add null/`instanceof`
checks and treat it as "not handled" (or throw a controlled exception).
##########
server/src/main/java/com/cloud/network/vpc/VpcManagerImpl.java:
##########
@@ -1844,6 +1854,8 @@ private Map<String, List<String>>
finalizeServicesAndProvidersForVpc(final long
if (provider == null) {
// Default to VPCVirtualRouter
provider = Provider.VPCVirtualRouter.getName();
+ } else {
+ provider = _ntwkModel.resolveProvider(provider).getName();
}
Review Comment:
Possible NullPointerException: `_ntwkModel.resolveProvider(provider)` may
return null for unknown provider names, but the code immediately calls
`.getName()`. Add a null check and throw an `InvalidParameterValueException`
(or keep the original provider string) when resolution fails.
##########
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java:
##########
@@ -4463,7 +4509,7 @@ private List<Provider> getNetworkProviders(final long
networkId) {
final List<String> providerNames =
_ntwkSrvcDao.getDistinctProviders(networkId);
final List<Provider> providers = new ArrayList<>();
for (final String providerName : providerNames) {
- providers.add(Network.Provider.getProvider(providerName));
+ providers.add(_networkModel.resolveProvider(providerName));
Review Comment:
`getNetworkProviders` adds the result of `resolveProvider` without filtering
nulls. If any provider name is not resolvable, this list will contain null and
can cause downstream NPEs if the providers list is iterated (or logged)
elsewhere. Consider skipping null providers (and optionally logging a warning).
```suggestion
final Provider provider =
_networkModel.resolveProvider(providerName);
if (provider != null) {
providers.add(provider);
}
```
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -3302,7 +3304,7 @@ public ProviderResponse
createNetworkServiceProviderResponse(PhysicalNetworkServ
}
response.setServices(services);
- Provider serviceProvider =
Provider.getProvider(result.getProviderName());
+ Provider serviceProvider =
networkModel.resolveProvider(result.getProviderName());
boolean canEnableIndividualServices =
ApiDBUtils.canElementEnableIndividualServices(serviceProvider);
response.setCanEnableIndividualServices(canEnableIndividualServices);
Review Comment:
`networkModel.resolveProvider(...)` can return null, but the result is
passed into `ApiDBUtils.canElementEnableIndividualServices(...)` which will NPE
on null. Consider calling the new
`ApiDBUtils.canElementEnableIndividualServicesByName(result.getProviderName())`,
or guard against null and default `canEnableIndividualServices` to `false`.
--
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]
