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

[thetumbled]

Motivation

pip-347 implementation pr：https://github.com/apache/pulsar/pull/22562

Modifications

Verifying this change

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

Matching PR in forked repository

PR in forked repository:

[thetumbled]

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

[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

[thetumbled]

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.

Can't we expose the role info to the producer/consumer of this topic? It is not a token that can be stolen, it is only a name. Someone else peek it can't use this info to do any bad things, right? If we regard the role name as sensitive, should we regard the ip address as sensitive?

If we expose the role info to the producer/consumer of this topic, we can reduce the workload of the administrator. During the operation and maintenance process, there are many users asking me to find out the consumer of this topic and nofity them about the business change. Administrators can call bin/pulsar-admin topics partitioned-stats to find out the ip of consumers, but no role info. So this pip can also help to solve such kind of problem. And if we allow the producer to do such kind of work, we can be free from such troublesome work -- we need to communicate with users and find out the cluster connect string and conscruct the command.

For example, we can use jack's token (the producer of this topic) to get the info, and know that it is a consumer created by fengwenzhi.max.

"consumers" : [ {
            "role" : "fengwenzhi.max",
            "msgRateOut" : 0.0,
            "msgThroughputOut" : 0.0,
            "bytesOutCounter" : 0,
            "msgOutCounter" : 0,
            "msgRateRedeliver" : 0.0,
            "messageAckRate" : 0.0,
            "chunkedMessageRate" : 0.0,
            "consumerName" : "a27d4",
            "availablePermits" : 100,
            "unackedMessages" : 0,
            "avgMessagesPerEntry" : 0,
            "blockedConsumerOnUnackedMsgs" : false,
            "lastAckedTimestamp" : 0,
            "lastConsumedTimestamp" : 0,
            "metadata" : { },
            "address" : "/XXX:34308",
            "connectedSince" : "2024-05-03T16:10:50.725405+08:00",
            "clientVersion" : "2.9.2"
          } ]

WDYT. @nodece @dao-jun @eolivelli @lhotari @codelipenghui @BewareMyPower

[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

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

[dao-jun]

PIP approved, merging...