Skip to content

fix(provisioner/terraform/tfparse): skip evaluation of unrelated parameters #16023

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jan 3, 2025
Merged

fix(provisioner/terraform/tfparse): skip evaluation of unrelated parameters #16023

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e56cac7
fix(provisioner/terraform/tfparse): skip evaluation of defaults if no…
johnstcn Jan 3, 2025
78d88fe
do some more intelligent HCL parsing to determine required variables
johnstcn Jan 3, 2025
222fd90
fallback to JSON-encoding default variable values
johnstcn Jan 3, 2025
de5541b
ensure unrelated variables without default values are allowed
johnstcn Jan 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 86 additions & 27 deletions coderd/templateversions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"},
Expand All @@ -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"},
},
Expand All @@ -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 = {
Expand All @@ -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"},
Expand All @@ -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"
}
Expand All @@ -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"
}
Expand All @@ -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 = {
Expand All @@ -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 = {
Expand All @@ -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 = {
Expand All @@ -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) {
Expand Down
5 changes: 5 additions & 0 deletions enterprise/coderd/workspaces_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
`

Expand Down
43 changes: 35 additions & 8 deletions provisioner/terraform/tfparse/tfparse.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if strings.Contains(tv, v.Name) {
if strings.Contains(tv, "var."+v.Name) {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

won't a strings.Contains also contain variable names that are subsets?

Like var.Foo will match both Foo and var.FooBar

Copy link
Member

@Emyrk Emyrk Jan 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a test case:

{
   name: "overlapping var name",
   files: map[string]string{
      `main.tf`: `
      // This file is the same as the above, except for this comment.
      variable "a" {
         type = string
         default = "1"
      }
      variable "ab" {
       description = "This is a variable of type string"
       type        = string
       default     = jsonencode(["a", "b"])
      }
      data "coder_workspace_tags" "tags" {
         tags = {
            "foo": "bar",
            "a": var.a,
         }
      }`,
   },
   reqTags:  map[string]string{"foo": "noclobber"},
   wantTags: map[string]string{"owner": "", "scope": "organization", "foo": "bar", "a": "1"},
},

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, we may then need to do some more "intelligent" parsing.
I hesitate to reach for a regexp.... but \b may be exactly what we need here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the reason you do a contains because it could be an expression like foo(var.name)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way to parse the expression and get the actual tokens? Or we only have string in the HCL?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. I'm looking into HCL traversals now as they might allow us to do this cleanly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I've modified the implementation to instead:

  • Gather the set of variables referenced by the tags up-front,
  • Only evaluate the default value(s) for coder_parameters referenced by those tags.
  • As default values of coder_parameters can reference other variables, to avoid needing to perform a graph traversal of all the dependencies here I've elected to simply continue passing in all the variable default values. We don't immediately enforce default values for all variables.
  • To avoid unrelated variables with default values of complex types being rejected, I added some 'last-ditch' support for JSON-encoding them.

found = true
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: you could extract this block and the one below to a common helper function

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
Expand All @@ -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}, ".")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, there is no way to break a variable reference over multiple lines e.g.

data.\
coder_parameter.\
foo.\
value

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct 👍

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)
Expand Down
Loading
Loading