[enhancement]: simplify systemd unit files

[holmanb]

Enhancement

Cloud-init's systemd service templates have grown in complexity over time. Changes to unit files have a broad impact, so to limit risk to other distros templates are sometimes used when making changes. Over time these templates have grown very complex, which has made them even more difficult to maintain and increased the risk of changes breaking one distro or another[1]. Some changes were made in the past to simplify these service files[2][3], but much complexity remains.

This complexity falls into four different categories:

Foregone spring cleaning

Is this is actually needed? Systemd loads the selinux security policy prior to running any cloud-init code (including the systemd generator), so won't the security policy already be the default one upon creation? If this actually is needed, it probably belongs somewhere besides this service file - either in cloud-init's code directly or elsewhere.

Unnecessary templating

Systemd allows ordering and dependency on units to behave correctly when referenced units do not exist. Ordering After=wicked.service will not harm distros which do not use Wicked[5]. Ordering After=networking.service will not harm those distros not using ifupdown.

The dbus issue

Cloud-init can update hostname very early in boot. Some distro implementations in cloud-init[4] implement this with hostnamectl, which depends on D-bus. Those distros have to run all of cloud-init's services much later in boot after D-bus is available.

I see two ways to resolve this divergent behavior:

1. implement _write_hostname() without hostnamectl (some distros do it already)
2. wait for hostnamctl to become Varlink-aware since existing systemd APIs will slowly acquire Varlink interfaces (eta: unknown)

How to contribute

If you are a distro maintainer, your participation and help with this issue would be greatly appreciated. A comment if you have anything to add (or disagree with any of the above), or even just a thumbs up to indicate that you think the approach seems reasonable would go a long ways.

Appendix: A

[1] #5755
[2] ec7dde8
[3] #5620
[4] RHEL, arch, opensuse, aosc, and photon, as well as distros derived from these in cloud-init's code
[5] note that the current suse dbus ordering misses host renames that could happen during Local stage - this is actually a bug

Appendix: B (completed)

• network.service doesn't appear used anywhere, if it ever was. The only reference that I can find to network.service is a systemd commit from 2015 which fixed a typo from network.service to network.target.
• cloud-init-local.service is ordered before NetworkManager.service and before network-pre.target, yet NetworkManager.service is itself ordered after network-pre.target. In this example, cloud-init-local.service doesn't need to be ordered before NetworkManager.service. Other redundant orderings like this exist.
• This looks like a requirement from before ds-identify was used. ds-identify creates this directory during generator timeframe. RHEL family distros use ds-identify, so I don't think that this is required anymore.
• This also looks like a carry-over from pre-ds-identify adoption. This file is normally generated by the systemd generator so I assume that this was to make sure that cloud-init reported the correct status, but since RHEL family distros use ds-identify now it shouldn't be necessary - this will always exist when ds-identify runs and cloud-init's systemd services starts.

[aciba90]

@holmanb: Was this completed as part of #5830? If not, what is left to do in this issue?

[holmanb]

@holmanb: Was this completed as part of #5830? If not, what is left to do in this issue?

Thanks for checking @aciba90, I just updated this to include the remaining items that need to happen.

[holmanb]

@Conan-Kudo Hi Neal. I'm working on cleaning up the templated service files. Could you please provide feedback on two parts of this:

1. An selinux question - see the section titled "foregone spring cleaning" above.
2. By default cloud-init sets hostname before dbus is available. The RHEL/SUSE family distros use hostnamctl to set the hostname in cloud-init, and therefore require a later systemd order. We could eliminate some templating from these files and allow cloud-init to start earlier if cloud-init didn't use hostnamectl directly. Do you have thoughts on this approach?

[Conan-Kudo]

1. It is possible that the policy needs to be changed at cloud-init time (particularly if you need to switch to the high-security MLS policy or a custom one).
2. Using hostnamectl ensures that transient (ie existing running stuff) and persistent (on disk stuff) are updated correctly. I would prefer we continue to use it.

[holmanb]

1. It is possible that the policy needs to be changed at cloud-init time (particularly if you need to switch to the high-security MLS policy or a custom one).

I think it would be easier to maintain if we could make use of cloud-init's distro-specific classes rather than in the upstream service file. When cloud-init starts, it calls Init._initialize_filesystem(), which creates some directories and asserts/changes file permissions. This call happens before any files are created or modified (except the log file). This seems like a logical place to set the security context. If that sounds reasonable, something like this should do it.

diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py
--- a/cloudinit/distros/__init__.py
+++ b/cloudinit/distros/__init__.py
@@ -1589,6 +1589,9 @@ class Distro(persistence.CloudInitPickleMixin, metaclass=abc.ABCMeta):
         that network connectivity is necessary in cloud-init's network stage.
         """
 
+    def initialize_filesystem(self) -> None:
+        """Prepares the filesystem at the start of each stage."""
+
 
 def _apply_hostname_transformations_to_url(url: str, transformations: list):
     """
diff --git a/cloudinit/distros/rhel.py b/cloudinit/distros/rhel.py
index 0e2141507..11afe66ce 100644
--- a/cloudinit/distros/rhel.py
+++ b/cloudinit/distros/rhel.py
@@ -215,3 +215,10 @@ class Distro(distros.Distro):
             ["makecache"],
             freq=PER_ALWAYS if force else PER_INSTANCE,
         )
+
+    def initialize_filesystem(self) -> None:
+        try:
+            subp.subp(["restorecon", "/run/cloud-init"])  # TODO: compare to SeLinuxGuard
+        except subp.ProcessExecutionError as e:
+            LOG.critical("Could not restore SELinux security context: %s", e)
+            raise e
diff --git a/cloudinit/stages.py b/cloudinit/stages.py
index 84bc0ce07..24694f303 100644
--- a/cloudinit/stages.py
+++ b/cloudinit/stages.py
@@ -239,6 +239,7 @@ class Init:
     def _initialize_filesystem(self):
         mode = 0o640
 
+        self.distro.initialize_filesystem()
         util.ensure_dirs(self._initial_subdirs())
         log_file = util.get_cfg_option_str(self.cfg, "def_log_file")
         if log_file:

This should enable changing security context at runtime while also dropping it from the service files. @Conan-Kudo any objections?

2. Using hostnamectl ensures that transient (ie existing running stuff) and persistent (on disk stuff) are updated correctly. I would prefer we continue to use it.

That's reasonable. Since hostnamed already has a varlink API, maybe hostnamectl is in the near-ish future anyways.