Certainly. I think it is reasonable to create a separate KIP to enforce the
topic creation policy. After all the administrator needs a guarantee that
the policy that they have specified in the broker will be enforce --
otherwise the feature doesn't seem complete.
On Tue, May 30, 2017 at 4:45 PM, Ismael Juma <ism...@juma.me.uk> wrote:
> I am not sure if the additional complexity in the Controller is worth it
> for this use case. It seems like it would be better to swap the tools to
> use AdminClient and then restrict access to ZK (via ACLs and/or network
> segmentation). Either way, that proposal should be done via a separate KIP
> as KIP-108 was specifically about create topic requests that are done via
> the Kafka protocol.
>
> Ismael
>
> On Wed, May 31, 2017 at 12:37 AM, Dong Lin <lindon...@gmail.com> wrote:
>
> > On Tue, May 30, 2017 at 4:26 PM, Colin McCabe <cmcc...@apache.org>
> wrote:
> >
> > > On Tue, May 30, 2017, at 15:55, Dong Lin wrote:
> > > > Hey Colin,
> > > >
> > > > I think one big advantage of the broker side config is that it can
> not
> > be
> > > > ignored by the malicious client, right?
> > >
> > > Hi Dong,
> > >
> > > The scenario I was thinking of is where a malicious client communicates
> > > directly with ZooKeeper, bypassing the broker. As far as I can see,
> > > nothing that we do on the broker can prevent this from happening. It
> > > has to be blocked by ZooKeeper itself.
> > >
> >
> > I see. I agree that malicious client can still create the topic via
> > zookeeper if we don't have ACL. The approach using the new config can
> > prevent non-malicious client from using old script to create topic via
> > zookeeper.
> >
> >
> > >
> > > >
> > > > Thanks,
> > > > Dong
> > > >
> > > > On Tue, May 30, 2017 at 3:53 PM, Dong Lin <lindon...@gmail.com>
> wrote:
> > > >
> > > > > Do we have an old version of bin/kafka-topics.sh which creates
> topic
> > > via
> > > > > ZK and still allows user to access ZK with ACL? Another concern is
> > that
> > > > > some user may not have ACL service deployed in their cluster. If
> > > neither of
> > > > > these is issue, then I would prefer the zookeeper approach instead
> > of
> > > > > adding a new broker config if the zookeeper approach is doable.
> > >
> > > Unfortunately, the latest version of kafka-topics.sh still creates
> > > topics by talking directly to ZK. It has not been converted to use the
> > > new AdminClient, although that is planned.
> > >
> >
> > Yep. Thus the new config-based approach still has its advantage over
> > ZK-based approach because it ensures that non-malicious user will not
> > create topic via zookeeper.
> >
> >
> > >
> > > best,
> > > Colin
> > >
> > >
> > > > >
> > > > > However, regardless of whether we secure the zookeeper from
> > > unauthorized
> > > > > user, I think KIP-108 should provide a solution to guarantee that
> all
> > > topic
> > > > > creation logic goes through the topic creation policy.
> > > > >
> > > > > Thanks,
> > > > > Dong
> > > > >
> > > > > On Tue, May 30, 2017 at 3:39 PM, Colin McCabe <cmcc...@apache.org>
> > > wrote:
> > > > >
> > > > >> It seems like, to make it really secure, we need the enforcement
> to
> > be
> > > > >> done at the ZooKepeer level. Any broker or client-side
> > configuration
> > > > >> can just be ignored by a malicious client. Do we have
> documentation
> > > or
> > > > >> code that configures ZK to prevent unprivileged users from
> modifying
> > > the
> > > > >> topic configurations?
> > > > >>
> > > > >> best,
> > > > >> Colin
> > > > >>
> > > > >>
> > > > >> On Tue, May 30, 2017, at 15:02, Dong Lin wrote:
> > > > >> > Hey Ismael,
> > > > >> >
> > > > >> > I agree that it makes sense not to cover ZK-based topic creation
> > > with
> > > > >> the
> > > > >> > topic creation policy and limit ZK access to brokers only going
> > > forward.
> > > > >> > My
> > > > >> > point is that we need a way to disable ZK-based topic creation
> so
> > > that
> > > > >> > all
> > > > >> > topic creation goes through the topic creation policy as
> specified
> > > in
> > > > >> > KIP-108. Does this make sense?
> > > > >> >
> > > > >> > One example solution is to add a broker-side config
> > > > >> > "enable.zookeeper.topic.creation"
> > > > >> > which defaults to "true". If user has overridden this config to
> be
> > > > >> > "false",
> > > > >> > then controller will delete the znode /brokers/topics/{topic}
> that
> > > is
> > > > >> not
> > > > >> > created by the controller. We probably need some trick to
> > > differentiate
> > > > >> > between znode created by controller and znode created by
> outdated
> > > tools.
> > > > >> > For example, the new controller code can add a new field
> > > "isController"
> > > > >> > in
> > > > >> > the znode /brokers/topics/{topic} when it creates this new
> znode.
> > > Then
> > > > >> if
> > > > >> > the znode doesn't have this field AND there is no child under
> this
> > > > >> znode,
> > > > >> > controller can be sure it is created by outdated tools and
> remove
> > > this
> > > > >> > znode from zookeeper. Users who are using outdated tools to
> create
> > > topic
> > > > >> > will find that the topic is not created.
> > > > >> >
> > > > >> > Dong
> > > > >> >
> > > > >> > On Tue, May 30, 2017 at 2:24 PM, Ismael Juma <ism...@juma.me.uk
> >
> > > wrote:
> > > > >> >
> > > > >> > > Hi Dong,
> > > > >> > >
> > > > >> > > No, ZK-based topic creation doesn't go through the policy
> since
> > it
> > > > >> doesn't
> > > > >> > > go through the broker. Given that, I am not sure how the
> broker
> > > config
> > > > >> > > would work. Can you please elaborate? It seems like the way
> > > forward
> > > > >> is to
> > > > >> > > limit ZK access to brokers only.
> > > > >> > >
> > > > >> > > Ismael
> > > > >> > >
> > > > >> > > On Tue, May 30, 2017 at 10:19 PM, Dong Lin <
> lindon...@gmail.com
> > >
> > > > >> wrote:
> > > > >> > >
> > > > >> > > > Hey Ismael,
> > > > >> > > >
> > > > >> > > > Thanks for the KIP. This is definitely useful.
> > > > >> > > >
> > > > >> > > > Does the KIP apply the topic creation policy to ZK-based
> topic
> > > > >> creation?
> > > > >> > > If
> > > > >> > > > not, which seems to be the case from my understanding,
> should
> > we
> > > > >> have a
> > > > >> > > new
> > > > >> > > > broker config to disable ZK-based topic creation? This seems
> > > > >> necessary to
> > > > >> > > > prevent user from using stray builds to evade the topic
> > creation
> > > > >> policy.
> > > > >> > > >
> > > > >> > > > Thanks,
> > > > >> > > > Dong
> > > > >> > > >
> > > > >> > > >
> > > > >> > > >
> > > > >> > > >
> > > > >> > > >
> > > > >> > > >
> > > > >> > > > On Mon, Jan 9, 2017 at 1:42 PM, Roger Hoover <
> > > > >> roger.hoo...@gmail.com>
> > > > >> > > > wrote:
> > > > >> > > >
> > > > >> > > > > Got it. Thanks, Ismael.
> > > > >> > > > >
> > > > >> > > > > On Mon, Jan 9, 2017 at 10:42 AM, Ismael Juma <
> > > ism...@juma.me.uk>
> > > > >> > > wrote:
> > > > >> > > > >
> > > > >> > > > > > Hi Roger,
> > > > >> > > > > >
> > > > >> > > > > > That's a good question. The server defaults are passed
> via
> > > the
> > > > >> > > > > `configure`
> > > > >> > > > > > method of the `Configurable` interface that is
> implemented
> > > by
> > > > >> > > > > > `CreateTopicPolicy`. I'll mention this explicitly in the
> > > KIP.
> > > > >> > > > > >
> > > > >> > > > > > Ismael
> > > > >> > > > > >
> > > > >> > > > > > On Mon, Jan 9, 2017 at 6:04 PM, Roger Hoover <
> > > > >> roger.hoo...@gmail.com
> > > > >> > > >
> > > > >> > > > > > wrote:
> > > > >> > > > > >
> > > > >> > > > > > > This is great. Thanks, Ismael.
> > > > >> > > > > > >
> > > > >> > > > > > > One question. When TopicDetails are passed to the
> > policy
> > > > >> > > > > implementation,
> > > > >> > > > > > > would the server defaults already have been merged?
> If
> > > not, I
> > > > >> > > think
> > > > >> > > > > the
> > > > >> > > > > > > policy also needs access to the server defaults.
> > > > >> > > > > > >
> > > > >> > > > > > > Cheers,
> > > > >> > > > > > >
> > > > >> > > > > > > Roger
> > > > >> > > > > > >
> > > > >> > > > > > > On Fri, Jan 6, 2017 at 9:26 AM, Ismael Juma <
> > > > >> ism...@juma.me.uk>
> > > > >> > > > wrote:
> > > > >> > > > > > >
> > > > >> > > > > > > > Thanks for the review Jun. Yes, that's a good
> point, I
> > > have
> > > > >> > > updated
> > > > >> > > > > the
> > > > >> > > > > > > > KIP.
> > > > >> > > > > > > >
> > > > >> > > > > > > > Ismael
> > > > >> > > > > > > >
> > > > >> > > > > > > > On Fri, Jan 6, 2017 at 5:15 PM, Jun Rao <
> > > j...@confluent.io>
> > > > >> > > wrote:
> > > > >> > > > > > > >
> > > > >> > > > > > > > > Hi, Ismael,
> > > > >> > > > > > > > >
> > > > >> > > > > > > > > Thanks for the KIP. Looks reasonable to me. To be
> > > > >> consistent
> > > > >> > > with
> > > > >> > > > > the
> > > > >> > > > > > > > > pattern used in other pluggable interfaces, we
> > > probably
> > > > >> should
> > > > >> > > > make
> > > > >> > > > > > the
> > > > >> > > > > > > > new
> > > > >> > > > > > > > > interface configurable and closable?
> > > > >> > > > > > > > >
> > > > >> > > > > > > > > Jun
> > > > >> > > > > > > > >
> > > > >> > > > > > > > > On Fri, Jan 6, 2017 at 4:16 AM, Ismael Juma <
> > > > >> ism...@juma.me.uk
> > > > >> > > >
> > > > >> > > > > > wrote:
> > > > >> > > > > > > > >
> > > > >> > > > > > > > > > Thanks Dan and Colin for the feedback. I updated
> > the
> > > > >> KIP to
> > > > >> > > > > include
> > > > >> > > > > > > the
> > > > >> > > > > > > > > > addition of a validation mode. Since we need to
> > > bump the
> > > > >> > > > protocol
> > > > >> > > > > > > > version
> > > > >> > > > > > > > > > for that, I also added an error message per
> topic
> > > to the
> > > > >> > > > > response.
> > > > >> > > > > > I
> > > > >> > > > > > > > had
> > > > >> > > > > > > > > > the latter as "Future Work", but I actually felt
> > > that it
> > > > >> > > should
> > > > >> > > > > be
> > > > >> > > > > > in
> > > > >> > > > > > > > the
> > > > >> > > > > > > > > > first version (good to have feedback confirming
> > > that).
> > > > >> > > > > > > > > >
> > > > >> > > > > > > > > > Let me know if the changes look good to you.
> > > > >> > > > > > > > > >
> > > > >> > > > > > > > > > Ismael
> > > > >> > > > > > > > > >
> > > > >> > > > > > > > > > On Thu, Jan 5, 2017 at 9:54 PM, Colin McCabe <
> > > > >> > > > cmcc...@apache.org
> > > > >> > > > > >
> > > > >> > > > > > > > wrote:
> > > > >> > > > > > > > > >
> > > > >> > > > > > > > > > > Yeah, I agree... having a validation mode
> would
> > be
> > > > >> nice.
> > > > >> > > We
> > > > >> > > > > > should
> > > > >> > > > > > > > be
> > > > >> > > > > > > > > > > explicit that passing validation doesn't 100%
> > > > >> guarantee
> > > > >> > > that
> > > > >> > > > a
> > > > >> > > > > > > > > > > subsequent call to create the topic will
> > succeed,
> > > > >> though.
> > > > >> > > > > There
> > > > >> > > > > > is
> > > > >> > > > > > > > an
> > > > >> > > > > > > > > > > obvious race condition there-- for example,
> > with a
> > > > >> plugin
> > > > >> > > > which
> > > > >> > > > > > > > > consults
> > > > >> > > > > > > > > > > some external authentication system, there
> could
> > > be a
> > > > >> > > change
> > > > >> > > > to
> > > > >> > > > > > the
> > > > >> > > > > > > > > > > privileges in between validation and attempted
> > > > >> creation.
> > > > >> > > > > > > > > > >
> > > > >> > > > > > > > > > > It also seems like we should try to provide a
> > > helpful
> > > > >> > > > exception
> > > > >> > > > > > > > message
> > > > >> > > > > > > > > > > for the cases where topic creation fails.
> This
> > > might
> > > > >> > > involve
> > > > >> > > > > > > adding
> > > > >> > > > > > > > > > > more detail about error conditions to
> > > > >> > > CreateTopicsRequest...
> > > > >> > > > > > right
> > > > >> > > > > > > > now
> > > > >> > > > > > > > > > > it just returns an error code, but a text
> > message
> > > > >> would be
> > > > >> > > a
> > > > >> > > > > nice
> > > > >> > > > > > > > > > > addition.
> > > > >> > > > > > > > > > >
> > > > >> > > > > > > > > > > cheers,
> > > > >> > > > > > > > > > > Colin
> > > > >> > > > > > > > > > >
> > > > >> > > > > > > > > > >
> > > > >> > > > > > > > > > > On Thu, Jan 5, 2017, at 13:41, dan wrote:
> > > > >> > > > > > > > > > > > it would be nice to have a dry-run or
> validate
> > > > >> ability
> > > > >> > > > added
> > > > >> > > > > to
> > > > >> > > > > > > > this
> > > > >> > > > > > > > > > kip.
> > > > >> > > > > > > > > > > > since we are offloading validation to a 3rd
> > > party
> > > > >> > > > > implementor a
> > > > >> > > > > > > > > random
> > > > >> > > > > > > > > > > > user
> > > > >> > > > > > > > > > > > can't know a priori (based solely on kafka
> > > configs)
> > > > >> > > > whether a
> > > > >> > > > > > > call
> > > > >> > > > > > > > > > should
> > > > >> > > > > > > > > > > > succeed without actually creating the topic.
> > > > >> > > > > > > > > > > >
> > > > >> > > > > > > > > > > > a similar case is in connect where there is
> a
> > > > >> separate
> > > > >> > > > > endpoint
> > > > >> > > > > > > > > > > > <https://github.com/apache/kaf
> > > > >> ka/blob/trunk/connect/
> > > > >> > > > > > > > > > > runtime/src/main/java/org/apac
> > > > >> > > he/kafka/connect/runtime/rest/
> > > > >> > > > > > > > resources/
> > > > >> > > > > > > > > > > ConnectorPluginsResource.java#L49-L58>
> > > > >> > > > > > > > > > > > to attempt to validate a connect
> configuration
> > > > >> without
> > > > >> > > > > actually
> > > > >> > > > > > > > > > creating
> > > > >> > > > > > > > > > > > the connector.
> > > > >> > > > > > > > > > > >
> > > > >> > > > > > > > > > > > thanks
> > > > >> > > > > > > > > > > > dan
> > > > >> > > > > > > > > > > >
> > > > >> > > > > > > > > > > >
> > > > >> > > > > > > > > > > > On Thu, Jan 5, 2017 at 7:34 AM, Ismael Juma
> <
> > > > >> > > > > ism...@juma.me.uk
> > > > >> > > > > > >
> > > > >> > > > > > > > > wrote:
> > > > >> > > > > > > > > > > >
> > > > >> > > > > > > > > > > > > Hi all,
> > > > >> > > > > > > > > > > > >
> > > > >> > > > > > > > > > > > > We've posted "KIP-108: Create Topic
> Policy"
> > > for
> > > > >> > > > discussion:
> > > > >> > > > > > > > > > > > >
> > > > >> > > > > > > > > > > > > https://cwiki.apache.org/confl
> > > > >> uence/display/KAFKA/KIP-
> > > > >> > > > > > > > > > > > > 108%3A+Create+Topic+Policy
> > > > >> > > > > > > > > > > > >
> > > > >> > > > > > > > > > > > > Please take a look. Your feedback is
> > > appreciated.
> > > > >> > > > > > > > > > > > >
> > > > >> > > > > > > > > > > > > Thanks,
> > > > >> > > > > > > > > > > > > Ismael
> > > > >> > > > > > > > > > > > >
> > > > >> > > > > > > > > > >
> > > > >> > > > > > > > > >
> > > > >> > > > > > > > >
> > > > >> > > > > > > >
> > > > >> > > > > > >
> > > > >> > > > > >
> > > > >> > > > >
> > > > >> > > >
> > > > >> > >
> > > > >>
> > > > >
> > > > >
> > >
> >
>
