From e45a006d619eeb99e3997be2117077b0c82f2ae8 Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Fri, 10 Nov 2017 16:30:08 -0800 Subject: [PATCH 1/7] clearly state that url is wrong at validation stage of build --- common/config.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/common/config.go b/common/config.go index cc7eced6a..02b214902 100644 --- a/common/config.go +++ b/common/config.go @@ -131,5 +131,12 @@ func DownloadableURL(original string) (string, error) { return "", fmt.Errorf("Unsupported URL scheme: %s", url.Scheme) } + // if cleaned filepath does not exist, error out now. + if url.Scheme == "file" { + if _, err := os.Stat(url.Path); err != nil { + return "", fmt.Errorf("The filepath doesn't exist either as a "+ + "relative or an absolute path: %s", err) + } + } return url.String(), nil } From 3a9dfb5b1897bc3fd795ca1e837bd7dd419221eb Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Fri, 10 Nov 2017 16:45:06 -0800 Subject: [PATCH 2/7] better --- common/config.go | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-) diff --git a/common/config.go b/common/config.go index 02b214902..572f2afa4 100644 --- a/common/config.go +++ b/common/config.go @@ -96,6 +96,8 @@ func DownloadableURL(original string) (string, error) { } url.Path = filepath.Clean(url.Path) + } else { + return "", err } if runtime.GOOS == "windows" { @@ -109,14 +111,6 @@ func DownloadableURL(original string) (string, error) { // Make sure it is lowercased url.Scheme = strings.ToLower(url.Scheme) - // This is to work around issue #5927. This can safely be removed once - // we distribute with a version of Go that fixes that bug. - // - // See: https://code.google.com/p/go/issues/detail?id=5927 - if url.Path != "" && url.Path[0] != '/' { - url.Path = "/" + url.Path - } - // Verify that the scheme is something we support in our common downloader. supported := []string{"file", "http", "https"} found := false @@ -131,12 +125,5 @@ func DownloadableURL(original string) (string, error) { return "", fmt.Errorf("Unsupported URL scheme: %s", url.Scheme) } - // if cleaned filepath does not exist, error out now. - if url.Scheme == "file" { - if _, err := os.Stat(url.Path); err != nil { - return "", fmt.Errorf("The filepath doesn't exist either as a "+ - "relative or an absolute path: %s", err) - } - } return url.String(), nil } From 0efcb1bba21ec8fe09a081f72076e33d186c5da0 Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Mon, 13 Nov 2017 10:53:17 -0800 Subject: [PATCH 3/7] dont error in the downloadableURL function; save validation for preflight steps --- common/config.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/common/config.go b/common/config.go index 572f2afa4..490553388 100644 --- a/common/config.go +++ b/common/config.go @@ -96,8 +96,6 @@ func DownloadableURL(original string) (string, error) { } url.Path = filepath.Clean(url.Path) - } else { - return "", err } if runtime.GOOS == "windows" { From 0d18de29428094689e3909ce21fd62b4e51ec06d Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Mon, 13 Nov 2017 11:44:59 -0800 Subject: [PATCH 4/7] do validation in vmx config stage --- builder/virtualbox/ovf/config.go | 8 ++++++++ builder/virtualbox/ovf/config_test.go | 6 +++--- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/builder/virtualbox/ovf/config.go b/builder/virtualbox/ovf/config.go index 77b7ffa0e..ff8080dd7 100644 --- a/builder/virtualbox/ovf/config.go +++ b/builder/virtualbox/ovf/config.go @@ -2,6 +2,7 @@ package ovf import ( "fmt" + "os" "strings" vboxcommon "github.com/hashicorp/packer/builder/virtualbox/common" @@ -101,6 +102,13 @@ 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. + if (len(c.SourcePath) > 7) && (c.SourcePath[:7] == "file://") { + if _, err := os.Stat(c.SourcePath[7:]); err != nil { + errs = packer.MultiErrorAppend(errs, + fmt.Errorf("source file needs to exist at time of config validation: %s", err)) + } + } } validMode := false diff --git a/builder/virtualbox/ovf/config_test.go b/builder/virtualbox/ovf/config_test.go index 980c4d4ef..bdab9b763 100644 --- a/builder/virtualbox/ovf/config_test.go +++ b/builder/virtualbox/ovf/config_test.go @@ -65,14 +65,14 @@ func TestNewConfig_sourcePath(t *testing.T) { t.Fatalf("should error with empty `source_path`") } - // Okay, because it gets caught during download + // Want this to fail on validation c = testConfig(t) c["source_path"] = "/i/dont/exist" _, warns, err = NewConfig(c) if len(warns) > 0 { t.Fatalf("bad: %#v", warns) } - if err != nil { + if err == nil { t.Fatalf("bad: %s", err) } @@ -84,7 +84,7 @@ func TestNewConfig_sourcePath(t *testing.T) { t.Fatalf("bad: %#v", warns) } if err == nil { - t.Fatal("should error") + t.Fatal("Nonexistent source file should be caught in validation") } // Good From 764be03876808d20f4b45a3bcf6c9063160b2d02 Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Mon, 13 Nov 2017 12:42:20 -0800 Subject: [PATCH 5/7] didn't mean for this error message to get changed --- builder/virtualbox/ovf/config_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builder/virtualbox/ovf/config_test.go b/builder/virtualbox/ovf/config_test.go index bdab9b763..0724968d3 100644 --- a/builder/virtualbox/ovf/config_test.go +++ b/builder/virtualbox/ovf/config_test.go @@ -84,7 +84,7 @@ func TestNewConfig_sourcePath(t *testing.T) { t.Fatalf("bad: %#v", warns) } if err == nil { - t.Fatal("Nonexistent source file should be caught in validation") + t.Fatal("should error") } // Good From 771349e58c9a350463678e76a63a46bb2979bc70 Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Mon, 13 Nov 2017 12:52:47 -0800 Subject: [PATCH 6/7] fix error message --- builder/virtualbox/ovf/config_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builder/virtualbox/ovf/config_test.go b/builder/virtualbox/ovf/config_test.go index 0724968d3..5e67b242d 100644 --- a/builder/virtualbox/ovf/config_test.go +++ b/builder/virtualbox/ovf/config_test.go @@ -73,7 +73,7 @@ func TestNewConfig_sourcePath(t *testing.T) { t.Fatalf("bad: %#v", warns) } if err == nil { - t.Fatalf("bad: %s", err) + t.Fatalf("Nonexistant file should throw a validation error!") } // Bad From 6756df9510fcd49793ef576e5fd61760b6069981 Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Mon, 13 Nov 2017 12:57:53 -0800 Subject: [PATCH 7/7] use url library instead of parsing string naiively --- builder/virtualbox/ovf/config.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/builder/virtualbox/ovf/config.go b/builder/virtualbox/ovf/config.go index ff8080dd7..3e4b9b879 100644 --- a/builder/virtualbox/ovf/config.go +++ b/builder/virtualbox/ovf/config.go @@ -2,6 +2,7 @@ package ovf import ( "fmt" + "net/url" "os" "strings" @@ -103,8 +104,9 @@ func NewConfig(raws ...interface{}) (*Config, []string, error) { errs = packer.MultiErrorAppend(errs, fmt.Errorf("source_path is invalid: %s", err)) } // file must exist now. - if (len(c.SourcePath) > 7) && (c.SourcePath[:7] == "file://") { - if _, err := os.Stat(c.SourcePath[7:]); err != nil { + 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)) }