Copilot commented on code in PR #12811:
URL: https://github.com/apache/cloudstack/pull/12811#discussion_r3200408054
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -2859,6 +2855,15 @@ public NetworkResponse
createNetworkResponse(ResponseView view, Network network)
}
}
+ // Add BGP peer information for full view
+ if (view == ResponseView.Full) {
+ List<BgpPeerVO> bgpPeerVOS =
bgpPeerDao.listNonRevokeByNetworkId(network.getId());
+ for (BgpPeerVO bgpPeerVO : bgpPeerVOS) {
+ BgpPeerResponse bgpPeerResponse =
routedIpv4Manager.createBgpPeerResponse(bgpPeerVO);
+ response.addBgpPeer(bgpPeerResponse);
+ }
+ }
Review Comment:
`createNetworkResponse` now always queries and attaches BGP peers for
`ResponseView.Full` (`bgpPeerDao.listNonRevokeByNetworkId(...)`) regardless of
whether the network is using dynamic routing. This can introduce an N+1
query/performance regression for full network listings. Consider guarding this
block (e.g., only when the network/VPC has an ASN or when
`routedIpv4Manager.isDynamicRoutedNetwork(network)` is true) so the extra DB
lookup only happens when BGP peers are relevant.
##########
ui/src/views/infra/zone/BgpPeersTab.vue:
##########
@@ -456,6 +456,9 @@ export default {
asnumber: [{ required: true, message: this.$t('label.required') }]
})
},
+ isIpRoutingEnabled () {
+ return !!(this.resource && (this.resource.ip4routing ||
this.resource.ip6routing))
Review Comment:
`isIpRoutingEnabled()` currently treats any truthy `ip4routing`/`ip6routing`
value as “enabled”. Since these fields are typically strings like
"Static"/"Dynamic", this will return true even for static routing, causing the
component to hide the zone-level BGP peer list/actions and instead read
`resource.bgppeers` (often undefined). Consider checking explicitly for dynamic
routing (e.g., `ip4routing === 'Dynamic' || ip6routing === 'Dynamic'`) to match
other UI gating and the intended behavior.
##########
server/src/main/java/com/cloud/bgp/BGPServiceImpl.java:
##########
@@ -396,9 +397,12 @@ public boolean applyBgpPeers(Network network, boolean
continueOnError) throws Re
if (!routedIpv4Manager.isDynamicRoutedNetwork(network)) {
return true;
}
- final String gatewayProviderStr =
ntwkSrvcDao.getProviderForServiceInNetwork(network.getId(),
Network.Service.Gateway);
- if (gatewayProviderStr != null) {
- NetworkElement provider =
networkModel.getElementImplementingProvider(gatewayProviderStr);
+ NetworkOffering networkOffering =
networkOfferingDao.findById(network.getNetworkOfferingId());
+ final String bgpServiceProvider =
NetworkOffering.NetworkMode.ROUTED.equals(networkOffering.getNetworkMode()) ?
+ ntwkSrvcDao.getProviderForServiceInNetwork(network.getId(),
Network.Service.Gateway):
+ ntwkSrvcDao.getProviderForServiceInNetwork(network.getId(),
Network.Service.SourceNat);
Review Comment:
`applyBgpPeers(Network, ...)` dereferences
`networkOffering.getNetworkMode()` without handling the case where
`networkOfferingDao.findById(...)` returns null (e.g., offering removed). This
can lead to a NullPointerException. Consider null-checking the offering (and/or
using `findByIdIncludingRemoved`) before selecting the provider service
(Gateway vs SourceNat).
##########
server/src/main/java/com/cloud/bgp/BGPServiceImpl.java:
##########
@@ -413,9 +417,12 @@ public boolean applyBgpPeers(Vpc vpc, boolean
continueOnError) throws ResourceUn
if (!routedIpv4Manager.isDynamicRoutedVpc(vpc)) {
return true;
}
- final String gatewayProviderStr =
vpcServiceMapDao.getProviderForServiceInVpc(vpc.getId(),
Network.Service.Gateway);
- if (gatewayProviderStr != null) {
- NetworkElement provider =
networkModel.getElementImplementingProvider(gatewayProviderStr);
+ VpcOffering vpcOffering =
vpcOfferingDao.findById(vpc.getVpcOfferingId());
Review Comment:
`applyBgpPeers(Vpc, ...)` dereferences `vpcOffering.getNetworkMode()`
without handling the case where `vpcOfferingDao.findById(...)` returns null
(e.g., offering removed). This can lead to a NullPointerException. Consider
null-checking the offering (and/or using `findByIdIncludingRemoved`) before
selecting the provider service (Gateway vs SourceNat).
##########
server/src/main/java/com/cloud/network/router/CommandSetupHelper.java:
##########
@@ -1468,14 +1468,29 @@ public void createBgpPeersCommands(final List<? extends
BgpPeer> bgpPeers, final
} else {
guestNetworks.add(network);
}
+ Map<Long, NetworkOfferingVO> guestNetworkOfferings = new HashMap<>();
+ for (Network guestNetwork : guestNetworks) {
+ final NetworkOfferingVO offering =
_networkOfferingDao.findByIdIncludingRemoved(guestNetwork.getNetworkOfferingId());
+ guestNetworkOfferings.put(guestNetwork.getId(), offering);
+ }
for (BgpPeer bgpPeer: bgpPeers) {
Map<BgpPeer.Detail, String> bgpPeerDetails =
bgpPeerDetailsDao.getBgpPeerDetails(bgpPeer.getId());
for (Network guestNetwork : guestNetworks) {
- bgpPeerTOs.add(new BgpPeerTO(bgpPeer.getId(),
bgpPeer.getIp4Address(), bgpPeer.getIp6Address(), bgpPeer.getAsNumber(),
bgpPeer.getPassword(),
- guestNetwork.getId(), asNumberVO.getAsNumber(),
guestNetwork.getCidr(), guestNetwork.getIp6Cidr(), bgpPeerDetails));
+ final NetworkOfferingVO offering =
guestNetworkOfferings.get(guestNetwork.getId());
+ if
(NetworkOffering.NetworkMode.ROUTED.equals(offering.getNetworkMode())) {
+ bgpPeerTOs.add(new BgpPeerTO(bgpPeer.getId(),
bgpPeer.getIp4Address(), bgpPeer.getIp6Address(), bgpPeer.getAsNumber(),
bgpPeer.getPassword(),
+ guestNetwork.getId(), asNumberVO.getAsNumber(),
guestNetwork.getCidr(), guestNetwork.getIp6Cidr(), bgpPeerDetails));
Review Comment:
`guestNetworkOfferings` may contain null values if
`_networkOfferingDao.findByIdIncludingRemoved(...)` returns null; the later
`offering.getNetworkMode()` dereference would then throw an NPE. Consider
skipping networks with missing offerings (or logging) and guarding the
`getNetworkMode()` call to avoid breaking BGP peer configuration for the whole
router due to one missing offering record.
--
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]
