KAFKA-18440: Admin does not convert the AuthorizationException to fatal error in using bootstrap controllers #18435
Conversation
| @@ -280,6 +281,9 @@ public void updateFailed(Throwable exception) { | |||
| if (exception instanceof AuthenticationException) { | |||
| log.warn("Metadata update failed due to authentication error", exception); | |||
| this.fatalException = (ApiException) exception; | |||
| } else if (exception instanceof ClusterAuthorizationException) { | |||
There was a problem hiding this comment.
should this be AuthorizationException here since all authZ exceptions are non-retriable?
| @@ -280,6 +281,9 @@ public void updateFailed(Throwable exception) { | |||
| if (exception instanceof AuthenticationException) { | |||
There was a problem hiding this comment.
Please add a function isFatalException and add AuthenticationException, AuthorizationException, SecurityDisabledException, UnsupportedEndpointTypeException, UnsupportedForMessageFormatException, UnsupportedVersionException etc.
You can add this function in RequestUtils class.
There was a problem hiding this comment.
Thanks for the review. This Jira would like to focus on fatal exception for metadata call in Admin.
IIRC, The SecurityDisabledException is used in create / delete ACLs and UnsupportedForMessageFormatException is used in produce request, so I don't add them.
Also, in UnsupportedVersionException, it logs different message for bootstrap and controller node, but other exceptions logs same message for different node type, so I don't encapsulate all exceptions in a utility function.
There was a problem hiding this comment.
I understand that this is increasing the scope of the JIRA. What I am trying to suggest here will help us prevent such problems in future for cases where a client retries on a fatal exception. If we have a utility method, we can consolidate the logic of what is considered fatal at one place and existing/future calls to server APIs can use that utility to handle exceptions. It will prevent bugs in future if new authors can use the utility method.
There was a problem hiding this comment.
Thanks for the comment. Yeah, it's good to have a utility function for it. I add it to RequestUtils#isFatalException.
89b6aeb to
6ba95ba
Compare
…al error in using bootstrap controllers Signed-off-by: PoAn Yang <payang@apache.org>
6ba95ba to
e9d5260
Compare
Admin use DescribeClusterRequest to build metadata when using bootstrap controllers, and controller APIs may return ClusterAuthorizationException when users have no "ALTER" permission (see #14306 (comment)).
However, admin does not convert the authorized exception to fatal exception (https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/clients/admin/internals/AdminMetadataManager.java#L276), so it keeps sending the request to controller until timeout.
Committer Checklist (excluded from commit message)