Consolidate XDG_RUNTIME code #1740

giuseppe · 2023-10-30T09:18:15Z

Closes: https://issues.redhat.com/browse/RUN-1169

openshift-ci · 2023-10-30T09:18:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe

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~~ [giuseppe]

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

giuseppe · 2023-10-30T09:50:58Z

testing the change here: containers/podman#20528

giuseppe · 2023-10-30T12:11:23Z

podman PR passes.

Ready to review

rhatdan · 2023-10-30T13:44:29Z

LGTM
@nalind @flouthoc @vrothberg @saschagrunert @Luap99 @mtrmac PTAL

nalind · 2023-10-30T13:49:49Z

pkg/homedir/homedir_unix.go

+	return rootlessConfigHomeDir, nil
+}
+
+// GetRuntimeDirUser returns the runtime directory


Is there guidance for someone who's using this package as to when they should use the value returned by this function instead of the one they get from GetRuntimeDir()?

I can specify it, the main difference is that this function tries to guess the value of XDG_RUNTIME_DIR if it is not set

nalind · 2023-10-30T13:51:26Z

pkg/homedir/homedir_unix.go

+	rootlessRuntimeDir        string
+)
+
+// GetRootlessConfigHomeDir returns the config home directory when running as non root


Is this supposed to be used as an alternative to GetConfigHome()? When?

the documentation of GetConfigHome() is very clear on the expectation of that function, and I didn't want to break it. I am not sure what is the best way to proceed.

Are you okay if we add the extra feature of trying to guess the ConfigHomeDir to the existing GetConfigHome() and not add a new function?

edsantiago · 2023-10-30T13:54:53Z

pkg/homedir/homedir_unix.go

+			}
+			tmpDir := filepath.Join(resolvedHome, ".config")
+			st, err := os.Stat(tmpDir)
+			if err == nil && int(st.Sys().(*syscall.Stat_t).Uid) == os.Geteuid() && st.Mode().Perm() >= 0o700 {


The >= made my eyes bug out. I understand that FileMode.Perm is only the ugo portion, but even so this just feels too weird. Is this a common Go idiom? If it isn't, would you mind changing it to .Perm() & 0o700 == 0o700?

that code is coming from containers/podman@f44b05f

This is a good occasion to fix it, I'll use your version

mtrmac

I appreciate this is mostly “just” moving code from Podman, but that is an occasion to revisit. Also, I think the introduction or 1/2/3 project boundaries between this subpackage and the original Podman callers mean that the API semantics and contracts must be much better documented, because with the ~stable API of c/storage (and presumably new non-Podman callers?), future maintainers of this subpackage need to now such things and “just see what Podman does” will be much less tenable.

pkg/homedir/homedir.go

pkg/homedir/homedir_others.go

pkg/homedir/homedir_unix.go

mtrmac · 2023-10-30T17:02:40Z

pkg/homedir/homedir_unix.go

+				rootlessRuntimeDirError = fmt.Errorf("cannot resolve %s: %w", home, err)
+				return
+			}
+			runtimeDir = filepath.Join(resolvedHome, "rundir")


$HOME/rundir, do we really want that?

yeah I think it would make sense to drop this if we "rework" this anyway.
I cannot imagine anyway wanting this fallback. Also podman sort of depends on runroot actually being tmpfs as the data must be gone after a reboot so this seems like a bad default. In all the issues I looked at in podman I never seen a path like this as the logic should already pick a path under /tmp before that I hardly imagine a case were this fails but $HOME would work.

And lastly I hate any program thinking they have the right to directly write into the my $HOME and not using some standard directory.

Weird things do happen in containers and other restricted environments… and this dates back to 2018 (containers/podman@4086a0f ).

I guess I don’t feel that strongly about this part — it is worth discussing but this function can change its fallback logic without changing its API (unless something records the value to persistent storage, and you hinted elsewhere that might be happening?).

pkg/homedir/homedir.go

pkg/homedir/homedir_others.go

giuseppe · 2023-10-30T20:21:02Z

I appreciate this is mostly “just” moving code from Podman, but that is an occasion to revisit. Also, I think the introduction or 1/2/3 project boundaries between this subpackage and the original Podman callers mean that the API semantics and contracts must be much better documented, because with the ~stable API of c/storage (and presumably new non-Podman callers?), future maintainers of this subpackage need to now such things and “just see what Podman does” will be much less tenable.

from how I understood it, the goal of this PR is effectively just to move code from Podman to c/storage so that it can be used from other packages. If there are small changes to accommodate other users then fine, but I don't think we need to re-evaluate all we did so far.

Currently, I am not even sure why we need this, I am working on it only because there is a Jira card for it.

If you think we don't need it, I am fine to close this PR and explain it in the related Jira card.

mtrmac · 2023-10-30T20:39:38Z

from how I understood it, the goal of this PR is effectively just to move code from Podman to c/storage so that it can be used from other packages.

With my Skopeo hat on, I don‘t know how I would review a PR to call a function which requires a parameter with an unknown value.

Somebody in our GitHub organization must know what the rootless parameter means. It’s hard for me to think that writing that down would be an excessive burden. If really no-one knows or cares, I 👿 …propose ripping that code out everywhere because it serves no known purpose? (Not really, but almost…)

For many of the other comments, quite possibly it’s just the comments that need adjusting.

(I think moving the code to c/storage is a bit of a higher burden, because c/storage mostly (entirely?) wants to keep a stable API, so the functions need to be merged roughly in the shape callers will want to use them. We have much less freedom to just adjust the function and all callers, and doing that cross-repo comes with a notably higher cost.)

Currently, I am not even sure why we need this, I am working on it only because there is a Jira card for it.

@vrothberg filed it. Valentin?

(My guess is

We almost certainly should share, in c/storage, a “get runtime directory” utility that deals with the su / sudo breakage by producing a nice error message. That seems clearly useful to me; but we also need to account for exotic platforms in some way. So maybe we should get the added error messages without the bit that makes functions not work at all on Windows.
For the bit that makes functions not work at all on Windows… I think that’s conceptually correct but if we do that, we ultimately have to replace that code in many callers with something. I don’t know what that is, and it seems to me that figuring that out is not much of a priority.
Similarly, for the functions to be useful, I think they should either return a value, or fail. "return "", deal with this yourself” — how is the caller supposed to do that?
I don’t know what the “unset variables when not rootless” bit is addressing.
Moving the code for XDG_RUNTIME_DIR fallback to c/storage, so that Skopeo etc. fall back the same way, and use the same files for skopeo login, would resolve a long-standing inconsistency, and seems desirable, but I don’t remember all of the history. At least the aspect of the current implementation, where this fallback is stored in a separate rootlessRuntimeDirOnce, and not overwritten back into the environment, would, I think, be quite an improvement for library use.
I don’t know what SetXdgDirs does / is intended to do so I don’t have an opinion on the code itself, apart from “I can’t LGTM code I don’t understand at all, especially when it clearly contradicts its own documentation.”

)

vrothberg · 2023-10-31T07:43:17Z

@vrothberg filed it. Valentin?

I filed the card during/after a meeting where we discussed XDG_ related issues. IIRC after looking at a bug and realizing that the tools don't behave the same and that the code is scattered. The goal of consolidating the code was to have 1 source of truth to make sure that a given bug must only get fixed in one place.

giuseppe · 2023-10-31T10:36:09Z

what do you think of the new version?

There are no API changes and it doesn't break any functionality on non-linux platforms.

The SetXdgDirs is copied from Podman. If it facilitates the merge of this PR, I can drop it from here and leave it in Podman

mtrmac · 2023-10-31T15:38:14Z

There are no API changes and it doesn't break any functionality on non-linux platforms.

See the non-resolved comments from the earlier review — these functions now return "" in some paths.

The Jira card asked for warnings, but that’s not a blocker for this PR as such.

The SetXdgDirs is copied from Podman. If it facilitates the merge of this PR, I can drop it from here and leave it in Podman

I personally can’t LGTM that underdocumented function as it exists today. So splitting that into a separate PR and finding another reviewer would work.

Alternatively, that other reviewer could LGTM this PR as a whole as it is — I’m not an expert in the problem space and I’m perfectly fine with leaving this PR to one.

pkg/homedir/homedir.go

pkg/unshare/unshare_linux.go

mtrmac · 2023-11-13T18:12:31Z

Although we don’t seem to have any “real users” of the UID parameter of getRootlessStorageOpts, I think just silently ignoring a parameter this deep in the call stack is confusing and makes the whole thing harder to maintain. That parameter should be removed outside-in: the callers, the public API, the implementation here.
I’m not really happy with dropping the unit tests using rootlessRuntimeDirEnvironmentTest, and replacing them with a call to a function with no tests. (I am open to being overridden by others on this, I guess.)

giuseppe · 2023-11-14T11:27:38Z

I think we should just drop DefaultStoreOptions() in its current form and only have DefaultStoreOptionsAutoDetectUID(). I forgot why we added it, maybe because Podman had a different way to track the rootless user compared to pkg/unshare. Since we are consolidating on that, there is no reason to pass the UID from the outside.

Do you want me to do this as part of this PR or can be done later? The issue with rootlessUID being partly ignored is already present in the current version

mtrmac · 2023-11-14T15:12:21Z

I agree that dropping DefaultStoreOptions (and UpdateStoreOptions) in favor of …AutoDetectUID (whether keeping the current names, or renaming …AutoDetectUID to just DefaultStoreOptions, it’s an API break either way) makes perfect sense.

Ideally, the ordering that makes most sense to me is #1740 (comment) . Priorities change, so I’d prefer not to make things a bit worse now to make them better only later; but I’ll defer to others if that seems unreasonable.

giuseppe · 2023-11-14T17:06:07Z

I agree that dropping DefaultStoreOptions (and UpdateStoreOptions) in favor of …AutoDetectUID (whether keeping the current names, or renaming …AutoDetectUID to just DefaultStoreOptions, it’s an API break either way) makes perfect sense.

Ideally, the ordering that makes most sense to me is #1740 (comment) . Priorities change, so I’d prefer not to make things a bit worse now to make them better only later; but I’ll defer to others if that seems unreasonable.

this complicates dependencies since now I've also to deal with buildah and c/image.

I've made these changes and opened the test PRs. If there are no more objections and the test PRs pass, let's merge as it is becoming kind of difficult to keep rebasing all our repos

mtrmac

I’m still unhappy with dropping the rootlessRuntimeDirEnvironmentTest tests.

types/options.go

types/utils.go

mtrmac · 2023-11-14T20:31:39Z

this complicates dependencies since now I've also to deal with buildah and c/image.

Yeah, and unfortunately the timing coincides with the soon-to-begin vendor dance.

I've made these changes and opened the test PRs. If there are no more objections and the test PRs pass, let's merge as it is becoming kind of difficult to keep rebasing all our repos

When a problem, during investigation, grows larger than anticipated, my go-to tool is to split it into smaller and more granular PRs, slicing the problem into isolated easier-to-handle chunks.

I don’t know how to deal with a PR that grows with the understood problem … or even faster than the understood problem, when issues come up more frequently than resolutions. (Historically, when there was enough business value for the feature, what has happened is that someone else approved and merged that PR. And I realize that included some in-retrospect-critical features. *shrug*)

copy the version from Podman that checks if the user owns the directory. [NO NEW TESTS NEEDED] moved from Podman Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

copy the version from Podman. [NO NEW TESTS NEEDED] moved from Podman Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

types/options.go

mtrmac · 2023-11-15T19:17:46Z

types/options.go

 // Everyone but the tests this is intended for should only call DefaultStoreOptions, never this function.
-func defaultStoreOptionsIsolated(rootless bool, rootlessUID int, storageConf string) (StoreOptions, error) {
+func loadStoreOptionsFromConfFile(storageConf string) (StoreOptions, error) {


(The idea of …Isolated is that it doesn’t depend on the environment, and it can be fully unit-tested in any environment; in this case, that would probably still mean having userPerUserStorage bool, rootlessUID int parameters; see [1]

… OTOH now that getRootlessStorageOpts effectively can’t be isolated that way any more, and considering that there is only one unit test exercising this, that might be not valuable enough to preserve… )

mtrmac · 2023-11-15T19:22:58Z

types/utils_test.go

+	storageOpts, err := loadStoreOptionsFromConfFile("./storage_test.conf")
+	expectedPath := filepath.Join(os.Getenv("HOME"), fmt.Sprintf("%d", unshare.GetRootlessUID()), "containers/storage")


[1] …Isolated had the (rootless, UID) parameters so that this test succeeds in all environments, even when run as root.

That would no longer be the case with these changes, but I also don’t know how much that matters. Maybe an explicit test skip would be enough. (Ideally I’d like the config logic to have good unit test coverage, but that’s clearly not this PR.)

types/options.go

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe · 2023-11-16T12:16:24Z

tests pass both in Podman and Buildah:

types/utils_test.go

drop the rootless argument from DefaultStoreOptions and UpdateStoreOptions since this can be retrieved internally through the unshare package. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe · 2023-11-16T21:15:25Z

can we merge now?

rhatdan · 2023-11-18T17:44:14Z

@mtrmac PTAL

mtrmac · 2023-11-20T14:32:58Z

I’d like the rootlessRuntimeDirEnvironmentTest tests to be ported/preserved/updated.

I don’t feel that strongly about that; if anyone else merges this, I won’t object.

rhatdan · 2023-11-20T14:41:42Z

/lgtm

openshift-ci bot added the do-not-merge/work-in-progress label Oct 30, 2023

openshift-ci bot added the approved label Oct 30, 2023

giuseppe force-pushed the consolidate-xdg-runtime-code branch from 66d84bc to 2ae9667 Compare October 30, 2023 09:21

giuseppe mentioned this pull request Oct 30, 2023

Consolidate XDG_RUNTIME code containers/podman#20528

Merged

giuseppe marked this pull request as ready for review October 30, 2023 12:11

openshift-ci bot removed the do-not-merge/work-in-progress label Oct 30, 2023

nalind reviewed Oct 30, 2023

View reviewed changes

edsantiago reviewed Oct 30, 2023

View reviewed changes

giuseppe force-pushed the consolidate-xdg-runtime-code branch from 2ae9667 to c7d2bb5 Compare October 30, 2023 16:02

mtrmac reviewed Oct 30, 2023

View reviewed changes

giuseppe force-pushed the consolidate-xdg-runtime-code branch 2 times, most recently from 99e7d57 to 8007023 Compare October 30, 2023 21:22

giuseppe force-pushed the consolidate-xdg-runtime-code branch from 8007023 to 0d648f1 Compare October 31, 2023 10:36

giuseppe marked this pull request as draft October 31, 2023 10:59

openshift-ci bot added the do-not-merge/work-in-progress label Oct 31, 2023

giuseppe force-pushed the consolidate-xdg-runtime-code branch 2 times, most recently from a3a48cc to 358e8f2 Compare October 31, 2023 15:03

mtrmac reviewed Oct 31, 2023

View reviewed changes

pkg/homedir/homedir.go Outdated Show resolved Hide resolved

pkg/unshare/unshare_linux.go Outdated Show resolved Hide resolved

giuseppe force-pushed the consolidate-xdg-runtime-code branch 2 times, most recently from 61e7c0a to 06acced Compare October 31, 2023 15:54

giuseppe force-pushed the consolidate-xdg-runtime-code branch 3 times, most recently from fb7b787 to 7341607 Compare November 14, 2023 17:02

mtrmac reviewed Nov 14, 2023

View reviewed changes

types/options.go Outdated Show resolved Hide resolved

types/options.go Outdated Show resolved Hide resolved

types/utils.go Outdated Show resolved Hide resolved

giuseppe added 2 commits November 15, 2023 09:51

pkg/homedir: enhance GetConfigHome()

61c1ece

copy the version from Podman that checks if the user owns the directory. [NO NEW TESTS NEEDED] moved from Podman Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

pkg/homedir: enhance GetRuntimeDir()

0e6a96b

copy the version from Podman. [NO NEW TESTS NEEDED] moved from Podman Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe force-pushed the consolidate-xdg-runtime-code branch from 7341607 to 51c2ed2 Compare November 15, 2023 09:40

mtrmac reviewed Nov 15, 2023

View reviewed changes

giuseppe force-pushed the consolidate-xdg-runtime-code branch from f45dd60 to 3737810 Compare November 15, 2023 21:11

mtrmac reviewed Nov 15, 2023

View reviewed changes

types/options.go Show resolved Hide resolved

types: use functionalities from pkg/homedir

c72a594

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe force-pushed the consolidate-xdg-runtime-code branch from 3737810 to b70af11 Compare November 16, 2023 07:36

giuseppe force-pushed the consolidate-xdg-runtime-code branch from b70af11 to 42cc08f Compare November 16, 2023 13:04

mtrmac reviewed Nov 16, 2023

View reviewed changes

types/utils_test.go Outdated Show resolved Hide resolved

store: drop rootless from arguments

b0885df

drop the rootless argument from DefaultStoreOptions and UpdateStoreOptions since this can be retrieved internally through the unshare package. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe force-pushed the consolidate-xdg-runtime-code branch from 42cc08f to b0885df Compare November 16, 2023 20:13

openshift-ci bot assigned rhatdan Nov 20, 2023

openshift-ci bot added the lgtm label Nov 20, 2023

openshift-merge-bot bot merged commit 2cf6198 into containers:main Nov 20, 2023
18 checks passed

ashley-cui mentioned this pull request Jan 26, 2024

Fix image filters parsing containers/podman#21260

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidate XDG_RUNTIME code #1740

Consolidate XDG_RUNTIME code #1740

giuseppe commented Oct 30, 2023

openshift-ci bot commented Oct 30, 2023

giuseppe commented Oct 30, 2023

giuseppe commented Oct 30, 2023

rhatdan commented Oct 30, 2023

nalind Oct 30, 2023

giuseppe Oct 30, 2023

nalind Oct 30, 2023

giuseppe Oct 30, 2023

edsantiago Oct 30, 2023

giuseppe Oct 30, 2023

mtrmac left a comment

mtrmac Oct 30, 2023

Luap99 Nov 3, 2023

mtrmac Nov 3, 2023

giuseppe commented Oct 30, 2023

mtrmac commented Oct 30, 2023

vrothberg commented Oct 31, 2023 •

edited

giuseppe commented Oct 31, 2023

mtrmac commented Oct 31, 2023

mtrmac commented Nov 13, 2023

giuseppe commented Nov 14, 2023

mtrmac commented Nov 14, 2023

giuseppe commented Nov 14, 2023

mtrmac left a comment

mtrmac commented Nov 14, 2023

mtrmac Nov 15, 2023

mtrmac Nov 15, 2023

giuseppe commented Nov 16, 2023

giuseppe commented Nov 16, 2023

rhatdan commented Nov 18, 2023

mtrmac commented Nov 20, 2023

rhatdan commented Nov 20, 2023

		storageOpts, err := loadStoreOptionsFromConfFile("./storage_test.conf")
		expectedPath := filepath.Join(os.Getenv("HOME"), fmt.Sprintf("%d", unshare.GetRootlessUID()), "containers/storage")

Consolidate XDG_RUNTIME code #1740

Consolidate XDG_RUNTIME code #1740

Conversation

giuseppe commented Oct 30, 2023

openshift-ci bot commented Oct 30, 2023

giuseppe commented Oct 30, 2023

giuseppe commented Oct 30, 2023

rhatdan commented Oct 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giuseppe commented Oct 30, 2023

mtrmac commented Oct 30, 2023

vrothberg commented Oct 31, 2023 • edited

giuseppe commented Oct 31, 2023

mtrmac commented Oct 31, 2023

mtrmac commented Nov 13, 2023

giuseppe commented Nov 14, 2023

mtrmac commented Nov 14, 2023

giuseppe commented Nov 14, 2023

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac commented Nov 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giuseppe commented Nov 16, 2023

giuseppe commented Nov 16, 2023

rhatdan commented Nov 18, 2023

mtrmac commented Nov 20, 2023

rhatdan commented Nov 20, 2023

vrothberg commented Oct 31, 2023 •

edited