No, we need to keep the KIP since we want to change/correct the existing behavior. But we do need to clarify in the KIP these edge cases that will change.
Thanks for the continued work on this, Sönke. Regards, Randall > On Nov 16, 2017, at 1:56 AM, Sönke Liebau > <soenke.lie...@opencore.com.INVALID> wrote: > > 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
