Thanks for the KIP, Mickael. And thanks to everyone else for the good discussion so far.
Overall this is pretty good, but I do have a few questions/comments: 1. Can we include the HTTP method/verb in each of the endpoints in the "Public Interfaces" section? 2. The sample output for `GET /connector-plugins?connectorsOnly=false` includes a `version` field for the connector plugin, but not for the transform or converter plugins. Neither the `Transformation` or `Converter` interfaces have a version nor implement the `Versioned` interface, so should we add the `Versioned` interface to the `Transformation` and `Converter` interfaces and add a default method that returns "unknown"? Yes, we could do that in a separate KIP, but I think it's appropriate to do it in this KIP for a few reasons. First, we're already modifying the `Converter` interface in this KIP and it would be quite straightforward to also extend the `Versioned` interface and add a default method; then doing that for `Transformation` would also be relatively straightforward. Second, this KIP provides the justification for adding a version to the `Transformation` and `Converter` interfaces. 3. In the "Public Interfaces" section, there are two inconsistencies between the `GET /connector-plugins` and `GET /connector-plugins/<plugin>/config` methods. Both will make it difficult to use the second method, since the input to the second method is not directly found in the response from the first method: a. The example for `GET /connector-plugins/Cast$Value/config` uses short name, or what Connect often refers to as the alias, "Cast$Value" for the plugin, rather than the fully-qualified name "org.apache.kafka.connect.transforms.Cast$Value". The KIP should specify whether it will support both the shortened name and the fully-qualified name, or whether only one of these will be supported. Not supporting the fully-qualified name will make it impossible to use the response from the first method as an input to the second. a. The example for the list method returns the `Transformation` base and abstract classes (e.g., "org.apache.kafka.connect.transforms.Cast") rather than the concrete classes (e.g., "org.apache.kafka.connect.transforms.Cast$Value" and "org.apache.kafka.connect.transforms.Cast$Key"), while the `GET /connector-plugins/<plugin>/config` will require the concrete class. 4. The "Public Interfaces" section's description of the `GET /connector-plugins/<plugin>/config` method should probably specify the expected behavior for `Converter` implementations that don't override the new `config()` method. 5. The "Compatibility, Deprecation, and Migration Plan" section should describe whether older `Converter` implementations (compiled with older versions of the Connect API without Converter extending Configurable) will be able to be installed into newer Connect runtimes, and what the behavior will be for the `GET /connector-plugins/<plugin>/config` method. (Although my suggestion for #5 will partially address the latter, we still need to talk about it in terms of compatibility.) 6. Can we add a small section addressing the security implications of these new APIs? KIP-507 [1] applied to APIs that existed at the time, and this KIP should at least mention how it aligns with that mechanism. 7. Do we really think adding `GET /worker-plugins` is necessary? While I understand the reasoning, I'm less convinced that it's worth adding to the public API. First, an administrator still has to modify the worker config for all workers to take advantage of that. Second, adding this method increases the surface area and therefore risk of additional attack vectors, despite not really being able to use the resulting information in any other part of the API. Best regards, Randall [1] https://cwiki.apache.org/confluence/display/KAFKA/KIP-507%3A+Securing+Internal+Connect+REST+Endpoints On Wed, Dec 1, 2021 at 7:52 AM Mickael Maison <mickael.mai...@gmail.com> wrote: > > Hi Chris, > > Thanks again for the feedback! I've updated the KIP based on our last > discussions. I've decided to include the new endpoint for worker > plugins. > > 1. Yes I agree, it's best to gate the new behavior. > 2. Yes, it was a remnant from the original proposal. I've now removed > the location field. > > Thanks, > Mickael > > On Tue, Nov 30, 2021 at 3:22 AM Chris Egerton > <chr...@confluent.io.invalid> wrote: > > > > Hi Mickael, > > > > I think that's a great idea! I especially like how we can establish the > > expectation that any plugin type that appears in the response from the GET > > /connector-plugins endpoint will have a corresponding GET > > /connector-plugins/<type>/config endpoint, but (if we decide to add them in > > the future), worker plugins won't be expected to expose this kind of > > information and the different root path helps give a decent hint about this. > > > > I also like the choice to return an empty ConfigDef from Converter::config > > instead of null. > > > > Two things come to mind: > > > > 1. We may want to gate this behind a URL query parameter (maybe something > > like "connectorsOnly") that defaults to the old behavior in order to avoid > > breaking existing tools such as programmatic UIs that use the endpoint > > today to discover the connectors that can be created by the user. We can > > even plan to change the default for that parameter to the newly-proposed > > behavior in the next major release, which should give people enough time to > > either adapt to the expanded response format or add the query parameter to > > their tooling. > > > > 2. The existing GET /connector-plugins endpoint doesn't contain information > > on the location of the plugin on the worker's file system. Do you think we > > should still include this info in the new response format? Correct me if > > I'm wrong but it seems it may have been proposed originally to help prevent > > already-addressed bugs in Connect classloading from striking; all else > > equal, I'd personally err on the side of leaving this info out or at least > > reducing permitted values for it to just "classpath" or "plugin path" in > > order to avoid leaking worker file paths into the REST API, which might > > bother super security-conscious users. > > > > Cheers, > > > > Chris > > > > On Mon, Nov 29, 2021 at 5:52 AM Mickael Maison <mickael.mai...@gmail.com> > > wrote: > > > > > Hi Chris, > > > > > > Yes to keep compatibility we want a default implementation for > > > Converter.configs(), I've updated the KIP. > > > > > > Regarding worker plugins, the use case you described seems valuable. > > > I'd prefer not mixing worker and connector plugins on the same > > > endpoint but I agree using /plugins and /worker-plugins could be > > > confusing. > > > > > > One alternative is to expose all connector-level plugins via the > > > existing /connector-plugins endpoint. In that case, we'd need to keep > > > the current JSON schema and not group plugins by type. As the current > > > schema already has a type field for each entry, we'll still be able to > > > tell them apart. Then we can have /worker-plugins and a relatively > > > clean API. What do you think? > > > > > > Thanks, > > > Mickael > > > > > > On Sun, Nov 28, 2021 at 8:21 PM Chris Egerton > > > <chr...@confluent.io.invalid> wrote: > > > > > > > > Hi Mickael, > > > > > > > > I think one potential use case for exposing worker-level plugins is that > > > it > > > > may make it easier to determine whether a worker is set up correctly > > > > (the > > > > current alternative requires looking through log files and can be a > > > little > > > > tedious), and might even make it possible to automatically identify > > > > discrepancies within a cluster by diffing the contents of that endpoint > > > > across each worker. But I don't think this has to be addressed by the > > > > current KIP; the only thing that bothers me a little is that "plugins" > > > > is > > > > generic and it may confuse people down the road if we add an endpoint > > > > for > > > > worker-level plugins ("why is one just called 'plugins' and the other > > > > one > > > > is 'worker-plugins'?"). Probably not worth blocking on, though. > > > > > > > > Agreed that the suggestion for improved validation should be made on the > > > > KIP-802 thread. > > > > > > > > I also noticed that the newly-proposed config method for the Converter > > > > interface doesn't have a default implementation, making it > > > > backwards-incompatible. Should we add a default implementation that > > > returns > > > > either null or an empty ConfigDef? > > > > > > > > Cheers, > > > > > > > > Chris > > > > > > > > On Fri, Nov 26, 2021 at 8:35 AM Mickael Maison <mickael.mai...@gmail.com > > > > > > > > wrote: > > > > > > > > > Hi Chris, > > > > > > > > > > 1. If we want to expose worker plugins, I think we should do it via a > > > > > separate endpoint. But to be honest, I'm not even sure I see strong > > > > > use cases for exposing them as they are either enabled or not and > > > > > can't be changed at runtime. So I'd prefer to stick to "connector > > > > > level" plugins in this KIP. Let me now if you have use cases, I'm open > > > > > to reconsider this choice. > > > > > I'll add that in the rejected alternatives section for now > > > > > > > > > > 2. I remembered seeing issues in the past with multiple plugin.path > > > > > entries but I tried today and I was able to mix and match plugins from > > > > > different paths. So my bad for getting confused. > > > > > Then I agree, it makes more sense to group them by plugin type. > > > > > > > > > > 3. Yes this should be covered in KIP-802: > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-802%3A+Validation+Support+for+Kafka+Connect+SMT+Options > > > > > > > > > > 4. No particular reason. We can support both formats like today. I've > > > > > updated the KIP > > > > > > > > > > Thanks, > > > > > Mickael > > > > > > > > > > > > > > > > > > > > On Tue, Nov 23, 2021 at 6:40 PM Chris Egerton > > > > > <chr...@confluent.io.invalid> wrote: > > > > > > > > > > > > Hi Mickael, > > > > > > > > > > > > I think the increase in scope here is great and the added value > > > certainly > > > > > > justifies the proposed changes. I have some thoughts but overall I > > > like > > > > > the > > > > > > direction this is going in now. > > > > > > > > > > > > 1. The new /plugins endpoint is described as containing "all plugins > > > that > > > > > > are Connectors, Transformations, Converters, HeaderConverters and > > > > > > Predicates". So essentially, it looks like we want to expose all > > > plugins > > > > > > that are configured on a per-connector basis, but exclude plugins > > > that > > > > > are > > > > > > configured on a per-worker basis (such as config providers and REST > > > > > > extensions). Do you think it may be valuable to expose information > > > > > > on > > > > > > worker-level plugins as well? > > > > > > > > > > > > 2. The description for the new /plugins endpoint also states that > > > > > "Plugins > > > > > > will be grouped by plugin.path. This will make it clear to users > > > what's > > > > > > available to use as it's not possible to use a Connector from one > > > path > > > > > with > > > > > > Transformations from another.". Is this true? I thought that > > > Connect's > > > > > > classloading made it possible to package > > > > > > converters/transformations/predicates completely independently from > > > each > > > > > > other, and to reference them from also-independently-packaged > > > connectors. > > > > > > If it turns out that this is the case, could we consider > > > restructuring > > > > > the > > > > > > response to be grouped by plugin type instead of by classloader? > > > There's > > > > > > also the ungrouped format proposed in KIP-494 ( > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=120740150 > > > > > ) > > > > > > which we might consider as well. > > > > > > > > > > > > 3. I think this can be left for a follow-up KIP if necessary, but > > > > > > I'm > > > > > > curious about your thoughts on adding new validate methods to all > > > > > > connector-level plugins that can be used similarly to how the > > > existing > > > > > > Connector::validate method ( > > > > > > > > > > > > > > https://github.com/apache/kafka/blob/1e0916580f16b99b911b0ed36e9740dcaeef520e/connect/api/src/main/java/org/apache/kafka/connect/connector/Connector.java#L131-L146 > > > > > ) > > > > > > is used. This would allow for plugins to perform validation that's > > > more > > > > > > sophisticated than what the ConfigDef is capable of, such as > > > validating > > > > > > combinations of properties like a hostname and credentials for > > > reaching > > > > > it. > > > > > > I know that at least Confluent's Avro, protobuf, and JSON schema > > > > > converters > > > > > > would benefit from this kind of feature. It's a little tangential to > > > this > > > > > > KIP (which at the moment is about discovering plugins and their > > > > > > configuration surfaces, as opposed to validating them), but I > > > figured I'd > > > > > > ask since we're going to be expanding the Converter interface and it > > > may > > > > > be > > > > > > useful to tackle this while we're in the neighborhood. > > > > > > > > > > > > 4. The description for the new /plugins/<type>/<name>/configdef > > > endpoint > > > > > > states that "Name must be the fully qualified class name of the > > > plugin". > > > > > > Any reason not to also support aliases (e.g., > > > "FileStreamSinkConnector" > > > > > or > > > > > > "FileStreamSink" instead of > > > > > > "org.apache.kafka.connect.file.FileStreamSinkConnector")? > > > > > > > > > > > > Cheers, > > > > > > > > > > > > Chris > > > > > > > > > > > > On Tue, Nov 23, 2021 at 12:07 PM Mickael Maison < > > > > > mickael.mai...@gmail.com> > > > > > > wrote: > > > > > > > > > > > > > Thanks all for the feedback! > > > > > > > > > > > > > > Chris, > > > > > > > I agree that fixing the current endpoint helps a lot. Thanks for > > > > > > > raising these JIRAs and submitting a PR! > > > > > > > However thinking about the issue further, I decided to expand the > > > > > > > scope of the KIP to cover all user-visible plugins. > > > > > > > In practice, users want to know about all available plugins not > > > only > > > > > > > connectors. This includes transformations, converters, > > > > > > > header_converters and predicates. As we also want to retrieve > > > > > > > configdef for these too, I think it makes sense to introduce a new > > > > > > > endpoint to do so. Alongside we obviously need a new endpoint for > > > > > > > listing all plugins. > > > > > > > > > > > > > > Gunnar, > > > > > > > I took a look at exposing valid values via the API. I think the > > > issue > > > > > > > is that Validators don't expose a way to retrieve valid values. > > > > > > > Changing validators will have an impact on all components so I'd > > > > > > > prefer to address this requirement in a separate KIP. I agree this > > > > > > > would be an interesting improvement and I'd happy to write a KIP > > > for > > > > > > > it too. > > > > > > > > > > > > > > > > > > > > > I have updated the KIP accordingly. Let me know if you have > > > > > > > further > > > > > > > feedback. > > > > > > > > > > > > > > Thanks, > > > > > > > Mickael > > > > > > > > > > > > > > On Tue, Nov 16, 2021 at 9:31 PM Gunnar Morling > > > > > > > <gunnar.morl...@googlemail.com.invalid> wrote: > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > I'm +1 for adding a GET endpoint for obtaining config > > > definitions. It > > > > > > > > always felt odd to me that one has to issue a PUT for that > > > purpose. > > > > > If > > > > > > > > nothing else, it'd be better in terms of discoverability of the > > > KC > > > > > REST > > > > > > > API. > > > > > > > > > > > > > > > > One additional feature request I'd have is to expose the valid > > > enum > > > > > > > > constants for enum-typed options. That'll help to display the > > > values > > > > > in a > > > > > > > > drop-down or via radio buttons in a UI, give us tab completion > > > > > > > > in > > > > > kcctl, > > > > > > > > etc. > > > > > > > > > > > > > > > > Best, > > > > > > > > > > > > > > > > --Gunnar > > > > > > > > > > > > > > > > > > > > > > > > Am Di., 16. Nov. 2021 um 16:31 Uhr schrieb Chris Egerton > > > > > > > > <chr...@confluent.io.invalid>: > > > > > > > > > > > > > > > > > Hi Viktor, > > > > > > > > > > > > > > > > > > It sounds like there are three major points here in favor of a > > > new > > > > > GET > > > > > > > > > endpoint for connector config defs. > > > > > > > > > > > > > > > > > > 1. You cannot issue a blank ("dummy") request for sink > > > connectors > > > > > > > because a > > > > > > > > > topic list/topic regex has to be supplied (otherwise the PUT > > > > > endpoint > > > > > > > > > returns a 500 response) > > > > > > > > > 2. A dummy request still triggers custom validations by the > > > > > connector, > > > > > > > > > which may be best to avoid if we know for sure that the config > > > > > isn't > > > > > > > worth > > > > > > > > > validating yet > > > > > > > > > 3. It's more ergonomic and intuitive to be able to issue a GET > > > > > request > > > > > > > > > without having to give a dummy connector config > > > > > > > > > > > > > > > > > > With regards to 1, this is actually a bug in Connect ( > > > > > > > > > https://issues.apache.org/jira/browse/KAFKA-13327) with a fix > > > > > already > > > > > > > > > implemented and awaiting committer review ( > > > > > > > > > https://github.com/apache/kafka/pull/11369). I think it'd be > > > > > better to > > > > > > > > > focus on fixing this bug in general instead of implementing a > > > new > > > > > REST > > > > > > > > > endpoint in order to allow people to work around it. > > > > > > > > > > > > > > > > > > With regards to 2, this is technically possible but I'm unsure > > > > > it'd be > > > > > > > too > > > > > > > > > common out in the wild given that most validations that could > > > be > > > > > > > expensive > > > > > > > > > would involve things like connecting to a database, checking > > > if a > > > > > cloud > > > > > > > > > storage bucket exists, etc., none of which are possible > > > > > > > > > without > > > > > some > > > > > > > > > configuration properties from the user (db hostname, bucket > > > name, > > > > > > > etc.). > > > > > > > > > > > > > > > > > > With regards to 3, I do agree that it'd be easier for people > > > > > designing > > > > > > > UIs > > > > > > > > > to have a GET API to work against. I'm just not sure it's > > > worth the > > > > > > > > > additional implementation, testing, and maintenance burden. If > > > it > > > > > were > > > > > > > > > possible to issue a PUT request without unexpected 500s for > > > invalid > > > > > > > > > configs, would that suffice? AFAICT it'd basically be as > > > simple as > > > > > > > issuing > > > > > > > > > a PUT request with a dummy body consisting of nothing except > > > the > > > > > > > connector > > > > > > > > > class (which at this point we might even make unnecessary and > > > just > > > > > > > > > automatically replace with the connector class from the URL) > > > and > > > > > then > > > > > > > > > filtering the response to just grab the "definition" field of > > > each > > > > > > > element > > > > > > > > > in the "configs" array in the response. > > > > > > > > > > > > > > > > > > Cheers, > > > > > > > > > > > > > > > > > > Chris > > > > > > > > > > > > > > > > > > On Tue, Nov 16, 2021 at 9:52 AM Viktor Somogyi-Vass < > > > > > > > > > viktorsomo...@gmail.com> > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > Hi Folks, > > > > > > > > > > > > > > > > > > > > I too think this would be a very useful feature. Some of our > > > > > > > management > > > > > > > > > > applications would provide a wizard for creating connectors. > > > In > > > > > this > > > > > > > > > > scenario the user basically would fill out a sample > > > configuration > > > > > > > > > generated > > > > > > > > > > by the UI which would send it back to Connect for validation > > > and > > > > > > > > > eventually > > > > > > > > > > create a new connector. The first part of this workflow can > > > be > > > > > > > enhanced > > > > > > > > > if > > > > > > > > > > we had an API that can return the configuration definition > > > of the > > > > > > > given > > > > > > > > > > type of connector as the UI application would be able to > > > > > generate a > > > > > > > > > sample > > > > > > > > > > for the user based on that (nicely drawn diagram: > > > > > > > > > > https://imgur.com/a/7S1Xwm5). > > > > > > > > > > The connector-plugins/{connectorType}/config/validate API > > > > > essentially > > > > > > > > > works > > > > > > > > > > and returns the data that we need, however it is a HTTP PUT > > > API > > > > > that > > > > > > > is a > > > > > > > > > > bit unintuitive for a fetch-like functionality and also > > > > > functionally > > > > > > > > > > different as it validates the given (dummy) request. In case > > > of > > > > > sink > > > > > > > > > > connectors one would need to also provide a topic name. > > > > > > > > > > > > > > > > > > > > A suggestion for the KIP: I think it can be useful to return > > > the > > > > > > > config > > > > > > > > > > groups and the connector class' name similarly to the > > > validate > > > > > API > > > > > > > just > > > > > > > > > in > > > > > > > > > > case any frontend needs them (and also the response would be > > > more > > > > > > > like > > > > > > > > > the > > > > > > > > > > validate API but simpler). > > > > > > > > > > > > > > > > > > > > Viktor > > > > > > > > > > > > > > > > > > > > On Fri, Aug 20, 2021 at 4:51 PM Ryanne Dolan < > > > > > ryannedo...@gmail.com> > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > I think it'd be worth adding a GET version, fwiw. Could be > > > the > > > > > same > > > > > > > > > > handler > > > > > > > > > > > with just a different spelling maybe. > > > > > > > > > > > > > > > > > > > > > > On Fri, Aug 20, 2021, 7:44 AM Mickael Maison < > > > > > > > mickael.mai...@gmail.com > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > Hi Chris, > > > > > > > > > > > > > > > > > > > > > > > > You're right, you can achieve the same functionality > > > using > > > > > the > > > > > > > > > > > > existing validate endpoint. > > > > > > > > > > > > In my mind it was only for validation once you have > > > build a > > > > > > > > > > > > configuration but when used with an empty configuration, > > > it > > > > > > > basically > > > > > > > > > > > > serves the same purpose as the proposed new endpoint. > > > > > > > > > > > > > > > > > > > > > > > > I think it's a bit easier to use a GET endpoint but I > > > don't > > > > > > > think it > > > > > > > > > > > > really warrants a different endpoint. > > > > > > > > > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Aug 19, 2021 at 2:56 PM Chris Egerton > > > > > > > > > > > > <chr...@confluent.io.invalid> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Mickael, > > > > > > > > > > > > > > > > > > > > > > > > > > I'm wondering about the use case here. The motivation > > > > > section > > > > > > > > > states > > > > > > > > > > > that > > > > > > > > > > > > > "Connect does not provide a way to see what > > > configurations > > > > > a > > > > > > > > > > connector > > > > > > > > > > > > > requires. Instead users have to go look at the > > > connector > > > > > > > > > > documentation > > > > > > > > > > > or > > > > > > > > > > > > > in the worst case, look directly at the connector > > > source > > > > > > > code.", > > > > > > > > > and > > > > > > > > > > > that > > > > > > > > > > > > > with this KIP, "users will be able to discover the > > > required > > > > > > > > > > > > configurations > > > > > > > > > > > > > for connectors installed in a Connect cluster" and > > > "tools > > > > > will > > > > > > > be > > > > > > > > > > able > > > > > > > > > > > to > > > > > > > > > > > > > generate wizards for configuring and starting > > > connectors". > > > > > > > > > > > > > > > > > > > > > > > > > > Does the existing "PUT > > > > > > > > > > > > /connector-plugins/{connector-type}/config/validate" > > > > > > > > > > > > > endpoint not address these points? What will the > > > > > newly-proposed > > > > > > > > > > > endpoint > > > > > > > > > > > > > allow users to do that they will not already be able > > > to do > > > > > > > with the > > > > > > > > > > > > > existing endpoint? > > > > > > > > > > > > > > > > > > > > > > > > > > Cheers, > > > > > > > > > > > > > > > > > > > > > > > > > > Chris > > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Aug 19, 2021 at 9:20 AM Mickael Maison < > > > > > > > > > > > mickael.mai...@gmail.com > > > > > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > > > > > > > > > > > I've created KIP-769 to expose connector > > > configuration > > > > > > > > > definitions > > > > > > > > > > in > > > > > > > > > > > > > > the Connect API > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-769%3A+Connect+API+to+retrieve+connector+configuration+definitions > > > > > > > > > > > > > > > > > > > > > > > > > > > > Please take a look and let me know if you have any > > > > > feedback. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
