-
Notifications
You must be signed in to change notification settings - Fork 162
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.
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
Add support for GCP scopes #1296
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @bgdanix ππ»
I left some comments about the implementation, I generally get the approach, I think code now lack of readability and testing.
Some issues I had while testing your code :
- The identifier for the organization is wrongly asserted and lead to a rpc error.
- I get an unexpected result when one of the GCP scopes is having permission issues, it ignores listing of some resources for all scopes. This is expected from a technical POV, but not from a user POV. We may have UX concerns about this behavior.
Could you address the comments until we review this again ? It'd be nice to get a review from someone else on this one.
Thank you @sundowndev for taking time to review this. I have addressed all your comments. The above reported issues should also be fixed now. |
@@ -58,10 +59,10 @@ func (s *cloudResourceManagerRepository) ListProjectsBindings() (map[string]map[ | |||
|
|||
if len(errorsByProject) > 0 { | |||
for project, errval := range errorsByProject { | |||
erorsString = erorsString + "Project: " + project + " had the following error: " + errval.Error() + "; " | |||
erorString = erorString + fmt.Sprintf("Project: %s had the following error: %s; ", project, errval.Error()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure to get the end of the template %s;
, the semicolon is unnecessary here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I've put the semicolon is to separate between multiple errors. E.g.
For scope projects/123456 on resource TheResource got error: rpc error: code = 123 desc = description; For scope projects/123456 on resource AnotherResource got error: abc
Codecov Report
@@ Coverage Diff @@
## main #1296 +/- ##
==========================================
+ Coverage 73.13% 81.78% +8.64%
==========================================
Files 469 398 -71
Lines 17412 14489 -2923
==========================================
- Hits 12735 11850 -885
+ Misses 4367 2338 -2029
+ Partials 310 301 -9
|
@sundowndev I've changed the code a bit to cater for cases when multiple scopes are used and one (or all) of them has permission error. Might not be the best way, but couldn't think of a better implementation for this case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR cannot work as it, terraform provider should be reworked.
I will do another review pass later
defer s.cache.Unlock(cacheKey) | ||
|
||
req := &assetpb.ListAssetsRequest{ | ||
Parent: scope, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that a folder is supported for listAssets
call according to the documentation here : https://cloud.google.com/asset-inventory/docs/reference/rpc/google.cloud.asset.v1#google.cloud.asset.v1.ListAssetsRequest
Can you double check that ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. Ideally we should not use listAssets but only searchAssets as the main difference is that list takes a snapshot of how the assets were at a particular time, which could result in a diff from what it's actually present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where did you get this info ? I asked this question here a couple of month ago and I didn't get any answer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From here there are a couple of hints about it:
Listing assets might not meet the performance requirements for large scale customers. If this is the case for you and calling list API encounters timeout, it's recommended to use export API instead.
Also in the API it's mentioned the purpose is to list resources from a specific timeframe
Lists assets with time and resource types and returns paged results in response.
readTime: Timestamp to take an asset snapshot. If not specified, the current time will be used. Due to delays in resource data collection and indexing, there is a volatile window during which running the same query may get different results.
While the search is more appropriate for our intent:
The Cloud Asset API allows you to use a custom query language to query resource metadata on a project, folder, or organization.
Method: searchAllResources. Searches all Cloud resources within the specified scope, such as a project, folder, or organization.
Region: os.Getenv("CLOUDSDK_COMPUTE_REGION"), | ||
Zone: os.Getenv("CLOUDSDK_COMPUTE_ZONE"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you removed Region and Zone parameters ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because they are not needed since we use scopes.
Project: os.Getenv("CLOUDSDK_CORE_PROJECT"), | ||
Region: os.Getenv("CLOUDSDK_COMPUTE_REGION"), | ||
Zone: os.Getenv("CLOUDSDK_COMPUTE_ZONE"), | ||
Scopes: scopes, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you missed the point, scopes are not what we are expecting here in this struct https://registry.terraform.io/providers/hashicorp/google/latest/docs/guides/provider_reference#scopes
I think this PR is more complicated than you think, it seems that it is not possible to initialize a google
terraform provider with a folder or organization scope (not to be confused with OAuth scopes). You can only configure a project.
We probably need to create another kind of detail fetcher that will lazy instantiate one terraform providers per project. We are doing that for S3 buckets, but it's a really tricky thing.
Did you try to run this PR in deep mode ? I think this is broken
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The entire point I had in mind was to support multiple or boarder scopes for scanning. I did not run it in deep mode since it's not recommended, but for a simple mode targeting various projects or an entire GCP org it works really good for me:
"summary": {
"total_resources": 400,
"total_changed": 0,
"total_unmanaged": 398,
"total_missing": 1,
"total_managed": 1
}
@@ -55,10 +53,8 @@ func (p *GCPTerraformProvider) Version() string { | |||
return p.version | |||
} | |||
|
|||
func (p *GCPTerraformProvider) GetConfig() config.GCPTerraformConfig { | |||
func (p *GCPTerraformProvider) SetConfig(scopes []string) config.GCPTerraformConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetConfig
was ok, we are not setting anything in this function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well there's also nothing to get as during automated tests we have to set the config (scope)
} else if to != common.RemoteGoogleTerraform && len(GCPScope) > 0 { | ||
return errors.New("gcp-scope can only be utilized when using " + common.RemoteGoogleTerraform + " flag") | ||
} else if to == common.RemoteGoogleTerraform && len(GCPScope) == 0 { | ||
return errors.New("gcp-scope must be specified when using " + common.RemoteGoogleTerraform + " flag") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plea try to avoid else-if statements, there is not reason do use them here
Description
Add support for specifying the search scopes when using GCP provider