kafka-jira mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Randall Hauch (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (KAFKA-4930) Connect Rest API allows creating connectors with an empty name
Date Thu, 09 Nov 2017 20:28:00 GMT

    [ https://issues.apache.org/jira/browse/KAFKA-4930?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16246460#comment-16246460
] 

Randall Hauch edited comment on KAFKA-4930 at 11/9/17 8:27 PM:
---------------------------------------------------------------

[~sliebau] created KIP-212 and then started the [KIP discussion on the AK dev mailing list|https://mail-archives.apache.org/mod_mbox/kafka-dev/201710.mbox/%3CCAMTc-3GXD3jYfknq3LHV726OFMRkbVyGjwnNAsOJL=UxaP0MNg@mail.gmail.com%3E].
After a few questions and time to research, Sönke reports the following:

{quote}
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 (see [ConnectorsResource.java|https://github.com/apache/kafka/blob/trunk/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java#L102]).

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.

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.
- 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?
{quote}

The important takeaway: Connect names should be URL-encoded in the REST request and will then
be properly handled by Connect. The only characters that need to be handled differently (before
URL-encoding the connector name) are the double quote character {{"}} and backslash character
{{\\}} that must be *_escaped_* with a preceding backslash character within the connector
name _before_ it is URL-encoded.


was (Author: rhauch):
[~sliebau] created KIP-212 and then started the [KIP discussion on the AK dev mailing list|https://mail-archives.apache.org/mod_mbox/kafka-dev/201710.mbox/%3CCAMTc-3GXD3jYfknq3LHV726OFMRkbVyGjwnNAsOJL=UxaP0MNg@mail.gmail.com%3E].
After a few questions and time to research, Sönke reports the following:

{quote}
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 (see [ConnectorsResource.java|https://github.com/apache/kafka/blob/trunk/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java#L102]).

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.

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.
- 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?
{quote}

The important takeaway: Connect names should be URL-encoded in the REST request and will then
be properly handled by Connect. The only characters that need to be handled differently (before
URL-encoding the connector name) are the double quote character {{"}} and backslash character
{{\\}} that must be *_escaped_* with a preceding backslash character within the connector
name _before_ it is URL-encoded.

> Connect Rest API allows creating connectors with an empty name
> --------------------------------------------------------------
>
>                 Key: KAFKA-4930
>                 URL: https://issues.apache.org/jira/browse/KAFKA-4930
>             Project: Kafka
>          Issue Type: Bug
>          Components: KafkaConnect
>    Affects Versions: 0.10.2.0
>            Reporter: Sönke Liebau
>            Assignee: Sönke Liebau
>            Priority: Minor
>
> The Connect Rest API allows to deploy connectors with an empty name field, which then
cannot be removed through the api.
> Sending the following request:
> {code}
> {
>     "name": "",
>     "config": {
>         "connector.class": "org.apache.kafka.connect.tools.MockSourceConnector",
>         "tasks.max": "1",
>         "topics": "test-topic"
>         
>     }
> }
> {code}
> Results in a connector being deployed which can be seen in the list of connectors:
> {code}
> [
> 	"",
> 	"testconnector"
> ]{code}
> But cannot be removed via a DELETE call, as the api thinks we are trying to delete the
/connectors endpoint and declines the request.
> I don't think there is a valid case for the connector name to be empty so perhaps we
should add a check for this. I am happy to work on this.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

Mime
View raw message