Configure HTTP-based ARP information fetching from Palo Alto PIO-OS firewalls using management profiles

[jorund1]

Deprecate the [paloaltoarp] section of ipdevpoll.conf in 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 APIHTTP REST API for now, but this name might be missing the mark since there really isn't anything REST-related about the management profile type. Perhaps HTTP WITH API KEY, would be a more fitting name?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.60%. Comparing base (05dfb18) to head (fe3a967).
Report is 86 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3147      +/-   ##
==========================================
+ Coverage   60.45%   60.60%   +0.14%     
==========================================
  Files         605      605              
  Lines       43745    43718      -27     
  Branches       48       48              
==========================================
+ Hits        26448    26494      +46     
+ Misses      17285    17212      -73     
  Partials       12       12              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[lunkwill42]

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.

[lunkwill42]

This needs a docstring

[jorund1]

Fixed! (Also moved functionality to the paloalto plugin.)

nav/python/nav/ipdevpoll/plugins/paloaltoarp.py

Lines 161 to 173 in 6c9d4b5

	def _paloalto_profile_queryset(netbox: Netbox):
	"""
	Creates a Django queryset which when iterated yields JSON dictionaries
	representing configurations for accessing Palo Alto ARP data of the given
	netbox via HTTP. The keys in these dictionaries are the attribute-names of
	the :py:class:`~nav.web.seeddb.page.management_profile.forms.HttpRestForm`
	Django form.
	"""
	return NetboxProfile.objects.filter(
	netbox_id=netbox.id,
	profile__protocol=ManagementProfile.PROTOCOL_HTTP_API,
	profile__configuration__contains={"service": "Palo Alto ARP"},
	).values_list("profile__configuration", flat=True)

[lunkwill42]

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.

[jorund1]

Moved the loop to the handle() function

nav/python/nav/ipdevpoll/plugins/paloaltoarp.py

Lines 55 to 67 in 6c9d4b5

	@defer.inlineCallbacks
	def handle(self):
	"""Handle plugin business, return a deferred."""
	self._logger.debug("Collecting IP/MAC mappings for Paloalto device")
	configurations = yield self._get_paloalto_configurations(self.netbox)
	for configuration in configurations:
	mappings = yield self._get_paloalto_arp_mappings(
	self.netbox.ip, configuration["api_key"]
	)
	if mappings:
	yield self._process_data(mappings)
	break

[lunkwill42]

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

	@classmethod
	@defer.inlineCallbacks
	def can_handle(cls, netbox):
	daddy_says_ok = super(LLDP, cls).can_handle(netbox)
	has_ifcs = yield run_in_thread(cls._has_interfaces, netbox)
	defer.returnValue(has_ifcs and daddy_says_ok)

for a simple example of how.

[jorund1]

Thanks for the catch, wrapped database queries appropriately

nav/python/nav/ipdevpoll/plugins/paloaltoarp.py

Lines 81 to 99 in bdc6e99

	@classmethod
	@db.synchronous_db_access
	def _has_paloalto_configurations(cls, netbox: Netbox):
	"""
	Make a database request to check if the netbox has any management
	profile that configures access to Palo Alto ARP data via HTTP
	"""
	queryset = _paloalto_profile_queryset(netbox)
	return queryset.exists()
	@classmethod
	@db.synchronous_db_access
	def _get_paloalto_configurations(cls, netbox: Netbox):
	"""
	Make a database request that fetches all management profiles of
	the netbox that configures access to Palo Alto ARP data via HTTP
	"""
	queryset = _paloalto_profile_queryset(netbox)
	return list(queryset)

[lunkwill42]

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 :)

[jorund1]

Totally agree, simplified this fixture and moved form-related parts to a new test file

nav/tests/integration/web/management_profile_test.py

Lines 11 to 61 in 6c9d4b5

	class TestServiceChoices:
	"""
	The HttpApiForm Django form, which creates HTTP API management profiles,
	stores a "service" key in the profile's configuration, used by other
	parts of NAV to differentiate between HTTP services:
	profile.configuration == {"service": "foo", ...}
	The HttpApiForm follows the convention of other management profile forms
	in not using named constants for ChoiceField values stored in the
	configuration dict. To help make sure that other parts of NAV use the
	same service values as those set by the form, we add tests below for
	each service choice.
	The tests first associates a service with a netbox through using the
	form, thus asserting that the service is a valid choice. Then it checks
	that the other parts of NAV accepts same service value as the one the
	form accepted.
	"""
	tested_services = ["Palo Alto ARP"]
	@pytest.mark.twisted
	@pytest_twisted.inlineCallbacks
	def test_paloalto_plugin_should_use_the_service_choice_used_in_form(
	self, localhost, send_add_management_profile_form
	):
	can_handle = yield PaloaltoArp.can_handle(localhost)
	assert not can_handle
	# Check that "Palo Alto ARP" is accepted by the form
	send_add_management_profile_form(
	localhost,
	protocol=ManagementProfile.PROTOCOL_HTTP_API,
	service="Palo Alto ARP",
	api_key="foo",
	)
	# Check that "Palo Alto ARP" is accepted by the plugin
	can_handle = yield PaloaltoArp.can_handle(localhost)
	assert can_handle
	def test_all_service_choices_should_have_a_test(self):
	all_services = HttpApiForm.service.choices
	missing_tests = set(all_services) - set(self.tested_services)
	error_message = (
	"If you've added a service choice to HttpApiForm, please add an "
	"integration test that checks that the service is correctly referenced "
	f"in other parts of NAV (missing tests for {', '.join(missing_tests)})"
	)
	assert not missing_tests, error_message

[sonarcloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[lunkwill42]

This looks mostly good to me, just a few issues with the tests still.

[lunkwill42]

This test has multiple asserts, which is often a smell.

This test will fail under multiple conditions, and when the test report says that test_netbox_with_paloalto_management_profile_with_valid_api_key_should_get_arp_mappings failed, it is not immediately clear what assertion failed.

There really should be a separate test that only asserts that PaloaltoArp.can_handle() returns True when a valid profile is configured for a device. If this test fails, one immediately knows something is wrong with either the profile or the can_handle implementation.

In fact, you have already implemented this test, but in the SeedDB form tests...

[lunkwill42]

Again, this test doesn't really test the form - it tests the Paloaltoarp plugin's can_handle() method.

How the profile got into the database is really irrelevant to this test, and so this isn't really a test of the form functionality.

This test would be better if it were in tests/integration/ipdevpoll/plugins/paloaltoarp_test.py.

A test for the actual form would mostly consist of submitting form data and asserting that the profile was saved to the database and that it has the expected format.

[lunkwill42]

Other than that, the layout of the management profile tests looks nice - I guess we'll see how it works in practice if/when we add other service types.

[lunkwill42]

This has an unfortunate side-effect: The logging statement will now not be in the context of the device being collected for, since you have stepped out of the class instance and into the class.

Maybe a linting tool complains that this can be a class method since it accesses no instance attributes, but I think maybe it should change back to keep the proper context.

[lunkwill42]

Also, we need to make a separate issue for something entirely unrelated to this PR: This logs the full contents of the secret API token, which is generally not a good idea from a security standpoint. It should be censored. Would you do the honors?