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

virtualbox: Fix crash when handling deeply nested hostvars #5348

Merged

Conversation

basicdays
Copy link
Contributor

@basicdays basicdays commented Oct 12, 2022

SUMMARY

Fixes #5332

Skip parsing values with keys that have both a value and nested data. Skip parsing values that are nested more than two keys deep.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

virtualbox

ADDITIONAL INFORMATION

Taken from the issue, calling ansible all -i vbox.yml --list -vvv produces an error that contains the following:

[WARNING]: * Failed to parse /home/etienne/playbook/vbox.yml with auto plugin: 'str' object does not support item assignment
File "/home/etienne/.local/lib/python3.8/site-packages/ansible/inventory/manager.py", line 290, in parse_source
plugin.parse(self._inventory, self._loader, source, cache=cache)
File "/home/etienne/.local/lib/python3.8/site-packages/ansible/plugins/inventory/auto.py", line 59, in parse
plugin.parse(inventory, loader, path, cache=cache)
File "/home/etienne/.ansible/collections/ansible_collections/community/general/plugins/inventory/virtualbox.py", line 281, in parse

This occurs when a virtualbox instance from the command VBoxManage showvminfo <uuid|vmname> has a key with both a value and nested data. This can be seen with the following vminfo segment:

Recording screens:           1
 Screen 0:
    Enabled:                 yes
    ID:                      0
    Record video:            yes
    Record audio:            no
    Destination:             File
    File:                    /Users/<USER>/VirtualBox VMs/<VBOX_INSTANCE>/<VBOX_INSTANCE>-screen0.webm
    Options:                 vc_enabled=true,ac_enabled=false,ac_profile=med
    Video dimensions:        1024x768
    Video rate:              512kbps
    Video FPS:               25fps

This fix now ignores the Recording screens value of 1. In addition, the nested information under Screen 0 is ignored.

This results in a hostvars dictionary of:

{
    "_meta": {
         hostvars": {
            "<VBOX_INSTANCE>" {
                ...
                "vbox_Recording_screens": {
                    "vbox_Screen_0": ""
                },
                ...
            }
        }
    }
}

Since the showvminfo is using a human readable output, I am not certain how nested data is formatted across the various Virtualbox versions. Because of that, I've decided to just skip deeply nested data. Another avenue to pull information
about a vm instance could be using VBoxManage showvminfo --machinereadable <uuid|vmname> instead. However
the data keys would change, meaning this would be a breaking change.

For reference, the following is the same information with the machinereadable option:

recording_screens=1
 rec_screen0
rec_screen_enabled="on"
rec_screen_id=0
rec_screen_video_enabled="on"
rec_screen_audio_enabled="off"
rec_screen_dest="File"
rec_screen_dest_filename="/Users/paulsanchez/VirtualBox VMs/rails-sandbox/rails-sandbox-screen0.webm"
rec_screen_opts="vc_enabled=true,ac_enabled=false,ac_profile=med"
rec_screen_video_res_xy="1024x768"
rec_screen_video_rate_kbps=512
rec_screen_video_fps=25

@ansibullbot ansibullbot added WIP Work in progress bug This issue/PR relates to a bug inventory inventory plugin new_contributor Help guide this first time contributor plugins plugin (any type) traceback labels Oct 12, 2022
- Skip parsing values with keys that have both a value and nested data.
- Skip parsing values that are nested more than two keys deep.
@basicdays basicdays marked this pull request as ready for review October 12, 2022 01:36
@ansibullbot ansibullbot removed the WIP Work in progress label Oct 12, 2022
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for creating this PR! I have a small comment regarding the changelog fragment, besides that the change looks good to me. (I'm not using the plugin, so I cannot test it though.)

@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-4 and removed backport-4 labels Oct 12, 2022
Co-authored-by: Felix Fontein <felix@fontein.de>
@github-actions
Copy link

github-actions bot commented Oct 12, 2022

Docs Build 📝

Thank you for contribution!✨

This PR has been merged and your docs changes will be incorporated when they are next published.

Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good as far as I can judge it.

Copy link
Collaborator

@russoz russoz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Oct 18, 2022
@felixfontein felixfontein merged commit b0bb994 into ansible-collections:main Oct 18, 2022
@patchback
Copy link

patchback bot commented Oct 18, 2022

Backport to stable-5: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-5/b0bb994c3e965381a7ce1f3b2dae6280d7e8b741/pr-5348

Backported as #5381

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

@felixfontein
Copy link
Collaborator

@basicdays thanks for your contribution!
@russoz thanks for reviewing!

patchback bot pushed a commit that referenced this pull request Oct 18, 2022
* virtualbox: Fix nested data parsing

- Skip parsing values with keys that have both a value and nested data.
- Skip parsing values that are nested more than two keys deep.

* Update changelogs/fragments/5348-fix-vbox-deeply-nested-hostvars.yml

Co-authored-by: Felix Fontein <felix@fontein.de>

Co-authored-by: Felix Fontein <felix@fontein.de>
(cherry picked from commit b0bb994)
felixfontein pushed a commit that referenced this pull request Oct 18, 2022
…5381)

* virtualbox: Fix nested data parsing

- Skip parsing values with keys that have both a value and nested data.
- Skip parsing values that are nested more than two keys deep.

* Update changelogs/fragments/5348-fix-vbox-deeply-nested-hostvars.yml

Co-authored-by: Felix Fontein <felix@fontein.de>

Co-authored-by: Felix Fontein <felix@fontein.de>
(cherry picked from commit b0bb994)

Co-authored-by: Paul Sanchez <124954+basicdays@users.noreply.github.com>
mpaulon pushed a commit to mpaulon/community.general that referenced this pull request Oct 18, 2022
…ollections#5348)

* virtualbox: Fix nested data parsing

- Skip parsing values with keys that have both a value and nested data.
- Skip parsing values that are nested more than two keys deep.

* Update changelogs/fragments/5348-fix-vbox-deeply-nested-hostvars.yml

Co-authored-by: Felix Fontein <felix@fontein.de>

Co-authored-by: Felix Fontein <felix@fontein.de>
@basicdays basicdays deleted the feature/fix-vbox-hostvars branch October 18, 2022 16:39
russoz pushed a commit to russoz-ansible/community.general that referenced this pull request Nov 5, 2022
…ollections#5348)

* virtualbox: Fix nested data parsing

- Skip parsing values with keys that have both a value and nested data.
- Skip parsing values that are nested more than two keys deep.

* Update changelogs/fragments/5348-fix-vbox-deeply-nested-hostvars.yml

Co-authored-by: Felix Fontein <felix@fontein.de>

Co-authored-by: Felix Fontein <felix@fontein.de>
bratwurzt pushed a commit to bratwurzt/community.general that referenced this pull request Nov 7, 2022
…ollections#5348)

* virtualbox: Fix nested data parsing

- Skip parsing values with keys that have both a value and nested data.
- Skip parsing values that are nested more than two keys deep.

* Update changelogs/fragments/5348-fix-vbox-deeply-nested-hostvars.yml

Co-authored-by: Felix Fontein <felix@fontein.de>

Co-authored-by: Felix Fontein <felix@fontein.de>
This was referenced Nov 20, 2022
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 inventory inventory plugin new_contributor Help guide this first time contributor plugins plugin (any type) traceback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to parse virtualbox inventory file "'str' object does not support item assignment"
4 participants