fix(terraform): hcl object expressions to return references #8271

Emyrk · 2025-01-21T22:39:53Z

Description

References included in hcl objects should be returned from 'Attribute.AllReferences()'. Traverse the object fields recursively to locate references.

Expressions such as:

data "foo" "bar" {
  tags = {
    "cacheTTL"   = var.cache
  }
}

Should return variable.cache in the AllReferences()

Checklist

I've read the guidelines for contributing to this repository.
I've followed the conventions in the PR title.
I've added tests that prove my fix is effective or that my feature works.
I've updated the documentation with the relevant information (if needed).
I've added usage information (if the PR introduces new options)
I've included a "before" and "after" example to the description (if the PR is a user interface change).

References included in hcl objects should be returned from 'Attribute.AllReferences()'. Traverse the object fields recursively to locate references.

pkg/iac/terraform/attribute.go

nikpivkin · 2025-01-22T06:47:13Z

pkg/iac/terraform/attribute.go

@@ -971,6 +971,10 @@ func (a *Attribute) AllReferences(blocks ...*Block) []*Reference {
 func (a *Attribute) referencesFromExpression(expression hcl.Expression) []*Reference {
 	var refs []*Reference
 	switch t := expression.(type) {
+	case *hclsyntax.ObjectConsExpr:
+		for _, item := range t.Items {
+			refs = append(refs, a.referencesFromExpression(item.ValueExpr)...)


You need to use createDotReferenceFromTraversal, not referencesFromExpression.

I modified a unit test.

This is an object that sets foo to a conditional expression. That conditional expression has 2 references in it.

{foo = 1 == 1 ? var.bar : data.aws_ami.ubuntu.most_recent}

Maybe I am misunderstanding, but if I do

if ref, err := createDotReferenceFromTraversal(a.module, item.KeyExpr.Variables()...); err == nil { refs = append(refs, ref) } // And do the same for the ValueExpression

I get variable.bar.data.aws_ami.ubuntu.most_recent. Since Variables() call just appends all traversal types:

https://github.com/hashicorp/hcl/blob/main/hclsyntax/variables.go#L18-L20

It is not intelligent to know the conditional expression has 2 different references, and just returns them as a single list. Rather than 2.

@nikpivkin let me know I am misusing createDotReferenceFromTraversal

nikpivkin · 2025-01-22T06:49:30Z

Hi @Emyrk! Thanks for contributing!

Left a couple of comments. Also can you fix the linting issues?

Emyrk · 2025-01-22T19:25:55Z

pkg/iac/terraform/attribute.go

+	case *hclsyntax.ParenthesesExpr:
+		refs = append(refs, a.referencesFromExpression(t.Expression)...)
+	case *hclsyntax.ObjectConsKeyExpr:
+		refs = append(refs, a.referencesFromExpression(t.Wrapped)...)


Using your example (local.foo): local.foo, I also had to add these @nikpivkin

Emyrk · 2025-01-22T20:54:13Z

@nikpivkin unrelated, but is there a reason the underlying *hcl.Attribute is unexported on Attribute? I ask because I am looking to use this library in my use case, however there are times where the Value() will be unknown. I want to look into the HCL to deduce further what is causing it to be unknown, primarily for debugging/logging purposes.

I was wondering if a getter like func (a *Attribute) HCLAttribute() *hcl.Attribute would be totally out of the question.

simar7 · 2025-01-22T21:15:01Z

@nikpivkin unrelated, but is there a reason the underlying *hcl.Attribute is unexported on Attribute? I ask because I am looking to use this library in my use case, however there are times where the Value() will be unknown. I want to look into the HCL to deduce further what is causing it to be unknown, primarily for debugging/logging purposes.

What's your use case for this? Value() is designed to return the attribute value in the case it is known and not null.

Emyrk · 2025-01-22T21:48:28Z

@simar7

What's your use case for this? Value() is designed to return the attribute value in the case it is known and not null.

I am doing work on https://github.com/coder/coder. Terraform is the configuration language we use for the underlying developer workspaces.

To give even more functionality/flexibility in the experience, we have a feature called "Build Parameters". See example image.

In order to speed up some of the inner loop and provide more immediate feedback on these parameters, I am seeking to use static anaylsis of the terraform (using your package, which is awesome by the way ❤️). Now this is not perfect, if the parameters include a reference to an external data source (docker_image.ubuntu.repo_digest), then the value will be unknown. This is correct, and I was using AllReferences() to fetch the dependencies.

I intend to use the existing terraform state to populate these unknown references in some second "pass/step". I do not see this package injesting a tfstate, so was going to build something to mutate the eval context.

My usage of the specific functionality is AllReferences(). This works well for an entire block (this PR included). However, in one of my use cases I have a block that looks like:

data "coder_workspace_tags" "custom_workspace_tags" {
  tags = {
    // AllReferences() = [docker_image.ubuntu.repo_digest, docker_image.centos]
    "foo" = docker_image.ubuntu.repo_digest
    "bar" = docker_image.centos.repo_digest
    "qux" = "quux"
  }
}

I am using attr.Each(func(key cty.Value, val cty.Value) to iterate over the fields foo and bar. Unfortunately, when calling .Value() to cast into a cty.Value, the references are lost into the cty.unknownType.

So I can tell the reference docker_image.<ubuntu | centos>.repo_digest is used by the block tags, but I cannot tell exactly where. Ideally I could deduce foo -> docker_image.ubuntu.repo_digest and bar -> docker_image.centos.repo_digest.

To do this, I would either need to have the underlying hcl.Expression myself, or expand upon AllReferences to pass in some traversal to do first?

I see additional value in exposing the hcl.Expression, as it is the raw information that backs a given cty.Value. I understand the point to encapsulate the expressions, it is just harder to hook into without the raw hcl.Expression.

simar7 · 2025-01-24T06:41:20Z

@simar7

What's your use case for this? Value() is designed to return the attribute value in the case it is known and not null.

I am doing work on https://github.com/coder/coder. Terraform is the configuration language we use for the underlying developer workspaces.

To give even more functionality/flexibility in the experience, we have a feature called "Build Parameters". See example image.

In order to speed up some of the inner loop and provide more immediate feedback on these parameters, I am seeking to use static anaylsis of the terraform (using your package, which is awesome by the way ❤️). Now this is not perfect, if the parameters include a reference to an external data source (docker_image.ubuntu.repo_digest), then the value will be unknown. This is correct, and I was using AllReferences() to fetch the dependencies.

I intend to use the existing terraform state to populate these unknown references in some second "pass/step". I do not see this package injesting a tfstate, so was going to build something to mutate the eval context.

My usage of the specific functionality is AllReferences(). This works well for an entire block (this PR included). However, in one of my use cases I have a block that looks like:
data "coder_workspace_tags" "custom_workspace_tags" {
  tags = {
    // AllReferences() = [docker_image.ubuntu.repo_digest, docker_image.centos]
    "foo" = docker_image.ubuntu.repo_digest
    "bar" = docker_image.centos.repo_digest
    "qux" = "quux"
  }
}
I am using attr.Each(func(key cty.Value, val cty.Value) to iterate over the fields foo and bar. Unfortunately, when calling .Value() to cast into a cty.Value, the references are lost into the cty.unknownType.

So I can tell the reference docker_image.<ubuntu | centos>.repo_digest is used by the block tags, but I cannot tell exactly where. Ideally I could deduce foo -> docker_image.ubuntu.repo_digest and bar -> docker_image.centos.repo_digest.

To do this, I would either need to have the underlying hcl.Expression myself, or expand upon AllReferences to pass in some traversal to do first?

I see additional value in exposing the hcl.Expression, as it is the raw information that backs a given cty.Value. I understand the point to encapsulate the expressions, it is just harder to hook into without the raw hcl.Expression.

Thanks for the explanation. I don't see anything inherently wrong with exposing it. WDYT @nikpivkin?

nikpivkin · 2025-01-25T10:58:23Z

@Emyrk It seems we can just use the Variables method to get the expression references without processing each type of expression. Also, the current implementation does not return an index reference for RelativeTraversalExpr. Example: foo.bar[local.idx].name. If you don't mind, I can make changes to this PR.

nikpivkin · 2025-01-25T11:01:30Z

I think we can provide a method to get the hcl.Attribute.

Emyrk · 2025-01-27T02:46:46Z

@nikpivkin feel free to make changes to this PR directly 👍. It is in my fork, so you might have to make a branch in this repo. Or I can give you perms in my fork.

I have copied some of your code to take the Variables() return into the human readable foo.bar

https://github.com/Emyrk/terraform-eval/blob/0cfb77f6f7b217e02e7b56cf39049e36ab8c9c29/engine/hclext/references.go#L10-L37

I think we can provide a method to get the hcl.Attribute.

Another benefit of exposing is I can create and decorate hcl.Diagnostic errors.

An example of some code I have that expects an attribute to have a given type:

func (a *expectedAttribute) expectedTypeError(attr *terraform.Attribute, expectedType string) {
	a.error(hcl.Diagnostic{
		Severity:   hcl.DiagError,
		Summary:    "Invalid attribute type",
		Detail:     fmt.Sprintf("The attribute %q must be of type %q, found type %q", attr.Name(), expectedType, attr.Type().FriendlyNameForConstraint()),
		Subject:    &attr.HCLAttribute().Range,
		Context:    &a.p.block.HCLBlock().DefRange,
		Expression: attr.HCLAttribute().Expr,

		EvalContext: a.p.block.Context().Inner(),
	})
}

expects a given attribute:

tagsAttr := block.GetAttribute("tags")
if tagsAttr.IsNil() {
	r := block.HCLBlock().Body.MissingItemRange()
	diags = diags.Append(&hcl.Diagnostic{
		Severity:    hcl.DiagError,
		Summary:     "Missing required argument",
		Detail:      `"tags" attribute is required by coder_workspace_tags blocks`,
		Subject:     &r,
		EvalContext: evCtx,
	})
	continue
}

Related, I did the same for type Block struct

// Added code to expose the HCL
func (b *Block) HCLBlock() *hcl.Block {
	return b.hclBlock
}

// Usage
func Foo() {
 // ...
  tagsAttr := block.GetAttribute("tags")
  if tagsAttr.IsNil() {
	  r := block.HCLBlock().Body.MissingItemRange()
	  diags = diags.Append(&hcl.Diagnostic{
		  Severity:    hcl.DiagError,
		  Summary:     "Missing required argument",
		  Detail:      `"tags" attribute is required by coder_workspace_tags blocks`,
		  Subject:     &r,
		  EvalContext: evCtx,
	  })
	  continue
  }
  // ...
}

And lastly to make these added diagnostic errors to work, I had to expose the parser.underlying.Files() method.

func (p *Parser) Files() map[string]*hcl.File {
	return p.underlying.Files()
}

It just makes sense for my use case to use the same hcl diagnostic errors, rather than making some new custom error type. And I can leverage the same error printers already provided in the HCL packages.

Signed-off-by: nikpivkin <[email protected]>

Emyrk · 2025-01-27T14:01:34Z

I see what you mean by using Variables() now 👍

Emyrk · 2025-01-28T17:23:33Z

@nikpivkin As for naming of a function to return the underlying raw HCL. Just a suggestion based on a recent finding.

tflint has a function AsNative() that returns the corresponding hcl type.

So Block, Attribute, etc could call their function AsNative()

simar7 · 2025-01-28T18:23:47Z

@nikpivkin As for naming of a function to return the underlying raw HCL. Just a suggestion based on a recent finding.

tflint has a function AsNative() that returns the corresponding hcl type.

So Block, Attribute, etc could call their function AsNative()

Can we do this in a separate PR? It seems unrelated to the current PR. Feel free to open a new PR.

Emyrk · 2025-01-28T18:36:23Z

@simar7 A separate PR for sure. I'll open one 👍

Emyrk · 2025-02-19T20:30:17Z

@nikpivkin I think this PR has gotten overlooked in the conversations.

nikpivkin · 2025-02-19T20:31:54Z

@simar7 Can you take a look?

simar7 · 2025-02-22T01:33:19Z

pkg/iac/scanners/terraform/parser/parser_test.go

+	fooAttr := foo.GetAttribute("attr")
+	b, err := root.GetReferencedBlock(fooAttr, foo)
+	require.NoError(t, err)
+	require.NotNil(t, b)


Maybe we can strengthen this assertion?

Suggested change

require.NotNil(t, b)

assert.Equal(t, "test", b.Values().AsValueSlice()[0].AsString())

simar7

lgtm, left one comment. I'd also get @nikpivkin to take a look at this again.

fix(terraform): hcl object expressions to return references

bd72f6f

References included in hcl objects should be returned from 'Attribute.AllReferences()'. Traverse the object fields recursively to locate references.

Emyrk requested review from simar7 and nikpivkin as code owners January 21, 2025 22:39

Emyrk added 2 commits January 21, 2025 16:42

add unit test cases

5687d2b

import order fix

096d9f3

nikpivkin reviewed Jan 22, 2025

View reviewed changes

pkg/iac/terraform/attribute.go Outdated Show resolved Hide resolved

nikpivkin reviewed Jan 22, 2025

View reviewed changes

Emyrk added 3 commits January 22, 2025 11:08

fix import order of attribute_test.go

a2d93da

key fields in objects can also be expressions that contain references

8eccb32

handle parenthesis wrapped and objKeyExpressions

11c2a02

Emyrk commented Jan 22, 2025

View reviewed changes

Emyrk requested a review from nikpivkin January 22, 2025 20:51

refactor: use Variables to extract refs from expr

aaa7322

Signed-off-by: nikpivkin <[email protected]>

simar7 reviewed Feb 22, 2025

View reviewed changes

simar7 self-requested a review February 22, 2025 01:46

simar7 approved these changes Feb 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(terraform): hcl object expressions to return references #8271

fix(terraform): hcl object expressions to return references #8271

Emyrk commented Jan 21, 2025

nikpivkin Jan 22, 2025

Emyrk Jan 22, 2025

Emyrk Jan 23, 2025

nikpivkin commented Jan 22, 2025

Emyrk Jan 22, 2025

Emyrk commented Jan 22, 2025 •

edited

Loading

simar7 commented Jan 22, 2025

Emyrk commented Jan 22, 2025 •

edited

Loading

simar7 commented Jan 24, 2025

nikpivkin commented Jan 25, 2025

nikpivkin commented Jan 25, 2025

Emyrk commented Jan 27, 2025

Emyrk commented Jan 27, 2025

Emyrk commented Jan 28, 2025 •

edited

Loading

simar7 commented Jan 28, 2025

Emyrk commented Jan 28, 2025

Emyrk commented Feb 19, 2025

nikpivkin commented Feb 19, 2025

simar7 Feb 22, 2025

simar7 left a comment

	require.NotNil(t, b)
	assert.Equal(t, "test", b.Values().AsValueSlice()[0].AsString())

fix(terraform): hcl object expressions to return references #8271

Are you sure you want to change the base?

fix(terraform): hcl object expressions to return references #8271

Conversation

Emyrk commented Jan 21, 2025

Description

Checklist

nikpivkin Jan 22, 2025

Choose a reason for hiding this comment

Emyrk Jan 22, 2025

Choose a reason for hiding this comment

Emyrk Jan 23, 2025

Choose a reason for hiding this comment

nikpivkin commented Jan 22, 2025

Emyrk Jan 22, 2025

Choose a reason for hiding this comment

Emyrk commented Jan 22, 2025 • edited Loading

simar7 commented Jan 22, 2025

Emyrk commented Jan 22, 2025 • edited Loading

simar7 commented Jan 24, 2025

nikpivkin commented Jan 25, 2025

nikpivkin commented Jan 25, 2025

Emyrk commented Jan 27, 2025

Emyrk commented Jan 27, 2025

Emyrk commented Jan 28, 2025 • edited Loading

simar7 commented Jan 28, 2025

Emyrk commented Jan 28, 2025

Emyrk commented Feb 19, 2025

nikpivkin commented Feb 19, 2025

simar7 Feb 22, 2025

Choose a reason for hiding this comment

simar7 left a comment

Choose a reason for hiding this comment

Emyrk commented Jan 22, 2025 •

edited

Loading

Emyrk commented Jan 22, 2025 •

edited

Loading

Emyrk commented Jan 28, 2025 •

edited

Loading