Merge lp:~jamesodhunt/snappy/add-lsblk-cache into lp:~snappy-dev/snappy/snappy-moved-to-github

[James Hunt]

* Allow 'snappy list -a' to be run as a non-root user by reading from the lsblk cache.

Note that this branch doesn't add a systemd service to generate the cache since we cannot create a working packaging branch until we've packaged up all the godeps into the archive.

Meantime, to test this:

# Should fail with an error telling you to re-run as root.
$ ./snappy list -a

# Should work
$ sudo ./snappy list -a

# Generate cache to allow non-priv user to run 'snappy list -a'
$ sudo ./snappy cache-lsblk

# Should now work
$ ./snappy list -a

[Michael Vogt (mvo)]

Thanks for this branch!

I think this looks good but there are some details in the inline comments. I also feel that the new functions in utils.go needs tests. It does not have to be done in this MP but I think we should add them soon.

Another note as suggested by Loic it might be worthwhile to investigate if we can get the information we need (NAME, LABEL, PKNAME, MOUNTPOINT) without the need to run as root as this would avoid having to have the extra systemd unit and the extra command in snappy. This can be done in a followup MP of course.

[Michael Vogt (mvo)]

I looked into the lsblk issue this morning and it seems like thats the place we can fix the underlying issue.

The problem appears to be that we do not build util-linux with libudev-dev as a build-dependency which means that the partition label is not visible for non-root-users. When libudev-dev is added it will show partition labels even for non-root users (udevadm info -x /dev/sda3 confirms that the partition label info is available via udev).

I uploaded https://launchpadlibrarian.net/196449981/util-linux_2.25.2-4ubuntu1_2.25.2-4ubuntu1%2Bppa1.diff.gz into the PPA and will push this change to vivid once I got feedback from upstream.

[James Hunt (jamesodhunt)]

After discussions yesterday, I'm going to remove the lsblk code, so this branch shouldn't now be merged.

review: Disapprove

Unmerged revisions

136. By James Hunt on 2015-02-02

* Update tests to take account of new lsblk cache code.

135. By James Hunt on 2015-02-02

* Allow 'snappy list -a' to be run as a non-root user by reading from
  the lsblk cache.
* Show "booted" command as an internal one in usage.
* Add "cache-lsblk" command to generate the cache.
* Move more functions into utils.go.

=== modified file 'cmd/snappy/cmd_booted.go'
--- cmd/snappy/cmd_booted.go	2015-01-27 20:46:45 +0000
+++ cmd/snappy/cmd_booted.go	2015-02-02 14:07:12 +0000
@@ -9,8 +9,8 @@
 
 func init() {
 	Parser.AddCommand("booted",
-		"Flag that rootfs booted successfully",
-		"Not necessary to run this command manually",
+		"Flag that rootfs booted successfully (INTERNAL)",
+		"Do not run this command manually",
 		&cmdBooted)
 }
 

=== added file 'cmd/snappy/cmd_cache_lsblk.go'
--- cmd/snappy/cmd_cache_lsblk.go	1970-01-01 00:00:00 +0000
+++ cmd/snappy/cmd_cache_lsblk.go	2015-02-02 14:07:12 +0000
@@ -0,0 +1,24 @@
+package main
+
+import "launchpad.net/snappy/snappy"
+
+type CmdCacheLsblk struct {
+}
+
+var cmdCacheLsblk CmdCacheLsblk
+
+func init() {
+	Parser.AddCommand("cache-lsblk",
+		"Cache lsblk(8) data (INTERNAL)",
+		"Do not run this command manually",
+		&cmdCacheLsblk)
+}
+
+func (x *CmdCacheLsblk) Execute(args []string) (err error) {
+	parts, err := snappy.InstalledSnapsByType(snappy.SnapTypeCore)
+	if err != nil {
+		return err
+	}
+
+	return parts[0].(*snappy.SystemImagePart).GenerateLsblkCache()
+}

=== modified file 'partition/bootloader_uboot.go'
--- partition/bootloader_uboot.go	2015-01-27 08:25:19 +0000
+++ partition/bootloader_uboot.go	2015-02-02 14:07:12 +0000
@@ -5,7 +5,6 @@
 package partition
 
 import (
-	"bufio"
 	"fmt"
 	"os"
 	"path"
@@ -172,46 +171,6 @@
 	return u.otherRootfs
 }
 
-// FIXME: put into utils package
-func readLines(path string) (lines []string, err error) {
-
-	file, err := os.Open(path)
-
-	if err != nil {
-		return nil, err
-	}
-
-	defer file.Close()
-
-	scanner := bufio.NewScanner(file)
-	for scanner.Scan() {
-		lines = append(lines, scanner.Text())
-	}
-
-	return lines, scanner.Err()
-}
-
-// FIXME: put into utils package
-func writeLines(lines []string, path string) (err error) {
-
-	file, err := os.Create(path)
-
-	if err != nil {
-		return err
-	}
-
-	defer file.Close()
-
-	writer := bufio.NewWriter(file)
-
-	for _, line := range lines {
-		if _, err = fmt.Fprintln(writer, line); err != nil {
-			return err
-		}
-	}
-	return writer.Flush()
-}
-
 // Returns name=value entries from the specified file, removing all
 // blank lines and comments.
 func getNameValuePairs(file string) (vars []string, err error) {
@@ -379,68 +338,3 @@
 	return err
 }
 
-// Write lines to file atomically. File does not have to preexist.
-// FIXME: put into utils package
-func atomicFileUpdate(file string, lines []string) (err error) {
-	tmpFile := fmt.Sprintf("%s.NEW", file)
-
-	if err := writeLines(lines, tmpFile); err != nil {
-		return err
-	}
-
-	// atomic update
-	if err = os.Rename(tmpFile, file); err != nil {
-		return err
-	}
-
-	return err
-}
-
-// Rewrite the specified file, applying the specified set of changes.
-// Lines not in the changes slice are left alone.
-// If the original file does not contain any of the name entries (from
-// the corresponding ConfigFileChange objects), those entries are
-// appended to the file.
-//
-// FIXME: put into utils package
-func modifyNameValueFile(file string, changes []ConfigFileChange) (err error) {
-	var lines []string
-	var updated []ConfigFileChange
-
-	if lines, err = readLines(file); err != nil {
-		return err
-	}
-
-	var new []string
-
-	for _, line := range lines {
-		for _, change := range changes {
-			if strings.HasPrefix(line, fmt.Sprintf("%s=", change.Name)) {
-				line = fmt.Sprintf("%s=%s", change.Name, change.Value)
-				updated = append(updated, change)
-			}
-		}
-		new = append(new, line)
-	}
-
-	lines = new
-
-	for _, change := range changes {
-		var got bool = false
-		for _, update := range updated {
-			if update.Name == change.Name {
-				got = true
-				break
-			}
-		}
-
-		if got == false {
-			// name/value pair did not exist in original
-			// file, so append
-			lines = append(lines, fmt.Sprintf("%s=%s",
-				change.Name, change.Value))
-		}
-	}
-
-	return atomicFileUpdate(file, lines)
-}

=== modified file 'partition/partition.go'
--- partition/partition.go	2015-01-30 08:52:10 +0000
+++ partition/partition.go	2015-02-02 14:07:12 +0000
@@ -36,6 +36,7 @@
 	"regexp"
 	"strings"
 	"syscall"
+	"path/filepath"
 
 	"gopkg.in/yaml.v2"
 )
@@ -67,9 +68,12 @@
 // diretory.
 const MOUNT_TARGET = "system"
 
-// File creation mode used when any directories are created
+// Default file creation mode used when any directories are created
 const DIR_MODE = 0750
 
+// Mode used for directories that must allow non-priv users access
+const DIR_MODE_PERMISSIVE = 0755
+
 var (
 	BootloaderError = errors.New("Unable to determine bootloader")
 
@@ -96,6 +100,22 @@
 // to the disk (such as uBoot, MLO)
 const FLASH_ASSETS_DIR = "flashtool-assets"
 
+// Full path to file containing lsblk(8) output, generated in early
+// boot.
+//
+// lsblk(8) has rather unfortunate semantics - if run as a non-root
+// user, it "works", but returns *incomplete* information. Specfically,
+// the partition label information is missing.
+//
+// Since this information is critical, and since snappy should be
+// runable as a non-root user for query-type operations we now run
+// lsblk(8) as root in early boot, and save the output.
+//
+// As the partition layout is never expected to change on an
+// operational system, this is a reasonable compromise (and avoids
+// set-uid nastiness).
+const lsblkCacheFile = "/run/snappy/lsblk.txt"
+
 //--------------------------------------------------------------------
 // FIXME: Globals
 
@@ -117,6 +137,8 @@
 type PartitionInterface interface {
 	UpdateBootloader() (err error)
 	MarkBootSuccessful() (err error)
+	GenerateLsblkCache() (err error)
+
 	// FIXME: could we make SyncBootloaderFiles part of UpdateBootloader
 	//        to expose even less implementation details?
 	SyncBootloaderFiles() (err error)
@@ -304,11 +326,43 @@
 }
 
 var runLsblk = func() (output []string, err error) {
-	return runCommandWithStdout(
-		"/bin/lsblk",
-		"--ascii",
-		"--output=NAME,LABEL,PKNAME,MOUNTPOINT",
-		"--pairs")
+	// always run the actual command as root since:
+	//
+	// - Doing so means we can potentially test that the code that
+	//   runs lsblk in early boot gives the same output as the command
+	//   below (by running tests as root and non-root).
+	//
+	// - The system remains upgradeable should the cache file be
+	//   removed inadvertently.
+	if os.Getuid() == 0 {
+		return getLsblkOutput()
+	}
+
+	if !fileExists(lsblkCacheFile) {
+		panic(fmt.Sprintf("ERROR: cannot find lsblk cache %s - " +
+		"re-run as root user",
+		lsblkCacheFile))
+	}
+
+	return readLines(lsblkCacheFile)
+}
+
+func (p *Partition) GenerateLsblkCache() (err error) {
+	var lines []string
+
+	// ensure the directory exists.
+
+	dir := filepath.Dir(lsblkCacheFile)
+	if err = os.MkdirAll(dir, DIR_MODE_PERMISSIVE); err != nil {
+		return err
+	}
+
+	lines, err = getLsblkOutput()
+	if err != nil {
+		return err
+	}
+
+	return writeLines(lines, lsblkCacheFile)
 }
 
 // Determine details of the recognised disk partitions

=== modified file 'partition/partition_test.go'
--- partition/partition_test.go	2015-01-29 14:33:54 +0000
+++ partition/partition_test.go	2015-02-02 14:07:12 +0000
@@ -34,6 +34,8 @@
 }
 
 func (s *PartitionTestSuite) TestHardwareSpec(c *C) {
+	runLsblk = mockRunLsblkDualSnappy
+
 	p := New()
 	c.Assert(p, NotNil)
 

=== modified file 'partition/utils.go'
--- partition/utils.go	2015-01-27 07:59:45 +0000
+++ partition/utils.go	2015-02-02 14:07:12 +0000
@@ -6,6 +6,7 @@
 	"os"
 	"os/exec"
 	"strings"
+	"bufio"
 )
 
 // Return true if given path exists.
@@ -75,5 +76,126 @@
 
 	output = strings.Split(string(bytes), "\n")
 
+	// remove last element if it's empty
+	if len(output) > 1 {
+		last := output[len(output)-1]
+		if last == "" {
+			output = output[:len(output)-1]
+		}
+	}
+
 	return output, err
 }
+
+// Return a string slice of all lines in file specified by path 
+func readLines(path string) (lines []string, err error) {
+
+	file, err := os.Open(path)
+
+	if err != nil {
+		return nil, err
+	}
+
+	defer file.Close()
+
+	scanner := bufio.NewScanner(file)
+	for scanner.Scan() {
+		lines = append(lines, scanner.Text())
+	}
+
+	return lines, scanner.Err()
+}
+
+// Write lines slice to file specified by path
+func writeLines(lines []string, path string) (err error) {
+
+	file, err := os.Create(path)
+
+	if err != nil {
+		return err
+	}
+
+	defer file.Close()
+
+	writer := bufio.NewWriter(file)
+
+	for _, line := range lines {
+		if _, err = fmt.Fprintln(writer, line); err != nil {
+			return err
+		}
+	}
+	return writer.Flush()
+}
+
+// Write lines to file atomically. File does not have to preexist.
+func atomicFileUpdate(file string, lines []string) (err error) {
+	tmpFile := fmt.Sprintf("%s.NEW", file)
+
+	if err := writeLines(lines, tmpFile); err != nil {
+		return err
+	}
+
+	// atomic update
+	if err = os.Rename(tmpFile, file); err != nil {
+		return err
+	}
+
+	return err
+}
+
+// Rewrite the specified file, applying the specified set of changes.
+// Lines not in the changes slice are left alone.
+// If the original file does not contain any of the name entries (from
+// the corresponding ConfigFileChange objects), those entries are
+// appended to the file.
+//
+func modifyNameValueFile(file string, changes []ConfigFileChange) (err error) {
+	var lines []string
+	var updated []ConfigFileChange
+
+	if lines, err = readLines(file); err != nil {
+		return err
+	}
+
+	var new []string
+
+	for _, line := range lines {
+		for _, change := range changes {
+			if strings.HasPrefix(line, fmt.Sprintf("%s=", change.Name)) {
+				line = fmt.Sprintf("%s=%s", change.Name, change.Value)
+				updated = append(updated, change)
+			}
+		}
+		new = append(new, line)
+	}
+
+	lines = new
+
+	for _, change := range changes {
+		var got bool = false
+		for _, update := range updated {
+			if update.Name == change.Name {
+				got = true
+				break
+			}
+		}
+
+		if got == false {
+			// name/value pair did not exist in original
+			// file, so append
+			lines = append(lines, fmt.Sprintf("%s=%s",
+				change.Name, change.Value))
+		}
+	}
+
+	return atomicFileUpdate(file, lines)
+}
+
+func getLsblkOutput() ([]string, error) {
+	return runCommandWithStdout(
+		"/bin/lsblk",
+		"--ascii",
+		"--output=NAME,LABEL,PKNAME,MOUNTPOINT",
+		"--pairs")
+}
+

=== modified file 'snappy/systemimage.go'
--- snappy/systemimage.go	2015-01-29 15:28:06 +0000
+++ snappy/systemimage.go	2015-02-02 14:07:12 +0000
@@ -121,9 +121,13 @@
 // Mark the *currently* booted rootfs as "good" (it booted :)
 // Note: Not part of the Part interface.
 func (s *SystemImagePart) MarkBootSuccessful() (err error) {
-
 	return s.partition.MarkBootSuccessful()
 }
+
+func (s *SystemImagePart) GenerateLsblkCache() (err error) {
+	return s.partition.GenerateLsblkCache()
+}
+
 func (s *SystemImagePart) Channel() string {
 
 	return s.channelName

=== modified file 'snappy/systemimage_test.go'
--- snappy/systemimage_test.go	2015-01-28 10:31:55 +0000
+++ snappy/systemimage_test.go	2015-02-02 14:07:12 +0000
@@ -293,6 +293,11 @@
 	p.markBootSuccessfulCalled = true
 	return nil
 }
+
+func (p *MockPartition) GenerateLsblkCache() (err error) {
+	return err
+}
+
 func (p *MockPartition) SyncBootloaderFiles() (err error) {
 	p.syncBootloaderFilesCalled = true
 	return nil