ekristen / aws-nuke

Remove all the resources from an AWS account
https://ekristen.github.io/aws-nuke/
MIT License
34 stars 6 forks source link

Tags are not respected? #110

Closed YuriGal closed 4 months ago

YuriGal commented 4 months ago

We tag our resources that we want to keep from nuking with key do-not-nuke and value true, then we add this to filter section - and for majority of resources it works. But for some resources it's ignored. For example we tagged a Sagemaker domain:

Screenshot 2024-03-07 at 4 29 54 PM

And using filter similar to this one:

accounts:
  '123456789':
    filters:
      SageMakerDomain:
      - property: "tag:do-not-nuke"
        value: "true"

Similar setup works for majority of other resources, yet here we're getting output

us-west-2 - SageMakerDomain - d-lkdfjldfadsf - [DomainID: "d-lkdfjldfadsf"] - would remove

is it a bug, or some omission on our side? How do we make it respect the tags?

ekristen commented 4 months ago

@YuriGal it looks like SageMakerDomain doesn't have tag support, should be trivial for me to add, I'll take a look tonight.

YuriGal commented 4 months ago

That would be awesome! Would it be possible to add this to other similar resources? I know some resources can't be tagged at all in AWS, but if all of those that can could be filtered by tags it would be great.

ekristen commented 4 months ago

Yes, as long as the resource supports tags, the resource in this project just needs to be identified as needing it's tags exposed and then filtering will work.

YuriGal commented 4 months ago

Not sure if I am doing something wrong, but with v3.0.0-beta.32 SageMakerDomain is always excluded, no matter tagged or not.

ekristen commented 4 months ago

What happens exactly? Does it show up as "filtered by config"? Can you please provide the output and the config?

ekristen commented 4 months ago

@YuriGal let me know when you get a chance. I'll see what I can do to replicate or test.

YuriGal commented 4 months ago

Ok, so to simplify testing I am using a basic config like this

regions:
  - global
  - us-west-2
  - us-east-1
  - eu-central-1
  - af-south-1
account-blocklist:
  - "123456789"
accounts:
  '987654321':
    filters:
      SageMakerDomain:
      - property: "tag:role:do-not-nuke"
        value: "true"
resource-types:
  targets:
    - SageMakerDomain

There is a sagemaker domain in us-west-2, but no matter whether it's tagged or not I am getting

Scan complete: 0 total, 0 nukeable, 0 filtered.
ekristen commented 4 months ago

@YuriGal I broke it when I added tests. Fix inbound.

YuriGal commented 4 months ago

Thanks! And thank you for such a fast response time.

ekristen commented 4 months ago

:tada: This issue has been resolved in version 3.0.0-beta.33 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket:

YuriGal commented 4 months ago

Looks like it's works properly for SageMakerDomain, thanks!

Would it be possible to do the same for other resources that support tags, but tags aren't exposed?

ekristen commented 4 months ago

Ideally just need an issue with a list of Resources that are missing tags that support tags and I can look into it. It depends on the resource and the API as to how much effort it takes.

YuriGal commented 4 months ago

I think I can provide the list, just have to collect it.

Meanwhile I am afraid I found another bug, will open a separate issue.

YuriGal commented 4 months ago

Ok, so teams provided me with following list of resources that were tagged, but got nuked anyway

SageMakerUserProfiles
ECSCluster
ServiceDiscoveryNamespace
SQSQueue
CognitoUserPoolDomain
CognitoUserPoolClient
CognitoUserPool
CognitoIdentityProvider
ElasticacheSubnetGroup
ElasticacheCacheCluster

I know for sure that this is true for ECSCluster and SQSQueue, but didn't get a chance to verify other ones.

YuriGal commented 4 months ago

btw, (unrelated) question: does this nuke supports --max-wait-retries option?

ekristen commented 4 months ago

It's suppose to, but looking at the options, it appears to be missing. The goal is to be 99% backwards compatible with the original, minus the nuke subcommand.

I'll fix this oversight, mind opening an issue? You'll get notified that way when it get's fixed.

ekristen commented 1 month ago

:tada: This issue has been resolved in version 3.0.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: