Merge ~rmalz/charm-advanced-routing:rmalz/modify_networkd_conf into charm-advanced-routing:master

[Robert Malz]

configure networkd during setup

Systemd in some scenarios can remove any rules defined outside
To fix that ManageForeignRouting* configuraton has been
Set networkd configuration during charm setup to make rules persistant.

Close LP #2034029

Currently advanced-routing charm assumes that networkd-dispatcher will always trigger /etc/networkd-dispatcher/routable.d/95-juju_routing script on configuration change which is currently not true for all cases.

networkd-dispatcher awaits for a operational state routable to execute above script, however systemd will not trigger such signal if all interfaces are configured with static ip addresses.

To reproduce problem configure all interfaces in unit to not use dhcp settings.
If at least one interface is configured with dhcp, systemd will generate signal for it and routable.d will be called, thus reproduction will not happen.
Once configured reload networkd service or perform networkctl reload

After changes in systemd [1], any foreign rules will be removed. To workaround this following config has been added [2]
This behavior has been confirmed with https://bugs.launchpad.net/charm-advanced-routing/+bug/2034029

1- https://github.com/systemd/systemd/pull/17477/commits/0b81225e5791f660506f7db0ab88078cf296b771
2- https://github.com/systemd/systemd/pull/19287

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

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

[Robert Malz (rmalz)]

Different approach for handle this scenario would be move dependencies from networkd-dispatcher to .network configuration of systemd.
These settings are applied once service is reloaded even if interface is configured with static IPs.
This approach is a lot more riskier

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

FAILED: Continuous integration, rev:861e5d37c66e020c25621daef71fbdc5ec6f2a9e
https://jenkins.canonical.com/bootstack/job/lp-charm-advanced-routing-ci/12/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest31/360/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/721/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-advanced-routing-ci/12//rebuild

review: Needs Fixing (continuous-integration)

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

FAILED: Continuous integration, rev:86fed4b716b7e94a5b462898b83b9b48a766e1db
https://jenkins.canonical.com/bootstack/job/lp-charm-advanced-routing-ci/13/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest31/361/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/722/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-advanced-routing-ci/13//rebuild

review: Needs Fixing (continuous-integration)

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

FAILED: Continuous integration, rev:800a7fc9c802abd87128362990e593ad8a2e2270
https://jenkins.canonical.com/bootstack/job/lp-charm-advanced-routing-ci/14/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest31/362/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/723/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-advanced-routing-ci/14//rebuild

review: Needs Fixing (continuous-integration)

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

PASSED: Continuous integration, rev:723e15264e95494cfb5923d7a7fd979db62b1f5b
https://jenkins.canonical.com/bootstack/job/lp-charm-advanced-routing-ci/15/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest31/369/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/730/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-advanced-routing-ci/15//rebuild

review: Approve (continuous-integration)

[Edward Hope-Morley (hopem)]

@rmalz a couple of comments:

 * can you add a unit test for this change please?

 * rather than modifying /etc/systemd/networkd.conf it would be more inline with how charms work to use a custom override in /usr/lib/systemd/network/ or perhaps /etc/systemd/network [1] e.g. /usr/lib/systemd/network/99-juju-advanced-routing.network

[1] https://wiki.archlinux.org/title/systemd-networkd#Configuration_files

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

PASSED: Continuous integration, rev:08ebc7571c3eef6768adcbd171654ae502a23261
https://jenkins.canonical.com/bootstack/job/lp-charm-advanced-routing-ci/16/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest31/425/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/745/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-advanced-routing-ci/16//rebuild

review: Approve (continuous-integration)

[Robert Malz (rmalz)]

Hey Ed,
Following proposal I have created new file based on [1].
Script will create a new file in /usr/lib/systemd/networkd.conf.d/juju-networkd.conf" and always overwrite it with:
root@juju-946413-myostmodel-21:/home/ubuntu# cat /usr/lib/systemd/networkd.conf.d/juju-networkd.conf
# This file is managed by Juju.
[Network]
ManageForeignRoutingPolicyRules=no
ManageForeignRoutes=no

I have tested this locally, works fine.

[1] https://manpages.ubuntu.com/manpages/jammy/man5/networkd.conf.5.html

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

PASSED: Continuous integration, rev:b67ea264b2880a821d0aefb9492d4a1f42b5628a
https://jenkins.canonical.com/bootstack/job/lp-charm-advanced-routing-ci/17/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest31/427/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/747/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-advanced-routing-ci/17//rebuild

review: Approve (continuous-integration)

[Andrea Ieri (aieri)]

Looks good overall, there are just some typos

review: Needs Fixing

[Robert Malz (rmalz)]

Comments addressed

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

PASSED: Continuous integration, rev:611bd22d011ece2fd343d08dd3e4ef3c5327f32f
https://jenkins.canonical.com/bootstack/job/lp-charm-advanced-routing-ci/18/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest31/430/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/750/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-advanced-routing-ci/18//rebuild

review: Approve (continuous-integration)

[Andrea Ieri (aieri)]

@rmalz somehow I don't see the updates. 611bd22 seems identical to b67ea26

[Robert Malz (rmalz)]

> @rmalz somehow I don't see the updates. 611bd22 seems identical to b67ea26

Interesting, re-pushed the changes rn.

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

FAILED: Continuous integration, rev:d0b05b63ad7e8cd3e06ec2c62a54a675b3c0feba
https://jenkins.canonical.com/bootstack/job/lp-charm-advanced-routing-ci/19/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest31/431/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/751/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-advanced-routing-ci/19//rebuild

review: Needs Fixing (continuous-integration)

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

PASSED: Continuous integration, rev:20c53a26e87b460f7b033ff77f8e7df790d6f159
https://jenkins.canonical.com/bootstack/job/lp-charm-advanced-routing-ci/20/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest31/432/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/752/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-advanced-routing-ci/20//rebuild

review: Approve (continuous-integration)

[Andrea Ieri (aieri)]

lgtm

review: Approve

[Robert Gildein (rgildein)]

LGTM

review: Approve

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

Change successfully merged at revision 28162e0fb93b07f3894b00ad40d2093c12bbe234

diff --git a/src/lib/advanced_routing_helper.py b/src/lib/advanced_routing_helper.py
index 636729b..afa06db 100644
--- a/src/lib/advanced_routing_helper.py
+++ b/src/lib/advanced_routing_helper.py
@@ -27,6 +27,9 @@ class AdvancedRoutingHelper:
     netplan_up_dir_path = pathlib.Path("/etc/networkd-dispatcher/routable.d")
     policy_routing_service_dir_path = pathlib.Path("/etc/systemd/system")
     table_name_path = pathlib.Path("/etc/iproute2/rt_tables.d/juju-managed.conf")
+    networkd_conf_path = pathlib.Path(
+        "/usr/lib/systemd/networkd.conf.d/95-juju-networkd.conf"
+    )
 
     def __init__(self):
         """Init function."""
@@ -105,6 +108,7 @@ class AdvancedRoutingHelper:
             cleanup_file.write("ip route flush cache\n")
         os.chmod(str(self.common_cleanup_path), 0o755)
 
+        self.setup_persistent_rules()
         self.post_setup()
 
     def apply_config(self):
@@ -151,6 +155,22 @@ class AdvancedRoutingHelper:
             else:
                 raise e
 
+    def setup_persistent_rules(self):
+        """Modify systemd config to not delete foreign rules."""
+        hookenv.log("Changing networkd configuration", level=hookenv.INFO)
+        conf_parent = self.networkd_conf_path.parent
+        if not conf_parent.exists():
+            conf_parent.mkdir(parents=True)
+            hookenv.log("Created {}".format(conf_parent), level=hookenv.INFO)
+        config = (
+            "# This file is managed by Juju.\n"
+            "[Network]\n"
+            "ManageForeignRoutingPolicyRules=no\n"
+            "ManageForeignRoutes=no\n"
+        )
+        with open(str(self.networkd_conf_path), "w") as config_file:
+            config_file.write(config)
+
     @property
     def etc_ifup_path(self):
         """Return path to ifup etc folder based on series."""
diff --git a/src/tests/unit/test_AdvancedRoutingHelper.py b/src/tests/unit/test_AdvancedRoutingHelper.py
index 4dbf93f..52550c0 100644
--- a/src/tests/unit/test_AdvancedRoutingHelper.py
+++ b/src/tests/unit/test_AdvancedRoutingHelper.py
@@ -11,9 +11,10 @@ class TestAdvancedRoutingHelper:
     """Main test class."""
 
     test_dir = pathlib.Path("/tmp/test/charm-advanced-routing")
-    test_ifup_path = test_dir / "symlink_test" / "ifup"
+    test_networkd_conf_path = test_dir / "networkd.conf.d" / "juju-networkd.conf"
     test_netplanup_path = test_dir / "symlink_test" / "netplanup"
     test_cleanup_path = test_dir / "symlink_test" / "netplandown"
+    test_ifup_path = test_dir / "symlink_test" / "ifup"
     test_script = "test-script"
 
     @classmethod
@@ -64,6 +65,7 @@ class TestAdvancedRoutingHelper:
         test_obj.routing_script_name = self.test_script
         test_obj.common_ifup_path = self.test_dir / "if-up" / self.test_script
         test_obj.common_cleanup_path = self.test_dir / "cleanup" / self.test_script
+        test_obj.networkd_conf_path = self.test_networkd_conf_path
 
         test_obj.post_setup = noop
         routing_validator.RoutingConfigValidator.__init__ = mock.Mock(return_value=None)
@@ -119,3 +121,12 @@ class TestAdvancedRoutingHelper:
         # link it again
         test_obj.symlink_force(target, link)
         assert link.exists()
+
+    def test_setup_persistent_rules(self, advanced_routing_helper):
+        """Test setup_persistent_rules."""
+        test_obj = advanced_routing_helper
+        test_obj.networkd_conf_path = self.test_networkd_conf_path
+
+        test_obj.setup_persistent_rules()
+
+        assert test_obj.networkd_conf_path.exists()
diff --git a/src/tests/unit/test_actions.py b/src/tests/unit/test_actions.py
index e07f0e3..8e39959 100644
--- a/src/tests/unit/test_actions.py
+++ b/src/tests/unit/test_actions.py
@@ -9,6 +9,7 @@ class TestActions:
     """Test suite for actions."""
 
     test_dir = pathlib.Path("/tmp/test/charm-advanced-routing")
+    test_networkd_conf_path = test_dir / "networkd.conf.d" / "juju-networkd.conf"
     test_script = "test-script"
 
     def test_action_apply_changes_apply_config(self, advanced_routing_helper):
@@ -23,6 +24,7 @@ class TestActions:
         test_obj.routing_script_name = self.test_script
         test_obj.common_ifup_path = self.test_dir / "if-up" / self.test_script
         test_obj.common_cleanup_path = self.test_dir / "cleanup" / self.test_script
+        test_obj.networkd_conf_path = self.test_networkd_conf_path
 
         test_obj.post_setup = noop
         routing_validator.RoutingConfigValidator.__init__ = mock.Mock(return_value=None)