Fail unpacking images with xattrs to filesystems without xattr support#45464
Fail unpacking images with xattrs to filesystems without xattr support#45464neersighted merged 2 commits intomoby:masterfrom
Conversation
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
we discussed this, and because vfs is also a bit of a "last resort", we'd like to provide an escape-hatch for this case ("I_KNOW_WHAT_IM_DOING_GET_ME_OUT_OF_HERE")
|
Also for "future self" (and other visitors); we initially had a concern "what happens if I pull an image, and it's extracted to a filesystem without xattr support (so extended attributes get discarded), and after that I would push that image? |
corhere
force-pushed
the
xattrs-matter-in-images
branch
2 times, most recently
from
0252fef to
972afeb
Compare
tianon
left a comment
tianon
left a comment
There was a problem hiding this comment.
👀 (looks good minus the naming comment Seb just made)
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
"LGTM" after changing the vfs prefix, and perhaps a logrus.Warn if the setting set.
Signed-off-by: Cory Snider <[email protected]>
Extended attributes are set on files in container images for a reason. Fail to unpack if extended attributes are present in a layer and setting the attributes on the unpacked files fails for any reason. Add an option to the vfs graph driver to opt into the old behaviour where ENOTSUPP and EPERM errors encountered when setting extended attributes are ignored. Make it abundantly clear to users and anyone triaging their bug reports that they are shooting themselves in the foot by enabling this option. Signed-off-by: Cory Snider <[email protected]>
corhere
force-pushed
the
xattrs-matter-in-images
branch
from
972afeb to
6690d29
Compare
|
Note for reviewers: all system-info warnings are logged by the daemon during startup in addition to being included in the Lines 1136 to 1139 in ccd834e |
neersighted
left a comment
neersighted
left a comment
There was a problem hiding this comment.
LGTM, and the warning language seems to strike a good balance now.
neersighted
added
status/2-code-review
impact/deprecation
and removed
status/1-design-review
labels
|
Test failure looks related |
|
...and that was the wrong button (I was trying to click 'Show all checks' but my brain wasn't quite there yet) -- @corhere, could you figure out the test issue in a follow-up? |
6 participants
- What I did
Following up #45420 (comment), I made it a fatal error to untar a file with extended attributes to a destination which does not support extended attributes. I added an escape hatch to the
vfsgraph driver to allow users to opt back into the old, broken behaviour.- How I did it
I made ignoring
EPERMandENOTSUPPerrors fromlsetxattrwhen extracting a file from a tarball conditional on a newBestEffortXattrsflag inarchive.TarOptionsand plumbed the flag through to thevfsdriver. I added a new storage optionvfs.xattrs=i_want_broken_containersto the driver to set the flag. A warning is logged at daemon startup and indocker infooutput if this storage option is enabled.- How to verify it
With the new unit test
- Description for the changelog
vfsdriver is a notable exception; the previous behaviour can be restored by configuring it with--storage-opt vfs.xattrs=i_want_broken_containers. Enabling this storage option is highly discouraged and bug reports submitted against a daemon with this option set will be summarily closed without comment.- A picture of a cute animal (not mandatory but encouraged)