Hi Colin,
Escaping at this level is making sense to me but let me think more and get
back to you.
But should we not just get rid of one of AclBinding or AclBindingFilter
then? Is there a reason to keep both given that AclBindingFilter and
AclBinding look exact copy of each other after this change? This will be a
one-time breaking change in APIs marked as "Evolving", but makes sense in
the long term? Am I missing something here?
Piyush Vijay
On Tue, May 15, 2018 at 9:01 AM, Colin McCabe <cmcc...@apache.org> wrote:
> Hi Piyush,
>
> I think AclBinding should operate the same way as AclBindingFilter.
>
> So you should be able to do something like this:
> > AclBindingFilter filter = new AclBindingFiler(new
> ResourceFilter(ResourceType.GROUP, "foo*"))
> > AclBinding binding = new AclBinding(new Resource(ResourceType.GROUP,
> "foo*"))
> > assertTrue(filter.matches(binding));
>
> Thinking about this more, it's starting to feel really messy to create new
> "pattern" constructors for Resource and ResourceFilter. I don't think
> people will be able to figure this out. Maybe we should just have a
> limited compatibility break here, where it is now required to escape weird
> consumer group names when creating ACLs for them.
>
> To future-proof this, we should reserve a bunch of characters at once,
> like *, @, $, %, ^, &, +, [, ], etc. If these characters appear in a
> resource name, it should be an error, unless they are escaped with a
> backslash. That way, we can use them in the future. We should create a
> Resource.escapeName function which adds the correct escape characters to
> resource names (so it would translate foo* into foo\*, foo+bar into
> foo\+bar, etc. etc.
>
> best,
> Colin
>
>
> On Mon, May 14, 2018, at 17:08, Piyush Vijay wrote:
> > Colin,
> >
> > createAcls take a AclBinding, however, instead of AclBindingFilter. What
> > are your thoughts here?
> >
> > public abstract DescribeAclsResult describeAcls(AclBindingFilter
> > filter, DescribeAclsOptions options);
> >
> > public abstract CreateAclsResult createAcls(Collection<AclBinding>
> > acls, CreateAclsOptions options);
> >
> > public abstract DeleteAclsResult
> > deleteAcls(Collection<AclBindingFilter> filters, DeleteAclsOptions
> > options);
> >
> >
> > Thanks
> >
> > Piyush Vijay
> >
> > On Mon, May 14, 2018 at 9:26 AM, Andy Coates <a...@confluent.io> wrote:
> >
> > > +1
> > >
> > > On 11 May 2018 at 17:14, Colin McCabe <cmcc...@apache.org> wrote:
> > >
> > > > Hi Andy,
> > > >
> > > > I see what you mean. I guess my thought here is that if the fields
> are
> > > > private, we can change it later if we need to.
> > > >
> > > > I definitely agree that we should use the scheme you describe for
> sending
> > > > ACLs over the wire (just the string + version number)
> > > >
> > > > cheers,
> > > > Colin
> > > >
> > > >
> > > > On Fri, May 11, 2018, at 09:39, Andy Coates wrote:
> > > > > i think I'm agreeing with you. I was merely suggesting that having
> an
> > > > > additional field that controls how the current field is
> interpreted is
> > > > more
> > > > > flexible / extensible in the future than using a 'union' style
> > > approach,
> > > > > where only one of several possible fields should be populated. But
> > > it's a
> > > > > minor thing.
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > On 10 May 2018 at 09:29, Colin McCabe <cmcc...@apache.org> wrote:
> > > > >
> > > > > > Hi Andy,
> > > > > >
> > > > > > The issue that I was trying to solve here is the Java API. Right
> > > now,
> > > > > > someone can write "new ResourceFilter(ResourceType.
> TRANSACTIONAL_ID,
> > > > > > "foo*") and have a ResourceFilter that applies to a
> Transactional ID
> > > > named
> > > > > > "foo*". This has to continue to work, or else we have broken
> > > > compatibility.
> > > > > >
> > > > > > I was proposing that there would be something like a new function
> > > like
> > > > > > ResourceFilter.fromPattern(ResourceType.TRANSACTIONAL_ID,
> "foo*")
> > > > which
> > > > > > would create a ResourceFilter that applied to transactional IDs
> > > > starting
> > > > > > with "foo", rather than transactional IDs named "foo*"
> specifically.
> > > > > >
> > > > > > I don't think it's important whether the Java class has an
> integer,
> > > an
> > > > > > enum, or two string fields. The important thing is that there's
> a
> > > new
> > > > > > static function, or new constructor overload, etc. that works for
> > > > patterns
> > > > > > rather than literal strings.
> > > > > >
> > > > > > On Thu, May 10, 2018, at 03:30, Andy Coates wrote:
> > > > > > > Rather than having name and pattern fields on the
> ResourceFilter,
> > > > where
> > > > > > > it’s only valid for one to be set, and we want to restrict the
> > > > character
> > > > > > > set in case future enhancements need them, we could instead
> add a
> > > new
> > > > > > > integer ‘nameType’ field, and use constants to indicate how the
> > > name
> > > > > > > field should be interpreted, e.g. 0 = literal, 1 = wildcard.
> This
> > > > would
> > > > > > > be extendable, e.g we can later add 2 = regex, or what ever,
> and
> > > > > > > wouldn’t require any escaping.
> > > > > >
> > > > > > This is very user-unfriendly, though. Users don't want to have
> to
> > > > > > explicitly supply a version number when using the API, which is
> what
> > > > this
> > > > > > would force them to do. I don't think users are going to want to
> > > > memorize
> > > > > > that version 4 supprted "+", whereas version 3 only supported
> > > "[0-9]",
> > > > or
> > > > > > whatever.
> > > > > >
> > > > > > Just as an example, do you remember which versions of
> FetchRequest
> > > > added
> > > > > > which features? I don't. I always have to look at the code to
> > > > remember.
> > > > > >
> > > > > > Also, escaping is still required any time you overload a
> character to
> > > > mean
> > > > > > two things. Escaping is required in the current proposal to be
> able
> > > to
> > > > > > create a pattern that matches only "foo*". You have to type
> "foo\*"
> > > > It
> > > > > > would be required if we forced users to specify a version, as
> well.
> > > > > >
> > > > > > best,
> > > > > > Colin
> > > > > >
> > > > > > >
> > > > > > > Sent from my iPhone
> > > > > > >
> > > > > > > > On 7 May 2018, at 05:16, Piyush Vijay <
> piyushvij...@gmail.com>
> > > > wrote:
> > > > > > > >
> > > > > > > > Makes sense. I'll update the KIP.
> > > > > > > >
> > > > > > > > Does anyone have any other comments? :)
> > > > > > > >
> > > > > > > > Thanks
> > > > > > > >
> > > > > > > >
> > > > > > > > Piyush Vijay
> > > > > > > >
> > > > > > > >> On Thu, May 3, 2018 at 11:55 AM, Colin McCabe <
> > > cmcc...@apache.org
> > > > >
> > > > > > wrote:
> > > > > > > >>
> > > > > > > >> Yeah, I guess that's a good point. It probably makes sense
> to
> > > > > > support the
> > > > > > > >> prefix scheme for consumer groups and transactional IDs as
> well
> > > as
> > > > > > topics.
> > > > > > > >>
> > > > > > > >> I agree that the current situation where anything goes in
> > > consumer
> > > > > > group
> > > > > > > >> names and transactional ID names is not ideal. I wish we
> could
> > > > > > rewind the
> > > > > > > >> clock and impose restrictions on the names. However, it
> doesn't
> > > > seem
> > > > > > > >> practical at the moment. Adding new restrictions would
> break a
> > > > lot of
> > > > > > > >> existing users after an upgrade. It would be a really bad
> > > upgrade
> > > > > > > >> experience.
> > > > > > > >>
> > > > > > > >> However, I think we can support this in a compatible way.
> From
> > > > the
> > > > > > > >> perspective of AdminClient, we just have to add a new field
> to
> > > > > > > >> ResourceFilter. Currently, it has two fields, resourceType
> and
> > > > name:
> > > > > > > >>
> > > > > > > >>> /**
> > > > > > > >>> * A filter which matches Resource objects.
> > > > > > > >>> *
> > > > > > > >>> * The API for this class is still evolving and we may break
> > > > > > > >> compatibility in minor releases, if necessary.
> > > > > > > >>> */
> > > > > > > >>> @InterfaceStability.Evolving
> > > > > > > >>> public class ResourceFilter {
> > > > > > > >>> private final ResourceType resourceType;
> > > > > > > >>> private final String name;
> > > > > > > >>
> > > > > > > >> We can add a third field, pattern.
> > > > > > > >>
> > > > > > > >> So the API will basically be, if I create a
> > > > > > ResourceFilter(resourceType=GROUP,
> > > > > > > >> name=foo*, pattern=null), it applies only to the consumer
> group
> > > > named
> > > > > > > >> "foo*". If I create a ResourceFilter(resourceType=GROUP,
> > > > name=null,
> > > > > > > >> pattern=foo*), it applies to any consumer group starting in
> > > "foo".
> > > > > > name
> > > > > > > >> and pattern cannot be both set at the same time. This
> preserves
> > > > > > > >> compatibility at the AdminClient level.
> > > > > > > >>
> > > > > > > >> It's possible that we will want to add more types of
> pattern in
> > > > the
> > > > > > > >> future. So we should reserve "special characters" such as
> +, /,
> > > > &,
> > > > > > %, #,
> > > > > > > >> $, etc. These characters should be treated as special
> unless
> > > > they are
> > > > > > > >> prefixed with a backslash to escape them. This will allow
> us to
> > > > add
> > > > > > > >> support for using these characters in the future without
> > > breaking
> > > > > > > >> compatibility.
> > > > > > > >>
> > > > > > > >> At the protocol level, we need a new API version for
> > > > > > CreateAclsRequest /
> > > > > > > >> DeleteAclsRequest. The new API version will send all
> special
> > > > > > characters
> > > > > > > >> over the wire escaped rather than directly. (So there is no
> > > need
> > > > for
> > > > > > the
> > > > > > > >> equivalent of both "name" and "pattern"-- we translate name
> > > into a
> > > > > > validly
> > > > > > > >> escaped pattern that matches only one thing, by adding
> escape
> > > > > > characters as
> > > > > > > >> appropriate.) The broker will validate the new API version
> and
> > > > reject
> > > > > > > >> malformed of unsupported patterns.
> > > > > > > >>
> > > > > > > >> At the ZK level, we can introduce a protocol version to the
> data
> > > > in
> > > > > > ZK--
> > > > > > > >> or store it under a different root.
> > > > > > > >>
> > > > > > > >> best,
> > > > > > > >> Colin
> > > > > > > >>
> > > > > > > >>
> > > > > > > >>> On Wed, May 2, 2018, at 18:09, Piyush Vijay wrote:
> > > > > > > >>> Thank you everyone for the interest and, prompt and
> valuable
> > > > > > feedback. I
> > > > > > > >>> really appreciate the quick turnaround. I’ve tried to
> organize
> > > > the
> > > > > > > >> comments
> > > > > > > >>> into common headings. See my replies below:
> > > > > > > >>>
> > > > > > > >>>
> > > > > > > >>>
> > > > > > > >>> *Case of ‘*’ might already be present in consumer groups
> and
> > > > > > > >> transactional
> > > > > > > >>> ids*
> > > > > > > >>>
> > > > > > > >>>
> > > > > > > >>>
> > > > > > > >>> - We definitely need wildcard ACLs support for resources
> like
> > > > > > consumer
> > > > > > > >>> groups and transactional ids for the reason Andy
> mentioned. A
> > > > big
> > > > > > win
> > > > > > > >> of
> > > > > > > >>> this feature is that service providers don’t have to
> track
> > > and
> > > > keep
> > > > > > > >>> up-to-date all the consumer groups their customers are
> using.
> > > > > > > >>> - I agree with Andy’s thoughts on the two possible ways.
> > > > > > > >>> - My vote would be to do the breaking change because we
> > > should
> > > > > > > >> restrict
> > > > > > > >>> the format of consumer groups and transactional ids
> sooner
> > > than
> > > > > > later.
> > > > > > > >>> - Consumer groups and transactional ids are basic
> Kafka
> > > > > > concepts.
> > > > > > > >>> There is a lot of value in having a defined naming
> > > > convention on
> > > > > > > >> these
> > > > > > > >>> concepts.
> > > > > > > >>> - This will help us not run into more issues down the
> > > line.
> > > > > > > >>> - I’m not sure if people actually use ‘*’ in their
> > > consumer
> > > > > > group
> > > > > > > >>> names anyway.
> > > > > > > >>> - Escaping ‘*’ isn’t trivial because ‘\’ is an allowed
> > > > character
> > > > > > > >> too.
> > > > > > > >>>
> > > > > > > >>>
> > > > > > > >>> *Why introduce two new APIs?*
> > > > > > > >>>
> > > > > > > >>>
> > > > > > > >>>
> > > > > > > >>> - It’s possible to make this change without introducing
> new
> > > > APIs
> > > > > > but
> > > > > > > >> new
> > > > > > > >>> APIs are required for inspection.
> > > > > > > >>> - For example: If I want to fetch all ACLs that match
> > > > ’topicA*’,
> > > > > > it’s
> > > > > > > >>> not possible without introducing new APIs AND maintaining
> > > > backwards
> > > > > > > >>> compatibility.
> > > > > > > >>>
> > > > > > > >>>
> > > > > > > >>> *Matching multiple hosts*
> > > > > > > >>>
> > > > > > > >>>
> > > > > > > >>>
> > > > > > > >>> - Rajini is right that wildcard ACLs aren’t the correct
> fit
> > > for
> > > > > > > >>> specifying range of hosts.
> > > > > > > >>> - We will rely on KIP-252 for the proper functionality (
> > > > > > > >>>
> > > > > > > >>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > > > > > >> 252+-+Extend+ACLs+to+allow+filtering+based+on+ip+ranges+
> > > > and+subnets
> > > > > > > >>> )
> > > > > > > >>>
> > > > > > > >>>
> > > > > > > >>> *Implementation of matching algorithm and performance
> concerns*
> > > > > > > >>>
> > > > > > > >>>
> > > > > > > >>>
> > > > > > > >>> - Updated the KIP with an implementation.
> > > > > > > >>> - Andy, you’re right. The length doesn’t play a part. The
> > > > request
> > > > > > will
> > > > > > > >>> be authorized *iff* there is at least one matching ALLOW
> and
> > > no
> > > > > > > >> matching
> > > > > > > >>> DENY irrespective of the prefix length. Included this
> detail
> > > > in the
> > > > > > > >> KIP.
> > > > > > > >>> - Since everything is stored in memory, the performance
> hit
> > > > isn’t
> > > > > > > >> really
> > > > > > > >>> significantly worse than the current behavior.
> > > > > > > >>> - Stephane’s performance improvement suggestion is a
> great
> > > > idea but
> > > > > > > >>> orthogonal.
> > > > > > > >>>
> > > > > > > >>>
> > > > > > > >>> *Why extend this for principal?*
> > > > > > > >>>
> > > > > > > >>>
> > > > > > > >>>
> > > > > > > >>> - Thanks for the amazing points Rajini.
> > > > > > > >>> - There is a use case where ALL principals from an org
> might
> > > > want
> > > > > > to
> > > > > > > >>> access fix set of topics.
> > > > > > > >>> - User group functionality is currently missing.
> > > > > > > >>> - IIRC SASL doesn’t use custom principal builder.
> > > > > > > >>> - However, prefixing is not the right choice in all
> cases.
> > > > Agreed.
> > > > > > > >>> - Thoughts?
> > > > > > > >>>
> > > > > > > >>>
> > > > > > > >>> *Changes to AdminClient to support wildcard ACLs*
> > > > > > > >>>
> > > > > > > >>>
> > > > > > > >>>
> > > > > > > >>> - Thanks Colin for the implementation. It’s good to have
> you
> > > > and
> > > > > > > >>> others
> > > > > > > >>> here for the expert opinions.
> > > > > > > >>> - The current implementation uses two classes:
> AclBinding and
> > > > > > > >>> AclBindingFilter. (
> > > > > > > >>>
> > > > > > > >>> https://github.com/apache/kafka/blob/trunk/clients/src/
> > > > > > > >> main/java/org/apache/kafka/common/acl/AclBinding.java
> > > > > > > >>> and
> > > > > > > >>>
> > > > > > > >>> https://github.com/apache/kafka/blob/trunk/clients/src/
> > > > > > > >> main/java/org/apache/kafka/common/acl/AclBindingFilter.java
> > > > > > > >>> )
> > > > > > > >>> - AclBinding is definition of an Acl. It’s used to create
> > > ACLs.
> > > > > > > >>> - AclBindingFilter is used to fetch or delete “matching’
> > > ACLs.
> > > > > > > >>> - In the context of wildcard suffixed ACLs, a stored ACL
> may
> > > > have
> > > > > > ‘*’
> > > > > > > >>> in
> > > > > > > >>> it. *It really removes the distinction between these two
> > > > classes.*
> > > > > > > >>> - The current implementation uses ‘null’ to define
> wildcard
> > > ACL
> > > > > > > >>> (‘*’). I
> > > > > > > >>> think it’s not a good pattern and we should use ‘*’ for
> the
> > > > > > wildcard
> > > > > > > >>> ACL (
> > > > > > > >>>
> > > > > > > >>> https://github.com/apache/kafka/blob/trunk/clients/src/
> > > > > > > >> main/java/org/apache/kafka/common/acl/AclBindingFilter.
> java#L39
> > > > > > > >>> and
> > > > > > > >>>
> > > > > > > >>> https://github.com/apache/kafka/blob/trunk/clients/src/
> > > > > > > >> main/java/org/apache/kafka/common/resource/
> > > > ResourceFilter.java#L37
> > > > > > > >>> ).
> > > > > > > >>> - However, the above two changes are breaking change but
> it
> > > > should
> > > > > > be
> > > > > > > >>> acceptable because the API is marked with
> > > > > > > >>> @InterfaceStability.Evolving.
> > > > > > > >>> - If everyone agrees to the above two changes (merging
> the
> > > two
> > > > > > > >>> classes
> > > > > > > >>> and using non-null values for blanket access), the only
> other
> > > > > > change
> > > > > > > >>> is
> > > > > > > >>> using the matching algorithm from the KIP to match ACLs.
> > > > > > > >>>
> > > > > > > >>>
> > > > > > > >>> Other comments:
> > > > > > > >>>
> > > > > > > >>>
> > > > > > > >>>
> > > > > > > >>> - > It may be worth excluding delegation token ACLs from
> > > using
> > > > > > > >> prefixed
> > > > > > > >>> wildcards since it doesn't make much sense.
> > > > > > > >>>
> > > > > > > >>> I want to ask for clarification on what delegation token
> ACLs
> > > are
> > > > > > before
> > > > > > > >>> commenting. Wildcard suffixed ACLs are supported only for
> > > > resource
> > > > > > and
> > > > > > > >>> principal names.
> > > > > > > >>>
> > > > > > > >>>
> > > > > > > >>>
> > > > > > > >>> - A quick read makes me believe that I’ve fixed the
> > > formatting
> > > > > > issues
> > > > > > > >>> reported by Ted. Let me know if something is still wrong
> and
> > > I
> > > > > > would
> > > > > > > >> be
> > > > > > > >>> happy to fix it.
> > > > > > > >>> - I’ve fixed the mismatch in signature reported by Ron.
> > > > > > > >>> - Andy, I’ve updated the KIP with the security hole
> related
> > > to
> > > > DENY
> > > > > > > >>> wildcard ACLs ‘*’ on the downgrade path.
> > > > > > > >>> - Wrt naming, wildcard suffixed ACLs sound reasonable to
> me
> > > > until
> > > > > > > >>> someone raise a major objection.
> > > > > > > >>>
> > > > > > > >>>
> > > > > > > >>>
> > > > > > > >>> Let me know your thoughts. Looking forward to the next
> > > iteration.
> > > > > > > >>>
> > > > > > > >>>
> > > > > > > >>> Best,
> > > > > > > >>>
> > > > > > > >>> Piyush
> > > > > > > >>>
> > > > > > > >>>
> > > > > > > >>>
> > > > > > > >>>
> > > > > > > >>>
> > > > > > > >>> Piyush Vijay
> > > > > > > >>>
> > > > > > > >>> On Wed, May 2, 2018 at 1:33 PM, Andy Coates <
> a...@confluent.io
> > > >
> > > > > > wrote:
> > > > > > > >>>
> > > > > > > >>>>> Perhaps there is a simpler way. It seems like the
> resource
> > > > that
> > > > > > > >> people
> > > > > > > >>>> really want to use prefixed ACLs with is topic names.
> Because
> > > > topic
> > > > > > > >> names
> > > > > > > >>>> can't contain "*", there is no ambiguity there, either. I
> > > think
> > > > > > maybe
> > > > > > > >> we
> > > > > > > >>>> should restrict the prefix handling to topic resources.
> > > > > > > >>>>
> > > > > > > >>>> The KIP should cover consumer groups for sure, otherwise
> we're
> > > > back
> > > > > > to
> > > > > > > >> the
> > > > > > > >>>> situation users need to know, up front, the set of
> consumer
> > > > groups
> > > > > > an
> > > > > > > >>>> KStreams topology is going to use.
> > > > > > > >>>>
> > > > > > > >>>> I'm assuming transactional producer Ids would be the same.
> > > > > > > >>>>
> > > > > > > >>>> The cleanest solution would be to restrict the characters
> in
> > > > group
> > > > > > and
> > > > > > > >>>> transaction producer ids, but that's a breaking change
> that
> > > > might
> > > > > > > >> affect a
> > > > > > > >>>> lot of people.
> > > > > > > >>>>
> > > > > > > >>>> Another solution might be to add a protocol version to the
> > > `Acl`
> > > > > > type,
> > > > > > > >> (not
> > > > > > > >>>> the current `version` field used for optimistic
> concurrency
> > > > > > control),
> > > > > > > >>>> defaulting it to version 1 if it is not present, and
> releasing
> > > > this
> > > > > > > >> change
> > > > > > > >>>> as version 2. This would at least allow us to leave the
> > > > version 1
> > > > > > ACLs
> > > > > > > >>>> as-is, (which makes for a cleaner storey if a cluster is
> > > > > > downgraded).
> > > > > > > >> There
> > > > > > > >>>> is then potential to escape '*' when writing version 2
> ACLs.
> > > > (And
> > > > > > > >> introduce
> > > > > > > >>>> code to check for supported ACL versions going forward).
> > > > Though...
> > > > > > how
> > > > > > > >> do
> > > > > > > >>>> we escape? If the consumer group is free form, then any
> escape
> > > > > > > >> sequence is
> > > > > > > >>>> also valid. Aren't there metrics that use the group
> name?
> > > If
> > > > > > there
> > > > > > > >> are,
> > > > > > > >>>> I'd of thought we'd need to restrict the char set anyway.
> > > > > > > >>>>
> > > > > > > >>>>> On 2 May 2018 at 18:57, charly molter <
> > > charly.mol...@gmail.com
> > > > >
> > > > > > wrote:
> > > > > > > >>>>>
> > > > > > > >>>>> The fact that consumer groups and transactionalProducerId
> > > don't
> > > > > > have
> > > > > > > >> a
> > > > > > > >>>>> strict format is problematic (we have problems with
> people
> > > with
> > > > > > empty
> > > > > > > >>>>> spaces at the end of their consumer group for example).
> > > > > > > >>>>> With 2.0 around the corner and the possibility to fix
> these
> > > > errors
> > > > > > > >> from
> > > > > > > >>>> the
> > > > > > > >>>>> past should we create a KIP to restrict these names like
> we
> > > do
> > > > for
> > > > > > > >>>> topics?
> > > > > > > >>>>>
> > > > > > > >>>>> Thanks!
> > > > > > > >>>>> Charly
> > > > > > > >>>>>
> > > > > > > >>>>> On Wed, May 2, 2018 at 6:22 PM Colin McCabe <
> > > > cmcc...@apache.org>
> > > > > > > >> wrote:
> > > > > > > >>>>>
> > > > > > > >>>>>> Hi Piyush,
> > > > > > > >>>>>>
> > > > > > > >>>>>> Thanks for the KIP! It seems like it will be really
> useful.
> > > > > > > >>>>>>
> > > > > > > >>>>>> As Rajini commented, the names for some resources (such
> as
> > > > > > consumer
> > > > > > > >>>>>> groups) can include stars. So your consumer group
> might be
> > > > named
> > > > > > > >>>> "foo*".
> > > > > > > >>>>>> We need a way of explicitly referring to that consumer
> group
> > > > name,
> > > > > > > >>>> rather
> > > > > > > >>>>>> than to the foo prefix. A simple way would be to
> escape the
> > > > star
> > > > > > > >> with
> > > > > > > >>>> a
> > > > > > > >>>>>> backslash: "foo\*" During the software upgrade
> process, we
> > > > also
> > > > > > > >> need
> > > > > > > >>>> to
> > > > > > > >>>>>> translate all ACLs that refer to "foo*" into "foo\*".
> > > > Otherwise,
> > > > > > > >> the
> > > > > > > >>>>>> upgrade could create a security hole by granting more
> access
> > > > than
> > > > > > > >> the
> > > > > > > >>>>>> administrator intended.
> > > > > > > >>>>>>
> > > > > > > >>>>>> Perhaps there is a simpler way. It seems like the
> resource
> > > > that
> > > > > > > >> people
> > > > > > > >>>>>> really want to use prefixed ACLs with is topic names.
> > > Because
> > > > > > > >> topic
> > > > > > > >>>>> names
> > > > > > > >>>>>> can't contain "*", there is no ambiguity there,
> either. I
> > > > think
> > > > > > > >> maybe
> > > > > > > >>>> we
> > > > > > > >>>>>> should restrict the prefix handling to topic resources.
> > > > > > > >>>>>>
> > > > > > > >>>>>> Are the new "getMatchingAcls" methods needed? It seems
> like
> > > > they
> > > > > > > >> break
> > > > > > > >>>>>> encapsulation. All that the calling code really needs
> to do
> > > > is
> > > > > > > >> call
> > > > > > > >>>>>> Authorizer#authorize(session, operation, resource). The
> > > > > > authorizer
> > > > > > > >>>> knows
> > > > > > > >>>>>> that if it has an ACL allowing access to topics starting
> > > with
> > > > > > > >> "foo" and
> > > > > > > >>>>>> someone calls authorize(foobar), it should allow access.
> > > > It's not
> > > > > > > >>>>>> necessary for the calling code to know exactly what the
> > > rules
> > > > are
> > > > > > > >> for
> > > > > > > >>>>>> authorization. The Authorizer#getAcls, APIs are only
> needed
> > > > when
> > > > > > > >> the
> > > > > > > >>>>>> AdminClient wants to list ACLs.
> > > > > > > >>>>>>
> > > > > > > >>>>>> best,
> > > > > > > >>>>>> Colin
> > > > > > > >>>>>>
> > > > > > > >>>>>>
> > > > > > > >>>>>>> On Wed, May 2, 2018, at 03:31, Rajini Sivaram wrote:
> > > > > > > >>>>>>> Hi Piyush,
> > > > > > > >>>>>>>
> > > > > > > >>>>>>> Thanks for the KIP for this widely requested feature.
> A few
> > > > > > > >>>>>>> comments/questions:
> > > > > > > >>>>>>>
> > > > > > > >>>>>>> 1. Some resource names can contain '*'. For example,
> > > consumer
> > > > > > > >> groups
> > > > > > > >>>> or
> > > > > > > >>>>>>> transactional ids. I am wondering whether we need to
> > > restrict
> > > > > > > >>>>> characters
> > > > > > > >>>>>>> for these entities or provide a way to distinguish
> > > wildcarded
> > > > > > > >>>> resource
> > > > > > > >>>>>> from
> > > > > > > >>>>>>> a resource containing the wildcard character.
> > > > > > > >>>>>>>
> > > > > > > >>>>>>> 2. I am not sure we want to do wildcarded principals.
> It
> > > > feels
> > > > > > > >> like a
> > > > > > > >>>>>>> workaround in the absence of user groups. In the longer
> > > > term, I
> > > > > > > >> think
> > > > > > > >>>>>>> groups (without wildcards) would be a better option to
> > > > configure
> > > > > > > >> ACLs
> > > > > > > >>>>> for
> > > > > > > >>>>>>> groups of users, rather than building user principals
> which
> > > > have
> > > > > > > >>>> common
> > > > > > > >>>>>>> prefixes. In the shorter term, since a configurable
> > > > > > > >> PrincipalBuilder
> > > > > > > >>>> is
> > > > > > > >>>>>>> used to build the principal used for authorization,
> perhaps
> > > > > > > >> using the
> > > > > > > >>>>>>> prefix itself as the principal would work (unless
> different
> > > > > > > >> quotas
> > > > > > > >>>> are
> > > > > > > >>>>>>> required for the full user name)? User principal
> strings
> > > take
> > > > > > > >>>> different
> > > > > > > >>>>>>> formats for different security protocols (eg.
> > > > CN=xxx,O=org,C=UK
> > > > > > > >> for
> > > > > > > >>>>> SSL)
> > > > > > > >>>>>>> and simple prefixing isn't probably the right grouping
> in
> > > > many
> > > > > > > >> cases.
> > > > > > > >>>>>>>
> > > > > > > >>>>>>> 3. I am assuming we don't want to do wildcarded hosts
> in
> > > > this KIP
> > > > > > > >>>> since
> > > > > > > >>>>>>> wildcard suffix doesn't really work for hosts.
> > > > > > > >>>>>>>
> > > > > > > >>>>>>> 4. It may be worth excluding delegation token ACLs from
> > > using
> > > > > > > >>>> prefixed
> > > > > > > >>>>>>> wildcards since it doesn't make much sense.
> > > > > > > >>>>>>>
> > > > > > > >>>>>>> Regards,
> > > > > > > >>>>>>>
> > > > > > > >>>>>>> Rajini
> > > > > > > >>>>>>>
> > > > > > > >>>>>>>
> > > > > > > >>>>>>> On Wed, May 2, 2018 at 2:05 AM, Stephane Maarek <
> > > > > > > >>>>>>> steph...@simplemachines.com.au> wrote:
> > > > > > > >>>>>>>
> > > > > > > >>>>>>>> Hi, thanks for this badly needed feature
> > > > > > > >>>>>>>>
> > > > > > > >>>>>>>> 1) Why introduce two new APIs in authorizer instead of
> > > > > > > >> replacing
> > > > > > > >>>> the
> > > > > > > >>>>>>>> implementation for simple ACL authorizer with adding
> the
> > > > > > > >> wildcard
> > > > > > > >>>>>>>> capability?
> > > > > > > >>>>>>>>
> > > > > > > >>>>>>>> 2) is there an impact to performance as now we're
> > > evaluating
> > > > > > > >> more
> > > > > > > >>>>>> rules ? A
> > > > > > > >>>>>>>> while back I had evaluated the concept of cached Acl
> > > result
> > > > so
> > > > > > > >>>>> swallow
> > > > > > > >>>>>> the
> > > > > > > >>>>>>>> cost of computing an authorisation cost once and then
> > > doing
> > > > in
> > > > > > > >>>> memory
> > > > > > > >>>>>>>> lookups. CF: https://issues.apache.org/
> > > > jira/browse/KAFKA-5261
> > > > > > > >>>>>>>>
> > > > > > > >>>>>>>> 3) is there any need to also extend this to consumer
> group
> > > > > > > >>>> resources
> > > > > > > >>>>> ?
> > > > > > > >>>>>>>>
> > > > > > > >>>>>>>> 4) create topics KIP as recently moved permissions
> out of
> > > > > > > >> Cluster
> > > > > > > >>>>> into
> > > > > > > >>>>>>>> Topic. Will wildcard be supported for this action too?
> > > > > > > >>>>>>>>
> > > > > > > >>>>>>>> Thanks a lot for this !
> > > > > > > >>>>>>>>
> > > > > > > >>>>>>>> On Wed., 2 May 2018, 1:37 am Ted Yu, <
> yuzhih...@gmail.com
> > > >
> > > > > > > >> wrote:
> > > > > > > >>>>>>>>
> > > > > > > >>>>>>>>> w.r.t. naming, we can keep wildcard and drop
> 'prefixed'
> > > (or
> > > > > > > >>>>>> 'suffixed')
> > > > > > > >>>>>>>>> since the use of regex would always start with
> > > non-wildcard
> > > > > > > >>>>> portion.
> > > > > > > >>>>>>>>>
> > > > > > > >>>>>>>>> Cheers
> > > > > > > >>>>>>>>>
> > > > > > > >>>>>>>>> On Tue, May 1, 2018 at 12:13 PM, Andy Coates <
> > > > > > > >> a...@confluent.io>
> > > > > > > >>>>>> wrote:
> > > > > > > >>>>>>>>>
> > > > > > > >>>>>>>>>> Hi Piyush,
> > > > > > > >>>>>>>>>>
> > > > > > > >>>>>>>>>> Can you also document in the Compatibility section
> what
> > > > > > > >> would
> > > > > > > >>>>>> happen
> > > > > > > >>>>>>>>> should
> > > > > > > >>>>>>>>>> the cluster be upgraded, wildcard-suffixed ACLs are
> > > added,
> > > > > > > >> and
> > > > > > > >>>>>> then the
> > > > > > > >>>>>>>>>> cluster is rolled back to the previous version. On
> > > > > > > >> downgrade
> > > > > > > >>>> the
> > > > > > > >>>>>>>> partial
> > > > > > > >>>>>>>>>> wildcard ACLs will be treated as literals and hence
> > > never
> > > > > > > >> match
> > > > > > > >>>>>>>> anything.
> > > > > > > >>>>>>>>>> This is fine for ALLOW ACLs, but some might consider
> > > this
> > > > a
> > > > > > > >>>>>> security
> > > > > > > >>>>>>>> hole
> > > > > > > >>>>>>>>>> if a DENY ACL is ignored, so this needs to be
> > > documented,
> > > > > > > >> both
> > > > > > > >>>> in
> > > > > > > >>>>>> the
> > > > > > > >>>>>>>> KIP
> > > > > > > >>>>>>>>>> and the final docs.
> > > > > > > >>>>>>>>>>
> > > > > > > >>>>>>>>>> For some reason I find the term 'prefixed wildcard
> ACLs'
> > > > > > > >> easier
> > > > > > > >>>>> to
> > > > > > > >>>>>> grok
> > > > > > > >>>>>>>>>> than 'wildcard suffixed ACLs'. Probably because in
> the
> > > > > > > >> former
> > > > > > > >>>> the
> > > > > > > >>>>>>>>>> 'wildcard' term comes after the positional
> adjective,
> > > > which
> > > > > > > >>>>>> matches the
> > > > > > > >>>>>>>>>> position of the wildcard char in the resource name,
> i.e.
> > > > > > > >>>> "some*".
> > > > > > > >>>>>> It's
> > > > > > > >>>>>>>>>> most likely a person thing, but I thought I'd
> mention it
> > > > as
> > > > > > > >>>>> naming
> > > > > > > >>>>>> is
> > > > > > > >>>>>>>>>> important when it comes to making this initiative
> for
> > > > > > > >> users.
> > > > > > > >>>>>>>>>>
> > > > > > > >>>>>>>>>> On 1 May 2018 at 19:57, Andy Coates <
> a...@confluent.io>
> > > > > > > >> wrote:
> > > > > > > >>>>>>>>>>
> > > > > > > >>>>>>>>>>> Hi Piyush,
> > > > > > > >>>>>>>>>>>
> > > > > > > >>>>>>>>>>> Thanks for raising this KIP - it's very much
> > > appreciated.
> > > > > > > >>>>>>>>>>>
> > > > > > > >>>>>>>>>>> I've not had chance to digest it yet, but...
> > > > > > > >>>>>>>>>>>
> > > > > > > >>>>>>>>>>> 1. you might want to add details of how the
> internals
> > > of
> > > > > > > >> the
> > > > > > > >>>>>>>>>>> `getMatchingAcls` is implemented. We'd want to make
> > > sure
> > > > > > > >> the
> > > > > > > >>>>>>>> complexity
> > > > > > > >>>>>>>>>> of
> > > > > > > >>>>>>>>>>> the operation isn't adversely affected.
> > > > > > > >>>>>>>>>>> 2. You might want to be more explicit that the
> length
> > > of
> > > > > > > >> a
> > > > > > > >>>>> prefix
> > > > > > > >>>>>>>> does
> > > > > > > >>>>>>>>>> not
> > > > > > > >>>>>>>>>>> play a part in the `authorize` call, e.g. given
> ACLs
> > > > > > > >> {DENY,
> > > > > > > >>>>>> some.*},
> > > > > > > >>>>>>>>>> {ALLOW,
> > > > > > > >>>>>>>>>>> some.prefix.*}, the longer, i.e. more specific,
> allow
> > > ACL
> > > > > > > >>>> does
> > > > > > > >>>>>> _not_
> > > > > > > >>>>>>>>>>> override the more general deny ACL.
> > > > > > > >>>>>>>>>>>
> > > > > > > >>>>>>>>>>> Thanks,
> > > > > > > >>>>>>>>>>>
> > > > > > > >>>>>>>>>>> Andy
> > > > > > > >>>>>>>>>>>
> > > > > > > >>>>>>>>>>> On 1 May 2018 at 16:59, Ron Dagostino <
> > > rndg...@gmail.com
> > > > > > > >>>
> > > > > > > >>>>> wrote:
> > > > > > > >>>>>>>>>>>
> > > > > > > >>>>>>>>>>>> Hi Piyush. I appreciated your talk at Kafka
> Summit
> > > and
> > > > > > > >>>>>> appreciate
> > > > > > > >>>>>>>> the
> > > > > > > >>>>>>>>>> KIP
> > > > > > > >>>>>>>>>>>> -- thanks.
> > > > > > > >>>>>>>>>>>>
> > > > > > > >>>>>>>>>>>> Could you explain these mismatching references?
> Near
> > > > > > > >> the
> > > > > > > >>>> top
> > > > > > > >>>>>> of the
> > > > > > > >>>>>>>>> KIP
> > > > > > > >>>>>>>>>>>> you refer to these proposed new method signatures:
> > > > > > > >>>>>>>>>>>>
> > > > > > > >>>>>>>>>>>> def getMatchingAcls(resource: Resource): Set[Acl]
> > > > > > > >>>>>>>>>>>> def getMatchingAcls(principal: KafkaPrincipal):
> > > > > > > >>>> Map[Resource,
> > > > > > > >>>>>>>>> Set[Acl]]
> > > > > > > >>>>>>>>>>>>
> > > > > > > >>>>>>>>>>>> But near the bottom of the KIP you refer to
> different
> > > > > > > >> method
> > > > > > > >>>>>>>>>>>> signatures that don't seem to match the above
> ones:
> > > > > > > >>>>>>>>>>>>
> > > > > > > >>>>>>>>>>>> getMatchingAcls(topicRegex)
> > > > > > > >>>>>>>>>>>> getMatchingAcls(principalRegex)
> > > > > > > >>>>>>>>>>>>
> > > > > > > >>>>>>>>>>>> Ron
> > > > > > > >>>>>>>>>>>>
> > > > > > > >>>>>>>>>>>>
> > > > > > > >>>>>>>>>>>> On Tue, May 1, 2018 at 11:48 AM, Ted Yu <
> > > > > > > >>>> yuzhih...@gmail.com>
> > > > > > > >>>>>>>> wrote:
> > > > > > > >>>>>>>>>>>>
> > > > > > > >>>>>>>>>>>>> The KIP was well written. Minor comment on
> > > formatting:
> > > > > > > >>>>>>>>>>>>>
> > > > > > > >>>>>>>>>>>>> https://github.com/apache/
> kafka/blob/trunk/core/src/
> > > > > > > >>>>>>>>>>>>> main/scala/kafka/admin/AclCommand.scala
> > > > > > > >>>>>>>>>>>>> to
> > > > > > > >>>>>>>>>>>>>
> > > > > > > >>>>>>>>>>>>> Leave space between the URL and 'to'
> > > > > > > >>>>>>>>>>>>>
> > > > > > > >>>>>>>>>>>>> Can you describe changes for the AdminClient ?
> > > > > > > >>>>>>>>>>>>>
> > > > > > > >>>>>>>>>>>>> Thanks
> > > > > > > >>>>>>>>>>>>>
> > > > > > > >>>>>>>>>>>>> On Tue, May 1, 2018 at 8:12 AM, Piyush Vijay <
> > > > > > > >>>>>>>>> piyushvij...@gmail.com>
> > > > > > > >>>>>>>>>>>>> wrote:
> > > > > > > >>>>>>>>>>>>>
> > > > > > > >>>>>>>>>>>>>> Hi all,
> > > > > > > >>>>>>>>>>>>>>
> > > > > > > >>>>>>>>>>>>>> I just opened a KIP to add support for wildcard
> > > > > > > >> suffixed
> > > > > > > >>>>>> ACLs.
> > > > > > > >>>>>>>>> This
> > > > > > > >>>>>>>>>> is
> > > > > > > >>>>>>>>>>>>> one
> > > > > > > >>>>>>>>>>>>>> of the feature I talked about in my Kafka summit
> > > > > > > >> talk
> > > > > > > >>>> and
> > > > > > > >>>>> we
> > > > > > > >>>>>>>>>> promised
> > > > > > > >>>>>>>>>>>> to
> > > > > > > >>>>>>>>>>>>>> upstream it :)
> > > > > > > >>>>>>>>>>>>>>
> > > > > > > >>>>>>>>>>>>>> The details are here -
> > > > > > > >>>>>>>>>>>>>> https://cwiki.apache.org/
> > > > > > > >> confluence/display/KAFKA/KIP-
> > > > > > > >>>>>>>>>>>>>> 290%3A+Support+for+wildcard+suffixed+ACLs
> > > > > > > >>>>>>>>>>>>>>
> > > > > > > >>>>>>>>>>>>>> There is an open question about the way to add
> the
> > > > > > > >>>> support
> > > > > > > >>>>>> in
> > > > > > > >>>>>>>> the
> > > > > > > >>>>>>>>>>>>>> AdminClient, which I can discuss here in more
> detail
> > > > > > > >>>> once
> > > > > > > >>>>>>>> everyone
> > > > > > > >>>>>>>>>> has
> > > > > > > >>>>>>>>>>>>>> taken a first look at the KIP.
> > > > > > > >>>>>>>>>>>>>>
> > > > > > > >>>>>>>>>>>>>> Looking forward to discuss the change.
> > > > > > > >>>>>>>>>>>>>>
> > > > > > > >>>>>>>>>>>>>> Best,
> > > > > > > >>>>>>>>>>>>>> Piyush Vijay
> > > > > > > >>>>>>>>>>>>>>
> > > > > > > >>>>>>>>>>>>>
> > > > > > > >>>>>>>>>>>>
> > > > > > > >>>>>>>>>>>
> > > > > > > >>>>>>>>>>>
> > > > > > > >>>>>>>>>>
> > > > > > > >>>>>>>>>
> > > > > > > >>>>>>>>
> > > > > > > >>>>>>
> > > > > > > >>>>>
> > > > > > > >>>>>
> > > > > > > >>>>> --
> > > > > > > >>>>> Charly Molter
> > > > > > > >>>>>
> > > > > > > >>>>
> > > > > > > >>
> > > > > >
> > > >
> > >
>
