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

features: mountExtensions: how best to represent feature support for idmap? #1223

Open
cyphar opened this issue Aug 23, 2023 · 5 comments
Open

Comments

@cyphar
Copy link
Member

cyphar commented Aug 23, 2023

I don't think a boolean is sufficient to describe the state of idmapped mount support.

At the moment, runc only supports this for one specific case (which will be fixed by opencontainers/runc#3985). Once we fix this, runc will only support these flags for bind mounts. However, the specification allows you to set these fields for any mount, which runc doesn't currently support (because it would require reworking a lot of our mount infrastructure). crun also only supports idmappings for bind-mounts at the moment, and youki doesn't yet support idmappings.

I don't know if there is a nice way to describe all of these cases. You could have a string saying which filesystem types are supported, but bind-mounts are not treated as a filesystem type by the spec... We can probably ignore the way runc currently supports idmapped mounts because I suspect nobody is going to release a runtime with the feature that is that restricted.

Originally posted by @cyphar in #1219 (comment)

@cyphar
Copy link
Member Author

cyphar commented Aug 23, 2023

Maybe this could be an object like:

"mountExtensions": {
  "idmap": {
    "modes": ["sameAsUserns", "arbitrary"],
    "filesystemTypes": ["ext4", "tmpfs", "foobarfs"],
    "supportsBind": true
  }
}

wdyt?

Also we need to figure out how to deal with recursive mounts -- see #1216. If we add a separate flag for this, we will need to add an entry to this struct.

Originally posted by @cyphar in #1219 (comment)

@cyphar
Copy link
Member Author

cyphar commented Aug 23, 2023

I think having a list of supported (or explicitly unsupported) filesystems would be a good idea. Now that I think about it, since runc will not release a version with restricted mapping support, it's probably not necessary to have a field for whether we support arbitrary mappings or not. I think it's fair to say the spec implies arbitrary mapping support is a requirement.

@rata
Copy link
Member

rata commented Aug 24, 2023

The parsing of the filesystems at runtime (besides I don't know how you plan to detect idmap mount support at runtime) seems not to be what we should do:

The Features structure is irrelevant to the actual availability of the features in the host operating system. Hence, the content of the Features structure SHOULD be determined on the compilation time of the runtime, not on the execution time.

@cyphar
Copy link
Member Author

cyphar commented Aug 27, 2023

besides I don't know how you plan to detect idmap mount support at runtime

That isn't my proposal -- the plan is to provide a list of filesystems that may work (or we can do it your way -- make a list that definitely don't work). If we want to return a list of filesystems that are definitely not supported (your suggestion), it will be necessary to copy the output of /proc/filesystems for container runtimes that only support bind-mounts (which is currently all OCI runtimes that support is mappings).

@rata
Copy link
Member

rata commented Aug 28, 2023

@cyphar

That isn't my proposal -- the plan is to provide a list of filesystems that may work

Then it won't remove the trial and error that was your use case.

If we want to return a list of filesystems that are definitely not supported (your suggestion)

I think that is cleaner, but I don't see any real use case for any of this.

The only thing I can think of is, if k8s wants to enable userns by default (I'd love to do that), it might be useful to know which things won't work. But not even in this case, as k8s only sets bind-mounts (and I don't think that will change soon), so this is probably not needed in that case either.

it will be necessary to copy the output of /proc/filesystems for container runtimes that only support bind-mounts

Hmm, why? I don't see it at all why this won't be just a list of fs we need to change the infrastructure to support idmapping them. Why would we parse that?

(which is currently all OCI runtimes that support is mappings).

crun in git now supports regular mounts too.

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

No branches or pull requests

2 participants