Extract enumeration to it's own module #1554

moadibfr · 2022-06-29T07:53:35Z

Q	A
🐛 Bug fix?	no
🚀 New feature?	no
⚠ Deprecations?	no
❌ BC Break	no
🔗 Related issues
❓ Documentation	no

Description

First step to extract enumeration as a new library. Enumeration is now in a package with no dependencies to driftctl package.

Acc test for aws are passing I did not tested for gcp and azure due to setup issues.
Alerter is now inside enumeration which is wrong but when we rework the api/arch we should be able to get it back inside driftctl.
Due to the fact we do not normalize resources attribute, some scanner test goldenfile changed name or content.
Also I had to change enumerator for ec2_network_rules and vpc_security_group_rules to give them an ID at the enumeration step instead of waiting for the normalisation. It seems that the ID did not change thought I'm not sure if in some case it could change...

craigfurman · 2022-06-29T10:42:30Z

Great that we're doing this, this is going to be super useful. I do have a few questions though.

What is the actual API to use here? Do clients instantiate NewScanner()?
2. If so, can we put a facade in this package that looks somewhat like Enumerate(resourceType string) ([]Resource, error), that encapsulates any no-op Alerters and whatever else a Scanner needs for the driftctl-scan use-case, that may be less useful in an enumeration library?
Can we filter resources by type?
4. Preferably preventing them from being enumerated at all, rather than discarded in-memory. More like driftignore than --filter, in driftctl-scan terms.

moadibfr · 2022-06-29T13:25:00Z

What is the actual API to use here? Do clients instantiate NewScanner()?

Exactly, this will be odd and ugly but this is a first step 😓

If so, can we put a facade in this package that looks somewhat like Enumerate(resourceType string) ([]Resource, error), that encapsulates any no-op Alerters and whatever else a Scanner needs for the driftctl-scan use-case, that may be less useful in an enumeration library?

We could totally ! this is actually a great idea so when we change the api those using tis facade would not have to much to change !

Can we filter resources by type?

Yes, but as the interface is ugly that mean creating a filter (driftignore style) to do this.

Preferably preventing them from being enumerated at all, rather than discarded in-memory. More like driftignore than --filter, in driftctl-scan terms.

yeah the filter thing passed to the newscanner does that and we could easily put some code doing the facade to transform the esourceType string to a filter response

codecov-commenter · 2022-07-06T14:44:25Z

Codecov Report

Merging #1554 (9b407ef) into main (1f727ef) will increase coverage by 1.63%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1554      +/-   ##
==========================================
+ Coverage   81.41%   83.05%   +1.63%     
==========================================
  Files         442      172     -270     
  Lines       16215     5959   -10256     
==========================================
- Hits        13202     4949    -8253     
+ Misses       2687      870    -1817     
+ Partials      326      140     -186

Impacted Files	Coverage Δ
pkg/resource/google/google_compute_network.go	`14.28% <0.00%> (-85.72%)`	⬇️
pkg/resource/aws/aws_iam_policy_attachment.go	`20.00% <0.00%> (-80.00%)`	⬇️
pkg/resource/google/google_storage_bucket.go	`25.00% <0.00%> (-75.00%)`	⬇️
pkg/resource/deserializer.go	`0.00% <0.00%> (-64.00%)`	⬇️
...g/resource/google/google_compute_instance_group.go	`25.00% <0.00%> (-41.67%)`	⬇️
pkg/resource/google/google_project_iam_member.go	`20.00% <0.00%> (-26.16%)`	⬇️
pkg/resource/aws/aws_s3_bucket_policy.go	`62.50% <0.00%> (-20.84%)`	⬇️
pkg/resource/aws/aws_sns_topic_policy.go	`66.66% <0.00%> (-17.55%)`	⬇️
pkg/resource/aws/aws_sqs_queue_policy.go	`62.50% <0.00%> (-14.43%)`	⬇️
pkg/resource/aws/aws_kms_key.go	`66.66% <0.00%> (-11.91%)`	⬇️
... and 334 more

eliecharra

Thanks for that awesome job 🎉

I have on an aside note a lot of things to be fixed, but that could be done in other PR.
That one is bloated by tons of changes in pkg names, I tried to fix some but this still could lead to merge conflicts (I already rebased it an fixed two of them)

I've also let some comment about metadata, but I'm happy if we decide to merge that one and continue cleaning up the extraction in separated PRs.

TL;DR : nothing critical in my comments to the point that prevent a merge, so feel free to merge it ASAP if you feel comfortable with it @moadibfr

eliecharra · 2022-07-06T15:55:03Z

pkg/resource/aws/aws_appautoscaling_target.go

-	resourceSchemaRepository.SetHumanReadableAttributesFunc(AwsAppAutoscalingTargetResourceType, func(res *resource.Resource) map[string]string {
-		attrs := make(map[string]string)
-		if v := res.Attributes().GetString("scalable_dimension"); v != nil && *v != "" {
-			attrs["Scalable dimension"] = *v
-		}
-		return attrs
-	})


I don't get why we removed that from there, this is 100% related to driftctl and should not be in the enum lib.

yeah we already had a chat about this. I was wondering where it fitted the best and decided not to move them (cause yeah how I did was move the whole package and then move back what should not be in the enum,eration package)
I think in the end I agree with that and we can move that back to driftctl. I think in the end enumeration will have no schema and metadata anyway...

eliecharra · 2022-07-06T15:56:49Z

pkg/resource/aws/aws_appautoscaling_target.go

-	resourceSchemaRepository.SetResolveReadAttributesFunc(AwsAppAutoscalingTargetResourceType, func(res *resource.Resource) map[string]string {
-		return map[string]string{
-			"service_namespace":  *res.Attributes().GetString("service_namespace"),
-			"scalable_dimension": *res.Attributes().GetString("scalable_dimension"),
-		}
-	})


I'm fine to remove that from here since it's related to the enumeration process, but I think that can also be removed from the enum lib and those fields could be set directly in the enumerator function.

I think we could drop metadata notion from the enum lib completley, let's do that in another PR I'll be happy to handle that.

Yeah We should get rid of this but I did not wanted to make to many change in this PR and just move things around

eliecharra · 2022-07-06T15:57:22Z

pkg/resource/aws/aws_appautoscaling_target.go

-		}
-		return attrs
-	})
-	resourceSchemaRepository.SetFlags(AwsAppAutoscalingTargetResourceType, resource.FlagDeepMode)


Flags are 100% related to driftctl, we should keep them here

eliecharra · 2022-07-06T15:58:54Z

pkg/resource/aws/aws_appautoscaling_target.go

@@ -1,25 +1,12 @@
 package aws

-import "github.com/snyk/driftctl/pkg/resource"
-
-const AwsAppAutoscalingTargetResourceType = "aws_appautoscaling_target"


I'm in flavor of keeping those const here for now, I think they will be moved under internal or even dropped in the future in the enum lib

eliecharra · 2022-07-06T16:02:53Z

pkg/resource/aws/aws_route.go

-	resourceSchemaRepository.SetFlags(AwsRouteResourceType, resource.FlagDeepMode)
-}
-
-func CalculateRouteID(tableId, CidrBlock, Ipv6CidrBlock, PrefixListId *string) string {


I think this is used in a route expander middleware nope ? In my mind in the future the enum lib will mostly be under an internal/ pkg and will only expose List and Refresh methods. We should avoid to add too many coupling, it's IMO OK to have duplicated code here for that kind of stuff.

not sure about this one but there are some place where we compute ID and that may be needed for driftctl too when middleware create it too.
I think I checked that it was used in the enum before moving it but I might be wrong for this one...

eliecharra · 2022-07-06T16:15:44Z

pkg/resource/init_metadatas.go

+	"github.com/snyk/driftctl/pkg/resource/google"
+)
+
+func InitMetadatas(remote string,


This is already done in remote.Activate(), is that really usefull to call that manually ?

this is not the same init. This one will add normalize function that only exists in driftctl so it cannot be call from the enumeration function

moadibfr requested a review from a team as a code owner June 29, 2022 07:53

moadibfr requested review from eliecharra and removed request for a team June 29, 2022 07:53

moadibfr force-pushed the chore/extracted_enumeration branch from 063a7da to c2d583a Compare June 29, 2022 08:38

eliecharra self-assigned this Jul 5, 2022

eliecharra added the kind/maintenance Refactoring or changes to the workspace label Jul 5, 2022

eliecharra force-pushed the chore/extracted_enumeration branch 9 times, most recently from a2a673f to 9b407ef Compare July 6, 2022 16:35

eliecharra previously approved these changes Jul 6, 2022

View reviewed changes

eliecharra dismissed their stale review via 70216c7 July 7, 2022 13:29

eliecharra force-pushed the chore/extracted_enumeration branch 3 times, most recently from 0b0f295 to 2a9c1a7 Compare July 7, 2022 13:37

Martin Guibert and others added 7 commits July 7, 2022 16:16

chore: extract enumeration to it's own submodule

dc58f94

chore: mess with cache imports

7fcbbf9

chore: fix mess with repository imports

4fcd0e8

chore: fix mess with common imports

9392281

chore: fix mess with terraform2 imports

066aed9

chore: fix mess with aws2 imports

26fd9aa

chore: fix mess with terraform3 imports

81f2efb

eliecharra added 5 commits July 7, 2022 16:16

chore: fix mess with azurerm2 imports

83be84e

chore: fix mess with github2 imports

034b82e

chore: fix mess in GCP repos imports

2aa8d73

chore: fix mess with google2 imports

5659a8e

chore: fix mess with alerter2 imports

3c7a52b

eliecharra force-pushed the chore/extracted_enumeration branch from 2a9c1a7 to 3c7a52b Compare July 7, 2022 14:16

eliecharra merged commit 172e121 into main Jul 7, 2022

eliecharra deleted the chore/extracted_enumeration branch July 7, 2022 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract enumeration to it's own module #1554

Extract enumeration to it's own module #1554

moadibfr commented Jun 29, 2022

craigfurman commented Jun 29, 2022

moadibfr commented Jun 29, 2022

codecov-commenter commented Jul 6, 2022 •

edited

Loading

eliecharra left a comment

eliecharra Jul 6, 2022

moadibfr Jul 6, 2022

eliecharra Jul 6, 2022

moadibfr Jul 6, 2022

eliecharra Jul 6, 2022

eliecharra Jul 6, 2022

eliecharra Jul 6, 2022

moadibfr Jul 6, 2022

eliecharra Jul 6, 2022

moadibfr Jul 6, 2022

Extract enumeration to it's own module #1554

Extract enumeration to it's own module #1554

Conversation

moadibfr commented Jun 29, 2022

Description

craigfurman commented Jun 29, 2022

moadibfr commented Jun 29, 2022

codecov-commenter commented Jul 6, 2022 • edited Loading

Codecov Report

eliecharra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 6, 2022 •

edited

Loading