feat: added verify method to gcp provider #690
Merged
Conversation
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
Contributor
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/providers/gcp/gcp.go(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
pkg/providers/gcp/gcp.go (2)
Learnt from: dogancanbakir
PR: projectdiscovery/cloudlist#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.
Learnt from: dogancanbakir
PR: projectdiscovery/cloudlist#687
File: pkg/providers/gcp/bucket.go:30-30
Timestamp: 2025-07-09T17:50:43.982Z
Learning: In Google Cloud Go SDK's assetpb package, ContentType_RESOURCE includes IAM policy data when available. There is no ContentType_RESOURCE_AND_IAM_POLICY constant in the assetpb package. The correct approach is to use ContentType_RESOURCE and access asset.IamPolicy directly, as shown in the GCP provider implementations.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: release-test
- GitHub Check: Test Builds (1.22.x, macOS-latest)
- GitHub Check: Lint Test
- GitHub Check: Test Builds (1.22.x, windows-latest)
- GitHub Check: Test Builds (1.22.x, ubuntu-latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
pkg/providers/gcp/gcp.go (1)
324-324: LGTM! Improved variable naming.The rename from
assetTypestoassetTypesGcpListAPImakes the variable's purpose clearer.
Comment on lines
+710
to
+737
| func (p *OrganizationProvider) Verify(ctx context.Context) error { | ||
| var assetTypesGcpListAPI = []string{ | ||
| "compute.googleapis.com/Instance", | ||
| "compute.googleapis.com/Address", | ||
| "compute.googleapis.com/ForwardingRule", | ||
| "dns.googleapis.com/ManagedZone", | ||
| "dns.googleapis.com/ResourceRecordSet", | ||
| "storage.googleapis.com/Bucket", | ||
| "run.googleapis.com/Service", | ||
| "cloudfunctions.googleapis.com/CloudFunction", | ||
| "container.googleapis.com/Cluster", | ||
| "tpu.googleapis.com/Node", | ||
| "file.googleapis.com/Instance", | ||
| } | ||
|
|
||
| // For extra verification, try a minimal API call on one service | ||
| iter := p.assetClient.SearchAllResources(ctx, &assetpb.SearchAllResourcesRequest{ | ||
| Scope: "organizations/" + p.organizationID, | ||
| AssetTypes: assetTypesGcpListAPI, | ||
| PageSize: 1, | ||
| }) | ||
|
|
||
| _, err := iter.Next() | ||
| if err != nil && !errors.Is(err, iterator.Done) { | ||
| return errorutil.NewWithErr(err).Msgf("failed to verify GCP Asset API access for organization %s", p.organizationID) | ||
| } | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract duplicated asset types list to avoid maintenance issues.
The assetTypesGcpListAPI list is duplicated from the getAllAssets method (line 324). This violates the DRY principle and could lead to inconsistencies.
Extract the asset types list as a package-level variable:
// At the package level, after line 57
var assetTypesGcpListAPI = []string{
"compute.googleapis.com/Instance",
"compute.googleapis.com/GlobalAddress",
"compute.googleapis.com/Address",
"compute.googleapis.com/ForwardingRule",
"dns.googleapis.com/ManagedZone",
"dns.googleapis.com/ResourceRecordSet",
"storage.googleapis.com/Bucket",
"run.googleapis.com/Service",
"cloudfunctions.googleapis.com/CloudFunction",
"container.googleapis.com/Cluster",
"tpu.googleapis.com/Node",
"file.googleapis.com/Instance",
}Then update both methods to use this shared variable:
- Remove the local declaration at line 324 in
getAllAssets - Remove the local declaration at line 711 in
Verify - Update line 340 to use the package-level variable
- Update line 728 to use the package-level variable
Note: The asset type compute.googleapis.com/GlobalAddress is missing from the Verify method's list but present in getAllAssets. Ensure both lists are consistent.
🤖 Prompt for AI Agents
In pkg/providers/gcp/gcp.go around lines 710 to 737, the assetTypesGcpListAPI
slice is duplicated from the getAllAssets method near line 324, causing
maintenance issues and inconsistencies. To fix this, extract the
assetTypesGcpListAPI slice as a package-level variable after line 57, including
the missing "compute.googleapis.com/GlobalAddress" entry. Then remove the local
declarations of this slice in both the Verify method (line 711) and getAllAssets
method (line 324), and update their usages at lines 728 and 340 respectively to
reference the new package-level variable.
Comment on lines
+664
to
+708
| // Verify checks if the GCP provider credentials are valid | ||
| func (p *Provider) Verify(ctx context.Context) error { | ||
| if len(p.projects) == 0 { | ||
| return errorutil.New("no accessible GCP projects found with provided credentials") | ||
| } | ||
|
|
||
| // For extra verification, try a minimal API call on one service | ||
| var err error | ||
| for _, project := range p.projects { | ||
| var success bool | ||
| if p.compute != nil { | ||
| if _, err = p.compute.Regions.List(project).Do(); err == nil { | ||
| success = true | ||
| } | ||
| } else if p.dns != nil { | ||
| if _, err = p.dns.ManagedZones.List(project).Do(); err == nil { | ||
| success = true | ||
| } | ||
| } else if p.storage != nil { | ||
| if _, err = p.storage.Buckets.List(project).Do(); err == nil { | ||
| success = true | ||
| } | ||
| } else if p.functions != nil { | ||
| if _, err = p.functions.Projects.Locations.List(project).Do(); err == nil { | ||
| success = true | ||
| } | ||
| } else if p.run != nil { | ||
| if _, err = p.run.Projects.Locations.List(project).Do(); err == nil { | ||
| success = true | ||
| } | ||
| } else if p.gke != nil { | ||
| if _, err = p.gke.Projects.Locations.Clusters.List(project).Do(); err == nil { | ||
| success = true | ||
| } | ||
| } | ||
| // For any one service to be successful, we can return nil | ||
| if success { | ||
| return nil | ||
| } | ||
| } | ||
| if err != nil { | ||
| return errorutil.NewWithErr(err).Msgf("failed to verify GCP services") | ||
| } | ||
| return errorutil.New("no accessible GCP services found with provided credentials") | ||
| } |
Contributor
There was a problem hiding this comment.
Fix error handling to preserve meaningful error messages.
The current implementation only preserves the last error, which might not be the most informative. Consider collecting errors from all attempts to provide better diagnostics.
Apply this diff to improve error handling:
func (p *Provider) Verify(ctx context.Context) error {
if len(p.projects) == 0 {
return errorutil.New("no accessible GCP projects found with provided credentials")
}
// For extra verification, try a minimal API call on one service
- var err error
+ var lastErr error
for _, project := range p.projects {
- var success bool
if p.compute != nil {
- if _, err = p.compute.Regions.List(project).Do(); err == nil {
- success = true
+ if _, err := p.compute.Regions.List(project).Do(); err == nil {
+ return nil
+ } else {
+ lastErr = err
}
} else if p.dns != nil {
- if _, err = p.dns.ManagedZones.List(project).Do(); err == nil {
- success = true
+ if _, err := p.dns.ManagedZones.List(project).Do(); err == nil {
+ return nil
+ } else {
+ lastErr = err
}
} else if p.storage != nil {
- if _, err = p.storage.Buckets.List(project).Do(); err == nil {
- success = true
+ if _, err := p.storage.Buckets.List(project).Do(); err == nil {
+ return nil
+ } else {
+ lastErr = err
}
} else if p.functions != nil {
- if _, err = p.functions.Projects.Locations.List(project).Do(); err == nil {
- success = true
+ if _, err := p.functions.Projects.Locations.List(project).Do(); err == nil {
+ return nil
+ } else {
+ lastErr = err
}
} else if p.run != nil {
- if _, err = p.run.Projects.Locations.List(project).Do(); err == nil {
- success = true
+ if _, err := p.run.Projects.Locations.List(project).Do(); err == nil {
+ return nil
+ } else {
+ lastErr = err
}
} else if p.gke != nil {
- if _, err = p.gke.Projects.Locations.Clusters.List(project).Do(); err == nil {
- success = true
+ if _, err := p.gke.Projects.Locations.Clusters.List(project).Do(); err == nil {
+ return nil
+ } else {
+ lastErr = err
}
}
- // For any one service to be successful, we can return nil
- if success {
- return nil
- }
}
- if err != nil {
- return errorutil.NewWithErr(err).Msgf("failed to verify GCP services")
+ if lastErr != nil {
+ return errorutil.NewWithErr(lastErr).Msgf("failed to verify GCP services")
}
return errorutil.New("no accessible GCP services found with provided credentials")
}📝 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
| // Verify checks if the GCP provider credentials are valid | |
| func (p *Provider) Verify(ctx context.Context) error { | |
| if len(p.projects) == 0 { | |
| return errorutil.New("no accessible GCP projects found with provided credentials") | |
| } | |
| // For extra verification, try a minimal API call on one service | |
| var err error | |
| for _, project := range p.projects { | |
| var success bool | |
| if p.compute != nil { | |
| if _, err = p.compute.Regions.List(project).Do(); err == nil { | |
| success = true | |
| } | |
| } else if p.dns != nil { | |
| if _, err = p.dns.ManagedZones.List(project).Do(); err == nil { | |
| success = true | |
| } | |
| } else if p.storage != nil { | |
| if _, err = p.storage.Buckets.List(project).Do(); err == nil { | |
| success = true | |
| } | |
| } else if p.functions != nil { | |
| if _, err = p.functions.Projects.Locations.List(project).Do(); err == nil { | |
| success = true | |
| } | |
| } else if p.run != nil { | |
| if _, err = p.run.Projects.Locations.List(project).Do(); err == nil { | |
| success = true | |
| } | |
| } else if p.gke != nil { | |
| if _, err = p.gke.Projects.Locations.Clusters.List(project).Do(); err == nil { | |
| success = true | |
| } | |
| } | |
| // For any one service to be successful, we can return nil | |
| if success { | |
| return nil | |
| } | |
| } | |
| if err != nil { | |
| return errorutil.NewWithErr(err).Msgf("failed to verify GCP services") | |
| } | |
| return errorutil.New("no accessible GCP services found with provided credentials") | |
| } | |
| func (p *Provider) Verify(ctx context.Context) error { | |
| if len(p.projects) == 0 { | |
| return errorutil.New("no accessible GCP projects found with provided credentials") | |
| } | |
| // For extra verification, try a minimal API call on one service | |
| var lastErr error | |
| for _, project := range p.projects { | |
| if p.compute != nil { | |
| if _, err := p.compute.Regions.List(project).Do(); err == nil { | |
| return nil | |
| } else { | |
| lastErr = err | |
| } | |
| } else if p.dns != nil { | |
| if _, err := p.dns.ManagedZones.List(project).Do(); err == nil { | |
| return nil | |
| } else { | |
| lastErr = err | |
| } | |
| } else if p.storage != nil { | |
| if _, err := p.storage.Buckets.List(project).Do(); err == nil { | |
| return nil | |
| } else { | |
| lastErr = err | |
| } | |
| } else if p.functions != nil { | |
| if _, err := p.functions.Projects.Locations.List(project).Do(); err == nil { | |
| return nil | |
| } else { | |
| lastErr = err | |
| } | |
| } else if p.run != nil { | |
| if _, err := p.run.Projects.Locations.List(project).Do(); err == nil { | |
| return nil | |
| } else { | |
| lastErr = err | |
| } | |
| } else if p.gke != nil { | |
| if _, err := p.gke.Projects.Locations.Clusters.List(project).Do(); err == nil { | |
| return nil | |
| } else { | |
| lastErr = err | |
| } | |
| } | |
| } | |
| if lastErr != nil { | |
| return errorutil.NewWithErr(lastErr).Msgf("failed to verify GCP services") | |
| } | |
| return errorutil.New("no accessible GCP services found with provided credentials") | |
| } |
🤖 Prompt for AI Agents
In pkg/providers/gcp/gcp.go lines 664 to 708, the Verify function currently only
retains the last error encountered during service checks, which may lose
important diagnostic information. Modify the code to collect all errors from
each service check attempt, aggregate them, and return a combined error message
if no service verification succeeds. This will preserve meaningful error details
from all failed attempts for better troubleshooting.
ehsandeep
approved these changes
Jul 11, 2025
visnetodev
pushed a commit
to visnetotest/cloudlist
that referenced
this pull request
Dec 7, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Refactor