Merge pull request #5217 from mheon/rework_label_parsing

Rework label parsing

cmd/podman/pod_create.go

@@ -6,6 +6,7 @@ import (
 
 	"github.com/containers/libpod/cmd/podman/cliconfig"
 	"github.com/containers/libpod/cmd/podman/shared"
+	"github.com/containers/libpod/cmd/podman/shared/parse"
 	"github.com/containers/libpod/libpod/define"
 	"github.com/containers/libpod/pkg/adapter"
 	"github.com/containers/libpod/pkg/errorhandling"
@@ -103,7 +104,7 @@ func podCreateCmd(c *cliconfig.PodCreateValues) error {
 		defer errorhandling.SyncQuiet(podIdFile)
 	}
 
-	labels, err := shared.GetAllLabels(c.LabelFile, c.Labels)
+	labels, err := parse.GetAllLabels(c.LabelFile, c.Labels)
 	if err != nil {
 		return errors.Wrapf(err, "unable to process labels")
 	}

cmd/podman/shared/create.go

@@ -488,7 +488,7 @@ func ParseCreateOpts(ctx context.Context, c *GenericCLIResults, runtime *libpod.
 	}
 
 	// LABEL VARIABLES
-	labels, err := GetAllLabels(c.StringSlice("label-file"), c.StringArray("label"))
+	labels, err := parse.GetAllLabels(c.StringSlice("label-file"), c.StringArray("label"))
 	if err != nil {
 		return nil, errors.Wrapf(err, "unable to process labels")
 	}

cmd/podman/shared/create_cli.go

@@ -4,7 +4,6 @@ import (
 	"fmt"
 	"strings"
 
-	"github.com/containers/libpod/cmd/podman/shared/parse"
 	"github.com/containers/libpod/pkg/cgroups"
 	cc "github.com/containers/libpod/pkg/spec"
 	"github.com/containers/libpod/pkg/sysinfo"
@@ -12,16 +11,6 @@ import (
 	"github.com/sirupsen/logrus"
 )
 
-// GetAllLabels ...
-func GetAllLabels(labelFile, inputLabels []string) (map[string]string, error) {
-	labels := make(map[string]string)
-	labelErr := parse.ReadKVStrings(labels, labelFile, inputLabels)
-	if labelErr != nil {
-		return labels, errors.Wrapf(labelErr, "unable to process labels from --label and label-file")
-	}
-	return labels, nil
-}
-
 // validateSysctl validates a sysctl and returns it.
 func validateSysctl(strSlice []string) (map[string]string, error) {
 	sysctl := make(map[string]string)

cmd/podman/shared/create_cli_test.go

@@ -1,33 +1,11 @@
 package shared
 
 import (
-	"io/ioutil"
-	"os"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 )
 
-var (
-	Var1 = []string{"ONE=1", "TWO=2"}
-)
-
-func createTmpFile(content []byte) (string, error) {
-	tmpfile, err := ioutil.TempFile(os.TempDir(), "unittest")
-	if err != nil {
-		return "", err
-	}
-
-	if _, err := tmpfile.Write(content); err != nil {
-		return "", err
-
-	}
-	if err := tmpfile.Close(); err != nil {
-		return "", err
-	}
-	return tmpfile.Name(), nil
-}
-
 func TestValidateSysctl(t *testing.T) {
 	strSlice := []string{"net.core.test1=4", "kernel.msgmax=2"}
 	result, _ := validateSysctl(strSlice)
@@ -39,32 +17,3 @@ func TestValidateSysctlBadSysctl(t *testing.T) {
 	_, err := validateSysctl(strSlice)
 	assert.Error(t, err)
 }
-
-func TestGetAllLabels(t *testing.T) {
-	fileLabels := []string{}
-	labels, _ := GetAllLabels(fileLabels, Var1)
-	assert.Equal(t, len(labels), 2)
-}
-
-func TestGetAllLabelsBadKeyValue(t *testing.T) {
-	inLabels := []string{"=badValue", "="}
-	fileLabels := []string{}
-	_, err := GetAllLabels(fileLabels, inLabels)
-	assert.Error(t, err, assert.AnError)
-}
-
-func TestGetAllLabelsBadLabelFile(t *testing.T) {
-	fileLabels := []string{"/foobar5001/be"}
-	_, err := GetAllLabels(fileLabels, Var1)
-	assert.Error(t, err, assert.AnError)
-}
-
-func TestGetAllLabelsFile(t *testing.T) {
-	content := []byte("THREE=3")
-	tFile, err := createTmpFile(content)
-	defer os.Remove(tFile)
-	assert.NoError(t, err)
-	fileLabels := []string{tFile}
-	result, _ := GetAllLabels(fileLabels, Var1)
-	assert.Equal(t, len(result), 3)
-}

cmd/podman/shared/parse/parse.go

@@ -79,6 +79,34 @@ func ValidateDomain(val string) (string, error) {
 	return "", fmt.Errorf("%s is not a valid domain", val)
 }
 
+// GetAllLabels retrieves all labels given a potential label file and a number
+// of labels provided from the command line.
+func GetAllLabels(labelFile, inputLabels []string) (map[string]string, error) {
+	labels := make(map[string]string)
+	for _, file := range labelFile {
+		// Use of parseEnvFile still seems safe, as it's missing the
+		// extra parsing logic of parseEnv.
+		// There's an argument that we SHOULD be doing that parsing for
+		// all environment variables, even those sourced from files, but
+		// that would require a substantial rework.
+		if err := parseEnvFile(labels, file); err != nil {
+			return nil, err
+		}
+	}
+	for _, label := range inputLabels {
+		split := strings.SplitN(label, "=", 2)
+		if split[0] == "" {
+			return nil, errors.Errorf("invalid label format: %q", label)
+		}
+		value := ""
+		if len(split) > 1 {
+			value = split[1]
+		}
+		labels[split[0]] = value
+	}
+	return labels, nil
+}
+
 // reads a file of line terminated key=value pairs, and overrides any keys
 // present in the file with additional pairs specified in the override parameter
 // for env-file and labels-file flags

cmd/podman/shared/parse/parse_test.go

@@ -4,9 +4,33 @@
 package parse
 
 import (
+	"io/ioutil"
+	"os"
 	"testing"
+
+	"github.com/stretchr/testify/assert"
 )
 
+var (
+	Var1 = []string{"ONE=1", "TWO=2"}
+)
+
+func createTmpFile(content []byte) (string, error) {
+	tmpfile, err := ioutil.TempFile(os.TempDir(), "unittest")
+	if err != nil {
+		return "", err
+	}
+
+	if _, err := tmpfile.Write(content); err != nil {
+		return "", err
+
+	}
+	if err := tmpfile.Close(); err != nil {
+		return "", err
+	}
+	return tmpfile.Name(), nil
+}
+
 func TestValidateExtraHost(t *testing.T) {
 	type args struct {
 		val string
@@ -97,3 +121,32 @@ func TestValidateFileName(t *testing.T) {
 		})
 	}
 }
+
+func TestGetAllLabels(t *testing.T) {
+	fileLabels := []string{}
+	labels, _ := GetAllLabels(fileLabels, Var1)
+	assert.Equal(t, len(labels), 2)
+}
+
+func TestGetAllLabelsBadKeyValue(t *testing.T) {
+	inLabels := []string{"=badValue", "="}
+	fileLabels := []string{}
+	_, err := GetAllLabels(fileLabels, inLabels)
+	assert.Error(t, err, assert.AnError)
+}
+
+func TestGetAllLabelsBadLabelFile(t *testing.T) {
+	fileLabels := []string{"/foobar5001/be"}
+	_, err := GetAllLabels(fileLabels, Var1)
+	assert.Error(t, err, assert.AnError)
+}
+
+func TestGetAllLabelsFile(t *testing.T) {
+	content := []byte("THREE=3")
+	tFile, err := createTmpFile(content)
+	defer os.Remove(tFile)
+	assert.NoError(t, err)
+	fileLabels := []string{tFile}
+	result, _ := GetAllLabels(fileLabels, Var1)
+	assert.Equal(t, len(result), 3)
+}

cmd/podman/volume_create.go

@@ -4,7 +4,7 @@ import (
 	"fmt"
 
 	"github.com/containers/libpod/cmd/podman/cliconfig"
-	"github.com/containers/libpod/cmd/podman/shared"
+	"github.com/containers/libpod/cmd/podman/shared/parse"
 	"github.com/containers/libpod/pkg/adapter"
 	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
@@ -51,12 +51,12 @@ func volumeCreateCmd(c *cliconfig.VolumeCreateValues) error {
 		return errors.Errorf("too many arguments, create takes at most 1 argument")
 	}
 
-	labels, err := shared.GetAllLabels([]string{}, c.Label)
+	labels, err := parse.GetAllLabels([]string{}, c.Label)
 	if err != nil {
 		return errors.Wrapf(err, "unable to process labels")
 	}
 
-	opts, err := shared.GetAllLabels([]string{}, c.Opt)
+	opts, err := parse.GetAllLabels([]string{}, c.Opt)
 	if err != nil {
 		return errors.Wrapf(err, "unable to process options")
 	}

pkg/api/handlers/libpod/pods.go

@@ -42,7 +42,8 @@ func PodCreate(w http.ResponseWriter, r *http.Request) {
 	}
 
 	if len(input.Labels) > 0 {
-		if err := parse.ReadKVStrings(labels, []string{}, input.Labels); err != nil {
+		labels, err = parse.GetAllLabels([]string{}, input.Labels)
+		if err != nil {
 			utils.Error(w, "Something went wrong.", http.StatusInternalServerError, err)
 			return
 		}

test/e2e/create_test.go

@@ -304,4 +304,42 @@ var _ = Describe("Podman create", func() {
 		session.WaitWithDefaultTimeout()
 		Expect(session).To(Not(Equal(0)))
 	})
+
+	It("podman create with unset label", func() {
+		// Alpine is assumed to have no labels here, which seems safe
+		ctrName := "testctr"
+		session := podmanTest.Podman([]string{"create", "--label", "TESTKEY1=", "--label", "TESTKEY2", "--name", ctrName, ALPINE, "top"})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(0))
+
+		inspect := podmanTest.Podman([]string{"inspect", ctrName})
+		inspect.WaitWithDefaultTimeout()
+		data := inspect.InspectContainerToJSON()
+		Expect(len(data)).To(Equal(1))
+		Expect(len(data[0].Config.Labels)).To(Equal(2))
+		_, ok1 := data[0].Config.Labels["TESTKEY1"]
+		Expect(ok1).To(BeTrue())
+		_, ok2 := data[0].Config.Labels["TESTKEY2"]
+		Expect(ok2).To(BeTrue())
+	})
+
+	It("podman create with set label", func() {
+		// Alpine is assumed to have no labels here, which seems safe
+		ctrName := "testctr"
+		session := podmanTest.Podman([]string{"create", "--label", "TESTKEY1=value1", "--label", "TESTKEY2=bar", "--name", ctrName, ALPINE, "top"})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(0))
+
+		inspect := podmanTest.Podman([]string{"inspect", ctrName})
+		inspect.WaitWithDefaultTimeout()
+		data := inspect.InspectContainerToJSON()
+		Expect(len(data)).To(Equal(1))
+		Expect(len(data[0].Config.Labels)).To(Equal(2))
+		val1, ok1 := data[0].Config.Labels["TESTKEY1"]
+		Expect(ok1).To(BeTrue())
+		Expect(val1).To(Equal("value1"))
+		val2, ok2 := data[0].Config.Labels["TESTKEY2"]
+		Expect(ok2).To(BeTrue())
+		Expect(val2).To(Equal("bar"))
+	})
 })