Add support for GCP scopes by bgdanix · Pull Request #1296 · snyk/driftctl

bgdanix · 2021-12-20T09:52:32Z

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

…gle_folder

…-scopes

sundowndev

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

pkg/remote/google/repository/cloudresourcemanager.go

pkg/remote/google/config/config.go

pkg/remote/google/repository/cloudresourcemanager.go

pkg/remote/google/repository/asset.go

…-scopes

bgdanix · 2021-12-22T12:02:13Z

* 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

pkg/remote/google/repository/asset.go

pkg/remote/google/repository/asset_test.go

sundowndev · 2021-12-22T12:21:45Z

pkg/remote/google/repository/cloudresourcemanager.go

 	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())


Not sure to get the end of the template %s; , the semicolon is unnecessary here

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 · 2021-12-22T12:59:07Z

Codecov Report

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

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

…-scopes

bgdanix · 2021-12-23T10:35:22Z

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

eliecharra

This PR cannot work as it, terraform provider should be reworked.
I will do another review pass later

eliecharra · 2021-12-23T10:22:48Z

pkg/remote/google/repository/asset.go

+		defer s.cache.Unlock(cacheKey)
+
+		req := &assetpb.ListAssetsRequest{
+			Parent:      scope,


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 ?

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.

Where did you get this info ? I asked this question here a couple of month ago and I didn't get any answer.

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.

eliecharra · 2021-12-23T10:24:32Z

pkg/remote/google/provider.go

-		Region:  os.Getenv("CLOUDSDK_COMPUTE_REGION"),
-		Zone:    os.Getenv("CLOUDSDK_COMPUTE_ZONE"),


Why do you removed Region and Zone parameters ?

Because they are not needed since we use scopes.

eliecharra · 2021-12-23T10:35:13Z

pkg/remote/google/provider.go

-		Project: os.Getenv("CLOUDSDK_CORE_PROJECT"),
-		Region:  os.Getenv("CLOUDSDK_COMPUTE_REGION"),
-		Zone:    os.Getenv("CLOUDSDK_COMPUTE_ZONE"),
+		Scopes: scopes,


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

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 }

eliecharra · 2021-12-23T10:35:56Z

pkg/remote/google/provider.go

 }

-func (p *GCPTerraformProvider) GetConfig() config.GCPTerraformConfig {
+func (p *GCPTerraformProvider) SetConfig(scopes []string) config.GCPTerraformConfig {


GetConfig was ok, we are not setting anything in this function

Well there's also nothing to get as during automated tests we have to set the config (scope)

eliecharra · 2021-12-23T10:42:05Z

pkg/cmd/scan.go

+			} 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")
+			}


Plea try to avoid else-if statements, there is not reason do use them here

bgdanix added 8 commits December 13, 2021 13:30

feat: add support for google_folder

9396bcb

fix: make CLOUDSDK_ORGANIZATION optional

15cb0d2

Merge branch 'main' of https://github.com/snyk/driftctl into feat/goo…

00a7829

…gle_folder

feat: add support for GCP scopes

8d57e8e

Merge branch 'main' of https://github.com/snyk/driftctl into feat/gcp…

ea6fb51

…-scopes

Merge branch 'main' of https://github.com/snyk/driftctl into feat/gcp…

45269ac

…-scopes

test: fix tests to cope with new cmd param gcp-scope

78cc7a9

fix: go formatted code

0a8f5f8

bgdanix marked this pull request as ready for review December 21, 2021 12:15

bgdanix requested a review from a team as a code owner December 21, 2021 12:15

bgdanix requested review from eliecharra and removed request for a team December 21, 2021 12:15

sundowndev suggested changes Dec 21, 2021

View reviewed changes

bgdanix added 3 commits December 22, 2021 11:54

fix: PR comments and errors for asset scanning

aca1cf5

Merge branch 'main' of https://github.com/snyk/driftctl into feat/gcp…

8264ab4

…-scopes

fix: Go formatter

dced4e9

bgdanix requested a review from sundowndev December 22, 2021 12:05

sundowndev suggested changes Dec 22, 2021

View reviewed changes

fix: improved tests and scan cmd option

1214a02

bgdanix requested a review from sundowndev December 22, 2021 12:57

bgdanix added 2 commits December 23, 2021 10:26

fix: support multiple errors by scope

2ccb9a9

Merge branch 'main' of https://github.com/snyk/driftctl into feat/gcp…

cc82707

…-scopes

eliecharra suggested changes Dec 23, 2021

View reviewed changes

		Region: os.Getenv("CLOUDSDK_COMPUTE_REGION"),
		Zone: os.Getenv("CLOUDSDK_COMPUTE_ZONE"),

Conversation

bgdanix commented Dec 20, 2021

Description

Uh oh!

sundowndev left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bgdanix commented Dec 22, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Dec 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bgdanix commented Dec 23, 2021

Uh oh!

eliecharra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sundowndev left a comment •

edited

Loading

codecov-commenter commented Dec 22, 2021 •

edited

Loading