[feat][client] Support configure MessageCrypto in ProducerBuilder by tisonkun · Pull Request #19939 · apache/pulsar

tisonkun · 2023-03-27T14:44:23Z

This closes #19139.

Motivation

It's a lightweight feature alignment. Since it somehow touches public API, I don't know if it requires a PIP. It may not be, just trivial.

Modifications

Support configure MessageCrypto in ProducerBuilder.

We already test this field but never pass or expose the config:

pulsar/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ProducerImpl.java

Lines 207 to 219 in 32ad906

    
           if (conf.getMessageCrypto() != null) { 
        
               this.msgCrypto = conf.getMessageCrypto(); 
        
           } else { 
        
               // default to use MessageCryptoBc; 
        
               MessageCrypto msgCryptoBc; 
        
               try { 
        
                   msgCryptoBc = new MessageCryptoBc(logCtx, true); 
        
               } catch (Exception e) { 
        
                   log.error("MessageCryptoBc may not included in the jar in Producer. e:", e); 
        
                   msgCryptoBc = null; 
        
               } 
        
               this.msgCrypto = msgCryptoBc; 
        
           }

Verifying this change

Trivial.

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

If the box was checked, please highlight the changes

Documentation

Maybe I should update some doc, but I'm not sure where is related doc.

doc
doc-required
doc-not-needed
doc-complete

Matching PR in forked repository

PR in forked repository:

Signed-off-by: tison <wander4096@gmail.com>

Technoboy- · 2023-03-28T09:15:20Z

LGTM

RobertIndie

I think it was the lack of testing that caused the missing exposure of this field to the ProducerBuilder. Better to add a test to cover this case.

tisonkun · 2023-03-29T08:58:53Z

@RobertIndie I was thinking of it, but the only test case comes to my mind is some getter/setter tests (i.e., we configure the message crypoto object, and verify that it's configured). I can add some if asked.

RobertIndie

Just found out that the consumer also lacks of function test of message crypto. I'm +1 for this PR.

tisonkun · 2023-03-29T13:21:25Z

Just found out that the consumer also lacks of function test of message crypto. I'm +1 for this PR.

Yep. This is a standard builder pattern and a complement. We don't test builders but upper-layer logic, do we?

eolivelli

I support this patch and I don't think it requires a PIP.

But I think that we need to need some test at least to prevent that someone changes the API in the future.

Please also add the test for the similar method on the ConsumerBuilder

Signed-off-by: tison <wander4096@gmail.com>

tisonkun · 2023-03-30T16:19:17Z

@eolivelli e73ad35 add basic set-and-get tests to prevent API breaking. ConsumerBuilderImplTest already has similar API call to prevent API breaking. Please take another look :)

tisonkun · 2023-04-01T06:17:56Z

Thanks for your reviews!

Merging...

Anonymitaet · 2023-04-10T03:10:56Z

@tisonkun thanks for introducing this improvement!

I see this PR is labeled with doc-required, so from the doc perspective:

Java Client - API doc: it has been added.
Java Client - User doc: do we need to add some usage examples to Java client - Work with producer?
Pulsar Feature Matrix (Client): do we need to add details to Java client producer - End-to-end Encryption?

Thank you!

…ache#19939) Signed-off-by: tison <wander4096@gmail.com> (cherry picked from commit 0214745)

tisonkun added the ready-to-test label Mar 27, 2023

tisonkun requested review from Technoboy-, codelipenghui, eolivelli and merlimat March 27, 2023 14:44

tisonkun self-assigned this Mar 27, 2023

github-actions Bot added the doc-required Your PR changes impact docs and you will update later. label Mar 27, 2023

[feat][client] Support configure MessageCrypto in ProducerBuilder

56b048d

Signed-off-by: tison <wander4096@gmail.com>

tisonkun force-pushed the producer-MessageCrypto branch from 000889e to 56b048d Compare March 27, 2023 14:59

Technoboy- added this to the 3.0.0 milestone Mar 28, 2023

RobertIndie self-requested a review March 29, 2023 08:33

RobertIndie reviewed Mar 29, 2023

View reviewed changes

RobertIndie approved these changes Mar 29, 2023

View reviewed changes

eolivelli requested changes Mar 30, 2023

View reviewed changes

tisonkun added 2 commits March 31, 2023 00:05

Merge branch 'master' into producer-MessageCrypto

6efdbad

Add basic test for messageCrypto to prevent API breaking

e73ad35

Signed-off-by: tison <wander4096@gmail.com>

tisonkun requested a review from eolivelli March 30, 2023 16:19

eolivelli approved these changes Apr 1, 2023

View reviewed changes

tisonkun merged commit 0214745 into apache:master Apr 1, 2023

tisonkun deleted the producer-MessageCrypto branch April 1, 2023 06:18

tisonkun mentioned this pull request Jun 30, 2023

[feat] [client] Enable custom Encrypt/Decrypt Methods for ReaderImplementation. #12599

Merged

1 task

poorbarcode pushed a commit to streamnative/pulsar-archived that referenced this pull request Aug 29, 2023

[feat][client] Support configure MessageCrypto in ProducerBuilder (ap…

86c4a97

…ache#19939) Signed-off-by: tison <wander4096@gmail.com> (cherry picked from commit 0214745)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feat][client] Support configure MessageCrypto in ProducerBuilder#19939

[feat][client] Support configure MessageCrypto in ProducerBuilder#19939
tisonkun merged 3 commits into
apache:masterfrom
tisonkun:producer-MessageCrypto

tisonkun commented Mar 27, 2023 •

edited

Loading

Uh oh!

Technoboy- commented Mar 28, 2023

Uh oh!

RobertIndie left a comment

Uh oh!

tisonkun commented Mar 29, 2023 •

edited

Loading

Uh oh!

RobertIndie left a comment

Uh oh!

tisonkun commented Mar 29, 2023

Uh oh!

eolivelli left a comment

Uh oh!

tisonkun commented Mar 30, 2023

Uh oh!

tisonkun commented Apr 1, 2023

Uh oh!

Anonymitaet commented Apr 10, 2023 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	if (conf.getMessageCrypto() != null) {
	this.msgCrypto = conf.getMessageCrypto();
	} else {
	// default to use MessageCryptoBc;
	MessageCrypto msgCryptoBc;
	try {
	msgCryptoBc = new MessageCryptoBc(logCtx, true);
	} catch (Exception e) {
	log.error("MessageCryptoBc may not included in the jar in Producer. e:", e);
	msgCryptoBc = null;
	}
	this.msgCrypto = msgCryptoBc;
	}

Conversation

tisonkun commented Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Verifying this change

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

Documentation

Matching PR in forked repository

Uh oh!

Technoboy- commented Mar 28, 2023

Uh oh!

RobertIndie left a comment

Choose a reason for hiding this comment

Uh oh!

tisonkun commented Mar 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RobertIndie left a comment

Choose a reason for hiding this comment

Uh oh!

tisonkun commented Mar 29, 2023

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

tisonkun commented Mar 30, 2023

Uh oh!

tisonkun commented Apr 1, 2023

Uh oh!

Anonymitaet commented Apr 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tisonkun commented Mar 27, 2023 •

edited

Loading

tisonkun commented Mar 29, 2023 •

edited

Loading

Anonymitaet commented Apr 10, 2023 •

edited

Loading