Copilot commented on code in PR #12811:
URL: https://github.com/apache/cloudstack/pull/12811#discussion_r2929970072
##########
ui/src/views/infra/zone/BgpPeersTab.vue:
##########
@@ -18,15 +18,15 @@
<template>
<a-spin :spinning="componentLoading">
<a-alert
- v-if="this.resource.ip4routing"
+ v-if="this.resource.ip4routing || this.resource.ip6routing"
type="info">
<template #message>
<div v-html="$t('message.bgp.peers.null')" />
</template>
</a-alert>
<br>
<a-button
- v-if="!this.resource.ip4routing"
+ v-if="!(this.resource.ip4routing || this.resource.ip6routing)"
Review Comment:
The condition (this.resource.ip4routing || this.resource.ip6routing) is
repeated in multiple v-if expressions in this component. Consider introducing a
computed/property (e.g., hasRoutingConfigured) to centralize the logic and
reduce the chance of inconsistent updates in the future.
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -3578,12 +3582,24 @@ public VpcResponse createVpcResponse(ResponseView view,
Vpc vpc) {
response.addIpv4Route(route);
}
}
- if (view == ResponseView.Full) {
- List<BgpPeerVO> bgpPeerVOS =
bgpPeerDao.listNonRevokeByVpcId(vpc.getId());
- for (BgpPeerVO bgpPeerVO : bgpPeerVOS) {
- BgpPeerResponse bgpPeerResponse =
routedIpv4Manager.createBgpPeerResponse(bgpPeerVO);
- response.addBgpPeer(bgpPeerResponse);
- }
+ }
+
+ // add IPv6 routes
+ ipv6Service.updateIpv6RoutesForVpcResponse(vpc, response);
+ if (CollectionUtils.isNotEmpty(response.getIpv6Routes())) {
+ if (Objects.nonNull(asNumberVO)) {
+ response.setIpv6Routing(Network.Routing.Dynamic.name());
+ } else {
+ response.setIpv6Routing(Network.Routing.Static.name());
+ }
Review Comment:
ipv6Routing is only set when ipv6Routes is non-empty. This can leave
ipv6Routing unset for VPCs that support IPv6 but currently have no routes
populated, which is problematic for clients/UI that rely on ipv6Routing
presence/value (e.g., to determine whether to show BGP-related UI/actions).
Consider setting ipv6Routing deterministically (Dynamic/Static) independent of
whether ipv6Routes is empty.
##########
api/src/main/java/org/apache/cloudstack/api/response/VpcResponse.java:
##########
@@ -165,6 +165,10 @@ public class VpcResponse extends
BaseResponseWithAnnotations implements Controll
@Param(description = "The IPv4 routing mode of VPC", since = "4.20.0")
private String ipv4Routing;
+ @SerializedName(ApiConstants.IPV6_ROUTING)
+ @Param(description = "The Ipv6 routing type of VPC", since = "4.22.1")
Review Comment:
In the `@Param` description, 'Ipv6' should be capitalized as 'IPv6' to match
standard terminology used elsewhere in the API.
##########
ui/src/views/infra/zone/BgpPeersTab.vue:
##########
@@ -18,15 +18,15 @@
<template>
<a-spin :spinning="componentLoading">
<a-alert
- v-if="this.resource.ip4routing"
+ v-if="this.resource.ip4routing || this.resource.ip6routing"
Review Comment:
The condition (this.resource.ip4routing || this.resource.ip6routing) is
repeated in multiple v-if expressions in this component. Consider introducing a
computed/property (e.g., hasRoutingConfigured) to centralize the logic and
reduce the chance of inconsistent updates in the future.
##########
server/src/main/java/com/cloud/network/router/CommandSetupHelper.java:
##########
@@ -1470,11 +1470,21 @@ public void createBgpPeersCommands(final List<? extends
BgpPeer> bgpPeers, final
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 =
_networkOfferingDao.findByIdIncludingRemoved(guestNetwork.getNetworkOfferingId());
+ 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));
+ } else if (guestNetwork.getIp6Cidr() != null &&
bgpPeer.getIp6Address() != null) {
+ bgpPeerTOs.add(new BgpPeerTO(bgpPeer.getId(), null,
bgpPeer.getIp6Address(), bgpPeer.getAsNumber(), bgpPeer.getPassword(),
+ guestNetwork.getId(), asNumberVO.getAsNumber(),
guestNetwork.getCidr(), guestNetwork.getIp6Cidr(), bgpPeerDetails));
Review Comment:
In the IPv6-only path (non-ROUTED offerings), the BgpPeerTO is constructed
with a null IPv4 peer address but still passes guestNetwork.getCidr() (IPv4
CIDR). If the agent/VR side assumes IPv4 CIDR is present whenever provided,
this can lead to incorrect IPv4 route/BGP config attempts. Consider passing
null for the IPv4 CIDR (or adjusting the TO/agent handling) when creating an
IPv6-only BGP peer entry.
##########
server/src/main/java/com/cloud/network/router/CommandSetupHelper.java:
##########
@@ -1470,11 +1470,21 @@ public void createBgpPeersCommands(final List<? extends
BgpPeer> bgpPeers, final
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 =
_networkOfferingDao.findByIdIncludingRemoved(guestNetwork.getNetworkOfferingId());
+ if
(NetworkOffering.NetworkMode.ROUTED.equals(offering.getNetworkMode())) {
Review Comment:
The network offering lookup is inside the nested (peers × guestNetworks)
loops, leading to repeated DAO calls for the same guestNetwork offering.
Consider precomputing offering/network-mode per guestNetwork (or per
networkOfferingId) before iterating bgpPeers, so each offering is fetched once
per network rather than once per peer.
##########
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:
networkOfferingDao.findById(...) (and similarly
vpcOfferingDao.findById(...)) can return null (e.g., removed/invalid offering),
which will NPE on getNetworkMode(). Add a null guard and decide on a safe
behavior (e.g., log and return true / skip applying peers, or throw a
ResourceUnavailableException) to avoid a hard failure.
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -2843,6 +2839,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:
This now performs a BGP peer DAO lookup for every NetworkResponse in Full
view, even when the network isn't eligible for dynamic routing/BGP (likely
yielding empty results). Consider gating this block behind the same eligibility
condition used for applying peers (e.g.,
routedIpv4Manager.isDynamicRoutedNetwork(network)), to avoid unnecessary
database queries in admin-heavy listing scenarios.
##########
server/src/test/java/com/cloud/network/router/CommandSetupHelperTest.java:
##########
@@ -226,6 +227,10 @@ public void testCreateBgpPeersCommandsForNetwork() {
when(dcDao.findById(zoneId)).thenReturn(dc);
when(dc.getNetworkType()).thenReturn(DataCenter.NetworkType.Advanced);
+ NetworkOfferingVO offering = Mockito.mock(NetworkOfferingVO.class);
+
when(networkOfferingDao.findByIdIncludingRemoved(anyLong())).thenReturn(offering);
+
when(offering.getNetworkMode()).thenReturn(NetworkOffering.NetworkMode.ROUTED);
Review Comment:
Current tests only cover the ROUTED network-mode path. The new logic adds an
IPv6-only path for non-ROUTED offerings (and early-return when bgpPeerTOs is
empty). Add unit tests that (1) verify BgpPeerTO creation for non-ROUTED + IPv6
CIDR (with IPv4 peer/IP fields absent as expected), and (2) verify no command
is added when no eligible peers/networks produce any BgpPeerTOs.
--
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]
