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

[fix][client] add support to TableView to read encrypted messages #19008

Merged
merged 9 commits into from
Dec 26, 2022

Conversation

rbarbey
Copy link
Contributor

@rbarbey rbarbey commented Dec 20, 2022

Fixes #19007

Motivation

This PR adds support for a TableView to read encrypted messages from a topic.

Modifications

Analogous to ReaderBuilder, I've added methods to TableViewBuilder that allow to specify a CryptoKeyReader and a ConsumerCryptoFailureAction. The respective values are stored in TableViewConfigurationData and forwarded to the ReaderBuilder instance inside TableViewImpl when the Reader is constructed.

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • Added unit tests for TableViewImpl and TableViewBuilderImpl
  • Extended integration test verifying that encrypted messages can be read inside a TableView

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

If the box was checked, please highlight the changes

  • The public API

Documentation

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

Matching PR in forked repository

PR in forked repository

@github-actions github-actions bot added the doc-complete Your PR changes impact docs and the related docs have been already added. label Dec 20, 2022
… TableView

- CryptoKeyReader is added to TableViewConfigurationData
- when the Reader inside the TableView is being built, add it in case
  it is not null
- by default, this action is set to ConsumerCryptoFailureAction.FAIL
- added to the reader when the TableView is being built
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Please add a test case

@rbarbey rbarbey force-pushed the tableview-cryptoreader branch from ece6706 to b1bf335 Compare December 20, 2022 14:08
@rbarbey
Copy link
Contributor Author

rbarbey commented Dec 20, 2022 via email

@labuladong
Copy link
Contributor

If you’re talking about unit tests

Yes, please add some tests to cover the new feature to make it easy to verify.

@Technoboy- Technoboy- added this to the 2.12.0 milestone Dec 21, 2022
@Technoboy- Technoboy- added doc-not-needed Your PR changes do not impact docs and removed doc-complete Your PR changes impact docs and the related docs have been already added. labels Dec 21, 2022
@github-actions github-actions bot added doc-complete Your PR changes impact docs and the related docs have been already added. and removed doc-not-needed Your PR changes do not impact docs labels Dec 21, 2022
@rbarbey rbarbey force-pushed the tableview-cryptoreader branch from ac4d3f8 to e15314c Compare December 21, 2022 09:51
@github-actions github-actions bot added doc-not-needed Your PR changes do not impact docs and removed doc-complete Your PR changes impact docs and the related docs have been already added. labels Dec 21, 2022
@rbarbey
Copy link
Contributor Author

rbarbey commented Dec 21, 2022

@eolivelli I've added unit tests for TableViewImpl and TableViewBuilderImpl (there weren't any before and I took heavy inspiration on similar tests relating to Producers and Consumers.

Then I found TableViewImplTest in the pulsar-broker module which is an integration and there I've added a test that actually shows that encrypted messages can be read inside TableViewImpl.

I've run the tests locally (using the Maven commands I found in the workflow YAML) and they were passing so they should pass here as well.

@rbarbey rbarbey requested a review from eolivelli December 21, 2022 10:05
@BewareMyPower
Copy link
Contributor

You should create a PR in your fork, the link you pasted is not a PR in your repo. See my PRs in my fork as example: https://github.com/BewareMyPower/pulsar/pulls

@rbarbey
Copy link
Contributor Author

rbarbey commented Dec 23, 2022

@BewareMyPower Thank you! I've created a PR in my fork and linked it in the original comment of this PR as well. I've also edited the title of this PR to follow the convention in this project.

@rbarbey rbarbey changed the title Add support to TableView to read encrypted messages [fix][client] add support to TableView to read encrypted messages Dec 23, 2022
@BewareMyPower
Copy link
Contributor

image

image

Please synchronize your master (there should be a sync button in your account). Otherwise the file changed might be different in the PR of your fork repo.

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

LGTM

@BewareMyPower
Copy link
Contributor

Creating a matching PR is to trigger the CI in your own account to avoid unnecessary CI resource usages in the Apache repo. But I see no CI has run in your PR.

image

Here is an example of a correct matching PR that ran the CI.

image

@rbarbey
Copy link
Contributor Author

rbarbey commented Dec 23, 2022

Synced my fork. Will look into the missing CI runs later. As I mentioned, I ran tests etc locally on my machine before I pushed my commits so I don't expect any tests to fail.

@rbarbey
Copy link
Contributor Author

rbarbey commented Dec 23, 2022

@BewareMyPower I've activated the CI checks in my fork and they ran successfully now. Please let me know if there's anything else left to do from my side. Thank you!

@codecov-commenter
Copy link

codecov-commenter commented Dec 26, 2022

Codecov Report

Merging #19008 (afce59c) into master (08591d9) will decrease coverage by 2.67%.
The diff coverage is 66.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #19008      +/-   ##
============================================
- Coverage     49.85%   47.17%   -2.68%     
- Complexity     8658    10649    +1991     
============================================
  Files           500      709     +209     
  Lines         54930    69436   +14506     
  Branches       5867     7451    +1584     
============================================
+ Hits          27386    32758    +5372     
- Misses        24464    32999    +8535     
- Partials       3080     3679     +599     
Flag Coverage Δ
unittests 47.17% <66.66%> (-2.68%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...pache/pulsar/client/impl/TableViewBuilderImpl.java 72.41% <55.55%> (ø)
...a/org/apache/pulsar/client/impl/TableViewImpl.java 29.16% <75.00%> (ø)
...pulsar/client/impl/TableViewConfigurationData.java 43.75% <100.00%> (ø)
...lsar/broker/loadbalance/impl/ThresholdShedder.java 3.27% <0.00%> (-27.05%) ⬇️
.../apache/pulsar/broker/loadbalance/LoadManager.java 61.11% <0.00%> (-16.67%) ⬇️
...ersistentStickyKeyDispatcherMultipleConsumers.java 46.07% <0.00%> (-13.73%) ⬇️
...tent/NonPersistentDispatcherMultipleConsumers.java 40.74% <0.00%> (-12.35%) ⬇️
...apache/pulsar/broker/service/TopicListService.java 40.80% <0.00%> (-12.00%) ⬇️
...org/apache/pulsar/broker/loadbalance/LoadData.java 58.33% <0.00%> (-8.34%) ⬇️
...g/apache/pulsar/broker/service/StreamingStats.java 75.67% <0.00%> (-8.11%) ⬇️
... and 250 more

@BewareMyPower BewareMyPower merged commit ef750a8 into apache:master Dec 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] TableView does not support reading from encrypted topics
6 participants