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 > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > > > > > > > >
