Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[python] Support CryptoKeyReader for Reader API in python clients #11447

Merged
merged 3 commits into from
Aug 21, 2021
Merged

[python] Support CryptoKeyReader for Reader API in python clients #11447

merged 3 commits into from
Aug 21, 2021

Conversation

sanjivr
Copy link
Contributor

@sanjivr sanjivr commented Jul 23, 2021

Motivation

The Reader API in the python client does not support reading an encrypted message.
This PR adds the same and leverages existing C++ Reader API which supports the same.

Modifications

  • Updated pulsar.Client.create_reader to accept crypto_key_reader argument
  • Update existing unit test for Python encryption.

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • Modified existing PulsarTest.test_encryption to also use a reader

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

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (yes / no)
  • The public API: Yes
    • This is a backward compatible update to the signature of a public method in Pulsar Python Client API
  • The schema: (yes / no / don't know)
  • The default values of configurations: (yes / no)
  • The wire protocol: (yes / no)
  • The rest endpoints: (yes / no)
  • The admin cli options: (yes / no)
  • Anything that affects deployment: (yes / no / don't know)

Documentation

For contributor

For this PR, do we need to update docs?

No. These updates will be picked up automatically by pdoc as described at https://pulsar.apache.org/docs/en/client-libraries-python/

For committer

For this PR, do we need to update docs?

  • If yes,

    • if you update docs in this PR, label this PR with the doc label.

    • if you plan to update docs later, label this PR with the doc-required label.

    • if you need help on updating docs, create a follow-up issue with the doc-required label.

  • If no, label this PR with the no-need-doc label and explain why.

@sanjivr sanjivr marked this pull request as ready for review July 23, 2021 20:19
@sanjivr
Copy link
Contributor Author

sanjivr commented Jul 23, 2021

@rdhabalia - Can you take a look at this?

@Anonymitaet Anonymitaet added the doc-not-needed Your PR changes do not impact docs label Jul 27, 2021
@sanjivr
Copy link
Contributor Author

sanjivr commented Aug 3, 2021

I'm not sure how the python unit-tests would have passed without 57690f7

Also, I believe this documentation section https://pulsar.apache.org/docs/en/client-libraries-python/#end-to-end-encryption would benefit from an update if this PR is merged.

@BewareMyPower
Copy link
Contributor

I'm not sure how the python unit-tests would have passed without 57690f7

It's because the CI of cpp-tests is broken, see #11549. The python tests must run after cpp tests passed. However currently cpp tests always failed while the CI still passed.

@BewareMyPower
Copy link
Contributor

Could you rebase to master for the fixed C++ CI?

@sanjivr
Copy link
Contributor Author

sanjivr commented Aug 10, 2021

Could you rebase to master for the fixed C++ CI?

Sure. However this will result in a force push to this branch/pr

@BewareMyPower
Copy link
Contributor

  File "pulsar_test.py", line 381
    MessageId.earliest,
SyntaxError: non-keyword arg after keyword arg

Could you fix this error?

@sijie sijie added doc-required Your PR changes impact docs and you will update later. and removed doc-not-needed Your PR changes do not impact docs labels Aug 21, 2021
Copy link
Member

@sijie sijie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need documentation for this.

@sijie sijie merged commit cf7de12 into apache:master Aug 21, 2021
@sanjivr sanjivr deleted the sanjivr/python-reader-crypto branch August 21, 2021 02:31
@Anonymitaet
Copy link
Member

Hi @sanjivr Thanks for your great contribution.
Want users to know your awesome code changes? Do not forget to add docs accordingly! And you can ping me to review the docs. Let's grow Pulsar together!

BewareMyPower pushed a commit that referenced this pull request Oct 15, 2021
…1447)

The Reader API in the python client does not support reading an encrypted message.
This PR adds the same and leverages existing  C++ Reader API which supports the same.

* Updated `pulsar.Client.create_reader` to accept  `crypto_key_reader` argument
* Update existing unit test for Python encryption.

(cherry picked from commit f7de12fa52e43f886404968691968b514831dac)

Solved conflicts by modifing following files:
- pulsar-client-cpp/python/pulsar_test.py
- pulsar-client-cpp/python/src/config.cc
@codelipenghui codelipenghui added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Oct 15, 2021
@Anonymitaet Anonymitaet removed the doc-required Your PR changes impact docs and you will update later. label Oct 21, 2021
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
…ache#11447)

### Motivation

The Reader API in the python client does not support reading an encrypted message. 
This PR adds the same and leverages existing  C++ Reader API which supports the same.

### Modifications

* Updated `pulsar.Client.create_reader` to accept  `crypto_key_reader` argument
* Update existing unit test for Python encryption.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/client cherry-picked/branch-2.8 Archived: 2.8 is end of life
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants