From 17b766c3eafc1df959d324f83ff13455f4d2056c Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Tue, 1 Jun 2021 12:30:56 -0700 Subject: [PATCH] configs: EntersNewPackage methods for descendant modules Now that we (in the previous commit) refactored how we deal with module sources to do the parsing at config loading time rather than at module installation time, we can expose a method to centralize the determination for whether a particular module call (and its resulting Config object) enters a new external package. We don't use this for anything yet, but in later commits we will use this for some cross-module features that are available only for modules belonging to the same package, because we assume that modules grouped together in a package can change together and thus it's okay to permit a little more coupling of internal details in that case, which would not be appropriate between modules that are versioned separately. --- internal/configs/config.go | 17 +++++ internal/configs/module_call.go | 104 +++++++++++++++++++-------- internal/configs/module_call_test.go | 46 ++++++++++++ 3 files changed, 136 insertions(+), 31 deletions(-) diff --git a/internal/configs/config.go b/internal/configs/config.go index 93632612a..cc3f06e79 100644 --- a/internal/configs/config.go +++ b/internal/configs/config.go @@ -177,6 +177,23 @@ func (c *Config) DescendentForInstance(path addrs.ModuleInstance) *Config { return current } +// EntersNewPackage returns true if this call is to an external module, either +// directly via a remote source address or indirectly via a registry source +// address. +// +// Other behaviors in Terraform may treat package crossings as a special +// situation, because that indicates that the caller and callee can change +// independently of one another and thus we should disallow using any features +// where the caller assumes anything about the callee other than its input +// variables, required provider configurations, and output values. +// +// It's not meaningful to ask if the Config representing the root module enters +// a new package because the root module is always outside of all module +// packages, and so this function will arbitrarily return false in that case. +func (c *Config) EntersNewPackage() bool { + return moduleSourceAddrEntersNewPackage(c.SourceAddr) +} + // ProviderRequirements searches the full tree of modules under the receiver // for both explicit and implicit dependencies on providers. // diff --git a/internal/configs/module_call.go b/internal/configs/module_call.go index 7213bea58..5d887dac0 100644 --- a/internal/configs/module_call.go +++ b/internal/configs/module_call.go @@ -60,41 +60,46 @@ func decodeModuleBlock(block *hcl.Block, override bool) (*ModuleCall, hcl.Diagno } if attr, exists := content.Attributes["source"]; exists { + mc.SourceSet = true + mc.SourceAddrRange = attr.Expr.Range() valDiags := gohcl.DecodeExpression(attr.Expr, nil, &mc.SourceAddrRaw) diags = append(diags, valDiags...) - mc.SourceAddrRange = attr.Expr.Range() - mc.SourceSet = true - - addr, err := addrs.ParseModuleSource(mc.SourceAddrRaw) - mc.SourceAddr = addr - if err != nil { - // NOTE: In practice it's actually very unlikely to end up here, - // because our source address parser can turn just about any string - // into some sort of remote package address, and so for most errors - // we'll detect them only during module installation. There are - // still a _few_ purely-syntax errors we can catch at parsing time, - // though, mostly related to remote package sub-paths and local - // paths. - switch err := err.(type) { - case *getmodules.MaybeRelativePathErr: - diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid module source address", - Detail: fmt.Sprintf( - "Terraform failed to determine your intended installation method for remote module package %q.\n\nIf you intended this as a path relative to the current module, use \"./%s\" instead. The \"./\" prefix indicates that the address is a relative filesystem path.", - err.Addr, err.Addr, - ), - Subject: mc.SourceAddrRange.Ptr(), - }) - default: - diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid module source address", - Detail: fmt.Sprintf("Failed to parse module source address: %s.", err), - Subject: mc.SourceAddrRange.Ptr(), - }) + if !valDiags.HasErrors() { + addr, err := addrs.ParseModuleSource(mc.SourceAddrRaw) + mc.SourceAddr = addr + if err != nil { + // NOTE: In practice it's actually very unlikely to end up here, + // because our source address parser can turn just about any string + // into some sort of remote package address, and so for most errors + // we'll detect them only during module installation. There are + // still a _few_ purely-syntax errors we can catch at parsing time, + // though, mostly related to remote package sub-paths and local + // paths. + switch err := err.(type) { + case *getmodules.MaybeRelativePathErr: + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid module source address", + Detail: fmt.Sprintf( + "Terraform failed to determine your intended installation method for remote module package %q.\n\nIf you intended this as a path relative to the current module, use \"./%s\" instead. The \"./\" prefix indicates that the address is a relative filesystem path.", + err.Addr, err.Addr, + ), + Subject: mc.SourceAddrRange.Ptr(), + }) + default: + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid module source address", + Detail: fmt.Sprintf("Failed to parse module source address: %s.", err), + Subject: mc.SourceAddrRange.Ptr(), + }) + } } } + // NOTE: We leave mc.SourceAddr as nil for any situation where the + // source attribute is invalid, so any code which tries to carefully + // use the partial result of a failed config decode must be + // resilient to that. } if attr, exists := content.Attributes["version"]; exists { @@ -196,6 +201,19 @@ func decodeModuleBlock(block *hcl.Block, override bool) (*ModuleCall, hcl.Diagno return mc, diags } +// EntersNewPackage returns true if this call is to an external module, either +// directly via a remote source address or indirectly via a registry source +// address. +// +// Other behaviors in Terraform may treat package crossings as a special +// situation, because that indicates that the caller and callee can change +// independently of one another and thus we should disallow using any features +// where the caller assumes anything about the callee other than its input +// variables, required provider configurations, and output values. +func (mc *ModuleCall) EntersNewPackage() bool { + return moduleSourceAddrEntersNewPackage(mc.SourceAddr) +} + // PassedProviderConfig represents a provider config explicitly passed down to // a child module, possibly giving it a new local address in the process. type PassedProviderConfig struct { @@ -234,3 +252,27 @@ var moduleBlockSchema = &hcl.BodySchema{ {Type: "provider", LabelNames: []string{"type"}}, }, } + +func moduleSourceAddrEntersNewPackage(addr addrs.ModuleSource) bool { + switch addr.(type) { + case nil: + // There are only two situations where we should get here: + // - We've been asked about the source address of the root module, + // which is always nil. + // - We've been asked about a ModuleCall that is part of the partial + // result of a failed decode. + // The root module exists outside of all module packages, so we'll + // just return false for that case. For the error case it doesn't + // really matter what we return as long as we don't panic, because + // we only make a best-effort to allow careful inspection of objects + // representing invalid configuration. + return false + case addrs.ModuleSourceLocal: + // Local source addresses are the only address type that remains within + // the same package. + return false + default: + // All other address types enter a new package. + return true + } +} diff --git a/internal/configs/module_call_test.go b/internal/configs/module_call_test.go index a652bebb6..eaf923739 100644 --- a/internal/configs/module_call_test.go +++ b/internal/configs/module_call_test.go @@ -142,3 +142,49 @@ func TestLoadModuleCall(t *testing.T) { t.Error(problem) } } + +func TestModuleSourceAddrEntersNewPackage(t *testing.T) { + tests := []struct { + Addr string + Want bool + }{ + { + "./", + false, + }, + { + "../bork", + false, + }, + { + "/absolute/path", + true, + }, + { + "github.com/example/foo", + true, + }, + { + "hashicorp/subnets/cidr", // registry module + true, + }, + { + "registry.terraform.io/hashicorp/subnets/cidr", // registry module + true, + }, + } + + for _, test := range tests { + t.Run(test.Addr, func(t *testing.T) { + addr, err := addrs.ParseModuleSource(test.Addr) + if err != nil { + t.Fatalf("parsing failed for %q: %s", test.Addr, err) + } + + got := moduleSourceAddrEntersNewPackage(addr) + if got != test.Want { + t.Errorf("wrong result for %q\ngot: %#v\nwant: %#v", addr, got, test.Want) + } + }) + } +}