You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "nodece (via GitHub)" <gi...@apache.org> on 2023/08/17 10:56:12 UTC
[GitHub] [pulsar] nodece opened a new pull request, #21013: [fix][fn] Fix using the correct admin and broker urls
nodece opened a new pull request, #21013:
URL: https://github.com/apache/pulsar/pull/21013
### Motivation
When authentication tls is enabled, the standalone cannot be running:
```
2023-08-15T15:23:26,736+0200 [main] INFO org.apache.pulsar.functions.worker.WorkerUtils - Create Pulsar Admin to service url http://localhost:8080/: authPlugin = org.apache.pulsar.client.impl.auth.AuthenticationTls, authParams = {"tlsCertFile":"/Users/alex/Desktop/pulsar/apache-pulsar-3.0.0-tls/conf/tls-certs/client.cert.pem","tlsKeyFile":"/Users/alex/Desktop/pulsar/apache-pulsar-3.0.0-tls/conf/tls-certs/client.key-pk8.pem"}, tlsTrustCerts = /Users/alex/Desktop/pulsar/apache-pulsar-3.0.0-tls/conf/tls-certs/ca.cert.pem, allowTlsInsecureConnection = false, enableTlsHostnameVerification = false
2023-08-15T15:23:26,910+0200 [pulsar-web-48-15] WARN org.apache.pulsar.broker.web.AuthenticationFilter - [127.0.0.1] Failed to authenticate HTTP request: Authentication required
2023-08-15T15:23:26,918+0200 [pulsar-web-48-15] INFO org.eclipse.jetty.server.RequestLog - 127.0.0.1 - - [15/Aug./2023:15:23:26 +0200] "PUT /admin/v2/persistent/public/functions/assignments HTTP/1.1" 401 0 "-" "Pulsar-Java-v3.0.0" 12
```
The service URL is incorrect, which should be https://localhost:8081, not http://localhost:8080.
### Modifications
- Fix check URL condition
### Verifying this change
- [x] Make sure that the change passes the CI checks.
Added a pulsar standalone with the authenticationTls test.
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
--
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] nodece commented on pull request #21013: [fix][fn] Fix using the correct admin and broker urls
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #21013:
URL: https://github.com/apache/pulsar/pull/21013#issuecomment-1683262889
/pulsarbot rerun-failure-checks
--
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] nodece commented on pull request #21013: [fix][fn] Fix using the correct admin and broker urls
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #21013:
URL: https://github.com/apache/pulsar/pull/21013#issuecomment-1729650931
/pulsarbot rerun-failure-checks
--
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 #21013: [fix][fn] Fix using the correct admin and broker urls
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #21013:
URL: https://github.com/apache/pulsar/pull/21013#issuecomment-1730920340
## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/21013?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#21013](https://app.codecov.io/gh/apache/pulsar/pull/21013?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (f31ba3c) into [master](https://app.codecov.io/gh/apache/pulsar/commit/eefc51712c97515e5913f6e5fceb4e98f5cd3b18?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (eefc517) will **increase** coverage by `36.35%`.
> Report is 4 commits behind head on master.
> The diff coverage is `16.66%`.
[](https://app.codecov.io/gh/apache/pulsar/pull/21013?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #21013 +/- ##
=============================================
+ Coverage 36.75% 73.10% +36.35%
+ Complexity 12193 3714 -8479
=============================================
Files 1698 1887 +189
Lines 130430 140118 +9688
Branches 14250 15427 +1177
=============================================
+ Hits 47940 102437 +54497
+ Misses 76167 29585 -46582
- Partials 6323 8096 +1773
```
| [Flag](https://app.codecov.io/gh/apache/pulsar/pull/21013/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [inttests](https://app.codecov.io/gh/apache/pulsar/pull/21013/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `23.01% <0.00%> (-1.13%)` | :arrow_down: |
| [systests](https://app.codecov.io/gh/apache/pulsar/pull/21013/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `24.11% <0.00%> (-0.65%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pulsar/pull/21013/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `72.49% <16.66%> (+40.60%)` | :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.
| [Files Changed](https://app.codecov.io/gh/apache/pulsar/pull/21013?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...g/apache/pulsar/functions/worker/WorkerConfig.java](https://app.codecov.io/gh/apache/pulsar/pull/21013?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWZ1bmN0aW9ucy9ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9wdWxzYXIvZnVuY3Rpb25zL3dvcmtlci9Xb3JrZXJDb25maWcuamF2YQ==) | `92.85% <ø> (+12.05%)` | :arrow_up: |
| [...e/pulsar/functions/worker/PulsarWorkerService.java](https://app.codecov.io/gh/apache/pulsar/pull/21013?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWZ1bmN0aW9ucy93b3JrZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3B1bHNhci9mdW5jdGlvbnMvd29ya2VyL1B1bHNhcldvcmtlclNlcnZpY2UuamF2YQ==) | `67.56% <ø> (+8.28%)` | :arrow_up: |
| [...rg/apache/pulsar/functions/worker/WorkerUtils.java](https://app.codecov.io/gh/apache/pulsar/pull/21013?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWZ1bmN0aW9ucy93b3JrZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3B1bHNhci9mdW5jdGlvbnMvd29ya2VyL1dvcmtlclV0aWxzLmphdmE=) | `77.38% <0.00%> (+13.88%)` | :arrow_up: |
| [...n/java/org/apache/pulsar/broker/PulsarService.java](https://app.codecov.io/gh/apache/pulsar/pull/21013?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9QdWxzYXJTZXJ2aWNlLmphdmE=) | `81.85% <20.00%> (+13.18%)` | :arrow_up: |
... and [1451 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/21013/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
Re: [PR] [fix][fn] Fix using the correct admin and broker urls [pulsar]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #21013:
URL: https://github.com/apache/pulsar/pull/21013#issuecomment-1774307196
The pr had no activity for 30 days, mark with Stale label.
--
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
Re: [PR] [fix][fn] Use TLS urls for the embedded function worker when brokerClientTlsEnabled is set [pulsar]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat merged PR #21013:
URL: https://github.com/apache/pulsar/pull/21013
--
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] Technoboy- closed pull request #21013: [fix][fn] Fix using the correct admin and broker urls
Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- closed pull request #21013: [fix][fn] Fix using the correct admin and broker urls
URL: https://github.com/apache/pulsar/pull/21013
--
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] nodece commented on pull request #21013: [fix][fn] Fix using the correct admin and broker urls
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #21013:
URL: https://github.com/apache/pulsar/pull/21013#issuecomment-1730876951
/pulsarbot rerun-failure-checks
--
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] nodece commented on pull request #21013: [fix][fn] Fix using the correct admin and broker urls
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #21013:
URL: https://github.com/apache/pulsar/pull/21013#issuecomment-1687315784
BTW, the fn can only depend on the `AuthenticationTls` to perform the TLS transport. After merging this PR, I will improve this point.
This is a TLS transport PIP from last year: https://github.com/apache/pulsar/issues/15289
--
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
Re: [PR] [fix][fn] Use TLS urls for the embedded function worker when brokerClientTlsEnabled is set [pulsar]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat commented on PR #21013:
URL: https://github.com/apache/pulsar/pull/21013#issuecomment-6355438068
Hi @nodece, I picked this up as part of the open-PR backlog triage, since the bug is still there on master: with both plaintext and TLS listeners, the embedded worker ignores `brokerClientTlsEnabled` and connects over plaintext.
I merged current master into your branch and narrowed the change to the URL selection in `PulsarService#startWorkerService`. It now uses TLS when either `useTls` or `brokerClientTlsEnabled` is set and a TLS listener exists. I dropped the rest:
- deriving the worker's `tlsEnabled` from `webServicePortTls`, which changes the `workerId` and would reshuffle function assignments on upgrade
- forcing `brokerClientAuthenticationEnabled`
- the `WorkerUtils` `useTls` changes, which aren't needed because the URL scheme already turns TLS on
- the `StandaloneContainer` / integration-test rewrite
I replaced the integration test with `PulsarFunctionTlsMixedListenersTest`, a unit test that reruns the mTLS function test with plaintext listeners enabled. Without the fix it fails at startup with the same 401 you reported.
Your commit is still in the history. Let me know if you'd like anything changed.
--
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