Skip to content

[improve][pip] PIP-432: Add isEncrypted field to EncryptionContext #24481

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.

Sign up for GitHub

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

Jump to bottom
Merged
codelipenghui merged 5 commits into from
Jul 15, 2025
Merged

[improve][pip] PIP-432: Add isEncrypted field to EncryptionContext #24481

codelipenghui merged 5 commits into from
Jul 15, 2025

Conversation

@codelipenghui
Copy link
Contributor

@codelipenghui codelipenghui commented Jul 3, 2025

Motivation

Applications using Pulsar's encryption feature with ConsumerCryptoFailureAction.CONSUME need to determine whether received messages were successfully decrypted or if decryption failed. This is essential for:

  1. Error handling: Applications need to know when they receive encrypted (undecrypted) data to handle it appropriately
  2. Monitoring: Applications want to track decryption success/failure rates for monitoring and alerting
  3. Manual decryption: When automatic decryption fails, applications may want to attempt manual decryption using the EncryptionContext
  4. Security compliance: Applications need to ensure they're not inadvertently processing encrypted data as plain text

Current situation:

  • Consumers with CONSUME action receive messages regardless of decryption success
  • No programmatic way to distinguish between successfully decrypted and failed decryption messages
  • Applications must implement workarounds to detect encrypted vs. decrypted content

Use cases this solves:

  1. Consumer without private key configured → should know decryption failed
  2. Consumer with mismatched private key → should know decryption failed
  3. Consumer with correct private key → should know decryption succeeded

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

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

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

@codelipenghui codelipenghui self-assigned this Jul 3, 2025
@github-actions github-actions bot added PIP and removed ready-to-test labels Jul 3, 2025
@codelipenghui codelipenghui added this to the 4.1.0 milestone Jul 3, 2025
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jul 3, 2025
@codelipenghui
Copy link
Contributor Author

codelipenghui commented Jul 3, 2025

/pulsarbot run-failure-checks

liangyepianzhou
liangyepianzhou approved these changes Jul 3, 2025
View reviewed changes
Copy link
Contributor

@liangyepianzhou liangyepianzhou left a comment

Choose a reason for hiding this comment

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

LGTM

@codelipenghui
Copy link
Contributor Author

codelipenghui commented Jul 3, 2025

/pulsarbot run-failure-checks

@codecov-commenter
Copy link

codecov-commenter commented Jul 3, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.27%. Comparing base (bbc6224) to head (e8ffd16).
⚠️ Report is 1250 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #24481      +/-   ##
============================================
+ Coverage     73.57%   74.27%   +0.70%     
+ Complexity    32624    32494     -130     
============================================
  Files          1877     1868       -9     
  Lines        139502   145939    +6437     
  Branches      15299    16734    +1435     
============================================
+ Hits         102638   108399    +5761     
- Misses        28908    28951      +43     
- Partials       7956     8589     +633
Flag Coverage Δ
inttests 26.68% <50.00%> (+2.10%) ⬆️
systests 23.28% <75.00%> (-1.04%) ⬇️
unittests 73.77% <100.00%> (+0.93%) ⬆️

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

Files with missing lines Coverage Δ
...rg/apache/pulsar/common/api/EncryptionContext.java 100.00% <ø> (ø)
...va/org/apache/pulsar/client/impl/ConsumerImpl.java 78.93% <100.00%> (+1.36%) ⬆️
.../pulsar/client/impl/MessagePayloadContextImpl.java 74.46% <ø> (ø)
...ache/pulsar/client/impl/ZeroQueueConsumerImpl.java 65.43% <ø> (ø)

... and 1094 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@3pacccccc
Copy link
Contributor

3pacccccc commented Jul 8, 2025

LGTM. Additionally, could we introduce a new decryptFailListener to handle decryption failures, instead of using if-else checks? This would provide a more elegant solution. The code might look like this:

pulsarClient.newConsumer()
    .topic(topicName)
    .subscriptionName(subName)
    .messageListener((consumer, msg) -> {
        // Handle successfully decrypted messages
        handleDecryptedMessage(msg);
    })
    .decryptFailListener((consumer, msg) -> {
        // Handle decryption failures
        handleEncryptedMessage(msg);
    });

@codelipenghui
Copy link
Contributor Author

codelipenghui commented Jul 8, 2025

@3pacccccc Yes, that's a good idea to have a listener for the decryption failures. Do you want to create a proposal for it? With the decryption failure listener, we can suggest users to use listener instead of ConsumerCryptoFailureAction. They can decide in the listener to discard, or decrypt, or anything they want.

@3pacccccc
Copy link
Contributor

3pacccccc commented Jul 8, 2025

@3pacccccc Yes, that's a good idea to have a listener for the decryption failures. Do you want to create a proposal for it? With the decryption failure listener, we can suggest users to use listener instead of ConsumerCryptoFailureAction. They can decide in the listener to discard, or decrypt, or anything they want.

yes，I‘m sooo glad if I can do anything to help

@codelipenghui
Copy link
Contributor Author

codelipenghui commented Jul 10, 2025

/pulsarbot run-failure-checks

codelipenghui and others added 4 commits July 14, 2025 21:48
@codelipenghui
Merge master branch and resolve conflicts
301fe08
@codelipenghui @claude
Improve comment for isEncrypted field in EncryptionContext
dd51972 
Clarify that the field indicates whether message payload remains
encrypted (true) or has been successfully decrypted (false).

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@codelipenghui @claude
[fix][test] Fix checkstyle errors in SimpleProducerConsumerTest
1b60dab 
- Remove trailing whitespace
- Fix line length violation by breaking long assertion message

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@codelipenghui
Fix test
e8ffd16
@codelipenghui codelipenghui merged commit 994b1be into apache:master Jul 15, 2025
51 checks passed
@codelipenghui codelipenghui deleted the penghui/pip-432 branch July 15, 2025 15:26
This was referenced Jul 30, 2025
Merged
Closed
KannarFr pushed a commit to CleverCloud/pulsar that referenced this pull request Sep 22, 2025
@codelipenghui @KannarFr
[improve][pip] PIP-432: Add isEncrypted field to EncryptionContext (a…
e94d705 
…pache#24481)
walkinggo pushed a commit to walkinggo/pulsar that referenced this pull request Oct 8, 2025
@codelipenghui @walkinggo
[improve][pip] PIP-432: Add isEncrypted field to EncryptionContext (a…
c0d9ade 
…pache#24481)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BewareMyPower BewareMyPower BewareMyPower left review comments

@RobertIndie RobertIndie RobertIndie approved these changes

@liangyepianzhou liangyepianzhou liangyepianzhou approved these changes

+1 more reviewer

@3pacccccc 3pacccccc 3pacccccc approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@codelipenghui codelipenghui

Labels

doc-not-needed Your PR changes do not impact docs PIP ready-to-test

Projects

None yet

Milestone

4.1.0

Development

Successfully merging this pull request may close these issues.

6 participants

@codelipenghui @codecov-commenter @3pacccccc @RobertIndie @BewareMyPower @liangyepianzhou