kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Randall Hauch <rha...@gmail.com>
Subject Re: [DISCUSS] KIP-208: Add SSL support to Kafka Connect REST interface
Date Mon, 08 Jan 2018 19:39:55 GMT
Nice feedback, Ewen. Thanks!

On Thu, Jan 4, 2018 at 5:11 PM, Ewen Cheslack-Postava <ewen@confluent.io>
wrote:

> Hey Jakub,
>
> Sorry for not getting to this sooner. Overall the proposal looks good to
> me, I just had a couple of questions.
>
> 1. For the configs/overrides, does this happen on a per-setting basis or if
> one override is included do we not use any of the original settings? I
> suspect that if you need to override one setting, it probably means you're
> using an entirely different config and so the latter behavior seems better
> to me. We've talked a bit about doing something similar for the
> producer/consumer security settings as well so you don't have to specify
> security configs in 3 places in your worker config.
>

Not sure if you were referring to
https://issues.apache.org/jira/browse/KAFKA-6387, but I just withdrew that
proposal (and the corresponding KIP-246) because behavior with existing
configurations was not backward compatible, so existing configs might have
very different behavior after the "inheritance" was implemented.

But regardless, I do think that in this case if you have to override one of
the settings you probably need to override multiple. So I'd be in favor of
requiring all configs to be specified in the overridden `listeners.*`
properties.


>
> 2. For using default values from the worker config, I am wondering how
> convinced we are that it will be common for them to be the same? I really
> don't have enough experience w/ these setups to know, so just a question
> here. I think the other thing to take into account here is that even though
> we're not dealing with authorization in this KIP, we will eventually want
> it for these APIs. Would we expect to be using the same principal for Kafka
> and the Connect REST API? In a case where a company has a Connect cluster
> that, e.g., an ops team manages and they are the only ones that are
> supposed to make changes, that would make sense to me. But for a setup
> where some dev team is allowed to use the REST API to create new connectors
> but the cluster is managed by an ops team, I would think the Kafka
> credentials would be different. I'm not sure how frequent each case would
> be, so I'm a bit unsure about the default of using the worker security
> configs by default. Thoughts?
>
> 3. We should probably specify the default in the table for
> rest.advertised.security.protocol because in ConfigDef if you don't
> specify
> a default value it becomes a required config. The HTTP default will
> probably need to be in there anyway.
>
> 4. Do we want to list the existing settings as deprecated and just move to
> using listeners for consistency? We don't need to remove them anytime soon,
> but given that the broker is doing the same, maybe we should just do that
> in this KIP?
>

Marking them as deprecated in this KIP sounds good to me.

>
> I think these are mostly small details, overall it looks like a good plan!
>

+1

Randall


>
> Thanks,
> Ewen
>
> On Tue, Oct 24, 2017 at 5:19 AM, Jakub Scholz <jakub@scholz.cz> wrote:
>
> > There has been no discussion since my last update week ago. Unless
> someone
> > has some further comments in the next 48 hours, I will start the voting
> for
> > this KIP.
> >
> > Thanks & Regards
> > Jakub
> >
> > On Tue, Oct 17, 2017 at 5:54 PM, Jakub Scholz <jakub@scholz.cz> wrote:
> >
> > > Ok, so I updated the KIP according to what we discussed. Please have a
> > > look at the updates. Two points I'm not 100% sure about:
> > >
> > > 1) Should we mark the rest.host.name and rest.port options as
> > deprecated?
> > >
> > > 2) I needed to also address the advertised hostname / port. With
> multiple
> > > listeners it is not clear anymore which one should be used. I saw as
> one
> > > option to add advertised.listeners option and some modified version of
> > > inter.broker.listener.name option to follow what is done in Kafka
> > > brokers. But for the Connect REST interface, we do not advertise the
> > > address to the clients like in Kafka broker. So we only need to tell
> > other
> > > workers how to connect - and for that we need only one advertised
> > address.
> > > So I decided to reuse the existing rest.advertised.host.name and
> > > rest.advertised.port options and add additional option
> > > rest.advertised.security.protocol to specify whether HTTP or HTTPS
> > should
> > > be used. Does this make sense to you? DO you think this is the right
> > > approach?
> > >
> > > Thanks & Regards
> > > Jakub
> > >
> > > On Mon, Oct 16, 2017 at 6:34 PM, Randall Hauch <rhauch@gmail.com>
> wrote:
> > >
> > >> The broker's configuration options are "listeners" (plural) and
> > >> "listeners.security.protocol.map". I agree that following the pattern
> > set
> > >> by the broker is better, so these are really good ideas. However, at
> > this
> > >> point I don't see a need for the "listeners.security.procotol.map",
> > which
> > >> for the broker must be set if the listener name is not a security
> > >> protocol.
> > >> Can we not simply just allow "HTTP" and "HTTPS" as the names of the
> > >> listeners (rather than the broker's "PLAINTEXT", "SSL", etc.)? If so,
> > then
> > >> for example "listeners" might be set to "http://myhost:8081,
> > >> https://myhost:80", which seems to work out nicely without needing
> > >> listener
> > >> names other than security protocols.
> > >>
> > >> I also like using the worker's SSL and SASL security configs by
> default
> > if
> > >> "https" is included in the listener, but allowing the overriding of
> this
> > >> via other additional properties. Here, I'm not a big fan of
> > >> "listeners.name.https.*" prefix, which I think is pretty verbose, but
> I
> > >> could see "listener.https.*" as a prefix. This allows us to add other
> > >> security protocols at some point, if that ever becomes necessary.
> > >>
> > >> +1 for continuing down this road. Nice work.
> > >>
> > >> On Mon, Oct 16, 2017 at 9:51 AM, Ted Yu <yuzhihong@gmail.com> wrote:
> > >>
> > >> > +1 to this proposal.
> > >> >
> > >> > On Mon, Oct 16, 2017 at 7:49 AM, Jakub Scholz <jakub@scholz.cz>
> > wrote:
> > >> >
> > >> > > I was having some more thoughts about it. We can simply take
over
> > what
> > >> > > Kafka broker implements for the listeners:
> > >> > > - We can take over the "listener" and "
> > listener.security.protocol.ma
> > >> p"
> > >> > > options to define multiple REST listeners and the security
> protocol
> > >> they
> > >> > > should use
> > >> > > - The HTTPS interface will by default use the default
> configuration
> > >> > options
> > >> > > ("ssl.keystore.localtion" etc.). But if desired, the values can
be
> > >> > > overridden for given listener (again, as in Kafka broker "
> > >> listener.name
> > >> > > .<LISTENER_NAME>.ssl.keystore.location")
> > >> > >
> > >> > > This should address both issues raised. But before I incorporate
> it
> > >> into
> > >> > > the KIP, I would love to get some feedback if this sounds OK.
> Please
> > >> let
> > >> > me
> > >> > > know what do you think ...
> > >> > >
> > >> > > Jakub
> > >> > >
> > >> > > On Sun, Oct 15, 2017 at 12:23 AM, Jakub Scholz <jakub@scholz.cz>
> > >> wrote:
> > >> > >
> > >> > > > I agree, adding both HTTP and HTTPS is not complicated.
I just
> > >> didn't
> > >> > saw
> > >> > > > the use case for it. But I can add it. Would you add just
> support
> > >> for a
> > >> > > > single HTTP and single HTTPS interface? Or do you see some
value
> > >> even
> > >> > in
> > >> > > > allowing more than 2 interfaces (for example one HTTP and
two
> > HTTPS
> > >> > with
> > >> > > > different configuration)? It could be done similarly to
how the
> > >> Kafka
> > >> > > > broker does it through the "listener" configuration parameter
> with
> > >> > comma
> > >> > > > separated list. What do you think?
> > >> > > >
> > >> > > > As for the "rest" prefix - if we remove it, some of the
same
> > >> > > configuration
> > >> > > > options are already used today as the option for connecting
from
> > >> Kafka
> > >> > > > Connect to Kafka broker. So I'm not sure we should mix them.
I
> can
> > >> > > > definitely imagine some cases where the client SSL configuration
> > >> will
> > >> > not
> > >> > > > be the same as the REST HTTPS configuration. That is why
I added
> > the
> > >> > > > prefix. If we remove the prefix, how would you deal with
this?
> > >> > > >
> > >> > > > On Fri, Oct 13, 2017 at 6:25 PM, Randall Hauch <
> rhauch@gmail.com>
> > >> > wrote:
> > >> > > >
> > >> > > >> Also, do we need these properties to be preceded with
`rest`?
> I'd
> > >> > argue
> > >> > > >> that we're just configuring the worker's SSL information,
and
> > that
> > >> the
> > >> > > >> REST
> > >> > > >> API would just use that. If we added another non-REST
API, we'd
> > >> want
> > >> > to
> > >> > > >> use
> > >> > > >> the same security configuration.
> > >> > > >>
> > >> > > >> It's not that complicated in Jetty to support both "http"
and
> > >> "https"
> > >> > > >> simultaneously, so IMO we should add that from the beginning.
> > >> > > >>
> > >> > > >> On Fri, Oct 13, 2017 at 9:34 AM, Randall Hauch <
> rhauch@gmail.com
> > >
> > >> > > wrote:
> > >> > > >>
> > >> > > >> > It'd be useful to specify the default values for
the
> > >> configuration
> > >> > > >> > properties.
> > >> > > >> >
> > >> > > >> > On Tue, Oct 10, 2017 at 2:53 AM, Jakub Scholz <
> jakub@scholz.cz
> > >
> > >> > > wrote:
> > >> > > >> >
> > >> > > >> >> FYI: Based on Ewen's suggestion from the related
JIRA, I
> > added a
> > >> > > >> >> clarification to the KIP that it doesn't do
anything around
> > >> > > >> authorization
> > >> > > >> >> /
> > >> > > >> >> ACLs. While authorization / ACLs would be for
sure valuable
> > >> > feature I
> > >> > > >> >> would
> > >> > > >> >> prefer to leave it for different KIP.
> > >> > > >> >>
> > >> > > >> >> Jakub
> > >> > > >> >>
> > >> > > >> >> On Mon, Oct 9, 2017 at 5:25 PM, Jakub Scholz
<
> jakub@scholz.cz
> > >
> > >> > > wrote:
> > >> > > >> >>
> > >> > > >> >> > Hi,
> > >> > > >> >> >
> > >> > > >> >> > I would like to start a discussion about
KIP-208: Add SSL
> > >> support
> > >> > > to
> > >> > > >> >> Kafka
> > >> > > >> >> > Connect REST interface (https://cwiki.apache.org/
> > >> > > >> >> > confluence/display/KAFKA/KIP-208%3A+Add+SSL+support+to+
> > >> > > >> >> > Kafka+Connect+REST+interface).
> > >> > > >> >> >
> > >> > > >> >> > I think this would be useful feature to
improve the
> security
> > >> of
> > >> > > Kafka
> > >> > > >> >> > Connect.
> > >> > > >> >> >
> > >> > > >> >> > Thanks & Regards
> > >> > > >> >> > Jakub
> > >> > > >> >> >
> > >> > > >> >>
> > >> > > >> >
> > >> > > >> >
> > >> > > >>
> > >> > > >
> > >> > > >
> > >> > >
> > >> >
> > >>
> > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message