Skip to content

add proposal for multi namespace topic operator#204

Open
KyriosGN0 wants to merge 2 commits intostrimzi:mainfrom
KyriosGN0:topic
Open

add proposal for multi namespace topic operator#204
KyriosGN0 wants to merge 2 commits intostrimzi:mainfrom
KyriosGN0:topic

Conversation

@KyriosGN0
Copy link

This PR aims to introduce a multi namespace topic operator, its based on the proposal for UTO, and uses a different approach as opposed to strimzi/strimzi-kafka-operator#1206

Signed-off-by: AvivGuiser <avivguiser@gmail.com>
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

Thanks for the proposal. I left some comments about things that would not work or would be blockers for me in terms of backwards compatiblity etc.

But I do not think that this is the right direction. While it might create something that somehow works, I think it would be a hacky solution with a bad design and user experience. I think if we want to seriously go into the multi-namespace world, we might need to start from scratch and create a better match/respect for the Kubernetes namespace model.


The Cluster Operator creates a `Role` and `RoleBinding` in each watched namespace, granting the Entity Operator's `ServiceAccount` permissions to watch, list, get, and update `KafkaTopic` resources and their status.

```yaml
Copy link
Member

Choose a reason for hiding this comment

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

Tha naming here as well as below does not work because it is not unique. You need to create a unique naming mechanism (likely cluster name, namespace + some suffix).

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, I updated the proposal accordingly to ensure uniqueness


The existing `strimzi.io/managed: false` annotation continues to work as before — it allows a `KafkaTopic` to be deleted without deleting the Kafka topic, enabling ownership transfers.

### Preventing overlapping Topic Operator instances
Copy link
Member

Choose a reason for hiding this comment

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

There is already an existing mechanim how to designate to which cluster the topic belongs through the strimzi.io/cluster label. Both from the user as well as from the TO perspective.

I do not understand why don't you use that? You will anyway need to extend it. It also has backwards compatibility implications that you would need to address.

Copy link
Author

Choose a reason for hiding this comment

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

Good Point, i updated the proposal accordingly

Copy link
Member

Choose a reason for hiding this comment

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

I don't think the fix works.

  • How does the operator know it is in single namespace or multi namespace mode?
  • How does it know if there are more clusters (more TOs watching the same namespace) or not?

So I'm not sure this works or creates backwards compatile solution.

**Adding a namespace to `watchedNamespaces`:**
The Cluster Operator creates RBAC resources in the new namespace and restarts/reconfigures the TO. The TO starts watching the new namespace and reconciles any existing `KafkaTopic` resources in it.

**Removing a namespace from `watchedNamespaces`:**
Copy link
Member

Choose a reason for hiding this comment

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

That does not work because once you remove the namespace, it is beyond the operator's reach. So it cannot remove the finalizers.

Copy link
Author

Choose a reason for hiding this comment

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

I think you mean when someone else deletes the namespaces, and not when someone removes a namespace from the watchedNamespaces list, in the first case you are right and manual intervention is required, but if i remove a namespace from the operator's config i can perform action before i remove RBAC, i updated the proposal to make sure this is clear

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, maybe my comment was too confusing.

  • Cluster Operator does not touch any topics. It should not remove any finalizers from them. (not 100% sure the CO is typo here or intention)
  • Topic Operator would never know that some namespace was removed as it is removed through restart. So it starts with the new namespace configuration and has no idea a namespace was removed.
  • And even if it knew about it, it would not have the RBAC rights to do anything with the topics because the RBAC rights are managed independently through CO, and there is no synchronization point.
  • Plus we in general do not delete the user-facing custom resources.

I do not think you can really fix it to be honest. All what can be done here is that this is user's responsibility to either delete the topics first or manually later.

spec:
entityOperator:
topicOperator:
watchedNamespaces:
Copy link
Member

Choose a reason for hiding this comment

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

How does this handle compatibility witht he existing watchNamespace field?

Copy link
Author

Choose a reason for hiding this comment

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

Updated the Proposal, i went with a route of depreacting this field in favor of the plural one

Copy link
Member

Choose a reason for hiding this comment

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

Deprecation is fine. But we are not abandoning fields from one day to another. We do things in backwards compatible way.


```yaml
status:
conditions:
Copy link
Member

Choose a reason for hiding this comment

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

If you don't manage it, you should not touch it. That will just cause tight reconciliation look as the two operators fight over the updates.

Copy link
Author

Choose a reason for hiding this comment

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

Addressed

Signed-off-by: AvivGuiser <avivguiser@gmail.com>
@KyriosGN0
Copy link
Author

Thanks @scholzj, I Have addressed your comments

  1. Updated the RBAC names to be unique
  2. added that the new plural watchNamespaces is deprecating watchNamespace
  3. clarified that removing a namespace from watchNamespaces is different from deleting a namespace

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.

2 participants