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 > > > > > > > > > >
