You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "michaeljmarshall (via GitHub)" <gi...@apache.org> on 2023/05/18 05:27:30 UTC
[GitHub] [pulsar] michaeljmarshall opened a new pull request, #20348: [fix][test] ProxyWithoutServiceDiscoveryTest should enable authz
michaeljmarshall opened a new pull request, #20348:
URL: https://github.com/apache/pulsar/pull/20348
### Motivation
The `ProxyWithoutServiceDiscoveryTest` is documented to verify `e2e tls + Authentication + Authorization (client -> proxy -> broker)`. However, the TLS is insecure and authorization is disabled. This PR cleans up that test.
### Modifications
* Enable authorization in the broker config.
* Replace the certs with ones provided by the parent class.
* Add `proxyRoles` so authorization will pass.
* Update the `superUserRoles` because the different certs have different CNs.
### Verifying this change
The test passes locally and the logs look right.
### Documentation
- [x] `doc-not-needed`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on a diff in pull request #20348: [fix][test] ProxyWithoutServiceDiscoveryTest should enable authz
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #20348:
URL: https://github.com/apache/pulsar/pull/20348#discussion_r1197486595
##########
pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyWithoutServiceDiscoveryTest.java:
##########
@@ -198,10 +196,10 @@ public void testDiscoveryService() throws Exception {
}
protected final PulsarClient createPulsarClient(Authentication auth, String lookupUrl) throws Exception {
- admin = spy(PulsarAdmin.builder().serviceHttpUrl(brokerUrlTls.toString()).tlsTrustCertsFilePath(TLS_TRUST_CERT_FILE_PATH)
- .allowTlsInsecureConnection(true).authentication(auth).build());
Review Comment:
Why remove `allowTlsInsecureConnection(true)`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codecov-commenter commented on pull request #20348: [fix][test] ProxyWithoutServiceDiscoveryTest should enable authz
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #20348:
URL: https://github.com/apache/pulsar/pull/20348#issuecomment-1553110942
## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/20348?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#20348](https://app.codecov.io/gh/apache/pulsar/pull/20348?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (7077502) into [master](https://app.codecov.io/gh/apache/pulsar/commit/1a66b640c3cd86bfca75dc9ab37bfdb37427a13f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (1a66b64) will **increase** coverage by `38.34%`.
> The diff coverage is `68.75%`.
[](https://app.codecov.io/gh/apache/pulsar/pull/20348?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #20348 +/- ##
=============================================
+ Coverage 34.55% 72.90% +38.34%
- Complexity 12631 31828 +19197
=============================================
Files 1614 1864 +250
Lines 126234 138281 +12047
Branches 13779 15172 +1393
=============================================
+ Hits 43622 100812 +57190
+ Misses 76999 29455 -47544
- Partials 5613 8014 +2401
```
| Flag | Coverage Δ | |
|---|---|---|
| inttests | `24.22% <15.62%> (+0.10%)` | :arrow_up: |
| systests | `24.92% <15.62%> (?)` | |
| unittests | `72.18% <68.75%> (+38.88%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/pulsar/pull/20348?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...hentication/oidc/AuthenticationProviderOpenID.java](https://app.codecov.io/gh/apache/pulsar/pull/20348?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci1hdXRoLW9pZGMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3B1bHNhci9icm9rZXIvYXV0aGVudGljYXRpb24vb2lkYy9BdXRoZW50aWNhdGlvblByb3ZpZGVyT3BlbklELmphdmE=) | `75.86% <ø> (+75.86%)` | :arrow_up: |
| [...org/apache/pulsar/broker/ServiceConfiguration.java](https://app.codecov.io/gh/apache/pulsar/pull/20348?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3B1bHNhci9icm9rZXIvU2VydmljZUNvbmZpZ3VyYXRpb24uamF2YQ==) | `99.37% <ø> (+1.45%)` | :arrow_up: |
| [...apache/pulsar/broker/namespace/OwnershipCache.java](https://app.codecov.io/gh/apache/pulsar/pull/20348?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9uYW1lc3BhY2UvT3duZXJzaGlwQ2FjaGUuamF2YQ==) | `85.26% <ø> (+12.63%)` | :arrow_up: |
| [...ache/pulsar/broker/namespace/NamespaceService.java](https://app.codecov.io/gh/apache/pulsar/pull/20348?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9uYW1lc3BhY2UvTmFtZXNwYWNlU2VydmljZS5qYXZh) | `69.93% <20.00%> (+26.29%)` | :arrow_up: |
| [...pache/pulsar/broker/admin/impl/NamespacesBase.java](https://app.codecov.io/gh/apache/pulsar/pull/20348?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9hZG1pbi9pbXBsL05hbWVzcGFjZXNCYXNlLmphdmE=) | `71.76% <66.66%> (+45.65%)` | :arrow_up: |
| [...e/pulsar/broker/authentication/oidc/JwksCache.java](https://app.codecov.io/gh/apache/pulsar/pull/20348?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci1hdXRoLW9pZGMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3B1bHNhci9icm9rZXIvYXV0aGVudGljYXRpb24vb2lkYy9Kd2tzQ2FjaGUuamF2YQ==) | `66.66% <93.75%> (+66.66%)` | :arrow_up: |
| [...ker/service/persistent/PersistentSubscription.java](https://app.codecov.io/gh/apache/pulsar/pull/20348?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL3BlcnNpc3RlbnQvUGVyc2lzdGVudFN1YnNjcmlwdGlvbi5qYXZh) | `75.91% <100.00%> (+25.83%)` | :arrow_up: |
... and [1503 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/20348/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #20348: [fix][test] ProxyWithoutServiceDiscoveryTest should enable authz
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on code in PR #20348:
URL: https://github.com/apache/pulsar/pull/20348#discussion_r1197827247
##########
pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyWithoutServiceDiscoveryTest.java:
##########
@@ -198,10 +196,10 @@ public void testDiscoveryService() throws Exception {
}
protected final PulsarClient createPulsarClient(Authentication auth, String lookupUrl) throws Exception {
- admin = spy(PulsarAdmin.builder().serviceHttpUrl(brokerUrlTls.toString()).tlsTrustCertsFilePath(TLS_TRUST_CERT_FILE_PATH)
- .allowTlsInsecureConnection(true).authentication(auth).build());
Review Comment:
The test is documented as verifying end to end TLS, but this setting disables verification of the TLS certificate, which defeats the purpose of testing TLS.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari merged pull request #20348: [fix][test] ProxyWithoutServiceDiscoveryTest should enable authz
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari merged PR #20348:
URL: https://github.com/apache/pulsar/pull/20348
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org