launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:code-import-scheduler-blacklisted-hostnames into launchpad:master.

Commit message:
Pass --exclude-host options to code import workers

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/393732

This is equivalent to the policy currently implemented in CodeImportBranchOpenPolicy.check_one_url, but requires less tight coupling between the code import workers and the rest of Launchpad.

https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/393216 must be deployed to production before landing this.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:code-import-scheduler-blacklisted-hostnames into launchpad:master.

diff --git a/lib/lp/code/model/codeimportjob.py b/lib/lp/code/model/codeimportjob.py
index fa81587..3afe6b4 100644
--- a/lib/lp/code/model/codeimportjob.py
+++ b/lib/lp/code/model/codeimportjob.py
@@ -36,7 +36,10 @@ from lp.code.enums import (
     GitRepositoryType,
     RevisionControlSystems,
     )
-from lp.code.interfaces.branch import IBranch
+from lp.code.interfaces.branch import (
+    get_blacklisted_hostnames,
+    IBranch,
+    )
 from lp.code.interfaces.codehosting import (
     branch_id_alias,
     compose_public_url,
@@ -165,6 +168,13 @@ class CodeImportJob(StormBase):
             # XXX cjwatson 2016-10-12: Consider arranging for this to be
             # passed to worker processes in the environment instead.
             result.extend(['--macaroon', macaroon.serialize()])
+        # Refuse pointless self-mirroring.
+        if rcs_type == target_rcs_type:
+            result.extend(['--exclude-host', config.vhost.mainsite.hostname])
+        # Refuse to import from configured hostnames, typically localhost
+        # and similar.
+        for hostname in get_blacklisted_hostnames():
+            result.extend(['--exclude-host', hostname])
         return result
 
     def destroySelf(self):
diff --git a/lib/lp/code/model/tests/test_codeimportjob.py b/lib/lp/code/model/tests/test_codeimportjob.py
index 14ea8f2..0bd2ebc 100644
--- a/lib/lp/code/model/tests/test_codeimportjob.py
+++ b/lib/lp/code/model/tests/test_codeimportjob.py
@@ -129,7 +129,9 @@ class TestCodeImportJob(TestCaseWithFactory):
         self.assertArgumentsMatch(
             code_import, Equals([
                 str(code_import.branch.id), 'bzr', 'bzr',
-                'http://example.com/foo']))
+                'http://example.com/foo',
+                '--exclude-host', 'launchpad.test',
+                ]))
 
     def test_git_arguments(self):
         code_import = self.factory.makeCodeImport(
@@ -152,7 +154,9 @@ class TestCodeImportJob(TestCaseWithFactory):
                 Equals('git'), Equals('git'),
                 Equals('git://git.example.com/project.git'),
                 Equals('--macaroon'),
-                CodeImportJobMacaroonVerifies(code_import)]),
+                CodeImportJobMacaroonVerifies(code_import),
+                Equals('--exclude-host'), Equals('launchpad.test'),
+                ]),
             # Start the job so that the macaroon can be verified.
             start_job=True)
 
@@ -183,7 +187,9 @@ class TestCodeImportJob(TestCaseWithFactory):
                 str(code_import.branch.id), 'bzr', 'bzr',
                 'bzr://bzr.example.com/foo',
                 '--stacked-on',
-                compose_public_url('http', branch_id_alias(devfocus))]))
+                compose_public_url('http', branch_id_alias(devfocus)),
+                '--exclude-host', 'launchpad.test',
+                ]))
 
     def test_bzr_stacked_private(self):
         # Code imports can't be stacked on private branches.
@@ -196,7 +202,23 @@ class TestCodeImportJob(TestCaseWithFactory):
         branch.stacked_on = devfocus
         self.assertArgumentsMatch(
             code_import, Equals([
-                str(branch.id), 'bzr', 'bzr', 'bzr://bzr.example.com/foo']))
+                str(branch.id), 'bzr', 'bzr', 'bzr://bzr.example.com/foo',
+                '--exclude-host', 'launchpad.test',
+                ]))
+
+    def test_blacklisted_hostnames(self):
+        # Additional blacklisted hostnames are passed as --exclude-host
+        # options.
+        self.pushConfig(
+            'codehosting', blacklisted_hostnames='localhost,127.0.0.1')
+        code_import = self.factory.makeCodeImport(
+            git_repo_url="git://git.example.com/project.git")
+        self.assertArgumentsMatch(
+            code_import, Equals([
+                str(code_import.branch.id), 'git', 'bzr',
+                'git://git.example.com/project.git',
+                '--exclude-host', 'localhost', '--exclude-host', '127.0.0.1',
+                ]))
 
 
 class TestCodeImportJobSet(TestCaseWithFactory):