Configure DLog Bookie, Pulsar, and Admin clients via pass through config #15818

michaeljmarshall · 2022-05-27T16:25:42Z

Motivation

The Pulsar broker, proxy, websocket proxy, and function worker each start several clients, including the Pulsar Client, the Pulsar Admin Client, and the Bookkeeper Client. Until now, there hasn't been an easy way to pass arbitrary configuration to these clients, which can cause problems. A recent example is the client memory limit that we fixed here: #15752. I propose that we enable pass through configuration for these clients, so that users can override any default configuration.

Enable pass through configuration support for the bookkeeper clients created by DistributedLog and BookKeeperPackagesStorage. This support is already present for the managed ledger bookkeeper client, which takes all bookkeeper_ prefixed configs. In order to simplify configuration, I reused the bookkeeper_ prefix since it's reasonable to assume that these configs will be the same.

Enable pass through configuration support for the broker clients using the brokerClient_ prefix. Note that setting this once affects all clients. I think this is ideal, but it could lead to some confusion. A good secondary feature might be to add another prefix that targets specific clients, like the replication client.

Initially implemented as #15763, but instead of submitting many PRs, I am just submitting one larger one.

Modifications

Update all Broker, Proxy, WebSocket Proxy, and Function Worker clients so they can take arbitrary config. The Pulsar Client and the Pulsar Admin Client take configuration starting with brokerClient_. The Bookkeeper Client and the DLog Bookeeper Client take configuration starting with bookkeeper_.
Create PropertiesUtils class to reduce code duplication for mapping prefixed properties to the Map<String, Object>
Add PulsarAdminBuilder#loadConf method to the interface to match the same method in the Pulsar Client builder.
Update the BookKeeperPackagesStorageConfiguration to expose the underlying properties object.
Add documentation to the broker.conf, the proxy.conf, the websocket.conf and the functions_worker.yml files.

Verifying this change

I added several tests and also manually verified that the configuration overrides applied.

Alternative Implementation

I had initially wanted to run the override logic at the end of all other configuration. However, it doesn't work correctly because of the issue highlighted here #8509. It might be helpful to change the implementation of the loadConf logic so that it doesn't serialize secrets as ****.

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

This is a backwards compatible change. The only nuance is that the package management dlog bookie client will inherit the same configs that the broker already uses for the bookie client. I think this is preferable.

dave2wave

LGTM - nicely done!

eolivelli

Great stuff

eolivelli · 2022-05-27T17:02:00Z

...ient-admin/src/main/java/org/apache/pulsar/client/admin/internal/PulsarAdminBuilderImpl.java

    }

+    @Override
+    public PulsarAdminBuilder loadConf(Map<String, Object> config) {


this is great ! I have seen a few issues requesting this feature !

lhotari

LGTM

…fig (#15818) (cherry picked from commit aa67349)

…fig (apache#15818) (cherry picked from commit aa67349) (cherry picked from commit d144f1e)

…fig (apache#15818) (cherry picked from commit aa67349) (cherry picked from commit d384076)

…fig (#15818) (cherry picked from commit aa67349)

momo-jun · 2022-07-06T01:43:53Z

Hi @michaeljmarshall , thanks for updating the docs. I have a quick question regarding the label "release/2.10.1".

The doc changes you made in this PR are not shown in 2.10/2.10.1 documentation, but only in the latest (master). Should they be synced to 2.10/2.10.1 doc or it's just mislabeled?

BTW, there is something wrong with the table formatting.

//cc @Anonymitaet

michaeljmarshall · 2022-07-06T02:16:14Z

@momo-jun - here is a PR adding docs to 2.10.1: #16410. Note that I couldn't have added the docs to 2.10.1 when the PR was added because those docs were not yet created. Similarly, there are not 2.8.4 docs or 2.9.3 docs.

momo-jun · 2022-07-06T03:00:04Z

@michaeljmarshall thanks for the clarification.
Maybe we need to set up a sub-process to make sure these doc changes can be synced as soon as the docs for a new release are created, and can be backward-synced to versioned docs like 2.8.4 and 2.9.3 in this case. So they won't be missed.

michaeljmarshall · 2022-07-06T03:05:48Z

@momo-jun - sure, I agree we could improve the process. This partially comes down to how we version docs. We could use some kind of -SNAPSHOT doc directory for each release branch, and then when releases are performed, we copy those docs to the next release version. It could be something different, too. The main nuance is that we shouldn't make future versions of the docs available on the website because that would create confusion.

…#15818 to 2.8.x docs

…fig (apache#15818) (cherry picked from commit aa67349) (cherry picked from commit e5d035a)

…19674) ### Motivation When we merged #15818 in order to make the broker's client configurable, we did not add an explicit config for hostname verification. This PR adds that config to the broker and the websocket proxy. I chose the name `tlsHostnameVerificationEnabled` because that is what is already used in the proxy. It diverges from the function worker's config of `tlsEnableHostnameVerification`. Before this PR, you would have enabled hostname verification by configuring `brokerClient_tlsHostnameVerificationEnable=true` in the broker and WS proxy configs. (Note that the variable name is slightly different because the `ClientConfiguration` does not have a `d` at the end of its name. The remaining follow up work will be to update the `ClusterData` objects to configure hostname verification there to make it easier to configure hostname verification for remote clusters. ### Modifications * Add `tlsHostnameVerificationEnabled` to the `broker.conf` and the `proxy.conf` * Update all of the relevant locations that were previously only relying on `brokerClient_tlsHostnameVerificationEnable` ### Verifying this change I added a single test to ensure that the `WebSocketProxyConfiguration` properly converts to the `ServiceConfiguration` object. Otherwise, I verified that anywhere we are using `"brokerClient_"`, this PR also adds the right configuration. ### Does this pull request potentially affect one of the following parts: This PR introduces a "new" configuration key, but not a new concept. All underlying behaviors are unchanged. ### Documentation - [x] `doc-not-needed` Docs are automatically updated by these changes.

…19674) When we merged #15818 in order to make the broker's client configurable, we did not add an explicit config for hostname verification. This PR adds that config to the broker and the websocket proxy. I chose the name `tlsHostnameVerificationEnabled` because that is what is already used in the proxy. It diverges from the function worker's config of `tlsEnableHostnameVerification`. Before this PR, you would have enabled hostname verification by configuring `brokerClient_tlsHostnameVerificationEnable=true` in the broker and WS proxy configs. (Note that the variable name is slightly different because the `ClientConfiguration` does not have a `d` at the end of its name. The remaining follow up work will be to update the `ClusterData` objects to configure hostname verification there to make it easier to configure hostname verification for remote clusters. * Add `tlsHostnameVerificationEnabled` to the `broker.conf` and the `proxy.conf` * Update all of the relevant locations that were previously only relying on `brokerClient_tlsHostnameVerificationEnable` I added a single test to ensure that the `WebSocketProxyConfiguration` properly converts to the `ServiceConfiguration` object. Otherwise, I verified that anywhere we are using `"brokerClient_"`, this PR also adds the right configuration. This PR introduces a "new" configuration key, but not a new concept. All underlying behaviors are unchanged. - [x] `doc-not-needed` Docs are automatically updated by these changes. (cherry picked from commit 6621fd3)

…19674) When we merged #15818 in order to make the broker's client configurable, we did not add an explicit config for hostname verification. This PR adds that config to the broker and the websocket proxy. I chose the name `tlsHostnameVerificationEnabled` because that is what is already used in the proxy. It diverges from the function worker's config of `tlsEnableHostnameVerification`. Before this PR, you would have enabled hostname verification by configuring `brokerClient_tlsHostnameVerificationEnable=true` in the broker and WS proxy configs. (Note that the variable name is slightly different because the `ClientConfiguration` does not have a `d` at the end of its name. The remaining follow up work will be to update the `ClusterData` objects to configure hostname verification there to make it easier to configure hostname verification for remote clusters. * Add `tlsHostnameVerificationEnabled` to the `broker.conf` and the `proxy.conf` * Update all of the relevant locations that were previously only relying on `brokerClient_tlsHostnameVerificationEnable` I added a single test to ensure that the `WebSocketProxyConfiguration` properly converts to the `ServiceConfiguration` object. Otherwise, I verified that anywhere we are using `"brokerClient_"`, this PR also adds the right configuration. This PR introduces a "new" configuration key, but not a new concept. All underlying behaviors are unchanged. - [x] `doc-not-needed` Docs are automatically updated by these changes. (cherry picked from commit 6621fd3) (cherry picked from commit b8083b0)

…19674) When we merged #15818 in order to make the broker's client configurable, we did not add an explicit config for hostname verification. This PR adds that config to the broker and the websocket proxy. I chose the name `tlsHostnameVerificationEnabled` because that is what is already used in the proxy. It diverges from the function worker's config of `tlsEnableHostnameVerification`. Before this PR, you would have enabled hostname verification by configuring `brokerClient_tlsHostnameVerificationEnable=true` in the broker and WS proxy configs. (Note that the variable name is slightly different because the `ClientConfiguration` does not have a `d` at the end of its name. The remaining follow up work will be to update the `ClusterData` objects to configure hostname verification there to make it easier to configure hostname verification for remote clusters. * Add `tlsHostnameVerificationEnabled` to the `broker.conf` and the `proxy.conf` * Update all of the relevant locations that were previously only relying on `brokerClient_tlsHostnameVerificationEnable` I added a single test to ensure that the `WebSocketProxyConfiguration` properly converts to the `ServiceConfiguration` object. Otherwise, I verified that anywhere we are using `"brokerClient_"`, this PR also adds the right configuration. This PR introduces a "new" configuration key, but not a new concept. All underlying behaviors are unchanged. - [x] `doc-not-needed` Docs are automatically updated by these changes. (cherry picked from commit 6621fd3) (cherry picked from commit b8083b0) (cherry picked from commit a3a242c)

…19674) When we merged #15818 in order to make the broker's client configurable, we did not add an explicit config for hostname verification. This PR adds that config to the broker and the websocket proxy. I chose the name `tlsHostnameVerificationEnabled` because that is what is already used in the proxy. It diverges from the function worker's config of `tlsEnableHostnameVerification`. Before this PR, you would have enabled hostname verification by configuring `brokerClient_tlsHostnameVerificationEnable=true` in the broker and WS proxy configs. (Note that the variable name is slightly different because the `ClientConfiguration` does not have a `d` at the end of its name. The remaining follow up work will be to update the `ClusterData` objects to configure hostname verification there to make it easier to configure hostname verification for remote clusters. * Add `tlsHostnameVerificationEnabled` to the `broker.conf` and the `proxy.conf` * Update all of the relevant locations that were previously only relying on `brokerClient_tlsHostnameVerificationEnable` I added a single test to ensure that the `WebSocketProxyConfiguration` properly converts to the `ServiceConfiguration` object. Otherwise, I verified that anywhere we are using `"brokerClient_"`, this PR also adds the right configuration. This PR introduces a "new" configuration key, but not a new concept. All underlying behaviors are unchanged. - [x] `doc-not-needed` Docs are automatically updated by these changes. (cherry picked from commit 6621fd3) (cherry picked from commit b8083b0) (cherry picked from commit a3a242c) (cherry picked from commit c84c3b7)

…pache#19674) When we merged apache#15818 in order to make the broker's client configurable, we did not add an explicit config for hostname verification. This PR adds that config to the broker and the websocket proxy. I chose the name `tlsHostnameVerificationEnabled` because that is what is already used in the proxy. It diverges from the function worker's config of `tlsEnableHostnameVerification`. Before this PR, you would have enabled hostname verification by configuring `brokerClient_tlsHostnameVerificationEnable=true` in the broker and WS proxy configs. (Note that the variable name is slightly different because the `ClientConfiguration` does not have a `d` at the end of its name. The remaining follow up work will be to update the `ClusterData` objects to configure hostname verification there to make it easier to configure hostname verification for remote clusters. * Add `tlsHostnameVerificationEnabled` to the `broker.conf` and the `proxy.conf` * Update all of the relevant locations that were previously only relying on `brokerClient_tlsHostnameVerificationEnable` I added a single test to ensure that the `WebSocketProxyConfiguration` properly converts to the `ServiceConfiguration` object. Otherwise, I verified that anywhere we are using `"brokerClient_"`, this PR also adds the right configuration. This PR introduces a "new" configuration key, but not a new concept. All underlying behaviors are unchanged. - [x] `doc-not-needed` Docs are automatically updated by these changes. (cherry picked from commit 6621fd3) (cherry picked from commit b8083b0) (cherry picked from commit a3a242c)

Configure DLog Bookie, Pulsar, and Admin clients via pass through config

9d1651b

michaeljmarshall added doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. area/config area/function area/broker area/proxy labels May 27, 2022

michaeljmarshall added this to the 2.11.0 milestone May 27, 2022

michaeljmarshall requested review from BewareMyPower, codelipenghui, dave2wave, eolivelli, gaozhangmin, lhotari and merlimat May 27, 2022 16:25

michaeljmarshall self-assigned this May 27, 2022

michaeljmarshall mentioned this pull request May 27, 2022

Support dlog bookie client configuration via pass through config #15763

Closed

dave2wave approved these changes May 27, 2022

View reviewed changes

eolivelli approved these changes May 27, 2022

View reviewed changes

eolivelli added release/2.9.3 release/2.8.4 release/2.10.1 labels May 27, 2022

michaeljmarshall added 2 commits May 31, 2022 14:45

Fix test

2ad8515

Fix another test

cad9159

eolivelli merged commit aa67349 into apache:master Jun 1, 2022

lhotari reviewed Jun 1, 2022

View reviewed changes

lhotari added the cherry-picked/branch-2.10 label Jun 1, 2022

lhotari pushed a commit that referenced this pull request Jun 1, 2022

Configure DLog Bookie, Pulsar, and Admin clients via pass through con…

d144f1e

…fig (#15818) (cherry picked from commit aa67349)

lhotari pushed a commit that referenced this pull request Jun 1, 2022

Configure DLog Bookie, Pulsar, and Admin clients via pass through con…

50d9502

…fig (#15818) (cherry picked from commit aa67349)

lhotari pushed a commit that referenced this pull request Jun 1, 2022

Configure DLog Bookie, Pulsar, and Admin clients via pass through con…

d384076

…fig (#15818) (cherry picked from commit aa67349)

nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jun 1, 2022

Configure DLog Bookie, Pulsar, and Admin clients via pass through con…

177a9dd

…fig (apache#15818) (cherry picked from commit aa67349) (cherry picked from commit d144f1e)

nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jun 1, 2022

Configure DLog Bookie, Pulsar, and Admin clients via pass through con…

7e5f4d5

…fig (apache#15818) (cherry picked from commit aa67349) (cherry picked from commit d384076)

lhotari added cherry-picked/branch-2.8 Archived: 2.8 is end of life cherry-picked/branch-2.9 Archived: 2.9 is end of life labels Jun 1, 2022

lhotari pushed a commit that referenced this pull request Jun 1, 2022

Configure DLog Bookie, Pulsar, and Admin clients via pass through con…

e5d035a

…fig (#15818) (cherry picked from commit aa67349)

michaeljmarshall deleted the make-clients-arbitrarily-configurable branch June 1, 2022 19:12

lhotari added cherry-picked/branch-2.7 Archived: 2.7 is end of life release/2.7.5 labels Jun 2, 2022

momo-jun mentioned this pull request Jul 6, 2022

[fix][doc] Add config of IO/acceptor threads in proxy and fix broken links #16413

Merged

1 task

momo-jun added a commit to momo-jun/pulsar that referenced this pull request Aug 4, 2022

Merge 2.9.3-specific docs from apache#15340 and apache#15818

86571b5

momo-jun added a commit to momo-jun/pulsar that referenced this pull request Aug 5, 2022

Merge 2.8.4-specific docs from apache#15340, apache#16616, and apache…

fe33323

…#15818 to 2.8.x docs

momo-jun mentioned this pull request Sep 7, 2022

[feature][doc] Sync 2.7.5 specific doc changes #17506

Merged

1 task

michaeljmarshall added a commit to datastax/pulsar that referenced this pull request Sep 16, 2022

Configure DLog Bookie, Pulsar, and Admin clients via pass through con…

f55ce33

…fig (apache#15818) (cherry picked from commit aa67349) (cherry picked from commit e5d035a)

momo-jun mentioned this pull request Nov 16, 2022

[Doc] Add the missing configuration override information #18490

Closed

2 tasks

sijie mentioned this pull request Nov 16, 2022

ISSUE-18490: [Doc] Add the missing configuration override information streamnative/pulsar-archived#5122

Open

2 tasks

This was referenced Nov 16, 2022

[fix][doc] Add client config override #18492

Closed

Add client config override apache/pulsar-site#294

Merged

michaeljmarshall mentioned this pull request Mar 1, 2023

[improve] Simplify enabling Broker, WS Proxy hostname verification #19674

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configure DLog Bookie, Pulsar, and Admin clients via pass through config #15818

Configure DLog Bookie, Pulsar, and Admin clients via pass through config #15818

Uh oh!

michaeljmarshall commented May 27, 2022

Uh oh!

dave2wave left a comment

Uh oh!

eolivelli left a comment

Uh oh!

eolivelli May 27, 2022

Uh oh!

lhotari left a comment

Uh oh!

momo-jun commented Jul 6, 2022 •

edited

Loading

Uh oh!

michaeljmarshall commented Jul 6, 2022

Uh oh!

momo-jun commented Jul 6, 2022

Uh oh!

michaeljmarshall commented Jul 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Configure DLog Bookie, Pulsar, and Admin clients via pass through config #15818

Configure DLog Bookie, Pulsar, and Admin clients via pass through config #15818

Uh oh!

Conversation

michaeljmarshall commented May 27, 2022

Motivation

Modifications

Verifying this change

Alternative Implementation

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

Uh oh!

dave2wave left a comment

Choose a reason for hiding this comment

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

eolivelli May 27, 2022

Choose a reason for hiding this comment

Uh oh!

lhotari left a comment

Choose a reason for hiding this comment

Uh oh!

momo-jun commented Jul 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaeljmarshall commented Jul 6, 2022

Uh oh!

momo-jun commented Jul 6, 2022

Uh oh!

michaeljmarshall commented Jul 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

momo-jun commented Jul 6, 2022 •

edited

Loading