From 40ec62c1391f65147c5b8239d97002705dfde074 Mon Sep 17 00:00:00 2001 From: kmoe <5575356+kmoe@users.noreply.github.com> Date: Mon, 1 Nov 2021 20:09:16 +0000 Subject: [PATCH 1/3] command: make module installation interruptible Earlier work to make "terraform init" interruptible made the getproviders package context-aware in order to allow provider installation to be cancelled. Here we make a similar change for module installation, which is now also cancellable with SIGINT. This involves plumbing context through initwd and getmodules. Functions which can make network requests now include a context parameter whose cancellation cancels those requests. Since the module installation code is shared, "terraform get" is now also interruptible during module installation. --- internal/command/command_test.go | 3 +- internal/command/get.go | 6 +-- internal/command/get_test.go | 32 +++++++++++++ internal/command/init.go | 24 +++++----- internal/command/init_test.go | 40 ++++++++++++++++- internal/command/meta_config.go | 45 ++++++++++++++----- internal/command/test.go | 2 +- .../testdata/init-registry-module/main.tf | 4 ++ internal/getmodules/getter.go | 4 +- internal/getmodules/installer.go | 8 +++- internal/initwd/from_module.go | 5 ++- internal/initwd/from_module_test.go | 7 +-- internal/initwd/module_install.go | 38 +++++++++++----- internal/initwd/module_install_test.go | 21 ++++----- internal/initwd/testing.go | 3 +- internal/refactoring/move_validate_test.go | 3 +- internal/registry/client.go | 8 +++- internal/registry/client_test.go | 24 +++++----- internal/terraform/terraform_test.go | 5 ++- 19 files changed, 207 insertions(+), 75 deletions(-) create mode 100644 internal/command/testdata/init-registry-module/main.tf diff --git a/internal/command/command_test.go b/internal/command/command_test.go index 5fab71f1f..4a56ecc73 100644 --- a/internal/command/command_test.go +++ b/internal/command/command_test.go @@ -2,6 +2,7 @@ package command import ( "bytes" + "context" "crypto/md5" "encoding/base64" "encoding/json" @@ -188,7 +189,7 @@ func testModuleWithSnapshot(t *testing.T, name string) (*configs.Config, *config // sources only this ultimately just records all of the module paths // in a JSON file so that we can load them below. inst := initwd.NewModuleInstaller(loader.ModulesDir(), registry.NewClient(nil, nil)) - _, instDiags := inst.InstallModules(dir, true, initwd.ModuleInstallHooksImpl{}) + _, instDiags := inst.InstallModules(context.Background(), dir, true, initwd.ModuleInstallHooksImpl{}) if instDiags.HasErrors() { t.Fatal(instDiags.Err()) } diff --git a/internal/command/get.go b/internal/command/get.go index 170e19400..0f541c3b1 100644 --- a/internal/command/get.go +++ b/internal/command/get.go @@ -33,9 +33,9 @@ func (c *GetCommand) Run(args []string) int { path = c.normalizePath(path) - diags := getModules(&c.Meta, path, update) + abort, diags := getModules(&c.Meta, path, update) c.showDiagnostics(diags) - if diags.HasErrors() { + if abort || diags.HasErrors() { return 1 } @@ -73,7 +73,7 @@ func (c *GetCommand) Synopsis() string { return "Install or upgrade remote Terraform modules" } -func getModules(m *Meta, path string, upgrade bool) tfdiags.Diagnostics { +func getModules(m *Meta, path string, upgrade bool) (abort bool, diags tfdiags.Diagnostics) { hooks := uiModuleInstallHooks{ Ui: m.Ui, ShowLocalPaths: true, diff --git a/internal/command/get_test.go b/internal/command/get_test.go index b2a3ea0a7..77daeb0b4 100644 --- a/internal/command/get_test.go +++ b/internal/command/get_test.go @@ -79,3 +79,35 @@ func TestGet_update(t *testing.T) { t.Fatalf("doesn't look like get: %s", output) } } + +func TestGet_cancel(t *testing.T) { + // This test runs `terraform get` as if SIGINT (or similar on other + // platforms) were sent to it, testing that it is interruptible. + + wd := tempWorkingDirFixture(t, "init-registry-module") + defer testChdir(t, wd.RootModuleDir())() + + // Our shutdown channel is pre-closed so init will exit as soon as it + // starts a cancelable portion of the process. + shutdownCh := make(chan struct{}) + close(shutdownCh) + + ui := cli.NewMockUi() + c := &GetCommand{ + Meta: Meta{ + testingOverrides: metaOverridesForProvider(testProvider()), + Ui: ui, + WorkingDir: wd, + ShutdownCh: shutdownCh, + }, + } + + args := []string{} + if code := c.Run(args); code == 0 { + t.Fatalf("succeeded; wanted error\n%s", ui.OutputWriter.String()) + } + + if got, want := ui.ErrorWriter.String(), `Module installation was canceled by an interrupt signal`; !strings.Contains(got, want) { + t.Fatalf("wrong error message\nshould contain: %s\ngot:\n%s", want, got) + } +} diff --git a/internal/command/init.go b/internal/command/init.go index 39a902c96..b8ba6b9ff 100644 --- a/internal/command/init.go +++ b/internal/command/init.go @@ -115,9 +115,9 @@ func (c *InitCommand) Run(args []string) int { ShowLocalPaths: false, // since they are in a weird location for init } - initDiags := c.initDirFromModule(path, src, hooks) - diags = diags.Append(initDiags) - if initDiags.HasErrors() { + initDirFromModuleAbort, initDirFromModuleDiags := c.initDirFromModule(path, src, hooks) + diags = diags.Append(initDirFromModuleDiags) + if initDirFromModuleAbort || initDirFromModuleDiags.HasErrors() { c.showDiagnostics(diags) return 1 } @@ -174,9 +174,9 @@ func (c *InitCommand) Run(args []string) int { } if flagGet { - modsOutput, modsDiags := c.getModules(path, rootModEarly, flagUpgrade) + modsOutput, modsAbort, modsDiags := c.getModules(path, rootModEarly, flagUpgrade) diags = diags.Append(modsDiags) - if modsDiags.HasErrors() { + if modsAbort || modsDiags.HasErrors() { c.showDiagnostics(diags) return 1 } @@ -326,10 +326,10 @@ func (c *InitCommand) Run(args []string) int { return 0 } -func (c *InitCommand) getModules(path string, earlyRoot *tfconfig.Module, upgrade bool) (output bool, diags tfdiags.Diagnostics) { +func (c *InitCommand) getModules(path string, earlyRoot *tfconfig.Module, upgrade bool) (output bool, abort bool, diags tfdiags.Diagnostics) { if len(earlyRoot.ModuleCalls) == 0 { // Nothing to do - return false, nil + return false, false, nil } if upgrade { @@ -342,8 +342,12 @@ func (c *InitCommand) getModules(path string, earlyRoot *tfconfig.Module, upgrad Ui: c.Ui, ShowLocalPaths: true, } - instDiags := c.installModules(path, upgrade, hooks) - diags = diags.Append(instDiags) + + installAbort, installDiags := c.installModules(path, upgrade, hooks) + diags = diags.Append(installDiags) + if installAbort || installDiags.HasErrors() { + return true, true, diags + } // Since module installer has modified the module manifest on disk, we need // to refresh the cache of it in the loader. @@ -358,7 +362,7 @@ func (c *InitCommand) getModules(path string, earlyRoot *tfconfig.Module, upgrad } } - return true, diags + return true, false, diags } func (c *InitCommand) initCloud(root *configs.Module) (be backend.Backend, output bool, diags tfdiags.Diagnostics) { diff --git a/internal/command/init_test.go b/internal/command/init_test.go index 56bc1f911..b952ea0a2 100644 --- a/internal/command/init_test.go +++ b/internal/command/init_test.go @@ -1417,7 +1417,45 @@ func TestInit_providerSource(t *testing.T) { } } -func TestInit_cancel(t *testing.T) { +func TestInit_cancelModules(t *testing.T) { + // This test runs `terraform init` as if SIGINT (or similar on other + // platforms) were sent to it, testing that it is interruptible. + + td := tempDir(t) + testCopyDir(t, testFixturePath("init-registry-module"), td) + defer os.RemoveAll(td) + defer testChdir(t, td)() + + // Our shutdown channel is pre-closed so init will exit as soon as it + // starts a cancelable portion of the process. + shutdownCh := make(chan struct{}) + close(shutdownCh) + + ui := cli.NewMockUi() + view, _ := testView(t) + m := Meta{ + testingOverrides: metaOverridesForProvider(testProvider()), + Ui: ui, + View: view, + ShutdownCh: shutdownCh, + } + + c := &InitCommand{ + Meta: m, + } + + args := []string{} + + if code := c.Run(args); code == 0 { + t.Fatalf("succeeded; wanted error\n%s", ui.OutputWriter.String()) + } + + if got, want := ui.ErrorWriter.String(), `Module installation was canceled by an interrupt signal`; !strings.Contains(got, want) { + t.Fatalf("wrong error message\nshould contain: %s\ngot:\n%s", want, got) + } +} + +func TestInit_cancelProviders(t *testing.T) { // This test runs `terraform init` as if SIGINT (or similar on other // platforms) were sent to it, testing that it is interruptible. diff --git a/internal/command/meta_config.go b/internal/command/meta_config.go index 6913db594..0abaf52a7 100644 --- a/internal/command/meta_config.go +++ b/internal/command/meta_config.go @@ -164,26 +164,39 @@ func (m *Meta) loadHCLFile(filename string) (hcl.Body, tfdiags.Diagnostics) { } // installModules reads a root module from the given directory and attempts -// recursively install all of its descendent modules. +// recursively to install all of its descendent modules. // // The given hooks object will be notified of installation progress, which -// can then be relayed to the end-user. The moduleUiInstallHooks type in +// can then be relayed to the end-user. The uiModuleInstallHooks type in // this package has a reasonable implementation for displaying notifications // via a provided cli.Ui. -func (m *Meta) installModules(rootDir string, upgrade bool, hooks initwd.ModuleInstallHooks) tfdiags.Diagnostics { - var diags tfdiags.Diagnostics +func (m *Meta) installModules(rootDir string, upgrade bool, hooks initwd.ModuleInstallHooks) (abort bool, diags tfdiags.Diagnostics) { rootDir = m.normalizePath(rootDir) err := os.MkdirAll(m.modulesDir(), os.ModePerm) if err != nil { diags = diags.Append(fmt.Errorf("failed to create local modules directory: %s", err)) - return diags + return true, diags } + // FIXME: KEM: does returning the abort here change behaviour in a particular error + // case? inst := m.moduleInstaller() - _, moreDiags := inst.InstallModules(rootDir, upgrade, hooks) + + // Installation can be aborted by interruption signals + ctx, done := m.InterruptibleContext() + defer done() + + _, moreDiags := inst.InstallModules(ctx, rootDir, upgrade, hooks) diags = diags.Append(moreDiags) - return diags + + if ctx.Err() == context.Canceled { + m.showDiagnostics(diags) + m.Ui.Error("Module installation was canceled by an interrupt signal.") + return true, diags + } + + return false, diags } // initDirFromModule initializes the given directory (which should be @@ -192,15 +205,23 @@ func (m *Meta) installModules(rootDir string, upgrade bool, hooks initwd.ModuleI // // Internally this runs similar steps to installModules. // The given hooks object will be notified of installation progress, which -// can then be relayed to the end-user. The moduleUiInstallHooks type in +// can then be relayed to the end-user. The uiModuleInstallHooks type in // this package has a reasonable implementation for displaying notifications // via a provided cli.Ui. -func (m *Meta) initDirFromModule(targetDir string, addr string, hooks initwd.ModuleInstallHooks) tfdiags.Diagnostics { - var diags tfdiags.Diagnostics +func (m *Meta) initDirFromModule(targetDir string, addr string, hooks initwd.ModuleInstallHooks) (abort bool, diags tfdiags.Diagnostics) { + // Installation can be aborted by interruption signals + ctx, done := m.InterruptibleContext() + defer done() + targetDir = m.normalizePath(targetDir) - moreDiags := initwd.DirFromModule(targetDir, m.modulesDir(), addr, m.registryClient(), hooks) + moreDiags := initwd.DirFromModule(ctx, targetDir, m.modulesDir(), addr, m.registryClient(), hooks) diags = diags.Append(moreDiags) - return diags + if ctx.Err() == context.Canceled { + m.showDiagnostics(diags) + m.Ui.Error("Module initialization was canceled by an interrupt signal.") + return true, diags + } + return false, diags } // inputForSchema uses interactive prompts to try to populate any diff --git a/internal/command/test.go b/internal/command/test.go index eff291179..fc6d7ef70 100644 --- a/internal/command/test.go +++ b/internal/command/test.go @@ -249,7 +249,7 @@ func (c *TestCommand) prepareSuiteDir(ctx context.Context, suiteName string) (te os.MkdirAll(suiteDirs.ModulesDir, 0755) // if this fails then we'll ignore it and let InstallModules below fail instead reg := c.registryClient() moduleInst := initwd.NewModuleInstaller(suiteDirs.ModulesDir, reg) - _, moreDiags := moduleInst.InstallModules(configDir, true, nil) + _, moreDiags := moduleInst.InstallModules(ctx, configDir, true, nil) diags = diags.Append(moreDiags) if diags.HasErrors() { return suiteDirs, diags diff --git a/internal/command/testdata/init-registry-module/main.tf b/internal/command/testdata/init-registry-module/main.tf new file mode 100644 index 000000000..cc388d7c5 --- /dev/null +++ b/internal/command/testdata/init-registry-module/main.tf @@ -0,0 +1,4 @@ +module "foo" { + source = "registry.does.not.exist/example_corp/foo/bar" + version = "0.1.0" +} diff --git a/internal/getmodules/getter.go b/internal/getmodules/getter.go index 67b5c2760..95f334762 100644 --- a/internal/getmodules/getter.go +++ b/internal/getmodules/getter.go @@ -1,6 +1,7 @@ package getmodules import ( + "context" "fmt" "log" "os" @@ -119,7 +120,7 @@ type reusingGetter map[string]string // end-user-actionable error messages. At this time we do not have any // reasonable way to improve these error messages at this layer because // the underlying errors are not separately recognizable. -func (g reusingGetter) getWithGoGetter(instPath, packageAddr string) error { +func (g reusingGetter) getWithGoGetter(ctx context.Context, instPath, packageAddr string) error { var err error if prevDir, exists := g[packageAddr]; exists { @@ -144,6 +145,7 @@ func (g reusingGetter) getWithGoGetter(instPath, packageAddr string) error { Detectors: goGetterNoDetectors, // our caller should've already done detection Decompressors: goGetterDecompressors, Getters: goGetterGetters, + Ctx: ctx, } err = client.Get() if err != nil { diff --git a/internal/getmodules/installer.go b/internal/getmodules/installer.go index 087c0cd3d..657c0e73a 100644 --- a/internal/getmodules/installer.go +++ b/internal/getmodules/installer.go @@ -1,5 +1,9 @@ package getmodules +import ( + "context" +) + // PackageFetcher is a low-level utility for fetching remote module packages // into local filesystem directories in preparation for use by higher-level // module installer functionality implemented elsewhere. @@ -35,6 +39,6 @@ func NewPackageFetcher() *PackageFetcher { // a module source address which includes a subdirectory portion then the // caller must resolve that itself, possibly with the help of the // getmodules.SplitPackageSubdir and getmodules.ExpandSubdirGlobs functions. -func (f *PackageFetcher) FetchPackage(instDir string, packageAddr string) error { - return f.getter.getWithGoGetter(instDir, packageAddr) +func (f *PackageFetcher) FetchPackage(ctx context.Context, instDir string, packageAddr string) error { + return f.getter.getWithGoGetter(ctx, instDir, packageAddr) } diff --git a/internal/initwd/from_module.go b/internal/initwd/from_module.go index 392370032..38a4e49b5 100644 --- a/internal/initwd/from_module.go +++ b/internal/initwd/from_module.go @@ -1,6 +1,7 @@ package initwd import ( + "context" "fmt" "io/ioutil" "log" @@ -41,7 +42,7 @@ const initFromModuleRootKeyPrefix = initFromModuleRootCallName + "." // references using ../ from that module to be unresolvable. Error diagnostics // are produced in that case, to prompt the user to rewrite the source strings // to be absolute references to the original remote module. -func DirFromModule(rootDir, modulesDir, sourceAddr string, reg *registry.Client, hooks ModuleInstallHooks) tfdiags.Diagnostics { +func DirFromModule(ctx context.Context, rootDir, modulesDir, sourceAddr string, reg *registry.Client, hooks ModuleInstallHooks) tfdiags.Diagnostics { var diags tfdiags.Diagnostics // The way this function works is pretty ugly, but we accept it because @@ -144,7 +145,7 @@ func DirFromModule(rootDir, modulesDir, sourceAddr string, reg *registry.Client, Wrapped: hooks, } fetcher := getmodules.NewPackageFetcher() - _, instDiags := inst.installDescendentModules(fakeRootModule, rootDir, instManifest, true, wrapHooks, fetcher) + _, instDiags := inst.installDescendentModules(ctx, fakeRootModule, rootDir, instManifest, true, wrapHooks, fetcher) diags = append(diags, instDiags...) if instDiags.HasErrors() { return diags diff --git a/internal/initwd/from_module_test.go b/internal/initwd/from_module_test.go index 8265f0e3a..a28ad2a7d 100644 --- a/internal/initwd/from_module_test.go +++ b/internal/initwd/from_module_test.go @@ -1,6 +1,7 @@ package initwd import ( + "context" "io/ioutil" "os" "path/filepath" @@ -38,7 +39,7 @@ func TestDirFromModule_registry(t *testing.T) { hooks := &testInstallHooks{} reg := registry.NewClient(nil, nil) - diags := DirFromModule(dir, modsDir, "hashicorp/module-installer-acctest/aws//examples/main", reg, hooks) + diags := DirFromModule(context.Background(), dir, modsDir, "hashicorp/module-installer-acctest/aws//examples/main", reg, hooks) assertNoDiagnostics(t, diags) v := version.Must(version.NewVersion("0.0.2")) @@ -154,7 +155,7 @@ func TestDirFromModule_submodules(t *testing.T) { } modInstallDir := filepath.Join(dir, ".terraform/modules") - diags := DirFromModule(dir, modInstallDir, fromModuleDir, nil, hooks) + diags := DirFromModule(context.Background(), dir, modInstallDir, fromModuleDir, nil, hooks) assertNoDiagnostics(t, diags) wantCalls := []testInstallHookCall{ { @@ -248,7 +249,7 @@ func TestDirFromModule_rel_submodules(t *testing.T) { modInstallDir := ".terraform/modules" sourceDir := "../local-modules" - diags := DirFromModule(".", modInstallDir, sourceDir, nil, hooks) + diags := DirFromModule(context.Background(), ".", modInstallDir, sourceDir, nil, hooks) assertNoDiagnostics(t, diags) wantCalls := []testInstallHookCall{ { diff --git a/internal/initwd/module_install.go b/internal/initwd/module_install.go index 83f4ed2bb..c5355ee96 100644 --- a/internal/initwd/module_install.go +++ b/internal/initwd/module_install.go @@ -1,6 +1,8 @@ package initwd import ( + "context" + "errors" "fmt" "log" "os" @@ -75,7 +77,7 @@ func NewModuleInstaller(modsDir string, reg *registry.Client) *ModuleInstaller { // If successful (the returned diagnostics contains no errors) then the // first return value is the early configuration tree that was constructed by // the installation process. -func (i *ModuleInstaller) InstallModules(rootDir string, upgrade bool, hooks ModuleInstallHooks) (*earlyconfig.Config, tfdiags.Diagnostics) { +func (i *ModuleInstaller) InstallModules(ctx context.Context, rootDir string, upgrade bool, hooks ModuleInstallHooks) (*earlyconfig.Config, tfdiags.Diagnostics) { log.Printf("[TRACE] ModuleInstaller: installing child modules for %s into %s", rootDir, i.modsDir) rootMod, diags := earlyconfig.LoadModule(rootDir) @@ -94,13 +96,13 @@ func (i *ModuleInstaller) InstallModules(rootDir string, upgrade bool, hooks Mod } fetcher := getmodules.NewPackageFetcher() - cfg, instDiags := i.installDescendentModules(rootMod, rootDir, manifest, upgrade, hooks, fetcher) + cfg, instDiags := i.installDescendentModules(ctx, rootMod, rootDir, manifest, upgrade, hooks, fetcher) diags = append(diags, instDiags...) return cfg, diags } -func (i *ModuleInstaller) installDescendentModules(rootMod *tfconfig.Module, rootDir string, manifest modsdir.Manifest, upgrade bool, hooks ModuleInstallHooks, fetcher *getmodules.PackageFetcher) (*earlyconfig.Config, tfdiags.Diagnostics) { +func (i *ModuleInstaller) installDescendentModules(ctx context.Context, rootMod *tfconfig.Module, rootDir string, manifest modsdir.Manifest, upgrade bool, hooks ModuleInstallHooks, fetcher *getmodules.PackageFetcher) (*earlyconfig.Config, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics if hooks == nil { @@ -209,13 +211,13 @@ func (i *ModuleInstaller) installDescendentModules(rootMod *tfconfig.Module, roo case addrs.ModuleSourceRegistry: log.Printf("[TRACE] ModuleInstaller: %s is a registry module at %s", key, addr.String()) - mod, v, mDiags := i.installRegistryModule(req, key, instPath, addr, manifest, hooks, fetcher) + mod, v, mDiags := i.installRegistryModule(ctx, req, key, instPath, addr, manifest, hooks, fetcher) diags = append(diags, mDiags...) return mod, v, diags case addrs.ModuleSourceRemote: log.Printf("[TRACE] ModuleInstaller: %s address %q will be handled by go-getter", key, addr.String()) - mod, mDiags := i.installGoGetterModule(req, key, instPath, manifest, hooks, fetcher) + mod, mDiags := i.installGoGetterModule(ctx, req, key, instPath, manifest, hooks, fetcher) diags = append(diags, mDiags...) return mod, nil, diags @@ -301,7 +303,7 @@ func (i *ModuleInstaller) installLocalModule(req *earlyconfig.ModuleRequest, key return mod, diags } -func (i *ModuleInstaller) installRegistryModule(req *earlyconfig.ModuleRequest, key string, instPath string, addr addrs.ModuleSourceRegistry, manifest modsdir.Manifest, hooks ModuleInstallHooks, fetcher *getmodules.PackageFetcher) (*tfconfig.Module, *version.Version, tfdiags.Diagnostics) { +func (i *ModuleInstaller) installRegistryModule(ctx context.Context, req *earlyconfig.ModuleRequest, key string, instPath string, addr addrs.ModuleSourceRegistry, manifest modsdir.Manifest, hooks ModuleInstallHooks, fetcher *getmodules.PackageFetcher) (*tfconfig.Module, *version.Version, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics hostname := addr.PackageAddr.Host @@ -324,7 +326,7 @@ func (i *ModuleInstaller) installRegistryModule(req *earlyconfig.ModuleRequest, } else { var err error log.Printf("[DEBUG] %s listing available versions of %s at %s", key, addr, hostname) - resp, err = reg.ModuleVersions(regsrcAddr) + resp, err = reg.ModuleVersions(ctx, regsrcAddr) if err != nil { if registry.IsModuleNotFound(err) { diags = diags.Append(tfdiags.Sourceless( @@ -332,6 +334,12 @@ func (i *ModuleInstaller) installRegistryModule(req *earlyconfig.ModuleRequest, "Module not found", fmt.Sprintf("Module %q (from %s:%d) cannot be found in the module registry at %s.", req.Name, req.CallPos.Filename, req.CallPos.Line, hostname), )) + } else if errors.Is(err, context.Canceled) { + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Module installation was interrupted", + fmt.Sprintf("Received interrupt signal while retrieving available versions for module %q.", req.Name), + )) } else { diags = diags.Append(tfdiags.Sourceless( tfdiags.Error, @@ -423,7 +431,7 @@ func (i *ModuleInstaller) installRegistryModule(req *earlyconfig.ModuleRequest, // first check the cache for the download URL moduleAddr := moduleVersion{module: packageAddr, version: latestMatch.String()} if _, exists := i.registryPackageSources[moduleAddr]; !exists { - realAddrRaw, err := reg.ModuleLocation(regsrcAddr, latestMatch.String()) + realAddrRaw, err := reg.ModuleLocation(ctx, regsrcAddr, latestMatch.String()) if err != nil { log.Printf("[ERROR] %s from %s %s: %s", key, addr, latestMatch, err) diags = diags.Append(tfdiags.Sourceless( @@ -463,7 +471,15 @@ func (i *ModuleInstaller) installRegistryModule(req *earlyconfig.ModuleRequest, log.Printf("[TRACE] ModuleInstaller: %s %s %s is available at %q", key, packageAddr, latestMatch, dlAddr.PackageAddr) - err := fetcher.FetchPackage(instPath, dlAddr.PackageAddr.String()) + err := fetcher.FetchPackage(ctx, instPath, dlAddr.PackageAddr.String()) + if errors.Is(err, context.Canceled) { + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Module download was interrupted", + fmt.Sprintf("Interrupt signal received when downloading module %s.", addr), + )) + return nil, nil, diags + } if err != nil { // Errors returned by go-getter have very inconsistent quality as // end-user error messages, but for now we're accepting that because @@ -519,7 +535,7 @@ func (i *ModuleInstaller) installRegistryModule(req *earlyconfig.ModuleRequest, return mod, latestMatch, diags } -func (i *ModuleInstaller) installGoGetterModule(req *earlyconfig.ModuleRequest, key string, instPath string, manifest modsdir.Manifest, hooks ModuleInstallHooks, fetcher *getmodules.PackageFetcher) (*tfconfig.Module, tfdiags.Diagnostics) { +func (i *ModuleInstaller) installGoGetterModule(ctx context.Context, req *earlyconfig.ModuleRequest, key string, instPath string, manifest modsdir.Manifest, hooks ModuleInstallHooks, fetcher *getmodules.PackageFetcher) (*tfconfig.Module, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics // Report up to the caller that we're about to start downloading. @@ -536,7 +552,7 @@ func (i *ModuleInstaller) installGoGetterModule(req *earlyconfig.ModuleRequest, return nil, diags } - err := fetcher.FetchPackage(instPath, packageAddr.String()) + err := fetcher.FetchPackage(ctx, instPath, packageAddr.String()) if err != nil { // go-getter generates a poor error for an invalid relative path, so // we'll detect that case and generate a better one. diff --git a/internal/initwd/module_install_test.go b/internal/initwd/module_install_test.go index 223ca1ca7..534ef5840 100644 --- a/internal/initwd/module_install_test.go +++ b/internal/initwd/module_install_test.go @@ -2,6 +2,7 @@ package initwd import ( "bytes" + "context" "flag" "fmt" "io/ioutil" @@ -39,7 +40,7 @@ func TestModuleInstaller(t *testing.T) { modulesDir := filepath.Join(dir, ".terraform/modules") inst := NewModuleInstaller(modulesDir, nil) - _, diags := inst.InstallModules(".", false, hooks) + _, diags := inst.InstallModules(context.Background(), ".", false, hooks) assertNoDiagnostics(t, diags) wantCalls := []testInstallHookCall{ @@ -100,7 +101,7 @@ func TestModuleInstaller_error(t *testing.T) { modulesDir := filepath.Join(dir, ".terraform/modules") inst := NewModuleInstaller(modulesDir, nil) - _, diags := inst.InstallModules(".", false, hooks) + _, diags := inst.InstallModules(context.Background(), ".", false, hooks) if !diags.HasErrors() { t.Fatal("expected error") @@ -135,7 +136,7 @@ func TestModuleInstaller_packageEscapeError(t *testing.T) { modulesDir := filepath.Join(dir, ".terraform/modules") inst := NewModuleInstaller(modulesDir, nil) - _, diags := inst.InstallModules(".", false, hooks) + _, diags := inst.InstallModules(context.Background(), ".", false, hooks) if !diags.HasErrors() { t.Fatal("expected error") @@ -170,7 +171,7 @@ func TestModuleInstaller_explicitPackageBoundary(t *testing.T) { modulesDir := filepath.Join(dir, ".terraform/modules") inst := NewModuleInstaller(modulesDir, nil) - _, diags := inst.InstallModules(".", false, hooks) + _, diags := inst.InstallModules(context.Background(), ".", false, hooks) if diags.HasErrors() { t.Fatalf("unexpected errors\n%s", diags.Err().Error()) @@ -186,7 +187,7 @@ func TestModuleInstaller_invalid_version_constraint_error(t *testing.T) { modulesDir := filepath.Join(dir, ".terraform/modules") inst := NewModuleInstaller(modulesDir, nil) - _, diags := inst.InstallModules(".", false, hooks) + _, diags := inst.InstallModules(context.Background(), ".", false, hooks) if !diags.HasErrors() { t.Fatal("expected error") @@ -204,7 +205,7 @@ func TestModuleInstaller_invalidVersionConstraintGetter(t *testing.T) { modulesDir := filepath.Join(dir, ".terraform/modules") inst := NewModuleInstaller(modulesDir, nil) - _, diags := inst.InstallModules(".", false, hooks) + _, diags := inst.InstallModules(context.Background(), ".", false, hooks) if !diags.HasErrors() { t.Fatal("expected error") @@ -222,7 +223,7 @@ func TestModuleInstaller_invalidVersionConstraintLocal(t *testing.T) { modulesDir := filepath.Join(dir, ".terraform/modules") inst := NewModuleInstaller(modulesDir, nil) - _, diags := inst.InstallModules(".", false, hooks) + _, diags := inst.InstallModules(context.Background(), ".", false, hooks) if !diags.HasErrors() { t.Fatal("expected error") @@ -240,7 +241,7 @@ func TestModuleInstaller_symlink(t *testing.T) { modulesDir := filepath.Join(dir, ".terraform/modules") inst := NewModuleInstaller(modulesDir, nil) - _, diags := inst.InstallModules(".", false, hooks) + _, diags := inst.InstallModules(context.Background(), ".", false, hooks) assertNoDiagnostics(t, diags) wantCalls := []testInstallHookCall{ @@ -313,7 +314,7 @@ func TestLoaderInstallModules_registry(t *testing.T) { hooks := &testInstallHooks{} modulesDir := filepath.Join(dir, ".terraform/modules") inst := NewModuleInstaller(modulesDir, registry.NewClient(nil, nil)) - _, diags := inst.InstallModules(dir, false, hooks) + _, diags := inst.InstallModules(context.Background(), dir, false, hooks) assertNoDiagnostics(t, diags) v := version.Must(version.NewVersion("0.0.1")) @@ -473,7 +474,7 @@ func TestLoaderInstallModules_goGetter(t *testing.T) { hooks := &testInstallHooks{} modulesDir := filepath.Join(dir, ".terraform/modules") inst := NewModuleInstaller(modulesDir, registry.NewClient(nil, nil)) - _, diags := inst.InstallModules(dir, false, hooks) + _, diags := inst.InstallModules(context.Background(), dir, false, hooks) assertNoDiagnostics(t, diags) wantCalls := []testInstallHookCall{ diff --git a/internal/initwd/testing.go b/internal/initwd/testing.go index 320540daa..406718159 100644 --- a/internal/initwd/testing.go +++ b/internal/initwd/testing.go @@ -1,6 +1,7 @@ package initwd import ( + "context" "testing" "github.com/hashicorp/terraform/internal/configs" @@ -35,7 +36,7 @@ func LoadConfigForTests(t *testing.T, rootDir string) (*configs.Config, *configl loader, cleanup := configload.NewLoaderForTests(t) inst := NewModuleInstaller(loader.ModulesDir(), registry.NewClient(nil, nil)) - _, moreDiags := inst.InstallModules(rootDir, true, ModuleInstallHooksImpl{}) + _, moreDiags := inst.InstallModules(context.Background(), rootDir, true, ModuleInstallHooksImpl{}) diags = diags.Append(moreDiags) if diags.HasErrors() { cleanup() diff --git a/internal/refactoring/move_validate_test.go b/internal/refactoring/move_validate_test.go index 49f8acd86..986c1d5db 100644 --- a/internal/refactoring/move_validate_test.go +++ b/internal/refactoring/move_validate_test.go @@ -1,6 +1,7 @@ package refactoring import ( + "context" "strings" "testing" @@ -419,7 +420,7 @@ func loadRefactoringFixture(t *testing.T, dir string) (*configs.Config, instance defer cleanup() inst := initwd.NewModuleInstaller(loader.ModulesDir(), registry.NewClient(nil, nil)) - _, instDiags := inst.InstallModules(dir, true, initwd.ModuleInstallHooksImpl{}) + _, instDiags := inst.InstallModules(context.Background(), dir, true, initwd.ModuleInstallHooksImpl{}) if instDiags.HasErrors() { t.Fatal(instDiags.Err()) } diff --git a/internal/registry/client.go b/internal/registry/client.go index 495048486..0204674bb 100644 --- a/internal/registry/client.go +++ b/internal/registry/client.go @@ -1,6 +1,7 @@ package registry import ( + "context" "encoding/json" "fmt" "io/ioutil" @@ -109,7 +110,7 @@ func (c *Client) Discover(host svchost.Hostname, serviceID string) (*url.URL, er } // ModuleVersions queries the registry for a module, and returns the available versions. -func (c *Client) ModuleVersions(module *regsrc.Module) (*response.ModuleVersions, error) { +func (c *Client) ModuleVersions(ctx context.Context, module *regsrc.Module) (*response.ModuleVersions, error) { host, err := module.SvcHost() if err != nil { return nil, err @@ -133,6 +134,7 @@ func (c *Client) ModuleVersions(module *regsrc.Module) (*response.ModuleVersions if err != nil { return nil, err } + req = req.WithContext(ctx) c.addRequestCreds(host, req.Request) req.Header.Set(xTerraformVersion, tfVersion) @@ -182,7 +184,7 @@ func (c *Client) addRequestCreds(host svchost.Hostname, req *http.Request) { // ModuleLocation find the download location for a specific version module. // This returns a string, because the final location may contain special go-getter syntax. -func (c *Client) ModuleLocation(module *regsrc.Module, version string) (string, error) { +func (c *Client) ModuleLocation(ctx context.Context, module *regsrc.Module, version string) (string, error) { host, err := module.SvcHost() if err != nil { return "", err @@ -211,6 +213,8 @@ func (c *Client) ModuleLocation(module *regsrc.Module, version string) (string, return "", err } + req = req.WithContext(ctx) + c.addRequestCreds(host, req.Request) req.Header.Set(xTerraformVersion, tfVersion) diff --git a/internal/registry/client_test.go b/internal/registry/client_test.go index 765c3b001..da3055110 100644 --- a/internal/registry/client_test.go +++ b/internal/registry/client_test.go @@ -103,7 +103,7 @@ func TestLookupModuleVersions(t *testing.T) { t.Fatal(err) } - resp, err := client.ModuleVersions(modsrc) + resp, err := client.ModuleVersions(context.Background(), modsrc) if err != nil { t.Fatal(err) } @@ -143,7 +143,7 @@ func TestInvalidRegistry(t *testing.T) { t.Fatal(err) } - if _, err := client.ModuleVersions(modsrc); err == nil { + if _, err := client.ModuleVersions(context.Background(), modsrc); err == nil { t.Fatal("expected error") } } @@ -160,11 +160,11 @@ func TestRegistryAuth(t *testing.T) { t.Fatal(err) } - _, err = client.ModuleVersions(mod) + _, err = client.ModuleVersions(context.Background(), mod) if err != nil { t.Fatal(err) } - _, err = client.ModuleLocation(mod, "1.0.0") + _, err = client.ModuleLocation(context.Background(), mod, "1.0.0") if err != nil { t.Fatal(err) } @@ -173,11 +173,11 @@ func TestRegistryAuth(t *testing.T) { client.services.SetCredentialsSource(nil) // both should fail without auth - _, err = client.ModuleVersions(mod) + _, err = client.ModuleVersions(context.Background(), mod) if err == nil { t.Fatal("expected error") } - _, err = client.ModuleLocation(mod, "1.0.0") + _, err = client.ModuleLocation(context.Background(), mod, "1.0.0") if err == nil { t.Fatal("expected error") } @@ -195,7 +195,7 @@ func TestLookupModuleLocationRelative(t *testing.T) { t.Fatal(err) } - got, err := client.ModuleLocation(mod, "0.2.0") + got, err := client.ModuleLocation(context.Background(), mod, "0.2.0") if err != nil { t.Fatal(err) } @@ -224,7 +224,7 @@ func TestAccLookupModuleVersions(t *testing.T) { } s := NewClient(regDisco, nil) - resp, err := s.ModuleVersions(modsrc) + resp, err := s.ModuleVersions(context.Background(), modsrc) if err != nil { t.Fatal(err) } @@ -277,7 +277,7 @@ func TestLookupLookupModuleError(t *testing.T) { return oldCheck(ctx, resp, err) } - _, err = client.ModuleLocation(mod, "0.2.0") + _, err = client.ModuleLocation(context.Background(), mod, "0.2.0") if err == nil { t.Fatal("expected error") } @@ -299,7 +299,7 @@ func TestLookupModuleRetryError(t *testing.T) { if err != nil { t.Fatal(err) } - resp, err := client.ModuleVersions(modsrc) + resp, err := client.ModuleVersions(context.Background(), modsrc) if err == nil { t.Fatal("expected requests to exceed retry", err) } @@ -328,7 +328,7 @@ func TestLookupModuleNoRetryError(t *testing.T) { if err != nil { t.Fatal(err) } - resp, err := client.ModuleVersions(modsrc) + resp, err := client.ModuleVersions(context.Background(), modsrc) if err == nil { t.Fatal("expected request to fail", err) } @@ -354,7 +354,7 @@ func TestLookupModuleNetworkError(t *testing.T) { if err != nil { t.Fatal(err) } - resp, err := client.ModuleVersions(modsrc) + resp, err := client.ModuleVersions(context.Background(), modsrc) if err == nil { t.Fatal("expected request to fail", err) } diff --git a/internal/terraform/terraform_test.go b/internal/terraform/terraform_test.go index 7eccb51c2..51c1579ec 100644 --- a/internal/terraform/terraform_test.go +++ b/internal/terraform/terraform_test.go @@ -1,6 +1,7 @@ package terraform import ( + "context" "flag" "io" "io/ioutil" @@ -61,7 +62,7 @@ func testModuleWithSnapshot(t *testing.T, name string) (*configs.Config, *config // sources only this ultimately just records all of the module paths // in a JSON file so that we can load them below. inst := initwd.NewModuleInstaller(loader.ModulesDir(), registry.NewClient(nil, nil)) - _, instDiags := inst.InstallModules(dir, true, initwd.ModuleInstallHooksImpl{}) + _, instDiags := inst.InstallModules(context.Background(), dir, true, initwd.ModuleInstallHooksImpl{}) if instDiags.HasErrors() { t.Fatal(instDiags.Err()) } @@ -119,7 +120,7 @@ func testModuleInline(t *testing.T, sources map[string]string) *configs.Config { // sources only this ultimately just records all of the module paths // in a JSON file so that we can load them below. inst := initwd.NewModuleInstaller(loader.ModulesDir(), registry.NewClient(nil, nil)) - _, instDiags := inst.InstallModules(cfgPath, true, initwd.ModuleInstallHooksImpl{}) + _, instDiags := inst.InstallModules(context.Background(), cfgPath, true, initwd.ModuleInstallHooksImpl{}) if instDiags.HasErrors() { t.Fatal(instDiags.Err()) } From 70228764940e0dbdd2bbfc506fe48c8d7b7bae06 Mon Sep 17 00:00:00 2001 From: Katy Moe Date: Thu, 4 Nov 2021 12:40:45 +0000 Subject: [PATCH 2/3] bump go-getter to v1.5.9 --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index e0dc22906..498cefd40 100644 --- a/go.mod +++ b/go.mod @@ -36,7 +36,7 @@ require ( github.com/hashicorp/go-azure-helpers v0.14.0 github.com/hashicorp/go-checkpoint v0.5.0 github.com/hashicorp/go-cleanhttp v0.5.2 - github.com/hashicorp/go-getter v1.5.2 + github.com/hashicorp/go-getter v1.5.9 github.com/hashicorp/go-hclog v0.15.0 github.com/hashicorp/go-multierror v1.1.1 github.com/hashicorp/go-plugin v1.4.3 diff --git a/go.sum b/go.sum index addfc11f7..ddbdbe998 100644 --- a/go.sum +++ b/go.sum @@ -350,8 +350,8 @@ github.com/hashicorp/go-cleanhttp v0.5.0/go.mod h1:JpRdi6/HCYpAwUzNwuwqhbovhLtng github.com/hashicorp/go-cleanhttp v0.5.1/go.mod h1:JpRdi6/HCYpAwUzNwuwqhbovhLtngrth3wmdIIUrZ80= github.com/hashicorp/go-cleanhttp v0.5.2 h1:035FKYIWjmULyFRBKPs8TBQoi0x6d9G4xc9neXJWAZQ= github.com/hashicorp/go-cleanhttp v0.5.2/go.mod h1:kO/YDlP8L1346E6Sodw+PrpBSV4/SoxCXGY6BqNFT48= -github.com/hashicorp/go-getter v1.5.2 h1:XDo8LiAcDisiqZdv0TKgz+HtX3WN7zA2JD1R1tjsabE= -github.com/hashicorp/go-getter v1.5.2/go.mod h1:orNH3BTYLu/fIxGIdLjLoAJHWMDQ/UKQr5O4m3iBuoo= +github.com/hashicorp/go-getter v1.5.9 h1:b7ahZW50iQiUek/at3CvZhPK1/jiV6CtKcsJiR6E4R0= +github.com/hashicorp/go-getter v1.5.9/go.mod h1:BrrV/1clo8cCYu6mxvboYg+KutTiFnXjMEgDD8+i7ZI= github.com/hashicorp/go-hclog v0.9.2/go.mod h1:5CU+agLiy3J7N7QjHK5d05KxGsuXiQLrjA0H7acj2lQ= github.com/hashicorp/go-hclog v0.12.0/go.mod h1:whpDNt7SSdeAju8AWKIWsul05p54N/39EeqMAyrmvFQ= github.com/hashicorp/go-hclog v0.14.1/go.mod h1:whpDNt7SSdeAju8AWKIWsul05p54N/39EeqMAyrmvFQ= From 6da61bf07b527e9c6c5ac1e04d6adb068a15ead3 Mon Sep 17 00:00:00 2001 From: Katy Moe Date: Tue, 9 Nov 2021 13:15:13 +0000 Subject: [PATCH 3/3] revert change to installModules diag handling Error diags from c.installModules() no longer cause getModules() to exit early. Whether installModules completed successfully, errored, or was cancelled, we try to update the manifest as best we can, preferring incomplete information to none. --- internal/command/init.go | 9 +++++---- internal/command/meta_config.go | 2 -- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/internal/command/init.go b/internal/command/init.go index b8ba6b9ff..a44fba3dc 100644 --- a/internal/command/init.go +++ b/internal/command/init.go @@ -345,9 +345,10 @@ func (c *InitCommand) getModules(path string, earlyRoot *tfconfig.Module, upgrad installAbort, installDiags := c.installModules(path, upgrade, hooks) diags = diags.Append(installDiags) - if installAbort || installDiags.HasErrors() { - return true, true, diags - } + + // At this point, installModules may have generated error diags or been + // aborted by SIGINT. In any case we continue and the manifest as best + // we can. // Since module installer has modified the module manifest on disk, we need // to refresh the cache of it in the loader. @@ -362,7 +363,7 @@ func (c *InitCommand) getModules(path string, earlyRoot *tfconfig.Module, upgrad } } - return true, false, diags + return true, installAbort, diags } func (c *InitCommand) initCloud(root *configs.Module) (be backend.Backend, output bool, diags tfdiags.Diagnostics) { diff --git a/internal/command/meta_config.go b/internal/command/meta_config.go index 0abaf52a7..d19389283 100644 --- a/internal/command/meta_config.go +++ b/internal/command/meta_config.go @@ -178,8 +178,6 @@ func (m *Meta) installModules(rootDir string, upgrade bool, hooks initwd.ModuleI diags = diags.Append(fmt.Errorf("failed to create local modules directory: %s", err)) return true, diags } - // FIXME: KEM: does returning the abort here change behaviour in a particular error - // case? inst := m.moduleInstaller()