Skip to content

Conversation

@nodece
Copy link
Member

@nodece nodece commented Oct 20, 2025

Motivation

The current authentication flow in broker is tightly coupled with the ServerCnx class, making it harder to extend authentication logic (e.g., for proxy authentication, role forwarding). This PR refactors the authentication handling into dedicated classes to make the flow more modular, extensible, and easier to maintain.

Modifications

  • Decouple authentication from server connection handling for cleaner separation of concerns
  • Enable reuse of authentication logic for proxy authentication

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@nodece nodece requested a review from Copilot October 20, 2025 11:14
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Oct 20, 2025
@nodece nodece requested review from dao-jun and lhotari October 20, 2025 11:14
@nodece nodece self-assigned this Oct 20, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Refactors broker authentication by extracting the logic from ServerCnx into dedicated classes for modularity and reuse (e.g., proxy/original-role authentication).

  • Introduces BinaryAuthSession and BinaryAuthContext to encapsulate auth flow and context
  • Updates ServerCnx to delegate auth/connect/auth-response handling to BinaryAuthSession
  • Adds a factory method in AuthenticationService to create BinaryAuthSession

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.

File Description
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java Replaces inline authentication logic with BinaryAuthSession orchestration during connect and auth responses.
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/BinaryAuthSession.java New class implementing the binary authentication flow, including proxy/original-client handling and challenges.
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/BinaryAuthContext.java New context container for auth session parameters (connect command, SSL, executor, etc.).
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationService.java Adds a simple factory method to create BinaryAuthSession.

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@nodece nodece marked this pull request as draft October 20, 2025 11:29
nodece and others added 4 commits October 21, 2025 10:56
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@nodece nodece marked this pull request as ready for review October 21, 2025 03:02
@nodece
Copy link
Member Author

nodece commented Oct 21, 2025

/pulsarbot rerun-failure-checks

@nodece nodece requested a review from Copilot October 21, 2025 13:40
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

nodece and others added 5 commits October 21, 2025 22:36
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-not-needed Your PR changes do not impact docs ready-to-test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants