Copilot commented on code in PR #14133:
URL: https://github.com/apache/cloudstack/pull/14133#discussion_r4518107606
##########
server/src/main/java/com/cloud/network/IpAddressManagerImpl.java:
##########
@@ -792,11 +792,15 @@ public boolean disassociatePublicIpAddress(IpAddress
ipAddress, long userId, Acc
logger.debug("Releasing ip {}; sourceNat = {}", ip,
ip.isSourceNat());
}
+ Network associatedNetwork = null;
if (ip.getAssociatedWithNetworkId() != null) {
- Network network =
_networksDao.findById(ip.getAssociatedWithNetworkId());
+ associatedNetwork =
_networksDao.findById(ip.getAssociatedWithNetworkId());
+ }
+
+ if (associatedNetwork != null) {
try {
- if (!applyIpAssociations(network, rulesContinueOnErrFlag))
{
- logger.warn("Unable to apply ip address associations
for " + network);
+ if (!applyIpAssociations(associatedNetwork,
rulesContinueOnErrFlag)) {
+ logger.warn("Unable to apply ip address associations
for " + associatedNetwork);
Review Comment:
When `ip.getAssociatedWithNetworkId()` is non-null but
`_networksDao.findById(...)` returns null, the code now silently skips
association application. This avoids the NPE (good), but it also hides a
data-integrity/ordering problem that will be hard to diagnose in production.
Consider adding an explicit log (warn or debug) in the `associatedNetwork ==
null && associatedNetworkId != null` case including the IP ID and associated
network ID, so operators can correlate inconsistent state during teardown.
##########
server/src/test/java/com/cloud/network/IpAddressManagerTest.java:
##########
@@ -130,6 +157,90 @@ public void setup() throws ResourceUnavailableException {
Mockito.when(networkOfferingDao.findById(Mockito.anyLong())).thenReturn(networkOfferingVO);
}
+ @Test
+ public void
disassociatePublicIpAddressUnassignsReleasingIpWhenAssociatedNetworkIsMissing()
throws ResourceUnavailableException {
+ long networkId = 2L;
+ long vpcId = 3L;
+ long vpcOfferingId = 4L;
+ prepareIpDisassociation(networkId);
+ when(ipAddressVoMock.getId()).thenReturn(dummyID);
+ when(ipAddressVoMock.getState()).thenReturn(IpAddress.State.Releasing);
+ when(ipAddressVoMock.getVpcId()).thenReturn(vpcId);
+ when(networkDao.findById(networkId)).thenReturn(null);
+
doReturn(publicIpQuarantineVOMock).when(ipAddressManager).addPublicIpAddressToQuarantine(ipAddressVoMock,
account.getDomainId());
+
+ VpcVO vpc = mock(VpcVO.class);
+ VpcOfferingVO offering = mock(VpcOfferingVO.class);
+ when(vpcDao.findById(vpcId)).thenReturn(vpc);
+ when(vpc.getVpcOfferingId()).thenReturn(vpcOfferingId);
+ when(vpcOfferingDao.findById(vpcOfferingId)).thenReturn(offering);
+ when(offering.getId()).thenReturn(vpcOfferingId);
+
when(vpcOfferingServiceMapDao.listProvidersForServiceForVpcOffering(vpcOfferingId,
Service.NetworkACL))
+ .thenReturn(Collections.singletonList(new
VpcOfferingServiceMapVO(vpcOfferingId, Service.NetworkACL,
Network.Provider.VPCVirtualRouter)));
+ NetworkElement element = mock(NetworkElement.class);
+ ipAddressManager._networkModel = mock(NetworkModel.class);
+
when(ipAddressManager._networkModel.getElementImplementingProvider(Network.Provider.VPCVirtualRouter.getName())).thenReturn(element);
+
+ boolean result =
ipAddressManager.disassociatePublicIpAddress(ipAddressMock, dummyID, account);
+
+ assertTrue(result);
+ verify(ipAddressManager, never()).applyIpAssociations(any(),
anyBoolean());
+
verify(ipAddressManager).addPublicIpAddressToQuarantine(ipAddressVoMock,
account.getDomainId());
+ verify(ipAddressDao).unassignIpAddress(dummyID);
+ verify(element).releaseIp(ipAddressVoMock);
+ verify(annotationDao).removeByEntityType("PUBLIC_IP_ADDRESS", UUID);
Review Comment:
This assertion hard-codes the entity type string. If the production code
uses a constant/enum (or the value changes), the test will become brittle for
reasons unrelated to behavior. Prefer verifying against the same constant/enum
value used in production (e.g., a shared constant for the PUBLIC_IP_ADDRESS
entity type), so refactors don’t require updating string literals across tests.
##########
server/src/test/java/com/cloud/network/IpAddressManagerTest.java:
##########
@@ -130,6 +157,90 @@ public void setup() throws ResourceUnavailableException {
Mockito.when(networkOfferingDao.findById(Mockito.anyLong())).thenReturn(networkOfferingVO);
}
+ @Test
+ public void
disassociatePublicIpAddressUnassignsReleasingIpWhenAssociatedNetworkIsMissing()
throws ResourceUnavailableException {
+ long networkId = 2L;
+ long vpcId = 3L;
+ long vpcOfferingId = 4L;
+ prepareIpDisassociation(networkId);
+ when(ipAddressVoMock.getId()).thenReturn(dummyID);
+ when(ipAddressVoMock.getState()).thenReturn(IpAddress.State.Releasing);
+ when(ipAddressVoMock.getVpcId()).thenReturn(vpcId);
+ when(networkDao.findById(networkId)).thenReturn(null);
+
doReturn(publicIpQuarantineVOMock).when(ipAddressManager).addPublicIpAddressToQuarantine(ipAddressVoMock,
account.getDomainId());
+
+ VpcVO vpc = mock(VpcVO.class);
+ VpcOfferingVO offering = mock(VpcOfferingVO.class);
+ when(vpcDao.findById(vpcId)).thenReturn(vpc);
+ when(vpc.getVpcOfferingId()).thenReturn(vpcOfferingId);
+ when(vpcOfferingDao.findById(vpcOfferingId)).thenReturn(offering);
+ when(offering.getId()).thenReturn(vpcOfferingId);
+
when(vpcOfferingServiceMapDao.listProvidersForServiceForVpcOffering(vpcOfferingId,
Service.NetworkACL))
+ .thenReturn(Collections.singletonList(new
VpcOfferingServiceMapVO(vpcOfferingId, Service.NetworkACL,
Network.Provider.VPCVirtualRouter)));
+ NetworkElement element = mock(NetworkElement.class);
+ ipAddressManager._networkModel = mock(NetworkModel.class);
+
when(ipAddressManager._networkModel.getElementImplementingProvider(Network.Provider.VPCVirtualRouter.getName())).thenReturn(element);
+
Review Comment:
This test mutates an internal field directly, which tightly couples the test
to the class’s internal structure and can break on refactors (e.g., field
rename/visibility changes). If available in the test setup, prefer injecting
the dependency via `@InjectMocks`/constructor/setter or using a supported test
utility for field injection, so the test stays focused on behavior rather than
internals.
--
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]
