Skip to content

Conversation

@harishva23
Copy link

@harishva23 harishva23 commented Nov 5, 2025

About this change - What it does

This PR fixes 2 issues - error handling inside the follower and error handling for file-based basic authentication

Earlier, the follower could not parse error messages from leader if write requests were made to the follower and it failed due to some error like incompatible or invalid schema , trying to delete a non-existent schema etc. Now this is able to handle these error messages and pass it on to the user or client.

In the file-based authentication, if a non-existent user was sent in the Headers ( a username not in auth file), karapace threw an Internal Server Error instead of Unauthorized message. This did not happen when a correct username but wrong password was passed. So I have fixed this issue and it returns an Unauthorized error message for both the cases.

References: #1154
Why this way

Since Confluent client always expects a FastAPI response, I have changed the code to return proper format of error messages from follower. This leads to consistent error messages from both leader and follower so that the user can decide the next steps.

For the file-based authentication, raising a generic ValueError results in Internal Server Error being thrown and a full stacktrace appears in the logs which reveals that User is not found. Since karapace already throws a Unauthorised Error if wrong password is sent, I have removed the logic raising ValueError and instead returning None type which is then handled in the auth.py file in the authenticate function.

I have also included test cases for forwarding client as well as the basic authentication tests as well. For the follower client, I have patched the SSL context creation for the tests.

@harishva23 harishva23 requested a review from a team as a code owner November 5, 2025 05:19
@muralibasani
Copy link
Contributor

@harishva23 I see a conflict. Can you pls fix ?

@harishva23
Copy link
Author

@harishva23 I see a conflict. Can you pls fix ?

Hi @muralibasani , the commit for #1140 has been reverted after I raised the PR. Therefore I shouldn't include that changes in my PR right?

@muralibasani
Copy link
Contributor

@harishva23 I see a conflict. Can you pls fix ?

Hi @muralibasani , the commit for #1140 has been reverted after I raised the PR. Therefore I shouldn't include that changes in my PR right?

Looks ok now.

@muralibasani
Copy link
Contributor

muralibasani commented Nov 6, 2025

@harishva23 some lint issues.

@harishva23
Copy link
Author

@harishva23 some lint issues.

Oh ok.. I will fix that whenever I am free and I will drop a comment when finished.

@harishva23
Copy link
Author

Hi @muralibasani due to an issue with a Github config, the commits are being duplicated . I will try resolving this issue when i can so that the branch is clean and then all Github actions can be run. Sorry about that.

@harishva23 harishva23 force-pushed the fix/follower-error-handling-clean branch from 3b0cfe0 to 1f3b517 Compare November 6, 2025 10:48
@harishva23
Copy link
Author

Hi @muralibasani I have fixed the lint issues using the pre-commit tool and I have ran my tests locally with all cases passing.

@muralibasani muralibasani force-pushed the fix/follower-error-handling-clean branch from 1f3b517 to 7dfdd23 Compare November 14, 2025 08:10
@muralibasani
Copy link
Contributor

@harishva23 harishva23 marked this pull request as draft December 4, 2025 14:03
@harishva23 harishva23 marked this pull request as ready for review December 5, 2025 12:58
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