[pve-devel] [PATCH qemu-server] cloudinit: allow non-root users to set ciupgrade option

[pve-devel] [PATCH qemu-server] cloudinit: allow non-root users to set ciupgrade option

[Friedrich Weber]

The new ciupgrade option was missing in $cloudinitoptions in
PVE::API2::Qemu, so $check_vm_modify_config_perm defaulted to
requiring root@pam for modifying the option. To fix this, add
ciupgrade to $cloudinitoptions. This also fixes an issue where
ciupgrade was missing in the output of `qm cloudinit pending`,
as it also relies on $cloudinitoptions.

This issue was originally reported in the forum [0].

Also add a comment to avoid similar issues when adding new options in
the future.

[0]: https://forum.proxmox.com/threads/131043/

Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
---

Notes:
    Not sure if this is the proper fix. Instead of maintaining two lists
    of cloudinit options, we could generate $cloudinitoptions from
    $confdesc_cloudinit?

 PVE/API2/Qemu.pm  | 1 +
 PVE/QemuServer.pm | 1 +
 2 files changed, 2 insertions(+)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 5930713..d57b8e8 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -562,6 +562,7 @@ my $cloudinitoptions = {
     cipassword => 1,
     citype => 1,
     ciuser => 1,
+    ciupgrade => 1,
     nameserver => 1,
     searchdomain => 1,
     sshkeys => 1,
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 430661a..e41d017 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -760,6 +760,7 @@ my $cicustom_fmt = {
 };
 PVE::JSONSchema::register_format('pve-qm-cicustom', $cicustom_fmt);
 
+# any new option might need to be added to $cloudinitoptions in PVE::API2::Qemu
 my $confdesc_cloudinit = {
     citype => {
 	optional => 1,
-- 
2.39.2

[pve-devel] applied: [PATCH qemu-server] cloudinit: allow non-root users to set ciupgrade option

[Thomas Lamprecht]

On 24/07/2023 13:33, Friedrich Weber wrote:
> The new ciupgrade option was missing in $cloudinitoptions in
> PVE::API2::Qemu, so $check_vm_modify_config_perm defaulted to
> requiring root@pam for modifying the option. To fix this, add
> ciupgrade to $cloudinitoptions. This also fixes an issue where
> ciupgrade was missing in the output of `qm cloudinit pending`,
> as it also relies on $cloudinitoptions.
> 
> This issue was originally reported in the forum [0].
> 
> Also add a comment to avoid similar issues when adding new options in
> the future.
> 
> [0]: https://forum.proxmox.com/threads/131043/
> 
> Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
> ---
> 
> Notes:
>     Not sure if this is the proper fix. Instead of maintaining two lists
>     of cloudinit options, we could generate $cloudinitoptions from
>     $confdesc_cloudinit?

It's not making anything worse and is a good stop-gap, but yeah those
list is a bit too distant from the schema definition.

How about a get_vm_user_cloudinit_options helper located directly
below the format definition, filtering out those keys that do not
make sense, or are off limits, and use that?

Maybe with some wider clean-up for the others? But this is also a
bit thankless work and would definitively need some test harness
to ensure no regression gets in.

> 
>  PVE/API2/Qemu.pm  | 1 +
>  PVE/QemuServer.pm | 1 +
>  2 files changed, 2 insertions(+)
> 
>

applied, thanks!

Re: [pve-devel] applied: [PATCH qemu-server] cloudinit: allow non-root users to set ciupgrade option

[Friedrich Weber]

On 25/07/2023 11:54, Thomas Lamprecht wrote:
> How about a get_vm_user_cloudinit_options helper located directly
> below the format definition, filtering out those keys that do not
> make sense, or are off limits, and use that?
The helper sounds good! AFAICT, $cloudinitoptions contains exactly all
keys of $confdesc_cloudinit anyway -- so currently the helper could just
return a hash that maps all keys of $confdesc_cloudinit to 1. If it's
located close to the definition of $confdesc_cloudinit, I'd hope that it
makes adding new options in the future less error-prone (even for adding
a hypothetical option that should actually be modifiable by root only)
I'll look into it.