Copilot commented on code in PR #14681:
URL: https://github.com/apache/cloudstack/pull/14681#discussion_r4541079264
##########
server/src/test/java/com/cloud/network/element/VpcVirtualRouterElementTest.java:
##########
@@ -214,4 +221,80 @@ public void testCanHandleFirewallUsesVpcCapability() {
verify(_networkMdl).isProviderSupportServiceInNetwork(200L,
Network.Service.Firewall, Network.Provider.VPCVirtualRouter);
verify(_vpcMgr, never()).isProviderSupportServiceInVpc(100L,
Network.Service.Firewall, Network.Provider.VPCVirtualRouter);
}
+
+ private Network mockVpcTier() {
+ final Network network = Mockito.mock(Network.class);
+ when(_networkMdl.getPhysicalNetworkId(network)).thenReturn(1L);
+ when(network.getId()).thenReturn(200L);
+ when(network.getVpcId()).thenReturn(100L);
+ when(_networkMdl.isProviderEnabledInPhysicalNetwork(1L,
Network.Provider.VPCVirtualRouter.getName())).thenReturn(true);
+ return network;
+ }
+
+ private VpcVO mockVpc(final boolean vpcProvidesFirewall) {
+ final VpcVO vpc = Mockito.mock(VpcVO.class);
+ when(vpc.getId()).thenReturn(100L);
+ when(vpc.getZoneId()).thenReturn(1L);
+ when(_vpcDao.findById(100L)).thenReturn(vpc);
+ when(_networkMdl.isProviderEnabledInZone(1L,
Network.Provider.VPCVirtualRouter.getName())).thenReturn(true);
+ when(_vpcMgr.isProviderSupportServiceInVpc(100L,
Network.Service.Firewall,
Network.Provider.VPCVirtualRouter)).thenReturn(vpcProvidesFirewall);
+ return vpc;
+ }
Review Comment:
These helpers repeat hard-coded IDs (`1L`, `100L`, `200L`) across multiple
stubbings. Consider promoting them to named constants (e.g., `ZONE_ID`,
`VPC_ID`, `NETWORK_ID`, `PHYS_NET_ID`) to make the tests easier to read and to
avoid inconsistencies if the values change.
##########
server/src/main/java/com/cloud/network/element/VpcVirtualRouterElement.java:
##########
@@ -563,6 +564,22 @@ public boolean applyIps(final Network network, final
List<? extends PublicIpAddr
return result;
}
+ /**
+ * The Firewall service for public IPs is a VPC service: the VPC offering
provides it, while the
+ * tier's network offering does not list it. Once a public IP of the tier
has a firewall rule,
+ * Firewall is part of the services passed to {@link #applyIps}, so it is
checked on the VPC
+ * here. Otherwise no public IP of that tier could be associated with the
VPC virtual router.
+ */
+ protected boolean canHandleIpService(final Network network, final Service
service) {
+ if (canHandle(network, service)) {
+ return true;
+ }
+ if (Service.Firewall.equals(service) && network.getVpcId() != null) {
+ return canHandle(_vpcDao.findById(network.getVpcId()), service);
Review Comment:
`_vpcDao.findById(network.getVpcId())` can return null; passing that into
`canHandle(...)` risks a NullPointerException (or incorrect behavior) during IP
association. Consider fetching into a local `Vpc` variable and returning
`false` (or logging) when the VPC lookup returns null.
--
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]
