Hi Boyang, Thanks for the KIP. Overall, it looks good to me. I really like the envelope RPC!
One minor comment regarding the `old-client-connections-count` metric. Is it really necessary? The number of connected clients whose version is not known (prior to KIP-511) is already reported but with an "unknown" software name and an "unknown" software version, which is, I suppose, similar to what you intend to expose with this new metric, isn't it? Regards, David On Thu, Apr 16, 2020 at 7:24 AM Boyang Chen <reluctanthero...@gmail.com> wrote: > Thanks Raymond and Colin for the detailed discussions! I totally agree > with the rational here. The new `Envelope` RPC has been added to the KIP > and the forwarding section logic has been revised, feel free to take > another look. > > On Wed, Apr 15, 2020 at 5:19 PM Colin McCabe <cmcc...@apache.org> wrote: > > > Hi Boyang, > > > > I agree that we need a version bump on the request types we are going to > > forward. The new versions will be able to return the NOT_CONTROLLER > error, > > and let the client do the retrying, which is what we typically prefer. > > The existing versions can't ever return NOT_CONTROLLER. > > > > Since we have to have a new version for all these requests, we could > > technically do everything with just optional fields, like we originally > > discussed. However, there is probably some value in having a real > > EnvelopeRequest (or whatever) that makes it clearer what is going on. > > Optional fields don't come with "guard rails" to prevent us from > > accidentally ignoring them on older versions of the broker. A new ApiKey > > certainly does. > > > > Another issue is that it's nice to avoid changing the version of the > > request when forwarding it. Sometimes different versions have slightly > > different semantics, and it simplifies things to avoid worrying about > that. > > > > We should restrict the use of forwarding to just principals that have > > CLUSTERACTION on CLUSTER for now, so that only the brokers and superusers > > can do it. > > > > best, > > Colin > > > > > > On Tue, Apr 14, 2020, at 13:15, Boyang Chen wrote: > > > Thanks Raymond for the proposal! Yea, adding a unified forwarding > > envelope > > > request is a good idea, but it doesn't really solve our problem in this > > KIP. > > > > > > On Mon, Apr 13, 2020 at 11:14 PM Raymond Ng <r...@confluent.io> wrote: > > > > > > > Hi Boyang, > > > > > > > > Thanks for the KIP. Overall looks great. > > > > > > > > One suggestion: instead of bumping the version and adding an optional > > field > > > > (PrincipalName) for a number of requests, can we consider adding a > > general > > > > ProxyRequest that acts as an "envelope" for the forwarded requests? > > > > > > > > A few advantages to this approach come to mind: > > > > > > > > 1. Add one (new) Request API instead of modifying a number of them > > > > 2. Make the forwarded nature of the request explicit instead of > > > > implicitly relying on an optional field and a specific version > that > > > > varies > > > > by type. > > > > 3. This approach is flexible enough to be potentially useful > beyond > > the > > > > current use case (e.g. federated, inter-cluster scenarios) > > > > > > > > As a bonus, the combination of 1. and 2. should also simplify > > > > implementation & validation. > > > > > > > > > > > Firstly the broker has to differentiate old and new admin clients as it > > > should only support forwarding for old ones. Without a version bump, > > broker > > > couldn't differentiate both. Besides the bumping of the existing > > > protocol is not a big overhead comparing with adding a new RPC, so I > > don't > > > worry too much about the complexity here. > > > > > > > > > > On the other hand, it's not clear if the underlying RPC request > > > > encoding/decode machinery supports embedded requests. Hopefully, even > > if it > > > > doesn't it would not be too difficult to extend. > > > > > > > > > > Making the forwarding behavior more general is great, but could also > come > > > with problems we couldn't anticipate such as usage abusiveness, more > > > changes to auto generation framework and increased metadata overhead. > At > > > the moment, we don't expect the direct forwarding would be a > bottleneck, > > so > > > I'm more inclined to make this proposal as simple as possible for now. > If > > > we do have a strong need in the future, getting the ProxyRequest is > > > definitely worth the effort. > > > > > > > > > > What do you think? > > > > > > > > Regards, > > > > Ray > > > > > > > > > > > > On Wed, Apr 8, 2020 at 4:36 PM Boyang Chen < > reluctanthero...@gmail.com > > > > > > > wrote: > > > > > > > > > Thanks for the info Agam! Will add to the KIP. > > > > > > > > > > On Wed, Apr 8, 2020 at 4:26 PM Agam Brahma <abra...@confluent.io> > > wrote: > > > > > > > > > > > Hi Boyang, > > > > > > > > > > > > The KIP already talks about incorporating changes for > > FindCoordinator > > > > > > request routing, wanted to point out one additional case where > > internal > > > > > > topics are created "as a side effect": > > > > > > > > > > > > As part of handling metadata requests, if we are looking for > > metadata > > > > for > > > > > > an internal topic and auto-topic-creation is enabled [1], the > > broker > > > > > > currently goes ahead and creates the internal topic in the same > > way [2] > > > > > as > > > > > > it would for the FindCoordinator request. > > > > > > > > > > > > -Agam > > > > > > > > > > > > [1] > > > > > > > > > > > > > > > > > > > > > > > > https://github.com/apache/kafka/blob/cd1e46c8bb46f1e5303c51f476c74e33b522fce8/core/src/main/scala/kafka/server/KafkaApis.scala#L1096 > > > > > > [2] > > > > > > > > > > > > > > > > > > > > > > > > https://github.com/apache/kafka/blob/cd1e46c8bb46f1e5303c51f476c74e33b522fce8/core/src/main/scala/kafka/server/KafkaApis.scala#L1041 > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Apr 6, 2020 at 8:25 PM Boyang Chen < > > reluctanthero...@gmail.com > > > > > > > > > > > wrote: > > > > > > > > > > > > > Thanks for the various inputs everyone! > > > > > > > > > > > > > > I think Sonke and Colin's suggestions make sense. The tagged > > field > > > > also > > > > > > > avoids the unnecessary protocol changes for affected requests. > > Will > > > > add > > > > > > it > > > > > > > to the header. As for the verification, I'm not sure whether > it's > > > > > > necessary > > > > > > > to require a higher permission level, as it is just an > ignorable > > > > field? > > > > > > > > > > > > > > Guozhang's suggestions about metrics also sound great, I will > > think > > > > > > through > > > > > > > the use cases and make some changes to the KIP. > > > > > > > > > > > > > > Best, > > > > > > > Boyang > > > > > > > > > > > > > > On Mon, Apr 6, 2020 at 4:28 PM Guozhang Wang < > wangg...@gmail.com > > > > > > > > wrote: > > > > > > > > > > > > > > > Thanks for the KIP Boyang, this looks good to me. Some minor > > > > > comments: > > > > > > > > > > > > > > > > 1) I think in order to implement the forwarding mechanism the > > > > brokers > > > > > > > needs > > > > > > > > some purgatory to keep the forwarded requests; if that's > true, > > > > should > > > > > > we > > > > > > > > add some broker-side metrics for those purgatories for > > debugging > > > > > > > purposes? > > > > > > > > > > > > > > > > 2) Should we also consider adding some extra metric counting > > old > > > > > > > versioned > > > > > > > > admin client request rates (this goes beyond > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-511%3A+Collect+and+Expose+Client%27s+Name+and+Version+in+the+Brokers > > > > > > > > since > > > > > > > > old versioned client would not report its Kafka version > > anyways); > > > > one > > > > > > use > > > > > > > > case I can think of besides debugging purposes, is that if we > > ever > > > > > > > decides > > > > > > > > to break compatibility in future versions way after the > bridge > > > > > > releases, > > > > > > > to > > > > > > > > reject any v1 requests and hence can totally remove this > > forwarding > > > > > > logic > > > > > > > > on brokers, we can leverage on this metric to find a safe > time > > to > > > > > > > upgrade. > > > > > > > > > > > > > > > > > > > > > > > > Guozhang > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Apr 6, 2020 at 3:50 PM Colin McCabe < > > cmcc...@apache.org> > > > > > > wrote: > > > > > > > > > > > > > > > > > Hi Sönke, > > > > > > > > > > > > > > > > > > Yeah, that was my thought too. The request has already > been > > > > > > validated > > > > > > > on > > > > > > > > > the forwarding broker, so we don't need to validate it > again. > > > > > > However, > > > > > > > > you > > > > > > > > > make a good point that it's unfortunate that the audit log > > would > > > > > lose > > > > > > > the > > > > > > > > > principal information if we didn't forward it as well. > > > > > > > > > > > > > > > > > > Perhaps we could add a tagged field to the request header > > for all > > > > > > > > > messages. This field would contain the principal name. Of > > > > course, > > > > > > > this > > > > > > > > > field should only be allowed if the request arrives with > the > > > > > highest > > > > > > > > > permission levels (Probably ClusterAction on Cluster, since > > > > that's > > > > > > what > > > > > > > > all > > > > > > > > > the brokers have.) > > > > > > > > > > > > > > > > > > regards, > > > > > > > > > Colin > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Apr 6, 2020, at 14:37, Sönke Liebau wrote: > > > > > > > > > > Hi Boyang, > > > > > > > > > > > > > > > > > > > > thanks for the KIP. Sounds good overall. > > > > > > > > > > > > > > > > > > > > @Tom: I thought about your remark a little and think that > > in > > > > > > > principle > > > > > > > > we > > > > > > > > > > can get away without forwarding the principal at all. > > Brokers > > > > > > > currently > > > > > > > > > > authenticate and authorize requests before performing > > writes to > > > > > > > > > Zookeeper - > > > > > > > > > > as long as we don't change that it shouldn't matter, > > whether > > > > the > > > > > > > write > > > > > > > > > goes > > > > > > > > > > to ZK or the controller, as long as that request is > > properly > > > > > > > > > authenticated. > > > > > > > > > > So the broker would simply authorize and authenticate the > > > > > original > > > > > > > > > request > > > > > > > > > > and then forward it to the controller using its own > > > > credentials. > > > > > > And > > > > > > > > the > > > > > > > > > > controller could simply trust that this is a bona-fide > > request, > > > > > > > because > > > > > > > > > it > > > > > > > > > > came from a trusted peer. > > > > > > > > > > > > > > > > > > > > I can see two issues here, one is a bit academic I > think.. > > > > > > > > > > > > > > > > > > > > 1. The controller would be unable to write a proper audit > > log, > > > > > > > because > > > > > > > > it > > > > > > > > > > cannot know who sent the original request. > > > > > > > > > > > > > > > > > > > > 2. In theory, clusters could use Plaintext Listeners for > > inter > > > > > > broker > > > > > > > > > > traffic because that is on a separate, secure network or > > > > similar > > > > > > > > reasons. > > > > > > > > > > In that case, the forwarded request would be > > unauthenticated - > > > > > then > > > > > > > > > again, > > > > > > > > > > so are all other requests between brokers, so nothing > lost > > > > > really. > > > > > > > > > > > > > > > > > > > > Overall though, I think that sending the principal along > > with > > > > the > > > > > > > > request > > > > > > > > > > shouldn't be a large issue though, it is just two Strings > > and a > > > > > > > > boolean. > > > > > > > > > > And the controller could bypass the PrincipalBuilder and > > just > > > > > pass > > > > > > > the > > > > > > > > > > Principal that was built and sent by the remote broker > > straight > > > > > to > > > > > > > the > > > > > > > > > > Authorizer. Since PrincipalBuilders are the same on all > > brokers > > > > > it > > > > > > > > > > shouldn't matter who does the processing I think. > > > > > > > > > > > > > > > > > > > > Best regards, > > > > > > > > > > Sönke > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, 6 Apr 2020 at 22:30, Boyang Chen < > > > > > > reluctanthero...@gmail.com > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > Thanks Tom for the question! I'm not super familiar > with > > the > > > > > > > > Principal > > > > > > > > > > > stuff, could you elaborate more on the two points you > > > > proposed > > > > > > > here? > > > > > > > > > > > > > > > > > > > > > > I looked up Admin client and just take > > > > `createDelegationToken` > > > > > > API > > > > > > > > for > > > > > > > > > an > > > > > > > > > > > example, the request data encodes the principal > > information > > > > > > > already, > > > > > > > > so > > > > > > > > > > > broker should also leverage that information to proxy > the > > > > > request > > > > > > > > IMHO. > > > > > > > > > > > > > > > > > > > > > > Boyang > > > > > > > > > > > > > > > > > > > > > > On Mon, Apr 6, 2020 at 9:21 AM Tom Bentley < > > > > > tbent...@redhat.com> > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > Hi Boyang, > > > > > > > > > > > > > > > > > > > > > > > > Thanks for the KIP! > > > > > > > > > > > > > > > > > > > > > > > > When a broker proxies a request to the controller how > > does > > > > > the > > > > > > > > > > > > authenticated principal get propagated? I think a > > couple of > > > > > > > things > > > > > > > > > might > > > > > > > > > > > > complicate this: > > > > > > > > > > > > > > > > > > > > > > > > 1. A PrincipalBuilder might be in use, > > > > > > > > > > > > 2. A Principal does not have to be serializable. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Kind regards, > > > > > > > > > > > > > > > > > > > > > > > > Tom > > > > > > > > > > > > > > > > > > > > > > > > On Sat, Apr 4, 2020 at 12:52 AM Boyang Chen < > > > > > > > > > reluctanthero...@gmail.com> > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > Hey all, > > > > > > > > > > > > > > > > > > > > > > > > > > I would like to start off the discussion for > > KIP-590, a > > > > > > > follow-up > > > > > > > > > > > > > initiative after KIP-500: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-590%3A+Redirect+Zookeeper+Mutation+Protocols+to+The+Controller > > > > > > > > > > > > > > > > > > > > > > > > > > This KIP proposes to migrate existing Zookeeper > > mutation > > > > > > paths, > > > > > > > > > > > including > > > > > > > > > > > > > configuration, security and quota changes, to > > > > > controller-only > > > > > > > by > > > > > > > > > always > > > > > > > > > > > > > routing these alterations to the controller. > > > > > > > > > > > > > > > > > > > > > > > > > > Let me know your thoughts! > > > > > > > > > > > > > > > > > > > > > > > > > > Best, > > > > > > > > > > > > > Boyang > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > Sönke Liebau > > > > > > > > > > Partner > > > > > > > > > > Tel. +49 179 7940878 > > > > > > > > > > OpenCore GmbH & Co. KG - Thomas-Mann-Straße 8 - 22880 > > Wedel - > > > > > > Germany > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > -- Guozhang > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > -Agam > > > > > > > > > > > > > > > > > > > > >
