[Broker/Proxy] Prevent invalid broker or proxy configuration for authorization #9746
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lhotari
force-pushed
the
lh-prevent-invalid-authorization-config
branch
from
5b961b2
to
50997e3
Compare
codelipenghui
requested review from
merlimat,
codelipenghui,
jiazhai,
sijie and
zymap
|
/pulsarbot run-failure-checks |
lhotari
force-pushed
the
lh-prevent-invalid-authorization-config
branch
from
50997e3
to
e5a0811
Compare
|
/pulsarbot run-failure-checks |
lhotari
force-pushed
the
lh-prevent-invalid-authorization-config
branch
2 times, most recently
from
27247b4
to
9d08f62
Compare
|
/pulsarbot run-failure-checks |
merlimat
approved these changes
Mar 4, 2021
|
I'll check the test failures tomorrow. |
### Motivation If user set incorrect `brokerClientTlsEnabled` config, the `PulsarService#getAdminClient` would throw NPE and the error logs is not clear. For example, start a standalone pulsar with `brokerClientTlsEnabled=true`, some admin APIs that don't involve `PulsarService#getAdminClient` work well, however some admin APIs like `GET /admin/v2/non-persistent/:tenant/:namespace` will throw NPE with following logs: ``` org.apache.pulsar.broker.PulsarServerException: java.lang.NullPointerException at org.apache.pulsar.broker.PulsarService.getAdminClient(PulsarService.java:1193) at org.apache.pulsar.broker.admin.v2.NonPersistentTopics.getList(NonPersistentTopics.java:273) ``` After this PR, the logs became: ``` org.apache.pulsar.broker.PulsarServerException: java.lang.IllegalArgumentException: adminApiUrl is null, isBrokerClientTlsEnabled: true, webServiceAddressTls: null, webServiceAddress: http://localhost:8080 ``` ### Modifications - Check if `adminApiUrl` is null in `PulsarService#getAdminClient` and give a human readable error message. ### Verifying this change - [ ] Make sure that the change passes the CI checks. This change is a trivial rework / code cleanup without any test coverage.
lhotari
force-pushed
the
lh-prevent-invalid-authorization-config
branch
from
a8b0929
to
70ccd3d
Compare
|
/pulsarbot run-failure-checks |
1 similar comment
|
/pulsarbot run-failure-checks |
mlyahmed
pushed a commit
to mlyahmed/pulsar
that referenced
this pull request
Mar 7, 2021
…9746) ### Motivation If user set incorrect `brokerClientTlsEnabled` config, the `PulsarService#getAdminClient` would throw NPE and the error logs is not clear. For example, start a standalone pulsar with `brokerClientTlsEnabled=true`, some admin APIs that don't involve `PulsarService#getAdminClient` work well, however some admin APIs like `GET /admin/v2/non-persistent/:tenant/:namespace` will throw NPE with following logs: ``` org.apache.pulsar.broker.PulsarServerException: java.lang.NullPointerException at org.apache.pulsar.broker.PulsarService.getAdminClient(PulsarService.java:1193) at org.apache.pulsar.broker.admin.v2.NonPersistentTopics.getList(NonPersistentTopics.java:273) ``` After this PR, the logs became: ``` org.apache.pulsar.broker.PulsarServerException: java.lang.IllegalArgumentException: adminApiUrl is null, isBrokerClientTlsEnabled: true, webServiceAddressTls: null, webServiceAddress: http://localhost:8080 ``` ### Modifications - Check if `adminApiUrl` is null in `PulsarService#getAdminClient` and give a human readable error message. ### Verifying this change - [ ] Make sure that the change passes the CI checks. This change is a trivial rework / code cleanup without any test coverage.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #9709
Motivation
Enabling authorization without enabling authentication leads to inconsistent behavior.
In some parts of the code, authorization is enforced only when both authorization is enabled and authentication is enabled. In a few places, it's enforced if authorization is enabled without checking for authentication.
To prevent misleading behavior, it's better to check at startup time that authentication is enabled when authorization is enabled.
There's also changes to make Pulsar configuration more consistent in tests by resetting the configuration in the cleanup method.
Modifications
About the fix to flaky test AuthorizationTest.simple
This change fixes the flaky test #9709, but it remains unclear why the test was passing in some cases.
It seems that the test is picking up state from some other test run. There are interesting observations in PR #9740 about the reasons why it passes in some cases.
Some effort was put into checking if the usage of Jersey / JAX RS
@Contextannotation on the PulsarWebResource's httpRequest field could be causing some concurrency issue, but that verification didn't reveal issues.