search

nautobot / pynautobot

Nautobot Python SDK
https://pynautobot.readthedocs.io/en/latest/index.html
Apache License 2.0
36 stars 32 forks source link

Handle app names including dash - #98

Closed johanek closed 1 year ago

johanek commented 1 year ago

Currently access app names including a - character results in an error, as underscores are not converted to dashes i.e.:

In [2]: nb.plugins.golden_config.config_compliance.all()
...
RequestError: The requested url: https://nautobot-staging.internal/api/plugins/golden_config/config-compliance/?limit=0 could not be found.
jvanderaa commented 1 year ago

Will check the code. If I remember back a bit, something like this caused some issues in the code and was changed to what it is now.

There is also this discussion that outlines the current methodology (if this PR is not an improvement on it): https://github.com/nautobot/pynautobot/discussions/62

jmcgill298 commented 1 year ago

what if the app name uses underscores instead of dashes?

nautics889 commented 1 year ago

@jmcgill298

what if the app name uses underscores instead of dashes?

You're right. This PR works only for specific case, because the plugin Golden Config sets them base_url with dashes.

Meanwhile in endpoint.py module the URL for request is formed with app's name, not app's base_url. I guess that is the point, that's the reason why the original request failed with 404. I mean, it'd work until the name of plugin app and it's base_url differed. And this is exactly the case @johanek encountered on.

nautics889 commented 1 year ago

TL;DR We should get base_url of a plugin app and use it to form URL rather than use app's package name.

When we're operating with any plugin application (like nb.plugins.golden_config e. g.), the name of attribute (i. e. the name of the plugin) is being used to form the URL (just like i've mentioned above).

pynautobot/core/app.py:

...
class PluginsApp(object):
    ...
    def __getattr__(self, name):
        return App(self.api, "plugins/{}".format(name))
    ...
...

And that's why if you made something like this, using dashes instead of underscores:

>>> getattr(nb.plugins, 'golden-config').config_compliance.all()  # it doesn't raise an error
[]

this wouldn't return 404, this would be a valid request.

I presume this is incorrect logic. We should use actual URL of the plugin application to form URL for API-request. Meanwhile the plugin app should be accessible by it's package name.

To make it clear. The behaviour we should reach, example: app's base url: golden-config (see here) app's package name: nautobot_golden_config (see here) The code which supposed to access plugin app:

>>> nb.plugins.nautobot_golden_config.whatever_endpoint_you_want.all()
# and it creates a request to https://<nb_host>/api/plugins/golden-config/config-compliance

I've investigated PluginsApp and it's installed_plugins() method to discover how we could get base_url of the plugin app. However i encountered that Nautobot does not return plugin's base_url at all. Here what i've got accessing /api/plugins/installed-plugins/:

GET /api/plugins/installed-plugins/
HTTP 200 OK
API-Version: 1.2
Allow: GET, HEAD, OPTIONS
Content-Type: application/json
Vary: Accept

[
    {
        "name": "Nautobot Plugin for Nornir",
        "package": "nautobot_plugin_nornir",
        "author": "Network to Code, LLC",
        "author_email": "",
        "description": "A plugin/library for using Nornir within Nautobot.",
        "verison": "1.0.0",
        "version": "1.0.0"
    },
    {
        "name": "Golden Configuration",
        "package": "nautobot_golden_config",
        "author": "Network to Code, LLC",
        "author_email": "[opensource@networktocode.com](mailto:opensource@networktocode.com)",
        "description": "A plugin for managing Golden Configurations.",
        "verison": "1.3.1",
        "version": "1.3.1"
    }
]

I don't see plugin's base_url above.

If we consider Nautobot's _get_plugin_data() staticmethod of the class InstalledPluginsAPIView, there is no key base_url in the result dict as you can see:

    @staticmethod
    def _get_plugin_data(plugin_app_config):
        return {
            "name": plugin_app_config.verbose_name,
            "package": plugin_app_config.name,
            "author": plugin_app_config.author,
            "author_email": plugin_app_config.author_email,
            "description": plugin_app_config.description,
            # 2.0 TODO: Remove verison key/value when bumping to major revision
            "verison": plugin_app_config.version,
            "version": plugin_app_config.version,
        }

However it can be added: "base_url": plugin_app_config.base_url. Once i added one, the plugin's base_url appeared in the /api/plugins/installed-plugins/: 

GET /api/plugins/installed-plugins/
HTTP 200 OK
API-Version: 1.2
Allow: GET, HEAD, OPTIONS
Content-Type: application/json
Vary: Accept

[
    {
        "name": "Nautobot Plugin for Nornir",
        "base_url": "plugin-nornir",
        "package": "nautobot_plugin_nornir",
        "author": "Network to Code, LLC",
        "author_email": "",
        "description": "A plugin/library for using Nornir within Nautobot.",
        "verison": "1.0.0",
        "version": "1.0.0"
    },
    {
        "name": "Golden Configuration",
        "base_url": "golden-config",
        "package": "nautobot_golden_config",
        "author": "Network to Code, LLC",
        "author_email": "[opensource@networktocode.com](mailto:opensource@networktocode.com)",
        "description": "A plugin for managing Golden Configurations.",
        "verison": "1.3.1",
        "version": "1.3.1"
    }
]

And then we are able to know the actual URL of a plugin.

In conclusion. My proposal:

  1. Open an issue in https://github.com/nautobot/nautobot/, suggest to update _get_plugin_data() with adding base_url like this:

    class InstalledPluginsAPIView(NautobotAPIVersionMixin, APIView):
...

@staticmethod
def _get_plugin_data(plugin_app_config):
    return {
        "name": plugin_app_config.verbose_name,
        "base_url": plugin_app_config.base_url,
        "package": plugin_app_config.name,
        ...
    }

...

  2. Update __getattr__() method of PluginsApp class, make something like this:

    def __getattr__(self, name):
    try:
        request_for_plugin = Request(
            base="{}/plugins/{}".format(
                self.api.base_url,
                name
            ),
            token=self.api.token,
            http_session=self.api.http_session,
        )
        # make sure the plugin can be accessed by it's package name
        request_for_plugin._make_call()
    except RequestError as error:
        # if not, iterate over installed plugin to try to find `base_url` of the plugin app
        if error.req.status_code == 404:
            for plugin in self.installed_plugins():
                if plugin.get('package') == name and 'base_url' in plugin:
                    return App(self.api, "plugins/{}".format(plugin['base_url']))
        raise error

    return App(self.api, "plugins/{}".format(name))

    I know, the code above may seem strange at first glance, since it causes to make an additional GET-request to plugin each time we access nb.plugins.some_plugin_of_mine, but that's the first came to mind, IDK how else we can check the plugin isn't accessible by it's package name and has different URL.

The code above works well:

>>> nb = api("http://localhost:8888", token=os.environ.get("NAUTOBOT_API_TOKEN"))
>>> print(nb.plugins.nautobot_golden_config.config_compliance.all())  # doesn't raise error
[]

@jmcgill298, @jvanderaa what you think?

jvanderaa commented 1 year ago

@pszulczewski is also taking a look at this some.

pszulczewski commented 1 year ago

Great input @johanek and @nautics889!

As mentioned before, the solution proposed by @johanek may affect other app names not only plugins.

Solutions proposed by @nautics889 introduce deviation to the api path documented in API Docs Swagger as you use plugin names and not the exact plugin base_url on pynautobot objects. It can also be a long shot to update nautobot with extra methods.

I would be leaning towards replacing _ to - but I would make it only for plugins, so PluginsApp __getattr__: https://github.com/nautobot/pynautobot/blob/4c3bad3a65dd0d746004dd638e8a33207ee46271/pynautobot/core/app.py#L146 becomes: return App(self.api, f"plugins/{name.replace('_', '-')}")

Then we only affect plugin names and we fix something what doesn't really work now. And that works with the base-url as documented in the API Docs.

nb_demo.plugins.golden_config.config_compliance.all()
[<pynautobot.core.response.Record ('jcy-bb-01.infra.ntc.com -> Cisco IOS - svc -> True') ...

nb_demo.plugins.firewall.address_object.all()
[<pynautobot.core.response.Record ('nat-destination') ...

nb_demo.plugins.nautobot_device_lifecycle_mgmt.cve.all()
[<pynautobot.core.response.Record ('CVE-2020-3508') at 0x10294f7f0>]

@jvanderaa I am not aware of previous issues you mentioned, what do think about this approach, to replace underscores to dashes only for plugin paths, which are base-urls?

nautics889 commented 1 year ago

@pszulczewski thank you!

Replacing _ to - in PluginsApp seems to me like good solution for now at least.

FYI: i've created topic in Nautobot's discussions related to this problem, so we can return to this further.

pszulczewski commented 1 year ago

@johanek fancy updating this PR with my suggestion above?

johanek commented 1 year ago

@pszulczewski Updated per your suggestion. Let me know if you'd like me to clean the git history

pszulczewski commented 1 year ago

There is a dependency issue with urllib3 as I see container pulls 2.0.2, where we have 1.26.13 specified in poetry.lock. Looking into this issue ...