[improve][broker] ServerCnx: go to Failed state when auth fails by michaeljmarshall · Pull Request #19312 · apache/pulsar

michaeljmarshall · 2023-01-23T20:53:49Z

Motivation

When authentication fails in the ServerCnx, the state is left in Start if the primary authData fails authentication and in Connecting or Connected if the originalAuthData authentication fails. To prevent any kind of unexpected behavior, we should go to Failed state.

Note that the tests verify the current behavior where a failed originalAuthData results first in a Connected command from the broker and then an Error command. I documented that I think this is sub optimal here #19311.

Modifications

Update ServerCnx state to Failed when there is an authentication exception during handleConnect and during handleAuthResponse.
Update handleAuthResponse reply to "Unable to authenticate" instead of the AuthenticationState exception.

Verifying this change

A new test is added. The added test covers the change made in #19295 where we updated ServerCnx so that we call AuthState#authenticate instead of relying on the implementation detail that the initialization calls authenticate. That PR should have added a test.

Does this pull request potentially affect one of the following parts:

This is not a breaking change.

Documentation

doc-not-needed

Matching PR in forked repository

PR in forked repository: michaeljmarshall#18

lhotari · 2023-01-24T07:25:35Z

/pulsarbot rerun-failure-checks

nicoloboschi

it looks like we have the same issue in the handleAuthResponse method.
It's fine to create a separated pull request

michaeljmarshall · 2023-01-24T23:32:13Z

Great suggestion @nicoloboschi. I updated this PR since it fits well within the scope. I also extended the tests to cover this part of the protocol.

codecov-commenter · 2023-01-25T01:17:31Z

Codecov Report

❌ Patch coverage is 66.66667% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.49%. Comparing base (8d81392) to head (6fecb6f).
⚠️ Report is 2951 commits behind head on master.

Files with missing lines	Patch %	Lines
...va/org/apache/pulsar/broker/service/ServerCnx.java	66.66%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #19312      +/-   ##
============================================
- Coverage     63.93%   63.49%   -0.44%     
- Complexity     3470    25900   +22430     
============================================
  Files          1818     1818              
  Lines        133061   133079      +18     
  Branches      14637    14637              
============================================
- Hits          85067    84499     -568     
- Misses        40270    40818     +548     
- Partials       7724     7762      +38

Flag	Coverage Δ
inttests	`24.88% <0.00%> (-0.07%)`	⬇️
systests	`25.62% <33.33%> (-0.08%)`	⬇️
unittests	`60.85% <66.66%> (-0.46%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...va/org/apache/pulsar/broker/service/ServerCnx.java	`52.98% <66.66%> (-1.94%)`	⬇️

... and 203 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…he#19312) PIP: apache#12105 When authentication fails in the `ServerCnx`, the state is left in `Start` if the primary `authData` fails authentication and in `Connecting` or `Connected` if the `originalAuthData` authentication fails. To prevent any kind of unexpected behavior, we should go to `Failed` state. Note that the tests verify the current behavior where a failed `originalAuthData` results first in a `Connected` command from the broker and then an `Error` command. I documented that I think this is sub optimal here apache#19311. * Update `ServerCnx` state to `Failed` when there is an authentication exception during `handleConnect` and during `handleAuthResponse`. * Update `handleAuthResponse` reply to `"Unable to authenticate"` instead of the `AuthenticationState` exception. A new test is added. The added test covers the change made in apache#19295 where we updated `ServerCnx` so that we call `AuthState#authenticate` instead of relying on the implementation detail that the initialization calls `authenticate`. That PR should have added a test. This is not a breaking change. - [x] `doc-not-needed` PR in forked repository: #18 (cherry picked from commit 8049690)

…he#19312) PIP: apache#12105 When authentication fails in the `ServerCnx`, the state is left in `Start` if the primary `authData` fails authentication and in `Connecting` or `Connected` if the `originalAuthData` authentication fails. To prevent any kind of unexpected behavior, we should go to `Failed` state. Note that the tests verify the current behavior where a failed `originalAuthData` results first in a `Connected` command from the broker and then an `Error` command. I documented that I think this is sub optimal here apache#19311. * Update `ServerCnx` state to `Failed` when there is an authentication exception during `handleConnect` and during `handleAuthResponse`. * Update `handleAuthResponse` reply to `"Unable to authenticate"` instead of the `AuthenticationState` exception. A new test is added. The added test covers the change made in apache#19295 where we updated `ServerCnx` so that we call `AuthState#authenticate` instead of relying on the implementation detail that the initialization calls `authenticate`. That PR should have added a test. This is not a breaking change. - [x] `doc-not-needed` PR in forked repository: #18 (cherry picked from commit 8049690) (cherry picked from commit 3ef3bf1)

…he#19312) PIP: apache#12105 When authentication fails in the `ServerCnx`, the state is left in `Start` if the primary `authData` fails authentication and in `Connecting` or `Connected` if the `originalAuthData` authentication fails. To prevent any kind of unexpected behavior, we should go to `Failed` state. Note that the tests verify the current behavior where a failed `originalAuthData` results first in a `Connected` command from the broker and then an `Error` command. I documented that I think this is sub optimal here apache#19311. * Update `ServerCnx` state to `Failed` when there is an authentication exception during `handleConnect` and during `handleAuthResponse`. * Update `handleAuthResponse` reply to `"Unable to authenticate"` instead of the `AuthenticationState` exception. A new test is added. The added test covers the change made in apache#19295 where we updated `ServerCnx` so that we call `AuthState#authenticate` instead of relying on the implementation detail that the initialization calls `authenticate`. That PR should have added a test. This is not a breaking change. - [x] `doc-not-needed` PR in forked repository: michaeljmarshall#18 (cherry picked from commit 8049690) (cherry picked from commit 3ef3bf1) (cherry picked from commit 168aa6a)

…he#19312) PIP: apache#12105 When authentication fails in the `ServerCnx`, the state is left in `Start` if the primary `authData` fails authentication and in `Connecting` or `Connected` if the `originalAuthData` authentication fails. To prevent any kind of unexpected behavior, we should go to `Failed` state. Note that the tests verify the current behavior where a failed `originalAuthData` results first in a `Connected` command from the broker and then an `Error` command. I documented that I think this is sub optimal here apache#19311. * Update `ServerCnx` state to `Failed` when there is an authentication exception during `handleConnect` and during `handleAuthResponse`. * Update `handleAuthResponse` reply to `"Unable to authenticate"` instead of the `AuthenticationState` exception. A new test is added. The added test covers the change made in apache#19295 where we updated `ServerCnx` so that we call `AuthState#authenticate` instead of relying on the implementation detail that the initialization calls `authenticate`. That PR should have added a test. This is not a breaking change. - [x] `doc-not-needed` PR in forked repository: #18 (cherry picked from commit 8049690) (cherry picked from commit 3ef3bf1) (cherry picked from commit 467cd32)

…he#19312) PIP: apache#12105 When authentication fails in the `ServerCnx`, the state is left in `Start` if the primary `authData` fails authentication and in `Connecting` or `Connected` if the `originalAuthData` authentication fails. To prevent any kind of unexpected behavior, we should go to `Failed` state. Note that the tests verify the current behavior where a failed `originalAuthData` results first in a `Connected` command from the broker and then an `Error` command. I documented that I think this is sub optimal here apache#19311. * Update `ServerCnx` state to `Failed` when there is an authentication exception during `handleConnect` and during `handleAuthResponse`. * Update `handleAuthResponse` reply to `"Unable to authenticate"` instead of the `AuthenticationState` exception. A new test is added. The added test covers the change made in apache#19295 where we updated `ServerCnx` so that we call `AuthState#authenticate` instead of relying on the implementation detail that the initialization calls `authenticate`. That PR should have added a test. This is not a breaking change. - [x] `doc-not-needed` PR in forked repository: #18 (cherry picked from commit 8049690) (cherry picked from commit 3ef3bf1) (cherry picked from commit 467cd32) (cherry picked from commit 1fab71b)

michaeljmarshall · 2023-02-27T22:03:15Z

I cherry picked this because it added some important tests that other important bug fixes relied on.

…ls (apache#19312)" This reverts commit 467cd32.

…ls (apache#19312)" This reverts commit 94de805.

…he#19312) PIP: apache#12105 When authentication fails in the `ServerCnx`, the state is left in `Start` if the primary `authData` fails authentication and in `Connecting` or `Connected` if the `originalAuthData` authentication fails. To prevent any kind of unexpected behavior, we should go to `Failed` state. Note that the tests verify the current behavior where a failed `originalAuthData` results first in a `Connected` command from the broker and then an `Error` command. I documented that I think this is sub optimal here apache#19311. * Update `ServerCnx` state to `Failed` when there is an authentication exception during `handleConnect` and during `handleAuthResponse`. * Update `handleAuthResponse` reply to `"Unable to authenticate"` instead of the `AuthenticationState` exception. A new test is added. The added test covers the change made in apache#19295 where we updated `ServerCnx` so that we call `AuthState#authenticate` instead of relying on the implementation detail that the initialization calls `authenticate`. That PR should have added a test. This is not a breaking change. - [x] `doc-not-needed` PR in forked repository: #18 (cherry picked from commit 8049690)

[improve][broker] ServerCnx: go to Failed state when auth fails

0b0731a

michaeljmarshall added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/broker labels Jan 23, 2023

michaeljmarshall added this to the 2.12.0 milestone Jan 23, 2023

michaeljmarshall requested review from congbobo184, lhotari, merlimat, nicoloboschi and nodece January 23, 2023 20:53

michaeljmarshall self-assigned this Jan 23, 2023

github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Jan 23, 2023

aymkhalil approved these changes Jan 23, 2023

View reviewed changes

lhotari approved these changes Jan 24, 2023

View reviewed changes

nicoloboschi approved these changes Jan 24, 2023

View reviewed changes

Use Close callback in ServerCnx; go to failed in AuthResponse

fa62ba5

fix typo

6fecb6f

michaeljmarshall merged commit 8049690 into apache:master Jan 25, 2023

michaeljmarshall deleted the pip-97-call-authenticate-on-auth-state branch January 25, 2023 03:38

michaeljmarshall added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Feb 27, 2023

michaeljmarshall added cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.10 cherry-picked/branch-2.11 release/2.8.5 release/2.11.1 release/2.10.5 release/2.9.6 labels Feb 27, 2023

coderzc added release/2.9.5 and removed release/2.9.6 labels Mar 2, 2023

hangc0276 added a commit to hangc0276/pulsar that referenced this pull request Mar 15, 2023

Revert "[improve][broker] ServerCnx: go to Failed state when auth fai…

4d485fd

…ls (apache#19312)" This reverts commit 467cd32.

hangc0276 mentioned this pull request Mar 15, 2023

[Branch-2.10] Revert [improve][broker] Require authRole is proxyRole to set originalPrincipal(#19455) #19824

Closed

15 tasks

hangc0276 added a commit to hangc0276/pulsar that referenced this pull request Mar 15, 2023

Revert "[improve][broker] ServerCnx: go to Failed state when auth fai…

e7ac93a

…ls (apache#19312)" This reverts commit 94de805.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[improve][broker] ServerCnx: go to Failed state when auth fails - #19312

[improve][broker] ServerCnx: go to Failed state when auth fails#19312
michaeljmarshall merged 3 commits into
apache:masterfrom
michaeljmarshall:pip-97-call-authenticate-on-auth-state

michaeljmarshall commented Jan 23, 2023 •

edited

Loading

Uh oh!

lhotari commented Jan 24, 2023

Uh oh!

nicoloboschi left a comment

Uh oh!

michaeljmarshall commented Jan 24, 2023

Uh oh!

codecov-commenter commented Jan 25, 2023 •

edited

Loading

Uh oh!

michaeljmarshall commented Feb 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

michaeljmarshall commented Jan 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Matching PR in forked repository

Uh oh!

lhotari commented Jan 24, 2023

Uh oh!

nicoloboschi left a comment

Choose a reason for hiding this comment

Uh oh!

michaeljmarshall commented Jan 24, 2023

Uh oh!

codecov-commenter commented Jan 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

michaeljmarshall commented Feb 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

michaeljmarshall commented Jan 23, 2023 •

edited

Loading

codecov-commenter commented Jan 25, 2023 •

edited

Loading