Aiven-Open / auth-for-apache-kafka

Aiven Authentication and Authorization Plugins for Apache Kafka®
Apache License 2.0
2 stars 3 forks source link

Reply with exception instead of empty list when creating/deleting ACLs #140

Open jeqo opened 1 year ago

jeqo commented 1 year ago

At the moment, the Authorizer is only designed for read-only capabilities. Though, create/delete ACLs is replying with an empty list and logging the error.

As a client calling these operations there's the misleading impression that ACLs may be created/deleted. Furthermore, this case an exception on the broker-side as broker also fails to map a successful response.

[2023-05-17 10:10:54,306] ERROR [KafkaApi-65] Unexpected error handling request RequestHeader(apiKey=CREATE_ACLS, apiVersion=3, clientId=adminclient-22, correlationId=11651) -- CreateAclsRequestData(cr>
java.lang.IndexOutOfBoundsException: 0 is out of bounds (min 0, max -1)
        at scala.collection.mutable.ArrayBuffer.apply(ArrayBuffer.scala:99)
        at kafka.server.AclApis.$anonfun$handleCreateAcls$8(AclApis.scala:115)
        at scala.collection.mutable.HashMap.getOrElse(HashMap.scala:436)
        at kafka.server.AclApis.$anonfun$handleCreateAcls$7(AclApis.scala:115)
        at scala.collection.StrictOptimizedIterableOps.map(StrictOptimizedIterableOps.scala:100)
        at scala.collection.StrictOptimizedIterableOps.map$(StrictOptimizedIterableOps.scala:87)
        at scala.collection.mutable.ArrayBuffer.map(ArrayBuffer.scala:43)
        at kafka.server.AclApis.sendResponseCallback$1(AclApis.scala:114)
        at kafka.server.AclApis.$anonfun$handleCreateAcls$10(AclApis.scala:127)
        at kafka.server.DelayedFuture.onComplete(DelayedFuture.scala:62)
        at kafka.server.DelayedOperation.forceComplete(DelayedOperation.scala:72)
        at kafka.server.DelayedFuture.tryComplete(DelayedFuture.scala:47)
        at kafka.server.DelayedOperation.safeTryCompleteOrElse(DelayedOperation.scala:110)
        at kafka.server.DelayedOperationPurgatory.tryCompleteElseWatch(DelayedOperation.scala:234)
        at kafka.server.DelayedFuturePurgatory.tryCompleteElseWatch(DelayedFuture.scala:85)
        at kafka.server.AclApis.handleCreateAcls(AclApis.scala:127)

Returning an exception should give a better experience until this functionality is supported.

ivanyu commented 1 year ago

This was done purposefully. I wouldn't advocate for or against this approach now, but we need to see how different clients may behave if we do this change.

jeqo commented 1 year ago

Agree, may require a major version bump as changes expected behavior