Hi Rajini, Yes, I agree that it's not ideal. However, doing this once at the broker is more manageable than pushing the additional complexity to the clients. Between the two options you outlined, the second one seemed the least bad (we do something similar for controlled shutdown because it was missing a header before 0.9.0.0).
Ismael On Tue, Apr 12, 2016 at 9:56 AM, Rajini Sivaram < rajinisiva...@googlemail.com> wrote: > Ismael, > > My only concern about wrapping SASL tokens in Kafka headers is backward > compatibility. We would either have a different format for GSSAPI alone to > match 0.9.0.x or we would need to support two different wire protocols for > GSSAPI. Neither sounds ideal. > > On Tue, Apr 12, 2016 at 9:18 AM, Ismael Juma <ism...@juma.me.uk> wrote: > > > Hi Jun, > > > > I understand the point about the SASL tokens being similar to the SSL > > handshake in a way. However, is there any SASL library that handles the > > network communication for these tokens? I couldn't find any and without > > that, there isn't much benefit in deviating from Kafka's protocol (we > > basically save the space taken by the request header). It's worth > > mentioning that we are already adding the message size before the opaque > > bytes provided by the library, so one could say we are already extending > > the protocol. > > > > If we leave versioning aside, adding the standard Kafka request header to > > those messages may also help from a debugging perspective as would then > > include client id and correlation id along with the message. > > > > Ismael > > > > On Tue, Apr 12, 2016 at 2:13 AM, Jun Rao <j...@confluent.io> wrote: > > > > > Magnus, > > > > > > That sounds reasonable. To reduce the changes on the server side, I'd > > > suggest the following minor tweaks on the proposal. > > > > > > 1. Continue supporting the separate SASL and SASL_SSL port. > > > > > > On SASL port, we support the new sequence > > > ApiVersionRequest (optional), SaslHandshakeRequest, SASL tokens, > > > regular > > > requests > > > > > > On SASL_SSL port, we support the new sequence > > > SSL handshake bytes, ApiVersionRequest (optional), > > > SaslHandshakeRequest, > > > SASL tokens, regular requests > > > > > > 2. We don't wrap SASL tokens in Kafka protocol. Similar to your > argument > > > about SSL handshake, those SASL tokens are generated by SASL library > and > > > Kafka doesn't really control its versioning. Kafka only controls the > > > selection of SASL mechanism, which will be versioned in > > > SaslHandshakeRequest. > > > > > > Does that work for you? > > > > > > Thanks, > > > > > > Jun > > > > > > > > > On Mon, Apr 11, 2016 at 11:15 AM, Magnus Edenhill <mag...@edenhill.se> > > > wrote: > > > > > > > Hey Jun, see inline > > > > > > > > 2016-04-11 19:19 GMT+02:00 Jun Rao <j...@confluent.io>: > > > > > > > > > Hi, Magnus, > > > > > > > > > > Let me understand your proposal in more details just from the > > client's > > > > > perspective. My understanding of your proposal is the following. > > > > > > > > > > On plaintext port, the client will send the following bytes in > order. > > > > > ApiVersionRequest, SaslHandshakeRequest, SASL tokens (if SASL > is > > > > > enabled), regular requests > > > > > > > > > > On SSL port, the client will send the following bytes in order. > > > > > SSL handshake bytes, ApiVersionRequest, SaslHandshakeRequest, > > SASL > > > > > tokens (if SASL is enabled), regular requests > > > > > > > > > > > > > > > > > Yup! > > > > "SASL tokens" is a series of proper Kafka protocol > > SaslHandshakeRequests > > > > until > > > > the handshake is done. > > > > > > > > > > > > > > > > > > > > > > Is that right? Since we can use either SSL or SASL for > > authentication, > > > > it's > > > > > weird that in one case, we require ApiVersionRequest to happen > before > > > > > authentication and in another case we require the reverse. > > > > > > > > > > > > > Since the SSL/TLS is standardised and taken care of for us by the SSL > > > > libraries it > > > > doesnt make sense to reimplement that on top of Kafka, so it isn't > > really > > > > comparable. > > > > But for SASL there is no standardised handshake protocol so we must > > > either > > > > conceive one from scratch, or use the protocol that we already have > > > > (Kafka). > > > > For the initial SASL implementation in 0.9 the first option was > chosen > > > and > > > > while > > > > it required a new protocol implementation in supporting clients and > the > > > > broker > > > > it served its purpose. But not for long, it already needs to evolve, > > > > and this gives us a golden[1] opportunity to make the implementation > > > > reusable, evolvable, less complex > > > > and in line with all our other protocol work, by using the protocol > > > stack > > > > of Kafka which the > > > > broker and all clients already have in place. > > > > > > > > Not taking this chance and instead diverging the custom SASL > handshake > > > > protocol > > > > even further from Kafka seems to me a weird choice. > > > > > > > > The current KIP-43 proposal does not have a clear compatibility > story; > > it > > > > doesnt seem to be possible > > > > to upgrade clients before brokers, while this might be okay for the > > Java > > > > client, the KIP-35 discussion > > > > has hopefully proven that this assumption can't be made for the > entire > > > > eco-system. > > > > > > > > Let me be clear that there isn't anything technically wrong with the > > > KIP-43 > > > > proposal (well, > > > > except for the hack to check byte[0] for 0x60 perhaps), but I'm > worried > > > the > > > > proposal will eventually lead to > > > > reimplementing Api Versioning, KIP-35, etc, in the custom SASL > > handshake, > > > > and this is just redundant, > > > > there is no technical reason for doing so and it'll just make > protocol > > > > semantics and implementations more complex. > > > > > > > > > > > > Regards, > > > > Magnus > > > > > > > > [1]: Timing is good for this change since only two clients, Java and > C, > > > > currently supports > > > > the existing SASL handshake so far. > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > Jun > > > > > > > > > > On Mon, Apr 11, 2016 at 12:20 AM, Magnus Edenhill < > > mag...@edenhill.se> > > > > > wrote: > > > > > > > > > > > 2016-04-11 3:01 GMT+02:00 Jun Rao <j...@confluent.io>: > > > > > > > > > > > > > Thinking about ApiVersionRequest a bit more. There are quite a > > few > > > > > things > > > > > > > special about it. In the ideal case, (1) its version should > never > > > > > change; > > > > > > > > > > > > > > > > > > > The only thing we know of the future is that we dont know > anything, > > > we > > > > > > can't > > > > > > think of every possible future use case, that's why need to be > able > > > to > > > > > > evolve interfaces > > > > > > as requirements and use-cases change. This is the gist of KIP-35, > > and > > > > > > hampering > > > > > > KIP-35 itself, by not letting it also evolve, does not seem right > > to > > > me > > > > > at > > > > > > all. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > (2) it needs to be done before authentication (either > SSL/SASL); > > > (3) > > > > it > > > > > > is > > > > > > > required to be issued at the beginning of each connection but > > never > > > > > needs > > > > > > > to be issued again on the same connection. So, instead of > > modeling > > > it > > > > > as > > > > > > a > > > > > > > regular request, it seems a cleaner approach is to just bake > that > > > > into > > > > > > the > > > > > > > initial connection handshake even before the authentication > > layer. > > > So > > > > > the > > > > > > > sequencing in a connection will be api discovery, > authentication, > > > > > > followed > > > > > > > by regular requests. I am not sure we can still add this in a > > > > backward > > > > > > > compatible way now (e.g., not sure what the initial bytes from > an > > > SSL > > > > > > > connection will look like). Even if we can do this in a > backward > > > > > > compatible > > > > > > > way, it's probably non-trivial amount of work though. > > > > > > > > > > > > > > > > > > > I have the luxory of not knowing the broker internals, so I can > > only > > > > > > discuss > > > > > > this on a conceptual design level. > > > > > > > > > > > > In its simplest form each API request type has a NeedsAuth flag > and > > > the > > > > > > broker protocol request layer simply checks if the current > session > > is > > > > > > Authenticated > > > > > > before processing the request: If not the session is closed and > an > > > > error > > > > > is > > > > > > logged. > > > > > > The only two API requests that dont have the NeedsAuth flag would > > be > > > > > > SaslHandshakeRequest > > > > > > and ApiVersionRequest, the latter could also use filtering to > only > > > > return > > > > > > the same two > > > > > > requests in ApiVersionResponse before the client is authenticated > > (as > > > > not > > > > > > to "leak" information). > > > > > > If authentication is not configured on the broker all sessions > are > > > > deemed > > > > > > authenticated by default. > > > > > > > > > > > > > > > > > > Re backwards compatibility: > > > > > > My suggestion is to keep the current special SASL handshake > > protocol > > > on > > > > > the > > > > > > SASL_PLAIN/SASL_SSL > > > > > > endpoints, but use the new in-band Kafka SaslHandshakeRequest API > > on > > > > the > > > > > > PLAIN/SSL endpoints. > > > > > > This way the broker is backwards compatible with older clients > that > > > > only > > > > > > supports the special SASL protocol, > > > > > > and newer cliets are also backwards compatible with older brokers > > > that > > > > > only > > > > > > supports the special SASL protocol. > > > > > > Newer clients connecting to new brokers will be configured to use > > > > > non-SASL > > > > > > ports and use the > > > > > > in-band Kafka SaslHandshakeRequest to authenticate. > > > > > > > > > > > > Using the existing standard Kafka protocol and the new > future-proof > > > > > > functionality of ApiVersionRequest > > > > > > allows the in-band authentication mechanisms and semantics to > > > naturally > > > > > > evolve over time > > > > > > without breaking existing clients. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > We started KIP-35 with supporting a client to know if a version > > is > > > > > > > supported by the broker. It's now evolved into supporting a > > client > > > to > > > > > > > implement multiple versions of the protocol and dynamically > pick > > a > > > > > > version > > > > > > > supported by the broker. The former is likely solvable without > > > > > > > ApiVersionRequest. How important is the latter? What if the C > > > client > > > > > just > > > > > > > follows the java client model by implementing one version of > > > protocol > > > > > > per C > > > > > > > client release (which seems easier to implement)? > > > > > > > > > > > > > > > > > > > We've discussed this at length and it is not an option for > > > librdkafka, > > > > > nor > > > > > > kafka-python, and > > > > > > probably other clients as well, due to usability/UX and > maintenance > > > > > > reasons. > > > > > > (There's even discussion of making the Java client more version > > > > > agnostic!) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > Jun > > > > > > > > > > > > > > > > > > > > > On Fri, Apr 8, 2016 at 4:20 PM, Jun Rao <j...@confluent.io> > > wrote: > > > > > > > > > > > > > > > Magnus, > > > > > > > > > > > > > > > > A while back, we had another proposal for the broker to just > > send > > > > the > > > > > > > > correlation id and an empty payload if it receives an > > unsupported > > > > > > version > > > > > > > > of the request. I didn't see that in the rejected section. It > > > seems > > > > > > > simpler > > > > > > > > than the current proposal where the client has to issue an > > > > > > > > ApiVersionRequest on every connection. Could you document the > > > > reason > > > > > > why > > > > > > > we > > > > > > > > rejected it? > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > > > Jun > > > > > > > > > > > > > > > > On Tue, Apr 5, 2016 at 1:47 PM, Ashish Singh < > > > asi...@cloudera.com> > > > > > > > wrote: > > > > > > > > > > > > > > > >> On Fri, Apr 1, 2016 at 1:32 AM, Ismael Juma < > > ism...@juma.me.uk> > > > > > > wrote: > > > > > > > >> > > > > > > > >> > Two more things: > > > > > > > >> > > > > > > > > >> > 3. We talk about backporting of new request versions to > > stable > > > > > > > branches > > > > > > > >> in > > > > > > > >> > the KIP. In practice, we can't do that until the Java > client > > > is > > > > > > > changed > > > > > > > >> so > > > > > > > >> > that it doesn't blindly use the latest protocol version. > > > > > Otherwise, > > > > > > if > > > > > > > >> new > > > > > > > >> > request versions were added to 0.9.0.2, the client would > > break > > > > > when > > > > > > > >> talking > > > > > > > >> > to a 0.9.0.1 broker (given Jason's proposal, it would > fail a > > > bit > > > > > > more > > > > > > > >> > gracefully, but that's still not good enough for a stable > > > > branch). > > > > > > It > > > > > > > >> may > > > > > > > >> > be worth making this clear in the KIP (yes, it is a bit > > > > orthogonal > > > > > > and > > > > > > > >> > doesn't prevent the KIP from being adopted, but good to > > avoid > > > > > > > >> confusion). > > > > > > > >> > > > > > > > > >> Good point. Adding this note and also adding a note that > Kafka > > > has > > > > > not > > > > > > > >> backported an api version so far. > > > > > > > >> > > > > > > > >> > > > > > > > > >> > 4. The paragraph below is a bit confusing. It starts > talking > > > > about > > > > > > > 0.9.0 > > > > > > > >> > and trunk and then switches to 0.9.1. Is that intentional? > > > > > > > >> > > > > > > > > >> Yes. > > > > > > > >> > > > > > > > >> > > > > > > > > >> > "Deprecation of a protocol version will be done by > marking a > > > > > > protocol > > > > > > > >> > version as deprecated in protocol documentation. > > Documentation > > > > > shall > > > > > > > >> also > > > > > > > >> > be used to indicate a protocol version that must not be > > used, > > > or > > > > > for > > > > > > > any > > > > > > > >> > such information.For instance, say 0.9.0 had protocol > > versions > > > > [0] > > > > > > for > > > > > > > >> api > > > > > > > >> > key 1. On trunk, version 1 of the api key was added. Users > > > > running > > > > > > off > > > > > > > >> > trunk started using version 1 of the api and found out a > > major > > > > > bug. > > > > > > To > > > > > > > >> > rectify that version 2 of the api is added to trunk. For > > some > > > > > > reason, > > > > > > > >> it is > > > > > > > >> > now deemed important to have version 2 of the api in 0.9.1 > > as > > > > > well. > > > > > > To > > > > > > > >> do > > > > > > > >> > so, version 1 and version 2 both of the api will be > > backported > > > > to > > > > > > the > > > > > > > >> 0.9.1 > > > > > > > >> > branch. 0.9.1 broker will return 0 as min supported > version > > > for > > > > > the > > > > > > > api > > > > > > > >> and > > > > > > > >> > 2 for the max supported version for the api. However, the > > > > version > > > > > 1 > > > > > > > >> should > > > > > > > >> > be clearly marked as deprecated on its documentation. It > > will > > > be > > > > > > > >> client's > > > > > > > >> > responsibility to make sure they are not using any such > > > > deprecated > > > > > > > >> version > > > > > > > >> > to the best knowledge of the client at the time of > > development > > > > (or > > > > > > > >> > alternatively by configuration)." > > > > > > > >> > > > > > > > > >> > Ismael > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > On Fri, Apr 1, 2016 at 9:22 AM, Ismael Juma < > > > ism...@juma.me.uk> > > > > > > > wrote: > > > > > > > >> > > > > > > > > >> > > A couple of questions: > > > > > > > >> > > > > > > > > > >> > > 1. The KIP says "Specific version may be deprecated > > through > > > > > > protocol > > > > > > > >> > > documentation but must still be supported (although it > is > > > fair > > > > > to > > > > > > > >> return > > > > > > > >> > an > > > > > > > >> > > error code if the specific API supports it).". It may be > > > worth > > > > > > > >> expanding > > > > > > > >> > > this a little more. For example, what does it mean to > > > support > > > > > the > > > > > > > >> API? I > > > > > > > >> > > guess this means that the broker must not disconnect the > > > > client > > > > > > and > > > > > > > >> the > > > > > > > >> > > broker must return a valid protocol response. Given that > > it > > > > says > > > > > > > that > > > > > > > >> it > > > > > > > >> > is > > > > > > > >> > > "fair" (I would probably replace "fair" with "valid") to > > > > return > > > > > an > > > > > > > >> error > > > > > > > >> > > code if the specific API supports it, it sounds like we > > are > > > > > saying > > > > > > > >> that > > > > > > > >> > we > > > > > > > >> > > don't have to maintain the semantic behaviour (i.e. we > > could > > > > > > > _always_ > > > > > > > >> > > return an error for a deprecated API?). Is this true? > > > > > > > >> > > > > > > > > > >> > > 2. ApiVersionQueryRequest seems a bit verbose, why not > > > > > > > >> ApiVersionRequest? > > > > > > > >> > > > > > > > > > >> > > Ismael > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > > >> > > > > > > > >> -- > > > > > > > >> > > > > > > > >> Regards, > > > > > > > >> Ashish > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > Regards, > > Rajini >
