Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add feature to request consumer group information #435

Open
wants to merge 35 commits into
base: master
Choose a base branch
from

Conversation

Ledostuff
Copy link

@Ledostuff Ledostuff commented Jun 4, 2018

Add feature to request consumer group information(start, end offsets, lags, consumer-instance-id, consumer-instance-ip)
Added rest paths:

  1. /groups - returns consumer groups list
[{"groupId":"perf-consumer-46894","coordinator":{"host":"{some_host_name}","port":9496}}]
  1. /groups/{groupId}/topics - returns topics for wich group is subscribed
[{"name":"1"}]
  1. /groups/{groupId}/topics/{topic} - returns consumer group information(start, end offsets, lags, consumer-instance-id, consumer-instance-ip...) restricted by spesified topic
{"topicPartitionList":[{"consumerId":"consumer-1-88792db6-99a2-4064-aad2-38be12b32e88","consumerIp":"/{some_ip}","topicName":"1","partitionId":0,"currentOffset":15338734,"lag":113812,"endOffset":15452546},{"consumerId":"consumer-1-88792db6-99a2-4064-aad2-38be12b32e88","consumerIp":"/{some_ip}","topicName":"1","partitionId":1,"currentOffset":15753823,"lag":136160,"endOffset":15889983},{"consumerId":"consumer-1-88792db6-99a2-4064-aad2-38be12b32e88","consumerIp":"/{some_ip}","topicName":"1","partitionId":2,"currentOffset":15649419,"lag":133052,"endOffset":15782471}],"topicPartitionCount":3,"coordinator":{"host":"{coordinator_host_name}","port":9496}}
  1. /groups/{groupId}/partitions - returns consumer group information(start, end offsets, lags, consumer-instance-id, consumer-instance-ip...) for all topics for wich group is subscribed
{"topicPartitionList":[{"consumerId":"consumer-1-88792db6-99a2-4064-aad2-38be12b32e88","consumerIp":"/{some_ip}","topicName":"1","partitionId":0,"currentOffset":15338734,"lag":113812,"endOffset":15452546},{"consumerId":"consumer-1-88792db6-99a2-4064-aad2-38be12b32e88","consumerIp":"/{some_ip}","topicName":"1","partitionId":1,"currentOffset":15753823,"lag":136160,"endOffset":15889983},{"consumerId":"consumer-1-88792db6-99a2-4064-aad2-38be12b32e88","consumerIp":"/{some_ip}","topicName":"1","partitionId":2,"currentOffset":15649419,"lag":133052,"endOffset":15782471}],"topicPartitionCount":3,"coordinator":{"host":"{coordinator_host_name}","port":9496}}

Add unit and integration tests

@ghost
Copy link

ghost commented Jun 4, 2018

It looks like @Ledostuff hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement here.

Once you've signed reply with [clabot:check] to prove it.

Appreciation of efforts,

clabot

@Ledostuff
Copy link
Author

[clabot:check]

@ghost
Copy link

ghost commented Jun 4, 2018

@confluentinc It looks like @Ledostuff just signed our Contributor License Agreement. 👍

Always at your service,

clabot

@Ledostuff Ledostuff force-pushed the feature/consumer_group_info branch from 2591e63 to bddeb50 Compare July 6, 2018 06:13
@thomaskwscott thomaskwscott requested review from ewencp and ijuma July 6, 2018 08:47
@Ledostuff Ledostuff force-pushed the feature/consumer_group_info branch from bddeb50 to 2454312 Compare August 3, 2018 10:36
@ewencp ewencp requested a review from a team September 13, 2018 03:25
@gAmUssA
Copy link

gAmUssA commented Sep 13, 2018

@Ledostuff PR build is failing. Could you check?

@cmccabe
Copy link
Member

cmccabe commented Sep 13, 2018

The build failed:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.6.1:testCompile (default-testCompile) on project kafka-rest: Compilation failure
[ERROR] /home/jenkins/workspace/entinc-pr_kafka-rest_PR-435-42EK4ZKFX7WUDVKYQNLWCJBUA3J5IBRM4CNHID3FE32PTTTFMBRQ/kafka-rest/src/test/java/io/confluent/kafkarest/TestUtils.java:[263,34] cannot find symbol
[ERROR]   symbol:   method createConsumerProperties(java.lang.String,java.lang.String,java.lang.String,int)
[ERROR]   location: class kafka.utils.TestUtils
[ERROR] -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException
[ERROR] 
[ERROR] After correcting the problems, you can resume the build with the command
[ERROR]   mvn <goals> -rf :kafka-rest

@cmccabe
Copy link
Member

cmccabe commented Sep 13, 2018

This PR shouldn't need to depend on ZooKeeper. We should be able to use the public AdminClient.

What's the security model here? How do we decide who can and cannot access admin APIs?

@Ledostuff
Copy link
Author

@gAmUssA Thank you for review. I`ll fix build and push changes asap.

@Ledostuff
Copy link
Author

@cmccabe Thank you for review.
I`ll fix build and push changes asap.
Zookeeper needs to get broker information. I mean security protocol in EndPoint class.
Do you mean I should move method from AdminClientWrapper to another place?
Could you explain in detail your sentence about security model?

@Ledostuff Ledostuff force-pushed the feature/consumer_group_info branch from 2454312 to 5d2c237 Compare September 14, 2018 06:13
@cmccabe
Copy link
Member

cmccabe commented Sep 14, 2018

Zookeeper needs to get broker information. I mean security protocol in EndPoint class.

Why is this information needed for REST proxy? If it is needed, then we should at least think about providing it through the AdminClient.

Could you explain in detail your sentence about security model?

This PR seems to give rest-proxy users a lot of powerful abilities to access administrative APIs. I'm not sure that rest-proxy is the right place to manage the Kafka cluster.

If we did want to do this we would need to figure out how to give certain users the ability to use the admin APIs and block everyone else. For example, users should not be able to list groups unless they have DESCRIBE permission on the group, or DESCRIBE permission on the Cluster resource.

To be honest, I'm not sure this is the right direction to go-- at least not without a lot more discussion.

@Ledostuff
Copy link
Author

Ledostuff commented Sep 19, 2018

@cmccabe Thank you for the answer.

This PR seems to give rest-proxy users a lot of powerful abilities to access administrative APIs. I'm not sure that rest-proxy is the right place to manage the Kafka cluster.

We are only show information about groups who consumes messges from clusters topics. We don`t try to change settings or do any other work with cluster.

If we did want to do this we would need to figure out how to give certain users the ability to use the admin APIs and block everyone else. For example, users should not be able to list groups unless they have DESCRIBE permission on the group, or DESCRIBE permission on the Cluster resource.

I don't see any problem about that. Because we allow users to consume messages through the rest-proxy. But why do we can't show information about consumer groups? Even if we allow users DESCRIBE permission on groups or DESCRIBE permission on the Cluster resource .
I'm open for discussion this PR)

Why is this information needed for REST proxy? If it is needed, then we should at least think about providing it through the AdminClient.

I didn't found similar method in the API of AdminClient. If you show me alternative method to get information about brokers in cluster with security protocol, I'll repair code in this PR gladly.)

@Ledostuff Ledostuff force-pushed the feature/consumer_group_info branch 2 times, most recently from fcaa5d5 to cc021ea Compare September 25, 2018 04:57
@stanislavkozlovski
Copy link
Contributor

Hey @Ledostuff, thanks for the PR!

I haven't had time to give this a proper look yet but I wanted to ask - do we need ZooKeeper to access the cluster information only ( {"brokerId":0,"endPointList":[{"protocol":"SSL","host":"127.0.0.1","port":9092}]}) ?
In regards to that, I feel it would be best if we do not re-introduce a ZooKeeper dependancy in the proxy.

The consumer information should be fetchable through the adminClient only, correct?
To me, it makes sense to expose consumer information. I think endpoints 2., 3., 4. and 5. will be useful.

There is a question of whether that information should be limited to consumers managed by this proxy instance or consumers in the Kafka Cluster.
As @cmccabe said, permissions are a valid concern:

For example, users should not be able to list groups unless they have DESCRIBE permission on the group, or DESCRIBE permission on the Cluster resource.
As far as I know, in the current model we allow a consumer to be created and join any arbitrary group present in the Kafka cluster. That means exposing consumer group information for the whole cluster would be consistent with the security model already present.

@Ledostuff Ledostuff force-pushed the feature/consumer_group_info branch from cc021ea to 870947f Compare November 19, 2018 10:52
@rootex-
Copy link

rootex- commented Nov 22, 2018

Hi @stanislavkozlovski,

"in the current model we allow a consumer to be created and join any arbitrary group present"

No, we don't. This is only valid for the case where authentication and acl's are off OR we elevate kafka-rest principal's resource permissions like READ/DESCRIBE on cluster/group/topic, depending on the operation type we want. We are free to choose which ACLs to give to kafka-rest's principal, should consumer groups be exposed or not. That is, users are free to choose their own security model.

users should not be able to list groups unless they have DESCRIBE permission on the group, or DESCRIBE permission on the Cluster resource.

That's right, unless we provide DESCRIBE permission on the Cluster resource.

Viewing consumer group offsets or listing consumer groups is just a feature that can be enabled or disabled, according to a security model adapted by the user.

@stanislavkozlovski
Copy link
Contributor

Hi @rootex-,

That's true. I agree with exposing consumer group information.

@analytik
Copy link

Not to be annoying, but... is there still interest in this? I'd welcome having a list of consumer groups available via REST proxy.

@rootex-
Copy link

rootex- commented Sep 20, 2019

Not to be annoying, but... is there still interest in this? I'd welcome having a list of consumer groups available via REST proxy.

Yes, there is still a huge interest in this feature.
So much useful work done and no one have ever reviewed this stuff.
We are also sick and tired of endless patching Kafka Rest in Sberbank.

Could we merge this pull request?

@Ledostuff Ledostuff force-pushed the feature/consumer_group_info branch from 870947f to 21627e9 Compare September 25, 2019 05:01
@Ledostuff
Copy link
Author

Hello there! Please review this PR. Thanks)

@Ledostuff
Copy link
Author

Hi @stanislavkozlovski,
Please, review my changes. Sorry for the delay)

@Ledostuff Ledostuff force-pushed the feature/consumer_group_info branch from 9c1f5b7 to 016d0fe Compare January 9, 2020 07:11
@Ledostuff
Copy link
Author

Hello, @stanislavkozlovski :)
Review my changes, please.
Check my suggestions about your last comment.

@Ledostuff
Copy link
Author

Hellooo. One month has already passed since last comment. PLease, review this pull-request.

Copy link

cla-assistant bot commented Sep 12, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants