Copilot commented on code in PR #14116:
URL: https://github.com/apache/cloudstack/pull/14116#discussion_r3977931453
##########
engine/schema/src/main/java/com/cloud/offerings/NetworkOfferingVO.java:
##########
@@ -346,10 +346,6 @@ public boolean isEgressDefaultPolicy() {
return egressdefaultpolicy;
}
Review Comment:
Removing the public `setEgressDefaultPolicy(...)` method is a breaking API
change at the model level and can complicate creation/update flows that need to
explicitly set this field. If the goal is only to revert defaulting behavior,
consider retaining the setter (or deprecating it first) and enforce defaults in
the relevant creation paths instead.
##########
server/src/main/java/com/cloud/server/ConfigurationServerImpl.java:
##########
@@ -1067,12 +1067,6 @@ public void
doInTransactionWithoutResult(TransactionStatus status) {
Network.GuestType.Isolated, true, false,
false, false, true, false);
defaultIsolatedSourceNatEnabledNetworkOffering.setState(NetworkOffering.State.Enabled);
- // Default egress policy is Allow on fresh installations,
consistent with the
- // createNetworkOffering API default (egressdefaultpolicy=true
when not specified).
- // Existing installations are not affected: this method only
runs on first boot
- // (guarded by the "init" configuration flag) and
persistDefaultNetworkOffering()
- // never updates an already existing offering.
-
defaultIsolatedSourceNatEnabledNetworkOffering.setEgressDefaultPolicy(true);
defaultIsolatedSourceNatEnabledNetworkOffering.setSupportsVmAutoScaling(true);
defaultIsolatedSourceNatEnabledNetworkOffering =
_networkOfferingDao.persistDefaultNetworkOffering(defaultIsolatedSourceNatEnabledNetworkOffering);
Review Comment:
This change alters the first-boot seeding behavior for default isolated
offerings by no longer explicitly setting the egress default policy. The prior
unit test asserting the seeded egress policy was removed, but there is no
remaining coverage ensuring the reverted default is correctly applied. Consider
adding/adjusting a test to assert the current expected seeded value (e.g.,
deny/false) for the built-in isolated offerings.
##########
ui/src/views/offering/AddNetworkOffering.vue:
##########
@@ -704,7 +704,7 @@ export default {
isolation: 'dedicated',
conservemode: true,
availability: 'optional',
- egressdefaultpolicy: 'allow',
+ egressdefaultpolicy: 'deny',
Review Comment:
The PR title/description indicate reverting the *fresh-install seeding*
behavior for isolated networks, but this UI default change affects all newly
created offerings via the UI (not just first-boot seeded offerings). If this
broader behavior change is intended, consider updating the PR description/title
to reflect it; otherwise, consider moving this UI change into a separate PR to
keep the revert tightly scoped.
--
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]
