[pve-devel] [PATCH storage] bwlimit: apply limits when there's no override
Wolfgang Bumiller
w.bumiller at proxmox.com
Fri Feb 9 09:35:59 CET 2018
Before, 'undef' was equivalent to unlimited, but '0' is the
"explicitly unlimited" value, so if the user doesn't request
an override, apply limits as if the user was unprivileged
(otherwise there's no way for privileged users to explicitly
ask to not override the configured limits).
Signed-off-by: Wolfgang Bumiller <w.bumiller at proxmox.com>
---
PVE/Storage.pm | 13 ++++----
test/run_bwlimit_tests.pl | 75 ++++++++++++++++++++++++++++++-----------------
2 files changed, 56 insertions(+), 32 deletions(-)
diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index 995ebd3..143ed2e 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -1562,8 +1562,11 @@ sub get_bandwidth_limit {
$use_global_limits = 1;
};
- my $rpcenv = PVE::RPCEnvironment->get();
- my $authuser = $rpcenv->get_user();
+ my ($rpcenv, $authuser);
+ if (defined($override)) {
+ $rpcenv = PVE::RPCEnvironment->get();
+ $authuser = $rpcenv->get_user();
+ }
# Apply per-storage limits - if there are storages involved.
if (@$storage_list) {
@@ -1573,7 +1576,7 @@ sub get_bandwidth_limit {
# limits, therefore it also allows us to override them.
# Since we have most likely multiple storages to check, do a quick check on
# the general '/storage' path to see if we can skip the checks entirely:
- return $override if $rpcenv->check($authuser, '/storage', ['Datastore.Allocate'], 1);
+ return $override if $rpcenv && $rpcenv->check($authuser, '/storage', ['Datastore.Allocate'], 1);
my %done;
foreach my $storage (@$storage_list) {
@@ -1582,7 +1585,7 @@ sub get_bandwidth_limit {
$done{$storage} = 1;
# Otherwise we may still have individual /storage/$ID permissions:
- if (!$rpcenv->check($authuser, "/storage/$storage", ['Datastore.Allocate'], 1)) {
+ if (!$rpcenv || !$rpcenv->check($authuser, "/storage/$storage", ['Datastore.Allocate'], 1)) {
# And if not: apply the limits.
my $storecfg = storage_config($config, $storage);
$apply_limit->($storecfg->{bwlimit});
@@ -1596,7 +1599,7 @@ sub get_bandwidth_limit {
# Sys.Modify on '/' means we can change datacenter.cfg which contains the
# global default limits.
- if (!$rpcenv->check($authuser, '/', ['Sys.Modify'], 1)) {
+ if (!$rpcenv || !$rpcenv->check($authuser, '/', ['Sys.Modify'], 1)) {
# So if we cannot modify global limits, apply them to our currently
# requested override.
my $dc = cfs_read_file('datacenter.cfg');
diff --git a/test/run_bwlimit_tests.pl b/test/run_bwlimit_tests.pl
index e4f723f..a835dc9 100755
--- a/test/run_bwlimit_tests.pl
+++ b/test/run_bwlimit_tests.pl
@@ -95,12 +95,18 @@ my $rpcenv = PVE::RPCEnvironment->init('pub');
my @tests = (
[ user => 'root at pam' ],
- [ ['unknown', ['nolimit'], undef], undef, 'root / generic default limit' ],
- [ ['move', ['nolimit'], undef], undef, 'root / specific default limit (move)' ],
- [ ['restore', ['nolimit'], undef], undef, 'root / specific default limit (restore)' ],
- [ ['unknown', ['d50m40r30'], undef], undef, 'root / storage default limit' ],
- [ ['move', ['d50m40r30'], undef], undef, 'root / specific storage limit (move)' ],
- [ ['restore', ['d50m40r30'], undef], undef, 'root / specific storage limit (restore)' ],
+ [ ['unknown', ['nolimit'], undef], 100, 'root / generic default limit, requesting default' ],
+ [ ['move', ['nolimit'], undef], 80, 'root / specific default limit, requesting default (move)' ],
+ [ ['restore', ['nolimit'], undef], 60, 'root / specific default limit, requesting default (restore)' ],
+ [ ['unknown', ['d50m40r30'], undef], 50, 'root / storage default limit' ],
+ [ ['move', ['d50m40r30'], undef], 40, 'root / specific storage limit (move)' ],
+ [ ['restore', ['d50m40r30'], undef], 30, 'root / specific storage limit (restore)' ],
+ [ ['unknown', ['nolimit'], 0], 0, 'root / generic default limit' ],
+ [ ['move', ['nolimit'], 0], 0, 'root / specific default limit (move)' ],
+ [ ['restore', ['nolimit'], 0], 0, 'root / specific default limit (restore)' ],
+ [ ['unknown', ['d50m40r30'], 0], 0, 'root / storage default limit' ],
+ [ ['move', ['d50m40r30'], 0], 0, 'root / specific storage limit (move)' ],
+ [ ['restore', ['d50m40r30'], 0], 0, 'root / specific storage limit (restore)' ],
[ user => 'user1 at test' ],
[ ['unknown', ['nolimit'], undef], 100, 'generic default limit' ],
@@ -118,23 +124,30 @@ my @tests = (
[ user => 'user2 at test' ],
[ ['unknown', ['nolimit'], 0], 0, 'generic default limit with Sys.Modify, passing unlimited' ],
- [ ['unknown', ['nolimit'], undef], undef, 'generic default limit with Sys.Modify' ],
- [ ['restore', ['nolimit'], undef], undef, 'specific default limit with Sys.Modify (restore)' ],
- [ ['move', ['nolimit'], undef], undef, 'specific default limit with Sys.Modify (move)' ],
+ [ ['unknown', ['nolimit'], undef], 100, 'generic default limit with Sys.Modify' ],
+ [ ['move', ['nolimit'], undef], 80, 'specific default limit with Sys.Modify (move)' ],
+ [ ['restore', ['nolimit'], undef], 60, 'specific default limit with Sys.Modify (restore)' ],
+ [ ['restore', ['nolimit'], 0], 0, 'specific default limit with Sys.Modify, passing unlimited (restore)' ],
+ [ ['move', ['nolimit'], 0], 0, 'specific default limit with Sys.Modify, passing unlimited (move)' ],
[ ['unknown', ['d50m40r30'], undef], 50, 'storage default limit with Sys.Modify' ],
- [ ['move', ['d50m40r30'], undef], 40, 'specific storage limit with Sys.Modify (move)' ],
[ ['restore', ['d50m40r30'], undef], 30, 'specific storage limit with Sys.Modify (restore)' ],
+ [ ['move', ['d50m40r30'], undef], 40, 'specific storage limit with Sys.Modify (move)' ],
[ user => 'user3 at test' ],
+ [ ['unknown', ['nolimit'], undef], 100, 'generic default limit with privileges on /' ],
[ ['unknown', ['nolimit'], 80], 80, 'generic default limit with privileges on /, passing an override value' ],
[ ['unknown', ['nolimit'], 0], 0, 'generic default limit with privileges on /, passing unlimited' ],
- [ ['unknown', ['nolimit'], undef], undef, 'generic default limit with privileges on /' ],
- [ ['move', ['nolimit'], undef], undef, 'specific default limit with privileges on / (move)' ],
- [ ['restore', ['nolimit'], undef], undef, 'specific default limit with privileges on / (restore)' ],
- [ ['unknown', ['d50m20r20'], 0], 0, 'storage default limit with privileges on /, passing unlimited' ],
- [ ['unknown', ['d50m20r20'], undef], undef, 'storage default limit with privileges on /' ],
- [ ['move', ['d50m20r20'], undef], undef, 'specific storage limit with privileges on / (move)' ],
- [ ['restore', ['d50m20r20'], undef], undef, 'specific storage limit with privileges on / (restore)' ],
+ [ ['move', ['nolimit'], undef], 80, 'specific default limit with privileges on / (move)' ],
+ [ ['move', ['nolimit'], 0], 0, 'specific default limit with privileges on /, passing unlimited (move)' ],
+ [ ['restore', ['nolimit'], undef], 60, 'specific default limit with privileges on / (restore)' ],
+ [ ['restore', ['nolimit'], 0], 0, 'specific default limit with privileges on /, passing unlimited (restore)' ],
+ [ ['unknown', ['d50m40r30'], 0], 0, 'storage default limit with privileges on /, passing unlimited' ],
+ [ ['unknown', ['d50m40r30'], undef], 50, 'storage default limit with privileges on /' ],
+ [ ['unknown', ['d50m40r30'], 0], 0, 'storage default limit with privileges on, passing unlimited /' ],
+ [ ['move', ['d50m40r30'], undef], 40, 'specific storage limit with privileges on / (move)' ],
+ [ ['move', ['d50m40r30'], 0], 0, 'specific storage limit with privileges on, passing unlimited / (move)' ],
+ [ ['restore', ['d50m40r30'], undef], 30, 'specific storage limit with privileges on / (restore)' ],
+ [ ['restore', ['d50m40r30'], 0], 0, 'specific storage limit with privileges on /, passing unlimited (restore)' ],
[ user => 'user4 at test' ],
[ ['unknown', ['nolimit'], 10], 10, 'generic default limit with privileges on a different storage, passing lower override' ],
@@ -145,13 +158,18 @@ my @tests = (
[ ['unknown', ['d50m40r30'], undef], 50, 'storage default limit with privileges on a different storage' ],
[ ['move', ['d50m40r30'], undef], 40, 'specific storage limit with privileges on a different storage (move)' ],
[ ['restore', ['d50m40r30'], undef], 30, 'specific storage limit with privileges on a different storage (restore)' ],
- [ ['unknown', ['d20m40r30'], undef], undef, 'storage default limit with privileges on that storage' ],
+ [ ['unknown', ['d20m40r30'], undef], 20, 'storage default limit with privileges on that storage' ],
[ ['unknown', ['d20m40r30'], 0], 0, 'storage default limit with privileges on that storage, passing unlimited' ],
- [ ['move', ['d20m40r30'], undef], undef, 'specific storage limit with privileges on that storage (move)' ],
- [ ['restore', ['d20m40r30'], undef], undef, 'specific storage limit with privileges on that storage (restore)' ],
- [ ['unknown', ['d50m40r30', 'd20m40r30'], undef], 50, 'multiple storages default limit with privileges on one of them' ],
- [ ['move', ['d50m40r30', 'd20m40r30'], undef], 40, 'multiple storages specific limit with privileges on one of them (move)' ],
- [ ['restore', ['d50m40r30', 'd20m40r30'], undef], 30, 'multiple storages specific limit with privileges on one of them (restore)' ],
+ [ ['move', ['d20m40r30'], undef], 40, 'specific storage limit with privileges on that storage (move)' ],
+ [ ['move', ['d20m40r30'], 0], 0, 'specific storage limit with privileges on that storage, passing unlimited (move)' ],
+ [ ['move', ['d20m40r30'], 10], 10, 'specific storage limit with privileges on that storage, passing low override (move)' ],
+ [ ['move', ['d20m40r30'], 300], 300, 'specific storage limit with privileges on that storage, passing high override (move)' ],
+ [ ['restore', ['d20m40r30'], undef], 30, 'specific storage limit with privileges on that storage (restore)' ],
+ [ ['restore', ['d20m40r30'], 0], 0, 'specific storage limit with privileges on that storage, passing unlimited (restore)' ],
+ [ ['unknown', ['d50m40r30', 'd20m40r30'], 0], 50, 'multiple storages default limit with privileges on one of them, passing unlimited' ],
+ [ ['move', ['d50m40r30', 'd20m40r30'], 0], 40, 'multiple storages specific limit with privileges on one of them, passing unlimited (move)' ],
+ [ ['restore', ['d50m40r30', 'd20m40r30'], 0], 30, 'multiple storages specific limit with privileges on one of them, passing unlimited (restore)' ],
+ [ ['unknown', ['d50m40r30', 'd20m40r30'], undef], 20, 'multiple storages default limit with privileges on one of them' ],
[ ['unknown', ['d10', 'd20m40r30'], undef], 10, 'multiple storages default limit with privileges on one of them (storage limited)' ],
[ ['move', ['d10', 'd20m40r30'], undef], 10, 'multiple storages specific limit with privileges on one of them (storage limited) (move)' ],
[ ['restore', ['d10', 'd20m40r30'], undef], 10, 'multiple storages specific limit with privileges on one of them (storage limited) (restore)' ],
@@ -161,10 +179,13 @@ my @tests = (
[ ['restore', ['d200', 'd200m400r300'], 0], 200, 'multiple storages specific limit (storage limited) (restore), passing unlimited' ],
[ ['restore', ['d200', 'd200m400r300'], 1], 1, 'multiple storages specific limit (storage limited) (restore), passing 1' ],
[ ['restore', ['d10', 'd20m40r30'], 500], 10, 'multiple storages specific limit with privileges on one of them (storage limited) (restore), passing higher override' ],
- [ ['unknown', ['nolimit', 'd20m40r30'], undef], 100, 'multiple storages default limit with privileges on one of them (default limited)' ],
- [ ['move', ['nolimit', 'd20m40r30'], undef], 80, 'multiple storages specific limit with privileges on one of them (default limited) (move)' ],
- [ ['restore', ['nolimit', 'd20m40r30'], undef], 60, 'multiple storages specific limit with privileges on one of them (default limited) (restore)' ],
- [ ['restore', ['d20m40r30', 'm50'], 200], 60, 'multiple storages specific limit with privileges on one of them (default limited) (restore)' ],
+ [ ['unknown', ['nolimit', 'd20m40r30'], 0], 100, 'multiple storages default limit with privileges on one of them, passing unlimited (default limited)' ],
+ [ ['move', ['nolimit', 'd20m40r30'], 0], 80, 'multiple storages specific limit with privileges on one of them, passing unlimited (default limited) (move)' ],
+ [ ['restore', ['nolimit', 'd20m40r30'], 0], 60, 'multiple storages specific limit with privileges on one of them, passing unlimited (default limited) (restore)' ],
+ [ ['unknown', ['nolimit', 'd20m40r30'], undef], 20, 'multiple storages default limit with privileges on one of them (default limited)' ],
+ [ ['move', ['nolimit', 'd20m40r30'], undef], 40, 'multiple storages specific limit with privileges on one of them (default limited) (move)' ],
+ [ ['restore', ['nolimit', 'd20m40r30'], undef], 30, 'multiple storages specific limit with privileges on one of them (default limited) (restore)' ],
+ [ ['restore', ['d20m40r30', 'm50'], 200], 60, 'multiple storages specific limit with privileges on one of them (global default limited) (restore)' ],
);
foreach my $t (@tests) {
--
2.11.0
More information about the pve-devel
mailing list