Copilot commented on code in PR #12487:
URL: https://github.com/apache/cloudstack/pull/12487#discussion_r2779371905
##########
ui/src/views/network/PortForwarding.vue:
##########
@@ -216,10 +216,10 @@
@cancel="closeModal">
<div v-ctrl-enter="addRule">
<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"
Review 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.
##########
ui/src/views/network/PortForwarding.vue:
##########
@@ -216,10 +216,10 @@
@cancel="closeModal">
<div v-ctrl-enter="addRule">
<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)"
Review 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.
```suggestion
v-focus="'vpcid' in resource && (!('associatednetworkid' in
resource) || this.vpcConserveMode)"
```
##########
ui/src/views/network/LoadBalancing.vue:
##########
@@ -487,10 +487,10 @@
>
<div @keyup.ctrl.enter="handleAddNewRule">
<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"
Review 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`).
##########
ui/src/views/network/PublicIpResource.vue:
##########
@@ -135,8 +135,10 @@ export default {
return
}
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'))
Review 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.
##########
server/src/main/java/com/cloud/network/firewall/FirewallManagerImpl.java:
##########
@@ -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);
Review 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.
##########
server/src/main/java/com/cloud/network/firewall/FirewallManagerImpl.java:
##########
@@ -395,6 +399,17 @@ public void detectRulesConflict(FirewallRule newRule)
throws NetworkRuleConflict
assert (rules.size() >= 1);
}
+ 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();
Review 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.
```suggestion
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();
}
```
##########
ui/src/views/network/PortForwarding.vue:
##########
@@ -788,7 +800,7 @@ export default {
this.newRule.virtualmachineid = e.target.value
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
Review 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.
```suggestion
networkid: ('vpcid' in this.resource && (!('associatednetworkid' in
this.resource) || this.vpcConserveMode)) ? this.selectedTier :
this.resource.associatednetworkid
```
--
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]
