Copilot commented on code in PR #14286:
URL: https://github.com/apache/cloudstack/pull/14286#discussion_r4246472490
##########
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java:
##########
@@ -636,6 +636,22 @@ 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.DefaultIsolatedNetworkOfferingWithSourceNatServiceEgressAllowed)
== null) {
+ offering =
_configMgr.createNetworkOffering(NetworkOffering.DefaultIsolatedNetworkOfferingWithSourceNatServiceEgressAllowed,
+ "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);
+ offering.setEgressDefaultPolicy(true);
+ offering.setKeepAliveEnabled(false);
+ _networkOfferingDao.update(offering.getId(), offering);
+ }
Review Comment:
The upgrade-path creation sets `dedicatedLB` and `eipAssociatePublicIp` to
`true` (and updates the DB), but the fresh-install path in
`ConfigurationServerImpl` does not set these fields for the same offering. This
makes the offering behave differently depending on whether it was created
during initial setup vs later orchestration. Align both code paths by either
setting the same fields in `ConfigurationServerImpl` for this offering, or
removing these extra setters here if they are not intended to be enabled.
##########
engine/schema/src/main/java/com/cloud/offerings/NetworkOfferingVO.java:
##########
@@ -404,7 +408,7 @@ public NetworkOfferingVO(String name, String displayText,
TrafficType trafficTyp
this.elasticLb = elasticLb;
this.inline = inline;
this.eipAssociatePublicIp = associatePublicIP;
- this.egressdefaultpolicy = egressdefaultpolicy;
+ this.egressDefaultPolicy = egressdefaultpolicy;
Review Comment:
After renaming the field to `egressDefaultPolicy`, the constructor parameter
name `egressdefaultpolicy` is now inconsistent and makes the assignment harder
to read. Rename the parameter to `egressDefaultPolicy` (or similar) to match
the field and getter/setter naming.
##########
server/src/main/java/com/cloud/server/ConfigurationServerImpl.java:
##########
@@ -1070,12 +1070,29 @@ public void
doInTransactionWithoutResult(TransactionStatus status) {
defaultIsolatedSourceNatEnabledNetworkOffering.setSupportsVmAutoScaling(true);
defaultIsolatedSourceNatEnabledNetworkOffering =
_networkOfferingDao.persistDefaultNetworkOffering(defaultIsolatedSourceNatEnabledNetworkOffering);
+ // Offering #3-2
+ NetworkOfferingVO
defaultIsolatedSourceNatEnabledNetworkOfferingWithDefaultEgressAllowed =
+ new
NetworkOfferingVO(NetworkOffering.DefaultIsolatedNetworkOfferingWithSourceNatServiceEgressAllowed,
+ "Offering for Isolated networks with Source
Nat service enabled and egress traffic allowed by default", TrafficType.Guest,
false, false, null, null, true, Availability.Optional, null,
+ Network.GuestType.Isolated, true, false,
false, false, true, false);
+
+
defaultIsolatedSourceNatEnabledNetworkOfferingWithDefaultEgressAllowed.setState(NetworkOffering.State.Enabled);
+
defaultIsolatedSourceNatEnabledNetworkOfferingWithDefaultEgressAllowed.setSupportsVmAutoScaling(true);
+
defaultIsolatedSourceNatEnabledNetworkOfferingWithDefaultEgressAllowed.setEgressDefaultPolicy(true);
+
defaultIsolatedSourceNatEnabledNetworkOfferingWithDefaultEgressAllowed =
_networkOfferingDao.persistDefaultNetworkOffering(defaultIsolatedSourceNatEnabledNetworkOfferingWithDefaultEgressAllowed);
Review Comment:
To prevent divergence between fresh-install and upgrade-path defaults for
this newly introduced offering, consider centralizing the offering's full
default configuration (e.g., dedicated LB, EIP associate public IP, keep-alive)
in one place (shared helper / builder), or explicitly setting the same set of
fields here that `NetworkOrchestrator` sets. Right now only a subset of
defaults are applied in this code path, which increases the risk of future
drift.
--
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]
