Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LCOW: WORKDIR correct handling #34405

Merged
merged 1 commit into from
Aug 18, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion builder/dockerfile/dispatchers.go
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,7 @@ func workdir(req dispatchRequest) error {
runConfig := req.state.runConfig
// This is from the Dockerfile and will not necessarily be in platform
// specific semantics, hence ensure it is converted.
runConfig.WorkingDir, err = normaliseWorkdir(runConfig.WorkingDir, req.args[0])
runConfig.WorkingDir, err = normaliseWorkdir(req.builder.platform, runConfig.WorkingDir, req.args[0])
if err != nil {
return err
}
Expand Down
2 changes: 1 addition & 1 deletion builder/dockerfile/dispatchers_unix.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (

// normaliseWorkdir normalises a user requested working directory in a
// platform semantically consistent way.
func normaliseWorkdir(current string, requested string) (string, error) {
func normaliseWorkdir(_ string, current string, requested string) (string, error) {
if requested == "" {
return "", errors.New("cannot normalise nothing")
}
Expand Down
3 changes: 2 additions & 1 deletion builder/dockerfile/dispatchers_unix_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
package dockerfile

import (
"runtime"
"testing"
)

Expand All @@ -16,7 +17,7 @@ func TestNormaliseWorkdir(t *testing.T) {
}

for _, test := range testCases {
normalised, err := normaliseWorkdir(test.current, test.requested)
normalised, err := normaliseWorkdir(runtime.GOOS, test.current, test.requested)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: normalize (do we standardize on us eng?)


if test.expectedError != "" && err == nil {
t.Fatalf("NormaliseWorkdir should return an error %s, got nil", test.expectedError)
Expand Down
29 changes: 28 additions & 1 deletion builder/dockerfile/dispatchers_windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"errors"
"fmt"
"os"
"path"
"path/filepath"
"regexp"
"strings"
Expand All @@ -15,7 +16,33 @@ var pattern = regexp.MustCompile(`^[a-zA-Z]:\.$`)

// normaliseWorkdir normalises a user requested working directory in a
// platform semantically consistent way.
func normaliseWorkdir(current string, requested string) (string, error) {
func normaliseWorkdir(platform string, current string, requested string) (string, error) {
if platform == "" {
platform = "windows"
}
if platform == "windows" {
return normaliseWorkdirWindows(current, requested)
}
return normaliseWorkdirUnix(current, requested)
}

// normaliseWorkdirUnix normalises a user requested working directory in a
// platform semantically consistent way.
func normaliseWorkdirUnix(current string, requested string) (string, error) {
if requested == "" {
return "", errors.New("cannot normalise nothing")
}
current = strings.Replace(current, string(os.PathSeparator), "/", -1)
requested = strings.Replace(requested, string(os.PathSeparator), "/", -1)
if !path.IsAbs(requested) {
return path.Join(`/`, current, requested), nil
}
return requested, nil
}

// normaliseWorkdirWindows normalises a user requested working directory in a
// platform semantically consistent way.
func normaliseWorkdirWindows(current string, requested string) (string, error) {
if requested == "" {
return "", errors.New("cannot normalise nothing")
}
Expand Down
40 changes: 23 additions & 17 deletions builder/dockerfile/dispatchers_windows_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,25 +5,31 @@ package dockerfile
import "testing"

func TestNormaliseWorkdir(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Normalize"

tests := []struct{ current, requested, expected, etext string }{
{``, ``, ``, `cannot normalise nothing`},
{``, `C:`, ``, `C:. is not a directory. If you are specifying a drive letter, please add a trailing '\'`},
{``, `C:.`, ``, `C:. is not a directory. If you are specifying a drive letter, please add a trailing '\'`},
{`c:`, `\a`, ``, `c:. is not a directory. If you are specifying a drive letter, please add a trailing '\'`},
{`c:.`, `\a`, ``, `c:. is not a directory. If you are specifying a drive letter, please add a trailing '\'`},
{``, `a`, `C:\a`, ``},
{``, `c:\foo`, `C:\foo`, ``},
{``, `c:\\foo`, `C:\foo`, ``},
{``, `\foo`, `C:\foo`, ``},
{``, `\\foo`, `C:\foo`, ``},
{``, `/foo`, `C:\foo`, ``},
{``, `C:/foo`, `C:\foo`, ``},
{`C:\foo`, `bar`, `C:\foo\bar`, ``},
{`C:\foo`, `/bar`, `C:\bar`, ``},
{`C:\foo`, `\bar`, `C:\bar`, ``},
tests := []struct{ platform, current, requested, expected, etext string }{
{"windows", ``, ``, ``, `cannot normalise nothing`},
{"windows", ``, `C:`, ``, `C:. is not a directory. If you are specifying a drive letter, please add a trailing '\'`},
{"windows", ``, `C:.`, ``, `C:. is not a directory. If you are specifying a drive letter, please add a trailing '\'`},
{"windows", `c:`, `\a`, ``, `c:. is not a directory. If you are specifying a drive letter, please add a trailing '\'`},
{"windows", `c:.`, `\a`, ``, `c:. is not a directory. If you are specifying a drive letter, please add a trailing '\'`},
{"windows", ``, `a`, `C:\a`, ``},
{"windows", ``, `c:\foo`, `C:\foo`, ``},
{"windows", ``, `c:\\foo`, `C:\foo`, ``},
{"windows", ``, `\foo`, `C:\foo`, ``},
{"windows", ``, `\\foo`, `C:\foo`, ``},
{"windows", ``, `/foo`, `C:\foo`, ``},
{"windows", ``, `C:/foo`, `C:\foo`, ``},
{"windows", `C:\foo`, `bar`, `C:\foo\bar`, ``},
{"windows", `C:\foo`, `/bar`, `C:\bar`, ``},
{"windows", `C:\foo`, `\bar`, `C:\bar`, ``},
{"linux", ``, ``, ``, `cannot normalise nothing`},
{"linux", ``, `foo`, `/foo`, ``},
{"linux", ``, `/foo`, `/foo`, ``},
{"linux", `/foo`, `bar`, `/foo/bar`, ``},
{"linux", `/foo`, `/bar`, `/bar`, ``},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since normaliseWorkdirUnix replaces Windows path separators with Unix, how about tests for that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added case {"linux", \a, b\c, /a/b/c, ``},

{"linux", `\a`, `b\c`, `/a/b/c`, ``},
}
for _, i := range tests {
r, e := normaliseWorkdir(i.current, i.requested)
r, e := normaliseWorkdir(i.platform, i.current, i.requested)

if i.etext != "" && e == nil {
t.Fatalf("TestNormaliseWorkingDir Expected error %s for '%s' '%s', got no error", i.etext, i.current, i.requested)
Expand Down
7 changes: 6 additions & 1 deletion container/container.go
Original file line number Diff line number Diff line change
Expand Up @@ -261,12 +261,17 @@ func (container *Container) WriteHostConfig() (*containertypes.HostConfig, error

// SetupWorkingDirectory sets up the container's working directory as set in container.Config.WorkingDir
func (container *Container) SetupWorkingDirectory(rootIDs idtools.IDPair) error {
// TODO @jhowardmsft, @gupta-ak LCOW Support. This will need revisiting.
// We will need to do remote filesystem operations here.
if container.Platform != runtime.GOOS {
return nil
}

if container.Config.WorkingDir == "" {
return nil
}

container.Config.WorkingDir = filepath.Clean(container.Config.WorkingDir)

pth, err := container.GetResourcePath(container.Config.WorkingDir)
if err != nil {
return err
Expand Down
22 changes: 18 additions & 4 deletions daemon/container.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@ package daemon
import (
"fmt"
"os"
"path"
"path/filepath"
"runtime"
"strings"
"time"

containertypes "github.com/docker/docker/api/types/container"
Expand Down Expand Up @@ -226,13 +229,24 @@ func (daemon *Daemon) setHostConfig(container *container.Container, hostConfig *

// verifyContainerSettings performs validation of the hostconfig and config
// structures.
func (daemon *Daemon) verifyContainerSettings(hostConfig *containertypes.HostConfig, config *containertypes.Config, update bool) ([]string, error) {

func (daemon *Daemon) verifyContainerSettings(platform string, hostConfig *containertypes.HostConfig, config *containertypes.Config, update bool) ([]string, error) {
// First perform verification of settings common across all platforms.
if config != nil {
if config.WorkingDir != "" {
config.WorkingDir = filepath.FromSlash(config.WorkingDir) // Ensure in platform semantics
if !system.IsAbs(config.WorkingDir) {
wdInvalid := false
if runtime.GOOS == platform {
Copy link
Member

@johnstep johnstep Aug 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a sufficient LCOW check? I think so, at least for now, but maybe system.LCOWSupported is preferred.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could be more explicit, but in reality, here it's redundant. It can only be LCOW.

config.WorkingDir = filepath.FromSlash(config.WorkingDir) // Ensure in platform semantics
if !system.IsAbs(config.WorkingDir) {
wdInvalid = true
}
} else {
// LCOW. Force Unix semantics
config.WorkingDir = strings.Replace(config.WorkingDir, string(os.PathSeparator), "/", -1)
if !path.IsAbs(config.WorkingDir) {
wdInvalid = true
}
}
if wdInvalid {
return nil, fmt.Errorf("the working directory '%s' is invalid, it needs to be an absolute path", config.WorkingDir)
}
}
Expand Down
22 changes: 11 additions & 11 deletions daemon/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,17 @@ func (daemon *Daemon) containerCreate(params types.ContainerCreateConfig, manage
return containertypes.ContainerCreateCreatedBody{}, validationError{errors.New("Config cannot be empty in order to create a container")}
}

warnings, err := daemon.verifyContainerSettings(params.HostConfig, params.Config, false)
// TODO: @jhowardmsft LCOW support - at a later point, can remove the hard-coding
// to force the platform to be linux.
// Default the platform if not supplied
if params.Platform == "" {
params.Platform = runtime.GOOS
}
if system.LCOWSupported() {
params.Platform = "linux"
}

warnings, err := daemon.verifyContainerSettings(params.Platform, params.HostConfig, params.Config, false)
if err != nil {
return containertypes.ContainerCreateCreatedBody{Warnings: warnings}, validationError{err}
}
Expand Down Expand Up @@ -75,16 +85,6 @@ func (daemon *Daemon) create(params types.ContainerCreateConfig, managed bool) (
err error
)

// TODO: @jhowardmsft LCOW support - at a later point, can remove the hard-coding
// to force the platform to be linux.
// Default the platform if not supplied
if params.Platform == "" {
params.Platform = runtime.GOOS
}
if system.LCOWSupported() {
params.Platform = "linux"
}

if params.Config.Image != "" {
img, err = daemon.GetImage(params.Config.Image)
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion daemon/start.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ func (daemon *Daemon) ContainerStart(name string, hostConfig *containertypes.Hos

// check if hostConfig is in line with the current system settings.
// It may happen cgroups are umounted or the like.
if _, err = daemon.verifyContainerSettings(container.HostConfig, nil, false); err != nil {
if _, err = daemon.verifyContainerSettings(container.Platform, container.HostConfig, nil, false); err != nil {
return validationError{err}
}
// Adapt for old containers in case we have updates in this function and
Expand Down
7 changes: 6 additions & 1 deletion daemon/update.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,12 @@ import (
func (daemon *Daemon) ContainerUpdate(name string, hostConfig *container.HostConfig) (container.ContainerUpdateOKBody, error) {
var warnings []string

warnings, err := daemon.verifyContainerSettings(hostConfig, nil, true)
c, err := daemon.GetContainer(name)
if err != nil {
return container.ContainerUpdateOKBody{Warnings: warnings}, err
}

warnings, err = daemon.verifyContainerSettings(c.Platform, hostConfig, nil, true)
if err != nil {
return container.ContainerUpdateOKBody{Warnings: warnings}, validationError{err}
}
Expand Down