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