Skip to content

Commit 05076db

Browse files
jmsperujmsperu
committed
Address Copilot review feedback on NAS backup PR #12898
- nasbackup.sh: Replace exit 1 with return 1 in encrypt_backup and verify_backup so callers can run cleanup before terminating - nasbackup.sh: Append (>>) instead of truncate (>) agent.log in qemu-img convert for stopped VM backups - nasbackup.sh: Add return 1 after cleanup on qemu-img convert failure to stop execution - nasbackup.sh: Callers of encrypt_backup/verify_backup now check return code and run cleanup on failure - LibvirtTakeBackupCommandWrapper: Fail with error when encryption is enabled but passphrase is missing instead of silently skipping - LibvirtTakeBackupCommandWrapper: Delete temp passphrase file in finally block, set 0600 permissions, use explicit UTF-8 charset - NASBackupProvider: Throw CloudRuntimeException when encryption is enabled but passphrase is null/empty - NASBackupProviderTest: Add tests for compression, bandwidth, integrity check, encryption+passphrase, and encryption-without- passphrase failure scenarios - TakeBackupCommand: Add @loglevel(Off) to details field to prevent passphrase leaking in debug logs - TakeBackupCommand: Normalize null to empty HashMap in setDetails
1 parent 2c40fdd commit 05076db

File tree

5 files changed

+263
-29
lines changed

5 files changed

+263
-29
lines changed

‎core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ public class TakeBackupCommand extends Command {
3737
private Boolean quiesce;
3838
@LogLevel(LogLevel.Log4jLevel.Off)
3939
private String mountOptions;
40+
@LogLevel(LogLevel.Log4jLevel.Off)
4041
private Map<String, String> details = new HashMap<>();
4142

4243
public TakeBackupCommand(String vmName, String backupPath) {
@@ -114,7 +115,7 @@ public Map<String, String> getDetails() {
114115
}
115116

116117
public void setDetails(Map<String, String> details) {
117-
this.details = details;
118+
this.details = details != null ? details : new HashMap<>();
118119
}
119120

120121
public void addDetail(String key, String value) {

‎plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -251,11 +251,12 @@ public Pair<Boolean, Backup> takeBackup(final VirtualMachine vm, Boolean quiesce
251251
command.addDetail("compression", "true");
252252
}
253253
if (Boolean.TRUE.equals(NASBackupEncryptionEnabled.valueIn(zoneId))) {
254-
command.addDetail("encryption", "true");
255254
String passphrase = NASBackupEncryptionPassphrase.valueIn(zoneId);
256-
if (passphrase != null && !passphrase.isEmpty()) {
257-
command.addDetail("encryption_passphrase", passphrase);
255+
if (passphrase == null || passphrase.isEmpty()) {
256+
throw new CloudRuntimeException("NAS backup encryption is enabled but no passphrase is configured (nas.backup.encryption.passphrase)");
258257
}
258+
command.addDetail("encryption", "true");
259+
command.addDetail("encryption_passphrase", passphrase);
259260
}
260261
Integer bandwidthLimit = NASBackupBandwidthLimitMbps.valueIn(zoneId);
261262
if (bandwidthLimit != null && bandwidthLimit > 0) {

‎plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java‎

Lines changed: 204 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,14 +19,18 @@
1919
import static org.mockito.ArgumentMatchers.anyLong;
2020
import static org.mockito.Mockito.mock;
2121

22+
import java.lang.reflect.Field;
2223
import java.util.Collections;
2324
import java.util.List;
25+
import java.util.Map;
2426
import java.util.Objects;
2527
import java.util.Optional;
2628

29+
import org.apache.cloudstack.framework.config.ConfigKey;
2730
import org.junit.Assert;
2831
import org.junit.Test;
2932
import org.junit.runner.RunWith;
33+
import org.mockito.ArgumentCaptor;
3034
import org.mockito.InjectMocks;
3135
import org.mockito.Mock;
3236
import org.mockito.Mockito;
@@ -47,6 +51,7 @@
4751
import com.cloud.storage.VolumeVO;
4852
import com.cloud.storage.dao.VolumeDao;
4953
import com.cloud.utils.Pair;
54+
import com.cloud.utils.exception.CloudRuntimeException;
5055
import com.cloud.vm.VMInstanceVO;
5156
import com.cloud.vm.dao.VMInstanceDao;
5257

@@ -349,4 +354,203 @@ public void testGetVMHypervisorHostFallbackToZoneWideKVMHost() {
349354
Mockito.verify(hostDao).findHypervisorHostInCluster(clusterId);
350355
Mockito.verify(resourceManager).findOneRandomRunningHostByHypervisor(Hypervisor.HypervisorType.KVM, zoneId);
351356
}
357+
358+
private void overrideConfigValue(final ConfigKey configKey, final Object value) {
359+
try {
360+
Field f = ConfigKey.class.getDeclaredField("_value");
361+
f.setAccessible(true);
362+
f.set(configKey, value);
363+
} catch (IllegalAccessException | NoSuchFieldException e) {
364+
Assert.fail(e.getMessage());
365+
}
366+
}
367+
368+
private VMInstanceVO setupVmForTakeBackup(Long vmId, Long hostId, Long backupOfferingId,
369+
Long accountId, Long domainId, Long zoneId) {
370+
VMInstanceVO vm = mock(VMInstanceVO.class);
371+
Mockito.when(vm.getId()).thenReturn(vmId);
372+
Mockito.when(vm.getHostId()).thenReturn(hostId);
373+
Mockito.when(vm.getInstanceName()).thenReturn("test-vm");
374+
Mockito.when(vm.getBackupOfferingId()).thenReturn(backupOfferingId);
375+
Mockito.when(vm.getAccountId()).thenReturn(accountId);
376+
Mockito.when(vm.getDomainId()).thenReturn(domainId);
377+
Mockito.when(vm.getDataCenterId()).thenReturn(zoneId);
378+
Mockito.when(vm.getState()).thenReturn(VMInstanceVO.State.Running);
379+
return vm;
380+
}
381+
382+
private void setupHostAndRepo(Long hostId, Long backupOfferingId) {
383+
BackupRepository backupRepository = mock(BackupRepository.class);
384+
Mockito.when(backupRepository.getType()).thenReturn("nfs");
385+
Mockito.when(backupRepository.getAddress()).thenReturn("address");
386+
Mockito.when(backupRepository.getMountOptions()).thenReturn("sync");
387+
Mockito.when(backupRepositoryDao.findByBackupOfferingId(backupOfferingId)).thenReturn(backupRepository);
388+
389+
HostVO host = mock(HostVO.class);
390+
Mockito.when(host.getId()).thenReturn(hostId);
391+
Mockito.when(host.getStatus()).thenReturn(Status.Up);
392+
Mockito.when(host.getHypervisorType()).thenReturn(Hypervisor.HypervisorType.KVM);
393+
Mockito.when(hostDao.findById(hostId)).thenReturn(host);
394+
}
395+
396+
@Test
397+
public void testTakeBackupDetailsCompressionEnabled() throws AgentUnavailableException, OperationTimedoutException {
398+
Long vmId = 1L; Long hostId = 2L; Long backupOfferingId = 3L;
399+
Long accountId = 4L; Long domainId = 5L; Long zoneId = 6L;
400+
401+
VMInstanceVO vm = setupVmForTakeBackup(vmId, hostId, backupOfferingId, accountId, domainId, zoneId);
402+
setupHostAndRepo(hostId, backupOfferingId);
403+
404+
VolumeVO volume = mock(VolumeVO.class);
405+
Mockito.when(volume.getState()).thenReturn(Volume.State.Ready);
406+
Mockito.when(volume.getSize()).thenReturn(100L);
407+
Mockito.when(volumeDao.findByInstance(vmId)).thenReturn(List.of(volume));
408+
409+
overrideConfigValue(nasBackupProvider.NASBackupCompressionEnabled, "true");
410+
411+
BackupAnswer answer = mock(BackupAnswer.class);
412+
Mockito.when(answer.getResult()).thenReturn(true);
413+
Mockito.when(answer.getSize()).thenReturn(100L);
414+
415+
ArgumentCaptor<TakeBackupCommand> cmdCaptor = ArgumentCaptor.forClass(TakeBackupCommand.class);
416+
Mockito.when(agentManager.send(anyLong(), cmdCaptor.capture())).thenReturn(answer);
417+
Mockito.when(backupDao.persist(Mockito.any(BackupVO.class))).thenAnswer(invocation -> invocation.getArgument(0));
418+
Mockito.when(backupDao.update(Mockito.anyLong(), Mockito.any(BackupVO.class))).thenReturn(true);
419+
420+
nasBackupProvider.takeBackup(vm, false);
421+
422+
TakeBackupCommand capturedCmd = cmdCaptor.getValue();
423+
Map<String, String> details = capturedCmd.getDetails();
424+
Assert.assertEquals("true", details.get("compression"));
425+
426+
// Reset config
427+
overrideConfigValue(nasBackupProvider.NASBackupCompressionEnabled, "false");
428+
}
429+
430+
@Test
431+
public void testTakeBackupDetailsBandwidthLimit() throws AgentUnavailableException, OperationTimedoutException {
432+
Long vmId = 1L; Long hostId = 2L; Long backupOfferingId = 3L;
433+
Long accountId = 4L; Long domainId = 5L; Long zoneId = 6L;
434+
435+
VMInstanceVO vm = setupVmForTakeBackup(vmId, hostId, backupOfferingId, accountId, domainId, zoneId);
436+
setupHostAndRepo(hostId, backupOfferingId);
437+
438+
VolumeVO volume = mock(VolumeVO.class);
439+
Mockito.when(volume.getState()).thenReturn(Volume.State.Ready);
440+
Mockito.when(volume.getSize()).thenReturn(100L);
441+
Mockito.when(volumeDao.findByInstance(vmId)).thenReturn(List.of(volume));
442+
443+
overrideConfigValue(nasBackupProvider.NASBackupBandwidthLimitMbps, "50");
444+
445+
BackupAnswer answer = mock(BackupAnswer.class);
446+
Mockito.when(answer.getResult()).thenReturn(true);
447+
Mockito.when(answer.getSize()).thenReturn(100L);
448+
449+
ArgumentCaptor<TakeBackupCommand> cmdCaptor = ArgumentCaptor.forClass(TakeBackupCommand.class);
450+
Mockito.when(agentManager.send(anyLong(), cmdCaptor.capture())).thenReturn(answer);
451+
Mockito.when(backupDao.persist(Mockito.any(BackupVO.class))).thenAnswer(invocation -> invocation.getArgument(0));
452+
Mockito.when(backupDao.update(Mockito.anyLong(), Mockito.any(BackupVO.class))).thenReturn(true);
453+
454+
nasBackupProvider.takeBackup(vm, false);
455+
456+
TakeBackupCommand capturedCmd = cmdCaptor.getValue();
457+
Map<String, String> details = capturedCmd.getDetails();
458+
Assert.assertEquals("50", details.get("bandwidth_limit"));
459+
460+
overrideConfigValue(nasBackupProvider.NASBackupBandwidthLimitMbps, "0");
461+
}
462+
463+
@Test
464+
public void testTakeBackupDetailsIntegrityCheck() throws AgentUnavailableException, OperationTimedoutException {
465+
Long vmId = 1L; Long hostId = 2L; Long backupOfferingId = 3L;
466+
Long accountId = 4L; Long domainId = 5L; Long zoneId = 6L;
467+
468+
VMInstanceVO vm = setupVmForTakeBackup(vmId, hostId, backupOfferingId, accountId, domainId, zoneId);
469+
setupHostAndRepo(hostId, backupOfferingId);
470+
471+
VolumeVO volume = mock(VolumeVO.class);
472+
Mockito.when(volume.getState()).thenReturn(Volume.State.Ready);
473+
Mockito.when(volume.getSize()).thenReturn(100L);
474+
Mockito.when(volumeDao.findByInstance(vmId)).thenReturn(List.of(volume));
475+
476+
overrideConfigValue(nasBackupProvider.NASBackupIntegrityCheckEnabled, "true");
477+
478+
BackupAnswer answer = mock(BackupAnswer.class);
479+
Mockito.when(answer.getResult()).thenReturn(true);
480+
Mockito.when(answer.getSize()).thenReturn(100L);
481+
482+
ArgumentCaptor<TakeBackupCommand> cmdCaptor = ArgumentCaptor.forClass(TakeBackupCommand.class);
483+
Mockito.when(agentManager.send(anyLong(), cmdCaptor.capture())).thenReturn(answer);
484+
Mockito.when(backupDao.persist(Mockito.any(BackupVO.class))).thenAnswer(invocation -> invocation.getArgument(0));
485+
Mockito.when(backupDao.update(Mockito.anyLong(), Mockito.any(BackupVO.class))).thenReturn(true);
486+
487+
nasBackupProvider.takeBackup(vm, false);
488+
489+
TakeBackupCommand capturedCmd = cmdCaptor.getValue();
490+
Map<String, String> details = capturedCmd.getDetails();
491+
Assert.assertEquals("true", details.get("integrity_check"));
492+
493+
overrideConfigValue(nasBackupProvider.NASBackupIntegrityCheckEnabled, "false");
494+
}
495+
496+
@Test
497+
public void testTakeBackupDetailsEncryptionWithPassphrase() throws AgentUnavailableException, OperationTimedoutException {
498+
Long vmId = 1L; Long hostId = 2L; Long backupOfferingId = 3L;
499+
Long accountId = 4L; Long domainId = 5L; Long zoneId = 6L;
500+
501+
VMInstanceVO vm = setupVmForTakeBackup(vmId, hostId, backupOfferingId, accountId, domainId, zoneId);
502+
setupHostAndRepo(hostId, backupOfferingId);
503+
504+
VolumeVO volume = mock(VolumeVO.class);
505+
Mockito.when(volume.getState()).thenReturn(Volume.State.Ready);
506+
Mockito.when(volume.getSize()).thenReturn(100L);
507+
Mockito.when(volumeDao.findByInstance(vmId)).thenReturn(List.of(volume));
508+
509+
overrideConfigValue(nasBackupProvider.NASBackupEncryptionEnabled, "true");
510+
overrideConfigValue(nasBackupProvider.NASBackupEncryptionPassphrase, "my-secret-passphrase");
511+
512+
BackupAnswer answer = mock(BackupAnswer.class);
513+
Mockito.when(answer.getResult()).thenReturn(true);
514+
Mockito.when(answer.getSize()).thenReturn(100L);
515+
516+
ArgumentCaptor<TakeBackupCommand> cmdCaptor = ArgumentCaptor.forClass(TakeBackupCommand.class);
517+
Mockito.when(agentManager.send(anyLong(), cmdCaptor.capture())).thenReturn(answer);
518+
Mockito.when(backupDao.persist(Mockito.any(BackupVO.class))).thenAnswer(invocation -> invocation.getArgument(0));
519+
Mockito.when(backupDao.update(Mockito.anyLong(), Mockito.any(BackupVO.class))).thenReturn(true);
520+
521+
nasBackupProvider.takeBackup(vm, false);
522+
523+
TakeBackupCommand capturedCmd = cmdCaptor.getValue();
524+
Map<String, String> details = capturedCmd.getDetails();
525+
Assert.assertEquals("true", details.get("encryption"));
526+
Assert.assertEquals("my-secret-passphrase", details.get("encryption_passphrase"));
527+
528+
overrideConfigValue(nasBackupProvider.NASBackupEncryptionEnabled, "false");
529+
overrideConfigValue(nasBackupProvider.NASBackupEncryptionPassphrase, "");
530+
}
531+
532+
@Test(expected = CloudRuntimeException.class)
533+
public void testTakeBackupEncryptionWithoutPassphraseThrows() throws AgentUnavailableException, OperationTimedoutException {
534+
Long vmId = 1L; Long hostId = 2L; Long backupOfferingId = 3L;
535+
Long accountId = 4L; Long domainId = 5L; Long zoneId = 6L;
536+
537+
VMInstanceVO vm = setupVmForTakeBackup(vmId, hostId, backupOfferingId, accountId, domainId, zoneId);
538+
setupHostAndRepo(hostId, backupOfferingId);
539+
540+
VolumeVO volume = mock(VolumeVO.class);
541+
Mockito.when(volume.getState()).thenReturn(Volume.State.Ready);
542+
Mockito.when(volume.getSize()).thenReturn(100L);
543+
Mockito.when(volumeDao.findByInstance(vmId)).thenReturn(List.of(volume));
544+
545+
overrideConfigValue(nasBackupProvider.NASBackupEncryptionEnabled, "true");
546+
overrideConfigValue(nasBackupProvider.NASBackupEncryptionPassphrase, "");
547+
548+
Mockito.when(backupDao.persist(Mockito.any(BackupVO.class))).thenAnswer(invocation -> invocation.getArgument(0));
549+
550+
try {
551+
nasBackupProvider.takeBackup(vm, false);
552+
} finally {
553+
overrideConfigValue(nasBackupProvider.NASBackupEncryptionEnabled, "false");
554+
}
555+
}
352556
}

‎plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java‎

Lines changed: 32 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -35,10 +35,16 @@
3535
import org.apache.cloudstack.storage.to.PrimaryDataStoreTO;
3636

3737
import java.io.File;
38-
import java.io.FileWriter;
3938
import java.io.IOException;
39+
import java.io.OutputStreamWriter;
40+
import java.io.FileOutputStream;
41+
import java.io.Writer;
42+
import java.nio.charset.StandardCharsets;
43+
import java.nio.file.Files;
44+
import java.nio.file.attribute.PosixFilePermission;
4045
import java.util.ArrayList;
4146
import java.util.Arrays;
47+
import java.util.EnumSet;
4248
import java.util.List;
4349
import java.util.Map;
4450
import java.util.Objects;
@@ -92,18 +98,24 @@ public Answer execute(TakeBackupCommand command, LibvirtComputingResource libvir
9298
}
9399
if ("true".equals(details.get("encryption"))) {
94100
String passphrase = details.get("encryption_passphrase");
95-
if (passphrase != null && !passphrase.isEmpty()) {
96-
try {
97-
passphraseFile = File.createTempFile("cs-backup-enc-", ".key");
98-
passphraseFile.deleteOnExit();
99-
try (FileWriter fw = new FileWriter(passphraseFile)) {
100-
fw.write(passphrase);
101-
}
102-
cmdArgs.add("-e"); cmdArgs.add(passphraseFile.getAbsolutePath());
103-
} catch (IOException e) {
104-
logger.error("Failed to create encryption passphrase file", e);
105-
return new BackupAnswer(command, false, "Failed to create encryption passphrase file: " + e.getMessage());
101+
if (passphrase == null || passphrase.isEmpty()) {
102+
return new BackupAnswer(command, false, "Encryption is enabled but no passphrase was provided");
103+
}
104+
try {
105+
passphraseFile = File.createTempFile("cs-backup-enc-", ".key");
106+
passphraseFile.deleteOnExit();
107+
Files.setPosixFilePermissions(passphraseFile.toPath(),
108+
EnumSet.of(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE));
109+
try (Writer fw = new OutputStreamWriter(new FileOutputStream(passphraseFile), StandardCharsets.UTF_8)) {
110+
fw.write(passphrase);
111+
}
112+
cmdArgs.add("-e"); cmdArgs.add(passphraseFile.getAbsolutePath());
113+
} catch (IOException e) {
114+
logger.error("Failed to create encryption passphrase file", e);
115+
if (passphraseFile != null && passphraseFile.exists()) {
116+
passphraseFile.delete();
106117
}
118+
return new BackupAnswer(command, false, "Failed to create encryption passphrase file: " + e.getMessage());
107119
}
108120
}
109121
String bwLimit = details.get("bandwidth_limit");
@@ -118,11 +130,14 @@ public Answer execute(TakeBackupCommand command, LibvirtComputingResource libvir
118130
List<String[]> commands = new ArrayList<>();
119131
commands.add(cmdArgs.toArray(new String[0]));
120132

121-
Pair<Integer, String> result = Script.executePipedCommands(commands, libvirtComputingResource.getCmdsTimeout());
122-
123-
// Clean up passphrase file after backup completes
124-
if (passphraseFile != null && passphraseFile.exists()) {
125-
passphraseFile.delete();
133+
Pair<Integer, String> result;
134+
try {
135+
result = Script.executePipedCommands(commands, libvirtComputingResource.getCmdsTimeout());
136+
} finally {
137+
// Clean up passphrase file after backup completes
138+
if (passphraseFile != null && passphraseFile.exists()) {
139+
passphraseFile.delete();
140+
}
126141
}
127142

128143
if (result.first() != 0) {

0 commit comments

Comments
 (0)