Copilot commented on code in PR #13032:
URL: https://github.com/apache/cloudstack/pull/13032#discussion_r3279351831
##########
api/src/main/java/com/cloud/network/Network.java:
##########
@@ -250,11 +252,47 @@ public static Provider getProvider(String providerName) {
return null;
}
+ /** Private constructor for transient (non-registered) providers. */
+ private Provider(String name) {
+ this.name = name;
+ this.isExternal = false;
+ this.needCleanupOnShutdown = true;
+ // intentionally NOT added to supportedProviders
+ }
+
+ /**
+ * Creates a transient (non-registered) {@link Provider} with the
given name.
+ *
+ * <p>The new instance is <em>not</em> added to {@code
supportedProviders}, so it
+ * will never be returned by {@link #getProvider(String)} and will not
pollute the
+ * global provider registry. Use this for dynamic / extension-backed
providers
+ * whose names are only known at runtime (e.g. NetworkOrchestrator
extensions).</p>
+ *
+ * @param name the provider name (typically the extension name)
+ * @return a transient {@link Provider} instance with the given name
+ */
+ public static Provider createTransientProvider(String name) {
+ return new Provider(name);
+ }
+
@Override public String toString() {
return new ToStringBuilder(this, ToStringStyle.SHORT_PREFIX_STYLE)
.append("name", name)
.toString();
}
+
+ @Override
+ public boolean equals(Object obj) {
+ if (this == obj) return true;
+ if (!(obj instanceof Provider)) return false;
+ Provider provider = (Provider) obj;
+ return this.name.equals(provider.name);
+ }
+
+ @Override
+ public int hashCode() {
+ return name.hashCode();
Review Comment:
Provider.getProvider(String) matches names case-insensitively, but
Provider.equals/hashCode are case-sensitive (name.equals / name.hashCode). This
can lead to duplicate Provider instances in sets/maps and inconsistent lookups
when provider names differ only by case (likely for user-supplied extension
provider names). Consider making equals/hashCode case-insensitive (e.g.,
normalize to lower-case) to align with getProvider semantics.
##########
server/src/main/java/com/cloud/network/NetworkModelImpl.java:
##########
@@ -1201,9 +1311,87 @@ public List<? extends Provider>
listSupportedNetworkServiceProviders(String serv
}
}
+ // Also include extension-backed NetworkExtension providers.
+ // resolveProvider() creates a transient Provider (not added to the
static list)
+ // for extension names that are not in the built-in registry.
+ try {
+ List<PhysicalNetworkServiceProviderVO> nsps = _pNSPDao.listAll();
+ if (CollectionUtils.isNotEmpty(nsps)) {
+ Set<String> extensionProviderNames = new HashSet<>();
+ List<Extension> extensions =
extensionHelper.listExtensionsByType(Extension.Type.NetworkOrchestrator);
+ if (extensions != null) {
+ for (Extension extension : extensions) {
+ if (extension == null ||
StringUtils.isBlank(extension.getName())) {
+ continue;
+ }
+
extensionProviderNames.add(extension.getName().toLowerCase());
+ }
+ }
+
+ if (!extensionProviderNames.isEmpty()) {
+ // Avoid duplicate provider names across multiple physical
networks.
+ Set<String> addedExtProviders = new HashSet<>();
+ for (PhysicalNetworkServiceProviderVO nsp : nsps) {
+ String provName = nsp.getProviderName();
+ if (StringUtils.isBlank(provName)) {
+ continue;
+ }
+
+ if (addedExtProviders.contains(provName) ||
!extensionProviderNames.contains(provName)) {
+ continue;
+ }
Review Comment:
In listSupportedNetworkServiceProviders, extensionProviderNames is stored
lowercased, but later compared against provName without normalizing case (and
addedExtProviders also tracks provName case-sensitively). This will skip valid
extension-backed providers unless the NSP providerName happens to already be
lowercase. Normalize provName (e.g., toLowerCase) for both membership checks
and the dedup set, or use case-insensitive comparisons consistently.
##########
framework/extensions/src/main/java/org/apache/cloudstack/framework/extensions/manager/ExtensionsManagerImpl.java:
##########
@@ -714,6 +815,14 @@ public List<ExtensionResponse>
listExtensions(ListExtensionsCmd cmd) {
sc.setParameters("keyword", "%" + keyword + "%");
}
+ if (typeStr != null) {
+ Extension.Type type = EnumUtils.getEnum(Extension.Type.class,
typeStr);
+ if (type == null) {
+ throw new InvalidParameterValueException("Invalid type: " +
typeStr);
+ }
+ sc.setParameters("type", type);
+ }
Review Comment:
listExtensions now validates the type parameter using EnumUtils.getEnum,
which is case-sensitive. The UI filter wiring (AutogenView) passes values like
"orchestrator"/"networkorchestrator", which will now throw "Invalid type"
errors. Use a case-insensitive enum parse (e.g., EnumUtils.getEnumIgnoreCase)
or normalize accepted values to avoid breaking existing UI filter behavior.
##########
ui/src/config/section/extension.js:
##########
@@ -45,7 +45,7 @@ export default {
return fields
},
details: ['name', 'description', 'id', 'type', 'details', 'path',
'pathready', 'isuserdefined', 'orchestratorrequirespreparevm',
'reservedresourcedetails', 'created'],
- filters: ['orchestrator'],
+ filters: ['orchestrator', 'networkorchestrator'],
tabs: [{
name: 'details',
Review Comment:
The extension list view filters use lowercase values ("orchestrator",
"networkorchestrator") which are sent as the API 'type' query param. With the
new server-side type validation, these values will be rejected unless they
match the enum names. Update the filter values to
"Orchestrator"/"NetworkOrchestrator" (or map them before calling
listExtensions) to keep filtering working.
##########
framework/extensions/src/main/java/org/apache/cloudstack/framework/extensions/manager/ExtensionsManagerImpl.java:
##########
@@ -1499,6 +1998,177 @@ public CustomActionResultResponse
runCustomAction(RunCustomActionCmd cmd) {
return response;
}
+ /**
+ * Executes a custom action for a Network resource by delegating to an
+ * available {@link NetworkCustomActionProvider} (e.g.
NetworkExtensionElement).
+ * This path does NOT go through the agent framework.
+ */
+ protected CustomActionResultResponse runNetworkCustomAction(Network
network,
+ ExtensionCustomActionVO customActionVO, ExtensionVO extensionVO,
+ ExtensionCustomAction.ResourceType actionResourceType,
+ Map<String, String> cmdParameters) {
+
+ final String actionName = customActionVO.getName();
+ CustomActionResultResponse response = new CustomActionResultResponse();
+ response.setId(customActionVO.getUuid());
+ response.setName(actionName);
+ response.setObjectName("customactionresult");
+ Map<String, String> result = new HashMap<>();
+ response.setSuccess(false);
+ result.put(ApiConstants.MESSAGE, getActionMessage(false,
customActionVO, extensionVO, actionResourceType, network));
+
+ // Resolve action parameters
+ List<ExtensionCustomAction.Parameter> actionParameters = null;
+ Pair<Map<String, String>, Map<String, String>> allDetails =
+
extensionCustomActionDetailsDao.listDetailsKeyPairsWithVisibility(customActionVO.getId());
+ if (allDetails.second().containsKey(ApiConstants.PARAMETERS)) {
+ actionParameters = ExtensionCustomAction.Parameter.toListFromJson(
+ allDetails.second().get(ApiConstants.PARAMETERS));
+ }
+ Map<String, Object> parameters = null;
+ if (CollectionUtils.isNotEmpty(actionParameters)) {
+ parameters =
ExtensionCustomAction.Parameter.validateParameterValues(actionParameters,
cmdParameters);
+ }
+
+ // Find the provider name for this network (try CustomAction first,
then other services)
+ String providerName = null;
+ for (Service service : new Service[]{Service.CustomAction,
Service.SourceNat, Service.StaticNat,
+ Service.PortForwarding, Service.Firewall, Service.Gateway}) {
+ providerName =
networkServiceMapDao.getProviderForServiceInNetwork(network.getId(), service);
+ if (StringUtils.isNotBlank(providerName)) {
+ break;
+ }
+ }
+ if (StringUtils.isBlank(providerName)) {
+ logger.error("No network service provider found for network {}",
network.getId());
+ result.put(ApiConstants.DETAILS, "No network service provider
found for this network");
+ response.setResult(result);
+ return response;
+ }
+
+ // Get the network element implementing that provider
+ NetworkElement element =
networkModel.getElementImplementingProvider(providerName);
+ if (element == null) {
+ logger.error("No NetworkElement found implementing provider '{}'
for network {}", providerName, network.getId());
+ result.put(ApiConstants.DETAILS, "No network element found for
provider: " + providerName);
+ response.setResult(result);
+ return response;
+ }
Review Comment:
runNetworkCustomAction/runVpcCustomAction choose providerName from the
network/vpc service-map and then dispatch to that provider, without verifying
it matches the custom action's owning extension (extensionVO). A caller can
potentially run an action belonging to one extension against a resource managed
by another provider (or vice versa), causing unexpected execution or confusing
failures. Enforce that providerName resolves to extensionVO.getName()
(case-insensitive) before dispatching, and fail fast if they differ.
##########
ui/src/views/infra/network/ServiceProvidersTab.vue:
##########
@@ -1148,6 +1171,51 @@ export default {
return
}
this.fetchServiceProvider()
+ this.fetchRegisteredExtensions()
+ },
+ fetchRegisteredExtensions () {
+ // Load NetworkOrchestrator extensions registered to this physical
network
+ getAPI('listExtensions', {
+ type: 'NetworkOrchestrator',
+ resourceid: this.resource.id,
+ resourcetype: 'PhysicalNetwork'
+ }).then(json => {
+ this.registeredExtensions = (json.listextensionsresponse &&
json.listextensionsresponse.extension) || []
Review Comment:
fetchRegisteredExtensions calls listExtensions with resourceid/resourcetype
parameters, but listExtensions doesn't define these parameters (only
id/name/keyword/details/type). If the API rejects unknown params, this request
will fail and dynamic provider tabs will never appear. Either add proper
resource-scoping params to listExtensions server-side, or fetch by type only
and filter client-side using the returned resources list.
--
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]
