From 16d45f731d7ae7d49287d33f21c2eb7ae141707e Mon Sep 17 00:00:00 2001 From: Fabricio Duarte Date: Mon, 18 Dec 2023 03:36:31 -0300 Subject: [PATCH] Save the account which deliberately removed a public IP from quarantine (#8339) When a public IP gets removed from quarantine, the removal reason gets saved to the database; however, it may also be useful for operators to know who removed the public IP from quarantine. For that reason, this PR extends the public IP quarantine feature so that the account that deliberately removed an IP from quarantine also gets saved to the database. --- .../com/cloud/network/PublicIpQuarantine.java | 2 + .../apache/cloudstack/api/ApiConstants.java | 1 + .../api/response/IpQuarantineResponse.java | 12 ++++ .../network/vo/PublicIpQuarantineVO.java | 12 ++++ .../upgrade/dao/Upgrade41810to41900.java | 4 ++ .../META-INF/db/schema-41810to41900.sql | 3 + .../java/com/cloud/api/ApiResponseHelper.java | 4 ++ .../cloud/network/IpAddressManagerImpl.java | 2 + .../com/cloud/api/ApiResponseHelperTest.java | 56 +++++++++++++++++++ .../cloud/network/IpAddressManagerTest.java | 27 +++++++++ 10 files changed, 123 insertions(+) diff --git a/api/src/main/java/com/cloud/network/PublicIpQuarantine.java b/api/src/main/java/com/cloud/network/PublicIpQuarantine.java index d1ec98afe46..625a1bbbe50 100644 --- a/api/src/main/java/com/cloud/network/PublicIpQuarantine.java +++ b/api/src/main/java/com/cloud/network/PublicIpQuarantine.java @@ -30,6 +30,8 @@ public interface PublicIpQuarantine extends InternalIdentity, Identity { String getRemovalReason(); + Long getRemoverAccountId(); + Date getRemoved(); Date getCreated(); diff --git a/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java b/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java index d7767721667..3ae0f319189 100644 --- a/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java +++ b/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java @@ -803,6 +803,7 @@ public class ApiConstants { public static final String IPSEC_PSK = "ipsecpsk"; public static final String GUEST_IP = "guestip"; public static final String REMOVED = "removed"; + public static final String REMOVER_ACCOUNT_ID = "removeraccountid"; public static final String REMOVAL_REASON = "removalreason"; public static final String COMPLETED = "completed"; public static final String IKE_VERSION = "ikeversion"; diff --git a/api/src/main/java/org/apache/cloudstack/api/response/IpQuarantineResponse.java b/api/src/main/java/org/apache/cloudstack/api/response/IpQuarantineResponse.java index 55720296315..c93f259382c 100644 --- a/api/src/main/java/org/apache/cloudstack/api/response/IpQuarantineResponse.java +++ b/api/src/main/java/org/apache/cloudstack/api/response/IpQuarantineResponse.java @@ -60,6 +60,10 @@ public class IpQuarantineResponse extends BaseResponse { @Param(description = "The reason for removing the IP from quarantine prematurely.") private String removalReason; + @SerializedName(ApiConstants.REMOVER_ACCOUNT_ID) + @Param(description = "ID of the account that removed the IP from quarantine.") + private String removerAccountId; + public IpQuarantineResponse() { super("quarantinedips"); } @@ -127,4 +131,12 @@ public class IpQuarantineResponse extends BaseResponse { public void setRemovalReason(String removalReason) { this.removalReason = removalReason; } + + public String getRemoverAccountId() { + return removerAccountId; + } + + public void setRemoverAccountId(String removerAccountId) { + this.removerAccountId = removerAccountId; + } } diff --git a/engine/schema/src/main/java/com/cloud/network/vo/PublicIpQuarantineVO.java b/engine/schema/src/main/java/com/cloud/network/vo/PublicIpQuarantineVO.java index 56d167a0060..89e02610bd2 100644 --- a/engine/schema/src/main/java/com/cloud/network/vo/PublicIpQuarantineVO.java +++ b/engine/schema/src/main/java/com/cloud/network/vo/PublicIpQuarantineVO.java @@ -63,6 +63,9 @@ public class PublicIpQuarantineVO implements PublicIpQuarantine { @Column(name = "removal_reason") private String removalReason = null; + @Column(name = "remover_account_id") + private Long removerAccountId = null; + public PublicIpQuarantineVO() { } @@ -98,6 +101,11 @@ public class PublicIpQuarantineVO implements PublicIpQuarantine { return removalReason; } + @Override + public Long getRemoverAccountId() { + return this.removerAccountId; + } + @Override public String getUuid() { return uuid; @@ -111,6 +119,10 @@ public class PublicIpQuarantineVO implements PublicIpQuarantine { this.removalReason = removalReason; } + public void setRemoverAccountId(Long removerAccountId) { + this.removerAccountId = removerAccountId; + } + @Override public Date getRemoved() { return removed; diff --git a/engine/schema/src/main/java/com/cloud/upgrade/dao/Upgrade41810to41900.java b/engine/schema/src/main/java/com/cloud/upgrade/dao/Upgrade41810to41900.java index bdfe58cbf89..13e30c0f6e2 100644 --- a/engine/schema/src/main/java/com/cloud/upgrade/dao/Upgrade41810to41900.java +++ b/engine/schema/src/main/java/com/cloud/upgrade/dao/Upgrade41810to41900.java @@ -77,6 +77,7 @@ public class Upgrade41810to41900 implements DbUpgrade, DbUpgradeSystemVmTemplate decryptConfigurationValuesFromAccountAndDomainScopesNotInSecureHiddenCategories(conn); migrateBackupDates(conn); addIndexes(conn); + addRemoverAccountIdForeignKeyToQuarantinedIps(conn); } @Override @@ -262,4 +263,7 @@ public class Upgrade41810to41900 implements DbUpgrade, DbUpgradeSystemVmTemplate DbUpgradeUtils.addIndexIfNeeded(conn, "event", "resource_type", "resource_id"); } + private void addRemoverAccountIdForeignKeyToQuarantinedIps(Connection conn) { + DbUpgradeUtils.addForeignKey(conn, "quarantined_ips", "remover_account_id", "account", "id"); + } } diff --git a/engine/schema/src/main/resources/META-INF/db/schema-41810to41900.sql b/engine/schema/src/main/resources/META-INF/db/schema-41810to41900.sql index 27170fcac14..308d48a311c 100644 --- a/engine/schema/src/main/resources/META-INF/db/schema-41810to41900.sql +++ b/engine/schema/src/main/resources/META-INF/db/schema-41810to41900.sql @@ -314,3 +314,6 @@ CREATE TABLE `cloud_usage`.`bucket_statistics` ( `size` bigint unsigned COMMENT 'total size of bucket objects', PRIMARY KEY(`id`) ) ENGINE=InnoDB DEFAULT CHARSET=utf8; + +-- Add remover account ID to quarantined IPs table. +CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.quarantined_ips', 'remover_account_id', 'bigint(20) unsigned DEFAULT NULL COMMENT "ID of the account that removed the IP from quarantine, foreign key to `account` table"'); diff --git a/server/src/main/java/com/cloud/api/ApiResponseHelper.java b/server/src/main/java/com/cloud/api/ApiResponseHelper.java index 5424a36cdd8..e2b72f6175c 100644 --- a/server/src/main/java/com/cloud/api/ApiResponseHelper.java +++ b/server/src/main/java/com/cloud/api/ApiResponseHelper.java @@ -5200,6 +5200,10 @@ public class ApiResponseHelper implements ResponseGenerator { quarantinedIpsResponse.setRemoved(quarantinedIp.getRemoved()); quarantinedIpsResponse.setEndDate(quarantinedIp.getEndDate()); quarantinedIpsResponse.setRemovalReason(quarantinedIp.getRemovalReason()); + if (quarantinedIp.getRemoverAccountId() != null) { + Account removerAccount = _accountMgr.getAccount(quarantinedIp.getRemoverAccountId()); + quarantinedIpsResponse.setRemoverAccountId(removerAccount.getUuid()); + } quarantinedIpsResponse.setResponseName("quarantinedip"); return quarantinedIpsResponse; diff --git a/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java b/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java index 75ea572491e..b5908935350 100644 --- a/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java +++ b/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java @@ -2471,9 +2471,11 @@ public class IpAddressManagerImpl extends ManagerBase implements IpAddressManage PublicIpQuarantineVO publicIpQuarantineVO = publicIpQuarantineDao.findById(quarantineProcessId); Ip ipAddress = _ipAddressDao.findById(publicIpQuarantineVO.getPublicIpAddressId()).getAddress(); Date removedDate = new Date(); + Long removerAccountId = CallContext.current().getCallingAccountId(); publicIpQuarantineVO.setRemoved(removedDate); publicIpQuarantineVO.setRemovalReason(removalReason); + publicIpQuarantineVO.setRemoverAccountId(removerAccountId); s_logger.debug(String.format("Removing public IP Address [%s] from quarantine by updating the removed date to [%s].", ipAddress, removedDate)); publicIpQuarantineDao.persist(publicIpQuarantineVO); diff --git a/server/src/test/java/com/cloud/api/ApiResponseHelperTest.java b/server/src/test/java/com/cloud/api/ApiResponseHelperTest.java index f681669c789..1bea3ac78f3 100644 --- a/server/src/test/java/com/cloud/api/ApiResponseHelperTest.java +++ b/server/src/test/java/com/cloud/api/ApiResponseHelperTest.java @@ -17,10 +17,12 @@ package com.cloud.api; import com.cloud.domain.DomainVO; +import com.cloud.network.PublicIpQuarantine; import com.cloud.network.as.AutoScaleVmGroup; import com.cloud.network.as.AutoScaleVmGroupVO; import com.cloud.network.as.AutoScaleVmProfileVO; import com.cloud.network.as.dao.AutoScaleVmGroupVmMapDao; +import com.cloud.network.dao.IPAddressDao; import com.cloud.network.dao.IPAddressVO; import com.cloud.network.dao.LoadBalancerVO; import com.cloud.network.dao.NetworkServiceMapDao; @@ -41,6 +43,7 @@ import org.apache.cloudstack.annotation.dao.AnnotationDao; import org.apache.cloudstack.api.response.AutoScaleVmGroupResponse; import org.apache.cloudstack.api.response.AutoScaleVmProfileResponse; import org.apache.cloudstack.api.response.DirectDownloadCertificateResponse; +import org.apache.cloudstack.api.response.IpQuarantineResponse; import org.apache.cloudstack.api.response.NicSecondaryIpResponse; import org.apache.cloudstack.api.response.UnmanagedInstanceResponse; import org.apache.cloudstack.api.response.UsageRecordResponse; @@ -97,6 +100,9 @@ public class ApiResponseHelperTest { @Mock UserDataDao userDataDaoMock; + @Mock + IPAddressDao ipAddressDaoMock; + @Spy @InjectMocks ApiResponseHelper apiResponseHelper = new ApiResponseHelper(); @@ -396,4 +402,54 @@ public class ApiResponseHelperTest { Assert.assertEquals(1, response.getDisks().size()); Assert.assertEquals(1, response.getNics().size()); } + + @Test + public void createQuarantinedIpsResponseTestReturnsObject() { + String quarantinedIpUuid = "quarantined_ip_uuid"; + Long previousOwnerId = 300L; + String previousOwnerUuid = "previous_owner_uuid"; + String previousOwnerName = "previous_owner_name"; + Long removerAccountId = 400L; + String removerAccountUuid = "remover_account_uuid"; + Long publicIpAddressId = 500L; + String publicIpAddress = "1.2.3.4"; + Date created = new Date(599L); + Date removed = new Date(600L); + Date endDate = new Date(601L); + String removalReason = "removalReason"; + + PublicIpQuarantine quarantinedIpMock = Mockito.mock(PublicIpQuarantine.class); + IPAddressVO ipAddressVoMock = Mockito.mock(IPAddressVO.class); + Account previousOwner = Mockito.mock(Account.class); + Account removerAccount = Mockito.mock(Account.class); + + Mockito.when(quarantinedIpMock.getUuid()).thenReturn(quarantinedIpUuid); + Mockito.when(quarantinedIpMock.getPreviousOwnerId()).thenReturn(previousOwnerId); + Mockito.when(quarantinedIpMock.getPublicIpAddressId()).thenReturn(publicIpAddressId); + Mockito.doReturn(ipAddressVoMock).when(ipAddressDaoMock).findById(publicIpAddressId); + Mockito.when(ipAddressVoMock.getAddress()).thenReturn(new Ip(publicIpAddress)); + Mockito.doReturn(previousOwner).when(accountManagerMock).getAccount(previousOwnerId); + Mockito.when(previousOwner.getUuid()).thenReturn(previousOwnerUuid); + Mockito.when(previousOwner.getName()).thenReturn(previousOwnerName); + Mockito.when(quarantinedIpMock.getCreated()).thenReturn(created); + Mockito.when(quarantinedIpMock.getRemoved()).thenReturn(removed); + Mockito.when(quarantinedIpMock.getEndDate()).thenReturn(endDate); + Mockito.when(quarantinedIpMock.getRemovalReason()).thenReturn(removalReason); + Mockito.when(quarantinedIpMock.getRemoverAccountId()).thenReturn(removerAccountId); + Mockito.when(removerAccount.getUuid()).thenReturn(removerAccountUuid); + Mockito.doReturn(removerAccount).when(accountManagerMock).getAccount(removerAccountId); + + IpQuarantineResponse result = apiResponseHelper.createQuarantinedIpsResponse(quarantinedIpMock); + + Assert.assertEquals(quarantinedIpUuid, result.getId()); + Assert.assertEquals(publicIpAddress, result.getPublicIpAddress()); + Assert.assertEquals(previousOwnerUuid, result.getPreviousOwnerId()); + Assert.assertEquals(previousOwnerName, result.getPreviousOwnerName()); + Assert.assertEquals(created, result.getCreated()); + Assert.assertEquals(removed, result.getRemoved()); + Assert.assertEquals(endDate, result.getEndDate()); + Assert.assertEquals(removalReason, result.getRemovalReason()); + Assert.assertEquals(removerAccountUuid, result.getRemoverAccountId()); + Assert.assertEquals("quarantinedip", result.getResponseName()); + } } diff --git a/server/src/test/java/com/cloud/network/IpAddressManagerTest.java b/server/src/test/java/com/cloud/network/IpAddressManagerTest.java index 935fb4e8c3b..5b8399ad000 100644 --- a/server/src/test/java/com/cloud/network/IpAddressManagerTest.java +++ b/server/src/test/java/com/cloud/network/IpAddressManagerTest.java @@ -36,12 +36,14 @@ import com.cloud.network.dao.PublicIpQuarantineDao; import com.cloud.network.vo.PublicIpQuarantineVO; import com.cloud.user.Account; import com.cloud.user.AccountManager; +import org.apache.cloudstack.context.CallContext; import org.junit.Assert; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.InjectMocks; import org.mockito.Mock; +import org.mockito.MockedStatic; import org.mockito.Mockito; import org.mockito.Spy; import org.mockito.runners.MockitoJUnitRunner; @@ -397,6 +399,31 @@ public class IpAddressManagerTest { Assert.assertFalse(result); } + @Test + public void removePublicIpAddressFromQuarantineTestPersistsObject() { + Long quarantineProcessId = 100L; + Long publicAddressId = 200L; + Long callingAccountId = 300L; + String removalReason = "removalReason"; + + try (MockedStatic callContextMockedStatic = Mockito.mockStatic(CallContext.class)) { + Mockito.doReturn(publicIpQuarantineVOMock).when(publicIpQuarantineDaoMock).findById(quarantineProcessId); + Mockito.when(publicIpQuarantineVOMock.getPublicIpAddressId()).thenReturn(publicAddressId); + Mockito.doReturn(ipAddressVO).when(ipAddressDao).findById(publicAddressId); + + CallContext callContextMock = Mockito.mock(CallContext.class); + Mockito.when(callContextMock.getCallingAccountId()).thenReturn(callingAccountId); + callContextMockedStatic.when(CallContext::current).thenReturn(callContextMock); + + ipAddressManager.removePublicIpAddressFromQuarantine(quarantineProcessId, removalReason); + + Mockito.verify(publicIpQuarantineVOMock).setRemoved(Mockito.any()); + Mockito.verify(publicIpQuarantineVOMock).setRemovalReason(removalReason); + Mockito.verify(publicIpQuarantineVOMock).setRemoverAccountId(callingAccountId); + Mockito.verify(publicIpQuarantineDaoMock).persist(publicIpQuarantineVOMock); + } + } + @Test public void updateSourceNatIpAddress() throws Exception { IPAddressVO requestedIp = Mockito.mock(IPAddressVO.class);