Re: [pve-devel] [PATCH container] create_vm: fix order of config creation/reading/locking
On April 29, 2020 11:58 am, Fabian Ebner wrote:
> The update_pct_config call leads to a write_config call and so the
> configuration file was created before it was intended to be created.
>
> When the CFS is updated in between the write_config call and the
> PVE::Cluster::check_vmid_unused call in create_and_lock_config,
> the container file would already exist and so creation would
> fail after writing out a basically empty config.
> (Meaning this is necessary for the proposed "update CFS after
> obtaining a configuration file lock" changes)
>
> Even worse, a race was possible for two containers created with the
> same ID at the same time:
> Assuming the initial PVE::Cluster::check_vmid_unused check in the
> parameter verification passes for both create_vm calls, the later one
> would potentially overwrite the earlier configuration file with its
> update_pct_config call.
>
> Additionally, the file read for $old_config was always the one written
> by update_pct_config. Meaning that for a create_vm call with force=1,
> already existing old volumes were not removed.
>
> Signed-off-by: Fabian Ebner
hmm, update_pct_config there was not intended to write out the config,
but just to fill the $conf hash.
three alternative approaches that would return to the original semantics:
1 skip hotplug/apply pending if pending section is empty (should always
be the case in this code path)
2 add explicit parameter to skip
3 drop the hotplug/apply pending calls in update_pct_config, add them to
the update_vm API endpoint instead.
I'd prefer 3 - 1 - 2 in that order ;)
3 would be called with the same semantics in both call sites (create_vm
and update_vm): here are add/delete/revert parameters, here's a conf
hash; merge those operating solely on the conf hash.
> ---
>
> Note that this should be applied before [0] to avoid temporary
> (further ;)) breakage of container creation.
>
> [0]: https://pve.proxmox.com/pipermail/pve-devel/2020-April/043155.html
>
> src/PVE/API2/LXC.pm | 9 -
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> index 148ba6a..46d2fd7 100644
> --- a/src/PVE/API2/LXC.pm
> +++ b/src/PVE/API2/LXC.pm
> @@ -334,10 +334,6 @@ __PACKAGE__->register_method({
> # check/activate default storage
> &$check_and_activate_storage($storage) if !defined($mp_param->{rootfs});
>
> - PVE::LXC::Config->update_pct_config($vmid, $conf, 0, $no_disk_param);
> -
> - $conf->{unprivileged} = 1 if $unprivileged;
> -
> my $emsg = $restore ? "unable to restore CT $vmid -" : "unable to
> create CT $vmid -";
>
> eval { PVE::LXC::Config->create_and_lock_config($vmid, $force) };
> @@ -346,8 +342,11 @@ __PACKAGE__->register_method({
> my $code = sub {
> my $old_conf = PVE::LXC::Config->load_config($vmid);
> my $was_template;
> -
> my $vollist = [];
> +
> + PVE::LXC::Config->update_pct_config($vmid, $conf, 0,
> $no_disk_param);
> + $conf->{unprivileged} = 1 if $unprivileged;
> +
> eval {
> my $orig_mp_param; # only used if $restore
> if ($restore) {
> --
> 2.20.1
>
>
> ___
> pve-devel mailing list
> pve-devel@pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] applied: [PATCH qemu-server 2/2] qm nbdstop: cope graceful with errors
On April 29, 2020 4:24 pm, Thomas Lamprecht wrote:
> as the nbd server could have been stopped by something else.
> Further, it makes no sense to die and mark the migration thus as
> failed, just because of a NBD server stop issue.
>
> At this point the migration hand off to the target was done already,
> so normally we're good, if it fails we have other (followup) problems
> anyway.
the second paragraph is not really true, nbd_stop is part of the
hand off, not afterwards - the sequence is
- migration converges (source VM suspends)
- block mirror converges
- block mirror get's completed via cancel (NBD client connection closed)
- nbd stop on target (to release write blocker)
- resume on target (needs to obtain write blocker)
so if NBD was stopped earlier already it is very likely that something
fishy is going one. we've passed the point of no return and can't go
back to the source VM (especially not when we don't know in which error
state we are ;)), so we might as well try to go ahead and attempt to
resume, but I'd assume that the most likely reason for nbd_stop to fail
is that the qemu process has crashed already (or a re-introduction of
the bug from your first patch that we mistakenly assume an NBD server is
running but haven't actually started one ;)).
so I guess dieing here would actually be the more safe choice (it's
unlikely we'll add much stuff on the source side between the nbd_stop
and the resume calls since it's THE critical section of the whole
migraiton process, but if we do that might operate under the assumption
of "every remote operation has worked or has died"). alternatively, move
the eval to the other side to at least make it explicit that we ignore
failure of nbd_stop and proceed anyway?
(this would all be nicer if we had a stateful mtunnel on the other end
that would just know whether it started an NBD server or not :-P)
>
> Signed-off-by: Thomas Lamprecht
> ---
> PVE/CLI/qm.pm | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
> index f99d401..282fa86 100755
> --- a/PVE/CLI/qm.pm
> +++ b/PVE/CLI/qm.pm
> @@ -274,7 +274,8 @@ __PACKAGE__->register_method ({
>
> my $vmid = $param->{vmid};
>
> - PVE::QemuServer::nbd_stop($vmid);
> + eval { PVE::QemuServer::nbd_stop($vmid) };
> + warn $@ if $@;
>
> return undef;
> }});
> --
> 2.20.1
>
>
> ___
> pve-devel mailing list
> pve-devel@pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] applied: [PATCH qemu-server 1/2] migrate: only stop NBD if we got a NBD url from the target
On 4/30/20 8:35 AM, Fabian Grünbichler wrote:
> On April 29, 2020 4:24 pm, Thomas Lamprecht wrote:
>> Signed-off-by: Thomas Lamprecht
>> ---
>>
>> This was rather quickly assembled to fix an obvious issue, some in depth look
>> at this would be nice, @Fabi or @Fabian :)
>
> LGTM!
>
It really should be OK, but I'm still a bit wondering why
"$self->{storage_migration}"
was true in my case, a VM with only a single disk on ceph..
Not that the reason for this can have other fallout.
>>
>> PVE/QemuMigrate.pm | 5 -
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
>> index 7472e9d..7644922 100644
>> --- a/PVE/QemuMigrate.pm
>> +++ b/PVE/QemuMigrate.pm
>> @@ -750,6 +750,7 @@ sub phase2 {
>> my $targetdrive = $3;
>> $targetdrive =~ s/drive-//g;
>>
>> +$self->{stopnbd} = 1;
>> $self->{target_drive}->{$targetdrive}->{drivestr} = $drivestr;
>> $self->{target_drive}->{$targetdrive}->{nbd_uri} = $nbd_uri;
>> } elsif ($line =~ m!^storage migration listens on
>> nbd:unix:(/run/qemu-server/(\d+)_nbd\.migrate):exportname=(\S+)
>> volume:(\S+)$!) {
>> @@ -760,6 +761,7 @@ sub phase2 {
>> my $targetdrive = $3;
>> $targetdrive =~ s/drive-//g;
>>
>> +$self->{stopnbd} = 1;
>> $self->{target_drive}->{$targetdrive}->{drivestr} = $drivestr;
>> $self->{target_drive}->{$targetdrive}->{nbd_uri} = $nbd_uri;
>> push @$tunnel_addr, "$nbd_unix_addr:$nbd_unix_addr";
>> @@ -1177,7 +1179,8 @@ sub phase3_cleanup {
>> $self->switch_replication_job_target() if $self->{replicated_volumes};
>>
>> if ($self->{livemigration}) {
>> -if ($self->{storage_migration}) {
>> +if ($self->{stopnbd}) {
>> +$self->log('info', "stopping NBD storage migration server on
>> target.");
>> # stop nbd server on remote vm - requirement for resume since 2.9
>> my $cmd = [@{$self->{rem_ssh}}, 'qm', 'nbdstop', $vmid];
>>
>> --
>> 2.20.1
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] applied: [PATCH qemu-server 1/2] migrate: only stop NBD if we got a NBD url from the target
On April 29, 2020 4:24 pm, Thomas Lamprecht wrote:
> Signed-off-by: Thomas Lamprecht
> ---
>
> This was rather quickly assembled to fix an obvious issue, some in depth look
> at this would be nice, @Fabi or @Fabian :)
LGTM!
>
> PVE/QemuMigrate.pm | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
> index 7472e9d..7644922 100644
> --- a/PVE/QemuMigrate.pm
> +++ b/PVE/QemuMigrate.pm
> @@ -750,6 +750,7 @@ sub phase2 {
> my $targetdrive = $3;
> $targetdrive =~ s/drive-//g;
>
> + $self->{stopnbd} = 1;
> $self->{target_drive}->{$targetdrive}->{drivestr} = $drivestr;
> $self->{target_drive}->{$targetdrive}->{nbd_uri} = $nbd_uri;
> } elsif ($line =~ m!^storage migration listens on
> nbd:unix:(/run/qemu-server/(\d+)_nbd\.migrate):exportname=(\S+)
> volume:(\S+)$!) {
> @@ -760,6 +761,7 @@ sub phase2 {
> my $targetdrive = $3;
> $targetdrive =~ s/drive-//g;
>
> + $self->{stopnbd} = 1;
> $self->{target_drive}->{$targetdrive}->{drivestr} = $drivestr;
> $self->{target_drive}->{$targetdrive}->{nbd_uri} = $nbd_uri;
> push @$tunnel_addr, "$nbd_unix_addr:$nbd_unix_addr";
> @@ -1177,7 +1179,8 @@ sub phase3_cleanup {
> $self->switch_replication_job_target() if $self->{replicated_volumes};
>
> if ($self->{livemigration}) {
> - if ($self->{storage_migration}) {
> + if ($self->{stopnbd}) {
> + $self->log('info', "stopping NBD storage migration server on
> target.");
> # stop nbd server on remote vm - requirement for resume since 2.9
> my $cmd = [@{$self->{rem_ssh}}, 'qm', 'nbdstop', $vmid];
>
> --
> 2.20.1
>
>
> ___
> pve-devel mailing list
> pve-devel@pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager v3] ui: dc/Auth: add sync button
On 4/29/20 4:50 PM, Thomas Lamprecht wrote: On 4/29/20 2:32 PM, Dominik Csapak wrote: opens a window with the parameters for the sync and two buttons: 'preview' and 'sync' both open the taskviewer, but the 'preview' one sets the 'dry-run' parameter so that it does not get written out to the user.cfg loads the realm config and prefills the selection with values from the config, and shows a hint about where to set the defaults if none are set Signed-off-by: Dominik Csapak --- changes from v2: * drop the '(from Config)' and '(Default)' options * show a hint about where to set default options * show window after closing the task of a preview www/manager6/Makefile | 1 + www/manager6/dc/AuthView.js | 21 www/manager6/dc/SyncWindow.js | 186 ++ 3 files changed, 208 insertions(+) create mode 100644 www/manager6/dc/SyncWindow.js Oh, some docs would be nice, especially explaining full, purge, .. so that we can add a nice onlineHelpButton which explains the fields and their effects nicely. yes ofc, i am on it. hopefully can send the patch today ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH to pve-docs] Also mentioning FC-based storage in pve
Signed-off-by: Andreas Steinel --- pvesm.adoc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pvesm.adoc b/pvesm.adoc index 5340c3d..b76ce87 100644 --- a/pvesm.adoc +++ b/pvesm.adoc @@ -84,8 +84,8 @@ data to different nodes. ^1^: On file based storages, snapshots are possible with the 'qcow2' format. -^2^: It is possible to use LVM on top of an iSCSI storage. That way -you get a `shared` LVM storage. +^2^: It is possible to use LVM on top of an iSCSI or FC-based storage. +That way you get a `shared` LVM storage. Thin Provisioning -- 2.24.2 (Apple Git-127) ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager v3] ui: dc/Auth: add sync button
On 4/29/20 2:32 PM, Dominik Csapak wrote: > opens a window with the parameters for the sync and two buttons: > 'preview' and 'sync' > > both open the taskviewer, but the 'preview' one sets the 'dry-run' > parameter so that it does not get written out to the user.cfg > > loads the realm config and prefills the selection with values from > the config, and shows a hint about where to set the defaults > if none are set > > Signed-off-by: Dominik Csapak > --- > changes from v2: > * drop the '(from Config)' and '(Default)' options > * show a hint about where to set default options > * show window after closing the task of a preview > > www/manager6/Makefile | 1 + > www/manager6/dc/AuthView.js | 21 > www/manager6/dc/SyncWindow.js | 186 ++ > 3 files changed, 208 insertions(+) > create mode 100644 www/manager6/dc/SyncWindow.js > Oh, some docs would be nice, especially explaining full, purge, .. so that we can add a nice onlineHelpButton which explains the fields and their effects nicely. ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH manager v3] ui: dc/Auth: add sync button
On 4/29/20 2:32 PM, Dominik Csapak wrote: > opens a window with the parameters for the sync and two buttons: > 'preview' and 'sync' > > both open the taskviewer, but the 'preview' one sets the 'dry-run' > parameter so that it does not get written out to the user.cfg > > loads the realm config and prefills the selection with values from > the config, and shows a hint about where to set the defaults > if none are set > > Signed-off-by: Dominik Csapak > --- > changes from v2: > * drop the '(from Config)' and '(Default)' options > * show a hint about where to set default options > * show window after closing the task of a preview > > www/manager6/Makefile | 1 + > www/manager6/dc/AuthView.js | 21 > www/manager6/dc/SyncWindow.js | 186 ++ > 3 files changed, 208 insertions(+) > create mode 100644 www/manager6/dc/SyncWindow.js > Applied, thanks! I added some emptyText "No default available", we could maybe disable "Purge ACLs" if "Full" is not set to yes, but not sure if that's clear or worth it.. Anyway, thanks! ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH qemu-server 2/2] qm nbdstop: cope graceful with errors
as the nbd server could have been stopped by something else.
Further, it makes no sense to die and mark the migration thus as
failed, just because of a NBD server stop issue.
At this point the migration hand off to the target was done already,
so normally we're good, if it fails we have other (followup) problems
anyway.
Signed-off-by: Thomas Lamprecht
---
PVE/CLI/qm.pm | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
index f99d401..282fa86 100755
--- a/PVE/CLI/qm.pm
+++ b/PVE/CLI/qm.pm
@@ -274,7 +274,8 @@ __PACKAGE__->register_method ({
my $vmid = $param->{vmid};
- PVE::QemuServer::nbd_stop($vmid);
+ eval { PVE::QemuServer::nbd_stop($vmid) };
+ warn $@ if $@;
return undef;
}});
--
2.20.1
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH qemu-server 1/2] migrate: only stop NBD if we got a NBD url from the target
Signed-off-by: Thomas Lamprecht
---
This was rather quickly assembled to fix an obvious issue, some in depth look
at this would be nice, @Fabi or @Fabian :)
PVE/QemuMigrate.pm | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index 7472e9d..7644922 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -750,6 +750,7 @@ sub phase2 {
my $targetdrive = $3;
$targetdrive =~ s/drive-//g;
+ $self->{stopnbd} = 1;
$self->{target_drive}->{$targetdrive}->{drivestr} = $drivestr;
$self->{target_drive}->{$targetdrive}->{nbd_uri} = $nbd_uri;
} elsif ($line =~ m!^storage migration listens on
nbd:unix:(/run/qemu-server/(\d+)_nbd\.migrate):exportname=(\S+) volume:(\S+)$!)
{
@@ -760,6 +761,7 @@ sub phase2 {
my $targetdrive = $3;
$targetdrive =~ s/drive-//g;
+ $self->{stopnbd} = 1;
$self->{target_drive}->{$targetdrive}->{drivestr} = $drivestr;
$self->{target_drive}->{$targetdrive}->{nbd_uri} = $nbd_uri;
push @$tunnel_addr, "$nbd_unix_addr:$nbd_unix_addr";
@@ -1177,7 +1179,8 @@ sub phase3_cleanup {
$self->switch_replication_job_target() if $self->{replicated_volumes};
if ($self->{livemigration}) {
- if ($self->{storage_migration}) {
+ if ($self->{stopnbd}) {
+ $self->log('info', "stopping NBD storage migration server on
target.");
# stop nbd server on remote vm - requirement for resume since 2.9
my $cmd = [@{$self->{rem_ssh}}, 'qm', 'nbdstop', $vmid];
--
2.20.1
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH firewall 3/3] introduce new icmp-type parameter
Currently icmp types are handled via 'dport'. This is not documented
anywhere except for a single line of comment in the code. To untangle
the icmp-type handling from the dport handling a new 'icmp-type'
parameter is introduced.
The valid 'icmp-type' values are limited to either the names
(icmp[v6]_type_names hash in the code, same as ip[6]tables provides) or
the combination of type and optional code (e.g. '3/0' for network-unreachable).
As both type and code can be values between 0 and 255, though not all
valid combinations, the checks limit it to range between 0/0 and
255/255.
Support for ipv6-icmp is added to icmp-type parameter handling. This makes it
possible to specify icmpv6 types via the GUI.
Signed-off-by: Mira Limbeck
---
src/PVE/API2/Firewall/Rules.pm | 4 +++
src/PVE/Firewall.pm| 63 --
2 files changed, 64 insertions(+), 3 deletions(-)
diff --git a/src/PVE/API2/Firewall/Rules.pm b/src/PVE/API2/Firewall/Rules.pm
index 0e93a4a..39a0d3a 100644
--- a/src/PVE/API2/Firewall/Rules.pm
+++ b/src/PVE/API2/Firewall/Rules.pm
@@ -144,6 +144,10 @@ sub register_get_rule {
log => PVE::Firewall::get_standard_option('pve-fw-loglevel', {
description => 'Log level for firewall rule',
}),
+ 'icmp-type' => {
+ type => 'string',
+ optional => 1,
+ },
iface => {
type => 'string',
optional => 1,
diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index 0cae9d8..34c2f15 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -1133,6 +1133,26 @@ sub pve_fw_verify_protocol_spec {
return $proto;
}
+PVE::JSONSchema::register_format('pve-fw-icmp-type-spec',
\&pve_fw_verify_icmp_type_spec);
+sub pve_fw_verify_icmp_type_spec {
+my ($icmp_type) = @_;
+
+if ($icmp_type_names->{$icmp_type} || $icmpv6_type_names->{$icmp_type}) {
+ return $icmp_type;
+}
+
+if ($icmp_type =~ m!^(\d+)(?:/(\d+))?$!) {
+ die "icmp-type value '$1' > 255\n" if $1 > 255;
+ die "icmp-type value '$2' > 255\n" if $2 && $2 > 255;
+
+ return $icmp_type;
+}
+
+die "invalid icmp-type value '$icmp_type'\n" if $icmp_type ne '';
+
+return $icmp_type;
+}
+
# helper function for API
@@ -1460,6 +1480,11 @@ my $rule_properties = {
type => 'string',
optional => 1,
},
+'icmp-type' => {
+ description => "Specify icmp-type. Only valid if proto equals 'icmp'.",
+ type => 'string', format => 'pve-fw-icmp-type-spec',
+ optional => 1,
+},
};
sub add_rule_properties {
@@ -1653,7 +1678,8 @@ sub verify_rule {
eval { pve_fw_verify_protocol_spec($rule->{proto}); };
&$add_error('proto', $@) if $@;
&$set_ip_version(4) if $rule->{proto} eq 'icmp';
- &$set_ip_version(6) if $rule->{proto} eq 'icmpv6';
+ &$set_ip_version(6) if $rule->{proto} eq 'icmpv6';
+ &$set_ip_version(6) if $rule->{proto} eq 'ipv6-icmp';
}
if ($rule->{dport}) {
@@ -1667,6 +1693,21 @@ sub verify_rule {
$proto ne 'icmp' && $proto ne 'icmpv6'; # special cases
}
+if (my $icmp_type = $rule ->{'icmp-type'}) {
+ my $proto = $rule->{proto};
+ &$add_error('proto', "missing property - 'icmp-type' requires this
property")
+ if $proto ne 'icmp' && $proto ne 'icmpv6' && $proto ne 'ipv6-icmp';
+ &$add_error('icmp-type', "'icmp-type' cannot be specified together with
'dport'")
+ if $rule->{dport};
+ my $is_numeric = $icmp_type =~ m/\d+/;
+ if ($proto eq 'icmp' && !$is_numeric &&
!$icmp_type_names->{$icmp_type}) {
+ &$add_error('icmp-type', "invalid icmp-type '$icmp_type' for proto
'icmp'");
+ } elsif (($proto eq 'icmpv6' || $proto eq 'ipv6-icmp') && !$is_numeric
+ && !$icmpv6_type_names->{$icmp_type}) {
+ &$add_error('icmp-type', "invalid icmp-type '$icmp_type' for proto
'$proto'");
+ }
+}
+
if ($rule->{sport}) {
eval { parse_port_name_number_or_range($rule->{sport}, 0); };
&$add_error('sport', $@) if $@;
@@ -2040,7 +2081,7 @@ sub ipt_rule_to_cmds {
return if !$rule->{dport};
if ($proto eq 'icmp') {
- # Note: we use dport to store --icmp-type
+ # Note: we allow dport to store --icmp-type for backwards
compatibility
die "unknown icmp-type '$rule->{dport}'\n"
if $rule->{dport} !~ /^\d+$/ &&
!defined($icmp_type_names->{$rule->{dport}});
# values for icmp-type range between 0 and 255
@@ -2048,7 +2089,7 @@ sub ipt_rule_to_cmds {
die "invalid icmp-type '$rule->{dport}'\n" if
($rule->{dport} =~ m/^(\d+)$/) && ($1 > 255);
push @match, "-m icmp --icmp-type $rule->{dport}";
} elsif ($proto eq 'icmpv6') {
-
[pve-devel] [PATCH manager 1/1] change icmp type selector to a combogrid
The combogrid contains all valid icmp types that iptables accepts. In
addition to the names, the Type[/Code] value is shown as well.
As the simple solution with setStore() does not work to change the store
for the combogrid and the values are only set on the first field with
the corresponding name, a workaround using names and IDs was used to switch
between the icmp and icmpv6 type selector and have only one selector
with the name 'icmp-type' at any time.
Because perl hashes have random order, we can't be sure that icmp-type
is set after proto has been set to 'icmp' so set 'icmp-type' again after
all values have been set once. This makes sure the icmp type selector
contains the right value when editing a rule.
Signed-off-by: Mira Limbeck
---
www/manager6/grid/FirewallRules.js | 173 -
1 file changed, 169 insertions(+), 4 deletions(-)
diff --git a/www/manager6/grid/FirewallRules.js
b/www/manager6/grid/FirewallRules.js
index 539e301d..289aee3c 100644
--- a/www/manager6/grid/FirewallRules.js
+++ b/www/manager6/grid/FirewallRules.js
@@ -46,6 +46,107 @@ Ext.define('PVE.form.FWMacroSelector', {
}
});
+Ext.define('PVE.form.ICMPTypeSelector', {
+extend: 'Proxmox.form.ComboGrid',
+alias: 'widget.pveICMPTypeSelector',
+allowBlank: true,
+autoSelect: false,
+valueField: 'name',
+displayField: 'name',
+listConfig: {
+ columns: [
+ {
+ header: gettext('Type'),
+ dataIndex: 'type',
+ hideable: false,
+ sortable: false,
+ width: 50,
+ },
+ {
+ header: gettext('Name'),
+ dataIndex: 'name',
+ hideable: false,
+ sortable: false,
+ flex: 1,
+ },
+ ],
+},
+setName: function(value) {
+ this.name = value;
+}
+});
+
+let ICMP_TYPE_NAMES_STORE = Ext.create('Ext.data.Store', {
+field: ['type', 'name'],
+data: [
+ { type: 'any', name: 'any' },
+ { type: '0', name: 'echo-reply' },
+ { type: '3', name: 'destination-unreachable' },
+ { type: '3/0', name: 'network-unreachable' },
+ { type: '3/1', name: 'host-unreachable' },
+ { type: '3/2', name: 'protocol-unreachable' },
+ { type: '3/3', name: 'port-unreachable' },
+ { type: '3/4', name: 'fragmentation-needed' },
+ { type: '3/5', name: 'source-route-failed' },
+ { type: '3/6', name: 'network-unknown' },
+ { type: '3/7', name: 'host-unknown' },
+ { type: '3/9', name: 'network-prohibited' },
+ { type: '3/10', name: 'host-prohibited' },
+ { type: '3/11', name: 'TOS-network-unreachable' },
+ { type: '3/12', name: 'TOS-host-unreachable' },
+ { type: '3/13', name: 'communication-prohibited' },
+ { type: '3/14', name: 'host-precedence-violation' },
+ { type: '3/15', name: 'precedence-cutoff' },
+ { type: '4', name: 'source-quench' },
+ { type: '5', name: 'redirect' },
+ { type: '5/0', name: 'network-redirect' },
+ { type: '5/1', name: 'host-redirect' },
+ { type: '5/2', name: 'TOS-network-redirect' },
+ { type: '5/3', name: 'TOS-host-redirect' },
+ { type: '8', name: 'echo-request' },
+ { type: '9', name: 'router-advertisement' },
+ { type: '10', name: 'router-solicitation' },
+ { type: '11', name: 'time-exceeded' },
+ { type: '11/0', name: 'ttl-zero-during-transit' },
+ { type: '11/1', name: 'ttl-zero-during-reassembly' },
+ { type: '12', name: 'parameter-problem' },
+ { type: '12/0', name: 'ip-header-bad' },
+ { type: '12/1', name: 'required-option-missing' },
+ { type: '13', name: 'timestamp-request' },
+ { type: '14', name: 'timestamp-reply' },
+ { type: '17', name: 'address-mask-request' },
+ { type: '18', name: 'address-mask-reply' },
+],
+});
+let ICMPV6_TYPE_NAMES_STORE = Ext.create('Ext.data.Store', {
+field: ['type', 'name'],
+data: [
+ { type: '1', name: 'destination-unreachable' },
+ { type: '1/0', name: 'no-route' },
+ { type: '1/1', name: 'communication-prohibited' },
+ { type: '1/2', name: 'beyond-scope' },
+ { type: '1/3', name: 'address-unreachable' },
+ { type: '1/4', name: 'port-unreachable' },
+ { type: '1/5', name: 'failed-policy' },
+ { type: '1/6', name: 'reject-route', },
+ { type: '2', name: 'packet-too-big' },
+ { type: '3', name: 'time-exceeded' },
+ { type: '3/0', name: 'ttl-zero-during-transit' },
+ { type: '3/1', name: 'ttl-zero-during-reassembly' },
+ { type: '4', name: 'parameter-problem' },
+ { type: '4/0', name: 'bad-header' },
+ { type: '4/1', name: 'unknown-header-type' },
+ { type: '4/2', name: 'unknown-option' },
+ { type: '128', name: 'echo-request' },
+ { type: '129', name: 'echo-reply' },
+ { type: '133', name: 'router-solicit
[pve-devel] [PATCH firewall 1/3] fix iptables-restore failing if icmp-type value > 255
This has to be done in both icmp and icmpv6 cases. Currently if
'ipv6-icmp' is set via the GUI ('icmpv6' is not available there) there
is no icmp-type handling. As this is meant to fix the iptables-restore
failure if an icmp-type > 255 is specified, no ipv6-icmp handling is
introduced.
These error messages are not logged as warnings are ignored. To get
these messages you have to run pve-firewall compile and look at the
output.
Signed-off-by: Mira Limbeck
---
src/PVE/Firewall.pm | 6 ++
1 file changed, 6 insertions(+)
diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index d22b15a..39f1bfc 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -2041,11 +2041,17 @@ sub ipt_rule_to_cmds {
# Note: we use dport to store --icmp-type
die "unknown icmp-type '$rule->{dport}'\n"
if $rule->{dport} !~ /^\d+$/ &&
!defined($icmp_type_names->{$rule->{dport}});
+ # values for icmp-type range between 0 and 255
+ # higher values and iptables-restore fails
+ die "invalid icmp-type '$rule->{dport}'\n" if
($rule->{dport} =~ m/^(\d+)$/) && ($1 > 255);
push @match, "-m icmp --icmp-type $rule->{dport}";
} elsif ($proto eq 'icmpv6') {
# Note: we use dport to store --icmpv6-type
die "unknown icmpv6-type '$rule->{dport}'\n"
if $rule->{dport} !~ /^\d+$/ &&
!defined($icmpv6_type_names->{$rule->{dport}});
+ # values for icmpv6-type range between 0 and 255
+ # higher values and iptables-restore fails
+ die "invalid icmpv6-type '$rule->{dport}'\n" if
($rule->{dport} =~ m/^(\d+)$/) && ($1 > 255);
push @match, "-m icmpv6 --icmpv6-type $rule->{dport}";
} elsif (!$PROTOCOLS_WITH_PORTS->{$proto}) {
die "protocol $proto does not have ports\n";
--
2.20.1
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH firewall 2/3] fix wrong icmpv6 types
This removes icmpv6-type 'any' as it is not supported by ip6tables. Also
introduced new icmpv6 types 'beyond-scope', 'failed-policy' and
'reject-route'. These values were taken from 'ip6tables -p icmpv6 -h'.
Signed-off-by: Mira Limbeck
---
src/PVE/Firewall.pm | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index 39f1bfc..0cae9d8 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -785,12 +785,14 @@ my $icmp_type_names = {
# ip6tables -p icmpv6 -h
my $icmpv6_type_names = {
-'any' => 1,
'destination-unreachable' => 1,
'no-route' => 1,
'communication-prohibited' => 1,
+'beyond-scope' => 1,
'address-unreachable' => 1,
'port-unreachable' => 1,
+'failed-policy' => 1,
+'reject-route' => 1,
'packet-too-big' => 1,
'time-exceeded' => 1,
'ttl-zero-during-transit' => 1,
--
2.20.1
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH docs 1/1] add documentation for the new icmp-type parameter
Signed-off-by: Mira Limbeck
---
pve-firewall-rules-opts.adoc | 4
1 file changed, 4 insertions(+)
diff --git a/pve-firewall-rules-opts.adoc b/pve-firewall-rules-opts.adoc
index 13ec8d8..5e8c01e 100644
--- a/pve-firewall-rules-opts.adoc
+++ b/pve-firewall-rules-opts.adoc
@@ -6,6 +6,10 @@ Restrict packet destination address. This can refer to a
single IP address, an I
Restrict TCP/UDP destination port. You can use service names or simple numbers
(0-65535), as defined in '/etc/services'. Port ranges can be specified with
'\d+:\d+', for example '80:85', and you can use comma separated list to match
several ports or ranges.
+`--icmp-type` `` ::
+
+Restrict ICMP packets to specific types. You can either use the names as
ip[6]tables ('ip[6]tables -p icmp[v6] -h') provides them, or use the
Type[/Code] value, for example 'network-unreachable' which corresponds to '3/0'.
+
`--iface` `` ::
Network interface name. You have to use network configuration key names for
VMs and containers ('net\d+'). Host related rules can use arbitrary strings.
--
2.20.1
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH firewall/docs/manager 0/5] fix #2645 and introduce new icmp-type parameter
The first 2 pve-firewall patches contain the actual fix to #2645. As we ignore warnings when pve-firewall is run as a daemon, we don't get any information regarding invalid icmp-types in the logs. To get these messages you have to run pve-firewall compile and look at the output. Patch 3 introduces the new 'icmp-type' parameter. Patch 5 depends on Patch 3 as it uses the new 'icmp-type' parameter in the API calls. Mira Limbeck (3): fix iptables-restore failing if icmp-type value > 255 fix wrong icmpv6 types introduce new icmp-type parameter src/PVE/API2/Firewall/Rules.pm | 4 ++ src/PVE/Firewall.pm| 73 -- 2 files changed, 73 insertions(+), 4 deletions(-) Mira Limbeck (1): add documentation for the new icmp-type parameter pve-firewall-rules-opts.adoc | 4 1 file changed, 4 insertions(+) Mira Limbeck (1): change icmp type selector to a combogrid www/manager6/grid/FirewallRules.js | 173 - 1 file changed, 169 insertions(+), 4 deletions(-) -- 2.20.1 ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH manager v3] ui: dc/Auth: add sync button
opens a window with the parameters for the sync and two buttons:
'preview' and 'sync'
both open the taskviewer, but the 'preview' one sets the 'dry-run'
parameter so that it does not get written out to the user.cfg
loads the realm config and prefills the selection with values from
the config, and shows a hint about where to set the defaults
if none are set
Signed-off-by: Dominik Csapak
---
changes from v2:
* drop the '(from Config)' and '(Default)' options
* show a hint about where to set default options
* show window after closing the task of a preview
www/manager6/Makefile | 1 +
www/manager6/dc/AuthView.js | 21
www/manager6/dc/SyncWindow.js | 186 ++
3 files changed, 208 insertions(+)
create mode 100644 www/manager6/dc/SyncWindow.js
diff --git a/www/manager6/Makefile b/www/manager6/Makefile
index 057a4211..ff93b224 100644
--- a/www/manager6/Makefile
+++ b/www/manager6/Makefile
@@ -232,6 +232,7 @@ JSSRC=
\
dc/RoleView.js \
dc/RoleEdit.js \
dc/ACLView.js \
+ dc/SyncWindow.js\
dc/AuthView.js \
dc/AuthEditBase.js \
dc/AuthEditAD.js\
diff --git a/www/manager6/dc/AuthView.js b/www/manager6/dc/AuthView.js
index a2486fef..3e5a8517 100644
--- a/www/manager6/dc/AuthView.js
+++ b/www/manager6/dc/AuthView.js
@@ -73,6 +73,19 @@ Ext.define('PVE.dc.AuthView', {
me.openEditWindow(rec.data.type, rec.data.realm);
},
+open_sync_window: function() {
+ let me = this;
+ let rec = me.getSelection()[0];
+ if (!rec) {
+ return;
+ }
+ Ext.create('PVE.dc.SyncWindow', {
+ realm: rec.data.realm,
+ listeners: {
+ destroy: () => me.reload(),
+ },
+ }).show();
+},
initComponent: function() {
var me = this;
@@ -107,6 +120,14 @@ Ext.define('PVE.dc.AuthView', {
enableFn: (rec) => PVE.Utils.authSchema[rec.data.type].add,
callback: () => me.reload(),
},
+ '-',
+ {
+ xtype: 'proxmoxButton',
+ text: gettext('Sync'),
+ disabled: true,
+ enableFn: (rec) =>
Boolean(PVE.Utils.authSchema[rec.data.type].syncipanel),
+ handler: () => me.open_sync_window(),
+ },
],
listeners: {
activate: () => me.reload(),
diff --git a/www/manager6/dc/SyncWindow.js b/www/manager6/dc/SyncWindow.js
new file mode 100644
index ..351a3f0f
--- /dev/null
+++ b/www/manager6/dc/SyncWindow.js
@@ -0,0 +1,186 @@
+Ext.define('PVE.dc.SyncWindow', {
+extend: 'Ext.window.Window',
+
+title: gettext('Realm Sync'),
+
+width: 600,
+bodyPadding: 10,
+modal: true,
+resizable: false,
+
+controller: {
+ xclass: 'Ext.app.ViewController',
+
+ control: {
+ 'form': {
+ validitychange: function(field, valid) {
+ let me = this;
+ me.lookup('preview_btn').setDisabled(!valid);
+ me.lookup('sync_btn').setDisabled(!valid);
+ },
+ },
+ 'button': {
+ click: function(btn) {
+ this.sync_realm(btn.reference === 'preview_btn');
+ },
+ },
+ },
+
+ sync_realm: function(is_preview) {
+ let me = this;
+ let view = me.getView();
+ let ipanel = me.lookup('ipanel');
+ let params = ipanel.getValues();
+ params['dry-run'] = is_preview ? 1 : 0;
+ Proxmox.Utils.API2Request({
+ url: `/access/domains/${view.realm}/sync`,
+ waitMsgTarget: view,
+ method: 'POST',
+ params,
+ failure: function(response) {
+ view.show();
+ Ext.Msg.alert(gettext('Error'), response.htmlStatus);
+ },
+ success: function(response) {
+ view.hide();
+ Ext.create('Proxmox.window.TaskViewer', {
+ upid: response.result.data,
+ listeners: {
+ destroy: function() {
+ if (is_preview) {
+ view.show();
+ } else {
+ view.close();
+ }
+ },
+ },
+ }).show();
+ },
+ });
+ },
+},
+
+items: [
+ {
+ xtype: 'form',
+ referen
[pve-devel] applied: [PATCH qemu-server] cleanup: get rid of unnecessary closures
On 4/29/20 1:34 PM, Fabian Ebner wrote:
> Signed-off-by: Fabian Ebner
> ---
>
> Follow-up for
> https://pve.proxmox.com/pipermail/pve-devel/2020-April/043041.html
>
> PVE/QemuServer.pm | 11 ++-
> 1 file changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 63b368f..efacc45 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -4339,17 +4339,10 @@ sub foreach_volid {
> include_unused => 1,
> };
>
> -PVE::QemuConfig->foreach_volume_full($conf, $include_opts, sub {
> - my ($ds, $drive) = @_;
> - $test_volid->($ds, $drive);
> -});
> -
> +PVE::QemuConfig->foreach_volume_full($conf, $include_opts, $test_volid);
> foreach my $snapname (keys %{$conf->{snapshots}}) {
> my $snap = $conf->{snapshots}->{$snapname};
> - PVE::QemuConfig->foreach_volume_full($snap, $include_opts, sub {
> - my ($ds, $drive) = @_;
> - $test_volid->($ds, $drive, $snapname);
> -});
> + PVE::QemuConfig->foreach_volume_full($snap, $include_opts, $test_volid,
> $snapname);
> }
>
> foreach my $volid (keys %$volhash) {
>
applied, thanks!
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager v2] ui: dc/Auth: add sync button
On 4/29/20 1:54 PM, Dominik Csapak wrote:
> copy/pasted the other mail to only write on answer:
>
> On 4/29/20 1:52 PM, Thomas Lamprecht wrote:
>> On 4/27/20 4:40 PM, Dominik Csapak wrote:
>>> opens a window with the parameters for the sync and two buttons:
>>> 'preview' and 'sync'
>>>
>>> both open the taskviewer, but the 'preview' one sets the 'no-write'
>>> parameter so that it does not get written out to the user.cfg
>>>
>>
>> Oh, and maybe it's nicer if the "Preview" kept the sync window open?
>> So that if the preview looks good a user can just do the real sync.
>>
>
> yeah that makes sense
>
> On 4/29/20 1:49 PM, Thomas Lamprecht wrote:
>> On 4/27/20 4:40 PM, Dominik Csapak wrote:
>>> opens a window with the parameters for the sync and two buttons:
>>> 'preview' and 'sync'
>>>
>>> both open the taskviewer, but the 'preview' one sets the 'no-write'
>>> parameter so that it does not get written out to the user.cfg
>>>
>>> loads the realm config and prefills the selection with values from
>>> the config
>>>
>>> Signed-off-by: Dominik Csapak
>>> ---
>>> changes from v1:
>>> * load realm config and set appropriate values
>>> * mark loaded values as such ({0} (from Config))
>>> * wrapped the ipanel in a form to easier get the validity
>>> * changed to a controller to better to have better access to
>>> the components and their handlers
>>> * remove the 'default' options from fields that do not have one,
>>> set the initial value to '' and allowBlank to false so that
>>> they are not valid by default (either set from config or manually chosen)
>>>
>>> i am not completely happy with how manual this whole thing is, but
>>> i (for now) could not come up with a better method without completely
>>> changing the ux (e.g showing the defaults separately; which i do not want)
>>>
>>> there are a few possibilities to make this easier when we want
>>> some of those features elsewhere, but for now it does not make sense
>>> to refactor it (e.g. the changing of text of a kvcombobox, or
>>> the manual management of buttons in an edit window)
>>>
>>
>> Hmm, you missed implementing the tooltip I suggested but that's not to
>> relevant..
>> Using this I got the feeling that for the user it is totally irrelevant from
>> where the values come.
>
> sorry, it seems i really missed this ...
>
>>
>> So I'd just show them what the value is, no "from Default", no "from Config",
>> and for the case where no default-sync-config values are set at all show a
>> small
>> additional hint alá "You can set the default sync options when editing the
>> realm"
>> or so. Not hard feelings for that hint, but I really thinks it's better to
>> simplify
>> selections here and just show the plain values which will be used.
>
> so you're ok with showing the empty fields, and when at least one is empty,
> show the hint?
Rather:
If neither the schema has and default nor the sync-default-options for that
field.
is set show it as empty and required
I'd only show the hint if no default sync option is set, as else I assume that
the
user already knows about them, but no hard feelings here.
>
> i'd rather say it like:
>
> "Default sync options can be set by editing the realm." sounds good?
>
Sounds ok.
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager v2] ui: dc/Auth: add sync button
copy/pasted the other mail to only write on answer:
On 4/29/20 1:52 PM, Thomas Lamprecht wrote:
> On 4/27/20 4:40 PM, Dominik Csapak wrote:
>> opens a window with the parameters for the sync and two buttons:
>> 'preview' and 'sync'
>>
>> both open the taskviewer, but the 'preview' one sets the 'no-write'
>> parameter so that it does not get written out to the user.cfg
>>
>
> Oh, and maybe it's nicer if the "Preview" kept the sync window open?
> So that if the preview looks good a user can just do the real sync.
>
yeah that makes sense
On 4/29/20 1:49 PM, Thomas Lamprecht wrote:
On 4/27/20 4:40 PM, Dominik Csapak wrote:
opens a window with the parameters for the sync and two buttons:
'preview' and 'sync'
both open the taskviewer, but the 'preview' one sets the 'no-write'
parameter so that it does not get written out to the user.cfg
loads the realm config and prefills the selection with values from
the config
Signed-off-by: Dominik Csapak
---
changes from v1:
* load realm config and set appropriate values
* mark loaded values as such ({0} (from Config))
* wrapped the ipanel in a form to easier get the validity
* changed to a controller to better to have better access to
the components and their handlers
* remove the 'default' options from fields that do not have one,
set the initial value to '' and allowBlank to false so that
they are not valid by default (either set from config or manually chosen)
i am not completely happy with how manual this whole thing is, but
i (for now) could not come up with a better method without completely
changing the ux (e.g showing the defaults separately; which i do not want)
there are a few possibilities to make this easier when we want
some of those features elsewhere, but for now it does not make sense
to refactor it (e.g. the changing of text of a kvcombobox, or
the manual management of buttons in an edit window)
Hmm, you missed implementing the tooltip I suggested but that's not to
relevant..
Using this I got the feeling that for the user it is totally irrelevant from
where the values come.
sorry, it seems i really missed this ...
So I'd just show them what the value is, no "from Default", no "from Config",
and for the case where no default-sync-config values are set at all show a small
additional hint alá "You can set the default sync options when editing the
realm"
or so. Not hard feelings for that hint, but I really thinks it's better to
simplify
selections here and just show the plain values which will be used.
so you're ok with showing the empty fields, and when at least one is
empty, show the hint?
i'd rather say it like:
"Default sync options can be set by editing the realm." sounds good?
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager v2] ui: dc/Auth: add sync button
On 4/27/20 4:40 PM, Dominik Csapak wrote: > opens a window with the parameters for the sync and two buttons: > 'preview' and 'sync' > > both open the taskviewer, but the 'preview' one sets the 'no-write' > parameter so that it does not get written out to the user.cfg > Oh, and maybe it's nicer if the "Preview" kept the sync window open? So that if the preview looks good a user can just do the real sync. ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager v2] ui: dc/Auth: add sync button
On 4/27/20 4:40 PM, Dominik Csapak wrote:
> opens a window with the parameters for the sync and two buttons:
> 'preview' and 'sync'
>
> both open the taskviewer, but the 'preview' one sets the 'no-write'
> parameter so that it does not get written out to the user.cfg
>
> loads the realm config and prefills the selection with values from
> the config
>
> Signed-off-by: Dominik Csapak
> ---
> changes from v1:
> * load realm config and set appropriate values
> * mark loaded values as such ({0} (from Config))
> * wrapped the ipanel in a form to easier get the validity
> * changed to a controller to better to have better access to
> the components and their handlers
> * remove the 'default' options from fields that do not have one,
> set the initial value to '' and allowBlank to false so that
> they are not valid by default (either set from config or manually chosen)
>
> i am not completely happy with how manual this whole thing is, but
> i (for now) could not come up with a better method without completely
> changing the ux (e.g showing the defaults separately; which i do not want)
>
> there are a few possibilities to make this easier when we want
> some of those features elsewhere, but for now it does not make sense
> to refactor it (e.g. the changing of text of a kvcombobox, or
> the manual management of buttons in an edit window)
>
Hmm, you missed implementing the tooltip I suggested but that's not to
relevant..
Using this I got the feeling that for the user it is totally irrelevant from
where the values come.
So I'd just show them what the value is, no "from Default", no "from Config",
and for the case where no default-sync-config values are set at all show a small
additional hint alá "You can set the default sync options when editing the
realm"
or so. Not hard feelings for that hint, but I really thinks it's better to
simplify
selections here and just show the plain values which will be used.
> www/manager6/Makefile | 1 +
> www/manager6/dc/AuthView.js | 21
> www/manager6/dc/SyncWindow.js | 185 ++
> 3 files changed, 207 insertions(+)
> create mode 100644 www/manager6/dc/SyncWindow.js
>
> diff --git a/www/manager6/Makefile b/www/manager6/Makefile
> index 057a4211..ff93b224 100644
> --- a/www/manager6/Makefile
> +++ b/www/manager6/Makefile
> @@ -232,6 +232,7 @@ JSSRC=
> \
> dc/RoleView.js \
> dc/RoleEdit.js \
> dc/ACLView.js \
> + dc/SyncWindow.js\
> dc/AuthView.js \
> dc/AuthEditBase.js \
> dc/AuthEditAD.js\
> diff --git a/www/manager6/dc/AuthView.js b/www/manager6/dc/AuthView.js
> index a2486fef..3e5a8517 100644
> --- a/www/manager6/dc/AuthView.js
> +++ b/www/manager6/dc/AuthView.js
> @@ -73,6 +73,19 @@ Ext.define('PVE.dc.AuthView', {
> me.openEditWindow(rec.data.type, rec.data.realm);
> },
>
> +open_sync_window: function() {
> + let me = this;
> + let rec = me.getSelection()[0];
> + if (!rec) {
> + return;
> + }
> + Ext.create('PVE.dc.SyncWindow', {
> + realm: rec.data.realm,
> + listeners: {
> + destroy: () => me.reload(),
> + },
> + }).show();
> +},
>
> initComponent: function() {
> var me = this;
> @@ -107,6 +120,14 @@ Ext.define('PVE.dc.AuthView', {
> enableFn: (rec) => PVE.Utils.authSchema[rec.data.type].add,
> callback: () => me.reload(),
> },
> + '-',
> + {
> + xtype: 'proxmoxButton',
> + text: gettext('Sync'),
> + disabled: true,
> + enableFn: (rec) =>
> Boolean(PVE.Utils.authSchema[rec.data.type].syncipanel),
> + handler: () => me.open_sync_window(),
> + },
> ],
> listeners: {
> activate: () => me.reload(),
> diff --git a/www/manager6/dc/SyncWindow.js b/www/manager6/dc/SyncWindow.js
> new file mode 100644
> index ..0865ef14
> --- /dev/null
> +++ b/www/manager6/dc/SyncWindow.js
> @@ -0,0 +1,185 @@
> +Ext.define('PVE.dc.SyncWindow', {
> +extend: 'Ext.window.Window',
> +
> +title: gettext('Realm Sync'),
> +
> +bodyPadding: 10,
> +modal: true,
> +resizable: false,
> +
> +controller: {
> + xclass: 'Ext.app.ViewController',
> +
> + control: {
> + 'form': {
> + validitychange: function(field, valid) {
> + let me = this;
> + me.lookup('preview_btn').setDisabled(!valid);
> + me.lookup('sync_btn').setDisabled(!valid);
> + },
> + },
> + 'button': {
> + click: function(btn) {
> +
[pve-devel] [PATCH qemu-server] cleanup: get rid of unnecessary closures
Signed-off-by: Fabian Ebner
---
Follow-up for https://pve.proxmox.com/pipermail/pve-devel/2020-April/043041.html
PVE/QemuServer.pm | 11 ++-
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 63b368f..efacc45 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -4339,17 +4339,10 @@ sub foreach_volid {
include_unused => 1,
};
-PVE::QemuConfig->foreach_volume_full($conf, $include_opts, sub {
- my ($ds, $drive) = @_;
- $test_volid->($ds, $drive);
-});
-
+PVE::QemuConfig->foreach_volume_full($conf, $include_opts, $test_volid);
foreach my $snapname (keys %{$conf->{snapshots}}) {
my $snap = $conf->{snapshots}->{$snapname};
- PVE::QemuConfig->foreach_volume_full($snap, $include_opts, sub {
- my ($ds, $drive) = @_;
- $test_volid->($ds, $drive, $snapname);
-});
+ PVE::QemuConfig->foreach_volume_full($snap, $include_opts, $test_volid,
$snapname);
}
foreach my $volid (keys %$volhash) {
--
2.20.1
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied-series: [PATCH qemu-server 1/3] Fix test_volid call for vmstate and fix check for snapshots on migration
On 4/16/20 2:54 PM, Fabian Ebner wrote:
> by excluding vmstate. It is referenced by snapshots, but
> is not a volume containing a snapshot.
>
> Signed-off-by: Fabian Ebner
> ---
> PVE/QemuMigrate.pm | 2 ++
> PVE/QemuServer.pm | 2 +-
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
> index 0a6277d..2e2e430 100644
> --- a/PVE/QemuMigrate.pm
> +++ b/PVE/QemuMigrate.pm
> @@ -404,6 +404,8 @@ sub sync_disks {
> die "owned by other VM (owner = VM $owner)\n"
> if !$owner || ($owner != $vmid);
>
> + return if $attr->{is_vmstate};
> +
> if (defined($snaprefs)) {
> $local_volumes->{$volid}->{snapshots} = 1;
>
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 6445508..5a42532 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -4320,7 +4320,7 @@ sub foreach_volid {
>
> foreach my $snapname (keys %{$conf->{snapshots}}) {
> my $snap = $conf->{snapshots}->{$snapname};
> - $test_volid->($snap->{vmstate}, 0, 1, $snapname);
> + $test_volid->($snap->{vmstate}, 0, 1, 0, $snapname);
> $volhash->{$snap->{vmstate}}->{is_vmstate} = 1 if $snap->{vmstate};
> PVE::QemuConfig->foreach_volume($snap, sub {
> my ($ds, $drive) = @_;
>
applied series, thanks!
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH qemu-server] api/resume: make nocheck root-only
On 4/27/20 9:19 AM, Fabian Grünbichler wrote:
> this is only used for migration via 'qm mtunnel', regular users should
> never need to resume a VM that does not logically belong to the node it
> is running on
>
> Signed-off-by: Fabian Grünbichler
> ---
> PVE/API2/Qemu.pm | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index b3683ae..9658a36 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -2593,6 +2593,8 @@ __PACKAGE__->register_method({
> if $skiplock && $authuser ne 'root@pam';
>
> my $nocheck = extract_param($param, 'nocheck');
> + raise_param_exc({ nocheck => "Only root may use this option." })
> + if $nocheck && $authuser ne 'root@pam';
>
> my $to_disk_suspended;
> eval {
>
applied, thanks!
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH container] create_vm: fix order of config creation/reading/locking
The update_pct_config call leads to a write_config call and so the
configuration file was created before it was intended to be created.
When the CFS is updated in between the write_config call and the
PVE::Cluster::check_vmid_unused call in create_and_lock_config,
the container file would already exist and so creation would
fail after writing out a basically empty config.
(Meaning this is necessary for the proposed "update CFS after
obtaining a configuration file lock" changes)
Even worse, a race was possible for two containers created with the
same ID at the same time:
Assuming the initial PVE::Cluster::check_vmid_unused check in the
parameter verification passes for both create_vm calls, the later one
would potentially overwrite the earlier configuration file with its
update_pct_config call.
Additionally, the file read for $old_config was always the one written
by update_pct_config. Meaning that for a create_vm call with force=1,
already existing old volumes were not removed.
Signed-off-by: Fabian Ebner
---
Note that this should be applied before [0] to avoid temporary
(further ;)) breakage of container creation.
[0]: https://pve.proxmox.com/pipermail/pve-devel/2020-April/043155.html
src/PVE/API2/LXC.pm | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 148ba6a..46d2fd7 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -334,10 +334,6 @@ __PACKAGE__->register_method({
# check/activate default storage
&$check_and_activate_storage($storage) if !defined($mp_param->{rootfs});
- PVE::LXC::Config->update_pct_config($vmid, $conf, 0, $no_disk_param);
-
- $conf->{unprivileged} = 1 if $unprivileged;
-
my $emsg = $restore ? "unable to restore CT $vmid -" : "unable to
create CT $vmid -";
eval { PVE::LXC::Config->create_and_lock_config($vmid, $force) };
@@ -346,8 +342,11 @@ __PACKAGE__->register_method({
my $code = sub {
my $old_conf = PVE::LXC::Config->load_config($vmid);
my $was_template;
-
my $vollist = [];
+
+ PVE::LXC::Config->update_pct_config($vmid, $conf, 0,
$no_disk_param);
+ $conf->{unprivileged} = 1 if $unprivileged;
+
eval {
my $orig_mp_param; # only used if $restore
if ($restore) {
--
2.20.1
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH firewall 1/7] configs: add locking helpers
On 4/29/20 10:52 AM, Fabian Grünbichler wrote:
> to allow some level of safe concurrent config modification, instead of
> the current free for all.
>
> Signed-off-by: Fabian Grünbichler
> ---
>
> Notes:
> require pve-cluster that provides cfs_lock_firewall, or switching to
> cfs_lock_domain as mentioned in pve-cluster#1
>
> lock_hostfw_conf could also use a node-local lock, but IMHO it's easier
> to have
> the same locking semantics/interface across all three levels (especially
> if we
> do the second patch in pve-cluster).
>
> it's easy enough to switch out though.
Hmm, are you aware that cfs_locks only protect from other nodes, and not
intra-node? I.e., if one process of the node has the lock any other can
aquire it too, just no other node can.
That's the reason for my a bit special corosync config change lock:
https://git.proxmox.com/?p=pve-cluster.git;a=blob;f=data/PVE/API2/ClusterConfig.pm;h=7a4bce07169c15cfe982ea34f2c238ad6ee4abf7;hb=HEAD#l188
>
> src/PVE/Firewall.pm | 32
> 1 file changed, 32 insertions(+)
>
> diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
> index d22b15a..eda39eb 100644
> --- a/src/PVE/Firewall.pm
> +++ b/src/PVE/Firewall.pm
> @@ -3053,6 +3053,8 @@ sub generic_fw_config_parser {
> return $res;
> }
>
> +# this is only used to prevent concurrent runs of rule
> compilation/application
> +# see lock_*_conf for cfs locks protectiong config modification
> sub run_locked {
> my ($code, @param) = @_;
>
> @@ -3101,6 +3103,18 @@ sub read_local_vm_config {
> return $vmdata;
> };
>
> +sub lock_vmfw_conf {
> +my ($vmid, $timeout, $code, @param) = @_;
> +
> +die "can't lock VM firewall config for undefined VMID\n"
> + if !defined($vmid);
> +
> +my $res = PVE::Cluster::cfs_lock_firewall("vm-$vmid", $timeout, $code,
> @param);
> +die $@ if $@;
> +
> +return $res;
> +}
> +
> sub load_vmfw_conf {
> my ($cluster_conf, $rule_env, $vmid, $dir) = @_;
>
> @@ -3448,6 +3462,15 @@ my $set_global_log_ratelimit = sub {
> }
> };
>
> +sub lock_clusterfw_conf {
> +my ($timeout, $code, @param) = @_;
> +
> +my $res = PVE::Cluster::cfs_lock_firewall("cluster", $timeout, $code,
> @param);
> +die $@ if $@;
> +
> +return $res;
> +}
> +
> sub load_clusterfw_conf {
> my ($filename) = @_;
>
> @@ -3511,6 +3534,15 @@ sub save_clusterfw_conf {
> }
> }
>
> +sub lock_hostfw_conf {
> +my ($timeout, $code, @param) = @_;
> +
> +my $res = PVE::Cluster::cfs_lock_firewall("host-$nodename", $timeout,
> $code, @param);
> +die $@ if $@;
> +
> +return $res;
> +}
> +
> sub load_hostfw_conf {
> my ($cluster_conf, $filename) = @_;
>
>
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [RFC cluster 2/2] cfs_lock: re-raise exceptions
On 4/29/20 10:52 AM, Fabian Grünbichler wrote:
> so that API paths that raise an exception while holding a CFS lock
> properly propagate that exception to the client, instead of the
> stringified version with added noise about locks added to the front.
>
> Signed-off-by: Fabian Grünbichler
> ---
>
> Notes:
> there seems to be nothing that matches on the prefix added by cfs_lock.
>
> some API calls in pve-access-control, pve-storage and Replication would
> now
> benefit from raising (parameter) exceptions instead of a plain death while
> having a config file locked (mainly checking for duplicates when adding
> entries)
>
> data/PVE/Cluster.pm | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/data/PVE/Cluster.pm b/data/PVE/Cluster.pm
> index b4de989..cce681b 100644
> --- a/data/PVE/Cluster.pm
> +++ b/data/PVE/Cluster.pm
> @@ -609,7 +609,13 @@ my $cfs_lock = sub {
> alarm($prev_alarm);
>
> if ($err) {
> -$@ = "error with cfs lock '$lockid': $err";
> + if (ref($err) eq 'PVE::Exception') {
> + # re-raise defined exceptions
> + $@ = $err;
> + } else {
> + # add lock info for plain errors
> + $@ = "error with cfs lock '$lockid': $err";
> + }
> return undef;
> }
>
>
applied, followed up with fixing the trailing whitespace you introduced and
adapting
the old "plain error" prefix message (which was there since the beginning of
time ^^)
thanks!
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH cluster 1/2] cfs_lock: add firewall lock helper
On 4/29/20 10:52 AM, Fabian Grünbichler wrote:
> Signed-off-by: Fabian Grünbichler
> ---
> alternatively we could re-use 'cfs_lock_domain', which is currently
> only used by HA and was intended as general-purpose cfs_lock wrapper..
> I'd shorten the firewall- prefix to fw- in that case though.
>
Initially I was more for using the domain one here, but looking at the lock
helper patch in firewall and the other lock code in PVE::Cluster made me
think that this fits actually quite well.
applied, thanks!
> domain-fw-host-$foo might be more confusing to end users though, as it's
> pretty visible in error messages.
>
> data/PVE/Cluster.pm | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/data/PVE/Cluster.pm b/data/PVE/Cluster.pm
> index 068d626..b4de989 100644
> --- a/data/PVE/Cluster.pm
> +++ b/data/PVE/Cluster.pm
> @@ -659,6 +659,14 @@ sub cfs_lock_authkey {
> $cfs_lock->('authkey', $timeout, $code, @param);
> }
>
> +sub cfs_lock_firewall {
> +my ($scope, $timeout, $code, @param) = @_;
> +
> +my $lockid = "firewall-$scope";
> +
> +$cfs_lock->($lockid, $timeout, $code, @param);
> +}
> +
> my $log_levels = {
> "emerg" => 0,
> "alert" => 1,
>
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH qemu-server] restore: use correct storage for format check for cloudinit drives
On 4/28/20 2:52 PM, Dominik Csapak wrote:
> when a backup includes a cloudinit disk on a non-existent storage,
> the restore fails with 'storage' does not exist
>
> this happens because we want to get the format of the disk, by
> checking the source storage
>
> we fix this by using the target storage first and only the source as
> fallback
>
> this will still fail if neither storage exists
> (which is ok, since we cannot restore then anyway)
>
> Signed-off-by: Dominik Csapak
> ---
> PVE/QemuServer.pm | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 37c7320..e355810 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -5739,7 +5739,7 @@ my $parse_backup_hints = sub {
> my $drive = parse_drive($virtdev, $2);
> if (drive_is_cloudinit($drive)) {
> my ($storeid, $volname) =
> PVE::Storage::parse_volume_id($drive->{file});
> - my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
> + my $scfg = PVE::Storage::storage_config($storecfg,
> $options->{storage} // $storeid);
> my $format = qemu_img_format($scfg, $volname); # has 'raw'
> fallback
>
> $virtdev_hash->{$virtdev} = {
>
applied with a small cleanup reducing the occurrence of
"$options->{storage} // $storeid" thanks.
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH firewall 3/7] api: lock configs
wherever we have a r-m-w cycle.
Signed-off-by: Fabian Grünbichler
---
Notes:
best viewed with -w
src/PVE/API2/Firewall/Aliases.pm | 80 +---
src/PVE/API2/Firewall/Cluster.pm | 36
src/PVE/API2/Firewall/Groups.pm | 52 ++-
src/PVE/API2/Firewall/Host.pm| 42 +
src/PVE/API2/Firewall/IPSet.pm | 152 ++-
src/PVE/API2/Firewall/Rules.pm | 94 +++
src/PVE/API2/Firewall/VM.pm | 43 -
7 files changed, 280 insertions(+), 219 deletions(-)
diff --git a/src/PVE/API2/Firewall/Aliases.pm b/src/PVE/API2/Firewall/Aliases.pm
index 9ea6f70..33ac669 100644
--- a/src/PVE/API2/Firewall/Aliases.pm
+++ b/src/PVE/API2/Firewall/Aliases.pm
@@ -143,19 +143,23 @@ sub register_create_alias {
code => sub {
my ($param) = @_;
- my ($fw_conf, $aliases) = $class->load_config($param);
+ $class->lock_config($param, sub {
+ my ($param) = @_;
- my $name = lc($param->{name});
+ my ($fw_conf, $aliases) = $class->load_config($param);
- raise_param_exc({ name => "alias '$param->{name}' already exists" })
- if defined($aliases->{$name});
+ my $name = lc($param->{name});
- my $data = { name => $param->{name}, cidr => $param->{cidr} };
- $data->{comment} = $param->{comment} if $param->{comment};
+ raise_param_exc({ name => "alias '$param->{name}' already
exists" })
+ if defined($aliases->{$name});
- $aliases->{$name} = $data;
+ my $data = { name => $param->{name}, cidr => $param->{cidr} };
+ $data->{comment} = $param->{comment} if $param->{comment};
- $class->save_aliases($param, $fw_conf, $aliases);
+ $aliases->{$name} = $data;
+
+ $class->save_aliases($param, $fw_conf, $aliases);
+ });
return undef;
}});
@@ -219,35 +223,39 @@ sub register_update_alias {
code => sub {
my ($param) = @_;
- my ($fw_conf, $aliases) = $class->load_config($param);
+ $class->lock_config($param, sub {
+ my ($param) = @_;
- my $list = &$aliases_to_list($aliases);
+ my ($fw_conf, $aliases) = $class->load_config($param);
- my (undef, $digest) = PVE::Firewall::copy_list_with_digest($list);
+ my $list = &$aliases_to_list($aliases);
- PVE::Tools::assert_if_modified($digest, $param->{digest});
+ my (undef, $digest) =
PVE::Firewall::copy_list_with_digest($list);
- my $name = lc($param->{name});
+ PVE::Tools::assert_if_modified($digest, $param->{digest});
- raise_param_exc({ name => "no such alias" }) if !$aliases->{$name};
+ my $name = lc($param->{name});
- my $data = { name => $param->{name}, cidr => $param->{cidr} };
- $data->{comment} = $param->{comment} if $param->{comment};
+ raise_param_exc({ name => "no such alias" }) if
!$aliases->{$name};
- $aliases->{$name} = $data;
+ my $data = { name => $param->{name}, cidr => $param->{cidr} };
+ $data->{comment} = $param->{comment} if $param->{comment};
- my $rename = $param->{rename};
- $rename = lc($rename) if $rename;
+ $aliases->{$name} = $data;
- if ($rename && ($name ne $rename)) {
- raise_param_exc({ name => "alias '$param->{rename}' already
exists" })
- if defined($aliases->{$rename});
- $aliases->{$name}->{name} = $param->{rename};
- $aliases->{$rename} = $aliases->{$name};
- delete $aliases->{$name};
- }
+ my $rename = $param->{rename};
+ $rename = lc($rename) if $rename;
- $class->save_aliases($param, $fw_conf, $aliases);
+ if ($rename && ($name ne $rename)) {
+ raise_param_exc({ name => "alias '$param->{rename}' already
exists" })
+ if defined($aliases->{$rename});
+ $aliases->{$name}->{name} = $param->{rename};
+ $aliases->{$rename} = $aliases->{$name};
+ delete $aliases->{$name};
+ }
+
+ $class->save_aliases($param, $fw_conf, $aliases);
+ });
return undef;
}});
@@ -276,16 +284,20 @@ sub register_delete_alias {
code => sub {
my ($param) = @_;
- my ($fw_conf, $aliases) = $class->load_config($param);
+ $class->lock_config($param, sub {
+ my ($param) = @_;
- my $list = &$aliases_to_list($aliases);
- my (undef, $digest) = PVE::Firewall::copy_list_with_digest($list);
- PVE::Tools::assert_if_modified($digest, $param->{digest});
+ my ($f
[pve-devel] [PATCH firewall 5/7] api/ipsets: parse_cidr before checking for duplicates
for example, the config parser drops a trailing /32 for IPv4, so we
should do the same here. otherwise we can have one entry for $IP and
one for $IP/32 with different properties until the next R-M-W cycle
drops one of them again.
Signed-off-by: Fabian Grünbichler
---
src/PVE/API2/Firewall/IPSet.pm | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/src/PVE/API2/Firewall/IPSet.pm b/src/PVE/API2/Firewall/IPSet.pm
index 913dd86..ec9326f 100644
--- a/src/PVE/API2/Firewall/IPSet.pm
+++ b/src/PVE/API2/Firewall/IPSet.pm
@@ -195,6 +195,13 @@ sub register_create_ip {
my ($cluster_conf, $fw_conf, $ipset) =
$class->load_config($param);
my $cidr = $param->{cidr};
+ if ($cidr =~ m/^${PVE::Firewall::ip_alias_pattern}$/) {
+ # make sure alias exists (if $cidr is an alias)
+ PVE::Firewall::resolve_alias($cluster_conf, $fw_conf,
$cidr);
+ } else {
+ # normalize like config parser, otherwise duplicates might
slip through
+ $cidr = PVE::Firewall::parse_ip_or_cidr($cidr);
+ }
foreach my $entry (@$ipset) {
raise_param_exc({ cidr => "address '$cidr' already exists"
})
@@ -204,9 +211,6 @@ sub register_create_ip {
raise_param_exc({ cidr => "a zero prefix is not allowed in
ipset entries" })
if $cidr =~ m!/0+$!;
- # make sure alias exists (if $cidr is an alias)
- PVE::Firewall::resolve_alias($cluster_conf, $fw_conf, $cidr)
- if $cidr =~ m/^${PVE::Firewall::ip_alias_pattern}$/;
my $data = { cidr => $cidr };
--
2.20.1
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH firewall 2/7] api: add locking helpers
for ipset, rules and alias API generation modules.
Signed-off-by: Fabian Grünbichler
---
Notes:
separated from using them for easier reviewing
src/PVE/API2/Firewall/Aliases.pm | 24
src/PVE/API2/Firewall/IPSet.pm | 48
src/PVE/API2/Firewall/Rules.pm | 36
3 files changed, 108 insertions(+)
diff --git a/src/PVE/API2/Firewall/Aliases.pm b/src/PVE/API2/Firewall/Aliases.pm
index 2a66abd..9ea6f70 100644
--- a/src/PVE/API2/Firewall/Aliases.pm
+++ b/src/PVE/API2/Firewall/Aliases.pm
@@ -25,6 +25,12 @@ my $api_properties = {
},
};
+sub lock_config {
+my ($class, $param, $code) = @_;
+
+die "implement this in subclass";
+}
+
sub load_config {
my ($class, $param) = @_;
@@ -308,6 +314,12 @@ sub rule_env {
return 'cluster';
}
+sub lock_config {
+my ($class, $param, $code) = @_;
+
+PVE::Firewall::lock_clusterfw_conf(10, $code, $param);
+}
+
sub load_config {
my ($class, $param) = @_;
@@ -345,6 +357,12 @@ __PACKAGE__->additional_parameters({
vmid => get_standard_option('pve-vmid'),
});
+sub lock_config {
+my ($class, $param, $code) = @_;
+
+PVE::Firewall::lock_vmfw_conf($param->{vmid}, 10, $code, $param);
+}
+
sub load_config {
my ($class, $param) = @_;
@@ -383,6 +401,12 @@ __PACKAGE__->additional_parameters({
vmid => get_standard_option('pve-vmid'),
});
+sub lock_config {
+my ($class, $param, $code) = @_;
+
+PVE::Firewall::lock_vmfw_conf($param->{vmid}, 10, $code, $param);
+}
+
sub load_config {
my ($class, $param) = @_;
diff --git a/src/PVE/API2/Firewall/IPSet.pm b/src/PVE/API2/Firewall/IPSet.pm
index e59a6f2..72e7524 100644
--- a/src/PVE/API2/Firewall/IPSet.pm
+++ b/src/PVE/API2/Firewall/IPSet.pm
@@ -25,6 +25,12 @@ my $api_properties = {
},
};
+sub lock_config {
+my ($class, $param, $code) = @_;
+
+die "implement this in subclass";
+}
+
sub load_config {
my ($class, $param) = @_;
@@ -353,6 +359,12 @@ sub rule_env {
return 'cluster';
}
+sub lock_config {
+my ($class, $param, $code) = @_;
+
+PVE::Firewall::lock_clusterfw_conf(10, $code, $param);
+}
+
sub load_config {
my ($class, $param) = @_;
@@ -390,6 +402,12 @@ __PACKAGE__->additional_parameters({
vmid => get_standard_option('pve-vmid'),
});
+sub lock_config {
+my ($class, $param, $code) = @_;
+
+PVE::Firewall::lock_vmfw_conf($param->{vmid}, 10, $code, $param);
+}
+
sub load_config {
my ($class, $param) = @_;
@@ -428,6 +446,12 @@ __PACKAGE__->additional_parameters({
vmid => get_standard_option('pve-vmid'),
});
+sub lock_config {
+my ($class, $param, $code) = @_;
+
+PVE::Firewall::lock_vmfw_conf($param->{vmid}, 10, $code, $param);
+}
+
sub load_config {
my ($class, $param) = @_;
@@ -457,6 +481,12 @@ use PVE::Firewall;
use base qw(PVE::RESTHandler);
+sub lock_config {
+my ($class, $param, $code) = @_;
+
+die "implement this in subclass";
+}
+
sub load_config {
my ($class, $param) = @_;
@@ -638,6 +668,12 @@ sub rule_env {
return 'cluster';
}
+sub lock_config {
+my ($class, $param, $code) = @_;
+
+PVE::Firewall::lock_clusterfw_conf(10, $code, $param);
+}
+
sub load_config {
my ($class, $param) = @_;
@@ -680,6 +716,12 @@ sub rule_env {
return 'vm';
}
+sub lock_config {
+my ($class, $param, $code) = @_;
+
+PVE::Firewall::lock_vmfw_conf($param->{vmid}, 10, $code, $param);
+}
+
sub load_config {
my ($class, $param) = @_;
@@ -723,6 +765,12 @@ sub rule_env {
return 'ct';
}
+sub lock_config {
+my ($class, $param, $code) = @_;
+
+PVE::Firewall::lock_vmfw_conf($param->{vmid}, 10, $code, $param);
+}
+
sub load_config {
my ($class, $param) = @_;
diff --git a/src/PVE/API2/Firewall/Rules.pm b/src/PVE/API2/Firewall/Rules.pm
index 0e93a4a..1fde596 100644
--- a/src/PVE/API2/Firewall/Rules.pm
+++ b/src/PVE/API2/Firewall/Rules.pm
@@ -17,6 +17,12 @@ my $api_properties = {
},
};
+sub lock_config {
+my ($class, $param, $code) = @_;
+
+die "implement this in subclass";
+}
+
sub load_config {
my ($class, $param) = @_;
@@ -380,6 +386,12 @@ sub rule_env {
return 'group';
}
+sub lock_config {
+my ($class, $param, $code) = @_;
+
+PVE::Firewall::lock_clusterfw_conf(10, $code, $param);
+}
+
sub load_config {
my ($class, $param) = @_;
@@ -446,6 +458,12 @@ sub rule_env {
return 'cluster';
}
+sub lock_config {
+my ($class, $param, $code) = @_;
+
+PVE::Firewall::lock_clusterfw_conf(10, $code, $param);
+}
+
sub load_config {
my ($class, $param) = @_;
@@ -480,6 +498,12 @@ sub rule_env {
return 'host';
}
+sub lock_config {
+my ($class, $param, $code) = @_;
+
+PVE::Firewall::lock_hostfw_conf(10, $code, $param);
+}
+
sub load_config {
my ($class, $param) = @_;
@@ -518,6 +542,12 @@ sub rule_env {
return 'vm';
}
+sub lock_config {
+my ($
[pve-devel] [PATCH firewall 6/7] configs: warn about duplicate ipset entries
instead of silently dropping them when writing the config out.
Signed-off-by: Fabian Grünbichler
---
src/PVE/Firewall.pm | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index 7b26ac5..4d86032 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -2897,6 +2897,8 @@ sub generic_fw_config_parser {
}
return {} if !$raw;
+my $curr_group_keys = {};
+
my $linenr = 0;
while ($raw =~ /^\h*(.*?)\h*$/gm) {
my $line = $1;
@@ -2957,6 +2959,8 @@ sub generic_fw_config_parser {
}
$res->{$section}->{$group} = [];
+ $curr_group_keys = {};
+
$res->{ipset_comments}->{$group} = decode('utf8', $comment)
if $comment;
next;
@@ -3021,6 +3025,8 @@ sub generic_fw_config_parser {
} else {
$cidr = parse_ip_or_cidr($cidr);
}
+ die "duplicate ipset entry for '$cidr'\n"
+ if defined($curr_group_keys->{$cidr});
};
if (my $err = $@) {
chomp $err;
@@ -3044,6 +3050,7 @@ sub generic_fw_config_parser {
}
push @{$res->{$section}->{$group}}, $entry;
+ $curr_group_keys->{$cidr} = 1;
} else {
warn "$prefix: skip line - unknown section\n";
next;
@@ -3221,7 +3228,13 @@ my $format_ipsets = sub {
my $nethash = {};
foreach my $entry (@$options) {
- $nethash->{$entry->{cidr}} = $entry;
+ my $cidr = $entry->{cidr};
+ if (defined($nethash->{$cidr})) {
+ warn "ignoring duplicate ipset entry '$cidr'\n";
+ next;
+ }
+
+ $nethash->{$cidr} = $entry;
}
foreach my $cidr (sort keys %$nethash) {
--
2.20.1
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH firewall 7/7] rules: verify referenced security group exists
while this was already handled properly (as empty rules), adding this as
error makes it much more visible (in the GUI as well).
Signed-off-by: Fabian Grünbichler
---
src/PVE/Firewall.pm | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index 4d86032..40468e4 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -1623,6 +1623,8 @@ sub verify_rule {
if !$allow_groups;
&$add_error('action', "invalid characters in security group name")
if $action && ($action !~ m/^${security_group_name_pattern}$/);
+ &$add_error('action', "security group '$action' does not exist")
+ if $action && !defined($cluster_conf->{groups}->{$action});
} else {
&$add_error('type', "unknown rule type '$type'");
}
--
2.20.1
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [RFC cluster 2/2] cfs_lock: re-raise exceptions
so that API paths that raise an exception while holding a CFS lock
properly propagate that exception to the client, instead of the
stringified version with added noise about locks added to the front.
Signed-off-by: Fabian Grünbichler
---
Notes:
there seems to be nothing that matches on the prefix added by cfs_lock.
some API calls in pve-access-control, pve-storage and Replication would now
benefit from raising (parameter) exceptions instead of a plain death while
having a config file locked (mainly checking for duplicates when adding
entries)
data/PVE/Cluster.pm | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/data/PVE/Cluster.pm b/data/PVE/Cluster.pm
index b4de989..cce681b 100644
--- a/data/PVE/Cluster.pm
+++ b/data/PVE/Cluster.pm
@@ -609,7 +609,13 @@ my $cfs_lock = sub {
alarm($prev_alarm);
if ($err) {
-$@ = "error with cfs lock '$lockid': $err";
+ if (ref($err) eq 'PVE::Exception') {
+ # re-raise defined exceptions
+ $@ = $err;
+ } else {
+ # add lock info for plain errors
+ $@ = "error with cfs lock '$lockid': $err";
+ }
return undef;
}
--
2.20.1
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH firewall 4/7] clone_vmfw_conf: lock new config
Signed-off-by: Fabian Grünbichler
---
Notes:
best viewed with -w
src/PVE/Firewall.pm | 16 +---
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index eda39eb..7b26ac5 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -3282,13 +3282,15 @@ sub clone_vmfw_conf {
my $sourcevm_conffile = "$pvefw_conf_dir/$vmid.fw";
my $clonevm_conffile = "$pvefw_conf_dir/$newid.fw";
-if (-f $clonevm_conffile) {
- unlink $clonevm_conffile;
-}
-if (-f $sourcevm_conffile) {
- my $data = PVE::Tools::file_get_contents($sourcevm_conffile);
- PVE::Tools::file_set_contents($clonevm_conffile, $data);
-}
+lock_vmfw_conf($newid, 10, sub {
+ if (-f $clonevm_conffile) {
+ unlink $clonevm_conffile;
+ }
+ if (-f $sourcevm_conffile) {
+ my $data = PVE::Tools::file_get_contents($sourcevm_conffile);
+ PVE::Tools::file_set_contents($clonevm_conffile, $data);
+ }
+});
}
sub read_vm_firewall_configs {
--
2.20.1
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH firewall 1/7] configs: add locking helpers
to allow some level of safe concurrent config modification, instead of
the current free for all.
Signed-off-by: Fabian Grünbichler
---
Notes:
require pve-cluster that provides cfs_lock_firewall, or switching to
cfs_lock_domain as mentioned in pve-cluster#1
lock_hostfw_conf could also use a node-local lock, but IMHO it's easier to
have
the same locking semantics/interface across all three levels (especially if
we
do the second patch in pve-cluster).
it's easy enough to switch out though.
src/PVE/Firewall.pm | 32
1 file changed, 32 insertions(+)
diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index d22b15a..eda39eb 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -3053,6 +3053,8 @@ sub generic_fw_config_parser {
return $res;
}
+# this is only used to prevent concurrent runs of rule compilation/application
+# see lock_*_conf for cfs locks protectiong config modification
sub run_locked {
my ($code, @param) = @_;
@@ -3101,6 +3103,18 @@ sub read_local_vm_config {
return $vmdata;
};
+sub lock_vmfw_conf {
+my ($vmid, $timeout, $code, @param) = @_;
+
+die "can't lock VM firewall config for undefined VMID\n"
+ if !defined($vmid);
+
+my $res = PVE::Cluster::cfs_lock_firewall("vm-$vmid", $timeout, $code,
@param);
+die $@ if $@;
+
+return $res;
+}
+
sub load_vmfw_conf {
my ($cluster_conf, $rule_env, $vmid, $dir) = @_;
@@ -3448,6 +3462,15 @@ my $set_global_log_ratelimit = sub {
}
};
+sub lock_clusterfw_conf {
+my ($timeout, $code, @param) = @_;
+
+my $res = PVE::Cluster::cfs_lock_firewall("cluster", $timeout, $code,
@param);
+die $@ if $@;
+
+return $res;
+}
+
sub load_clusterfw_conf {
my ($filename) = @_;
@@ -3511,6 +3534,15 @@ sub save_clusterfw_conf {
}
}
+sub lock_hostfw_conf {
+my ($timeout, $code, @param) = @_;
+
+my $res = PVE::Cluster::cfs_lock_firewall("host-$nodename", $timeout,
$code, @param);
+die $@ if $@;
+
+return $res;
+}
+
sub load_hostfw_conf {
my ($cluster_conf, $filename) = @_;
--
2.20.1
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH firewall/cluster 0/9] add locking to firewall config changes
the second cluster patch is optional, but improves usability of
non-worker API calls that do
cfs_lock_foo(..., sub {
raise_foo
});
the last three firewall patches are unrelated bug fixes that I found
while testing.
pve-cluster:
Fabian Grünbichler (2):
cfs_lock: add firewall lock helper
cfs_lock: re-raise exceptions
data/PVE/Cluster.pm | 16 +++-
1 file changed, 15 insertions(+), 1 deletion(-)
pve-firewall:
Fabian Grünbichler (7):
configs: add locking helpers
api: add locking helpers
api: lock configs
clone_vmfw_conf: lock new config
api/ipsets: parse_cidr before checking for duplicates
configs: warn about duplicate ipset entries
rules: verify referenced security group exists
src/PVE/API2/Firewall/Aliases.pm | 104 ++--
src/PVE/API2/Firewall/Cluster.pm | 36 +++---
src/PVE/API2/Firewall/Groups.pm | 52
src/PVE/API2/Firewall/Host.pm| 42 ---
src/PVE/API2/Firewall/IPSet.pm | 204 +--
src/PVE/API2/Firewall/Rules.pm | 130 ++--
src/PVE/API2/Firewall/VM.pm | 43 +++
src/PVE/Firewall.pm | 65 --
8 files changed, 449 insertions(+), 227 deletions(-)
--
2.20.1
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH cluster 1/2] cfs_lock: add firewall lock helper
Signed-off-by: Fabian Grünbichler
---
alternatively we could re-use 'cfs_lock_domain', which is currently
only used by HA and was intended as general-purpose cfs_lock wrapper..
I'd shorten the firewall- prefix to fw- in that case though.
domain-fw-host-$foo might be more confusing to end users though, as it's
pretty visible in error messages.
data/PVE/Cluster.pm | 8
1 file changed, 8 insertions(+)
diff --git a/data/PVE/Cluster.pm b/data/PVE/Cluster.pm
index 068d626..b4de989 100644
--- a/data/PVE/Cluster.pm
+++ b/data/PVE/Cluster.pm
@@ -659,6 +659,14 @@ sub cfs_lock_authkey {
$cfs_lock->('authkey', $timeout, $code, @param);
}
+sub cfs_lock_firewall {
+my ($scope, $timeout, $code, @param) = @_;
+
+my $lockid = "firewall-$scope";
+
+$cfs_lock->($lockid, $timeout, $code, @param);
+}
+
my $log_levels = {
"emerg" => 0,
"alert" => 1,
--
2.20.1
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied-series: [PATCH v2 container 0/5] futher cgroup improvements
On 4/9/20 1:28 PM, Wolfgang Bumiller wrote: > v2 just has minor fixups to the previous series. > > Initial cover letter: > > There's probably more to come, and not all our settings are yet > automatically converted to cgroupv2 in `update_lxc_config`, but the one > most people probably run into is the cpuset one, so this at least let's > some containers start. > > Note that for systemd containers you also need to add the following raw > config entry: > > lxc.mount.auto: cgroup:rw:force > > Depending on how the non-systemd distros fare with that we might make > this either default or an option. > > I've also reorganized the cgroup path fetching functions a bit. The > `cgroup_mode()` call is actually not all that useful outside, since in > theory the hybrid layout allows you to choose which controllers are > in which cgroup. So this now contains a combined "fetch path & version > of controller X" helper method. > > The last patch should be viewed with `-w`, because a lot of it is > indentation. > > Wolfgang Bumiller (5): > config: whitelist lxc.cgroup2 raw keys > consider lxc.cgroup2.cpuset.cpus as explicit cpuset > cgroup: more generic get_cgroup_controllers function > support cpuset cgroupv2 controller > cgroup: use version returned from get_path() > > src/PVE/LXC.pm| 21 +-- > src/PVE/LXC/CGroup.pm | 322 +++--- > src/PVE/LXC/Config.pm | 2 +- > 3 files changed, 215 insertions(+), 130 deletions(-) > applied series, with the s/Cgroup::/CGroup::/ fixup for 3/5, thanks! ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
