c8d: Use a specific containerd namespace when userns are remapped#46786
c8d: Use a specific containerd namespace when userns are remapped#46786thaJeztah merged 1 commit intomoby:masterfrom
Conversation
rumpl
added
status/2-code-review
containerd-integration
rumpl
force-pushed
the
c8d-userns-namespace
branch
from
276778e to
d74ec7f
Compare
rumpl
force-pushed
the
c8d-userns-namespace
branch
from
d74ec7f to
17f698a
Compare
|
@thaJeztah I rebased this one, the whole config situation needs a closer look, I think we should clean it up massively but that would be outside of the scope of this one |
laurazard
left a comment
laurazard
left a comment
There was a problem hiding this comment.
LGTM, left a couple comments on a typo, but overall LGTM! (and we really should try to refactor that config code)
rumpl
force-pushed
the
c8d-userns-namespace
branch
from
17f698a to
35dbc02
Compare
| ns = config.ContainerdNamespace | ||
| if _, ok := config.ValuesSet["containerd-namespace"]; !ok { | ||
| ns = fmt.Sprintf("%s-%d.%d", config.ContainerdNamespace, root.UID, root.GID) | ||
| } | ||
|
|
||
| pluginNs = config.ContainerdPluginNamespace | ||
| if _, ok := config.ValuesSet["containerd-plugin-namespace"]; !ok { | ||
| pluginNs = fmt.Sprintf("%s-%d.%d", config.ContainerdPluginNamespace, root.UID, root.GID) | ||
| } |
There was a problem hiding this comment.
There might be a usability issue; if someone specifies the namespaces in their config, we will use those. Which means that if someone specified a custom namespace, and that namespace is not empty, things will likely start failing;
--containerd-namespace string Containerd namespace to use (default "moby")
--containerd-plugins-namespace string Containerd namespace to use for plugins (default "plugins.moby")
Wondering if there's a way for us to detect;
- if the namespace is non-empty
- if it's not empty, if it was created with the correct user-mapping
And to otherwise bail out and produce an error
(Should be fine for a follow-up, but maybe we should create a tracking ticket?)
There was a problem hiding this comment.
My idea was that, if they tell us which namespace to use then they know what they are doing. Now yeah, the namespace could have an incorrect mapping, we should do something about that, we could add a label to the namespace (looks like that's possible in containerd), but then that would work only for the ones we create, I'm not sure how we can check that an existing namespace has the right user-mapping... Maybe we can make it work only with the namespaces we create? Ok let's open an issue for this, a comment in a PR is not the best place to find a solution for this
There was a problem hiding this comment.
Yes, it's not a blocker for this PR, but something to look at. My consideration here was;
- user configures daemon (with namespace)
- runs the daemon with that config
- user now decides to enable user-namespaces
- daemon now fails with potentially confusing errors
- and.. now how to solve? (how to clean the namespace etc?)
We need to isolate the images that we are remapping to a userns, we can't mix them with "normal" images. In the graph driver case this means we create a new root directory where we store the images and everything else, in the containerd case we can use a new namespace. Signed-off-by: Djordje Lukic <[email protected]>
rumpl
force-pushed
the
c8d-userns-namespace
branch
from
35dbc02 to
3a617e5
Compare
| conf.CDISpecDirs = nil | ||
| } | ||
|
|
||
| if err := loadCLIPlatformConfig(conf); err != nil { |
There was a problem hiding this comment.
Thanks! Yup, I think this is the cleanest way to do this indeed, as it keeps it internal.
4 participants
- What I did
We need to isolate the images that we are remapping to a userns, we can't mix them with "normal" images. In the graph driver case this means that we create a new root directory where we store the images and everything else, in the containerd case we can use a new namespace.
- How I did it
- How to verify it
Run a daemon with
--userns-remap=defaultand pull and image, then:And you shouldn't see the image that you pulled.
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)