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.

Sign up for GitHub

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

Jump to bottom

lldp: Handling attributes that are defined multiple times #9657

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

lldp: Handling attributes that are defined multiple times #9657

wants to merge 2 commits into from
+31 −5

Conversation

securitym0nkey
Copy link

@securitym0nkey securitym0nkey commented Jan 31, 2025 •
edited
Loading

This fixes crashes when the lldpctl output has lines for unknown tlvs that redefine a key in the middle of the nested dict data structure.

Also adds a function to handle attributes that are defined multiple times.

SUMMARY

When the lldpctl command outputs something like the following the module crashes with the following error

Traceback

Traceback (most recent call last):
  File "/home/autom8/.ansible/tmp/ansible-tmp-1738338445.7424972-37051-54323009070053/AnsiballZ_lldp.py", line 107, in <module>
    _ansiballz_main()
  File "/home/autom8/.ansible/tmp/ansible-tmp-1738338445.7424972-37051-54323009070053/AnsiballZ_lldp.py", line 99, in _ansiballz_main
    invoke_module(zipped_mod, temp_path, ANSIBALLZ_PARAMS)
  File "/home/autom8/.ansible/tmp/ansible-tmp-1738338445.7424972-37051-54323009070053/AnsiballZ_lldp.py", line 47, in invoke_module
    runpy.run_module(mod_name='ansible_collections.community.general.plugins.modules.lldp', init_globals=dict(_module_fqn='ansible_collections.community.general.plugins.modules.lldp', _modlib_path=modlib_path),
  File "/usr/lib/python3.8/runpy.py", line 207, in run_module
    return _run_module_code(code, init_globals, run_name, mod_spec)
  File "/usr/lib/python3.8/runpy.py", line 97, in _run_module_code
    _run_code(code, mod_globals, init_globals,
  File "/usr/lib/python3.8/runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "/tmp/ansible_community.general.lldp_payload_e18eaejw/ansible_community.general.lldp_payload.zip/ansible_collections/community/general/plugins/modules/lldp.py", line 86, in <module>
  File "/tmp/ansible_community.general.lldp_payload_e18eaejw/ansible_community.general.lldp_payload.zip/ansible_collections/community/general/plugins/modules/lldp.py", line 77, in main
  File "/tmp/ansible_community.general.lldp_payload_e18eaejw/ansible_community.general.lldp_payload.zip/ansible_collections/community/general/plugins/modules/lldp.py", line 70, in gather_lldp
TypeError: 'str' object does not support item assignment

Example output that causes this problem

lldp.eno1.unknown-tlvs.unknown-tlv.oui=00,90,69
lldp.eno1.unknown-tlvs.unknown-tlv.subtype=1
lldp.eno1.unknown-tlvs.unknown-tlv.len=12
lldp.eno1.unknown-tlvs.unknown-tlv=4A,5A,30,32,31,38,33,30,30,37,31,31
lldp.eno1.unknown-tlvs.unknown-tlv.oui=00,90,69
lldp.eno1.unknown-tlvs.unknown-tlv.subtype=12
lldp.eno1.unknown-tlvs.unknown-tlv.len=8
lldp.eno1.unknown-tlvs.unknown-tlv=00,00,00,65,00,00,00,00

As seen above such output has problematic output. lldp.eno1.unknown-tlvs.unknown-tlv has "subkeys" and is also set to a string (The comma separated bytes).

The fix will move the string value to the subkey "value". So the result looks like this:

                "unknown-tlvs": {
                    "unknown-tlv": {
                        "len": "8",
                        "oui": "00,90,69",
                        "subtype": "12",
                        "value": "00,00,00,65,00,00,00,00"
                    }
                }

Under normal condition we do not expect lldpctl to redefine values. Therefore the simple fix is to just ignore those lines.

Sometimes lldpctl outputs an attribute mutliple times, therefore this PR also introduces the multivalues options to control how to handle such output.

This new option defaults to false to keep the current behavior

Example lldpctl output having the same attribute multiple times

lldp.eno2.chassis.mgmt-ip=172.28.1.1
lldp.eno2.chassis.mgmt-ip=10.182.99.243
lldp.eno2.chassis.mgmt-ip=fe80::ce2d:e0ff:abce:ef09

If multivalues is true it results in this structure

{
    "lldp": {
        "eno2": {
            "chassis": {
                "mgmt-ip": [
                    "172.28.1.1",
                    "10.182.99.243",
                    "fe80::ce2d:e0ff:abce:ef09"
                ]
            }
        }
    }
}
ISSUE TYPE
  • Bugfix Pull Request
  • Feature Pull Request
COMPONENT NAME

lldp

ADDITIONAL INFORMATION

@securitym0nkey
lldp: Ignoring values for keys already defined
7b46703 
This fixes crashes when the lldpctl output has lines for unknown tlvs that
redefine a key in the middle of the nested dict data structure.
@ansibullbot ansibullbot added bug This issue/PR relates to a bug module module needs_maintainer new_contributor Help guide this first time contributor plugins plugin (any type) small_patch Hopefully easy to review traceback labels Jan 31, 2025
@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-9 Automatically create a backport for the stable-9 branch backport-10 Automatically create a backport for the stable-10 branch labels Jan 31, 2025
felixfontein
felixfontein reviewed Jan 31, 2025
View reviewed changes
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 for your contribution! Can you please add a changelog fragment? Thanks.

plugins/modules/lldp.py Outdated
@@ -67,7 +67,8 @@ def gather_lldp(module):
for path_component in path_components:
current_dict[path_component] = current_dict.get(path_component, {})
current_dict = current_dict[path_component]
current_dict[final] = value
if final not in current_dict:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure this is the right fix. The error was TypeError: 'str' object does not support item assignment, which means that current_dict was not a dictionary, but a string.

In that case, final not in current_dict will test whether final (if it happens to be a string) appears in the string final. (If final happens to be an integer, for example, this will cause another exception: TypeError: 'in <string>' requires string as left operand, not int.) I don't think this is what you intended.

Copy link
Author

Choose a reason for hiding this comment

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

The Idea of my fix was that when
lldp.eno1.unknown-tlvs.unknown-tlv=4A,5A,30,32,31,38,33,30,30,37,31,31 is processed it's not overwriting the dict at lldp['eno1']['unknown-tlvs']['unknown-tlv']. So current_dict never becomes a string in the first place if we prevent the overwriting behavior.

But indeed this is not the most robust fix. It would not prevent the problem if the lldp.eno1.unknown-tlvs.unknown-tlv=4A,5A,30,32,31,38,33,30,30,37,31,31 line is the first one in the output.

Let me comeback with a solution that covers more cases.

@securitym0nkey securitym0nkey changed the title lldp: Ignoring values for keys already defined lldp: Handling attributes that are defined multiple times Feb 1, 2025
@securitym0nkey
lldp: handling attributes that are defined multiple times
12c9d57 
- Fix crash caused by certain lldpctl output where an attribute is defined as branch and leaf
- Adds multivalues parameter to control behavior when lldpctl outputs an attribute multiple times
@securitym0nkey
Copy link
Author

I've added more robust handling of the lldpctl output. See the updated PR description.

@ansibullbot
Copy link
Collaborator

The test ansible-test sanity --test validate-modules [explain] failed with 5 errors:

plugins/modules/lldp.py:0:0: invalid-ansiblemodule-schema: AnsibleModule.argument_spec.multivalues.equired: extra keys not allowed @ data['argument_spec']['multivalues']['equired']. Got False
plugins/modules/lldp.py:75:0: unidiomatic-typecheck: Type comparison using type() found. Use isinstance() instead
plugins/modules/lldp.py:79:0: unidiomatic-typecheck: Type comparison using type() found. Use isinstance() instead
plugins/modules/lldp.py:85:0: unidiomatic-typecheck: Type comparison using type() found. Use isinstance() instead
plugins/modules/lldp.py:87:0: unidiomatic-typecheck: Type comparison using type() found. Use isinstance() instead

The test ansible-test sanity --test pylint [explain] failed with 4 errors:

plugins/modules/lldp.py:75:19: unidiomatic-typecheck: Use isinstance() rather than type() for a typecheck.
plugins/modules/lldp.py:79:41: unidiomatic-typecheck: Use isinstance() rather than type() for a typecheck.
plugins/modules/lldp.py:85:17: unidiomatic-typecheck: Use isinstance() rather than type() for a typecheck.
plugins/modules/lldp.py:87:17: unidiomatic-typecheck: Use isinstance() rather than type() for a typecheck.

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

plugins/modules/lldp.py:0:0: invalid-ansiblemodule-schema: AnsibleModule.argument_spec.multivalues.equired: extra keys not allowed @ data['argument_spec']['multivalues']['equired']. Got False

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

plugins/modules/lldp.py:0:0: invalid-ansiblemodule-schema: AnsibleModule.argument_spec.multivalues.equired: extra keys not allowed @ data['argument_spec']['multivalues']['equired']. Got False

The test ansible-test sanity --test pylint [explain] failed with 4 errors:

plugins/modules/lldp.py:75:19: unidiomatic-typecheck: Use isinstance() rather than type() for a typecheck.
plugins/modules/lldp.py:79:41: unidiomatic-typecheck: Use isinstance() rather than type() for a typecheck.
plugins/modules/lldp.py:85:17: unidiomatic-typecheck: Use isinstance() rather than type() for a typecheck.
plugins/modules/lldp.py:87:17: unidiomatic-typecheck: Use isinstance() rather than type() for a typecheck.

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

plugins/modules/lldp.py:0:0: invalid-ansiblemodule-schema: AnsibleModule.argument_spec.multivalues.equired: extra keys not allowed @ data['argument_spec']['multivalues']['equired']. Got False

The test ansible-test sanity --test pylint [explain] failed with 4 errors:

plugins/modules/lldp.py:75:19: unidiomatic-typecheck: Use isinstance() rather than type() for a typecheck.
plugins/modules/lldp.py:79:41: unidiomatic-typecheck: Use isinstance() rather than type() for a typecheck.
plugins/modules/lldp.py:85:17: unidiomatic-typecheck: Use isinstance() rather than type() for a typecheck.
plugins/modules/lldp.py:87:17: unidiomatic-typecheck: Use isinstance() rather than type() for a typecheck.

click here for bot help

@ansibullbot ansibullbot added ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed small_patch Hopefully easy to review labels Feb 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixfontein felixfontein felixfontein left review comments

Assignees
No one assigned
Labels
backport-9 Automatically create a backport for the stable-9 branch backport-10 Automatically create a backport for the stable-10 branch bug This issue/PR relates to a bug check-before-release PR will be looked at again shortly before release and merged if possible. ci_verified Push fixes to PR branch to re-run CI module module needs_maintainer needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR new_contributor Help guide this first time contributor plugins plugin (any type) traceback
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@securitym0nkey @ansibullbot @felixfontein