Configure HTTP-based ARP information fetching from Palo Alto PIO-OS firewalls using management profiles #3147
Conversation
Prior to this commit, the netboxes handled by the PaloaltoArp ipdevpoll plugin used to be configured in the `ipdevpoll.conf` configuration file, but since the netboxes the plugin wants to handle (i.e. collect Arp information from) already should reside in the NAV database, this configuration is now instead done through the SeedDB tool by assigning a HTTP_REST_API ManagementProfile (with `service` set `Palo Alto ARP` and api_key set to some secret API key) to the netboxes to be handled. The prior way to configure the netboxes handled by the PaloAltoArp plugin implicitly only allowed one API key per netbox (both enforced in code but also by the configuration syntax). With ManagementProfiles, it is perfectly possible to assign multiple profiles (e.g. configurations) of the same type but with different parameters (e.g. API keys) to the same netbox. Hence the new way to configure the netboxes allow many API keys per netbox. Thus the semantics of the plugin must change a little: For any given netbox, the plugin now assumes there may be multiple API keys, and uses the ARP results of first API key for which the _do_request method returns a successful response. IMPORTANT: This commit removes the ability to configure the netboxes handled by the PaloaltoArp plugin the NAV version 5.10 - 5.11 way through the `[paloaltoarp]` section in the `ipdevpoll.conf` configuration file.
One netbox may now have multiple API keys with the recent updates to the PaloaltoArp plugin, so we test that the plugin correctly handles the case where the first few keys it uses are invalid.
Simple integration tests are added both for the case where the job's netbox has a valid key and the ARP set should be updated and for the case where the job's netbox has only an invalid key and the ARP set should not changed. The tests go through the hassle of creating/editing Netboxes and ManagementProfiles using the SeedDB endpoints, and will be able to pick up bugs caused by changes in the naming scheme of the values and keys in a HTTP_REST_API ManagementProfile instance's configuration json (e.g. the event that the HTTPRestForm Django form class is changed so that user-supplied values suddenly are written to different keys in the resulting ManagementProfile instance's configuration json than the PaloaltoArp plugin expects).
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3147 +/- ##
==========================================
+ Coverage 60.42% 60.46% +0.03%
==========================================
Files 605 605
Lines 43745 43754 +9
Branches 48 48
==========================================
+ Hits 26435 26457 +22
+ Misses 17298 17285 -13
Partials 12 12 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
This is shaping up really nicely!
I'm really glad you took the time to write a proper release notes entry, though I am not sure that we need to delve deeply into rationalizing why we are changing the palo alto API key config stuff. The bit about ARP being available only through the API I believe was explained in an earlier release note too.
You added a separate "make linter happy" commit, which should be unnecessary. These are formatting changes that your pre-commit hooks should have fixed for you in the original commits (you did install the pre-commit hooks, right?)
The "Fix unittests" commit is also probably best if squashed into the already existing commit that updates the tests.
python/nav/models/manage.py
Outdated
| def get_http_rest_management_profiles( | ||
| self, service: str | ||
| ) -> models.QuerySet: |
| for i, api_key in enumerate(api_keys): | ||
| arptable = yield self._do_request(ip, api_key) | ||
| if arptable is not None: | ||
| # process arpdata into an array of mappings | ||
| mappings = parse_arp(arptable.decode('utf-8')) | ||
| break | ||
| self._logger.info( | ||
| "Could not fetch ARP table from Paloalto device When using API key %d of %d", | ||
| i, | ||
| len(api_keys), | ||
| ) |
There was a problem hiding this comment.
IMNSHO, the key iteration functionality presented here should not really be the concern of the _get_paloalto_arp_mappings() method. It should be factored out.
| netbox.sysname in cls.configured_devices | ||
| or str(netbox.ip) in cls.configured_devices | ||
| ) | ||
| return netbox.get_http_rest_management_profiles("Palo Alto ARP").exists() |
There was a problem hiding this comment.
There's a big fat gotcha here: The can_handle() method runs inside the main event loop thread, so you should absolutely not run database queries from this method: They will block everything ipdevpoll is working on until the response comes back from the database.
If you need to access the database at this point, you should defer the lookup to a database thread instead. See
nav/python/nav/ipdevpoll/plugins/lldp.py
Lines 51 to 56 in 518e622
| @pytest.fixture | ||
| def paloalto_netbox_1234(db, client): | ||
| box = Netbox( | ||
| ip='127.0.0.1', | ||
| sysname='localhost.example.org', | ||
| organization_id='myorg', | ||
| room_id='myroom', | ||
| category_id='SRV', | ||
| ) | ||
| box.save() | ||
| profile = ManagementProfile( | ||
| name="PaloAlto Test Management Profile", | ||
| protocol=ManagementProfile.PROTOCOL_SNMP, # Correct protocol is set with HTTP POST below | ||
| configuration={ | ||
| "version": 2, | ||
| "community": "public", | ||
| "write": False, | ||
| }, | ||
| ) | ||
| profile.save() | ||
|
|
||
|
|
||
| netbox_url = reverse("seeddb-netbox-edit", args=(box.id,)) | ||
| management_profile_url = reverse( | ||
| "seeddb-management-profile-edit", args=(profile.id,) | ||
| ) | ||
|
|
||
| # Manually sending this post request helps reveal regression bugs in case | ||
| # HTTPRestForm.service.choices keys are altered; because the post's thus | ||
| # invalid service field should then cause the django form cleaning stage to | ||
| # fail. (Changing the HTTPRestForm.choice map to use enums as keys instead | ||
| # of strings would enable static analysis to reveal this.) | ||
| client.post( | ||
| management_profile_url, | ||
| follow=True, | ||
| data={ | ||
| "name": profile.name, | ||
| "description": profile.description, | ||
| "protocol": ManagementProfile.PROTOCOL_HTTP_REST, | ||
| "service": "Palo Alto ARP", | ||
| "api_key": "1234", | ||
| } | ||
| ) | ||
|
|
||
| client.post( | ||
| netbox_url, | ||
| follow=True, | ||
| data={ | ||
| "ip": box.ip, | ||
| "room": box.room_id, | ||
| "category": box.category_id, | ||
| "organization": box.organization_id, | ||
| "profiles": [profile.id], | ||
| }, | ||
| ) | ||
|
|
There was a problem hiding this comment.
According to the commit log, you seem to be arguing that this fixture is also a test for the web forms. I would advocate that it shouldn't be. Testing the web forms should be an explicit test function, with a name that makes it abundantly clear what kind of assertion is breaking when it fails.
For the purposes of testing the ipdevpoll plugin itself, it is much simpler for this fixture to just insert the required boxes/profiles directly into the database.
The fixture code as-is could/should be re-used in an explicit management profile form test :)
Shadowed netboxes (ipdevpoll.shadows.Netbox instances as opposed to models.manage.Netbox instances) lacks some functionality that the paloaltoplugin relied on, namely the ability to look at foreign relationship sets, and the usage of some methods that isn't available in the shadowed counterpart. Since the netbox instance supplied to the plugin is the shadowed version, we must hence find alternative ways to realise this functionality; this is basically done by using the shadowed netbox's id to look up the actual netbox in the models.manage.Netbox query_set when needed.
|
Deprecate the
[paloaltoarp]section ofipdevpoll.confin favor of using management profiles to configure HTTP-based fetching of ARP information from Palo Alto PIO-OS firewalls, analogous to how configuration of SNMP-based fetching is done.The new management profile protocol to configure HTTP-based fetching has been given the name
HTTP REST APIfor now, but this name might be missing the mark since there really isn't anything REST-related about the management profile type. PerhapsHTTP WITH API KEY, would be a more fitting name?