all lists on lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH qemu-server] cloudinit: allow non-root users to set ciupgrade option
@ 2023-07-24 11:33 Friedrich Weber
  2023-07-25  9:54 ` [pve-devel] applied: " Thomas Lamprecht
  0 siblings, 1 reply; 3+ messages in thread
From: Friedrich Weber @ 2023-07-24 11:33 UTC (permalink / raw)
  To: pve-devel

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





^ permalink raw reply	[flat|nested] 3+ messages in thread

* [pve-devel] applied: [PATCH qemu-server] cloudinit: allow non-root users to set ciupgrade option
  2023-07-24 11:33 [pve-devel] [PATCH qemu-server] cloudinit: allow non-root users to set ciupgrade option Friedrich Weber
@ 2023-07-25  9:54 ` Thomas Lamprecht
  2023-07-25 13:55   ` Friedrich Weber
  0 siblings, 1 reply; 3+ messages in thread
From: Thomas Lamprecht @ 2023-07-25  9:54 UTC (permalink / raw)
  To: Proxmox VE development discussion, Friedrich Weber

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!




^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [pve-devel] applied: [PATCH qemu-server] cloudinit: allow non-root users to set ciupgrade option
  2023-07-25  9:54 ` [pve-devel] applied: " Thomas Lamprecht
@ 2023-07-25 13:55   ` Friedrich Weber
  0 siblings, 0 replies; 3+ messages in thread
From: Friedrich Weber @ 2023-07-25 13:55 UTC (permalink / raw)
  To: Thomas Lamprecht, Proxmox VE development discussion

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.




^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2023-07-25 13:56 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-24 11:33 [pve-devel] [PATCH qemu-server] cloudinit: allow non-root users to set ciupgrade option Friedrich Weber
2023-07-25  9:54 ` [pve-devel] applied: " Thomas Lamprecht
2023-07-25 13:55   ` Friedrich Weber

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal