[Design Doc] config file parsing changes for podman 6

[Luap99]

As part of podman 6 we like to improve and consolidate how we parse our various config files.

Does this PR introduce a user-facing change?

None

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [Luap99]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[Luap99]

I am not to sure how to handle these best. I guess it would be nice to consolidate the logic there as well but it also increased the scope.

[mheon]

Is this defined and working on MacOS?

[Luap99]

You mean the XDG_CONFIG_HOME env?

That env either is set or it doesn't in which case we fall back to $HOME/.config just like on linux. If your question is if macos uses the XDG_ vars at all then not that I know off, that would however not prevent a users from using it. And also that is the current behaviour for containers.conf on macos as well to use XDG_CONFIG_HOME so I didn't see a reason to change this.

[mtrmac]

https://developer.apple.com/library/archive/documentation/FileManagement/Conceptual/FileSystemProgrammingGuide/FileSystemOverview/FileSystemOverview.html#//apple_ref/doc/uid/TP40010672-CH2-SW1 is the traditional macOS expectation, not .config.

I don’t have a strong opinion here … consistency with Linux might be easier for everyone.

[Luap99]

yeah though in this case I find it easier to just keep doing what we have been doing for consistency.

And we haven't gotten any complains for macos users about AFAIACT so I didn't saw a reason to justify this changee

[baude]

Suggested change

	get all implement their own parsing logic and have a different feature set. The goal is to
	all implement their own parsing logic and have a different feature set. The goal is to

[baude]

Paul, this LGTM. I had just a nit and you answered my other questions already. My one concern about this document is there are a couple of loose ends (JSON files, and containers.conf split) that I don't want to loose. Could you add a section at the end where we can cite follow-up items?

This was excellent work BTW. Thank you.

[mtrmac]

https://developer.apple.com/library/archive/documentation/FileManagement/Conceptual/FileSystemProgrammingGuide/FileSystemOverview/FileSystemOverview.html#//apple_ref/doc/uid/TP40010672-CH2-SW1 is the traditional macOS expectation, not .config.

I don’t have a strong opinion here … consistency with Linux might be easier for everyone.

[mtrmac]

I’m generally not a fan of environment variables in libraries (I’d prefer to consolidate environment variable handling into an optionally-imported subpackage instead, compare c/image/pkg/cli/environment… never used anywhere it turns out), but, *shrug*, I’ll go with the majority.

[mtrmac]

Assuming entries without the special {append=true} continue to be work without change, as a full override, WFM.

[Luap99]

Yes without the special entry thing work the same. Now there is one caveat here of course that this means changing the data type of the config fields.
V2RegistriesConf seems to be exported in same cases (TryUpdatingCache, which says it is deprecated). I am not sure how important preserving type compatibility there is, it doesn't seem like there is much/any use for callers to use this struct directly but who knows. If we can break it that would be easiest to implement though if you prefer we don't we could still create a second type and copy filed into the current one before returning but it would mean a bit more extra code to maintain.

[mtrmac]

We already have a separate private parsedConfig.

The major external user I’m aware of is openshift/machine-config-operator + openshift/runtime-utils, parsing ~arbitrary files and updating fields in them, and that includes the affected UnqualifiedSearchRegistries. So, whether we add a new type or not, they will need to update c/image. (The good news is that they don’t ~export the type, and don’t have stable API commitments…)

Supporting external config file users / editors, as features evolve, is always tricky (compare podman image sign for registries.d elsewhere).

I’m inclined to freeze/deprecate the V2RegistriesConf type, and to… have AI write… a tedious encapsulated API where we don’t expose the data structure but provide a set of functions (“find registry entry for $scope; add mirror”…) — under the theory that we can always add more functions/methods — but every imaginable API always commits us to something about the design, so encapsulating is by no means future-proof.

Freezing+deprecating V2RegistriesConf as is, and adding … V2RegistriesConfV2 as a public type … would also not be too bad.

[mtrmac]

Is that practical? Wouldn't the rootful and rootless configs differ quite a bit, making a rootless drop-in pretty annoying to maintain, with most containers.conf updates requiring an immediate drop-in update to undo/replace that change for rootless?

(I have no idea.)

[Luap99]

I am not sure I got your point.

The way I see it the default file a vendor ships should be a fully commented out file. Any customisations a package wants to do should be put into a drop in, i.e. 50-fedora.conf or so something.

Then if the fields are for everything put it into the regular conf.d, if it is root only rootful.conf.d and rootless into rootless.conf.d, so there should be no need to undo anything as rootless.

[mtrmac]

Yes, that would work. It’s not a very natural workflow — tutorials are going to say “edit containers.conf” — but users who run into this can be pointed at this approach.

[mtrmac]

… and ignore environment variables, to be explicit.

[mtrmac]

There’s another different behavior, see newConfigWrapperWithHomeDir : If there is a per-user registries.conf, the system-wide registries.conf.d are completely ignored. That was designed that way intentionally, historically; the UAPI spec explicitly defines the opposite.

I don’t know what we want to do here…

[Luap99]

I think the UAPI behaviour is better and we should adopt that.

The best argument I can make is consider the shortnames currently shipped as /etc/containers/registries.conf.d/000-shortnames.conf

Now say I want to add a registry for my user and do this in my home dir I then loose access to all these shortnames and have to copy the file (or I guess better symlink to receive updates)

In practise I would say almost all people would want to keep the shortnames, now with UAPI if people want to unset the default shortnames all they would need to do is add an empty file ~/.config/containers/registries.conf.d/000-shortnames.conf so they still can do that if that is what they wanted.

[mtrmac]

My thinking here was that I don’t want any ambiguities in the security-critical config, so duplicates within the single policy.json are strictly rejected. Drop-ins would sort of force us to support duplicates/overrides. Not a disaster but a bit of a worry.

[Luap99]

yeah I am not trying to push for drop-ins policy.json. I think single fine is fine for it atm.

[mtrmac]

The worry here is “If you make a typo when installing /etc/containers/policy.json, your configuration is going to be ignored”.

I could see an argument that this is no different from any other config file… yet, right now, on a typo, the policy.json mechanism “fails closed” and forces the user to fix that before being able to pull images. (Admittedly there are other typos where the “fails closed” property can’t be upheld, e.g. a typo when installing an updated policy is undetectable by c/image.)

[Luap99]

Well the obvious argument is you have the exact same problem today.
If I typo the path for my custom homedir config it falls back to the /etc one which I think basically on all distro which ship is is a accept anything? So really adding one more location is not going to change that?

Users/Admins are responsible for configuring the systems we cannot protect for so things, podman image trust show outputs the policy so admin should verify that and I guess log-level debug etc...
If you have an important policy it is always the end user would should actually tests that the policy gets enforced by us.

[mtrmac]

A downside to this is that we lose control on upgrades. With /etc, there’s at least .rpmold/.rpmnew — or even a conflict and an upgrade failure might be better than silently continuing with an obsolete config.

Probably not enough of a downside, just something to think about.

[Luap99]

I think it is the advantage of /usr to not have these kinds of package install conflicts at all,
systemd people are clearly advocating for this /usr model
Practically speaking I have no idea how people use the .rpmold/.rpmnew files, personally I never touched any of them nor looked at them anywhere on all my systems.

[mtrmac]

Practically speaking I have no idea how people use the .rpmold/.rpmnew files

diff / diff3 I guess. But that assumes an admin who pays attention and notices that the file was created at all — relevant only for “pet” systems.

[mtrmac]

… oh, and: a config file in /usr/ being automatically updated protects the packager’s configuration. The “benefit” of conflicts and failures is that it protects the user’s configuration. Sure, ideally users would read release notes.

[Honny1]

So, if I understand correctly, the Podman machine will have XDG_CONFIG_HOME set and will load configs from the host's filesystem through mounts. Am I correct?

[Luap99]

podman machine is fully out of scope of this doc

Right now we do lookup containers.conf and registries.conf on the client side as well thus macos paths need to be documented as well.

Now yes long term for machine since most files are still needed on the server it would make sense for machine to expose like a single mount on the host where they configure the inside from the host.

[giuseppe]

How this will work with nested Podman? Will it be treated as rootful or rootless? It has EUID==0 but no capabilities in the initial namespace.

Should we instead differentiate by privileges (admin vs user) instead of (rootful vs rootless)?

[Luap99]

The current containers.conf version uses euid.

I think that is the most logical one because if I am uid 0 I likely still want to use /var/lib/containers as storage.conf setting for example ,etc...
I find the way we deal with the rexec (CAP_SYS_ADAMIN) highly confusing and not something that would be easily understood by users. We also need to keep in mind that our podman re-exec is different from buildah, skopeo or other library users so I find using the uid 0 is the best way to keep it reasonable.

[giuseppe]

one thing worth thinking about is how --storage-driver and --storage-opt can be made to work nicely with the new configuration proposed here.

The fact that using these flags reset completely anything coming from the storage.conf configuration file is very annoying and it happened more than once that users got bitten by this.

[Luap99]

That is a good point, I find it definitely relates to this work though it doesn't really intersect directly I think?

I don't know the history behind why this unset logic was chosen. If there is demand and others agree we can certainly add this.

[giuseppe]

I don't know the reason behind it, but it is highly confusing.

The intersection is that now we don't "merge" storage configuration files, so to behave like the CLI does, we would just need to pick the last storage.conf that overrides everything that was configured earlier (which is not nice).

[mheon]

I think the original logic was that if you overrode driver, your storage options would become invalid so it was necessary to unset them all. But, if that were the case, why not just say that it is required to pass --storage-opt if you pass --storage-driver? I don't see a good reason to retain the current behavior.

[mtrmac]

The intersection is that now we don't "merge" storage configuration files,

… that reminds me of containers/storage#1885, and the various “stringly typed” storage options. And how I am scared of the path + parsing control flow in c/storage/types.

Consider yourself warned that adding drop-in support to this is going to be demanding.