strimzi / strimzi-kafka-operator

Apache Kafka® running on Kubernetes
https://strimzi.io/
Apache License 2.0
4.78k stars 1.28k forks source link

[Enhancement] Add support for AclAuthorizer class for authorization #2880

Closed roland-haeusler closed 4 years ago

roland-haeusler commented 4 years ago

Is your feature request related to a problem? Please describe. Kafka 2.4.0 contains KIP-504 which contains a new class AclAuthorizer to replace the SimpleAclAuthorizer class. Strimzi is currently using the SimpleAclAuthorizer class for authorization.

The new AclAuthorizer contains new and important features (such as the ability to control the amount of authorization logs in the broker logs) and the SimpleAclAuthorizer will be deprecated.

Describe the solution you'd like Therefore, I would like to raise this feature request to add support for the new AclAuthorizer class for authorization in Strimzi.

Thanks & Kind Regards

Roland

scholzj commented 4 years ago

2891 will remove support for KAfka 2.3.1. Afterwards, this can be done. It sounds like a good stating task.

AndrewJSchofield commented 4 years ago

I'm happy to take this one on if no one else is working on it. I understand Kafka ACLs already which will be a help.

scholzj commented 4 years ago

Go for it - I assigned the issue to you.

AndrewJSchofield commented 4 years ago

Looking more into this issue, it seems to me that the new AclAuthorizer is intended to be compatible with the previous SimpleAclAuthorizer, just using a new Java API rather than the previous Scala trait.

It would be possible to introduce a third type of authorisation to go alongside simple and keycloak, but I think that this new Authorizer is intended as the replacement of the SimpleAclAuthorizer. So, I propose to try using AclAuthorizer when simple is configured and see how that works.

I think I will create new clusters with AclAuthorizer, and to permit existing ones to be modified from SimpleAclAuthorizer to AclAuthorizer. For clusters still using SimpleAclAuthorizer, I think it's appropriate to set a condition about a deprecated Authorizer. I could auto-migrate the Kafka cluster's configured Authorizer class over to AclAuthorizer, but that seems a little risky.

scholzj commented 4 years ago

That is my understanding as well. It should be just rewritte to Java. So TBH the Pr should basically by just changing the class and testing that everything still works. So I do not think we should add a new authorizer type nless we actually find any incompatibilities. We should just move from one to another during the Strimzi upgrade.

AndrewJSchofield commented 4 years ago

Works nicely just replacing the old Authorizer with the new one. Of course there's the upgrade path to consider and I'm new to this so I'll proceed carefully. My guess is that I'll target 0.19 with this rather than try to rush it in to 0.18.

scholzj commented 4 years ago

Yeah, I think that is totally fine.It will also give us more time to test and catch any issues.