Conversation
boring-cyborg
bot
added
component:api
component:networking
component:UI
component:vpc
labels
|
@blueorangutan package |
|
@nvazquez a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12487 +/- ##
==========================================
Coverage 17.84% 17.85%
- Complexity 15980 15997 +17
==========================================
Files 5929 5930 +1
Lines 531084 531511 +427
Branches 64914 64989 +75
==========================================
+ Hits 94783 94882 +99
- Misses 425686 425999 +313
- Partials 10615 10630 +15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 16471 |
|
@blueorangutan package |
|
@nvazquez a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✔️ debian ✖️ suse15. SL-JID 16473 |
|
@blueorangutan package |
|
@nvazquez a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✖️ debian ✔️ suse15. SL-JID 16475 |
|
@blueorangutan test keepEnv |
|
@nvazquez a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-15239)
|
nvazquez
requested review from
DaanHoogland,
Pearl1594,
shwstppr,
sureshanaparti and
weizhouapache
nvazquez
changed the title
|
@blueorangutan package |
|
@nvazquez a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✖️ debian ✔️ suse15. SL-JID 16549 |
|
@blueorangutan package |
|
@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
weizhouapache
left a comment
weizhouapache
left a comment
There was a problem hiding this comment.
- can we create VPC offering with conserve mode via API and UI ?
- can you add smoke tests ?
- source nat can be used by vpc tiers
- other public Ips too
engine/schema/src/main/resources/META-INF/db/schema-42210to42300.sql
Outdated
Show resolved
Hide resolved
engine/schema/src/main/resources/META-INF/db/schema-42210to42300.sql
Outdated
Show resolved
Hide resolved
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16553 |
nvazquez
changed the title
| // (except for VPCs with conserve mode = true) | ||
| if ((!isNewRuleOnVpcNetwork || !isVpcConserveModeEnabled) | ||
| && 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 (isNewRuleOnVpcNetwork) { | ||
| errMsg += String.format(" - VPC id=%s is not using conserve mode", newRuleNetwork.getVpcId()); | ||
| } | ||
| throw new NetworkRuleConflictException(errMsg); | ||
| } |
| NetworkVO newRuleNetwork = _networkDao.findById(newRule.getNetworkId()); | ||
| if (newRuleNetwork == null) { | ||
| throw new InvalidParameterValueException("Unable to create firewall rule as cannot find network by id=" + newRule.getNetworkId()); | ||
| } | ||
| boolean isNewRuleOnVpcNetwork = newRuleNetwork.getVpcId() != null; | ||
| boolean isVpcConserveModeEnabled = false; | ||
| if (isNewRuleOnVpcNetwork) { | ||
| VpcOfferingVO vpcOffering = vpcOfferingDao.findById(newRuleNetwork.getVpcId()); | ||
| isVpcConserveModeEnabled = vpcOffering != null && vpcOffering.isConserveMode(); | ||
| } |
RosiKyu
left a comment
RosiKyu
left a comment
There was a problem hiding this comment.
PR needs some fixes:
Issues Identified:
- The
conservemodeparameter is not being passed from API command to the service layer (value not saved to DB) - The
conservemodefield is not being returned in the API response (VpcOfferingResponse)
TC1: Create VPC Offering with Conserve Mode = true
Objective: Verify VPC offering can be created with conserve mode enabled and the field is properly stored and returned via API.
Test Steps:
- Create VPC offering with
conservemode=trueparameter - Verify the offering via
list vpcofferingsAPI - Verify the value is stored correctly in the database
Expected Result:
- VPC offering is created successfully
- API response includes
conservemode: true - Database shows
conserve_mode = 1
Actual Result:
- VPC offering created successfully with ID
b9b783b2-b1fb-4588-b383-ff36cc9e54ab - BUG: API response does NOT include
conservemodefield at all - BUG: Database shows
conserve_mode = 0instead of1despite passingconservemode=true
Status: Failed
Test Evidence:
(localcloud) 🐱 > create vpcoffering name="Test-Conserve-Mode-VPC-Offering" displaytext="VPC Offering with Conserve Mode Enabled" supportedservices="Dhcp,Dns,SourceNat,PortForwarding,Lb,UserData,StaticNat,NetworkACL" conservemode=true
{
"vpcoffering": {
"created": "2026-01-30T09:02:54+0000",
"displaytext": "VPC Offering with Conserve Mode Enabled",
"distributedvpcrouter": false,
"fornsx": false,
"id": "b9b783b2-b1fb-4588-b383-ff36cc9e54ab",
"internetprotocol": "IPv4",
"isdefault": false,
"name": "Test-Conserve-Mode-VPC-Offering",
"service": [
{
"name": "Dhcp",
"provider": [
{
"name": "VpcVirtualRouter"
}
]
},
{
"name": "StaticNat",
"provider": [
{
"name": "VpcVirtualRouter"
}
]
},
{
"name": "UserData",
"provider": [
{
"name": "VpcVirtualRouter"
}
]
},
{
"name": "SourceNat",
"provider": [
{
"name": "VpcVirtualRouter"
}
]
},
{
"name": "Lb",
"provider": [
{
"name": "VpcVirtualRouter"
}
]
},
{
"name": "NetworkACL",
"provider": [
{
"name": "VpcVirtualRouter"
}
]
},
{
"name": "Dns",
"provider": [
{
"name": "VpcVirtualRouter"
}
]
},
{
"name": "PortForwarding",
"provider": [
{
"name": "VpcVirtualRouter"
}
]
}
],
"specifyasnumber": false,
"state": "Disabled",
"supportsregionLevelvpc": false
}
}
(localcloud) 🐱 > list vpcofferings name="Test-Conserve-Mode-VPC-Offering"
{
"count": 1,
"vpcoffering": [
{
"created": "2026-01-30T09:02:54+0000",
"displaytext": "VPC Offering with Conserve Mode Enabled",
"distributedvpcrouter": false,
"fornsx": false,
"id": "b9b783b2-b1fb-4588-b383-ff36cc9e54ab",
"internetprotocol": "IPv4",
"isdefault": false,
"name": "Test-Conserve-Mode-VPC-Offering",
"service": [...],
"specifyasnumber": false,
"state": "Disabled",
"supportsregionLevelvpc": false
}
]
}
mysql> SELECT id, name, conserve_mode FROM cloud.vpc_offerings WHERE name='Test-Conserve-Mode-VPC-Offering';
+----+---------------------------------+---------------+
| id | name | conserve_mode |
+----+---------------------------------+---------------+
| 8 | Test-Conserve-Mode-VPC-Offering | 0 |
+----+---------------------------------+---------------+
1 row in set (0.00 sec)
|
@blueorangutan package |
|
@nvazquez a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16654 |
|
@blueorangutan test |
|
@nvazquez a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-15348)
|
|
@blueorangutan test keepEnv |
|
@nvazquez a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-15355)
|
There was a problem hiding this comment.
Pull request overview
This PR extends “IP conserve mode” behavior into VPC offerings and relaxes certain per-tier restrictions so that, when conserve mode is enabled, a VPC’s Source NAT public IP can be reused for additional services and public-IP rules can be created across multiple VPC tiers.
Changes:
- Add
conservemodeto VPC offerings end-to-end (DB schema/view/entity, API params/responses, UI create form + details). - Adjust backend rule/network selection and conflict checks to allow cross-tier usage in VPC conserve mode (PF/LB/firewall paths).
- Update UI logic for Public IP / PF / LB screens to reflect conserve-mode behavior (tier selection and tab availability).
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/views/offering/AddVpcOffering.vue | Adds a Conserve Mode switch and passes conservemode to createVPCOffering. |
| ui/src/views/network/PublicIpResource.vue | Changes tab filtering for VPC SourceNAT IPs based on VPC offering conserve mode. |
| ui/src/views/network/PortForwarding.vue | Allows tier selection when VPC conserve mode is enabled; uses VPC offering conserve mode flag. |
| ui/src/views/network/LoadBalancing.vue | Allows tier selection when VPC conserve mode is enabled; uses VPC offering conserve mode flag. |
| ui/src/config/section/offering.js | Shows conservemode in VPC offering details. |
| server/src/main/java/com/cloud/network/firewall/FirewallManagerImpl.java | Relaxes cross-network firewall-rule conflict restriction for VPC conserve mode. |
| server/src/test/java/com/cloud/network/firewall/FirewallManagerTest.java | Updates unit test setup for new network lookup in conflict detection. |
| server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java | Threads a networkId parameter to support VPC-tier-aware LB creation. |
| engine/components-api/src/main/java/com/cloud/network/lb/LoadBalancingRulesManager.java | Updates API surface to include networkId parameter. |
| server/src/main/java/com/cloud/network/IpAddressManagerImpl.java | Adjusts Source NAT detection when associating IPs within VPCs. |
| api/src/main/java/org/apache/cloudstack/api/command/admin/vpc/CreateVPCOfferingCmd.java | Adds conservemode parameter for VPC offering creation. |
| engine/schema/src/main/java/com/cloud/network/vpc/VpcOfferingVO.java | Persists conserve_mode on VPC offerings. |
| engine/schema/src/main/resources/META-INF/db/schema-42210to42300.sql | Adds conserve_mode column to vpc_offerings. |
| engine/schema/src/main/resources/META-INF/db/views/cloud.vpc_offering_view.sql | Exposes conserve_mode via VPC offering DB view. |
| server/src/main/java/com/cloud/api/query/vo/VpcOfferingJoinVO.java | Exposes conserve_mode on the VPC offering join view VO. |
| server/src/main/java/com/cloud/api/query/dao/VpcOfferingJoinDaoImpl.java | Populates conserve mode in VpcOfferingResponse. |
| api/src/main/java/org/apache/cloudstack/api/response/VpcOfferingResponse.java | Adds conservemode field to VPC offering response. |
| api/src/main/java/org/apache/cloudstack/api/response/VpcResponse.java | Adds vpcofferingconservemode field to VPC response. |
| server/src/main/java/com/cloud/api/ApiResponseHelper.java | Sets vpcofferingconservemode in VPC response. |
| api/src/main/java/org/apache/cloudstack/api/ApiConstants.java | Adds vpcofferingconservemode constant. |
| api/src/main/java/com/cloud/network/vpc/VpcOffering.java | Adds isConserveMode() to VPC offering interface. |
| api/src/main/java/com/cloud/network/vpc/VpcProvisioningService.java | Extends createVpcOffering signature to include conserve mode. |
| server/src/main/java/com/cloud/network/vpc/VpcManagerImpl.java | Plumbs conserve mode into VPC offering creation/persist. |
| api/src/main/java/org/apache/cloudstack/api/command/user/firewall/CreatePortForwardingRuleCmd.java | Adjusts network-id resolution behavior for VPC IPs. |
| plugins/network-elements/elastic-loadbalancer/.../LoadBalanceRuleHandler.java | Updates LB manager call to include networkId. |
| plugins/network-elements/juniper-contrail/.../ContrailManagerImpl.java | Updates VPC offering creation call signature. |
Comments suppressed due to low confidence (1)
ui/src/views/network/PortForwarding.vue:649
- The networkId selection logic for createPortForwardingRule is inverted due to parentheses:
!('associatednetworkid' in this.resource || this.vpcConserveMode)becomes true only when BOTH there is no associated network AND conserve mode is false. This will incorrectly chooseassociatednetworkidwhen conserve mode is enabled (the opposite of what the UI/feature intends). Use the same condition used elsewhere in the file: no associated network OR vpcConserveMode => selectedTier; otherwise associatednetworkid.
this.loading = true
this.addVmModalVisible = false
const networkId = ('vpcid' in this.resource && (!('associatednetworkid' in this.resource || this.vpcConserveMode))) ? this.selectedTier : this.resource.associatednetworkid
postAPI('createPortForwardingRule', {
...this.newRule,
ipaddressid: this.resource.id,
networkid: networkId
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -443,8 +458,14 @@ public void detectRulesConflict(FirewallRule newRule) throws NetworkRuleConflict | |||
| } | |||
|
|
|||
| // Checking if the rule applied is to the same network that is passed in the rule. | |||
| if (rule.getNetworkId() != newRule.getNetworkId() && rule.getState() != State.Revoke) { | |||
| throw new NetworkRuleConflictException("New rule is for a different network than what's specified in rule " + rule.getXid()); | |||
| // (except for VPCs with conserve mode = true) | |||
| if ((!isNewRuleOnVpcNetwork || !isVpcConserveModeEnabled) | |||
| && 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 (isNewRuleOnVpcNetwork) { | |||
| errMsg += String.format(" - VPC id=%s is not using conserve mode", newRuleNetwork.getVpcId()); | |||
| } | |||
| throw new NetworkRuleConflictException(errMsg); | |||
There was a problem hiding this comment.
The new VPC-conserve-mode branch in detectRulesConflict isn’t covered by unit tests: there are no assertions for (1) conserve mode enabled allowing rules across different VPC tiers and (2) conserve mode disabled still rejecting cross-network rules. Adding focused tests here would prevent regressions in the conflict detection logic.
| <span | ||
| v-if="'vpcid' in resource && !('associatednetworkid' in resource)"> | ||
| v-if="'vpcid' in resource && (!('associatednetworkid' in resource) || this.vpcConserveMode)"> | ||
| <strong>{{ $t('label.select.tier') }} </strong> | ||
| <a-select | ||
| :v-focus="'vpcid' in resource && !('associatednetworkid' in resource)" | ||
| :v-focus="'vpcid' in resource && (!('associatednetworkid' in resource) || this.vpcConserveMode)" | ||
| v-model:value="selectedTier" |
There was a problem hiding this comment.
Template expressions should not reference component state via this (in Vue 3 templates this is not in scope), so this.vpcConserveMode will evaluate incorrectly and can break the tier selector visibility/focus. Drop this. and reference vpcConserveMode directly in the v-if / focus expressions.
| <strong>{{ $t('label.select.tier') }} </strong> | ||
| <a-select | ||
| :v-focus="'vpcid' in resource && !('associatednetworkid' in resource)" | ||
| :v-focus="'vpcid' in resource && (!('associatednetworkid' in resource) || this.vpcConserveMode)" |
There was a problem hiding this comment.
:v-focus is treated as a bound attribute, not a directive. If the intent is to conditionally apply the focus directive, this should be v-focus="..." (matching the other uses of v-focus in the UI), otherwise it won’t do anything.
| :v-focus="'vpcid' in resource && (!('associatednetworkid' in resource) || this.vpcConserveMode)" | |
| v-focus="'vpcid' in resource && (!('associatednetworkid' in resource) || this.vpcConserveMode)" |
| getAPI('listNics', { | ||
| virtualmachineid: e.target.value, | ||
| networkId: ('vpcid' in this.resource && !('associatednetworkid' in this.resource)) ? this.selectedTier : this.resource.associatednetworkid | ||
| networkId: ('vpcid' in this.resource && (!('associatednetworkid' in this.resource) || this.vpcConserveMode)) ? this.selectedTier : this.resource.associatednetworkid |
There was a problem hiding this comment.
The listNics call uses networkId as the parameter name, but other listNics usages in the UI (and the API) use networkid. Using the wrong key can cause the backend to ignore the network filter and return the wrong NICs (or none). Rename the parameter to networkid for consistency with other calls.
| networkId: ('vpcid' in this.resource && (!('associatednetworkid' in this.resource) || this.vpcConserveMode)) ? this.selectedTier : this.resource.associatednetworkid | |
| networkid: ('vpcid' in this.resource && (!('associatednetworkid' in this.resource) || this.vpcConserveMode)) ? this.selectedTier : this.resource.associatednetworkid |
| <span | ||
| v-if="'vpcid' in resource && !('associatednetworkid' in resource)"> | ||
| v-if="'vpcid' in resource && (!('associatednetworkid' in resource) || this.vpcConserveMode)"> | ||
| <strong>{{ $t('label.select.tier') }} </strong> | ||
| <a-select | ||
| v-focus="'vpcid' in resource && !('associatednetworkid' in resource)" | ||
| v-focus="'vpcid' in resource && (!('associatednetworkid' in resource) || this.vpcConserveMode)" | ||
| v-model:value="selectedTier" |
There was a problem hiding this comment.
Same as PortForwarding.vue: this.vpcConserveMode in the template is not valid in Vue 3 template scope and can break the tier selector visibility/focus. Reference vpcConserveMode directly (without this).
| if (this.resource && this.resource.vpcid) { | ||
| // VPC IPs with source nat have only VPN | ||
| if (this.resource.issourcenat) { | ||
| const vpc = await this.fetchVpc() | ||
|
|
||
| // VPC IPs with source nat have only VPN when VPC offering conserve mode = false | ||
| if (this.resource.issourcenat && vpc.vpcofferingconservemode === false) { | ||
| this.tabs = this.defaultTabs.concat(this.$route.meta.tabs.filter(tab => tab.name === 'vpn')) |
There was a problem hiding this comment.
filterTabs assumes fetchVpc always returns a VPC object and that vpcofferingconservemode is always present. If listVPCs returns no results (or the call fails), vpc can be null and vpc.vpcofferingconservemode will throw, leaving the page stuck loading. Also, comparing === false treats undefined as “conserve mode enabled”; for safer defaults/backwards compatibility, treat missing/undefined as false (i.e., restrict SourceNAT IPs unless the flag is explicitly true). Handle null/error from fetchVpc (try/catch or default object) and use a boolean-safe check.
| VpcOfferingVO vpcOffering = vpcOfferingDao.findById(newRuleNetwork.getVpcId()); | ||
| isVpcConserveModeEnabled = vpcOffering != null && vpcOffering.isConserveMode(); |
There was a problem hiding this comment.
In detectRulesConflict, the code uses newRuleNetwork.getVpcId() (a VPC id) to look up a VpcOffering via vpcOfferingDao.findById(...). This will query the vpc_offerings table by offering id and can return the wrong offering (or null), causing conserve mode detection to be incorrect. Fetch the VPC (e.g., via VpcDao using the VPC id from the network) and then look up the offering by vpc.getVpcOfferingId(), or otherwise resolve the offering id explicitly before calling vpcOfferingDao.
| VpcOfferingVO vpcOffering = vpcOfferingDao.findById(newRuleNetwork.getVpcId()); | |
| isVpcConserveModeEnabled = vpcOffering != null && vpcOffering.isConserveMode(); | |
| com.cloud.network.vpc.VpcVO vpc = _entityMgr.findById(com.cloud.network.vpc.VpcVO.class, newRuleNetwork.getVpcId()); | |
| if (vpc != null && vpc.getVpcOfferingId() != null) { | |
| VpcOfferingVO vpcOffering = vpcOfferingDao.findById(vpc.getVpcOfferingId()); | |
| isVpcConserveModeEnabled = vpcOffering != null && vpcOffering.isConserveMode(); | |
| } |
6 participants
Description
This PR extends the conserve mode for VPCs tiers added on the previous PRs: #8309, #10744 by allowing:
This PR also introduces the following changes:
Fixes: #8317
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?