Fix netbox url with string concat

[heathdbrown]

SUMMARY

Fixes #46705

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

• netbox_inventory

ADDITIONAL INFORMATION

The fix in #47655 made the problem worse.

The issue was with how urljoin() removes paths from the configuration.

>>> from urlparse import urljoin
>>> urljoin('http://netbox.internalurl.com/netbox', '/api/dcim/platforms/?limit=0')
'http://netbox.internalurl.com/api/dcim/platforms/?limit=0'

>>> urljoin('http://netbox.internalurl.com/netbox', 'api/dcim/platforms/?limit=0')
'http://netbox.internalurl.com/api/dcim/platforms/?limit=0'

>>> urljoin('netbox.internalurl.com/netbox/', 'api/dcim/platforms/?limit=0')
'netbox.internalurl.com/netbox/api/dcim/platforms/?limit=0'

The way the netbox.py is written the api urls are joined but they all have /api and when merged with /netbox overwrites to /api only which is a missing URL and causes 404s.

Fixing this would mean adjusting /api URL fragments to api OR adjusting the netbox.py to not use urljoin() and use string concat.

[ansibot]

cc @Anthony25 @FragmentedPacket @nikkytub @pilou- @sieben
click here for bot help

[remyleone]

If this patch we would need to be sure that there is no trailing slash for the base_url. // are not welcomed by netbox AFAIK

[heathdbrown]

@sieben What would be the suggestion in handling this?

(I do not have a ton of coding experience)

Documentation?
Data validation on the api_endpoint?

[gundalow]

@heathdbrown if api_endpoint ends with a / you could trim it that variable is used

[winem]

Did a test on our netbox setup and API calls work with any number of slashes (tested with 1-6 out of curiosity) between the domain part and the actual path. A // in the path itself fails with a 404.

Examples:
working:

• http://netbox.intern//api/virtualization//clusters/
• http://netbox.intern////api/virtualization//clusters/

not working:

• http://netbox.intern/api//virtualization/clusters/
• http://netbox.intern/api/virtualization//clusters/
• http://netbox.intern/api/virtualization//clusters//

[gundalow]

@heathdbrown Could you please rebase this so we can get it merged

[heathdbrown]

@gundalow I rebased and I believe this is ready now.

[gundalow]

@Anthony25 @FragmentedPacket @nikkytub @pilou- @sieben could we please get a final review on this?

[ansibot]

@heathdbrown This PR contains @ mentions in at least one commit message. Those mentions can cause cascading notifications through GitHub and need to be removed. Please squash or amend your commits to remove the mentions.

click here for bot help

[FragmentedPacket]

Sorry about this. Just reviewing it, might be a little cleaner way.

        REMOVE: self.api_endpoint = self.get_option("api_endpoint")
        # Handle extra "/" from api_endpoint configuration and trim if necessary, see PR#49943
        if self.get_option("api_endpoint").endswith("/"):
            self.api_endpoint = self.get_option("api_endpoint").strip('/')
        else:
            self.api_endpoint = self.get_option("api_endpoint")

[heathdbrown]

@FragmentedPacket that fails as the self.api_endpoint above the if is the first assignment.

After commenting it out the plugin fails.

[FragmentedPacket]

@heathdbrown Did you change the if statement to look at the self.get_option?

[heathdbrown]

@FragmentedPacket well that was embarrassing. Thanks for the patience. I have included the complete suggestion.

[winem]

Imho, there is no need for the if if you use .strip('/'). Instead you can just use it when you assign the variable.

.strip('/') will neither do anything nor fail if there is no trailing slash.

self.api_endpoint = self.get_option("api_endpoint").strip('/') should just work.

>>> api_endpoint='netbox.com/'
>>> api_endpoint.strip('/')
'netbox.com'
>>> api_endpoint='netbox.com////'
>>> api_endpoint.strip('/')
'netbox.com'
>>> api_endpoint='netbox.com'
>>> api_endpoint.strip('/')
'netbox.com'

[heathdbrown]

@winem updated to your suggestion and removed the if logic.

[winem]

Looks good to me!

[FragmentedPacket]

@heathdbrown Hey i'm in the same boat as you lol

Looks good to me as well

[FragmentedPacket]

@sieben Do you mind looking this over and approving? 2.8 RC stuff starts the 14th of March

[remyleone]

LGTM

[remyleone]

@Anthony25 What do you think about it?

[nikkytub]

LGTM as well

[resmo]

rebuild_merge