allow input validation with variable in source or version #5041
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR fixes validation failures when Terraform modules use variables in source or version fields by filtering out "variables not allowed" diagnostics during module variable extraction. It adds test fixtures and an integration test suite to verify the fix works correctly. Changes
Sequence DiagramsequenceDiagram
participant User
participant Terragrunt
participant TFConfig as Module Config
participant Diagnostics
User->>Terragrunt: terragrunt hcl validate --inputs
Terragrunt->>TFConfig: Extract module variables
TFConfig-->>Diagnostics: Generate diagnostics<br/>(includes "variables not allowed")
alt Before Fix
Diagnostics->>Terragrunt: Return all diagnostics
Terragrunt->>Terragrunt: Error on "variables not allowed"
Terragrunt-->>User: ❌ Validation failed
else After Fix (This PR)
Diagnostics->>Terragrunt: All diagnostics
Terragrunt->>Terragrunt: Filter ignorable diagnostics<br/>(keep only input-affecting errors)
Terragrunt->>Terragrunt: Validate inputs against<br/>extracted variables
Terragrunt-->>User: ✓ Validation result (pass/fail on inputs)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related issues
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ThisGuyCodes
force-pushed
the
thisguycodes/allow-vars-in-source-version
branch
from
739b926 to
9d7f9b1
Compare
| --- | ||
|
|
||
| When enabled, Terragrunt will validate that all variables are set by the module a unit provisions. | ||
| When enabled, Terragrunt will validate that all variables a module (provisioned by a unit) requires are set. |
There was a problem hiding this comment.
Open to input on this, or even removing it. But I definitely don't think the current wording is correct (since we're checking that the unit sets variables consumed by the module, the module doesn't set them).
There was a problem hiding this comment.
Would better wording be: "When enabled, Terragrunt will validate that all inputs set by Terragrunt units are valid OpenTofu/Terraform variables consumed by their respective modules"?
There was a problem hiding this comment.
I was able to make an even more minimal example than the one in #4986
There was a problem hiding this comment.
I know empty files seem silly, but in the spirit of "minimal reproduction" I don't think you can get more minimal than empty!
There was a problem hiding this comment.
Nothing wrong with it! If you want to be explicit about this, you can leave a one-line comment in the file that it's intentionally empty.
| t.Run("using --all", func(t *testing.T) { | ||
| t.Parallel() | ||
| helpers.CleanupTerraformFolder(t, testFixtureHclvalidate) | ||
| tmpEnvPath := helpers.CopyEnvironment(t, testFixtureHclvalidate) | ||
| rootPath := util.JoinPath(tmpEnvPath, testFixtureHclvalidate) | ||
|
|
||
| _, _, err := helpers.RunTerragruntCommandWithOutput(t, "terragrunt hcl validate --all --strict --inputs --working-dir "+path.Join(rootPath, "valid")) | ||
| require.NoError(t, err) | ||
| }) |
There was a problem hiding this comment.
I added this test, but actually it was already passing! Is --all not supposed to return non-zero?
There was a problem hiding this comment.
It's entirely possible there's a bug in the error handling. I think it should be returning an error if anything fails during an --all run.
| ignorableErrors := []tfconfig.Diagnostic{ | ||
| // These two occur when a module block uses a variable in the `version` or `source` fields. | ||
| // Terraform doesn't support this, but OpenTofu does. Either way our ability to extract variables is unaffected. | ||
| // | ||
| // What we really need is an OpenTofu version of terraform-config-inspect. This may work for now but as / if | ||
| // syntax continues to diverge we may run into other issues. | ||
| {Summary: "Variables not allowed", Detail: "Variables may not be used here."}, | ||
| {Summary: "Unsuitable value type", Detail: "Unsuitable value: value must be known"}, | ||
| } |
There was a problem hiding this comment.
I wanted to keep this scoped to this function, plus putting it outside the function would require a more descriptive name.
I'm 90% sure the compiler notices it's never changed and pre-allocates it, but am happy to be proven wrong (and will move it if so).
There was a problem hiding this comment.
I don't think it's a big deal either way.
ThisGuyCodes
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs-starlight/src/data/flags/hcl-validate-inputs.mdx(1 hunks)test/fixtures/hclvalidate/valid/var-in-source/main.tf(1 hunks)test/fixtures/hclvalidate/valid/var-in-version/main.tf(1 hunks)test/integration_test.go(1 hunks)tf/tf.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
docs-starlight/**/*.md*
⚙️ CodeRabbit configuration file
Review the documentation for clarity, grammar, and spelling. Make sure that the documentation is easy to understand and follow. There is currently a migration underway from the Jekyll based documentation in
docsto the Starlight + Astro based documentation indocs-starlight. Make sure that thedocs-starlightdocumentation is accurate and up-to-date with thedocsdocumentation, and that any difference between them results in an improvement in thedocs-starlightdocumentation.
Files:
docs-starlight/src/data/flags/hcl-validate-inputs.mdx
**/*.go
⚙️ CodeRabbit configuration file
Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
Files:
tf/tf.gotest/integration_test.go
🧠 Learnings (2)
📚 Learning: 2025-08-19T16:05:54.723Z
Learnt from: Resonance1584
Repo: gruntwork-io/terragrunt PR: 4683
File: go.mod:86-90
Timestamp: 2025-08-19T16:05:54.723Z
Learning: When analyzing Go module dependencies for removal, always check for both direct imports and API usage across all Go files in the repository, not just a quick search. The github.com/mattn/go-zglob library is used for filesystem walking and glob expansion in multiple Terragrunt files including util/file.go, format commands, and AWS provider patch functionality.
Applied to files:
tf/tf.go
📚 Learning: 2025-02-10T13:36:19.542Z
Learnt from: levkohimins
Repo: gruntwork-io/terragrunt PR: 3723
File: cli/commands/stack/action.go:160-160
Timestamp: 2025-02-10T13:36:19.542Z
Learning: The project uses a custom error package `github.com/gruntwork-io/terragrunt/internal/errors` which provides similar functionality to `fmt.Errorf` but includes stack traces. Prefer using this package's error functions (e.g., `errors.Errorf`, `errors.New`) over the standard library's error handling.
Applied to files:
tf/tf.go
🧬 Code graph analysis (2)
tf/tf.go (1)
internal/view/diagnostic/diagnostic.go (1)
Diagnostic(21-27)
test/integration_test.go (2)
test/helpers/package.go (3)
CleanupTerraformFolder(879-886)CopyEnvironment(89-102)RunTerragruntCommandWithOutput(1004-1008)util/file.go (1)
JoinPath(626-628)
🪛 Checkov (3.2.334)
test/fixtures/hclvalidate/valid/var-in-source/main.tf
[medium] 5-8: Ensure Terraform module sources use a commit hash
(CKV_TF_1)
[high] 5-8: Ensure Terraform module sources use a tag with a version number
(CKV_TF_2)
test/fixtures/hclvalidate/valid/var-in-version/main.tf
[medium] 5-8: Ensure Terraform module sources use a commit hash
(CKV_TF_1)
[high] 5-8: Ensure Terraform module sources use a tag with a version number
(CKV_TF_2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: base_tests / Test (macos)
- GitHub Check: base_tests / Test (ubuntu)
- GitHub Check: Pull Request has non-contributor approval
| return true | ||
| } | ||
|
|
||
| i := slices.IndexFunc(ignorableErrors, func(ie tfconfig.Diagnostic) bool { |
There was a problem hiding this comment.
I was thinking that this can be simplified to
return slices.ContainsFunc(ignorableErrors, func(ie tfconfig.Diagnostic) bool {
return ie.Summary == d.Summary && ie.Detail == d.Detail
})
There was a problem hiding this comment.
That makes way more sense. I already know about contains so I don't know what I was thinking when I wrote this
There was a problem hiding this comment.
I think this does the job, so I'll approve it. It's not how I thought you'd tackle this.
I was assuming you'd do something like this untested code:
type Variable struct {
Name string `hcl:"name,label"`
Default string `hcl:"default,optional"`
Remainder hcl.Body `hcl:",remain"`
}
type ModuleVars struct {
Variables []Variable `hcl:"variable,block"`
}
// ModuleVariables will return all the variables defined in the downloaded terraform modules, taking into
// account all the generated sources. This function will return the required and optional variables separately.
func ModuleVariables(modulePath string) ([]string, []string, error) {
parser := hclparse.NewParser()
files, err := os.ReadDir(modulePath)
if err != nil {
return nil, nil, err
}
hclFiles := []*hcl.File{}
errs := []error{}
for _, file := range files {
if file.IsDir() {
continue
}
// This would only solve the issue for HCL files. We'd need to also check JSON files.
if !strings.HasSuffix(file.Name(), ".tf") && !strings.HasSuffix(file.Name(), ".tofu") {
continue
}
hclFile, err := parser.ParseFromFile(file.Name())
if err != nil {
errs = append(errs, err)
}
hclFiles = append(hclFiles, hclFile.File)
}
if len(errs) > 0 {
return nil, nil, errors.New(errs)
}
body := hcl.MergeFiles(hclFiles)
moduleVars := ModuleVars{}
if err := gohcl.DecodeBody(body, nil, &moduleVars); err != nil {
return nil, nil, err
}
required := []string{}
optional := []string{}
for _, variable := range moduleVars.Variables {
if variable.Default != "" {
optional = append(optional, variable.Name)
}
}
return required, optional, nil
}To fully drop the dependency on github.com/hashicorp/terraform-config-inspect/tfconfig.
It's a bigger change than what you're doing here, so I'll accept that you might prefer your current solution. Please confirm that you've evaluated this, and don't want to make this or the changes that @denis256 recommended before merging if you're happy with the solution.
| --- | ||
|
|
||
| When enabled, Terragrunt will validate that all variables are set by the module a unit provisions. | ||
| When enabled, Terragrunt will validate that all variables a module (provisioned by a unit) requires are set. |
There was a problem hiding this comment.
Would better wording be: "When enabled, Terragrunt will validate that all inputs set by Terragrunt units are valid OpenTofu/Terraform variables consumed by their respective modules"?
There was a problem hiding this comment.
Nothing wrong with it! If you want to be explicit about this, you can leave a one-line comment in the file that it's intentionally empty.
| t.Run("using --all", func(t *testing.T) { | ||
| t.Parallel() | ||
| helpers.CleanupTerraformFolder(t, testFixtureHclvalidate) | ||
| tmpEnvPath := helpers.CopyEnvironment(t, testFixtureHclvalidate) | ||
| rootPath := util.JoinPath(tmpEnvPath, testFixtureHclvalidate) | ||
|
|
||
| _, _, err := helpers.RunTerragruntCommandWithOutput(t, "terragrunt hcl validate --all --strict --inputs --working-dir "+path.Join(rootPath, "valid")) | ||
| require.NoError(t, err) | ||
| }) |
There was a problem hiding this comment.
It's entirely possible there's a bug in the error handling. I think it should be returning an error if anything fails during an --all run.
There was a problem hiding this comment.
Love the names on these fixtures. Reading the name of the fixture should tell you what the fixture is for.
| ignorableErrors := []tfconfig.Diagnostic{ | ||
| // These two occur when a module block uses a variable in the `version` or `source` fields. | ||
| // Terraform doesn't support this, but OpenTofu does. Either way our ability to extract variables is unaffected. | ||
| // | ||
| // What we really need is an OpenTofu version of terraform-config-inspect. This may work for now but as / if | ||
| // syntax continues to diverge we may run into other issues. | ||
| {Summary: "Variables not allowed", Detail: "Variables may not be used here."}, | ||
| {Summary: "Unsuitable value type", Detail: "Unsuitable value: value must be known"}, | ||
| } |
There was a problem hiding this comment.
I don't think it's a big deal either way.
Description
Terragrunt does not support using variables in the
sourceorversionfields ofmoduleblocks, but OpenTofu does.This becomes a problem because when validating inputs via
terragrunt hcl validate --inputswe use github.com/hashicorp/terraform-config-inspect to read the variables defined by the target module; since this is a Go module designed for Terraform it returns error diagnostics when variables are used as defined above.Thus a valid OpenTofu module will fail a run of
terragrunt hcl validate -inputs.Fixes #4986
Despite these error diagnostics, as long as the syntax is valid we don't actually care (we just want to know what variables are defined).
Ideally OpenTofu would have an equivalent Go module we could use; but it doesn't. For now I've put in logic to selectively ignore diagnostics in this context; as Terraform and OpenTofu syntax's (may) continue to diverge, we may see more issues that this solution may not solve for.
Also reworded the docs on
--inputs, as I found it a bit confusing.TODOs
Release Notes (draft)
Allow variables in module source / version
Summary by CodeRabbit
New Features
Tests
Documentation