Copilot commented on code in PR #13332:
URL: https://github.com/apache/cloudstack/pull/13332#discussion_r3410002663
##########
api/src/main/java/org/apache/cloudstack/backup/BackupManager.java:
##########
@@ -57,9 +58,16 @@ public interface BackupManager extends BackupService,
Configurable, PluggableSer
ConfigKey<String> BackupProviderPlugin = new
ValidatedConfigKey<>("Advanced", String.class,
"backup.framework.provider.plugin",
"dummy",
- "The backup and recovery provider plugin. Valid plugin values:
dummy, veeam, networker and nas",
+ "The backup and recovery provider plugin. Default built-in
plugins: dummy, veeam, networker, nas. Additional plugins can be allowed via
backup.framework.provider.plugin.allowed setting.",
true, ConfigKey.Scope.Zone, BackupFrameworkEnabled.key(), value ->
validateBackupProviderConfig((String)value));
+ ConfigKey<String> AllowedBackupProviderPlugins = new ConfigKey<>(
+ "Advanced", String.class,
+ "backup.framework.provider.plugin.allowed",
+ "dummy,veeam,networker,nas",
+ "Comma-separated list of allowed backup provider plugins.",
+ true, ConfigKey.Scope.Zone);
Review Comment:
`AllowedBackupProviderPlugins` is declared with `ConfigKey.Scope.Zone`, but
`validateBackupProviderConfig` reads it via
`AllowedBackupProviderPlugins.value()`, which (per ConfigKey.value()) always
fetches the **Global** scope. As a result, any per-zone value of
`backup.framework.provider.plugin.allowed` will be ignored during validation,
which is confusing and can block intended zone-specific configuration.
##########
api/src/main/java/org/apache/cloudstack/backup/BackupManager.java:
##########
@@ -254,9 +262,14 @@ static void validateBackupProviderConfig(String value) {
if (value != null && (value.contains(",") || value.trim().contains("
"))) {
throw new IllegalArgumentException("Multiple backup provider
plugins are not supported. Please provide a single plugin value.");
}
- List<String> validPlugins = List.of("dummy", "veeam", "networker",
"nas");
- if (value != null && !validPlugins.contains(value)) {
- throw new IllegalArgumentException("Invalid backup provider
plugin: " + value + ". Valid plugin values are: " + String.join(", ",
validPlugins));
+ String allowed = AllowedBackupProviderPlugins.value();
+ if (allowed != null && value != null) {
+ List<String> validPlugins = Arrays.asList(allowed.split(","));
+ if (!validPlugins.contains(value.trim())) {
+ throw new IllegalArgumentException("Invalid backup provider
plugin: " + value +
+ ". Valid plugin values are: " + allowed +
+ ". You can add more plugins via
backup.framework.provider.plugin.allowed setting.");
+ }
}
Review Comment:
`allowed.split(",")` does not trim entries, so an allowed list like `dummy,
veeam` (common for comma-separated configs) yields `" veeam"` and incorrectly
rejects `veeam`. Also, when `allowed` is null/blank the current guard can skip
validation entirely (null) or produce an unhelpful error message (blank).
Normalize the allowed list by trimming/filtering blanks and always validate
when `value` is non-null.
--
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]
