-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Support subpath paths for -v or --mount #20661
Comments
The trend is to require the use of --mount rather then --volume, for ultimate flexibility and this seems reasonable. |
I recall someone had security concerns with this when it was originally
proposed - maybe Giuseppe?
…On Sun, Nov 12, 2023 at 08:23 Daniel J Walsh ***@***.***> wrote:
The trend is to require the use of --mount rather then --volume, for
ultimate flexibility and this seems reasonable.
WDYT @vrothberg <https://github.com/vrothberg> @mheon
<https://github.com/mheon> ?
—
Reply to this email directly, view it on GitHub
<#20661 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB3AOCGSNPOJFXVWTS6WOHDYEDEUNAVCNFSM6AAAAAA7HVODDCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBXGEZDONJWGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
yes, it was not implemented safely originally. If we open the mount first as we already do for the kube path, then it is safe as another container cannot create a malicious symlink to escape it. |
It looks like Docker has just added this feature in v26.0.0; there is now a
|
Ran head first into this today. My I think this is another important concern to address, because if it's implemented without special attention to and handling of backward compatibility, the effective file layout within the volume will become inconsistent with actual data. As a consequence of this, at best, users of compose files may find their data "gone" after an update (because it now mounts an empty subpath); and at worst, the container may overwrite existing files at incorrect locations, causing a data corruption. Please correct me if I'm wrong (and I really hope I'm wrong), but I don't think there is a way to retroactively sort existing data (which is often a mixed blend of data from multiple containers) into subpaths cleanly. So I suppose this would have to be a breaking change with the appropriate warnings put in place. |
I have the same issue as @cyqsimon. I'm building a framework that I want podman and docker users to be able to make use of. My compose files work beautifully in both except that subpath is silently ignored when using podman. I don't have a good workaround to this. I'll probably have to create multiple volumes and then somehow track them all in the consumer. |
Re the breaking change point. For me there are file name clashes so I'm already broken in podman. If that were not the case then I may never have noticed the discrepancy and would be prone to data loss if podman is updated to support sub path. Therefore I suggest a config option to enable the feature, defaulting to 'legacy', i.e. ignore sub-path in compose.yml. |
as a workaround to podman's inability to use sub_path volume mounts see containers/podman#20661
All the backend work was done a while back for image volumes, so this is effectively just plumbing the option in for volumes in the parser logic. It's a little uglier than I'd like because our volume mount parser produces an OCI Spec mount struct, which doesn't have a field for subpath, so we have to pass through the options array and parse it back out after, which is a little gross. But not gross enough to rewrite that parsing logic. Fixes containers#20661 Signed-off-by: Matt Heon <[email protected]>
All the backend work was done a while back for image volumes, so this is effectively just plumbing the option in for volumes in the parser logic. It's a little uglier than I'd like because our volume mount parser produces an OCI Spec mount struct, which doesn't have a field for subpath, so we have to pass through the options array and parse it back out after, which is a little gross. But not gross enough to rewrite that parsing logic. Fixes containers#20661 Signed-off-by: Matt Heon <[email protected]>
All the backend work was done a while back for image volumes, so this is effectively just plumbing the option in for volumes in the parser logic. It's a little uglier than I'd like because our volume mount parser produces an OCI Spec mount struct, which doesn't have a field for subpath, so we have to pass through the options array and parse it back out after, which is a little gross. But not gross enough to rewrite that parsing logic. Fixes containers#20661 Signed-off-by: Matt Heon <[email protected]>
All the backend work was done a while back for image volumes, so this is effectively just plumbing the option in for volumes in the parser logic. We do need to change the return type of the volume parser as it only worked on spec.Mount before (which does not have subpath support, so we'd have to pass it as an option and parse it again) but that is cleaner than the alternative. Fixes containers#20661 Signed-off-by: Matt Heon <[email protected]>
Feature request description
I'm glad that support for subpath mounts in pod manifests was added in #16803. I'd like to see the same capability exposed via
podman run
, either by extending the syntax of--volume
, so that we can write:Or by adding an additional
subpath
keyword to--mount
, so that we can write:Suggest potential solution
I think the reasons for wanting to do this are covered in #12929. I just want to be able to do it from the command line because in many situations a simple
podman run ...
invocation is both faster and more effective than setting up a pod manifest.I think adding the
subpath
keyword to--mount
would be the cleanest and least disruptive way of implementing this feature.Have you considered any alternatives?
While it's possible to do this using a pod manifest, the behavior of
podman kube play
is different from the behavior ofpodman run
; in particular, it's not really designed for running a container interactively from the command line.Additional context
Add any other context or screenshots about the feature request here.
The text was updated successfully, but these errors were encountered: