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

Add netbox pillar #46559

Merged
merged 1 commit into from Mar 21, 2018

Conversation

Projects
None yet
4 participants
@ksofa2
Copy link
Contributor

commented Mar 15, 2018

What does this PR do?

Add a pillar module that adds data to the Pillar structure from a NetBox API. Optionally creates the proxy structure.

Tests written?

No

Commits signed with GPG?

No

@ksofa2 ksofa2 requested a review from saltstack/team-core as a code owner Mar 15, 2018

@terminalmage
Copy link
Contributor

left a comment

We need some imports from __future__ here. Also, please update the logging here so that you pass the args to the log handler instead of performing string replacement manually? Our logging handler normalizes str/unicode mismatches on Python 2, so you should just do your logging like this:

log.debug('foo = %s', foo)

Doing the logging the way you have it now will result in the string replacement happening before our logging handler has a chance to normalize any str/unicode mismatches.

If any additional options for the proxy setup are needed they should also be
configured in pillar_roots.
'''

This comment has been minimized.

Copy link
@terminalmage

terminalmage Mar 15, 2018

Contributor

Please add the following here:

from __future__ import absolute_import, print_function, unicode_literals

@ksofa2 ksofa2 force-pushed the ksofa2:add-netbox-pillar branch 2 times, most recently from 214bd2d to c396c51 Mar 15, 2018

@ksofa2

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2018

@terminalmage Thanks. We've made the requested changes. Also squashed the commits to clean up a mess I made in merging. (Sorry about that.)

if len(devices) == 1:
ret['netbox'] = devices[0]
elif len(devices) > 1:
log.error('More than one device found for "%s"' % minion_id)

This comment has been minimized.

Copy link
@terminalmage

terminalmage Mar 16, 2018

Contributor

This log entry still needs to be changed per my earlier comments.

@ksofa2

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2018

@mirceaulinic
Copy link
Member

left a comment

Nice work @ksofa2. I think we should rename this module appropriate to its scope, and document what it does exactly (the docstring doesn't currently reflect that).

'host': str(ipaddress.IPv4Interface(
ret['netbox']['primary_ip4']['address']).ip),
'driver': platform_results.json()['napalm_driver'],
'proxytype': 'napalm',

This comment has been minimized.

Copy link
@mirceaulinic

mirceaulinic Mar 19, 2018

Member

This should not be hardcoded like that. Maybe at least have it as an option or something.

This comment has been minimized.

Copy link
@ksofa2

ksofa2 Mar 19, 2018

Author Contributor

Good point, but given the structures we get back from NetBox, I'd like to leave it hardcoded and update the documentation so say that only napalm proxtypes are supported.

I added a code change to only produce the proxy key if the napalm_driver is defined in the device's platform entry.

.. code-block:: yaml
ext_pillar:
- netbox:

This comment has been minimized.

Copy link
@mirceaulinic

mirceaulinic Mar 19, 2018

Member

I would argue that this Pillar module should not be named as netbox, as its scope is very, very specific to retrieving data from NetBox and populating the proxy key into the Pillar. While this is very useful and nice, maybe we should emphasise the scope and rename it, say netbox_proxy or something similar.

To clarify: a netbox external Pillar I would expect to provide me all the NetBox data related to that device (i.e, from the dcim/devices/<device ID>/ endpoint).

This comment has been minimized.

Copy link
@ksofa2

ksofa2 Mar 19, 2018

Author Contributor

It's creating the proxy key if requested, but this isn't just doing that. It also is returning the device info in the netbox key. It's a raw dump of what it gets back from the API call.

This comment has been minimized.

Copy link
@mirceaulinic

mirceaulinic Mar 19, 2018

Member

Ah, okay, I misread that - please disregard.


# Pull settings from kwargs
api_url = kwargs['api_url'].rstrip('/')
api_token = kwargs['api_token']

This comment has been minimized.

Copy link
@mirceaulinic

mirceaulinic Mar 20, 2018

Member

I believe the token should not be mandatory, as this Pillar module only fetches data (the token is required only when writing data into NetBox) - see https://docs.saltstack.com/en/develop/ref/modules/all/salt.modules.netbox.html as an example.

@mirceaulinic
Copy link
Member

left a comment

I wouldn't have any further comments on this, just to ask you to rebase and squash (but make sure to leave aside commits such as ed6cdb7 or 2c6d44a which are unrelated to this addition). So I will defer to @terminalmage for more input.

Thanks for your contribution @ksofa2.

@ksofa2 ksofa2 force-pushed the ksofa2:add-netbox-pillar branch 2 times, most recently from e87283d to bacfc01 Mar 20, 2018

@terminalmage
Copy link
Contributor

left a comment

👍

Add NetBox pillar
Co-authored-by: Kris Steinhoff <steinhof@umich.edu>
Co-authored-by: Nicholas Grundler <grundler@umich.edu>

@ksofa2 ksofa2 force-pushed the ksofa2:add-netbox-pillar branch from bacfc01 to 93a2caa Mar 20, 2018

@ksofa2

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2018

Thanks @mirceaulinic and @terminalmage!

I've squashed and rebased onto develop. Please let me know if there's anything else I need to do.

@rallytime rallytime merged commit 95be4e4 into saltstack:develop Mar 21, 2018

4 of 10 checks passed

jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #3323 — ABORTED
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #17365 — ABORTED
Details
codeclimate 2 issues to fix
Details
default Build finished.
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #20986 — FAILURE
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #8049 — FAILURE
Details
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #23444 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #15675 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #20292 — SUCCESS
Details

@ksofa2 ksofa2 deleted the ksofa2:add-netbox-pillar branch Mar 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.