AddSystemd should only check for NOTIFY_SOCKET env

[crozone]

Microsoft.Extensions.Hosting.SystemdHostBuilderExtensions.AddSystemd() calls SystemdHelpers.IsSystemdService() to determine whether it should add the SystemdNotifier and SystemdLifetime services to the services container.

IsSystemdService() goes out of its way to try and verify that its parent process is actually called "systemd", and also has an additional edgecase for PID 1 for if it is invoked in a service container.

Reference: https://github.com/dotnet/runtime/blob/076f8c58984b009033204ad1dd9f86c691ae42fb/src/libraries/Microsoft.Extensions.Hosting.Systemd/src/SystemdHostBuilderExtensions.cs#L68-L77`

I can't see any reason why the check needs to be this strict and specific. It also appears to have a few drawbacks:

• This check only works with services directly invoked from systemd, but this isn't actually required. AddSystemd() is primarily concerned with notifying the NOTIFY_SOCKET (aka sd_notify) that the service has started up or is shutting down. Non-systemd systems often include tools like start-stop-daemon which also support a minimal NOTIFY_SOCKET interface, but are needlessly excluded by this check.

• This check causes false positives when the service is invoked from systemd on an interactive shell, for example: UseSystemd can detect an interactive shell as running as systemd service extensions#2525. AFAIK this issue is marked as resolved but has not actually been fixed.

• This check causes a false positive if the service is not configured as Type=notify.

• The code is more complicated and requires a workaround for container invocation where the PID is 1 and there is no parent systemd process.

A significantly simpler and more robust solution would be to only check for the presence of the NOTIFY_SOCKET environment variable (and also that we're on Unix). If NOTIFY_SOCKET is set, it should be notified, otherwise don't notify it. The presence of the environment variable should be enough of an indication that the interface is supported and notification is required.

The current SystemdHelpers.IsSystemdService() could still be used for changing the log format.

Example of AddSystemd() with changes:

public static IServiceCollection AddSystemd(this IServiceCollection services)
{
    ThrowHelper.ThrowIfNull(services);

    if (SystemdHelpers.IsSystemdService())
    {
        services.Configure<ConsoleLoggerOptions>(options =>
        {
            options.FormatterName = ConsoleFormatterNames.Systemd;
        });
    }

    if (Environment.OSVersion.Platform == PlatformID.Unix &&
        !string.IsNullOrEmpty(Environment.GetEnvironmentVariable("NOTIFY_SOCKET")))
    {
        services.AddSingleton<ISystemdNotifier, SystemdNotifier>();
        services.AddSingleton<IHostLifetime, SystemdLifetime>();
    }

    return services;
}

Does this seem like a sensible change to consider?

Tagging subscribers to this area: @dotnet/area-extensions-hosting
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Microsoft.Extensions.Hosting.SystemdHostBuilderExtensions.AddSystemd() calls SystemdHelpers.IsSystemdService() to determine whether it should add the SystemdNotifier and SystemdLifetime services to the services container.

IsSystemdService() goes out of its way to try and verify that its parent process is actually called "systemd", and also has an additional edgecase for PID 1 for if it is invoked in a service container.

Reference: https://github.com/dotnet/runtime/blob/076f8c58984b009033204ad1dd9f86c691ae42fb/src/libraries/Microsoft.Extensions.Hosting.Systemd/src/SystemdHostBuilderExtensions.cs#L68-L77`

I can't see any reason why the check needs to be this strict and specific. It also appears to have a few drawbacks:

• This check only works with services directly invoked from systemd, but this isn't actually required. AddSystemd() is primarily concerned with notifying the NOTIFY_SOCKET (aka sd_notify) that the service has started up or is shutting down. Non-systemd systems often include tools like start-stop-daemon which also support a minimal NOTIFY_SOCKET interface, but are needlessly excluded by this check.

• This check causes false positives when the service is invoked from systemd on an interactive shell, for example: UseSystemd can detect an interactive shell as running as systemd service extensions#2525. AFAIK this issue is marked as resolved but has not actually been fixed.

• This check causes a false positive if the service is not configured as Type=notify.

• The code is more complicated and requires a workaround for container invocation where the PID is 1 and there is no parent systemd process.

A significantly simpler and more robust solution would be to only check for the presence of the NOTIFY_SOCKET environment variable (and also that we're on Unix). If NOTIFY_SOCKET is set, it should be notified, otherwise don't notify it. The presence of the environment variable should be enough of an indication that the interface is supported and notification is required.

The current SystemdHelpers.IsSystemdService() could still be used for changing the log format.

Example of AddSystemd() with changes:

public static IServiceCollection AddSystemd(this IServiceCollection services)
{
    ThrowHelper.ThrowIfNull(services);

    if (SystemdHelpers.IsSystemdService())
    {
        services.Configure<ConsoleLoggerOptions>(options =>
        {
            options.FormatterName = ConsoleFormatterNames.Systemd;
        });
    }

    if (Environment.OSVersion.Platform == PlatformID.Unix &&
        !string.IsNullOrEmpty(Environment.GetEnvironmentVariable("NOTIFY_SOCKET")))
    {
        services.AddSingleton<ISystemdNotifier, SystemdNotifier>();
        services.AddSingleton<IHostLifetime, SystemdLifetime>();
    }

    return services;
}

Does this seem like a sensible change to consider?

Author:	crozone
Assignees:	-
Labels:	untriaged, area-Extensions-Hosting
Milestone:	-

[buyaa-n]

Tagging area experts @tmds @eerhardt @steveharter

[tmds]

This check causes false positives when the service is invoked from systemd on an interactive shell, for example:
dotnet/extensions#2525. AFAIK this issue is marked as resolved but has not actually been fixed.

This was fixed by dotnet/extensions#2734 which restricted the systemd check to be the direct parent.

I think it's worth considering to enable the notifier based on NOTIFY_SOCKET being set independent of systemd being the direct parent.

I wonder if we should something that avoids .NET children to assume the notifier responsibility?
The C API has a flag causes the envvar to be unset.
If we do, at what point should we unset it?

[steveharter]

I think it's worth considering to enable the notifier based on NOTIFY_SOCKET being set independent of systemd being the direct parent.
I wonder if we should something that avoids .NET children to assume the notifier responsibility?
The C API has a flag causes the envvar to be unset.
If we do, at what point should we unset it?

Triage: we have a partial fix for the original with some additional work \ questions remaining.

[karelz]

Triage discussion -- things to consider (not final decisions):

• It seems like a good idea to allow non-Systemd init systems (at developer's risk - without integration / official support)
• We might want to keep AddSystemd as strict and add a new method, which is less strict and allows non-Systemd init systems.
• We should add test when implementing.

Given that non-Systemd init systems are rare, we will likely not get to it, but we are willing to take a contribution (we will help with the API design if needed).

[CybCorv]

ProtectProc=invisible silently breaks UseSystemd()

Reference : https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.Hosting.Systemd/src/SystemdHelpers.cs

Context

The systemd.exec(5) man page states:

It is generally recommended to run most system services with this option set to "invisible".

Yet setting ProtectProc=invisible (or ptraceable) on a .NET service causes UseSystemd() to silently do nothing — READY=1 is never sent, systemd times out.

Root cause

GetIsSystemdService() reads /proc/{ppid}/comm to verify the parent is named systemd. With ProtectProc=invisible, that file is hidden, the exception is swallowed, the function returns false, and no IHostLifetime is registered. No error, no warning.

The fix is in the official reference implementation

The official sd_notify example in the freedesktop documentation is unambiguous:

/* If the variable is not set, the protocol is a noop */
socket_path = getenv("NOTIFY_SOCKET");
if (!socket_path)
    return 0; /* Not set? Nothing to do */

No /proc read. No parent process check. NOTIFY_SOCKET is the sole criterion defined by the protocol.
I'd be happy to work on a PR. Given the coupling between GetIsSystemdService() and the missing unset of NOTIFY_SOCKET in SystemdNotifier, I'd welcome guidance on the intended scope before starting.

Related: #123798.

[crozone]

@CybCorvI I suspect the reason for this complication is that AddSystemd() is doing double duty: It activates both the ConsoleFormatterNames.Systemd console format, as well as enabling the sd_notify mechanism. Although still, I'm not sure why.

This should really be split out into two separate functions. A dedicated function eg. AddSdNotify() for detecting the presence of NOTIFY_SOCKET, to enable the SystemdNotifier and SystemdLifetime.

A second function should detect and enable Systemd logging via sd_journal_send, using the actual logging interface.

Really, everything about the current implementation of AddSystemd() is overly complicated and incorrect. I really can't see any reason it was written the way that it was. Simply using feature detection for the individual features and then implementing those features would have just worked.