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