I'm sorry guys, I'm a bit busy with something else this week. But I will get back to his till the end of the week.
Thanks & Regards Jakub On Tue, Jan 9, 2018 at 1:19 AM, Ewen Cheslack-Postava <e...@confluent.io> wrote: > On Mon, Jan 8, 2018 at 11:39 AM, Randall Hauch <rha...@gmail.com> wrote: > > > Nice feedback, Ewen. Thanks! > > > > On Thu, Jan 4, 2018 at 5:11 PM, Ewen Cheslack-Postava <e...@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. > > > > Yeah, a related case i was thinking of is how key.converter and > value.converter overrides work in Connectors. It's not exactly the same, > but in that case, if you include the key.converter setting in the connector > config, then nothing with key.converter prefix from the worker is passed > along. Just might be worth clarifying the all-or-nothing behavior. Also how > we apply it in this case (e.g. is there one key setting we can use that, if > it appears, then we do not inherit any security configs from the worker?) > > > > > > > > > > > > 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 <ja...@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 <ja...@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 <rha...@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 <yuzhih...@gmail.com> > > wrote: > > > > >> > > > > >> > +1 to this proposal. > > > > >> > > > > > >> > On Mon, Oct 16, 2017 at 7:49 AM, Jakub Scholz <ja...@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 < > ja...@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 < > > > rha...@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 < > > > rha...@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 < > > > ja...@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 < > > > ja...@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 > > > > >> > > >> >> > > > > > >> > > >> >> > > > > >> > > >> > > > > > >> > > >> > > > > > >> > > >> > > > > >> > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > > > > > > > > > > > > > > > > >
