Merge pull request #734 from giuseppe/fix-map-handling

storage: fix UID/GID map handling for shifting layers

Author: mheon

storage/cmd/containers-storage/create.go

@@ -122,7 +122,12 @@ func createLayer(flags *mflag.FlagSet, action string, m storage.Store, args []st
 	if err != nil {
 		return 1, err
 	}
-	options := &storage.LayerOptions{IDMappingOptions: *mappings}
+	options := &storage.LayerOptions{IDMappingOptions: storage.LayerIDMappingOptions{
+		HostUIDMapping: mappings.HostUIDMapping,
+		HostGIDMapping: mappings.HostGIDMapping,
+		UIDMap:         mappings.UIDMap,
+		GIDMap:         mappings.GIDMap,
+	}}
 	layer, err := m.CreateLayer(paramID, parent, paramNames, paramMountLabel, !paramCreateRO, options)
 	if err != nil {
 		return 1, err
@@ -155,7 +160,12 @@ func importLayer(flags *mflag.FlagSet, action string, m storage.Store, args []st
 	if err != nil {
 		return 1, err
 	}
-	options := &storage.LayerOptions{IDMappingOptions: *mappings}
+	options := &storage.LayerOptions{IDMappingOptions: storage.LayerIDMappingOptions{
+		HostUIDMapping: mappings.HostUIDMapping,
+		HostGIDMapping: mappings.HostGIDMapping,
+		UIDMap:         mappings.UIDMap,
+		GIDMap:         mappings.GIDMap,
+	}}
 	layer, _, err := m.PutLayer(paramID, parent, paramNames, paramMountLabel, !paramCreateRO, options, diffStream)
 	if err != nil {
 		return 1, err

storage/containers.go

@@ -76,9 +76,12 @@ type Container struct {
 	// is set before using it.
 	Created time.Time `json:"created"`
 
-	// UIDMap and GIDMap are used for setting up a container's root
-	// filesystem for use inside of a user namespace where UID mapping is
-	// being used.
+	// UIDMap and GIDMap are the caller's requested UID/GID mapping for this
+	// container's user namespace.  They always reflect what the caller
+	// asked for, regardless of whether the mapping was applied at layer
+	// creation time (chown) or is deferred to mount time (idmapped mounts).
+	// At mount time, these maps are passed to the graph driver so that the
+	// container sees the expected file ownership.
 	UIDMap []idtools.IDMap `json:"uidmap,omitempty"`
 	GIDMap []idtools.IDMap `json:"gidmap,omitempty"`
 

storage/drivers/driver.go

@@ -95,9 +95,6 @@ type MountOpts struct {
 	// Volatile specifies whether the container storage can be optimized
 	// at the cost of not syncing all the dirty files in memory.
 	Volatile bool
-
-	// DisableShifting forces the driver to not do any ID shifting at runtime.
-	DisableShifting bool
 }
 
 // ApplyDiffOpts contains optional arguments for ApplyDiff methods.

storage/drivers/overlay/overlay.go

@@ -1504,7 +1504,7 @@ func (d *Driver) get(id string, disableShifting bool, options graphdriver.MountO
 
 	readWrite := !inAdditionalStore
 
-	if !d.SupportsShifting(options.UidMaps, options.GidMaps) || options.DisableShifting {
+	if !d.SupportsShifting(options.UidMaps, options.GidMaps) {
 		disableShifting = true
 	}
 

storage/layers.go

@@ -167,8 +167,11 @@ type Layer struct {
 	// Flags is arbitrary data about the layer.
 	Flags map[string]any `json:"flags,omitempty"`
 
-	// UIDMap and GIDMap are used for setting up a layer's contents
-	// for use inside of a user namespace where UID mapping is being used.
+	// UIDMap and GIDMap are the on-disk ID mappings for this layer: the
+	// chown mapping that was applied to the layer's files at creation
+	// time.  When the driver supports shifting (idmapped mounts), no
+	// chown occurs and these fields are empty.  The caller's requested
+	// mapping is applied at mount time instead (see Container.UIDMap/GIDMap).
 	UIDMap []idtools.IDMap `json:"uidmap,omitempty"`
 	GIDMap []idtools.IDMap `json:"gidmap,omitempty"`
 
@@ -1600,8 +1603,8 @@ func (r *layerStore) create(id string, parentLayer *Layer, names []string, mount
 		UIDs:               templateUIDs,
 		GIDs:               templateGIDs,
 		Flags:              newMapFrom(moreOptions.Flags),
-		UIDMap:             copySlicePreferringNil(moreOptions.UIDMap),
-		GIDMap:             copySlicePreferringNil(moreOptions.GIDMap),
+		UIDMap:             copySlicePreferringNil(moreOptions.IDMappingOptions.UIDMap),
+		GIDMap:             copySlicePreferringNil(moreOptions.IDMappingOptions.GIDMap),
 		BigDataNames:       []string{},
 		location:           r.pickStoreLocation(moreOptions.Volatile, writeable),
 	}
@@ -1645,7 +1648,10 @@ func (r *layerStore) create(id string, parentLayer *Layer, names []string, mount
 		}
 	}
 
-	idMappings := idtools.NewIDMappingsFromMaps(moreOptions.UIDMap, moreOptions.GIDMap)
+	idMappings := idtools.NewIDMappingsFromMaps(moreOptions.IDMappingOptions.UIDMap, moreOptions.IDMappingOptions.GIDMap)
+	if moreOptions.IDMappingOptions.HostUIDMapping && moreOptions.IDMappingOptions.HostGIDMapping {
+		idMappings = &idtools.IDMappings{}
+	}
 	opts := drivers.CreateOpts{
 		MountLabel: mountLabel,
 		StorageOpt: options,
@@ -2601,7 +2607,7 @@ func (r *layerStore) stageWithUnlockedStore(sl *maybeStagedLayerExtraction, pare
 	result, err := applyDiff(layerOptions, sl.diff, f, func(payload io.Reader) (int64, error) {
 		cleanup, stagedLayer, size, err := sl.staging.StartStagingDiffToApply(parent, drivers.ApplyDiffOpts{
 			Diff:     payload,
-			Mappings: idtools.NewIDMappingsFromMaps(layerOptions.UIDMap, layerOptions.GIDMap),
+			Mappings: idtools.NewIDMappingsFromMaps(layerOptions.IDMappingOptions.UIDMap, layerOptions.IDMappingOptions.GIDMap),
 			// MountLabel is not supported for the unlocked extraction, see the comment in (*store).PutLayer()
 			MountLabel: "",
 		})

storage/store.go

@@ -633,13 +633,39 @@ type AutoUserNsOptions = types.AutoUserNsOptions
 
 type IDMappingOptions = types.IDMappingOptions
 
+// LayerIDMappingOptions are the on-disk ID mappings for a layer.
+//
+// Unlike the caller-facing IDMappingOptions (which expresses what mapping
+// the caller wants), these record how files are actually stored.  The two
+// may differ: when the graph driver supports shifting, no chown
+// occurs so HostUIDMapping/HostGIDMapping are true and UIDMap/GIDMap
+// are empty, even though the caller requested a non-trivial mapping.
+// The caller's requested mapping is still honored at mount time via
+// the Container's UIDMap/GIDMap.
+type LayerIDMappingOptions struct {
+	// HostUIDMapping is true when files in this layer are stored with host
+	// UIDs.
+	HostUIDMapping bool
+	// HostGIDMapping is true when files in this layer are stored with host
+	// GIDs.  See HostUIDMapping for details.
+	HostGIDMapping bool
+	// UIDMap is the on-disk UID mapping: it records the chown that was
+	// applied to the layer's files at creation time.  Empty when
+	// HostUIDMapping is true.
+	UIDMap []idtools.IDMap
+	// GIDMap is the on-disk GID mapping: it records the chown that was
+	// applied to the layer's files at creation time.  Empty when
+	// HostGIDMapping is true.
+	GIDMap []idtools.IDMap
+}
+
 // LayerOptions is used for passing options to a Store's CreateLayer() and PutLayer() methods.
 type LayerOptions struct {
 	// IDMappingOptions specifies the type of ID mapping which should be
 	// used for this layer.  If nothing is specified, the layer will
 	// inherit settings from its parent layer or, if it has no parent
 	// layer, the Store object.
-	types.IDMappingOptions
+	IDMappingOptions LayerIDMappingOptions
 	// TemplateLayer is the ID of a layer whose contents will be used to
 	// initialize this layer.  If set, it should be a child of the layer
 	// which we want to use as the parent of the new layer.
@@ -708,10 +734,18 @@ type ImageBigDataOption struct {
 
 // ContainerOptions is used for passing options to a Store's CreateContainer() method.
 type ContainerOptions struct {
-	// IDMappingOptions specifies the type of ID mapping which should be
-	// used for this container's layer.  If nothing is specified, the
-	// container's layer will inherit settings from the image's top layer
-	// or, if it is not being created based on an image, the Store object.
+	// IDMappingOptions specifies the caller's desired ID mapping for the
+	// container's user namespace.
+	//
+	// These express what the caller wants, not what ends up on disk.
+	// The store records them in the Container and uses them at mount
+	// time.  How the layer's files are stored depends on whether the
+	// driver supports shifting: if it does, no chown occurs and the
+	// mapping is applied at mount time; otherwise files are chowned at
+	// layer creation time.
+	//
+	// If nothing is specified, mappings are inherited from the image's top
+	// layer or, if no image, from the Store's defaults.
 	types.IDMappingOptions
 	LabelOpts []string
 	// Flags is a set of named flags and their values to store with the container.
@@ -1518,14 +1552,14 @@ func populateLayerOptions(s *store, rlstore rwLayerStore, rlstores []roLayerStor
 		options.BigData = slices.Clone(lOptions.BigData)
 		options.Flags = copyMapPreferringNil(lOptions.Flags)
 	}
-	if options.HostUIDMapping {
-		options.UIDMap = nil
+	if options.IDMappingOptions.HostUIDMapping {
+		options.IDMappingOptions.UIDMap = nil
 	}
-	if options.HostGIDMapping {
-		options.GIDMap = nil
+	if options.IDMappingOptions.HostGIDMapping {
+		options.IDMappingOptions.GIDMap = nil
 	}
-	uidMap := options.UIDMap
-	gidMap := options.GIDMap
+	uidMap := options.IDMappingOptions.UIDMap
+	gidMap := options.IDMappingOptions.GIDMap
 	if parent != "" {
 		var err error
 		parentLayer, unlock, err = getParentLayer(rlstore, rlstores, parent)
@@ -1546,26 +1580,26 @@ func populateLayerOptions(s *store, rlstore rwLayerStore, rlstores []roLayerStor
 				return nil, nil, unlock, ErrParentIsContainer
 			}
 		}
-		if !options.HostUIDMapping && len(options.UIDMap) == 0 {
+		if !options.IDMappingOptions.HostUIDMapping && len(options.IDMappingOptions.UIDMap) == 0 {
 			uidMap = parentLayer.UIDMap
 		}
-		if !options.HostGIDMapping && len(options.GIDMap) == 0 {
+		if !options.IDMappingOptions.HostGIDMapping && len(options.IDMappingOptions.GIDMap) == 0 {
 			gidMap = parentLayer.GIDMap
 		}
 	} else {
-		if !options.HostUIDMapping && len(options.UIDMap) == 0 {
+		if !options.IDMappingOptions.HostUIDMapping && len(options.IDMappingOptions.UIDMap) == 0 {
 			uidMap = s.uidMap
 		}
-		if !options.HostGIDMapping && len(options.GIDMap) == 0 {
+		if !options.IDMappingOptions.HostGIDMapping && len(options.IDMappingOptions.GIDMap) == 0 {
 			gidMap = s.gidMap
 		}
 	}
 	if s.canUseShifting(uidMap, gidMap) {
-		options.IDMappingOptions = types.IDMappingOptions{HostUIDMapping: true, HostGIDMapping: true, UIDMap: nil, GIDMap: nil}
+		options.IDMappingOptions = LayerIDMappingOptions{HostUIDMapping: true, HostGIDMapping: true, UIDMap: nil, GIDMap: nil}
 	} else {
-		options.IDMappingOptions = types.IDMappingOptions{
-			HostUIDMapping: options.HostUIDMapping,
-			HostGIDMapping: options.HostGIDMapping,
+		options.IDMappingOptions = LayerIDMappingOptions{
+			HostUIDMapping: options.IDMappingOptions.HostUIDMapping,
+			HostGIDMapping: options.IDMappingOptions.HostGIDMapping,
 			UIDMap:         copySlicePreferringNil(uidMap),
 			GIDMap:         copySlicePreferringNil(gidMap),
 		}
@@ -1856,14 +1890,14 @@ func (s *store) imageTopLayerForMapping(image *Image, ristore roImageStore, rlst
 	// mappings, and register it as an alternate top layer in the image.
 	var layerOptions LayerOptions
 	if s.canUseShifting(options.UIDMap, options.GIDMap) {
-		layerOptions.IDMappingOptions = types.IDMappingOptions{
+		layerOptions.IDMappingOptions = LayerIDMappingOptions{
 			HostUIDMapping: true,
 			HostGIDMapping: true,
 			UIDMap:         nil,
 			GIDMap:         nil,
 		}
 	} else {
-		layerOptions.IDMappingOptions = types.IDMappingOptions{
+		layerOptions.IDMappingOptions = LayerIDMappingOptions{
 			HostUIDMapping: options.HostUIDMapping,
 			HostGIDMapping: options.HostGIDMapping,
 			UIDMap:         copySlicePreferringNil(options.UIDMap),
@@ -2008,20 +2042,12 @@ func (s *store) CreateContainer(id string, names []string, image, layer, metadat
 		// But in transient store mode, all container layers are volatile.
 		Volatile: options.Volatile || s.transientStore,
 	}
-	if s.canUseShifting(uidMap, gidMap) {
-		layerOptions.IDMappingOptions = types.IDMappingOptions{
-			HostUIDMapping: true,
-			HostGIDMapping: true,
-			UIDMap:         nil,
-			GIDMap:         nil,
-		}
-	} else {
-		layerOptions.IDMappingOptions = types.IDMappingOptions{
-			HostUIDMapping: idMappingsOptions.HostUIDMapping,
-			HostGIDMapping: idMappingsOptions.HostGIDMapping,
-			UIDMap:         copySlicePreferringNil(uidMap),
-			GIDMap:         copySlicePreferringNil(gidMap),
-		}
+	useHostMapping := idMappingsOptions.HostUIDMapping || s.canUseShifting(uidMap, gidMap)
+	layerOptions.IDMappingOptions = LayerIDMappingOptions{
+		HostUIDMapping: useHostMapping,
+		HostGIDMapping: useHostMapping,
+		UIDMap:         copySlicePreferringNil(uidMap),
+		GIDMap:         copySlicePreferringNil(gidMap),
 	}
 	if options.Flags == nil {
 		options.Flags = make(map[string]any)
@@ -3074,10 +3100,6 @@ func (s *store) Mount(id, mountLabel string) (string, error) {
 	if err != nil {
 		return "", err
 	}
-	if options.UidMaps != nil || options.GidMaps != nil {
-		options.DisableShifting = !s.canUseShifting(options.UidMaps, options.GidMaps)
-	}
-
 	if err := rlstore.startWriting(); err != nil {
 		return "", err
 	}

storage/tests/helpers.bash

@@ -21,9 +21,6 @@ function setup() {
 	TESTDIR=${BATS_TMPDIR}/tmp.${suffix}
 	rm -fr ${TESTDIR}
 	mkdir -p ${TESTDIR}/{root,runroot}
-        # disable idmapped mounts in the overlay driver, since that
-        # is the expectation in the idmaps.bats tests.
-        export _CONTAINERS_OVERLAY_DISABLE_IDMAP=yes
 }
 
 # Teardown the basic storage setup