Hi Chris, I understand your points, and I agree that this path is safer in terms of future plans, I accept it. Updated the KIP accordingly.
Thanks, Daniel Chris Egerton <chr...@aiven.io.invalid> ezt írta (időpont: 2022. szept. 21., Sze, 18:54): > Hi Daniel, > > I'm a little hesitant to add support for REST extensions and the admin API > to dedicated MM2 nodes because they may restrict our options in the future > if/when we add a public-facing REST API. > > Regarding REST extensions specifically, I understand their security value > for public-facing APIs, but for the internal API--which should only ever be > targeted by MM2 nodes, and never by humans, UIs, CLIs, etc.--I'm not sure > there's enough need there to add that API this time around. The internal > endpoints used by Kafka Connect should be secured by the session key > mechanism introduced in KIP-507 [1], and IIUC, with this KIP, users will > also be able to configure their cluster to use mTLS. > > Regarding the admin API, I consider it part of the public-facing REST API > for Kafka Connect, so I was assuming we wouldn't want to add it to this KIP > since we're intentionally slimming down the REST API to just the bare > essentials (i.e., just the internal endpoints). I can see value for it, but > it's similar to the status endpoints in the Kafka Connect REST API; we > might choose to expose this sometime down the line, but IMO it'd be better > to do that in a separate KIP so that we can iron out the details of exactly > what kind of REST API would best suit dedicated MM2 clusters, and how that > would differ from the API provided by vanilla Kafka Connect. > > Let me know what you think! > > Cheers, > > Chris > > [1] - > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-507%3A+Securing+Internal+Connect+REST+Endpoints > > On Wed, Sep 21, 2022 at 4:59 AM Dániel Urbán <urb.dani...@gmail.com> > wrote: > > > Hi Chris, > > > > About the worker id: makes sense. I changed the wording, but kept it > listed > > as it is a change compared to existing MM2 code. Updated the KIP > > accordingly. > > > > About the REST server configurations: again, I agree, it should be > > generalized. But I'm not sure about those exceptions you listed, as all > of > > them make sense in MM2 as well. For example, security related rest > > extensions could work with MM2 as well. Admin listeners are also useful, > as > > they would allow the same admin operations for MM2 nodes. Am I mistaken > > here? Updated the KIP without mentioning exceptions for now. > > > > I think yes, the lazy config resolution should be unconditional. Even if > we > > don't consider the distributed mode of MM2, the eager resolution does not > > allow using sensitive configs in the mm2 properties, as they will be > > written by value into the config topic. I didn't really consider this as > > breaking change, but I might be wrong. > > > > Enable flag property name: also makes sense, updated the KIP. > > > > Thanks a lot! > > Daniel > > > > Chris Egerton <chr...@aiven.io.invalid> ezt írta (időpont: 2022. szept. > > 20., K, 20:29): > > > > > Hi Daniel, > > > > > > Looking pretty good! Regarding the worker ID generation--apologies, I > was > > > unclear with my question. I was wondering if we had to adopt any > special > > > logic at all for MM2, or if we could use the same logic that vanilla > > Kafka > > > Connect does in distributed mode, where the worker ID is its advertised > > URL > > > (e.g., "connect:8083" or "localhost:25565"). Unless I'm mistaken, this > > > should allow MM2 nodes to be identified unambiguously. Do you think it > > > makes sense to follow this strategy? > > > > > > Now that the details on the new REST interface have been fleshed out, > I'm > > > also wondering if we want to add support for the " > > > rest.advertised.host.name", > > > "rest.advertised.port", and "rest.advertised.listener" properties, > which > > > are vital for being able to run Kafka Connect in distributed mode from > > > within containers. In fact, I'm wondering if we can generalize some of > > the > > > specification in the KIP around which REST properties are accepted by > > > stating that all REST-related properties that are available with > vanilla > > > Kafka Connect will be supported for dedicated MM2 nodes when > > > "mm.enable.rest" is set to "true", except for ones related to the > > > public-facing REST API like "rest.extension.classes", > "admin.listeners", > > > and "admin.listeners.https.*". > > > > > > One other thought--is the lazy evaluation of config provider references > > > going to take place unconditionally, or only when the internal REST API > > is > > > enabled on a worker? > > > > > > Finally, do you think we could change "mm.enable.rest" to > > > "mm.enable.internal.rest"? That way, if we want to introduce a > > > public-facing REST API later on, we can use "mm.enable.rest" as the > name > > > for that property. > > > > > > Cheers, > > > > > > Chris > > > > > > On Fri, Sep 16, 2022 at 9:28 AM Dániel Urbán <urb.dani...@gmail.com> > > > wrote: > > > > > > > Hi Chris, > > > > > > > > I went through the KIP and updated it based on our discussion. I > think > > > your > > > > suggestions simplified (and shortened) the KIP significantly. > > > > > > > > Thanks, > > > > Daniel > > > > > > > > Dániel Urbán <dur...@cloudera.com.invalid> ezt írta (időpont: 2022. > > > szept. > > > > 16., P, 15:15): > > > > > > > > > Hi Chris, > > > > > > > > > > 1. For the REST-server-per-flow setup, it made sense to introduce > > some > > > > > simplified configuration. With a single REST server, it doesn't > make > > > > sense > > > > > anymore, I'm removing it from the KIP. > > > > > 2. I think that changing the worker ID generation still makes > sense, > > > > > otherwise there is no way to differentiate between the MM2 > instances. > > > > > > > > > > Thanks, > > > > > Daniel > > > > > > > > > > On Wed, Aug 31, 2022 at 8:39 PM Chris Egerton > > <chr...@aiven.io.invalid > > > > > > > > > wrote: > > > > > > > > > > > Hi Daniel, > > > > > > > > > > > > I've taken a look at the KIP in detail. Here are my complete > > thoughts > > > > > > (minus the aforementioned sections that may be affected by > changes > > to > > > > an > > > > > > internal-only REST API): > > > > > > > > > > > > 1. Why introduce new mm.host.name and mm.rest.protocol > properties > > > > > instead > > > > > > of using the properties that are already used by Kafka Connect: > > > > > listeners, > > > > > > rest.advertised.host.name, rest.advertised.host.port, and > > > > > > rest.advertised.listener? We used to have the rest.host.name and > > > > > rest.port > > > > > > properties in Connect but deprecated and eventually removed them > in > > > > favor > > > > > > of the listeners property in KIP-208 [1]; I'm hoping we can keep > > > things > > > > > as > > > > > > similar as possible between MM2 and Connect in order to make it > > > easier > > > > > for > > > > > > users to work with both. I'm also hoping that we can allow users > to > > > > > > configure the port that their MM2 nodes listen on instead of > > > hardcoding > > > > > MM2 > > > > > > to bind to port 0. > > > > > > > > > > > > 2. Do we still need to change the worker IDs that get used in the > > > > status > > > > > > topic? > > > > > > > > > > > > Everything else looks good, or should change once the KIP is > > updated > > > > with > > > > > > the internal-only REST API alternative. > > > > > > > > > > > > Cheers, > > > > > > > > > > > > Chris > > > > > > > > > > > > [1] - > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-208%3A+Add+SSL+support+to+Kafka+Connect+REST+interface > > > > > > > > > > > > On Mon, Aug 29, 2022 at 1:55 PM Chris Egerton <chr...@aiven.io> > > > wrote: > > > > > > > > > > > > > Hi Daniel, > > > > > > > > > > > > > > Yeah, I think that's the way to go. Adding multiple servers for > > > each > > > > > > > herder seems like it'd be too much of a pain for users to > > > configure, > > > > > and > > > > > > if > > > > > > > we keep the API strictly internal for now, we shouldn't be > > painting > > > > > > > ourselves into too much of a corner if/when we decide to > expose a > > > > > > > public-facing REST API for dedicated MM2 clusters. > > > > > > > > > > > > > > I plan to take a look at the rest of the KIP and provide a > > complete > > > > > > review > > > > > > > sometime this week; I'll hold off on commenting on anything > that > > > > seems > > > > > > like > > > > > > > it'll be affected by switching to an internal-only REST API > until > > > > those > > > > > > > changes are published, but should be able to review everything > > > else. > > > > > > > > > > > > > > Cheers, > > > > > > > > > > > > > > Chris > > > > > > > > > > > > > > On Mon, Aug 29, 2022 at 6:57 AM Dániel Urbán < > > > urb.dani...@gmail.com> > > > > > > > wrote: > > > > > > > > > > > > > >> Hi Chris, > > > > > > >> > > > > > > >> I understand your point, sounds good to me. > > > > > > >> So in short, we should opt for an internal-only API, and > > > preferably > > > > a > > > > > > >> single server solution. Is that right? > > > > > > >> > > > > > > >> Thanks > > > > > > >> Daniel > > > > > > >> > > > > > > >> Chris Egerton <chr...@aiven.io.invalid> ezt írta (időpont: > > 2022. > > > > aug. > > > > > > >> 26., > > > > > > >> P, 17:36): > > > > > > >> > > > > > > >> > Hi Daniel, > > > > > > >> > > > > > > > >> > Glad to hear from you! > > > > > > >> > > > > > > > >> > With regards to the stripped-down REST API alternative, I > > don't > > > > see > > > > > > how > > > > > > >> > this would prevent us from introducing the fully-fledged > > Connect > > > > > REST > > > > > > >> API, > > > > > > >> > or even an augmented variant of it, at some point down the > > road. > > > > If > > > > > we > > > > > > >> go > > > > > > >> > with the internal-only API now, and want to expand later, > > can't > > > we > > > > > > gate > > > > > > >> the > > > > > > >> > expansion behind a feature flag configuration property that > by > > > > > default > > > > > > >> > disables the new feature? > > > > > > >> > > > > > > > >> > I'm also not sure that we'd ever want to expose the raw > > Connect > > > > REST > > > > > > API > > > > > > >> > for dedicated MM2 clusters. If people want that capability, > > they > > > > can > > > > > > >> > already spin up a vanilla Connect cluster and run as many > MM2 > > > > > > >> connectors as > > > > > > >> > they'd like on it, and as of KIP-458 [1], it's even possible > > to > > > > use > > > > > a > > > > > > >> > single Connect cluster to replicate between any two Kafka > > > clusters > > > > > > >> instead > > > > > > >> > of only targeting the Kafka cluster that the vanilla Connect > > > > cluster > > > > > > >> > operates on top of. I do agree that it'd be great to be able > > to > > > > > > >> dynamically > > > > > > >> > adjust things like topic filters without having to restart a > > > > > dedicated > > > > > > >> MM2 > > > > > > >> > node; I'm just not sure that the vanilla Connect REST API is > > the > > > > > > >> > appropriate way to do that, especially since the exact > > > mechanisms > > > > > that > > > > > > >> make > > > > > > >> > a single Connect cluster viable for replicating across any > two > > > > Kafka > > > > > > >> > clusters could be abused and cause a dedicated MM2 cluster > to > > > > start > > > > > > >> writing > > > > > > >> > to a completely different Kafka cluster that's not even > > defined > > > in > > > > > its > > > > > > >> > config file. > > > > > > >> > > > > > > > >> > Finally, as far as security goes--since this is essentially > a > > > bug > > > > > fix, > > > > > > >> I'm > > > > > > >> > inclined to make it as easy as possible for users to adopt > it. > > > > MTLS > > > > > > is a > > > > > > >> > great start for securing a REST API, but it's not sufficient > > on > > > > its > > > > > > own > > > > > > >> > since anyone who could issue an authenticated REST request > > > against > > > > > the > > > > > > >> MM2 > > > > > > >> > cluster would still be able to make any changes they want > > (with > > > > the > > > > > > >> > exception of accessing internal endpoints, which were > secured > > > with > > > > > > >> > KIP-507). If we were to bring up the fully-fledged Connect > > REST > > > > API, > > > > > > >> > cluster administrators would also likely have to add some > kind > > > of > > > > > > >> > authorization layer to prevent people from using the REST > API > > to > > > > > mess > > > > > > >> with > > > > > > >> > the configurations of the connectors that MM2 brought up. > One > > > way > > > > of > > > > > > >> doing > > > > > > >> > that is to add a REST extension to your Connect cluster, but > > > > > > >> implementing > > > > > > >> > and configuring one in order to be able to run a multi-node > > MM2 > > > > > > cluster > > > > > > >> > without hitting this bug seems like too much work to be > worth > > > it. > > > > > > >> > > > > > > > >> > I think if we had a better picture of what a REST API for > > > > dedicated > > > > > > MM2 > > > > > > >> > clusters would/should look like, then it would be easier to > go > > > > along > > > > > > >> with > > > > > > >> > this, and we could even just add the feature flag in this > KIP > > > > right > > > > > > now > > > > > > >> to > > > > > > >> > address any security concerns. My instinct would be to > address > > > > this > > > > > > in a > > > > > > >> > follow-up KIP in order to reduce scope creep, though, and > keep > > > > this > > > > > > KIP > > > > > > >> > focused on addressing the bug with multi-node dedicated MM2 > > > > > clusters. > > > > > > >> What > > > > > > >> > do you think? > > > > > > >> > > > > > > > >> > Cheers, > > > > > > >> > > > > > > > >> > Chris > > > > > > >> > > > > > > > >> > [1] - > > > > > > >> > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-458%3A+Connector+Client+Config+Override+Policy > > > > > > >> > > > > > > > >> > On Thu, Aug 25, 2022 at 3:55 AM Dániel Urbán < > > > > urb.dani...@gmail.com > > > > > > > > > > > > >> > wrote: > > > > > > >> > > > > > > > >> > > Hi Chris, > > > > > > >> > > > > > > > > >> > > Thanks for bringing this up again :) > > > > > > >> > > > > > > > > >> > > 1. I think that is reasonable, though I find the current > > state > > > > of > > > > > > MM2 > > > > > > >> to > > > > > > >> > be > > > > > > >> > > confusing. The current issue with the distributed mode is > > not > > > > > > >> documented > > > > > > >> > > properly, but maybe the logging will help a bit. > > > > > > >> > > > > > > > > >> > > 2. Going for an internal-only Connect REST version would > > lock > > > > MM2 > > > > > > out > > > > > > >> of > > > > > > >> > a > > > > > > >> > > path where the REST API can be used to dynamically > > reconfigure > > > > > > >> > > replications. For now, I agree, it would be easy to > corrupt > > > the > > > > > > state > > > > > > >> of > > > > > > >> > > MM2 if someone wanted to use the properties and the REST > at > > > the > > > > > same > > > > > > >> > time, > > > > > > >> > > but in the future, we might have a chance to introduce a > > > > different > > > > > > >> config > > > > > > >> > > mechanism, where only the cluster connections have to be > > > > specified > > > > > > in > > > > > > >> the > > > > > > >> > > properties file, and everything else can be configured > > through > > > > > REST > > > > > > >> > > (enabling replications, changing topic filters, etc.). > > Because > > > > of > > > > > > >> this, > > > > > > >> > I'm > > > > > > >> > > leaning towards a full Connect REST API. To avoid issues > > with > > > > > > >> conflicts > > > > > > >> > > between the props file and the REST, we could document > > > security > > > > > best > > > > > > >> > > practices (e.g. turn on basic auth or mTLS on the Connect > > REST > > > > to > > > > > > >> avoid > > > > > > >> > > unwanted interactions). > > > > > > >> > > > > > > > > >> > > 3. That is a good point, and I agree, a big plus for > > > motivation. > > > > > > >> > > > > > > > > >> > > I have a working version of this in which all flows spin > up > > a > > > > > > >> dedicated > > > > > > >> > > Connect REST, but I can give other solutions a try, too. > > > > > > >> > > > > > > > > >> > > Thanks, > > > > > > >> > > Daniel > > > > > > >> > > > > > > > > >> > > Chris Egerton <chr...@aiven.io.invalid> ezt írta > (időpont: > > > > 2022. > > > > > > aug. > > > > > > >> > 24., > > > > > > >> > > Sze, 17:46): > > > > > > >> > > > > > > > > >> > > > Hi Daniel, > > > > > > >> > > > > > > > > > >> > > > I'd like to resurface this KIP in case you're still > > > interested > > > > > in > > > > > > >> > > pursuing > > > > > > >> > > > it. I know it's been a while since you published it, and > > it > > > > > hasn't > > > > > > >> > > received > > > > > > >> > > > much attention, but I'm hoping we can give it a try now > > and > > > > > > finally > > > > > > >> put > > > > > > >> > > > this long-standing bug to rest. To that end, I have some > > > > > thoughts > > > > > > >> about > > > > > > >> > > the > > > > > > >> > > > proposal. This isn't a complete review, but I wanted to > > give > > > > > > enough > > > > > > >> to > > > > > > >> > > get > > > > > > >> > > > the ball rolling: > > > > > > >> > > > > > > > > > >> > > > 1. Some environments with firewalls or strict security > > > > policies > > > > > > may > > > > > > >> not > > > > > > >> > > be > > > > > > >> > > > able to bring up a REST server for each MM2 node. If we > > > decide > > > > > > that > > > > > > >> > we'd > > > > > > >> > > > like to use the Connect REST API (or even just parts of > > it) > > > to > > > > > > >> address > > > > > > >> > > this > > > > > > >> > > > bug with MM2, it does make sense to eventually make the > > > > > > >> availability of > > > > > > >> > > the > > > > > > >> > > > REST API a hard requirement for running MM2, but it > might > > > be a > > > > > bit > > > > > > >> too > > > > > > >> > > > abrupt to do that all in a single release. What do you > > think > > > > > about > > > > > > >> > making > > > > > > >> > > > the REST API optional for now, but noting that it will > > > become > > > > > > >> required > > > > > > >> > > in a > > > > > > >> > > > later release (probably 4.0.0 or, if that's not enough > > time, > > > > > > >> 5.0.0)? We > > > > > > >> > > > could choose not to bring the REST server for any node > > whose > > > > > > >> > > configuration > > > > > > >> > > > doesn't explicitly opt into one, and maybe log a warning > > > > message > > > > > > on > > > > > > >> > > startup > > > > > > >> > > > if none is configured. In effect, we'd be marking the > > > current > > > > > mode > > > > > > >> (no > > > > > > >> > > REST > > > > > > >> > > > server) as deprecated. > > > > > > >> > > > > > > > > > >> > > > 2. I'm not sure that we should count out the "Creating > an > > > > > > >> internal-only > > > > > > >> > > > derivation of the Connect REST API" rejected > alternative. > > > > Right > > > > > > now, > > > > > > >> > the > > > > > > >> > > > single source of truth for the configuration of a MM2 > > > cluster > > > > > > >> (assuming > > > > > > >> > > > it's being run in dedicated mode, and not as a connector > > in > > > a > > > > > > >> vanilla > > > > > > >> > > > Connect cluster) is the configuration file used for the > > > > process. > > > > > > By > > > > > > >> > > > bringing up the REST API, we'd expose endpoints to > modify > > > > > > connector > > > > > > >> > > > configurations, which would not only add complexity to > the > > > > > > operation > > > > > > >> > of a > > > > > > >> > > > MM2 cluster, but even qualify as an attack vector for > > > > malicious > > > > > > >> > entities. > > > > > > >> > > > Thanks to KIP-507 we have some amount of security around > > the > > > > > > >> > > internal-only > > > > > > >> > > > endpoints used by the Connect framework, but for any > > public > > > > > > >> endpoints, > > > > > > >> > > the > > > > > > >> > > > Connect REST API doesn't come with any security out of > the > > > > box. > > > > > > >> > > > > > > > > > >> > > > 3. Small point, but with support for exactly-once source > > > > > > connectors > > > > > > >> > > coming > > > > > > >> > > > out in 3.3.0, it's also worth noting that that's another > > > > feature > > > > > > >> that > > > > > > >> > > won't > > > > > > >> > > > work properly with multi-node MM2 clusters without > adding > > a > > > > REST > > > > > > >> server > > > > > > >> > > for > > > > > > >> > > > each node (or some substitute that accomplishes the same > > > > goal). > > > > > I > > > > > > >> don't > > > > > > >> > > > think this will affect the direction of the design > > > discussion > > > > > too > > > > > > >> much, > > > > > > >> > > but > > > > > > >> > > > it does help strengthen the motivation. > > > > > > >> > > > > > > > > > >> > > > Cheers, > > > > > > >> > > > > > > > > > >> > > > Chris > > > > > > >> > > > > > > > > > >> > > > On 2021/02/18 15:57:36 Dániel Urbán wrote: > > > > > > >> > > > > Hello everyone, > > > > > > >> > > > > > > > > > > >> > > > > * Sorry, I meant KIP-710. > > > > > > >> > > > > > > > > > > >> > > > > Right now the MirrorMaker cluster is somewhat > > unreliable, > > > > and > > > > > > not > > > > > > >> > > > > supporting running in a cluster properly. I'd say that > > > > fixing > > > > > > this > > > > > > >> > > would > > > > > > >> > > > be > > > > > > >> > > > > a nice addition. > > > > > > >> > > > > Does anyone have some input on this? > > > > > > >> > > > > > > > > > > >> > > > > Thanks in advance > > > > > > >> > > > > Daniel > > > > > > >> > > > > > > > > > > >> > > > > Dániel Urbán <ur...@gmail.com> ezt írta (időpont: > 2021. > > > > jan. > > > > > > >> 26., K, > > > > > > >> > > > > 15:56): > > > > > > >> > > > > > > > > > > >> > > > > > Hello everyone, > > > > > > >> > > > > > > > > > > > >> > > > > > I would like to start a discussion on KIP-709, which > > > > > addresses > > > > > > >> some > > > > > > >> > > > > > missing features in MM2 dedicated mode. > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-710%3A+Full+support+for+distributed+mode+in+dedicated+MirrorMaker+2.0+clusters > > > > > > >> > > > > > > > > > > > >> > > > > > Currently, the dedicated mode of MM2 does not fully > > > > support > > > > > > >> running > > > > > > >> > > in > > > > > > >> > > > a > > > > > > >> > > > > > cluster. The core issue is that the Connect REST > > Server > > > is > > > > > not > > > > > > >> > > included > > > > > > >> > > > in > > > > > > >> > > > > > the dedicated mode, which makes follower->leader > > > > > communication > > > > > > >> > > > impossible. > > > > > > >> > > > > > In some cases, this results in the cluster not being > > > able > > > > to > > > > > > >> react > > > > > > >> > to > > > > > > >> > > > > > dynamic configuration changes (e.g. dynamic topic > > filter > > > > > > >> changes). > > > > > > >> > > > > > Another smaller detail is that MM2 dedicated mode > > > eagerly > > > > > > >> resolves > > > > > > >> > > > config > > > > > > >> > > > > > provider references in the Connector configurations, > > > which > > > > > is > > > > > > >> > > > undesirable > > > > > > >> > > > > > and a breaking change compared to vanilla Connect. > > This > > > > can > > > > > > >> cause > > > > > > >> > an > > > > > > >> > > > issue > > > > > > >> > > > > > for example when there is an environment variable > > > > reference, > > > > > > >> which > > > > > > >> > > > contains > > > > > > >> > > > > > some host-specific information, like a file path. > The > > > > leader > > > > > > >> > resolves > > > > > > >> > > > the > > > > > > >> > > > > > reference eagerly, and the resolved value is > > propagated > > > to > > > > > > other > > > > > > >> > MM2 > > > > > > >> > > > nodes > > > > > > >> > > > > > instead of the reference being resolved locally, > > > > separately > > > > > on > > > > > > >> each > > > > > > >> > > > node. > > > > > > >> > > > > > > > > > > > >> > > > > > The KIP addresses these by adding the Connect REST > > > Server > > > > to > > > > > > the > > > > > > >> > MM2 > > > > > > >> > > > > > dedicated mode for each replication flow, and > > postponing > > > > the > > > > > > >> config > > > > > > >> > > > > > provider reference resolution. > > > > > > >> > > > > > > > > > > > >> > > > > > Please discuss, I know this is a major change, but > > also > > > an > > > > > > >> > important > > > > > > >> > > > > > feature for MM2 users. > > > > > > >> > > > > > > > > > > > >> > > > > > Daniel > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > >
