Skip to content

feat: added verify method to gcp provider #690

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ehsandeep merged 1 commit into from
Jul 11, 2025
Merged

feat: added verify method to gcp provider #690

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 78 additions & 3 deletions pkg/providers/gcp/gcp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,8 +321,7 @@ func (p *OrganizationProvider) Resources(ctx context.Context) (*schema.Resources
func (p *OrganizationProvider) getAllAssets(ctx context.Context, parent string) (*schema.Resources, error) {
gologger.Info().Msgf("Starting Asset API discovery for parent: %s", parent)

// Define asset types that provide IP addresses or DNS names
assetTypes := []string{
var assetTypesGcpListAPI = []string{
"compute.googleapis.com/Instance",
"compute.googleapis.com/GlobalAddress",
"compute.googleapis.com/Address",
Expand All @@ -337,7 +336,8 @@ func (p *OrganizationProvider) getAllAssets(ctx context.Context, parent string)
"file.googleapis.com/Instance",
}

batchResources, err := p.getAssetsForTypes(ctx, parent, assetTypes)
// Define asset types that provide IP addresses or DNS names
batchResources, err := p.getAssetsForTypes(ctx, parent, assetTypesGcpListAPI)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -660,3 +660,78 @@ func (p *OrganizationProvider) extractFilestoreIP(data *structpb.Struct, resourc
resource.PublicIPv4 = ipList.Values[0].GetStringValue()
}
}

// 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")
}
Comment on lines +664 to +708
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.


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
}
Comment on lines +710 to +737
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Loading