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 a note on filtering #10

Merged
merged 1 commit into from
Mar 9, 2021
Merged

Add a note on filtering #10

merged 1 commit into from
Mar 9, 2021

Conversation

eliecharra
Copy link
Contributor

@eliecharra eliecharra requested a review from wbeuil March 9, 2021 09:25
docs/usage/filtering/rules.mdx Outdated Show resolved Hide resolved
docs/usage/filtering/rules.mdx Outdated Show resolved Hide resolved
@eliecharra eliecharra merged commit ab76433 into main Mar 9, 2021
@eliecharra eliecharra deleted the add_note_filtering branch March 9, 2021 14:28
@@ -3,6 +3,12 @@ id: rules
title: Filter Rules
---

:::info
Filter rules can be used not only to **scan** resources, but also to **ignore** resources.
Copy link

@adamdonahue adamdonahue Apr 15, 2021

Choose a reason for hiding this comment

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

I still think this is somewhat confusing. Even when you're excluding resources you're really just including all the others. A filter essentially says "run a scan on these objects only" -- and if the filter happens to say Type != "aws_ecr_repository you could think of this as "exclude these objects," but from a filtering point of view (for consistency) what you're really saying, as I understand it, is "include objects where the type isn't aws_ecr_repository." Yes, they mean the same thing, but in terms of the --filter option, it's the latter I feel captures the real semantics.

Copy link

@adamdonahue adamdonahue Apr 15, 2021

Choose a reason for hiding this comment

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

Regarding --filter, it appears you can specify this option multiple times:

driftctl scan --filter "Type == 'aws_s3_bucket'"  --filter "Type == 'aws_s3_bucket'"

which raises the question, if this is allowed, how are the two filters applied? As an intersection or a union?

What is the precedence of the .driftignore and --filter options. Does --filter override ignored resources, or does .driftignore override the filter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @adamdonahue thanks for your interest in driftctl and thanks for theses feedbacks 🙏🏻

I still think this is somewhat confusing. Even when you're excluding resources you're really just including all the others. A filter essentially says "run a scan on these objects only"

That's true, we should find a better wording for this.

which raises the question, if this is allowed, how are the two filters applied? As an intersection or a union?

Actually the latest filter flag only is taken into account, we should add a validation to enforce one and only one --filter flag at time.

What is the precedence of the .driftignore and --filter options. Does --filter override ignored resources, or does .driftignore override the filter?

Nice catch, this is something missing from the doc. Actually we run the filtering pass and then the driftignore pass. So if you include something explicitly in a filter expression but exclude it in driftignore, the resource will still be ignored. So this is not really an override but an union as you driftignore logic is used for exclusion only.

Choose a reason for hiding this comment

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

This is good to know, both cases. Thank you!

I hope you don't think I'm just being nitpicky and piling it on. Our use case is that we've implemented a .driftfilters file and a wrapper, which combines those lines into multiple --filter arguments. So we'll need to change that to create a chain of ORs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hope you don't think I'm just being nitpicky and piling it on. Our use case is that we've implemented a .driftfilters file and a wrapper, which combines those lines into multiple --filter arguments. So we'll need to change that to create a chain of ORs.

This is something very interesting, you are more interested on allow list that an exclude list. We should definitively think about integrating this kind of feature, thanks a lot for this feedback 🙏🏻

Copy link

@adamdonahue adamdonahue Apr 16, 2021

Choose a reason for hiding this comment

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

The reason is we have existing AWS accounts with thousands of resources that we are slowly moving to Terraform. So we would like to gradually opt-in those we expect to be there to differentiate from the ones we haven't yet migrated and are expected to be diffs. Does this make sense as a use case to you? If you have another suggestion, we're all ears.

You're right that we could also use an exclude list, but there will be mixed and matched cases, and the include feels safer. I like declaring what we want to look at vs. indicating what we don't, the reason being, if you suddenly implement a new resource check then it might slip in because we forgot to add a .driftignore file, whereas with a filter we always declare what we are specifically looking for.

This is all good stuff. The fact you have this tooling is excellent for compliance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are working on a driftignore generator command, I think it can cover this usecase snyk/driftctl#415 (comment)

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.

3 participants