[DISCUSS] KIP-565: Using AclCommand,avoid call the global method loadcache in SimpleAclAuthorizer

[Steven Lu]

Hello all,

In the class Named AclCommand,configure SimpleAclAuthorizer,but no need call 
loadCache.
now we have 20,000 topics in kafka cluster,everytime I run AclCommand,all these 
topics's Alcs need to be authed, it will be very slow.
The purpose of this optimization is：we can choose to not load the acl of all 
topics into memory, mainly for adding and deleting permissions.

PR Available here: https://github.com/apache/kafka/pull/7706
KIP Available here: 
https://cwiki.apache.org/confluence/display/KAFKA/KIP-565%3A+Using+AclCommand%2Cavoid+call+the+global+method+loadcache+in+SimpleAclAuthorizer
Issue Available here: https://issues.apache.org/jira/browse/KAFKA-9424

mainly for adding and deleting permissions,we can choose to not load the acl of 
all topics into memory,then we can add two args "--load-acl-cache" "false" in 
AclCommand.main;else you don't add these args, it will load the acl cache 
defaultly.

we can choose improve the running time from minutes to less than one second.

Thanks,
Steven

[VOTE] KIP-547: Extend ConsumerInterceptor to allow modification of Consumer Commits

[Eric Azama]

Hi all,

I'd like to open a vote on KIP-547: Extend ConsumerInterceptor to allow
modification of Consumer Commits

This KIP is looking to improve the access to the Metadata included with
Consumer Commits by adding a method to the ConsumerInterceptor that allows
modification of the commit payloads.

KIP Link:
https://cwiki.apache.org/confluence/display/KAFKA/KIP-547%3A+Extend+ConsumerInterceptor+to+allow+modification+of+Consumer+Commits

Thanks,
Eric A.

Re: [DISCUSS] KIP-390: Allow fine-grained configuration for compression (Rebooted)

[Guozhang Wang]

Hello Dongjin,

I'm wondering if you have looked into the different implementor's buffer
usage? So far as I read from the code:

1. LZ4 used a shared 64KB for decompression, and when reading it used
ByteBuffer copy from the decompression buffer.
2. Snappy used shared uncompressed buffer, but when reading it uses
SnappyNative.arrayCopy
JNI which could be slow.
3. GZIP used shared 8KB (inflator), and another shared 16KB for reading it
out with System.arraycopy.
4. ZSTD: dynamically allocate 128KB (i.e. not shared), and also use read
internal for skip, so skip does not benefit that much.

It seems to me that for different types the buffer used quite differently.
Also, aside from ZSTD, are there any other types that have levels?


Guozhang


On Mon, Jun 24, 2019 at 4:30 PM Dongjin Lee  wrote:

> Hello. Here is the new discussion thread for KIP-390: Allow fine-grained
> configuration for compression.
>
>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-390%3A+Allow+fine-grained+configuration+for+compression
>
> Here is some history: Initially, the draft implementation was done with
> traditional configuration scheme (Type A). Then, Becket Qin (
> becket@gmail.com) and Mickael Maison (mickael.mai...@gmail.com)
> proposed that the map style configuration
> like listener.security.protocol.map
> or max.connections.per.ip.overrides (Type B) would be better. From then on,
> the discussion got struck.
>
> So last weekend, I re-implemented the feature against the latest trunk, for
> all public interface alternatives (i.e., Type A & B.), and updated the KIP
> document. You can find the details in this PR:
> https://github.com/apache/kafka/pull/5927
>
> Please have a look when you are free. All kinds of feedbacks are welcomed!
>
> Regards,
> Dongjin
>
> --
> *Dongjin Lee*
>
> *A hitchhiker in the mathematical world.*
> *github:  github.com/dongjinleekr
> linkedin: kr.linkedin.com/in/dongjinleekr
> speakerdeck:
> speakerdeck.com/dongjin
> *
>


-- 
-- Guozhang

Build failed in Jenkins: kafka-trunk-jdk11 #1097

[Apache Jenkins Server]

See 


Changes:

[manikumar.reddy] MINOR: Update introduction page in Kafka documentation


--
[...truncated 2.84 MB...]
org.apache.kafka.streams.test.ConsumerRecordFactoryTest > 
shouldRequireCustomTopicNameIfNotDefaultFactoryTopicName PASSED

org.apache.kafka.streams.test.ConsumerRecordFactoryTest > 
shouldRequireCustomTopicNameIfNotDefaultFactoryTopicNameWithNullKey STARTED

org.apache.kafka.streams.test.ConsumerRecordFactoryTest > 
shouldRequireCustomTopicNameIfNotDefaultFactoryTopicNameWithNullKey PASSED

org.apache.kafka.streams.test.ConsumerRecordFactoryTest > 
shouldCreateConsumerRecord STARTED

org.apache.kafka.streams.test.ConsumerRecordFactoryTest > 
shouldCreateConsumerRecord PASSED

org.apache.kafka.streams.test.ConsumerRecordFactoryTest > 
shouldCreateNullKeyConsumerRecord STARTED

org.apache.kafka.streams.test.ConsumerRecordFactoryTest > 
shouldCreateNullKeyConsumerRecord PASSED

org.apache.kafka.streams.test.ConsumerRecordFactoryTest > 
shouldCreateConsumerRecordWithOtherTopicName STARTED

org.apache.kafka.streams.test.ConsumerRecordFactoryTest > 
shouldCreateConsumerRecordWithOtherTopicName PASSED

org.apache.kafka.streams.test.ConsumerRecordFactoryTest > shouldAdvanceTime 
STARTED

org.apache.kafka.streams.test.ConsumerRecordFactoryTest > shouldAdvanceTime 
PASSED

org.apache.kafka.streams.test.ConsumerRecordFactoryTest > 
shouldNotAllowToCreateTopicWithNullTopicNameWithKeyValuePairs STARTED

org.apache.kafka.streams.test.ConsumerRecordFactoryTest > 
shouldNotAllowToCreateTopicWithNullTopicNameWithKeyValuePairs PASSED

org.apache.kafka.streams.test.ConsumerRecordFactoryTest > 
shouldRequireCustomTopicNameIfNotDefaultFactoryTopicNameWithKeyValuePairsAndCustomTimestamps
 STARTED

org.apache.kafka.streams.test.ConsumerRecordFactoryTest > 
shouldRequireCustomTopicNameIfNotDefaultFactoryTopicNameWithKeyValuePairsAndCustomTimestamps
 PASSED

org.apache.kafka.streams.test.ConsumerRecordFactoryTest > 
shouldRequireCustomTopicNameIfNotDefaultFactoryTopicNameWithKeyValuePairs 
STARTED

org.apache.kafka.streams.test.ConsumerRecordFactoryTest > 
shouldRequireCustomTopicNameIfNotDefaultFactoryTopicNameWithKeyValuePairs PASSED

org.apache.kafka.streams.test.ConsumerRecordFactoryTest > 
shouldCreateConsumerRecordWithOtherTopicNameAndTimestamp STARTED

org.apache.kafka.streams.test.ConsumerRecordFactoryTest > 
shouldCreateConsumerRecordWithOtherTopicNameAndTimestamp PASSED

org.apache.kafka.streams.test.ConsumerRecordFactoryTest > 
shouldNotAllowToCreateTopicWithNullTopicNameWithKeyValuePairsAndCustomTimestamps
 STARTED

org.apache.kafka.streams.test.ConsumerRecordFactoryTest > 
shouldNotAllowToCreateTopicWithNullTopicNameWithKeyValuePairsAndCustomTimestamps
 PASSED

org.apache.kafka.streams.test.ConsumerRecordFactoryTest > 
shouldNotAllowToCreateTopicWithNullTopicName STARTED

org.apache.kafka.streams.test.ConsumerRecordFactoryTest > 
shouldNotAllowToCreateTopicWithNullTopicName PASSED

org.apache.kafka.streams.test.ConsumerRecordFactoryTest > 
shouldCreateConsumerRecordsFromKeyValuePairsWithCustomTimestampAndIncrementsAndNotAdvanceTime
 STARTED

org.apache.kafka.streams.test.ConsumerRecordFactoryTest > 
shouldCreateConsumerRecordsFromKeyValuePairsWithCustomTimestampAndIncrementsAndNotAdvanceTime
 PASSED

org.apache.kafka.streams.test.ConsumerRecordFactoryTest > 
shouldCreateNullKeyConsumerRecordWithTimestampWithTimestamp STARTED

org.apache.kafka.streams.test.ConsumerRecordFactoryTest > 
shouldCreateNullKeyConsumerRecordWithTimestampWithTimestamp PASSED

org.apache.kafka.streams.test.ConsumerRecordFactoryTest > 
shouldRequireCustomTopicNameIfNotDefaultFactoryTopicNameWithNullKeyAndDefaultTimestamp
 STARTED

org.apache.kafka.streams.test.ConsumerRecordFactoryTest > 
shouldRequireCustomTopicNameIfNotDefaultFactoryTopicNameWithNullKeyAndDefaultTimestamp
 PASSED

org.apache.kafka.streams.test.ConsumerRecordFactoryTest > 
shouldCreateConsumerRecordsFromKeyValuePairsWithTimestampAndIncrements STARTED

org.apache.kafka.streams.test.ConsumerRecordFactoryTest > 
shouldCreateConsumerRecordsFromKeyValuePairsWithTimestampAndIncrements PASSED

org.apache.kafka.streams.MockTimeTest > shouldGetNanosAsMillis STARTED

org.apache.kafka.streams.MockTimeTest > shouldGetNanosAsMillis PASSED

org.apache.kafka.streams.MockTimeTest > shouldSetStartTime STARTED

org.apache.kafka.streams.MockTimeTest > shouldSetStartTime PASSED

org.apache.kafka.streams.MockTimeTest > shouldNotAllowNegativeSleep STARTED

org.apache.kafka.streams.MockTimeTest > shouldNotAllowNegativeSleep PASSED

org.apache.kafka.streams.MockTimeTest > shouldAdvanceTimeOnSleep STARTED

org.apache.kafka.streams.MockTimeTest > shouldAdvanceTimeOnSleep PASSED

org.apache.kafka.streams.internals.KeyValueStoreFacadeTest > shouldReturnIsOpen 
STARTED

org.apache.kafka.streams.internals.KeyValueStoreFacadeTest > shouldReturnIsOpen 
PASSED

Re: [VOTE] KIP-526: Reduce Producer Metadata Lookups for Large Number of Topics

[Guozhang Wang]

Hello Brian,

I looked at the new proposal again, and I'd like to reason about its
rationale from the listed motivations in your wiki:

1) more RPCs: we may send metadata requests more frequently than
appropriate. This is especially the case during producer start-up, where
the more topics it needs to send to, the more metadata requests it needs to
send. This the original reported issue as in KAFKA-8904.

2) large RPCs: we including all topics in the work set when sending
metadata request. But I think our conjecture (as Colin has pointed out) is
that this alone is fine most of the time, assuming e.g. you are sending
such large RPC only once every 10 minutes. It is only because of 1) where
you are sending large RPC too frequently which is a common issue.

3) we want to have a configurable eviction period than hard-coded values. I
consider it as a semi-orthogonal motivation compared with 2) / 3) but we
wanted to piggy-back this fix along with the KIP.

So from there, 1) and 2) does not contradict to each other since our belief
is that large RPCs is usually okay as long as it is not large-and-frequent
RPCs, and we actually prefer large-infrequent RPC > smaller-frequent RPC >
large-and-frequent RPC (of course).

The current proposal basically tries to un-tangle 2) from 1), i.e. for the
scenario of KAFKA-8904 it would result in smaller-frequent RPC during
startup than large-and-frequent RPC. But I'm wondering why don't we just do
even better and make it large-infrequent RPC? More specifically, just like
Lucas suggested in the ticket:

a. when there's new topic with unknown metadata enqueued, instead of
requesting a metadata immediately just delay it for a short period (no more
than seconds) hoping that more unknown topics would be requested in the
period; during this period we would not know which partition it would go to
of course, so we buffer it in a different manner.

b. when we are about to send metadata, if there are unknown topic(s) --
consider them "urgent topics" -- just send them without other topics;
otherwise, send the work set in the request. If we want to go even fancier,
we can still piggy-back some non-urgent along with urgent ones but it is
more complicated to reason about the trade-off so a simpler approach is
fine too.

c. fixing 3) with a new config, which is relatively orthogonal to a) and b).



Guozhang




On Tue, Jan 14, 2020 at 10:39 AM Brian Byrne  wrote:

> Hello all,
>
> After further offline discussion, I've removed any efforts to control
> metadata RPC sizes. There are now only two items proposed in this KIP:
>
> (1) When encountering a new topic, only issue a metadata request for that
> particular topic. For all other cases, continue as it does today with a
> full working set refresh.
>
> (2) Introduces client configuration flag "metadata.eviction.period.ms" to
> control cache eviction duration. I've reset the default back to the current
> (hard-coded) value of 5 minutes since we can identify cases where changing
> it would cause surprises.
>
> The votes have been cleared. My apologies for continually interrupting and
> making changes to the KIP, but hopefully this is an agreeable minimum
> solution to move forward.
>
> Thanks,
> Brian
>
> On Mon, Jan 6, 2020 at 5:23 PM Colin McCabe  wrote:
>
> > On Mon, Jan 6, 2020, at 14:40, Brian Byrne wrote:
> > > So the performance of a metadata RPC that occurs every once every 10
> > > seconds should not be measured strictly in CPU cost, but rather the
> > effect
> > > on the 95-99%. The larger the request is, the more opportunity there is
> > to
> > > put a burst stress on the producer and broker, and the larger the
> > response
> > > payload to push through the control plane socket. Maybe that's not at
> 5k
> > > topics, but there are groups that are 10k+ topics and pushing further.
> >
> > KAFKA-7019 made reading the metadata lock-free.  There is no a priori
> > reason to prefer lots of small requests to a few big requests (within
> > reason!)  In fact, it's quite the opposite: when we make lots of small
> > requests, it uses more network bandwidth than when we make a few big
> ones.
> > There are a few reasons for this: the request and response headers have a
> > fixed overhead, one big array takes less space when serialized than
> several
> > small ones, etc.  There is also TCP and IP overhead, etc.
> >
> > The broker can only push a few tens of thousands of metadata requests a
> > second, due to the overhead of message processing.  This is why almost
> all
> > of the admin commands support batching.  So if you need to create 1,000
> > topics, you make one request, not 1,000 requests, for example.
> >
> > It's definitely reasonable to limit the number of topics made per
> metadata
> > request.  But the reason for this is not improving performance, but
> > preventing certain bad corner cases that happen when RPCs get too big.
> For
> > example, one problem that can happen when a metadata response gets too
> big
> > is that the client 

[VOTE] KIP-562: Allow fetching a key from a single partition rather than iterating over all the stores on an instance

[Navinder Brar]

Hello all,

I'd like to propose a vote to serve keys from a specific partition-store 
instead of iterating over all the local stores of an instance to locate the 
key, as which happens currently.
The full KIP is provided here:
 
https://cwiki.apache.org/confluence/display/KAFKA/KIP-562%3A+Allow+fetching+a+key+from+a+single+partition+rather+than+iterating+over+all+the+stores+on+an+instance


Thanks,
Navinder

Re: [VOTE] KIP-373: Allow users to create delegation tokens for other users

[Viktor Somogyi-Vass]

Hi Rajini,

1) I think we can to keep the conventions in the tool. As an addition we
wouldn't have to retain certain characters (for creating the list).
2) Yes, so based on 1) and this --users changes to --user-principal (and
accepts one single user principal).
3) Looking at it again probably we'll want to increase the version of the
ACL protocols as new resource and operation types are getting added and
currently sending such requests to old brokers would result in
serialization errors. So it would be nicer to handle them on the API
handshake. Besides this I don't see if we need to do anything else as these
operations should be able to handle these changes on the code level. I'll
make sure to test this ACL scenario and report back about it (although I
need a few days as the code I have is very old and contains a lot of
conflicts with the current trunk). Please let me know if I'm missing
something here.

Thanks,
Viktor

On Fri, Jan 17, 2020 at 5:23 PM Rajini Sivaram 
wrote:

> Hi Viktor,
>
> Thanks for the KIP. A few questions:
>
> 1) kafka-acls.sh has options like* --topic* that specifies a single topic.
> Is there a reason why we want to have *--users* instead of *--user *with a
> single user?
> 2) We use user principal rather than just the name everywhere else. Can we
> do the same here, or do we not want to treat this as a principal?
> 3) If we update AclCommand, don't we also need equivalent AdminClient
> changes to configure this ACL? I believe we are deprecating ZK-based ACL
> updates, so we need to add this to AdminClient?
>
> Regards,
>
> Rajini
>
> On Fri, Jan 17, 2020 at 3:15 PM Viktor Somogyi-Vass <
> viktorsomo...@gmail.com>
> wrote:
>
> > Hi Jun & Richard,
> >
> > Jun, thanks for your feedback and vote.
> >
> > 100. Thanks, I'll correct that.
> >
> > 101. (@Richard) in this case the principal names will be something like
> > "CN=writeuser,OU=Unknown,O=Unknown,L=Unknown,ST=Unknown,C=Unknown" unless
> > principal mapping or builder is defined (refer to [1]). I think Jun was
> > referring to this case which is correct, semicolon seems to be a better
> fit
> > in this case.
> >
> > Viktor
> >
> > https://docs.confluent.io/current/kafka/authorization.html
> >
> > On Thu, Jan 16, 2020 at 11:45 PM Richard Yu 
> > wrote:
> >
> > > Hi Jun,
> > >
> > > Can the SSL username really include the comma?
> > >
> > > From what I could tell, when I searched it up, I couldn't find anything
> > > that indicated comma can be a delimiter.
> > > A related doc below:
> > > https://knowledge.digicert.com/solution/SO12401.html
> > >
> > > Cheers,
> > > Richard
> > >
> > >
> > >
> > >
> > > On Thu, Jan 16, 2020 at 1:37 PM Jun Rao  wrote:
> > >
> > > > Hi, Viktor,
> > > >
> > > > Thanks for the KIP. +1 from me. Just a couple of minor comments
> below.
> > > >
> > > > 100. CreateDelegationTokenResponse/DescribeDelegationTokenResponse.
> It
> > > > seems that "validVersions" should be "0-2".
> > > >
> > > > 101. The option --users "owner1,owner2" in AclCommand. Since SSL user
> > > name
> > > > can include comma, perhaps we could use semicolon as the separator.
> > > >
> > > > Jun
> > > >
> > > > On Wed, Jan 15, 2020 at 2:11 AM Viktor Somogyi-Vass <
> > > > viktorsomo...@gmail.com>
> > > > wrote:
> > > >
> > > > > Hey folks, bumping this again as KIP freeze is nearing and I hope
> to
> > > get
> > > > > this into the next release.
> > > > > We need only one binding vote.
> > > > >
> > > > > Thanks,
> > > > > Viktor
> > > > >
> > > > > On Thu, Jan 9, 2020 at 1:56 PM Viktor Somogyi-Vass <
> > > > > viktorsomo...@gmail.com>
> > > > > wrote:
> > > > >
> > > > > > Bumping this in the hope of a vote or additional feedback.
> > > > > >
> > > > > > Viktor
> > > > > >
> > > > > > On Tue, Dec 3, 2019 at 1:07 PM Viktor Somogyi-Vass <
> > > > > > viktorsomo...@gmail.com> wrote:
> > > > > >
> > > > > >> Hi Folks,
> > > > > >>
> > > > > >> I'd like to bump this once more in the hope of a binding vote or
> > any
> > > > > >> additional feedback.
> > > > > >>
> > > > > >> Thanks,
> > > > > >> Viktor
> > > > > >>
> > > > > >> On Fri, Oct 25, 2019 at 2:24 PM Viktor Somogyi-Vass <
> > > > > >> viktorsomo...@gmail.com> wrote:
> > > > > >>
> > > > > >>> Hi All,
> > > > > >>>
> > > > > >>> Would like to bump this in the hope of one binding vote (or any
> > > > > >>> additional feedback).
> > > > > >>>
> > > > > >>> Thanks,
> > > > > >>> Viktor
> > > > > >>>
> > > > > >>> On Wed, Sep 18, 2019 at 5:25 PM Viktor Somogyi-Vass <
> > > > > >>> viktorsomo...@gmail.com> wrote:
> > > > > >>>
> > > > >  Hi All,
> > > > > 
> > > > >  Harsha, Ryanne: thanks for the vote!
> > > > > 
> > > > >  I'd like to bump this again as today is the KIP freeze date
> and
> > > > there
> > > > >  is still one binding vote needed which I'm hoping to get in
> > order
> > > to
> > > > > have
> > > > >  this included in 2.4.
> > > > > 
> > > > >  Thanks,
> > > > >  Viktor
> > > > > 
> > > > >  On Tue, Sep 17, 2019 at 1:18 AM 

Re: [VOTE] KIP-515: Hardened TLS Configs to ZooKeeper

[Manikumar]

+1 (binding).

Thanks for the KIP.

On Mon, Jan 20, 2020 at 9:21 PM Rajini Sivaram 
wrote:

> +1 (binding)
>
> Thanks for the KIP, Ron!
>
> Regards,
>
> Rajini
>
>
> On Mon, Jan 20, 2020 at 3:36 PM Gwen Shapira  wrote:
>
> > +1 (binding), this has been an on-going concern. Thank you for
> > addressing this, Ron.
> >
> > On Mon, Jan 20, 2020 at 5:22 AM Ron Dagostino  wrote:
> > >
> > > Hi everyone.  I would like to start a vote on KIP-515: Enable ZK
> > > client to use the new TLS supported authentication.
> > >
> > > The KIP is here:
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-515%3A+Enable+ZK+client+to+use+the+new+TLS+supported+authentication
> > >
> > > The discussion thread is here:
> > >
> >
> https://lists.apache.org/thread.html/519d07e607cf6598a8126139c964d31fa46f2c028b88b1d44086b8a9%40%3Cdev.kafka.apache.org%3E
> > >
> > > Thanks,
> > >
> > > Ron
> >
>

[jira] [Created] (KAFKA-9456) EndToEndLatency: Add support for printing all latencies

[Jira]

Fábio Silva created KAFKA-9456:
--

 Summary: EndToEndLatency: Add support for printing all latencies
 Key: KAFKA-9456
 URL: https://issues.apache.org/jira/browse/KAFKA-9456
 Project: Kafka
  Issue Type: New Feature
  Components: tools
Reporter: Fábio Silva
Assignee: Fábio Silva


The EndToEndLatency tool already stores all the latencies but it only prints a 
brief report containing the mean latency and some percentiles. The main idea of 
this feature is to have a flag to enable printing all the latencies instead of 
a report.

Printing all the latencies is important if the goal of testing is to plot a 
chart (such a boxplot) for example, extract confidence intervals, wherever.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

Re: [VOTE] KIP-515: Hardened TLS Configs to ZooKeeper

[Rajini Sivaram]

+1 (binding)

Thanks for the KIP, Ron!

Regards,

Rajini


On Mon, Jan 20, 2020 at 3:36 PM Gwen Shapira  wrote:

> +1 (binding), this has been an on-going concern. Thank you for
> addressing this, Ron.
>
> On Mon, Jan 20, 2020 at 5:22 AM Ron Dagostino  wrote:
> >
> > Hi everyone.  I would like to start a vote on KIP-515: Enable ZK
> > client to use the new TLS supported authentication.
> >
> > The KIP is here:
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-515%3A+Enable+ZK+client+to+use+the+new+TLS+supported+authentication
> >
> > The discussion thread is here:
> >
> https://lists.apache.org/thread.html/519d07e607cf6598a8126139c964d31fa46f2c028b88b1d44086b8a9%40%3Cdev.kafka.apache.org%3E
> >
> > Thanks,
> >
> > Ron
>

Re: [VOTE] KIP-515: Hardened TLS Configs to ZooKeeper

[Gwen Shapira]

+1 (binding), this has been an on-going concern. Thank you for
addressing this, Ron.

On Mon, Jan 20, 2020 at 5:22 AM Ron Dagostino  wrote:
>
> Hi everyone.  I would like to start a vote on KIP-515: Enable ZK
> client to use the new TLS supported authentication.
>
> The KIP is here:
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-515%3A+Enable+ZK+client+to+use+the+new+TLS+supported+authentication
>
> The discussion thread is here:
> https://lists.apache.org/thread.html/519d07e607cf6598a8126139c964d31fa46f2c028b88b1d44086b8a9%40%3Cdev.kafka.apache.org%3E
>
> Thanks,
>
> Ron

Re: [VOTE] KIP-515: Hardened TLS Configs to ZooKeeper

[Mitchell]

+1 (non-binding).

Super +1 on this.
-mitch

On Mon, Jan 20, 2020 at 7:22 AM Ron Dagostino  wrote:
>
> Hi everyone.  I would like to start a vote on KIP-515: Enable ZK
> client to use the new TLS supported authentication.
>
> The KIP is here:
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-515%3A+Enable+ZK+client+to+use+the+new+TLS+supported+authentication
>
> The discussion thread is here:
> https://lists.apache.org/thread.html/519d07e607cf6598a8126139c964d31fa46f2c028b88b1d44086b8a9%40%3Cdev.kafka.apache.org%3E
>
> Thanks,
>
> Ron

[VOTE] KIP-515: Hardened TLS Configs to ZooKeeper

[Ron Dagostino]

Hi everyone.  I would like to start a vote on KIP-515: Enable ZK
client to use the new TLS supported authentication.

The KIP is here:
https://cwiki.apache.org/confluence/display/KAFKA/KIP-515%3A+Enable+ZK+client+to+use+the+new+TLS+supported+authentication

The discussion thread is here:
https://lists.apache.org/thread.html/519d07e607cf6598a8126139c964d31fa46f2c028b88b1d44086b8a9%40%3Cdev.kafka.apache.org%3E

Thanks,

Ron

Re: [DISCUSS] KIP-515: Enable ZK client to use the new TLS supported authentication

[Ron Dagostino]

Hi Rajini.  Thanks.  Yes, I’ll update the KIP, and then I’ll start the vote.

Ron

> On Jan 20, 2020, at 6:30 AM, Rajini Sivaram  wrote:
> 
> Hi Ron,
> 
> Good point. Keystore and truststore configs (type/location/password) are
> dynamically reconfigurable. A few of them like protocol, enabled.protocols,
> cipher.suites and endpoint.identification.algorithm are not. But to keep it
> simple, it may be better not to inherit any of the SSL configs. Perhaps
> just document that in Rejected Alternatives?
> 
>> On Sat, Jan 18, 2020 at 12:43 AM Ron Dagostino  wrote:
>> 
>> Thanks, Rajini.  Still not sure what the answer is, but I thought of a
>> couple more issues with config inheritance that I wanted to raise.
>> The first is a minor issue, but just to document it (and I will add it
>> to the KIP as well), ZooKeeper does not support a key password that
>> differs from the keystore password. So inheriting from Kafka's TLS
>> configs would only work if those configs use the same key and keystore
>> password.  Again, not a big deal -- just something to document.
>> 
>> However...
>> 
>> I think there may be a fundamental problem with inheriting broker
>> configs for ZooKeeper access.  The problem is that the broker's TLS
>> configs are dynamically reconfigurable, and the dynamic values
>> themselves are stored in ZooKeeper.  It would be impossible to know
>> those values without connecting to ZooKeeper, and it would be
>> impossible to connect to ZooKeeper without the values.  Given this, I
>> wonder if inheritance is impossible in this particular case.
>> 
>> Ron
>> 
>>> On Jan 17, 2020, at 5:39 PM, Rajini Sivaram 
>> wrote:
>>> 
>>> Hi Ron,
>>> 
>>> For Kafka, we moved from hostname verification disabled-by-default to
>>> enabled-by-default under
>>> 
>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-294+-+Enable+TLS+hostname+verification+by+default
>> .
>>> So we have tested empty String for disabling hostname verification and we
>>> know that it works, not only in test environments, but also in actual
>>> deployments. Basically, "ssl.endpoint.identification.algorithm=" in the
>>> file results in a config value of empty String and not setting it results
>>> in the default value of HTTPS since the value is null.I would probably do
>>> the same for the ZK config. If you are creating certificates for
>> ZooKeeper
>>> without hostnames, then you may be doing the same for Kafka too and in
>> both
>>> cases, the config would be an empty string. This is insecure anyway and
>> we
>>> would really expect this only in test environments. Not sure if it is
>> worth
>>> making an exception for this in terms of consistent config names.
>>> 
 On Fri, Jan 17, 2020 at 2:37 PM Ron Dagostino 
>> wrote:
 
 Hi again, Rajini.  I've updated the KIP, and while doing it I became
 concerned about using zookeeper.ssl.endpoint.identification.algorithm
 for enabling/disabling hostname verification.  The KIP reflects what
 we decided, but upon reading it, I wonder if it is workable.  Here's
 what it says for this config:
 
 "Specifies whether to enable hostname verification in the ZooKeeper
 TLS negotiation process, with (case-insensitively) "https" meaning
 ZooKeeper hostname verification is enabled and an explicit blank value
 meaning it is disabled (disabling it is only recommended for testing
 purposes).  Overrides any explicit "true" or "false" value set via the
 zookeeper.ssl.hostnameVerification system property (true
 implying https and false implying blank) and inherits the value of
 ssl.endpoint.identification.algorithm (if any) if no
 explicit value or non-value is set both here and via the system
 property."
 
 I wonder if someone sets an explicit empty value
 (zookeeper.ssl.endpoint.identification.algorithm=) in the config file,
 will we be able to distinguish between that and a config file that
 doesn't have it at all?  And even if we can, this feels pretty forced
 to me, like we are bending over backwards for consistency when
 "zookeeper.ssl.hostname.verification.enable" is a natural fit.  I
 doubt this config is going to be used very much, so it really does
 feel to me like we should just go with the natural fit and ditch the
 compatibility and inheritance for this one config.
 
 Ron
 
> On Fri, Jan 17, 2020 at 8:07 AM Ron Dagostino 
>> wrote:
> 
> Hi Rajini.  I’ll submit a documentation PR to clarify the Kafka
>> behavior
 of #1 and will adopt the same config for ZK.  I agree now we should
>> inherit
 for AclAuthorizer too — I was just stuck on the “no inheritance” idea
>> more
 than I realized, and while the ZK quorums are different that doesn’t
 necessarily mean the same client identity wouldn’t be valid on both
>> (and if
 not then the configs can still be overridden).
> 
> I’ll update the KIP today and let this version soak for a day or two,
 and 

Re: [DISCUSS] KIP-515: Enable ZK client to use the new TLS supported authentication

[Rajini Sivaram]

Hi Ron,

Good point. Keystore and truststore configs (type/location/password) are
dynamically reconfigurable. A few of them like protocol, enabled.protocols,
cipher.suites and endpoint.identification.algorithm are not. But to keep it
simple, it may be better not to inherit any of the SSL configs. Perhaps
just document that in Rejected Alternatives?

On Sat, Jan 18, 2020 at 12:43 AM Ron Dagostino  wrote:

> Thanks, Rajini.  Still not sure what the answer is, but I thought of a
> couple more issues with config inheritance that I wanted to raise.
> The first is a minor issue, but just to document it (and I will add it
> to the KIP as well), ZooKeeper does not support a key password that
> differs from the keystore password. So inheriting from Kafka's TLS
> configs would only work if those configs use the same key and keystore
> password.  Again, not a big deal -- just something to document.
>
> However...
>
> I think there may be a fundamental problem with inheriting broker
> configs for ZooKeeper access.  The problem is that the broker's TLS
> configs are dynamically reconfigurable, and the dynamic values
> themselves are stored in ZooKeeper.  It would be impossible to know
> those values without connecting to ZooKeeper, and it would be
> impossible to connect to ZooKeeper without the values.  Given this, I
> wonder if inheritance is impossible in this particular case.
>
> Ron
>
> > On Jan 17, 2020, at 5:39 PM, Rajini Sivaram 
> wrote:
> >
> > Hi Ron,
> >
> > For Kafka, we moved from hostname verification disabled-by-default to
> > enabled-by-default under
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-294+-+Enable+TLS+hostname+verification+by+default
> .
> > So we have tested empty String for disabling hostname verification and we
> > know that it works, not only in test environments, but also in actual
> > deployments. Basically, "ssl.endpoint.identification.algorithm=" in the
> > file results in a config value of empty String and not setting it results
> > in the default value of HTTPS since the value is null.I would probably do
> > the same for the ZK config. If you are creating certificates for
> ZooKeeper
> > without hostnames, then you may be doing the same for Kafka too and in
> both
> > cases, the config would be an empty string. This is insecure anyway and
> we
> > would really expect this only in test environments. Not sure if it is
> worth
> > making an exception for this in terms of consistent config names.
> >
> >> On Fri, Jan 17, 2020 at 2:37 PM Ron Dagostino 
> wrote:
> >>
> >> Hi again, Rajini.  I've updated the KIP, and while doing it I became
> >> concerned about using zookeeper.ssl.endpoint.identification.algorithm
> >> for enabling/disabling hostname verification.  The KIP reflects what
> >> we decided, but upon reading it, I wonder if it is workable.  Here's
> >> what it says for this config:
> >>
> >> "Specifies whether to enable hostname verification in the ZooKeeper
> >> TLS negotiation process, with (case-insensitively) "https" meaning
> >> ZooKeeper hostname verification is enabled and an explicit blank value
> >> meaning it is disabled (disabling it is only recommended for testing
> >> purposes).  Overrides any explicit "true" or "false" value set via the
> >> zookeeper.ssl.hostnameVerification system property (true
> >> implying https and false implying blank) and inherits the value of
> >> ssl.endpoint.identification.algorithm (if any) if no
> >> explicit value or non-value is set both here and via the system
> >> property."
> >>
> >> I wonder if someone sets an explicit empty value
> >> (zookeeper.ssl.endpoint.identification.algorithm=) in the config file,
> >> will we be able to distinguish between that and a config file that
> >> doesn't have it at all?  And even if we can, this feels pretty forced
> >> to me, like we are bending over backwards for consistency when
> >> "zookeeper.ssl.hostname.verification.enable" is a natural fit.  I
> >> doubt this config is going to be used very much, so it really does
> >> feel to me like we should just go with the natural fit and ditch the
> >> compatibility and inheritance for this one config.
> >>
> >> Ron
> >>
> >>> On Fri, Jan 17, 2020 at 8:07 AM Ron Dagostino 
> wrote:
> >>>
> >>> Hi Rajini.  I’ll submit a documentation PR to clarify the Kafka
> behavior
> >> of #1 and will adopt the same config for ZK.  I agree now we should
> inherit
> >> for AclAuthorizer too — I was just stuck on the “no inheritance” idea
> more
> >> than I realized, and while the ZK quorums are different that doesn’t
> >> necessarily mean the same client identity wouldn’t be valid on both
> (and if
> >> not then the configs can still be overridden).
> >>>
> >>> I’ll update the KIP today and let this version soak for a day or two,
> >> and then I’ll start a vote if no other issues/comments arise.
> >>>
> >>> Ron
> >>>
>  On Jan 17, 2020, at 6:08 AM, Rajini Sivaram 
> >> wrote:
> 
>  Hi Ron,
> 
>  Unresolved item #1: Yes, Kafka 

Re: [DISCUSS] : KIP-562: Allow fetching a key from a single partition rather than iterating over all the stores on an instance

[Navinder Brar]

Thanks John. If there are no other comments to be addressed, I will start a 
vote today so that we are on track for this release.~Navinder


On Monday, January 20, 2020, 8:32 AM, John Roesler  wrote:

Thanks, Navinder,

The Param object looks a bit different than I would have done, but it certainly 
is explicit. We might have to deprecate those particular factory methods and 
move to a builder pattern if we need to add any more options in the future, but 
I’m fine with that possibility. 

The KIP also discusses some implementation details that aren’t necessary here. 
We really only need to see the public interfaces. We can discuss the 
implementation in the PR.

That said, the public API part of the current proposal looks good to me! I 
would be a +1 if you called for a vote. 

Thanks,
John

On Sun, Jan 19, 2020, at 20:50, Navinder Brar wrote:
> I have made some edits in the KIP, please take another look. It would 
> be great if we can push it in 2.5.0.
> ~Navinder
> 
> 
> On Sunday, January 19, 2020, 12:59 AM, Navinder Brar 
>  wrote:
> 
> Sure John, I will update the StoreQueryParams with static factory 
> methods.
> @Ted, we would need to create taskId only in case a user provides one 
> single partition. In case user wants to query all partitions of an 
> instance the current code is good enough where we iterate over all 
> stream threads and go over all taskIds to match the store. But in case 
> a user requests for a single partition-based store, we need to create a 
> taskId out of that partition and store name(using 
> internalTopologyBuilder class) and match with the taskIds belonging to 
> that instance. I will add the code in the KIP. 
> 
>     On Sunday, 19 January, 2020, 12:47:08 am IST, Ted Yu 
>  wrote:  
>  
>  Looking at the current KIP-562:
> 
> bq. Create a taskId from the combination of store name and partition
> provided by the user
> 
> I wonder if a single taskId would be used for the “all partitions” case.
> If so, we need to choose a numerical value for the partition portion of the
> taskId.
> 
> On Sat, Jan 18, 2020 at 10:27 AM John Roesler  wrote:
> 
> > Thanks, Ted!
> >
> > This makes sense, but it seems like we should lean towards explicit
> > semantics in the public API. ‘-1’ meaning “all partitions” is reasonable,
> > but not explicit. That’s why I suggested the Boolean for “all partitions”.
> > I guess this also means getPartition() should either throw an exception or
> > return null if the partition is unspecified.
> >
> > Thanks,
> > John
> >
> > On Sat, Jan 18, 2020, at 08:43, Ted Yu wrote:
> > > I wonder if the following two methods can be combined:
> > >
> > > Integer getPartition() // would be null if unset or if "all partitions"
> > > boolean getAllLocalPartitions() // true/false if "all partitions"
> > requested
> > >
> > > into:
> > >
> > > Integer getPartition() // would be null if unset or -1 if "all
> > partitions"
> > >
> > > Cheers
> > >
> > > On Fri, Jan 17, 2020 at 9:56 PM John Roesler 
> > wrote:
> > >
> > > > Thanks, Navinder!
> > > >
> > > > I took a look at the KIP.
> > > >
> > > > We tend to use static factory methods instead of public constructors,
> > and
> > > > also builders for optional parameters.
> > > >
> > > > Given that, I think it would be more typical to have a factory method:
> > > > storeQueryParams()
> > > >
> > > > and also builders for setting the optional parameters, like:
> > > > withPartitions(List partitions)
> > > > withStaleStoresEnabled()
> > > > withStaleStoresDisabled()
> > > >
> > > >
> > > > I was also thinking this over today, and it really seems like there are
> > > > two main cases for specifying partitions,
> > > > 1. you know exactly what partition you want. In this case, you'll only
> > > > pass in a single number.
> > > > 2. you want to get a handle on all the stores for this instance (the
> > > > current behavior). In this case, it's not clear how to use
> > withPartitions
> > > > to achieve the goal, unless you want to apply a-priori knowledge of the
> > > > number of partitions in the store. We could consider an empty list, or
> > a
> > > > null, to indicate "all", but that seems a little complicated.
> > > >
> > > > Thus, maybe it would actually be better to eschew withPartitions for
> > now
> > > > and instead just offer:
> > > > withPartition(int partition)
> > > > withAllLocalPartitions()
> > > >
> > > > and the getters:
> > > > Integer getPartition() // would be null if unset or if "all partitions"
> > > > boolean getAllLocalPartitions() // true/false if "all partitions"
> > requested
> > > >
> > > > Sorry, I know I'm stirring the pot, but what do you think about this?
> > > >
> > > > Oh, also, the KIP is missing the method signature for the new
> > > > KafkaStreams#store overload.
> > > >
> > > > Thanks!
> > > > -John
> > > >
> > > > On Fri, Jan 17, 2020, at 08:07, Navinder Brar wrote:
> > > > > Hi all,
> > > > > I have created a new
> > > > > KIP:
> > > >
> > 

Re: [DISCUSS] KIP-552: Add new cached authorizer:change the dim of cache

[Rajini Sivaram]

Hi Steven,

Because of the change in KIP numbers, the KIP numbers above are misleading.
The subject says of the discussion thread says KIP-552 and the link says
KIP-553. But it is actually none of those since the KIP is now KIP-565.

A couple of questions regarding the KIP:
1) Do we bound the number of entries in the cache? At the moment, it looks
like we add every resource to the cache and only remove entries when ACLs
are updated. I think we also need to limit the number of entries we cache.
Otherwise, in a deployment with changing resource access (short-lived
topics, some hosts that access topics for a short time etc.) the cache
would keep growing unless there are ACL changes.

2) Will caching be optional for AclAuthorizer?

3) Why do we have a separate CachedAuthorizer if AclAuthorizer is going to
extend that?

On Sat, Jan 18, 2020 at 11:02 PM Steven Lu  wrote:

> Hello all,
>
> I wrote a KIP about adding the new cached authorizer,this improvement can
> reduce greatly the CPU usage in the long run.
> Please take a look:
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-553%3A+Using+AclCommand%2Cavoid+call+the+global+method+loadcache+in+SimpleAclAuthorizer
>
> Thanks,
> Steven
>