Address review comments

nvazquez · nvazquez · commit 801510041237 · 2026-02-26T16:09:15.000-03:00
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -146,6 +146,7 @@ jobs:
                   smoke/test_vm_snapshot_kvm
                   smoke/test_vm_snapshots
                   smoke/test_volumes
+                  smoke/test_vpc_conserve_mode
                   smoke/test_vpc_ipv6
                   smoke/test_vpc_redundant
                   smoke/test_vpc_router_nics
diff --git a/api/src/main/java/com/cloud/network/NetworkService.java b/api/src/main/java/com/cloud/network/NetworkService.java
@@ -275,4 +275,6 @@ Network createPrivateNetwork(String networkName, String displayText, long physic
     IpAddresses getIpAddressesFromIps(String ipAddress, String ip6Address, String macAddress);
 
     String getNicVlanValueForExternalVm(NicTO nic);
+
+    Long getPreferredNetworkIdForPublicIpRuleAssignment(IpAddress ip, Long networkId);
 }
diff --git a/api/src/main/java/org/apache/cloudstack/api/command/admin/vpc/CreateVPCOfferingCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/admin/vpc/CreateVPCOfferingCmd.java
@@ -163,7 +163,7 @@ public class CreateVPCOfferingCmd extends BaseAsyncCreateCmd {
 
     @Parameter(name = ApiConstants.CONSERVE_MODE, type = CommandType.BOOLEAN,
             since = "4.23.0",
-            description = "True if the VPC offering is IP conserve mode enabled, allowing public IP services to be used across multiple VPC tiers")
+            description = "True if the VPC offering is IP conserve mode enabled, allowing public IPs to be used across multiple VPC tiers. Default value is false")
     private Boolean conserveMode;
 
 
diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/firewall/CreatePortForwardingRuleCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/firewall/CreatePortForwardingRuleCmd.java
@@ -54,7 +54,6 @@
         requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
 public class CreatePortForwardingRuleCmd extends BaseAsyncCreateCmd implements PortForwardingRule {
 
-
     // ///////////////////////////////////////////////////
     // ////////////// API parameters /////////////////////
     // ///////////////////////////////////////////////////
@@ -278,13 +277,7 @@ public State getState() {
     @Override
     public long getNetworkId() {
         IpAddress ip = _entityMgr.findById(IpAddress.class, getIpAddressId());
-        Long ntwkId = null;
-
-        if (ip.getVpcId() == null && ip.getAssociatedWithNetworkId() != null) {
-            ntwkId = ip.getAssociatedWithNetworkId();
-        } else {
-            ntwkId = networkId;
-        }
+        Long ntwkId = _networkService.getPreferredNetworkIdForPublicIpRuleAssignment(ip, networkId);
         if (ntwkId == null) {
             throw new InvalidParameterValueException("Unable to create port forwarding rule for the ipAddress id=" + ipAddressId +
                     " as ip is not associated with any network and no networkId is passed in");
diff --git a/api/src/main/java/org/apache/cloudstack/api/response/VpcOfferingResponse.java b/api/src/main/java/org/apache/cloudstack/api/response/VpcOfferingResponse.java
@@ -103,7 +103,7 @@ public class VpcOfferingResponse extends BaseResponse {
     private String routingMode;
 
     @SerializedName(ApiConstants.CONSERVE_MODE)
-    @Param(description = "True if the VPC offering is IP conserve mode enabled, allowing public IP services to be used across multiple VPC tiers.")
+    @Param(description = "True if the VPC offering is IP conserve mode enabled, allowing public IP services to be used across multiple VPC tiers.", since = "4.23.0")
     private Boolean conserveMode;
 
     public void setId(String id) {
diff --git a/engine/components-api/src/main/java/com/cloud/network/IpAddressManager.java b/engine/components-api/src/main/java/com/cloud/network/IpAddressManager.java
@@ -288,4 +288,6 @@ List<IPAddressVO> listAvailablePublicIps(final long dcId,
     PublicIpQuarantine updatePublicIpAddressInQuarantine(Long quarantineProcessId, Date endDate);
 
     void updateSourceNatIpAddress(IPAddressVO requestedIp, List<IPAddressVO> userIps) throws Exception;
+
+    Long getPreferredNetworkIdForPublicIpRuleAssignment(IpAddress ip, Long networkId);
 }
diff --git a/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java b/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java
@@ -2659,4 +2659,31 @@ public void updateSourceNatIpAddress(IPAddressVO requestedIp, List<IPAddressVO>
         });
     }
 
+    @Override
+    public Long getPreferredNetworkIdForPublicIpRuleAssignment(IpAddress ip, Long networkId) {
+        boolean vpcConserveMode = isPublicIpOnVpcConserveMode(ip);
+        return getPreferredNetworkIdForRule(ip, vpcConserveMode, networkId);
+    }
+
+    protected Long getPreferredNetworkIdForRule(IpAddress ip, boolean vpcConserveModeEnabled, Long networkId) {
+        if (vpcConserveModeEnabled) {
+            // Since VPC Conserve mode allows rules from multiple VPC tiers, always check the networkId parameter first
+            return networkId != null ? networkId : ip.getAssociatedWithNetworkId();
+        } else {
+            // In case of Guest Networks or VPC Tier Networks VPC Conserve mode disabled prefer the associated networkId
+            return ip.getAssociatedWithNetworkId() != null ? ip.getAssociatedWithNetworkId() : networkId;
+        }
+    }
+
+    protected boolean isPublicIpOnVpcConserveMode(IpAddress ip) {
+        if (ip.getVpcId() == null) {
+            return false;
+        }
+        Vpc vpc =  _vpcMgr.getActiveVpc(ip.getVpcId());
+        if (vpc == null) {
+            return false;
+        }
+        VpcOffering vpcOffering = vpcOfferingDao.findById(vpc.getVpcOfferingId());
+        return vpcOffering != null && vpcOffering.isConserveMode();
+    }
 }
diff --git a/server/src/main/java/com/cloud/network/NetworkServiceImpl.java b/server/src/main/java/com/cloud/network/NetworkServiceImpl.java
@@ -6415,6 +6415,11 @@ public String getNicVlanValueForExternalVm(NicTO nic) {
         return Networks.BroadcastDomainType.getValue(nic.getBroadcastUri());
     }
 
+    @Override
+    public Long getPreferredNetworkIdForPublicIpRuleAssignment(IpAddress ip, Long networkId) {
+        return _ipAddrMgr.getPreferredNetworkIdForPublicIpRuleAssignment(ip, networkId);
+    }
+
     @Override
     public Network.IpAddresses getIpAddressesFromIps(String ipAddress, String ip6Address, String macAddress) {
         if (ip6Address != null) {
diff --git a/server/src/main/java/com/cloud/network/firewall/FirewallManagerImpl.java b/server/src/main/java/com/cloud/network/firewall/FirewallManagerImpl.java
@@ -457,7 +457,8 @@ public void detectRulesConflict(FirewallRule newRule) throws NetworkRuleConflict
                     && rule.getNetworkId() != newRule.getNetworkId() && rule.getState() != State.Revoke) {
                 String errMsg = String.format("New rule is for a different network than what's specified in rule %s", rule.getXid());
                 if (newRuleIsOnVpcNetwork) {
-                    errMsg += String.format(" - VPC id=%s is not using conserve mode", newRuleNetwork.getVpcId());
+                    Vpc vpc = _vpcMgr.getActiveVpc(newRuleNetwork.getVpcId());
+                    errMsg += String.format(" - VPC id=%s is not using conserve mode", vpc.getUuid());
                 }
                 throw new NetworkRuleConflictException(errMsg);
             }
diff --git a/server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java b/server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java
@@ -1853,7 +1853,7 @@ public LoadBalancer createPublicLoadBalancer(final String xId, final String name
 
             _accountMgr.checkAccess(caller.getCallingAccount(), null, true, ipAddr);
 
-            final Long networkId = ipAddr.getVpcId() == null ? ipAddr.getAssociatedWithNetworkId() : networkIdParam;
+            final Long networkId = _ipAddrMgr.getPreferredNetworkIdForPublicIpRuleAssignment(ipAddr, networkIdParam);
             if (networkId == null) {
                 InvalidParameterValueException ex =
                         new InvalidParameterValueException("Unable to create load balancer rule ; specified sourceip id is not associated with any network");
diff --git a/server/src/test/java/com/cloud/vpc/MockNetworkManagerImpl.java b/server/src/test/java/com/cloud/vpc/MockNetworkManagerImpl.java
@@ -1141,4 +1141,9 @@ public void expungeLbVmRefs(List<Long> vmIds, Long batchSize) {
     public String getNicVlanValueForExternalVm(NicTO nic) {
         return null;
     }
+
+    @Override
+    public Long getPreferredNetworkIdForPublicIpRuleAssignment(IpAddress ip, Long networkId) {
+        return null;
+    }
 }
diff --git a/test/integration/smoke/test_domain_vpc_offerings.py b/test/integration/smoke/test_domain_vpc_offerings.py
@@ -28,9 +28,16 @@
 from marvin.lib.base import (Domain,
                              VpcOffering,
                              Account,
-                             VPC)
+                             VPC,
+                             NetworkOffering,
+                             Network,
+                             VirtualMachine,
+                             ServiceOffering,
+                             PublicIPAddress,
+                             NATRule)
 from marvin.lib.common import (get_domain,
-                               get_zone)
+                               get_zone,
+                               get_test_template)
 from nose.plugins.attrib import attr
 
 import time
@@ -222,6 +229,7 @@ def setUpClass(cls):
         cls.apiclient = testClient.getApiClient()
         cls.localservices = Services().services
         cls.services = testClient.getParsedTestDataConfig()
+        cls.hypervisor = cls.testClient.getHypervisorInfo()
         # Create domains
         cls.domain_1 = Domain.create(
             cls.apiclient,
@@ -402,3 +410,161 @@ def test_03_create_vpc_domain_vpc_offering(self):
         self.debug("Vpc created for first child subdomain %s" % self.valid_account_3.domainid)
 
         return
+
+    @attr(
+        tags=[
+            "advanced",
+            "eip",
+            "sg",
+            "advancedns",
+            "smoke"],
+        required_hardware="true")
+    def test_04_validate_vpc_offering_conserve_mode_disabled(self):
+        """Test to create and validate vpc with conserve mode disabled for an existing domain specified vpc offering"""
+
+        # Validate the following:
+        # 1. Create Vpc for user in domain for which offering is specified
+        # 2. Validate that conserve mode is disabled for the vpc (cannot reuse ip address on multiple VPC tiers)
+
+        template = get_test_template(
+            self.apiclient,
+            self.zone.id,
+            self.hypervisor)
+        if template == FAILED:
+            assert False, "get_test_template() failed to return template"
+
+        valid_account_1 = Account.create(
+            self.apiclient,
+            self.services["account"],
+            domainid=self.domain_1.id
+        )
+        self.cleanup.append(valid_account_1)
+
+        service_offering = ServiceOffering.create(
+            self.apiclient,
+            self.services["service_offerings"]["tiny"]
+        )
+        self.cleanup.append(service_offering)
+
+        vpc = VPC.create(
+            apiclient=self.apiclient,
+            services=self.services["vpc"],
+            account=valid_account_1.name,
+            domainid=valid_account_1.domainid,
+            zoneid=self.zone.id,
+            vpcofferingid=self.vpc_offering.id
+        )
+        self.debug("Vpc created for subdomain %s" % valid_account_1.domainid)
+
+        self.services["network_offering"]["supportedservices"] = 'Vpn,Dhcp,Dns,SourceNat,Lb,UserData,StaticNat,NetworkACL,PortForwarding'
+        self.services["network_offering"]["serviceProviderList"] = {
+            "Vpn": 'VpcVirtualRouter',
+            "Dhcp": 'VpcVirtualRouter',
+            "Dns": 'VpcVirtualRouter',
+            "SourceNat": 'VpcVirtualRouter',
+            "Lb": 'VpcVirtualRouter',
+            "UserData": 'VpcVirtualRouter',
+            "StaticNat": 'VpcVirtualRouter',
+            "NetworkACL": 'VpcVirtualRouter',
+            "PortForwarding": 'VpcVirtualRouter'
+        }
+        network_offering = NetworkOffering.create(
+            self.apiclient,
+            self.services["network_offering"]
+        )
+        network_offering.update(self.apiclient, state="Enabled")
+        self.cleanup.append(network_offering)
+
+        gateway_tier1 = "10.0.20.1"
+        netmask_tiers = "255.255.255.0"
+
+        self.services["network_offering"]["name"] = "tier1-" + vpc.id
+        self.services["network_offering"]["displayname"] = "tier1-" + vpc.id
+        tier1 = Network.create(
+            self.apiclient,
+            services=self.services["network_offering"],
+            accountid=valid_account_1.name,
+            domainid=valid_account_1.domainid,
+            networkofferingid=network_offering.id,
+            zoneid=self.zone.id,
+            vpcid=vpc.id,
+            gateway=gateway_tier1,
+            netmask=netmask_tiers,
+        )
+        self.cleanup.append(tier1)
+
+        gateway_tier2 = "10.0.30.1"
+        self.services["network_offering"]["name"] = "tier2-" + vpc.id
+        self.services["network_offering"]["displayname"] = "tier2-" + vpc.id
+        tier2 = Network.create(
+            self.apiclient,
+            services=self.services["network_offering"],
+            accountid=valid_account_1.name,
+            domainid=valid_account_1.domainid,
+            networkofferingid=network_offering.id,
+            zoneid=self.zone.id,
+            vpcid=vpc.id,
+            gateway=gateway_tier2,
+            netmask=netmask_tiers,
+        )
+        self.cleanup.append(tier2)
+
+        self.services["virtual_machine"]["displayname"] = "vm1" + vpc.id
+        vm1 = VirtualMachine.create(
+            self.apiclient,
+            services=self.services["virtual_machine"],
+            templateid=template.id,
+            zoneid=self.zone.id,
+            accountid=valid_account_1.name,
+            domainid=valid_account_1.domainid,
+            serviceofferingid=service_offering.id,
+            networkids=[tier1.id],
+        )
+
+        self.services["virtual_machine"]["displayname"] = "vm2" + vpc.id
+        vm2 = VirtualMachine.create(
+            self.apiclient,
+            services=self.services["virtual_machine"],
+            templateid=template.id,
+            zoneid=self.zone.id,
+            accountid=valid_account_1.name,
+            domainid=valid_account_1.domainid,
+            serviceofferingid=service_offering.id,
+            networkids=[tier2.id],
+        )
+        self.cleanup.append(vm1)
+        self.cleanup.append(vm2)
+
+        public_ip = PublicIPAddress.create(
+            self.apiclient,
+            zoneid=self.zone.id,
+            accountid=valid_account_1.name,
+            domainid=valid_account_1.domainid,
+            vpcid=vpc.id,
+        )
+
+        nat_rule = NATRule.create(
+            self.apiclient,
+            vm1,
+            self.services["natrule"],
+            ipaddressid=public_ip.ipaddress.id,
+            vpcid=vpc.id,
+            networkid=tier1.id,
+        )
+
+        self.services["natrule"]["privateport"] = 80
+        self.services["natrule"]["publicport"] = 80
+        try:
+            NATRule.create(
+                self.apiclient,
+                vm2,
+                self.services["natrule"],
+                ipaddressid=public_ip.ipaddress.id,
+                vpcid=vpc.id,
+                networkid=tier2.id,
+            )
+            self.fail(
+                "Expected cross-tier rule creation to fail with conserveMode=False, but succeeded"
+            )
+        except CloudstackAPIException as e:
+            self.debug("Expected cross-tier rule creation to failure with conserveMode=False")
diff --git a/test/integration/smoke/test_vpc_conserve_mode.py b/test/integration/smoke/test_vpc_conserve_mode.py
@@ -272,7 +272,7 @@ def test_02_vpc_conserve_mode_reuse_source_nat_ip_address(self):
         source_nat_ip = source_nat_ip_resp[0]
 
         self.logger.debug(
-            "Creating Port Forwarding rule on tier-1 (networkid=%s) "
+            "Creating Port Forwarding rule on tier-2 (networkid=%s) "
             "using the source NAT public IP %s – should succeed with conserve mode",
             self.tier1.id,
             source_nat_ip.ipaddress,
@@ -290,8 +290,25 @@ def test_02_vpc_conserve_mode_reuse_source_nat_ip_address(self):
                 nat_rule,
                 "Port Forwarding rule creation on tier-2 failed unexpectedly",
             )
+            self.logger.debug(
+                "Creating LB rule on tier-1 (networkid=%s) "
+                "using the source NAT public IP %s – should succeed with conserve mode",
+                self.tier1.id,
+                source_nat_ip.ipaddress,
+            )
+            lb_rule_tier1 = LoadBalancerRule.create(
+                self.apiclient,
+                self.services["lbrule"],
+                ipaddressid=source_nat_ip.id,
+                accountid=self.account.name,
+                vpcid=self.vpc.id,
+                networkid=self.tier2.id,
+                domainid=self.account.domainid,
+            )
+            self.assertIsNotNone(lb_rule_tier1, "LB rule creation on tier-2 failed")
+            lb_rule_tier1.assign(self.apiclient, [self.vm2])
         except CloudstackAPIException as e:
             self.fail(
-                "Expected cross-tier Port Forwarding rule to succeed with "
+                "Expected multiple rules on VPC Source NAT IP to succeed with "
                 "conserveMode=True, but got exception: %s" % e
             )

Original file line number	Diff line number	Diff line change
`@@ -275,4 +275,6 @@ Network createPrivateNetwork(String networkName, String displayText, long physic`
`275`	`275`	`IpAddresses getIpAddressesFromIps(String ipAddress, String ip6Address, String macAddress);`
`276`	`276`
`277`	`277`	`String getNicVlanValueForExternalVm(NicTO nic);`
	`278`	`+`
	`279`	`+ Long getPreferredNetworkIdForPublicIpRuleAssignment(IpAddress ip, Long networkId);`
`278`	`280`	`}`
Original file line number	Diff line number	Diff line change
`@@ -288,4 +288,6 @@ List<IPAddressVO> listAvailablePublicIps(final long dcId,`
`288`	`288`	`PublicIpQuarantine updatePublicIpAddressInQuarantine(Long quarantineProcessId, Date endDate);`
`289`	`289`
`290`	`290`	`void updateSourceNatIpAddress(IPAddressVO requestedIp, List<IPAddressVO> userIps) throws Exception;`
	`291`	`+`
	`292`	`+ Long getPreferredNetworkIdForPublicIpRuleAssignment(IpAddress ip, Long networkId);`
`291`	`293`	`}`
Original file line number	Diff line number	Diff line change
`@@ -457,7 +457,8 @@ public void detectRulesConflict(FirewallRule newRule) throws NetworkRuleConflict`
`457`	`457`	`&& rule.getNetworkId() != newRule.getNetworkId() && rule.getState() != State.Revoke) {`
`458`	`458`	`String errMsg = String.format("New rule is for a different network than what's specified in rule %s", rule.getXid());`
`459`	`459`	`if (newRuleIsOnVpcNetwork) {`
`460`		`- errMsg += String.format(" - VPC id=%s is not using conserve mode", newRuleNetwork.getVpcId());`
	`460`	`+ Vpc vpc = _vpcMgr.getActiveVpc(newRuleNetwork.getVpcId());`
	`461`	`+ errMsg += String.format(" - VPC id=%s is not using conserve mode", vpc.getUuid());`
`461`	`462`	`}`
`462`	`463`	`throw new NetworkRuleConflictException(errMsg);`
`463`	`464`	`}`