Hi everyone. This concludes the vote for KIP 368. The vote passes with three binding + 1 votes, from Rajini, Harsha, and Jun, and seven non-binding +1 votes, from Mike, Konstantin, Boerge, Edoardo, Stanislav, Mickael, and myself.
I have marked the KIP as "Accepted". The pull request, available at *https://github.com/apache/kafka/pull/5582 <https://github.com/apache/kafka/pull/5582>*, is far along and should be finished in the next day or two. Ron On Mon, Sep 24, 2018 at 9:25 PM Ron Dagostino <rndg...@gmail.com> wrote: > 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 >> > >> >
