jmsperu commented on code in PR #12898:
URL: https://github.com/apache/cloudstack/pull/12898#discussion_r3844114863
##########
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##########
@@ -254,6 +297,32 @@ public Pair<Boolean, Backup> takeBackup(final
VirtualMachine vm, Boolean quiesce
}
}
+ /**
+ * Translates the zone-scoped backup-enhancement settings (compression,
encryption,
+ * bandwidth limit, integrity check) into details on the {@link
TakeBackupCommand}.
+ * Fails fast if encryption is enabled without a configured passphrase.
+ */
+ protected void applyBackupEnhancementDetails(TakeBackupCommand command,
Long zoneId) {
+ if (Boolean.TRUE.equals(NASBackupCompressionEnabled.valueIn(zoneId))) {
+ command.addDetail(TakeBackupCommand.DETAIL_COMPRESSION, "true");
+ }
+ if (Boolean.TRUE.equals(NASBackupEncryptionEnabled.valueIn(zoneId))) {
+ String passphrase = NASBackupEncryptionPassphrase.valueIn(zoneId);
+ if (passphrase == null || passphrase.isEmpty()) {
+ throw new CloudRuntimeException("NAS backup encryption is
enabled but no passphrase is configured (nas.backup.encryption.passphrase)");
+ }
Review Comment:
You're right, and thanks for catching it: the restore side had no way to
open a LUKS image, so an encrypted backup could be taken but neither verified
nor restored. Fixed in 4d919dc:
- `RestoreBackupCommand` carries the zone's passphrase (`@LogLevel(Off)`, as
on the take side); the provider sets it whenever a passphrase is configured, so
backups taken before encryption was switched off still restore.
- `LibvirtRestoreBackupCommandWrapper` probes `qemu-img info --output=json`
for `encrypted: true` and then runs check/convert with `--object secret,...`
and `--image-opts driver=qcow2,...,encrypt.key-secret=sec0`. File-based pools
are decrypted during a qcow2 convert instead of being rsync'd (a copied LUKS
volume would be unbootable); RBD/LINSTOR use the same secret on the raw convert.
- An encrypted backup with no passphrase configured fails with an explicit
message rather than an opaque qemu-img error.
- `NasBackupPassphraseFile` is the shared 0600 temp key file helper for both
wrappers.
Unit tests cover the encrypted check/convert path, the missing-passphrase
failure and the provider side.
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -87,6 +91,52 @@ sanity_checks() {
log -ne "Environment Sanity Checks successfully passed"
}
+encrypt_backup() {
+ local backup_dir="$1"
+ if [[ -z "$ENCRYPT_PASSFILE" ]]; then
+ return
+ fi
+ if [[ ! -f "$ENCRYPT_PASSFILE" ]]; then
+ echo "Encryption passphrase file not found: $ENCRYPT_PASSFILE"
+ exit 1
+ fi
Review Comment:
Done: encrypt_backup() returns 1 and both callers run cleanup() before
returning.
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -87,6 +91,52 @@ sanity_checks() {
log -ne "Environment Sanity Checks successfully passed"
}
+encrypt_backup() {
+ local backup_dir="$1"
+ if [[ -z "$ENCRYPT_PASSFILE" ]]; then
+ return
+ fi
+ if [[ ! -f "$ENCRYPT_PASSFILE" ]]; then
+ echo "Encryption passphrase file not found: $ENCRYPT_PASSFILE"
+ exit 1
+ fi
+ log -ne "Encrypting backup files with LUKS"
+ for img in "$backup_dir"/*.qcow2; do
+ [[ -f "$img" ]] || continue
+ local tmp_img="${img}.luks"
+ if qemu-img convert -O qcow2 \
+ --object "secret,id=sec0,file=$ENCRYPT_PASSFILE" \
+ -o "encrypt.format=luks,encrypt.key-secret=sec0" \
+ "$img" "$tmp_img" 2>&1 | tee -a "$logFile"; then
+ mv "$tmp_img" "$img"
+ log -ne "Encrypted: $img"
+ else
+ echo "Encryption failed for $img"
+ rm -f "$tmp_img"
+ exit 1
+ fi
+ done
+}
+
+verify_backup() {
+ local backup_dir="$1"
+ local failed=0
+ for img in "$backup_dir"/*.qcow2; do
+ [[ -f "$img" ]] || continue
+ if ! qemu-img check "$img" > /dev/null 2>&1; then
+ echo "Backup verification failed for $img"
+ log -ne "Backup verification FAILED: $img"
+ failed=1
+ else
+ log -ne "Backup verification passed: $img"
+ fi
+ done
+ if [[ $failed -ne 0 ]]; then
+ echo "One or more backup files failed verification"
+ exit 1
+ fi
Review Comment:
Done: verify_backup() returns 1 and the callers clean up. It also passes the
LUKS secret to `qemu-img check`, so `--verify` works on encrypted backups.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java:
##########
@@ -68,21 +72,59 @@ public Answer execute(TakeBackupCommand command,
LibvirtComputingResource libvir
}
}
+ List<String> cmdArgs = new ArrayList<>();
+ cmdArgs.add(libvirtComputingResource.getNasBackupPath());
+ cmdArgs.add("-o"); cmdArgs.add("backup");
+ cmdArgs.add("-v"); cmdArgs.add(vmName);
+ cmdArgs.add("-t"); cmdArgs.add(backupRepoType);
+ cmdArgs.add("-s"); cmdArgs.add(backupRepoAddress);
+ cmdArgs.add("-m"); cmdArgs.add(Objects.nonNull(mountOptions) ?
mountOptions : "");
+ cmdArgs.add("-p"); cmdArgs.add(backupPath);
+ cmdArgs.add("-q"); cmdArgs.add(command.getQuiesce() != null &&
command.getQuiesce() ? "true" : "false");
+ cmdArgs.add("-d"); cmdArgs.add(diskPaths.isEmpty() ? "" :
String.join(",", diskPaths));
+
+ // Append optional enhancement flags from management server config
+ File passphraseFile = null;
+ Map<String, String> details = command.getDetails();
+ if (details != null) {
+ if ("true".equals(details.get("compression"))) {
+ cmdArgs.add("-c");
+ }
+ if ("true".equals(details.get("encryption"))) {
+ String passphrase = details.get("encryption_passphrase");
+ if (passphrase != null && !passphrase.isEmpty()) {
+ try {
+ passphraseFile = File.createTempFile("cs-backup-enc-",
".key");
+ passphraseFile.deleteOnExit();
+ try (FileWriter fw = new FileWriter(passphraseFile)) {
+ fw.write(passphrase);
+ }
+ cmdArgs.add("-e");
cmdArgs.add(passphraseFile.getAbsolutePath());
+ } catch (IOException e) {
+ logger.error("Failed to create encryption passphrase
file", e);
+ return new BackupAnswer(command, false, "Failed to
create encryption passphrase file: " + e.getMessage());
+ }
Review Comment:
Done: appendEnhancementFlags() throws BackupConfigException when encryption
is requested without a passphrase, and the command fails with that message
instead of proceeding unencrypted.
##########
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##########
@@ -205,6 +245,26 @@ public Pair<Boolean, Backup> takeBackup(final
VirtualMachine vm, Boolean quiesce
command.setMountOptions(backupRepository.getMountOptions());
command.setQuiesce(quiesceVM);
+ // Pass optional backup enhancement settings from zone-scoped configs
+ Long zoneId = vm.getDataCenterId();
+ if (Boolean.TRUE.equals(NASBackupCompressionEnabled.valueIn(zoneId))) {
+ command.addDetail("compression", "true");
+ }
+ if (Boolean.TRUE.equals(NASBackupEncryptionEnabled.valueIn(zoneId))) {
+ command.addDetail("encryption", "true");
+ String passphrase = NASBackupEncryptionPassphrase.valueIn(zoneId);
+ if (passphrase != null && !passphrase.isEmpty()) {
+ command.addDetail("encryption_passphrase", passphrase);
+ }
+ }
+ Integer bandwidthLimit = NASBackupBandwidthLimitMbps.valueIn(zoneId);
+ if (bandwidthLimit != null && bandwidthLimit > 0) {
+ command.addDetail("bandwidth_limit",
String.valueOf(bandwidthLimit));
+ }
+ if
(Boolean.TRUE.equals(NASBackupIntegrityCheckEnabled.valueIn(zoneId))) {
+ command.addDetail("integrity_check", "true");
+ }
Review Comment:
Done: NASBackupProviderTest asserts the details for compression, bandwidth,
integrity and encryption, and that encryption without a passphrase throws.
--
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]
