feat: configure required tag findings - #316
Conversation
- make cost tag findings opt in through inspector.required_tags - report only missing policy keys and document the configuration
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
WalkthroughThe change adds ChangesInspector required-tag policy
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant InspectorScreen
participant SecurityScanOptions
participant CostWasteScanner
participant AWSResource
participant untaggedCostFinding
InspectorScreen->>SecurityScanOptions: Set configured RequiredTags
SecurityScanOptions->>CostWasteScanner: Run configured scan
CostWasteScanner->>AWSResource: Inspect resource tags
AWSResource->>untaggedCostFinding: Provide tags and required tags
untaggedCostFinding-->>CostWasteScanner: Return missing-tag finding or no finding
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
The implementation correctly adds configurable required tag enforcement to the Security Inspector. The feature properly threads inspector.required_tags from global configuration through the cost-waste scanner, disables missing-tag findings when no policy is configured, and reports only missing required keys when enabled. The config loading uses defensive copying for the RequiredTags slice, and the inspection logic handles edge cases appropriately (empty policies, duplicate tags, empty strings). Tests validate both configuration parsing and the inspection behavior. No blocking issues found.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
YoungJinJung
left a comment
YoungJinJung
left a comment
There was a problem hiding this comment.
Findings
-
[P2] Preserve exact AWS tag keys during policy matching —
internal/inspector/inspector_rules_cost.go:278-280The new membership map stores
strings.TrimSpace(awssdk.ToString(tag.Key)). EC2 tag keys are case-sensitive strings, so surrounding whitespace is part of the key: a resource tagged" Owner "will currently satisfy a configured required key ofOwner, suppressing the finding even though the resource does not have that exact policy key. Keep the AWS-returned key unchanged when populatingpresent(while trimming configured entries if that is the intended config normalization), and add a regression case proving that a whitespace-different resource tag remains missing.
Reviewed head: f75bb40
- keep AWS tag keys unchanged during policy comparison - cover whitespace-different resource keys with a regression
|
Applied the exact tag-key matching feedback in d5aecf7.
Docs remain unchanged because this corrects the implementation to match the documented exact required-key behavior. |
|
/q review |
There was a problem hiding this comment.
This PR successfully implements configurable required tag findings for the Security Inspector. The implementation is solid with proper threading of configuration, correct disable-by-default behavior when no tags are configured, and comprehensive test coverage. The changes follow established patterns and integrate cleanly with the existing codebase.
Key Strengths:
- Proper configuration flow from YAML → Config → SecurityScanOptions
- Correct conditional logic: rule is disabled when
required_tagslist is empty - Thorough test coverage for both config loading and inspector rule logic
- Clear, specific finding messages that list the actual missing tag names
No blocking issues found. Ready for merge.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
YoungJinJung
left a comment
YoungJinJung
left a comment
There was a problem hiding this comment.
Findings
-
[P2] State the actual required-tag coverage —
README.md:344(alsodocs/project-overview.en.md:36anddocs/project-overview.ko.md:36)The changed text says generic “resources missing configured required tags,” but
requiredTagsis only evaluated frominspectElasticIPs,inspectEBSVolumes,inspectEC2InstancesForWaste, andinspectEBSSnapshotsForWaste; target groups and resources handled by the other Inspector rule packs are not checked. This can give operators a false impression thatinspector.required_tagsenforces an account-wide tagging policy. Keep the documentation scoped to Elastic IPs, EBS volumes/snapshots, and EC2 instances (or extend the implementation if broader coverage is intended).
Reviewed head: d5aecf7
YoungJinJung
left a comment
YoungJinJung
left a comment
There was a problem hiding this comment.
Reviewed head: 70e6b9f
No actionable findings. The documentation-only follow-up accurately limits required-tag coverage to Elastic IPs, EBS volumes and snapshots, and EC2 instances, resolving the prior review finding. The full diff remains aligned with issue #312.
Validation passed: make test, make build, focused config and Inspector regressions, and git diff --check. CI and CodeRabbit are green. The PR is mergeable but still requires an independent approval.
|
/q review |
There was a problem hiding this comment.
This PR successfully implements configurable required-tag policies for Security Inspector scans. The implementation is clean and well-integrated:
Key strengths:
- Properly threads
RequiredTagsconfiguration from config file through to scan execution - Correctly disables missing-tag findings when no policy is configured (empty slice check)
- Reports specific missing tag names when policy is enabled
- Includes comprehensive test coverage including edge cases (duplicates, whitespace, empty values)
- Maintains backward compatibility - existing configs without
required_tagscontinue to work
Implementation quality:
- Defensive programming with proper empty checks and deduplication
- Clean API design using
SecurityScanOptionsstruct for configuration - Proper separation between old generic "untagged" behavior and new specific missing-tag detection
- Good test coverage validating the policy behavior
The code is ready to merge.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
…uired-tags # Conflicts: # docs/project-overview.en.md # docs/project-overview.ko.md
1 participant
Summary
inspector.required_tagsto the global configuration and thread it into Security Inspector scansRelated Issues
Closes #312
Validation
make testmake buildgit diff --checkChecklist
docs/branch-naming-harness.mddocs/documentation-harness.md)docs/pages updated if architecture, auth, config, or workflow changedSummary by CodeRabbit
New Features
Documentation