fix: handle api errors for gcp and cloudflare failure#713
Conversation
- Updated the cloudflare provider to use a more flexible apiClient interface instead of the specific cloudflare.API type. - Improved error handling in the Resources method to return errors directly from the DNS provider. - Ensured that resources are merged only after successful retrieval, enhancing the reliability of resource management.
WalkthroughThe changes introduce an Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Provider
participant dnsProvider
rect rgb(200, 150, 255)
Note over Provider,dnsProvider: Old Behavior (Error Dropped)
Caller->>Provider: Resources(ctx)
Provider->>dnsProvider: GetResource(ctx)
dnsProvider-->>Provider: error (e.g., rate limit)
Provider->>Provider: Check: if err == nil (false)
Provider-->>Caller: finalResources, nil (empty + no error)
end
rect rgb(150, 200, 255)
Note over Provider,dnsProvider: New Behavior (Error Propagated)
Caller->>Provider: Resources(ctx)
Provider->>dnsProvider: GetResource(ctx)
dnsProvider-->>Provider: error (e.g., rate limit)
Provider->>Provider: Check error: not nil
Provider-->>Caller: nil, error (failure surfaced)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/providers/cloudflare/dns.go (1)
39-42: Fix misleading error message and inconsistent error return.Two issues on line 41:
- The error message says "could not list zones" but should say "could not list DNS records" to accurately reflect the failure.
- Returning partial data (
list) with an error is inconsistent with line 35, which returnsnilon error, and contradicts the PR objective to merge resources only after successful retrieval.Apply this diff to fix both issues:
- return list, errors.Wrap(err, "could not list zones") + return nil, errors.Wrap(err, "could not list DNS records")pkg/providers/gcp/gcp.go (1)
372-403: OrganizationProvider.Resources still swallows asset API errors and can return empty inventoriesFor the org-level provider, errors from
getAllAssets/getAssetsForServiceare only logged and ignored. If the Cloud Asset API fails (auth, quota, network),finalResourcesmay remain empty while the method still returnsfinalResources, nil, which is the bug this PR aims to eliminate.You should mirror the individual provider’s pattern here by tracking errors and returning an error when no resources were merged:
func (p *OrganizationProvider) Resources(ctx context.Context) (*schema.Resources, error) { gologger.Info().Msgf("OrgProvider.Resources called with organization_id: '%s', projects: %v, services: %v", p.organizationID, p.projects, p.services.Keys()) parent := "organizations/" + p.organizationID gologger.Info().Msgf("Using organization-level discovery with parent: %s", parent) - finalResources := schema.NewResources() + finalResources := schema.NewResources() + var errs []error // Use Cloud Asset Inventory API to get assets if p.services.Has("all") { gologger.Info().Msgf("Found 'all' service, starting comprehensive asset discovery") allAssets, err := p.getAllAssets(ctx, parent) if err != nil { - gologger.Warning().Msgf("Could not get all assets: %s", err) + gologger.Warning().Msgf("Could not get all assets: %s", err) + errs = append(errs, fmt.Errorf("all: %w", err)) } else { finalResources.Merge(allAssets) } } else { // Get assets for specific services for _, service := range p.services.Keys() { assets, err := p.getAssetsForService(ctx, parent, service) if err != nil { gologger.Warning().Msgf("Could not get assets for service %s: %s", service, err) + errs = append(errs, fmt.Errorf("%s: %w", service, err)) } else { finalResources.Merge(assets) } } } - return finalResources, nil + if len(finalResources.Items) == 0 && len(errs) > 0 { + return nil, errs[0] + } + + return finalResources, nil }This keeps behavior consistent across GCP providers and guarantees that pure error cases are visible to callers instead of being misinterpreted as “no assets”. As per coding guidelines.
🧹 Nitpick comments (2)
pkg/providers/gcp/gcp.go (2)
92-197: Error aggregation in Provider.Resources prevents silent empty inventoriesCollecting per-service errors into
errsand returning a non-nil error whenfinalResources.Itemsis empty ensures the individual GCP provider no longer turns API failures into “no assets” responses while still allowing partial results when at least one service succeeds. Consider adding unit tests that assert this behavior when underlying service calls fail. Based on learnings.
524-540: Consider surfacing invalid read_time_offset_seconds valuesCurrently, non-numeric
read_time_offset_secondsvalues are silently ignored. Emitting a warning when parsing fails would make misconfigurations more obvious while still defaulting to “no offset”:// Check for extended metadata flag if extendedMetadata, ok := options.GetMetadata("extended_metadata"); ok { provider.extendedMetadata = extendedMetadata == "true" } - if offsetStr, ok := options.GetMetadata("read_time_offset_seconds"); ok { - if offset, err := strconv.Atoi(offsetStr); err == nil && offset > 0 { - provider.readTimeOffsetSeconds = offset - } - } + if offsetStr, ok := options.GetMetadata("read_time_offset_seconds"); ok { + offset, err := strconv.Atoi(offsetStr) + if err != nil { + gologger.Warning().Msgf("Invalid read_time_offset_seconds %q, ignoring: %s", offsetStr, err) + } else if offset > 0 { + provider.readTimeOffsetSeconds = offset + } + }This keeps behavior unchanged for valid/zero/negative values while giving feedback on bad input.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/providers/cloudflare/cloudflare.go(2 hunks)pkg/providers/cloudflare/cloudflare_test.go(1 hunks)pkg/providers/cloudflare/dns.go(1 hunks)pkg/providers/gcp/gcp.go(13 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
pkg/providers/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
pkg/providers/**/*.go: All providers must implement the schema.Provider interface with methods: Name(), ID(), Resources(ctx), Services()
For complex providers, keep service-specific logic in separate files (e.g., instances.go, dns.go, route53.go, s3.go, eks.go) and return *schema.Resources merged by the main Resources()
Parse provider configuration via block.GetMetadata(key); support environment variables using $VAR syntax; return &schema.ErrNoSuchKey{Name: key} for missing required config
Respect service filtering: Services() must list supported services, and providers should honor -s filters when gathering resources
Always pass and honor context.Context in Resources() and long-running API calls to support cancellation
Do not manually deduplicate resources; use Resources.Append() to rely on schema’s ResourceDeduplicator
Each emitted resource must have at least one of IP or DNS populated; empty resources are invalid
Treat provider config id as a user-defined identifier for filtering, not a cloud resource ID
Files:
pkg/providers/cloudflare/cloudflare_test.gopkg/providers/cloudflare/dns.gopkg/providers/cloudflare/cloudflare.gopkg/providers/gcp/gcp.go
pkg/providers/*/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Each provider directory must include a main file named .go defining New(block schema.OptionBlock) and Resources(ctx) orchestration
Files:
pkg/providers/cloudflare/cloudflare_test.gopkg/providers/cloudflare/dns.gopkg/providers/cloudflare/cloudflare.gopkg/providers/gcp/gcp.go
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer unit tests with mocked cloud provider clients; use table-driven tests for resource parsing; include tests for error handling on missing/invalid credentials
Files:
pkg/providers/cloudflare/cloudflare_test.go
pkg/providers/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Skip or isolate integration tests that require real cloud credentials; keep unit tests deterministic for CI
Files:
pkg/providers/cloudflare/cloudflare_test.go
pkg/providers/gcp/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
GCP provider: switch discovery mode based on presence of organization_id; use cloud.google.com/go/asset for Asset API mode, else service-specific clients
Files:
pkg/providers/gcp/gcp.go
🧠 Learnings (19)
📓 Common learnings
Learnt from: CR
Repo: projectdiscovery/cloudlist PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T18:34:55.853Z
Learning: Applies to **/*_test.go : Prefer unit tests with mocked cloud provider clients; use table-driven tests for resource parsing; include tests for error handling on missing/invalid credentials
📚 Learning: 2025-10-14T18:34:55.853Z
Learnt from: CR
Repo: projectdiscovery/cloudlist PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T18:34:55.853Z
Learning: Applies to **/*_test.go : Prefer unit tests with mocked cloud provider clients; use table-driven tests for resource parsing; include tests for error handling on missing/invalid credentials
Applied to files:
pkg/providers/cloudflare/cloudflare_test.go
📚 Learning: 2025-10-14T18:34:55.853Z
Learnt from: CR
Repo: projectdiscovery/cloudlist PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T18:34:55.853Z
Learning: Applies to pkg/providers/**/*_test.go : Skip or isolate integration tests that require real cloud credentials; keep unit tests deterministic for CI
Applied to files:
pkg/providers/cloudflare/cloudflare_test.go
📚 Learning: 2025-10-14T18:34:55.853Z
Learnt from: CR
Repo: projectdiscovery/cloudlist PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T18:34:55.853Z
Learning: Applies to pkg/providers/**/*.go : All providers must implement the schema.Provider interface with methods: Name(), ID(), Resources(ctx), Services()
Applied to files:
pkg/providers/cloudflare/cloudflare_test.gopkg/providers/cloudflare/dns.gopkg/providers/cloudflare/cloudflare.gopkg/providers/gcp/gcp.go
📚 Learning: 2025-10-14T18:34:55.853Z
Learnt from: CR
Repo: projectdiscovery/cloudlist PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T18:34:55.853Z
Learning: Applies to pkg/providers/**/*.go : Each emitted resource must have at least one of IP or DNS populated; empty resources are invalid
Applied to files:
pkg/providers/cloudflare/cloudflare_test.gopkg/providers/cloudflare/cloudflare.gopkg/providers/gcp/gcp.go
📚 Learning: 2025-10-14T18:34:55.853Z
Learnt from: CR
Repo: projectdiscovery/cloudlist PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T18:34:55.853Z
Learning: Applies to pkg/providers/**/*.go : For complex providers, keep service-specific logic in separate files (e.g., instances.go, dns.go, route53.go, s3.go, eks.go) and return *schema.Resources merged by the main Resources()
Applied to files:
pkg/providers/cloudflare/cloudflare_test.gopkg/providers/cloudflare/cloudflare.gopkg/providers/gcp/gcp.go
📚 Learning: 2025-10-14T18:34:55.853Z
Learnt from: CR
Repo: projectdiscovery/cloudlist PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T18:34:55.853Z
Learning: Applies to pkg/providers/*/*.go : Each provider directory must include a main file named <provider>.go defining New(block schema.OptionBlock) and Resources(ctx) orchestration
Applied to files:
pkg/providers/cloudflare/cloudflare_test.gopkg/providers/gcp/gcp.go
📚 Learning: 2025-10-14T18:34:55.853Z
Learnt from: CR
Repo: projectdiscovery/cloudlist PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T18:34:55.853Z
Learning: Applies to pkg/providers/**/*.go : Respect service filtering: Services() must list supported services, and providers should honor -s filters when gathering resources
Applied to files:
pkg/providers/cloudflare/cloudflare_test.gopkg/providers/gcp/gcp.go
📚 Learning: 2025-10-14T18:34:55.853Z
Learnt from: CR
Repo: projectdiscovery/cloudlist PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T18:34:55.853Z
Learning: Applies to pkg/providers/**/*.go : Always pass and honor context.Context in Resources() and long-running API calls to support cancellation
Applied to files:
pkg/providers/cloudflare/cloudflare_test.gopkg/providers/gcp/gcp.go
📚 Learning: 2025-10-14T18:34:55.853Z
Learnt from: CR
Repo: projectdiscovery/cloudlist PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T18:34:55.853Z
Learning: Applies to pkg/providers/**/*.go : Treat provider config id as a user-defined identifier for filtering, not a cloud resource ID
Applied to files:
pkg/providers/cloudflare/dns.gopkg/providers/cloudflare/cloudflare.gopkg/providers/gcp/gcp.go
📚 Learning: 2025-10-14T18:34:55.853Z
Learnt from: CR
Repo: projectdiscovery/cloudlist PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T18:34:55.853Z
Learning: Applies to pkg/providers/**/*.go : Do not manually deduplicate resources; use Resources.Append() to rely on schema’s ResourceDeduplicator
Applied to files:
pkg/providers/cloudflare/cloudflare.gopkg/providers/gcp/gcp.go
📚 Learning: 2025-10-14T18:34:55.853Z
Learnt from: CR
Repo: projectdiscovery/cloudlist PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T18:34:55.853Z
Learning: Applies to pkg/providers/gcp/**/*.go : GCP provider: switch discovery mode based on presence of organization_id; use cloud.google.com/go/asset for Asset API mode, else service-specific clients
Applied to files:
pkg/providers/gcp/gcp.go
📚 Learning: 2025-10-14T18:34:55.853Z
Learnt from: CR
Repo: projectdiscovery/cloudlist PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T18:34:55.853Z
Learning: Applies to docs/GCP_ASSET_API.md : Follow the detailed implementation guide for GCP’s Organization-Level Asset API in docs/GCP_ASSET_API.md
Applied to files:
pkg/providers/gcp/gcp.go
📚 Learning: 2025-10-14T18:34:55.853Z
Learnt from: CR
Repo: projectdiscovery/cloudlist PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T18:34:55.853Z
Learning: Cloudlist is a Go-based multi-cloud asset discovery tool; follow Go conventions and repository structure noted here
Applied to files:
pkg/providers/gcp/gcp.go
📚 Learning: 2025-07-09T17:50:43.982Z
Learnt from: dogancanbakir
Repo: projectdiscovery/cloudlist PR: 687
File: pkg/providers/gcp/bucket.go:30-30
Timestamp: 2025-07-09T17:50:43.982Z
Learning: The Google Cloud Go SDK assetpb package does not have a ContentType_RESOURCE_AND_IAM_POLICY constant. The available constants are ContentType_RESOURCE and ContentType_IAM_POLICY as separate values. Using ContentType_RESOURCE appears to include IAM policy data in the response, allowing access to asset.IamPolicy.
Applied to files:
pkg/providers/gcp/gcp.go
📚 Learning: 2025-10-14T18:34:55.853Z
Learnt from: CR
Repo: projectdiscovery/cloudlist PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T18:34:55.853Z
Learning: Applies to pkg/inventory/inventory.go : Register every new provider in nameToProvider() using a case mapping from provider name to implementation
Applied to files:
pkg/providers/gcp/gcp.go
📚 Learning: 2025-10-14T18:34:55.853Z
Learnt from: CR
Repo: projectdiscovery/cloudlist PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T18:34:55.853Z
Learning: Applies to pkg/inventory/inventory.go : When adding a provider, also update the Providers map with the provider and its supported services
Applied to files:
pkg/providers/gcp/gcp.go
📚 Learning: 2025-10-14T18:34:55.853Z
Learnt from: CR
Repo: projectdiscovery/cloudlist PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T18:34:55.853Z
Learning: Applies to pkg/providers/**/*.go : Parse provider configuration via block.GetMetadata(key); support environment variables using $VAR syntax; return &schema.ErrNoSuchKey{Name: key} for missing required config
Applied to files:
pkg/providers/gcp/gcp.go
📚 Learning: 2025-10-14T18:34:55.853Z
Learnt from: CR
Repo: projectdiscovery/cloudlist PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T18:34:55.853Z
Learning: Applies to pkg/inventory/inventory.go : Keep service names consistent: entries in each provider’s Services() must match the inventory Providers map
Applied to files:
pkg/providers/gcp/gcp.go
🧬 Code graph analysis (2)
pkg/providers/cloudflare/cloudflare_test.go (2)
pkg/providers/cloudflare/cloudflare.go (2)
Provider(14-19)New(23-77)pkg/schema/schema.go (1)
ServiceMap(254-254)
pkg/providers/gcp/gcp.go (2)
pkg/schema/schema.go (1)
ServiceMap(254-254)pkg/providers/cloudflare/cloudflare.go (1)
New(23-77)
🔇 Additional comments (12)
pkg/providers/cloudflare/cloudflare.go (2)
16-16: LGTM: Interface-based design improves testability.The shift to the
apiClientinterface enables dependency injection and simplifies unit testing with mocks, directly supporting the new error propagation tests.
106-110: LGTM: Error propagation correctly implemented.Resources are now merged only after successful retrieval, and API errors propagate to callers as intended. This prevents transient failures from being misinterpreted as empty inventories.
pkg/providers/cloudflare/cloudflare_test.go (4)
13-23: LGTM: Mock correctly simulates zone listing failure.The mock implementation is clean and focused on testing the zone error path.
25-36: LGTM: Mock correctly simulates DNS record listing failure.The mock properly simulates the scenario where zone listing succeeds but record fetching fails, covering the second error path.
38-52: LGTM: Test correctly verifies zone error propagation.The test validates that zone listing errors propagate through
Resources()as intended, aligning with PR objectives.
54-69: LGTM: Test correctly verifies DNS record error propagation.The test validates that DNS record listing errors propagate through
Resources()as intended, covering the second failure path.pkg/providers/cloudflare/dns.go (2)
14-17: LGTM: Interface definition enables testability.The
apiClientinterface properly abstracts the Cloudflare SDK dependencies, enabling the mock-based tests incloudflare_test.go.
21-21: LGTM: Field type change aligns with interface-based design.Consistent with the Provider struct change and enables mock-based testing.
pkg/providers/gcp/gcp.go (4)
3-28: New imports for read-time configuration look correctThe added imports (
strconv,time,timestamppb) are all used by the newread_time_offset_secondsparsing andListAssetsRequest.ReadTimelogic; nothing extraneous or missing here.
45-61: OrganizationProvider struct extensions align with new functionalityAdding
readTimeOffsetSecondsand wiring in per-service clients for extended metadata is consistent with their usage ingetAssetsForTypesandnewOrganizationProvider; the struct shape looks coherent and forward-compatible.
405-436: Read-time offset logging in getAllAssets is straightforward and helpfulConditionally computing and logging the effective
read_timewhenreadTimeOffsetSeconds > 0gives operators useful visibility into the snapshot time used for Asset API discovery without affecting behavior.
438-494: Applying read_time_offset_seconds to ListAssetsRequest is correctSetting
req.ReadTimewithtimestamppb.New(readTime)whenreadTimeOffsetSeconds > 0correctly pushes the configured offset into the Cloud Asset Inventory API, and the defensive clamp tonowkeeps the timestamp from drifting into the future if the logic changes later.
…ry#713) * refactor(cloudflare): replace cloudflare.API with apiClient interface - Updated the cloudflare provider to use a more flexible apiClient interface instead of the specific cloudflare.API type. - Improved error handling in the Resources method to return errors directly from the DNS provider. - Ensured that resources are merged only after successful retrieval, enhancing the reliability of resource management. * fix: added readtime param
2 participants
closes: #712
Summary by CodeRabbit
New Features
Bug Fixes