From 1376a6d8fc7a5d1c6d6fb84a47562b5cc9992e6b Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 17 Apr 2018 10:52:54 -0700 Subject: [PATCH 01/14] TestContainerStopTimeout: fix The unit test is checking that setting of non-default StopTimeout works, but it checked the value of StopSignal instead. Amazingly, the test was working since the default StopSignal is SIGTERM, which has the numeric value of 15. Fixes: commit e66d21089 ("Add config parameter to change ...") Signed-off-by: Kir Kolyshkin Upstream-commit: 743f5afcd13659e2b6b76791415876797e2ede52 Component: engine --- components/engine/container/container_unit_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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) } } From c51303ad1718c2120a4d64a549930c0f599c2854 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 16 Apr 2018 16:58:42 -0700 Subject: [PATCH 02/14] daemon.ContainerStop(): fix for a negative timeout 1. As daemon.ContainerStop() documentation says, > If a negative number of seconds is given, ContainerStop > will wait for a graceful termination. but since commit cfdf84d5d04c8ee (PR #32237) this is no longer the case. This happens because `context.WithTimeout(ctx, timeout)` is implemented as `WithDeadline(ctx, time.Now().Add(timeout))`, resulting in a deadline which is in the past. To fix, don't use WithDeadline() if the timeout is negative. 2. Add a test case to validate the correct behavior and as a means to prevent a similar regression in the future. 3. Fix/improve daemon.ContainerStop() and client.ContainerStop() description for clarity and completeness. 4. Fix/improve DefaultStopTimeout description. Fixes: cfdf84d5d04c ("Update Container Wait") Signed-off-by: Kir Kolyshkin Upstream-commit: 69b4fe406540c7989bd99b2244ed67ced96e17c7 Component: engine --- components/engine/client/container_stop.go | 9 ++- components/engine/container/container_unix.go | 3 +- components/engine/daemon/stop.go | 36 ++++++------ .../engine/integration/container/stop_test.go | 55 +++++++++++++++++++ 4 files changed, 83 insertions(+), 20 deletions(-) diff --git a/components/engine/client/container_stop.go b/components/engine/client/container_stop.go index ca316666a9..dcb1bed5eb 100644 --- a/components/engine/client/container_stop.go +++ b/components/engine/client/container_stop.go @@ -8,8 +8,13 @@ import ( "golang.org/x/net/context" ) -// 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_unix.go b/components/engine/container/container_unix.go index e5cecf3166..c85c78bb91 100644 --- a/components/engine/container/container_unix.go +++ b/components/engine/container/container_unix.go @@ -22,7 +22,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/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 5cb970ba58..592f585327 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.IfCondition(t, testEnv.DaemonInfo.Driver != "devicemapper") From c8db46ad053daa297bf3b3f1a4e8b6e840d1e81f Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 23 May 2018 16:31:49 +0200 Subject: [PATCH 03/14] builder: fix processing of invalid substitusion syntax The builder did not detect syntax errors in substitusions in the Dockerfile, causing those values to be processed incorrectly instead of producing an error. Example 1: missing `}` docker build --no-cache -<<'EOF' FROM busybox ARG var=${aaa:-bbb RUN echo $var EOF Before: Step 3/3 : RUN echo $var ---> Running in f06571e77146 bbb After: Step 2/3 : ARG var=${aaa:-bbb failed to process "${aaa:-bbb": syntax error: missing '}' Example 2: missing closing `}`, no default value docker build --no-cache -<<'EOF' FROM busybox ARG var=${aaa RUN echo $var EOF Before: Step 2/3 : ARG var=${aaa failed to process "${aaa": missing ':' in substitution After: Step 2/3 : ARG var=${aaa failed to process "${aaa": syntax error: missing '}' Example 3: double opening bracket (`{`) docker build --no-cache -<<'EOF' FROM busybox ARG var=${{aaa:-bbb} RUN echo $var EOF Before: Step 2/3 : ARG var=${{aaa:-bbb} failed to process "${{aaa:-bbb}": missing ':' in substitution After: Step 2/3 : ARG var=${{aaa:-bbb} failed to process "${{aaa:-bbb}": syntax error: bad substitution Example 4: double opening bracket (`{`), no default value docker build --no-cache -<<'EOF' FROM busybox ARG var=${{aaa} RUN echo $var EOF Before: Step 2/3 : ARG var=${{aaa} failed to process "${{aaa}": missing ':' in substitution After: Step 2/3 : ARG var=${{aaa} failed to process "${{aaa}": syntax error: bad substitution Signed-off-by: Sebastiaan van Stijn Upstream-commit: 955a6ad95f7891a45692d975793abf1eeb07cdd5 Component: engine --- .../builder/dockerfile/shell/envVarTest | 11 ++++++++++ .../engine/builder/dockerfile/shell/lex.go | 20 ++++++++++++++----- .../builder/dockerfile/shell/lex_test.go | 2 +- 3 files changed, 27 insertions(+), 6 deletions(-) diff --git a/components/engine/builder/dockerfile/shell/envVarTest b/components/engine/builder/dockerfile/shell/envVarTest index 946b278592..677b33d9a7 100644 --- a/components/engine/builder/dockerfile/shell/envVarTest +++ b/components/engine/builder/dockerfile/shell/envVarTest @@ -119,3 +119,14 @@ 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 + diff --git a/components/engine/builder/dockerfile/shell/lex.go b/components/engine/builder/dockerfile/shell/lex.go index bd3fac525a..d0d6f53e66 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 } @@ -261,12 +263,17 @@ func (sw *shellWord) processDollar() (string, error) { sw.scanner.Next() name := sw.processName() ch := sw.scanner.Peek() - if ch == '}' { + switch ch { + case '}': // Normal ${xx} case sw.scanner.Next() return sw.getEnv(name), nil - } - if ch == ':' { + case '{': + // Invalid ${{xx} case + return "", errors.New("syntax error: bad substitution") + case scanner.EOF: + return "", errors.New("syntax error: missing '}'") + case ':': // Special ${xx:...} format processing // Yes it allows for recursive $'s in the ... spot @@ -275,6 +282,9 @@ func (sw *shellWord) processDollar() (string, error) { word, _, err := sw.processStopOn('}') if err != nil { + if sw.scanner.Peek() == scanner.EOF { + return "", errors.New("syntax error: missing '}'") + } return "", err } diff --git a/components/engine/builder/dockerfile/shell/lex_test.go b/components/engine/builder/dockerfile/shell/lex_test.go index f38da2026f..765ae14b25 100644 --- a/components/engine/builder/dockerfile/shell/lex_test.go +++ b/components/engine/builder/dockerfile/shell/lex_test.go @@ -53,7 +53,7 @@ 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)) From 1f9af8d4f95f861f4bc323f33cd768f759692925 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 25 May 2018 17:31:14 +0200 Subject: [PATCH 04/14] Fix detection for missing parameter in substitution `${}`, `${:}` and so on are invalid because there's no parameter within the brackets; fix detection for this situation and add/update tests. There were some existing test-cases that were testing for the wrong behavior, which are now updated. Signed-off-by: Sebastiaan van Stijn Upstream-commit: 334bf3ea76004d0abe02dd1698989f9eaf87a86a Component: engine --- .../engine/builder/dockerfile/shell/envVarTest | 15 +++++++++++---- .../engine/builder/dockerfile/shell/lex.go | 17 ++++++++--------- 2 files changed, 19 insertions(+), 13 deletions(-) diff --git a/components/engine/builder/dockerfile/shell/envVarTest b/components/engine/builder/dockerfile/shell/envVarTest index 677b33d9a7..2c0cc1fcf4 100644 --- a/components/engine/builder/dockerfile/shell/envVarTest +++ b/components/engine/builder/dockerfile/shell/envVarTest @@ -29,10 +29,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 +92,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 @@ -129,4 +133,7 @@ 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 diff --git a/components/engine/builder/dockerfile/shell/lex.go b/components/engine/builder/dockerfile/shell/lex.go index d0d6f53e66..983e5c0517 100644 --- a/components/engine/builder/dockerfile/shell/lex.go +++ b/components/engine/builder/dockerfile/shell/lex.go @@ -261,23 +261,22 @@ func (sw *shellWord) processDollar() (string, error) { } sw.scanner.Next() + 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") + } name := sw.processName() - ch := sw.scanner.Peek() + ch := sw.scanner.Next() switch ch { case '}': // Normal ${xx} case - sw.scanner.Next() return sw.getEnv(name), nil - case '{': - // Invalid ${{xx} case - return "", errors.New("syntax error: bad substitution") - case scanner.EOF: - return "", errors.New("syntax error: missing '}'") 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('}') From ad9f08c7259aa480579f8607fb20e529f1d6cdd6 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 25 May 2018 17:44:35 +0200 Subject: [PATCH 05/14] Add line-numbers to asserts Signed-off-by: Sebastiaan van Stijn Upstream-commit: b80e0309d220268a2b9e6aec5bb05d7af330e591 Component: engine --- components/engine/builder/dockerfile/shell/lex_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/components/engine/builder/dockerfile/shell/lex_test.go b/components/engine/builder/dockerfile/shell/lex_test.go index 765ae14b25..fa42c92989 100644 --- a/components/engine/builder/dockerfile/shell/lex_test.go +++ b/components/engine/builder/dockerfile/shell/lex_test.go @@ -55,8 +55,8 @@ func TestShellParser4EnvVars(t *testing.T) { if expected == "error" { 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) } } } From 0ed9886f87c0aa7f092c5350e54728834a86753b Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 25 May 2018 17:45:25 +0200 Subject: [PATCH 06/14] Add more test-cases for positional parameters Signed-off-by: Sebastiaan van Stijn Upstream-commit: 8687a3f4b8b7ffbc3f32dfe959cb771d662211e6 Component: engine --- .../builder/dockerfile/shell/envVarTest | 39 ++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) diff --git a/components/engine/builder/dockerfile/shell/envVarTest b/components/engine/builder/dockerfile/shell/envVarTest index 2c0cc1fcf4..005a54e99d 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 @@ -137,3 +136,41 @@ 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 From 45e37846106deda69399e3198e0c671c4b416b58 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 25 May 2018 17:59:32 +0200 Subject: [PATCH 07/14] Add detection of "special parameters" for substitution Detect Special parameters as defined in http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_05_02 Treat these as parameters that are not set, instead of producing an error that a modifier is missing. Signed-off-by: Sebastiaan van Stijn Upstream-commit: 9654e9b6f80e1b931763c04400a19596e256c99a Component: engine --- .../builder/dockerfile/shell/envVarTest | 35 +++++++++++++++++++ .../engine/builder/dockerfile/shell/lex.go | 14 +++++++- .../builder/dockerfile/shell/lex_test.go | 8 ++--- 3 files changed, 51 insertions(+), 6 deletions(-) diff --git a/components/engine/builder/dockerfile/shell/envVarTest b/components/engine/builder/dockerfile/shell/envVarTest index 005a54e99d..6071caebd2 100644 --- a/components/engine/builder/dockerfile/shell/envVarTest +++ b/components/engine/builder/dockerfile/shell/envVarTest @@ -174,3 +174,38 @@ A|$9 | A|${9} | A|${9:+bbb} | A|${9:-bbb} | bbb + +# 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|${*} | +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 983e5c0517..a51a2665aa 100644 --- a/components/engine/builder/dockerfile/shell/lex.go +++ b/components/engine/builder/dockerfile/shell/lex.go @@ -318,7 +318,7 @@ func (sw *shellWord) processName() string { for sw.scanner.Peek() != scanner.EOF { ch := sw.scanner.Peek() - if name.Len() == 0 && unicode.IsDigit(ch) { + if name.Len() == 0 && (unicode.IsDigit(ch) || isSpecialParam(ch)) { ch = sw.scanner.Next() return string(ch) } @@ -332,6 +332,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 fa42c92989..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 } From 87e43e176248537b74f9d5f919d6fd4c801ced7e Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 25 May 2018 18:49:54 +0200 Subject: [PATCH 08/14] Handle multi-digit positional parameters Signed-off-by: Sebastiaan van Stijn Upstream-commit: 2628896b5e813491f8767ec7fa9d0f057ed4a86e Component: engine --- .../builder/dockerfile/shell/envVarTest | 21 +++++++++++++++++++ .../engine/builder/dockerfile/shell/lex.go | 10 ++++++++- 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/components/engine/builder/dockerfile/shell/envVarTest b/components/engine/builder/dockerfile/shell/envVarTest index 6071caebd2..08011801c5 100644 --- a/components/engine/builder/dockerfile/shell/envVarTest +++ b/components/engine/builder/dockerfile/shell/envVarTest @@ -174,6 +174,22 @@ 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 @@ -181,6 +197,11 @@ 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} | diff --git a/components/engine/builder/dockerfile/shell/lex.go b/components/engine/builder/dockerfile/shell/lex.go index a51a2665aa..0c80900ade 100644 --- a/components/engine/builder/dockerfile/shell/lex.go +++ b/components/engine/builder/dockerfile/shell/lex.go @@ -318,7 +318,15 @@ func (sw *shellWord) processName() string { for sw.scanner.Peek() != scanner.EOF { ch := sw.scanner.Peek() - if name.Len() == 0 && (unicode.IsDigit(ch) || isSpecialParam(ch)) { + 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) } From 4c7a840b18efea35000c0706a6cb0dc4bb057fc8 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Tue, 24 Apr 2018 21:45:00 -0400 Subject: [PATCH 09/14] Move plugin client creation to the extension point Signed-off-by: Brian Goff Upstream-commit: f51a96c0165fdcbbe11f62b66b582b7e202f211b Component: engine --- components/engine/api/swagger.yaml | 7 +++ components/engine/api/types/plugin.go | 3 ++ .../engine/daemon/graphdriver/plugin.go | 20 +++++++- components/engine/daemon/graphdriver/proxy.go | 35 ++++++------- components/engine/daemon/logger/plugin.go | 28 +++++++++-- components/engine/daemon/metrics.go | 49 ++++++++++++++++--- components/engine/daemon/metrics_unix.go | 8 ++- components/engine/pkg/plugingetter/getter.go | 11 +++++ components/engine/pkg/plugins/plugins.go | 8 +++ components/engine/plugin/manager_linux.go | 20 +++++--- components/engine/plugin/v2/plugin.go | 45 +++++++++++++++++ .../engine/plugin/v2/plugin_unsupported.go | 2 +- components/engine/volume/drivers/adapter.go | 4 +- components/engine/volume/drivers/extpoint.go | 36 +++++++++++--- 14 files changed, 229 insertions(+), 47 deletions(-) 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/daemon/graphdriver/plugin.go b/components/engine/daemon/graphdriver/plugin.go index d8058d9236..f5940dfe82 100644 --- a/components/engine/daemon/graphdriver/plugin.go +++ b/components/engine/daemon/graphdriver/plugin.go @@ -5,7 +5,9 @@ import ( "path/filepath" "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 +30,22 @@ func newPluginDriver(name string, pl plugingetter.CompatPlugin, config Options) } } } - proxy := &graphDriverProxy{name, pl, Capabilities{}} + + var proxy *graphDriverProxy + + pa, ok := pl.(plugingetter.PluginAddr) + if !ok { + proxy = &graphDriverProxy{name, pl, Capabilities{}, pl.Client()} + } else { + 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 plugin client") + } + proxy = &graphDriverProxy{name, pl, Capabilities{}, client} + } 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..4cbb469515 100644 --- a/components/engine/daemon/logger/plugin.go +++ b/components/engine/daemon/logger/plugin.go @@ -8,6 +8,7 @@ import ( "github.com/docker/docker/api/types/plugins/logdriver" 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 +38,32 @@ 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) { + pa, ok := p.(getter.PluginAddr) + if !ok { + return &logPluginProxy{p.Client()}, nil + } + + 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..4bf4310cda 100644 --- a/components/engine/daemon/metrics.go +++ b/components/engine/daemon/metrics.go @@ -4,6 +4,7 @@ import ( "sync" "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 +119,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 +137,39 @@ func (d *Daemon) cleanupMetricsPlugins() { } } -func pluginStartMetricsCollection(p plugingetter.CompatPlugin) error { +type metricsPlugin interface { + StartMetrics() error + StopMetrics() error +} + +func makePluginAdapter(p plugingetter.CompatPlugin) (metricsPlugin, error) { + pa, ok := p.(plugingetter.PluginAddr) + if !ok { + return &metricsPluginAdapter{p.Client(), p.Name()}, nil + } + 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 +178,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/pkg/plugingetter/getter.go b/components/engine/pkg/plugingetter/getter.go index 0e1699d913..031b5049c2 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" ) @@ -21,6 +24,14 @@ type CompatPlugin interface { IsV1() bool } +// PluginAddr is a plugin that exposes the socket address for creating custom clients rather than the built-in `*plugins.Client` +type PluginAddr interface { + CompatPlugin + Addr() net.Addr + Timeout() time.Duration + Protocol() string +} + // CountedPlugin is a plugin which is reference counted. type CountedPlugin interface { Acquire() 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/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..f7d3617caa 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,22 @@ func (s *Store) GetAllDrivers() ([]volume.Driver, error) { } return ds, nil } + +func makePluginAdapter(p getter.CompatPlugin) (*volumeDriverAdapter, error) { + pa, ok := p.(getter.PluginAddr) + if !ok { + return &volumeDriverAdapter{name: p.Name(), scopePath: p.ScopedPath, proxy: &volumeDriverProxy{p.Client()}}, nil + } + + 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 +} From a99e1e1ff6f9a7c75bae632d4416d16f9bb6356b Mon Sep 17 00:00:00 2001 From: Anda Xu Date: Tue, 29 May 2018 15:09:11 -0700 Subject: [PATCH 10/14] add api version checking for tests from new feature Signed-off-by: Anda Xu Upstream-commit: 8ed0fdebe714c64aa697ed7e2187d2233e34eb22 Component: engine --- components/engine/integration/network/service_test.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/components/engine/integration/network/service_test.go b/components/engine/integration/network/service_test.go index 8a92786fd8..6f6da18cd1 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" @@ -24,7 +25,7 @@ func delInterface(t *testing.T, ifName string) { } func TestDaemonRestartWithLiveRestore(t *testing.T) { - skip.If(t, testEnv.IsRemoteDaemon()) + skip.If(t, testEnv.IsRemoteDaemon(), versions.LessThan(testEnv.DaemonAPIVersion(), "1.38")) d := daemon.New(t) defer d.Stop(t) d.Start(t) @@ -44,7 +45,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, testEnv.IsRemoteDaemon(), versions.LessThan(testEnv.DaemonAPIVersion(), "1.38")) defaultNetworkBridge := "docker0" delInterface(t, defaultNetworkBridge) d := daemon.New(t) @@ -89,7 +90,7 @@ func TestDaemonDefaultNetworkPools(t *testing.T) { } func TestDaemonRestartWithExistingNetwork(t *testing.T) { - skip.If(t, testEnv.IsRemoteDaemon()) + skip.If(t, testEnv.IsRemoteDaemon(), versions.LessThan(testEnv.DaemonAPIVersion(), "1.38")) defaultNetworkBridge := "docker0" d := daemon.New(t) d.Start(t) @@ -123,7 +124,7 @@ func TestDaemonRestartWithExistingNetwork(t *testing.T) { } func TestDaemonRestartWithExistingNetworkWithDefaultPoolRange(t *testing.T) { - skip.If(t, testEnv.IsRemoteDaemon()) + skip.If(t, testEnv.IsRemoteDaemon(), versions.LessThan(testEnv.DaemonAPIVersion(), "1.38")) defaultNetworkBridge := "docker0" d := daemon.New(t) d.Start(t) @@ -179,7 +180,7 @@ func TestDaemonRestartWithExistingNetworkWithDefaultPoolRange(t *testing.T) { } func TestDaemonWithBipAndDefaultNetworkPool(t *testing.T) { - skip.If(t, testEnv.IsRemoteDaemon()) + skip.If(t, testEnv.IsRemoteDaemon(), versions.LessThan(testEnv.DaemonAPIVersion(), "1.38")) defaultNetworkBridge := "docker0" d := daemon.New(t) defer d.Stop(t) From 90dfa0b4ee9fe841656b1a8285b353f7c1ac4d26 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 30 May 2018 11:06:06 +0200 Subject: [PATCH 11/14] Be explicit about github.com prefix being a legacy feature Signed-off-by: Sebastiaan van Stijn Upstream-commit: babb0c14fd1281b309de46c3481a5bea88b8031d Component: engine --- components/engine/pkg/urlutil/urlutil.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) 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://"}, } From 285424548ce8db7631342628e5acea37be124703 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Wed, 30 May 2018 15:00:42 -0400 Subject: [PATCH 12/14] Move plugin client to separate interface This makes it a bit simpler to remove this interface for v2 plugins and not break external projects (libnetwork and swarmkit). Note that before we remove the `Client()` interface from `CompatPlugin` libnetwork and swarmkit must be updated to explicitly check for the v1 client interface as is done int his PR. This is just a minor tweak that I realized is needed after trying to implement the needed changes on libnetwork. Signed-off-by: Brian Goff Upstream-commit: 7c77df8acc597cd4f540d873de5fe53a3d414ba9 Component: engine --- .../engine/daemon/graphdriver/plugin.go | 20 +++++++++++-------- components/engine/daemon/logger/plugin.go | 6 +++++- components/engine/daemon/metrics.go | 10 ++++++++-- components/engine/pkg/plugingetter/getter.go | 8 ++++++-- components/engine/volume/drivers/extpoint.go | 6 +++++- 5 files changed, 36 insertions(+), 14 deletions(-) diff --git a/components/engine/daemon/graphdriver/plugin.go b/components/engine/daemon/graphdriver/plugin.go index f5940dfe82..b0983c5667 100644 --- a/components/engine/daemon/graphdriver/plugin.go +++ b/components/engine/daemon/graphdriver/plugin.go @@ -4,6 +4,7 @@ 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" @@ -33,19 +34,22 @@ func newPluginDriver(name string, pl plugingetter.CompatPlugin, config Options) var proxy *graphDriverProxy - pa, ok := pl.(plugingetter.PluginAddr) - if !ok { - proxy = &graphDriverProxy{name, pl, Capabilities{}, pl.Client()} - } else { - if pa.Protocol() != plugins.ProtocolSchemeHTTPV1 { - return nil, errors.Errorf("plugin protocol not supported: %s", pa.Protocol()) + 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 := pa.Addr() - client, err := plugins.NewClientWithTimeout(addr.Network()+"://"+addr.String(), nil, pa.Timeout()) + 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/logger/plugin.go b/components/engine/daemon/logger/plugin.go index 4cbb469515..c66540ce52 100644 --- a/components/engine/daemon/logger/plugin.go +++ b/components/engine/daemon/logger/plugin.go @@ -7,6 +7,7 @@ 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" @@ -46,9 +47,12 @@ func getPlugin(name string, mode int) (Creator, error) { } 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 &logPluginProxy{p.Client()}, nil + return nil, errdefs.System(errors.Errorf("got unknown plugin type %T", p)) } if pa.Protocol() != plugins.ProtocolSchemeHTTPV1 { diff --git a/components/engine/daemon/metrics.go b/components/engine/daemon/metrics.go index 4bf4310cda..f6961a3553 100644 --- a/components/engine/daemon/metrics.go +++ b/components/engine/daemon/metrics.go @@ -3,6 +3,7 @@ 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" @@ -142,11 +143,16 @@ type metricsPlugin interface { StopMetrics() error } -func makePluginAdapter(p plugingetter.CompatPlugin) (metricsPlugin, 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 &metricsPluginAdapter{p.Client(), p.Name()}, nil + 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()) } diff --git a/components/engine/pkg/plugingetter/getter.go b/components/engine/pkg/plugingetter/getter.go index 031b5049c2..370e0d5b97 100644 --- a/components/engine/pkg/plugingetter/getter.go +++ b/components/engine/pkg/plugingetter/getter.go @@ -18,15 +18,19 @@ 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 { - CompatPlugin Addr() net.Addr Timeout() time.Duration Protocol() string diff --git a/components/engine/volume/drivers/extpoint.go b/components/engine/volume/drivers/extpoint.go index f7d3617caa..5a3c4ce95a 100644 --- a/components/engine/volume/drivers/extpoint.go +++ b/components/engine/volume/drivers/extpoint.go @@ -212,9 +212,13 @@ func (s *Store) GetAllDrivers() ([]volume.Driver, error) { } 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 &volumeDriverAdapter{name: p.Name(), scopePath: p.ScopedPath, proxy: &volumeDriverProxy{p.Client()}}, nil + return nil, errdefs.System(errors.Errorf("got unknown plugin instance %T", p)) } if pa.Protocol() != plugins.ProtocolSchemeHTTPV1 { From 442215fe6a835e0cd733c984ce836c9d370ccbbd Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Wed, 30 May 2018 14:41:17 -0700 Subject: [PATCH 13/14] builder: fix layer leak on multi-stage wildcard copy Signed-off-by: Tonis Tiigi Upstream-commit: 4a18c11bdc1300692def2453c441190d73e1e942 Component: engine --- components/engine/builder/dockerfile/copy.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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 { From 98281f1a9cd0977c895cc73c6ab8bb33a1272dc4 Mon Sep 17 00:00:00 2001 From: Anda Xu Date: Wed, 30 May 2018 16:58:06 -0700 Subject: [PATCH 14/14] fix the mis-used skip condition Signed-off-by: Anda Xu Upstream-commit: d8e6f273b5016880c5d49673ddf531ceecc2bc0d Component: engine --- .../engine/integration/network/service_test.go | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/components/engine/integration/network/service_test.go b/components/engine/integration/network/service_test.go index 6f6da18cd1..77ef870911 100644 --- a/components/engine/integration/network/service_test.go +++ b/components/engine/integration/network/service_test.go @@ -25,7 +25,8 @@ func delInterface(t *testing.T, ifName string) { } func TestDaemonRestartWithLiveRestore(t *testing.T) { - skip.If(t, testEnv.IsRemoteDaemon(), versions.LessThan(testEnv.DaemonAPIVersion(), "1.38")) + 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,7 +46,8 @@ 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(), versions.LessThan(testEnv.DaemonAPIVersion(), "1.38")) + 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,7 +92,8 @@ func TestDaemonDefaultNetworkPools(t *testing.T) { } func TestDaemonRestartWithExistingNetwork(t *testing.T) { - skip.If(t, testEnv.IsRemoteDaemon(), versions.LessThan(testEnv.DaemonAPIVersion(), "1.38")) + 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,7 +127,8 @@ func TestDaemonRestartWithExistingNetwork(t *testing.T) { } func TestDaemonRestartWithExistingNetworkWithDefaultPoolRange(t *testing.T) { - skip.If(t, testEnv.IsRemoteDaemon(), versions.LessThan(testEnv.DaemonAPIVersion(), "1.38")) + 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,7 +184,8 @@ func TestDaemonRestartWithExistingNetworkWithDefaultPoolRange(t *testing.T) { } func TestDaemonWithBipAndDefaultNetworkPool(t *testing.T) { - skip.If(t, testEnv.IsRemoteDaemon(), versions.LessThan(testEnv.DaemonAPIVersion(), "1.38")) + 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)