Copilot commented on code in PR #14286:
URL: https://github.com/apache/cloudstack/pull/14286#discussion_r4246106119
##########
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.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,
+ false, 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);
+ _networkOfferingDao.update(offering.getId(), offering);
Review Comment:
This code creates an offering whose unique name/description indicates egress
is allowed by default, but it never sets egressDefaultPolicy=true (unlike the
fresh-install path in ConfigurationServerImpl where it’s explicitly set). That
means upgraded zones may end up with an offering that does not match its
intended behavior. Set the offering’s egress default policy to true here before
persisting the update (or pass the correct flag via createNetworkOffering if
that API supports it).
##########
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);
+
for (Service service :
defaultIsolatedSourceNatEnabledNetworkOfferingProviders.keySet()) {
NetworkOfferingServiceMapVO offService =
new
NetworkOfferingServiceMapVO(defaultIsolatedSourceNatEnabledNetworkOffering.getId(),
service,
defaultIsolatedSourceNatEnabledNetworkOfferingProviders.get(service));
_ntwkOfferingServiceMapDao.persist(offService);
- logger.trace("Added service for the network offering: " +
offService);
+ logger.trace("Added service {} for the network offering:
{}", offService,
defaultIsolatedSourceNatEnabledNetworkOffering.getUniqueName());
+
+ offService =
+ new
NetworkOfferingServiceMapVO(defaultIsolatedSourceNatEnabledNetworkOfferingWithDefaultEgressAllowed.getId(),
service,
+
defaultIsolatedSourceNatEnabledNetworkOfferingProviders.get(service));
+ _ntwkOfferingServiceMapDao.persist(offService);
+ logger.trace("Added service {} for the network offering:
{}", offService,
defaultIsolatedSourceNatEnabledNetworkOfferingWithDefaultEgressAllowed.getUniqueName());
Review Comment:
The log message says \"Added service {}\" but the first placeholder is
populated with the entire NetworkOfferingServiceMapVO (offService), which is
confusing/misleading when scanning logs. Prefer logging the `service` variable
(and/or provider set) and the offering unique name, or update the message to
explicitly say you're logging the service-map object.
--
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]
