Skip to content

Conversation

@michaeljmarshall
Copy link
Member

Fixes #19431

Motivation

The originalAuthState and the authState are not thread safe members of the ServerCnx class. As such, we should only access them on the context's event loop thread.

Modifications

  • Replace scheduled task to check all connections for authentication refresh with a scheduled task per ServerCnx that will run on the appropriate event loop. This logic could theoretically be optimized to minimize scheduled tasks. However, I am following the keep alive implementation and making the assumption that the Netty scheduled tasks are not too expensive.
  • Slightly refactor conditional logic in refreshAuthenticationCredentials method to remove the logic that it only short circuits when the ServerCnx is in Failed state. We should never call this method when the isActive flag has gone to false, and the only case when the state isn't Connected is when we recently failed due to an exception.

Verifying this change

Added new test and make sure existing tests pass. Also, I added some new assert lines. These only run when the correct JVM arg is passed.

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

This is not a breaking change.

Documentation

  • doc-not-needed

This is an internal improvement.

Matching PR in forked repository

PR in forked repository: michaeljmarshall#28

@michaeljmarshall michaeljmarshall added type/bug The PR fixed a bug or issue reported a bug area/broker doc-not-needed Your PR changes do not impact docs release/2.8.5 release/2.11.1 release/2.9.5 release/2.10.4 labels Feb 13, 2023
@michaeljmarshall michaeljmarshall added this to the 3.0.0 milestone Feb 13, 2023
@michaeljmarshall michaeljmarshall self-assigned this Feb 13, 2023
@michaeljmarshall michaeljmarshall changed the title [fix][broker] Make authentication refresh threadsafe [fix][broker] Make authentication refresh threadsafe Feb 13, 2023
@michaeljmarshall
Copy link
Member Author

/pulsarbot rerun-failure-checks

@codecov-commenter
Copy link

codecov-commenter commented Feb 14, 2023

Codecov Report

Merging #19506 (2d408cd) into master (950ff44) will decrease coverage by 0.47%.
The diff coverage is 37.22%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #19506      +/-   ##
============================================
- Coverage     63.73%   63.27%   -0.47%     
- Complexity    26234    26239       +5     
============================================
  Files          1843     1844       +1     
  Lines        135158   135238      +80     
  Branches      14856    14871      +15     
============================================
- Hits          86149    85575     -574     
- Misses        41176    41897     +721     
+ Partials       7833     7766      -67     
Flag Coverage Δ
inttests 24.69% <9.44%> (+0.02%) ⬆️
systests 25.36% <17.22%> (+<0.01%) ⬆️
unittests 60.60% <37.22%> (-0.56%) ⬇️

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

Impacted Files Coverage Δ
...pulsar/broker/admin/impl/PersistentTopicsBase.java 59.67% <ø> (+0.03%) ⬆️
...dbalance/extensions/ExtensibleLoadManagerImpl.java 4.93% <0.00%> (-0.10%) ⬇️
...dbalance/extensions/scheduler/UnloadScheduler.java 0.00% <0.00%> (ø)
...ulsar/broker/service/PulsarChannelInitializer.java 95.00% <ø> (+1.57%) ⬆️
...va/org/apache/pulsar/broker/service/ServerCnx.java 55.02% <53.48%> (-0.55%) ⬇️
...sar/broker/authorization/AuthorizationService.java 33.51% <56.52%> (-1.55%) ⬇️
...ervice/AbstractDispatcherSingleActiveConsumer.java 82.05% <96.29%> (+1.67%) ⬆️
...rg/apache/pulsar/broker/web/PulsarWebResource.java 58.39% <100.00%> (-1.04%) ⬇️
...lsar/client/impl/auth/AuthenticationDataBasic.java 0.00% <0.00%> (-100.00%) ⬇️
...ar/broker/stats/prometheus/ManagedLedgerStats.java 0.00% <0.00%> (-100.00%) ⬇️
... and 217 more

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

Great work @michaeljmarshall

michaeljmarshall added a commit to michaeljmarshall/pulsar that referenced this pull request Feb 15, 2023
Co-authored-by: Lari Hotari <lhotari@users.noreply.github.com>
(cherry picked from commit 153e4d4)
michaeljmarshall added a commit that referenced this pull request Feb 22, 2023
Co-authored-by: Lari Hotari <lhotari@users.noreply.github.com>
(cherry picked from commit 153e4d4)
(cherry picked from commit 161ec5a)
michaeljmarshall added a commit to michaeljmarshall/pulsar that referenced this pull request Feb 22, 2023
Co-authored-by: Lari Hotari <lhotari@users.noreply.github.com>
(cherry picked from commit 153e4d4)
(cherry picked from commit 161ec5a)
(cherry picked from commit 26e1053)
michaeljmarshall added a commit to datastax/pulsar that referenced this pull request Feb 23, 2023
Co-authored-by: Lari Hotari <lhotari@users.noreply.github.com>
(cherry picked from commit 153e4d4)
(cherry picked from commit 161ec5a)
(cherry picked from commit 26e1053)
michaeljmarshall added a commit to michaeljmarshall/pulsar that referenced this pull request Feb 23, 2023
Co-authored-by: Lari Hotari <lhotari@users.noreply.github.com>
(cherry picked from commit 153e4d4)
(cherry picked from commit 161ec5a)
(cherry picked from commit 26e1053)
(cherry picked from commit 336dcbb)
michaeljmarshall added a commit to michaeljmarshall/pulsar that referenced this pull request Feb 23, 2023
Co-authored-by: Lari Hotari <lhotari@users.noreply.github.com>
(cherry picked from commit 153e4d4)
(cherry picked from commit 161ec5a)
(cherry picked from commit 26e1053)
(cherry picked from commit 336dcbb)
hangc0276 added a commit to hangc0276/pulsar that referenced this pull request Mar 15, 2023
hangc0276 added a commit to hangc0276/pulsar that referenced this pull request Mar 15, 2023
@michaeljmarshall michaeljmarshall deleted the make-auth-refresh-thread-safe branch April 20, 2023 05:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Address thread safety concerns in ServerCnx for auth related data

5 participants