[improve][broker] PIP-402: Optionally prevent role/originalPrincipal logging #23386

KannarFr · 2024-10-01T19:48:59Z

Motivation

Add option in broker and proxy to optionally prevent them from logging role or orignalPrincipal. If the option is activated they will log [REDACTED].

If I missed some logs, please share them.

Verifying this change

Make sure that the change passes the CI checks.

This change is a trivial rework / code cleanup without any test coverage.

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

Documentation

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

lhotari · 2024-10-01T20:53:13Z

Add option in broker and proxy to optionally prevent them from logging role or orignalPrincipal. If the option is activated they will log [REDACTED].

Do you have a chance to share some details of the use case where this is needed? Does the role contain sensitive information? Is this a compliance requirement to not log it?

pulsar-common/src/main/java/org/apache/pulsar/common/util/RedactedRole.java

pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java

pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConnection.java

pulsar-common/src/main/java/org/apache/pulsar/common/util/RedactedRole.java

KannarFr · 2024-10-02T08:52:14Z

Add option in broker and proxy to optionally prevent them from logging role or orignalPrincipal. If the option is activated they will log [REDACTED].

Do you have a chance to share some details of the use case where this is needed? Does the role contain sensitive information? Is this a compliance requirement to not log it?

We use token-based authN/authZ, so the role contains the token. This PR will remove logging them and reduce log storage as we produce like 30 log/s just for logging role content (with a 500bytes token).

KannarFr · 2024-10-03T11:06:35Z

@lhotari I defined two configuration keys for broker and proxy. One anonymizes using SHA-256. The other put REDACTED. Is that ok?

pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java

pulsar-common/src/main/java/org/apache/pulsar/common/util/RoleObfuscator.java

lhotari · 2024-10-03T11:55:06Z

@lhotari I defined two configuration keys for broker and proxy. One anonymizes using SHA-256. The other put REDACTED. Is that ok?

@KannarFr good progress, I added review comments.

...ava/org/apache/pulsar/common/util/anonymizer/DefaultAuthenticationRoleLoggingAnonymizer.java

lhotari

Great work @KannarFr! 2 minor comments remaining. It would be good to document this as a PIP since most new features are documented that way. You can take a minimal approach for the PIP document and cover the relevant details that are provided in this PR.

...c/main/java/org/apache/pulsar/common/configuration/anonymizer/DefaultRoleAnonymizerType.java

...pache/pulsar/common/configuration/anonymizer/DefaultAuthenticationRoleLoggingAnonymizer.java

KannarFr · 2024-10-17T09:50:56Z

Great work @KannarFr! 2 minor comments remaining. It would be good to document this as a PIP since most new features are documented that way. You can take a minimal approach for the PIP document and cover the relevant details that are provided in this PR.

Done!

lhotari · 2024-10-30T12:43:32Z

Please split the PIP document to a separate PR. That's what is usually done. The PIP document PR would get merged before the implementation PR.

lhotari · 2024-10-30T12:45:08Z

When you create the PIP document PR, please make the file name pip-XXX.md since most PIP docs have such file name. You should check the mailing list and existing PRs to find a PIP number that isn't used yet. PIP-385 is already taken.

lhotari · 2025-04-23T11:08:49Z

@KannarFr Please remove the PIP document from this PR and rebase this PR. https://github.com/apache/pulsar/blob/master/pip/pip-402.md has been merged.

lhotari · 2025-04-23T12:33:19Z

@KannarFr There are 2 checkstyle errors:

Error:  src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:[56,8] (imports) UnusedImports: Unused import: org.apache.pulsar.common.util.anonymizer.DefaultRoleAnonymizerType.
Error:  src/main/java/org/apache/pulsar/common/configuration/anonymizer/DefaultAuthenticationRoleLoggingAnonymizer.java:[1] (javadoc) JavadocPackage: Missing package-info.java file.

Please also search the code base for any other locations where the role might be logged so that the solution is consistent. Just to ensure that no new log statements were added after you made the original code changes.

lhotari

LGTM

pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java

pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConfiguration.java

lhotari

Added some comments about the config

conf/broker.conf

conf/proxy.conf

conf/standalone.conf

lhotari

LGTM

codecov-commenter · 2025-09-23T14:32:57Z

Codecov Report

❌ Patch coverage is 65.95745% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.26%. Comparing base (0c6ba1c) to head (ba990a2).
⚠️ Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
...guration/anonymizer/DefaultRoleAnonymizerType.java	57.14%	9 Missing ⚠️
...er/DefaultAuthenticationRoleLoggingAnonymizer.java	62.50%	2 Missing and 1 partial ⚠️
...va/org/apache/pulsar/broker/service/ServerCnx.java	72.72%	3 Missing ⚠️
...rg/apache/pulsar/proxy/server/ProxyConnection.java	80.00%	1 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             master   #23386       +/-   ##
=============================================
+ Coverage     39.04%   74.26%   +35.21%     
- Complexity    13345    33642    +20297     
=============================================
  Files          1844     1904       +60     
  Lines        144311   148507     +4196     
  Branches      16730    17214      +484     
=============================================
+ Hits          56353   110283    +53930     
+ Misses        80460    29440    -51020     
- Partials       7498     8784     +1286

Flag	Coverage Δ
inttests	`26.51% <53.19%> (-0.26%)`	⬇️
systests	`22.68% <53.19%> (-0.16%)`	⬇️
unittests	`73.79% <65.95%> (+38.62%)`	⬆️

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

Files with missing lines	Coverage Δ
...org/apache/pulsar/broker/ServiceConfiguration.java	`98.18% <100.00%> (+3.13%)`	⬆️
...apache/pulsar/proxy/server/ProxyConfiguration.java	`95.51% <100.00%> (+16.15%)`	⬆️
...rg/apache/pulsar/proxy/server/ProxyConnection.java	`57.90% <80.00%> (+23.16%)`	⬆️
...er/DefaultAuthenticationRoleLoggingAnonymizer.java	`62.50% <62.50%> (ø)`
...va/org/apache/pulsar/broker/service/ServerCnx.java	`72.70% <72.72%> (+26.46%)`	⬆️
...guration/anonymizer/DefaultRoleAnonymizerType.java	`57.14% <57.14%> (ø)`

... and 1396 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.

…logging (#23386) (cherry picked from commit 7c8d3c9)

…logging (apache#23386)

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Oct 1, 2024

lhotari reviewed Oct 1, 2024

View reviewed changes

pulsar-common/src/main/java/org/apache/pulsar/common/util/RedactedRole.java Outdated Show resolved Hide resolved

KannarFr force-pushed the optionallyPreventRoleLogging branch from 7e68070 to 29687ce Compare October 3, 2024 11:04

lhotari reviewed Oct 3, 2024

View reviewed changes

lhotari reviewed Oct 4, 2024

View reviewed changes

...ava/org/apache/pulsar/common/util/anonymizer/DefaultAuthenticationRoleLoggingAnonymizer.java Show resolved Hide resolved

lhotari reviewed Oct 4, 2024

View reviewed changes

...ava/org/apache/pulsar/common/util/anonymizer/DefaultAuthenticationRoleLoggingAnonymizer.java Show resolved Hide resolved

KannarFr force-pushed the optionallyPreventRoleLogging branch from b4c3ff3 to 368d2e4 Compare October 15, 2024 14:02

lhotari reviewed Oct 15, 2024

View reviewed changes

...c/main/java/org/apache/pulsar/common/configuration/anonymizer/DefaultRoleAnonymizerType.java Show resolved Hide resolved

...pache/pulsar/common/configuration/anonymizer/DefaultAuthenticationRoleLoggingAnonymizer.java Outdated Show resolved Hide resolved

KannarFr force-pushed the optionallyPreventRoleLogging branch from 7af5587 to e153616 Compare October 17, 2024 09:50

github-actions bot added the PIP label Oct 17, 2024

KannarFr mentioned this pull request Nov 12, 2024

[feat][pip] PIP-402: Role Anonymizer for Pulsar Logging #23592

Merged

KannarFr force-pushed the optionallyPreventRoleLogging branch from 7023948 to 0050f45 Compare April 23, 2025 12:24

lhotari added the ready-to-test label Apr 23, 2025

KannarFr force-pushed the optionallyPreventRoleLogging branch from 55e19fe to 99381ad Compare September 22, 2025 22:14

lhotari changed the title ~~[improve][broker/proxy] Optionally prevent role/originalPrincipal logging~~ [improve][broker] PIP-402: Optionally prevent role/originalPrincipal logging Sep 22, 2025

lhotari approved these changes Sep 22, 2025

View reviewed changes

lhotari added the release/4.1.1 label Sep 22, 2025

optionally prevent role/originalPrincipal logging

7434d9e

lhotari reviewed Sep 22, 2025

View reviewed changes

pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java Show resolved Hide resolved

KannarFr force-pushed the optionallyPreventRoleLogging branch from 99381ad to 7434d9e Compare September 22, 2025 22:44

lhotari reviewed Sep 22, 2025

View reviewed changes

pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConfiguration.java Show resolved Hide resolved

add default conf

4347a9e

KannarFr force-pushed the optionallyPreventRoleLogging branch from 078e5c1 to 4347a9e Compare September 22, 2025 22:50

lhotari reviewed Sep 23, 2025

View reviewed changes

conf/broker.conf Outdated Show resolved Hide resolved

conf/proxy.conf Outdated Show resolved Hide resolved

conf/standalone.conf Outdated Show resolved Hide resolved

fixup! add default conf

ba990a2

lhotari approved these changes Sep 23, 2025

View reviewed changes

lhotari merged commit 7c8d3c9 into apache:master Sep 23, 2025
51 checks passed

lhotari pushed a commit that referenced this pull request Sep 23, 2025

[improve][broker] PIP-402: Optionally prevent role/originalPrincipal …

517cdce

…logging (#23386) (cherry picked from commit 7c8d3c9)

lhotari added release/4.1.2 cherry-picked/branch-4.1 and removed release/4.1.1 labels Sep 27, 2025

walkinggo pushed a commit to walkinggo/pulsar that referenced this pull request Oct 8, 2025

[improve][broker] PIP-402: Optionally prevent role/originalPrincipal …

e6b4c40

…logging (apache#23386)

[improve][broker] PIP-402: Optionally prevent role/originalPrincipal logging #23386

[improve][broker] PIP-402: Optionally prevent role/originalPrincipal logging #23386

Uh oh!

Conversation

KannarFr commented Oct 1, 2024

Motivation

Verifying this change

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

Documentation

Uh oh!

lhotari commented Oct 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

KannarFr commented Oct 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KannarFr commented Oct 3, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lhotari commented Oct 3, 2024

Uh oh!

Uh oh!

Uh oh!

lhotari left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

KannarFr commented Oct 17, 2024

Uh oh!

lhotari commented Oct 30, 2024

Uh oh!

lhotari commented Oct 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lhotari commented Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lhotari commented Apr 23, 2025

Uh oh!

lhotari left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lhotari left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lhotari left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Sep 23, 2025

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lhotari commented Oct 1, 2024 •

edited

Loading

KannarFr commented Oct 2, 2024 •

edited

Loading

lhotari commented Oct 30, 2024 •

edited

Loading

lhotari commented Apr 23, 2025 •

edited

Loading