Thanks, Jun. <<<100 I added this line to the KIP to clarify the SSL issue: "This KIP has no impact on non-SASL connections (e.g. connections that use the PLAINTEXT or SSL security protocols) – no such connection will be re-authenticated, and no such connection will be closed."
<<<101 Your comment points out two issues, I think. First, there is a direct clarity problem: in the KIP, by the phrase "requests that queued up during re-authentication" I was really intending to refer to both the in-flight responses that might return from the broker during re-authentication along with any pending Send request that is set on the KafkaChannel instance and has not yet been transmitted to the broker (this is the Send that triggers the re-authentication check to occur, and when the session is "expired" the re-authentication process then begins). So I clarified the text in the KIP tp refer directly to both of these things. But before I insert that amended text, note that the second issue (the implementation option of marking the channel unavailable for send during re-authentication) also points out a clarity problem in the KIP because the channel is in fact unavailable for send during re-authentication. The reason is because KafkaChannel#ready() will return false until the Authenticator finishes the re-authentication, and this causes KafkaClient#isReady(Node, long) and KafkaClient#ready(Node, long) to both return false. So in fact the client will not be able to queue up send after send. I've therefore updated the KIP text as follows: "If re-authentication succeeds then any received responses that queued up during re-authentication along with the Send that triggered the re-authentication to occur in the first place will subsequently be able to flow through (back to the client and along to the broker, respectively), and eventually the connection will re-authenticate again, etc. Note also that the client cannot queue up additional send requests beyond the one that triggers re-authentication to occur until re-authentication succeeds and the triggering one is sent." <<<102 Good catch about the client-side metric not having a name or a clear definition of what it measures. That is an oversight. I will resolve this via a post on the DISCUSS thread: https://lists.apache.org/thread.html/a63c1612fe9ba2f31272087a00419c59ed7a9917c398721069cd1d01@%3Cdev.kafka.apache.org%3E <<<103 We re-use the existing authentication code paths for re-authentication, and it appears ( https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/common/security/authenticator/SaslClientAuthenticator.java#L182) that the version used by the broker when it is acting as an inter-broker SASL client is the max version supported by the destination broker. Am I missing something? Thanks again, Jun. Ron On Mon, Sep 24, 2018 at 7:46 PM Jun Rao <j...@confluent.io> wrote: > Hi, Ron, > > Thanks for the KIP. Looks good to me overall. So, +1 assuming the following > minor comments will be addressed. > > 100. connections.max.reauth.ms: A client can authenticate with the broker > using SSL. This config has no impact on such connections. It would be > useful to make it clear in the documentation. Also, in this case, I guess > the broker won't kill the SSL connection after connections.max.reauth.ms? > > 101. "If re-authentication succeeds then any requests that queued up during > re-authentication will subsequently be able to flow through, and eventually > the connection will re-authenticate again, etc.". This is more of an > implementation detail. I guess the proposal is to queue up new requests in > the client when there is is pending re-authentication. An alternative is to > mark the Channel unavailable for send during re-authentication. This has > the slight benefit of reducing the client memory footprint. > > 102. "A client-side metric will be created that documents the latency > imposed by re-authentication." What's the name of this metric? Does it > measure avg or max? > > 103. "Upgrade all brokers to v2.1.0 or later at whatever rate is desired > with 'connections.max.reauth.ms' allowed to default to 0. If SASL is used > for the inter-broker protocol then brokers will check the SASL_AUTHENTICATE > API version and use a V1 request when communicating to a broker that has > been upgraded to 2.1.0, but the client will see the "0" session max > lifetime and will not re-authenticate. ". Currently, for the inter broker > usage of NetworkClient (ReplicaFetcherThread, ControllerChannelManager, > etc), the broker version discovery logic is actually disabled and the > client is expected to use the new version of the request after > inter.broker.protocol.version is set to the current version. So, we will > need to rely on this for deciding whether the NetworkClient should use the > re-authenticate request or not, during upgrade. > > Jun > > On Mon, Sep 24, 2018 at 4:39 PM, Ron Dagostino <rndg...@gmail.com> wrote: > > > Still looking for a final +1 binding vote to go with the 9 votes so far > (2 > > binding, 7 non-binding). > > > > Ron > > > > > On Sep 24, 2018, at 3:53 PM, Ron Dagostino <rndg...@gmail.com> wrote: > > > > > > **Please vote** . It's getting late in the day and this KIP still > > requires 1 more binding up-vote to be considered for the 2.1.0 release. > > > > > > The current vote is 2 binding +1 votes (Rajini and Harsha) and 7 > > non-binding +1 votes (myself, Mike, Konstantin, Boerge, Edoardo, > Stanislav, > > and Mickael). > > > > > > Ron > > > > > >> On Mon, Sep 24, 2018 at 9:47 AM Ron Dagostino <rndg...@gmail.com> > > wrote: > > >> Hi Everyone. This KIP still requires 1 more binding up-vote to be > > considered for the 2.1.0 release. **Please vote before today's > end-of-day > > deadline.** > > >> > > >> The current vote is 2 binding +1 votes (Rajini and Harsha) and 7 > > non-binding +1 votes (myself, Mike, Konstantin, Boerge, Edoardo, > Stanislav, > > and Mickael). > > >> > > >> Ron > > >> > > >>> On Fri, Sep 21, 2018 at 11:48 AM Mickael Maison < > > mickael.mai...@gmail.com> wrote: > > >>> +1 (non-binding) > > >>> Thanks for the KIP, this is a very nice feature. > > >>> On Fri, Sep 21, 2018 at 4:56 PM Stanislav Kozlovski > > >>> <stanis...@confluent.io> wrote: > > >>> > > > >>> > Thanks for the KIP, Ron! > > >>> > +1 (non-binding) > > >>> > > > >>> > On Fri, Sep 21, 2018 at 5:26 PM Ron Dagostino <rndg...@gmail.com> > > wrote: > > >>> > > > >>> > > Hi Everyone. This KIP requires 1 more binding up-vote to be > > considered for > > >>> > > the 2.1.0 release; please vote before the Monday deadline. > > >>> > > > > >>> > > The current vote is 2 binding +1 votes (Rajini and Harsha) and 5 > > >>> > > non-binding +1 votes (myself, Mike, Konstantin, Boerge, and > > Edoardo). > > >>> > > > > >>> > > Ron > > >>> > > > > >>> > > On Wed, Sep 19, 2018 at 12:40 PM Harsha <ka...@harsha.io> wrote: > > >>> > > > > >>> > > > KIP looks good. +1 (binding) > > >>> > > > > > >>> > > > Thanks, > > >>> > > > Harsha > > >>> > > > > > >>> > > > On Wed, Sep 19, 2018, at 7:44 AM, Rajini Sivaram wrote: > > >>> > > > > Hi Ron, > > >>> > > > > > > >>> > > > > Thanks for the KIP! > > >>> > > > > > > >>> > > > > +1 (binding) > > >>> > > > > > > >>> > > > > On Tue, Sep 18, 2018 at 6:24 PM, Konstantin Chukhlomin < > > >>> > > > chuhlo...@gmail.com> > > >>> > > > > wrote: > > >>> > > > > > > >>> > > > > > +1 (non binding) > > >>> > > > > > > > >>> > > > > > > On Sep 18, 2018, at 1:18 PM, > michael.kamin...@nytimes.com > > wrote: > > >>> > > > > > > > > >>> > > > > > > > > >>> > > > > > > > > >>> > > > > > > On 2018/09/18 14:59:09, Ron Dagostino <rndg...@gmail.com > > > > wrote: > > >>> > > > > > >> Hi everyone. I would like to start the vote for > KIP-368: > > >>> > > > > > >> https://cwiki.apache.org/confluence/display/KAFKA/KIP- > > >>> > > > > > 368%3A+Allow+SASL+Connections+to+Periodically+Re- > > Authenticate > > >>> > > > > > >> > > >>> > > > > > >> This KIP proposes adding the ability for SASL clients > > (and brokers > > >>> > > > when > > >>> > > > > > a > > >>> > > > > > >> SASL mechanism is the inter-broker protocol) to > > re-authenticate > > >>> > > > their > > >>> > > > > > >> connections to brokers and for brokers to close > > connections that > > >>> > > > > > continue > > >>> > > > > > >> to use expired sessions. > > >>> > > > > > >> > > >>> > > > > > >> Ron > > >>> > > > > > >> > > >>> > > > > > > > > >>> > > > > > > +1 (non binding) > > >>> > > > > > > > >>> > > > > > > > >>> > > > > > >>> > > > > >>> > > > >>> > > > >>> > -- > > >>> > Best, > > >>> > Stanislav > > >
