Copilot commented on code in PR #12499:
URL: https://github.com/apache/cloudstack/pull/12499#discussion_r2715779155
##########
server/src/main/java/org/apache/cloudstack/storage/template/VnfTemplateManagerImpl.java:
##########
@@ -213,6 +216,19 @@ public void
validateVnfApplianceNics(VirtualMachineTemplate template, List<Long>
}
}
+ @Override
+ public void validateVnfApplianceNetworksMap(VirtualMachineTemplate
template, Map<Integer, Long> vmNetworkMap) {
+ if (MapUtils.isEmpty(vmNetworkMap)) {
+ throw new InvalidParameterValueException("VNF networks map is
empty");
+ }
+ List<VnfTemplateNicVO> vnfNics =
vnfTemplateNicDao.listByTemplateId(template.getId());
+ for (VnfTemplateNicVO vnfNic : vnfNics) {
+ if (vnfNic.isRequired() && vmNetworkMap.size() <=
vnfNic.getDeviceId()) {
Review Comment:
The validation logic has a bug. It checks if `vmNetworkMap.size() <=
vnfNic.getDeviceId()` to determine if a required NIC is missing. However, this
checks the total size of the map against the device ID, not whether the
specific device ID exists in the map. A map could have size 3 but contain keys
{0, 5, 10}, and this check would incorrectly pass for deviceId 2. The correct
approach is to check if the map contains the specific deviceId as a key:
`!vmNetworkMap.containsKey(vnfNic.getDeviceId())`.
```suggestion
if (vnfNic.isRequired() &&
!vmNetworkMap.containsKey(vnfNic.getDeviceId())) {
```
##########
server/src/main/java/org/apache/cloudstack/storage/template/VnfTemplateManagerImpl.java:
##########
@@ -213,6 +216,19 @@ public void
validateVnfApplianceNics(VirtualMachineTemplate template, List<Long>
}
}
+ @Override
+ public void validateVnfApplianceNetworksMap(VirtualMachineTemplate
template, Map<Integer, Long> vmNetworkMap) {
+ if (MapUtils.isEmpty(vmNetworkMap)) {
+ throw new InvalidParameterValueException("VNF networks map is
empty");
+ }
+ List<VnfTemplateNicVO> vnfNics =
vnfTemplateNicDao.listByTemplateId(template.getId());
+ for (VnfTemplateNicVO vnfNic : vnfNics) {
+ if (vnfNic.isRequired() && vmNetworkMap.size() <=
vnfNic.getDeviceId()) {
+ throw new InvalidParameterValueException("VNF nic is required
but not found: " + vnfNic);
+ }
+ }
+ }
Review Comment:
The new method `validateVnfApplianceNetworksMap` is not covered by any unit
tests. Since this is a critical validation method for deploy-as-is VNF
templates and similar validation methods in the codebase have test coverage,
unit tests should be added to verify the validation logic works correctly.
##########
api/src/main/java/org/apache/cloudstack/storage/template/VnfTemplateUtils.java:
##########
@@ -149,4 +153,18 @@ public static void validateVnfCidrList(List<String>
cidrList) {
}
}
}
+
+ public static void validateDeployAsIsTemplateVnfNics(List<OVFNetworkTO>
ovfNetworks, List<VNF.VnfNic> vnfNics) {
+ if (CollectionUtils.isEmpty(vnfNics)) {
+ return;
+ }
+ if (CollectionUtils.isEmpty(ovfNetworks)) {
+ throw new InvalidParameterValueException("The list of networks
read from OVA is empty. Please wait until the template is fully downloaded and
processed.");
+ }
+ for (VNF.VnfNic vnfNic : vnfNics) {
+ if (vnfNic.getDeviceId() < ovfNetworks.size() &&
!vnfNic.isRequired()) {
+ throw new InvalidParameterValueException(String.format("The
VNF nic [device ID: %s ] is required as it is defined in the OVA template.",
vnfNic.getDeviceId()));
+ }
+ }
+ }
Review Comment:
The new method `validateDeployAsIsTemplateVnfNics` is not covered by any
unit tests. This is a new validation method for deploy-as-is VNF templates, and
given that similar validation methods in the codebase have test coverage, unit
tests should be added to ensure the validation logic works as expected.
##########
ui/src/views/compute/wizard/VnfNicsSelection.vue:
##########
@@ -50,6 +50,7 @@
<template #network="{ record }">
<a-form-item style="display: block" :name="'nic-' + record.deviceid">
<a-select
+ disabled="templateNics || templateNics.length === 0"
Review Comment:
The disabled attribute logic is incorrect. It should be
`disabled="templateNics && templateNics.length > 0"` (when templateNics exist
and have items), not `disabled="templateNics || templateNics.length === 0"`
(which means disabled when templateNics exist OR when it's empty). The current
logic would disable the select when templateNics is truthy OR when it's empty,
which is always true. The intent should be to disable the network selection
when deploy-as-is template networks are already configured.
```suggestion
disabled="templateNics && templateNics.length > 0"
```
##########
api/src/main/java/org/apache/cloudstack/storage/template/VnfTemplateUtils.java:
##########
@@ -149,4 +153,18 @@ public static void validateVnfCidrList(List<String>
cidrList) {
}
}
}
+
+ public static void validateDeployAsIsTemplateVnfNics(List<OVFNetworkTO>
ovfNetworks, List<VNF.VnfNic> vnfNics) {
+ if (CollectionUtils.isEmpty(vnfNics)) {
+ return;
+ }
+ if (CollectionUtils.isEmpty(ovfNetworks)) {
+ throw new InvalidParameterValueException("The list of networks
read from OVA is empty. Please wait until the template is fully downloaded and
processed.");
+ }
+ for (VNF.VnfNic vnfNic : vnfNics) {
+ if (vnfNic.getDeviceId() < ovfNetworks.size() &&
!vnfNic.isRequired()) {
+ throw new InvalidParameterValueException(String.format("The
VNF nic [device ID: %s ] is required as it is defined in the OVA template.",
vnfNic.getDeviceId()));
+ }
Review Comment:
The validation logic has a flaw. It checks if `vnfNic.getDeviceId() <
ovfNetworks.size() && !vnfNic.isRequired()`, which throws an error if the
vnfNic is NOT required. This is backwards - the error message says "The VNF nic
[device ID: %s ] is required" but the condition checks `!vnfNic.isRequired()`.
The logic should be: if the device ID corresponds to an OVF network, the VNF
nic MUST be required. So the condition should be checking if it's defined in
OVF but marked as not required in the VNF configuration.
--
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]
