[IO] Pass client builder to debezium database history by sijie · Pull Request #11293 · apache/pulsar

sijie · 2021-07-13T04:30:14Z

an alternative approach for #11251

nlu90

LGTM for functionality purpose.

I'm just a little concerned about:

the new exposed API.
the security of passing this object string around.

If you can help explain it's secure to do so, that will be great. Specifically, what happens a user gets the ClientBuilder and change certain field while keep some sensitive auth fields the same. Will this cause unexpected access to another pulsarcluser/tenant/topic?

nlu90 · 2021-07-13T04:45:59Z

        .withWidth(Width.LONG)
        .withImportance(Importance.HIGH)
        .withDescription("Pulsar service url")
        .withValidation(Field::isRequired);


we can set either this serviceUrl field or the following clientBuilder field. so they might not be required now.

eolivelli

Overall the approach looks good to me, it is a great idea indeed.

One question related to security:
with this change the Function code is able to easily access the Authentication information (because it can Serialize the Builder and then read the credentials).
Before this change (with the previous approach) the Function was not able to access the credentials but only to use the PulsarClient, for instance a JWT token could not be stolen. Now you can deploy the Function (the auth information is set by the Admin who deploys the function, not by the author of the function) and the Function can get the credentials and send them outside the cluster.

is it something we should care about ?

nlu90 · 2021-07-28T21:52:46Z

/pulsarbot run-failure-checks

sijie · 2021-07-28T23:14:21Z

+     *
+     * @return the instance of pulsar client builder.
+     */
+    default ClientBuilder getPulsarClientBuilder() {


@nlu90 You need to implement this method in the sub-classes.

working on it

nlu90 · 2021-07-29T21:42:58Z

/pulsarbot run-failure-checks

nlu90 · 2021-07-29T23:14:19Z

/pulsarbot run-failure-checks

nlu90 · 2021-07-30T05:37:23Z

/pulsarbot run-failure-checks

eolivelli

I have reviewed the patch again.
I left two comments, PTAL

nlu90 · 2021-08-02T23:02:20Z

/pulsarbot run-failure-checks

nlu90 · 2021-08-03T20:22:35Z

/pulsarbot run-failure-checks

hangc0276 · 2021-08-05T09:30:04Z

/pulsarbot run-failure-checks

hangc0276 · 2021-08-05T12:48:01Z

@eolivelli Cloud you please help review this PR again, thanks.

eolivelli · 2021-08-05T17:11:44Z

There are still some unaddressed comments of mine:

do not use commons io base64 but Java util
create a utility method to serialize/deserialize the ClientBuilder

eolivelli

looks great to me

eolivelli · 2021-08-10T06:39:58Z

integration tests failed, regarding Debezium, please check

testDebeziumMySqlSourceAvro(org.apache.pulsar.tests.integration.io.sources.debezium.PulsarDebeziumSourcesTest)  Time elapsed: 56.866 s  <<< FAILURE!
java.lang.AssertionError: expected [1] but found [0]
	at org.testng.Assert.fail(Assert.java:99)
	at org.testng.Assert.failNotEquals(Assert.java:1037)
	at org.testng.Assert.assertEqualsImpl(Assert.java:140)
	at org.testng.Assert.assertEquals(Assert.java:122)
	at org.testng.Assert.assertEquals(Assert.java:907)
	at org.testng.Assert.assertEquals(Assert.java:917)
	at org.apache.pulsar.tests.integration.io.sources.PulsarIOSourceRunner.getSourceStatus(PulsarIOSourceRunner.java:210)
	at org.apache.pulsar.tests.integration.io.sources.debezium.PulsarIODebeziumSourceRunner.lambda$testSource$0(PulsarIODebeziumSourceRunner.java:76)
	at net.jodah.failsafe.Functions.lambda$toSupplier$8(Functions.java:236)
	at net.jodah.failsafe.Functions.lambda$get$0(Functions.java:47)
	at net.jodah.failsafe.RetryPolicyExecutor.lambda$supply$0(RetryPolicyExecutor.jav

hangc0276 · 2021-08-10T14:04:07Z

move to 2.8.2.

eolivelli · 2021-08-10T14:09:28Z

@hangc0276 this is a new API, probably it won't be delivered in 2.8.x. I suggest to remove the 2.8.2 label.

in point releases we should not add new APIs

nlu90 · 2021-08-11T15:21:43Z

/pulsarbot run-failure-checks

eolivelli · 2021-08-18T06:50:32Z


-    <dependency>
-      <groupId>${project.groupId}</groupId>
-      <artifactId>pulsar-client-original</artifactId>


This is very interesting.
it looks like that we do not need to import the pulsar-client-original anymore

eolivelli

LGTM

Cherry pick #11293 ## Motivation The Debezium requires pulsar a service URL for history database usage. In #11056 , the service.url field from PulsarKafkaWorkerConfig is no longer available. And the value is also deleted from multiple yaml config files in this commit. This causes the integration test for Debezium connector to fail. Based on the Debezium paradigm, all configurations should be passed as strings. There's no easy way to inject a PulsarClient via configuration. We need to ask user to provide the pulsar url explicitly and probably auth info also. ## Modifications Make the database.history.pulsar.service.url field required Add the config value back to example yaml files Update the integration test config

…m connector (#12145) (#14040) # Conflicts: # pulsar-io/debezium/core/src/main/java/org/apache/pulsar/io/debezium/DebeziumSource.java # pulsar-io/debezium/core/src/main/java/org/apache/pulsar/io/debezium/PulsarDatabaseHistory.java # tests/integration/src/test/java/org/apache/pulsar/tests/integration/io/sources/debezium/DebeziumMySqlSourceTester.java # tests/integration/src/test/java/org/apache/pulsar/tests/integration/io/sources/debezium/PulsarDebeziumSourcesTest.java ### Motivation #11293 allows to passing client builder to debezium database history, but it still requires passing `database.history.pulsar.service.url` as well. With client builder, the `database.history.pulsar.service.url` is not been used anymore. This PR fixes the logic and only pass client builder with no `database.history.pulsar.service.url` provided. Cherry-pick #12145 into branch-2.8

an alternative approach for apache#11251

sijie mentioned this pull request Jul 13, 2021

[pulsar-io] pass the pulsar service url to debezium source for history database #11251

Merged

1 task

nlu90 reviewed Jul 13, 2021

View reviewed changes

eolivelli reviewed Jul 13, 2021

View reviewed changes

Comment thread pulsar-functions/api-java/src/main/java/org/apache/pulsar/functions/api/BaseContext.java Outdated

Comment thread pulsar-io/debezium/core/src/main/java/org/apache/pulsar/io/debezium/DebeziumSource.java Outdated

eolivelli approved these changes Jul 28, 2021

View reviewed changes

sijie changed the title ~~(WIP) [IO] Pass client builder to debezium database history~~ [IO] Pass client builder to debezium database history Jul 28, 2021

sijie assigned nlu90 Jul 28, 2021

sijie added this to the 2.9.0 milestone Jul 28, 2021

sijie added area/connector release/2.8.1 labels Jul 28, 2021

sijie commented Jul 28, 2021

View reviewed changes

eolivelli requested changes Jul 30, 2021

View reviewed changes

Comment thread ...-io/debezium/core/src/test/java/org/apache/pulsar/io/debezium/PulsarDatabaseHistoryTest.java Outdated

nlu90 force-pushed the wip_current_context branch from 3ce2769 to 028210f Compare August 3, 2021 20:53

eolivelli reviewed Aug 5, 2021

View reviewed changes

Comment thread pulsar-io/debezium/core/src/main/java/org/apache/pulsar/io/debezium/PulsarDatabaseHistory.java Outdated

nlu90 force-pushed the wip_current_context branch from 028210f to 2f4919d Compare August 10, 2021 03:01

eolivelli approved these changes Aug 10, 2021

View reviewed changes

hangc0276 added release/2.8.2 and removed release/2.8.1 labels Aug 10, 2021

nlu90 force-pushed the wip_current_context branch from cc2a4d6 to 04d72d8 Compare August 18, 2021 01:57

eolivelli reviewed Aug 18, 2021

View reviewed changes

eolivelli approved these changes Aug 18, 2021

View reviewed changes

freeznet force-pushed the wip_current_context branch from 8b60fe1 to 350950a Compare August 19, 2021 16:08

sijie and others added 12 commits August 20, 2021 09:05

[IO] Pass client builder to debezium database history

6ad320f

fix

e289790

fix

a06d6fb

pass ClientBuilder via context

64643eb

fix test

4cce161

use jul base64

36bfab9

utils class

cbd6c78

close client at last

502a31e

fix test

d5ecacb

not moving client into instance

848f6a7

fix CI

25f2990

allow deserialize to target classloader

bcbcabf

freeznet force-pushed the wip_current_context branch from 350950a to bcbcabf Compare August 20, 2021 01:05

freeznet added 2 commits August 20, 2021 11:44

fix CI

19c1fea

fix CI

28dc104

codelipenghui approved these changes Aug 20, 2021

View reviewed changes

codelipenghui merged commit 12f6566 into apache:master Aug 20, 2021

This was referenced Sep 21, 2021

[Cherry-pick] Pass client builder to debezium database history #12112

Merged

[Cherry-pick] [Functions] reorganize the context hierarchy for functions (#10631) #12117

Merged

freeznet mentioned this pull request Sep 23, 2021

[pulsar-io] pass client builder if no service url provided to debezium connector #12145

Merged

4 tasks

merlimat removed the release/2.8.2 label Oct 30, 2021

This was referenced Jan 29, 2022

[branch-2.8] #12145 pass client builder if no service url provided to debezium connector #14040

Merged

[branch-2.9] #12145 pass client builder if no service url provided to debezium connector #14041

Merged

bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022

[IO] Pass client builder to debezium database history (apache#11293)

04cc940

an alternative approach for apache#11251

Uh oh!

Conversation

sijie commented Jul 13, 2021

Uh oh!

nlu90 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nlu90 Jul 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nlu90 commented Jul 28, 2021

Uh oh!

sijie Jul 28, 2021

Choose a reason for hiding this comment

Uh oh!

nlu90 Jul 28, 2021

Choose a reason for hiding this comment

Uh oh!

nlu90 commented Jul 29, 2021

Uh oh!

nlu90 commented Jul 29, 2021

Uh oh!

nlu90 commented Jul 30, 2021

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nlu90 commented Aug 2, 2021

Uh oh!

nlu90 commented Aug 3, 2021

Uh oh!

hangc0276 commented Aug 5, 2021

Uh oh!

hangc0276 commented Aug 5, 2021

Uh oh!

eolivelli commented Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

eolivelli commented Aug 10, 2021

Uh oh!

hangc0276 commented Aug 10, 2021

Uh oh!

eolivelli commented Aug 10, 2021

Uh oh!

nlu90 commented Aug 11, 2021

Uh oh!

eolivelli Aug 18, 2021

Choose a reason for hiding this comment

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

nlu90 left a comment •

edited

Loading

nlu90 Jul 13, 2021 •

edited

Loading

eolivelli commented Aug 5, 2021 •

edited

Loading