[jira] [Commented] (KAFKA-4930) Connect Rest API allows creating connectors with an empty name
[
https://issues.apache.org/jira/browse/KAFKA-4930?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15964886#comment-15964886
]
Sönke Liebau commented on KAFKA-4930:
-
[~gwenshap]: Did you have a chance yet to have a look at my PR? I'm happy to
add tests, but would like to wait for a +1 in principle to the approach chosen
before doing so :)
> 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
>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.3.15#6346)
[jira] [Commented] (KAFKA-4930) Connect Rest API allows creating connectors with an empty name
[
https://issues.apache.org/jira/browse/KAFKA-4930?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15945987#comment-15945987
]
Sönke Liebau commented on KAFKA-4930:
-
I've created a PR with my approach as it currently stands. I probably still
need to create and additional test case or two, for now I just fixed one broken
test (moved the check for / in connector name to a different part of the code
that was being mocked in that test).
If we go with this route we should probably discuss what characters we want to
prohibit in connector names, I did a bit of googling in the hope of finding a
rfc that states "these 5 characters are illegal in a rest url" - but that seems
to be a somewhat more [complicated
topic|http://stackoverflow.com/questions/2366260/whats-valid-and-whats-not-in-a-uri-query].
Also there is of course the risk of prohibiting something that someone out
there already used, so we should be very careful here I think.
> 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
>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.3.15#6346)
[jira] [Commented] (KAFKA-4930) Connect Rest API allows creating connectors with an empty name
[
https://issues.apache.org/jira/browse/KAFKA-4930?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15945974#comment-15945974
]
ASF GitHub Bot commented on KAFKA-4930:
---
GitHub user soenkeliebau opened a pull request:
https://github.com/apache/kafka/pull/2755
KAFKA-4930: Added connector name validator …
…to check for empty connector name and illegal characters in connector
name. This also fixes KAFKA-4938 by removing the check for slashes in
connector name from ConnectorsResource.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/soenkeliebau/kafka KAFKA-4930
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/kafka/pull/2755.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2755
commit 348d75dce006ddeb5beba0fc5ea5d761a4a87a82
Author: Soenke Liebau
Date: 2017-03-28T21:06:43Z
KAFKA-4930: Added connector name validator to check for empty connector
name and illegal characters in connector name. This also fixes KAFKA-4938 by
removing the check for slashes in connector name from ConnectorsResource.
> 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
>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.3.15#6346)
[jira] [Commented] (KAFKA-4930) Connect Rest API allows creating connectors with an empty name
[
https://issues.apache.org/jira/browse/KAFKA-4930?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15943621#comment-15943621
]
Gwen Shapira commented on KAFKA-4930:
-
[~sliebau] I like your validName approach. Can you create a PR? It is much
easier for me to review that way :)
> 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
>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.3.15#6346)
[jira] [Commented] (KAFKA-4930) Connect Rest API allows creating connectors with an empty name
[
https://issues.apache.org/jira/browse/KAFKA-4930?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15938458#comment-15938458
]
Sönke Liebau commented on KAFKA-4930:
-
After a good nights sleep I am beginning to think that between this and
[KAFKA-4938|https://issues.apache.org/jira/browse/KAFKA-4938] a better approach
might be to change the _nonEmpty_ Validator to a more specialised _ValidName_
validator, which could check for empty strings, backslashes and other
problematic characters (I think there are a few more jiras around).
The problematic check in
[KAFKA-4938|https://issues.apache.org/jira/browse/KAFKA-4938] could be replaced
by a check for null, if no name parameter is present in the request at all,
which we could consider a bad request I think, so the BadRequest Exception
makes sense here.
> 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
>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.3.15#6346)
[jira] [Commented] (KAFKA-4930) Connect Rest API allows creating connectors with an empty name
[
https://issues.apache.org/jira/browse/KAFKA-4930?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15937220#comment-15937220
]
Sönke Liebau commented on KAFKA-4930:
-
I've created a potential fix for this issue and pushed to
https://github.com/soenkeliebau/kafka/tree/KAFKA-4930
However I am unsure, whether this fully addresses all potential issues tbh. All
tests pass, but I won't pretend that I have fully understood all dependencies
in the validation model, so some feedback would be very welcome.
My fundamental worry with this implementation is, that I create a validator for
a option that cannot have a default value, which means that the validator won't
greenlight a default config, which I then get around by allowing a null value.
This seems not entirely clean.
An alternative approach would be to add checks
[here|https://github.com/apache/kafka/blob/trunk/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java#L91]
but I actually found that the existing check here throws a
NullPointerException for requests that don't have the _name_ parameter (will
create an issue for that), so there are caveats here as well.
Anyway, thoughts?
> 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
>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.3.15#6346)
[jira] [Commented] (KAFKA-4930) Connect Rest API allows creating connectors with an empty name
[
https://issues.apache.org/jira/browse/KAFKA-4930?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15936720#comment-15936720
]
Nacho Munoz commented on KAFKA-4930:
Actually, ignore my comment. I have just realised that validation is now part
of the creation process in the latest release :)
> 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
>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.3.15#6346)
[jira] [Commented] (KAFKA-4930) Connect Rest API allows creating connectors with an empty name
[
https://issues.apache.org/jira/browse/KAFKA-4930?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15936679#comment-15936679
]
Nacho Munoz commented on KAFKA-4930:
[~gwenshap] is there any reason why there's no validation when creating a
connector? I'm aware of the validating endpoint to test the connector
configuration but, would it make sense to validate the configuration as part of
the creation process?
> 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
>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.3.15#6346)
[jira] [Commented] (KAFKA-4930) Connect Rest API allows creating connectors with an empty name
[
https://issues.apache.org/jira/browse/KAFKA-4930?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15936374#comment-15936374
]
Gwen Shapira commented on KAFKA-4930:
-
Good catch! If you fix this, we'll gladly take the PR. Let me know if you need
help or pointers.
> 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
>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.3.15#6346)
