You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/12/13 00:40:10 UTC

[GitHub] [pulsar] nicoloboschi opened a new pull request, #18898: [fix][misc] do not require encryption on system topics

nicoloboschi opened a new pull request, #18898:
URL: https://github.com/apache/pulsar/pull/18898

   Fixes #18897
   
   ### Motivation
   
   System topics on user namespace may be subject to have encryption set on the producer side. However they can't be forced to be encrypted since the encryption keys are set on the client side. Also they don't contain sensitive data. 
   
   The two main cases that I encountered are:
   1. __change_events: if you enable topic level policies
   2. transaction side topics
   
   ### Modifications
   
   * Always allow messages in the system topics to not be encrypted
   * Fix writability check for txn on the broker's producer 
   
   ### Documentation
   
   <!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
   
   - [ ] `doc` <!-- Your PR contains doc changes. Please attach the local preview screenshots (run `sh start.sh` at `pulsar/site2/website`) to your PR description, or else your PR might not get merged. -->
   - [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
   - [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
   - [ ] `doc-complete` <!-- Docs have been already added -->
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] nicoloboschi commented on pull request #18898: [fix][misc] do not require encryption on system topics

Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on PR #18898:
URL: https://github.com/apache/pulsar/pull/18898#issuecomment-1349581801

   > @nicoloboschi - I support this change, but I think it needs a little more work.
   > 
   > This code is relevant:
   > 
   > https://github.com/apache/pulsar/blob/5b062f37bd6688c09384abbe908f7c14e28052fc/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Producer.java#L785-L791
   > 
   > It is used here and in the non-partitioned topic:
   > 
   > https://github.com/apache/pulsar/blob/5b062f37bd6688c09384abbe908f7c14e28052fc/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java#L2642
   > 
   > If I am reading the code correctly, without updating the `Producer#checkEncryption` method, an update to a namespace policy will result in an unnecessary disconnection for producers to system topics when encryption becomes required for a namespace.
   
   
   I've implemented isEncryptionRequired to always return false so the disconnection is never triggered. or maybe I didn't get your comments 🤔 
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] nicoloboschi merged pull request #18898: [fix][misc] do not require encryption on system topics

Posted by GitBox <gi...@apache.org>.
nicoloboschi merged PR #18898:
URL: https://github.com/apache/pulsar/pull/18898


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] lhotari commented on pull request #18898: [fix][misc] do not require encryption on system topics

Posted by GitBox <gi...@apache.org>.
lhotari commented on PR #18898:
URL: https://github.com/apache/pulsar/pull/18898#issuecomment-1349216143

   /pulsarbot rerun-failure-checks


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] nicoloboschi commented on a diff in pull request #18898: [fix][misc] do not require encryption on system topics

Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on code in PR #18898:
URL: https://github.com/apache/pulsar/pull/18898#discussion_r1047647258


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Producer.java:
##########
@@ -800,7 +800,9 @@ public void checkEncryption() {
 
     public void publishTxnMessage(TxnID txnID, long producerId, long sequenceId, long highSequenceId,
                                   ByteBuf headersAndPayload, long batchSize, boolean isChunked, boolean isMarker) {
-        checkAndStartPublish(producerId, sequenceId, headersAndPayload, batchSize, null);
+        if (!checkAndStartPublish(producerId, sequenceId, headersAndPayload, batchSize, null)) {
+            return;
+        }

Review Comment:
   removed. will open another pull



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #18898: [fix][misc] do not require encryption on system topics

Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on code in PR #18898:
URL: https://github.com/apache/pulsar/pull/18898#discussion_r1047535924


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Producer.java:
##########
@@ -800,7 +800,9 @@ public void checkEncryption() {
 
     public void publishTxnMessage(TxnID txnID, long producerId, long sequenceId, long highSequenceId,
                                   ByteBuf headersAndPayload, long batchSize, boolean isChunked, boolean isMarker) {
-        checkAndStartPublish(producerId, sequenceId, headersAndPayload, batchSize, null);
+        if (!checkAndStartPublish(producerId, sequenceId, headersAndPayload, batchSize, null)) {
+            return;
+        }

Review Comment:
   Is this its own bug? It seems unrelated to encryption.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] nicoloboschi commented on a diff in pull request #18898: [fix][misc] do not require encryption on system topics

Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on code in PR #18898:
URL: https://github.com/apache/pulsar/pull/18898#discussion_r1047643177


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Producer.java:
##########
@@ -800,7 +800,9 @@ public void checkEncryption() {
 
     public void publishTxnMessage(TxnID txnID, long producerId, long sequenceId, long highSequenceId,
                                   ByteBuf headersAndPayload, long batchSize, boolean isChunked, boolean isMarker) {
-        checkAndStartPublish(producerId, sequenceId, headersAndPayload, batchSize, null);
+        if (!checkAndStartPublish(producerId, sequenceId, headersAndPayload, batchSize, null)) {
+            return;
+        }

Review Comment:
   yes it's a bit out of the encryption fix scope. I can open another pull request 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] codecov-commenter commented on pull request #18898: [fix][misc] do not require encryption on system topics

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #18898:
URL: https://github.com/apache/pulsar/pull/18898#issuecomment-1349313739

   # [Codecov](https://codecov.io/gh/apache/pulsar/pull/18898?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#18898](https://codecov.io/gh/apache/pulsar/pull/18898?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6513872) into [master](https://codecov.io/gh/apache/pulsar/commit/1be5a69a079594d8d96d2a0ab7ab8b389da8865e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1be5a69) will **increase** coverage by `36.65%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/pulsar/pull/18898/graphs/tree.svg?width=650&height=150&src=pr&token=acYqCpsK9J&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/pulsar/pull/18898?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@              Coverage Diff              @@
   ##             master   #18898       +/-   ##
   =============================================
   + Coverage     33.99%   70.65%   +36.65%     
   + Complexity     6476      436     -6040     
   =============================================
     Files           623       26      -597     
     Lines         59103     2246    -56857     
     Branches       6147      245     -5902     
   =============================================
   - Hits          20095     1587    -18508     
   + Misses        36347      486    -35861     
   + Partials       2661      173     -2488     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | unittests | `70.65% <ø> (+36.65%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/pulsar/pull/18898?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...ava/org/apache/pulsar/broker/service/Producer.java](https://codecov.io/gh/apache/pulsar/pull/18898/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL1Byb2R1Y2VyLmphdmE=) | | |
   | [...va/org/apache/pulsar/broker/service/ServerCnx.java](https://codecov.io/gh/apache/pulsar/pull/18898/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL1NlcnZlckNueC5qYXZh) | | |
   | [.../pulsar/broker/service/persistent/SystemTopic.java](https://codecov.io/gh/apache/pulsar/pull/18898/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL3BlcnNpc3RlbnQvU3lzdGVtVG9waWMuamF2YQ==) | | |
   | [...che/pulsar/client/impl/schema/LocalDateSchema.java](https://codecov.io/gh/apache/pulsar/pull/18898/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL3NjaGVtYS9Mb2NhbERhdGVTY2hlbWEuamF2YQ==) | | |
   | [.../client/impl/schema/generic/GenericJsonReader.java](https://codecov.io/gh/apache/pulsar/pull/18898/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL3NjaGVtYS9nZW5lcmljL0dlbmVyaWNKc29uUmVhZGVyLmphdmE=) | | |
   | [...va/org/apache/pulsar/client/impl/ProducerBase.java](https://codecov.io/gh/apache/pulsar/pull/18898/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL1Byb2R1Y2VyQmFzZS5qYXZh) | | |
   | [...a/org/apache/pulsar/broker/admin/v1/Functions.java](https://codecov.io/gh/apache/pulsar/pull/18898/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9hZG1pbi92MS9GdW5jdGlvbnMuamF2YQ==) | | |
   | [.../apache/pulsar/broker/admin/impl/PackagesBase.java](https://codecov.io/gh/apache/pulsar/pull/18898/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9hZG1pbi9pbXBsL1BhY2thZ2VzQmFzZS5qYXZh) | | |
   | [...va/org/apache/pulsar/broker/admin/v2/Clusters.java](https://codecov.io/gh/apache/pulsar/pull/18898/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9hZG1pbi92Mi9DbHVzdGVycy5qYXZh) | | |
   | [.../pulsar/broker/service/SharedConsumerAssignor.java](https://codecov.io/gh/apache/pulsar/pull/18898/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL1NoYXJlZENvbnN1bWVyQXNzaWdub3IuamF2YQ==) | | |
   | ... and [587 more](https://codecov.io/gh/apache/pulsar/pull/18898/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] nicoloboschi commented on a diff in pull request #18898: [fix][misc] do not require encryption on system topics

Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on code in PR #18898:
URL: https://github.com/apache/pulsar/pull/18898#discussion_r1047637753


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/SystemTopic.java:
##########
@@ -76,4 +76,10 @@ public boolean isCompactionEnabled() {
         // even though is not explicitly set in the policies.
         return !NamespaceService.isHeartbeatNamespace(TopicName.get(topic));
     }
+
+    @Override
+    public boolean isEncryptionRequired() {
+        // System topics are only written by the broker that can't know the encryption context.

Review Comment:
   AFAIK we haven't state that ONLY the broker (or pulsar internals) write to these topics but they for sure they do so they need this fix 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] nicoloboschi commented on a diff in pull request #18898: [fix][misc] do not require encryption on system topics

Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on code in PR #18898:
URL: https://github.com/apache/pulsar/pull/18898#discussion_r1047662308


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -1326,7 +1327,9 @@ protected void handleProducer(final CommandProducer cmdProducer) {
 
                 backlogQuotaCheckFuture.thenRun(() -> {
                     // Check whether the producer will publish encrypted messages or not
-                    if ((topic.isEncryptionRequired() || encryptionRequireOnProducer) && !isEncrypted) {
+                    if ((topic.isEncryptionRequired() || encryptionRequireOnProducer)
+                            && !isEncrypted
+                            && !SystemTopicNames.isSystemTopic(topicName)) {

Review Comment:
   the previous condition was: topic requires encryption or producer requested encryption 
   if I leave it as is, the `encryptionRequireOnProducer` will be valid also for system topics and then the `opic.isEncryptionRequired()` would be useless



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #18898: [fix][misc] do not require encryption on system topics

Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on code in PR #18898:
URL: https://github.com/apache/pulsar/pull/18898#discussion_r1047656463


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -1326,7 +1327,9 @@ protected void handleProducer(final CommandProducer cmdProducer) {
 
                 backlogQuotaCheckFuture.thenRun(() -> {
                     // Check whether the producer will publish encrypted messages or not
-                    if ((topic.isEncryptionRequired() || encryptionRequireOnProducer) && !isEncrypted) {
+                    if ((topic.isEncryptionRequired() || encryptionRequireOnProducer)
+                            && !isEncrypted
+                            && !SystemTopicNames.isSystemTopic(topicName)) {

Review Comment:
   If `topic.isEncryptionRequired()` is always `false` for system topics, why do we need the check on the `topicName` on line 1332? 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #18898: [fix][misc] do not require encryption on system topics

Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on code in PR #18898:
URL: https://github.com/apache/pulsar/pull/18898#discussion_r1047663584


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -1326,7 +1327,9 @@ protected void handleProducer(final CommandProducer cmdProducer) {
 
                 backlogQuotaCheckFuture.thenRun(() -> {
                     // Check whether the producer will publish encrypted messages or not
-                    if ((topic.isEncryptionRequired() || encryptionRequireOnProducer) && !isEncrypted) {
+                    if ((topic.isEncryptionRequired() || encryptionRequireOnProducer)
+                            && !isEncrypted
+                            && !SystemTopicNames.isSystemTopic(topicName)) {

Review Comment:
   Oh, of course. Thanks for explaining.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #18898: [fix][misc] do not require encryption on system topics

Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on code in PR #18898:
URL: https://github.com/apache/pulsar/pull/18898#discussion_r1047522423


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/SystemTopic.java:
##########
@@ -76,4 +76,10 @@ public boolean isCompactionEnabled() {
         // even though is not explicitly set in the policies.
         return !NamespaceService.isHeartbeatNamespace(TopicName.get(topic));
     }
+
+    @Override
+    public boolean isEncryptionRequired() {
+        // System topics are only written by the broker that can't know the encryption context.

Review Comment:
   It's tangential, but have we explicitly stated the limitation that system topics are only written to by the broker?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] nicoloboschi commented on pull request #18898: [fix][misc] do not require encryption on system topics

Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on PR #18898:
URL: https://github.com/apache/pulsar/pull/18898#issuecomment-1350594205

   Since 2.11.0 system topics are enabled by defaults so I think we must include this fix in 2.11.0. Otherwise if users upgrade, they will start seeing a lot of errors in the broker


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org