pkg/config: rework system connections and farm storage #1826
Conversation
|
first look, LGTM code-wise ... i see it is a draft as well. |
|
Only draft so nobody merges it before podman PR is ready to go because this a major breaking change in terms of the API usage and I don't want to block c/common work from others. |
There was a problem hiding this comment.
I still perceive storing machine-only data in .config/ as an anti-pattern. It can certainly be documented but IMHO it is the equivalent to /etc on a system level.
It's certainly a big improvement over the status quo as it addresses the problem of podman doing funny things with user configs.
One thing that worries me more though is that (IIUIC) sudo podman may now start reading my user configs in .config/. I understand the motivation for this change but this may break valid use cases of having separate configs for root/non-root and managing that from one sessions/script/automation. Assuming I understand it correctly, what is the recommended workaround to restore the old behavior?
| return "", err | ||
| } | ||
| // file is stored next to containers.conf | ||
| return filepath.Join(filepath.Dir(path), connectionsFile), nil |
There was a problem hiding this comment.
What if I run CONTAINERS_CONF=/read/only/dir/containers.conf podman ...? Would try to place it in the same directory?
There was a problem hiding this comment.
no it does not use CONTAINERS_CONF, it will still use the default ~/.config/containers even when CONTAINERS_CONF is set
To change the location for this new file PODMAN_CONNECTIONS_CONF must be used.
Many programs store data in
I don't understand what you are saying here? Previously it was impossible to have a root only config. IF you want a config that is read by all users then you can still use /etc |
Could be brain fog on my end. If now run |
It reads |
|
OK, that makes sense 👍 |
|
@Luap99 Is this still draft? |
|
This is done, good to review but please do not merge yet (thus marked as draft) because the Podman PR containers/podman#21384 needs more test fixes. |
Signed-off-by: Paul Holzinger <[email protected]>
The current code has a small race it first stats the file and if it exists it tries to read the file. Between this it is possible that the file was removed and thus cause a fatal error when reading the config. The better way is to simply read the file and ignore the ENOENT error instead where we want this behavior. This avoids the need for the extra stat syscalls. For CONTAINERS_CONF and modules we still need the hard error if the file does not exists so we have to keep it there. Signed-off-by: Paul Holzinger <[email protected]>
It is not called by podman or buildah, it also makes no sense to return a path string with $HOME in it. Just delete it. Signed-off-by: Paul Holzinger <[email protected]>
There is really no need to limit reading the config under $XDG_CONFIG_HOME or $HOME to rootless users only. This poses two problems, first on a multi user system any config that should be only applied to root in /etc will also be read by all other users which makes this impossible to use without having all user overwrite that option with their local containers.conf. If we read the config from $HOME as root as well then such changes are easy. Second, because connections/farms are currently written by the cli it means as root is tries to write under /etc which is not good as in some envs /etc is mounted read only. Signed-off-by: Paul Holzinger <[email protected]>
podman systemd conenction and farm currently both write containers.conf to store their settings. Each write removes comments from the user config file and thus makes it not great to use. The new approach is to have a seperate file connections.conf (json format) to store both conenctions and farms for podman. We continue to read containers.conf for the connections and farms as well and podman can read both. This means we have a read only store in containers.conf (manually added by users), they cannot be removed by the podman cli. This is a breaking chnage and will require many chnages in podman to migrate to the new APIs added in this commit. Signed-off-by: Paul Holzinger <[email protected]>
|
I changed the GetFarmConnections() function to make podman tests, pass as it also needed to return the name of the default farm so podman farm can log it correctly. |
|
With a bit of luck the Podman PR should now pass, please also have a look over there. |
|
LGTM |
|
Podman tests look good so this ready for merge, once this is in I rebase the podman PR |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99, vrothberg 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:
Approvers can indicate their approval by writing |
see commits
WIP podman PR containers/podman#21384