Merge ~seyeongkim/charm-ubuntu-advantage:adding_proxy_configs into charm-ubuntu-advantage:master

[Seyeong Kim]

Adding override-http-proxy

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

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

[Andrew Polukhin (andrew-polukhin)]

LGTM and thanks for the contribution! Please wait for the second review.

review: Approve

[Allan Vidal (alnvdl)]

Hi Seyeong,

Thanks for this patch! I deployed it, and it seems to be working. I do have a few suggestions though:

1. If you change the config parameters, it's not getting updated up by the charm. That's because you need to reload the env config whenever the config changes:

    def config_changed(self, event):
        """Install and configure ubuntu-advantage tools and attachment."""
        logger.info("Beginning config_changed")
        self.unit.status = MaintenanceStatus("Configuring")
        self._setup_proxy_env() <-- This is missing
        self._handle_ppa_state()
        self._handle_package_state()
        self._handle_subscription_state()
        ...

2. It would be best to also try to capture the behavior above in a unit test (something like `test_config_changed_set_and_unset_proxy`).

3. I think `no-proxy` is not supported by the Pro client, so maybe we should
   just remove this override? See the output of
   `sudo ubuntu-advantage config show` for details.

4. I think documentation could be improved a bit in the README as well. Here's
   my suggestion to add there (feel free to tweak it to your liking):

   ## Proxy config

    By default, this charm will pick up the proxy configuration from the Juju
    model. If you want to use a different proxy instead for the units, you can
    override that with configs `override-http-proxy` and `override-https-proxy`.

    Please note that in all cases, the Ubuntu Pro client will check if these proxy
    configs are valid. If it cannot use them (e.g., proxy is not reachable), you
    will most likely get a `hook failed: "config-changed"` message. You can check
    the causes with `juju debug-log`.

Allan

[Seyeong Kim (seyeongkim)]

Hey Allen, Thanks for the review.

I re-uploaded as you mentioned.

I'm not familiar with unit test yet, Please check it again if it is ok.

Also, I removed no_proxy in test_setup_proxy_env. but I'm not sure i need to remove as it is not included in my patch.

Thanks.

[Allan Vidal (alnvdl)]

Thanks for the progress Seyeong! I have a few suggestions still.

review: Needs Fixing

[Seyeong Kim (seyeongkim)]

Thanks a lot. I changed it and reformatted with black

[Allan Vidal (alnvdl)]

LGTM, thanks for this work @seyeongkim!

review: Approve

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

Change successfully merged at revision 94fed8b7a73b66d4e416430a02147c1ecbcd2036

diff --git a/README.md b/README.md
index e9bd371..87ecf55 100644
--- a/README.md
+++ b/README.md
@@ -30,3 +30,14 @@ not promulgated.
 In the past, it was also published as https://charmhub.io/ubuntu-pro (also
 not promulgated), but that was discontinued until proper support for charm
 renames is introduced.
+
+## Proxy config
+
+By default, this charm will pick up the proxy configuration from the Juju
+model. If you want to use a different proxy instead for the units, you can
+override that with configs `override-http-proxy` and `override-https-proxy`.
+
+Please note that in all cases, the Ubuntu Pro client will check if these proxy
+configs are valid. If it cannot use them (e.g., proxy is not reachable), you
+will most likely get a `hook failed: "config-changed"` message. You can check
+the causes with `juju debug-log`.
diff --git a/config.yaml b/config.yaml
index a6fb1a7..e81d8b5 100644
--- a/config.yaml
+++ b/config.yaml
@@ -16,3 +16,11 @@ options:
     default: ""
     description: Ubuntu Pro token obtained from https://ubuntu.com/pro.
     type: string
+  override-http-proxy:
+    default: ""
+    description: http-proxy which can override juju http-proxy
+    type: string
+  override-https-proxy:
+    default: ""
+    description: https-proxy which can override juju https-proxy
+    type: string
diff --git a/src/charm.py b/src/charm.py
index 04bd738..bf5051c 100755
--- a/src/charm.py
+++ b/src/charm.py
@@ -76,8 +76,12 @@ class UbuntuAdvantageCharm(CharmBase):
     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["http_proxy"] = self.config.get("override-http-proxy") or self.env.get(
+            "JUJU_CHARM_HTTP_PROXY", ""
+        )
+        self.env["https_proxy"] = self.config.get("override-https-proxy") or 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
@@ -95,6 +99,7 @@ class UbuntuAdvantageCharm(CharmBase):
         """Install and configure ubuntu-advantage tools and attachment."""
         logger.info("Beginning config_changed")
         self.unit.status = MaintenanceStatus("Configuring")
+        self._setup_proxy_env()
         self._handle_ppa_state()
         self._handle_package_state()
         self._handle_subscription_state()
@@ -181,14 +186,24 @@ class UbuntuAdvantageCharm(CharmBase):
     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 self.env[config_key]:
+                subprocess.check_call(
+                    [
+                        "ubuntu-advantage",
+                        "config",
+                        "set",
+                        "{}={}".format(config_key, self.env[config_key]),
+                    ]
+                )
+            else:
+                subprocess.check_call(
+                    [
+                        "ubuntu-advantage",
+                        "config",
+                        "unset",
+                        config_key,
+                    ]
+                )
 
 
 if __name__ == "__main__":  # pragma: nocover
diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py
index f544c99..da7c0f0 100644
--- a/tests/unit/test_charm.py
+++ b/tests/unit/test_charm.py
@@ -442,6 +442,65 @@ class TestCharm(TestCase):
         self.mocks["call"].assert_not_called()
         self.assertEqual(self.harness.model.unit.status, BlockedStatus("No token configured"))
 
+    def test_config_changed_set_and_unset_proxy_override(self):
+        # Set proxy override once.
+        self.mocks["check_output"].side_effect = [STATUS_DETACHED]
+        self.harness.update_config(
+            {
+                "override-http-proxy": "http://localhost:3128",
+                "override-https-proxy": "http://localhost:3128",
+            }
+        )
+        self.mocks["check_call"].assert_has_calls(
+            [
+                call(["ubuntu-advantage", "config", "set", "http_proxy=http://localhost:3128"]),
+                call(["ubuntu-advantage", "config", "set", "https_proxy=http://localhost:3128"]),
+            ]
+        )
+        self.mocks["check_call"].reset_mock()
+
+        # Set proxy override again.
+        self.mocks["check_output"].side_effect = [STATUS_DETACHED]
+        self.harness.update_config(
+            {
+                "override-http-proxy": "http://squid.internal:3128",
+                "override-https-proxy": "http://squid.internal:3128",
+            }
+        )
+        self.mocks["check_call"].assert_has_calls(
+            [
+                call(
+                    ["ubuntu-advantage", "config", "set", "http_proxy=http://squid.internal:3128"]
+                ),
+                call(
+                    ["ubuntu-advantage", "config", "set", "https_proxy=http://squid.internal:3128"]
+                ),
+            ]
+        )
+        self.mocks["check_call"].reset_mock()
+
+        # Unset proxy override.
+        self.mocks["check_output"].side_effect = [STATUS_DETACHED]
+        self.harness.update_config({"override-http-proxy": "", "override-https-proxy": ""})
+        self.mocks["check_call"].assert_has_calls(
+            [
+                call(["ubuntu-advantage", "config", "unset", "http_proxy"]),
+                call(["ubuntu-advantage", "config", "unset", "https_proxy"]),
+            ]
+        )
+
+    def test_setup_proxy_config(self):
+        self.harness.update_config(
+            {
+                "override-http-proxy": TEST_PROXY_URL,
+                "override-https-proxy": TEST_PROXY_URL,
+            }
+        )
+
+        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)
+
     def test_setup_proxy_env(self):
         self.mocks["environ"].update(
             {
@@ -458,24 +517,53 @@ class TestCharm(TestCase):
 
     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"]),
-                ]
-            ),
-        ]
+        proxy_calls = []
+        if self.env["http_proxy"]:
+            proxy_calls.append(
+                call(
+                    [
+                        "ubuntu-advantage",
+                        "config",
+                        "set",
+                        "http_proxy={}".format(self.env["http_proxy"]),
+                    ]
+                )
+            )
+        else:
+            proxy_calls.append(
+                call(
+                    [
+                        "ubuntu-advantage",
+                        "config",
+                        "unset",
+                        "http_proxy",
+                    ]
+                )
+            )
+
+        if self.env["https_proxy"]:
+            proxy_calls.append(
+                call(
+                    [
+                        "ubuntu-advantage",
+                        "config",
+                        "set",
+                        "https_proxy={}".format(self.env["https_proxy"]),
+                    ]
+                )
+            )
+        else:
+            proxy_calls.append(
+                call(
+                    [
+                        "ubuntu-advantage",
+                        "config",
+                        "unset",
+                        "https_proxy",
+                    ]
+                )
+            )
+
         return call_list + proxy_calls if append else proxy_calls + call_list
 
     def _assert_apt_calls(self):