diff --git a/components/engine/api/swagger.yaml b/components/engine/api/swagger.yaml index fe4e2d6494..af3bd6d484 100644 --- a/components/engine/api/swagger.yaml +++ b/components/engine/api/swagger.yaml @@ -1859,6 +1859,13 @@ definitions: type: "string" x-nullable: false example: "plugins.sock" + ProtocolScheme: + type: "string" + example: "some.protocol/v1.0" + description: "Protocol to use for clients connecting to the plugin." + enum: + - "" + - "moby.plugins.http/v1" Entrypoint: type: "array" items: diff --git a/components/engine/api/types/plugin.go b/components/engine/api/types/plugin.go index cab333e01a..abae48b9ab 100644 --- a/components/engine/api/types/plugin.go +++ b/components/engine/api/types/plugin.go @@ -121,6 +121,9 @@ type PluginConfigArgs struct { // swagger:model PluginConfigInterface type PluginConfigInterface struct { + // Protocol to use for clients connecting to the plugin. + ProtocolScheme string `json:"ProtocolScheme,omitempty"` + // socket // Required: true Socket string `json:"Socket"` diff --git a/components/engine/builder/dockerfile/copy.go b/components/engine/builder/dockerfile/copy.go index cb9f24d205..43f40b62f9 100644 --- a/components/engine/builder/dockerfile/copy.go +++ b/components/engine/builder/dockerfile/copy.go @@ -172,7 +172,9 @@ func (o *copier) calcCopyInfo(origPath string, allowWildcards bool) ([]copyInfo, // TODO: do this when creating copier. Requires validateCopySourcePath // (and other below) to be aware of the difference sources. Why is it only // done on image Source? - if imageSource != nil { + if imageSource != nil && o.activeLayer == nil { + // this needs to be protected against repeated calls as wildcard copy + // will call it multiple times for a single COPY var err error rwLayer, err := imageSource.NewRWLayer() if err != nil { diff --git a/components/engine/builder/dockerfile/shell/envVarTest b/components/engine/builder/dockerfile/shell/envVarTest index 946b278592..08011801c5 100644 --- a/components/engine/builder/dockerfile/shell/envVarTest +++ b/components/engine/builder/dockerfile/shell/envVarTest @@ -18,7 +18,6 @@ A|'hello\there' | hello\there A|'hello\\there' | hello\\there A|"''" | '' A|$. | $. -A|$1 | A|he$1x | hex A|he$.x | he$.x # Next one is different on Windows as $pwd==$PWD @@ -29,10 +28,14 @@ A|he\$PWD | he$PWD A|he\\$PWD | he\/home A|"he\$PWD" | he$PWD A|"he\\$PWD" | he\/home +A|\${} | ${} +A|\${}aaa | ${}aaa A|he\${} | he${} A|he\${}xx | he${}xx -A|he${} | he -A|he${}xx | hexx +A|${} | error +A|${}aaa | error +A|he${} | error +A|he${}xx | error A|he${hi} | he A|he${hi}xx | hexx A|he${PWD} | he/home @@ -88,8 +91,8 @@ A|안녕\$PWD | 안녕$PWD A|안녕\\$PWD | 안녕\/home A|안녕\${} | 안녕${} A|안녕\${}xx | 안녕${}xx -A|안녕${} | 안녕 -A|안녕${}xx | 안녕xx +A|안녕${} | error +A|안녕${}xx | error A|안녕${hi} | 안녕 A|안녕${hi}xx | 안녕xx A|안녕${PWD} | 안녕/home @@ -119,3 +122,111 @@ A|안녕${XXX:-\$PWD:}xx | 안녕$PWD:xx A|안녕${XXX:-\${PWD}z}xx | 안녕${PWDz}xx A|$KOREAN | 한국어 A|안녕$KOREAN | 안녕한국어 +A|${{aaa} | error +A|${aaa}} | } +A|${aaa | error +A|${{aaa:-bbb} | error +A|${aaa:-bbb}} | bbb} +A|${aaa:-bbb | error +A|${aaa:-bbb} | bbb +A|${aaa:-${bbb:-ccc}} | ccc +A|${aaa:-bbb ${foo} | error +A|${aaa:-bbb {foo} | bbb {foo +A|${:} | error +A|${:-bbb} | error +A|${:+bbb} | error + +# Positional parameters won't be set: +# http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_05_01 +A|$1 | +A|${1} | +A|${1:+bbb} | +A|${1:-bbb} | bbb +A|$2 | +A|${2} | +A|${2:+bbb} | +A|${2:-bbb} | bbb +A|$3 | +A|${3} | +A|${3:+bbb} | +A|${3:-bbb} | bbb +A|$4 | +A|${4} | +A|${4:+bbb} | +A|${4:-bbb} | bbb +A|$5 | +A|${5} | +A|${5:+bbb} | +A|${5:-bbb} | bbb +A|$6 | +A|${6} | +A|${6:+bbb} | +A|${6:-bbb} | bbb +A|$7 | +A|${7} | +A|${7:+bbb} | +A|${7:-bbb} | bbb +A|$8 | +A|${8} | +A|${8:+bbb} | +A|${8:-bbb} | bbb +A|$9 | +A|${9} | +A|${9:+bbb} | +A|${9:-bbb} | bbb +A|$999 | +A|${999} | +A|${999:+bbb} | +A|${999:-bbb} | bbb +A|$999aaa | aaa +A|${999}aaa | aaa +A|${999:+bbb}aaa | aaa +A|${999:-bbb}aaa | bbbaaa +A|$001 | +A|${001} | +A|${001:+bbb} | +A|${001:-bbb} | bbb +A|$001aaa | aaa +A|${001}aaa | aaa +A|${001:+bbb}aaa | aaa +A|${001:-bbb}aaa | bbbaaa + +# Special parameters won't be set in the Dockerfile: +# http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_05_02 +A|$@ | +A|${@} | +A|${@:+bbb} | +A|${@:-bbb} | bbb +A|$@@@ | @@ +A|$@aaa | aaa +A|${@}aaa | aaa +A|${@:+bbb}aaa | aaa +A|${@:-bbb}aaa | bbbaaa +A|$* | +A|${*} | +A|${*:+bbb} | +A|${*:-bbb} | bbb +A|$# | +A|${#} | +A|${#:+bbb} | +A|${#:-bbb} | bbb +A|$? | +A|${?} | +A|${?:+bbb} | +A|${?:-bbb} | bbb +A|$- | +A|${-} | +A|${-:+bbb} | +A|${-:-bbb} | bbb +A|$$ | +A|${$} | +A|${$:+bbb} | +A|${$:-bbb} | bbb +A|$! | +A|${!} | +A|${!:+bbb} | +A|${!:-bbb} | bbb +A|$0 | +A|${0} | +A|${0:+bbb} | +A|${0:-bbb} | bbb diff --git a/components/engine/builder/dockerfile/shell/lex.go b/components/engine/builder/dockerfile/shell/lex.go index bd3fac525a..0c80900ade 100644 --- a/components/engine/builder/dockerfile/shell/lex.go +++ b/components/engine/builder/dockerfile/shell/lex.go @@ -131,7 +131,7 @@ func (sw *shellWord) processStopOn(stopChar rune) (string, []string, error) { if stopChar != scanner.EOF && ch == stopChar { sw.scanner.Next() - break + return result.String(), words.getWords(), nil } if fn, ok := charFuncMapping[ch]; ok { // Call special processing func for certain chars @@ -166,7 +166,9 @@ func (sw *shellWord) processStopOn(stopChar rune) (string, []string, error) { result.WriteRune(ch) } } - + if stopChar != scanner.EOF { + return "", []string{}, errors.Errorf("unexpected end of statement while looking for matching %s", string(stopChar)) + } return result.String(), words.getWords(), nil } @@ -259,22 +261,29 @@ func (sw *shellWord) processDollar() (string, error) { } sw.scanner.Next() - name := sw.processName() - ch := sw.scanner.Peek() - if ch == '}' { - // Normal ${xx} case - sw.scanner.Next() - return sw.getEnv(name), nil + switch sw.scanner.Peek() { + case scanner.EOF: + return "", errors.New("syntax error: missing '}'") + case '{', '}', ':': + // Invalid ${{xx}, ${:xx}, ${:}. ${} case + return "", errors.New("syntax error: bad substitution") } - if ch == ':' { + name := sw.processName() + ch := sw.scanner.Next() + switch ch { + case '}': + // Normal ${xx} case + return sw.getEnv(name), nil + case ':': // Special ${xx:...} format processing // Yes it allows for recursive $'s in the ... spot - - sw.scanner.Next() // skip over : modifier := sw.scanner.Next() word, _, err := sw.processStopOn('}') if err != nil { + if sw.scanner.Peek() == scanner.EOF { + return "", errors.New("syntax error: missing '}'") + } return "", err } @@ -310,6 +319,14 @@ func (sw *shellWord) processName() string { for sw.scanner.Peek() != scanner.EOF { ch := sw.scanner.Peek() if name.Len() == 0 && unicode.IsDigit(ch) { + for sw.scanner.Peek() != scanner.EOF && unicode.IsDigit(sw.scanner.Peek()) { + // Keep reading until the first non-digit character, or EOF + ch = sw.scanner.Next() + name.WriteRune(ch) + } + return name.String() + } + if name.Len() == 0 && isSpecialParam(ch) { ch = sw.scanner.Next() return string(ch) } @@ -323,6 +340,18 @@ func (sw *shellWord) processName() string { return name.String() } +// isSpecialParam checks if the provided character is a special parameters, +// as defined in http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_05_02 +func isSpecialParam(char rune) bool { + switch char { + case '@', '*', '#', '?', '-', '$', '!', '0': + // Special parameters + // http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_05_02 + return true + } + return false +} + func (sw *shellWord) getEnv(name string) string { for _, env := range sw.envs { i := strings.Index(env, "=") diff --git a/components/engine/builder/dockerfile/shell/lex_test.go b/components/engine/builder/dockerfile/shell/lex_test.go index f38da2026f..4b30c32f2b 100644 --- a/components/engine/builder/dockerfile/shell/lex_test.go +++ b/components/engine/builder/dockerfile/shell/lex_test.go @@ -26,13 +26,11 @@ func TestShellParser4EnvVars(t *testing.T) { line := scanner.Text() lineCount++ - // Trim comments and blank lines - i := strings.Index(line, "#") - if i >= 0 { - line = line[:i] + // Skip comments and blank lines + if strings.HasPrefix(line, "#") { + continue } line = strings.TrimSpace(line) - if line == "" { continue } @@ -53,10 +51,10 @@ func TestShellParser4EnvVars(t *testing.T) { ((platform == "U" || platform == "A") && runtime.GOOS != "windows") { newWord, err := shlex.ProcessWord(source, envs) if expected == "error" { - assert.Check(t, is.ErrorContains(err, "")) + assert.Check(t, is.ErrorContains(err, ""), "input: %q, result: %q", source, newWord) } else { - assert.Check(t, err) - assert.Check(t, is.Equal(newWord, expected)) + assert.Check(t, err, "at line %d of %s", lineCount, fn) + assert.Check(t, is.Equal(newWord, expected), "at line %d of %s", lineCount, fn) } } } diff --git a/components/engine/client/container_stop.go b/components/engine/client/container_stop.go index 5d7f1606ec..629d7ab64c 100644 --- a/components/engine/client/container_stop.go +++ b/components/engine/client/container_stop.go @@ -8,8 +8,13 @@ import ( timetypes "github.com/docker/docker/api/types/time" ) -// ContainerStop stops a container without terminating the process. -// The process is blocked until the container stops or the timeout expires. +// ContainerStop stops a container. In case the container fails to stop +// gracefully within a time frame specified by the timeout argument, +// it is forcefully terminated (killed). +// +// If the timeout is nil, the container's StopTimeout value is used, if set, +// otherwise the engine default. A negative timeout value can be specified, +// meaning no timeout, i.e. no forceful termination is performed. func (cli *Client) ContainerStop(ctx context.Context, containerID string, timeout *time.Duration) error { query := url.Values{} if timeout != nil { diff --git a/components/engine/container/container_unit_test.go b/components/engine/container/container_unit_test.go index bf45df942e..fbee6e5eb0 100644 --- a/components/engine/container/container_unit_test.go +++ b/components/engine/container/container_unit_test.go @@ -52,9 +52,9 @@ func TestContainerStopTimeout(t *testing.T) { c = &Container{ Config: &container.Config{StopTimeout: &stopTimeout}, } - s = c.StopSignal() - if s != 15 { - t.Fatalf("Expected 15, got %v", s) + s = c.StopTimeout() + if s != stopTimeout { + t.Fatalf("Expected %v, got %v", stopTimeout, s) } } diff --git a/components/engine/container/container_unix.go b/components/engine/container/container_unix.go index c77ea07a18..597acf1218 100644 --- a/components/engine/container/container_unix.go +++ b/components/engine/container/container_unix.go @@ -23,7 +23,8 @@ import ( ) const ( - // DefaultStopTimeout is the timeout (in seconds) for the syscall signal used to stop a container. + // DefaultStopTimeout sets the default time, in seconds, to wait + // for the graceful container stop before forcefully terminating it. DefaultStopTimeout = 10 containerSecretMountPath = "/run/secrets" diff --git a/components/engine/daemon/graphdriver/plugin.go b/components/engine/daemon/graphdriver/plugin.go index d8058d9236..b0983c5667 100644 --- a/components/engine/daemon/graphdriver/plugin.go +++ b/components/engine/daemon/graphdriver/plugin.go @@ -4,8 +4,11 @@ import ( "fmt" "path/filepath" + "github.com/docker/docker/errdefs" "github.com/docker/docker/pkg/plugingetter" + "github.com/docker/docker/pkg/plugins" "github.com/docker/docker/plugin/v2" + "github.com/pkg/errors" ) func lookupPlugin(name string, pg plugingetter.PluginGetter, config Options) (Driver, error) { @@ -28,6 +31,25 @@ func newPluginDriver(name string, pl plugingetter.CompatPlugin, config Options) } } } - proxy := &graphDriverProxy{name, pl, Capabilities{}} + + var proxy *graphDriverProxy + + switch pt := pl.(type) { + case plugingetter.PluginWithV1Client: + proxy = &graphDriverProxy{name, pl, Capabilities{}, pt.Client()} + case plugingetter.PluginAddr: + if pt.Protocol() != plugins.ProtocolSchemeHTTPV1 { + return nil, errors.Errorf("plugin protocol not supported: %s", pt.Protocol()) + } + addr := pt.Addr() + client, err := plugins.NewClientWithTimeout(addr.Network()+"://"+addr.String(), nil, pt.Timeout()) + if err != nil { + return nil, errors.Wrap(err, "error creating plugin client") + } + proxy = &graphDriverProxy{name, pl, Capabilities{}, client} + default: + return nil, errdefs.System(errors.Errorf("got unknown plugin type %T", pt)) + } + return proxy, proxy.Init(filepath.Join(home, name), config.DriverOptions, config.UIDMaps, config.GIDMaps) } diff --git a/components/engine/daemon/graphdriver/proxy.go b/components/engine/daemon/graphdriver/proxy.go index 10a7a527ae..cb350d8074 100644 --- a/components/engine/daemon/graphdriver/proxy.go +++ b/components/engine/daemon/graphdriver/proxy.go @@ -13,9 +13,10 @@ import ( ) type graphDriverProxy struct { - name string - p plugingetter.CompatPlugin - caps Capabilities + name string + p plugingetter.CompatPlugin + caps Capabilities + client *plugins.Client } type graphDriverRequest struct { @@ -57,7 +58,7 @@ func (d *graphDriverProxy) Init(home string, opts []string, uidMaps, gidMaps []i GIDMaps: gidMaps, } var ret graphDriverResponse - if err := d.p.Client().Call("GraphDriver.Init", args, &ret); err != nil { + if err := d.client.Call("GraphDriver.Init", args, &ret); err != nil { return err } if ret.Err != "" { @@ -74,7 +75,7 @@ func (d *graphDriverProxy) Init(home string, opts []string, uidMaps, gidMaps []i func (d *graphDriverProxy) fetchCaps() (Capabilities, error) { args := &graphDriverRequest{} var ret graphDriverResponse - if err := d.p.Client().Call("GraphDriver.Capabilities", args, &ret); err != nil { + if err := d.client.Call("GraphDriver.Capabilities", args, &ret); err != nil { if !plugins.IsNotFound(err) { return Capabilities{}, err } @@ -108,7 +109,7 @@ func (d *graphDriverProxy) create(method, id, parent string, opts *CreateOpts) e args.StorageOpt = opts.StorageOpt } var ret graphDriverResponse - if err := d.p.Client().Call(method, args, &ret); err != nil { + if err := d.client.Call(method, args, &ret); err != nil { return err } if ret.Err != "" { @@ -120,7 +121,7 @@ func (d *graphDriverProxy) create(method, id, parent string, opts *CreateOpts) e func (d *graphDriverProxy) Remove(id string) error { args := &graphDriverRequest{ID: id} var ret graphDriverResponse - if err := d.p.Client().Call("GraphDriver.Remove", args, &ret); err != nil { + if err := d.client.Call("GraphDriver.Remove", args, &ret); err != nil { return err } if ret.Err != "" { @@ -135,7 +136,7 @@ func (d *graphDriverProxy) Get(id, mountLabel string) (containerfs.ContainerFS, MountLabel: mountLabel, } var ret graphDriverResponse - if err := d.p.Client().Call("GraphDriver.Get", args, &ret); err != nil { + if err := d.client.Call("GraphDriver.Get", args, &ret); err != nil { return nil, err } var err error @@ -148,7 +149,7 @@ func (d *graphDriverProxy) Get(id, mountLabel string) (containerfs.ContainerFS, func (d *graphDriverProxy) Put(id string) error { args := &graphDriverRequest{ID: id} var ret graphDriverResponse - if err := d.p.Client().Call("GraphDriver.Put", args, &ret); err != nil { + if err := d.client.Call("GraphDriver.Put", args, &ret); err != nil { return err } if ret.Err != "" { @@ -160,7 +161,7 @@ func (d *graphDriverProxy) Put(id string) error { func (d *graphDriverProxy) Exists(id string) bool { args := &graphDriverRequest{ID: id} var ret graphDriverResponse - if err := d.p.Client().Call("GraphDriver.Exists", args, &ret); err != nil { + if err := d.client.Call("GraphDriver.Exists", args, &ret); err != nil { return false } return ret.Exists @@ -169,7 +170,7 @@ func (d *graphDriverProxy) Exists(id string) bool { func (d *graphDriverProxy) Status() [][2]string { args := &graphDriverRequest{} var ret graphDriverResponse - if err := d.p.Client().Call("GraphDriver.Status", args, &ret); err != nil { + if err := d.client.Call("GraphDriver.Status", args, &ret); err != nil { return nil } return ret.Status @@ -180,7 +181,7 @@ func (d *graphDriverProxy) GetMetadata(id string) (map[string]string, error) { ID: id, } var ret graphDriverResponse - if err := d.p.Client().Call("GraphDriver.GetMetadata", args, &ret); err != nil { + if err := d.client.Call("GraphDriver.GetMetadata", args, &ret); err != nil { return nil, err } if ret.Err != "" { @@ -199,7 +200,7 @@ func (d *graphDriverProxy) Cleanup() error { args := &graphDriverRequest{} var ret graphDriverResponse - if err := d.p.Client().Call("GraphDriver.Cleanup", args, &ret); err != nil { + if err := d.client.Call("GraphDriver.Cleanup", args, &ret); err != nil { return nil } if ret.Err != "" { @@ -213,7 +214,7 @@ func (d *graphDriverProxy) Diff(id, parent string) (io.ReadCloser, error) { ID: id, Parent: parent, } - body, err := d.p.Client().Stream("GraphDriver.Diff", args) + body, err := d.client.Stream("GraphDriver.Diff", args) if err != nil { return nil, err } @@ -226,7 +227,7 @@ func (d *graphDriverProxy) Changes(id, parent string) ([]archive.Change, error) Parent: parent, } var ret graphDriverResponse - if err := d.p.Client().Call("GraphDriver.Changes", args, &ret); err != nil { + if err := d.client.Call("GraphDriver.Changes", args, &ret); err != nil { return nil, err } if ret.Err != "" { @@ -238,7 +239,7 @@ func (d *graphDriverProxy) Changes(id, parent string) ([]archive.Change, error) func (d *graphDriverProxy) ApplyDiff(id, parent string, diff io.Reader) (int64, error) { var ret graphDriverResponse - if err := d.p.Client().SendFile(fmt.Sprintf("GraphDriver.ApplyDiff?id=%s&parent=%s", id, parent), diff, &ret); err != nil { + if err := d.client.SendFile(fmt.Sprintf("GraphDriver.ApplyDiff?id=%s&parent=%s", id, parent), diff, &ret); err != nil { return -1, err } if ret.Err != "" { @@ -253,7 +254,7 @@ func (d *graphDriverProxy) DiffSize(id, parent string) (int64, error) { Parent: parent, } var ret graphDriverResponse - if err := d.p.Client().Call("GraphDriver.DiffSize", args, &ret); err != nil { + if err := d.client.Call("GraphDriver.DiffSize", args, &ret); err != nil { return -1, err } if ret.Err != "" { diff --git a/components/engine/daemon/logger/plugin.go b/components/engine/daemon/logger/plugin.go index cd0e60b7cd..c66540ce52 100644 --- a/components/engine/daemon/logger/plugin.go +++ b/components/engine/daemon/logger/plugin.go @@ -7,7 +7,9 @@ import ( "path/filepath" "github.com/docker/docker/api/types/plugins/logdriver" + "github.com/docker/docker/errdefs" getter "github.com/docker/docker/pkg/plugingetter" + "github.com/docker/docker/pkg/plugins" "github.com/docker/docker/pkg/stringid" "github.com/pkg/errors" ) @@ -37,11 +39,35 @@ func getPlugin(name string, mode int) (Creator, error) { return nil, fmt.Errorf("error looking up logging plugin %s: %v", name, err) } - d := &logPluginProxy{p.Client()} - return makePluginCreator(name, d, p.ScopedPath), nil + client, err := makePluginClient(p) + if err != nil { + return nil, err + } + return makePluginCreator(name, client, p.ScopedPath), nil } -func makePluginCreator(name string, l *logPluginProxy, scopePath func(s string) string) Creator { +func makePluginClient(p getter.CompatPlugin) (logPlugin, error) { + if pc, ok := p.(getter.PluginWithV1Client); ok { + return &logPluginProxy{pc.Client()}, nil + } + pa, ok := p.(getter.PluginAddr) + if !ok { + return nil, errdefs.System(errors.Errorf("got unknown plugin type %T", p)) + } + + if pa.Protocol() != plugins.ProtocolSchemeHTTPV1 { + return nil, errors.Errorf("plugin protocol not supported: %s", p) + } + + addr := pa.Addr() + c, err := plugins.NewClientWithTimeout(addr.Network()+"://"+addr.String(), nil, pa.Timeout()) + if err != nil { + return nil, errors.Wrap(err, "error making plugin client") + } + return &logPluginProxy{c}, nil +} + +func makePluginCreator(name string, l logPlugin, scopePath func(s string) string) Creator { return func(logCtx Info) (logger Logger, err error) { defer func() { if err != nil { diff --git a/components/engine/daemon/metrics.go b/components/engine/daemon/metrics.go index 8ee6432eaa..f6961a3553 100644 --- a/components/engine/daemon/metrics.go +++ b/components/engine/daemon/metrics.go @@ -3,7 +3,9 @@ package daemon // import "github.com/docker/docker/daemon" import ( "sync" + "github.com/docker/docker/errdefs" "github.com/docker/docker/pkg/plugingetter" + "github.com/docker/docker/pkg/plugins" "github.com/docker/go-metrics" "github.com/pkg/errors" "github.com/prometheus/client_golang/prometheus" @@ -118,7 +120,15 @@ func (d *Daemon) cleanupMetricsPlugins() { p := plugin go func() { defer wg.Done() - pluginStopMetricsCollection(p) + + adapter, err := makePluginAdapter(p) + if err != nil { + logrus.WithError(err).WithField("plugin", p.Name()).Error("Error creating metrics plugin adapater") + return + } + if err := adapter.StopMetrics(); err != nil { + logrus.WithError(err).WithField("plugin", p.Name()).Error("Error stopping plugin metrics collection") + } }() } wg.Wait() @@ -128,12 +138,44 @@ func (d *Daemon) cleanupMetricsPlugins() { } } -func pluginStartMetricsCollection(p plugingetter.CompatPlugin) error { +type metricsPlugin interface { + StartMetrics() error + StopMetrics() error +} + +func makePluginAdapter(p plugingetter.CompatPlugin) (metricsPlugin, error) { // nolint: interfacer + if pc, ok := p.(plugingetter.PluginWithV1Client); ok { + return &metricsPluginAdapter{pc.Client(), p.Name()}, nil + } + + pa, ok := p.(plugingetter.PluginAddr) + if !ok { + return nil, errdefs.System(errors.Errorf("got unknown plugin type %T", p)) + } + + if pa.Protocol() != plugins.ProtocolSchemeHTTPV1 { + return nil, errors.Errorf("plugin protocol not supported: %s", pa.Protocol()) + } + + addr := pa.Addr() + client, err := plugins.NewClientWithTimeout(addr.Network()+"://"+addr.String(), nil, pa.Timeout()) + if err != nil { + return nil, errors.Wrap(err, "error creating metrics plugin client") + } + return &metricsPluginAdapter{client, p.Name()}, nil +} + +type metricsPluginAdapter struct { + c *plugins.Client + name string +} + +func (a *metricsPluginAdapter) StartMetrics() error { type metricsPluginResponse struct { Err string } var res metricsPluginResponse - if err := p.Client().Call(metricsPluginType+".StartMetrics", nil, &res); err != nil { + if err := a.c.Call(metricsPluginType+".StartMetrics", nil, &res); err != nil { return errors.Wrap(err, "could not start metrics plugin") } if res.Err != "" { @@ -142,8 +184,9 @@ func pluginStartMetricsCollection(p plugingetter.CompatPlugin) error { return nil } -func pluginStopMetricsCollection(p plugingetter.CompatPlugin) { - if err := p.Client().Call(metricsPluginType+".StopMetrics", nil, nil); err != nil { - logrus.WithError(err).WithField("name", p.Name()).Error("error stopping metrics collector") +func (a *metricsPluginAdapter) StopMetrics() error { + if err := a.c.Call(metricsPluginType+".StopMetrics", nil, nil); err != nil { + return errors.Wrap(err, "error stopping metrics collector") } + return nil } diff --git a/components/engine/daemon/metrics_unix.go b/components/engine/daemon/metrics_unix.go index 9311915249..452424e685 100644 --- a/components/engine/daemon/metrics_unix.go +++ b/components/engine/daemon/metrics_unix.go @@ -49,8 +49,12 @@ func registerMetricsPluginCallback(store *plugin.Store, sockPath string) { return } - if err := pluginStartMetricsCollection(p); err != nil { - logrus.WithError(err).WithField("name", name).Error("error while initializing metrics plugin") + adapter, err := makePluginAdapter(p) + if err != nil { + logrus.WithError(err).WithField("plugin", p.Name()).Error("Error creating plugin adapater") + } + if err := adapter.StartMetrics(); err != nil { + logrus.WithError(err).WithField("plugin", p.Name()).Error("Error starting metrics collector plugin") } }) } diff --git a/components/engine/daemon/stop.go b/components/engine/daemon/stop.go index 45f523848f..c3ac09056a 100644 --- a/components/engine/daemon/stop.go +++ b/components/engine/daemon/stop.go @@ -10,13 +10,15 @@ import ( "github.com/sirupsen/logrus" ) -// ContainerStop looks for the given container and terminates it, -// waiting the given number of seconds before forcefully killing the -// container. If a negative number of seconds is given, ContainerStop -// will wait for a graceful termination. An error is returned if the -// container is not found, is already stopped, or if there is a -// problem stopping the container. -func (daemon *Daemon) ContainerStop(name string, seconds *int) error { +// ContainerStop looks for the given container and stops it. +// In case the container fails to stop gracefully within a time duration +// specified by the timeout argument, in seconds, it is forcefully +// terminated (killed). +// +// If the timeout is nil, the container's StopTimeout value is used, if set, +// otherwise the engine default. A negative timeout value can be specified, +// meaning no timeout, i.e. no forceful termination is performed. +func (daemon *Daemon) ContainerStop(name string, timeout *int) error { container, err := daemon.GetContainer(name) if err != nil { return err @@ -24,21 +26,17 @@ func (daemon *Daemon) ContainerStop(name string, seconds *int) error { if !container.IsRunning() { return containerNotModifiedError{running: false} } - if seconds == nil { + if timeout == nil { stopTimeout := container.StopTimeout() - seconds = &stopTimeout + timeout = &stopTimeout } - if err := daemon.containerStop(container, *seconds); err != nil { + if err := daemon.containerStop(container, *timeout); err != nil { return errdefs.System(errors.Wrapf(err, "cannot stop container: %s", name)) } return nil } -// containerStop halts a container by sending a stop signal, waiting for the given -// duration in seconds, and then calling SIGKILL and waiting for the -// process to exit. If a negative duration is given, Stop will wait -// for the initial signal forever. If the container is not running Stop returns -// immediately. +// containerStop sends a stop signal, waits, sends a kill signal. func (daemon *Daemon) containerStop(container *containerpkg.Container, seconds int) error { if !container.IsRunning() { return nil @@ -69,8 +67,12 @@ func (daemon *Daemon) containerStop(container *containerpkg.Container, seconds i } // 2. Wait for the process to exit on its own - ctx, cancel := context.WithTimeout(context.Background(), time.Duration(seconds)*time.Second) - defer cancel() + ctx := context.Background() + if seconds >= 0 { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, time.Duration(seconds)*time.Second) + defer cancel() + } if status := <-container.Wait(ctx, containerpkg.WaitConditionNotRunning); status.Err() != nil { logrus.Infof("Container %v failed to exit within %d seconds of signal %d - using the force", container.ID, seconds, stopSignal) diff --git a/components/engine/integration/container/stop_test.go b/components/engine/integration/container/stop_test.go index c22a0302f6..2932ae0bf9 100644 --- a/components/engine/integration/container/stop_test.go +++ b/components/engine/integration/container/stop_test.go @@ -3,6 +3,7 @@ package container // import "github.com/docker/docker/integration/container" import ( "context" "fmt" + "strconv" "strings" "testing" "time" @@ -42,6 +43,60 @@ func TestStopContainerWithRestartPolicyAlways(t *testing.T) { } } +// TestStopContainerWithTimeout checks that ContainerStop with +// a timeout works as documented, i.e. in case of negative timeout +// waiting is not limited (issue #35311). +func TestStopContainerWithTimeout(t *testing.T) { + defer setupTest(t)() + client := request.NewAPIClient(t) + ctx := context.Background() + + testCmd := container.WithCmd("sh", "-c", "sleep 2 && exit 42") + testData := []struct { + doc string + timeout int + expectedExitCode int + }{ + // In case container is forcefully killed, 137 is returned, + // otherwise the exit code from the above script + { + "zero timeout: expect forceful container kill", + 0, 137, + }, + { + "too small timeout: expect forceful container kill", + 1, 137, + }, + { + "big enough timeout: expect graceful container stop", + 3, 42, + }, + { + "unlimited timeout: expect graceful container stop", + -1, 42, + }, + } + + for _, d := range testData { + d := d + t.Run(strconv.Itoa(d.timeout), func(t *testing.T) { + t.Parallel() + id := container.Run(t, ctx, client, testCmd) + + timeout := time.Duration(d.timeout) * time.Second + err := client.ContainerStop(ctx, id, &timeout) + assert.NilError(t, err) + + poll.WaitOn(t, container.IsStopped(ctx, client, id), + poll.WithDelay(100*time.Millisecond)) + + inspect, err := client.ContainerInspect(ctx, id) + assert.NilError(t, err) + assert.Equal(t, inspect.State.ExitCode, d.expectedExitCode) + }) + } +} + func TestDeleteDevicemapper(t *testing.T) { skip.If(t, testEnv.DaemonInfo.Driver != "devicemapper") skip.If(t, testEnv.IsRemoteDaemon, "cannot start daemon on remote test run") diff --git a/components/engine/integration/network/service_test.go b/components/engine/integration/network/service_test.go index 8a92786fd8..77ef870911 100644 --- a/components/engine/integration/network/service_test.go +++ b/components/engine/integration/network/service_test.go @@ -7,6 +7,7 @@ import ( "github.com/docker/docker/api/types" swarmtypes "github.com/docker/docker/api/types/swarm" + "github.com/docker/docker/api/types/versions" "github.com/docker/docker/client" "github.com/docker/docker/integration/internal/swarm" "github.com/docker/docker/internal/test/daemon" @@ -25,6 +26,7 @@ func delInterface(t *testing.T, ifName string) { func TestDaemonRestartWithLiveRestore(t *testing.T) { skip.If(t, testEnv.IsRemoteDaemon()) + skip.If(t, versions.LessThan(testEnv.DaemonAPIVersion(), "1.38"), "skip test from new feature") d := daemon.New(t) defer d.Stop(t) d.Start(t) @@ -45,6 +47,7 @@ func TestDaemonRestartWithLiveRestore(t *testing.T) { func TestDaemonDefaultNetworkPools(t *testing.T) { // Remove docker0 bridge and the start daemon defining the predefined address pools skip.If(t, testEnv.IsRemoteDaemon()) + skip.If(t, versions.LessThan(testEnv.DaemonAPIVersion(), "1.38"), "skip test from new feature") defaultNetworkBridge := "docker0" delInterface(t, defaultNetworkBridge) d := daemon.New(t) @@ -90,6 +93,7 @@ func TestDaemonDefaultNetworkPools(t *testing.T) { func TestDaemonRestartWithExistingNetwork(t *testing.T) { skip.If(t, testEnv.IsRemoteDaemon()) + skip.If(t, versions.LessThan(testEnv.DaemonAPIVersion(), "1.38"), "skip test from new feature") defaultNetworkBridge := "docker0" d := daemon.New(t) d.Start(t) @@ -124,6 +128,7 @@ func TestDaemonRestartWithExistingNetwork(t *testing.T) { func TestDaemonRestartWithExistingNetworkWithDefaultPoolRange(t *testing.T) { skip.If(t, testEnv.IsRemoteDaemon()) + skip.If(t, versions.LessThan(testEnv.DaemonAPIVersion(), "1.38"), "skip test from new feature") defaultNetworkBridge := "docker0" d := daemon.New(t) d.Start(t) @@ -180,6 +185,7 @@ func TestDaemonRestartWithExistingNetworkWithDefaultPoolRange(t *testing.T) { func TestDaemonWithBipAndDefaultNetworkPool(t *testing.T) { skip.If(t, testEnv.IsRemoteDaemon()) + skip.If(t, versions.LessThan(testEnv.DaemonAPIVersion(), "1.38"), "skip test from new feature") defaultNetworkBridge := "docker0" d := daemon.New(t) defer d.Stop(t) diff --git a/components/engine/pkg/plugingetter/getter.go b/components/engine/pkg/plugingetter/getter.go index 0e1699d913..370e0d5b97 100644 --- a/components/engine/pkg/plugingetter/getter.go +++ b/components/engine/pkg/plugingetter/getter.go @@ -1,6 +1,9 @@ package plugingetter // import "github.com/docker/docker/pkg/plugingetter" import ( + "net" + "time" + "github.com/docker/docker/pkg/plugins" ) @@ -15,10 +18,22 @@ const ( // CompatPlugin is an abstraction to handle both v2(new) and v1(legacy) plugins. type CompatPlugin interface { - Client() *plugins.Client Name() string ScopedPath(string) string IsV1() bool + PluginWithV1Client +} + +// PluginWithV1Client is a plugin that directly utilizes the v1/http plugin client +type PluginWithV1Client interface { + Client() *plugins.Client +} + +// PluginAddr is a plugin that exposes the socket address for creating custom clients rather than the built-in `*plugins.Client` +type PluginAddr interface { + Addr() net.Addr + Timeout() time.Duration + Protocol() string } // CountedPlugin is a plugin which is reference counted. diff --git a/components/engine/pkg/plugins/plugins.go b/components/engine/pkg/plugins/plugins.go index 3ee4720a19..6962079df9 100644 --- a/components/engine/pkg/plugins/plugins.go +++ b/components/engine/pkg/plugins/plugins.go @@ -31,6 +31,9 @@ import ( "github.com/sirupsen/logrus" ) +// ProtocolSchemeHTTPV1 is the name of the protocol used for interacting with plugins using this package. +const ProtocolSchemeHTTPV1 = "moby.plugins.http/v1" + var ( // ErrNotImplements is returned if the plugin does not implement the requested driver. ErrNotImplements = errors.New("Plugin does not implement the requested driver") @@ -88,6 +91,11 @@ func (p *Plugin) Client() *Client { return p.client } +// Protocol returns the protocol name/version used for plugins in this package. +func (p *Plugin) Protocol() string { + return ProtocolSchemeHTTPV1 +} + // IsV1 returns true for V1 plugins and false otherwise. func (p *Plugin) IsV1() bool { return true diff --git a/components/engine/pkg/urlutil/urlutil.go b/components/engine/pkg/urlutil/urlutil.go index eaf2535da3..9cf348c723 100644 --- a/components/engine/pkg/urlutil/urlutil.go +++ b/components/engine/pkg/urlutil/urlutil.go @@ -9,7 +9,15 @@ import ( var ( validPrefixes = map[string][]string{ - "url": {"http://", "https://"}, + "url": {"http://", "https://"}, + + // The github.com/ prefix is a special case used to treat context-paths + // starting with `github.com` as a git URL if the given path does not + // exist locally. The "github.com/" prefix is kept for backward compatibility, + // and is a legacy feature. + // + // Going forward, no additional prefixes should be added, and users should + // be encouraged to use explicit URLs (https://github.com/user/repo.git) instead. "git": {"git://", "github.com/", "git@"}, "transport": {"tcp://", "tcp+tls://", "udp://", "unix://", "unixgram://"}, } diff --git a/components/engine/plugin/manager_linux.go b/components/engine/plugin/manager_linux.go index 25297d077f..0029ff7868 100644 --- a/components/engine/plugin/manager_linux.go +++ b/components/engine/plugin/manager_linux.go @@ -71,14 +71,20 @@ func (pm *Manager) enable(p *v2.Plugin, c *controller, force bool) error { func (pm *Manager) pluginPostStart(p *v2.Plugin, c *controller) error { sockAddr := filepath.Join(pm.config.ExecRoot, p.GetID(), p.GetSocket()) - client, err := plugins.NewClientWithTimeout("unix://"+sockAddr, nil, time.Duration(c.timeoutInSecs)*time.Second) - if err != nil { - c.restart = false - shutdownPlugin(p, c, pm.executor) - return errors.WithStack(err) - } + p.SetTimeout(time.Duration(c.timeoutInSecs) * time.Second) + addr := &net.UnixAddr{Net: "unix", Name: sockAddr} + p.SetAddr(addr) - p.SetPClient(client) + if p.Protocol() == plugins.ProtocolSchemeHTTPV1 { + client, err := plugins.NewClientWithTimeout(addr.Network()+"://"+addr.String(), nil, p.Timeout()) + if err != nil { + c.restart = false + shutdownPlugin(p, c, pm.executor) + return errors.WithStack(err) + } + + p.SetPClient(client) + } // Initial sleep before net Dial to allow plugin to listen on socket. time.Sleep(500 * time.Millisecond) diff --git a/components/engine/plugin/v2/plugin.go b/components/engine/plugin/v2/plugin.go index 1c451691ce..6852511c5e 100644 --- a/components/engine/plugin/v2/plugin.go +++ b/components/engine/plugin/v2/plugin.go @@ -2,9 +2,11 @@ package v2 // import "github.com/docker/docker/plugin/v2" import ( "fmt" + "net" "path/filepath" "strings" "sync" + "time" "github.com/docker/docker/api/types" "github.com/docker/docker/pkg/plugingetter" @@ -27,6 +29,8 @@ type Plugin struct { modifyRuntimeSpec func(*specs.Spec) SwarmServiceID string + timeout time.Duration + addr net.Addr } const defaultPluginRuntimeDestination = "/run/docker/plugins" @@ -50,6 +54,7 @@ func (p *Plugin) ScopedPath(s string) string { } // Client returns the plugin client. +// Deprecated: use p.Addr() and manually create the client func (p *Plugin) Client() *plugins.Client { p.mu.RLock() defer p.mu.RUnlock() @@ -58,6 +63,7 @@ func (p *Plugin) Client() *plugins.Client { } // SetPClient set the plugin client. +// Deprecated: Hardcoded plugin client is deprecated func (p *Plugin) SetPClient(client *plugins.Client) { p.mu.Lock() defer p.mu.Unlock() @@ -264,3 +270,42 @@ func (p *Plugin) SetSpecOptModifier(f func(*specs.Spec)) { p.modifyRuntimeSpec = f p.mu.Unlock() } + +// Timeout gets the currently configured connection timeout. +// This should be used when dialing the plugin. +func (p *Plugin) Timeout() time.Duration { + p.mu.RLock() + t := p.timeout + p.mu.RUnlock() + return t +} + +// SetTimeout sets the timeout to use for dialing. +func (p *Plugin) SetTimeout(t time.Duration) { + p.mu.Lock() + p.timeout = t + p.mu.Unlock() +} + +// Addr returns the net.Addr to use to connect to the plugin socket +func (p *Plugin) Addr() net.Addr { + p.mu.RLock() + addr := p.addr + p.mu.RUnlock() + return addr +} + +// SetAddr sets the plugin address which can be used for dialing the plugin. +func (p *Plugin) SetAddr(addr net.Addr) { + p.mu.Lock() + p.addr = addr + p.mu.Unlock() +} + +// Protocol is the protocol that should be used for interacting with the plugin. +func (p *Plugin) Protocol() string { + if p.PluginObj.Config.Interface.ProtocolScheme != "" { + return p.PluginObj.Config.Interface.ProtocolScheme + } + return plugins.ProtocolSchemeHTTPV1 +} diff --git a/components/engine/plugin/v2/plugin_unsupported.go b/components/engine/plugin/v2/plugin_unsupported.go index 734b2ac664..5242fe124c 100644 --- a/components/engine/plugin/v2/plugin_unsupported.go +++ b/components/engine/plugin/v2/plugin_unsupported.go @@ -5,7 +5,7 @@ package v2 // import "github.com/docker/docker/plugin/v2" import ( "errors" - specs "github.com/opencontainers/runtime-spec/specs-go" + "github.com/opencontainers/runtime-spec/specs-go" ) // InitSpec creates an OCI spec from the plugin's config. diff --git a/components/engine/volume/drivers/adapter.go b/components/engine/volume/drivers/adapter.go index d5d9ea823e..2d891383cc 100644 --- a/components/engine/volume/drivers/adapter.go +++ b/components/engine/volume/drivers/adapter.go @@ -17,7 +17,7 @@ type volumeDriverAdapter struct { name string scopePath func(s string) string capabilities *volume.Capability - proxy *volumeDriverProxy + proxy volumeDriver } func (a *volumeDriverAdapter) Name() string { @@ -114,7 +114,7 @@ func (a *volumeDriverAdapter) getCapabilities() volume.Capability { } type volumeAdapter struct { - proxy *volumeDriverProxy + proxy volumeDriver name string scopePath func(string) string driverName string diff --git a/components/engine/volume/drivers/extpoint.go b/components/engine/volume/drivers/extpoint.go index 14e3b4f625..5a3c4ce95a 100644 --- a/components/engine/volume/drivers/extpoint.go +++ b/components/engine/volume/drivers/extpoint.go @@ -10,6 +10,7 @@ import ( "github.com/docker/docker/errdefs" "github.com/docker/docker/pkg/locker" getter "github.com/docker/docker/pkg/plugingetter" + "github.com/docker/docker/pkg/plugins" "github.com/docker/docker/volume" "github.com/pkg/errors" "github.com/sirupsen/logrus" @@ -17,12 +18,6 @@ import ( const extName = "VolumeDriver" -// NewVolumeDriver returns a driver has the given name mapped on the given client. -func NewVolumeDriver(name string, scopePath func(string) string, c client) volume.Driver { - proxy := &volumeDriverProxy{c} - return &volumeDriverAdapter{name: name, scopePath: scopePath, proxy: proxy} -} - // volumeDriver defines the available functions that volume plugins must implement. // This interface is only defined to generate the proxy objects. // It's not intended to be public or reused. @@ -93,7 +88,10 @@ func (s *Store) lookup(name string, mode int) (volume.Driver, error) { return nil, errors.Wrap(err, "error looking up volume plugin "+name) } - d := NewVolumeDriver(p.Name(), p.ScopedPath, p.Client()) + d, err := makePluginAdapter(p) + if err != nil { + return nil, errors.Wrap(err, "error making plugin client") + } if err := validateDriver(d); err != nil { if mode > 0 { // Undo any reference count changes from the initial `Get` @@ -201,7 +199,10 @@ func (s *Store) GetAllDrivers() ([]volume.Driver, error) { continue } - ext := NewVolumeDriver(name, p.ScopedPath, p.Client()) + ext, err := makePluginAdapter(p) + if err != nil { + return nil, errors.Wrap(err, "error making plugin client") + } if p.IsV1() { s.extensions[name] = ext } @@ -209,3 +210,26 @@ func (s *Store) GetAllDrivers() ([]volume.Driver, error) { } return ds, nil } + +func makePluginAdapter(p getter.CompatPlugin) (*volumeDriverAdapter, error) { + if pc, ok := p.(getter.PluginWithV1Client); ok { + return &volumeDriverAdapter{name: p.Name(), scopePath: p.ScopedPath, proxy: &volumeDriverProxy{pc.Client()}}, nil + } + + pa, ok := p.(getter.PluginAddr) + if !ok { + return nil, errdefs.System(errors.Errorf("got unknown plugin instance %T", p)) + } + + if pa.Protocol() != plugins.ProtocolSchemeHTTPV1 { + return nil, errors.Errorf("plugin protocol not supported: %s", p) + } + + addr := pa.Addr() + client, err := plugins.NewClientWithTimeout(addr.Network()+"://"+addr.String(), nil, pa.Timeout()) + if err != nil { + return nil, errors.Wrap(err, "error creating plugin client") + } + + return &volumeDriverAdapter{name: p.Name(), scopePath: p.ScopedPath, proxy: &volumeDriverProxy{client}}, nil +}