From 8e87ccb6898c05497371752518cc2ef09196a5b4 Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Tue, 7 Jul 2020 14:36:04 -0400 Subject: [PATCH 1/4] providercache: Lazily detect executable file Instead of searching the installed provider package directory for a binary as we install it, we can lazily detect the executable as it is required. Doing so allows us to separately report an invalid unpacked package, giving the user more actionable error messages. --- command/init_test.go | 119 ++++++++---------- command/meta_providers.go | 7 +- internal/providercache/cached_provider.go | 82 ++++++++++-- .../providercache/cached_provider_test.go | 61 ++++++++- internal/providercache/dir.go | 88 +------------ internal/providercache/dir_modify_test.go | 9 +- internal/providercache/dir_test.go | 44 ++++--- .../executable/2.0.0/linux_amd64/executable | 1 + 8 files changed, 217 insertions(+), 194 deletions(-) create mode 100755 internal/providercache/testdata/cachedir/registry.terraform.io/missing/executable/2.0.0/linux_amd64/executable diff --git a/command/init_test.go b/command/init_test.go index 9fdc29ba0..a44054665 100644 --- a/command/init_test.go +++ b/command/init_test.go @@ -1060,26 +1060,23 @@ func TestInit_providerSource(t *testing.T) { wantPackages := map[addrs.Provider][]providercache.CachedProvider{ addrs.NewDefaultProvider("test"): { { - Provider: addrs.NewDefaultProvider("test"), - Version: getproviders.MustParseVersion("1.2.3"), - PackageDir: expectedPackageInstallPath("test", "1.2.3", false), - ExecutableFile: expectedPackageInstallPath("test", "1.2.3", true), + Provider: addrs.NewDefaultProvider("test"), + Version: getproviders.MustParseVersion("1.2.3"), + PackageDir: expectedPackageInstallPath("test", "1.2.3", false), }, }, addrs.NewDefaultProvider("test-beta"): { { - Provider: addrs.NewDefaultProvider("test-beta"), - Version: getproviders.MustParseVersion("1.2.4"), - PackageDir: expectedPackageInstallPath("test-beta", "1.2.4", false), - ExecutableFile: expectedPackageInstallPath("test-beta", "1.2.4", true), + Provider: addrs.NewDefaultProvider("test-beta"), + Version: getproviders.MustParseVersion("1.2.4"), + PackageDir: expectedPackageInstallPath("test-beta", "1.2.4", false), }, }, addrs.NewDefaultProvider("source"): { { - Provider: addrs.NewDefaultProvider("source"), - Version: getproviders.MustParseVersion("1.2.3"), - PackageDir: expectedPackageInstallPath("source", "1.2.3", false), - ExecutableFile: expectedPackageInstallPath("source", "1.2.3", true), + Provider: addrs.NewDefaultProvider("source"), + Version: getproviders.MustParseVersion("1.2.3"), + PackageDir: expectedPackageInstallPath("source", "1.2.3", false), }, }, } @@ -1094,22 +1091,19 @@ func TestInit_providerSource(t *testing.T) { } wantSelected := map[addrs.Provider]*providercache.CachedProvider{ addrs.NewDefaultProvider("test-beta"): { - Provider: addrs.NewDefaultProvider("test-beta"), - Version: getproviders.MustParseVersion("1.2.4"), - PackageDir: expectedPackageInstallPath("test-beta", "1.2.4", false), - ExecutableFile: expectedPackageInstallPath("test-beta", "1.2.4", true), + Provider: addrs.NewDefaultProvider("test-beta"), + Version: getproviders.MustParseVersion("1.2.4"), + PackageDir: expectedPackageInstallPath("test-beta", "1.2.4", false), }, addrs.NewDefaultProvider("test"): { - Provider: addrs.NewDefaultProvider("test"), - Version: getproviders.MustParseVersion("1.2.3"), - PackageDir: expectedPackageInstallPath("test", "1.2.3", false), - ExecutableFile: expectedPackageInstallPath("test", "1.2.3", true), + Provider: addrs.NewDefaultProvider("test"), + Version: getproviders.MustParseVersion("1.2.3"), + PackageDir: expectedPackageInstallPath("test", "1.2.3", false), }, addrs.NewDefaultProvider("source"): { - Provider: addrs.NewDefaultProvider("source"), - Version: getproviders.MustParseVersion("1.2.3"), - PackageDir: expectedPackageInstallPath("source", "1.2.3", false), - ExecutableFile: expectedPackageInstallPath("source", "1.2.3", true), + Provider: addrs.NewDefaultProvider("source"), + Version: getproviders.MustParseVersion("1.2.3"), + PackageDir: expectedPackageInstallPath("source", "1.2.3", false), }, } if diff := cmp.Diff(wantSelected, gotSelected); diff != "" { @@ -1169,27 +1163,24 @@ func TestInit_getUpgradePlugins(t *testing.T) { // the newest available version that matched the version constraints. addrs.NewDefaultProvider("between"): { { - Provider: addrs.NewDefaultProvider("between"), - Version: getproviders.MustParseVersion("2.3.4"), - PackageDir: expectedPackageInstallPath("between", "2.3.4", false), - ExecutableFile: expectedPackageInstallPath("between", "2.3.4", true), + Provider: addrs.NewDefaultProvider("between"), + Version: getproviders.MustParseVersion("2.3.4"), + PackageDir: expectedPackageInstallPath("between", "2.3.4", false), }, }, // The existing version of "exact" did not match the version constraints, // so we installed what the configuration selected as well. addrs.NewDefaultProvider("exact"): { { - Provider: addrs.NewDefaultProvider("exact"), - Version: getproviders.MustParseVersion("1.2.3"), - PackageDir: expectedPackageInstallPath("exact", "1.2.3", false), - ExecutableFile: expectedPackageInstallPath("exact", "1.2.3", true), + Provider: addrs.NewDefaultProvider("exact"), + Version: getproviders.MustParseVersion("1.2.3"), + PackageDir: expectedPackageInstallPath("exact", "1.2.3", false), }, // Previous version is still there, but not selected { - Provider: addrs.NewDefaultProvider("exact"), - Version: getproviders.MustParseVersion("0.0.1"), - PackageDir: expectedPackageInstallPath("exact", "0.0.1", false), - ExecutableFile: expectedPackageInstallPath("exact", "0.0.1", true), + Provider: addrs.NewDefaultProvider("exact"), + Version: getproviders.MustParseVersion("0.0.1"), + PackageDir: expectedPackageInstallPath("exact", "0.0.1", false), }, }, // The existing version of "greater-than" _did_ match the constraints, @@ -1197,17 +1188,15 @@ func TestInit_getUpgradePlugins(t *testing.T) { // -upgrade and so we upgraded it anyway. addrs.NewDefaultProvider("greater-than"): { { - Provider: addrs.NewDefaultProvider("greater-than"), - Version: getproviders.MustParseVersion("2.3.4"), - PackageDir: expectedPackageInstallPath("greater-than", "2.3.4", false), - ExecutableFile: expectedPackageInstallPath("greater-than", "2.3.4", true), + Provider: addrs.NewDefaultProvider("greater-than"), + Version: getproviders.MustParseVersion("2.3.4"), + PackageDir: expectedPackageInstallPath("greater-than", "2.3.4", false), }, // Previous version is still there, but not selected { - Provider: addrs.NewDefaultProvider("greater-than"), - Version: getproviders.MustParseVersion("2.3.3"), - PackageDir: expectedPackageInstallPath("greater-than", "2.3.3", false), - ExecutableFile: expectedPackageInstallPath("greater-than", "2.3.3", true), + Provider: addrs.NewDefaultProvider("greater-than"), + Version: getproviders.MustParseVersion("2.3.3"), + PackageDir: expectedPackageInstallPath("greater-than", "2.3.3", false), }, }, } @@ -1222,22 +1211,19 @@ func TestInit_getUpgradePlugins(t *testing.T) { } wantSelected := map[addrs.Provider]*providercache.CachedProvider{ addrs.NewDefaultProvider("between"): { - Provider: addrs.NewDefaultProvider("between"), - Version: getproviders.MustParseVersion("2.3.4"), - PackageDir: expectedPackageInstallPath("between", "2.3.4", false), - ExecutableFile: expectedPackageInstallPath("between", "2.3.4", true), + Provider: addrs.NewDefaultProvider("between"), + Version: getproviders.MustParseVersion("2.3.4"), + PackageDir: expectedPackageInstallPath("between", "2.3.4", false), }, addrs.NewDefaultProvider("exact"): { - Provider: addrs.NewDefaultProvider("exact"), - Version: getproviders.MustParseVersion("1.2.3"), - PackageDir: expectedPackageInstallPath("exact", "1.2.3", false), - ExecutableFile: expectedPackageInstallPath("exact", "1.2.3", true), + Provider: addrs.NewDefaultProvider("exact"), + Version: getproviders.MustParseVersion("1.2.3"), + PackageDir: expectedPackageInstallPath("exact", "1.2.3", false), }, addrs.NewDefaultProvider("greater-than"): { - Provider: addrs.NewDefaultProvider("greater-than"), - Version: getproviders.MustParseVersion("2.3.4"), - PackageDir: expectedPackageInstallPath("greater-than", "2.3.4", false), - ExecutableFile: expectedPackageInstallPath("greater-than", "2.3.4", true), + Provider: addrs.NewDefaultProvider("greater-than"), + Version: getproviders.MustParseVersion("2.3.4"), + PackageDir: expectedPackageInstallPath("greater-than", "2.3.4", false), }, } if diff := cmp.Diff(wantSelected, gotSelected); diff != "" { @@ -1480,22 +1466,19 @@ func TestInit_pluginDirProviders(t *testing.T) { } wantSelected := map[addrs.Provider]*providercache.CachedProvider{ addrs.NewDefaultProvider("between"): { - Provider: addrs.NewDefaultProvider("between"), - Version: getproviders.MustParseVersion("2.3.4"), - PackageDir: expectedPackageInstallPath("between", "2.3.4", false), - ExecutableFile: expectedPackageInstallPath("between", "2.3.4", true), + Provider: addrs.NewDefaultProvider("between"), + Version: getproviders.MustParseVersion("2.3.4"), + PackageDir: expectedPackageInstallPath("between", "2.3.4", false), }, addrs.NewDefaultProvider("exact"): { - Provider: addrs.NewDefaultProvider("exact"), - Version: getproviders.MustParseVersion("1.2.3"), - PackageDir: expectedPackageInstallPath("exact", "1.2.3", false), - ExecutableFile: expectedPackageInstallPath("exact", "1.2.3", true), + Provider: addrs.NewDefaultProvider("exact"), + Version: getproviders.MustParseVersion("1.2.3"), + PackageDir: expectedPackageInstallPath("exact", "1.2.3", false), }, addrs.NewDefaultProvider("greater-than"): { - Provider: addrs.NewDefaultProvider("greater-than"), - Version: getproviders.MustParseVersion("2.3.4"), - PackageDir: expectedPackageInstallPath("greater-than", "2.3.4", false), - ExecutableFile: expectedPackageInstallPath("greater-than", "2.3.4", true), + Provider: addrs.NewDefaultProvider("greater-than"), + Version: getproviders.MustParseVersion("2.3.4"), + PackageDir: expectedPackageInstallPath("greater-than", "2.3.4", false), }, } if diff := cmp.Diff(wantSelected, gotSelected); diff != "" { diff --git a/command/meta_providers.go b/command/meta_providers.go index 896553d2a..548868ee7 100644 --- a/command/meta_providers.go +++ b/command/meta_providers.go @@ -209,12 +209,17 @@ func providerFactory(meta *providercache.CachedProvider) providers.Factory { Output: os.Stderr, }) + execFile, err := meta.ExecutableFile() + if err != nil { + return nil, err + } + config := &plugin.ClientConfig{ HandshakeConfig: tfplugin.Handshake, Logger: logger, AllowedProtocols: []plugin.Protocol{plugin.ProtocolGRPC}, Managed: true, - Cmd: exec.Command(meta.ExecutableFile), + Cmd: exec.Command(execFile), AutoMTLS: enableProviderAutoMTLS, VersionedPlugins: tfplugin.VersionedPlugins, } diff --git a/internal/providercache/cached_provider.go b/internal/providercache/cached_provider.go index 55e8fd52d..25c864c00 100644 --- a/internal/providercache/cached_provider.go +++ b/internal/providercache/cached_provider.go @@ -1,6 +1,11 @@ package providercache import ( + "fmt" + "io/ioutil" + "path/filepath" + "strings" + "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/internal/getproviders" ) @@ -20,16 +25,6 @@ type CachedProvider struct { // both slashes and backslashes as long as the separators are consistent // within a particular path string. PackageDir string - - // ExecutableFile is the local filesystem path to the main plugin executable - // for the provider, which is always a file within the directory given - // in PackageDir. - // - // The path always uses slashes as path separators, even on Windows, so - // that the results are consistent between platforms. Windows accepts - // both slashes and backslashes as long as the separators are consistent - // within a particular path string. - ExecutableFile string } // PackageLocation returns the package directory given in the PackageDir field @@ -77,3 +72,70 @@ func (cp *CachedProvider) MatchesHash(want string) (bool, error) { func (cp *CachedProvider) HashV1() (string, error) { return getproviders.PackageHashV1(cp.PackageLocation()) } + +// ExecutableFile inspects the cached provider's unpacked package directory for +// something that looks like it's intended to be the executable file for the +// plugin. +// +// This is a bit messy and heuristic-y because historically Terraform used the +// filename itself for local filesystem discovery, allowing some variance in +// the filenames to capture extra metadata, whereas now we're using the +// directory structure leading to the executable instead but need to remain +// compatible with the executable names bundled into existing provider packages. +// +// It will return an error if it can't find a file following the expected +// convention in the given directory. +// +// If found, the path always uses slashes as path separators, even on Windows, +// so that the results are consistent between platforms. Windows accepts both +// slashes and backslashes as long as the separators are consistent within a +// particular path string. +func (cp *CachedProvider) ExecutableFile() (string, error) { + infos, err := ioutil.ReadDir(cp.PackageDir) + if err != nil { + // If the directory itself doesn't exist or isn't readable then we + // can't access an executable in it. + return "", fmt.Errorf("could not read package directory: %s", err) + } + + // For a provider named e.g. tf.example.com/awesomecorp/happycloud, we + // expect an executable file whose name starts with + // "terraform-provider-happycloud", followed by zero or more additional + // characters. If there _are_ additional characters then the first one + // must be an underscore or a period, like in thse examples: + // - terraform-provider-happycloud_v1.0.0 + // - terraform-provider-happycloud.exe + // + // We don't require the version in the filename to match because the + // executable's name is no longer authoritative, but packages of "official" + // providers may continue to use versioned executable names for backward + // compatibility with Terraform 0.12. + // + // We also presume that providers packaged for Windows will include the + // necessary .exe extension on their filenames but do not explicitly check + // for that. If there's a provider package for Windows that has a file + // without that suffix then it will be detected as an executable but then + // we'll presumably fail later trying to run it. + wantPrefix := "terraform-provider-" + cp.Provider.Type + + // We'll visit all of the directory entries and take the first (in + // name-lexical order) that looks like a plausible provider executable + // name. A package with multiple files meeting these criteria is degenerate + // but we will tolerate it by ignoring the subsequent entries. + for _, info := range infos { + if info.IsDir() { + continue // A directory can never be an executable + } + name := info.Name() + if !strings.HasPrefix(name, wantPrefix) { + continue + } + remainder := name[len(wantPrefix):] + if len(remainder) > 0 && (remainder[0] != '_' && remainder[0] != '.') { + continue // subsequent characters must be delimited by _ or . + } + return filepath.ToSlash(filepath.Join(cp.PackageDir, name)), nil + } + + return "", fmt.Errorf("could not find executable file starting with %s", wantPrefix) +} diff --git a/internal/providercache/cached_provider_test.go b/internal/providercache/cached_provider_test.go index 8d366e637..84315cbcc 100644 --- a/internal/providercache/cached_provider_test.go +++ b/internal/providercache/cached_provider_test.go @@ -15,8 +15,7 @@ func TestCachedProviderHash(t *testing.T) { ), Version: getproviders.MustParseVersion("2.0.0"), - PackageDir: "testdata/cachedir/registry.terraform.io/hashicorp/null/2.0.0/darwin_amd64", - ExecutableFile: "testdata/cachedir/registry.terraform.io/hashicorp/null/2.0.0/darwin_amd64/terraform-provider-null", + PackageDir: "testdata/cachedir/registry.terraform.io/hashicorp/null/2.0.0/darwin_amd64", } want := "h1:qjsREM4DqEWECD43FcPqddZ9oxCG+IaMTxvWPciS05g=" @@ -46,8 +45,7 @@ func TestCachedProviderHash(t *testing.T) { ), Version: getproviders.MustParseVersion("2.0.0"), - PackageDir: "testdata/cachedir/registry.terraform.io/hashicorp/null/2.0.0/windows_amd64", - ExecutableFile: "testdata/cachedir/registry.terraform.io/hashicorp/null/2.0.0/windows_amd64/terraform-provider-null", + PackageDir: "testdata/cachedir/registry.terraform.io/hashicorp/null/2.0.0/windows_amd64", } gotMatches, err = cp2.MatchesHash(want) if err != nil { @@ -58,3 +56,58 @@ func TestCachedProviderHash(t *testing.T) { } } + +func TestExecutableFile(t *testing.T) { + testCases := map[string]struct { + cp *CachedProvider + file string + err string + }{ + "linux": { + cp: &CachedProvider{ + Provider: addrs.NewProvider(addrs.DefaultRegistryHost, "hashicorp", "null"), + Version: getproviders.MustParseVersion("2.0.0"), + PackageDir: "testdata/cachedir/registry.terraform.io/hashicorp/null/2.0.0/linux_amd64", + }, + file: "testdata/cachedir/registry.terraform.io/hashicorp/null/2.0.0/linux_amd64/terraform-provider-null", + }, + "windows": { + cp: &CachedProvider{ + Provider: addrs.NewProvider(addrs.DefaultRegistryHost, "hashicorp", "null"), + Version: getproviders.MustParseVersion("2.0.0"), + PackageDir: "testdata/cachedir/registry.terraform.io/hashicorp/null/2.0.0/windows_amd64", + }, + file: "testdata/cachedir/registry.terraform.io/hashicorp/null/2.0.0/windows_amd64/terraform-provider-null.exe", + }, + "missing-executable": { + cp: &CachedProvider{ + Provider: addrs.NewProvider(addrs.DefaultRegistryHost, "missing", "executable"), + Version: getproviders.MustParseVersion("2.0.0"), + PackageDir: "testdata/cachedir/registry.terraform.io/missing/executable/2.0.0/linux_amd64", + }, + err: "could not find executable file starting with terraform-provider-executable", + }, + "missing-dir": { + cp: &CachedProvider{ + Provider: addrs.NewProvider(addrs.DefaultRegistryHost, "missing", "packagedir"), + Version: getproviders.MustParseVersion("2.0.0"), + PackageDir: "testdata/cachedir/registry.terraform.io/missing/packagedir/2.0.0/linux_amd64", + }, + err: "could not read package directory: open testdata/cachedir/registry.terraform.io/missing/packagedir/2.0.0/linux_amd64: no such file or directory", + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + file, err := tc.cp.ExecutableFile() + if file != tc.file { + t.Errorf("wrong file\n got: %q\nwant: %q", file, tc.file) + } + if err == nil && tc.err != "" { + t.Fatalf("no error returned, want: %q", tc.err) + } else if err != nil && err.Error() != tc.err { + t.Errorf("wrong error\n got: %q\nwant: %q", err, tc.err) + } + }) + } +} diff --git a/internal/providercache/dir.go b/internal/providercache/dir.go index 72688f92e..576af7ef4 100644 --- a/internal/providercache/dir.go +++ b/internal/providercache/dir.go @@ -1,11 +1,9 @@ package providercache import ( - "io/ioutil" "log" "path/filepath" "sort" - "strings" "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/internal/getproviders" @@ -165,20 +163,12 @@ func (d *Dir) fillMetaCache() error { } packageDir := filepath.Clean(string(meta.Location.(getproviders.PackageLocalDir))) - execFile := findProviderExecutableInLocalPackage(meta) - if execFile == "" { - // If the package doesn't contain a suitable executable then - // it isn't considered to be part of our cache. - log.Printf("[TRACE] providercache.fillMetaCache: ignoring %s because it is does not seem to contain a suitable plugin executable", meta.Location) - continue - } log.Printf("[TRACE] providercache.fillMetaCache: including %s as a candidate package for %s %s", meta.Location, providerAddr, meta.Version) data[providerAddr] = append(data[providerAddr], CachedProvider{ - Provider: providerAddr, - Version: meta.Version, - PackageDir: filepath.ToSlash(packageDir), - ExecutableFile: filepath.ToSlash(execFile), + Provider: providerAddr, + Version: meta.Version, + PackageDir: filepath.ToSlash(packageDir), }) } } @@ -200,75 +190,3 @@ func (d *Dir) fillMetaCache() error { d.metaCache = data return nil } - -// This is a helper function to peep into the unpacked directory associated -// with the given package meta and find something that looks like it's intended -// to be the executable file for the plugin. -// -// This is a bit messy and heuristic-y because historically Terraform used the -// filename itself for local filesystem discovery, allowing some variance in -// the filenames to capture extra metadata, whereas now we're using the -// directory structure leading to the executable instead but need to remain -// compatible with the executable names bundled into existing provider packages. -// -// It will return a zero-length string if it can't find a file following -// the expected convention in the given directory. -func findProviderExecutableInLocalPackage(meta getproviders.PackageMeta) string { - packageDir, ok := meta.Location.(getproviders.PackageLocalDir) - if !ok { - // This should never happen because the providercache package only - // uses the local unpacked directory layout. If anything else ends - // up in here then we'll indicate that no executable is available, - // because all other locations require a fetch/unpack step first. - return "" - } - - infos, err := ioutil.ReadDir(string(packageDir)) - if err != nil { - // If the directory itself doesn't exist or isn't readable then we - // can't access an executable in it. - return "" - } - - // For a provider named e.g. tf.example.com/awesomecorp/happycloud, we - // expect an executable file whose name starts with - // "terraform-provider-happycloud", followed by zero or more additional - // characters. If there _are_ additional characters then the first one - // must be an underscore or a period, like in thse examples: - // - terraform-provider-happycloud_v1.0.0 - // - terraform-provider-happycloud.exe - // - // We don't require the version in the filename to match because the - // executable's name is no longer authoritative, but packages of "official" - // providers may continue to use versioned executable names for backward - // compatibility with Terraform 0.12. - // - // We also presume that providers packaged for Windows will include the - // necessary .exe extension on their filenames but do not explicitly check - // for that. If there's a provider package for Windows that has a file - // without that suffix then it will be detected as an executable but then - // we'll presumably fail later trying to run it. - wantPrefix := "terraform-provider-" + meta.Provider.Type - - // We'll visit all of the directory entries and take the first (in - // name-lexical order) that looks like a plausible provider executable - // name. A package with multiple files meeting these criteria is degenerate - // but we will tolerate it by ignoring the subsequent entries. - for _, info := range infos { - if info.IsDir() { - continue // A directory can never be an executable - } - name := info.Name() - if !strings.HasPrefix(name, wantPrefix) { - continue - } - remainder := name[len(wantPrefix):] - if len(remainder) > 0 && (remainder[0] != '_' && remainder[0] != '.') { - continue // subsequent characters must be delimited by _ - } - return filepath.Join(string(packageDir), name) - } - - // If we fall out here then nothing has matched. - return "" -} diff --git a/internal/providercache/dir_modify_test.go b/internal/providercache/dir_modify_test.go index b55961b5b..9d1c8ce56 100644 --- a/internal/providercache/dir_modify_test.go +++ b/internal/providercache/dir_modify_test.go @@ -58,8 +58,7 @@ func TestInstallPackage(t *testing.T) { Version: versions.MustParseVersion("2.1.0"), - PackageDir: tmpDirPath + "/registry.terraform.io/hashicorp/null/2.1.0/linux_amd64", - ExecutableFile: tmpDirPath + "/registry.terraform.io/hashicorp/null/2.1.0/linux_amd64/terraform-provider-null", + PackageDir: tmpDirPath + "/registry.terraform.io/hashicorp/null/2.1.0/linux_amd64", }, }, } @@ -101,8 +100,7 @@ func TestLinkFromOtherCache(t *testing.T) { // still packed and thus not considered to be a cache member. Version: versions.MustParseVersion("2.0.0"), - PackageDir: "testdata/cachedir/registry.terraform.io/hashicorp/null/2.0.0/windows_amd64", - ExecutableFile: "testdata/cachedir/registry.terraform.io/hashicorp/null/2.0.0/windows_amd64/terraform-provider-null.exe", + PackageDir: "testdata/cachedir/registry.terraform.io/hashicorp/null/2.0.0/windows_amd64", }, }, } @@ -138,8 +136,7 @@ func TestLinkFromOtherCache(t *testing.T) { // still packed and thus not considered to be a cache member. Version: versions.MustParseVersion("2.0.0"), - PackageDir: tmpDirPath + "/registry.terraform.io/hashicorp/null/2.0.0/windows_amd64", - ExecutableFile: tmpDirPath + "/registry.terraform.io/hashicorp/null/2.0.0/windows_amd64/terraform-provider-null.exe", + PackageDir: tmpDirPath + "/registry.terraform.io/hashicorp/null/2.0.0/windows_amd64", }, }, } diff --git a/internal/providercache/dir_test.go b/internal/providercache/dir_test.go index b59d4c75a..ada5c83fd 100644 --- a/internal/providercache/dir_test.go +++ b/internal/providercache/dir_test.go @@ -37,6 +37,9 @@ func TestDirReading(t *testing.T) { addrs.DefaultRegistryHost, "bloop", "nonexist", ) legacyProvider := addrs.NewLegacyProvider("legacy") + missingExecutableProvider := addrs.NewProvider( + addrs.DefaultRegistryHost, "missing", "executable", + ) t.Run("ProviderLatestVersion", func(t *testing.T) { t.Run("exists", func(t *testing.T) { @@ -50,8 +53,7 @@ func TestDirReading(t *testing.T) { // still packed and thus not considered to be a cache member. Version: versions.MustParseVersion("2.0.0"), - PackageDir: "testdata/cachedir/registry.terraform.io/hashicorp/null/2.0.0/windows_amd64", - ExecutableFile: "testdata/cachedir/registry.terraform.io/hashicorp/null/2.0.0/windows_amd64/terraform-provider-null.exe", + PackageDir: "testdata/cachedir/registry.terraform.io/hashicorp/null/2.0.0/windows_amd64", } if diff := cmp.Diff(want, got); diff != "" { @@ -91,8 +93,7 @@ func TestDirReading(t *testing.T) { Provider: nullProvider, Version: versions.MustParseVersion("2.0.0"), - PackageDir: "testdata/cachedir/registry.terraform.io/hashicorp/null/2.0.0/windows_amd64", - ExecutableFile: "testdata/cachedir/registry.terraform.io/hashicorp/null/2.0.0/windows_amd64/terraform-provider-null.exe", + PackageDir: "testdata/cachedir/registry.terraform.io/hashicorp/null/2.0.0/windows_amd64", } if diff := cmp.Diff(want, got); diff != "" { @@ -141,34 +142,37 @@ func TestDirReading(t *testing.T) { want := map[addrs.Provider][]CachedProvider{ legacyProvider: { { - Provider: legacyProvider, - Version: versions.MustParseVersion("1.0.0"), - PackageDir: "testdata/cachedir/registry.terraform.io/-/legacy/1.0.0/linux_amd64", - ExecutableFile: "testdata/cachedir/registry.terraform.io/-/legacy/1.0.0/linux_amd64/terraform-provider-legacy", + Provider: legacyProvider, + Version: versions.MustParseVersion("1.0.0"), + PackageDir: "testdata/cachedir/registry.terraform.io/-/legacy/1.0.0/linux_amd64", }, }, nullProvider: { { - Provider: nullProvider, - Version: versions.MustParseVersion("2.0.0"), - PackageDir: "testdata/cachedir/registry.terraform.io/hashicorp/null/2.0.0/linux_amd64", - ExecutableFile: "testdata/cachedir/registry.terraform.io/hashicorp/null/2.0.0/linux_amd64/terraform-provider-null", + Provider: nullProvider, + Version: versions.MustParseVersion("2.0.0"), + PackageDir: "testdata/cachedir/registry.terraform.io/hashicorp/null/2.0.0/linux_amd64", }, }, randomProvider: { { - Provider: randomProvider, - Version: versions.MustParseVersion("1.2.0"), - PackageDir: "testdata/cachedir/registry.terraform.io/hashicorp/random/1.2.0/linux_amd64", - ExecutableFile: "testdata/cachedir/registry.terraform.io/hashicorp/random/1.2.0/linux_amd64/terraform-provider-random", + Provider: randomProvider, + Version: versions.MustParseVersion("1.2.0"), + PackageDir: "testdata/cachedir/registry.terraform.io/hashicorp/random/1.2.0/linux_amd64", }, }, randomBetaProvider: { { - Provider: randomBetaProvider, - Version: versions.MustParseVersion("1.2.0"), - PackageDir: "testdata/cachedir/registry.terraform.io/hashicorp/random-beta/1.2.0/linux_amd64", - ExecutableFile: "testdata/cachedir/registry.terraform.io/hashicorp/random-beta/1.2.0/linux_amd64/terraform-provider-random-beta", + Provider: randomBetaProvider, + Version: versions.MustParseVersion("1.2.0"), + PackageDir: "testdata/cachedir/registry.terraform.io/hashicorp/random-beta/1.2.0/linux_amd64", + }, + }, + missingExecutableProvider: { + { + Provider: missingExecutableProvider, + Version: versions.MustParseVersion("2.0.0"), + PackageDir: "testdata/cachedir/registry.terraform.io/missing/executable/2.0.0/linux_amd64", }, }, } diff --git a/internal/providercache/testdata/cachedir/registry.terraform.io/missing/executable/2.0.0/linux_amd64/executable b/internal/providercache/testdata/cachedir/registry.terraform.io/missing/executable/2.0.0/linux_amd64/executable new file mode 100755 index 000000000..f7a5e529a --- /dev/null +++ b/internal/providercache/testdata/cachedir/registry.terraform.io/missing/executable/2.0.0/linux_amd64/executable @@ -0,0 +1 @@ +This file represents a misnamed provider executable. From a18b531b144efda99347f449fd549179df8dca5c Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Tue, 7 Jul 2020 14:41:45 -0400 Subject: [PATCH 2/4] getproviders: FakeInstallablePackageMeta filename Add an optional execFilename argument to the test helper function FakeInstallablePackageMeta, which allows the creation of invalid packages. --- command/init_test.go | 4 ++-- internal/getproviders/mock_source.go | 16 ++++++++++------ 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/command/init_test.go b/command/init_test.go index a44054665..86a5466f9 100644 --- a/command/init_test.go +++ b/command/init_test.go @@ -1710,7 +1710,7 @@ func newMockProviderSource(t *testing.T, availableProviderVersions map[string][] close() t.Fatalf("failed to parse %q as a version number for %q: %s", versionStr, addr.ForDisplay(), err) } - meta, close, err := getproviders.FakeInstallablePackageMeta(addr, version, getproviders.VersionList{getproviders.MustParseVersion("5.0")}, getproviders.CurrentPlatform) + meta, close, err := getproviders.FakeInstallablePackageMeta(addr, version, getproviders.VersionList{getproviders.MustParseVersion("5.0")}, getproviders.CurrentPlatform, "") if err != nil { close() t.Fatalf("failed to prepare fake package for %s %s: %s", addr.ForDisplay(), versionStr, err) @@ -1770,7 +1770,7 @@ func installFakeProviderPackagesElsewhere(t *testing.T, cacheDir *providercache. if err != nil { t.Fatalf("failed to parse %q as a version number for %q: %s", versionStr, name, err) } - meta, close, err := getproviders.FakeInstallablePackageMeta(addr, version, getproviders.VersionList{getproviders.MustParseVersion("5.0")}, getproviders.CurrentPlatform) + meta, close, err := getproviders.FakeInstallablePackageMeta(addr, version, getproviders.VersionList{getproviders.MustParseVersion("5.0")}, getproviders.CurrentPlatform, "") // We're going to install all these fake packages before we return, // so we don't need to preserve them afterwards. defer close() diff --git a/internal/getproviders/mock_source.go b/internal/getproviders/mock_source.go index f85abfd4a..9390ac95b 100644 --- a/internal/getproviders/mock_source.go +++ b/internal/getproviders/mock_source.go @@ -143,13 +143,15 @@ func FakePackageMeta(provider addrs.Provider, version Version, protocols Version // to a temporary archive file that could actually be installed in principle. // // Installing it will not produce a working provider though: just a fake file -// posing as an executable. +// posing as an executable. The filename for the executable defaults to the +// standard terraform-provider-NAME_X.Y.Z format, but can be overridden with +// the execFilename argument. // // It's the caller's responsibility to call the close callback returned // alongside the result in order to clean up the temporary file. The caller // should call the callback even if this function returns an error, because // some error conditions leave a partially-created file on disk. -func FakeInstallablePackageMeta(provider addrs.Provider, version Version, protocols VersionList, target Platform) (PackageMeta, func(), error) { +func FakeInstallablePackageMeta(provider addrs.Provider, version Version, protocols VersionList, target Platform, execFilename string) (PackageMeta, func(), error) { f, err := ioutil.TempFile("", "terraform-getproviders-fake-package-") if err != nil { return PackageMeta{}, func() {}, err @@ -162,10 +164,12 @@ func FakeInstallablePackageMeta(provider addrs.Provider, version Version, protoc os.Remove(f.Name()) } - execFilename := fmt.Sprintf("terraform-provider-%s_%s", provider.Type, version.String()) - if target.OS == "windows" { - // For a little more (technically unnecessary) realism... - execFilename += ".exe" + if execFilename == "" { + execFilename = fmt.Sprintf("terraform-provider-%s_%s", provider.Type, version.String()) + if target.OS == "windows" { + // For a little more (technically unnecessary) realism... + execFilename += ".exe" + } } zw := zip.NewWriter(f) From 3b1347ac1ab82136f8033910ad2be0206a32d7d6 Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Tue, 7 Jul 2020 14:46:23 -0400 Subject: [PATCH 3/4] providercache: Validate provider executable file At the end of the EnsureProviderVersions process, we generate a lockfile of the selected and installed provider versions. This includes a hash of the unpacked provider directory. When calculating this hash and generating the lockfile, we now also verify that the provider directory contains a valid executable file. If not, we return an error for this provider and trigger the installer's HashPackageFailure event. Note that this event is not yet processed by terraform init; that comes in the next commit. --- internal/providercache/installer.go | 8 +++ internal/providercache/installer_test.go | 90 ++++++++++++++++++++++++ 2 files changed, 98 insertions(+) diff --git a/internal/providercache/installer.go b/internal/providercache/installer.go index c0fe08406..bd1d95619 100644 --- a/internal/providercache/installer.go +++ b/internal/providercache/installer.go @@ -400,6 +400,14 @@ NeedProvider: } continue } + if _, err := cached.ExecutableFile(); err != nil { + err := fmt.Errorf("provider binary not found: %s", err) + errs[provider] = err + if cb := evts.HashPackageFailure; cb != nil { + cb(provider, version, err) + } + continue + } hash, err := cached.Hash() if err != nil { errs[provider] = fmt.Errorf("failed to calculate checksum for installed provider %s package: %s", provider, err) diff --git a/internal/providercache/installer_test.go b/internal/providercache/installer_test.go index 46d962407..22b20962d 100644 --- a/internal/providercache/installer_test.go +++ b/internal/providercache/installer_test.go @@ -11,12 +11,102 @@ import ( "strings" "testing" + "github.com/google/go-cmp/cmp" svchost "github.com/hashicorp/terraform-svchost" "github.com/hashicorp/terraform-svchost/disco" "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/internal/getproviders" ) +func TestEnsureProviderVersions_local_source(t *testing.T) { + // create filesystem source using the test provider cache dir + source := getproviders.NewFilesystemMirrorSource("testdata/cachedir") + + // create a temporary workdir + tmpDirPath, err := ioutil.TempDir("", "terraform-test-providercache") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(tmpDirPath) + + // set up the installer using the temporary directory and filesystem source + platform := getproviders.Platform{OS: "linux", Arch: "amd64"} + dir := NewDirWithPlatform(tmpDirPath, platform) + installer := NewInstaller(dir, source) + + tests := map[string]struct { + provider string + version string + installed bool + err string + }{ + "install-unpacked": { + provider: "null", + version: "2.0.0", + installed: true, + }, + "invalid-zip-file": { + provider: "null", + version: "2.1.0", + installed: false, + err: "zip: not a valid zip file", + }, + "version-constraint-unmet": { + provider: "null", + version: "2.2.0", + installed: false, + err: "no available releases match the given constraints 2.2.0", + }, + "missing-executable": { + provider: "missing/executable", + version: "2.0.0", + installed: true, + err: "provider binary not found: could not find executable file starting with terraform-provider-executable", + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + ctx := context.TODO() + + provider := addrs.MustParseProviderSourceString(test.provider) + versionConstraint := getproviders.MustParseVersionConstraints(test.version) + version := getproviders.MustParseVersion(test.version) + reqs := getproviders.Requirements{ + provider: versionConstraint, + } + wantSelected := getproviders.Selections{provider: version} + if !test.installed { + wantSelected = getproviders.Selections{} + } + + selected, err := installer.EnsureProviderVersions(ctx, reqs, InstallNewProvidersOnly) + + if diff := cmp.Diff(wantSelected, selected); diff != "" { + t.Errorf("wrong selected\n%s", diff) + } + + if test.err == "" && err == nil { + return + } + + switch err := err.(type) { + case InstallerError: + providerError, ok := err.ProviderErrors[provider] + if !ok { + t.Fatalf("did not get error for provider %s", provider) + } + + if got := providerError.Error(); got != test.err { + t.Fatalf("wrong result\ngot: %s\nwant: %s\n", got, test.err) + } + default: + t.Fatalf("wrong error type. Expected InstallerError, got %T", err) + } + }) + } +} + // This test only verifies protocol errors and does not try for successfull // installation (at the time of writing, the test files aren't signed so the // signature verification fails); that's left to the e2e tests. From 87d1fb4006f364c8b7840620f04f557258ae2816 Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Tue, 7 Jul 2020 14:48:45 -0400 Subject: [PATCH 4/4] command/init: Display provider validation errors After installing providers, we validate the presence of an executable file, and generate a selected versions lockfile. If this process fails, notify the user. One possible cause for this is an invalid provider package with a missing or misnamed executable file. --- command/init.go | 12 ++++ command/init_test.go | 61 +++++++++++++++++++ .../init-get-provider-invalid-package/main.tf | 8 +++ 3 files changed, 81 insertions(+) create mode 100644 command/testdata/init-get-provider-invalid-package/main.tf diff --git a/command/init.go b/command/init.go index 2f93c1b4e..c76c7175a 100644 --- a/command/init.go +++ b/command/init.go @@ -612,6 +612,18 @@ func (c *InitCommand) getProviders(config *configs.Config, state *states.State, "https://www.terraform.io/docs/plugins/signing.html")) } }, + HashPackageFailure: func(provider addrs.Provider, version getproviders.Version, err error) { + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Failed to validate installed provider", + fmt.Sprintf( + "Validating provider %s v%s failed: %s", + provider.ForDisplay(), + version, + err, + ), + )) + }, } mode := providercache.InstallNewProvidersOnly diff --git a/command/init_test.go b/command/init_test.go index 86a5466f9..61744f0b3 100644 --- a/command/init_test.go +++ b/command/init_test.go @@ -951,6 +951,67 @@ func TestInit_getProviderSource(t *testing.T) { } } +func TestInit_getProviderInvalidPackage(t *testing.T) { + // Create a temporary working directory that is empty + td := tempDir(t) + copy.CopyDir(testFixturePath("init-get-provider-invalid-package"), td) + defer os.RemoveAll(td) + defer testChdir(t, td)() + + overrides := metaOverridesForProvider(testProvider()) + ui := new(cli.MockUi) + + // create a provider source which allows installing an invalid package + addr := addrs.MustParseProviderSourceString("invalid/package") + version := getproviders.MustParseVersion("1.0.0") + meta, close, err := getproviders.FakeInstallablePackageMeta( + addr, + version, + getproviders.VersionList{getproviders.MustParseVersion("5.0")}, + getproviders.CurrentPlatform, + "terraform-package", // should be "terraform-provider-package" + ) + defer close() + if err != nil { + t.Fatalf("failed to prepare fake package for %s %s: %s", addr.ForDisplay(), version, err) + } + providerSource := getproviders.NewMockSource([]getproviders.PackageMeta{meta}, nil) + + m := Meta{ + testingOverrides: overrides, + Ui: ui, + ProviderSource: providerSource, + } + + c := &InitCommand{ + Meta: m, + } + + args := []string{ + "-backend=false", // should be possible to install plugins without backend init + } + if code := c.Run(args); code != 1 { + t.Fatalf("got exit status %d; want 1\nstderr:\n%s\n\nstdout:\n%s", code, ui.ErrorWriter.String(), ui.OutputWriter.String()) + } + + // invalid provider should be installed + packagePath := fmt.Sprintf(".terraform/plugins/registry.terraform.io/invalid/package/1.0.0/%s/terraform-package", getproviders.CurrentPlatform) + if _, err := os.Stat(packagePath); os.IsNotExist(err) { + t.Fatal("provider 'invalid/package' not downloaded") + } + + wantErrors := []string{ + "Failed to validate installed provider", + "could not find executable file starting with terraform-provider-package", + } + got := ui.ErrorWriter.String() + for _, wantError := range wantErrors { + if !strings.Contains(got, wantError) { + t.Fatalf("missing error:\nwant: %q\n got: %q", wantError, got) + } + } +} + func TestInit_getProviderDetectedLegacy(t *testing.T) { // Create a temporary working directory that is empty td := tempDir(t) diff --git a/command/testdata/init-get-provider-invalid-package/main.tf b/command/testdata/init-get-provider-invalid-package/main.tf new file mode 100644 index 000000000..5f93ef7fc --- /dev/null +++ b/command/testdata/init-get-provider-invalid-package/main.tf @@ -0,0 +1,8 @@ +terraform { + required_providers { + package = { + source = "invalid/package" + version = "1.0.0" + } + } +}