search

theforeman / foreman-ansible-modules

Ansible modules for interacting with the Foreman API and various plugin APIs such as Katello
GNU General Public License v3.0
149 stars 166 forks source link

looking up entities with the name set to an empty string sometimes results in unexpected errors #1771

Open evgeni opened 3 months ago

evgeni commented 3 months ago
SUMMARY

Because of the "empty string is special to unset things" logic introduced in f50bd16e9bf0dfe06762785434f3fd3d7e0c05f6, we do not immediately fail when such an input is provided, which leads to errors in the code later on that relies on the fact that the entity was properly found.

we probably should just not apply the "empty string to unset" logic to parameters that are set as required=True (like content_view in the content_view_version module, which the example below uses).

ISSUE TYPE
ANSIBLE VERSION

all

COLLECTION VERSION

since f50bd16e9bf0dfe06762785434f3fd3d7e0c05f6

KATELLO/FOREMAN VERSION

all

STEPS TO REPRODUCE
---
- hosts: localhost
  tasks:
    - name: lol
      theforeman.foreman.content_view_version:
        server_url: https://foreman.example.com
        validate_certs: false
        username: admin
        password: changeme
        organization: Default Organization
        content_view: ""
EXPECTED RESULTS
fatal: [localhost]: FAILED! => {"changed": false, "msg": "Found no results while searching for content_views with name=\"\""}
ACTUAL RESULTS
fatal: [localhost]: FAILED! => {"changed": false, "module_stderr": "/usr/lib/python3.12/site-packages/urllib3/connectionpool.py:1063: InsecureRequestWarning: Unverified HTTPS request is being made to host 'sat-stream-qa-rhel8.tanso.example.com'. Adding certificate verification is strongly advised. See: https://urllib3.readthedocs.io/en/1.26.x/advanced-usage.html#ssl-warnings
  warnings.warn(
/usr/lib/python3.12/site-packages/urllib3/connectionpool.py:1063: InsecureRequestWarning: Unverified HTTPS request is being made to host 'sat-stream-qa-rhel8.tanso.example.com'. Adding certificate verification is strongly advised. See: https://urllib3.readthedocs.io/en/1.26.x/advanced-usage.html#ssl-warnings
  warnings.warn(
Traceback (most recent call last):
  File \"/home/evgeni/.ansible/tmp/ansible-tmp-1724857997.5646284-1871287-265642613579476/AnsiballZ_content_view_version.py\", line 107, in <module>
    _ansiballz_main()
  File \"/home/evgeni/.ansible/tmp/ansible-tmp-1724857997.5646284-1871287-265642613579476/AnsiballZ_content_view_version.py\", line 99, in _ansiballz_main
    invoke_module(zipped_mod, temp_path, ANSIBALLZ_PARAMS)
  File \"/home/evgeni/.ansible/tmp/ansible-tmp-1724857997.5646284-1871287-265642613579476/AnsiballZ_content_view_version.py\", line 47, in invoke_module
    runpy.run_module(mod_name='ansible_collections.theforeman.foreman.plugins.modules.content_view_version', init_globals=dict(_module_fqn='ansible_collections.theforeman.foreman.plugins.modules.content_view_version', _modlib_path=modlib_path),
  File \"<frozen runpy>\", line 226, in run_module
  File \"<frozen runpy>\", line 98, in _run_module_code
  File \"<frozen runpy>\", line 88, in _run_code
  File \"/tmp/ansible_theforeman.foreman.content_view_version_payload_2vsp0rrh/ansible_theforeman.foreman.content_view_version_payload.zip/ansible_collections/theforeman/foreman/plugins/modules/content_view_version.py\", line 275, in <module>
  File \"/tmp/ansible_theforeman.foreman.content_view_version_payload_2vsp0rrh/ansible_theforeman.foreman.content_view_version_payload.zip/ansible_collections/theforeman/foreman/plugins/modules/content_view_version.py\", line 238, in main
TypeError: type 'NoEntity' is not subscriptable
", "module_stdout": "", "msg": "MODULE FAILURE
See stdout/stderr for the exact error", "rc": 1}
mdellweg commented 2 months ago

So to understand this correctly, the module is failing under the right circumstances, but for the wrong reason, right?

To pick up your idea: Can we pass the required flag from the foreman_spec to find_resource_by and then change that to start with

def find_resource_by(self, resource, search_field, value, required=False, **kwargs):
    if not value:
        if required:
            raise WhateverRequiredEntityNotSpecifiedError()
        return NoEntity
    ...
evgeni commented 2 months ago

So to understand this correctly, the module is failing under the right circumstances, but for the wrong reason, right?

Yeah. Most importantly, it gives an ugly stack trace instead of erroring with "couldn't find resource X".

Can we pass the required flag from the foreman_spec to find_resource_by and then change that to start with

Yes, that's roughly the direction I was heading.

evgeni commented 2 months ago

Actually, I think this should be sufficient:

def find_resource_by(self, resource, search_field, value, required=False, **kwargs):
    if not value and not required:
        return NoEntity
…

As the rest of the code will already correctly error when nothing was found.

mdellweg commented 2 months ago

Right. My line of thought was "Nothing found." is a different complaint than "You didn't specify a required thing.".

evgeni commented 2 months ago

Yeah… but they did specify it ("" is a specification), so imho the error "nothing found with name ''" is the correct one here.

mdellweg commented 2 months ago

Fair. Can we spare a lookup request when we already know that "" will not find us anything?

evgeni commented 2 months ago

Maybe? ;-)