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

Extract metadata from enumeration pkg #1572

Merged
merged 9 commits into from
Aug 1, 2022
Merged

Extract metadata from enumeration pkg #1572

merged 9 commits into from
Aug 1, 2022

Conversation

eliecharra
Copy link
Contributor

@eliecharra eliecharra commented Jul 28, 2022

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

Description

Metadata are now in driftctl pkg.
I removed completely func SetResolveReadAttributesFunc, attributes set in the enumerator are now used as it byt the detail fetcher.
The main issue was that the detail fetcher was only using string attributes in ReadResource, so I added some casting logic in it (see latest commits on tests fix).

This had some side effects on following resources:

  • aws_network_acl_rule (and by heritance we also had impact on aws_network_acl too)
  • aws_cloudformation_stack
  • aws_security_group_rule

I added a normalization pass on driftctl side after the remote scan to apply normalization. No now we retrieve resources then normalize them instead of doing the normalization directly while retrieving resources. That's why now there is two resource factories, one in enum pkg that only create the struct but without any modification. We got another factory in drifctl that is applying normalization.

All golden files rename can be ommited in the review, it's a side effect to have more attributes fields. Because now we are not only using fields from SetResolveReadAttributesFunc. That should not cause any issues to retrieve details if we send additional (useless) fields to the ReadResource call.

@eliecharra eliecharra added the kind/maintenance Refactoring or changes to the workspace label Jul 28, 2022
@eliecharra eliecharra requested a review from moadibfr July 28, 2022 09:57
elie.tf Outdated
@@ -0,0 +1,23 @@
provider "aws" {
region = "us-east-1"
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 added this file by mistake ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah good catch thanks a lot

@@ -69,7 +70,7 @@ func (e *S3BucketAnalyticEnumerator) Enumerate() ([]*resource.Resource, error) {
string(e.SupportedType()),
id,
map[string]interface{}{
"region": region,
"alias": region,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no sure I understand this change ˆˆ

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you read the previous func that defined read attributes for those s3 related resources, we were retrieviing the regions attribute and setting it to the "alias" field. Since we do not have this method anymore, I needed to change it there directly.

image

Does it answer your question ?

Those tests were broken due to removal of SetResolveReadAttributesFunc.
Sometimes in those methods we were casting field from different types to string.
If we loose that case it causes some issues since in the detail fetcher we only take into account
strings attributes. To fix that I added some cast directly in the detail fetcher.
That should not cause any issues to retrieve details if we send additional (useless) fields to the
ReadResource call.
@codecov-commenter
Copy link

Codecov Report

Merging #1572 (a18af39) into main (1b23b4e) will decrease coverage by 4.95%.
The diff coverage is 52.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1572      +/-   ##
==========================================
- Coverage   83.81%   78.86%   -4.96%     
==========================================
  Files         179      219      +40     
  Lines        6131     6918     +787     
==========================================
+ Hits         5139     5456     +317     
- Misses        847     1266     +419     
- Partials      145      196      +51     
Impacted Files Coverage Δ
pkg/cmd/scan.go 65.01% <0.00%> (-25.61%) ⬇️
pkg/middlewares/aws_alb_listener_transformer.go 100.00% <ø> (ø)
pkg/middlewares/aws_alb_transformer.go 100.00% <ø> (ø)
pkg/middlewares/aws_api_gateway_api_expander.go 79.50% <ø> (ø)
...es/aws_api_gateway_base_path_mapping_reconciler.go 100.00% <ø> (ø)
...middlewares/aws_api_gateway_deployment_expander.go 100.00% <ø> (ø)
...lewares/aws_api_gateway_domain_names_reconciler.go 100.00% <ø> (ø)
...g/middlewares/aws_api_gateway_resource_expander.go 91.42% <ø> (ø)
pkg/driftctl.go 76.76% <100.00%> (+1.38%) ⬆️
pkg/iac/terraform/state/terraform_state_reader.go 63.01% <100.00%> (-8.22%) ⬇️
... and 135 more

@eliecharra eliecharra merged commit d4cc400 into main Aug 1, 2022
@eliecharra eliecharra deleted the extract_metadata branch August 1, 2022 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/maintenance Refactoring or changes to the workspace
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants