Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import org.apache.cloudstack.api.command.user.vm.DeployVnfApplianceCmd;
import org.apache.cloudstack.framework.config.ConfigKey;
import java.util.List;
import java.util.Map;

public interface VnfTemplateManager {

Expand All @@ -44,9 +45,12 @@ public interface VnfTemplateManager {

void validateVnfApplianceNics(VirtualMachineTemplate template, List<Long> networkIds);

void validateVnfApplianceNetworksMap(VirtualMachineTemplate template, Map<Integer, Long> vmNetworkMap);

SecurityGroup createSecurityGroupForVnfAppliance(DataCenter zone, VirtualMachineTemplate template, Account owner, DeployVnfApplianceCmd cmd);

void createIsolatedNetworkRulesForVnfAppliance(DataCenter zone, VirtualMachineTemplate template, Account owner,
UserVm vm, DeployVnfApplianceCmd cmd)
throws InsufficientAddressCapacityException, ResourceAllocationException, ResourceUnavailableException;

}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
// under the License.
package org.apache.cloudstack.storage.template;

import com.cloud.agent.api.to.deployasis.OVFNetworkTO;
import com.cloud.exception.InvalidParameterValueException;
import com.cloud.network.VNF;
import com.cloud.storage.Storage;
Expand Down Expand Up @@ -124,6 +125,9 @@ public static void validateVnfNics(List<VNF.VnfNic> nicsList) {
public static void validateApiCommandParams(BaseCmd cmd, VirtualMachineTemplate template) {
if (cmd instanceof RegisterVnfTemplateCmd) {
RegisterVnfTemplateCmd registerCmd = (RegisterVnfTemplateCmd) cmd;
if (registerCmd.isDeployAsIs() && CollectionUtils.isNotEmpty(registerCmd.getVnfNics())) {
throw new InvalidParameterValueException("VNF Template cannot be registered with VNF nics as Template settings are read from OVA.");
}
Comment on lines +128 to +130
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want this always to be true? I could imagine a VNF getting a default gateway… Probably my lack of knowledge. I hope this is in the documentation.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (registerCmd.isDeployAsIs() && CollectionUtils.isNotEmpty(registerCmd.getVnfNics())) {
throw new InvalidParameterValueException("VNF Template cannot be registered with VNF nics as Template settings are read from OVA.");
}
if (registerCmd.isDeployAsIs() && CollectionUtils.isNotEmpty(registerCmd.getVnfNics())) {
throw new InvalidParameterValueException("VNF nics cannot be specified when register a deploy-as-is Template. Please wait until Template settings are read from OVA.");
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it clear now ? @DaanHoogland

validateApiCommandParams(registerCmd.getVnfDetails(), registerCmd.getVnfNics(), registerCmd.getTemplateType());
} else if (cmd instanceof UpdateVnfTemplateCmd) {
UpdateVnfTemplateCmd updateCmd = (UpdateVnfTemplateCmd) cmd;
Expand All @@ -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()));
}
Comment on lines +165 to +167
Copy link

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
}
}
Comment on lines +157 to +169
Copy link

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
}
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@
import com.cloud.agent.api.to.DiskTO;
import com.cloud.agent.api.to.NfsTO;
import com.cloud.agent.api.to.VirtualMachineTO;
import com.cloud.agent.api.to.deployasis.OVFNetworkTO;
import com.cloud.api.ApiDBUtils;
import com.cloud.api.query.dao.UserVmJoinDao;
import com.cloud.api.query.vo.UserVmJoinVO;
Expand All @@ -131,6 +132,7 @@
import com.cloud.dc.DataCenterVO;
import com.cloud.dc.dao.DataCenterDao;
import com.cloud.deploy.DeployDestination;
import com.cloud.deployasis.dao.TemplateDeployAsIsDetailsDao;
import com.cloud.domain.Domain;
import com.cloud.domain.dao.DomainDao;
import com.cloud.event.ActionEvent;
Expand Down Expand Up @@ -313,6 +315,8 @@ public class TemplateManagerImpl extends ManagerBase implements TemplateManager,
protected SnapshotHelper snapshotHelper;
@Inject
VnfTemplateManager vnfTemplateManager;
@Inject
TemplateDeployAsIsDetailsDao templateDeployAsIsDetailsDao;

@Inject
private SecondaryStorageHeuristicDao secondaryStorageHeuristicDao;
Expand Down Expand Up @@ -2172,6 +2176,11 @@ private VMTemplateVO updateTemplateOrIso(BaseUpdateTemplateOrIsoCmd cmd) {
templateType = validateTemplateType(cmd, isAdmin, template.isCrossZones());
if (cmd instanceof UpdateVnfTemplateCmd) {
VnfTemplateUtils.validateApiCommandParams(cmd, template);
UpdateVnfTemplateCmd updateCmd = (UpdateVnfTemplateCmd) cmd;
if (template.isDeployAsIs() && CollectionUtils.isNotEmpty(updateCmd.getVnfNics())) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nvazquez
when register a deploy-as-is template, the template NICs are not available until the template is downloaded successfully.
I think it is better that user configures VNF nics only when template NICs are fetched from OVA template.

List<OVFNetworkTO> ovfNetworks = templateDeployAsIsDetailsDao.listNetworkRequirementsByTemplateId(template.getId());
VnfTemplateUtils.validateDeployAsIsTemplateVnfNics(ovfNetworks, updateCmd.getVnfNics());
}
vnfTemplateManager.updateVnfTemplate(template.getId(), (UpdateVnfTemplateCmd) cmd);
}
templateTag = ((UpdateTemplateCmd)cmd).getTemplateTag();
Expand Down
6 changes: 5 additions & 1 deletion server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -6127,7 +6127,11 @@ public UserVm createVirtualMachine(DeployVMCmd cmd) throws InsufficientCapacityE
throw new InvalidParameterValueException("Unable to use template " + templateId);
}
if (TemplateType.VNF.equals(template.getTemplateType())) {
vnfTemplateManager.validateVnfApplianceNics(template, cmd.getNetworkIds());
if (template.isDeployAsIs()) {
vnfTemplateManager.validateVnfApplianceNetworksMap(template, cmd.getVmNetworkMap());
} else {
vnfTemplateManager.validateVnfApplianceNics(template, cmd.getNetworkIds());
}
} else if (cmd instanceof DeployVnfApplianceCmd) {
throw new InvalidParameterValueException("Can't deploy VNF appliance from a non-VNF template");
}
Comment thread
weizhouapache marked this conversation as resolved.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,9 @@ public void validateVnfApplianceNics(VirtualMachineTemplate template, List<Long>
if (CollectionUtils.isEmpty(networkIds)) {
throw new InvalidParameterValueException("VNF nics list is empty");
}
if (CollectionUtils.isNotEmpty(networkIds) && template.isDeployAsIs()) {
throw new InvalidParameterValueException("VNF nics list should be empty for deploy-as-is templates");
}
List<VnfTemplateNicVO> vnfNics = vnfTemplateNicDao.listByTemplateId(template.getId());
for (VnfTemplateNicVO vnfNic : vnfNics) {
if (vnfNic.isRequired() && networkIds.size() <= vnfNic.getDeviceId()) {
Expand All @@ -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()) {
Comment thread
weizhouapache marked this conversation as resolved.
throw new InvalidParameterValueException("VNF nic is required but not found: " + vnfNic);
}
}
}
Copy link

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

protected Set<Integer> getOpenPortsForVnfAppliance(VirtualMachineTemplate template) {
Set<Integer> ports = new HashSet<>();
VnfTemplateDetailVO accessMethodsDetail = vnfTemplateDetailsDao.findDetail(template.getId(), VNF.AccessDetail.ACCESS_METHODS.name().toLowerCase());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import com.cloud.api.query.dao.UserVmJoinDao;
import com.cloud.configuration.Resource;
import com.cloud.dc.dao.DataCenterDao;
import com.cloud.deployasis.dao.TemplateDeployAsIsDetailsDao;
import com.cloud.domain.dao.DomainDao;
import com.cloud.event.dao.UsageEventDao;
import com.cloud.exception.InvalidParameterValueException;
Expand Down Expand Up @@ -204,6 +205,8 @@ public class TemplateManagerImplTest {
AccountManager _accountMgr;
@Inject
VnfTemplateManager vnfTemplateManager;
@Inject
TemplateDeployAsIsDetailsDao templateDeployAsIsDetailsDao;

@Inject
HeuristicRuleHelper heuristicRuleHelperMock;
Expand Down Expand Up @@ -956,6 +959,11 @@ public VnfTemplateManager vnfTemplateManager() {
return Mockito.mock(VnfTemplateManager.class);
}

@Bean
public TemplateDeployAsIsDetailsDao templateDeployAsIsDetailsDao() {
return Mockito.mock(TemplateDeployAsIsDetailsDao.class);
}

@Bean
public SnapshotHelper snapshotHelper() {
return Mockito.mock(SnapshotHelper.class);
Expand Down
14 changes: 12 additions & 2 deletions ui/src/views/compute/DeployVnfAppliance.vue
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,7 @@
<div>
<vnf-nics-selection
:items="templateVnfNics"
:templateNics="templateNics"
:networks="networks"
@update-vnf-nic-networks="($event) => updateVnfNicNetworks($event)" />
</div>
Expand Down Expand Up @@ -1293,7 +1294,8 @@ export default {
return tabList
},
showVnfNicsSection () {
return this.networks && this.networks.length > 0 && this.vm.templateid && this.templateVnfNics && this.templateVnfNics.length > 0
return ((this.networks && this.networks.length > 0) || (this.templateNics && this.templateNics.length > 0)) &&
this.vm.templateid && this.templateVnfNics && this.templateVnfNics.length > 0
},
showVnfConfigureManagement () {
const managementDeviceIds = []
Expand All @@ -1303,6 +1305,11 @@ export default {
}
}
for (const deviceId of managementDeviceIds) {
if (this.templateNics && this.templateNics[deviceId] &&
((this.templateNics[deviceId].selectednetworktype === 'Isolated' && this.templateNics[deviceId].selectednetworkvpcid === undefined) ||
(this.templateNics[deviceId].selectednetworktype === 'Shared' && this.templateNics[deviceId].selectednetworkwithsg))) {
return true
}
if (this.vnfNicNetworks && this.vnfNicNetworks[deviceId] &&
((this.vnfNicNetworks[deviceId].type === 'Isolated' && this.vnfNicNetworks[deviceId].vpcid === undefined) ||
(this.vnfNicNetworks[deviceId].type === 'Shared' && this.zone.securitygroupsenabled))) {
Expand Down Expand Up @@ -2005,7 +2012,7 @@ export default {
// All checked networks should be used and only once.
// Required NIC must be associated to a network
// DeviceID must be consequent
if (this.templateVnfNics && this.templateVnfNics.length > 0) {
if (this.templateVnfNics && this.templateVnfNics.length > 0 && (!this.templateNics || this.templateNics.length === 0)) {
let nextDeviceId = 0
const usedNetworkIds = []
const keys = Object.keys(this.vnfNicNetworks)
Expand Down Expand Up @@ -2629,6 +2636,9 @@ export default {
var network = this.options.networks[Math.min(i, this.options.networks.length - 1)]
nic.selectednetworkid = network.id
nic.selectednetworkname = network.name
nic.selectednetworktype = network.type
nic.selectednetworkvpcid = network.vpcid
nic.selectednetworkwithsg = network.service.filter(svc => svc.name === 'SecurityGroupProvider').length > 0
this.nicToNetworkSelection.push({ nic: nic.id, network: network.id })
}
}
Expand Down
5 changes: 5 additions & 0 deletions ui/src/views/compute/wizard/VnfNicsSelection.vue
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
<template #network="{ record }">
<a-form-item style="display: block" :name="'nic-' + record.deviceid">
<a-select
disabled="templateNics || templateNics.length === 0"
Comment thread
weizhouapache marked this conversation as resolved.
Outdated
@change="updateNicNetworkValue($event, record.deviceid)"
optionFilterProp="label"
:filterOption="(input, option) => {
Expand All @@ -75,6 +76,10 @@ export default {
type: Array,
default: () => []
},
templateNics: {
type: Array,
default: () => []
},
networks: {
type: Array,
default: () => []
Expand Down
Loading