kafka-ops / julie

A solution to help you build automation and gitops in your Apache Kafka deployments. The Kafka gitops!
MIT License
418 stars 113 forks source link

Prevent delete topics if no managed resource prefixes #514

Closed damien-malescot closed 2 years ago

damien-malescot commented 2 years ago

Is your feature request related to a problem? Please describe. By default, if with have an topology.topic.managed.prefixes.0 JulieOps use it to create or delete topics. But if for some reasons we make a mistake when populate this property, we can delete all topics inside the cluster, there is no guard.

In example if we have an empty property :

topology.topic.managed.prefixes.0=""

When julieOps is executed, all topics will be deleted. It's not confortable for production use.

Describe the solution you'd like Add another property :

allow.delete.empty.prefixes=false

When true, JulieOps act as now, if a prefix is empty it delete all topics When false, JulieOps just ignore empty prefixes.

purbon commented 2 years ago

Hi Damien, certainly being a bit more protective is it always a good idea. Right now three configuration variables can help you protect what the tool can do.

  1. kafka.internal.topic.prefixes (default="_"), but you can add a list in similar fashion as managed.prefixes. Add https://github.com/kafka-ops/julie/commit/24a8843fcb64ee9ef8f7133430783e933ea490b5
  2. topology.topic.managed.prefixes (default=[]).
  3. allow.topic.delete (default=false)

The first will allow us to tell Julie what she can't manage, so it is like this to avoid any topic started with _, and avoid clearly the root question in your issue.

Because allow.topic.delete is by default false, you have to activate this future knownly.

What left me with the question of what happens if you write an empty topology.topic.managed.prefixes.0="" as you propose in your issue.

Currently the filtering code looks like:

 private boolean matchesPrefixList(String topic) {
    boolean matches =
        managedPrefixes.size() == 0 || managedPrefixes.stream().anyMatch(topic::startsWith);
    LOGGER.debug(String.format("Topic %s matches %s with $s", topic, matches, managedPrefixes));
    return matches;
  }

Thinking out loud, what might be meaningful to add is protection and fail at startup saying something like:

What do you think?

ludovic-boutros commented 2 years ago

Hi @purbon,

I think your proposal is great. An empty value is not a correct configuration value and should therefore throw an error.

Thank you.

damien-malescot commented 2 years ago

Hi,

Great proposal for me too, its perfect to secure our deployments.

Thanks

purbon commented 2 years ago

The add-on in #522 should do the trick for now. Feel free to reopen if necessary.

pfeigl commented 1 year ago

We just realized (thankfully on a test-cluster), that not having any prefixes defined still deletes all topics.

The change in #522 only tests for an empty value in the prefixes list, but does not test for a completely empty list!