From 4a82628ef2e5e954fbfe83b0f51e6b89e8fc5b87 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Fri, 5 Apr 2024 18:04:43 -0700 Subject: [PATCH 1/3] experiments: template_string_func experiment keyword This doesn't actually do anything yet, but we'll make it do something in a future commit. --- internal/experiments/experiment.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/experiments/experiment.go b/internal/experiments/experiment.go index 486fda368597..7fa4b902f0e6 100644 --- a/internal/experiments/experiment.go +++ b/internal/experiments/experiment.go @@ -20,6 +20,7 @@ const ( VariableValidationCrossRef = Experiment("variable_validation_crossref") ModuleVariableOptionalAttrs = Experiment("module_variable_optional_attrs") SuppressProviderSensitiveAttrs = Experiment("provider_sensitive_attrs") + TemplateStringFunc = Experiment("template_string_func") ConfigDrivenMove = Experiment("config_driven_move") PreconditionsPostconditions = Experiment("preconditions_postconditions") UnknownInstances = Experiment("unknown_instances") @@ -32,6 +33,7 @@ func init() { registerConcludedExperiment(VariableValidation, "Custom variable validation can now be used by default, without enabling an experiment.") registerCurrentExperiment(VariableValidationCrossRef) registerConcludedExperiment(SuppressProviderSensitiveAttrs, "Provider-defined sensitive attributes are now redacted by default, without enabling an experiment.") + registerCurrentExperiment(TemplateStringFunc) registerConcludedExperiment(ConfigDrivenMove, "Declarations of moved resource instances using \"moved\" blocks can now be used by default, without enabling an experiment.") registerConcludedExperiment(PreconditionsPostconditions, "Condition blocks can now be used by default, without enabling an experiment.") registerConcludedExperiment(ModuleVariableOptionalAttrs, "The final feature corresponding to this experiment differs from the experimental form and is available in the Terraform language from Terraform v1.3.0 onwards.") From cce293cc36a1d9c9a85eaa9c29dfe27b72ce23b7 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Fri, 5 Apr 2024 19:10:33 -0700 Subject: [PATCH 2/3] collections: NewSetCmp takes optional initial elements We previously updated the other two constructors to work this way, but missed this one. NewSetCmp is now consistent with NewSet and NewSetFunc. --- internal/collections/set.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/collections/set.go b/internal/collections/set.go index 2cb30ee672c8..5c583dada659 100644 --- a/internal/collections/set.go +++ b/internal/collections/set.go @@ -42,8 +42,8 @@ func NewSetFunc[T any](keyFunc func(T) UniqueKey[T], elems ...T) Set[T] { // NewSetCmp constructs a new set for any comparable type, using the built-in // == operator as the definition of element equivalence. -func NewSetCmp[T comparable]() Set[T] { - return NewSetFunc(cmpUniqueKeyFunc[T]) +func NewSetCmp[T comparable](elems ...T) Set[T] { + return NewSetFunc(cmpUniqueKeyFunc[T], elems...) } // Has returns true if the given value is present in the set, or false From bbea9641d472dcf2acc75c9a346e08a7930ee29b Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Mon, 8 Apr 2024 08:40:25 -0700 Subject: [PATCH 3/3] lang/funcs: Experimental "templatestring" function This function complements the existing "templatefile" to deal with the unusual situation of rendering a template that comes from somewhere outside of the current module's source code, such as from a data resource result. We have some historical experience with the now-deprecated hashicorp/template provider and its template_file data source, where we found that new authors would find it via web search and assume it was "the way" to render templates in Terraform, and then get frustrated dealing with the confusing situation of writing a string template that generates another string template for a second round of template rendering. To try to support those who have this unusual need without creating another attractive nuisance that would derail new authors, this function imposes the artificial extra rule that its template argument may only be populated using a single reference to a symbol defined elsewhere in the same module. This is intended to entice folks trying to use this function for something other than its intended purpose to refer to its documentation (once written) and then hopefully learn what other Terraform language feature they ought to have used instead. The syntax restriction only goes one level deep, so particularly-determined authors can still intentionally misuse this function by adding one level of indirection, such as by building template source code in a local value and then passing that local value as the template argument. The restriction is in place only to reduce the chances of someone _misunderstanding_ the purpose of this function; we don't intend to prevent someone from actively deciding to misuse it, if they have a good reason to do so. This new function inherits the same restriction as templatefile where it does not allow recursively calling other template-rendering functions. This is to dissuade from trying to use Terraform templates "at large", since Terraform's template language is not designed for such uses. It would be better to build a Terraform provider that wraps a more featureful template system like Gonja if someone really does need advanced templating, beyond Terraform's basic goals of being able to build small configuration files, etc. Because this function's intended purpose is rendering templates obtained from elsewhere, this function also blocks calls to any of Terraform's functions that would read from the filesystem of the computer where Terraform is running. This is a small additional measure of isolation to reduce the risk of an attacker somehow modifying a dynamically-fetched template to inspire Terraform to write sensitive data from the host computer into a location accessible to the same attacker, or similar. This is currently only a language experiment and so will not yet be available in stable releases of Terraform. Before stabilizing this and committing to supporting it indefinitely we'll want to gather feedback on whether this function actually meets the intended narrow set of use-cases around dynamic template rendering. --- internal/lang/funcs/descriptions.go | 7 + internal/lang/funcs/filesystem.go | 90 ++----- internal/lang/funcs/filesystem_test.go | 26 ++- internal/lang/funcs/string.go | 232 ++++++++++++++++++ internal/lang/funcs/string_test.go | 310 +++++++++++++++++++++++++ internal/lang/functions.go | 60 ++++- 6 files changed, 633 insertions(+), 92 deletions(-) diff --git a/internal/lang/funcs/descriptions.go b/internal/lang/funcs/descriptions.go index cfe529b1b0aa..1967793ea794 100644 --- a/internal/lang/funcs/descriptions.go +++ b/internal/lang/funcs/descriptions.go @@ -420,6 +420,13 @@ var DescriptionList = map[string]descriptionEntry{ Description: "`templatefile` reads the file at the given path and renders its content as a template using a supplied set of template variables.", ParamDescription: []string{"", ""}, }, + "templatestring": { + Description: "`templatestring` takes a string from elsewhere in the module and renders its content as a template using a supplied set of template variables.", + ParamDescription: []string{ + "a simple reference to a string value containing the template source code", + "object of variables to expose in the template scope", + }, + }, "textdecodebase64": { Description: "`textdecodebase64` function decodes a string that was previously Base64-encoded, and then interprets the result as characters in a specified character encoding.", ParamDescription: []string{"", ""}, diff --git a/internal/lang/funcs/filesystem.go b/internal/lang/funcs/filesystem.go index 5447d698ebb0..4092cd504607 100644 --- a/internal/lang/funcs/filesystem.go +++ b/internal/lang/funcs/filesystem.go @@ -6,7 +6,7 @@ package funcs import ( "encoding/base64" "fmt" - "io/ioutil" + "io" "os" "path/filepath" "unicode/utf8" @@ -17,6 +17,8 @@ import ( homedir "github.com/mitchellh/go-homedir" "github.com/zclconf/go-cty/cty" "github.com/zclconf/go-cty/cty/function" + + "github.com/hashicorp/terraform/internal/collections" ) // MakeFileFunc constructs a function that takes a file path and returns the @@ -69,20 +71,7 @@ func MakeFileFunc(baseDir string, encBase64 bool) function.Function { // As a special exception, a referenced template file may not recursively call // the templatefile function, since that would risk the same file being // included into itself indefinitely. -func MakeTemplateFileFunc(baseDir string, funcsCb func() map[string]function.Function) function.Function { - - params := []function.Parameter{ - { - Name: "path", - Type: cty.String, - AllowMarked: true, - }, - { - Name: "vars", - Type: cty.DynamicPseudoType, - }, - } - +func MakeTemplateFileFunc(baseDir string, funcsCb func() (funcs map[string]function.Function, fsFuncs collections.Set[string], templateFuncs collections.Set[string])) function.Function { loadTmpl := func(fn string, marks cty.ValueMarks) (hcl.Expression, error) { // We re-use File here to ensure the same filename interpretation // as it does, along with its other safety checks. @@ -99,65 +88,20 @@ func MakeTemplateFileFunc(baseDir string, funcsCb func() map[string]function.Fun return expr, nil } - renderTmpl := func(expr hcl.Expression, varsVal cty.Value) (cty.Value, error) { - if varsTy := varsVal.Type(); !(varsTy.IsMapType() || varsTy.IsObjectType()) { - return cty.DynamicVal, function.NewArgErrorf(1, "invalid vars value: must be a map") // or an object, but we don't strongly distinguish these most of the time - } - - ctx := &hcl.EvalContext{ - Variables: varsVal.AsValueMap(), - } - - // We require all of the variables to be valid HCL identifiers, because - // otherwise there would be no way to refer to them in the template - // anyway. Rejecting this here gives better feedback to the user - // than a syntax error somewhere in the template itself. - for n := range ctx.Variables { - if !hclsyntax.ValidIdentifier(n) { - // This error message intentionally doesn't describe _all_ of - // the different permutations that are technically valid as an - // HCL identifier, but rather focuses on what we might - // consider to be an "idiomatic" variable name. - return cty.DynamicVal, function.NewArgErrorf(1, "invalid template variable name %q: must start with a letter, followed by zero or more letters, digits, and underscores", n) - } - } - - // We'll pre-check references in the template here so we can give a - // more specialized error message than HCL would by default, so it's - // clearer that this problem is coming from a templatefile call. - for _, traversal := range expr.Variables() { - root := traversal.RootName() - if _, ok := ctx.Variables[root]; !ok { - return cty.DynamicVal, function.NewArgErrorf(1, "vars map does not contain key %q, referenced at %s", root, traversal[0].SourceRange()) - } - } - - givenFuncs := funcsCb() // this callback indirection is to avoid chicken/egg problems - funcs := make(map[string]function.Function, len(givenFuncs)) - for name, fn := range givenFuncs { - if name == "templatefile" || name == "core::templatefile" { - // We stub this one out to prevent recursive calls. - funcs[name] = function.New(&function.Spec{ - Params: params, - Type: func(args []cty.Value) (cty.Type, error) { - return cty.NilType, fmt.Errorf("cannot recursively call templatefile from inside templatefile call") - }, - }) - continue - } - funcs[name] = fn - } - ctx.Functions = funcs - - val, diags := expr.Value(ctx) - if diags.HasErrors() { - return cty.DynamicVal, diags - } - return val, nil - } + renderTmpl := makeRenderTemplateFunc(funcsCb, true) return function.New(&function.Spec{ - Params: params, + Params: []function.Parameter{ + { + Name: "path", + Type: cty.String, + AllowMarked: true, + }, + { + Name: "vars", + Type: cty.DynamicPseudoType, + }, + }, Type: func(args []cty.Value) (cty.Type, error) { if !(args[0].IsKnown() && args[1].IsKnown()) { return cty.DynamicPseudoType, nil @@ -426,7 +370,7 @@ func readFileBytes(baseDir, path string, marks cty.ValueMarks) ([]byte, error) { } defer f.Close() - src, err := ioutil.ReadAll(f) + src, err := io.ReadAll(f) if err != nil { return nil, fmt.Errorf("failed to read file: %w", err) } diff --git a/internal/lang/funcs/filesystem_test.go b/internal/lang/funcs/filesystem_test.go index e54eae6ce33d..cab8f2797615 100644 --- a/internal/lang/funcs/filesystem_test.go +++ b/internal/lang/funcs/filesystem_test.go @@ -9,11 +9,13 @@ import ( "path/filepath" "testing" - "github.com/hashicorp/terraform/internal/lang/marks" homedir "github.com/mitchellh/go-homedir" "github.com/zclconf/go-cty/cty" "github.com/zclconf/go-cty/cty/function" "github.com/zclconf/go-cty/cty/function/stdlib" + + "github.com/hashicorp/terraform/internal/collections" + "github.com/hashicorp/terraform/internal/lang/marks" ) func TestFile(t *testing.T) { @@ -149,13 +151,13 @@ func TestTemplateFile(t *testing.T) { cty.StringVal("testdata/recursive.tmpl"), cty.MapValEmpty(cty.String), cty.NilVal, - `testdata/recursive.tmpl:1,3-16: Error in function call; Call to function "templatefile" failed: cannot recursively call templatefile from inside templatefile call.`, + `testdata/recursive.tmpl:1,3-16: Error in function call; Call to function "templatefile" failed: cannot recursively call templatefile from inside another template function.`, }, { cty.StringVal("testdata/recursive_namespaced.tmpl"), cty.MapValEmpty(cty.String), cty.NilVal, - `testdata/recursive_namespaced.tmpl:1,3-22: Error in function call; Call to function "core::templatefile" failed: cannot recursively call templatefile from inside templatefile call.`, + `testdata/recursive_namespaced.tmpl:1,3-22: Error in function call; Call to function "core::templatefile" failed: cannot recursively call templatefile from inside another template function.`, }, { cty.StringVal("testdata/list.tmpl"), @@ -187,14 +189,16 @@ func TestTemplateFile(t *testing.T) { }, } - templateFileFn := MakeTemplateFileFunc(".", func() map[string]function.Function { - return map[string]function.Function{ - "join": stdlib.JoinFunc, - "core::join": stdlib.JoinFunc, - "templatefile": MakeFileFunc(".", false), // just a placeholder, since templatefile itself overrides this - "core::templatefile": MakeFileFunc(".", false), // just a placeholder, since templatefile itself overrides this - } - }) + funcs := map[string]function.Function{ + "join": stdlib.JoinFunc, + "core::join": stdlib.JoinFunc, + } + funcsFunc := func() (funcTable map[string]function.Function, fsFuncs collections.Set[string], templateFuncs collections.Set[string]) { + return funcs, collections.NewSetCmp[string](), collections.NewSetCmp[string]("templatefile") + } + templateFileFn := MakeTemplateFileFunc(".", funcsFunc) + funcs["templatefile"] = templateFileFn + funcs["core::templatefile"] = templateFileFn for _, test := range tests { t.Run(fmt.Sprintf("TemplateFile(%#v, %#v)", test.Path, test.Vars), func(t *testing.T) { diff --git a/internal/lang/funcs/string.go b/internal/lang/funcs/string.go index 311ee41ed4d3..33142f2952d4 100644 --- a/internal/lang/funcs/string.go +++ b/internal/lang/funcs/string.go @@ -4,11 +4,18 @@ package funcs import ( + "fmt" "regexp" "strings" + "github.com/hashicorp/hcl/v2" + "github.com/hashicorp/hcl/v2/ext/customdecode" + "github.com/hashicorp/hcl/v2/hclsyntax" "github.com/zclconf/go-cty/cty" + "github.com/zclconf/go-cty/cty/convert" "github.com/zclconf/go-cty/cty/function" + + "github.com/hashicorp/terraform/internal/collections" ) // StartsWithFunc constructs a function that checks if a string starts with @@ -153,6 +160,231 @@ var StrContainsFunc = function.New(&function.Spec{ }, }) +// TemplateStringFunc renders a template presented either as a literal string +// or as a reference to a string from elsewhere. +func MakeTemplateStringFunc(funcsCb func() (funcs map[string]function.Function, fsFuncs collections.Set[string], templateFuncs collections.Set[string])) function.Function { + return function.New(&function.Spec{ + Params: []function.Parameter{ + { + Name: "template", + Type: customdecode.ExpressionClosureType, + }, + { + Name: "vars", + Type: cty.DynamicPseudoType, + }, + }, + Type: function.StaticReturnType(cty.String), + RefineResult: refineNotNull, + Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) { + templateClosure := customdecode.ExpressionClosureFromVal(args[0]) + varsVal := args[1] + + // Our historical experience with the hashicorp/template provider's + // template_file data source tells us that situations where authors + // must write a string template that generates a string template + // cause all sorts of confusion, because the same syntax ends up + // being evaluated in two different contexts with different variables + // in scope, and new authors tend to be attracted to a function + // named "template" and so miss that the language has built-in + // support for inline template expressions. + // + // As a compromise to try to meet the (relatively unusual) use-cases + // where dynamic template fetching is needed without creating an + // attractive nuisance for those who would be better off just writing + // a plain inline template, this function imposes constraints on how + // the template argument may be provided and thus allows us + // to return slightly more helpful error messages. + // + // The only valid way to provide the template argument is as a + // simple, direct reference to some other value in scope that is + // of type string: + // templatestring(local.greeting_template, { name = "Alex" }) + // + // Those with more unusual desires, such as dynamically generating + // a template at runtime by trying to concatenate template chunks + // together, can still do such things by placing the template + // construction expression in a separate local value and then passing + // that local value to the template argument. But the restriction is + // intended to intentionally add an extra "roadbump" so that + // anyone who mistakenly thinks they need templatestring to render + // an inline template (a common mistake for new authors with + // template_file) will hopefully hit this roadblock and refer to + // the function documentation to learn about the other options that + // are probably more suitable for what they need. + switch expr := templateClosure.Expression.(type) { + case *hclsyntax.ScopeTraversalExpr: + // A standalone traversal is always acceptable. + case *hclsyntax.TemplateWrapExpr: + // This situation occurs when someone writes an interpolation-only + // expression as was required in Terraform v0.11 and earlier. + // Because older versions of Terraform required this and this + // habit has been sticky for some authors, we'll return a + // special error message. + return cty.UnknownVal(retType), function.NewArgErrorf( + 0, "invalid template expression: templatestring is only for rendering templates retrieved dynamically from elsewhere; to treat the inner expression as template syntax, write the reference expression directly without any template interpolation syntax", + ) + case *hclsyntax.TemplateExpr: + // This is the more general case of someone trying to write + // an inline template as the argument. In this case we'll + // distinguish between an entirely-literal template, which + // probably suggests someone was trying to escape their template + // for the function to consume, vs. a template with other + // sequences that suggests someone was just trying to write + // an inline template and so probably doesn't need to call + // this function at all. + literal := true + if len(expr.Parts) != 1 { + literal = false + } else if _, ok := expr.Parts[0].(*hclsyntax.LiteralValueExpr); !ok { + literal = false + } + if literal { + return cty.UnknownVal(retType), function.NewArgErrorf( + 0, "invalid template expression: templatestring is only for rendering templates retrieved dynamically from elsewhere, and so does not support providing a literal template; consider using a template string expression instead", + ) + } else { + return cty.UnknownVal(retType), function.NewArgErrorf( + 0, "invalid template expression: templatestring is only for rendering templates retrieved dynamically from elsewhere; to render an inline template, consider using a plain template string expression", + ) + } + default: + // Nothing else is allowed. + // Someone who really does want to construct a template dynamically + // can factor out that construction into a local value and refer + // to it in the templatestring call, but it's not really feasible + // to explain that clearly in a short error message so we'll deal + // with that option on the function's documentation page instead, + // where we can show a full example. + return cty.UnknownVal(retType), function.NewArgErrorf( + 0, "invalid template expression: must be a direct reference to a single string from elsewhere, containing valid Terraform template syntax", + ) + } + + templateVal, diags := templateClosure.Value() + if diags.HasErrors() { + // With the constraints we imposed above the possible errors + // here are pretty limited: it must be some kind of invalid + // traversal. As usual HCL diagnostics don't make for very + // good function errors but we've already filtered out many + // common reasons for error here, so we should get here pretty + // rarely. + return cty.UnknownVal(retType), function.NewArgErrorf( + 0, "invalid template expression: %s", + diags.Error(), + ) + } + if !templateVal.IsKnown() { + // We'll need to wait until we actually know what the template is. + return cty.UnknownVal(retType), nil + } + if templateVal.Type() != cty.String || templateVal.IsNull() { + // We're being a little stricter than usual here and requiring + // exactly a string, rather than just anything that can convert + // to one. This is because the stringification of a number or + // boolean value cannot be a useful template (it wouldn't have + // any template sequences in it) and so far more likely to be + // a mistake than actually intentional. + return cty.UnknownVal(retType), function.NewArgErrorf( + 0, "invalid template value: a string is required", + ) + } + templateVal, templateMarks := templateVal.Unmark() + templateStr := templateVal.AsString() + expr, diags := hclsyntax.ParseTemplate([]byte(templateStr), "", hcl.Pos{Line: 1, Column: 1}) + if diags.HasErrors() { + return cty.UnknownVal(retType), function.NewArgErrorf( + 0, "invalid template: %s", + diags.Error(), + ) + } + + render := makeRenderTemplateFunc(funcsCb, false) + retVal, err := render(expr, varsVal) + if err != nil { + return cty.UnknownVal(retType), err + } + retVal, err = convert.Convert(retVal, cty.String) + if err != nil { + return cty.UnknownVal(retType), fmt.Errorf("invalid template result: %s", err) + } + return retVal.WithMarks(templateMarks), nil + }, + }) +} + +func makeRenderTemplateFunc(funcsCb func() (funcs map[string]function.Function, fsFuncs collections.Set[string], templateFuncs collections.Set[string]), allowFS bool) func(expr hcl.Expression, varsVal cty.Value) (cty.Value, error) { + return func(expr hcl.Expression, varsVal cty.Value) (cty.Value, error) { + if varsTy := varsVal.Type(); !(varsTy.IsMapType() || varsTy.IsObjectType()) { + return cty.DynamicVal, function.NewArgErrorf(1, "invalid vars value: must be a map") // or an object, but we don't strongly distinguish these most of the time + } + + ctx := &hcl.EvalContext{ + Variables: varsVal.AsValueMap(), + } + + // We require all of the variables to be valid HCL identifiers, because + // otherwise there would be no way to refer to them in the template + // anyway. Rejecting this here gives better feedback to the user + // than a syntax error somewhere in the template itself. + for n := range ctx.Variables { + if !hclsyntax.ValidIdentifier(n) { + // This error message intentionally doesn't describe _all_ of + // the different permutations that are technically valid as an + // HCL identifier, but rather focuses on what we might + // consider to be an "idiomatic" variable name. + return cty.DynamicVal, function.NewArgErrorf(1, "invalid template variable name %q: must start with a letter, followed by zero or more letters, digits, and underscores", n) + } + } + + // We'll pre-check references in the template here so we can give a + // more specialized error message than HCL would by default, so it's + // clearer that this problem is coming from a templatefile call. + for _, traversal := range expr.Variables() { + root := traversal.RootName() + if _, ok := ctx.Variables[root]; !ok { + return cty.DynamicVal, function.NewArgErrorf(1, "vars map does not contain key %q, referenced at %s", root, traversal[0].SourceRange()) + } + } + + givenFuncs, fsFuncs, templateFuncs := funcsCb() // this callback indirection is to avoid chicken/egg problems + funcs := make(map[string]function.Function, len(givenFuncs)) + for name, fn := range givenFuncs { + plainName := strings.TrimPrefix(name, "core::") + switch { + case templateFuncs.Has(plainName): + funcs[name] = function.New(&function.Spec{ + Params: fn.Params(), + VarParam: fn.VarParam(), + Type: func(args []cty.Value) (cty.Type, error) { + return cty.NilType, fmt.Errorf("cannot recursively call %s from inside another template function", plainName) + }, + }) + case !allowFS && fsFuncs.Has(plainName): + // Note: for now this assumes that allowFS is false only for + // the templatestring function, and so mentions that name + // directly in the error message. + funcs[name] = function.New(&function.Spec{ + Params: fn.Params(), + VarParam: fn.VarParam(), + Type: func(args []cty.Value) (cty.Type, error) { + return cty.NilType, fmt.Errorf("cannot use filesystem access functions like %s in templatestring templates; consider passing the function result as a template variable instead", plainName) + }, + }) + default: + funcs[name] = fn + } + } + ctx.Functions = funcs + + val, diags := expr.Value(ctx) + if diags.HasErrors() { + return cty.DynamicVal, diags + } + return val, nil + } +} + // Replace searches a given string for another given substring, // and replaces all occurences with a given replacement string. func Replace(str, substr, replace cty.Value) (cty.Value, error) { diff --git a/internal/lang/funcs/string_test.go b/internal/lang/funcs/string_test.go index b66c2611d7c2..c0de6b3239dd 100644 --- a/internal/lang/funcs/string_test.go +++ b/internal/lang/funcs/string_test.go @@ -5,9 +5,16 @@ package funcs import ( "fmt" + "strings" "testing" + "github.com/hashicorp/hcl/v2" + "github.com/hashicorp/hcl/v2/ext/customdecode" + "github.com/hashicorp/hcl/v2/hclsyntax" "github.com/zclconf/go-cty/cty" + "github.com/zclconf/go-cty/cty/function" + + "github.com/hashicorp/terraform/internal/collections" ) func TestReplace(t *testing.T) { @@ -253,3 +260,306 @@ func TestStartsWith(t *testing.T) { }) } } + +func TestTemplateString(t *testing.T) { + // This function has some special restrictions on what syntax is valid + // in its first argument, so we'll test this one using HCL expressions + // as the inputs, rather than direct cty values as we do for most other + // functions in this package. + tests := []struct { + templateExpr string + exprScope map[string]cty.Value + vars cty.Value + want cty.Value + wantErr string + }{ + { + `template`, + map[string]cty.Value{ + "template": cty.StringVal(`it's ${a}`), + }, + cty.ObjectVal(map[string]cty.Value{ + "a": cty.StringVal("a value"), + }), + cty.StringVal(`it's a value`), + ``, + }, + { + `template`, + map[string]cty.Value{ + "template": cty.StringVal(`${a}`), + }, + cty.ObjectVal(map[string]cty.Value{ + "a": cty.True, + }), + // The special treatment of a template with only a single + // interpolation sequence does not apply to templatestring, because + // we're expecting to be evaluating templates fetched dynamically + // from somewhere else and want to avoid callers needing to deal + // with anything other than string results. + cty.StringVal(`true`), + ``, + }, + { + `template`, + map[string]cty.Value{ + "template": cty.StringVal(`${a}`), + }, + cty.ObjectVal(map[string]cty.Value{ + "a": cty.EmptyTupleVal, + }), + // The special treatment of a template with only a single + // interpolation sequence does not apply to templatestring, because + // we're expecting to be evaluating templates fetched dynamically + // from somewhere else and want to avoid callers needing to deal + // with anything other than string results. + cty.NilVal, + `invalid template result: string required`, + }, + { + `data.whatever.whatever["foo"].result`, + map[string]cty.Value{ + "data": cty.ObjectVal(map[string]cty.Value{ + "whatever": cty.ObjectVal(map[string]cty.Value{ + "whatever": cty.MapVal(map[string]cty.Value{ + "foo": cty.ObjectVal(map[string]cty.Value{ + "result": cty.StringVal("it's ${a}"), + }), + }), + }), + }), + }, + cty.ObjectVal(map[string]cty.Value{ + "a": cty.StringVal("a value"), + }), + cty.StringVal(`it's a value`), + ``, + }, + { + `"can't write $${not_allowed}"`, + map[string]cty.Value{}, + cty.ObjectVal(map[string]cty.Value{ + "not_allowed": cty.StringVal("a literal template"), + }), + cty.NilVal, + `invalid template expression: templatestring is only for rendering templates retrieved dynamically from elsewhere, and so does not support providing a literal template; consider using a template string expression instead`, + }, + { + `"can't write ${not_allowed}"`, + map[string]cty.Value{}, + cty.ObjectVal(map[string]cty.Value{ + "not_allowed": cty.StringVal("a literal template"), + }), + cty.NilVal, + `invalid template expression: templatestring is only for rendering templates retrieved dynamically from elsewhere; to render an inline template, consider using a plain template string expression`, + }, + { + `"can't write %%{for x in things}a literal template%%{endfor}"`, + map[string]cty.Value{}, + cty.ObjectVal(map[string]cty.Value{ + "things": cty.ListVal([]cty.Value{cty.True}), + }), + cty.NilVal, + `invalid template expression: templatestring is only for rendering templates retrieved dynamically from elsewhere, and so does not support providing a literal template; consider using a template string expression instead`, + }, + { + `"can't write %{for x in things}a literal template%{endfor}"`, + map[string]cty.Value{}, + cty.ObjectVal(map[string]cty.Value{ + "things": cty.ListVal([]cty.Value{cty.True}), + }), + cty.NilVal, + `invalid template expression: templatestring is only for rendering templates retrieved dynamically from elsewhere; to render an inline template, consider using a plain template string expression`, + }, + { + `"${not_allowed}"`, + map[string]cty.Value{}, + cty.ObjectVal(map[string]cty.Value{ + "not allowed": cty.StringVal("an interp-only template"), + }), + cty.NilVal, + `invalid template expression: templatestring is only for rendering templates retrieved dynamically from elsewhere; to treat the inner expression as template syntax, write the reference expression directly without any template interpolation syntax`, + }, + { + `1 + 1`, + map[string]cty.Value{}, + cty.ObjectVal(map[string]cty.Value{}), + cty.NilVal, + `invalid template expression: must be a direct reference to a single string from elsewhere, containing valid Terraform template syntax`, + }, + { + `not_a_string`, + map[string]cty.Value{ + "not_a_string": cty.True, + }, + cty.ObjectVal(map[string]cty.Value{}), + cty.NilVal, + `invalid template value: a string is required`, + }, + { + `with_lower`, + map[string]cty.Value{ + "with_lower": cty.StringVal(`it's ${lower(a)}`), + }, + cty.ObjectVal(map[string]cty.Value{ + "a": cty.StringVal("A VALUE"), + }), + cty.StringVal("it's a value"), + ``, + }, + { + `with_core_lower`, + map[string]cty.Value{ + "with_core_lower": cty.StringVal(`it's ${core::lower(a)}`), + }, + cty.ObjectVal(map[string]cty.Value{ + "a": cty.StringVal("A VALUE"), + }), + cty.StringVal("it's a value"), + ``, + }, + { + `with_fsfunc`, + map[string]cty.Value{ + "with_fsfunc": cty.StringVal(`it's ${fsfunc()}`), + }, + cty.ObjectVal(map[string]cty.Value{}), + cty.NilVal, + `:1,8-15: Error in function call; Call to function "fsfunc" failed: cannot use filesystem access functions like fsfunc in templatestring templates; consider passing the function result as a template variable instead.`, + }, + { + `with_core_fsfunc`, + map[string]cty.Value{ + "with_core_fsfunc": cty.StringVal(`it's ${core::fsfunc()}`), + }, + cty.ObjectVal(map[string]cty.Value{}), + cty.NilVal, + `:1,8-21: Error in function call; Call to function "core::fsfunc" failed: cannot use filesystem access functions like fsfunc in templatestring templates; consider passing the function result as a template variable instead.`, + }, + { + `with_templatefunc`, + map[string]cty.Value{ + "with_templatefunc": cty.StringVal(`it's ${templatefunc()}`), + }, + cty.ObjectVal(map[string]cty.Value{}), + cty.NilVal, + `:1,8-21: Error in function call; Call to function "templatefunc" failed: cannot recursively call templatefunc from inside another template function.`, + }, + { + `with_core_templatefunc`, + map[string]cty.Value{ + "with_core_templatefunc": cty.StringVal(`it's ${core::templatefunc()}`), + }, + cty.ObjectVal(map[string]cty.Value{}), + cty.NilVal, + `:1,8-27: Error in function call; Call to function "core::templatefunc" failed: cannot recursively call templatefunc from inside another template function.`, + }, + { + `with_fstemplatefunc`, + map[string]cty.Value{ + "with_fstemplatefunc": cty.StringVal(`it's ${fstemplatefunc()}`), + }, + cty.ObjectVal(map[string]cty.Value{}), + cty.NilVal, + // The template function error takes priority over the filesystem + // function error if calling a function that's in both categories. + `:1,8-23: Error in function call; Call to function "fstemplatefunc" failed: cannot recursively call fstemplatefunc from inside another template function.`, + }, + { + `with_core_fstemplatefunc`, + map[string]cty.Value{ + "with_core_fstemplatefunc": cty.StringVal(`it's ${core::fstemplatefunc()}`), + }, + cty.ObjectVal(map[string]cty.Value{}), + cty.NilVal, + // The template function error takes priority over the filesystem + // function error if calling a function that's in both categories. + `:1,8-29: Error in function call; Call to function "core::fstemplatefunc" failed: cannot recursively call fstemplatefunc from inside another template function.`, + }, + } + + funcToTest := MakeTemplateStringFunc(func() (funcs map[string]function.Function, fsFuncs collections.Set[string], templateFuncs collections.Set[string]) { + // These are the functions available for use inside the nested template + // evaluation context. These are here only to test that we can call + // functions and that the template/filesystem functions get blocked + // with suitable error messages. This is not a realistic set of + // functions that would be available in a real call. + funcs = map[string]function.Function{ + "lower": function.New(&function.Spec{ + Params: []function.Parameter{ + { + Name: "str", + Type: cty.String, + }, + }, + Type: function.StaticReturnType(cty.String), + Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) { + s := args[0].AsString() + return cty.StringVal(strings.ToLower(s)), nil + }, + }), + "fsfunc": function.New(&function.Spec{ + Type: function.StaticReturnType(cty.String), + Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) { + return cty.UnknownVal(retType), fmt.Errorf("should not be able to call fsfunc") + }, + }), + "templatefunc": function.New(&function.Spec{ + Type: function.StaticReturnType(cty.String), + Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) { + return cty.UnknownVal(retType), fmt.Errorf("should not be able to call templatefunc") + }, + }), + "fstemplatefunc": function.New(&function.Spec{ + Type: function.StaticReturnType(cty.String), + Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) { + return cty.UnknownVal(retType), fmt.Errorf("should not be able to call fstemplatefunc") + }, + }), + } + funcs["core::lower"] = funcs["lower"] + funcs["core::fsfunc"] = funcs["fsfunc"] + funcs["core::templatefunc"] = funcs["templatefunc"] + funcs["core::fstemplatefunc"] = funcs["fstemplatefunc"] + return funcs, collections.NewSetCmp("fsfunc", "fstemplatefunc"), collections.NewSetCmp("templatefunc", "fstemplatefunc") + }) + + for _, test := range tests { + t.Run(test.templateExpr, func(t *testing.T) { + // The following mimics what HCL itself would do when preparing + // the first argument to this function, since the parameter + // uses the special "expression closure type" which causes + // HCL to delay evaluation of the expression and let the + // function handle it directly itself. + expr, diags := hclsyntax.ParseExpression([]byte(test.templateExpr), "", hcl.InitialPos) + if diags.HasErrors() { + t.Fatalf("unexpected errors: %s", diags.Error()) + } + exprClosure := &customdecode.ExpressionClosure{ + Expression: expr, + EvalContext: &hcl.EvalContext{ + Variables: test.exprScope, + }, + } + exprClosureVal := customdecode.ExpressionClosureVal(exprClosure) + + got, gotErr := funcToTest.Call([]cty.Value{exprClosureVal, test.vars}) + + if test.wantErr != "" { + if gotErr == nil { + t.Fatalf("unexpected success\ngot: %#v\nwant error: %s", got, test.wantErr) + } + if got, want := gotErr.Error(), test.wantErr; got != want { + t.Fatalf("wrong error\ngot: %s\nwant: %s", got, want) + } + return + } + if gotErr != nil { + t.Errorf("unexpected error: %s", gotErr.Error()) + } + if !test.want.RawEquals(got) { + t.Errorf("wrong result\ngot: %#v\nwant: %#v", got, test.want) + } + }) + } +} diff --git a/internal/lang/functions.go b/internal/lang/functions.go index 10c72981a66b..f811f0b503a4 100644 --- a/internal/lang/functions.go +++ b/internal/lang/functions.go @@ -12,6 +12,7 @@ import ( "github.com/zclconf/go-cty/cty/function" "github.com/zclconf/go-cty/cty/function/stdlib" + "github.com/hashicorp/terraform/internal/collections" "github.com/hashicorp/terraform/internal/experiments" "github.com/hashicorp/terraform/internal/lang/funcs" ) @@ -22,6 +23,32 @@ var impureFunctions = []string{ "uuid", } +// filesystemFunctions are the functions that allow interacting with arbitrary +// paths in the local filesystem, and which can therefore have their results +// vary based on something other than their arguments, and might allow template +// rendering to expose details about the system where Terraform is running. +var filesystemFunctions = collections.NewSetCmp[string]( + "file", + "fileexists", + "fileset", + "filebase64", + "filebase64sha256", + "filebase64sha512", + "filemd5", + "filesha1", + "filesha256", + "filesha512", + "templatefile", +) + +// templateFunctions are functions that render nested templates. These are +// callable from module code but not from within the templates they are +// rendering. +var templateFunctions = collections.NewSetCmp[string]( + "templatefile", + "templatestring", +) + // Functions returns the set of functions that should be used to when evaluating // expressions in the receiving scope. func (s *Scope) Functions() map[string]function.Function { @@ -29,6 +56,10 @@ func (s *Scope) Functions() map[string]function.Function { if s.funcs == nil { s.funcs = baseFunctions(s.BaseDir) + // If you're adding something here, please consider whether it meets + // the criteria for either or both of the sets [filesystemFunctions] + // and [templateFunctions] and add it there if so, to ensure that + // functions relying on those classifications will behave correctly. coreFuncs := map[string]function.Function{ "abs": stdlib.AbsoluteFunc, "abspath": funcs.AbsPathFunc, @@ -148,12 +179,20 @@ func (s *Scope) Functions() map[string]function.Function { "zipmap": stdlib.ZipmapFunc, } - coreFuncs["templatefile"] = funcs.MakeTemplateFileFunc(s.BaseDir, func() map[string]function.Function { - // The templatefile function prevents recursive calls to itself - // by copying this map and overwriting the "templatefile" and - // "core:templatefile" entries. - return s.funcs - }) + // Our two template-rendering functions want to be able to call + // all of the other functions themselves, but we pass them indirectly + // via a callback to avoid chicken/egg problems while initializing + // the functions table. + funcsFunc := func() (funcs map[string]function.Function, fsFuncs collections.Set[string], templateFuncs collections.Set[string]) { + // The templatefile and templatestring functions prevent recursive + // calls to themselves and each other by copying this map and + // overwriting the relevant entries. + return s.funcs, filesystemFunctions, templateFunctions + } + coreFuncs["templatefile"] = funcs.MakeTemplateFileFunc(s.BaseDir, funcsFunc) + if s.activeExperiments.Has(experiments.TemplateStringFunc) { + coreFuncs["templatestring"] = funcs.MakeTemplateStringFunc(funcsFunc) + } if s.ConsoleMode { // The type function is only available in terraform console. @@ -228,6 +267,11 @@ func baseFunctions(baseDir string) map[string]function.Function { // in the "funcs" directory and potentially graduate to cty stdlib // later if the functionality seems to be something domain-agnostic // that would be useful to all applications using cty functions. + // + // If you're adding something here, please consider whether it meets + // the criteria for either or both of the sets [filesystemFunctions] + // and [templateFunctions] and add it there if so, to ensure that + // functions relying on those classifications will behave correctly. fs := map[string]function.Function{ "abs": stdlib.AbsoluteFunc, "abspath": funcs.AbsPathFunc, @@ -347,10 +391,10 @@ func baseFunctions(baseDir string) map[string]function.Function { "zipmap": stdlib.ZipmapFunc, } - fs["templatefile"] = funcs.MakeTemplateFileFunc(baseDir, func() map[string]function.Function { + fs["templatefile"] = funcs.MakeTemplateFileFunc(baseDir, func() (map[string]function.Function, collections.Set[string], collections.Set[string]) { // The templatefile function prevents recursive calls to itself // by copying this map and overwriting the "templatefile" entry. - return fs + return fs, filesystemFunctions, templateFunctions }) return fs