Functionality changes to the following flags

	--group-add
	--blkio-weight-device
	--device-read-bps
	--device-write-bps
	--device-read-iops
	--device-write-iops

--group-add now supports group names as well as the gid associated with them.
All the --device flags work now with moderate changes to the code to support both
bps and iops.
Added tests for all the flags.

Signed-off-by: umohnani8 <umohnani@redhat.com>

Closes: #590
Approved by: mheon

10 files changed, 170 insertions, 39 deletions

diff --git a/cmd/podman/create.go b/cmd/podman/create.go
index a25f31717..8bf61a2ca 100644
--- a/cmd/podman/create.go
+++ b/cmd/podman/create.go
@@ -86,7 +86,7 @@ type createConfig struct {
 	Entrypoint         []string          //entrypoint
 	Env                map[string]string //env
 	ExposedPorts       map[nat.Port]struct{}
-	GroupAdd           []uint32 // group-add
+	GroupAdd           []string // group-add
 	HostAdd            []string //add-host
 	Hostname           string   //hostname
 	Image              string
@@ -208,6 +208,7 @@ func createCmd(c *cli.Context) error {
 	options = append(options, libpod.WithUser(createConfig.User))
 	options = append(options, libpod.WithShmDir(createConfig.ShmDir))
 	options = append(options, libpod.WithShmSize(createConfig.Resources.ShmSize))
+	options = append(options, libpod.WithGroups(createConfig.GroupAdd))
 	ctr, err := runtime.NewContainer(runtimeSpec, options...)
 	if err != nil {
 		return err
@@ -406,11 +407,6 @@ func parseCreateOpts(c *cli.Context, runtime *libpod.Runtime, imageName string,
 		return nil, errors.Wrapf(err, "invalid value for sysctl")
 	}
 
-	groupAdd, err := stringSlicetoUint32Slice(c.StringSlice("group-add"))
-	if err != nil {
-		return nil, errors.Wrapf(err, "invalid value for groups provided")
-	}
-
 	if c.String("memory") != "" {
 		memoryLimit, err = units.RAMInBytes(c.String("memory"))
 		if err != nil {
@@ -625,7 +621,7 @@ func parseCreateOpts(c *cli.Context, runtime *libpod.Runtime, imageName string,
 		Entrypoint:        entrypoint,
 		Env:               env,
 		//ExposedPorts:   ports,
-		GroupAdd:       groupAdd,
+		GroupAdd:       c.StringSlice("group-add"),
 		Hostname:       c.String("hostname"),
 		HostAdd:        c.StringSlice("add-host"),
 		Image:          imageName,
diff --git a/cmd/podman/run.go b/cmd/podman/run.go
index e9eaf83d2..4966316c5 100644
--- a/cmd/podman/run.go
+++ b/cmd/podman/run.go
@@ -3,6 +3,7 @@ package main
 import (
 	"encoding/json"
 	"fmt"
+	"os"
 	"strings"
 
 	"github.com/pkg/errors"
@@ -10,7 +11,6 @@ import (
 	"github.com/projectatomic/libpod/libpod/image"
 	"github.com/sirupsen/logrus"
 	"github.com/urfave/cli"
-	"os"
 )
 
 var runDescription = "Runs a command in a new container from the given image"
@@ -94,6 +94,7 @@ func runCmd(c *cli.Context) error {
 	options = append(options, libpod.WithUser(createConfig.User))
 	options = append(options, libpod.WithShmDir(createConfig.ShmDir))
 	options = append(options, libpod.WithShmSize(createConfig.Resources.ShmSize))
+	options = append(options, libpod.WithGroups(createConfig.GroupAdd))
 
 	// Default used if not overridden on command line
 
diff --git a/cmd/podman/run_test.go b/cmd/podman/run_test.go
index 344fdcce5..3baee4615 100644
--- a/cmd/podman/run_test.go
+++ b/cmd/podman/run_test.go
@@ -101,7 +101,7 @@ func TestPIDsLimit(t *testing.T) {
 // TestBLKIOWeightDevice verifies the inputed blkio weigh device is correctly defined in the spec
 func TestBLKIOWeightDevice(t *testing.T) {
 	a := createCLI()
-	args := []string{"--blkio-weight-device", "/dev/sda:100"}
+	args := []string{"--blkio-weight-device", "/dev/zero:100"}
 	a.Run(append(cmd, args...))
 	runtimeSpec, err := getRuntimeSpec(CLI)
 	if err != nil {
diff --git a/cmd/podman/spec.go b/cmd/podman/spec.go
index 5e98d5b50..14fe3a38a 100644
--- a/cmd/podman/spec.go
+++ b/cmd/podman/spec.go
@@ -181,9 +181,7 @@ func createConfigToOCISpec(config *createConfig) (*spec.Spec, error) {
 	g.SetProcessCwd(config.WorkDir)
 	g.SetProcessArgs(config.Command)
 	g.SetProcessTerminal(config.Tty)
-	for _, gid := range config.GroupAdd {
-		g.AddProcessAdditionalGid(gid)
-	}
+
 	for key, val := range config.GetAnnotations() {
 		g.AddAnnotation(key, val)
 	}
@@ -369,7 +367,7 @@ func createConfigToOCISpec(config *createConfig) (*spec.Spec, error) {
 	}
 
 	// BLOCK IO
-	blkio, err := config.CreateBlockIO()
+	blkio, err := config.createBlockIO()
 	if err != nil {
 		return nil, errors.Wrapf(err, "error creating block io")
 	}
@@ -403,7 +401,12 @@ func createConfigToOCISpec(config *createConfig) (*spec.Spec, error) {
 	return configSpec, nil
 }
 
-func (c *createConfig) CreateBlockIO() (*spec.LinuxBlockIO, error) {
+const (
+	bps = iota
+	iops
+)
+
+func (c *createConfig) createBlockIO() (*spec.LinuxBlockIO, error) {
 	bio := &spec.LinuxBlockIO{}
 	bio.Weight = &c.Resources.BlkioWeight
 	if len(c.Resources.BlkioWeightDevice) > 0 {
@@ -413,7 +416,10 @@ func (c *createConfig) CreateBlockIO() (*spec.LinuxBlockIO, error) {
 			if err != nil {
 				return bio, errors.Wrapf(err, "invalid values for blkio-weight-device")
 			}
-			wdStat := getStatFromPath(wd.path)
+			wdStat, err := getStatFromPath(wd.path)
+			if err != nil {
+				return bio, errors.Wrapf(err, "error getting stat from path %q", wd.path)
+			}
 			lwd := spec.LinuxWeightDevice{
 				Weight: &wd.weight,
 			}
@@ -424,28 +430,28 @@ func (c *createConfig) CreateBlockIO() (*spec.LinuxBlockIO, error) {
 		bio.WeightDevice = lwds
 	}
 	if len(c.Resources.DeviceReadBps) > 0 {
-		readBps, err := makeThrottleArray(c.Resources.DeviceReadBps)
+		readBps, err := makeThrottleArray(c.Resources.DeviceReadBps, bps)
 		if err != nil {
 			return bio, err
 		}
 		bio.ThrottleReadBpsDevice = readBps
 	}
 	if len(c.Resources.DeviceWriteBps) > 0 {
-		writeBpds, err := makeThrottleArray(c.Resources.DeviceWriteBps)
+		writeBpds, err := makeThrottleArray(c.Resources.DeviceWriteBps, bps)
 		if err != nil {
 			return bio, err
 		}
 		bio.ThrottleWriteBpsDevice = writeBpds
 	}
 	if len(c.Resources.DeviceReadIOps) > 0 {
-		readIOps, err := makeThrottleArray(c.Resources.DeviceReadIOps)
+		readIOps, err := makeThrottleArray(c.Resources.DeviceReadIOps, iops)
 		if err != nil {
 			return bio, err
 		}
 		bio.ThrottleReadIOPSDevice = readIOps
 	}
 	if len(c.Resources.DeviceWriteIOps) > 0 {
-		writeIOps, err := makeThrottleArray(c.Resources.DeviceWriteIOps)
+		writeIOps, err := makeThrottleArray(c.Resources.DeviceWriteIOps, iops)
 		if err != nil {
 			return bio, err
 		}
@@ -454,6 +460,35 @@ func (c *createConfig) CreateBlockIO() (*spec.LinuxBlockIO, error) {
 	return bio, nil
 }
 
+func makeThrottleArray(throttleInput []string, rateType int) ([]spec.LinuxThrottleDevice, error) {
+	var (
+		ltds []spec.LinuxThrottleDevice
+		t    *throttleDevice
+		err  error
+	)
+	for _, i := range throttleInput {
+		if rateType == bps {
+			t, err = validateBpsDevice(i)
+		} else {
+			t, err = validateIOpsDevice(i)
+		}
+		if err != nil {
+			return []spec.LinuxThrottleDevice{}, err
+		}
+		ltdStat, err := getStatFromPath(t.path)
+		if err != nil {
+			return ltds, errors.Wrapf(err, "error getting stat from path %q", t.path)
+		}
+		ltd := spec.LinuxThrottleDevice{
+			Rate: t.rate,
+		}
+		ltd.Major = int64(unix.Major(ltdStat.Rdev))
+		ltd.Minor = int64(unix.Minor(ltdStat.Rdev))
+		ltds = append(ltds, ltd)
+	}
+	return ltds, nil
+}
+
 // GetAnnotations returns the all the annotations for the container
 func (c *createConfig) GetAnnotations() map[string]string {
 	a := getDefaultAnnotations()
@@ -668,27 +703,10 @@ func (c *createConfig) GetContainerCreateOptions() ([]libpod.CtrCreateOption, er
 	return options, nil
 }
 
-func getStatFromPath(path string) unix.Stat_t {
+func getStatFromPath(path string) (unix.Stat_t, error) {
 	s := unix.Stat_t{}
-	_ = unix.Stat(path, &s)
-	return s
-}
-
-func makeThrottleArray(throttleInput []string) ([]spec.LinuxThrottleDevice, error) {
-	var ltds []spec.LinuxThrottleDevice
-	for _, i := range throttleInput {
-		t, err := validateBpsDevice(i)
-		if err != nil {
-			return []spec.LinuxThrottleDevice{}, err
-		}
-		ltd := spec.LinuxThrottleDevice{}
-		ltd.Rate = t.rate
-		ltdStat := getStatFromPath(t.path)
-		ltd.Major = int64(unix.Major(ltdStat.Rdev))
-		ltd.Minor = int64(unix.Major(ltdStat.Rdev))
-		ltds = append(ltds, ltd)
-	}
-	return ltds, nil
+	err := unix.Stat(path, &s)
+	return s, err
 }
 
 // CreatePortBindings iterates ports mappings and exposed ports into a format CNI understands
diff --git a/libpod/container.go b/libpod/container.go
index cbfa09538..f778933c2 100644
--- a/libpod/container.go
+++ b/libpod/container.go
@@ -201,6 +201,8 @@ type ContainerConfig struct {
 	// User and group to use in the container
 	// Can be specified by name or UID/GID
 	User string `json:"user,omitempty"`
+	// Additional groups to add
+	Groups []string `json:"groups, omitempty"`
 
 	// Namespace Config
 	// IDs of container to share namespaces with
diff --git a/libpod/container_internal.go b/libpod/container_internal.go
index f3247b1c0..c9454db8a 100644
--- a/libpod/container_internal.go
+++ b/libpod/container_internal.go
@@ -8,6 +8,7 @@ import (
 	"os"
 	"path/filepath"
 	"regexp"
+	"strconv"
 	"strings"
 	"syscall"
 	"time"
@@ -956,6 +957,29 @@ func (c *Container) generateSpec() (*spec.Spec, error) {
 		g.SetProcessGID(gid)
 	}
 
+	// Add addition groups if c.config.GroupAdd is not empty
+	if len(c.config.Groups) > 0 {
+		if !c.state.Mounted {
+			return nil, errors.Wrapf(ErrCtrStateInvalid, "container %s must be mounted in order to add additional groups", c.ID())
+		}
+		for _, group := range c.config.Groups {
+			_, gid, err := chrootuser.GetUser(c.state.Mountpoint, strconv.Itoa(int(g.Spec().Process.User.UID))+":"+group)
+			if err != nil {
+				return nil, err
+			}
+			g.AddProcessAdditionalGid(uint32(gid))
+		}
+	}
+
+	// Look up and add groups the user belongs to
+	groups, err := chrootuser.GetAdditionalGroupsForUser(c.state.Mountpoint, uint64(g.Spec().Process.User.UID))
+	if err != nil {
+		return nil, err
+	}
+	for _, gid := range groups {
+		g.AddProcessAdditionalGid(gid)
+	}
+
 	// Add shared namespaces from other containers
 	if c.config.IPCNsCtr != "" {
 		if err := c.addNamespaceContainer(&g, IPCNS, c.config.IPCNsCtr, spec.IPCNamespace); err != nil {
diff --git a/libpod/options.go b/libpod/options.go
index f9d6cb211..2c51b5834 100644
--- a/libpod/options.go
+++ b/libpod/options.go
@@ -823,3 +823,14 @@ func WithConmonPidFile(path string) CtrCreateOption {
 		return nil
 	}
 }
+
+// WithGroups sets additional groups for the container, which are defined by the user
+func WithGroups(groups []string) CtrCreateOption {
+	return func(ctr *Container) error {
+		if ctr.valid {
+			return ErrCtrFinalized
+		}
+		ctr.config.Groups = groups
+		return nil
+	}
+}
diff --git a/pkg/chrootuser/user.go b/pkg/chrootuser/user.go
index 9be386ef9..22ba5ff8a 100644
--- a/pkg/chrootuser/user.go
+++ b/pkg/chrootuser/user.go
@@ -69,3 +69,8 @@ func GetUser(rootdir, userspec string) (uint32, uint32, error) {
 	}
 	return 0, 0, err
 }
+
+// GetAdditionalGroupsForUser returns a list of gids that userid is associated with
+func GetAdditionalGroupsForUser(rootdir string, userid uint64) ([]uint32, error) {
+	return lookupAdditionalGroupsForUIDInContainer(rootdir, userid)
+}
diff --git a/pkg/chrootuser/user_linux.go b/pkg/chrootuser/user_linux.go
index 2baf9ea33..64ff7cef6 100644
--- a/pkg/chrootuser/user_linux.go
+++ b/pkg/chrootuser/user_linux.go
@@ -88,6 +88,7 @@ type lookupPasswdEntry struct {
 type lookupGroupEntry struct {
 	name string
 	gid  uint64
+	user string
 }
 
 func readWholeLine(rc *bufio.Reader) ([]byte, error) {
@@ -153,6 +154,7 @@ func parseNextGroup(rc *bufio.Reader) *lookupGroupEntry {
 	return &lookupGroupEntry{
 		name: fields[0],
 		gid:  gid,
+		user: fields[3],
 	}
 }
 
@@ -208,6 +210,36 @@ func lookupGroupForUIDInContainer(rootdir string, userid uint64) (username strin
 	return "", 0, user.UnknownUserError(fmt.Sprintf("error looking up user with UID %d", userid))
 }
 
+func lookupAdditionalGroupsForUIDInContainer(rootdir string, userid uint64) (gid []uint32, err error) {
+	// Get the username associated with userid
+	username, _, err := lookupGroupForUIDInContainer(rootdir, userid)
+	if err != nil {
+		return nil, err
+	}
+
+	cmd, f, err := openChrootedFile(rootdir, "/etc/group")
+	if err != nil {
+		return nil, err
+	}
+	defer func() {
+		_ = cmd.Wait()
+	}()
+	rc := bufio.NewReader(f)
+	defer f.Close()
+
+	lookupGroup.Lock()
+	defer lookupGroup.Unlock()
+
+	grp := parseNextGroup(rc)
+	for grp != nil {
+		if strings.Contains(grp.user, username) {
+			gid = append(gid, uint32(grp.gid))
+		}
+		grp = parseNextGroup(rc)
+	}
+	return gid, nil
+}
+
 func lookupGroupInContainer(rootdir, groupname string) (gid uint64, err error) {
 	cmd, f, err := openChrootedFile(rootdir, "/etc/group")
 	if err != nil {
diff --git a/test/e2e/run_test.go b/test/e2e/run_test.go
index 21c577d9a..a9b2a474f 100644
--- a/test/e2e/run_test.go
+++ b/test/e2e/run_test.go
@@ -185,6 +185,34 @@ var _ = Describe("Podman run", func() {
 		Expect(session.OutputToString()).To(ContainSubstring("15"))
 	})
 
+	It("podman run device-read-bps test", func() {
+		session := podmanTest.Podman([]string{"run", "--rm", "--device-read-bps=/dev/zero:1mb", ALPINE, "cat", "/sys/fs/cgroup/blkio/blkio.throttle.read_bps_device"})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(0))
+		Expect(session.OutputToString()).To(ContainSubstring("1048576"))
+	})
+
+	It("podman run device-write-bps test", func() {
+		session := podmanTest.Podman([]string{"run", "--rm", "--device-write-bps=/dev/zero:1mb", ALPINE, "cat", "/sys/fs/cgroup/blkio/blkio.throttle.write_bps_device"})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(0))
+		Expect(session.OutputToString()).To(ContainSubstring("1048576"))
+	})
+
+	It("podman run device-read-iops test", func() {
+		session := podmanTest.Podman([]string{"run", "--rm", "--device-read-iops=/dev/zero:100", ALPINE, "cat", "/sys/fs/cgroup/blkio/blkio.throttle.read_iops_device"})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(0))
+		Expect(session.OutputToString()).To(ContainSubstring("100"))
+	})
+
+	It("podman run device-write-iops test", func() {
+		session := podmanTest.Podman([]string{"run", "--rm", "--device-write-iops=/dev/zero:100", ALPINE, "cat", "/sys/fs/cgroup/blkio/blkio.throttle.write_iops_device"})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(0))
+		Expect(session.OutputToString()).To(ContainSubstring("100"))
+	})
+
 	It("podman run notify_socket", func() {
 		sock := "/run/sock"
 		os.Setenv("NOTIFY_SOCKET", sock)
@@ -258,4 +286,18 @@ var _ = Describe("Podman run", func() {
 		Expect(err).To(BeNil())
 	})
 
+	It("podman run without group-add", func() {
+		session := podmanTest.Podman([]string{"run", "--rm", ALPINE, "id"})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(0))
+		Expect(session.OutputToString()).To(Equal("uid=0(root) gid=0(root) groups=0(root),1(bin),2(daemon),3(sys),4(adm),6(disk),10(wheel),11(floppy),20(dialout),26(tape),27(video)"))
+	})
+
+	It("podman run with group-add", func() {
+		session := podmanTest.Podman([]string{"run", "--rm", "--group-add=audio", "--group-add=nogroup", "--group-add=777", ALPINE, "id"})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(0))
+		Expect(session.OutputToString()).To(Equal("uid=0(root) gid=0(root) groups=0(root),1(bin),2(daemon),3(sys),4(adm),6(disk),10(wheel),11(floppy),18(audio),20(dialout),26(tape),27(video),777,65533(nogroup)"))
+	})
+
 })