Conversation
| inReadLock(lock) { | ||
| aclCache | ||
| .filterKeys(aclResource => AclUtils.matchResource(aclResource.toJava, filter)) | ||
| .flatMap(_._2.acls) |
There was a problem hiding this comment.
@junrao, if you can suggest an alternative to this flatmap(_._2.acls) I'm happy to plug it in, but my Scala's a little rusty.
There was a problem hiding this comment.
You can just do .flatMap{ case(resource, versionedAcls) => versionedAcls.acls} .
|
test this please |
junrao
left a comment
junrao
left a comment
There was a problem hiding this comment.
@big-andy-coates : Thanks for the patch. A few comments below. Also, (1) I agree that we can fix AclBinding in the adminClient to just include AclFilter in a followup patch in this release. We can fix SimpleAuthorizer as part of KIP-50. (2) We can probably just consolidate the acl changes on /kafka-acl-changes. During down grade, we will load the full acls from the resources on startup. So, as long as subsequent acl changes are made through the old format. It should be fine.
|
|
||
| /** | ||
| * The version number is bumped to indicate that on quota violation brokers send out responses before throttling. | ||
| * V1 sees a new `RESOURCE_NAME_TYPE` that controls how the resource name is interpreted. |
There was a problem hiding this comment.
We want to preserve the comment on quota violation.
|
|
||
| /** | ||
| * The version number is bumped to indicate that on quota violation brokers send out responses before throttling. | ||
| * V1 sees a new `RESOURCE_NAME_TYPE_FILTER` that controls how the filter handles different resource name types. |
There was a problem hiding this comment.
We want to preserve the comment on quota violation.
|
|
||
| /** | ||
| * The version number is bumped to indicate that on quota violation brokers send out responses before throttling. | ||
| * V1 sees a new `RESOURCE_NAME_TYPE` field added to MATCHING_ACL_V1, that describes how the resource name is interpreted. |
There was a problem hiding this comment.
We want to preserve the comment on quota violation.
|
|
||
| /** | ||
| * The version number is bumped to indicate that on quota violation brokers send out responses before throttling. | ||
| * V1 sees a new `RESOURCE_NAME_TYPE_FILTER` that controls how the filter handles different resource name types. |
There was a problem hiding this comment.
We want to preserve the comment on quota violation.
|
|
||
| /** | ||
| * The version number is bumped to indicate that on quota violation brokers send out responses before throttling. | ||
| * V1 sees a new `RESOURCE_NAME_TYPE` field added to DESCRIBE_ACLS_RESOURCE_V1, that describes how the resource name is interpreted. |
There was a problem hiding this comment.
We want to preserve the comment on quota violation.
| public void shouldMatchPrefixedIfNamePrefixedAnyFilterTypeIsAny() { | ||
| assertTrue(AclUtils.matchResource( | ||
| new Resource(TOPIC, "Name", PREFIXED), | ||
| new ResourceFilter(TOPIC, "Name-something", ResourceNameType.ANY) |
There was a problem hiding this comment.
This makes sense from authorize()'s perspective. However, for AdminClient, does this make sense?
There was a problem hiding this comment.
Yes, see below.
| public void shouldMatchLiteralWildcardIfFilterHasNameTypeOfAny() { | ||
| assertTrue(AclUtils.matchResource( | ||
| new Resource(TOPIC, "*", LITERAL), | ||
| new ResourceFilter(TOPIC, "Name", ResourceNameType.ANY) |
There was a problem hiding this comment.
This makes sense from authorize()'s perspective. However, for AdminClient, does this make sense?
There was a problem hiding this comment.
Yes, I think so. This works well to allow users to discover all the ACLs affecting a resource, be they literal, wildcard or prefixed.
Old functionality:
Each call retrieves ACLs stored in one path in ZK:
adminClient.describeAcls(... TOPIC, "foobar" ...) -> would return only ACLs from '/kafka-acls/foobar' path
adminClient.describeAcls(... TOPIC, "*" ...) -> would return only ACLs from '/kafka-acls/*' path
New functionality:
Using legacy constructors:
Legacy constructors default resourceNameType to Literal and maintains existing contract:
adminClient.describeAcls(... TOPIC, "foobar" ...) -> still returns only ACLs from '/kafka-acls/foobar' path
adminClient.describeAcls(... TOPIC, "*" ...) -> still returns only ACLs from '/kafka-acls/*' path
Using constructors with explicit resourceNameType:
The user needs to know, up front, which prefixed resource paths exist to be able to query them:
adminClient.describeAcls(... TOPIC, "foobar", Literal ...)-> will return only ACLs from '/kafka-acls/foobar' path
adminClient.describeAcls(... TOPIC, "*", Literal ...) -> will return only ACLs from '/kafka-acls/*' path
adminClient.describeAcls(... TOPIC, "foo", Prefixed ...) -> will return only ACLs from '/kafka-prefixed-acls/foo' path
Using 'ANY' resource name type:
This allows user to discover all the ACLs affecting a specific resource:
adminClient.describeAcls(... TOPIC, "foobar", Any ...) -> will return all ACLs affecting topic "foobar", including any prefixed and wildcard ACLs.
The return value from describeAcls will contain the resourceNameType field for each ACL, so the user can determine if it is literal or prefixed.
Acls on prefixed resource paths are never returned to earlier clients. Nor can older clients delete ACLs on prefixed resource paths.
There was a problem hiding this comment.
Sounds good. It would be useful to add the following comment when we make the changes in adminClient.
This allows user to discover all the ACLs affecting a specific resource:
adminClient.describeAcls(... TOPIC, "foobar", Any ...) -> will return all ACLs affecting topic "foobar", including any prefixed and wildcard ACLs.
There was a problem hiding this comment.
Yer, I already had that in a local java doc change. Pushed.
|
|
||
| // now authorize the user for the internal topic and verify that we can subscribe | ||
| addAndVerifyAcls(Set(new Acl(KafkaPrincipal.ANONYMOUS, Allow, Acl.WildCardHost, Read)), Resource(Topic, | ||
| addAndVerifyAcls(Set(Acl(KafkaPrincipal.ANONYMOUS, Allow, Acl.WildCardHost, Read)), new Resource(Topic, |
There was a problem hiding this comment.
Should we use the new constructor for Resource?
There was a problem hiding this comment.
Yes, as mentioned above, I'll do this all in one pass now that the tests are all passing.
| closeSasl() | ||
| } | ||
|
|
||
| val anyAcl = new AclBinding(new Resource(ResourceType.TOPIC, "*"), |
There was a problem hiding this comment.
Should we use the new Resource constructor? Ditto below.
There was a problem hiding this comment.
Yes, as mentioned above, I'll do this all in one pass now that the tests are all passing.
| assertEquals(2, simpleAclAuthorizer.getAcls(principal).size) | ||
|
|
||
| val acl2 = new Acl(Acl.WildCardPrincipal, Allow, WildCardHost, Write) | ||
| simpleAclAuthorizer.addAcls(Set[Acl](acl1), new Resource(Group, "groupA")) |
There was a problem hiding this comment.
Should we use the new Resource constructor?
There was a problem hiding this comment.
Yes, as mentioned above, I'll do this all in one pass now that the tests are all passing.
- AclCommand (kafka-acls.sh) now supported prefixed - Documentation for kafka-acls.has updated for prefixed. - Optimised SimpleAclAuthorizer.getMatchingAcls call.
|
@junrao latest version pushed. Includes:
Are you OK with merging this and picking up follow on changes in separate PRs? Still outstanding:
|
# Conflicts: # core/src/test/scala/integration/kafka/api/AuthorizerIntegrationTest.scala # docs/upgrade.html
|
@junrao Please note that Piyush may wish to resubmit these changes under his own name once we're all happy with the code. So hold of merging until Piyush has made his wisher clear, (or until the deadline looms!) |
| public static final Field.Str RESOURCE_NAME = new Field.Str("resource_name", "The resource name"); | ||
| public static final Field.NullableStr RESOURCE_NAME_FILTER = new Field.NullableStr("resource_name", "The resource name filter"); | ||
| public static final Field.Int8 RESOURCE_NAME_TYPE = new Field.Int8("resource_name_type", "The resource name type", AclUtils.LITERAL.code()); | ||
| public static final Field.Int8 RESOURCE_NAME_TYPE_FILTER = new Field.Int8("resource_name_type", "The resource name type filter", AclUtils.LITERAL.code()); |
There was a problem hiding this comment.
s/resource_name_type/resource_name_type_filter?
|
test this please |
|
@junrao, @piyushvijay is happy for us to merge from this PR, but could you please call out his contribution in the commit message? |
|
test this please. (unrelated test failure). |
| /** | ||
| * The version number is bumped to indicate that on quota violation brokers send out responses before throttling. | ||
| * V1 sees a new `RESOURCE_NAME_TYPE` that controls how the resource name is interpreted and version | ||
| * was bumped to indicate that, on quota violation, brokers send out responses before throttling. |
There was a problem hiding this comment.
I think this could be phrased a little bit better. Something like "Version 1 adds RESOURCE_NAME_TYPE. Also, when the quota is violated, brokers will respond to a version 1 or later request before throttling."
There was a problem hiding this comment.
I thought so to, but didn't want to change other peoples wording! Consider it done.
| HOST, | ||
| OPERATION, | ||
| PERMISSION_TYPE)))); | ||
|
|
There was a problem hiding this comment.
I can't figure out how to add a comment to the correct line using github's terrible user interface. But on line 110 where you have the code:
@Override
public CreateAclsRequest build(short version) {
return new CreateAclsRequest(version, creations);
}
You must add verification that the version you are using supports the ACLs you are trying to send. If someone tries to set a prefix ACL when using CREATE_ACLS_REQUEST_V0, the code must throw UnsupportedVersionException. In fact, anything besides ResourceNameType.LITERAL should trigger a UVE there.
There was a problem hiding this comment.
Consider it done.
| /** | ||
| * The version number is bumped to indicate that on quota violation brokers send out responses before throttling. | ||
| * The version number is bumped to indicate that, on quota violation, brokers send out responses before throttling | ||
| * and request has additional resource_name_type field. |
There was a problem hiding this comment.
It's not necessary to duplicate the comment about CreateAclsRequest inside CreateAclsResponse. The response doesn't have any additional fields. I don't think there's anything to document here beyond the original comment about quota stuff.
|
Can you add a unit or integration test of
That would be a breaking API change, which would mean that current users of the API would need to be rewritten. I agree that A quick example might help to explain. When I remove The filter can have fields set to null, in which case they match everything. So if I want to remove all ACLs, can remove Overall, I think we should leave the class names as-is. I don't think there's a lot to be gained by changing it now, and it would be a big inconvenience for users to break compatibility. Although it doesn't need to be done in this PR, I do think we should remove the internal classes which just duplicate the Resource class. I'm thinking of |
| import org.apache.kafka.common.resource.ResourceNameType; | ||
| import org.apache.kafka.common.resource.ResourceType; | ||
|
|
||
| public final class AclUtils { |
There was a problem hiding this comment.
I don't understand why we need a separate class and a static method for this. The only caller is ResourceFilter#matches. That's where this logic was before, and I think that's where it still belongs.
There was a problem hiding this comment.
Agreed, this is a hang over of previous changes.
| case LITERAL: | ||
| return aclPath.name().equals(filter.name()) || aclPath.name().equals(WILDCARD_RESOURCE); | ||
|
|
||
| case PREFIXED: |
There was a problem hiding this comment.
This is wrong. AclFilter objects should match exactly or not at all. Trying to remove a wildcard AclBinding should not remove every AclBinding. Trying to remove a prefix AclBinding should not remove every AclBinding with that prefix.
There was a problem hiding this comment.
This KIP / PR has to see the semantics of matching a filter to a resource change. Please see #5117 (comment)
There was a problem hiding this comment.
Sorry, but this is wrong. There are two "semantics of matching".
- The semantics of an AclBindingFilter matching an AclBinding
- The semantics of an AclBinding matching an ACL
Prefix comparisons are only relevant to #2, not to #1.
When I try to remove an AclBinding for a prefix ACL beginning with "foo", I don't want to remove ACLs that start with foobar, foo2, etc. When I remove a wildcard ACL, I don't want to remove all ACLs. And so forth.
There was a problem hiding this comment.
This code is purely responsible for matching a Resource to a ResourceFilter. There are no ACLs involved. So as far as I can tell there is only one matching semantic here, which is the matching of a ResourceFilter and a Resource.
If trying to remove an AclBinding for a prefixed topic beginning with 'foo' then the filter would be new ResourceFilter(Topic, "foo", Prefixed), which would NOT match a Resource(Topic, "foobar", Prefixed) or indeed Resource(Topic, "foobar", Literal). There are unit tests covering this.
There was a problem hiding this comment.
Have you maybe missed the:
if (nameType().equals(other.nameType())) {
return other.name().equals(name());
}
statement that means any match between a filter and resource of the same ResourceNameType requires an exact match on the name?
There was a problem hiding this comment.
To summarize our offline discussion, the proposal is to override ResourceNameType.ANY in a somewhat odd, but potentially useful way. Let's pick up this discussion in a follow on PR.
| @@ -0,0 +1,49 @@ | |||
| /* | |||
There was a problem hiding this comment.
Can't we just use the Java class here rather than duplicating it?
The only reason the other classes are duplicated is that the Scala class came first, and later the Java classes were added for the clients.
There was a problem hiding this comment.
Sure, I'll clean up in a follow on PR. (It's past midnight here!)
junrao
left a comment
junrao
left a comment
There was a problem hiding this comment.
@big-andy-coates : Thanks for the patch. Just a few more minor comments below.
| <pre class="brush: bash;">bin/kafka-acls.sh --authorizer-properties zookeeper.connect=localhost:2181 --add --allow-principal User:Peter --allow-host 198.51.200.1 --producer --topic *</pre> | ||
| You can add acls on resources matching a certain prefix, e.g. suppose you want to add an acl "Principal User:Jane is allowed to produce to any Topic whose name is prefixed with 'Test-' from any host". | ||
| You can do that by executing the CLI with following options: | ||
| <pre class="brush: bash;">bin/kafka-acls.sh --authorizer-properties zookeeper.connect=localhost:2181 --add --allow-principal User:Jane --producer --topic Test- --resource-name-type Prefixed</pre> |
There was a problem hiding this comment.
Test- => Test-topic? Ditto below in a few other places.
There was a problem hiding this comment.
The 'Test-' is deliberate. This command is adding a prefixed ACL, i.e. one matching any Topic whose name starts with 'Test-', so it would match 'Test-topic'
| } | ||
| } | ||
|
|
||
| private object ResourceOrdering extends Ordering[Resource] { |
There was a problem hiding this comment.
Could we add a comment that we want to sort in reverse resource name order?
There was a problem hiding this comment.
I'm not a fan of such comments, that can easily end up out of sync with the code. But I can add one.
| private def getResourceFilter(opts: AclCommandOptions, dieIfNoResourceFound: Boolean = true): Set[ResourceFilter] = { | ||
| val resourceNameType: JResourceNameType = opts.options.valueOf(opts.resourceNameType) | ||
|
|
||
| var resources = Set.empty[ResourceFilter] |
There was a problem hiding this comment.
resources => resourceFilters?
| import org.apache.kafka.common.resource.{ResourceFilter, ResourceNameType => JResourceNameType} | ||
| import org.apache.kafka.common.security.auth.KafkaPrincipal | ||
| import org.apache.kafka.common.utils.{SecurityUtils, Time} | ||
| import org.apache.kafka.common.utils.{AclUtils, SecurityUtils, Time} |
|
|
||
| @Test | ||
| def testAclsOnPrefixedResources(): Unit = { | ||
| val cmd = Array("--allow-principal", principal.toString, "--producer", "--topic", "Test-", "--resource-name-type", "Prefixed") |
There was a problem hiding this comment.
Again, this is intentional as its adding a ACLs on a prefixed resource name.
|
Hi @big-andy-coates, As discussed offline, there is a standard way to add co-authors: https://blog.github.com/2018-01-29-commit-together-with-co-authors/ (cc: @cmccabe @junrao). Probably add me before merging. Thanks |
Yep, done.
a) I need to remind myself of the reason why this was necessary! I'll come back to you in a separate PR.
The changes in this PR are completely backwards compatible, both in terms of working with older clients and in terms of maintaining the existing contracts with new clients. Additionally, they extend the semantics by adding
You may be right. I'll remind myself of the reasons and, if appropriate, come back with a case and a PR that we can discuss.
As I've mentioned in comments to Jun, I don't think we should tackle removing these under this KIP. o.a.k.c.r.Resource is used exclusively for dealing with topic config, i.e. o.a.k.c.r.Resource only ever refers to a concrete topic resource, and never the wildcard topic or a prefixed topic. I think this work is better done under KIP-50. Which, BTW, I'm happy to pick up. In this KIP we can split the Are you pushing for these duplicate classes to be dropped now because the next release is a major version bump? |
|
@cmccabe, just seen your comment:
That makes more sense - so we don't replace the use of org.apache.kafka.common.requests.Resource with org.apache.kafka.common.resource.Resource, but with org.apache.kafka.common.config.ConfigResource. |
|
test this please |
…refix * apache-github/trunk: KAFKA-6726: Fine Grained ACL for CreateTopics (KIP-277) (apache#4795) KAFKA-5588: Remove deprecated --new-consumer tools option (apache#5097) MINOR: Fix for the location of the trogdor.sh executable file in the documentation. (apache#5040) KAFKA-6997: Exclude test-sources.jar when $INCLUDE_TEST_JARS is FALSE MINOR: docs should point to latest version (apache#5132) KAFKA-6981: Move the error handling configuration properties into the ConnectorConfig and SinkConnectorConfig classes (KIP-298) [KAFKA-6730] Simplify State Store Recovery (apache#5013) MINOR: Rename package `internal` to `internals` for consistency (apache#5137) KAFKA-6704: InvalidStateStoreException from IQ when StreamThread closes store (apache#4801) MINOR: Add missing configs for resilience settings MINOR: Add regression tests for KTable mapValues and filter (apache#5134) KAFKA-6750: Add listener name to authentication context (KIP-282) (apache#4829) KAFKA-3665: Enable TLS hostname verification by default (KIP-294) (apache#4956) KAFKA-6938: Add documentation for accessing Headers on Kafka Streams Processor API (apache#5128) KAFKA-6813: return to double-counting for count topology names (apache#5075) KAFKA-5919; Adding checks on "version" field for tools using it MINOR: Remove deprecated KafkaStreams constructors in docs (apache#5118)
|
I merged this PR with trunk and resolved conflicts. |
|
I synced with Jun offline and he is happy for this PR to be merged and for unresolved issues to be addressed in follow-up PRs. I will merge this once the tests pass and will set Piyush and Andy as a co-authors. @big-andy-coates, can you please file one or more JIRAs with "Fix version" 2.0.0 for the remaining work? |
|
OK, have created the following Jira:
I've not created a Jira to "Remove duplicate Scala Resource class", which @cmccabe suggested, as I've realised that this would require changes to the |
|
Thanks @big-andy-coates |
…5160) The initial PR for KIP-290 #5117 added a `ResourceNameType` field to the Java and Scala `Resource` classes to introduce the concept of Prefixed ACLS. This does not make a lot of sense as these classes are meant to represent cluster resources, which would not have a concept of 'name type'. This work has not been released yet, so we have time to change it. This PR looks to refactor the code to remove the name type field from the Java `Resource` class. (The Scala one will age out once KIP-290 is done, and removing it would involve changes to the `Authorizer` interface, so this class was not touched). This is achieved by replacing the use of `Resource` with `ResourcePattern` and `ResourceFilter` with `ResourceFilterPattern`. A `ResourcePattern` is a combination of resource type, name and name type, where each field needs to be defined. A `ResourcePatternFilter` is used to select patterns during describe and delete operations. The adminClient uses `AclBinding` and `AclBindingFilter`. These types have been switched over to use the new pattern types. The AclCommands class, used by Kafka-acls.sh, has been converted to use the new pattern types. The result is that the original `Resource` and `ResourceFilter` classes are not really used anywhere, except deprecated methods. However, the `Resource` class will be used if/when KIP-50 is done. Reviewers: Colin Patrick McCabe <colin@cmccabe.xyz>, Jun Rao <junrao@gmail.com>
…5160) The initial PR for KIP-290 #5117 added a `ResourceNameType` field to the Java and Scala `Resource` classes to introduce the concept of Prefixed ACLS. This does not make a lot of sense as these classes are meant to represent cluster resources, which would not have a concept of 'name type'. This work has not been released yet, so we have time to change it. This PR looks to refactor the code to remove the name type field from the Java `Resource` class. (The Scala one will age out once KIP-290 is done, and removing it would involve changes to the `Authorizer` interface, so this class was not touched). This is achieved by replacing the use of `Resource` with `ResourcePattern` and `ResourceFilter` with `ResourceFilterPattern`. A `ResourcePattern` is a combination of resource type, name and name type, where each field needs to be defined. A `ResourcePatternFilter` is used to select patterns during describe and delete operations. The adminClient uses `AclBinding` and `AclBindingFilter`. These types have been switched over to use the new pattern types. The AclCommands class, used by Kafka-acls.sh, has been converted to use the new pattern types. The result is that the original `Resource` and `ResourceFilter` classes are not really used anywhere, except deprecated methods. However, the `Resource` class will be used if/when KIP-50 is done. Reviewers: Colin Patrick McCabe <colin@cmccabe.xyz>, Jun Rao <junrao@gmail.com>
#5152) remove duplicate Scala ResourceNameType in preference to in preference to Java ResourceNameType. This is follow on work for KIP-290 and PR #5117, which saw the Scala ResourceNameType class introduced. I've added tests to ensure AclBindings can't be created with ResourceNameType.ANY or UNKNOWN. Reviewers: Colin Patrick McCabe <colin@cmccabe.xyz>, Jun Rao <junrao@gmail.com>
#5152) remove duplicate Scala ResourceNameType in preference to in preference to Java ResourceNameType. This is follow on work for KIP-290 and PR #5117, which saw the Scala ResourceNameType class introduced. I've added tests to ensure AclBindings can't be created with ResourceNameType.ANY or UNKNOWN. Reviewers: Colin Patrick McCabe <colin@cmccabe.xyz>, Jun Rao <junrao@gmail.com>
This is a follow-on change requested as part of the initial PR for KIP-290 #5117. @cmccabe requested that the `resource.Resource` class be factored out in favour of `ConfigResource` to avoid confusion between all the `Resource` implementations. Colin Patrick McCabe <colin@cmccabe.xyz>, Jun Rao <junrao@gmail.com>
This is a follow-on change requested as part of the initial PR for KIP-290 #5117. @cmccabe requested that the `resource.Resource` class be factored out in favour of `ConfigResource` to avoid confusion between all the `Resource` implementations. Colin Patrick McCabe <colin@cmccabe.xyz>, Jun Rao <junrao@gmail.com>
The initial PR for KIP-290 #5117 added a new `ResourceNameType`, which was initially a field on `Resource` and `ResourceFilter`. However, follow on PRs have now moved the name type fields to new `ResourcePattern` and `ResourcePatternFilter` classes. This means the old name is no longer valid and may be confusing. The PR looks to rename the class to a more intuitive `resource.PatternType`. @cmccabe also requested that the current `ANY` value for this class be renamed to avoid confusion. `PatternType.ANY` currently causes `ResourcePatternFilter` to bring back all ACLs that would affect the supplied resource, i.e. it brings back literal, wildcard ACLs, and also does pattern matching to work out which prefix acls would affect the resource. This is very different from the behaviour of `ResourceType.ANY`, which just means the filter ignores the type of resources. `ANY` is to be renamed to `MATCH` to disambiguate it from other `ANY` filter types. A new `ANY` will be added that works in the same way as others, i.e. it will cause the filter to ignore the pattern type, (but won't do any pattern matching). Reviewers: Colin Patrick McCabe <colin@cmccabe.xyz>, Jun Rao <junrao@gmail.com>
The initial PR for KIP-290 #5117 added a new `ResourceNameType`, which was initially a field on `Resource` and `ResourceFilter`. However, follow on PRs have now moved the name type fields to new `ResourcePattern` and `ResourcePatternFilter` classes. This means the old name is no longer valid and may be confusing. The PR looks to rename the class to a more intuitive `resource.PatternType`. @cmccabe also requested that the current `ANY` value for this class be renamed to avoid confusion. `PatternType.ANY` currently causes `ResourcePatternFilter` to bring back all ACLs that would affect the supplied resource, i.e. it brings back literal, wildcard ACLs, and also does pattern matching to work out which prefix acls would affect the resource. This is very different from the behaviour of `ResourceType.ANY`, which just means the filter ignores the type of resources. `ANY` is to be renamed to `MATCH` to disambiguate it from other `ANY` filter types. A new `ANY` will be added that works in the same way as others, i.e. it will cause the filter to ignore the pattern type, (but won't do any pattern matching). Reviewers: Colin Patrick McCabe <colin@cmccabe.xyz>, Jun Rao <junrao@gmail.com>
Reviewers: Colin Patrick McCabe <colin@cmccabe.xyz>, Jun Rao <junrao@gmail.com> Co-authored-by: Piyush Vijay <pvijay@apple.com> Co-authored-by: Andy Coates <big-andy-coates@users.noreply.github.com>
…pache#5160) The initial PR for KIP-290 apache#5117 added a `ResourceNameType` field to the Java and Scala `Resource` classes to introduce the concept of Prefixed ACLS. This does not make a lot of sense as these classes are meant to represent cluster resources, which would not have a concept of 'name type'. This work has not been released yet, so we have time to change it. This PR looks to refactor the code to remove the name type field from the Java `Resource` class. (The Scala one will age out once KIP-290 is done, and removing it would involve changes to the `Authorizer` interface, so this class was not touched). This is achieved by replacing the use of `Resource` with `ResourcePattern` and `ResourceFilter` with `ResourceFilterPattern`. A `ResourcePattern` is a combination of resource type, name and name type, where each field needs to be defined. A `ResourcePatternFilter` is used to select patterns during describe and delete operations. The adminClient uses `AclBinding` and `AclBindingFilter`. These types have been switched over to use the new pattern types. The AclCommands class, used by Kafka-acls.sh, has been converted to use the new pattern types. The result is that the original `Resource` and `ResourceFilter` classes are not really used anywhere, except deprecated methods. However, the `Resource` class will be used if/when KIP-50 is done. Reviewers: Colin Patrick McCabe <colin@cmccabe.xyz>, Jun Rao <junrao@gmail.com>
apache#5152) remove duplicate Scala ResourceNameType in preference to in preference to Java ResourceNameType. This is follow on work for KIP-290 and PR apache#5117, which saw the Scala ResourceNameType class introduced. I've added tests to ensure AclBindings can't be created with ResourceNameType.ANY or UNKNOWN. Reviewers: Colin Patrick McCabe <colin@cmccabe.xyz>, Jun Rao <junrao@gmail.com>
This is a follow-on change requested as part of the initial PR for KIP-290 apache#5117. @cmccabe requested that the `resource.Resource` class be factored out in favour of `ConfigResource` to avoid confusion between all the `Resource` implementations. Colin Patrick McCabe <colin@cmccabe.xyz>, Jun Rao <junrao@gmail.com>
The initial PR for KIP-290 apache#5117 added a new `ResourceNameType`, which was initially a field on `Resource` and `ResourceFilter`. However, follow on PRs have now moved the name type fields to new `ResourcePattern` and `ResourcePatternFilter` classes. This means the old name is no longer valid and may be confusing. The PR looks to rename the class to a more intuitive `resource.PatternType`. @cmccabe also requested that the current `ANY` value for this class be renamed to avoid confusion. `PatternType.ANY` currently causes `ResourcePatternFilter` to bring back all ACLs that would affect the supplied resource, i.e. it brings back literal, wildcard ACLs, and also does pattern matching to work out which prefix acls would affect the resource. This is very different from the behaviour of `ResourceType.ANY`, which just means the filter ignores the type of resources. `ANY` is to be renamed to `MATCH` to disambiguate it from other `ANY` filter types. A new `ANY` will be added that works in the same way as others, i.e. it will cause the filter to ignore the pattern type, (but won't do any pattern matching). Reviewers: Colin Patrick McCabe <colin@cmccabe.xyz>, Jun Rao <junrao@gmail.com>
5 participants
KIP-290 / KAFKA-6841
This PR supersedes #5079. (As I lack rights to push to that PR).
Still to do (can be done in this or another PR):
The following outstanding items have been moved to additional Jiras or deemed unnecessary. See #5117 (comment).
Resourceclass.Resourceclass.ResourceNameTypeclass and just Java one./kafka-acl-changespath and store ResourceNameType a JSON Value.ResourceinAclBindingwithResourceFilteror aResourceMatcherCommitter Checklist (excluded from commit message)