Hi Piyush,
It is possible to configure PrincipalBuilder for SASL (
https://cwiki.apache.org/confluence/display/KAFKA/KIP-189%3A+Improve+principal+builder+interface+and+add+support+for+SASL).
If that satisfies your requirements, perhaps we can move wildcarded
principals out of this KIP and focus on wildcarded resources?
On Tue, May 15, 2018 at 7:15 PM, Piyush Vijay <piyushvij...@gmail.com>
wrote:
> 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
> > > > > > > > >>>>>
> > > > > > > > >>>>
> > > > > > > > >>
> > > > > > >
> > > > >
> > > >
> >
>
