Hacking N362: Don't abbrev/alias privsep import

As noted in [1]:

We always import privsep modules like this:

    import nova.privsep.libvirt

Not like this:

    from nova.privsep import libvirt

This is because it makes it obvious at the caller that a priviledged
operation is occuring:

    nova.privsep.libvirt.destroy_root_filesystem()

Not just:

    libvirt.destroy_root_filesystem()

This is especially true when the imported module is called "libvirt",
which is a very common term in the codebase and super hard to grep
for specific uses of.

This commit introduces hacking rule N362 to enforce the above.

Change-Id: I9b6aefa015acbf28e49a9ff1713a8bb544586579
Co-Authored-By: Eric Fried <openstack@fried.cc>

HACKING.rst

@@ -70,6 +70,9 @@ Nova Specific Commandments
 - [N360] Yield must always be followed by a space when yielding a value.
 - [N361] Check for usage of deprecated assertRegexpMatches and
   assertNotRegexpMatches
+- [N362] Imports for privsep modules should be specific. Use "import nova.privsep.path",
+  not "from nova.privsep import path". This ensures callers know that the method they're
+  calling is using priviledge escalation.
 
 Creating Unit Tests
 -------------------

nova/hacking/checks.py

@@ -102,6 +102,9 @@ redundant_import_alias_re = re.compile(r"import (?:.*\.)?(.+) as \1$")
 yield_not_followed_by_space = re.compile(r"^\s*yield(?:\(|{|\[|\"|').*$")
 asse_regexpmatches = re.compile(
     r"(assertRegexpMatches|assertNotRegexpMatches)\(")
+privsep_file_re = re.compile('^nova/privsep[./]')
+privsep_import_re = re.compile(
+    r"^(?:import|from).*\bprivsep\b")
 
 
 class BaseASTChecker(ast.NodeVisitor):
@@ -851,6 +854,34 @@ def assert_regexpmatches(logical_line):
                   "of assertRegexpMatches/assertNotRegexpMatches.")
 
 
+def privsep_imports_not_aliased(logical_line, filename):
+    """Do not abbreviate or alias privsep module imports.
+
+    When accessing symbols under nova.privsep in code or tests, the full module
+    path (e.g. nova.privsep.linux_net.delete_bridge(...)) should be used
+    explicitly rather than importing and using an alias/abbreviation such as:
+
+      from nova.privsep import linux_net
+      ...
+      linux_net.delete_bridge(...)
+
+    See Ief177dbcb018da6fbad13bb0ff153fc47292d5b9.
+
+    N362
+    """
+    if (
+            # Give modules under nova.privsep a pass
+            not privsep_file_re.match(filename) and
+            # Any style of import of privsep...
+            privsep_import_re.match(logical_line) and
+            # ...that isn't 'import nova.privsep[.foo...]'
+            logical_line.count(' ') > 1):
+        yield (0, "N362: always import privsep modules so that the use of "
+                  "escalated permissions is obvious to callers. For example, "
+                  "use 'import nova.privsep.path' instead of "
+                  "'from nova.privsep import path'.")
+
+
 def factory(register):
     register(import_no_db_in_virt)
     register(no_db_session_in_public_api)
@@ -895,3 +926,4 @@ def factory(register):
     register(no_redundant_import_alias)
     register(yield_followed_by_space)
     register(assert_regexpmatches)
+    register(privsep_imports_not_aliased)

nova/tests/fixtures.py

@@ -56,7 +56,7 @@ from nova.network.neutronv2 import constants as neutron_constants
 from nova import objects
 from nova.objects import base as obj_base
 from nova.objects import service as service_obj
-from nova import privsep
+import nova.privsep
 from nova import quota as nova_quota
 from nova import rpc
 from nova import service
@@ -2047,7 +2047,7 @@ class PrivsepFixture(fixtures.Fixture):
     def setUp(self):
         super(PrivsepFixture, self).setUp()
         self.useFixture(fixtures.MockPatchObject(
-            privsep.sys_admin_pctxt, 'client_mode', False))
+            nova.privsep.sys_admin_pctxt, 'client_mode', False))
 
 
 class NoopQuotaDriverFixture(fixtures.Fixture):

nova/tests/unit/test_hacking.py

@@ -860,3 +860,37 @@ class HackingTestCase(test.NoDBTestCase):
                    self.assertNotRegexpMatchesbar("Notmatch", output)
                """
         self._assert_has_no_errors(code, checks.assert_regexpmatches)
+
+    def test_import_alias_privsep(self):
+        code = """
+                  from nova import privsep
+                  import nova.privsep as nova_privsep
+                  from nova.privsep import linux_net
+                  import nova.privsep.linux_net as privsep_linux_net
+               """
+        errors = [(x + 1, 0, 'N362') for x in range(4)]
+        bad_filenames = ('nova/foo/bar.py',
+                         'nova/foo/privsep.py',
+                         'nova/privsep_foo/bar.py')
+        for filename in bad_filenames:
+            self._assert_has_errors(
+                code, checks.privsep_imports_not_aliased,
+                expected_errors=errors,
+                filename=filename)
+        good_filenames = ('nova/privsep.py',
+                          'nova/privsep/__init__.py',
+                          'nova/privsep/foo.py')
+        for filename in good_filenames:
+            self._assert_has_no_errors(
+                code, checks.privsep_imports_not_aliased, filename=filename)
+        code = """
+                  import nova.privsep
+                  import nova.privsep.foo
+                  import nova.privsep.foo.bar
+                  import nova.foo.privsep
+                  import nova.foo.privsep.bar
+                  import nova.tests.unit.whatever
+               """
+        for filename in (good_filenames + bad_filenames):
+            self._assert_has_no_errors(
+                code, checks.privsep_imports_not_aliased, filename=filename)