Merge lp:~simpoir/landscape-client/bug_1531150_package_reporter_proxy into lp:~landscape/landscape-client/trunk

[Simon Poirier]

Add proxy handling to package reporter.

Add proxy handling to package reporter.

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 916
Branch: lp:~simpoir/landscape-client/bug_1531150_package_reporter_proxy
Jenkins: https://ci.lscape.net/job/latch-test/5865/

review: Approve (test results)

[Eric Snow (ericsnowcurrently)]

Aside from a couple small issues and a few questions, LGTM.

Just to make sure I understand, currently there are 2 separate situations where package-reporter ignores the landscape-configured proxy values: the apt-update helper and fetching the hash-id-database. Are there other situations where package-reporter is handling the proxy correctly?

Also, you're solving the apt-update problem by passing the proxy settings from the config into the env vars under which the apt-update helper runs. For the fetch issue, you're passing the proxy in to fetch() and on to curl. Is all that right?

review: Needs Information

[lp:~simpoir/landscape-client/bug_1531150_package_reporter_proxy]

917. By Simon Poirier on 2016-08-04

address some comments from ericsnow

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 917
Branch: lp:~simpoir/landscape-client/bug_1531150_package_reporter_proxy
Jenkins: https://ci.lscape.net/job/latch-test/6189/

review: Approve (test results)

[Eric Snow (ericsnowcurrently)]

+1

review: Approve

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 917
Branch: lp:~simpoir/landscape-client/bug_1531150_package_reporter_proxy
Jenkins: https://ci.lscape.net/job/latch-test/6578/

review: Approve (test results)

[Adam Collard (adam-collard)]

review: Needs Information

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 917
Branch: lp:~simpoir/landscape-client/bug_1531150_package_reporter_proxy
Jenkins: https://ci.lscape.net/job/latch-test/7166/

review: Approve (test results)

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 917
Branch: lp:~simpoir/landscape-client/bug_1531150_package_reporter_proxy
Jenkins: https://ci.lscape.net/job/latch-test/7455/

review: Approve (test results)

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 917
Branch: lp:~simpoir/landscape-client/bug_1531150_package_reporter_proxy
Jenkins: https://ci.lscape.net/job/latch-test/7832/

review: Approve (test results)

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 917
Branch: lp:~simpoir/landscape-client/bug_1531150_package_reporter_proxy
Jenkins: https://ci.lscape.net/job/latch-test/7845/

review: Approve (test results)

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 917
Branch: lp:~simpoir/landscape-client/bug_1531150_package_reporter_proxy
Jenkins: https://ci.lscape.net/job/latch-test/7890/

review: Approve (test results)

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 917
Branch: lp:~simpoir/landscape-client/bug_1531150_package_reporter_proxy
Jenkins: https://ci.lscape.net/job/latch-test/7929/

review: Approve (test results)

[Adam Collard (adam-collard)]

review: Approve

=== modified file 'apt-update/apt-update.c'
--- apt-update/apt-update.c	2011-11-08 10:54:08 +0000
+++ apt-update/apt-update.c	2016-08-04 16:13:30 +0000
@@ -19,7 +19,7 @@
 int main(int argc, char *argv[], char *envp[])
 {
   char *apt_argv[] = {"/usr/bin/apt-get", "-q", "update", NULL};
-  char *apt_envp[] = {"PATH=/bin:/usr/bin", NULL, NULL};
+  char *apt_envp[] = {"PATH=/bin:/usr/bin", NULL, NULL, NULL, NULL};
 
   // Set the HOME environment variable
   struct passwd *pwd = getpwuid(geteuid());
@@ -33,6 +33,24 @@
     exit(1);
   }
 
+  // Pass proxy environment variables
+  int proxy_arg = 2;
+  char *http_proxy = getenv("http_proxy");
+  if (http_proxy) {
+      if (asprintf(&apt_envp[proxy_arg], "http_proxy=%s", http_proxy) == -1) {
+        perror("error: Unable to set http_proxy environment variable");
+        exit(1);
+      }
+      proxy_arg++;
+  }
+  char *https_proxy = getenv("https_proxy");
+  if (https_proxy) {
+      if (asprintf(&apt_envp[proxy_arg], "https_proxy=%s", https_proxy) == -1) {
+        perror("error: Unable to set https_proxy environment variable");
+        exit(1);
+      }
+  }
+
   // Drop any supplementary group
   if (setgroups(0, NULL) == -1) {
     perror("error: Unable to set supplementary groups IDs");

=== modified file 'landscape/lib/fetch.py'
--- landscape/lib/fetch.py	2014-12-04 09:48:07 +0000
+++ landscape/lib/fetch.py	2016-08-04 16:13:30 +0000
@@ -45,7 +45,7 @@
 
 def fetch(url, post=False, data="", headers={}, cainfo=None, curl=None,
           connect_timeout=30, total_timeout=600, insecure=False, follow=True,
-          user_agent=None):
+          user_agent=None, proxy=None):
     """Retrieve a URL and return the content.
 
     @param url: The url to be fetched.
@@ -61,6 +61,7 @@
         during autodiscovery)
     @param follow: If True, follow HTTP redirects (default True).
     @param user_agent: The user-agent to set in the request.
+    @param proxy: The proxy url to use for the request.
     """
     import pycurl
     output = StringIO(data)
@@ -94,6 +95,9 @@
     if user_agent is not None:
         curl.setopt(pycurl.USERAGENT, user_agent)
 
+    if proxy is not None:
+        curl.setopt(pycurl.PROXY, proxy)
+
     curl.setopt(pycurl.MAXREDIRS, 5)
     curl.setopt(pycurl.CONNECTTIMEOUT, connect_timeout)
     curl.setopt(pycurl.LOW_SPEED_LIMIT, 1)

=== modified file 'landscape/lib/tests/test_fetch.py'
--- landscape/lib/tests/test_fetch.py	2015-06-08 08:16:10 +0000
+++ landscape/lib/tests/test_fetch.py	2016-08-04 16:13:30 +0000
@@ -286,6 +286,14 @@
         self.assertEqual(result, "result")
         self.assertEqual("user-agent", curl.options[pycurl.USERAGENT])
 
+    def test_pycurl_proxy(self):
+        """If provided, the proxy is set in the request."""
+        curl = CurlStub("result")
+        proxy = "http://my.little.proxy"
+        result = fetch("http://example.com", curl=curl, proxy=proxy)
+        self.assertEqual("result", result)
+        self.assertEqual(proxy, curl.options[pycurl.PROXY])
+
     def test_create_curl(self):
         curls = []
 

=== modified file 'landscape/package/reporter.py'
--- landscape/package/reporter.py	2015-01-12 12:53:36 +0000
+++ landscape/package/reporter.py	2016-08-04 16:13:30 +0000
@@ -37,6 +37,10 @@
         parser.add_option("--force-apt-update", default=False,
                           action="store_true",
                           help="Force running apt-update.")
+        parser.add_option("--http-proxy", metavar="URL",
+                          help="The URL of the HTTP proxy, if one is needed.")
+        parser.add_option("--https-proxy", metavar="URL",
+                          help="The URL of the HTTPS proxy, if one is needed.")
         return parser
 
 
@@ -132,8 +136,14 @@
                 logging.warning("Couldn't download hash=>id database: %s" %
                                 str(exception))
 
+            if url.startswith("https"):
+                proxy = self._config.get("https_proxy")
+            else:
+                proxy = self._config.get("http_proxy")
+
             result = fetch_async(url,
-                                 cainfo=self._config.get("ssl_public_key"))
+                                 cainfo=self._config.get("ssl_public_key"),
+                                 proxy=proxy)
             result.addCallback(fetch_ok)
             result.addErrback(fetch_error)
 
@@ -261,7 +271,12 @@
         Run apt-update using the passed in deferred, which allows for callers
         to inspect the result code.
         """
-        result = spawn_process(self.apt_update_filename)
+        env = {}
+        if self._config.http_proxy:
+            env["http_proxy"] = self._config.http_proxy
+        if self._config.https_proxy:
+            env["https_proxy"] = self._config.https_proxy
+        result = spawn_process(self.apt_update_filename, env=env)
 
         def callback((out, err, code), deferred):
             return deferred.callback((out, err, code))

=== modified file 'landscape/package/tests/test_reporter.py'
--- landscape/package/tests/test_reporter.py	2016-06-16 22:52:21 +0000
+++ landscape/package/tests/test_reporter.py	2016-08-04 16:13:30 +0000
@@ -300,7 +300,35 @@
 
         logging_mock.assert_called_once_with(
             "Downloaded hash=>id database from %s" % hash_id_db_url)
-        mock_fetch_async.assert_called_once_with(hash_id_db_url, cainfo=None)
+        mock_fetch_async.assert_called_once_with(
+            hash_id_db_url, cainfo=None, proxy=None)
+        return result
+
+    @mock.patch("landscape.package.reporter.fetch_async",
+                return_value=succeed("hash-ids"))
+    @mock.patch("logging.info", return_value=None)
+    def test_fetch_hash_id_db_with_proxy(self, logging_mock, mock_fetch_async):
+        """fetching hash-id-db uses proxy settings"""
+        # Assume package_hash_id_url is set
+        self.config.data_path = self.makeDir()
+        self.config.package_hash_id_url = "https://fake.url/path/"
+        os.makedirs(os.path.join(self.config.data_path, "package", "hash-id"))
+
+        # Fake uuid, codename and arch
+        message_store = self.broker_service.message_store
+        message_store.set_server_uuid("uuid")
+        self.reporter.lsb_release_filename = self.makeFile(SAMPLE_LSB_RELEASE)
+        self.facade.set_arch("arch")
+
+        # Let's say fetch_async is successful
+        hash_id_db_url = self.config.package_hash_id_url + "uuid_codename_arch"
+
+        # set proxy settings
+        self.config.https_proxy = "http://helloproxy:8000"
+
+        result = self.reporter.fetch_hash_id_db()
+        mock_fetch_async.assert_called_once_with(
+            hash_id_db_url, cainfo=None, proxy="http://helloproxy:8000")
         return result
 
     @mock.patch("landscape.package.reporter.fetch_async")
@@ -409,7 +437,8 @@
             self.assertTrue(os.path.exists(hash_id_db_filename))
             self.assertEqual(open(hash_id_db_filename).read(), "hash-ids")
         result.addCallback(callback)
-        mock_fetch_async.assert_called_once_with(hash_id_db_url, cainfo=None)
+        mock_fetch_async.assert_called_once_with(
+            hash_id_db_url, cainfo=None, proxy=None)
         return result
 
     @mock.patch("landscape.package.reporter.fetch_async",
@@ -443,7 +472,8 @@
 
         logging_mock.assert_called_once_with(
             "Couldn't download hash=>id database: fetch error")
-        mock_fetch_async.assert_called_once_with(hash_id_db_url, cainfo=None)
+        mock_fetch_async.assert_called_once_with(
+            hash_id_db_url, cainfo=None, proxy=None)
         return result
 
     @mock.patch("logging.warning", return_value=None)
@@ -497,7 +527,7 @@
         # Now go!
         result = self.reporter.fetch_hash_id_db()
         mock_fetch_async.assert_called_once_with(
-            hash_id_db_url, cainfo=self.config.ssl_public_key)
+            hash_id_db_url, cainfo=self.config.ssl_public_key, proxy=None)
 
         return result
 
@@ -1473,6 +1503,44 @@
         reactor.callWhenRunning(do_test)
         return deferred
 
+    @mock.patch("landscape.package.reporter.spawn_process",
+                return_value=succeed(("", "", 0)))
+    def test_run_apt_update_honors_http_proxy(self, mock_spawn_process):
+        """
+        The PackageReporter.run_apt_update method honors the http_proxy
+        config when calling the apt-update wrapper.
+        """
+        self.config.http_proxy = "http://proxy_server:8080"
+        self.reporter.sources_list_filename = "/I/Dont/Exist"
+
+        update_result = self.reporter.run_apt_update()
+        # run_apt_update uses reactor.call_later so advance a bit
+        self.reactor.advance(0)
+        self.successResultOf(update_result)
+
+        mock_spawn_process.assert_called_once_with(
+            self.reporter.apt_update_filename,
+            env={"http_proxy": "http://proxy_server:8080"})
+
+    @mock.patch("landscape.package.reporter.spawn_process",
+                return_value=succeed(("", "", 0)))
+    def test_run_apt_update_honors_https_proxy(self, mock_spawn_process):
+        """
+        The PackageReporter.run_apt_update method honors the https_proxy
+        config when calling the apt-update wrapper.
+        """
+        self.config.https_proxy = "http://proxy_server:8443"
+        self.reporter.sources_list_filename = "/I/Dont/Exist"
+
+        update_result = self.reporter.run_apt_update()
+        # run_apt_update uses reactor.call_later, so advance a bit
+        self.reactor.advance(0)
+        self.successResultOf(update_result)
+
+        mock_spawn_process.assert_called_once_with(
+            self.reporter.apt_update_filename,
+            env={"https_proxy": "http://proxy_server:8443"})
+
     def test_run_apt_update_error_on_cache_file(self):
         """
         L{PackageReporter.run_apt_update} succeeds if the command fails because