Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow tags strings containing commas in proxmox inventory plug-in #1949

Merged
merged 10 commits into from
Mar 11, 2021

Conversation

Ajpantuso
Copy link
Collaborator

@Ajpantuso Ajpantuso commented Mar 2, 2021

SUMMARY

Allowing proxmox tags strings containing commas to be passed as facts

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

plugins/inventory/proxmox.py

ADDITIONAL INFORMATION

The current behavior of the proxmox inventory plugin will attempt to parse values containing commas from the PVE API configs endpoint as dictionaries. However tags strings disallow '=' characters which will always cause parsing to fail for tags strings presented as comma separated list. This change will explicitly allow the 'tags' config to bypass dict parsing.

@ansibullbot ansibullbot added affects_2.10 community_review feature This issue/PR relates to a feature request inventory inventory plugin needs_triage new_contributor Help guide this first time contributor plugins plugin (any type) tests tests unit tests/unit labels Mar 2, 2021
@Ajpantuso Ajpantuso changed the title Included explicit parsing for proxmox guest tags and updated correspo… Adding tag parsing to proxmox inventory plugin Mar 2, 2021
@Ajpantuso Ajpantuso changed the title Adding tag parsing to proxmox inventory plugin Adding tag parsing to proxmox inventory plug-in Mar 2, 2021
@Ajpantuso Ajpantuso marked this pull request as ready for review March 2, 2021 23:37
@Ajpantuso
Copy link
Collaborator Author

I did notice issue #1503 was a similar concept to this PR, but as stated in the summary the proxmox tag string is not suited for key/value pairs and using the "notes" field as mentioned in the issue is a dead-end as the proxmox API has no method for returning it. (Probably because it's relatively free-formatted text)

Proxmox only permits periods when surrounded by alphanumeric characters
plugins/inventory/proxmox.py Outdated Show resolved Hide resolved
1949-proxmox-inventory-tags.yml Outdated Show resolved Hide resolved
@felixfontein
Copy link
Collaborator

CC @Aversiste @Thulium-Drake

@Ajpantuso Ajpantuso changed the title Adding tag parsing to proxmox inventory plug-in Allow tags strings containing commas in proxmox inventory plug-in Mar 3, 2021
@ansibullbot ansibullbot added the bug This issue/PR relates to a bug label Mar 3, 2021
@pabelanger
Copy link

recheck

@Thulium-Drake
Copy link
Contributor

Maybe a silly question, as I don't use it (or never knew I needed this in life ;-) ), but where would one even configure tags for VMs? 😅
So I can test your change 😄

@ansibullbot ansibullbot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed community_review labels Mar 3, 2021
@Ajpantuso
Copy link
Collaborator Author

Maybe a silly question, as I don't use it (or never knew I needed this in life ;-) ), but where would one even configure tags for VMs? 😅
So I can test your change 😄

Not a silly question at all because PVE does not display tags in the UI and has a single line in the docs for them.

You can use the CLI on a proxmox node (I'm testing on 6.3-3) or the equivalent paths from the JSON API to set and get tags:
pvesh set /nodes/{node}/{lxc | qemu}/{vmid}/config -tags "tag string"
pvesh get /nodes/{node}/lxc | qemu}/{vmid}/config

For reference I'm using a terraform provider to set these and honestly despite this PR I wouldn't use tags in prod.
One big caveat is that the api paths for lxc throw 400's whenever a property is null even if it handles the method operation correctly so client's report failure for no reason. (I reported this on the PVE forums)

Also I tried using the "notes" field again like in issue #1503 and they are in fact returned by the API, but as the "description" field.
If you fill the notes with something like "key1=value1,key2=value2,..." the inventory plugin will parse it is a dict and you can use those keys for keyed groups with the constructed plugin.

However just like with "tags" if you wanted arbitrary text containing commas the inventory plugin will fail to parse returning no description fact.

So with the current functionality of the inventory plugin you can leverage what is probably a symptom of the side-effect caused by always attempting to parse strings containing commas as dicts to get tagged instances.

@Thulium-Drake
Copy link
Contributor

Thulium-Drake commented Mar 7, 2021

Nice function, I must say it's kinda obscure (which IMHO is a shame, stuff like this can be really useful), maybe the Proxmox devs will actually expose it via the GUI in the future. FYI, you can also tag machines by adding the following key to a config file in /etc/pve/lxc|qemu/$VMID.conf

tags: my,first,tags

However, I'd like to propose 1 addition. My gut feeling is that tags themselves are separated by commas so they can form a list.

Can you convert the string with tags to a list? That's probably more in line of what people expect.

@ansibullbot

This comment has been minimized.

@ansibullbot ansibullbot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Mar 7, 2021
@Thulium-Drake
Copy link
Contributor

Thulium-Drake commented Mar 10, 2021

Tested your code, results are very neat:

...
    "proxmox_swap": 2048,
    "proxmox_tags": "some,tags",
    "proxmox_tags_parsed": [
        "some",
        "tags"
    ],
    "proxmox_unprivileged": 1,
    "proxmox_vmid": "106",

...

LGTM

changelogs/fragments/1949-proxmox-inventory-tags.yml Outdated Show resolved Hide resolved
changelogs/fragments/1949-proxmox-inventory-tags.yml Outdated Show resolved Hide resolved
plugins/inventory/proxmox.py Outdated Show resolved Hide resolved
@felixfontein felixfontein added backport-1 check-before-release PR will be looked at again shortly before release and merged if possible. labels Mar 11, 2021
@ansibullbot ansibullbot added community_review and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Mar 11, 2021
@felixfontein felixfontein merged commit d0bb74a into ansible-collections:main Mar 11, 2021
patchback bot pushed a commit that referenced this pull request Mar 11, 2021
)

* Included explicit parsing for proxmox guest tags and updated corresponding unit test with tags key

* Including changelog fragment for PR 1949

* Removed ellipsis from test

Proxmox only permits periods when surrounded by alphanumeric characters

* Corrected punctuation for changelog entry

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

* Allowing tags string to contain commas

* Incorporated new parsed tags fact with bugfix

* Correcting whitespace issues

* Update changelogs/fragments/1949-proxmox-inventory-tags.yml

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

* Update plugins/inventory/proxmox.py

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

* Update changelogs/fragments/1949-proxmox-inventory-tags.yml

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

Co-authored-by: Felix Fontein <[email protected]>
(cherry picked from commit d0bb74a)
patchback bot pushed a commit that referenced this pull request Mar 11, 2021
)

* Included explicit parsing for proxmox guest tags and updated corresponding unit test with tags key

* Including changelog fragment for PR 1949

* Removed ellipsis from test

Proxmox only permits periods when surrounded by alphanumeric characters

* Corrected punctuation for changelog entry

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

* Allowing tags string to contain commas

* Incorporated new parsed tags fact with bugfix

* Correcting whitespace issues

* Update changelogs/fragments/1949-proxmox-inventory-tags.yml

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

* Update plugins/inventory/proxmox.py

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

* Update changelogs/fragments/1949-proxmox-inventory-tags.yml

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

Co-authored-by: Felix Fontein <[email protected]>
(cherry picked from commit d0bb74a)
@felixfontein felixfontein removed backport-1 check-before-release PR will be looked at again shortly before release and merged if possible. labels Mar 11, 2021
@Ajpantuso Ajpantuso deleted the proxmox_inv_tags branch March 11, 2021 21:08
@felixfontein
Copy link
Collaborator

@Ajpantuso thanks for implementing this!
@Thulium-Drake thanks for reviewing!

felixfontein pushed a commit that referenced this pull request Mar 12, 2021
) (#1998)

* Included explicit parsing for proxmox guest tags and updated corresponding unit test with tags key

* Including changelog fragment for PR 1949

* Removed ellipsis from test

Proxmox only permits periods when surrounded by alphanumeric characters

* Corrected punctuation for changelog entry

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

* Allowing tags string to contain commas

* Incorporated new parsed tags fact with bugfix

* Correcting whitespace issues

* Update changelogs/fragments/1949-proxmox-inventory-tags.yml

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

* Update plugins/inventory/proxmox.py

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

* Update changelogs/fragments/1949-proxmox-inventory-tags.yml

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

Co-authored-by: Felix Fontein <[email protected]>
(cherry picked from commit d0bb74a)

Co-authored-by: Ajpantuso <[email protected]>
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Apr 2, 2021
v3.2.0

community.crypto
- acme module_utils - the ``acme`` module_utils has been split up into several Python modules (ansible-collections/community.crypto#184).
- acme_* modules - codebase refactor which should not be visible to end-users (ansible-collections/community.crypto#184).
- acme_* modules - support account key passphrases for ``cryptography`` backend (ansible-collections/community.crypto#197, ansible-collections/community.crypto#207).
- acme_certificate_revoke - support revoking by private keys that are passphrase protected for ``cryptography`` backend (ansible-collections/community.crypto#207).
- acme_challenge_cert_helper - add ``private_key_passphrase`` parameter (ansible-collections/community.crypto#207).

community.docker
- docker_swarm_service - change ``publish.published_port`` option from mandatory to optional. Docker will assign random high port if not specified (ansible-collections/community.docker#99).

community.general
- archive - refactored some reused code out into a couple of functions (ansible-collections/community.general#2061).
- csv module utils - new module_utils for shared functions between ``from_csv`` filter and ``read_csv`` module (ansible-collections/community.general#2037).
- ipa_sudorule - add support for setting sudo runasuser (ansible-collections/community.general#2031).
- jenkins_job - add a ``validate_certs`` parameter that allows disabling TLS/SSL certificate validation (ansible-collections/community.general#255).
- kibana_plugin - add parameter for passing ``--allow-root`` flag to kibana and kibana-plugin commands (ansible-collections/community.general#2014).
- proxmox - added ``purge`` module parameter for use when deleting lxc's with HA options (ansible-collections/community.general#2013).
- proxmox inventory plugin - added ``tags_parsed`` fact containing tags parsed as a list (ansible-collections/community.general#1949).
- proxmox_kvm - added new module parameter ``tags`` for use with PVE 6+ (ansible-collections/community.general#2000).
- rax - elements of list parameters are now validated (ansible-collections/community.general#2006).
- rax_cdb_user - elements of list parameters are now validated (ansible-collections/community.general#2006).
- rax_scaling_group - elements of list parameters are now validated (ansible-collections/community.general#2006).
- read_csv - refactored read_csv module to use shared csv functions from csv module_utils (ansible-collections/community.general#2037).
- redfish_* modules, redfish_utils module utils - add support for Redfish session create, delete, and authenticate (ansible-collections/community.general#1975).
- snmp_facts - added parameters ``timeout`` and ``retries`` to module (ansible-collections/community.general#980).
- vdo - add ``force`` option (ansible-collections/community.general#2101).

community.network
- edgeos_config - match the space after ``set`` and ``delete`` commands (ansible-collections/community.network#199).
- nclu - execute ``net commit description <description>`` only if changed ``net pending``'s diff field (ansible-collections/community.network#219).

community.postgresql
- postgresql_info - add the ``patch``, ``full``, and ``raw`` values of the ``version`` return value (ansible-collections/community.postgresql#68).
- postgresql_ping - add the ``patch``, ``full``, and ``raw`` values of the ``server_version`` return value (ansible-collections/community.postgresql#70).

community.zabbix
- zabbix_agent - added support for installations on arm64 systems (ansible-collections/community.zabbix#320).
- zabbix_proxy - now supports configuring StatsAllowedIP (ansible-collections/community.zabbix#337).
- zabbix_server - added support for installtions on arm64 systems (ansible-collections/community.zabbix#320).
- zabbix_web - added support for installtions on arm64 systems (ansible-collections/community.zabbix#320).

dellemc.openmanage
- ome_template - Allows to deploy a template on device groups.

hetzner.hcloud
- Add firewalls to hcloud_server module

ovirt.ovirt
- cluster_upgrade - Add correlation-id header (oVirt/ovirt-ansible-collection#222).
- engine_setup - Add skip renew pki confirm (oVirt/ovirt-ansible-collection#228).
- examples - Add recipe for removing DM device (oVirt/ovirt-ansible-collection#233).
- hosted_engine_setup - Filter devices with unsupported bond mode (oVirt/ovirt-ansible-collection#226).
- infra - Add reboot host parameters (oVirt/ovirt-ansible-collection#231).
- ovirt_disk - Add SATA support (oVirt/ovirt-ansible-collection#225).
- ovirt_user - Add ssh_public_key (oVirt/ovirt-ansible-collection#232)

purestorage.flasharray
- purefa_maintenance - New module to set maintenance windows
- purefa_pg - Add support to rename protection groups
- purefa_syslog - Add support for naming SYSLOG servers for Purity//FA 6.1 or higher

purestorage.flashblade
- purefb_certs - Add update functionality for array cert
- purefb_fs - Add multiprotocol ACL support
- purefb_info - Add information regarding filesystem multiprotocol (where available)
- purefb_info - Add new parameter to provide details on admin users
- purefb_info - Add replication performace statistics
- purefb_s3user - Add ability to remove an S3 users existing access key
@Andersson007
Copy link
Contributor

@Ajpantuso hi, thanks for your contribution to proxmox and other modules/plugins! We could add your GH login to .github/BOTMETA.yml in a corresponding team as a maintainer.
You'll be notified about Issues/PRs related to this stuff and your shipit will be counted by bot for automerge (needs two for bugfixes and minor changes).
What do you think?

@Ajpantuso
Copy link
Collaborator Author

@Ajpantuso hi, thanks for your contribution to proxmox and other modules/plugins! We could add your GH login to .github/BOTMETA.yml in a corresponding team as a maintainer.
You'll be notified about Issues/PRs related to this stuff and your shipit will be counted by bot for automerge (needs two for bugfixes and minor changes).
What do you think?

Sure, I am willing to help maintain the modules/plugins I have contributed to.

@Andersson007
Copy link
Contributor

@Ajpantuso great! Thanks! I created a PR #2286

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue/PR relates to a bug community_review feature This issue/PR relates to a feature request has_issue inventory inventory plugin needs_triage new_contributor Help guide this first time contributor plugins plugin (any type) tests tests unit tests/unit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants