KAFKA-7010: Rename ResourceNameType to PatternType by big-andy-coates · Pull Request #5205 · apache/kafka

big-andy-coates · 2018-06-12T22:41:39Z

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).

cc @junrao

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

…CH, and add ANY

junrao

@big-andy-coates : Thanks for the patch. LGTM. Just a few minor comments below.

junrao · 2018-06-12T22:53:08Z

clients/src/main/java/org/apache/kafka/common/resource/PatternType.java

    ANY((byte) 1),

+    /**
+     * In a filter, matches any resource pattern type.


Hmm, this has to be different from the comment for ANY?

Classic cut & paste error - how embarrassing. :)

junrao · 2018-06-12T23:04:34Z

core/src/main/scala/kafka/admin/AclCommand.scala

      .ofType(classOf[String])

-    val resourceNameType = parser.accepts("resource-name-type", "The type of the resource name, or any.")
+    val resourcePatternType = parser.accepts("resource-pattern-type", "The type of the resource pattern, or any.")


We want to add MATCH in the description and explain its meaning.

Added more detail

junrao · 2018-06-12T23:09:58Z

docs/security.html

-                    <li><b>Prefixed</b> Match any resource whose name starts with the prefix.</li>
-                    <li><b>All</b> (list|remove only) Matching any name type, including the Wildcard name.</li>
+                    <li><b>Literal (default):</b> a pattern that only matches a resource if the name is an exact match, or, in the case of the Wildcard name '*', matches all resources.
+                        All operations, (add, remove, list), will only affect or return acls on literal resource patterns with the exact supplied name.


Is this intended?

Erm... yes...?

Happy to replace if you can word it better :D

junrao · 2018-06-12T23:10:07Z

docs/security.html

+                        All operations, (add, remove, list), will only affect or return acls on literal resource patterns with the exact supplied name.
+                    </li>
+                    <li><b>Prefixed:</b> a pattern that will match a resource whose name starts with the supplied name.
+                        All operations, (add, remove, list), will only affect or return acls on prefixed resource patterns with the exact supplied name.


Is this intended?

As above, yes.

junrao · 2018-06-12T23:12:39Z

docs/security.html

+                    </li>
+                    <li><b>Prefixed:</b> a pattern that will match a resource whose name starts with the supplied name.
+                        All operations, (add, remove, list), will only affect or return acls on prefixed resource patterns with the exact supplied name.
+                    <li><b>All</b> (list|remove only)


Is this All or Any?

Any - updated.

Should this be MATCH, as per our earlier discussion?

cmccabe · 2018-06-13T15:45:01Z

clients/src/main/java/org/apache/kafka/common/requests/CreateAclsRequest.java

-     * Version 1 adds RESOURCE_NAME_TYPE.
-     * Also, when the quota is violated, brokers will respond to a version 1 or later request before throttling.
+     * Version 1 adds RESOURCE_PATTERN_TYPE, to support more than just literal resource patterns.
+     * For more info, see {@link PatternType}.


Does the JavaDoc work without the full class path?

Unlike ScalaDoc, yes, as long as the type is imported, which it is.

cmccabe · 2018-06-13T16:12:19Z

clients/src/main/java/org/apache/kafka/common/resource/PatternType.java

-     * In a filter, matches any resource name type.
+     * In a filter, matches any resource pattern type.
     */
    ANY((byte) 1),


We should rename this to "MATCH" as per our earlier discussion, since its behavior doesn't match the other "ANY" fields

MATCH is the next one down.

big-andy-coates · 2018-06-13T16:17:02Z

Hi @junrao, @cmccabe, I've pushed changes, as requested by @junrao, and also spent some time overhauling security.html to slightly introduce the user to the concept of resource patterns, and then refer them to KIP-290. I think this page now hangs together better.

Let me know what you think, or merge if you're happy.

big-andy-coates · 2018-06-13T18:35:05Z

Test failure is unrelated DeleteConsumerGroupTest.testConsumptionOnRecreatedTopicAfterTopicWideDeleteInZK

junrao

@big-andy-coates : Thanks for the updated PR. LGTM. Just a few minor typos listed below.

junrao · 2018-06-14T00:38:07Z

core/src/main/scala/kafka/admin/AclCommand.scala


-    val resourceNameType = parser.accepts("resource-name-type", "The type of the resource name, or any.")
+    val resourcePatternType = parser.accepts("resource-pattern-type", "The type of the resource pattern or pattern filter. " +
+      "When adding acls, this should a specific pattern type, e.g. 'literal' or 'prefixed'. " +


this should a =>this should be a

junrao · 2018-06-14T00:40:08Z

core/src/main/scala/kafka/admin/AclCommand.scala

-    val resourceNameType = parser.accepts("resource-name-type", "The type of the resource name, or any.")
+    val resourcePatternType = parser.accepts("resource-pattern-type", "The type of the resource pattern or pattern filter. " +
+      "When adding acls, this should a specific pattern type, e.g. 'literal' or 'prefixed'. " +
+      "When listing or removing acls, a specific pattern type can be used to remove acls from specific resource patterns, " +


to remove => to list or remove ?

junrao · 2018-06-14T00:44:49Z

docs/security.html

    Kafka ships with a pluggable Authorizer and an out-of-box authorizer implementation that uses zookeeper to store all the acls. The Authorizer is configured by setting <code>authorizer.class.name</code> in server.properties. To enable the out of the box implementation use:
    <pre>authorizer.class.name=kafka.security.auth.SimpleAclAuthorizer</pre>
-    Kafka acls are defined in the general format of "Principal P is [Allowed/Denied] Operation O From Host H On Resource R". You can read more about the acl structure on KIP-11. In order to add, remove or list acls you can use the Kafka authorizer CLI. By default, if a Resource R has no associated acls, no one other than super users is allowed to access R. If you want to change that behavior, you can include the following in server.properties.
+    Kafka acls are defined in the general format of "Principal P is [Allowed/Denied] Operation O From Host H On Any Resoure R matching ResourcePattern RP". You can read more about the acl structure in KIP-11 and resource patterns in KIP-290. In order to add, remove or list acls you can use the Kafka authorizer CLI. By default, if no ResourcePatterns match a specific Resource R, then R has no associated acls, and therefore no one other than super users is allowed to access R. If you want to change that behavior, you can include the following in server.properties.


Any => any ?

junrao · 2018-06-14T00:45:34Z

docs/security.html

        <tr>
            <td>--cluster</td>
-            <td>Specifies cluster as resource.</td>
+            <td>Indicates to the script tha the user is trying to interact with acls on the singular cluster resource.</td>


tha => that

junrao · 2018-06-14T00:46:02Z

docs/security.html

        <tr>
            <td>--topic [topic-name]</td>
-            <td>Specifies the topic as resource.</td>
+            <td>Indicates to the script tha the user is trying to interact with acls on topic resource pattern(s).</td>


tha => that

junrao · 2018-06-14T00:51:45Z

docs/security.html

            You can do that by using the wildcard resource '*', e.g. 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: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 add acls on prefixed resource patterns, e.g. suppose you want to add an acl "Principal User:Jane is allowed to produce to any Topic whose name is starts with 'Test-' from any host".


is starts => starts

junrao · 2018-06-14T01:00:50Z

docs/security.html

+                <pre class="brush: bash;">bin/kafka-acls.sh --authorizer-properties zookeeper.connect=localhost:2181 --list --topic *</pre>
+                However, it is not necessarily possible to explicitly query for acls on prefixed resource patterns that match Test-topic as the name of such patterns may not be known.
+                We can list <i>all</i> acls affecting Test-topic by using '--resource-pattern-type match', e.g.
+                <pre class="brush: bash;">bin/kafka-acls.sh --authorizer-properties zookeeper.connect=localhost:2181 --list --topic Test-topic --resource-pattern-type any</pre>


--resource-pattern-type any => --resource-pattern-type match

big-andy-coates · 2018-06-14T13:08:40Z

test this please

@cmccabe

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>

@cmccabe

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>

big-andy-coates added 2 commits June 12, 2018 23:34

KAFKA-7010: Rename ResourceNameType to PatternType, rename ANY to MAT…

31511ac

…CH, and add ANY

Jun's requested change

2c85b6c

junrao reviewed Jun 12, 2018

View reviewed changes

cmccabe reviewed Jun 13, 2018

View reviewed changes

Jun's requested changes + more documentation.

cb5d564

junrao approved these changes Jun 14, 2018

View reviewed changes

big-andy-coates added 3 commits June 14, 2018 09:06

Jun's requested changes.

c448cb8

Ismael's requested changes.

ddc243d

Merge branch 'trunk' into pattern_type

cf7c1d3

squelch tests

e6a1693

junrao merged commit 642a977 into apache:trunk Jun 14, 2018

big-andy-coates deleted the pattern_type branch June 14, 2018 21:09

Conversation

big-andy-coates commented Jun 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

big-andy-coates commented Jun 13, 2018

Uh oh!

big-andy-coates commented Jun 13, 2018

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

big-andy-coates commented Jun 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

big-andy-coates commented Jun 12, 2018 •

edited

Loading