Labels set on the command line always override labels in Dockerfile

This fix tries to address the inconsistency in #22036 where labels
set on the command line will not override labels specified in
Dockerfile, but will override labels inherited from `FROM` images.

The fix add a LABEL with command line options at the end of the
processed Dockerfile so that command line options labels always
override the LABEL in Dockerfiles (or through `FROM`).

An integration test has been added for test cases specified in #22036.

This fix fixes #22036.

NOTE: Some changes are from #22266 (@tiborvass).

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

builder/dockerfile/builder.go

@@ -212,12 +212,20 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri
 		return "", err
 	}
 
+	if len(b.options.Labels) > 0 {
+		line := "LABEL "
+		for k, v := range b.options.Labels {
+			line += fmt.Sprintf("%q=%q ", k, v)
+		}
+		_, node, err := parser.ParseLine(line)
+		if err != nil {
+			return "", err
+		}
+		b.dockerfile.Children = append(b.dockerfile.Children, node)
+	}
+
 	var shortImgID string
 	for i, n := range b.dockerfile.Children {
-		// we only want to add labels to the last layer
-		if i == len(b.dockerfile.Children)-1 {
-			b.addLabels()
-		}
 		select {
 		case <-b.clientCtx.Done():
 			logrus.Debug("Builder: build cancelled!")
@@ -233,16 +241,6 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri
 			return "", err
 		}
 
-		// Commit the layer when there are only one children in
-		// the dockerfile, this is only the `FROM` tag, and
-		// build labels. Otherwise, the new image won't be
-		// labeled properly.
-		// Commit here, so the ID of the final image is reported
-		// properly.
-		if len(b.dockerfile.Children) == 1 && len(b.options.Labels) > 0 {
-			b.commit("", b.runConfig.Cmd, "")
-		}
-
 		shortImgID = stringid.TruncateID(b.image)
 		fmt.Fprintf(b.Stdout, " ---> %s\n", shortImgID)
 		if b.options.Remove {

builder/dockerfile/internals.go

@@ -40,19 +40,6 @@ import (
 	"github.com/docker/engine-api/types/strslice"
 )
 
-func (b *Builder) addLabels() {
-	// merge labels
-	if len(b.options.Labels) > 0 {
-		logrus.Debugf("[BUILDER] setting labels %v", b.options.Labels)
-		if b.runConfig.Labels == nil {
-			b.runConfig.Labels = make(map[string]string)
-		}
-		for kL, vL := range b.options.Labels {
-			b.runConfig.Labels[kL] = vL
-		}
-	}
-}
-
 func (b *Builder) commit(id string, autoCmd strslice.StrSlice, comment string) error {
 	if b.disableCommit {
 		return nil
@@ -418,20 +405,7 @@ func (b *Builder) processImageFrom(img builder.Image) error {
 		b.image = img.ImageID()
 
 		if img.RunConfig() != nil {
-			imgConfig := *img.RunConfig()
-			// inherit runConfig labels from the current
-			// state if they've been set already.
-			// Ensures that images with only a FROM
-			// get the labels populated properly.
-			if b.runConfig.Labels != nil {
-				if imgConfig.Labels == nil {
-					imgConfig.Labels = make(map[string]string)
-				}
-				for k, v := range b.runConfig.Labels {
-					imgConfig.Labels[k] = v
-				}
-			}
-			b.runConfig = &imgConfig
+			b.runConfig = img.RunConfig()
 		}
 	}
 

builder/dockerfile/parser/line_parsers.go

@@ -34,7 +34,7 @@ func parseSubCommand(rest string) (*Node, map[string]bool, error) {
 		return nil, nil, nil
 	}
 
-	_, child, err := parseLine(rest)
+	_, child, err := ParseLine(rest)
 	if err != nil {
 		return nil, nil, err
 	}

builder/dockerfile/parser/parser.go

@@ -68,8 +68,8 @@ func init() {
 	}
 }
 
-// parse a line and return the remainder.
-func parseLine(line string) (string, *Node, error) {
+// ParseLine parse a line and return the remainder.
+func ParseLine(line string) (string, *Node, error) {
 	if line = stripComments(line); line == "" {
 		return "", nil, nil
 	}
@@ -111,7 +111,7 @@ func Parse(rwc io.Reader) (*Node, error) {
 	for scanner.Scan() {
 		scannedLine := strings.TrimLeftFunc(scanner.Text(), unicode.IsSpace)
 		currentLine++
-		line, child, err := parseLine(scannedLine)
+		line, child, err := ParseLine(scannedLine)
 		if err != nil {
 			return nil, err
 		}
@@ -126,7 +126,7 @@ func Parse(rwc io.Reader) (*Node, error) {
 					continue
 				}
 
-				line, child, err = parseLine(line + newline)
+				line, child, err = ParseLine(line + newline)
 				if err != nil {
 					return nil, err
 				}
@@ -136,7 +136,7 @@ func Parse(rwc io.Reader) (*Node, error) {
 				}
 			}
 			if child == nil && line != "" {
-				_, child, err = parseLine(line)
+				_, child, err = ParseLine(line)
 				if err != nil {
 					return nil, err
 				}

integration-cli/docker_cli_build_test.go

@@ -6894,3 +6894,94 @@ func (s *DockerRegistryAuthHtpasswdSuite) TestBuildWithExternalAuth(c *check.C)
 	out, _, err := runCommandWithOutput(buildCmd)
 	c.Assert(err, check.IsNil, check.Commentf(out))
 }
+
+// Test cases in #22036
+func (s *DockerSuite) TestBuildLabelsOverride(c *check.C) {
+	testRequires(c, DaemonIsLinux)
+
+	// Command line option labels will always override
+	name := "scratchy"
+	expected := `{"bar":"from-flag","foo":"from-flag"}`
+	_, err := buildImage(name,
+		`FROM scratch
+                LABEL foo=from-dockerfile`,
+		true, "--label", "foo=from-flag", "--label", "bar=from-flag")
+	c.Assert(err, check.IsNil)
+
+	res := inspectFieldJSON(c, name, "Config.Labels")
+	if res != expected {
+		c.Fatalf("Labels %s, expected %s", res, expected)
+	}
+
+	name = "from"
+	expected = `{"foo":"from-dockerfile"}`
+	_, err = buildImage(name,
+		`FROM scratch
+                LABEL foo from-dockerfile`,
+		true)
+	c.Assert(err, check.IsNil)
+
+	res = inspectFieldJSON(c, name, "Config.Labels")
+	if res != expected {
+		c.Fatalf("Labels %s, expected %s", res, expected)
+	}
+
+	// Command line option label will override even via `FROM`
+	name = "new"
+	expected = `{"bar":"from-dockerfile2","foo":"new"}`
+	_, err = buildImage(name,
+		`FROM from
+                LABEL bar from-dockerfile2`,
+		true, "--label", "foo=new")
+	c.Assert(err, check.IsNil)
+
+	res = inspectFieldJSON(c, name, "Config.Labels")
+	if res != expected {
+		c.Fatalf("Labels %s, expected %s", res, expected)
+	}
+
+	// Command line option without a value set (--label foo, --label bar=)
+	// will be treated as --label foo="", --label bar=""
+	name = "scratchy2"
+	expected = `{"bar":"","foo":""}`
+	_, err = buildImage(name,
+		`FROM scratch
+                LABEL foo=from-dockerfile`,
+		true, "--label", "foo", "--label", "bar=")
+	c.Assert(err, check.IsNil)
+
+	res = inspectFieldJSON(c, name, "Config.Labels")
+	if res != expected {
+		c.Fatalf("Labels %s, expected %s", res, expected)
+	}
+
+	// Command line option without a value set (--label foo, --label bar=)
+	// will be treated as --label foo="", --label bar=""
+	// This time is for inherited images
+	name = "new2"
+	expected = `{"bar":"","foo":""}`
+	_, err = buildImage(name,
+		`FROM from
+                LABEL bar from-dockerfile2`,
+		true, "--label", "foo=", "--label", "bar")
+	c.Assert(err, check.IsNil)
+
+	res = inspectFieldJSON(c, name, "Config.Labels")
+	if res != expected {
+		c.Fatalf("Labels %s, expected %s", res, expected)
+	}
+
+	// Command line option labels with only `FROM`
+	name = "scratchy"
+	expected = `{"bar":"from-flag","foo":"from-flag"}`
+	_, err = buildImage(name,
+		`FROM scratch`,
+		true, "--label", "foo=from-flag", "--label", "bar=from-flag")
+	c.Assert(err, check.IsNil)
+
+	res = inspectFieldJSON(c, name, "Config.Labels")
+	if res != expected {
+		c.Fatalf("Labels %s, expected %s", res, expected)
+	}
+
+}