[improve][pip] PIP-347: add role field in consumer's stat

[thetumbled]

Motivation

pip-347
implementation pr：#22562

Modifications

Verifying this change

• Make sure that the change passes the CI checks.

(Please pick either of the following options)

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

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

• Added integration tests for end-to-end deployment with large payloads (10MB)
• Extended integration test for recovery after broker failure

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

Matching PR in forked repository

PR in forked repository:

[thetumbled]

PTAL, thanks. @BewareMyPower @lhotari @codelipenghui @poorbarcode @Technoboy-

[michaeljmarshall]

Thanks for your PIP. I agree that there is a gap here, but I think the solution requires a more nuanced approach to accommodate existing use cases.

[nodece]

Leaving aside the issue above.

It looks like you want to add role to the consumer stats, that is a good idea to track the relationship between subscriptions and users.

There are times when the role is more sensitive information and I would recommend adding a configuration to expose the role.

[thetumbled]

May be we can prioritize reaching consensus on this PIP? We do need this pip to handle the problem we meet, anyone who use Subscription Permission Control Mechanism will also need this.
@michaeljmarshall @nodece @dao-jun @codelipenghui @BewareMyPower @Technoboy-

[dao-jun]

@thetumbled After @michaeljmarshall think it's OK, you can sent a vote thread to the mail list

[eolivelli]

I think that this PIP is telling too much about some kind of security vulnerability and I would shrink it a lot.
You should mention only the need for administrators to bind a consumer to a role, that is definitely useful.

Regarding the auto creation of subscriptions, we should tackle the problem as a separate work, and not a PIP

[thetumbled]

I think that this PIP is telling too much about some kind of security vulnerability and I would shrink it a lot. You should mention only the need for administrators to bind a consumer to a role, that is definitely useful.

Regarding the auto creation of subscriptions, we should tackle the problem as a separate work, and not a PIP

Agree, i have updated the motivation, PTAL, thanks. @eolivelli @nodece

[nodece]

Good point.

[dao-jun]

Leaving aside the issue above.

It looks like you want to add role to the consumer stats, that is a good idea to track the relationship between subscriptions and users.

There are times when the role is more sensitive information and I would recommend adding a configuration to expose the role.

@nodece I don't understand why role name can be sensitive.
It's not accessKey or username or token or something else, it is just a role name, users cannot accessed in by the role name at all. Like us, we are Software engineer, is Software engineer sensitive?
The PIP is a simple change, let's keep it simple, I don't think add a exposeRole param is reasonable.

/cc @eolivelli

[nodece]

In other words, I just don't want to expose the role to the regular users.

[dao-jun]

@nodece I think it doesn't matter, expose role name will not affect anything

[thetumbled]

WDYT, should we expose it to regular users(not everyone actually, but those who own the lookup permission of this topic)? @eolivelli @lhotari @codelipenghui @Technoboy- @michaeljmarshall

[codelipenghui]

There are times when the role is more sensitive information and I would recommend adding a configuration to expose the role.

It should be fine since we already exposed the sub name, consumer name and more information for users who has LOOKUP permission with default Pulsar authorization. And Pulsar can also support fine-grained access control by authorization plugin.

[eolivelli]

+1

[dao-jun]

PIP approved, merging...