core/service: imply Type=exec if credentials are used

[YHNdnzj]

See also: #32583 (comment)

[github-actions]

Important

An -rc1 tag has been created and a release is being prepared, so please note that PRs introducing new features and APIs will be held back until the new version has been released.

[yuwata]

When credentials are used with Type=simple, i.e. when
multiple sd-executor instances are running in parallel
for a single service, the state of final credential dir
might be unexpected wrt path_is_mount_point() and other
steps.

What happens when Type=simple is forcibly used with credentials?? Should we refuse such combination? Or, should we fix something in loading credentials??

[YHNdnzj]

What happens when Type=simple is forcibly used with credentials?? Should we refuse such combination?

I don't feel too strong about this. But note that this race only happens if ExecStartPost= is used, so refusing Type=simple might be overkill?

Or, should we fix something in loading credentials??

The credentials are created in a shared final dir, so that pretty much requires locking? Instead of doing locking in sd-executor, I think our job logic is sufficient to handle this in a reasonable enough way, i.e. Type=exec.

[poettering]

So I take it that the problem this is addresses is if creds are used together with Type=simple and at least one ExecStartPost= line?

I have the suspicion that ExecStartPost= is not that commonly used. It is definitely used by some, but probably not the most typical thing.

In an ideal world we'd just make this combo work, i.e. make the credential setup logic safe so that multiple of them can run in parallel (a simple way could be via some fd we pass to all instances of systemd-executor that we use file locks on, for the time window when we are setting up the creds).

Now, if we are too lazy to just make it work for I think the next best thing would be to simply catch the explicit case (i.e. creds + Type=simple + ExecStartPost=) and then warn about this, explaining in a log message that this is currently not safe, and people should use Type=exec to make it work.

So basically what I am saying, that fixing this properly, or keeping the door open to fixing this properly is better than changing the logic to route around it.

Does that make any sense?

[YHNdnzj]

In an ideal world we'd just make this combo work, i.e. make the credential setup logic safe so that multiple of them can run in parallel (a simple way could be via some fd we pass to all instances of systemd-executor that we use file locks on, for the time window when we are setting up the creds).

I'd still say that the way we make this work could (and IMO should just) be Type=exec. We generally recommend Type=exec over Type=simple, and it solves this race perfectly. We only run each control process in sequence, so only the main process needs to be synchronized.

Adding a warning for people that use Type=simple + creds + ExecStartPost= is good, and that should be done nonetheless. But shoving in another synchronization fd to solve a problem that can just be easily solved using job engine is not worth it.

[YHNdnzj]

(I'll be off for a few days. Unsetting the please-review flag since the warning shall be added)

[YHNdnzj]

Another reason why I think explicit ordering through state engine is desirable: If there're multiple sd-executor instances, the order in which exec_setup_credentials gets called isn't deterministic, and locking doesn't prevent the one for ExecStartPost= to run before the one for ExecStart=.