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