search

ekristen / libnuke

Common Golang Packages for use by the Various Cloud Nuke Tools
MIT License
20 stars 3 forks source link

Deprecation resolution for ResourceTypes #61

Open darkowlzz opened 2 months ago

darkowlzz commented 2 months ago

While migrating from rebuy-de/aws-nuke to libnuke in https://github.com/fluxcd/test-infra/pull/39, aws-nuke failed to delete EKSCluster. There were multiple errors. Some of them related to removing role from instance profile that lead me to https://github.com/rebuy-de/aws-nuke/issues/702, which didn't lead to any conclusion. But @ekristen your last comment about limiting the resource types lead me to checking the includes that I have configured. Coming from rebuy-de/aws-nuke, I had EKSNodegroups in my configuration. I did see the warning

WARN[0002] deprecated resource type 'EKSNodegroups' - converting to 'EKSNodegroup'

but the listed resources didn't contain any nodegroup. When I used the new name, it worked perfectly.

In Config.ResolveDeprecations(), refer https://github.com/ekristen/libnuke/blob/v0.16.0/pkg/config/config.go#L239-L255, where this conversion takes place, only the Account.Filters are updated with the new replacement. The Account.ResourceTypes.Includes and other fields remain as they are. I'm not sure if this is intended but I tried replacing the includes as well in a local copy and that made it work as expected. Is this the intended behavior for deprecation resolution?

darkowlzz commented 2 months ago

I may be holding it incorrectly, but another thing I noticed was that I didn't see the deprecated resource type warning when using the CLI. I saw this when I created a libnuke config and passed a logger. In aws-nuke https://github.com/ekristen/aws-nuke/blob/v3.1.0/pkg/commands/nuke/nuke.go#L73-L76, no logger is passed and libnuke seems to create a discard logger for that in https://github.com/ekristen/libnuke/blob/v0.16.0/pkg/config/config.go#L97-L101, leading to no visible deprecation log even if I enable debug log with -l debug. Is there some other way to see such warning logs?

ekristen commented 2 months ago

https://github.com/ekristen/libnuke/blob/v0.16.0/pkg/config/config.go#L239-L255

This was the original behavior, debatable if that should be changed. It's one thing to morph a bad config another to include a resource that might not meant to have been included.

What do you think about throwing an error if using the a deprecated alias OR a non-existent resource when using includes/excludes?

I may be holding it incorrectly, but another thing I noticed was that I didn't see the deprecated resource type warning when using the CLI.

It does look like I have a bug in aws-nuke proper where I'm not passing in the logger, so I do need to fix that.

darkowlzz commented 2 months ago

What do you think about throwing an error if using the a deprecated alias OR a non-existent resource when using includes/excludes?

For non-existing resource types, throwing error seems better as the provided resource is not known and has no use. But I think that's different from deprecated alias. Deprecated aliases are well defined, finite and strictly map to their new values. There's less of a change of anything going wrong unless multiple things have the same alias. My understanding is that the deprecation resolution is to help smoothly migrate and retain backwards compatibility with rebuy-de/aws-nuke. If an error is returned for the now deprecated resource names, it won't be backwards compatible anymore.

Compared to the current behavior, automatic migration or returning error will make it much easier to figure out what went wrong in the scenario I had.