kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Sönke Liebau <soenke.lie...@opencore.com.INVALID>
Subject Re: [DISCUSS] KIP-212: Enforce set of legal characters for connector names
Date Thu, 16 Nov 2017 16:35:32 GMT
Sounds good. I've added a few sentences to this effect to the KIP.

On Thu, Nov 16, 2017 at 5:02 PM, Randall Hauch <rhauch@gmail.com> wrote:

> Nice job updating the KIP. The PR (
> https://github.com/apache/kafka/pull/2755/files) for the proposed
> implementation does prevent names from being empty, and it trims whitespace
> from the name only when creating a new connector. However, the KIP's
> "Proposed Change" section should probably be very clear about this, and the
> migration section should address how a connector that was created with
> leading and/or trailing whitespace characters will still be able to be
> updated and deleted. I think that decreases the likelihood of this change
> negatively impacting existing users. Basically, going forward, the names of
> new connectors will be trimmed.
>
> WDYT?
>
> On Thu, Nov 16, 2017 at 9:32 AM, Sönke Liebau <
> soenke.liebau@opencore.com.invalid> wrote:
>
> > I've added some more detail to the KIP [1] around current scenarios that
> > might break in the future. I actually came up with a second limitation
> that
> > we'd impose on users and also documented this.
> >
> > Let me know what you think.
> >
> > Kind regards,
> > Sönke
> >
> > [1]
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > 212%3A+Enforce+set+of+legal+characters+for+connector+names
> >
> >
> > On Thu, Nov 16, 2017 at 2:59 PM, Sönke Liebau <
> soenke.liebau@opencore.com>
> > wrote:
> >
> > > Hi Randall,
> > >
> > > I had mentioned this edge case in the KIP, but will add some further
> > > detail to further clarify all changing scenarios post pull request.
> > >
> > > Kind regards,
> > > Sönke
> > >
> > >
> > >
> > >
> > >
> > > On Thu, Nov 16, 2017 at 2:06 PM, Randall Hauch <rhauch@gmail.com>
> wrote:
> > >
> > >> 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.liebau@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/9363745cff23560fcc234d9b64ac14
> c4
> > >> >
> > >> > 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 <rhauch@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.liebau@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 <rhauch@gmail.com>
> > >> wrote:
> > >> >>>>
> > >> >>>> Any progress on updating the PR and withdrawing KIP-212?
> > >> >>>>
> > >> >>>> On Fri, Oct 27, 2017 at 5:19 PM, Randall Hauch <rhauch@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 <
> cmccabe@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
<
> > cmccabe@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 <
> > >> >>> cmccabe@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.liebau@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 <+49%20179%207940878>
> > >> >>> OpenCore GmbH & Co. KG - Thomas-Mann-Straße 8 - 22880
Wedel -
> > Germany
> > >> >>>
> > >> >>
> > >> >
> > >> >
> > >> >
> > >> > --
> > >> > Sönke Liebau
> > >> > Partner
> > >> > Tel. +49 179 7940878 <+49%20179%207940878>
> > >> > OpenCore GmbH & Co. KG - Thomas-Mann-Straße 8 - 22880 Wedel -
> Germany
> > >>
> > >
> > >
> > >
> > > --
> > > Sönke Liebau
> > > Partner
> > > Tel. +49 179 7940878 <+49%20179%207940878>
> > > 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

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message