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