proxmox_kvm - new param to support unsafe updates #7843

nxet · 2024-01-14T17:07:40Z

SUMMARY

Add parameter update_unsafe to avoid limitations when updating dangerous values.

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

proxmox_kvm

ADDITIONAL INFORMATION

This simply allows to override the limits present on specific config options during update.

ansibullbot · 2024-01-14T17:10:35Z

cc @Ajpantuso @Thulium-Drake @UnderGreen @helldorado @joshainglis @karmab
click here for bot help

felixfontein

Thanks for your contribution! I've added some first comments.

changelogs/fragments/7843-proxmox_kvm-update_unsafe.yml

plugins/modules/proxmox_kvm.py

Co-authored-by: Felix Fontein <[email protected]>

nxet · 2024-01-16T08:20:53Z

Thank you for taking the time to review the PR.

While we have the thread open, I'd like to ping the module creator @helldorado and quickly discuss the original update parameter.

While I understand that reducing the number of ways an user can shoot themself in the foot is always a good approach, in this instance I believe the module should not enforce this restriction, and instead transparently implement the Proxmox API.
This is based on the assumption that the API devs know better where to place hard limits on their interfaces (e.g. many calls are restricted to authentication as root@pam + password, completely disabling token auth).
Any extra layer of security implemented on our part should be exclusively related to ansible and its inner workings, otherwise it looks quite arbitrary IMO. This also means extra documentation to be maintained.
Not to mention it forces the user to keep an eye on these extra docs thus fragmenting the API knowledge base, and possibly requiring workarounds to achieve tasks otherwise easily doable with direct API access.

Therefore I believe the module should avoid this opinion, thus removing the lines of code clearing up "unsafe" parameters.

What do you think?
Why was this feature implemented in the first place?

Thanks for your time

felixfontein · 2024-01-21T13:07:57Z

I don't know the proxmox API (and proxmox in general), but generally options that can avoid in data loss that potentially surprises users should be taken care of specially. For disk-related options, it could be that changing their values would re-create the disks and thus make all the data stored on the current disk(s) vanish. TPM state could contain cryptographic keys that get lost (and can't be recovered if they weren't stored anywhere else). (Losing network access is inconvenient, but nothing that cannot be undone without permanent loss of data. So no idea why net is part of this.)

felixfontein · 2024-01-21T13:10:23Z

plugins/modules/proxmox_kvm.py

@@ -525,6 +525,12 @@
      - Update of O(pool) is disabled. It needs an additional API endpoint not covered by this module.
    type: bool
    default: false
+  update_unsafe:
+    description:
+      - If V(true), does not enforce limitations on parameters O(net), O(virtio), O(ide), O(sata), and O(scsi).


This is missing two affected parameters:

Suggested change

- If V(true), does not enforce limitations on parameters O(net), O(virtio), O(ide), O(sata), and O(scsi).

- If V(true), does not enforce limitations on parameters O(net), O(virtio), O(ide), O(sata), O(scsi), O(efidisk0), and O(tpmstate0).

felixfontein · 2024-01-21T13:12:05Z

Actually I just saw that the documentation for the update option contains some rationale, and also mentions why net is on the list.

Your PR should also update documentation for update to mention the new update_safe option.

nxet · 2024-01-31T13:12:38Z

Sorry it took a while.
I totally understand and agree with your point, it makes a lot of sense to implement this extra security step.
I just pushed a couple small improvements to the docs, let me know if there's anything else!

felixfontein

Looks good to me. If nobody objects, I'll merge this in ~a week.

patchback · 2024-02-07T13:30:49Z

Backport to stable-8: cherry-pick succeeded

Backport PR branch: patchback/backports/stable-8/c7a2e28daa262bebe45b76173ceca85b86043046/pr-7843

PR branch created, proceeding with making a PR.

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

* proxmox_kvm - new param to support unsafe updates * changelog fragments * Apply suggestions from code review Co-authored-by: Felix Fontein <[email protected]> * improved docs * updated `version_added` --------- Co-authored-by: Felix Fontein <[email protected]> (cherry picked from commit c7a2e28)

felixfontein · 2024-02-07T13:31:02Z

@nxet thanks for your contribution!

patchback · 2024-02-07T13:35:30Z

Backport to stable-8: cherry-pick succeeded

Backport PR branch: patchback/backports/stable-8/c7a2e28daa262bebe45b76173ceca85b86043046/pr-7843

PR branch created, proceeding with making a PR.

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

* proxmox_kvm - new param to support unsafe updates * changelog fragments * Apply suggestions from code review Co-authored-by: Felix Fontein <[email protected]> * improved docs * updated `version_added` --------- Co-authored-by: Felix Fontein <[email protected]> (cherry picked from commit c7a2e28)

…7954) proxmox_kvm - new param to support unsafe updates (#7843) * proxmox_kvm - new param to support unsafe updates * changelog fragments * Apply suggestions from code review Co-authored-by: Felix Fontein <[email protected]> * improved docs * updated `version_added` --------- Co-authored-by: Felix Fontein <[email protected]> (cherry picked from commit c7a2e28) Co-authored-by: nxet <[email protected]>

nxet · 2024-02-07T17:09:59Z

@felixfontein thanks to you for the review and merge!

nxet added 2 commits January 14, 2024 18:03

proxmox_kvm - new param to support unsafe updates

7bd9d0b

changelog fragments

bd2b41f

ansibullbot added feature This issue/PR relates to a feature request module module plugins plugin (any type) labels Jan 14, 2024

felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-8 Automatically create a backport for the stable-8 branch labels Jan 14, 2024

felixfontein reviewed Jan 14, 2024

View reviewed changes

changelogs/fragments/7843-proxmox_kvm-update_unsafe.yml Outdated Show resolved Hide resolved

plugins/modules/proxmox_kvm.py Outdated Show resolved Hide resolved

plugins/modules/proxmox_kvm.py Outdated Show resolved Hide resolved

Apply suggestions from code review

7940d43

Co-authored-by: Felix Fontein <[email protected]>

felixfontein reviewed Jan 21, 2024

View reviewed changes

ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Jan 29, 2024

improved docs

f11d8d7

ansibullbot removed the stale_ci CI is older than 7 days, rerun before merging label Jan 31, 2024

updated version_added

cbe38e4

felixfontein approved these changes Feb 1, 2024

View reviewed changes

felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Feb 7, 2024

felixfontein merged commit c7a2e28 into ansible-collections:main Feb 7, 2024
121 checks passed

felixfontein added backport-8 Automatically create a backport for the stable-8 branch and removed backport-8 Automatically create a backport for the stable-8 branch labels Feb 7, 2024

felixfontein mentioned this pull request Feb 7, 2024

[stable-8] proxmox_kvm - new param to support unsafe updates (#7843) #7954

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

proxmox_kvm - new param to support unsafe updates #7843

proxmox_kvm - new param to support unsafe updates #7843

nxet commented Jan 14, 2024

ansibullbot commented Jan 14, 2024

felixfontein left a comment

nxet commented Jan 16, 2024

felixfontein commented Jan 21, 2024

felixfontein Jan 21, 2024

felixfontein commented Jan 21, 2024

nxet commented Jan 31, 2024

felixfontein left a comment

patchback bot commented Feb 7, 2024 •

edited

Loading

felixfontein commented Feb 7, 2024

patchback bot commented Feb 7, 2024 •

edited

Loading

nxet commented Feb 7, 2024

	- If V(true), does not enforce limitations on parameters O(net), O(virtio), O(ide), O(sata), and O(scsi).
	- If V(true), does not enforce limitations on parameters O(net), O(virtio), O(ide), O(sata), O(scsi), O(efidisk0), and O(tpmstate0).

proxmox_kvm - new param to support unsafe updates #7843

proxmox_kvm - new param to support unsafe updates #7843

Conversation

nxet commented Jan 14, 2024

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

ansibullbot commented Jan 14, 2024

felixfontein left a comment

Choose a reason for hiding this comment

nxet commented Jan 16, 2024

felixfontein commented Jan 21, 2024

felixfontein Jan 21, 2024

Choose a reason for hiding this comment

felixfontein commented Jan 21, 2024

nxet commented Jan 31, 2024

felixfontein left a comment

Choose a reason for hiding this comment

patchback bot commented Feb 7, 2024 • edited Loading

Backport to stable-8: cherry-pick succeeded

felixfontein commented Feb 7, 2024

patchback bot commented Feb 7, 2024 • edited Loading

Backport to stable-8: cherry-pick succeeded

nxet commented Feb 7, 2024

patchback bot commented Feb 7, 2024 •

edited

Loading

patchback bot commented Feb 7, 2024 •

edited

Loading