From e56cac7ad63e7c354d5ccb2ca2da755894c85823 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 3 Jan 2025 10:34:59 +0000 Subject: [PATCH 1/4] fix(provisioner/terraform/tfparse): skip evaluation of defaults if no workspace_tags found --- coderd/templateversions_test.go | 113 +++++++++--- enterprise/coderd/workspaces_test.go | 5 + provisioner/terraform/tfparse/tfparse.go | 43 ++++- provisioner/terraform/tfparse/tfparse_test.go | 165 +++++++++++++++--- 4 files changed, 267 insertions(+), 59 deletions(-) diff --git a/coderd/templateversions_test.go b/coderd/templateversions_test.go index 1a67508880188..b2ec822f998bc 100644 --- a/coderd/templateversions_test.go +++ b/coderd/templateversions_test.go @@ -293,6 +293,11 @@ func TestPostTemplateVersionsByOrganization(t *testing.T) { type = string default = "2" } + data "coder_parameter" "unrelated" { + name = "unrelated" + type = "list(string)" + default = jsonencode(["a", "b"]) + } resource "null_resource" "test" {}`, }, wantTags: map[string]string{"owner": "", "scope": "organization"}, @@ -301,18 +306,23 @@ func TestPostTemplateVersionsByOrganization(t *testing.T) { name: "main.tf with empty workspace tags", files: map[string]string{ `main.tf`: ` - variable "a" { - type = string - default = "1" - } - data "coder_parameter" "b" { - type = string - default = "2" - } - resource "null_resource" "test" {} - data "coder_workspace_tags" "tags" { - tags = {} - }`, + variable "a" { + type = string + default = "1" + } + data "coder_parameter" "b" { + type = string + default = "2" + } + data "coder_parameter" "unrelated" { + name = "unrelated" + type = "list(string)" + default = jsonencode(["a", "b"]) + } + resource "null_resource" "test" {} + data "coder_workspace_tags" "tags" { + tags = {} + }`, }, wantTags: map[string]string{"owner": "", "scope": "organization"}, }, @@ -328,6 +338,11 @@ func TestPostTemplateVersionsByOrganization(t *testing.T) { type = string default = "2" } + data "coder_parameter" "unrelated" { + name = "unrelated" + type = "list(string)" + default = jsonencode(["a", "b"]) + } resource "null_resource" "test" {} data "coder_workspace_tags" "tags" { tags = { @@ -343,22 +358,28 @@ func TestPostTemplateVersionsByOrganization(t *testing.T) { name: "main.tf with workspace tags and request tags", files: map[string]string{ `main.tf`: ` - variable "a" { - type = string - default = "1" - } - data "coder_parameter" "b" { - type = string - default = "2" - } - resource "null_resource" "test" {} - data "coder_workspace_tags" "tags" { - tags = { - "foo": "bar", - "a": var.a, - "b": data.coder_parameter.b.value, + // This file is the same as the above, except for this comment. + variable "a" { + type = string + default = "1" + } + data "coder_parameter" "b" { + type = string + default = "2" } - }`, + data "coder_parameter" "unrelated" { + name = "unrelated" + type = "list(string)" + default = jsonencode(["a", "b"]) + } + resource "null_resource" "test" {} + data "coder_workspace_tags" "tags" { + tags = { + "foo": "bar", + "a": var.a, + "b": data.coder_parameter.b.value, + } + }`, }, reqTags: map[string]string{"baz": "zap", "foo": "noclobber"}, wantTags: map[string]string{"owner": "", "scope": "organization", "foo": "bar", "baz": "zap", "a": "1", "b": "2"}, @@ -375,6 +396,11 @@ func TestPostTemplateVersionsByOrganization(t *testing.T) { type = string default = "2" } + data "coder_parameter" "unrelated" { + name = "unrelated" + type = "list(string)" + default = jsonencode(["a", "b"]) + } resource "null_resource" "test" { name = "foo" } @@ -401,6 +427,11 @@ func TestPostTemplateVersionsByOrganization(t *testing.T) { type = string default = "2" } + data "coder_parameter" "unrelated" { + name = "unrelated" + type = "list(string)" + default = jsonencode(["a", "b"]) + } resource "null_resource" "test" { name = "foo" } @@ -423,6 +454,11 @@ func TestPostTemplateVersionsByOrganization(t *testing.T) { name: "main.tf with workspace tags that attempts to set user scope", files: map[string]string{ `main.tf`: ` + data "coder_parameter" "unrelated" { + name = "unrelated" + type = "list(string)" + default = jsonencode(["a", "b"]) + } resource "null_resource" "test" {} data "coder_workspace_tags" "tags" { tags = { @@ -437,6 +473,11 @@ func TestPostTemplateVersionsByOrganization(t *testing.T) { name: "main.tf with workspace tags that attempt to clobber org ID", files: map[string]string{ `main.tf`: ` + data "coder_parameter" "unrelated" { + name = "unrelated" + type = "list(string)" + default = jsonencode(["a", "b"]) + } resource "null_resource" "test" {} data "coder_workspace_tags" "tags" { tags = { @@ -451,6 +492,11 @@ func TestPostTemplateVersionsByOrganization(t *testing.T) { name: "main.tf with workspace tags that set scope=user", files: map[string]string{ `main.tf`: ` + data "coder_parameter" "unrelated" { + name = "unrelated" + type = "list(string)" + default = jsonencode(["a", "b"]) + } resource "null_resource" "test" {} data "coder_workspace_tags" "tags" { tags = { @@ -460,6 +506,19 @@ func TestPostTemplateVersionsByOrganization(t *testing.T) { }, wantTags: map[string]string{"owner": templateAdminUser.ID.String(), "scope": "user"}, }, + // Ref: https://github.com/coder/coder/issues/16021 + { + name: "main.tf with no workspace_tags and a function call in a parameter default", + files: map[string]string{ + `main.tf`: ` + data "coder_parameter" "unrelated" { + name = "unrelated" + type = "list(string)" + default = jsonencode(["a", "b"]) + }`, + }, + wantTags: map[string]string{"owner": "", "scope": "organization"}, + }, } { tt := tt t.Run(tt.name, func(t *testing.T) { diff --git a/enterprise/coderd/workspaces_test.go b/enterprise/coderd/workspaces_test.go index b6924a734b8fd..fb5d0eeea8651 100644 --- a/enterprise/coderd/workspaces_test.go +++ b/enterprise/coderd/workspaces_test.go @@ -1412,6 +1412,11 @@ func TestWorkspaceTagsTerraform(t *testing.T) { provider "coder" {} data "coder_workspace" "me" {} data "coder_workspace_owner" "me" {} + data "coder_parameter" "unrelated" { + name = "unrelated" + type = "list(string)" + default = jsonencode(["a", "b"]) + } %s ` diff --git a/provisioner/terraform/tfparse/tfparse.go b/provisioner/terraform/tfparse/tfparse.go index a3a7f971fac2e..a1c1ffe2106e5 100644 --- a/provisioner/terraform/tfparse/tfparse.go +++ b/provisioner/terraform/tfparse/tfparse.go @@ -80,7 +80,7 @@ func New(workdir string, opts ...Option) (*Parser, tfconfig.Diagnostics) { } // WorkspaceTags looks for all coder_workspace_tags datasource in the module -// and returns the raw values for the tags. Use +// and returns the raw values for the tags. func (p *Parser) WorkspaceTags(ctx context.Context) (map[string]string, error) { tags := map[string]string{} var skipped []string @@ -166,13 +166,17 @@ func (p *Parser) WorkspaceTagDefaults(ctx context.Context) (map[string]string, e return nil, xerrors.Errorf("extract workspace tags: %w", err) } + if len(tags) == 0 { + return map[string]string{}, nil + } + // To evaluate the expressions, we need to load the default values for // variables and parameters. - varsDefaults, err := p.VariableDefaults(ctx) + varsDefaults, err := p.VariableDefaults(ctx, tags) if err != nil { return nil, xerrors.Errorf("load variable defaults: %w", err) } - paramsDefaults, err := p.CoderParameterDefaults(ctx, varsDefaults) + paramsDefaults, err := p.CoderParameterDefaults(ctx, varsDefaults, tags) if err != nil { return nil, xerrors.Errorf("load parameter defaults: %w", err) } @@ -247,28 +251,39 @@ func WriteArchive(bs []byte, mimetype string, path string) error { return nil } -// VariableDefaults returns the default values for all variables passed to it. -func (p *Parser) VariableDefaults(ctx context.Context) (map[string]string, error) { +// VariableDefaults returns the default values for all variables referenced in the values of tags. +func (p *Parser) VariableDefaults(ctx context.Context, tags map[string]string) (map[string]string, error) { + var skipped []string // iterate through vars to get the default values for all - // variables. + // required variables. m := make(map[string]string) for _, v := range p.module.Variables { if v == nil { continue } + var found bool + for _, tv := range tags { + if strings.Contains(tv, v.Name) { + found = true + } + } + if !found { + skipped = append(skipped, "var."+v.Name) + continue + } sv, err := interfaceToString(v.Default) if err != nil { return nil, xerrors.Errorf("can't convert variable default value to string: %v", err) } m[v.Name] = strings.Trim(sv, `"`) } - p.logger.Debug(ctx, "found default values for variables", slog.F("defaults", m)) + p.logger.Debug(ctx, "found default values for variables", slog.F("defaults", m), slog.F("skipped", skipped)) return m, nil } // CoderParameterDefaults returns the default values of all coder_parameter data sources // in the parsed module. -func (p *Parser) CoderParameterDefaults(ctx context.Context, varsDefaults map[string]string) (map[string]string, error) { +func (p *Parser) CoderParameterDefaults(ctx context.Context, varsDefaults map[string]string, tags map[string]string) (map[string]string, error) { defaultsM := make(map[string]string) var ( skipped []string @@ -290,6 +305,18 @@ func (p *Parser) CoderParameterDefaults(ctx context.Context, varsDefaults map[st continue } + var found bool + needle := strings.Join([]string{"data", dataResource.Type, dataResource.Name}, ".") + for _, tv := range tags { + if strings.Contains(tv, needle) { + found = true + } + } + if !found { + skipped = append(skipped, needle) + continue + } + // We know in which HCL file is the data resource defined. // NOTE: hclparse.Parser will cache multiple successive calls to parse the same file. file, diags = p.underlying.ParseHCLFile(dataResource.Pos.Filename) diff --git a/provisioner/terraform/tfparse/tfparse_test.go b/provisioner/terraform/tfparse/tfparse_test.go index 9d9bcc4526584..21863e7391c59 100644 --- a/provisioner/terraform/tfparse/tfparse_test.go +++ b/provisioner/terraform/tfparse/tfparse_test.go @@ -49,8 +49,10 @@ func Test_WorkspaceTagDefaultsFromFile(t *testing.T) { type = string default = "us" } - data "base" "ours" { - all = true + data "coder_parameter" "unrelated" { + name = "unrelated" + type = "list(string)" + default = jsonencode(["a", "b"]) } data "coder_parameter" "az" { name = "az" @@ -71,8 +73,10 @@ func Test_WorkspaceTagDefaultsFromFile(t *testing.T) { type = string default = "us" } - data "base" "ours" { - all = true + data "coder_parameter" "unrelated" { + name = "unrelated" + type = "list(string)" + default = jsonencode(["a", "b"]) } data "coder_parameter" "az" { name = "az" @@ -94,8 +98,10 @@ func Test_WorkspaceTagDefaultsFromFile(t *testing.T) { type = string default = "us" } - data "base" "ours" { - all = true + data "coder_parameter" "unrelated" { + name = "unrelated" + type = "list(string)" + default = jsonencode(["a", "b"]) } data "coder_parameter" "az" { name = "az" @@ -128,8 +134,10 @@ func Test_WorkspaceTagDefaultsFromFile(t *testing.T) { type = string default = "${""}${"a"}" } - data "base" "ours" { - all = true + data "coder_parameter" "unrelated" { + name = "unrelated" + type = "list(string)" + default = jsonencode(["a", "b"]) } data "coder_parameter" "az" { name = "az" @@ -158,8 +166,10 @@ func Test_WorkspaceTagDefaultsFromFile(t *testing.T) { type = string default = "us" } - data "base" "ours" { - all = true + data "coder_parameter" "unrelated" { + name = "unrelated" + type = "list(string)" + default = jsonencode(["a", "b"]) } data "coder_parameter" "az" { type = string @@ -195,8 +205,10 @@ func Test_WorkspaceTagDefaultsFromFile(t *testing.T) { type = string default = "eu" } - data "base" "ours" { - all = true + data "coder_parameter" "unrelated" { + name = "unrelated" + type = "list(string)" + default = jsonencode(["a", "b"]) } data "coder_parameter" "az" { name = "az" @@ -235,8 +247,10 @@ func Test_WorkspaceTagDefaultsFromFile(t *testing.T) { type = string default = "us" } - data "base" "ours" { - all = true + data "coder_parameter" "unrelated" { + name = "unrelated" + type = "list(string)" + default = jsonencode(["a", "b"]) } data "coder_parameter" "az" { name = "az" @@ -263,8 +277,10 @@ func Test_WorkspaceTagDefaultsFromFile(t *testing.T) { type = string default = "us" } - data "base" "ours" { - all = true + data "coder_parameter" "unrelated" { + name = "unrelated" + type = "list(string)" + default = jsonencode(["a", "b"]) } data "coder_parameter" "az" { name = "az" @@ -300,8 +316,10 @@ func Test_WorkspaceTagDefaultsFromFile(t *testing.T) { variable "notregion" { type = string } - data "base" "ours" { - all = true + data "coder_parameter" "unrelated" { + name = "unrelated" + type = "list(string)" + default = jsonencode(["a", "b"]) } data "coder_parameter" "az" { name = "az" @@ -332,8 +350,10 @@ func Test_WorkspaceTagDefaultsFromFile(t *testing.T) { type = string default = "us" } - data "base" "ours" { - all = true + data "coder_parameter" "unrelated" { + name = "unrelated" + type = "list(string)" + default = jsonencode(["a", "b"]) } data "coder_parameter" "az" { name = "az" @@ -368,8 +388,10 @@ func Test_WorkspaceTagDefaultsFromFile(t *testing.T) { type = string default = "us" } - data "base" "ours" { - all = true + data "coder_parameter" "unrelated" { + name = "unrelated" + type = "list(string)" + default = jsonencode(["a", "b"]) } data "coder_parameter" "az" { name = "az" @@ -402,8 +424,10 @@ func Test_WorkspaceTagDefaultsFromFile(t *testing.T) { type = string default = "region.us" } - data "base" "ours" { - all = true + data "coder_parameter" "unrelated" { + name = "unrelated" + type = "list(string)" + default = jsonencode(["a", "b"]) } data "coder_parameter" "az" { name = "az" @@ -422,6 +446,99 @@ func Test_WorkspaceTagDefaultsFromFile(t *testing.T) { expectTags: nil, expectError: `Function calls not allowed; Functions may not be called here.`, }, + { + name: "supported types", + files: map[string]string{ + "main.tf": ` + variable "stringvar" { + type = string + default = "a" + } + variable "numvar" { + type = number + default = 1 + } + variable "boolvar" { + type = bool + default = true + } + data "coder_parameter" "stringparam" { + name = "stringparam" + type = "string" + default = "a" + } + data "coder_parameter" "numparam" { + name = "numparam" + type = "number" + default = 1 + } + data "coder_parameter" "boolparam" { + name = "boolparam" + type = "bool" + default = true + } + data "coder_parameter" "listparam" { + name = "listparam" + type = "list(string)" + default = "[\"a\", \"b\"]" + } + data "coder_workspace_tags" "tags" { + tags = { + "stringvar" = var.stringvar + "numvar" = var.numvar + "boolvar" = var.boolvar + "stringparam" = data.coder_parameter.stringparam.value + "numparam" = data.coder_parameter.numparam.value + "boolparam" = data.coder_parameter.boolparam.value + "listparam" = data.coder_parameter.listparam.value + } + }`, + }, + expectTags: map[string]string{ + "stringvar": "a", + "numvar": "1", + "boolvar": "true", + "stringparam": "a", + "numparam": "1", + "boolparam": "true", + "listparam": `["a", "b"]`, + }, + expectError: ``, + }, + { + name: "unsupported list variable", + files: map[string]string{ + "main.tf": ` + variable "listvar" { + type = list(string) + default = ["a"] + } + data "coder_workspace_tags" "tags" { + tags = { + listvar = var.listvar + } + }`, + }, + expectTags: nil, + expectError: `can't convert variable default value to string: unsupported type []interface {}`, + }, + { + name: "unsupported map variable", + files: map[string]string{ + "main.tf": ` + variable "mapvar" { + type = map(string) + default = {"a": "b"} + } + data "coder_workspace_tags" "tags" { + tags = { + mapvar = var.mapvar + } + }`, + }, + expectTags: nil, + expectError: `can't convert variable default value to string: unsupported type map[string]interface {}`, + }, } { tc := tc t.Run(tc.name+"/tar", func(t *testing.T) { From 78d88fed357e834d1bb2db468b0f7db4f6cd7053 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 3 Jan 2025 18:21:30 +0000 Subject: [PATCH 2/4] do some more intelligent HCL parsing to determine required variables --- provisioner/terraform/parse.go | 2 +- provisioner/terraform/tfparse/tfparse.go | 117 +++++++++++------- provisioner/terraform/tfparse/tfparse_test.go | 28 ++++- 3 files changed, 101 insertions(+), 46 deletions(-) diff --git a/provisioner/terraform/parse.go b/provisioner/terraform/parse.go index 9c60102fc8579..7aa78e401c503 100644 --- a/provisioner/terraform/parse.go +++ b/provisioner/terraform/parse.go @@ -26,7 +26,7 @@ func (s *server) Parse(sess *provisionersdk.Session, _ *proto.ParseRequest, _ <- return provisionersdk.ParseErrorf("load module: %s", formatDiagnostics(sess.WorkDirectory, diags)) } - workspaceTags, err := parser.WorkspaceTags(ctx) + workspaceTags, _, err := parser.WorkspaceTags(ctx) if err != nil { return provisionersdk.ParseErrorf("can't load workspace tags: %v", err) } diff --git a/provisioner/terraform/tfparse/tfparse.go b/provisioner/terraform/tfparse/tfparse.go index a1c1ffe2106e5..42c1778d6e85d 100644 --- a/provisioner/terraform/tfparse/tfparse.go +++ b/provisioner/terraform/tfparse/tfparse.go @@ -80,10 +80,12 @@ func New(workdir string, opts ...Option) (*Parser, tfconfig.Diagnostics) { } // WorkspaceTags looks for all coder_workspace_tags datasource in the module -// and returns the raw values for the tags. -func (p *Parser) WorkspaceTags(ctx context.Context) (map[string]string, error) { +// and returns the raw values for the tags. It also returns the set of +// variables referenced by any expressions in the raw values of tags. +func (p *Parser) WorkspaceTags(ctx context.Context) (map[string]string, map[string]struct{}, error) { tags := map[string]string{} - var skipped []string + skipped := []string{} + requiredVars := map[string]struct{}{} for _, dataResource := range p.module.DataResources { if dataResource.Type != "coder_workspace_tags" { skipped = append(skipped, strings.Join([]string{"data", dataResource.Type, dataResource.Name}, ".")) @@ -99,13 +101,13 @@ func (p *Parser) WorkspaceTags(ctx context.Context) (map[string]string, error) { // We know in which HCL file is the data resource defined. file, diags = p.underlying.ParseHCLFile(dataResource.Pos.Filename) if diags.HasErrors() { - return nil, xerrors.Errorf("can't parse the resource file: %s", diags.Error()) + return nil, nil, xerrors.Errorf("can't parse the resource file: %s", diags.Error()) } // Parse root to find "coder_workspace_tags". content, _, diags := file.Body.PartialContent(rootTemplateSchema) if diags.HasErrors() { - return nil, xerrors.Errorf("can't parse the resource file: %s", diags.Error()) + return nil, nil, xerrors.Errorf("can't parse the resource file: %s", diags.Error()) } // Iterate over blocks to locate the exact "coder_workspace_tags" data resource. @@ -117,7 +119,7 @@ func (p *Parser) WorkspaceTags(ctx context.Context) (map[string]string, error) { // Parse "coder_workspace_tags" to find all key-value tags. resContent, _, diags := block.Body.PartialContent(coderWorkspaceTagsSchema) if diags.HasErrors() { - return nil, xerrors.Errorf(`can't parse the resource coder_workspace_tags: %s`, diags.Error()) + return nil, nil, xerrors.Errorf(`can't parse the resource coder_workspace_tags: %s`, diags.Error()) } if resContent == nil { @@ -125,43 +127,91 @@ func (p *Parser) WorkspaceTags(ctx context.Context) (map[string]string, error) { } if _, ok := resContent.Attributes["tags"]; !ok { - return nil, xerrors.Errorf(`"tags" attribute is required by coder_workspace_tags`) + return nil, nil, xerrors.Errorf(`"tags" attribute is required by coder_workspace_tags`) } expr := resContent.Attributes["tags"].Expr tagsExpr, ok := expr.(*hclsyntax.ObjectConsExpr) if !ok { - return nil, xerrors.Errorf(`"tags" attribute is expected to be a key-value map`) + return nil, nil, xerrors.Errorf(`"tags" attribute is expected to be a key-value map`) } // Parse key-value entries in "coder_workspace_tags" for _, tagItem := range tagsExpr.Items { key, err := previewFileContent(tagItem.KeyExpr.Range()) if err != nil { - return nil, xerrors.Errorf("can't preview the resource file: %v", err) + return nil, nil, xerrors.Errorf("can't preview the resource file: %v", err) } key = strings.Trim(key, `"`) value, err := previewFileContent(tagItem.ValueExpr.Range()) if err != nil { - return nil, xerrors.Errorf("can't preview the resource file: %v", err) + return nil, nil, xerrors.Errorf("can't preview the resource file: %v", err) } if _, ok := tags[key]; ok { - return nil, xerrors.Errorf(`workspace tag %q is defined multiple times`, key) + return nil, nil, xerrors.Errorf(`workspace tag %q is defined multiple times`, key) } tags[key] = value + + // Find values referenced by the expression. + refVars := referencedVariablesExpr(tagItem.ValueExpr) + for _, refVar := range refVars { + requiredVars[refVar] = struct{}{} + } } } } - p.logger.Debug(ctx, "found workspace tags", slog.F("tags", maps.Keys(tags)), slog.F("skipped", skipped)) - return tags, nil + + requiredVarNames := maps.Keys(requiredVars) + slices.Sort(requiredVarNames) + p.logger.Debug(ctx, "found workspace tags", slog.F("tags", maps.Keys(tags)), slog.F("skipped", skipped), slog.F("required_vars", requiredVarNames)) + return tags, requiredVars, nil +} + +// referencedVariablesExpr determines the variables referenced in expr +// and returns the names of those variables. +func referencedVariablesExpr(expr hclsyntax.Expression) (names []string) { + var parts []string + for _, expVar := range expr.Variables() { + for _, tr := range expVar { + switch v := tr.(type) { + case hcl.TraverseRoot: + parts = append(parts, v.Name) + case hcl.TraverseAttr: + parts = append(parts, v.Name) + default: // skip + } + } + + cleaned := cleanupTraversalName(parts) + names = append(names, strings.Join(cleaned, ".")) + } + return names +} + +// cleanupTraversalName chops off extraneous pieces of the traversal. +// for example: +// - var.foo -> unchanged +// - data.coder_parameter.bar.value -> data.coder_parameter.bar +// - null_resource.baz.zap -> null_resource.baz +func cleanupTraversalName(parts []string) []string { + if len(parts) == 0 { + return parts + } + if len(parts) > 3 && parts[0] == "data" { + return parts[:3] + } + if len(parts) > 2 { + return parts[:2] + } + return parts } func (p *Parser) WorkspaceTagDefaults(ctx context.Context) (map[string]string, error) { // This only gets us the expressions. We need to evaluate them. // Example: var.region -> "us" - tags, err := p.WorkspaceTags(ctx) + tags, requiredVars, err := p.WorkspaceTags(ctx) if err != nil { return nil, xerrors.Errorf("extract workspace tags: %w", err) } @@ -172,11 +222,11 @@ func (p *Parser) WorkspaceTagDefaults(ctx context.Context) (map[string]string, e // To evaluate the expressions, we need to load the default values for // variables and parameters. - varsDefaults, err := p.VariableDefaults(ctx, tags) + varsDefaults, err := p.VariableDefaults(ctx) if err != nil { return nil, xerrors.Errorf("load variable defaults: %w", err) } - paramsDefaults, err := p.CoderParameterDefaults(ctx, varsDefaults, tags) + paramsDefaults, err := p.CoderParameterDefaults(ctx, varsDefaults, requiredVars) if err != nil { return nil, xerrors.Errorf("load parameter defaults: %w", err) } @@ -251,9 +301,8 @@ func WriteArchive(bs []byte, mimetype string, path string) error { return nil } -// VariableDefaults returns the default values for all variables referenced in the values of tags. -func (p *Parser) VariableDefaults(ctx context.Context, tags map[string]string) (map[string]string, error) { - var skipped []string +// VariableDefaults returns the default values for all variables in the module. +func (p *Parser) VariableDefaults(ctx context.Context) (map[string]string, error) { // iterate through vars to get the default values for all // required variables. m := make(map[string]string) @@ -261,29 +310,19 @@ func (p *Parser) VariableDefaults(ctx context.Context, tags map[string]string) ( if v == nil { continue } - var found bool - for _, tv := range tags { - if strings.Contains(tv, v.Name) { - found = true - } - } - if !found { - skipped = append(skipped, "var."+v.Name) - continue - } sv, err := interfaceToString(v.Default) if err != nil { return nil, xerrors.Errorf("can't convert variable default value to string: %v", err) } m[v.Name] = strings.Trim(sv, `"`) } - p.logger.Debug(ctx, "found default values for variables", slog.F("defaults", m), slog.F("skipped", skipped)) + p.logger.Debug(ctx, "found default values for variables", slog.F("defaults", m)) return m, nil } // CoderParameterDefaults returns the default values of all coder_parameter data sources // in the parsed module. -func (p *Parser) CoderParameterDefaults(ctx context.Context, varsDefaults map[string]string, tags map[string]string) (map[string]string, error) { +func (p *Parser) CoderParameterDefaults(ctx context.Context, varsDefaults map[string]string, names map[string]struct{}) (map[string]string, error) { defaultsM := make(map[string]string) var ( skipped []string @@ -296,23 +335,17 @@ func (p *Parser) CoderParameterDefaults(ctx context.Context, varsDefaults map[st continue } - if dataResource.Type != "coder_parameter" { - skipped = append(skipped, strings.Join([]string{"data", dataResource.Type, dataResource.Name}, ".")) - continue - } - if !strings.HasSuffix(dataResource.Pos.Filename, ".tf") { continue } - var found bool needle := strings.Join([]string{"data", dataResource.Type, dataResource.Name}, ".") - for _, tv := range tags { - if strings.Contains(tv, needle) { - found = true - } + if dataResource.Type != "coder_parameter" { + skipped = append(skipped, needle) + continue } - if !found { + + if _, found := names[needle]; !found { skipped = append(skipped, needle) continue } diff --git a/provisioner/terraform/tfparse/tfparse_test.go b/provisioner/terraform/tfparse/tfparse_test.go index 21863e7391c59..31757205e7651 100644 --- a/provisioner/terraform/tfparse/tfparse_test.go +++ b/provisioner/terraform/tfparse/tfparse_test.go @@ -34,7 +34,7 @@ func Test_WorkspaceTagDefaultsFromFile(t *testing.T) { name: "single text file", files: map[string]string{ "file.txt": ` - hello world`, + hello world`, }, expectTags: map[string]string{}, expectError: "", @@ -539,6 +539,28 @@ func Test_WorkspaceTagDefaultsFromFile(t *testing.T) { expectTags: nil, expectError: `can't convert variable default value to string: unsupported type map[string]interface {}`, }, + { + name: "overlapping var name", + files: map[string]string{ + `main.tf`: ` + variable "a" { + type = string + default = "1" + } + variable "ab" { + description = "This is a variable of type string" + type = string + default = "ab" + } + data "coder_workspace_tags" "tags" { + tags = { + "foo": "bar", + "a": var.a, + } + }`, + }, + expectTags: map[string]string{"foo": "bar", "a": "1"}, + }, } { tc := tc t.Run(tc.name+"/tar", func(t *testing.T) { @@ -622,7 +644,7 @@ func BenchmarkWorkspaceTagDefaultsFromFile(b *testing.B) { tfparse.WriteArchive(tarFile, "application/x-tar", tmpDir) parser, diags := tfparse.New(tmpDir, tfparse.WithLogger(logger)) require.NoError(b, diags.Err()) - _, err := parser.WorkspaceTags(ctx) + _, _, err := parser.WorkspaceTags(ctx) if err != nil { b.Fatal(err) } @@ -636,7 +658,7 @@ func BenchmarkWorkspaceTagDefaultsFromFile(b *testing.B) { tfparse.WriteArchive(zipFile, "application/zip", tmpDir) parser, diags := tfparse.New(tmpDir, tfparse.WithLogger(logger)) require.NoError(b, diags.Err()) - _, err := parser.WorkspaceTags(ctx) + _, _, err := parser.WorkspaceTags(ctx) if err != nil { b.Fatal(err) } From 222fd902aaa11bdaed1cd52d335ab580cbf60bb3 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 3 Jan 2025 18:34:30 +0000 Subject: [PATCH 3/4] fallback to JSON-encoding default variable values --- provisioner/terraform/tfparse/tfparse.go | 8 ++- provisioner/terraform/tfparse/tfparse_test.go | 50 ++++++------------- 2 files changed, 22 insertions(+), 36 deletions(-) diff --git a/provisioner/terraform/tfparse/tfparse.go b/provisioner/terraform/tfparse/tfparse.go index 42c1778d6e85d..de767a833207f 100644 --- a/provisioner/terraform/tfparse/tfparse.go +++ b/provisioner/terraform/tfparse/tfparse.go @@ -598,7 +598,11 @@ func interfaceToString(i interface{}) (string, error) { return strconv.FormatFloat(v, 'f', -1, 64), nil case bool: return strconv.FormatBool(v), nil - default: - return "", xerrors.Errorf("unsupported type %T", v) + default: // just try to JSON-encode it. + var sb strings.Builder + if err := json.NewEncoder(&sb).Encode(i); err != nil { + return "", xerrors.Errorf("convert %T: %w", v, err) + } + return strings.TrimSpace(sb.String()), nil } } diff --git a/provisioner/terraform/tfparse/tfparse_test.go b/provisioner/terraform/tfparse/tfparse_test.go index 31757205e7651..24ebfe20de63d 100644 --- a/provisioner/terraform/tfparse/tfparse_test.go +++ b/provisioner/terraform/tfparse/tfparse_test.go @@ -462,6 +462,14 @@ func Test_WorkspaceTagDefaultsFromFile(t *testing.T) { type = bool default = true } + variable "listvar" { + type = list(string) + default = ["a"] + } + variable "mapvar" { + type = map(string) + default = {"a": "b"} + } data "coder_parameter" "stringparam" { name = "stringparam" type = "string" @@ -487,6 +495,8 @@ func Test_WorkspaceTagDefaultsFromFile(t *testing.T) { "stringvar" = var.stringvar "numvar" = var.numvar "boolvar" = var.boolvar + "listvar" = var.listvar + "mapvar" = var.mapvar "stringparam" = data.coder_parameter.stringparam.value "numparam" = data.coder_parameter.numparam.value "boolparam" = data.coder_parameter.boolparam.value @@ -498,6 +508,8 @@ func Test_WorkspaceTagDefaultsFromFile(t *testing.T) { "stringvar": "a", "numvar": "1", "boolvar": "true", + "listvar": `["a"]`, + "mapvar": `{"a":"b"}`, "stringparam": "a", "numparam": "1", "boolparam": "true", @@ -505,40 +517,6 @@ func Test_WorkspaceTagDefaultsFromFile(t *testing.T) { }, expectError: ``, }, - { - name: "unsupported list variable", - files: map[string]string{ - "main.tf": ` - variable "listvar" { - type = list(string) - default = ["a"] - } - data "coder_workspace_tags" "tags" { - tags = { - listvar = var.listvar - } - }`, - }, - expectTags: nil, - expectError: `can't convert variable default value to string: unsupported type []interface {}`, - }, - { - name: "unsupported map variable", - files: map[string]string{ - "main.tf": ` - variable "mapvar" { - type = map(string) - default = {"a": "b"} - } - data "coder_workspace_tags" "tags" { - tags = { - mapvar = var.mapvar - } - }`, - }, - expectTags: nil, - expectError: `can't convert variable default value to string: unsupported type map[string]interface {}`, - }, { name: "overlapping var name", files: map[string]string{ @@ -547,6 +525,10 @@ func Test_WorkspaceTagDefaultsFromFile(t *testing.T) { type = string default = "1" } + variable "unused" { + type = map(string) + default = {"a" : "b"} + } variable "ab" { description = "This is a variable of type string" type = string From de5541b41230913e6098ebcbe5a5674979d986b2 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 3 Jan 2025 18:40:53 +0000 Subject: [PATCH 4/4] ensure unrelated variables without default values are allowed --- provisioner/terraform/tfparse/tfparse_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/provisioner/terraform/tfparse/tfparse_test.go b/provisioner/terraform/tfparse/tfparse_test.go index 24ebfe20de63d..afbec4d0b8d4b 100644 --- a/provisioner/terraform/tfparse/tfparse_test.go +++ b/provisioner/terraform/tfparse/tfparse_test.go @@ -98,6 +98,9 @@ func Test_WorkspaceTagDefaultsFromFile(t *testing.T) { type = string default = "us" } + variable "unrelated" { + type = bool + } data "coder_parameter" "unrelated" { name = "unrelated" type = "list(string)"