Fix/function/update auth data #21
Open
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
15 tasks
|
@labuladong Please add the following content to your PR description and select a checkbox: |
|
The pr had no activity for 30 days, mark with Stale label. |
…e#19913) After apache#13833, we change the signature of LedgerOffloaderFactory#create. But some users may use the old version LedgerOffloaderFactory in the offloader nar, when pulsar load the offloader nar, it will use the LedgerOffloaderFactory in the offloader nar, the LedgerOffloaderFactory#create has no param offloaderStats. Modifications: Make LedgerOffloaderFactory can load the old nar.
…ache#20046) ### Motivation In apache#19975, we introduced a wrapper for all authentication parameters. This PR adds that wrapper to the Rest Producer. ### Modifications * Use `AuthenticationParameters` to simplify parameter management in Rest Producer. * Add method to the `AuthorizationService` that takes the `AuthenticationParameters`. * Update annotations on Rest Producer to indicate that a 401 is an expected response. ### Verifying this change This change is covered by the `TopicsAuthTest`. ### Documentation - [x] `doc-not-needed` This is an internal change that does not need to be documented. ### Matching PR in forked repository PR in forked repository: skipping PR since the relevant tests pass locally
…apache#19605) Co-authored-by: Vineeth <[email protected]>
…umers when TCP/IP connections get orphaned (apache#20026)
PIP: apache#19771 ### Motivation This is the primary PR for PIP 257 (apache#19771). It adds an OpenID Conenct `AuthenticationProvider` implementation. The implementation is intended to be compliant with the OpenID Specs defined here: https://openid.net/developers/specs/. We specifically implement the discovery and these two: > * [OpenID Connect Core](https://openid.net/specs/openid-connect-core-1_0.html) – Defines the core OpenID Connect functionality: authentication built on top of OAuth 2.0 and the use of claims to communicate information about the End-User > * [OpenID Connect Discovery](https://openid.net/specs/openid-connect-discovery-1_0.html) – Defines how clients dynamically discover information about OpenID Providers ### Modifications * Add new module `pulsar-broker-auth-oidc` * Add implementation that relies on auth0 client libraries to verify the signature and claims of the JWT * Use async http client for all http requests * Cache the provider metadata and the JWKS results * Support different types of `FallbackDiscoveryMode`s, as documented in the code. Essentially, this setting allows users to more easily integrate with k8s. We need this coupling with kubernetes to deal with some of the nuances of the k8s implementation. Note that this part of the code is experimental and is subject to change as requirements and cloud provider implementations change. One important reason we use the k8s client is because the API Server requires special configuration for authentication and TLS. Since these do not appear to be generic requirements, the K8s client simplifies this integration. Here is a reference to the decision that requires authentication by default for getting OIDC info kubernetes/kubernetes#80724. That discussion also indicate to me that this is an isolated design decision in k8s. If we find that authentication is a generic requirement, we should easily be able to expand the existing feature at a later time. * Add metrics to help quantify success and failure. (I had thought I would add audit logging, but that is an independent feature that we can add to the Pulsar framework. It seems outside the scope of an Authentication Provider implementation to implement this feature.) ### Verifying this change There are many new tests to cover this new implementation. Some of the tests are unit tests while others are integration tests that rely on Wire Mock to return the public key information. ### Documentation - [x] `doc-required` This feature will need new docs. ### Matching PR in forked repository PR in forked repository: michaeljmarshall#35
### Motivation With BK 4.16, we don't need to pass `SafeRunnable` instances to the `OrderedExecutor` anymore. The executor has embedded the logic of checking and logging exceptions. Removing the SafeRun will avoid extra allocations in critical path and clutter of the code ### Verifying this change - [ ] Make sure that the change passes the CI checks. *(Please pick either of the following options)* This change is a trivial rework / code cleanup without any test coverage. *(or)* This change is already covered by existing tests, such as *(please describe tests)*. *(or)* This change added tests and can be verified as follows: *(example:)* - *Added integration tests for end-to-end deployment with large payloads (10MB)* - *Extended integration test for recovery after broker failure* ### Does this pull request potentially affect one of the following parts: <!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. --> *If the box was checked, please highlight the changes* - [ ] Dependencies (add or upgrade a dependency) - [ ] The public API - [ ] The schema - [ ] The default values of configurations - [ ] The threading model - [ ] The binary protocol - [ ] The REST endpoints - [ ] The admin CLI options - [ ] The metrics - [ ] Anything that affects deployment ### 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 --> ### Matching PR in forked repository PR in forked repository: (merlimat#6) <!-- After opening this PR, the build in apache/pulsar will fail and instructions will be provided for opening a PR in the PR author's forked repository. apache/pulsar pull requests should be first tested in your own fork since the apache/pulsar CI based on GitHub Actions has constrained resources and quota. GitHub Actions provides separate quota for pull requests that are executed in a forked repository. The tests will be run in the forked repository until all PR review comments have been handled, the tests pass and the PR is approved by a reviewer. -->
Co-authored-by: Baodi Shi <[email protected]>
Motivation: PulsarClient using ExecutorService as ScheduledExecutorService Modifications: Use exactly ScheduledExecutorService
…e#19514) Motivation: If automatic topic creation is enabled, producers and consumers can create topics with long names, but once created, such topics cannot be managed by the Admin API, it will respond "414 URI is too large". Modifications: add a config `pulsar.conf.httpMaxRequestHeaderSize` to make the jetty can accept the request with a longer Uri
### Motivation After PR apache#8611, the acquiring permits can be larger than configured msg-rate if used by subscribing. But doc was not updated in time. ### Modifications fix the outdated doc
… for large clusters (apache#20059)
…ache#19304) Co-authored-by: tison <[email protected]>
…ersistentTopic (apache#19737) part-1 of PIP-240: add a new method unloadSubscription( String subName ) for PersistentTopic
…ls (apache#20031) ### Motivation After apache#15868, we allow `PULSAR_MEM` & `PULSAR_GC` to be overridden in `pulsar_tool_env.sh`. Many users set `-Xms` to `2G` or larger in `PULSAR_MEM`, this will make the tools(such as `pulsar-admin`) cost a lot of memory, and if users execute `pulsar-admin` or another tool on the machine where the Broker is deployed, the current device will not have enough memory to allocate, resulting in a broker crash. ### Modifications When `PULSAR_MEM` is overridden in `pulsar_tool_env.sh`, delete parameter `-Xms`
PIP apache#19623 Relates to apache#19540 ### Motivation In order to get more information about connections, it is helpful for the proxy to supply its version to the broker. ### Modifications * Add `proxy_version` field to the `CommandConnect` protobuf message * Update proxy and broker to handle this new field ### Verifying this change New tests are added with this PR. ### Does this pull request potentially affect one of the following parts: - [x] The binary protocol This will be submitted as part of a PIP. ### Documentation - [x] `doc-not-needed`
Fixes: apache#10816 PIP: apache#19771 Supersedes: apache#19026 Depends on: apache#20062 ### Motivation The Pulsar Proxy does not properly handle authentication data refresh when in state `ProxyLookupRequests`. The consequence is described in apache#10816. Essentially, the problem is that the proxy caches stale authentication data and sends it to the broker leading to connection failures. apache#17831 attempted to fix the underlying problem, but it missed an important edge cases. Specifically, it missed the case that the `ConnectionPool` will have multiple connections when a lookup gets redirected. As such, the following problem exists (and is fixed by this PR): 1. Client opens connection to perform lookups. 2. Proxy connects to broker 1 to get the topic ownership info. 3. Time passes. 4. Client does an additional lookup, and this topic is on a newly created broker 2. In this case, the proxy opens a new connection with the stale client auth data. 5. Broker 2 rejects the connection because it fails with expired authentication. ### Modifications * Remove some of the implementation from apache#17831. This new implementation still allows a broker to challenge the client through the proxy, but notably, it limits the number of challenges sent to the client. Further, the proxy does not challenge the client when the auth data is not expired. * Introduce authentication refresh in the proxy so that the proxy challenges the client any time the auth data is expired. * Update the `ProxyClientCnx` to get the `clientAuthData` from the `ProxyConnection` to ensure that it gets new authentication data. * Add clock skew to the `AuthenticationProviderToken`. This is necessary to make some of the testing not flaky and it will also be necessary for users to configure in their clusters. ### Verifying this change The `ProxyRefreshAuthTest` covers the existing behavior and I expanded it to cover the edge case described above. Additionally, testing this part of the code will be much easier to test once we implement apache#19624. ### Documentation - [x] `doc-not-needed` ### Matching PR in forked repository PR in forked repository: the relevant tests pass locally, so I am going to skip the forked tests.
apache#20070) ### Motivation BK deployed with Pulsar fail on start with ``` ERROR org.apache.bookkeeper.common.component.AbstractLifecycleComponent - Failed to start Component: http-service java.lang.NoSuchMethodError: 'io.vertx.core.Future io.vertx.core.Vertx.deployVerticle(io.vertx.core.Verticle)' at org.apache.bookkeeper.http.vertx.VertxHttpServer.startServer(VertxHttpServer.java:93) ~[org.apache.bookkeeper.http-vertx-http-server-4.16.0.jar:4.16.0] at org.apache.bookkeeper.server.service.HttpService.doStart(HttpService.java:62) ~[org.apache.bookkeeper-bookkeeper-server-4.16.0.jar:4.16.0] at org.apache.bookkeeper.common.component.AbstractLifecycleComponent.start(AbstractLifecycleComponent.java:83) ~[org.apache.bookkeeper-bookkeeper-common-4.16.0.jar:4.16.0] at org.apache.bookkeeper.common.component.LifecycleComponentStack.lambda$start$4(LifecycleComponentStack.java:144) ~[org.apache.bookkeeper-bookkeeper-common-4.16.0.jar:4.16.0] at com.google.common.collect.ImmutableList.forEach(ImmutableList.java:422) ~[com.google.guava-guava-31.0.1-jre.jar:?] at org.apache.bookkeeper.common.component.LifecycleComponentStack.start(LifecycleComponentStack.java:144) ~[org.apache.bookkeeper-bookkeeper-common-4.16.0.jar:4.16.0] at org.apache.bookkeeper.common.component.ComponentStarter.startComponent(ComponentStarter.java:84) ~[org.apache.bookkeeper-bookkeeper-common-4.16.0.jar:4.16.0] at org.apache.bookkeeper.server.Main.doMain(Main.java:224) ~[org.apache.bookkeeper-bookkeeper-server-4.16.0.jar:4.16.0] at org.apache.bookkeeper.server.Main.main(Main.java:199) ~[org.apache.bookkeeper-bookkeeper-server-4.16.0.jar:4.16.0] ``` The root cause is that BK upgraded to 4.16.0 and needs newer version of vertx that introduced API changes apache/bookkeeper#3435 Pulsar forces older version of dependency. ### Modifications Upgrade Vertx to the same version as BK
…apache#19929) Co-authored-by: wangjinlong <[email protected]>
### Motivation Upgrade Ubuntu to 22.04 to fix some CVEs - CVE-2017-14063: https://nvd.nist.gov/vuln/detail/cve-2017-14063 - CVE-2021-27291: https://nvd.nist.gov/vuln/detail/CVE-2021-27291 - CVE-2021-20270: https://nvd.nist.gov/vuln/detail/CVE-2021-20270
Co-authored-by: tison <[email protected]>
apache#20435) Signed-off-by: Zike Yang <[email protected]> Co-authored-by: Lari Hotari <[email protected]>
### Motivation apache#20482 broke the function download command with this error: ``` Expected a command, got false Usage: pulsar-admin [options] [command] [command options] Options: --admin-url Admin Service URL to which to connect. Default: http://localhost:8080/ ``` The problem is that the TLS args for hostname verification and for insecure TLS are zero arity, and therefore, we should not add the `false` or the `true` arguments. ### Modifications * Correct the changes made to the TLS args passed to the `pulsar-admin` CLI tool ### Documentation - [x] `doc-not-needed`
…code `1 (failed)` instead of `0 (success)` (apache#20503)
…list in bundle admin API (apache#20528) PIP: apache#16691 ### Motivation When using `ExtensibleLoadManager` and list in bundle admin API, it will redirect forever because `isServiceUnitOwned` method is checking the `ownershipCache` as the ownership storage, however, when using `ExtensibleLoadManager`, it stored the ownership to table view. ### Modifications * Call `isServiceUnitOwnedAsync ` when using `isServiceUnitOwned `. * Add unit test to cover this case.
…y exists. (apache#20539) ## Motivation Make the Exception more clear. ## Modification Return the `409 Conflict` instead of `404 NoFound` when metadata already exists.
Co-authored-by: Matt Anderson <> Fixes apache#20536 ### Motivation When disabling HTTP ports in the Pulsar broker, the [REST Client Producer](https://pulsar.apache.org/docs/3.0.x/client-libraries-rest/) fails to produce messages. For reproduction steps, please reference issue details. ### Modifications Change the following lines: ```java LookupResult result = optionalResult.get(); if (result.getLookupData().getHttpUrl().equals(pulsar().getWebServiceAddress())) { // Current broker owns the topic, add to owning topic. ``` To: ```java LookupResult result = optionalResult.get(); if (result.getLookupData().getHttpUrl().equals(pulsar().getWebServiceAddress()) || result.getLookupData().getHttpUrlTls().equals(pulsar().getWebServiceAddressTls())) { // Current broker owns the topic, add to owning topic. ``` ### Verifying this change This change is a trivial rework / code cleanup without any test coverage. (outside of the reproduction tests described in the apache#20536) <!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. --> *If the box was checked, please highlight the changes* - [ ] Dependencies (add or upgrade a dependency) - [ ] The public API - [ ] The schema - [ ] The default values of configurations - [ ] The threading model - [ ] The binary protocol - [x] The REST endpoints - [ ] The admin CLI options - [ ] The metrics - [ ] Anything that affects deployment ### 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 --> ### Matching PR in forked repository PR in forked repository: https://github.com/maanders-tibco/pulsar <!-- After opening this PR, the build in apache/pulsar will fail and instructions will be provided for opening a PR in the PR author's forked repository. apache/pulsar pull requests should be first tested in your own fork since the apache/pulsar CI based on GitHub Actions has constrained resources and quota. GitHub Actions provides separate quota for pull requests that are executed in a forked repository. The tests will be run in the forked repository until all PR review comments have been handled, the tests pass and the PR is approved by a reviewer. -->
…apache#20549) ### Motivation Move the authorization check a few steps earlier in the delete subscription admin endpoint. ### Modifications * Move the authz check earlier ### Verifying this change We do not have any tests for these endpoints. We should add them. This change is trivial enough that I think it is fine to defer on testing the authz change. ### Documentation - [x] `doc-not-needed`
…DispatcherSingleActiveConsumer (apache#20522) ### Motivation Currently, all subscriptions of one topic will do `consuemrFlow` action in a single thread, which is chosen by topicName: ``` this.topicExecutor = topic.getBrokerService().getTopicOrderedExecutor().chooseThread(topicName); ``` If there is a large number of subscriptions in a topic, all the work will focus on one thread ---- the chosen thread, which will reduce the consume performance. So this this patch , I'd like to choose a ramdom thread for `consumerFlow` in `PersistentDispatcherSingleActiveConsumer` to improve the consume performance. ### Modifications * `topic.getBrokerService().getTopicOrderedExecutor().chooseThread(topicName);` -> `topic.getBrokerService().getTopicOrderedExecutor().chooseThread();` * `this.topicExecutor ` -> `this.executor`
…ssages. (apache#19775) Co-authored-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
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 #xyz
Master Issue: #xyz
PIP: #xyz
Motivation
Modifications
Verifying this change
(Please pick either of the following options)
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: