Skip to content
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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Add support for GCP scopes #1296

wants to merge 14 commits into from

Conversation

bgdanix
Copy link

@bgdanix bgdanix commented Dec 20, 2021

Q A
πŸ› Bug fix? no
πŸš€ New feature? yes
⚠ Deprecations? no
❌ BC Break yes
πŸ”— Related issues #1284
❓ Documentation yes

Description

Add support for specifying the search scopes when using GCP provider

@bgdanix bgdanix marked this pull request as ready for review December 21, 2021 12:15
@bgdanix bgdanix requested a review from a team as a code owner December 21, 2021 12:15
@bgdanix bgdanix requested review from eliecharra and removed request for a team December 21, 2021 12:15
Copy link
Contributor

@sundowndev sundowndev left a 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.

pkg/cmd/scan.go Outdated Show resolved Hide resolved
pkg/cmd/scan.go Outdated Show resolved Hide resolved
pkg/remote/google/repository/cloudresourcemanager.go Outdated Show resolved Hide resolved
pkg/remote/google/config/config.go Outdated Show resolved Hide resolved
pkg/remote/google/repository/cloudresourcemanager.go Outdated Show resolved Hide resolved
pkg/remote/google/repository/asset.go Outdated Show resolved Hide resolved
@bgdanix
Copy link
Author

bgdanix commented Dec 22, 2021

* 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.

Thank you @sundowndev for taking time to review this. I have addressed all your comments. The above reported issues should also be fixed now.

pkg/cmd/scan.go Outdated Show resolved Hide resolved
pkg/remote/google/repository/asset.go Outdated Show resolved Hide resolved
pkg/remote/google/repository/asset_test.go Outdated Show resolved Hide resolved
@@ -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())
Copy link
Contributor

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

Copy link
Author

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-commenter
Copy link

codecov-commenter commented Dec 22, 2021

Codecov Report

Merging #1296 (cc82707) into main (7203ca8) will increase coverage by 8.64%.
The diff coverage is 64.94%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Ξ”
pkg/driftctl.go 73.38% <ΓΈ> (ΓΈ)
pkg/remote/google/init.go 0.00% <0.00%> (-77.78%) ⬇️
...g/remote/google/repository/cloudresourcemanager.go 0.00% <0.00%> (-76.93%) ⬇️
pkg/remote/remote.go 40.00% <0.00%> (-50.00%) ⬇️
pkg/cmd/scan.go 72.18% <51.35%> (-20.15%) ⬇️
pkg/remote/google/provider.go 65.71% <66.66%> (-9.97%) ⬇️
pkg/remote/google/repository/asset.go 90.90% <85.98%> (-6.82%) ⬇️
pkg/terraform/plugin_client.go 0.00% <0.00%> (-100.00%) ⬇️
pkg/remote/aws/client/s3_client_factory.go 0.00% <0.00%> (-100.00%) ⬇️
pkg/remote/aws/init.go 0.00% <0.00%> (-95.57%) ⬇️
... and 133 more

@bgdanix
Copy link
Author

bgdanix commented Dec 23, 2021

@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.

Copy link
Contributor

@eliecharra eliecharra left a 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,
Copy link
Contributor

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 ?

Copy link
Author

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.

Copy link
Contributor

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.

Copy link
Author

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. 

Comment on lines -61 to -62
Region: os.Getenv("CLOUDSDK_COMPUTE_REGION"),
Zone: os.Getenv("CLOUDSDK_COMPUTE_ZONE"),
Copy link
Contributor

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 ?

Copy link
Author

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,
Copy link
Contributor

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

Copy link
Author

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 {
Copy link
Contributor

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

Copy link
Author

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)

Comment on lines +73 to +77
} 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")
}
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants