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 support of entity-default kafka quotas #267

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

azhurbilo
Copy link

previously there is no ability to create default "kafka_quotas" for any user/client

https://kafka.apache.org/26/documentation.html#quotas

It is possible to set default quotas for each (user, client-id), user or client-id group by specifying --entity-default option instead of --entity-name

https://cwiki.apache.org/confluence/display/KAFKA/KIP-257+-+Configurable+Quota+Management

For default quotas, an empty string is returned.

@azhurbilo
Copy link
Author

@luisfmcalado @Mongey could you review please? and how to trigger pipeline with github actions here?

@Mongey
Copy link
Owner

Mongey commented Jun 22, 2022

@azhurbilo Thanks for the PR.

I wonder if we should change this so that entity_name can be omitted, which will use the entity default, rather than use an empty string. What do you think?

@azhurbilo
Copy link
Author

@azhurbilo Thanks for the PR.

I wonder if we should change this so that entity_name can be omitted, which will use the entity default, rather than use an empty string. What do you think?

in this case existed code/tests about "quotas" should be refactored and be more complex as resource ID depends on it and most of functions have "entity_name" param.
@luisfmcalado what do you think, do we need leave AS IS in this MR now or better add ability to fully remove "entity_name" ?

@azhurbilo
Copy link
Author

@luisfmcalado UP, do you agree?

@Mongey or maybe you can implement omitted entity_name yourself in more optimal way to compare with this MR?

@palson
Copy link

palson commented Sep 20, 2022

@azhurbilo
Sorry if this is out of place.
perhaps this MP should also add support for both client and user quotas in the same resource.
The current implementation allows you to set either client or user quota, but not both.

analogous to kafka scripts:

kafka-configs \
  --bootstrap-server localhost:9092 --alter \
  --add-config 'producer_byte_rate=1024' \
  --entity-type users --entity-name alice \
  --entity-type clients --entity-name my-app

kafka-configs \
  --bootstrap-server localhost:9092 --alter \
  --add-config 'producer_byte_rate=1024' \
  --entity-type users --entity-name alice \
  --entity-type clients --entity-default

@azhurbilo
Copy link
Author

The current implementation allows you to set either client or user quota, but not both.

@palson not sure that it's good idea implement support for both entity-type at the same time.

There is no problem with this PR to create 2 separate resources to control quotas for "client" and "users" without any conflict, do you agree?

resource "kafka_quota" "clients_default" {
entity_name = ""
entity_type = "client-id"
config = {
  "consumer_byte_rate" = 10000
  "producer_byte_rate" = 10000
}
}

resource "kafka_quota" "users_default" {
entity_name = ""
entity_type = "user"
config = {
  "consumer_byte_rate" = 10000
  "producer_byte_rate" = 10000
}
}

@palson
Copy link

palson commented Sep 25, 2022

@azhurbilo I know that implementing two features in the same PR is not the best approach, but since you've already reviewed the quota logic, this would be a good chance to add support for both types as well.

As for your example - it won't work. According to the quota precedense order the quota rule {specific user, <default client>} matches requests first and the client's quouta will never work.

Because the first one resource creates the /config/users/<default>/clients/<client-id> rule type and the second one - /config/users/<user>/clients/<default>.
To create the /config/users/<user>/clients/<client-id> rule, both - client id and user must be in the same create quota request.

@azhurbilo
Copy link
Author

@palson yes, got it. But If we want support "/config/users/*/clients/*" path we need brake compatibility of kafka_quota resource as now it supports only "/config/users/*" or "/config/clients/*" and I can not imagine how to add support for both without breaking changes

@Dugong42
Copy link

Dugong42 commented Oct 12, 2023

Hello @Mongey, it's been a while since the last activity on this subject.
If there are no immediate plans to implement the entity tuples of users/clients, could the "default" entity-name feature be merged ? Thanks !

@Dugong42
Copy link

Dugong42 commented Oct 12, 2023

@azhurbilo I'm not sure about the details of the breaking changes.

In regards to the resource format, what about adding a new entity_tuple parameter like so ?

resource "kafka_quota" "users_default" {
  entity_tuple = {
    client_id = "<clientname>"
    user = "<username>"
  }
  config = {
    "consumer_byte_rate" = 10000
    "producer_byte_rate" = 10000
  }
}

I think the entity_name and entity_type parameters could be made optional without introducing a breaking change.
There could also be a new kind of resource like kafka_quota_tuple if the change to kafka_quota causes to many issues.

@azhurbilo
Copy link
Author

azhurbilo commented Oct 18, 2023

@Dugong42 absolutely agree with your last message proposal! but I stopped working in company where we use Kafka, maybe someone else can implement "kafka_quota_tuple" or "kafka_quotas" to support /config/users/*/clients/*

@dstrates
Copy link

@Mongey what changes are required to implement this feature? Happy to raise a PR if you can elaborate on the requirements

cc: @palson @Dugong42

@LeoQuote
Copy link

LeoQuote commented Aug 5, 2024

@dstrates I would also want this feature, it seems the author want it implemented as entity_default instead of an empty entity name to match the command line more.

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.

6 participants