adestis-mf commented on code in PR #14681:
URL: https://github.com/apache/cloudstack/pull/14681#discussion_r4541285628
##########
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:
`canHandle(Vpc, Service)` already returns `false` for a null VPC (it is the
first check in that
method), so there is no NullPointerException here: if the VPC cannot be
found, the IP association is
skipped, as it was before this change, and the new debug line logs that.
`applyFWRulesInVPC` goes
through the same method. I would leave it as it is.
##########
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:
The new helpers use the same values as the existing
`testCanHandleFirewallUsesVpcCapability` in this
class, which has `1L`, `100L` and `200L` inline as well. I kept that style
so the new tests read like
the existing one and the change stays small. Happy to switch both to named
constants if reviewers
prefer.
--
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]
