archive: honor ignoreChownErrors with ForceMode

[giuseppe]

when ignore_chown_errors is set, ignore also failures from setting the override mode xattr.

Closes: #2174

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [giuseppe]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[rhatdan]

LGTM

[giuseppe]

@mtrmac PTAL

[mtrmac]

I don’t see why this makes sense.

• Just formally, this PR is underdocumented:
  • ignore_chown_errors is documented to only affect file ownership. Nothing says it interacts with ContainersOverrideXattr, and it hasn’t behaved that way so far.
  • ContainersOverrideXattr is documented for the force_mask option, and nothing says that ignore_chown_errors should affect that behavior.
• What is the use case? With force_mask, AFAICS ContainersOverrideXattr must be set for the container to behave as expected. The documentation explicitly talks about sharing stores with other users, or using private stores by unprivileged users — in both cases I think the users must be told if setting ContainersOverrideXattr fails, at least by default.
  • I can imagine a hypothetical option that means “I don’t care about any attributes at all, don’t set UIDs, don’t set modes, don’t bother with the extended attribute either” … but, what for? It isn’t useful for running containers in general (ownership and permissions matter), it isn’t necessary for specially-restricted containers that don’t care (they can be specially built to only be root:root 700:700). My best guess is that this would be useful for some kind of “inspect the filesystem” operation, but then that can be done by extracting the tarball without any privileges at all, and possibly without writing to the filesystem, so I don’t think an extra option is warranted.
• Investigating further, per Setting shared xattr fails on images with named pipes #2174 (comment) , this has nothing to do with ignore_chown_errors: setting user.… extended attributes on devices/pipes/symlinks never works, even if the user has CAP_SYS_ADMIN. This is a fundamental limitation of the current force_mask design. I don’t know what we want to do here.
  • Hypothetically, we could have an option that means “I want to use force_mask with images that contain devices/pipes and I’m fine with those objects having incorrect ownership and permissions”. That option has different meaning from ignore_chown_errors, so it should be a new option — but, anyway, I don’t know why anyone should set this option. Users who have these objects in their container filesystems, presumably, need them; if not, they can always build (squashed?) images with the objects removed.

[giuseppe]

from an user PoV, with ignore_chown_errors the expectation is that the files inside a container/image might not have the correct ownership. Since the container might behave differently in this case, we opted for an explicit setting instead of doing it transparently. For example, ignore_chown_errors is useful when the user has not enough IDs allocated, and the end result is that all the files are owned only by a subset of IDs in the image, depending how many IDs are available.

That situation is similar to using force_mask and failing to set the ContainersOverrideXattr. The end result is that the files inside the container have the wrong ownership.

Ideally, images should not have special files, but still there are images using them and we need to handle them in some way, I don't think this should be done transparently but must be an explicit setting. Sure we could add yet another option to make it clearer, but the semantic would be identical and it will just cover what seems to be a corner case.

[mtrmac]

from an user PoV, with ignore_chown_errors the expectation is that the files inside a container/image might not have the correct ownership. Since the container might behave differently in this case, we opted for an explicit setting instead of doing it transparently.

Hum, having ignore_chown_errors effectively mean “I am opting in into not representing the image correctly” (and include not just ownership, but permissions) does make some sense, as long as it is documented that way.

But #2174 (comment) seems much nicer to me — that makes things work correctly and then we won’t have to worry users about choosing between non-ideal options.

[giuseppe]

closed in favor of: #2183

I am still working on it, but opened to get some early feedback

[giuseppe]

#2183 is ready for review