Hi Colin,
The KIP states in the Compatibility section (not Future work):
"To support the proxy of requests, we need to build a channel for brokers
to talk directly to the controller. This part of the design is internal
change only and won’t block the KIP progress."
I am clarifying that this is not internal only due to the config. If we say
that this KIP depends on another KIP before we can merge it, that's fine
although it feels a bit unnecessary.
"However, I can't help but feel that the fact that create topic policy can
be bypassed via auto topic creation is a bug."
Yeah, I totally agree that it's a bug. And I think the change here is good.
But we need to explain the details. One suggestion is to return
TopicAuthorizationFailed for older clients (closest non retriable exception
I could find) and introduce logic in new clients to handle PolicyViolation
from MetadataRequest. And mention this in the release notes.
Ismael
On Fri, Jun 19, 2020 at 1:02 PM Colin McCabe <cmcc...@apache.org> wrote:
> On Thu, Jun 18, 2020, at 10:20, Ismael Juma wrote:
> > Hi Boyang,
> >
> > Sorry for being late on this. I'm generally in favor of the KIP, but it
> > seems like it has two missing things:
> >
> > 1. It doesn't cover how this new channel will be configured. That's a
> > critical part of having a KIP that comprises all that is needed to merge
> > the relevant PR.
>
> Hi Ismael,
>
> Thanks for taking a look.
>
> To be clear, this KIP doesn't add the new controller channel. It just
> discussed the fact that we're going to have a dedicated channel in the
> KIP-500 controller in the future. We'll have a KIP in the future that will
> spell this out more concretely.
>
> That's why this is in the future work section. We won't do it without
> having another KIP which will specify it in more detail.
>
> > 2. It doesn't state the compatibility impact of enforcing the create
> topics
> > policy on auto topic creation. Furthermore, it doesn't explain the error
> > code that will be returned if the topic policy fails. Older clients won't
> > expect a PolicyValidationError during metadata requests, so we need to
> > discuss the approach there and what will happen for newer clients.
>
> I agree that it's good to surface this compatibility issue. However, I
> can't help but feel that the fact that create topic policy can be bypassed
> via auto topic creation is a bug. It's hard to imagine how a create topic
> policy could be useful if it's trivially bypassable by sending an
> unprivileged metadata request that any client could send.
>
> I suppose we could have a compatibility setting where if your broker has a
> create topics policy set, it uses the old direct-to-zk path. And then we
> could start defaulting that to false in AK 3 and remove later on....
>
> best,
> Colin
>
> >
> > Ismael
> >
> > On Thu, Jun 18, 2020 at 7:57 AM Boyang Chen <reluctanthero...@gmail.com>
> > wrote:
> >
> > > Thanks everyone for the vote! We already got 3 binding votes (Colin,
> > > Guozhang, Rajini) and 2 non-binding votes (Jose, David). The KIP is now
> > > approved.
> > >
> > > Best,
> > > Boyang
> > >
> > > On Thu, Jun 18, 2020 at 1:43 AM Rajini Sivaram <
> rajinisiva...@gmail.com>
> > > wrote:
> > >
> > > > +1 (binding)
> > > >
> > > > Thanks for the KIP, Boyang!
> > > >
> > > > Regards,
> > > >
> > > > Rajini
> > > >
> > > >
> > > >
> > > >
> > > > On Thu, Jun 18, 2020 at 8:15 AM David Jacot <dja...@confluent.io>
> wrote:
> > > >
> > > > > +1 (non-binding)
> > > > >
> > > > > Thanks for the KIP!
> > > > >
> > > > > On Thu, Jun 18, 2020 at 12:00 AM Jose Garcia Sancio <
> > > > jsan...@confluent.io>
> > > > > wrote:
> > > > >
> > > > > > +1.
> > > > > >
> > > > > > Thanks for the KIP and looking forward to the improvement
> > > > implementation.
> > > > > >
> > > > > > On Wed, Jun 17, 2020 at 2:24 PM Guozhang Wang <
> wangg...@gmail.com>
> > > > > wrote:
> > > > > > >
> > > > > > > Thanks for the KIP Boyang, +1 from me.
> > > > > > >
> > > > > > >
> > > > > > > Guozhang
> > > > > > >
> > > > > > > On Wed, Jun 17, 2020 at 1:40 PM Colin McCabe <
> cmcc...@apache.org>
> > > > > wrote:
> > > > > > >
> > > > > > > > Thanks, Boyang! +1 (binding)
> > > > > > > >
> > > > > > > > best,
> > > > > > > > Colin
> > > > > > > >
> > > > > > > > On Mon, Jun 15, 2020, at 12:59, Boyang Chen wrote:
> > > > > > > > > Thanks for more feedback Colin! I have addressed them in
> the
> > > KIP.
> > > > > > > > >
> > > > > > > > > Boyang
> > > > > > > > >
> > > > > > > > > On Mon, Jun 15, 2020 at 11:29 AM Colin McCabe <
> > > > cmcc...@apache.org>
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > > On Fri, Jun 12, 2020, at 15:30, Boyang Chen wrote:
> > > > > > > > > > > Thanks Colin for the suggestions!
> > > > > > > > > > >
> > > > > > > > > > > On Fri, Jun 12, 2020 at 2:40 PM Colin McCabe <
> > > > > cmcc...@apache.org
> > > > > > >
> > > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > > Hi Boyang,
> > > > > > > > > > > >
> > > > > > > > > > > > Thanks for the KIP! I think it's getting close.
> > > > > > > > > > > >
> > > > > > > > > > > > > For older requests that need redirection,
> forwarding
> > > > > > > > > > > > > broker will just use its own authorizer to verify
> the
> > > > > > > > principals.
> > > > > > > > > > When
> > > > > > > > > > > > the
> > > > > > > > > > > > > request looks good, it will just forward the
> request
> > > > with
> > > > > > its
> > > > > > > > own
> > > > > > > > > > > > > credentials, no second validation needed
> > > > > > > > > > > >
> > > > > > > > > > > > Just to be clear, the controller will still validate
> the
> > > > > > request,
> > > > > > > > > > right?
> > > > > > > > > > > > But at that point the principal will be the broker
> > > > principal.
> > > > > > It
> > > > > > > > > > would be
> > > > > > > > > > > > good to note that here.
> > > > > > > > > > > >
> > > > > > > > > > > > Sounds good, cleared in the KIP.
> > > > > > > > > > >
> > > > > > > > > > > > Internal CreateTopicsRequest Routing
> > > > > > > > > > > >
> > > > > > > > > > > > The forwarding broker is sending the request as the
> > > latest
> > > > > > version,
> > > > > > > > > > > > right? It would be good to add a note of this. This
> > > also
> > > > > > prevents
> > > > > > > > > > routing
> > > > > > > > > > > > loops since the latest version is not forwardable
> > > (another
> > > > > good
> > > > > > > > thing
> > > > > > > > > > to
> > > > > > > > > > > > add, I think...)
> > > > > > > > > > > >
> > > > > > > > > > > We are not bumping the CreateTopic RPC here, so it
> should
> > > be
> > > > > the
> > > > > > > > latest
> > > > > > > > > > > by default.
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Sorry, CreateTopics was a bad example here, since it
> already
> > > > must
> > > > > > be
> > > > > > > > sent
> > > > > > > > > > to the controller. Oops.
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > And just to be clear, we are not "forwarding" but
> actually
> > > > > > > > > > > sending a CreateTopicRequest from the receiving broker
> to
> > > the
> > > > > > > > controller
> > > > > > > > > > > broker.
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Right. I think we agree on this point. But we do need a
> > > term
> > > > to
> > > > > > > > describe
> > > > > > > > > > the broker which initially receives the user request and
> > > > resends
> > > > > > it to
> > > > > > > > the
> > > > > > > > > > controller. Resending broker?
> > > > > > > > > >
> > > > > > > > > > And I do think it's important to note that the request we
> > > send
> > > > to
> > > > > > the
> > > > > > > > > > controller can't be itself resent.
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > As we discussed in the request routing section, to
> work
> > > > with
> > > > > > an
> > > > > > > > older
> > > > > > > > > > > > > client, the first contacted broker need to act as
> a
> > > > proxy
> > > > > to
> > > > > > > > > > redirect
> > > > > > > > > > > > the
> > > > > > > > > > > > > write request to the controller. To support the
> proxy
> > > of
> > > > > > > > requests,
> > > > > > > > > > we
> > > > > > > > > > > > need
> > > > > > > > > > > > > to build a channel for brokers to talk directly
> to the
> > > > > > > > controller.
> > > > > > > > > > This
> > > > > > > > > > > > > part of the design is internal change only and
> won’t
> > > > block
> > > > > > the
> > > > > > > > KIP
> > > > > > > > > > > > > progress.
> > > > > > > > > > > >
> > > > > > > > > > > > I think it's good to note that we eventually want a
> > > > separate
> > > > > > > > controller
> > > > > > > > > > > > endpoint in KIP-500. However, we don't need it to
> > > > implement
> > > > > > > > KIP-590,
> > > > > > > > > > > > right? The other brokers could forward to the
> existing
> > > > > > internal
> > > > > > > > > > endpoint
> > > > > > > > > > > > for the controller. So maybe it's best to discuss
> the
> > > > > separate
> > > > > > > > > > endpoint in
> > > > > > > > > > > > "future work" rather than here.
> > > > > > > > > > > >
> > > > > > > > > > > > I moved the new endpoint part towards the future
> work and
> > > > > > > > addressed the
> > > > > > > > > > > > usage of controller internal endpoint for routing
> > > requests.
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Thanks.
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > > =============== Start Old Proposal ===============
> > > > > > > > > > > >
> > > > > > > > > > > > I'm glad the old proposal shows up here, but I think
> this
> > > > is
> > > > > > too
> > > > > > > > much
> > > > > > > > > > > > detail. It would be better to just have a one or two
> > > > > paragraph
> > > > > > > > > > summary of
> > > > > > > > > > > > the main points. As it is, the old proposal takes
> up 40%
> > > > of
> > > > > > the
> > > > > > > > doc
> > > > > > > > > > which
> > > > > > > > > > > > is pretty confusing for someone reading through.
> Let's
> > > > also
> > > > > > not
> > > > > > > > forget
> > > > > > > > > > > > that someone can just read the old version by using
> the
> > > > "page
> > > > > > > > history"
> > > > > > > > > > > > function on the wiki. So there's no need to keep
> that
> > > all
> > > > > > here.
> > > > > > > > > > > >
> > > > > > > > > > > > Make sense, removed.
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Thanks again.
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > { "name": "PrincipalName", "type": "string", "tag":
> 0,
> > > > > > > > > > "taggedVersions": "2+", "ignorable": true,
> > > > > > > > > > > "about": "Optional value of the principal name
> when
> > > the
> > > > > > request
> > > > > > > > is
> > > > > > > > > > redirected by a broker." },
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Maybe "InitialPrincipalName" would be better here?
> > > > PrincipalName
> > > > > > is a
> > > > > > > > bit
> > > > > > > > > > confusing since the message already has a principal name,
> > > after
> > > > > > all...
> > > > > > > > > >
> > > > > > > > > > cheers,
> > > > > > > > > > Colin
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > -- Guozhang
> > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > -Jose
> > > > > >
> > > > >
> > > >
> > >
> >
>
