Merge lp:~simpoir/landscape-charm/1682105_revisit_config into lp:~landscape/landscape-charm/trunk

[Simon Poirier]

Make proxy, system-email and root-url configurations updatable through the charm config.

Adds a few configuration to the charm.
Those were previously only updatable from the UI (proxy, system-email, root-url) or on bootstrap.

Requires changes which are fix-commited to landscape/trunk, for it to do anything.

Testing:

I have repackaged 19.10 with the trunk patch to actually be able to test this branch.
Trying to change those conf with the current 19.10 ppa will do nothing (the branch checks if the flag exists)

download https://pastebin.canonical.com/p/hk84D8kqtg/ as bundle.yml
juju deploy ./bundle.yml
juju config landscape-server system-email=noreply@localhost

log to the ui, check the settings tab.

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-test
Result: Fail
Revno: 404
Branch: lp:~simpoir/landscape-charm/1682105_revisit_config
Jenkins: https://jenkins.canonical.com/landscape/job/latch-test-xenial/1476/

review: Needs Fixing (test results)

[lp:~simpoir/landscape-charm/1682105_revisit_config]

405. By Simon Poirier on 2021-12-03

lint

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-test
Result: Success
Revno: 405
Branch: lp:~simpoir/landscape-charm/1682105_revisit_config
Jenkins: https://jenkins.canonical.com/landscape/job/latch-test-xenial/1477/

review: Approve (test results)

[Kevin Nasto (silverdrake11)]

LGTM except for the typo in the yaml

review: Approve

[lp:~simpoir/landscape-charm/1682105_revisit_config]

406. By Simon Poirier on 2021-12-08

typo

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-test
Result: Success
Revno: 406
Branch: lp:~simpoir/landscape-charm/1682105_revisit_config
Jenkins: https://jenkins.canonical.com/landscape/job/latch-test-xenial/1486/

review: Approve (test results)

=== modified file 'config.yaml'
--- config.yaml	2021-11-09 09:20:31 +0000
+++ config.yaml	2021-12-08 22:56:37 +0000
@@ -99,6 +99,11 @@
             IP of the first related HAproxy unit will be used instead.
         type: string
         default: ""
+    system-email:
+        type: string
+        description: |
+            The address emails from Landscape will appear to come from.
+        default: ""
     ssl-cert:
         type: string
         description: |
@@ -117,6 +122,24 @@
         description: |
             The SMTP server to use to deliver outgoing mail.
         default: ""
+    http-proxy:
+        default:
+        type: string
+        description: |
+            The http proxy URL Landscape will use. If left blank, the
+            model-config value will be used.
+    https-proxy:
+        default:
+        type: string
+        description: |
+            The https proxy Landscape will use. If left blank, the model-config
+            value will be used.
+    no-proxy:
+        default:
+        type: string
+        description: |
+            Comma separated list of hosts for which no proxy should be used.
+            If left blank, the model-config value will be used.
     site-name:
         type: string
         default: ''

=== modified file 'lib/callbacks/scripts.py'
--- lib/callbacks/scripts.py	2019-07-10 19:02:28 +0000
+++ lib/callbacks/scripts.py	2021-12-08 22:56:37 +0000
@@ -1,4 +1,3 @@
-import os
 import subprocess
 
 from charmhelpers.core import hookenv
@@ -14,12 +13,15 @@
 # Database relation keys for which, in case of change, a restart is not needed.
 NO_RESTART_DB_RELATION_KEYS = {"allowed-units"}
 
+CONFIG_ONLY_FLAG = "--configure-lds-only"
+
 
 class ScriptCallback(ManagerCallback):
     """Callback class for invoking Landscape scripts."""
 
-    def __init__(self, subprocess=subprocess):
+    def __init__(self, subprocess=subprocess, hookenv=hookenv):
         self._subprocess = subprocess
+        self._hookenv = hookenv
 
     def _run(self, name, options=()):
         """Run the script with the given name and options."""
@@ -34,12 +36,21 @@
     This will invoke the schema script with the --bootstrap flag, if it hasn't
     been called yet.
     """
+
     def __call__(self, manager, service_name, event_name):
+        self._schema_doc = self._subprocess.check_output([SCHEMA_SCRIPT, "-h"])
         if not manager.was_ready(service_name):
             options = ["--bootstrap"]
             options.extend(self._get_proxy_options())
             self._run(SCHEMA_SCRIPT, options)
 
+        if CONFIG_ONLY_FLAG in self._schema_doc:
+            options = [CONFIG_ONLY_FLAG]
+            options.extend(self._get_proxy_options())
+            options.extend(self._get_settings_options())
+            if len(options) > 1:
+                self._run(SCHEMA_SCRIPT, options)
+
     def _get_proxy_options(self):
         """Return the HTTP proxy options to set.
 
@@ -48,15 +59,39 @@
         at the environment variables that Juju sets for us.
         """
         options = []
+        config = self._hookenv.config()
 
-        help_output = self._subprocess.check_output([SCHEMA_SCRIPT, "-h"])
-        if "--with-http-proxy" in help_output:
+        if "--with-http-proxy" in self._schema_doc:
             # Forward any proxy configuration set in the environment
+            model_proxy = self._hookenv.env_proxy_settings() or {}
             for proxy_variable in ("http_proxy", "https_proxy", "no_proxy"):
-                if proxy_variable in os.environ:
-                    options.append("--with-%s=%s" % (
-                        proxy_variable.replace("_", "-"),
-                        os.environ[proxy_variable]))
+                model_proxy_value = model_proxy.get(proxy_variable, "")
+
+                # XXX There is no charm hook for model-config changes,
+                # so updating proxies has to be done using charm config.
+                # See juju issue (LP: #1835050).
+                proxy_variable = proxy_variable.replace("_", "-")
+                proxy_value = config.get(proxy_variable)
+                if proxy_value is None:
+                    proxy_value = model_proxy_value
+                options.append("--with-{}".format(proxy_variable))
+                options.append(proxy_value)
+
+        return options
+
+    def _get_settings_options(self):
+        """Return options for updating charm-managed settings."""
+        config = self._hookenv.config()
+        options = []
+        root_url = config.get("root-url")
+        if root_url:
+            options.append("--with-root-url")
+            options.append(root_url)
+
+        system_email = config.get("system-email")
+        if system_email:
+            options.append("--with-system-email")
+            options.append(system_email)
 
         return options
 
@@ -64,10 +99,6 @@
 class LSCtl(ScriptCallback):
     """Call the lsctl script to start or stop services."""
 
-    def __init__(self, subprocess=subprocess, hookenv=hookenv):
-        super(LSCtl, self).__init__(subprocess=subprocess)
-        self._hookenv = hookenv
-
     def __call__(self, manager, service_name, event_name):
         current_status, current_status_message = self._hookenv.status_get()
         action_status_message = ""

=== modified file 'lib/callbacks/tests/test_scripts.py'
--- lib/callbacks/tests/test_scripts.py	2019-07-15 20:01:06 +0000
+++ lib/callbacks/tests/test_scripts.py	2021-12-08 22:56:37 +0000
@@ -3,7 +3,7 @@
 from charmhelpers.core.services.base import ServiceManager
 
 from lib.paths import LSCTL, SCHEMA_SCRIPT
-from lib.callbacks.scripts import SchemaBootstrap, LSCtl
+from lib.callbacks.scripts import SchemaBootstrap, LSCtl, CONFIG_ONLY_FLAG
 from lib.utils import update_persisted_data
 from lib.tests.helpers import HookenvTest
 from lib.tests.stubs import SubprocessStub
@@ -19,7 +19,8 @@
         self.subprocess = SubprocessStub()
         self.subprocess.add_fake_executable(SCHEMA_SCRIPT)
         self.manager = ServiceManager(services=[{"service": "landscape"}])
-        self.callback = SchemaBootstrap(subprocess=self.subprocess)
+        self.callback = SchemaBootstrap(
+            subprocess=self.subprocess, hookenv=self.hookenv)
 
     def test_options(self):
         """
@@ -56,18 +57,154 @@
         self.callback(self.manager, "landscape", None)
         self.assertEqual(
             ["/usr/bin/landscape-schema", "--bootstrap",
-             "--with-http-proxy=http://foo:3128",
-             "--with-https-proxy=http://bar:3128",
-             "--with-no-proxy=localhost"],
+             "--with-http-proxy", "http://foo:3128",
+             "--with-https-proxy", "http://bar:3128",
+             "--with-no-proxy", "localhost"],
             self.subprocess.calls[1][0])
 
     def test_was_ready(self):
         """
-        If the services was ready, the schema script is not invoked again.
+        If the services was ready, the schema bootstrap is not invoked again.
         """
         self.manager.save_ready("landscape")
         self.callback(self.manager, "landscape", None)
-        self.assertEqual([], self.subprocess.calls)
+        self.assertEqual([
+            (['/usr/bin/landscape-schema', '-h'], {})], self.subprocess.calls)
+
+    def test_update_config(self):
+        """Updating config are updated in Landscape."""
+        self.subprocess.add_fake_executable(
+            SCHEMA_SCRIPT, args=["-h"], stdout="Usage: " + CONFIG_ONLY_FLAG)
+
+        self.hookenv.hook = "config-changed"
+        config = self.hookenv.config()
+        config["root-url"] = "https://old.sad/"
+        config.save()
+        self.manager.save_ready("landscape")
+
+        config["root-url"] = "https://happy.new/"
+        self.callback(self.manager, "landscape", None)
+        self.assertEqual([
+            (["/usr/bin/landscape-schema", "-h"], {}),
+            (["/usr/bin/landscape-schema", CONFIG_ONLY_FLAG, "--with-root-url",
+              "https://happy.new/"], {}),
+        ], self.subprocess.calls)
+
+    def test_update_config_unsupported_flag(self):
+        """Configuration is NOOP if the configure-lds flag is not supported."""
+        self.subprocess.add_fake_executable(
+            SCHEMA_SCRIPT, args=["-h"], stdout="Usage: --spam")
+
+        self.hookenv.hook = "config-changed"
+        config = self.hookenv.config()
+        config["root-url"] = "https://old.sad/"
+        config.save()
+        self.manager.save_ready("landscape")
+        config["root-url"] = "https://happy.new/"
+
+        self.callback(self.manager, "landscape", None)
+        self.assertEqual([
+            (["/usr/bin/landscape-schema", "-h"], {}),
+        ], self.subprocess.calls)
+
+    def test_bootstrap_and_configure(self):
+        """Configuration is done after bootstrap, if config values exist."""
+        self.subprocess.add_fake_executable(
+            SCHEMA_SCRIPT, args=["-h"], stdout="Usage: " + CONFIG_ONLY_FLAG)
+
+        self.hookenv.hook = "config-changed"
+        config = self.hookenv.config()
+        config["system-email"] = "noreply@spam"
+        config.save()
+        config["system-email"] = "noreply@scape"
+
+        self.callback(self.manager, "landscape", None)
+        self.assertEqual([
+            (["/usr/bin/landscape-schema", "-h"], {}),
+            (["/usr/bin/landscape-schema", "--bootstrap"], {}),
+            (["/usr/bin/landscape-schema", CONFIG_ONLY_FLAG,
+              "--with-system-email", "noreply@scape"], {}),
+        ], self.subprocess.calls)
+
+    def test_inherit_model_proxy(self):
+        """Proxy config is inherited from the model if unset on charm."""
+        self.subprocess.add_fake_executable(
+            SCHEMA_SCRIPT, args=["-h"],
+            stdout="Usage: --with-http-proxy " + CONFIG_ONLY_FLAG)
+        self.hookenv.hook = "config-changed"
+        config = self.hookenv.config()
+        config["http-proxy"] = "spam"
+        config["https-proxy"] = "spam"
+        config["no-proxy"] = "spam"
+        config.save()
+        del config["http-proxy"]
+        del config["https-proxy"]
+        del config["no-proxy"]
+        self.useFixture(EnvironmentVariable("http_proxy", "http://foo:3128"))
+        self.useFixture(EnvironmentVariable("https_proxy", "http://bar:3128"))
+        self.useFixture(EnvironmentVariable("no_proxy", "localhost"))
+        self.manager.save_ready("landscape")
+
+        self.callback(self.manager, "landscape", None)
+        self.assertEqual([
+            (["/usr/bin/landscape-schema", "-h"], {}),
+            (["/usr/bin/landscape-schema", CONFIG_ONLY_FLAG,
+              "--with-http-proxy", "http://foo:3128",
+              "--with-https-proxy", "http://bar:3128",
+              "--with-no-proxy", "localhost"], {}),
+        ], self.subprocess.calls)
+
+    def test_change_model_proxy(self):
+        """Proxy config override model proxy if configured on charm."""
+        self.subprocess.add_fake_executable(
+            SCHEMA_SCRIPT, args=["-h"],
+            stdout="Usage: --with-http-proxy " + CONFIG_ONLY_FLAG)
+        self.hookenv.hook = "config-changed"
+        config = self.hookenv.config()
+        self.useFixture(EnvironmentVariable("http_proxy", "spam"))
+        self.useFixture(EnvironmentVariable("https_proxy", "spam"))
+        self.useFixture(EnvironmentVariable("no_proxy", "spam"))
+        self.manager.save_ready("landscape")
+        config["http-proxy"] = "http://foo:3128"
+        config["https-proxy"] = "http://bar:3128"
+        config["no-proxy"] = "localhost"
+
+        self.callback(self.manager, "landscape", None)
+        self.assertEqual([
+            (["/usr/bin/landscape-schema", "-h"], {}),
+            (["/usr/bin/landscape-schema", CONFIG_ONLY_FLAG,
+              "--with-http-proxy", "http://foo:3128",
+              "--with-https-proxy", "http://bar:3128",
+              "--with-no-proxy", "localhost"], {}),
+        ], self.subprocess.calls)
+
+    def test_unset_model_proxy(self):
+        """Proxy config can explicitly unset model proxy."""
+        self.subprocess.add_fake_executable(
+            SCHEMA_SCRIPT, args=["-h"],
+            stdout="Usage: --with-http-proxy " + CONFIG_ONLY_FLAG)
+        self.hookenv.hook = "config-changed"
+        config = self.hookenv.config()
+        self.useFixture(EnvironmentVariable("http_proxy", "spam"))
+        self.useFixture(EnvironmentVariable("https_proxy", "spam"))
+        self.useFixture(EnvironmentVariable("no_proxy", "spam"))
+        self.manager.save_ready("landscape")
+        config["http-proxy"] = ""
+        config["https-proxy"] = ""
+        config["no-proxy"] = ""
+
+        self.callback(self.manager, "landscape", None)
+        self.assertEqual([
+            (["/usr/bin/landscape-schema", "-h"], {}),
+            (["/usr/bin/landscape-schema", CONFIG_ONLY_FLAG,
+              "--with-http-proxy", "",
+              "--with-https-proxy", "",
+              "--with-no-proxy", ""], {}),
+        ], self.subprocess.calls)
+
+    def test_reconfigure_noop(self):
+        """Nothing happens if there is no proxy and no config change."""
+        # TODO
 
 
 class LSCtlTest(HookenvTest):

=== modified file 'lib/services.py'
--- lib/services.py	2021-11-09 09:20:31 +0000
+++ lib/services.py	2021-12-08 22:56:37 +0000
@@ -86,7 +86,8 @@
                     subprocess=self._subprocess),
                 EnsureConfigDir(paths=self._paths),
                 WriteCustomSSLCertificate(paths=self._paths),
-                SchemaBootstrap(subprocess=self._subprocess),
+                SchemaBootstrap(
+                    subprocess=self._subprocess, hookenv=self._hookenv),
                 WriteLicenseFile(host=self._host, paths=self._paths),
                 ConfigureSMTP(
                     hookenv=self._hookenv, subprocess=self._subprocess),
@@ -95,4 +96,8 @@
             ],
             "start": LSCtl(subprocess=self._subprocess, hookenv=self._hookenv),
         }])
-        manager.manage()
+        try:
+            manager.manage()
+        except Exception as e:
+            self._hookenv.log(str(e), level=hookenv.ERROR)
+            raise

=== modified file 'lib/tests/stubs.py'
--- lib/tests/stubs.py	2021-11-10 05:34:36 +0000
+++ lib/tests/stubs.py	2021-12-08 22:56:37 +0000
@@ -1,3 +1,4 @@
+import os
 import subprocess
 
 from charmhelpers.core.hookenv import Config
@@ -33,6 +34,13 @@
     def config(self):
         return self._config
 
+    def env_proxy_settings(self):
+        return {
+            k: os.environ[k]
+            for k in ("http_proxy", "https_proxy", "no_proxy")
+            if k in os.environ
+        } or None
+
     def relations_of_type(self, reltype):
         return self.relations.get(reltype, None)