zabbly / incus

Incus package repository
176 stars 14 forks source link

Management of subuid/subgid as part of package #15

Closed f-bn closed 7 months ago

f-bn commented 7 months ago

Hello !

I was looking at the LXD Debian package sources and see that when user are created, the lxd and root users get some subuid/gid for user namespacing stuff.

Moreover in the docs, when building it from sources, it is mentioned that we need to add at least 10M range of UID/GID for the root user: https://linuxcontainers.org/incus/docs/main/installing/#machine-setup

I don't see anything related to this in the Incus package, do we still need to handle it as part of package or there is some new kernel stuff that manage it automatically ?

Thanks !

stgraber commented 7 months ago

We could put the same logic in place.

You need to have those entries in /etc/subuid and /etc/subgid if your system has the uidmap package installed (specifically has the newuidmap and newgidmap commands).

If you don't, then Incus just assumes it has free reign over the uid/gid allocations and will pick the default allocation of 0:1000000:1000000000.

We could try to match that pretty closely with the packaging, basically using the logic you pointed to but with NEXT_UID/NEXT_GID set to 1000000 (this avoids clashes with some central auth systems) and set the map size to be 1000000000.

f-bn commented 7 months ago

OK, didn't know about Incus assumption of UID/GID allocations, I thought this was mandatory and Incus will fail to launch containers if not configured therefore, that's good to know. Regarding logic to apply, do we need to do the same kind of calculation even with an high range start as you mentionned to avoid overlap ?

Moreover, do we need to apply those also for an incus user (for example the one we create on #14) like in the Debian package ? Is there a use-case since it's not mentionned in the documentation ? Since it seems that this user is only used for Incus sub-processes and QEMU VMs, I don't see it but I could be wrong.

Thanks :)

stgraber commented 7 months ago

OK, didn't know about Incus assumption of UID/GID allocations, I thought this was mandatory and Incus will fail to launch containers if not configured therefore, that's good to know. Regarding logic to apply, do we need to do the same kind of calculation even with an high range start as you mentionned to avoid overlap ?

We need to keep the loop around just in case someone has manually defined some allocations in that range. It's unlikely, but probably still a good idea to do it.

Moreover, do we need to apply those also for an incus user (for example the one we create on #14) like in the Debian package ? Is there a use-case since it's not mentionned in the documentation ? Since it seems that this user is only used for Incus sub-processes and QEMU VMs, I don't see it but I could be wrong.

We don't. The only reason why we usually were assigning the map to both the user and root was so we could easily track down why root has a specific allocation. So it was purely done for cosmetic reasons and never actually used by the lxd/incus user.

As I don't think anyone ever really looks at the file anyway, only assigning to the root user is probably fine.

f-bn commented 7 months ago

Thanks for your answers. I could also send a PR on this one (or maybe putting it in the same PR as #14, wdyt ?)

stgraber commented 7 months ago

Yeah, you can do one PR with each change as a separate commit in there.