containerd/crio passes invalid config.json

[cyphar]

However, for this to work with containerd I had to do this change:

$ git diff
diff --git libcontainer/specconv/spec_linux.go libcontainer/specconv/spec_linux.go
index 991962c4..203bf694 100644
--- libcontainer/specconv/spec_linux.go
+++ libcontainer/specconv/spec_linux.go
@@ -1006,9 +1006,9 @@ func setupUserNamespace(spec *specs.Spec, config *configs.Config) error {
        if path := config.Namespaces.PathOf(configs.NEWUSER); path != "" {
                // We cannot allow uid or gid mappings to be set if we are also asked
                // to join a userns.
-               if config.UIDMappings != nil || config.GIDMappings != nil {
-                       return errors.New("user namespaces enabled, but both namespace path and mapping specified -- you may only provide one")
-               }
+               //if config.UIDMappings != nil || config.GIDMappings != nil {
+               //      return errors.New("user namespaces enabled, but both namespace path and mapping specified -- you may only provide one")
+               //}
                // Cache the current userns mappings in our configuration, so that we
                // can calculate uid and gid mappings within runc. These mappings are
                // never used for configuring the container if the path is set.

Those lines are not part of this PR, though.

CRIO triggers the very same error too.

I think we should change that to a warning, change CRIO, containerd (and maybe more tools), and change it back to an error in a few releases. I haven't checked what is done today.

I think both (containerd and CRIO) when sending both, the path and the mappings, those are consistent (i.e. the path's mappings and the mappings in the config.json are the same). In that case, we can just print a warning here saying we will ignore one (probably the mappings and just use the path) would be safe.

Originally posted by @rata in #3985 (review)

Due to the invalid config.jsons being passed by containerd and crio (possibly among others), we have to downgrade the relevant error added in 09822c3 to a warning if the mappings match the passed path.

[rata]

@cyphar AFAIK runc behavior is at fault here too. runc before that commit (i.e. all releases) I think the only way to join a userns path was if you ALSO specified the mappings. So tools are using that way and now we want to change it, which creates issues, of course.

[cyphar]

Yeah, you're right. I forgot that I fixed this in that PR 😅.

I've suggested some of the text in opencontainers/runtime-spec#1237 be reworded and a reference to this incorrect behaviour be added to help clarify things for folks who run into this in the future.