Skip to content
This repository has been archived by the owner on Oct 15, 2024. It is now read-only.

Allow to invert Filter Results #226

Merged
merged 1 commit into from
Jul 10, 2018
Merged

Allow to invert Filter Results #226

merged 1 commit into from
Jul 10, 2018

Conversation

leflamm
Copy link
Contributor

@leflamm leflamm commented Jul 2, 2018

Hi!

invert: true can be added to any filter statement and would invert such filter result.

Can be useful to not only protect resources from being nuked, but to also explicitly define resources to be removed by inverting a filter result (else used for resource protection).

Cheers, Christian

@svenwltr
Copy link
Member

svenwltr commented Jul 2, 2018

Hello @leflamm,

what would happen, if you specify more than one filter with invert? It looks like it would filter everything then.

@leflamm
Copy link
Contributor Author

leflamm commented Jul 2, 2018

Hi @svenwltr, can you give a specific example? Thx!

@leflamm
Copy link
Contributor Author

leflamm commented Jul 2, 2018

Hi @svenwltr , I believe you are correct regarding two inverted exact matches.

IIUC, for glob or regex it's slightly different. As implemented at the moment filters defined like this

accounts:
  <some account>: # stage12
    filters:
      CloudFormationStack:
        - type: glob
          value: "*u*"
          invert: true
        - type: glob
          value: "*o*"
          invert: true

would filter (as in "protect from removal") all resources that:

  • contain "u"
  • contain "o"
  • contain neither

or to say it the other way: it would only remove resources containing both - "o" and "u".

@leflamm
Copy link
Contributor Author

leflamm commented Jul 2, 2018

But thanks for your hint! I believe I may have to think a little more about the desired behavior.

@svenwltr
Copy link
Member

svenwltr commented Jul 3, 2018

aws-nuke internally takes every resource and applies every filter on it. If a filter matches, it marks the node as filtered.

So for example, assume we have these Cloud Formation Stacks with your example filter: blub, blob, blib, bloub

  • blub: rule 1: false, rule 2: true → filtered
  • blob: rule 1: true, rule 2: false → filtered
  • blib: rule 1: true, rule 2: true → filtered
  • bloub: rule 1: false, rule 2: false → not filtered

So you are right. The inverting thing confused me a bit. Is this the behavior you want?

Also /cc @rebuy-de/prp-aws-nuke

@leflamm
Copy link
Contributor Author

leflamm commented Jul 3, 2018

It makes perfect sense for aws-nuke to work that way and to apply filters that way. For such a tiny feature I would never interfere with that logic and change any of it.

It's definitely a behavior I can live with - users simply need to understand how filters get applied.

There may be users who write down this...

accounts:
  <some account>: # stage12
    filters:
      CloudFormationStack:
        - type: glob
          value: "*u*"
          invert: true
        - type: glob
          value: "*o*"
          invert: true

... and wrongly assume any CloudFormationStack containing either "u" or "o" or both to be marked for removal - which is not how applying filters works. Still, it can easily be achieved by e. g.:

accounts:
  <some account>: # stage12
    filters:
      CloudFormationStack:
        - type: regex
          value: ".*[uv].*"
          invert: true

Maybe the README.md could use a hint like

aws-nuke internally takes every resource and applies every filter on it. If a filter matches, it marks the node as filtered.

Cheers, Christian

@svenwltr
Copy link
Member

svenwltr commented Jul 4, 2018

Sorry for the late response. It feels like this is something we might want to implement in a different way (eg something not in filter). I need some days to think about this.

/cc @rebuy-de/prp-aws-nuke @tomvachon Maybe you have some opinions on this?

@svenwltr
Copy link
Member

svenwltr commented Jul 6, 2018

Okay, since there are no other opinions and I still do not have new thoughts I guess we can just do this. Could you please improve the README like you suggested and squash your commits?

@bjoernhaeuser
Copy link
Member

@svenwltr Hm, i think that could easily be a foot-gun, right?

@svenwltr
Copy link
Member

svenwltr commented Jul 6, 2018

@bjoernhaeuser In what sense? People doing the configuration wrong? Encouraging people to use this for a wrong use case?

@bjoernhaeuser
Copy link
Member

Yeah, or having a configuration which noone understands anymore if you need a lot of filtering. But on the other side you probably do not need more than one filter a lot of times..

@bjoernhaeuser
Copy link
Member

Therefore: 👍

@leflamm
Copy link
Contributor Author

leflamm commented Jul 6, 2018

But on the other side you probably do not need more than one filter a lot of times

That's what I figured. Even if I want to nuke specific resources using 3 different patterns I can write

      <resource>:
        - type: regex
          property: <property>
          value: (<pattern1>|<pattern2>|<pattern3>)
          invert: true

I'll provide a README.md proposal and will squash the commits.

* introduce `invert` flag
* update readme, explain filter application
@leflamm
Copy link
Contributor Author

leflamm commented Jul 9, 2018

@svenwltr plz let me know if the README.md change works for you or if this needs to be more elaborate.

Cheers

Copy link
Member

@svenwltr svenwltr left a comment

Choose a reason for hiding this comment

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

@rebuy-de/prp-aws-nuke Please review.

@svenwltr svenwltr merged commit f6bbba1 into rebuy-de:master Jul 10, 2018
@svenwltr svenwltr added the kind/enhancement New core feature or improvement of existing ones. label Jul 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/enhancement New core feature or improvement of existing ones.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants