You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/05/10 00:28:41 UTC
[GitHub] [kafka] jsancio opened a new pull request #8639: KAFKA-9962: Make the auth operations fields ignorable
jsancio opened a new pull request #8639:
URL: https://github.com/apache/kafka/pull/8639
*More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.*
*Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.*
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma edited a comment on pull request #8639: KAFKA-9962: Make the auth operations fields ignorable
Posted by GitBox <gi...@apache.org>.
ijuma edited a comment on pull request #8639:
URL: https://github.com/apache/kafka/pull/8639#issuecomment-626483766
The system test `client_compatibility_features_test.py` (also see `ClientCompatibilityTest.java`).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jsancio commented on pull request #8639: KAFKA-9962: Make the auth operations fields ignorable
Posted by GitBox <gi...@apache.org>.
jsancio commented on pull request #8639:
URL: https://github.com/apache/kafka/pull/8639#issuecomment-626264546
>
>
> Thanks for the PR. I think we should update the admin client compatibility test to include this.
@ijuma, I am not sure I follow. Do you mean these kind of tests in `.../admin/KafkaAdminClientTest.java`?
```
public void testDescribeClientQuotas() throws Exception {
try (AdminClientUnitTestEnv env = mockClientEnv()) {
env.kafkaClient().setNodeApiVersions(NodeApiVersions.create());
...
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jsancio commented on pull request #8639: KAFKA-9962: Make the auth operations fields ignorable
Posted by GitBox <gi...@apache.org>.
jsancio commented on pull request #8639:
URL: https://github.com/apache/kafka/pull/8639#issuecomment-626254232
@omkreddy When we merge this we need to cherry pick this to the versions enumerated in the Jira.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #8639: KAFKA-9962: Make the auth operations fields ignorable
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #8639:
URL: https://github.com/apache/kafka/pull/8639#issuecomment-626256222
Thanks for the PR. I think we should update the admin client compatibility test to include this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe edited a comment on pull request #8639: KAFKA-9962: Make the auth operations fields ignorable
Posted by GitBox <gi...@apache.org>.
cmccabe edited a comment on pull request #8639:
URL: https://github.com/apache/kafka/pull/8639#issuecomment-637823813
The KIP clearly specifies that `describeCluster` will throw a UVE if `authorizedOperations` is not supported, yet is requested:
> When the AdminClient is talking to a broker which does not support KIP-430, it will fill in either null or UnsupportedVersionException for the returned ACL operations fields in objects. For example, ConsumerGroupDescription#authorizedOperations will be null if the broker did not supply this information. DescribeClusterResult#authorizedOperations will throw an UnsupportedVersionException if the broker did not supply this information.
The discussion above was about `describeConsumerGroups`, but the PR seems to be changing both.
> [UVEs are] not fine grain enough for all cases. There are already a few reasons why the call may throw an UnsupportedVersionException.
In the future, KIP-584 will make new features (that can't be faked by old brokers) more discoverable to clients. In the meantime, KSQL can have a fallback loop where they turn this thing off when they get a UVE and retry.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jsancio commented on pull request #8639: KAFKA-9962: Make the auth operations fields ignorable
Posted by GitBox <gi...@apache.org>.
jsancio commented on pull request #8639:
URL: https://github.com/apache/kafka/pull/8639#issuecomment-637749796
> Well, the client can handle the `UnsupportedVersionException and stop requesting the authorized operations.
This is not fine grain enough for all cases. There are already a few reasons why the call may throw an `UnsupportedVersionException`. This is one example:
https://github.com/jsancio/kafka/blob/kafka-9962-ignorable-auth-ops/clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java#L1681-L1687
I have added compatibility tests that verify that the client return `null` when describe topics against a cluster version that doesn't support this feature.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jsancio commented on pull request #8639: KAFKA-9962: Make the auth operations fields ignorable
Posted by GitBox <gi...@apache.org>.
jsancio commented on pull request #8639:
URL: https://github.com/apache/kafka/pull/8639#issuecomment-638286953
The bug report in [KAFKA-9962](https://issues.apache.org/jira/browse/KAFKA-9962) is for `describeCluster`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jsancio commented on pull request #8639: KAFKA-9962: Make the auth operations fields ignorable
Posted by GitBox <gi...@apache.org>.
jsancio commented on pull request #8639:
URL: https://github.com/apache/kafka/pull/8639#issuecomment-636256515
> In the case described in this JIRA, KSQL is requesting authorized operations from a broker that doesn't support it. What is it going to do if we don't throw an exception and quietly ignore the request to provide authorized operations?
I don't know @rajinisivaram. Based on what the `Admin` client exposes there is no way for the client/KSQL to determine if the server version supports this feature, no?
My understanding is that KSQL is setting this to true if some broker config is set. That broker config can be set even if the broker doesn't support this feature.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe edited a comment on pull request #8639: KAFKA-9962: Make the auth operations fields ignorable
Posted by GitBox <gi...@apache.org>.
cmccabe edited a comment on pull request #8639:
URL: https://github.com/apache/kafka/pull/8639#issuecomment-637823813
**The KIP clearly specifies that `describeCluster` will throw a UVE in this case:
bq. When the AdminClient is talking to a broker which does not support KIP-430, it will fill in either null or UnsupportedVersionException for the returned ACL operations fields in objects. For example, ConsumerGroupDescription#authorizedOperations will be null if the broker did not supply this information. DescribeClusterResult#authorizedOperations will throw an UnsupportedVersionException if the broker did not supply this information.
The discussion above was about `describeConsumerGroups`, but the PR seems to be changing both.
> [UVEs are] not fine grain enough for all cases. There are already a few reasons why the call may throw an UnsupportedVersionException.
In the future, KIP-584 will make new features (that can't be faked by old brokers) more discoverable to clients. In the meantime, KSQL can have a fallback loop where they turn this thing off when they get a UVE and retry.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jsancio commented on pull request #8639: KAFKA-9962: Make the auth operations fields ignorable
Posted by GitBox <gi...@apache.org>.
jsancio commented on pull request #8639:
URL: https://github.com/apache/kafka/pull/8639#issuecomment-636255975
> The system test client_compatibility_features_test.py (also see ClientCompatibilityTest.java).
Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jsancio closed pull request #8639: KAFKA-9962: Make the auth operations fields ignorable
Posted by GitBox <gi...@apache.org>.
jsancio closed pull request #8639:
URL: https://github.com/apache/kafka/pull/8639
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jsancio edited a comment on pull request #8639: KAFKA-9962: Make the auth operations fields ignorable
Posted by GitBox <gi...@apache.org>.
jsancio edited a comment on pull request #8639:
URL: https://github.com/apache/kafka/pull/8639#issuecomment-626254232
@omkreddy When we merge this, we need to cherry pick this to the versions enumerated in the Jira.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #8639: KAFKA-9962: Make the auth operations fields ignorable
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #8639:
URL: https://github.com/apache/kafka/pull/8639#issuecomment-626483766
The system test `client_compatibility_features_test.py`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on pull request #8639: KAFKA-9962: Make the auth operations fields ignorable
Posted by GitBox <gi...@apache.org>.
cmccabe commented on pull request #8639:
URL: https://github.com/apache/kafka/pull/8639#issuecomment-636286034
@rajinisivaram wrote:
> @cmccabe found a compatibility issue with the KIP and made some changes. I think the change was to allow ConsumerGroupDescription#authorizedOperations to return null instead of throwing an exception since there wasn't an easy way to throw an exception for this case.
Hmm. I think the JIRA you're thinking of is KAFKA-8158. The bug was that new client + old server was silently returning an empty set of auth operations, rather than throwing an exception OR returning null. My change was intended to fix that, not to change the behavior from exception to null (or vice versa).
@jsancio wrote:
> I don't know @rajinisivaram. Based on what the Admin client exposes there is no way for the client/KSQL to determine if the server version supports this feature, no?
Well, the client can handle the `UnsupportedVersionException and stop requesting the authorized operations.
Anyway, if you folks feel strongly that it should silently return null rather than throwing a UVE, I'm OK with this change. To be clear, the null (rather than empty) set does let the client know for sure that the authorized operations request was not supported.
I suppose the fact that we did return null for this in 2.3 and 2.4 rather than a UVE is a strong argument for doing it that way in 2.6 and beyond. Let's just update the KIP and also bump the KIP thread as well. I agree with @ijuma that we should also change the compatibility tests for this in ducktape.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on pull request #8639: KAFKA-9962: Make the auth operations fields ignorable
Posted by GitBox <gi...@apache.org>.
cmccabe commented on pull request #8639:
URL: https://github.com/apache/kafka/pull/8639#issuecomment-637823813
**The KIP clearly specifies that `describeCluster` will throw a UVE in this case:
bq. When the AdminClient is talking to a broker which does not support KIP-430, it will fill in either null or UnsupportedVersionException for the returned ACL operations fields in objects. For example, ConsumerGroupDescription#authorizedOperations will be null if the broker did not supply this information. DescribeClusterResult#authorizedOperations will throw an UnsupportedVersionException if the broker did not supply this information.
The discussion above was about `describeConsumerGroups`, but the PR seems to be changing both.
bq. [UVEs are] not fine grain enough for all cases. There are already a few reasons why the call may throw an UnsupportedVersionException.
In the future, KIP-584 will make new features (that can't be faked by old brokers) more discoverable to clients. In the meantime, KSQL can have a fallback loop where they turn this thing off when they get a UVE and retry.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jsancio commented on pull request #8639: KAFKA-9962: Make the auth operations fields ignorable
Posted by GitBox <gi...@apache.org>.
jsancio commented on pull request #8639:
URL: https://github.com/apache/kafka/pull/8639#issuecomment-635524753
> One observation is, we were not throwing exception in 2.3 and 2.4 releases. This was enforced in #7710 (2.5 release)
Thanks @omkreddy and sorry for the delay response.
It looks like ignoring the field was the old behavior. This PR makes it explicit in the message schema.
Adding the system tests @ijuma suggested.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe edited a comment on pull request #8639: KAFKA-9962: Make the auth operations fields ignorable
Posted by GitBox <gi...@apache.org>.
cmccabe edited a comment on pull request #8639:
URL: https://github.com/apache/kafka/pull/8639#issuecomment-637823813
The KIP clearly specifies that `describeCluster` will throw a UVE if `authorizedOperations` is not supported, yet is requested:
bq. When the AdminClient is talking to a broker which does not support KIP-430, it will fill in either null or UnsupportedVersionException for the returned ACL operations fields in objects. For example, ConsumerGroupDescription#authorizedOperations will be null if the broker did not supply this information. DescribeClusterResult#authorizedOperations will throw an UnsupportedVersionException if the broker did not supply this information.
The discussion above was about `describeConsumerGroups`, but the PR seems to be changing both.
> [UVEs are] not fine grain enough for all cases. There are already a few reasons why the call may throw an UnsupportedVersionException.
In the future, KIP-584 will make new features (that can't be faked by old brokers) more discoverable to clients. In the meantime, KSQL can have a fallback loop where they turn this thing off when they get a UVE and retry.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org