Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (8)
WalkthroughGCP scraping now supports organization roots, project lists, and a singular project alias. It resolves resource ancestry, scopes IAM and Security Center requests, attributes audit logs and Cloud SQL backups to affected projects, and updates schemas, validation, and tests. ChangesGCP organization and multi-project scope
Sequence Diagram(s)sequenceDiagram
participant Scrape
participant TargetResolver
participant AssetInventory
participant IAM
participant SecurityCenter
participant BigQuery
Scrape->>TargetResolver: resolve organization and project roots
TargetResolver->>AssetInventory: list organization projects when needed
TargetResolver-->>Scrape: return qualified parent roots
Scrape->>AssetInventory: fetch assets for each parent
Scrape->>IAM: fetch IAM policies for each parent
Scrape->>SecurityCenter: list findings for each parent
Scrape->>BigQuery: query the configured audit-log dataset
BigQuery-->>Scrape: return affected project IDs
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
BenchstatBase: ✅ No significant performance changes detectedFull benchstat output |
Gavel summary
Totals: 1078 passed · 0 failed · 5 skipped · 3m37s |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scrapers/gcp/audit_logs.go (1)
197-217: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReconcile existing GCP audit log access rows when changing the affected project.
FetchAuditLogsnow keysconfig_access.idandconfig_external_idon the audit event’s affected project instead of the configured BigQuery dataset project. Existing single-project scrapes that already populatedauditedProjectwill generate new access IDs and points while the olddeleted_at <> nullrows remain unless the reconcile path deletes orphaned accesses, or unless affected-project logs currently only come from an aggregated audit sink. Document the key change in upgrade notes and add a migration or reconcile clean-up if orphaned rows are possible.🤖 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 `@scrapers/gcp/audit_logs.go` around lines 197 - 217, Update FetchAuditLogs and its reconciliation flow to clean up previously persisted access rows keyed to the configured dataset project when affectedProject changes, deleting or reconciling orphaned rows so stale deleted_at entries do not remain. Preserve the new affectedProject-based ExternalID and access ID generation, and document this key change in the upgrade notes.
🧹 Nitpick comments (10)
scrapers/gcp/cloudsql_backup_test.go (1)
100-109: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the
errorscounter.
errorsis an int counter here. The package under test uses the standarderrorspackage in scrapers/gcp/cloudsql_backup.go. The name compiles because this file does not importerrors, but it invites a shadowing mistake if an import is added later.♻️ Proposed rename
var changes []v1.ChangeResult - var errors int + var errorCount int for _, result := range results { changes = append(changes, result.Changes...) if result.Error != nil { - errors++ + errorCount++ Expect(result.Error.Error()).To(ContainSubstring("gcp-proj-2")) } } - Expect(errors).To(Equal(1)) + Expect(errorCount).To(Equal(1))🤖 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 `@scrapers/gcp/cloudsql_backup_test.go` around lines 100 - 109, Rename the integer counter currently named errors in the results loop to a more specific name, and update its increment and final Expect assertion accordingly; leave the standard errors package and other result handling unchanged.scrapers/gcp/cloudsql_backup.go (1)
104-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLog instances that are dropped for a missing project.
instancesByProjectsilently discards any instance whose project is empty. Under an organization root,projectFromParentreturns an empty fallback, so an instance whose ancestry resolution failed loses its operation changes with no signal. Add a warning so the gap is visible in scrape logs.♻️ Proposed observability change
-func instancesByProject(instances []instanceInfo) map[string][]instanceInfo { +func instancesByProject(ctx *GCPContext, instances []instanceInfo) map[string][]instanceInfo { grouped := make(map[string][]instanceInfo) for _, instance := range instances { if instance.project == "" { + ctx.Warnf("gcp cloudsql: skipping operations for instance %s, its project is unresolved", instance.name) continue } grouped[instance.project] = append(grouped[instance.project], instance) } return grouped }🤖 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 `@scrapers/gcp/cloudsql_backup.go` around lines 104 - 116, Update instancesByProject to emit a scrape warning whenever an instance has an empty project before skipping it, using the existing logging mechanism and including enough instance context to identify the dropped instance. Preserve the current grouping behavior for instances with a non-empty project.scrapers/gcp/iam_test.go (1)
537-541: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMerge the duplicated role loop.
Lines 538-541 iterate the same three roles as the loop that ends at Line 537 and call
findRoleConfiga second time. Move the parent assertion into the first loop and reuserc.♻️ Proposed merge
g.Expect(rc.Aliases).To(gomega.ContainElement(role)) - } - for _, role := range []string{roleOwner, roleStorage, roleCustom} { - g.Expect(findRoleConfig(res.RoleConfigs, role).Parents).To(gomega.ContainElement( + g.Expect(rc.Parents).To(gomega.ContainElement( v1.ConfigExternalKey{Type: v1.GCPProject, ExternalID: project}, )) }🤖 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 `@scrapers/gcp/iam_test.go` around lines 537 - 541, Merge the duplicated role iteration in the IAM test by moving the parent assertion into the preceding loop, reusing its existing role configuration variable (rc) instead of calling findRoleConfig again. Remove the second loop while preserving the assertions for roleOwner, roleStorage, and roleCustom.scrapers/gcp/iam_hierarchy.go (1)
53-61: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winA non-project, non-organization parent would produce a wrong
OrganizationID.Line 53 treats any parent without a project segment as an organization root. For a hypothetical
folders/123parent,strings.TrimPrefixdoes not matchorganizationPrefix, soOrganizationIDbecomes"folders/123". That value is then stamped as the tenant on every identity throughscopeFor.The current configuration surface exposes only
organization,projects, andproject, so this path is unreachable today. Add an explicit prefix check so a future folder root fails loudly instead of tenanting incorrectly.♻️ Proposed guard
if projectFromParent(parent) == "" { - hierarchy := resourceHierarchy{OrganizationID: strings.TrimPrefix(parent, organizationPrefix)} + organization, ok := strings.CutPrefix(parent, organizationPrefix) + if !ok { + return resourceHierarchy{}, fmt.Errorf("unsupported GCP scrape root %q", parent) + } + hierarchy := resourceHierarchy{OrganizationID: organization} node, err := fetchResourceManagerNode(ctx, service, parent)🤖 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 `@scrapers/gcp/iam_hierarchy.go` around lines 53 - 61, Update the non-project branch in the resource hierarchy logic around projectFromParent to validate that parent starts with organizationPrefix before constructing resourceHierarchy. Return an error for any unsupported non-project, non-organization parent such as a folder, and preserve the existing organization-root handling only for valid organization parents.scrapers/gcp/iam.go (1)
334-342: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
coalesceIAMRoleConfigsmutates the caller'sConfigmap in place.
existing.Configand the duplicateresult.Configare distinct maps, buttargetis the same map instance held by the input slice element that was kept. Writing into it mutates a value the caller still owns.processResultsdiscards the input slice, so there is no current defect. Copy the map before merging if this function is later reused on results that are retained.🤖 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 `@scrapers/gcp/iam.go` around lines 334 - 342, Update coalesceIAMRoleConfigs so merging duplicate IAM role configs does not mutate the map owned by the retained input element. Before adding missing entries from result.Config, create a copy of existing.Config, merge into that copy, and assign the copy to the retained result; preserve existing values when keys overlap.scrapers/gcp/gcp.go (2)
438-443: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
securityCenterParentsis an identity function.The function returns its argument unchanged. It adds a call layer without logic. Inline it and keep the explanation as a comment at the call site.
♻️ Proposed simplification
-// securityCenterParents returns the already-resolved scrape roots. An -// unrestricted organization has one organization root, while a narrowed -// organization has only its selected project roots. -func securityCenterParents(parents []string) []string { - return parents -} -- for _, parent := range securityCenterParents(parents) { + for _, parent := range parents {🤖 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 `@scrapers/gcp/gcp.go` around lines 438 - 443, Remove the redundant securityCenterParents function and inline its callers to use the resolved parents slice directly. Preserve the explanation about organization and project roots as a comment at the relevant call site.
288-293: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueIndex parity between
resultsandancestriesis correct but implicit.Both slices are appended only together at Lines 350-354, so
ancestries[i]always matchesresults[i]. Any futurecontinueadded between the two appends would break this pairing and panic or mis-attribute projects. Consider storing the ancestry on a small struct alongside the result, or adding a length assertion before the resolution loop.♻️ Optional: make the pairing explicit
for i := range results { + // results and ancestries are appended in lockstep above. ancestry := ancestries[i]Also applies to: 304-305, 351-384
🤖 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 `@scrapers/gcp/gcp.go` around lines 288 - 293, Make the relationship between each result and its ancestry explicit in the flow around resolver, results, and ancestries. Prefer storing both values together in a small paired structure and update the append and resolution loop accordingly; if retaining separate slices, add a length assertion before resolution to prevent mismatched indexing.scrapers/gcp/audit_logs_test.go (1)
117-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the organization-scope acceptance case.
Both assertions at Lines 117-121 pass because
BigQueryRow{}has an emptyProjectID. The organization entry therefore proves only the empty-project rejection, not the organization behavior. The documented organization path — an organization parent produces noscopedProjects, so every resolved project is accepted — has no coverage.💚 Proposed additional case
_, ok = auditLogAffectedProject(BigQueryRow{}, []string{"organizations/1234"}) g.Expect(ok).To(gomega.BeFalse(), "an organization scrape must not fall back to the sink project") + + project, ok = auditLogAffectedProject( + BigQueryRow{ProjectID: "any-member-project"}, + []string{"organizations/1234"}, + ) + g.Expect(ok).To(gomega.BeTrue(), "an organization scrape accepts every affected project") + g.Expect(project).To(gomega.Equal("any-member-project")) }🤖 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 `@scrapers/gcp/audit_logs_test.go` around lines 117 - 122, Add an organization-scope acceptance test alongside the existing auditLogAffectedProject cases, using a BigQueryRow with a non-empty project ID and organization scope input such as organizations/1234. Assert that the project is returned and the success flag is true, covering the documented behavior that organization scope accepts every resolved project.scrapers/gcp/iam_scope_test.go (1)
61-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis spec does not exercise
fetchResourceManagerHierarchy.The
Describeblock is namedfetchResourceManagerHierarchy salvage, but the body builds aresourceHierarchyliteral and calls onlyscopeFor.scopeForis already covered at Lines 14-32. The documented salvage behavior —fetchResourceManagerHierarchyrecordingOrganizationIDfrom the parent chain before a node read fails — has no coverage.Rename the block to describe what it checks, or extend
fetchResourceManagerHierarchywith an injectable node fetcher so the salvage path can be tested directly. I can draft the injectable-fetcher version if that helps.🤖 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 `@scrapers/gcp/iam_scope_test.go` around lines 61 - 69, The test labeled for fetchResourceManagerHierarchy salvage only verifies scopeFor and does not cover the documented traversal behavior. Rename the Describe block and test description to reflect the scopeFor organization tenant mapping they actually exercise, or modify fetchResourceManagerHierarchy to accept an injectable node fetcher and add a test that triggers a node-read failure after recording the parent OrganizationID.api/v1/gcp.go (1)
49-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider CEL admission validation for organization/projects requirement.
Validate()requires an organization or at least one project, but this check only runs at scrape time. Ifgcp.Organizationis empty andgcp.ConfiguredProjects()has no entries, the config returns "one of organization or projects must be set". AScrapeConfigmissing both fields is still accepted by the Kubernetes API server, and the invalid state only surfaces as a scrape-time error rather than atkubectl applytime.Since the CRD is generated with
controller-gen.kubebuilder.io/version: v0.19.0, add a+kubebuilder:validation:XValidationCEL rule on theGCPstruct to enforce this at admission time, then regenerate the CRD.Also applies to: 136-142
🤖 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 `@api/v1/gcp.go` around lines 49 - 99, The GCP configuration currently defers the required organization-or-projects check to scrape time. Add a kubebuilder XValidation CEL rule on the GCP struct that rejects configurations where Organization is empty and the effective project list, including the Project alias via ConfiguredProjects(), has no entries; then regenerate the CRD using the existing controller-gen setup.
🤖 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 `@fixtures/gcp.yaml`:
- Around line 8-13: Update the include-related fixture comments near the
organization/projects example to clarify that IAMPolicy and IAMGroupMembers run
by default only when include is empty. Extend the asset-type filtering example
to explicitly list the IAM collection flags so enabling the filter does not omit
IAM data unintentionally.
In `@scrapers/gcp/iam_hierarchy.go`:
- Around line 259-271: Document the Resource Manager conditional-binding
behavior change around the binding conversion in the IAM hierarchy flow:
bindings with a non-nil Condition are preserved but excluded from effective
access by buildIAMAccess, which emits a bounded warning. Include an
operator-facing change notice for users relying on the previous unconditional
edges; no implementation changes are needed.
---
Outside diff comments:
In `@scrapers/gcp/audit_logs.go`:
- Around line 197-217: Update FetchAuditLogs and its reconciliation flow to
clean up previously persisted access rows keyed to the configured dataset
project when affectedProject changes, deleting or reconciling orphaned rows so
stale deleted_at entries do not remain. Preserve the new affectedProject-based
ExternalID and access ID generation, and document this key change in the upgrade
notes.
---
Nitpick comments:
In `@api/v1/gcp.go`:
- Around line 49-99: The GCP configuration currently defers the required
organization-or-projects check to scrape time. Add a kubebuilder XValidation CEL
rule on the GCP struct that rejects configurations where Organization is empty
and the effective project list, including the Project alias via
ConfiguredProjects(), has no entries; then regenerate the CRD using the existing
controller-gen setup.
In `@scrapers/gcp/audit_logs_test.go`:
- Around line 117-122: Add an organization-scope acceptance test alongside the
existing auditLogAffectedProject cases, using a BigQueryRow with a non-empty
project ID and organization scope input such as organizations/1234. Assert that
the project is returned and the success flag is true, covering the documented
behavior that organization scope accepts every resolved project.
In `@scrapers/gcp/cloudsql_backup_test.go`:
- Around line 100-109: Rename the integer counter currently named errors in the
results loop to a more specific name, and update its increment and final Expect
assertion accordingly; leave the standard errors package and other result
handling unchanged.
In `@scrapers/gcp/cloudsql_backup.go`:
- Around line 104-116: Update instancesByProject to emit a scrape warning
whenever an instance has an empty project before skipping it, using the existing
logging mechanism and including enough instance context to identify the dropped
instance. Preserve the current grouping behavior for instances with a non-empty
project.
In `@scrapers/gcp/gcp.go`:
- Around line 438-443: Remove the redundant securityCenterParents function and
inline its callers to use the resolved parents slice directly. Preserve the
explanation about organization and project roots as a comment at the relevant
call site.
- Around line 288-293: Make the relationship between each result and its
ancestry explicit in the flow around resolver, results, and ancestries. Prefer
storing both values together in a small paired structure and update the append
and resolution loop accordingly; if retaining separate slices, add a length
assertion before resolution to prevent mismatched indexing.
In `@scrapers/gcp/iam_hierarchy.go`:
- Around line 53-61: Update the non-project branch in the resource hierarchy
logic around projectFromParent to validate that parent starts with
organizationPrefix before constructing resourceHierarchy. Return an error for
any unsupported non-project, non-organization parent such as a folder, and
preserve the existing organization-root handling only for valid organization
parents.
In `@scrapers/gcp/iam_scope_test.go`:
- Around line 61-69: The test labeled for fetchResourceManagerHierarchy salvage
only verifies scopeFor and does not cover the documented traversal behavior.
Rename the Describe block and test description to reflect the scopeFor
organization tenant mapping they actually exercise, or modify
fetchResourceManagerHierarchy to accept an injectable node fetcher and add a
test that triggers a node-read failure after recording the parent
OrganizationID.
In `@scrapers/gcp/iam_test.go`:
- Around line 537-541: Merge the duplicated role iteration in the IAM test by
moving the parent assertion into the preceding loop, reusing its existing role
configuration variable (rc) instead of calling findRoleConfig again. Remove the
second loop while preserving the assertions for roleOwner, roleStorage, and
roleCustom.
In `@scrapers/gcp/iam.go`:
- Around line 334-342: Update coalesceIAMRoleConfigs so merging duplicate IAM
role configs does not mutate the map owned by the retained input element. Before
adding missing entries from result.Config, create a copy of existing.Config,
merge into that copy, and assign the copy to the retained result; preserve
existing values when keys overlap.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: 28495f7d-9678-4687-a32d-f61435b5e35f
📒 Files selected for processing (23)
api/v1/gcp.goapi/v1/gcp_test.goapi/v1/zz_generated.deepcopy.gochart/crds/configs.flanksource.com_scrapeconfigs.yamlconfig/schemas/config_gcp.schema.jsonconfig/schemas/scrape_config.schema.jsonfixtures/gcp.yamlscrapers/gcp/ancestry.goscrapers/gcp/ancestry_test.goscrapers/gcp/audit_logs.goscrapers/gcp/audit_logs_test.goscrapers/gcp/cloudsql_backup.goscrapers/gcp/cloudsql_backup_test.goscrapers/gcp/gcp.goscrapers/gcp/gcp_test.goscrapers/gcp/iam.goscrapers/gcp/iam_group_members.goscrapers/gcp/iam_hierarchy.goscrapers/gcp/iam_scope_test.goscrapers/gcp/iam_test.goscrapers/gcp/security_center.goscrapers/gcp/targets.goscrapers/gcp/targets_test.go
2 participants
Fixes: #2341
Summary by CodeRabbit
New Features
Bug Fixes
Documentation