PR #1788: reverse default behavior for xattrs

bin/ch-completion.bash

@@ -114,7 +114,7 @@ _image_common_opts="-a --arch --always-download --auth --cache
                     --cache-large --dependencies -h --help
                     --no-cache --no-lock --no-xattrs --profile
                     --rebuild --password-many -q --quiet -s --storage
-                    --tls-no-verify -v --verbose --version"
+                    --tls-no-verify -v --verbose --version --xattrs"
 
 _image_subcommands="build build-cache delete gestalt
                     import list pull push reset undelete"

bin/ch-convert

@@ -473,6 +473,9 @@ chimage_out_validate () {
     if [ -d "$img" ] && [ -n "$no_clobber" ]; then
         FATAL "exists in ch-image storage, not deleting per --no-clobber: ${1}"
     fi
+    if [ -n "$xattrs" ]; then
+        WARNING "--xattrs unsupported for out format \"ch-image\""
+    fi
 }
 
 # Validate that $1 can be used as an input directory.
@@ -820,6 +823,14 @@ tmpdir_setup () {
     tmpdir=$(mktemp -d --tmpdir="$tmpdir" ch-convert.XXXXXX)
 }
 
+# Error out if “--xattrs” and “--no-xattrs” are specified in the same command
+# line.
+xattr_opt_err () {
+    if [ -n "$xattrs" ] && [ -n "$no_xattrs" ]; then
+        FATAL "\"--xattrs\" incompatible with \"--no-xattrs\""
+    fi
+}
+
 
 ## main ######################################################################
 
@@ -841,6 +852,7 @@ while true; do
                 ;;
             --no-xattrs)
                 no_xattrs=yes
+                xattr_opt_err
                 ;;
             -o|--out-fmt)
                 shift
@@ -857,6 +869,10 @@ while true; do
                 shift
                 tmpdir=$1
                 ;;
+            --xattrs)
+                xattrs=yes
+                xattr_opt_err
+                ;;
             *)
                 break
                 ;;
@@ -867,12 +883,15 @@ done
 if [ "$#" -ne 2 ]; then
     usage
 fi
-if [ -n "$no_xattrs" ]; then
-    tar_xattr_args=
-    squash_xattr_arg=
-else
+# This bizarre bit of syntax comes from https://unix.stackexchange.com/a/28782
+if [ -n "$xattrs" ] || { [ -n "$CH_XATTRS" ] && [ -z "$no_xattrs" ]; }; then
+    echo "preserving xattrs..."
     tar_xattr_args='--xattrs-include=user.* --xattrs-include=system.*'
     squash_xattr_arg=-xattrs
+else
+    echo "discarding xattrs..."
+    tar_xattr_args=
+    squash_xattr_arg=
 fi
 in_desc=$1
 out_desc=$2

bin/ch-image.py.in

@@ -124,7 +124,7 @@ def main():
               "help": "allow concurrent storage directory access (risky!)" }],
            [["--no-xattrs"],
             { "action": "store_true",
-              "help": "build cache ignores xattrs and ACLs"}],
+              "help": "disable xattrs and ACLs (overrides $CH_XATTRS)" }],
            [["--password-many"],
             { "action": "store_true",
               "help": "re-prompt each time a registry password is needed" }],
@@ -148,7 +148,10 @@ def main():
               "help": "print extra chatter (can be repeated)" }],
            [["--version"],
             { "action": misc.Version,
-              "help": "print version and exit" }] ] }
+              "help": "print version and exit" }],
+           [["--xattrs"],
+            { "action": "store_true",
+              "help": "enable build cache support for xattrs and ACLs"}] ] }
 
    # Most, but not all, subcommands need to check dependencies before doing
    # anything (the exceptions being basic information commands like

bin/ch-run-oci.py.in

@@ -90,12 +90,13 @@ def args_parse():
    args_.func = abs  # needs to have __module__ attribute
    args_.no_cache = None
    args_.no_lock = False
+   args_.no_xattrs = False
    args_.password_many = False
    args_.profile = False
    args_.quiet = False
    args_.storage = None
    args_.tls_no_verify = False
-   args_.no_xattrs = False
+   args_.xattrs = False
    ch.init(args_)
 
    if len(sys.argv) < 2:

bin/ch-test

@@ -17,6 +17,9 @@ fi
 # Set environment variable for ch-image --debug flag
 export CH_IMAGE_DEBUG=true
 
+# Set environment variable for ch-convert and ch-image --xattrs flag
+export CH_XATTRS=true
+
 ### Functions we need right away
 
 fatal () {

doc/ch-convert.rst

@@ -50,7 +50,7 @@ producing the final format actually needed.
     Error if :code:`OUT` already exists, rather than replacing it.
 
   :code:`--no-xattrs`
-    Ignore xattrs and ACLs when converting.
+    Ignore xattrs and ACLs when converting. Overrides :code:`$CH_XATTRS`.
 
   :code:`-o`, :code:`--out-fmt FMT`
     Output image format is :code:`FMT`; inferred if omitted.
@@ -74,6 +74,9 @@ producing the final format actually needed.
   :code:`-v`, :code:`--verbose`
     Print extra chatter. Can be repeated.
 
+  :code:`--xattrs`
+    Preserve xattrs and ACLs when converting.
+
 .. Notes:
 
    1. It’s a deliberate choice to use UNIXey options rather than the Skopeo

doc/ch-image.rst

@@ -93,7 +93,9 @@ Common options placed before or after the sub-command:
     which risks corruption but may be OK for some workloads.
 
   :code:`--no-xattrs`
-    Ignore xattrs and ACLs.
+    Enforce default handling of xattrs, i.e. do not save them in the build cache
+    or restore them on rebuild. This is the default, but the option is provided
+    to override the :code:`$CH_XATTRS` environment variable.
 
   :code:`--password-many`
     Re-prompt the user every time a registry password is needed.
@@ -126,6 +128,10 @@ Common options placed before or after the sub-command:
     Print extra chatter; can be repeated. See the :ref:`FAQ entry on verbosity
     <faq_verbosity>` for details.
 
+  :code:`--xattrs`
+    Save xattrs and ACLs in the build cache, and restore them when rebuilding
+    from the cache.
+
 
 Architecture
 ============
@@ -313,11 +319,14 @@ repositories within the image should work. **Important exception**: No files
 named :code:`.git*` or other Git metadata are permitted in the image’s root
 directory.
 
-`Extended attributes <https://man7.org/linux/man-pages/man7/xattr.7.html>`_ (xattrs)
-belonging to unprivileged namespaces (e.g. :code:`user`) are also saved and
-restored by the cache by default. Notably, extended attributes in privileged
-namespaces (e.g. :code:`trusted`) cannot be read by :code:`ch-image` and will be
-lost without warning.
+`Extended attributes <https://man7.org/linux/man-pages/man7/xattr.7.html>`_
+(xattrs) are ignored by the build cache by default. Cache support for xattrs
+belonging to unprivileged xattr namespaces (e.g. :code:`user`) can be enabled by
+specifying the :code:`--xattrs` option or by setting the :code:`CH_XATTRS`
+environment variable. If :code:`CH_XATTRS` is set, you override it with
+:code:`--no-xattrs`. **Note that extended attributes in privileged xattr
+namespaces (e.g. :code:`trusted`) cannot be read by :code:`ch-image` and will
+always be lost without warning.**
 
 The cache has three modes: *enabled*, *disabled*, and a hybrid mode called
 *rebuild* where the cache is fully enabled for :code:`FROM` instructions, but

doc/faq.rst

@@ -1246,5 +1246,38 @@ Notes:
    :code:`RUN`) and sometimes it’s captured for internal use (e.g., many
    :code:`git(1)` invocations).
 
+.. _faq_xattrs:
+
+How do I handle extended attributes in Charliecloud?
+----------------------------------------------------
+
+As noted in section :ref:`ch-image_build-cache`, Charliecloud doesn’t support
+extended attributes (xattrs) by default. Support for xattrs  can be enabled for
+:code:`ch-image` and :code:`ch-convert` by specifying :code:`--xattrs` or
+setting :code:`$CH_XATTRS`. This will make :code:`ch-image` save and restore
+xattrs via the build cache, and will make :code:`ch-convert` preserve xattrs on
+conversion. Important caveats include:
+
+1. :code:`ch-image` and :code:`ch-convert` cannot read xattrs in privileged
+   namespaces (e.g. :code:`trusted` and :code:`security`). Extended attributes
+   in these namespaces will never be saved or restored via the cache, and will
+   never be preserved when converting between image formats.
+
+2. :code:`ch-image import` cannot handle xattrs. This is a limitation of the
+   Python `tarfile <https://docs.python.org/3/library/tarfile.html>`_ library,
+   which as of version 3.12.1 doesn’t support xattrs (see CPython issue `#113293
+   <https://github.com/python/cpython/issues/113293>`_).
+
+3. :code:`ch-convert -o ch-image` uses :code:`ch-image import` under the hood.
+   This in conjunction with (2) means that :code:`ch-convert` cannot preserve
+   xattrs when converting to the :code:`ch-image` format.
+
+4. :code:`ch-image pull` uses the tarfile library, so xattrs will be lost when
+   pulling from a registry.
+
+5. Support for xattrs varies among filesystems, e.g. tmpfs didn’t support xattrs
+   in the :code:`user` namespace prior to Linux kernel `upstream 6.6
+   <https://kernelnewbies.org/Linux_6.6#TMPFSe>`_ (Oct 2023).
+
 ..  LocalWords:  CAs SY Gutmann AUTH rHsFFqwwqh MrieaQ Za loc mpihello mvo du
 ..  LocalWords:  VirtualSize linuxcontainers jour uk lxd rwxr xr qq qqq

doc/py_env.rst

@@ -6,3 +6,7 @@
 
 :code:`CH_LOG_FESTOON`
   If set, prepend PID and timestamp to logged chatter.
+
+:code:`CH_XATTRS`
+  If set, save xattrs in the build cache and restore them when rebuilding from
+  the cache (equivalent to :code:`--xattrs`).

lib/build_cache.py

@@ -275,7 +275,7 @@ def __init__(self, image_root, path):
       self.hardlink_to = None
       self.large_name = None
       self.xattrs = dict()
-      if ch.save_xattrs:
+      if ch.xattrs_save:
          for xattr in ch.ossafe("can’t list xattrs: %s" % self.path_abs,
                                 os.listxattr, self.path_abs,
                                 follow_symlinks=False):

lib/charliecloud.py

@@ -103,6 +103,10 @@ def __lt__(self, other):
 ARCH_MAP_FALLBACK = { "arm/v7": ("arm",),
                       "arm64/v8": ("arm64",) }
 
+# Incompatible option pairs for the ch-image command line
+CLI_INCOMPATIBLE_OPTS = [("quiet", "verbose"),
+                         ("xattrs", "no_xattrs")]
+
 # String to use as hint when we throw an error that suggests a bug.
 BUG_REPORT_PLZ = "please report this bug: https://github.com/hpc/charliecloud/issues"
 
@@ -653,12 +657,17 @@ def exit(code):
 
 def init(cli):
    # logging
-   global log_festoon, log_fp, log_level, trace_fatal, save_xattrs
-   save_xattrs = (not cli.no_xattrs)
+   global log_festoon, log_fp, log_level, trace_fatal, xattrs_save
+   incomp_opts = 0
+   for (x,y) in CLI_INCOMPATIBLE_OPTS:
+      if (getattr(cli, x) and getattr(cli, y)):
+         ERROR("“--%s” incompatible with “--%s”" % ((x.replace("_","-"),
+                                                     y.replace("_","-"))))
+         incomp_opts += 1
+   if (incomp_opts > 0):
+      FATAL("%d incompatible option pair(s)" % incomp_opts)
+   xattrs_save = ((cli.xattrs) or (("CH_XATTRS" in os.environ) and (not cli.no_xattrs)))
    trace_fatal = (cli.debug or bool(os.environ.get("CH_IMAGE_DEBUG", False)))
-   if (cli.quiet and cli.verbose):
-      ERROR("“--quiet” incompatible with “--verbose”")
-      FATAL("incompatible option")
    log_level = Log_Level(cli.verbose - cli.quiet)
    assert (-3 <= log_level.value <= 3)
    if (log_level <= Log_Level.STDERR):
@@ -672,6 +681,7 @@ def init(cli):
    atexit.register(color_reset, log_fp)
    VERBOSE("version: %s" % version.VERSION)
    VERBOSE("verbose level: %d (%s))" % (log_level.value, log_level.name))
+   VERBOSE("save xattrs: %s" % str(xattrs_save))
    # signal handling
    signal.signal(signal.SIGINT, sigterm)
    signal.signal(signal.SIGTERM, sigterm)

lib/filesystem.py

@@ -952,18 +952,18 @@ def rmtree(self):
                   % (self, x.filename, x.strerror))
 
    def setxattr(self, name, value):
-      if (ch.save_xattrs):
+      if (ch.xattrs_save):
          try:
             os.setxattr(self, name, value, follow_symlinks=False)
          except OSError as x:
             if (x.errno == errno.ENOTSUP):  # no OSError subclass
                ch.WARNING("xattrs not supported on %s, setting --no-xattr"
                           % self.mountpoint())
-               ch.save_xattrs = False
+               ch.xattrs_save = False
             else:
                ch.FATAL("can’t set xattr: %s: %s: %s"
                         % (self, name, x.strerror))
-      if (not ch.save_xattrs):  # not “else” because maybe changed in “if”
+      if (not ch.xattrs_save):  # not “else” because maybe changed in “if”
          ch.DEBUG("xattrs disabled, ignoring: %s: %s" % (self, name))
          return
 

lib/misc.py

@@ -99,6 +99,8 @@ def gestalt_storage_path(cli):
 def import_(cli):
    if (not os.path.exists(cli.path)):
       ch.FATAL("can’t copy: not found: %s" % cli.path)
+   if (ch.xattrs_save):
+      ch.WARNING("--xattrs unsupported by “ch-image import” (see FAQ)")
    pathstr = im.Reference.ref_to_pathstr(cli.image_ref)
    if (cli.bucache == ch.Build_Mode.ENABLED):
       # Un-tag previously deleted branch, if it exists.

lib/pull.py

@@ -1,4 +1,5 @@
 import json
+import os
 import os.path
 
 import charliecloud as ch
@@ -28,6 +29,8 @@ def main(cli):
    if (cli.parse_only):
       print(src_ref.as_verbose_str)
       ch.exit(0)
+   if (ch.xattrs_save):
+      ch.WARNING("--xattrs unsupported for “ch-image pull” (see FAQ)")
    dst_img = im.Image(dst_ref)
    ch.INFO("pulling image:    %s" % src_ref)
    if (src_ref != dst_ref):