[improve][broker/proxy] Optionally prevent role/originalPrincipal logging

[KannarFr]

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
• [x] doc-not-needed
• [ ] doc-complete

[lhotari]

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?

[KannarFr]

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]

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

[lhotari]

@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.

[KannarFr]

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]

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]

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]

@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]

@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.

[codecov-commenter]

Codecov Report

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

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

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%)	:arrow_down:
systests	22.68% <53.19%> (-0.16%)	:arrow_down:
unittests	73.79% <65.95%> (+38.62%)	:arrow_up:

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%)	:arrow_up:
...apache/pulsar/proxy/server/ProxyConfiguration.java	95.51% <100.00%> (+16.15%)	:arrow_up:
...rg/apache/pulsar/proxy/server/ProxyConnection.java	57.90% <80.00%> (+23.16%)	:arrow_up:
...er/DefaultAuthenticationRoleLoggingAnonymizer.java	62.50% <62.50%> (ø)
...va/org/apache/pulsar/broker/service/ServerCnx.java	72.70% <72.72%> (+26.46%)	:arrow_up:
...guration/anonymizer/DefaultRoleAnonymizerType.java	57.14% <57.14%> (ø)

... and 1396 files with indirect coverage changes

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.