Merge pull request #22080 from giuseppe/fix-duplicate-mapping-keep-id

utils: do not generate duplicate range
containers · Mar 20, 2024 · 2efbba0 · 2efbba0
2 parents e5059fc + 816c727
commit 2efbba0
Show file tree

Hide file tree

Showing 2 changed files with 84 additions and 29 deletions.
diff --git a/pkg/util/utils.go b/pkg/util/utils.go
@@ -180,14 +180,47 @@ func ParseSignal(rawSignal string) (syscall.Signal, error) {
 	return sig, nil
 }
 
-// GetKeepIDMapping returns the mappings and the user to use when keep-id is used
-func GetKeepIDMapping(opts *namespaces.KeepIDUserNsOptions) (*stypes.IDMappingOptions, int, int, error) {
+func getRootlessKeepIDMapping(uid, gid int, uids, gids []idtools.IDMap) (*stypes.IDMappingOptions, int, int, error) {
 	options := stypes.IDMappingOptions{
 		HostUIDMapping: false,
 		HostGIDMapping: false,
 	}
+	maxUID, maxGID := 0, 0
+	for _, u := range uids {
+		maxUID += u.Size
+	}
+	for _, g := range gids {
+		maxGID += g.Size
+	}
+
+	options.UIDMap, options.GIDMap = nil, nil
+
+	if len(uids) > 0 && uid != 0 {
+		options.UIDMap = append(options.UIDMap, idtools.IDMap{ContainerID: 0, HostID: 1, Size: min(uid, maxUID)})
+	}
+	options.UIDMap = append(options.UIDMap, idtools.IDMap{ContainerID: uid, HostID: 0, Size: 1})
+	if maxUID > uid {
+		options.UIDMap = append(options.UIDMap, idtools.IDMap{ContainerID: uid + 1, HostID: uid + 1, Size: maxUID - uid})
+	}
+
+	if len(gids) > 0 && gid != 0 {
+		options.GIDMap = append(options.GIDMap, idtools.IDMap{ContainerID: 0, HostID: 1, Size: min(gid, maxGID)})
+	}
+	options.GIDMap = append(options.GIDMap, idtools.IDMap{ContainerID: gid, HostID: 0, Size: 1})
+	if maxGID > gid {
+		options.GIDMap = append(options.GIDMap, idtools.IDMap{ContainerID: gid + 1, HostID: gid + 1, Size: maxGID - gid})
+	}
+
+	return &options, uid, gid, nil
+}
 
+// GetKeepIDMapping returns the mappings and the user to use when keep-id is used
+func GetKeepIDMapping(opts *namespaces.KeepIDUserNsOptions) (*stypes.IDMappingOptions, int, int, error) {
 	if !rootless.IsRootless() {
+		options := stypes.IDMappingOptions{
+			HostUIDMapping: false,
+			HostGIDMapping: false,
+		}
 		uids, err := rootless.ReadMappingsProc("/proc/self/uid_map")
 		if err != nil {
 			return nil, 0, 0, err
@@ -224,33 +257,7 @@ func GetKeepIDMapping(opts *namespaces.KeepIDUserNsOptions) (*stypes.IDMappingOp
 		return nil, -1, -1, fmt.Errorf("cannot read mappings: %w", err)
 	}
 
-	maxUID, maxGID := 0, 0
-	for _, u := range uids {
-		maxUID += u.Size
-	}
-	for _, g := range gids {
-		maxGID += g.Size
-	}
-
-	options.UIDMap, options.GIDMap = nil, nil
-
-	if len(uids) > 0 {
-		options.UIDMap = append(options.UIDMap, idtools.IDMap{ContainerID: 0, HostID: 1, Size: min(uid, maxUID)})
-	}
-	options.UIDMap = append(options.UIDMap, idtools.IDMap{ContainerID: uid, HostID: 0, Size: 1})
-	if maxUID > uid {
-		options.UIDMap = append(options.UIDMap, idtools.IDMap{ContainerID: uid + 1, HostID: uid + 1, Size: maxUID - uid})
-	}
-
-	if len(gids) > 0 {
-		options.GIDMap = append(options.GIDMap, idtools.IDMap{ContainerID: 0, HostID: 1, Size: min(gid, maxGID)})
-	}
-	options.GIDMap = append(options.GIDMap, idtools.IDMap{ContainerID: gid, HostID: 0, Size: 1})
-	if maxGID > gid {
-		options.GIDMap = append(options.GIDMap, idtools.IDMap{ContainerID: gid + 1, HostID: gid + 1, Size: maxGID - gid})
-	}
-
-	return &options, uid, gid, nil
+	return getRootlessKeepIDMapping(uid, gid, uids, gids)
 }
 
 // GetNoMapMapping returns the mappings and the user to use when nomap is used

diff --git a/pkg/util/utils_test.go b/pkg/util/utils_test.go
@@ -7,6 +7,7 @@ import (
 	"time"
 
 	"github.com/containers/storage/pkg/idtools"
+	stypes "github.com/containers/storage/types"
 	ruser "github.com/moby/sys/user"
 	"github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/stretchr/testify/assert"
@@ -588,3 +589,50 @@ func TestConvertTimeout(t *testing.T) {
 	timeout = ConvertTimeout(-100)
 	assert.Equal(t, uint(math.MaxUint32), timeout)
 }
+
+func TestGetRootlessKeepIDMapping(t *testing.T) {
+	tests := []struct {
+		uid, gid                 int
+		uids, gids               []idtools.IDMap
+		expectedOptions          *stypes.IDMappingOptions
+		expectedUID, expectedGID int
+		expectedError            error
+	}{
+		{
+			uid:  1000,
+			gid:  1000,
+			uids: []idtools.IDMap{},
+			gids: []idtools.IDMap{},
+			expectedOptions: &stypes.IDMappingOptions{
+				HostUIDMapping: false,
+				HostGIDMapping: false,
+				UIDMap:         []idtools.IDMap{{ContainerID: 1000, HostID: 0, Size: 1}},
+				GIDMap:         []idtools.IDMap{{ContainerID: 1000, HostID: 0, Size: 1}},
+			},
+			expectedUID: 1000,
+			expectedGID: 1000,
+		},
+		{
+			uid:  0,
+			gid:  0,
+			uids: []idtools.IDMap{{ContainerID: 0, HostID: 100000, Size: 65536}},
+			gids: []idtools.IDMap{{ContainerID: 0, HostID: 100000, Size: 65536}},
+			expectedOptions: &stypes.IDMappingOptions{
+				HostUIDMapping: false,
+				HostGIDMapping: false,
+				UIDMap:         []idtools.IDMap{{ContainerID: 0, HostID: 0, Size: 1}, {ContainerID: 1, HostID: 1, Size: 65536}},
+				GIDMap:         []idtools.IDMap{{ContainerID: 0, HostID: 0, Size: 1}, {ContainerID: 1, HostID: 1, Size: 65536}},
+			},
+			expectedUID: 0,
+			expectedGID: 0,
+		},
+	}
+
+	for _, test := range tests {
+		options, uid, gid, err := getRootlessKeepIDMapping(test.uid, test.gid, test.uids, test.gids)
+		assert.Nil(t, err)
+		assert.Equal(t, test.expectedOptions, options)
+		assert.Equal(t, test.expectedUID, uid)
+		assert.Equal(t, test.expectedGID, gid)
+	}
+}