From 54bd057bb90e40c3e5c5a5b40002f9e11b2e8520 Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Wed, 3 Jan 2018 14:34:11 -0800 Subject: [PATCH 01/11] fix nasty edge case where we can't find guest additions on windows if they are on a different drive --- common/config.go | 6 ------ common/download.go | 6 ++++++ 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/common/config.go b/common/config.go index 49055338849..4ee68f9702b 100644 --- a/common/config.go +++ b/common/config.go @@ -74,12 +74,6 @@ func DownloadableURL(original string) (string, error) { url.Path = url.Host url.Host = "" } - - // For Windows absolute file paths, remove leading / prior to processing - // since net/url turns "C:/" into "/C:/" - if len(url.Path) > 0 && url.Path[0] == '/' { - url.Path = url.Path[1:len(url.Path)] - } } // Only do the filepath transformations if the file appears diff --git a/common/download.go b/common/download.go index 8eaf236fcdf..ada4c97561d 100644 --- a/common/download.go +++ b/common/download.go @@ -15,6 +15,7 @@ import ( "net/http" "net/url" "os" + "runtime" ) // DownloadConfig is the configuration given to instantiate a new @@ -129,6 +130,11 @@ func (d *DownloadClient) Get() (string, error) { // locally and we don't make a copy. Normally we would copy or download. log.Printf("[DEBUG] Using local file: %s", finalPath) + // Remove forward slash on absolute Windows file URLs before processing + if runtime.GOOS == "windows" && len(finalPath) > 0 && finalPath[0] == '/' { + finalPath = finalPath[1:] + } + // Keep track of the source so we can make sure not to delete this later sourcePath = finalPath if _, err = os.Stat(finalPath); err != nil { From 4f3b4708046a2e299547bdd41dc077d57e514e4d Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Wed, 3 Jan 2018 16:53:47 -0800 Subject: [PATCH 02/11] add helper function to manage validation of filepaths created using DownloadableURL --- builder/virtualbox/ovf/config.go | 14 +++++-------- common/config.go | 34 ++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 9 deletions(-) diff --git a/builder/virtualbox/ovf/config.go b/builder/virtualbox/ovf/config.go index 3e4b9b87936..9c6d5dd0635 100644 --- a/builder/virtualbox/ovf/config.go +++ b/builder/virtualbox/ovf/config.go @@ -2,8 +2,6 @@ package ovf import ( "fmt" - "net/url" - "os" "strings" vboxcommon "github.com/hashicorp/packer/builder/virtualbox/common" @@ -103,14 +101,12 @@ func NewConfig(raws ...interface{}) (*Config, []string, error) { if err != nil { errs = packer.MultiErrorAppend(errs, fmt.Errorf("source_path is invalid: %s", err)) } - // file must exist now. - fileURL, _ := url.Parse(c.SourcePath) - if fileURL.Scheme == "file" { - if _, err := os.Stat(fileURL.Path); err != nil { - errs = packer.MultiErrorAppend(errs, - fmt.Errorf("source file needs to exist at time of config validation: %s", err)) - } + fileExists, err := common.FileExistsLocally(c.SourcePath) + if err != nil { + packer.MultiErrorAppend(errs, + fmt.Errorf("Source file needs to exist at time of config validation: %s", err)) } + } validMode := false diff --git a/common/config.go b/common/config.go index 4ee68f9702b..c9c8f9e7a49 100644 --- a/common/config.go +++ b/common/config.go @@ -119,3 +119,37 @@ func DownloadableURL(original string) (string, error) { return url.String(), nil } + +// FileExistsLocally takes the URL output from DownloadableURL, and determines +// whether it is present on the file system. +// example usage: +// +// myFile, err = common.DownloadableURL(c.SourcePath) +// ... +// fileExists, err := common.StatURL(myFile) +// possible output: +// true, nil -- should occur if the file is present +// false, nil -- should occur if the file is not present, but is not supposed to +// be (e.g. the schema is http://, not file://) +// true, error -- shouldn't occur ever +// false, error -- should occur if there was an error stating the file, so the +// file is not present when it should be. + +func FileExistsLocally(original string) (bool, error) { + fileURL, _ := url.Parse(original) + fileExists := false + err := nil + + if fileURL.Scheme == "file" { + // Remove forward slash on absolute Windows file URLs before processing + if runtime.GOOS == "windows" && len(fileURL.Path) > 0 && fileURL.Path[0] == '/' { + filePath := fileURL.Path[1:] + } + if _, err := os.Stat(filePath); err != nil { + err = fmt.Errorf("source file needs to exist at time of config validation: %s", err) + } else { + fileExists = true + } + } + return fileExists, err +} From 216c44b153e92752580a682073881b65238c6ad6 Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Thu, 4 Jan 2018 10:51:59 -0800 Subject: [PATCH 03/11] fix FileExistsLocally --- builder/virtualbox/ovf/config.go | 2 +- common/config.go | 26 ++++++++++++++------------ 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/builder/virtualbox/ovf/config.go b/builder/virtualbox/ovf/config.go index 9c6d5dd0635..ab8eccc41d2 100644 --- a/builder/virtualbox/ovf/config.go +++ b/builder/virtualbox/ovf/config.go @@ -101,7 +101,7 @@ func NewConfig(raws ...interface{}) (*Config, []string, error) { if err != nil { errs = packer.MultiErrorAppend(errs, fmt.Errorf("source_path is invalid: %s", err)) } - fileExists, err := common.FileExistsLocally(c.SourcePath) + _, err := common.FileExistsLocally(c.SourcePath) if err != nil { packer.MultiErrorAppend(errs, fmt.Errorf("Source file needs to exist at time of config validation: %s", err)) diff --git a/common/config.go b/common/config.go index c9c8f9e7a49..7f05f7f60a7 100644 --- a/common/config.go +++ b/common/config.go @@ -47,6 +47,7 @@ func ChooseString(vals ...string) string { // a completely valid URL. For example, the original URL might be "local/file.iso" // which isn't a valid URL. DownloadableURL will return "file:///local/file.iso" func DownloadableURL(original string) (string, error) { + fmt.Printf("Swampy: user input was %s\n", original) if runtime.GOOS == "windows" { // If the distance to the first ":" is just one character, assume // we're dealing with a drive letter and thus a file path. @@ -89,7 +90,7 @@ func DownloadableURL(original string) (string, error) { return "", err } - url.Path = filepath.Clean(url.Path) + // url.Path = filepath.Clean(url.Path) } if runtime.GOOS == "windows" { @@ -116,7 +117,7 @@ func DownloadableURL(original string) (string, error) { if !found { return "", fmt.Errorf("Unsupported URL scheme: %s", url.Scheme) } - + fmt.Printf("Swampy: parsed string after DownloadableURL is %s\n", url.String()) return url.String(), nil } @@ -136,20 +137,21 @@ func DownloadableURL(original string) (string, error) { // file is not present when it should be. func FileExistsLocally(original string) (bool, error) { - fileURL, _ := url.Parse(original) - fileExists := false - err := nil + // original should be something like file://C:/my/path.iso + // on windows, c drive will be parsed as host if it's file://c instead of file:///c + prefix = "file://" + filePath = strings.Replace(original, prefix, "", 1) + fmt.Printf("Swampy: original is %s\n", original) + fmt.Printf("Swampy: filePath is %#v\n", filePath) if fileURL.Scheme == "file" { - // Remove forward slash on absolute Windows file URLs before processing - if runtime.GOOS == "windows" && len(fileURL.Path) > 0 && fileURL.Path[0] == '/' { - filePath := fileURL.Path[1:] - } - if _, err := os.Stat(filePath); err != nil { - err = fmt.Errorf("source file needs to exist at time of config validation: %s", err) + _, err := os.Stat(filePath) + if err != nil { + err = fmt.Errorf("could not stat file: %s\n", err) + return fileExists, err } else { fileExists = true } } - return fileExists, err + return fileExists, nil } From 2838a2371dc98237f506da305af1b98d407149a8 Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Tue, 9 Jan 2018 14:27:09 -0800 Subject: [PATCH 04/11] disambiguate url variable from url library --- common/config.go | 65 ++++++++++++++++++++++++++++++------------------ 1 file changed, 41 insertions(+), 24 deletions(-) diff --git a/common/config.go b/common/config.go index 7f05f7f60a7..3f618e58a46 100644 --- a/common/config.go +++ b/common/config.go @@ -47,78 +47,87 @@ func ChooseString(vals ...string) string { // a completely valid URL. For example, the original URL might be "local/file.iso" // which isn't a valid URL. DownloadableURL will return "file:///local/file.iso" func DownloadableURL(original string) (string, error) { - fmt.Printf("Swampy: user input was %s\n", original) if runtime.GOOS == "windows" { // If the distance to the first ":" is just one character, assume // we're dealing with a drive letter and thus a file path. + // prepend with "file:///"" now so that url.Parse won't accidentally + // parse the drive letter into the url scheme. + // See https://blogs.msdn.microsoft.com/ie/2006/12/06/file-uris-in-windows/ + // for more info about valid windows URIs idx := strings.Index(original, ":") if idx == 1 { original = "file:///" + original } } - - url, err := url.Parse(original) + u, err := url.Parse(original) if err != nil { return "", err } - if url.Scheme == "" { - url.Scheme = "file" + if u.Scheme == "" { + u.Scheme = "file" } - if url.Scheme == "file" { + if u.Scheme == "file" { // Windows file handling is all sorts of tricky... if runtime.GOOS == "windows" { // If the path is using Windows-style slashes, URL parses // it into the host field. - if url.Path == "" && strings.Contains(url.Host, `\`) { - url.Path = url.Host - url.Host = "" + if u.Path == "" && strings.Contains(u.Host, `\`) { + u.Path = u.Host + u.Host = "" } } // Only do the filepath transformations if the file appears // to actually exist. - if _, err := os.Stat(url.Path); err == nil { - url.Path, err = filepath.Abs(url.Path) + if _, err := os.Stat(u.Path); err == nil { + u.Path, err = filepath.Abs(u.Path) if err != nil { return "", err } - url.Path, err = filepath.EvalSymlinks(url.Path) + u.Path, err = filepath.EvalSymlinks(u.Path) if err != nil { return "", err } - // url.Path = filepath.Clean(url.Path) + u.Path = filepath.Clean(u.Path) } if runtime.GOOS == "windows" { // Also replace all backslashes with forwardslashes since Windows // users are likely to do this but the URL should actually only // contain forward slashes. - url.Path = strings.Replace(url.Path, `\`, `/`, -1) + u.Path = strings.Replace(u.Path, `\`, `/`, -1) + // prepend absolute windows paths with "/" so that when we + // compose u.String() below the outcome will be correct + // file:///c/blah syntax; otherwise u.String() will only add + // file:// which is not technically a correct windows URI + if filepath.IsAbs(u.Path) && !strings.HasPrefix(u.Path, "/") { + u.Path = "/" + u.Path + } + } } // Make sure it is lowercased - url.Scheme = strings.ToLower(url.Scheme) + u.Scheme = strings.ToLower(u.Scheme) // Verify that the scheme is something we support in our common downloader. supported := []string{"file", "http", "https"} found := false for _, s := range supported { - if url.Scheme == s { + if u.Scheme == s { found = true break } } if !found { - return "", fmt.Errorf("Unsupported URL scheme: %s", url.Scheme) + return "", fmt.Errorf("Unsupported URL scheme: %s", u.Scheme) } - fmt.Printf("Swampy: parsed string after DownloadableURL is %s\n", url.String()) - return url.String(), nil + return u.String(), nil } // FileExistsLocally takes the URL output from DownloadableURL, and determines @@ -138,13 +147,21 @@ func DownloadableURL(original string) (string, error) { func FileExistsLocally(original string) (bool, error) { // original should be something like file://C:/my/path.iso - // on windows, c drive will be parsed as host if it's file://c instead of file:///c - prefix = "file://" - filePath = strings.Replace(original, prefix, "", 1) - fmt.Printf("Swampy: original is %s\n", original) - fmt.Printf("Swampy: filePath is %#v\n", filePath) + + fileURL, _ := url.Parse(original) + fileExists := false if fileURL.Scheme == "file" { + // on windows, correct URI is file:///c:/blah/blah.iso. + // url.Parse will pull out the scheme "file://" and leave the path as + // "/c:/blah/blah/iso". Here we remove this forward slash on absolute + // Windows file URLs before processing + // see https://blogs.msdn.microsoft.com/ie/2006/12/06/file-uris-in-windows/ + // for more info about valid windows URIs + filePath := fileURL.Path + if runtime.GOOS == "windows" && len(filePath) > 0 && filePath[0] == '/' { + filePath = filePath[1:] + } _, err := os.Stat(filePath) if err != nil { err = fmt.Errorf("could not stat file: %s\n", err) From 40f0cc6dfe00b8da21db8de73ea285134ded6f17 Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Tue, 9 Jan 2018 15:53:54 -0800 Subject: [PATCH 05/11] I don't think this is needed anymore --- common/config.go | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/common/config.go b/common/config.go index 3f618e58a46..5ef4d336954 100644 --- a/common/config.go +++ b/common/config.go @@ -69,16 +69,6 @@ func DownloadableURL(original string) (string, error) { } if u.Scheme == "file" { - // Windows file handling is all sorts of tricky... - if runtime.GOOS == "windows" { - // If the path is using Windows-style slashes, URL parses - // it into the host field. - if u.Path == "" && strings.Contains(u.Host, `\`) { - u.Path = u.Host - u.Host = "" - } - } - // Only do the filepath transformations if the file appears // to actually exist. if _, err := os.Stat(u.Path); err == nil { From 154973241f65913b843544de044df153a23eab7e Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Tue, 9 Jan 2018 16:11:29 -0800 Subject: [PATCH 06/11] add a bunch of windows filepath tests --- common/config_test.go | 61 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/common/config_test.go b/common/config_test.go index 8bd68473919..1a39207330b 100644 --- a/common/config_test.go +++ b/common/config_test.go @@ -71,6 +71,67 @@ func TestDownloadableURL(t *testing.T) { } } +func TestDownloadableURL_WindowsFiles(t *testing.T) { + if runtime.GOOS == "windows" { + dirCases := []struct { + InputString string + OutputURL string + ErrExpected bool + }{ // TODO: add different directories + { + "C:\\Temp\\SomeDir\\myfile.txt", + "file:///C:/Temp/SomeDir/myfile.txt", + false, + }, + { // need windows drive + "\\Temp\\SomeDir\\myfile.txt", + "", + true, + }, + { // need windows drive + "/Temp/SomeDir/myfile.txt", + "", + true, + }, + { + "file:///C:\\Temp\\SomeDir\\myfile.txt", + "file:///c:/Temp/SomeDir/myfile.txt", + false, + }, + { + "file:///c:/Temp/Somedir/myfile.txt", + "file:///c:/Temp/SomeDir/myfile.txt", + false, + }, + } + // create absolute-pathed tempfile to play with + err := os.Mkdir("C:\\Temp\\SomeDir", 0755) + if err != nil { + t.Fatalf("err creating test dir: %s", err) + } + fi, err := os.Create("C:\\Temp\\SomeDir\\myfile.txt") + if err != nil { + t.Fatalf("err creating test file: %s", err) + } + fi.Close() + defer os.Remove("C:\\Temp\\SomeDir\\myfile.txt") + defer os.Remove("C:\\Temp\\SomeDir") + + // Run through test cases to make sure they all parse correctly + for _, tc := range dirCases { + u, err := DownloadableURL(tc.InputString) + if (err != nil) != tc.ErrExpected { + t.Fatalf("Test Case failed: Expected err = %#v, err = %#v, input = %s", + tc.ErrExpected, err, tc.InputString) + } + if u != tc.OutputURL { + t.Fatalf("Test Case failed: Expected %s but received %s from input %s", + tc.OutputURL, u, tc.InputString) + } + } + } +} + func TestDownloadableURL_FilePaths(t *testing.T) { tf, err := ioutil.TempFile("", "packer") if err != nil { From a04a921c2d04d88bd4b6dedd15ed7e1ad119ca09 Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Tue, 9 Jan 2018 17:14:32 -0800 Subject: [PATCH 07/11] add UNC path to test cases, so I can try to enable it in future --- common/config_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/common/config_test.go b/common/config_test.go index 1a39207330b..ad577b3b0b8 100644 --- a/common/config_test.go +++ b/common/config_test.go @@ -93,6 +93,11 @@ func TestDownloadableURL_WindowsFiles(t *testing.T) { "", true, }, + { // UNC paths; why not? + "\\\\?\\c:\\Temp\\SomeDir\\myfile.txt", + "", + true, + }, { "file:///C:\\Temp\\SomeDir\\myfile.txt", "file:///c:/Temp/SomeDir/myfile.txt", From 898dadd53c3011e05764e32f2936ae6e95f3e8b3 Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Wed, 10 Jan 2018 10:03:36 -0800 Subject: [PATCH 08/11] re-add this block. I still don't think we need it but I don't want to risk breaking things with this bugfix. --- common/config.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/common/config.go b/common/config.go index 5ef4d336954..f1666058c26 100644 --- a/common/config.go +++ b/common/config.go @@ -69,6 +69,15 @@ func DownloadableURL(original string) (string, error) { } if u.Scheme == "file" { + // Windows file handling is all sorts of tricky... + if runtime.GOOS == "windows" { + // If the path is using Windows-style slashes, URL parses + // it into the host field. + if url.Path == "" && strings.Contains(url.Host, `\`) { + url.Path = url.Host + url.Host = "" + } + } // Only do the filepath transformations if the file appears // to actually exist. if _, err := os.Stat(u.Path); err == nil { From 55ddbf476574fcef8344f2cafc6703e3673c158a Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Wed, 10 Jan 2018 10:08:23 -0800 Subject: [PATCH 09/11] sloppy copypasta --- common/config.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/common/config.go b/common/config.go index f1666058c26..2b0de0c5c6b 100644 --- a/common/config.go +++ b/common/config.go @@ -73,9 +73,9 @@ func DownloadableURL(original string) (string, error) { if runtime.GOOS == "windows" { // If the path is using Windows-style slashes, URL parses // it into the host field. - if url.Path == "" && strings.Contains(url.Host, `\`) { - url.Path = url.Host - url.Host = "" + if u.Path == "" && strings.Contains(u.Host, `\`) { + u.Path = u.Host + u.Host = "" } } // Only do the filepath transformations if the file appears From 3ace5bb91becc645dd8acfa0fca0199e8ceed115 Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Wed, 10 Jan 2018 16:11:17 -0800 Subject: [PATCH 10/11] simplify FileExistsLocally --- builder/virtualbox/ovf/config.go | 6 +++--- common/config.go | 17 +++++++---------- 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/builder/virtualbox/ovf/config.go b/builder/virtualbox/ovf/config.go index ab8eccc41d2..41a013ebdd7 100644 --- a/builder/virtualbox/ovf/config.go +++ b/builder/virtualbox/ovf/config.go @@ -101,10 +101,10 @@ func NewConfig(raws ...interface{}) (*Config, []string, error) { if err != nil { errs = packer.MultiErrorAppend(errs, fmt.Errorf("source_path is invalid: %s", err)) } - _, err := common.FileExistsLocally(c.SourcePath) - if err != nil { + fileOK := common.FileExistsLocally(c.SourcePath) + if !fileOK { packer.MultiErrorAppend(errs, - fmt.Errorf("Source file needs to exist at time of config validation: %s", err)) + fmt.Errorf("Source file needs to exist at time of config validation!")) } } diff --git a/common/config.go b/common/config.go index 2b0de0c5c6b..7bef253eedc 100644 --- a/common/config.go +++ b/common/config.go @@ -135,16 +135,14 @@ func DownloadableURL(original string) (string, error) { // // myFile, err = common.DownloadableURL(c.SourcePath) // ... -// fileExists, err := common.StatURL(myFile) +// fileExists := common.StatURL(myFile) // possible output: -// true, nil -- should occur if the file is present -// false, nil -- should occur if the file is not present, but is not supposed to -// be (e.g. the schema is http://, not file://) -// true, error -- shouldn't occur ever -// false, error -- should occur if there was an error stating the file, so the +// true -- should occur if the file is present, or if the file is not present, +// but is not supposed to be (e.g. the schema is http://, not file://) +// false -- should occur if there was an error stating the file, so the // file is not present when it should be. -func FileExistsLocally(original string) (bool, error) { +func FileExistsLocally(original string) bool { // original should be something like file://C:/my/path.iso fileURL, _ := url.Parse(original) @@ -163,11 +161,10 @@ func FileExistsLocally(original string) (bool, error) { } _, err := os.Stat(filePath) if err != nil { - err = fmt.Errorf("could not stat file: %s\n", err) - return fileExists, err + return fileExists } else { fileExists = true } } - return fileExists, nil + return fileExists } From bdd186fa2b2b0465f507a6f05de10a13ce691ef6 Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Wed, 10 Jan 2018 16:44:27 -0800 Subject: [PATCH 11/11] add tests for fileexistslocally helper function --- common/config_test.go | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/common/config_test.go b/common/config_test.go index ad577b3b0b8..0320b86fe9b 100644 --- a/common/config_test.go +++ b/common/config_test.go @@ -208,6 +208,43 @@ func TestDownloadableURL_FilePaths(t *testing.T) { } } +func test_FileExistsLocally(t *testing.T) { + if runtime.GOOS == "windows" { + dirCases := []struct { + Input string + Output bool + }{ + // file exists locally + {"file:///C:/Temp/SomeDir/myfile.txt", true}, + // file is not supposed to exist locally + {"https://myfile.iso", true}, + // file does not exist locally + {"file:///C/i/dont/exist", false}, + } + // create absolute-pathed tempfile to play with + err := os.Mkdir("C:\\Temp\\SomeDir", 0755) + if err != nil { + t.Fatalf("err creating test dir: %s", err) + } + fi, err := os.Create("C:\\Temp\\SomeDir\\myfile.txt") + if err != nil { + t.Fatalf("err creating test file: %s", err) + } + fi.Close() + defer os.Remove("C:\\Temp\\SomeDir\\myfile.txt") + defer os.Remove("C:\\Temp\\SomeDir") + + // Run through test cases to make sure they all parse correctly + for _, tc := range dirCases { + fileOK := FileExistsLocally(tc.Input) + if !fileOK { + t.Fatalf("Test Case failed: Expected %#v, received = %#v, input = %s", + tc.Output, fileOK, tc.Input) + } + } + } +} + func TestScrubConfig(t *testing.T) { type Inner struct { Baz string