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.

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

Jump to bottom

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
9396bcb
feat: add support for google_folder
bgdanix Dec 13, 2021
15cb0d2
fix: make CLOUDSDK_ORGANIZATION optional
bgdanix Dec 14, 2021
00a7829
Merge branch 'main' of https://github.com/snyk/driftctl into feat/goo…
bgdanix Dec 15, 2021
8d57e8e
feat: add support for GCP scopes
bgdanix Dec 20, 2021
ea6fb51
Merge branch 'main' of https://github.com/snyk/driftctl into feat/gcp…
bgdanix Dec 20, 2021
45269ac
Merge branch 'main' of https://github.com/snyk/driftctl into feat/gcp…
bgdanix Dec 21, 2021
78cc7a9
test: fix tests to cope with new cmd param gcp-scope
bgdanix Dec 21, 2021
0a8f5f8
fix: go formatted code
bgdanix Dec 21, 2021
aca1cf5
fix: PR comments and errors for asset scanning
bgdanix Dec 22, 2021
8264ab4
Merge branch 'main' of https://github.com/snyk/driftctl into feat/gcp…
bgdanix Dec 22, 2021
dced4e9
fix: Go formatter
bgdanix Dec 22, 2021
1214a02
fix: improved tests and scan cmd option
bgdanix Dec 22, 2021
2ccb9a9
fix: support multiple errors by scope
bgdanix Dec 23, 2021
cc82707
Merge branch 'main' of https://github.com/snyk/driftctl into feat/gcp…
bgdanix Dec 23, 2021
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
4 changes: 2 additions & 2 deletions pkg/cmd/scan.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ func NewScanCmd(opts *pkg.ScanOptions) *cobra.Command {
)
fl.StringSliceP(
"gcp-scope",
"s",
"",
bgdanix marked this conversation as resolved.
Show resolved Hide resolved
[]string{},
"Set the GCP scope for search",
)
Expand Down Expand Up @@ -347,7 +347,7 @@ func scanRun(opts *pkg.ScanOptions) error {

func parseScopeFlag(scope []string) ([]string, error) {

scopeRegex := `projects/\S*$|folders/\d*$|organizations/\S*$`
scopeRegex := `projects/\S*$|folders/\d*$|organizations/\d*$`
r := regexp.MustCompile(scopeRegex)

for _, v := range scope {
Expand Down
2 changes: 1 addition & 1 deletion pkg/remote/google/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
package config

type GCPTerraformConfig struct {
Scope []string `cty:"scope"`
Scopes []string `cty:"scopes"`
}
4 changes: 2 additions & 2 deletions pkg/remote/google/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,8 @@ func (p *GCPTerraformProvider) Version() string {
return p.version
}

func (p *GCPTerraformProvider) SetConfig(scope []string) 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)

return config.GCPTerraformConfig{
Scope: scope,
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
	}

}
}
33 changes: 27 additions & 6 deletions pkg/remote/google/repository/asset.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package repository

import (
"context"
"errors"
"fmt"

asset "cloud.google.com/go/asset/apiv1"
"github.com/snyk/driftctl/pkg/remote/cache"
Expand Down Expand Up @@ -76,9 +78,10 @@ func NewAssetRepository(client *asset.Client, config config.GCPTerraformConfig,
func (s assetRepository) listAllResources(ty string) ([]*assetpb.Asset, error) {

filteredResults := []*assetpb.Asset{}
var erorString string
bgdanix marked this conversation as resolved.
Show resolved Hide resolved

for _, scope := range s.config.Scope {
cacheKey := "listAllResources" + scope
for _, scope := range s.config.Scopes {
cacheKey := fmt.Sprintf("listAllResources_%s", scope)
cachedResults := s.cache.GetAndLock(cacheKey)
defer s.cache.Unlock(cacheKey)

Expand Down Expand Up @@ -108,7 +111,11 @@ func (s assetRepository) listAllResources(ty string) ([]*assetpb.Asset, error) {
if err == iterator.Done {
break
}
if err != nil {
if err != nil && resource != nil{
erorString = erorString + fmt.Sprintf("For scope %s on resource %s got error: %s; ", scope, resource.AssetType, err.Error())
continue
}
if err != nil && resource == nil{
return nil, err
}
results = append(results, resource)
Expand All @@ -122,15 +129,21 @@ func (s assetRepository) listAllResources(ty string) ([]*assetpb.Asset, error) {
}
}
}

if len(erorString) > 0 {
return filteredResults, errors.New(erorString)
}

return filteredResults, nil
}

func (s assetRepository) searchAllResources(ty string) ([]*assetpb.ResourceSearchResult, error) {

filteredResults := []*assetpb.ResourceSearchResult{}
var erorString string

for _, scope := range s.config.Scope {
cacheKey := "SearchAllResources" + scope
for _, scope := range s.config.Scopes {
cacheKey := fmt.Sprintf("SearchAllResources_%s", scope)
cachedResults := s.cache.GetAndLock(cacheKey)
defer s.cache.Unlock(cacheKey)

Expand Down Expand Up @@ -167,7 +180,11 @@ func (s assetRepository) searchAllResources(ty string) ([]*assetpb.ResourceSearc
if err == iterator.Done {
break
}
if err != nil {
if err != nil && resource != nil{
erorString = erorString + fmt.Sprintf("For scope %s on resource %s got error: %s; ", scope, resource.AssetType, err.Error())
continue
}
if err != nil && resource == nil{
return nil, err
}
results = append(results, resource)
Expand All @@ -182,6 +199,10 @@ func (s assetRepository) searchAllResources(ty string) ([]*assetpb.ResourceSearc
}
}

if len(erorString) > 0 {
return filteredResults, errors.New(erorString)
}

return filteredResults, nil
}

Expand Down
14 changes: 7 additions & 7 deletions pkg/remote/google/repository/asset_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,9 @@ func Test_assetRepository_searchAllResources_CacheHit(t *testing.T) {
}

c := &cache.MockCache{}
c.On("GetAndLock", "SearchAllResources").Return(expectedResults).Times(1)
c.On("Unlock", "SearchAllResources").Times(1)
repo := NewAssetRepository(nil, config.GCPTerraformConfig{Scope: []string{""}}, c)
c.On("GetAndLock", "SearchAllResources_").Return(expectedResults).Times(1)
c.On("Unlock", "SearchAllResources_").Times(1)
repo := NewAssetRepository(nil, config.GCPTerraformConfig{Scopes: []string{""}}, c)

got, err := repo.searchAllResources("google_fake_type")
c.AssertExpectations(t)
Expand All @@ -52,10 +52,10 @@ func Test_assetRepository_searchAllResources_CacheMiss(t *testing.T) {
t.Fatal(err)
}
c := &cache.MockCache{}
c.On("GetAndLock", "SearchAllResources").Return(nil).Times(1)
c.On("Unlock", "SearchAllResources").Times(1)
c.On("Put", "SearchAllResources", mock.IsType([]*assetpb.ResourceSearchResult{})).Return(false).Times(1)
repo := NewAssetRepository(assetClient, config.GCPTerraformConfig{Scope: []string{""}}, c)
c.On("GetAndLock", "SearchAllResources_").Return(nil).Times(1)
c.On("Unlock", "SearchAllResources_").Times(1)
c.On("Put", "SearchAllResources_", mock.IsType([]*assetpb.ResourceSearchResult{})).Return(false).Times(1)
repo := NewAssetRepository(assetClient, config.GCPTerraformConfig{Scopes: []string{""}}, c)
bgdanix marked this conversation as resolved.
Show resolved Hide resolved

got, err := repo.searchAllResources("google_fake_type")
c.AssertExpectations(t)
Expand Down
13 changes: 7 additions & 6 deletions pkg/remote/google/repository/cloudresourcemanager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package repository

import (
"errors"
"fmt"
"strings"

"github.com/snyk/driftctl/pkg/remote/cache"
Expand Down Expand Up @@ -31,9 +32,9 @@ func (s *cloudResourceManagerRepository) ListProjectsBindings() (map[string]map[

bindingsByProject := make(map[string]map[string][]string)
errorsByProject := make(map[string]error)
var erorsString string
var erorString string

for _, scope := range s.config.Scope {
for _, scope := range s.config.Scopes {
if strings.Contains(scope, "projects/") {
project := strings.Split(scope, "projects/")[1]
request := new(cloudresourcemanager.GetIamPolicyRequest)
Expand All @@ -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

}
return bindingsByProject, errors.New(erorsString)
} else {
return bindingsByProject, nil
return bindingsByProject, errors.New(erorString)
}

return bindingsByProject, nil
}