Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Manually assign uid/gid to username and groupname in dom0 #3989

Merged
merged 1 commit into from
Jun 25, 2024

Conversation

shjala
Copy link
Member

@shjala shjala commented Jun 21, 2024

Linuxkit dosn't use container's /etc/passwd and /etc/group files to resolve user and group names defined in build.yml. Instead it assignes increamentally created ids to the container[1] based on their delared position in rootfs.yml. It only respects the container's build.yml uid/gid value if its integer[2].

By assigning a fixed uid/gid in dom0, we can use the same value in the build.yml of the containers and be sure that access to resources work as expected, and adding/reordering containers in rootfs.yml won't break the access control.

[1] https://github.com/linuxkit/linuxkit/blob/4f89f4f67e392ffa8c8bab63dfaf31746e3c11a0/src/cmd/linuxkit/moby/build/build.go#L188
[2] https://github.com/linuxkit/linuxkit/blob/4f89f4f67e392ffa8c8bab63dfaf31746e3c11a0/src/cmd/linuxkit/moby/config.go#L708

@deitch
Copy link
Contributor

deitch commented Jun 21, 2024

@shjala this looks good to me. I like that we can fix it.

But, if linuxkit is doing this incorrectly, let's fix it. Open an issue there?

@shjala
Copy link
Member Author

shjala commented Jun 24, 2024

But, if linuxkit is doing this incorrectly, let's fix it. Open an issue there?

@deitch I thought about it, but I'm not sure if this is considered an issue in linuxkit. Should linuxkit resolve the names from the containers /etc/passwd ? or the containers host's /etc/passwd (in our case dom0)? or the system that is building the container?

Using fixed IDs seems straightforward and avoids possible confusion. But If you still think it should be fixed, just le me know I'll open an issue.

@deitch
Copy link
Contributor

deitch commented Jun 24, 2024

Actually, this is done in a funny way. These are all set for the user part of the OCI spec.

Normally, these are a property of the container, e.g. if you build a container with USER foo then that is what is put into the spec (ie config.json). So I do not think linuxkit should be digging into various /etc/passed to find it. But maybe it should be checking the properties of the OCI image that it is processing, the same way that docker or containerd would to generate that field when passing to runc?

@shjala
Copy link
Member Author

shjala commented Jun 25, 2024

@deitch Lets get this merged, I've opened a issue linuxkit/linuxkit#4047 and I'll send a new PR to eve once the issue is resolved.

@deitch
Copy link
Contributor

deitch commented Jun 25, 2024

Let eden run!

Linuxkit dosn't use container's /etc/passwd and /etc/group files to
resolve user and group names defined in build.yml. Instead it assignes
increamentally created ids to the container[1] based on their delared
position in rootfs.yml. It only respects the container's build.yml
uid/gid value if its integer[2].

By assigning a fixed uid/gid in dom0, we can use the same value in the
build.yml of the containers and be sure that access to resources work as
expected, and adding/reordering containers in rootfs.yml won't break
the access control.

[1] https://github.com/linuxkit/linuxkit/blob/4f89f4f67e392ffa8c8bab63dfaf31746e3c11a0/src/cmd/linuxkit/moby/build/build.go#L188
[2] https://github.com/linuxkit/linuxkit/blob/4f89f4f67e392ffa8c8bab63dfaf31746e3c11a0/src/cmd/linuxkit/moby/config.go#L708

Signed-off-by: Shahriyar Jalayeri <shahriyar@zededa.com>
@milan-zededa
Copy link
Contributor

Let eden run!

Note that we have some new regressions which soon will be fixed (by Andrew and Pavel).
I would recommend to wait with running eden here, rebase once fixes are submitted and then trigger it here.

Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@eriknordmark eriknordmark merged commit 865058c into lf-edge:master Jun 25, 2024
24 of 31 checks passed
@milan-zededa
Copy link
Contributor

milan-zededa commented Jun 25, 2024

@eriknordmark Although eden test failures reported for this PR are known and patches are being prepared (#3996 + #3998), I would appreciate if we could stop merging new PRs if anything but the Virtualization test suite fails (it fails due to an issue with the runner which we continue investigating). Sometimes we may get some Linux kernel crash in other test suites, which is likely irrelevant and also runner-related, but it should be the responsibility of the PR owner to investigate and possibly rerun the failed test suite. It does not make much sense to trigger rerun of failed tests and merge at the same time.
I don't have the capacity to keep chasing test regressions of already merged PRs. Also, Andrew just spent many hours investigating failing test caused by an already merged PR. It would be much easier if we caught it in the PR where the issue originated.
I understand that we didn't care about eden tests for the last month because they were completely broken, but I made some effort to fix all the issues apart from those caused by runners, so now we should all start paying attention to test results (and in particular PR owners)...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants