Resolve Terraform plan line numbers from searchKey#215
Conversation
|
🎯 Code Coverage (details) 🔗 Commit SHA: 76706bb | Docs | Datadog PR Page | Give us feedback! |
7c7ee11 to
8a8e323
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
8a8e323 to
a552f98
Compare
a552f98 to
5b879c5
Compare
ChouraquiBen
left a comment
ChouraquiBen
left a comment
There was a problem hiding this comment.
A few comments but the core of the idea is good!
| } | ||
|
|
||
| kind := c.Parsers.GetKind() | ||
| if ck, ok := c.Parsers.(contentKindParser); ok { |
There was a problem hiding this comment.
Remark: This is stylish as it allows not to create many changes in the parsers but it creates an abstraction that is used by only one parser. Are there any other parsers that could benefit from it?
There was a problem hiding this comment.
I don’t see another parser with the same “same extension, different semantic kind” case today, but it could be possible in the future yes
There was a problem hiding this comment.
Okay, let's leave it as is then
5b879c5 to
d6b6d8b
Compare
d6b6d8b to
e3d3f44
Compare
ChouraquiBen
left a comment
ChouraquiBen
left a comment
There was a problem hiding this comment.
Two comments, one is about performance and can imo be overlooked, the other one is a bit more important 🙏
Thanks for the time you're taking to tackle my comments
| // KindForContent overrides the static kind for Terraform plan JSON so line | ||
| // detection can resolve plan resources structurally instead of by text match. | ||
| func (p *Parser) KindForContent(content []byte) (model.FileKind, bool) { | ||
| if contentIsTerraformPlan(content) { |
There was a problem hiding this comment.
Remark: this function calls parseTFPlan and raises the total number of parseTFPlan usage to 3 per TFPlan. There should be 1 or 2 TFPlan scanned per repo but this is to be noticed
| // structured gjson lookup (precise, handles array indices and numeric map keys). On | ||
| // failure it strips numerics and returns the remainder for the text-matcher fallback. | ||
| // Returns (nil, nil) when no numeric segment is present. | ||
| func handleArrayIndex( |
There was a problem hiding this comment.
Question: Here, the array index is being read but in [1], the bracket values have been taken away. Is the behaviour broken or am I missing the place where they are being populated back?
There was a problem hiding this comment.
Not broken, the values are restored in extractStructuralPath. GetBracketValues captures [name] as a pair ["[name]", "name"] in extractedString. The sanitization loop replaces the full bracket expression [name] with {{0}} so the dot-split works cleanly. Then extractStructuralPath replaces {{0}} back with ext[1] = "name" (the value without brackets) before passing the path to GetLineBySearchLine. gjson expects plain dotted keys, not bracket notation, so stripping [] while preserving the value is intentional
… with a fast byte-level heuristic.
| // co-presence is a reliable signal. Used by KindForContent and StringifyContent | ||
| // where a full parseTFPlan call would be redundant (Parse already does it). | ||
| func looksLikeTerraformPlan(content []byte) bool { | ||
| return bytes.Contains(content, []byte(`"format_version"`)) && |
|
Thanks for going through all the comments 🙏 |
2 participants
Motivation
Terraform plan JSON is remapped under a top-level
resourcekey, so line detection cannot rely on text matching for attribute-level findings. Rules today duplicate location hints via asearchLinefield alongsidesearchKey. This change lets plan files resolve lines fromsearchKeyalone so rules can dropsearchLinefor attribute-level paths.Changes
File kind
Introduce
KindTerraformPlanand set it when the JSON parser recognizes Terraform plan content, so plan files are routed to the correct line-detection path.Line detection
For
KindTerraformPlan, convert bracket-stylesearchKeyvalues (e.g.aws_s3_bucket[b].acl) into a structural path underresource.<type>.<name>.<attr>and resolve the line via the existing gjson_dd_lineslookup. Attribute-level keys are resolved structurally; resource-only keys (no attribute segment) still fall back to text matching so existing expected lines are preserved.Tests
Add unit tests for path conversion and an integration test that parses a minimal plan and asserts the resolved line.
Author Checklist
QA Instruction
go test ./pkg/detector/... ./pkg/parser/...go test ./...go mod edit -replace github.com/DataDog/datadog-iac-scanner=../datadog-iac-scanner) and run./tool.sh test.searchLinefrom a Terraform rule with a plan JSON fixture (e.g.terraform-alicloud-rds-instance-address-publicly-accessible) and confirm plan line numbers still match expected results.Blast Radius
Datadog IaC Scanner line detection for Terraform plan JSON files. No change to
.tfHCL detection, CloudFormation, Kubernetes, or other JSON/YAML formats.Additional Notes
This PR does not read the rego
searchLinefield. The engine still honorssearchLinewhen rules provide it (searchLineCalculatorinvulnerability_builder.go); follow-up work in the default-rules repo can remove those fields once this ships. Resource-onlysearchKeyvalues on plans continue to use text matching rather than structural lookup.I submit this contribution under the Apache-2.0 license.