Merge ~jfguedez/charm-ubuntu-advantage:bug/1944591 into charm-ubuntu-advantage:master

[Jose Guedez]

Add support for proxy settings from the model

Supersedes this MP: https://code.launchpad.net/~mastier1/charm-ubuntu-advantage/+git/charm-ubuntu-advantage/+merge/412774

Includes the original code/commit and additionally:
* Fixes the broken unit tests
* Add a new test for the new functionality
* Removes the proxy variables for the apt calls, as it's already supported by apt-http[s]-proxy model variables
* Adds the proxy config to the UA client itself, instead of installing it with proxy environment variables. This ensures it's functional after the charm installs it.
* Keeps the environment variables for the `add-apt-repository` calls.

Tested in Focal/Bionic/Xenial with and without proxy config in the juju model.

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[Tom Haddon (mthaddon)]

A few comments/questions inline, otherwise looks good to me. Thanks.

review: Needs Fixing

[Jose Guedez (jfguedez)]

Thanks for the review. I have replied to your comments and hopefully addressed the issues in comments/logging. Please take a look again when you have a chance.

[Tom Haddon (mthaddon)]

I'm still a bit confused as to why we're setting this in `_setup_proxy_env` if UA client doesn't have an option for that:

self.env['no_proxy'] = self.env.get('JUJU_CHARM_NO_PROXY', '')

Can you clarify?

[Jose Guedez (jfguedez)]

@Tom, happy to clarify:

The charm needs connectivity via proxy for 4 different things:

1-To install the ppa (function `install_ppa`), which relies on the command `add-apt-repository`. This command supports using proxies via environment variables, which are mapped from the standard model-config variables juju-*-proxy variables:

* juju-http-proxy => JUJU_CHARM_HTTP_PROXY => http_proxy
* juju-https-proxy => JUJU_CHARM_HTTPs_PROXY => https_proxy
* juju-no-proxy => JUJU_CHARM_NO_PROXY => no_proxy

the values extracted from the environment in `_setup_proxy_env`

2-To install packages (function `install_packages`). The original MP used environment variables for this as well. I reverted this, as it should rely on model config apt-http[s]-proxy, that handles this case already. No changes in this MP to support this.

3-To install snaps (e.g. canonical-livepatch, as part of the UA installation). The original MP did not contain changes for this. The charm should rely on model config snap-http[s]-proxy, that handles this case already. No changes in this MP to support this either.

4-To configure the UA client (functions `detach_subscription`, `attach_subscription`, `get_status_output` + tool itself). The original MP passed environment variables as in #1 for this, and didn't configure ubuntu-advantage itself. This MP sets the proxy configuration of ubuntu-advantage itself via the CLI (which makes it's way to /etc/ubuntu-advantage/uaclient.conf), in the helper `_configure_ua_proxy`. It re-uses the values extracted from the environment in `_setup_proxy_env`, but not no_proxy which is not used in the UA tool. I guess it's not needed because the tool already knows which URLs it's interested in, and uses the proxy for them (no need to include/exclude)

* juju-http-proxy => JUJU_CHARM_HTTP_PROXY => http_proxy
* juju-https-proxy => JUJU_CHARM_HTTPs_PROXY => https_proxy

Hope that clarifies the different requirements of connectivity within the charm as I understand it. In the end it's trying to use the settings in the juju model, and either pass them to the functionality that is proxy aware: apt/snap are built into the model, UA has it's own configuration, and the PPA tool requires the information via the standard environment variables.

[Tom Haddon (mthaddon)]

Thanks for the explanation.

Could you add a comment in _setup_proxy_env to explain that the http_proxy and https_proxy env variables will be used for package installs and ua, while no_proxy will only be used be used for package installs as it's not a supported variable of ua itself?

[Jose Guedez (jfguedez)]

Thanks for the review. Added the comment as requested.

[Tom Haddon (mthaddon)]

LGTM, thx

review: Approve

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision 2ff56b61e24664d01cb2ddd843474706b8ce2e31

diff --git a/src/charm.py b/src/charm.py
index 25d7d0a..34878c0 100755
--- a/src/charm.py
+++ b/src/charm.py
@@ -6,6 +6,7 @@
 import hashlib
 import json
 import logging
+import os
 import subprocess
 import yaml
 
@@ -18,14 +19,14 @@ from ops.model import ActiveStatus, BlockedStatus, MaintenanceStatus
 logger = logging.getLogger(__name__)
 
 
-def install_ppa(ppa):
+def install_ppa(ppa, env):
     """Install specified ppa"""
-    subprocess.check_call(["add-apt-repository", "--yes", ppa])
+    subprocess.check_call(["add-apt-repository", "--yes", ppa], env=env)
 
 
-def remove_ppa(ppa):
+def remove_ppa(ppa, env):
     """Remove specified ppa"""
-    subprocess.check_call(["add-apt-repository", "--remove", "--yes", ppa])
+    subprocess.check_call(["add-apt-repository", "--remove", "--yes", ppa], env=env)
 
 
 def install_package(package):
@@ -74,13 +75,33 @@ class UbuntuAdvantageCharm(CharmBase):
 
     def __init__(self, *args):
         super().__init__(*args)
+        self._setup_proxy_env()
         self._state.set_default(
             contract_url=None,
             hashed_token=None,
             package_needs_installing=True,
             ppa=None)
+
         self.framework.observe(self.on.config_changed, self.config_changed)
 
+    def _setup_proxy_env(self):
+        """Setup proxy variables from model"""
+        self.env = dict(os.environ)
+        self.env['http_proxy'] = self.env.get('JUJU_CHARM_HTTP_PROXY', '')
+        self.env['https_proxy'] = self.env.get('JUJU_CHARM_HTTPS_PROXY', '')
+        self.env['no_proxy'] = self.env.get('JUJU_CHARM_NO_PROXY', '')
+
+        # The values for 'http_proxy' and 'https_proxy' will be used for the
+        # PPA install/remove operations (passed as environment variables), as
+        # well as for configuring the UA client.
+        # The value for 'no_proxy' is only used by the PPA install/remove
+        # operations (passed as an environment variable).
+
+        # log proxy environment variables for debugging
+        for envvar, value in self.env.items():
+            if envvar.endswith("proxy".lower()):
+                logger.debug("Envvar '%s' => '%s'", envvar, value)
+
     def config_changed(self, event):
         """Install and configure ubuntu-advantage tools and attachment"""
         logger.info("Beginning config_changed")
@@ -100,14 +121,14 @@ class UbuntuAdvantageCharm(CharmBase):
 
         if old_ppa and old_ppa != ppa:
             logger.info("Removing previously installed ppa (%s)", old_ppa)
-            remove_ppa(old_ppa)
+            remove_ppa(old_ppa, self.env)
             self._state.ppa = None
             # If ppa is changed, want to remove the previous version of the package for consistency
             self._state.package_needs_installing = True
 
         if ppa and ppa != old_ppa:
             logger.info("Installing ppa: %s", ppa)
-            install_ppa(ppa)
+            install_ppa(ppa, self.env)
             self._state.ppa = ppa
             # If ppa is changed, want to force an install of the package for potential updates
             self._state.package_needs_installing = True
@@ -132,6 +153,11 @@ class UbuntuAdvantageCharm(CharmBase):
         old_contract_url = self._state.contract_url
         config_changed = contract_url != old_contract_url
 
+        # Add the proxy configuration to the UA client.
+        # This is needed for attach/detach/status commands used by the charm,
+        # as well as regular tool operations.
+        self._configure_ua_proxy()
+
         status = get_status_output()
         if status["attached"] and (config_changed or token_changed):
             logger.info("Detaching ubuntu-advantage subscription")
@@ -165,6 +191,18 @@ class UbuntuAdvantageCharm(CharmBase):
         message = "Attached (" + ",".join(services) + ")"
         self.unit.status = ActiveStatus(message)
 
+    def _configure_ua_proxy(self):
+        """Configure the proxy options for the ubuntu-advantage client"""
+        for config_key in ("http_proxy", "https_proxy"):
+            subprocess.check_call(
+                [
+                    "ubuntu-advantage",
+                    "config",
+                    "set",
+                    "{}={}".format(config_key, self.env[config_key]),
+                ]
+            )
+
 
 if __name__ == "__main__":  # pragma: nocover
     main(UbuntuAdvantageCharm)
diff --git a/tests/test_charm.py b/tests/test_charm.py
index 1415f7d..f55d844 100644
--- a/tests/test_charm.py
+++ b/tests/test_charm.py
@@ -63,6 +63,9 @@ log_level: debug
 log_file: /var/log/ubuntu-advantage.log
 """
 
+TEST_PROXY_URL = "http://squid.internal:3128"
+TEST_NO_PROXY = "127.0.0.1,localhost,::1"
+
 
 def _written(handle):
     contents = "".join(["".join(a.args) for a in handle.write.call_args_list])
@@ -76,7 +79,8 @@ class TestCharm(TestCase):
             "call": patch("subprocess.call").start(),
             "check_call": patch("subprocess.check_call").start(),
             "check_output": patch("subprocess.check_output").start(),
-            "open": patch("builtins.open").start()
+            "open": patch("builtins.open").start(),
+            "environ": patch.dict("os.environ", clear=True).start()
         }
         self.mocks["check_output"].side_effect = [
             STATUS_DETACHED
@@ -86,6 +90,7 @@ class TestCharm(TestCase):
         self.harness = Harness(UbuntuAdvantageCharm)
         self.addCleanup(self.harness.cleanup)
         self.harness.begin()
+        self.env = self.harness.charm.env
 
     def test_config_defaults(self):
         self.assertEqual(self.harness.charm.config.get("contract_url"),
@@ -95,25 +100,25 @@ class TestCharm(TestCase):
 
     def test_config_changed_ppa_new(self):
         self.harness.update_config({"ppa": "ppa:ua-client/stable"})
-        self.assertEqual(self.mocks["check_call"].call_count, 4)
-        self.mocks["check_call"].assert_has_calls([
-            call(["add-apt-repository", "--yes", "ppa:ua-client/stable"]),
+        self.assertEqual(self.mocks["check_call"].call_count, 6)
+        self.mocks["check_call"].assert_has_calls(self._add_ua_proxy_setup_calls([
+            call(["add-apt-repository", "--yes", "ppa:ua-client/stable"], env=self.env),
             call(["apt", "remove", "--yes", "--quiet", "ubuntu-advantage-tools"]),
             call(["apt", "update"]),
             call(["apt", "install", "--yes", "--quiet", "ubuntu-advantage-tools"])
-        ])
+        ]))
         self.assertEqual(self.harness.charm._state.ppa, "ppa:ua-client/stable")
         self.assertFalse(self.harness.charm._state.package_needs_installing)
 
     def test_config_changed_ppa_updated(self):
         self.harness.update_config({"ppa": "ppa:ua-client/stable"})
-        self.assertEqual(self.mocks["check_call"].call_count, 4)
-        self.mocks["check_call"].assert_has_calls([
-            call(["add-apt-repository", "--yes", "ppa:ua-client/stable"]),
+        self.assertEqual(self.mocks["check_call"].call_count, 6)
+        self.mocks["check_call"].assert_has_calls(self._add_ua_proxy_setup_calls([
+            call(["add-apt-repository", "--yes", "ppa:ua-client/stable"], env=self.env),
             call(["apt", "remove", "--yes", "--quiet", "ubuntu-advantage-tools"]),
             call(["apt", "update"]),
             call(["apt", "install", "--yes", "--quiet", "ubuntu-advantage-tools"])
-        ])
+        ]))
         self.assertEqual(self.harness.charm._state.ppa, "ppa:ua-client/stable")
         self.assertFalse(self.harness.charm._state.package_needs_installing)
 
@@ -122,26 +127,28 @@ class TestCharm(TestCase):
         ]
         self.mocks["check_call"].reset_mock()
         self.harness.update_config({"ppa": "ppa:different-client/unstable"})
-        self.assertEqual(self.mocks["check_call"].call_count, 5)
-        self.mocks["check_call"].assert_has_calls([
-            call(["add-apt-repository", "--remove", "--yes", "ppa:ua-client/stable"]),
-            call(["add-apt-repository", "--yes", "ppa:different-client/unstable"]),
+        self.assertEqual(self.mocks["check_call"].call_count, 7)
+        self.mocks["check_call"].assert_has_calls(self._add_ua_proxy_setup_calls([
+            call(["add-apt-repository", "--remove", "--yes", "ppa:ua-client/stable"],
+                 env=self.env),
+            call(["add-apt-repository", "--yes", "ppa:different-client/unstable"],
+                 env=self.env),
             call(["apt", "remove", "--yes", "--quiet", "ubuntu-advantage-tools"]),
             call(["apt", "update"]),
             call(["apt", "install", "--yes", "--quiet", "ubuntu-advantage-tools"])
-        ])
+        ]))
         self.assertEqual(self.harness.charm._state.ppa, "ppa:different-client/unstable")
         self.assertFalse(self.harness.charm._state.package_needs_installing)
 
     def test_config_changed_ppa_unmodified(self):
         self.harness.update_config({"ppa": "ppa:ua-client/stable"})
-        self.assertEqual(self.mocks["check_call"].call_count, 4)
-        self.mocks["check_call"].assert_has_calls([
-            call(["add-apt-repository", "--yes", "ppa:ua-client/stable"]),
+        self.assertEqual(self.mocks["check_call"].call_count, 6)
+        self.mocks["check_call"].assert_has_calls(self._add_ua_proxy_setup_calls([
+            call(["add-apt-repository", "--yes", "ppa:ua-client/stable"], env=self.env),
             call(["apt", "remove", "--yes", "--quiet", "ubuntu-advantage-tools"]),
             call(["apt", "update"]),
             call(["apt", "install", "--yes", "--quiet", "ubuntu-advantage-tools"])
-        ])
+        ]))
         self.assertEqual(self.harness.charm._state.ppa, "ppa:ua-client/stable")
         self.assertFalse(self.harness.charm._state.package_needs_installing)
 
@@ -151,19 +158,20 @@ class TestCharm(TestCase):
             STATUS_DETACHED
         ]
         self.harness.update_config({"ppa": "ppa:ua-client/stable"})
-        self.assertEqual(self.mocks["check_call"].call_count, 0)
+        self.assertEqual(self.mocks["check_call"].call_count, 2)
+        self.mocks["check_call"].assert_has_calls(self._add_ua_proxy_setup_calls([]))
         self.assertEqual(self.harness.charm._state.ppa, "ppa:ua-client/stable")
         self.assertFalse(self.harness.charm._state.package_needs_installing)
 
     def test_config_changed_ppa_unset(self):
         self.harness.update_config({"ppa": "ppa:ua-client/stable"})
-        self.assertEqual(self.mocks["check_call"].call_count, 4)
-        self.mocks["check_call"].assert_has_calls([
-            call(["add-apt-repository", "--yes", "ppa:ua-client/stable"]),
+        self.assertEqual(self.mocks["check_call"].call_count, 6)
+        self.mocks["check_call"].assert_has_calls(self._add_ua_proxy_setup_calls([
+            call(["add-apt-repository", "--yes", "ppa:ua-client/stable"], env=self.env),
             call(["apt", "remove", "--yes", "--quiet", "ubuntu-advantage-tools"]),
             call(["apt", "update"]),
             call(["apt", "install", "--yes", "--quiet", "ubuntu-advantage-tools"])
-        ])
+        ]))
         self.assertEqual(self.harness.charm._state.ppa, "ppa:ua-client/stable")
         self.assertFalse(self.harness.charm._state.package_needs_installing)
 
@@ -173,13 +181,14 @@ class TestCharm(TestCase):
             STATUS_DETACHED
         ]
         self.harness.update_config({"ppa": ""})
-        self.assertEqual(self.mocks["check_call"].call_count, 4)
-        self.mocks["check_call"].assert_has_calls([
-            call(["add-apt-repository", "--remove", "--yes", "ppa:ua-client/stable"]),
+        self.assertEqual(self.mocks["check_call"].call_count, 6)
+        self.mocks["check_call"].assert_has_calls(self._add_ua_proxy_setup_calls([
+            call(["add-apt-repository", "--remove", "--yes", "ppa:ua-client/stable"],
+                 env=self.env),
             call(["apt", "remove", "--yes", "--quiet", "ubuntu-advantage-tools"]),
             call(["apt", "update"]),
             call(["apt", "install", "--yes", "--quiet", "ubuntu-advantage-tools"])
-        ])
+        ]))
         self.assertIsNone(self.harness.charm._state.ppa)
         self.assertFalse(self.harness.charm._state.package_needs_installing)
 
@@ -223,13 +232,13 @@ class TestCharm(TestCase):
             STATUS_ATTACHED
         ]
         self.harness.update_config({"token": "test-token"})
-        self.assertEqual(self.mocks["check_call"].call_count, 4)
-        self.mocks["check_call"].assert_has_calls([
-            call(["add-apt-repository", "--yes", "ppa:ua-client/stable"]),
+        self.assertEqual(self.mocks["check_call"].call_count, 6)
+        self.mocks["check_call"].assert_has_calls(self._add_ua_proxy_setup_calls([
+            call(["add-apt-repository", "--yes", "ppa:ua-client/stable"], env=self.env),
             call(["apt", "remove", "--yes", "--quiet", "ubuntu-advantage-tools"]),
             call(["apt", "update"]),
             call(["apt", "install", "--yes", "--quiet", "ubuntu-advantage-tools"])
-        ])
+        ]))
         self.mocks["open"].assert_called_with("/etc/ubuntu-advantage/uaclient.conf", "r+")
         handle = self.mocks["open"]()
         expected = dedent("""\
@@ -255,10 +264,10 @@ class TestCharm(TestCase):
             STATUS_ATTACHED
         ]
         self.harness.update_config({"token": "test-token-2"})
-        self.assertEqual(self.mocks["check_call"].call_count, 1)
-        self.mocks["check_call"].assert_has_calls([
+        self.assertEqual(self.mocks["check_call"].call_count, 3)
+        self.mocks["check_call"].assert_has_calls(self._add_ua_proxy_setup_calls([
             call(["ubuntu-advantage", "detach", "--assume-yes"])
-        ])
+        ], append=False))
         self.assertEqual(self.mocks["call"].call_count, 1)
         self.mocks["call"].assert_has_calls([
             call(["ubuntu-advantage", "attach", "test-token-2"])
@@ -295,10 +304,10 @@ class TestCharm(TestCase):
             STATUS_DETACHED
         ]
         self.harness.update_config({"token": ""})
-        self.assertEqual(self.mocks["check_call"].call_count, 1)
-        self.mocks["check_call"].assert_has_calls([
+        self.assertEqual(self.mocks["check_call"].call_count, 3)
+        self.mocks["check_call"].assert_has_calls(self._add_ua_proxy_setup_calls([
             call(["ubuntu-advantage", "detach", "--assume-yes"])
-        ])
+        ], append=False))
         self.assertEqual(self.mocks["call"].call_count, 0)
         self.assertIsNone(self.harness.charm._state.hashed_token)
         self.assertEqual(self.harness.model.unit.status, BlockedStatus("No token configured"))
@@ -335,7 +344,7 @@ class TestCharm(TestCase):
     def test_config_changed_ppa_contains_newline(self):
         self.harness.update_config({"ppa": "ppa:ua-client/stable\n"})
         self.mocks["check_call"].assert_has_calls([
-            call(["add-apt-repository", "--yes", "ppa:ua-client/stable"]),
+            call(["add-apt-repository", "--yes", "ppa:ua-client/stable"], env=self.env),
         ])
         self.assertEqual(self.harness.charm._state.ppa, "ppa:ua-client/stable")
 
@@ -460,3 +469,39 @@ class TestCharm(TestCase):
         handle.truncate.assert_called_once()
         self.mocks["call"].assert_not_called()
         self.assertEqual(self.harness.model.unit.status, BlockedStatus("No token configured"))
+
+    def test_setup_proxy_env(self):
+        self.mocks["environ"].update(
+            {
+                "JUJU_CHARM_HTTP_PROXY": TEST_PROXY_URL,
+                "JUJU_CHARM_HTTPS_PROXY": TEST_PROXY_URL,
+                "JUJU_CHARM_NO_PROXY": TEST_NO_PROXY,
+            }
+        )
+
+        self.harness.charm._setup_proxy_env()
+        self.assertEqual(self.harness.charm.env["http_proxy"], TEST_PROXY_URL)
+        self.assertEqual(self.harness.charm.env["https_proxy"], TEST_PROXY_URL)
+        self.assertEqual(self.harness.charm.env["no_proxy"], TEST_NO_PROXY)
+
+    def _add_ua_proxy_setup_calls(self, call_list, append=True):
+        """Helper to generate the calls used for UA proxy setup"""
+        proxy_calls = [
+            call(
+                [
+                    "ubuntu-advantage",
+                    "config",
+                    "set",
+                    "http_proxy={}".format(self.env["http_proxy"]),
+                ]
+            ),
+            call(
+                [
+                    "ubuntu-advantage",
+                    "config",
+                    "set",
+                    "https_proxy={}".format(self.env["https_proxy"]),
+                ]
+            ),
+        ]
+        return call_list + proxy_calls if append else proxy_calls + call_list