Merge pull request #35470 from thaJeztah/error-on-duplicate-labels

Remove deprecated support for duplicate label-keys

cmd/dockerd/daemon.go

@@ -480,22 +480,12 @@ func loadDaemonCliConfig(opts *daemonOptions) (*config.Config, error) {
 		logrus.Warnf(`The "-g / --graph" flag is deprecated. Please use "--data-root" instead`)
 	}
 
-	// Labels of the docker engine used to allow multiple values associated with the same key.
-	// This is deprecated in 1.13, and, be removed after 3 release cycles.
-	// The following will check the conflict of labels, and report a warning for deprecation.
-	//
-	// TODO: After 3 release cycles (17.12) an error will be returned, and labels will be
-	// sanitized to consolidate duplicate key-value pairs (config.Labels = newLabels):
-	//
-	// newLabels, err := daemon.GetConflictFreeLabels(config.Labels)
-	// if err != nil {
-	//	return nil, err
-	// }
-	// config.Labels = newLabels
-	//
-	if _, err := config.GetConflictFreeLabels(conf.Labels); err != nil {
-		logrus.Warnf("Engine labels with duplicate keys and conflicting values have been deprecated: %s", err)
+	// Check if duplicate label-keys with different values are found
+	newLabels, err := config.GetConflictFreeLabels(conf.Labels)
+	if err != nil {
+		return nil, err
 	}
+	conf.Labels = newLabels
 
 	// Regardless of whether the user sets it to true or false, if they
 	// specify TLSVerify at all then we need to turn on TLS

cmd/dockerd/daemon_test.go

@@ -61,6 +61,28 @@ func TestLoadDaemonCliConfigWithConflicts(t *testing.T) {
 	testutil.ErrorContains(t, err, "as a flag and in the configuration file: labels")
 }
 
+func TestLoadDaemonCliWithConflictingLabels(t *testing.T) {
+	opts := defaultOptions("")
+	flags := opts.flags
+
+	assert.NoError(t, flags.Set("label", "foo=bar"))
+	assert.NoError(t, flags.Set("label", "foo=baz"))
+
+	_, err := loadDaemonCliConfig(opts)
+	assert.EqualError(t, err, "conflict labels for foo=baz and foo=bar")
+}
+
+func TestLoadDaemonCliWithDuplicateLabels(t *testing.T) {
+	opts := defaultOptions("")
+	flags := opts.flags
+
+	assert.NoError(t, flags.Set("label", "foo=the-same"))
+	assert.NoError(t, flags.Set("label", "foo=the-same"))
+
+	_, err := loadDaemonCliConfig(opts)
+	assert.NoError(t, err)
+}
+
 func TestLoadDaemonCliConfigWithTLSVerify(t *testing.T) {
 	tempFile := fs.NewFile(t, "config", fs.WithContent(`{"tlsverify": true}`))
 	defer tempFile.Remove()

daemon/config/config.go

@@ -258,22 +258,12 @@ func Reload(configFile string, flags *pflag.FlagSet, reload func(*Config)) error
 		return fmt.Errorf("file configuration validation failed (%v)", err)
 	}
 
-	// Labels of the docker engine used to allow multiple values associated with the same key.
-	// This is deprecated in 1.13, and, be removed after 3 release cycles.
-	// The following will check the conflict of labels, and report a warning for deprecation.
-	//
-	// TODO: After 3 release cycles (17.12) an error will be returned, and labels will be
-	// sanitized to consolidate duplicate key-value pairs (config.Labels = newLabels):
-	//
-	// newLabels, err := GetConflictFreeLabels(newConfig.Labels)
-	// if err != nil {
-	//      return err
-	// }
-	// newConfig.Labels = newLabels
-	//
-	if _, err := GetConflictFreeLabels(newConfig.Labels); err != nil {
-		logrus.Warnf("Engine labels with duplicate keys and conflicting values have been deprecated: %s", err)
+	// Check if duplicate label-keys with different values are found
+	newLabels, err := GetConflictFreeLabels(newConfig.Labels)
+	if err != nil {
+		return err
 	}
+	newConfig.Labels = newLabels
 
 	reload(newConfig)
 	return nil

daemon/config/config_test.go

@@ -9,6 +9,7 @@ import (
 	"github.com/docker/docker/daemon/discovery"
 	"github.com/docker/docker/internal/testutil"
 	"github.com/docker/docker/opts"
+	"github.com/gotestyourself/gotestyourself/fs"
 	"github.com/spf13/pflag"
 	"github.com/stretchr/testify/assert"
 )
@@ -459,3 +460,29 @@ func TestReloadBadDefaultConfig(t *testing.T) {
 	assert.Error(t, err)
 	testutil.ErrorContains(t, err, "unable to configure the Docker daemon with file")
 }
+
+func TestReloadWithConflictingLabels(t *testing.T) {
+	tempFile := fs.NewFile(t, "config", fs.WithContent(`{"labels":["foo=bar","foo=baz"]}`))
+	defer tempFile.Remove()
+	configFile := tempFile.Path()
+
+	var lbls []string
+	flags := pflag.NewFlagSet("test", pflag.ContinueOnError)
+	flags.String("config-file", configFile, "")
+	flags.StringSlice("labels", lbls, "")
+	err := Reload(configFile, flags, func(c *Config) {})
+	testutil.ErrorContains(t, err, "conflict labels for foo=baz and foo=bar")
+}
+
+func TestReloadWithDuplicateLabels(t *testing.T) {
+	tempFile := fs.NewFile(t, "config", fs.WithContent(`{"labels":["foo=the-same","foo=the-same"]}`))
+	defer tempFile.Remove()
+	configFile := tempFile.Path()
+
+	var lbls []string
+	flags := pflag.NewFlagSet("test", pflag.ContinueOnError)
+	flags.String("config-file", configFile, "")
+	flags.StringSlice("labels", lbls, "")
+	err := Reload(configFile, flags, func(c *Config) {})
+	assert.NoError(t, err)
+}

integration-cli/docker_cli_info_test.go

@@ -233,17 +233,6 @@ func (s *DockerDaemonSuite) TestRegistryMirrors(c *check.C) {
 	c.Assert(out, checker.Contains, fmt.Sprintf(" %s", registryMirror2))
 }
 
-// Test case for #24392
-func (s *DockerDaemonSuite) TestInfoLabels(c *check.C) {
-	testRequires(c, SameHostDaemon, DaemonIsLinux)
-
-	s.d.Start(c, "--label", `test.empty=`, "--label", `test.empty=`, "--label", `test.label="1"`, "--label", `test.label="2"`)
-
-	out, err := s.d.Cmd("info")
-	c.Assert(err, checker.IsNil)
-	c.Assert(out, checker.Contains, "WARNING: labels with duplicate keys and conflicting values have been deprecated")
-}
-
 func existingContainerStates(c *check.C) map[string]int {
 	out, _ := dockerCmd(c, "info", "--format", "{{json .}}")
 	var m map[string]interface{}