Feat/full vpc checks#10
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThis PR refactors the plugin into a multi-region, multi-resource AWS VPC compliance evaluator: region resolution, paginated collectors, a generic policy-evaluation engine, evidence-context builders for VPC networking resources, per-resource policy-input builders, orchestration dispatching, subject templates, and tests. ChangesMulti-resource VPC compliance evaluation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build-and-upload.yml:
- Line 25: Replace the unsafe inline expression in the run command by injecting
github.ref_name via a GitHub Actions environment variable and reference that env
var in the YAML expression (so the expansion happens at workflow evaluation, not
via shell interpolation); set env: REF_NAME: ${{ github.ref_name }} for the step
and update the run entry that currently contains "gooci upload dist/ ghcr.io/${{
github.repository_owner }}/${{ github.event.repository.name
}}:${{github.ref_name}} ..." to use the REF_NAME env (e.g., ...:${{ env.REF_NAME
}}), ensuring the run step's command string no longer directly interpolates
github.ref_name in the shell.
In `@internal/evidence.go`:
- Around line 349-352: The Value is printing the pointer for acl.IsDefault (a
*bool) using fmt.Sprintf("%v"); update the mapping to dereference safely by
converting to a bool first (e.g., use aws.ToBool(acl.IsDefault) inside the
fmt.Sprintf or use strconv.FormatBool(aws.ToBool(acl.IsDefault))) so the actual
boolean value is logged for the Name "is-default".
- Around line 209-212: The code is formatting a *bool pointer
(subnet.MapPublicIpOnLaunch) which prints the pointer address; update the
formatting to dereference safely using aws.ToBool so the actual boolean is
logged — replace the current fmt.Sprintf("%v", subnet.MapPublicIpOnLaunch) usage
with fmt.Sprintf("%v", aws.ToBool(subnet.MapPublicIpOnLaunch)) (or use "%t" with
aws.ToBool) where the Name "map-public-ip-on-launch" entry is created in
internal/evidence.go.
- Around line 125-128: The evidence stringification is incorrectly formatting
*bool pointers (producing "&true"/"&false"/"&<nil>"); update the three places
that build proto.Property.Value to safely dereference the pointers and produce
"true"/"false" strings (treat nil as false) for vpc.IsDefault,
subnet.MapPublicIpOnLaunch, and acl.IsDefault—e.g., replace fmt.Sprintf("%v",
vpc.IsDefault) with a boolean-to-string conversion that checks for nil and uses
the dereferenced value (same fix for subnet.MapPublicIpOnLaunch and
acl.IsDefault); ensure all three locations in internal/evidence.go use the same
nil-safe conversion.
In `@internal/network_acl_context_test.go`:
- Line 74: The unchecked type assertions on contextMap["current"] and the other
similar assertion cause forcetypeassert lint errors; replace them with comma-ok
style checks: retrieve contextMap["current"] as v, ok :=
contextMap["current"].(map[string]interface{}) (and the other assertion
similarly), and if !ok call t.Fatalf (or t.Fatalf/require.Errorf) with a clear
error message so the test fails cleanly instead of panicking; locate the
assertions by the symbols contextMap and the "current" key in
internal/network_acl_context_test.go and update both occurrences.
In `@internal/network_acl_test.go`:
- Around line 41-46: Replace direct proto field access in the test with the
generated getter to satisfy protogetter and avoid nil panics: when asserting
titles use evidences[0].GetTitle() and evidences[1].GetTitle() (instead of
evidences[0].Title / evidences[1].Title) inside the two t.Fatalf checks in
network_acl_test.go so the linter is happy and the checks remain nil-safe.
In `@internal/policy_evaluation.go`:
- Line 120: Replace the direct proto field access evidence.Title with the
generated getter: use evidence.GetTitle() when assigning to title (the
assignment currently using title := strings.TrimSpace(evidence.Title)); update
that line to call evidence.GetTitle(), leaving the subsequent assignments on
Lines 122/126 unchanged so the rest of the function (policy evaluation) uses the
getter rather than direct field access.
In `@internal/region_datasets.go`:
- Around line 30-31: CollectRegionDatasets currently uses the ec2.Client and
cloudwatchlogs.Client (symbols: client, logsClient) without nil checks which can
panic when a required dataset is requested; update CollectRegionDatasets to
validate client and logsClient early: before calling any EC2 or CloudWatch Logs
specific collection functions (the branches that consult requiredDatasets for
EC2-related and logs-related keys), check if the corresponding client is nil and
if so return a clear error indicating the missing client for that required
dataset, or skip non-required datasets; ensure the guards cover all code paths
where client or logsClient are dereferenced (including the code at the
alternative branch mentioned around the later dataset collection calls).
- Around line 44-50: The vpc_attributes path assumes datasets.Vpcs is populated,
causing empty results when requiredDatasets["vpc_attributes"] is true but
requiredDatasets["vpcs"] is false; update the logic so that before calling
CollectVpcAttributes you ensure datasets.Vpcs is populated—if
requiredDatasets["vpcs"] is false then call the VPC collection function (e.g.,
CollectVpcs(ctx, client)) to populate datasets.Vpcs, handle and propagate errors
from that call, and only then call CollectVpcAttributes(ctx, client,
datasets.Vpcs); reference the symbols requiredDatasets, "vpc_attributes",
"vpcs", datasets.Vpcs, CollectVpcs, and CollectVpcAttributes to locate and
change the branch in RegionDatasets construction.
In `@internal/route_table_context_test.go`:
- Line 66: The test currently uses unchecked type assertions like
contextMap["current"].(map[string]interface{}) which can panic; change these to
the comma-ok form (e.g., val, ok :=
contextMap["current"].(map[string]interface{})) and handle the false case with a
test failure (t.Fatalf or require/assert) so shape mismatches fail cleanly;
apply the same pattern to other unchecked assertions in this file (the
[]interface{} and map[string]interface{} casts around the code paths near the
usages of contextMap, the variables named like current and the slices at the
other noted locations).
In `@internal/subnet_test.go`:
- Around line 41-44: The test reads proto fields directly (evidences[0].Title
and evidences[1].Title) which trips the protogetter linter; update the
assertions to use the generated getters (evidences[0].GetTitle() and
evidences[1].GetTitle()) instead so the linter is satisfied and behavior remains
identical—locate the assertions in the test that reference Title and replace
with GetTitle().
In `@internal/util.go`:
- Around line 43-45: The current branch returns strings.TrimSpace(regionStr)
even when regionStr is whitespace-only; change the guard to trim first and only
return a slice when the trimmed value is non-empty (e.g., let trimmed :=
strings.TrimSpace(regionStr); if trimmed != "" { return []string{trimmed} });
otherwise fall through (return nil or the existing default) so downstream code
never receives an empty/invalid region string.
In `@Makefile`:
- Around line 26-37: Make the Makefile targets explicit phony targets to prevent
accidental collisions with files named test, clean, build, run (and help) by
adding a .PHONY declaration; update the Makefile to include a line such as
".PHONY: test clean build run help" (placed near the top or alongside the target
definitions) so make always runs these recipes regardless of filesystem entries,
keeping the existing target names and behaviors (test, clean, build, run, help).
In `@README.md`:
- Around line 80-84: Replace the backticked repo names listed
(`plugin-aws-vpc-policies`, `plugin-aws-vpc-subnet-policies`,
`plugin-aws-vpc-sg-policies`, `plugin-aws-vpc-nacl-policies`,
`plugin-aws-vpc-rt-policies`) with Markdown inline links to their repository
URLs (e.g.
[plugin-aws-vpc-policies](https://github.com/your-org/plugin-aws-vpc-policies));
remove the surrounding backticks so they render as clickable links and ensure
each link points to the correct repo URL.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2545e220-167d-45d8-88f3-621e191b5c06
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (33)
.github/workflows/build-and-upload.yml.gitignoreMakefileREADME.mdgo.modinternal/evidence.gointernal/flow_log.gointernal/internet_gateway.gointernal/log_group.gointernal/network_acl.gointernal/network_acl_context.gointernal/network_acl_context_test.gointernal/network_acl_test.gointernal/pagination.gointernal/policy_evaluation.gointernal/region_datasets.gointernal/route_table.gointernal/route_table_context.gointernal/route_table_context_test.gointernal/security_group.gointernal/subnet.gointernal/subnet_context.gointernal/subnet_context_test.gointernal/subnet_test.gointernal/types.gointernal/util.gointernal/vpc.gointernal/vpc_context.gointernal/vpc_context_test.gointernal/vpc_endpoint.gointernal/vpc_test.gomain.gomain_test.go
Comment on lines
+26
to
+37
| test: | ||
| @go test ./... | ||
|
|
||
| clean: # Cleanup build artifacts | ||
| @rm -rf dist/* | ||
|
|
||
| build: clean ## Build the plugin package | ||
| @mkdir -p dist/ | ||
| @go build -o dist/plugin main.go | ||
|
|
||
| run: build ## Execute the Concom agent with the built plugin | ||
| @../agent/dist/./concom agent --config ./.config/config.yaml No newline at end of file |
There was a problem hiding this comment.
Declare command targets as phony to avoid file-name collisions.
If files named test, clean, build, or run exist, make may skip these recipes unexpectedly. Add .PHONY for these targets (and help targets).
Suggested patch
+ .PHONY: help help-all test clean build run
+
test:
`@go` test ./...📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test: | |
| @go test ./... | |
| clean: # Cleanup build artifacts | |
| @rm -rf dist/* | |
| build: clean ## Build the plugin package | |
| @mkdir -p dist/ | |
| @go build -o dist/plugin main.go | |
| run: build ## Execute the Concom agent with the built plugin | |
| @../agent/dist/./concom agent --config ./.config/config.yaml | |
| .PHONY: help help-all test clean build run | |
| test: | |
| `@go` test ./... | |
| clean: # Cleanup build artifacts | |
| `@rm` -rf dist/* | |
| build: clean ## Build the plugin package | |
| `@mkdir` -p dist/ | |
| `@go` build -o dist/plugin main.go | |
| run: build ## Execute the Concom agent with the built plugin | |
| @../agent/dist/./concom agent --config ./.config/config.yaml |
🧰 Tools
🪛 checkmake (0.3.2)
[warning] 36-36: Required target "all" is missing from the Makefile.
(minphony)
[warning] 36-36: Required target "clean" must be declared PHONY.
(minphony)
[warning] 36-36: Required target "test" must be declared PHONY.
(minphony)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Makefile` around lines 26 - 37, Make the Makefile targets explicit phony
targets to prevent accidental collisions with files named test, clean, build,
run (and help) by adding a .PHONY declaration; update the Makefile to include a
line such as ".PHONY: test clean build run help" (placed near the top or
alongside the target definitions) so make always runs these recipes regardless
of filesystem entries, keeping the existing target names and behaviors (test,
clean, build, run, help).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@main_test.go`:
- Around line 41-58: The test
TestSupportedPolicyBehaviorsOnlyIncludesPrimaryVpcBundles currently only checks
presence/absence but doesn't fail if extra behaviors are added; update it to
assert the exact set returned by supportedPolicyBehaviors(): build an expected
set (e.g. {"vpc","subnet","sg","acl","rt"}), convert supportedPolicyBehaviors()
into a set, assert the lengths match and that every item in the supported set
exists in the expected set (or vice versa), and fail with a clear message if
there's any difference; use the supportedPolicyBehaviors() call and the test
name to locate where to replace the current loose membership checks with this
exact-set comparison.
In `@subject_templates_test.go`:
- Around line 19-54: Replace direct proto field accesses with the generated
nil-safe getters throughout the tests: use template.GetName() instead of
template.Name, subjectTemplate.GetType() for subjectTemplate.Type,
subjectTemplate.GetSelectorLabels() for subjectTemplate.SelectorLabels,
subjectTemplate.GetLabelSchema() for subjectTemplate.LabelSchema,
subjectTemplate.GetIdentityLabelKeys() for subjectTemplate.IdentityLabelKeys,
and selector.GetKey() for selector.Key; update the code paths in
TestSubjectTemplatesHaveSelectorsAndSchemasForIdentity and the earlier loop that
builds the names map to call these getters so the protogetter linter is
satisfied and nil-safe access is ensured.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 48bd82fe-6f31-41ff-8303-ff0910f920bd
📒 Files selected for processing (6)
internal/evidence.gointernal/evidence_labels_test.gomain.gomain_test.gosubject_templates.gosubject_templates_test.go
Comment on lines
+19
to
+54
| for _, template := range templates { | ||
| names[template.Name] = true | ||
| } | ||
|
|
||
| for _, expected := range []string{ | ||
| "aws-vpc", | ||
| "aws-vpc-subnet", | ||
| "aws-vpc-security-group", | ||
| "aws-vpc-network-acl", | ||
| "aws-vpc-route-table", | ||
| } { | ||
| if !names[expected] { | ||
| t.Fatalf("missing subject template %s", expected) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func TestSubjectTemplatesHaveSelectorsAndSchemasForIdentity(t *testing.T) { | ||
| for _, subjectTemplate := range buildSubjectTemplates() { | ||
| if subjectTemplate.Type != proto.SubjectType_SUBJECT_TYPE_COMPONENT { | ||
| t.Fatalf("template %s must use component subject type", subjectTemplate.Name) | ||
| } | ||
| if len(subjectTemplate.SelectorLabels) == 0 { | ||
| t.Fatalf("template %s missing selector labels", subjectTemplate.Name) | ||
| } | ||
| if !containsSchemaKey(subjectTemplate.LabelSchema, "type") { | ||
| t.Fatalf("template %s must declare type selector in label schema", subjectTemplate.Name) | ||
| } | ||
|
|
||
| for _, selector := range subjectTemplate.SelectorLabels { | ||
| if strings.Contains(selector.Key, "-") { | ||
| t.Fatalf("template %s selector key %s must use underscore notation", subjectTemplate.Name, selector.Key) | ||
| } | ||
| } | ||
|
|
||
| for _, identityKey := range subjectTemplate.IdentityLabelKeys { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Use generated getters for proto fields to satisfy protogetter.
golangci-lint (protogetter) flags the direct proto field accesses in this file (e.g. template.Name, subjectTemplate.Type, subjectTemplate.SelectorLabels, subjectTemplate.LabelSchema, subjectTemplate.IdentityLabelKeys, selector.Key). Switch to the nil-safe getters to avoid lint failures.
♻️ Representative changes
- names[template.Name] = true
+ names[template.GetName()] = true- if subjectTemplate.Type != proto.SubjectType_SUBJECT_TYPE_COMPONENT {
- t.Fatalf("template %s must use component subject type", subjectTemplate.Name)
+ if subjectTemplate.GetType() != proto.SubjectType_SUBJECT_TYPE_COMPONENT {
+ t.Fatalf("template %s must use component subject type", subjectTemplate.GetName())
}
- if len(subjectTemplate.SelectorLabels) == 0 {
- t.Fatalf("template %s missing selector labels", subjectTemplate.Name)
+ if len(subjectTemplate.GetSelectorLabels()) == 0 {
+ t.Fatalf("template %s missing selector labels", subjectTemplate.GetName())
}Apply the same getter substitution to the remaining flagged accesses (GetLabelSchema(), GetIdentityLabelKeys(), selector.GetKey()).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for _, template := range templates { | |
| names[template.Name] = true | |
| } | |
| for _, expected := range []string{ | |
| "aws-vpc", | |
| "aws-vpc-subnet", | |
| "aws-vpc-security-group", | |
| "aws-vpc-network-acl", | |
| "aws-vpc-route-table", | |
| } { | |
| if !names[expected] { | |
| t.Fatalf("missing subject template %s", expected) | |
| } | |
| } | |
| } | |
| func TestSubjectTemplatesHaveSelectorsAndSchemasForIdentity(t *testing.T) { | |
| for _, subjectTemplate := range buildSubjectTemplates() { | |
| if subjectTemplate.Type != proto.SubjectType_SUBJECT_TYPE_COMPONENT { | |
| t.Fatalf("template %s must use component subject type", subjectTemplate.Name) | |
| } | |
| if len(subjectTemplate.SelectorLabels) == 0 { | |
| t.Fatalf("template %s missing selector labels", subjectTemplate.Name) | |
| } | |
| if !containsSchemaKey(subjectTemplate.LabelSchema, "type") { | |
| t.Fatalf("template %s must declare type selector in label schema", subjectTemplate.Name) | |
| } | |
| for _, selector := range subjectTemplate.SelectorLabels { | |
| if strings.Contains(selector.Key, "-") { | |
| t.Fatalf("template %s selector key %s must use underscore notation", subjectTemplate.Name, selector.Key) | |
| } | |
| } | |
| for _, identityKey := range subjectTemplate.IdentityLabelKeys { | |
| for _, template := range templates { | |
| names[template.GetName()] = true | |
| } | |
| for _, expected := range []string{ | |
| "aws-vpc", | |
| "aws-vpc-subnet", | |
| "aws-vpc-security-group", | |
| "aws-vpc-network-acl", | |
| "aws-vpc-route-table", | |
| } { | |
| if !names[expected] { | |
| t.Fatalf("missing subject template %s", expected) | |
| } | |
| } | |
| } | |
| func TestSubjectTemplatesHaveSelectorsAndSchemasForIdentity(t *testing.T) { | |
| for _, subjectTemplate := range buildSubjectTemplates() { | |
| if subjectTemplate.GetType() != proto.SubjectType_SUBJECT_TYPE_COMPONENT { | |
| t.Fatalf("template %s must use component subject type", subjectTemplate.GetName()) | |
| } | |
| if len(subjectTemplate.GetSelectorLabels()) == 0 { | |
| t.Fatalf("template %s missing selector labels", subjectTemplate.GetName()) | |
| } | |
| if !containsSchemaKey(subjectTemplate.GetLabelSchema(), "type") { | |
| t.Fatalf("template %s must declare type selector in label schema", subjectTemplate.GetName()) | |
| } | |
| for _, selector := range subjectTemplate.GetSelectorLabels() { | |
| if strings.Contains(selector.GetKey(), "-") { | |
| t.Fatalf("template %s selector key %s must use underscore notation", subjectTemplate.GetName(), selector.GetKey()) | |
| } | |
| } | |
| for _, identityKey := range subjectTemplate.GetIdentityLabelKeys() { |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 20-20: avoid direct access to proto field template.Name, use template.GetName() instead
(protogetter)
[error] 38-38: avoid direct access to proto field subjectTemplate.Type, use subjectTemplate.GetType() instead
(protogetter)
[error] 39-39: avoid direct access to proto field subjectTemplate.Name, use subjectTemplate.GetName() instead
(protogetter)
[error] 41-41: avoid direct access to proto field subjectTemplate.SelectorLabels, use subjectTemplate.GetSelectorLabels() instead
(protogetter)
[error] 42-42: avoid direct access to proto field subjectTemplate.Name, use subjectTemplate.GetName() instead
(protogetter)
[error] 44-44: avoid direct access to proto field subjectTemplate.LabelSchema, use subjectTemplate.GetLabelSchema() instead
(protogetter)
[error] 45-45: avoid direct access to proto field subjectTemplate.Name, use subjectTemplate.GetName() instead
(protogetter)
[error] 48-48: avoid direct access to proto field subjectTemplate.SelectorLabels, use subjectTemplate.GetSelectorLabels() instead
(protogetter)
[error] 49-49: avoid direct access to proto field selector.Key, use selector.GetKey() instead
(protogetter)
[error] 54-54: avoid direct access to proto field subjectTemplate.IdentityLabelKeys, use subjectTemplate.GetIdentityLabelKeys() instead
(protogetter)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@subject_templates_test.go` around lines 19 - 54, Replace direct proto field
accesses with the generated nil-safe getters throughout the tests: use
template.GetName() instead of template.Name, subjectTemplate.GetType() for
subjectTemplate.Type, subjectTemplate.GetSelectorLabels() for
subjectTemplate.SelectorLabels, subjectTemplate.GetLabelSchema() for
subjectTemplate.LabelSchema, subjectTemplate.GetIdentityLabelKeys() for
subjectTemplate.IdentityLabelKeys, and selector.GetKey() for selector.Key;
update the code paths in TestSubjectTemplatesHaveSelectorsAndSchemasForIdentity
and the earlier loop that builds the names map to call these getters so the
protogetter linter is satisfied and nil-safe access is ensured.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores