Skip to content

VPC: associate tier public IPs when a public IP of the tier has firewall rules #14681

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
adestis-mf wants to merge 1 commit into
base: main
Choose a base branch
from
+101 −1
Open

VPC: associate tier public IPs when a public IP of the tier has firewall rules #14681

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion server/src/main/java/com/cloud/network/element/VpcVirtualRouterElement.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,7 +539,8 @@ public boolean deletePrivateGateway(final PrivateGateway gateway) throws Concurr
public boolean applyIps(final Network network, final List<? extends PublicIpAddress> ipAddress, final Set<Service> services) throws ResourceUnavailableException {
boolean canHandle = true;
for (final Service service : services) {
if (!canHandle(network, service)) {
if (!canHandleIpService(network, service)) {
logger.debug("{} element can't handle service {} in the network {}; skipping IP association", getName(), service.getName(), network);
canHandle = false;
break;
}
Expand All @@ -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);

@adestis-mf adestis-mf Nov 11, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
return false;
}

@Override
public boolean applyNetworkACLs(final Network network, final List<? extends NetworkACLItem> rules) throws ResourceUnavailableException {
boolean result = true;
Expand Down
83 changes: 83 additions & 0 deletions server/src/test/java/com/cloud/network/element/VpcVirtualRouterElementTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,21 @@
import com.cloud.exception.ResourceUnavailableException;
import com.cloud.network.Network;
import com.cloud.network.NetworkModel;
import com.cloud.network.PublicIpAddress;
import com.cloud.network.RemoteAccessVpn;
import com.cloud.network.VpnUser;
import com.cloud.network.router.VirtualRouter;
import com.cloud.network.router.VpcVirtualNetworkApplianceManagerImpl;
import com.cloud.network.vpc.Vpc;
import com.cloud.network.vpc.VpcManager;
import com.cloud.network.vpc.VpcVO;
import com.cloud.network.vpc.dao.VpcDao;
import com.cloud.utils.db.EntityManager;
import com.cloud.vm.DomainRouterVO;
import com.cloud.vm.dao.DomainRouterDao;
import org.apache.cloudstack.network.topology.AdvancedNetworkTopology;
import org.apache.cloudstack.network.topology.BasicNetworkTopology;
import org.apache.cloudstack.network.topology.NetworkTopology;
import org.apache.cloudstack.network.topology.NetworkTopologyContext;
import org.junit.Test;
import org.junit.runner.RunWith;
Expand All @@ -41,9 +45,12 @@
import org.mockito.junit.MockitoJUnitRunner;

import java.util.ArrayList;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Set;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
Expand Down Expand Up @@ -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;
}
Comment on lines +225 to +242

@adestis-mf adestis-mf Nov 11, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


private Set<Network.Service> lbAndFirewall() {
final Set<Network.Service> services = new LinkedHashSet<>();
services.add(Network.Service.Lb);
services.add(Network.Service.Firewall);
return services;
}

@Test
public void testApplyIpsAssociatesTierIpsWhenTheVpcProvidesFirewall() throws ResourceUnavailableException {
final Network network = mockVpcTier();
when(network.getDataCenterId()).thenReturn(1L);
when(_networkMdl.isProviderSupportServiceInNetwork(200L, Network.Service.Lb, Network.Provider.VPCVirtualRouter)).thenReturn(true);
when(_networkMdl.isProviderSupportServiceInNetwork(200L, Network.Service.Firewall, Network.Provider.VPCVirtualRouter)).thenReturn(false);
mockVpc(true);

final DomainRouterVO router = Mockito.mock(DomainRouterVO.class);
when(_routerDao.listByNetworkAndRole(200L, VirtualRouter.Role.VIRTUAL_ROUTER)).thenReturn(List.of(router));
final DataCenterVO dc = Mockito.mock(DataCenterVO.class);
when(_dcDao.findById(1L)).thenReturn(dc);
final NetworkTopology topology = Mockito.mock(NetworkTopology.class);
when(networkTopologyContext.retrieveNetworkTopology(dc)).thenReturn(topology);
final List<PublicIpAddress> ips = List.of(Mockito.mock(PublicIpAddress.class));
when(topology.associatePublicIP(network, ips, router)).thenReturn(true);

assertTrue(vpcVirtualRouterElement.applyIps(network, ips, lbAndFirewall()));
verify(topology).associatePublicIP(network, ips, router);
}

@Test
public void testApplyIpsSkipsAssociationWhenNeitherTierNorVpcProvidesFirewall() throws ResourceUnavailableException {
final Network network = mockVpcTier();
when(_networkMdl.isProviderSupportServiceInNetwork(200L, Network.Service.Lb, Network.Provider.VPCVirtualRouter)).thenReturn(true);
when(_networkMdl.isProviderSupportServiceInNetwork(200L, Network.Service.Firewall, Network.Provider.VPCVirtualRouter)).thenReturn(false);
mockVpc(false);

assertTrue(vpcVirtualRouterElement.applyIps(network, List.of(Mockito.mock(PublicIpAddress.class)), lbAndFirewall()));
verify(_routerDao, never()).listByNetworkAndRole(Mockito.anyLong(), Mockito.any());
}

@Test
public void testCanHandleIpServiceUsesTheVpcOnlyForFirewall() {
final Network network = mockVpcTier();
when(_networkMdl.isProviderSupportServiceInNetwork(200L, Network.Service.PortForwarding, Network.Provider.VPCVirtualRouter)).thenReturn(false);

assertFalse(vpcVirtualRouterElement.canHandleIpService(network, Network.Service.PortForwarding));
verify(_vpcDao, never()).findById(Mockito.anyLong());
}

@Test
public void testCanHandleIpServiceKeepsTierFirewall() {
final Network network = mockVpcTier();
when(_networkMdl.isProviderSupportServiceInNetwork(200L, Network.Service.Firewall, Network.Provider.VPCVirtualRouter)).thenReturn(true);

assertTrue(vpcVirtualRouterElement.canHandleIpService(network, Network.Service.Firewall));
verify(_vpcDao, never()).findById(Mockito.anyLong());
}
}