[lxc-devel] [lxd/master] Idmap bugfixes

stgraber on Github lxc-bot at linuxcontainers.org
Tue Nov 22 05:13:35 UTC 2016


A non-text attachment was scrubbed...
Name: not available
Type: text/x-mailbox
Size: 301 bytes
Desc: not available
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20161122/65e0a0d8/attachment.bin>
-------------- next part --------------
From b20c90c054761c7fab4871a351bfd761ff09ad82 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?St=C3=A9phane=20Graber?= <stgraber at ubuntu.com>
Date: Mon, 21 Nov 2016 19:12:24 -0500
Subject: [PATCH 1/8] idmap: Fix handling on container/profile update
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

 - Don't re-generated unless something changed.
 - Use the expanded config to also respect profiles.

Signed-off-by: Stéphane Graber <stgraber at ubuntu.com>
---
 lxd/container_lxc.go | 55 +++++++++++++++++++++++++++-------------------------
 1 file changed, 29 insertions(+), 26 deletions(-)

diff --git a/lxd/container_lxc.go b/lxd/container_lxc.go
index 0a51860..839232e 100644
--- a/lxd/container_lxc.go
+++ b/lxd/container_lxc.go
@@ -2805,30 +2805,6 @@ func (c *containerLXC) Update(args containerArgs, userRequested bool) error {
 		}
 	}()
 
-	// update the idmap
-	idmap, base, err := findIdmap(
-		c.daemon,
-		c.Name(),
-		args.Config["security.idmap.isolated"],
-		args.Config["security.idmap.size"],
-		args.Config["raw.idmap"],
-	)
-	if err != nil {
-		return err
-	}
-	var jsonIdmap string
-	if idmap != nil {
-		idmapBytes, err := json.Marshal(idmap.Idmap)
-		if err != nil {
-			return err
-		}
-		jsonIdmap = string(idmapBytes)
-	} else {
-		jsonIdmap = "[]"
-	}
-	args.Config["volatile.idmap.next"] = jsonIdmap
-	args.Config["volatile.idmap.base"] = fmt.Sprintf("%v", base)
-
 	// Apply the various changes
 	c.architecture = args.Architecture
 	c.ephemeral = args.Ephemeral
@@ -2887,14 +2863,41 @@ func (c *containerLXC) Update(args containerArgs, userRequested bool) error {
 		return err
 	}
 
-	// If apparmor changed, re-validate the apparmor profile
 	for _, key := range changedConfig {
-		if key == "raw.apparmor" || key == "security.nesting" {
+		// If apparmor changed, re-validate the apparmor profile
+		if shared.StringInSlice(key, []string{"raw.apparmor", "security.nesting"}) {
 			err = AAParseProfile(c)
 			if err != nil {
 				return err
 			}
 		}
+
+		if shared.StringInSlice(key, []string{"security.idmap.isolated", "security.idmap.size", "raw.idmap"}) {
+			// update the idmap
+			idmap, base, err := findIdmap(
+				c.daemon,
+				c.Name(),
+				c.expandedConfig["security.idmap.isolated"],
+				c.expandedConfig["security.idmap.size"],
+				c.expandedConfig["raw.idmap"],
+			)
+			if err != nil {
+				return err
+			}
+
+			var jsonIdmap string
+			if idmap != nil {
+				idmapBytes, err := json.Marshal(idmap.Idmap)
+				if err != nil {
+					return err
+				}
+				jsonIdmap = string(idmapBytes)
+			} else {
+				jsonIdmap = "[]"
+			}
+			c.localConfig["volatile.idmap.next"] = jsonIdmap
+			c.localConfig["volatile.idmap.base"] = fmt.Sprintf("%v", base)
+		}
 	}
 
 	// Apply disk quota changes

From a7d757c9695947e6cdfe645d7656baed043f4f68 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?St=C3=A9phane=20Graber?= <stgraber at ubuntu.com>
Date: Mon, 21 Nov 2016 19:19:54 -0500
Subject: [PATCH 2/8] idmap: Properly parse booleans
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Stéphane Graber <stgraber at ubuntu.com>
---
 lxd/container_lxc.go | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lxd/container_lxc.go b/lxd/container_lxc.go
index 839232e..2fcbb69 100644
--- a/lxd/container_lxc.go
+++ b/lxd/container_lxc.go
@@ -375,7 +375,7 @@ func (c *containerLXC) waitOperation() error {
 
 func idmapSize(daemon *Daemon, isolatedStr string, size string) (int, error) {
 	isolated := false
-	if isolatedStr == "true" {
+	if shared.IsTrue(isolatedStr) {
 		isolated = true
 	}
 
@@ -487,7 +487,7 @@ func parseRawIdmap(value string) ([]shared.IdmapEntry, error) {
 
 func findIdmap(daemon *Daemon, cName string, isolatedStr string, configSize string, rawIdmap string) (*shared.IdmapSet, int, error) {
 	isolated := false
-	if isolatedStr == "true" {
+	if shared.IsTrue(isolatedStr) {
 		isolated = true
 	}
 
@@ -533,7 +533,7 @@ func findIdmap(daemon *Daemon, cName string, isolatedStr string, configSize stri
 			return nil, 0, err
 		}
 
-		if container.ExpandedConfig()["security.idmap.isolated"] != "true" {
+		if !shared.IsTrue(container.ExpandedConfig()["security.idmap.isolated"]) {
 			continue
 		}
 

From 9a8b407edef1769ee4e259322d34fbf6ef090cdd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?St=C3=A9phane=20Graber?= <stgraber at ubuntu.com>
Date: Mon, 21 Nov 2016 19:31:10 -0500
Subject: [PATCH 3/8] idmap: Grow fallback allocation to work isolated
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

On systems that don't have shadow support for idmaps, we fallback to a
fixed map. Until now this map was made fo 100000 uid/gid. This was
absolutely fine until we added support for multiple ranges.

Since we can usually assume that such systems aren't allocating chunks
of uid/gid to other users, lets just take a big chunk of a billion
uid/gid for use by LXD.

Signed-off-by: Stéphane Graber <stgraber at ubuntu.com>
---
 shared/idmapset_linux.go | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/shared/idmapset_linux.go b/shared/idmapset_linux.go
index a2b0db1..6851ce0 100644
--- a/shared/idmapset_linux.go
+++ b/shared/idmapset_linux.go
@@ -453,9 +453,9 @@ func DefaultIdmapSet() (*IdmapSet, error) {
 	}
 
 	umin := 1000000
-	urange := 100000
+	urange := 1000000000
 	gmin := 1000000
-	grange := 100000
+	grange := 1000000000
 
 	newuidmap, _ := exec.LookPath("newuidmap")
 	newgidmap, _ := exec.LookPath("newgidmap")

From 63d6548194ed054698a032f3935616d3287c1285 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?St=C3=A9phane=20Graber?= <stgraber at ubuntu.com>
Date: Mon, 21 Nov 2016 20:29:12 -0500
Subject: [PATCH 4/8] idmap: Respect profiles
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Move idmap initialization to containerLXCCreate and use the expanded
configuration as input so we can respect any profile that the container
uses.

This also lets us skip some of the old idmap code, detecting and storing
the next map only once.

Signed-off-by: Stéphane Graber <stgraber at ubuntu.com>
---
 lxd/container.go     | 24 ------------------------
 lxd/container_lxc.go | 28 ++++++++++++++++++++++++++--
 2 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/lxd/container.go b/lxd/container.go
index b40961a..95e9c12 100644
--- a/lxd/container.go
+++ b/lxd/container.go
@@ -1,7 +1,6 @@
 package main
 
 import (
-	"encoding/json"
 	"fmt"
 	"io"
 	"os"
@@ -646,29 +645,6 @@ func containerCreateInternal(d *Daemon, args containerArgs) (container, error) {
 	// Wipe any existing log for this container name
 	os.RemoveAll(shared.LogPath(args.Name))
 
-	idmap, base, err := findIdmap(
-		d,
-		args.Name,
-		args.Config["security.idmap.isolated"],
-		args.Config["security.idmap.size"],
-		args.Config["raw.idmap"],
-	)
-	if err != nil {
-		return nil, err
-	}
-	var jsonIdmap string
-	if idmap != nil {
-		idmapBytes, err := json.Marshal(idmap.Idmap)
-		if err != nil {
-			return nil, err
-		}
-		jsonIdmap = string(idmapBytes)
-	} else {
-		jsonIdmap = "[]"
-	}
-	args.Config["volatile.idmap.next"] = jsonIdmap
-	args.Config["volatile.idmap.base"] = fmt.Sprintf("%v", base)
-
 	// Create the container entry
 	id, err := dbContainerCreate(d.db, args)
 	if err != nil {
diff --git a/lxd/container_lxc.go b/lxd/container_lxc.go
index 2fcbb69..e7cf863 100644
--- a/lxd/container_lxc.go
+++ b/lxd/container_lxc.go
@@ -233,7 +233,18 @@ func containerLXCCreate(d *Daemon, args containerArgs) (container, error) {
 	}
 
 	// Setup initial idmap config
-	idmap := c.IdmapSet()
+	idmap, base, err := findIdmap(
+		d,
+		args.Name,
+		c.expandedConfig["security.idmap.isolated"],
+		c.expandedConfig["security.idmap.size"],
+		c.expandedConfig["raw.idmap"],
+	)
+	if err != nil {
+		c.Delete()
+		return nil, err
+	}
+
 	var jsonIdmap string
 	if idmap != nil {
 		idmapBytes, err := json.Marshal(idmap.Idmap)
@@ -246,13 +257,26 @@ func containerLXCCreate(d *Daemon, args containerArgs) (container, error) {
 		jsonIdmap = "[]"
 	}
 
+	err = c.ConfigKeySet("volatile.idmap.next", jsonIdmap)
+	if err != nil {
+		c.Delete()
+		return nil, err
+	}
+
+	err = c.ConfigKeySet("volatile.idmap.base", fmt.Sprintf("%v", base))
+	if err != nil {
+		c.Delete()
+		return nil, err
+	}
+
 	err = c.ConfigKeySet("volatile.last_state.idmap", jsonIdmap)
 	if err != nil {
 		c.Delete()
 		return nil, err
 	}
 
-	err = c.ConfigKeySet("volatile.idmap.next", jsonIdmap)
+	// Re-run init to update the idmap
+	err = c.init()
 	if err != nil {
 		c.Delete()
 		return nil, err

From b7baff664a9b91170d018e9774763062bc2276bd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?St=C3=A9phane=20Graber?= <stgraber at ubuntu.com>
Date: Mon, 21 Nov 2016 21:22:49 -0500
Subject: [PATCH 5/8] idmap: Support isolated container copy
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

We need to keep the old idmap around in volatile so that container
copies can properly remap uid/gid.

Signed-off-by: Stéphane Graber <stgraber at ubuntu.com>
---
 lxd/container_lxc.go   | 11 +++++++----
 lxd/containers_post.go |  2 +-
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/lxd/container_lxc.go b/lxd/container_lxc.go
index e7cf863..dd3508a 100644
--- a/lxd/container_lxc.go
+++ b/lxd/container_lxc.go
@@ -269,10 +269,13 @@ func containerLXCCreate(d *Daemon, args containerArgs) (container, error) {
 		return nil, err
 	}
 
-	err = c.ConfigKeySet("volatile.last_state.idmap", jsonIdmap)
-	if err != nil {
-		c.Delete()
-		return nil, err
+	// Set last_state to the map we have on disk
+	if c.localConfig["volatile.last_state.idmap"] == "" {
+		err = c.ConfigKeySet("volatile.last_state.idmap", jsonIdmap)
+		if err != nil {
+			c.Delete()
+			return nil, err
+		}
 	}
 
 	// Re-run init to update the idmap
diff --git a/lxd/containers_post.go b/lxd/containers_post.go
index 659de32..6f844e1 100644
--- a/lxd/containers_post.go
+++ b/lxd/containers_post.go
@@ -346,7 +346,7 @@ func createFromCopy(d *Daemon, req *containerPostReq) Response {
 	}
 
 	for key, value := range sourceConfig {
-		if len(key) > 8 && key[0:8] == "volatile" && key[9:] != "base_image" {
+		if len(key) > 8 && key[0:8] == "volatile" && !shared.StringInSlice(key[9:], []string{"base_image", "last_state.idmap"}) {
 			shared.LogDebug("Skipping volatile key from copy source",
 				log.Ctx{"key": key})
 			continue

From 278420beb5fcbe8fe238a08b3efc786f64d90a53 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?St=C3=A9phane=20Graber?= <stgraber at ubuntu.com>
Date: Mon, 21 Nov 2016 22:41:29 -0500
Subject: [PATCH 6/8] Improve performance of update by only doing changes once
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Stéphane Graber <stgraber at ubuntu.com>
---
 lxd/container_lxc.go | 56 +++++++++++++++++++++++++---------------------------
 1 file changed, 27 insertions(+), 29 deletions(-)

diff --git a/lxd/container_lxc.go b/lxd/container_lxc.go
index dd3508a..c1a9264 100644
--- a/lxd/container_lxc.go
+++ b/lxd/container_lxc.go
@@ -2890,41 +2890,39 @@ func (c *containerLXC) Update(args containerArgs, userRequested bool) error {
 		return err
 	}
 
-	for _, key := range changedConfig {
-		// If apparmor changed, re-validate the apparmor profile
-		if shared.StringInSlice(key, []string{"raw.apparmor", "security.nesting"}) {
-			err = AAParseProfile(c)
-			if err != nil {
-				return err
-			}
+	// If apparmor changed, re-validate the apparmor profile
+	if shared.StringInSlice("raw.apparmor", changedConfig) || shared.StringInSlice("security.nesting", changedConfig) {
+		err = AAParseProfile(c)
+		if err != nil {
+			return err
+		}
+	}
+
+	if shared.StringInSlice("security.idmap.isolated", changedConfig) || shared.StringInSlice("security.idmap.size", changedConfig) || shared.StringInSlice("raw.idmap", changedConfig) {
+		// update the idmap
+		idmap, base, err := findIdmap(
+			c.daemon,
+			c.Name(),
+			c.expandedConfig["security.idmap.isolated"],
+			c.expandedConfig["security.idmap.size"],
+			c.expandedConfig["raw.idmap"],
+		)
+		if err != nil {
+			return err
 		}
 
-		if shared.StringInSlice(key, []string{"security.idmap.isolated", "security.idmap.size", "raw.idmap"}) {
-			// update the idmap
-			idmap, base, err := findIdmap(
-				c.daemon,
-				c.Name(),
-				c.expandedConfig["security.idmap.isolated"],
-				c.expandedConfig["security.idmap.size"],
-				c.expandedConfig["raw.idmap"],
-			)
+		var jsonIdmap string
+		if idmap != nil {
+			idmapBytes, err := json.Marshal(idmap.Idmap)
 			if err != nil {
 				return err
 			}
-
-			var jsonIdmap string
-			if idmap != nil {
-				idmapBytes, err := json.Marshal(idmap.Idmap)
-				if err != nil {
-					return err
-				}
-				jsonIdmap = string(idmapBytes)
-			} else {
-				jsonIdmap = "[]"
-			}
-			c.localConfig["volatile.idmap.next"] = jsonIdmap
-			c.localConfig["volatile.idmap.base"] = fmt.Sprintf("%v", base)
+			jsonIdmap = string(idmapBytes)
+		} else {
+			jsonIdmap = "[]"
 		}
+		c.localConfig["volatile.idmap.next"] = jsonIdmap
+		c.localConfig["volatile.idmap.base"] = fmt.Sprintf("%v", base)
 	}
 
 	// Apply disk quota changes

From 477092caf3eea16065c418272e6cf6aacdc37569 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?St=C3=A9phane=20Graber?= <stgraber at ubuntu.com>
Date: Mon, 21 Nov 2016 23:03:31 -0500
Subject: [PATCH 7/8] Always call Update() on profile updates
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Stéphane Graber <stgraber at ubuntu.com>
---
 lxd/profiles.go | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/lxd/profiles.go b/lxd/profiles.go
index 30a9d45..4babe81 100644
--- a/lxd/profiles.go
+++ b/lxd/profiles.go
@@ -138,7 +138,7 @@ func profileGet(d *Daemon, r *http.Request) Response {
 	return SyncResponseETag(true, resp, resp)
 }
 
-func getRunningContainersWithProfile(d *Daemon, profile string) []container {
+func getContainersWithProfile(d *Daemon, profile string) []container {
 	results := []container{}
 
 	output, err := dbProfileContainersGet(d.db, profile)
@@ -154,6 +154,7 @@ func getRunningContainersWithProfile(d *Daemon, profile string) []container {
 		}
 		results = append(results, c)
 	}
+
 	return results
 }
 
@@ -256,16 +257,8 @@ func doProfileUpdate(d *Daemon, name string, id int64, profile *shared.ProfileCo
 		return BadRequest(err)
 	}
 
-	// Get the running container list
-	clist := getRunningContainersWithProfile(d, name)
-	var containers []container
-	for _, c := range clist {
-		if !c.IsRunning() {
-			continue
-		}
-
-		containers = append(containers, c)
-	}
+	// Get the container list
+	containers := getContainersWithProfile(d, name)
 
 	// Update the database
 	tx, err := dbBegin(d.db)
@@ -385,7 +378,7 @@ func profileDelete(d *Daemon, r *http.Request) Response {
 		return SmartError(err)
 	}
 
-	clist := getRunningContainersWithProfile(d, name)
+	clist := getContainersWithProfile(d, name)
 	if len(clist) != 0 {
 		return BadRequest(fmt.Errorf("Profile is currently in use"))
 	}

From 189e2cbb413d1e6347e4b077f617f07f7d4c999f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?St=C3=A9phane=20Graber?= <stgraber at ubuntu.com>
Date: Mon, 21 Nov 2016 23:25:02 -0500
Subject: [PATCH 8/8] idmap: Fix off by one range calculation
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Stéphane Graber <stgraber at ubuntu.com>
---
 lxd/container_lxc.go | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/lxd/container_lxc.go b/lxd/container_lxc.go
index c1a9264..d26a2cb 100644
--- a/lxd/container_lxc.go
+++ b/lxd/container_lxc.go
@@ -542,7 +542,7 @@ func findIdmap(daemon *Daemon, cName string, isolatedStr string, configSize stri
 		return nil, 0, err
 	}
 
-	offset := daemon.IdmapSet.Idmap[0].Hostid + 65536 + 1
+	offset := daemon.IdmapSet.Idmap[0].Hostid + 65536
 	size, err := idmapSize(daemon, isolatedStr, configSize)
 	if err != nil {
 		return nil, 0, err
@@ -595,7 +595,7 @@ func findIdmap(daemon *Daemon, cName string, isolatedStr string, configSize stri
 	for i := range mapentries {
 		if i == 0 {
 			if mapentries[0].Hostid < offset+size {
-				offset = mapentries[0].Hostid + mapentries[0].Maprange + 1
+				offset = mapentries[0].Hostid + mapentries[0].Maprange
 				continue
 			}
 
@@ -603,15 +603,15 @@ func findIdmap(daemon *Daemon, cName string, isolatedStr string, configSize stri
 		}
 
 		if mapentries[i-1].Hostid+mapentries[i-1].Maprange > offset {
-			offset = mapentries[i-1].Hostid + mapentries[i-1].Maprange + 1
+			offset = mapentries[i-1].Hostid + mapentries[i-1].Maprange
 			continue
 		}
 
-		offset = mapentries[i-1].Hostid + mapentries[i-1].Maprange + 1
+		offset = mapentries[i-1].Hostid + mapentries[i-1].Maprange
 		if offset+size < mapentries[i].Hostid {
 			return mkIdmap(offset, size), offset, nil
 		}
-		offset = mapentries[i].Hostid + mapentries[i].Maprange + 1
+		offset = mapentries[i].Hostid + mapentries[i].Maprange
 	}
 
 	if offset+size < daemon.IdmapSet.Idmap[0].Hostid+daemon.IdmapSet.Idmap[0].Maprange {


More information about the lxc-devel mailing list