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

Problem: Detail information are hidden by search information #102

Open
jiuka opened this issue Sep 30, 2024 · 5 comments
Open

Problem: Detail information are hidden by search information #102

jiuka opened this issue Sep 30, 2024 · 5 comments
Assignees
Labels

Comments

@jiuka
Copy link
Contributor

jiuka commented Sep 30, 2024

Modules

The python class ansible_collections.ansibleguy.opnsense.plugins.module_utils.base.base.Base

Version

1.2.10 and latest

Ansible Version

No response

OPNSense Version

24.4 and 24.7

OPNSense-Plugin Version

No response

Issue

The using Base.search the simpler search dict and (may) extend it with the more comprehensive information from the detail call. In the way the combining is done the simpler information take precedence. This can hide important information if they are present on both versions. For example in the case of the Dhcrelay API the interfaceand destination file which are both select field in the getRelay are overwritten with the simple text representation. This version of the data is not sufficient to create the update request as the key for interface and destination are required.
The content of self.raw is not the same depending on if the item in question exists or not. If it does the simplified version is returned, otherwise the detailed version (for a empty item) is returned.

Would it be possible/feasible to switch the precedence in the mare of the two dicts around?

Example

/api/dhcrelay/settings/searchRelay

{
    "rows": [
        {
            "uuid": "01234567-0123-0123-0123-012345678901",
            "enabled": "1",
            "interface": "Printers",
            "destination": "mydhcp",
            "agent_info": "0",
            "status": "grey"
        }
    ],
    "rowCount": 1,
    "total": 1,
    "current": 1
}

/api/dhcrelay/settings/getRelay/

{
    "relay": {
        "enabled": "1",
        "interface": {
            "lan": {
                "value": "lan",
                "selected": 0
            },
            "opt1": {
                "value": "Printers",
                "selected": 1
            }
        },
        "destination": {
            "01234567-0123-0123-0123-012345678901": {
                "value": "mydhcp",
                "selected": 1
            }
        },
        "agent_info": "0"
    }
}

Final data

{
    "uuid": "01234567-0123-0123-0123-012345678901",
    "enabled": "1",
    "interface": "Printers",
    "destination": "mydhcp",
    "agent_info": "0",
    "status": "grey"
}

Config Ansible

No response

Config OPNSense

No response

Debug Output

No response

Profiling Output

No response

@ansibleguy
Copy link
Owner

ansibleguy commented Oct 7, 2024

I would generally prefer using the search calls, as we have to send one detail request or every entry that exists.
If there are 100+ entries for the targeted 'controller' the user will have to wait long.

What I can provide is an attribute that enforces the detail call on all entries: SEARCH_DETAIL_ALL: True

Your opinion and feedback would be appreciated 👍🏼

BTW: I have exchanged my thoughts about a detail call that returns all entries in this OPNSense issue: opnsense/core#7094
(would be my preferred way of using it..)

@ansibleguy
Copy link
Owner

ansibleguy commented Oct 7, 2024

Sidenote: I need to document the functionality of those attributes.. have been a little to lazy when it comes to those
#106

@jiuka
Copy link
Contributor Author

jiuka commented Oct 9, 2024

Given the module is not a management module and has a FIELD_ID (which is returned from the /searchItem endpoint). After the /searchItem call only one call to /getItem should be necessary. Even for 100+ Items only two calls should be necessary. However this only works if only the matching item(s) are returnd and not all. This would reduce the amount of items which need to be simplified. For collections with 100+ entries this could be an advantage.

I experimented with the searchRelay api in dhcrelay_relay module. If the relay was not existing the self.raw end with the return of /api/dhcrelay/settings/getRelay with interface and destination as dictionary usefull to translate from the value to the index (like openvpn_client does) however if the relay existed the detail dict for merge with the dict from the search call. As the both have all the same keys all information from the detail call got overwritten again and where not available for the translation.

The detail call is already enforced on all items by default. Base._call_search(match_fields=['X']) will call BaseModule._search_call() without the match_fields. As all BaseModule or GeneralModule derived module haf a _search_call method. Base.search(match_fields) seems never to be called. And without the match_fields we already have an enforced detail fetching. Without it currently does not work eigther, as all results return are simplified, which does not work of the if for example a select/list field is just a string as it often is in the search api.

@ansibleguy ansibleguy self-assigned this Dec 8, 2024
@ansibleguy
Copy link
Owner

ansibleguy commented Dec 8, 2024

First of all - I've just updated the developer-docs. This might add some more context to the way it is currently designed: https://opnsense.ansibleguy.net/usage/4_develop.html#abstraction-features

Just to make sure we are on the same page:

  • This might also be related to the two ways the OPNSense-API is behaving: Feature: API-usage refactoring #51 and related OPNSense issue

    • The convenient 'old' way - that returns details for all entries when calling the get endpoint
    • And the 'new' way - that has a very restricted get endpoint and thus only returns a very basic set of fields at the searchItem endpoint

The way I've implemented it currently - the getItem gets called if:

  • the module CMDS search-entry starts with search (else we use the more convenient get endpoint)
  • and
    • we are unable to match the configured entry because at least one match_field is not in the response of searchItem (all entries produce a getItem)
    • OR a single entry was matched (only one entry call getItem)

The use of match_fields is problematic in that case - I agree.

@ansibleguy
Copy link
Owner

BTW: Sorry for the delayed response. Was very busy :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants