Merge ~cjwatson/charm-postfix-relay:sender-dependent-relayhost-maps into charm-postfix-relay:master

[Colin Watson]

Add sender_dependent_relayhost_maps option

I'd like to use this charm in Launchpad deployments. We use sender-dependent relay hosts on at least some Launchpad machines in order to ensure that only messages with expected envelope senders are sent out to our authenticated relay, and this will let us reproduce the necessary configuration. I imagine it might have other use cases as well.

(Resubmitting without the previous prerequisite, since that's now landed; and I believe I've fixed the issues previously raised in review. Please re-review.)

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

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

[Tianqi Xiao (txiao)]

When adding new functions, I suggest creating corresponding unit tests for the best practice.

review: Needs Fixing

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

FAILED: Continuous integration, rev:f7de4100c30ea9eca7891189dcaa47f1a801c8c1
https://jenkins.canonical.com/bootstack/job/lp-charm-postfix-relay-ci/6/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1095/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/1511/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-postfix-relay-ci/6//rebuild

review: Needs Fixing (continuous-integration)

[Erhan Sunar (esunar)]

It is failing on bionic tests, because it is unable to install on Bionic. Possibly https://github.com/juju/charm-tools/issues/638 Can you fix it?

review: Needs Fixing

[Robert Gildein (rgildein)]

I agree with Tianqi, you should add unit tests for these changes.

IMO, bionic failing tests should be handled in separated MP.

review: Needs Fixing

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

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

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:634c68573b80b62c34413f9151e4e7f6365d8a7f
https://jenkins.canonical.com/bootstack/job/lp-charm-postfix-relay-ci/1/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/271/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/99/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-postfix-relay-ci/1//rebuild

review: Approve (continuous-integration)

[Robert Gildein (rgildein)]

review: Approve

[Tianqi Xiao (txiao)]

LGTM. Thanks for the MP.

review: Approve

[Erhan Sunar (esunar)]

LGTM

review: Approve

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

Change successfully merged at revision b237249f9853b98eb83bbd0fa3476c299ff37ad5

diff --git a/src/config.yaml b/src/config.yaml
index a28cf40..b9aea05 100644
--- a/src/config.yaml
+++ b/src/config.yaml
@@ -31,6 +31,13 @@ options:
             An upstream mail relay, sometimes called "smart host".  Enclose it
             in square brackets to skip MX lookup.
         type: string
+    sender_dependent_relayhost_maps:
+        default: ''
+        description:
+            A YAML-encoded list of (sender, relay host) pairs.  If set,
+            messages from each given sender will be sent via the given relay
+            host.
+        type: string
     smtpd_helo_required:
         default: True
         description:
diff --git a/src/lib/charm/postfix/postfix_relay.py b/src/lib/charm/postfix/postfix_relay.py
index 1cbd45b..2ec90f5 100644
--- a/src/lib/charm/postfix/postfix_relay.py
+++ b/src/lib/charm/postfix/postfix_relay.py
@@ -25,6 +25,7 @@ from jinja2 import Template
 TEMPLATES = "templates/"
 MAIN_CFG = "/etc/postfix/main.cf"
 AUTH_CONFIG = "/etc/postfix/sasl_passwd"
+RELAYHOST_MAP = "/etc/postfix/relayhost_map"
 POSTFIX_CERTIFICATE_DIR = "/etc/postfix/"
 
 
@@ -41,6 +42,8 @@ def write_configs():
         # create domain rewriting rules
         filename = "/etc/postfix/smtp_generic_maps.pcre"
         create_rewrite_map(config("domain_rewrite_map"), filename)
+    if config("sender_dependent_relayhost_maps"):
+        write_relayhost_map()
     restart_postfix()
 
 
@@ -75,6 +78,15 @@ def create_rewrite_map(rewrite, filename):
     f.close()
 
 
+def write_relayhost_map():
+    """Render a relayhost map."""
+    template_path = os.path.join(TEMPLATES, "relayhost_map")
+    with open(template_path) as template_file, open(RELAYHOST_MAP, "w+") as file:
+        template = Template(template_file.read())
+        file.write(template.render(PostfixContext()()))
+    subprocess.check_call(["postmap", "hash:{}".format(RELAYHOST_MAP)])
+
+
 def restart_postfix():
     """Restart postfix."""
     service_restart("postfix")
diff --git a/src/templates/main.cf b/src/templates/main.cf
index a1df893..966fec4 100644
--- a/src/templates/main.cf
+++ b/src/templates/main.cf
@@ -29,6 +29,9 @@ myhostname = {{ config.myhostname }}
 mynetworks = {{ config.mynetworks }}
 mydestination = $myhostname, localhost.$mydomain, localhost
 relayhost = {{ config.relayhost }}
+{%- if sender_dependent_relayhost_maps %}
+sender_dependent_relayhost_maps = hash:/etc/postfix/relayhost_map
+{%- endif %}
 smtpd_helo_required = {{ smtpd_helo_required }}
 smtpd_recipient_restrictions = {{ config.smtpd_recipient_restrictions }}
 smtpd_relay_restrictions = permit_mynetworks permit_sasl_authenticated defer_unauth_destination
diff --git a/src/templates/relayhost_map b/src/templates/relayhost_map
new file mode 100644
index 0000000..6d77537
--- /dev/null
+++ b/src/templates/relayhost_map
@@ -0,0 +1,3 @@
+{% for sender, relayhost in config.sender_dependent_relayhost_maps -%}
+{{ sender }} {{ relayhost }}
+{% endfor -%}
diff --git a/src/tests/unit/requirements.txt b/src/tests/unit/requirements.txt
index e69de29..2bcb017 100644
--- a/src/tests/unit/requirements.txt
+++ b/src/tests/unit/requirements.txt
@@ -0,0 +1 @@
+systemfixtures
diff --git a/src/tests/unit/test_example.py b/src/tests/unit/test_example.py
deleted file mode 100644
index 5cbe63d..0000000
--- a/src/tests/unit/test_example.py
+++ /dev/null
@@ -1,3 +0,0 @@
-def test_example():
-    """This test is to avoid collected 0 items in pytest."""
-    assert True
diff --git a/src/tests/unit/test_postfix_relay.py b/src/tests/unit/test_postfix_relay.py
new file mode 100644
index 0000000..74cf14c
--- /dev/null
+++ b/src/tests/unit/test_postfix_relay.py
@@ -0,0 +1,69 @@
+"""Unit tests for charm.postfix.postfix_relay."""
+
+import os
+import subprocess
+from unittest import TestCase, mock
+
+from systemfixtures import FakeFilesystem, FakeProcesses
+
+from charm.postfix import postfix_relay
+
+
+class WriteRelayhostMapTests(TestCase):
+    @mock.patch("charm.postfix.postfix_relay.config")
+    def test_single_sender(self, mock_config):
+        mock_config.return_value = {
+            "sender_dependent_relayhost_maps": [
+                ["[e-mail]", "relay.example.com"]
+            ]
+        }
+        with FakeFilesystem() as fs, FakeProcesses() as processes:
+            fs.add("/etc")
+            os.makedirs("/etc/postfix")
+            processes.add(lambda _: {}, name="postmap")
+            postfix_relay.write_relayhost_map()
+            with open("/etc/postfix/relayhost_map") as rmap:
+                self.assertEqual("[e-mail] relay.example.com\n", rmap.read())
+            self.assertEqual(
+                [["postmap", "hash:/etc/postfix/relayhost_map"]],
+                [proc._args["args"] for proc in processes.procs],
+            )
+
+    @mock.patch("charm.postfix.postfix_relay.config")
+    def test_multiple_senders(self, mock_config):
+        mock_config.return_value = {
+            "sender_dependent_relayhost_maps": [
+                ["[e-mail]", "relay1.example.com"],
+                ["[e-mail]", "relay2.example.com"],
+            ]
+        }
+        with FakeFilesystem() as fs, FakeProcesses() as processes:
+            fs.add("/etc")
+            os.makedirs("/etc/postfix")
+            processes.add(lambda _: {}, name="postmap")
+            postfix_relay.write_relayhost_map()
+            with open("/etc/postfix/relayhost_map") as rmap:
+                self.assertEqual(
+                    "[e-mail] relay1.example.com\n"
+                    "[e-mail] relay2.example.com\n",
+                    rmap.read(),
+                )
+            self.assertEqual(
+                [["postmap", "hash:/etc/postfix/relayhost_map"]],
+                [proc._args["args"] for proc in processes.procs],
+            )
+
+    @mock.patch("charm.postfix.postfix_relay.config")
+    def test_postmap_fails(self, mock_config):
+        mock_config.return_value = {
+            "sender_dependent_relayhost_maps": [
+                ["[e-mail]", "relay.example.com"]
+            ]
+        }
+        with FakeFilesystem() as fs, FakeProcesses() as processes:
+            fs.add("/etc")
+            os.makedirs("/etc/postfix")
+            processes.add(lambda _: {"returncode": 1}, name="postmap")
+            self.assertRaises(
+                subprocess.CalledProcessError, postfix_relay.write_relayhost_map
+            )