summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorMatthew Heon <matthew.heon@pm.me>2020-02-14 15:46:18 -0500
committerMatthew Heon <matthew.heon@pm.me>2020-02-14 16:06:20 -0500
commit36a0ed9702bf4e6ef50650404c838a26f13ba879 (patch)
tree316a75c4cc66f02fed91cb40918e6396b314bc54
parent97fdfd0a80b14b83abca3d8e0aa1b5c64ceac79b (diff)
downloadpodman-36a0ed9702bf4e6ef50650404c838a26f13ba879.tar.gz
podman-36a0ed9702bf4e6ef50650404c838a26f13ba879.tar.bz2
podman-36a0ed9702bf4e6ef50650404c838a26f13ba879.zip
Rework label parsing
We attempted to share all logic for parsing labels and environment variables, which on the surface makes lots of sense (both are formatted key=value so parsing logic should be identical) but has begun to fall apart now that we have added additional logic to environment variable handling. Environment variables that are unset, for example, are looked up against environment variables set for the process. We don't want this for labels, so we have to split parsing logic. Fixes #3854 Signed-off-by: Matthew Heon <matthew.heon@pm.me>
-rw-r--r--cmd/podman/pod_create.go3
-rw-r--r--cmd/podman/shared/create.go2
-rw-r--r--cmd/podman/shared/create_cli.go11
-rw-r--r--cmd/podman/shared/create_cli_test.go51
-rw-r--r--cmd/podman/shared/parse/parse.go28
-rw-r--r--cmd/podman/shared/parse/parse_test.go53
-rw-r--r--cmd/podman/volume_create.go6
-rw-r--r--pkg/api/handlers/libpod/pods.go3
-rw-r--r--test/e2e/create_test.go38
9 files changed, 127 insertions, 68 deletions
diff --git a/cmd/podman/pod_create.go b/cmd/podman/pod_create.go
index cee6476ea..0f72780f9 100644
--- a/cmd/podman/pod_create.go
+++ b/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")
}
diff --git a/cmd/podman/shared/create.go b/cmd/podman/shared/create.go
index be5adcccb..99538b3dc 100644
--- a/cmd/podman/shared/create.go
+++ b/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")
}
diff --git a/cmd/podman/shared/create_cli.go b/cmd/podman/shared/create_cli.go
index 00b83906d..10e27350b 100644
--- a/cmd/podman/shared/create_cli.go
+++ b/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)
diff --git a/cmd/podman/shared/create_cli_test.go b/cmd/podman/shared/create_cli_test.go
index fea1a2390..a045962cb 100644
--- a/cmd/podman/shared/create_cli_test.go
+++ b/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)
-}
diff --git a/cmd/podman/shared/parse/parse.go b/cmd/podman/shared/parse/parse.go
index 3a75ff7a8..79449029d 100644
--- a/cmd/podman/shared/parse/parse.go
+++ b/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
diff --git a/cmd/podman/shared/parse/parse_test.go b/cmd/podman/shared/parse/parse_test.go
index 1359076a0..a6ddc2be9 100644
--- a/cmd/podman/shared/parse/parse_test.go
+++ b/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)
+}
diff --git a/cmd/podman/volume_create.go b/cmd/podman/volume_create.go
index e5a576749..52189657b 100644
--- a/cmd/podman/volume_create.go
+++ b/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")
}
diff --git a/pkg/api/handlers/libpod/pods.go b/pkg/api/handlers/libpod/pods.go
index e9297d91b..d043b1204 100644
--- a/pkg/api/handlers/libpod/pods.go
+++ b/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
}
diff --git a/test/e2e/create_test.go b/test/e2e/create_test.go
index 134b7b162..10742a0e8 100644
--- a/test/e2e/create_test.go
+++ b/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"))
+ })
})