Hi Randall, zero length definitely works, that's what sent me down this hole in the first place. I had a customer accidentally create a connector without a name in his environment and then be unable to delete it. No connector name doesn't work, as this throws a null pointer exception due to KAFKA-4938 , but once that is fixed would create a connector named "null" I think. Have not retested this, but seen it in the past.
Also, it is possible to create connectors with trailing and leading whitespaces, this errors out on the create request (which will be fixed when KAFKA-4827 is merged), but correctly creates the connector and you can access it if you percent-escape the curl call. This for me is the main reason why a KIP might be needed, as we are changing public facing behavior here. I agree with you, that this will probably not affect anyone or hardly anyone, but in principle it is a change that should need a KIP I think. I've retested and documented this for Confluent 3.3.0: https://gist.github.com/soenkeliebau/9363745cff23560fcc234d9b64ac14c4 I am of course happy to withdraw the KIP if you think it is unnecessary, I've also updated the pull request for KAFKA-4930 to reflect the changes stated in the KIP and tested the code with Arjuns pull request for KAFKA-4827 to ensure they don't interfere with each other. Let me know what you think. Kind regards, Sönke ᐧ On Tue, Nov 14, 2017 at 7:03 PM, Randall Hauch <rha...@gmail.com> wrote: > Thanks for updating the KIP to reflect the current process. However, I > still question whether it is necessary to have a KIP - it depends on > whether it was possible with prior versions to have connectors with > zero-length or blank names. Have you tried both of these cases? > > On Fri, Nov 10, 2017 at 3:52 AM, Sönke Liebau < > soenke.lie...@opencore.com.invalid> wrote: > > > Hi Randall, > > > > I have set aside some time to work on this next week. The fix itself is > > quite simple, but I've yet to write tests to properly catch this, which > > turns out to be a bit more complex, as it needs a running restserver > which > > is mocked in the tests I've looked at so far. > > > > Should I withdraw the KIP or update it to reflect the documentation > changes > > and enforced rules around trimming and zero length connector names? This > is > > a change to existing behavior, even if it is quite small and probably > won't > > even be noticed by many people.. > > > > best regards, > > Sönke > > > > On Thu, Nov 9, 2017 at 9:10 PM, Randall Hauch <rha...@gmail.com> wrote: > > > > > Any progress on updating the PR and withdrawing KIP-212? > > > > > > On Fri, Oct 27, 2017 at 5:19 PM, Randall Hauch <rha...@gmail.com> > wrote: > > > > > > > Yes, connector names should not be blank or contain just whitespace. > In > > > > fact, I might recommend that we trim whitespace at the front and rear > > of > > > > new connector names and then disallowing any zero-length name. > Existing > > > > connectors would remain valid, and this would not break backward > > > > compatibility. That might require a small kip simply to update the > > > > documentation and specify what names are valid. > > > > > > > > WDYT? > > > > > > > > Randall > > > > > > > > On Fri, Oct 27, 2017 at 1:08 PM, Colin McCabe <cmcc...@apache.org> > > > wrote: > > > > > > > >> On Wed, Oct 25, 2017, at 01:07, Sönke Liebau wrote: > > > >> > I've spent some time looking at this and testing various > characters > > > and > > > >> > it > > > >> > would appear that Randall's suspicion was spot on. I think we can > > > >> support > > > >> > a > > > >> > fairly large set of characters with very minor changes. > > > >> > > > > >> > I was put of by the exceptions that were thrown when creating > > > connectors > > > >> > with certain characters and suspected a larger underlying problem > > when > > > >> in > > > >> > fact the only issue is, that the URL in the rest request used to > > > >> retrieve > > > >> > the response for the create connector request needs to be percent > > > >> encoded > > > >> > [1]. > > > >> > > > > >> > I've fixed this and done some local testing which worked out quite > > > >> > nicely, > > > >> > apart from two special cases, I've not been able to find > characters > > > that > > > >> > created issues, even space and slash work. > > > >> > The mentioned special cases are: > > > >> > \ - if the name contains a backslash that is not the beginning > > of a > > > >> > valid escape sequence the request fails before we ever get it in > > > >> > ConnectorsResource, so a backslash would need to be escaped: \\ > > > >> > " - Quotation marks need to be escaped as well to keep the json > > > body > > > >> > of > > > >> > the request legal: \" > > > >> > In both cases the escape character will be part of the connector > > name > > > >> and > > > >> > need to be specified in the url to retrieve the connector as well, > > > even > > > >> > though we could URL encode it in a legal way without escaping > here. > > So > > > >> > they > > > >> > work, not sure if I'd recommend using those characters, but no > real > > > >> > reason > > > >> > to prohibit people from using them that I can see either. > > > >> > > > >> Good research, Sönke. > > > >> > > > >> > > > > >> > > > > >> > What I'd do going forward is: > > > >> > - withdraw the KIP, as I don't see a real need for one, since this > > is > > > >> not > > > >> > changing anything, just fixing things. > > > >> > - add a section to the documentation around legal characters, > > specify > > > >> the > > > >> > ones I tested explicitly (url encoded %20 - %7F) and mention that > > most > > > >> > other characters should work as well but no guarantees are given > > > >> > - update the pull request for KAFKA-4930 to allow all characters > but > > > >> > still > > > >> > prohibit creating a connector with an empty name. I'd propose to > > keep > > > >> the > > > >> > validator though as it'll give us a central location to do any > > > checking > > > >> > that might turn out to be necessary later on. > > > >> > > > >> Are empty names currently allowed? That's unfortunate. > > > >> > > > >> > - add some integration tests to check connectors with special > > > characters > > > >> > in > > > >> > their names work > > > >> > - fix the url encoding line in ConnectorsResource > > > >> > > > > >> > Does that sound fair to everybody? > > > >> > > > >> It sounds good to me, but I will let someone more knowledgeable > about > > > >> connect chime in. > > > >> > > > >> best, > > > >> Colin > > > >> > > > >> > > > > >> > Kind regards, > > > >> > Sönke > > > >> > > > > >> > [1] > > > >> > https://github.com/apache/kafka/blob/trunk/connect/runtime/ > > > >> src/main/java/org/apache/kafka/connect/runtime/rest/ > > > >> resources/ConnectorsResource.java#L102 > > > >> > > > > >> > On Tue, Oct 24, 2017 at 8:40 PM, Colin McCabe <cmcc...@apache.org > > > > > >> wrote: > > > >> > > > > >> > > On Tue, Oct 24, 2017, at 11:28, Sönke Liebau wrote: > > > >> > > > Hi, > > > >> > > > > > > >> > > > after reading your messages I'll grant that I might have > picked > > a > > > >> > > > somewhat > > > >> > > > draconic option to solve these issues. > > > >> > > > > > > >> > > > In general I believe that properly encoding the URLs after > > having > > > >> created > > > >> > > > the connectors should solve a lot of the issues already. For > > some > > > >> > > > characters the rest api returns an error on creating the > > connector > > > >> as > > > >> > > > well, > > > >> > > > so for that URL encoding won't help. However the connectors do > > get > > > >> > > > created > > > >> > > > even though an error is returned, I've never investigated if > > they > > > >> are in > > > >> > > > a > > > >> > > > consistent state tbh - I'll give this another look. > > > >> > > > > > > >> > > > @colin: Entity encoding would allow us to encode a lot of > > > >> characters, > > > >> > > > however I am unsure whether we should prefer it over url > > encoding > > > >> in this > > > >> > > > case, as mostly the end user would have to encode the > characters > > > >> himself. > > > >> > > > And due to entity encoding ending every character with a ; > which > > > >> causes > > > >> > > > the > > > >> > > > embedded jetty server to cut the connector name at that > > character > > > >> we'd > > > >> > > > probably need to encode that character in URL encoding again > for > > > >> that to > > > >> > > > work out - which might get a bit too complex tbh. > > > >> > > > > > >> > > Sorry, I meant to write percent-encoding, not entity refs. > > > >> > > https://en.wikipedia.org/wiki/Percent-encoding > > > >> > > > > > >> > > best, > > > >> > > Colin > > > >> > > > > > >> > > > > > >> > > > I will further investigate which characters the url decoding > > that > > > >> jetty > > > >> > > > brings to the table will let us use and if all of these are > > > >> correctly > > > >> > > > handled during connector creation and report back with a new > > list > > > of > > > >> > > > characters that I think we can support fairly easily. > > > >> > > > > > > >> > > > Kind regards, > > > >> > > > Sönke > > > >> > > > > > > >> > > > > > > >> > > > On Tue, Oct 24, 2017 at 6:42 PM, Colin McCabe < > > cmcc...@apache.org > > > > > > > >> > > wrote: > > > >> > > > > > > >> > > > > It should be possible to use entity references to encode > these > > > >> > > > > characters in URLs. See https://dev.w3.org/html5/html- > > > >> author/charref > > > >> > > > > Maybe I'm misunderstanding the problem, but can we simply > > encode > > > >> the > > > >> > > > > URLs, rather than restricting the names? > > > >> > > > > > > > >> > > > > best, > > > >> > > > > Colin > > > >> > > > > > > > >> > > > > > > > >> > > > > On Mon, Oct 23, 2017, at 14:12, Randall Hauch wrote: > > > >> > > > > > Here's the link to KIP-212: > > > >> > > > > > https://cwiki.apache.org/confluence/pages/viewpage. > > > >> > > > > action?pageId=74684586 > > > >> > > > > > > > > >> > > > > > I do think it's worthwhile to define the rules for > connector > > > >> names. > > > >> > > > > > However, I think it would be better to describe the > current > > > >> > > restrictions > > > >> > > > > > for names outside of them appearing within URLs. For > > example, > > > >> if we > > > >> > > can > > > >> > > > > > keep connector names relatively free of constraints but > > > instead > > > >> > > define > > > >> > > > > > how > > > >> > > > > > names should be encoded when used within URLs (e.g., URL > > > >> encoding), > > > >> > > then > > > >> > > > > > we > > > >> > > > > > may not have (m)any backward compatibility issues other > than > > > >> fixing > > > >> > > some > > > >> > > > > > bugs related to proper encoding/decoding. > > > >> > > > > > > > > >> > > > > > Thoughts? > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > On Mon, Oct 23, 2017 at 3:44 PM, Sönke Liebau < > > > >> > > > > > soenke.lie...@opencore.com.invalid> wrote: > > > >> > > > > > > > > >> > > > > > > All, > > > >> > > > > > > > > > >> > > > > > > I've created a KIP to discuss enforcing of rules on what > > > >> > > characters are > > > >> > > > > > > allowed in connector names. > > > >> > > > > > > > > > >> > > > > > > Since this may break api calls that are currently > working > > I > > > >> > > figured a > > > >> > > > > KIP > > > >> > > > > > > is the better way to go than to just create a jira. > > > >> > > > > > > > > > >> > > > > > > I'd love to hear your input on this! > > > >> > > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > > >> > > > > > > >> > > > -- > > > >> > > > Sönke Liebau > > > >> > > > Partner > > > >> > > > Tel. +49 179 7940878 > > > >> > > > OpenCore GmbH & Co. KG - Thomas-Mann-Straße 8 - 22880 Wedel - > > > >> Germany > > > >> > > > > > >> > > > > >> > > > > >> > > > > >> > -- > > > >> > Sönke Liebau > > > >> > Partner > > > >> > Tel. +49 179 7940878 > > > >> > OpenCore GmbH & Co. KG - Thomas-Mann-Straße 8 - 22880 Wedel - > > Germany > > > >> > > > > > > > > > > > > > > > > > > > -- > > Sönke Liebau > > Partner > > Tel. +49 179 7940878 > > OpenCore GmbH & Co. KG - Thomas-Mann-Straße 8 - 22880 Wedel - Germany > > > -- Sönke Liebau Partner Tel. +49 179 7940878 OpenCore GmbH & Co. KG - Thomas-Mann-Straße 8 - 22880 Wedel - Germany
