Merge pull request #36306 from cpuguy83/fix_logopt_validator_plugins

Move log validator logic after plugins are loaded

cmd/dockerd/daemon.go

@@ -34,7 +34,6 @@ import (
 	"github.com/docker/docker/daemon/cluster"
 	"github.com/docker/docker/daemon/config"
 	"github.com/docker/docker/daemon/listeners"
-	"github.com/docker/docker/daemon/logger"
 	"github.com/docker/docker/dockerversion"
 	"github.com/docker/docker/libcontainerd"
 	dopts "github.com/docker/docker/opts"
@@ -106,12 +105,6 @@ func (cli *DaemonCli) start(opts *daemonOptions) (err error) {
 		return fmt.Errorf("Failed to set umask: %v", err)
 	}
 
-	if len(cli.LogConfig.Config) > 0 {
-		if err := logger.ValidateLogOpts(cli.LogConfig.Type, cli.LogConfig.Config); err != nil {
-			return fmt.Errorf("Failed to set log opts: %v", err)
-		}
-	}
-
 	// Create the daemon root before we create ANY other files (PID, or migrate keys)
 	// to ensure the appropriate ACL is set (particularly relevant on Windows)
 	if err := daemon.CreateDaemonRoot(cli.Config); err != nil {

daemon/daemon.go

@@ -671,8 +671,6 @@ func NewDaemon(config *config.Config, registryService registry.Service, containe
 		return nil, fmt.Errorf("error setting default isolation mode: %v", err)
 	}
 
-	logrus.Debugf("Using default logging driver %s", config.LogConfig.Type)
-
 	if err := configureMaxThreads(config); err != nil {
 		logrus.Warnf("Failed to configure golang's threads limit: %v", err)
 	}
@@ -753,6 +751,10 @@ func NewDaemon(config *config.Config, registryService registry.Service, containe
 		return nil, errors.Wrap(err, "couldn't create plugin manager")
 	}
 
+	if err := d.setupDefaultLogConfig(); err != nil {
+		return nil, err
+	}
+
 	for operatingSystem, gd := range d.graphDrivers {
 		d.layerStores[operatingSystem], err = layer.NewStoreFromOptions(layer.StoreOptions{
 			Root: config.Root,
@@ -873,10 +875,7 @@ func NewDaemon(config *config.Config, registryService registry.Service, containe
 	d.trustKey = trustKey
 	d.idIndex = truncindex.NewTruncIndex([]string{})
 	d.statsCollector = d.newStatsCollector(1 * time.Second)
-	d.defaultLogConfig = containertypes.LogConfig{
+
-		Type:   config.LogConfig.Type,
-		Config: config.LogConfig.Config,
-	}
 	d.EventsService = eventsService
 	d.volumes = volStore
 	d.root = config.Root

daemon/logs.go

@@ -1,7 +1,6 @@
 package daemon // import "github.com/docker/docker/daemon"
 
 import (
-	"errors"
 	"strconv"
 	"time"
 
@@ -14,6 +13,7 @@ import (
 	"github.com/docker/docker/container"
 	"github.com/docker/docker/daemon/logger"
 	"github.com/docker/docker/errdefs"
+	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 )
 
@@ -184,3 +184,18 @@ func (daemon *Daemon) mergeAndVerifyLogConfig(cfg *containertypes.LogConfig) err
 
 	return logger.ValidateLogOpts(cfg.Type, cfg.Config)
 }
+
+func (daemon *Daemon) setupDefaultLogConfig() error {
+	config := daemon.configStore
+	if len(config.LogConfig.Config) > 0 {
+		if err := logger.ValidateLogOpts(config.LogConfig.Type, config.LogConfig.Config); err != nil {
+			return errors.Wrap(err, "failed to set log opts")
+		}
+	}
+	daemon.defaultLogConfig = containertypes.LogConfig{
+		Type:   config.LogConfig.Type,
+		Config: config.LogConfig.Config,
+	}
+	logrus.Debugf("Using default logging driver %s", daemon.defaultLogConfig.Type)
+	return nil
+}

integration-cli/docker_cli_daemon_test.go

@@ -1697,7 +1697,7 @@ func (s *DockerDaemonSuite) TestDaemonCorruptedLogDriverAddress(c *check.C) {
 		Experimental: testEnv.DaemonInfo.ExperimentalBuild,
 	})
 	c.Assert(d.StartWithError("--log-driver=syslog", "--log-opt", "syslog-address=corrupted:42"), check.NotNil)
-	expected := "Failed to set log opts: syslog-address should be in form proto://address"
+	expected := "syslog-address should be in form proto://address"
 	icmd.RunCommand("grep", expected, d.LogFileName()).Assert(c, icmd.Success)
 }
 
@@ -1707,7 +1707,7 @@ func (s *DockerDaemonSuite) TestDaemonCorruptedFluentdAddress(c *check.C) {
 		Experimental: testEnv.DaemonInfo.ExperimentalBuild,
 	})
 	c.Assert(d.StartWithError("--log-driver=fluentd", "--log-opt", "fluentd-address=corrupted:c"), check.NotNil)
-	expected := "Failed to set log opts: invalid fluentd-address corrupted:c: "
+	expected := "invalid fluentd-address corrupted:c: "
 	icmd.RunCommand("grep", expected, d.LogFileName()).Assert(c, icmd.Success)
 }
 

integration/plugin/logging/cmd/cmd_test.go

@@ -0,0 +1 @@
+package cmd

integration/plugin/logging/cmd/dummy/main.go

@@ -0,0 +1,19 @@
+package main
+
+import (
+	"net"
+	"net/http"
+)
+
+func main() {
+	l, err := net.Listen("unix", "/run/docker/plugins/plugin.sock")
+	if err != nil {
+		panic(err)
+	}
+
+	server := http.Server{
+		Addr:    l.Addr().String(),
+		Handler: http.NewServeMux(),
+	}
+	server.Serve(l)
+}

integration/plugin/logging/cmd/dummy/main_test.go

@@ -0,0 +1 @@
+package main

integration/plugin/logging/helpers_test.go

@@ -0,0 +1,69 @@
+package logging
+
+import (
+	"context"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"testing"
+	"time"
+
+	"github.com/docker/docker/api/types"
+	"github.com/docker/docker/integration-cli/fixtures/plugin"
+	"github.com/docker/docker/pkg/locker"
+	"github.com/pkg/errors"
+)
+
+const dockerdBinary = "dockerd"
+
+var pluginBuildLock = locker.New()
+
+func ensurePlugin(t *testing.T, name string) string {
+	pluginBuildLock.Lock(name)
+	defer pluginBuildLock.Unlock(name)
+
+	installPath := filepath.Join(os.Getenv("GOPATH"), "bin", name)
+	if _, err := os.Stat(installPath); err == nil {
+		return installPath
+	}
+
+	goBin, err := exec.LookPath("go")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	cmd := exec.Command(goBin, "build", "-o", installPath, "./"+filepath.Join("cmd", name))
+	cmd.Env = append(cmd.Env, "CGO_ENABLED=0")
+	if out, err := cmd.CombinedOutput(); err != nil {
+		t.Fatal(errors.Wrapf(err, "error building basic plugin bin: %s", string(out)))
+	}
+
+	return installPath
+}
+
+func withSockPath(name string) func(*plugin.Config) {
+	return func(cfg *plugin.Config) {
+		cfg.Interface.Socket = name
+	}
+}
+
+func createPlugin(t *testing.T, client plugin.CreateClient, alias, bin string, opts ...plugin.CreateOpt) {
+	pluginBin := ensurePlugin(t, bin)
+
+	opts = append(opts, withSockPath("plugin.sock"))
+	opts = append(opts, plugin.WithBinary(pluginBin))
+
+	ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second)
+	err := plugin.Create(ctx, client, alias, opts...)
+	cancel()
+
+	if err != nil {
+		t.Fatal(err)
+	}
+}
+
+func asLogDriver(cfg *plugin.Config) {
+	cfg.Interface.Types = []types.PluginInterfaceType{
+		{Capability: "logdriver", Prefix: "docker", Version: "1.0"},
+	}
+}

integration/plugin/logging/validation_test.go

@@ -0,0 +1,33 @@
+package logging
+
+import (
+	"context"
+	"testing"
+
+	"github.com/docker/docker/api/types"
+	"github.com/docker/docker/integration-cli/daemon"
+	"github.com/stretchr/testify/assert"
+)
+
+// Regression test for #35553
+// Ensure that a daemon with a log plugin set as the default logger for containers
+// does not keep the daemon from starting.
+func TestDaemonStartWithLogOpt(t *testing.T) {
+	t.Parallel()
+
+	d := daemon.New(t, "", dockerdBinary, daemon.Config{})
+	d.Start(t, "--iptables=false")
+	defer d.Stop(t)
+
+	client, err := d.NewClient()
+	assert.NoError(t, err)
+	ctx := context.Background()
+
+	createPlugin(t, client, "test", "dummy", asLogDriver)
+	err = client.PluginEnable(ctx, "test", types.PluginEnableOptions{Timeout: 30})
+	assert.NoError(t, err)
+	defer client.PluginRemove(ctx, "test", types.PluginRemoveOptions{Force: true})
+
+	d.Stop(t)
+	d.Start(t, "--iptables=false", "--log-driver=test", "--log-opt=foo=bar")
+}