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>
This commit is contained in:
Matthew Heon
2020-02-14 15:46:18 -05:00
parent 97fdfd0a80
commit 36a0ed9702
9 changed files with 127 additions and 68 deletions

View File

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

View File

@ -488,7 +488,7 @@ func ParseCreateOpts(ctx context.Context, c *GenericCLIResults, runtime *libpod.
} }
// LABEL VARIABLES // 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 { if err != nil {
return nil, errors.Wrapf(err, "unable to process labels") return nil, errors.Wrapf(err, "unable to process labels")
} }

View File

@ -4,7 +4,6 @@ import (
"fmt" "fmt"
"strings" "strings"
"github.com/containers/libpod/cmd/podman/shared/parse"
"github.com/containers/libpod/pkg/cgroups" "github.com/containers/libpod/pkg/cgroups"
cc "github.com/containers/libpod/pkg/spec" cc "github.com/containers/libpod/pkg/spec"
"github.com/containers/libpod/pkg/sysinfo" "github.com/containers/libpod/pkg/sysinfo"
@ -12,16 +11,6 @@ import (
"github.com/sirupsen/logrus" "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. // validateSysctl validates a sysctl and returns it.
func validateSysctl(strSlice []string) (map[string]string, error) { func validateSysctl(strSlice []string) (map[string]string, error) {
sysctl := make(map[string]string) sysctl := make(map[string]string)

View File

@ -1,33 +1,11 @@
package shared package shared
import ( import (
"io/ioutil"
"os"
"testing" "testing"
"github.com/stretchr/testify/assert" "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) { func TestValidateSysctl(t *testing.T) {
strSlice := []string{"net.core.test1=4", "kernel.msgmax=2"} strSlice := []string{"net.core.test1=4", "kernel.msgmax=2"}
result, _ := validateSysctl(strSlice) result, _ := validateSysctl(strSlice)
@ -39,32 +17,3 @@ func TestValidateSysctlBadSysctl(t *testing.T) {
_, err := validateSysctl(strSlice) _, err := validateSysctl(strSlice)
assert.Error(t, err) 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)
}

View File

@ -79,6 +79,34 @@ func ValidateDomain(val string) (string, error) {
return "", fmt.Errorf("%s is not a valid domain", val) 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 // 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 // present in the file with additional pairs specified in the override parameter
// for env-file and labels-file flags // for env-file and labels-file flags

View File

@ -4,9 +4,33 @@
package parse package parse
import ( import (
"io/ioutil"
"os"
"testing" "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) { func TestValidateExtraHost(t *testing.T) {
type args struct { type args struct {
val string 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)
}

View File

@ -4,7 +4,7 @@ import (
"fmt" "fmt"
"github.com/containers/libpod/cmd/podman/cliconfig" "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/containers/libpod/pkg/adapter"
"github.com/pkg/errors" "github.com/pkg/errors"
"github.com/spf13/cobra" "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") 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 { if err != nil {
return errors.Wrapf(err, "unable to process labels") 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 { if err != nil {
return errors.Wrapf(err, "unable to process options") return errors.Wrapf(err, "unable to process options")
} }

View File

@ -42,7 +42,8 @@ func PodCreate(w http.ResponseWriter, r *http.Request) {
} }
if len(input.Labels) > 0 { 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) utils.Error(w, "Something went wrong.", http.StatusInternalServerError, err)
return return
} }

View File

@ -304,4 +304,42 @@ var _ = Describe("Podman create", func() {
session.WaitWithDefaultTimeout() session.WaitWithDefaultTimeout()
Expect(session).To(Not(Equal(0))) 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"))
})
}) })