Copilot commented on code in PR #13591:
URL: https://github.com/apache/cloudstack/pull/13591#discussion_r4155801935
##########
plugins/network-elements/netris/src/main/java/org/apache/cloudstack/service/NetrisApiClientImpl.java:
##########
@@ -372,6 +394,17 @@ public boolean deleteNatRule(DeleteNetrisNatRuleCommand
cmd) {
}
String natRuleName = cmd.getNatRuleName();
NatGetBody existingNatRule = netrisNatRuleExists(natRuleName);
+ // Backward compatibility: rules created before the public-IP
suffix was added use the legacy name
+ if (existingNatRule == null &&
"STATICNAT".equals(cmd.getNatRuleType())) {
+ String legacyName = getLegacyStaticNatRuleName(natRuleName);
+ if (legacyName != null) {
+ logger.debug("Static NAT rule not found with name '{}',
falling back to legacy name '{}'", natRuleName, legacyName);
+ existingNatRule = netrisNatRuleExists(legacyName);
+ if (existingNatRule != null) {
+ natRuleName = legacyName;
+ }
Review Comment:
The unconditional legacy fallback can delete a different static-NAT rule.
When the requested IP-suffixed rule is absent but the VM still has a legacy
rule for another public IP, this lookup selects and deletes that legacy rule.
Verify that the legacy rule's NAT/public IP equals `cmd.getNatIp()` before
falling back.
##########
server/src/main/java/com/cloud/network/IpAddressManagerImpl.java:
##########
@@ -1425,15 +1450,19 @@ public IpAddress allocateIp(final Account ipOwner,
final boolean isSystem, Accou
final VlanType vlanType = VlanType.VirtualNetwork;
final boolean assign = false;
- checkPublicIpOnExternalProviderZone(zone, ipaddress);
-
if (Grouping.AllocationState.Disabled == zone.getAllocationState() &&
!_accountMgr.isRootAdmin(caller.getId())) {
// zone is of type DataCenter. See DataCenterVO.java.
PermissionDeniedException ex = new
PermissionDeniedException(generateErrorMessageForOperationOnDisabledZone("allocate
IP addresses", zone));
ex.addProxyObject(zone.getUuid(), "zoneId");
throw ex;
}
+ checkPublicIpOnExternalProviderZone(zone, ipaddress);
+
+ // Only steer the range when no explicit IP was requested: an explicit
ipaddress is already
+ // validated against the provider's pool above by
checkPublicIpOnExternalProviderZone.
+ final List<Long> vlanDbIds = ipaddress == null ?
getNetrisVlanDbIds(zone) : null;
Review Comment:
This restriction is based only on whether Netris exists in the zone, not on
the network/provider for which the IP is being acquired.
`NetworkService.allocateIP` accepts any `networkId` but calls this method
without passing that context, so an unspecified-IP request for a normal
VR-backed network in a mixed zone is also forced into the Netris VLANs (or
fails if none are available). Apply Netris steering only when the target
network/VPC actually uses Netris, while retaining the generic pool for other
offerings.
##########
plugins/network-elements/netris/src/main/java/org/apache/cloudstack/service/NetrisApiClientImpl.java:
##########
@@ -1542,6 +1703,18 @@ public boolean
createStaticNatRule(CreateOrUpdateNetrisNatCommand cmd) {
logger.error("Could not find the Netris VPC resource with name
{} and tenant ID {}", netrisVpcName, tenantId);
return false;
}
+
+ NatGetBody existingRule = netrisNatRuleExists(staticNatRuleName);
+ if (existingRule != null) {
+ logger.debug("Static NAT rule '{}' already exists on Netris,
skipping creation", staticNatRuleName);
+ return true;
+ }
+ // Backward compatibility: rule with legacy naming convention (no
public IP post-fixed) exists - don't create a duplicate
+ String legacyName = getLegacyStaticNatRuleName(staticNatRuleName);
+ if (legacyName != null && netrisNatRuleExists(legacyName) != null)
{
+ logger.debug("Legacy static NAT rule '{}' already exists on
Netris, skipping creation of '{}'", legacyName, staticNatRuleName);
+ return true;
Review Comment:
Treating any legacy rule for the VM as the same rule breaks the new
multiple-static-NAT support. If an upgraded VM already has a legacy rule for
public IP A, creating static NAT for secondary public IP B finds the shared
legacy name and returns success without creating B. Only accept the legacy rule
when its NAT/public IP matches this command; otherwise create the new
IP-suffixed rule.
##########
ui/src/components/view/DetailsTab.vue:
##########
@@ -381,7 +381,17 @@ export default {
}
return null
},
+ isNetrisNetwork () {
+ if (!this.resource.service) {
+ return false
+ }
+ const networkAclService = this.resource.service.find(svc => svc.name ===
'NetworkACL')
+ return networkAclService && networkAclService.provider &&
networkAclService.provider.some(p => p.name === 'Netris')
Review Comment:
This only recognizes VPC Netris networks because it looks specifically at
NetworkACL. A non-VPC Netris routed network exposes Netris through
Firewall/other services, so its upstream-route warning is still shown even
though this change intends to hide it for Netris routed networks. Detect Netris
in any service provider instead.
##########
ui/src/views/offering/AddNetworkOffering.vue:
##########
@@ -60,7 +60,7 @@
<a-radio-button value="isolated">
{{ $t('label.isolated') }}
</a-radio-button>
- <a-radio-button value="l2" v-if="form.provider !== 'NSX' &&
form.provider !== 'Netris'">
+ <a-radio-button value="l2" v-if="form.provider !== 'NSX'">
Review Comment:
The newly exposed Netris L2 option cannot produce a valid offering. This
form omits `networkmode` for L2, while
`CreateNetworkOfferingCmd.getSupportedServices()` treats every external
offering as routed/isolated and generates Dhcp, Dns, UserData, and Firewall
rather than a single Netris Connectivity service;
`NetworkOrchestrator.checkL2OfferingServices()` then rejects that service set.
Add an L2-specific service/provider path across the form and command before
exposing this choice.
##########
ui/src/views/offering/AddNetworkOffering.vue:
##########
@@ -958,88 +967,93 @@ export default {
}
return svc
})
- self.supportedSvcs = self.supportedServices
+
+ const externalSelectedProviders = {}
+ supportedServices.forEach(svc => {
+ const providerName = svc.provider?.[0]?.name
+ if (providerName) {
+ externalSelectedProviders[svc.name] = providerName
+ }
+ })
+ this.selectedServiceProviderMap = externalSelectedProviders
+ this.sourceNatServiceChecked = 'SourceNat' in externalSelectedProviders
+ this.lbServiceChecked = 'Lb' in externalSelectedProviders
+ this.lbServiceProvider = this.lbServiceChecked ?
externalSelectedProviders.Lb : ''
+ if (this.lbServiceProvider === 'Netris') {
+ this.form.vmautoscalingcapability = true
+ }
+ this.staticNatServiceChecked = 'StaticNat' in externalSelectedProviders
+ this.staticNatServiceProvider = this.staticNatServiceChecked ?
externalSelectedProviders.StaticNat : ''
+ this.connectivityServiceChecked = 'Connectivity' in
externalSelectedProviders
+ this.firewallServiceChecked = 'Firewall' in externalSelectedProviders
+ this.firewallServiceProvider = this.firewallServiceChecked ?
externalSelectedProviders.Firewall : ''
+ const selectedProviders = Object.values(externalSelectedProviders)
+ this.isVirtualRouterForAtLeastOneService =
selectedProviders.includes('VirtualRouter')
+ this.isVpcVirtualRouterForAtLeastOneService =
selectedProviders.includes('VpcVirtualRouter')
+ if ((this.isVirtualRouterForAtLeastOneService ||
this.isVpcVirtualRouterForAtLeastOneService) &&
+ this.serviceOfferings.length === 0) {
+ this.fetchServiceOfferingData()
+ }
+
self.supportedServices = supportedServices
self.supportedServiceLoading = false
}
},
+ refreshExternalSupportedServicesMap () {
+ const selectedProvider = this.form.provider || this.provider
+ if (selectedProvider !== 'NSX' && selectedProvider !== 'Netris') {
+ return
+ }
+
+ const selectedNetworkMode = this.form.networkmode || this.networkmode
+ const effectiveNetworkMode = selectedProvider === 'Netris' &&
!selectedNetworkMode ? 'NATTED' : selectedNetworkMode
+ const externalProvider = selectedProvider === 'NSX' ? this.NSX :
this.Netris
+ const isNsxProvider = selectedProvider === 'NSX'
+ const commonServices = {
+ Dhcp: this.forVpc ? this.VPCVR : this.VR,
+ Dns: this.forVpc ? this.VPCVR : this.VR,
+ UserData: this.forVpc ? this.VPCVR : this.VR,
+ ...(this.forVpc && { NetworkACL: externalProvider }),
+ ...(!this.forVpc && { Firewall: externalProvider })
+ }
+
+ const nattedServices = {
+ SourceNat: externalProvider,
+ StaticNat: externalProvider,
+ PortForwarding: externalProvider,
+ Vpn: this.forVpc ? this.VPCVR : this.VR,
Review Comment:
This adds VPN to the shared NATTED service map for NSX as well as Netris.
The NSX UI will therefore display/select VPN even though
`CreateNetworkOfferingCmd.getSupportedServices()` only enables it for Netris,
so the created offering silently lacks the service shown in the form. Restrict
this entry to Netris.
##########
plugins/network-elements/netris/src/main/java/org/apache/cloudstack/service/NetrisApiClientImpl.java:
##########
@@ -329,6 +331,11 @@ private InlineResponse2004Data
createIpamAllocationInternal(String ipamName, Str
@Override
public boolean createVpc(CreateNetrisVpcCommand cmd) {
String netrisVpcName =
NetrisResourceObjectUtils.retrieveNetrisResourceObjectName(cmd,
NetrisResourceObjectUtils.NetrisObjectType.VPC);
+ VPCListing existingNetrisVpc = getVpcByNameAndTenant(netrisVpcName);
+ if (existingNetrisVpc != null) {
+ logger.info("Netris VPC {} already exists, skipping creation",
netrisVpcName);
+ return true;
Review Comment:
An existing VPC does not prove that its required IPAM allocation exists. If
allocation creation failed and rollback also failed, the next retry takes this
early return, reports success, and later subnet/vNet creation fails against the
incomplete VPC indefinitely. On the idempotent path, look up or create the
expected CIDR allocation before returning success.
##########
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java:
##########
@@ -3173,9 +3182,10 @@ private boolean hasGuestBypassVlanOverlapCheck(final
boolean bypassVlanOverlapCh
}
/**
- * Checks for L2 network offering services. Only 2 cases allowed:
+ * Checks for L2 network offering services. Only 3 cases allowed:
* - No services
- * - User Data service only, provided by ConfigDrive
+ * - UserData service only, provided by ConfigDrive
+ * - Connectivity service only, provided by Netris
Review Comment:
The validator immediately below still only accepts no services or
UserData-only: a sole Connectivity service enters this condition because
UserData is absent and throws "only UserData service is allowed." Consequently
the documented Netris Connectivity case remains unusable; update the validation
(including provider verification) to actually allow exactly Connectivity/Netris.
##########
plugins/network-elements/netris/src/main/java/org/apache/cloudstack/service/NetrisApiClientImpl.java:
##########
@@ -1179,31 +1279,44 @@ public boolean createVnet(CreateNetrisVnetCommand cmd) {
} else {
vNetName = String.format("N%s-%s", networkId, networkName);
}
- String netrisVnetName =
NetrisResourceObjectUtils.retrieveNetrisResourceObjectName(cmd,
NetrisResourceObjectUtils.NetrisObjectType.VNET, vNetName) ;
- String netrisSubnetName =
NetrisResourceObjectUtils.retrieveNetrisResourceObjectName(cmd,
NetrisResourceObjectUtils.NetrisObjectType.IPAM_SUBNET,
String.valueOf(cmd.getVpcId()), vnetCidr) ;
-
- createIpamSubnetInternal(netrisSubnetName, vnetCidr,
SubnetBody.PurposeEnum.COMMON, associatedVpc, isGlobalRouting);
- if (Objects.nonNull(netrisV6Cidr)) {
- String netrisV6IpamAllocationName =
NetrisResourceObjectUtils.retrieveNetrisResourceObjectName(cmd,
NetrisResourceObjectUtils.NetrisObjectType.IPAM_ALLOCATION, netrisV6Cidr);
- String netrisV6SubnetName =
NetrisResourceObjectUtils.retrieveNetrisResourceObjectName(cmd,
NetrisResourceObjectUtils.NetrisObjectType.IPAM_SUBNET,
String.valueOf(cmd.getVpcId()), netrisV6Cidr) ;
- BigDecimal ipamAllocationId =
getIpamAllocationIdByPrefixAndVpc(netrisV6Cidr, associatedVpc);
- if (ipamAllocationId == null) {
- InlineResponse2004Data createdIpamAllocation =
createIpamAllocationInternal(netrisV6IpamAllocationName, netrisV6Cidr,
associatedVpc);
- if (Objects.isNull(createdIpamAllocation)) {
- throw new CloudRuntimeException(String.format("Failed
to create Netris IPAM Allocation %s for VPC %s", netrisV6IpamAllocationName,
netrisVpcName));
+ String netrisVnetName =
NetrisResourceObjectUtils.retrieveNetrisResourceObjectName(cmd,
NetrisResourceObjectUtils.NetrisObjectType.VNET, vNetName);
+
NetrisResourceObjectUtils.validateNetrisVnetNameLength(netrisVnetName,
networkName);
+
+ if (!isL2) {
+ netrisSubnetName =
NetrisResourceObjectUtils.retrieveNetrisResourceObjectName(cmd,
NetrisResourceObjectUtils.NetrisObjectType.IPAM_SUBNET,
String.valueOf(cmd.getVpcId()), vnetCidr);
+ createIpamSubnetInternal(netrisSubnetName, vnetCidr,
SubnetBody.PurposeEnum.COMMON, associatedVpc, ipv4GlobalRouting);
+ if (Objects.nonNull(netrisV6Cidr)) {
+ netrisV6IpamAllocationName =
NetrisResourceObjectUtils.retrieveNetrisResourceObjectName(cmd,
NetrisResourceObjectUtils.NetrisObjectType.IPAM_ALLOCATION, netrisV6Cidr);
+ netrisV6SubnetName =
NetrisResourceObjectUtils.retrieveNetrisResourceObjectName(cmd,
NetrisResourceObjectUtils.NetrisObjectType.IPAM_SUBNET,
String.valueOf(cmd.getVpcId()), netrisV6Cidr);
+ BigDecimal ipamAllocationId =
getIpamAllocationIdByPrefixAndVpc(netrisV6Cidr, associatedVpc);
+ if (ipamAllocationId == null) {
+ InlineResponse2004Data createdIpamAllocationData =
createIpamAllocationInternal(netrisV6IpamAllocationName, netrisV6Cidr,
associatedVpc);
+ if (Objects.isNull(createdIpamAllocationData)) {
+ rollbackVnetResources(associatedVpc,
netrisSubnetName, null, null, networkName);
+ throw new
CloudRuntimeException(String.format("Failed to create Netris IPAM Allocation %s
for VPC %s", netrisV6IpamAllocationName, associatedVpc.getName()));
+ }
+ createdIpv6Allocation = true;
}
+ createIpamSubnetInternal(netrisV6SubnetName, netrisV6Cidr,
SubnetBody.PurposeEnum.COMMON, associatedVpc, ipv6GlobalRouting);
}
- createIpamSubnetInternal(netrisV6SubnetName, netrisV6Cidr,
SubnetBody.PurposeEnum.COMMON, associatedVpc, isGlobalRouting);
+ logger.debug("Successfully created IPAM Subnet for network {}
on Netris", networkName);
}
- logger.debug("Successfully created IPAM Subnet {} for network {}
on Netris", netrisSubnetName, networkName);
- VnetResAddBody vnetResponse = createVnetInternal(associatedVpc,
netrisVnetName, netrisGateway, netrisV6Cidr, vxlanId, netrisTag);
+ VnetResAddBody vnetResponse = createVnetInternal(associatedVpc,
netrisVnetName, netrisGateway, isL2 ? null : netrisV6Cidr, vxlanId, netrisTag);
if (vnetResponse == null || !vnetResponse.isIsSuccess()) {
String reason = vnetResponse == null ? "Empty response" :
"Operation failed on Netris";
logger.debug("The Netris vNet creation {} failed: {}",
vNetName, reason);
+ if (!isL2) {
+ rollbackVnetResources(associatedVpc, netrisSubnetName,
netrisV6SubnetName,
+ createdIpv6Allocation ? netrisV6IpamAllocationName
: null, networkName);
+ }
Review Comment:
For L2, `getOrCreateL2Vpc()` may have just created a dedicated VPC, but both
this failure branch and the catch block explicitly skip rollback when `isL2` is
true. A rejected vNet creation (including name-length validation or API
failure) therefore leaves an orphan Netris VPC. Track whether the L2 VPC was
created by this attempt and delete it on failure, without deleting a
pre-existing retry target.
--
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]
