Copilot commented on code in PR #14286:
URL: https://github.com/apache/cloudstack/pull/14286#discussion_r4245690769
##########
api/src/main/java/com/cloud/offering/NetworkOffering.java:
##########
@@ -70,6 +70,7 @@ enum RoutingMode {
public static final String DEFAULT_ROUTED_NSX_OFFERING =
"DefaultRoutedNSXNetworkOffering";
public final static String QuickCloudNoServices = "QuickCloudNoServices";
public final static String
DefaultIsolatedNetworkOfferingWithSourceNatService =
"DefaultIsolatedNetworkOfferingWithSourceNatService";
+ public final static String
DefaultIsolatedNetworkOfferingWithSourceNatServiceDefaultEgressAllowed =
"DefaultIsolatedNetworkOfferingWithSourceNatServiceEgressAllowed";
Review Comment:
The PR description and the unique-name string use
`...SourceNatServiceEgressAllowed`, but the Java constant identifier is
`...SourceNatServiceDefaultEgressAllowed`. This inconsistency makes the API
harder to reason about and increases the chance of using the wrong identifier.
Align the constant name with the unique-name string (preferred, since the
string is persisted/used for lookups).
##########
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java:
##########
@@ -636,6 +636,20 @@ public void doInTransactionWithoutResult(final
TransactionStatus status) {
true, false, false, false, false, null, null,
null, true, null, null, false);
}
+ //#4-2 - default isolated offering with Source nat service and
egress traffic allowed by default
+ if
(_networkOfferingDao.findByUniqueName(NetworkOffering.DefaultIsolatedNetworkOfferingWithSourceNatServiceDefaultEgressAllowed)
== null) {
+ offering =
_configMgr.createNetworkOffering(NetworkOffering.DefaultIsolatedNetworkOfferingWithSourceNatServiceDefaultEgressAllowed,
+ "Offering for Isolated networks with Source Nat
service enabled and egress traffic allowed by default", TrafficType.Guest,
null, false, Availability.Optional, null,
+
defaultIsolatedSourceNatEnabledNetworkOfferingProviders, true,
Network.GuestType.Isolated, false, null, true, null, false, false, null, true,
null,
+ true, false, false, false, false, null, null,
null, true, null, null, false);
+ // keep consistent with the offering created on fresh
installation (ConfigurationServerImpl)
+ offering.setMulticastRateMbps(null);
+ offering.setDedicatedLB(true);
+ offering.setEipAssociatePublicIp(true);
+ offering.setSupportsVmAutoScaling(true);
Review Comment:
The orchestrator path that creates the offering on existing installs does
not set `egressDefaultPolicy=true`, while the fresh-install path (in
`ConfigurationServerImpl`) does. This makes behavior inconsistent across new vs
upgraded deployments, and contradicts the intent/comment. Set
`offering.setEgressDefaultPolicy(true)` before
`_networkOfferingDao.update(...)`.
--
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]
