KAFKA-4055: System tests for secure quotas #1860
Conversation
|
The implementation for this feature is in KAFKA-3492 (not yet committed). |
rajinisivaram
force-pushed
the
KAFKA-4055
branch
from
01497fd
to
6b98311
Compare
rajinisivaram
force-pushed
the
KAFKA-4055
branch
from
6b98311
to
e2b5d0c
Compare
| self.producer_quota = 3750000 | ||
| self.consumer_quota = 3000000 | ||
| self.configure_quota(kafka, self.producer_quota, self.consumer_quota, ['clients', self.client_id]) | ||
| self.configure_quota(kafka, QuotaConfig.LARGE_QUOTA, QuotaConfig.LARGE_QUOTA, ['clients', None]) |
There was a problem hiding this comment.
Is None used to set the default quota? If so, should we handle that specially in configure_quota?
There was a problem hiding this comment.
@junrao Yes, None is used to set default quota. configure_quota uses the method entity_name_opt to set the option for quota entity name. That uses --entity-default to set default quota to handle None.
| self.consumer_quota = 3000000 | ||
| self.configure_quota(kafka, self.producer_quota, self.consumer_quota, ['users', QuotaConfig.USER_PRINCIPAL]) | ||
| self.configure_quota(kafka, QuotaConfig.LARGE_QUOTA, QuotaConfig.LARGE_QUOTA, ['users', None]) | ||
| self.configure_quota(kafka, QuotaConfig.LARGE_QUOTA, QuotaConfig.LARGE_QUOTA, ['clients', self.client_id]) |
There was a problem hiding this comment.
Do we need this since users always takes precedence?
| self.producer_quota = 2500000 | ||
| self.consumer_quota = 2000000 | ||
| self.configure_quota(kafka, self.producer_quota, self.consumer_quota, ['users', None]) | ||
| self.configure_quota(kafka, QuotaConfig.LARGE_QUOTA, QuotaConfig.LARGE_QUOTA, ['clients', None]) |
There was a problem hiding this comment.
Do we need this since users always takes precedence?
| self.configure_quota(kafka, self.producer_quota, self.consumer_quota, ['users', QuotaConfig.USER_PRINCIPAL, 'clients', self.client_id]) | ||
| self.configure_quota(kafka, QuotaConfig.LARGE_QUOTA, QuotaConfig.LARGE_QUOTA, ['users', QuotaConfig.USER_PRINCIPAL, 'clients', None]) | ||
| self.configure_quota(kafka, QuotaConfig.LARGE_QUOTA, QuotaConfig.LARGE_QUOTA, ['users', None]) | ||
| self.configure_quota(kafka, QuotaConfig.LARGE_QUOTA, QuotaConfig.LARGE_QUOTA, ['clients', self.client_id]) |
There was a problem hiding this comment.
Actually, I am wondering do we need the above 3 lines at all?
There was a problem hiding this comment.
@junrao You are right, the three lines above are not strictly required. They verify that <user, client> quota does indeed have the highest precedence. The lines with lower precedence (which set a high quota) are just there to ensure that precedence is correctly implemented.
I can remove them if the code is confusing since precedence is also tested in unit tests. Let me know what you think.
|
@rajinisivaram : Thanks for the patch. Were you able to run the system tests with this patch? |
|
@junrao Thank you for the review. Yes, I ran the system tests yesterday and the test results are here: https://jenkins.confluent.io/job/system-test-kafka-branch-builder/550/console. There were 4 unrelated failures. The rest including the quota tests have passed. |
|
@rajinisivaram : Thanks for the patch. LGTM |
Fix existing client-id quota test which currently don't configure quota overrides correctly. Add new tests for user and (user, client-id) quota overrides and default quotas. Author: Rajini Sivaram <rajinisivaram@googlemail.com> Reviewers: Jun Rao <junrao@gmail.com> Closes #1860 from rajinisivaram/KAFKA-4055 (cherry picked from commit c0a62b7) Signed-off-by: Jun Rao <junrao@gmail.com>
Fix existing client-id quota test which currently don't configure quota overrides correctly. Add new tests for user and (user, client-id) quota overrides and default quotas.