Hi, Ron, Thanks for the explanation. That makes sense. So, this is not an issue.
Jun On Tue, Sep 25, 2018 at 11:53 AM, Ron Dagostino <rndg...@gmail.com> wrote: > Hi Jun. I see now what you were getting at. This is actually not a > problem because the authentication code path does not leverage the > NetworkClient instance -- it reads to/writes from the TransportLayer > directly and sets its own API version numbers manually. Re-authentication > leverages the same code path. For example, see > https://github.com/apache/kafka/blob/trunk/clients/src/ > main/java/org/apache/kafka/common/security/authenticator/ > SaslClientAuthenticator.java#L191 > -- it is setting the version manually, and then it writes the request to > the TransportLayer. The versions that it decides to use come from an > ApiVersionsRequest/Response that it sends to the broker itself upon initial > authentication. The re-authentication leverages that same response from > the original authentication to know what versions to send. > > The issue you point out would certainly have been an issue if we had gone > with the higher-level alternatives that we rejected -- I'm increasingly > glad that we went with Rajini's solution instead. > > Ron > > > > > Ron > > On Tue, Sep 25, 2018 at 1:27 PM Jun Rao <j...@confluent.io> wrote: > > > Hi, Ron, > > > > 103. I was referring to the following code in ReplicaFetcherBlockingSend. > > As you can see, when instantiating NetworkClient, we disable > > the discoverBrokerVersions flag. So the network client won't know the > > version that the destination broker supports. Instead, the source broker > is > > responsible for sending requests with the correction version that's > > understandable by the destination broker. Currently, this is done by > > checking inter.broker.protocol. The caller will only send the new > requests > > if inter.broker.protocol is >= the expected version. We expect when > > inter.broker.protocol > > is updated, every broker has been updated to the new version of the code. > > > > new NetworkClient( > > selector, > > new ManualMetadataUpdater(), > > clientId, > > 1, > > 0, > > 0, > > Selectable.USE_DEFAULT_BUFFER_SIZE, > > brokerConfig.replicaSocketReceiveBufferBytes, > > brokerConfig.requestTimeoutMs, > > time, > > false, > > new ApiVersions, > > logContext > > ) > > > > Thanks, > > > > Jun > > > > On Mon, Sep 24, 2018 at 6: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/a63c1612fe9ba2f31272087a00419c > > > 59ed7a9917c398721069cd1d01@%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 > > > > > > > > > > > > > > >
