You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/09/17 11:29:58 UTC
[GitHub] [kafka] lhunyady opened a new pull request #11333: KAFKA-13306: Null connector config value passes validation, but fails creation
lhunyady opened a new pull request #11333:
URL: https://github.com/apache/kafka/pull/11333
This patch adds null value check to the connector config validation, and extends unit tests to cover this functionality.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] lhunyady commented on pull request #11333: KAFKA-13306: Null connector config value passes validation, but fails creation
Posted by GitBox <gi...@apache.org>.
lhunyady commented on pull request #11333:
URL: https://github.com/apache/kafka/pull/11333#issuecomment-929910705
Thanks @C0urante for the feedback, I agree with you and updated the PR accordingly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] lhunyady commented on pull request #11333: KAFKA-13306: Null connector config value passes validation, but fails creation
Posted by GitBox <gi...@apache.org>.
lhunyady commented on pull request #11333:
URL: https://github.com/apache/kafka/pull/11333#issuecomment-929910705
Thanks @C0urante for the feedback, I agree with you and updated the PR accordingly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] lhunyady commented on pull request #11333: KAFKA-13306: Null connector config value passes validation, but fails creation
Posted by GitBox <gi...@apache.org>.
lhunyady commented on pull request #11333:
URL: https://github.com/apache/kafka/pull/11333#issuecomment-1015284006
@C0urante what are the next steps to get this pull request merged?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante edited a comment on pull request #11333: KAFKA-13306: Null connector config value passes validation, but fails creation
Posted by GitBox <gi...@apache.org>.
C0urante edited a comment on pull request #11333:
URL: https://github.com/apache/kafka/pull/11333#issuecomment-924133203
Thanks @lhunyady. I wonder if instead of throwing an exception we can add an error message to the offending config properties? This would allow other configuration errors to be surfaced immediately in calls to `PUT /connector-plugins/{connectorClass}/config/validate` instead of requiring users to issue a follow-up request, and would return a more standard request type than the 500 that I believe would be returned in response to an uncaught exception.
If this approach is desirable, we could implement it with an additional step in [`AbstractHerder::validateConnectorConfig`](https://github.com/apache/kafka/blob/f650a14d56c0cc33263c29d8d242760406943c5b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/AbstractHerder.java#L418) that uses [`ConfigValue::addErrorMessage`](https://kafka.apache.org/28/javadoc/org/apache/kafka/common/config/ConfigValue.html#addErrorMessage(java.lang.String)) to report to the user that literal `null` values are not permitted in connector configurations, after adding a new `ConfigValue` for the offending property if one isn't already defined by the connector, the Connect framework, etc.
Thoughts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] lhunyady commented on pull request #11333: KAFKA-13306: Null connector config value passes validation, but fails creation
Posted by GitBox <gi...@apache.org>.
lhunyady commented on pull request #11333:
URL: https://github.com/apache/kafka/pull/11333#issuecomment-1004848659
Thank you @C0urante, I have updated the pull request with your valuable suggestions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante commented on pull request #11333: KAFKA-13306: Null connector config value passes validation, but fails creation
Posted by GitBox <gi...@apache.org>.
C0urante commented on pull request #11333:
URL: https://github.com/apache/kafka/pull/11333#issuecomment-1015468012
@tombentley @kkonstantine would either of you have time for this small but valuable fix?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison merged pull request #11333: KAFKA-13306: Null connector config value passes validation, but fails creation
Posted by GitBox <gi...@apache.org>.
mimaison merged pull request #11333:
URL: https://github.com/apache/kafka/pull/11333
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] abbccdda commented on a change in pull request #11333: KAFKA-13306: Null connector config value passes validation, but fails creation
Posted by GitBox <gi...@apache.org>.
abbccdda commented on a change in pull request #11333:
URL: https://github.com/apache/kafka/pull/11333#discussion_r711655376
##########
File path: connect/api/src/main/java/org/apache/kafka/connect/connector/Connector.java
##########
@@ -150,4 +152,15 @@ public Config validate(Map<String, String> connectorConfigs) {
* @return The ConfigDef for this connector; may not be null.
*/
public abstract ConfigDef config();
+
+ private void validateConfigDoesNotContainNull(Map<String, String> connectorConfigs) {
+ final String keysWithNullValue = connectorConfigs.entrySet().stream()
+ .filter(entry -> entry.getValue() == null)
+ .map(Map.Entry::getKey)
+ .collect(Collectors.joining(", "));
+
+ if (!keysWithNullValue.isEmpty()) {
+ throw new ConnectException(String.format("Null value found in config for key(s) %s", keysWithNullValue));
Review comment:
Should this just be IllegalArgumentException?
##########
File path: connect/runtime/src/test/java/org/apache/kafka/connect/runtime/AbstractHerderTest.java
##########
@@ -289,6 +290,23 @@ public void testBuildRestartPlanForUnknownConnector() {
assertFalse(mayBeRestartPlan.isPresent());
}
+ @Test
+ public void testConfigValidationNullConfig() {
Review comment:
Could you also add a test case with multiple null values as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] lhunyady commented on a change in pull request #11333: KAFKA-13306: Null connector config value passes validation, but fails creation
Posted by GitBox <gi...@apache.org>.
lhunyady commented on a change in pull request #11333:
URL: https://github.com/apache/kafka/pull/11333#discussion_r712103493
##########
File path: connect/api/src/main/java/org/apache/kafka/connect/connector/Connector.java
##########
@@ -150,4 +152,15 @@ public Config validate(Map<String, String> connectorConfigs) {
* @return The ConfigDef for this connector; may not be null.
*/
public abstract ConfigDef config();
+
+ private void validateConfigDoesNotContainNull(Map<String, String> connectorConfigs) {
+ final String keysWithNullValue = connectorConfigs.entrySet().stream()
+ .filter(entry -> entry.getValue() == null)
+ .map(Map.Entry::getKey)
+ .collect(Collectors.joining(", "));
+
+ if (!keysWithNullValue.isEmpty()) {
+ throw new ConnectException(String.format("Null value found in config for key(s) %s", keysWithNullValue));
Review comment:
Thank you for the comment, updated it in the latest commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante commented on pull request #11333: KAFKA-13306: Null connector config value passes validation, but fails creation
Posted by GitBox <gi...@apache.org>.
C0urante commented on pull request #11333:
URL: https://github.com/apache/kafka/pull/11333#issuecomment-924133203
Thanks @lhunyady. I wonder if instead of throwing an exception we can add an error message to the offending config properties? This would allow other configuration errors to be surfaced immediately in calls to `PUT /connector-plugins/{connectorClass}/config/validate` instead of requiring users to issue a follow-up request, and would return a more standard request type than the 500 that I believe would be returned in response to an uncaught exception.
If this approach is desirable, we could implement it with an additional step in [`AbstractHerder::validateConnectorConfig`](https://github.com/apache/kafka/blob/f650a14d56c0cc33263c29d8d242760406943c5b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/AbstractHerder.java#L418) that uses [`ConfigValue::addErrorMessage`](https://kafka.apache.org/28/javadoc/org/apache/kafka/common/config/ConfigValue.html#addErrorMessage(java.lang.String)) to report to the user that literal `null` values are not permitted in connector configurations, adding a new `ConfigValue` for the offending property if one isn't already defined by the connector, the Connect framework, etc.
Thoughts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] lhunyady commented on a change in pull request #11333: KAFKA-13306: Null connector config value passes validation, but fails creation
Posted by GitBox <gi...@apache.org>.
lhunyady commented on a change in pull request #11333:
URL: https://github.com/apache/kafka/pull/11333#discussion_r712087974
##########
File path: connect/runtime/src/test/java/org/apache/kafka/connect/runtime/AbstractHerderTest.java
##########
@@ -289,6 +290,23 @@ public void testBuildRestartPlanForUnknownConnector() {
assertFalse(mayBeRestartPlan.isPresent());
}
+ @Test
+ public void testConfigValidationNullConfig() {
Review comment:
Thank you for the comment, updated it in the latest commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] lhunyady commented on pull request #11333: KAFKA-13306: Null connector config value passes validation, but fails creation
Posted by GitBox <gi...@apache.org>.
lhunyady commented on pull request #11333:
URL: https://github.com/apache/kafka/pull/11333#issuecomment-923881582
@C0urante @kkonstantine Can you please review this change for me?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] lhunyady commented on a change in pull request #11333: KAFKA-13306: Null connector config value passes validation, but fails creation
Posted by GitBox <gi...@apache.org>.
lhunyady commented on a change in pull request #11333:
URL: https://github.com/apache/kafka/pull/11333#discussion_r712087974
##########
File path: connect/runtime/src/test/java/org/apache/kafka/connect/runtime/AbstractHerderTest.java
##########
@@ -289,6 +290,23 @@ public void testBuildRestartPlanForUnknownConnector() {
assertFalse(mayBeRestartPlan.isPresent());
}
+ @Test
+ public void testConfigValidationNullConfig() {
Review comment:
yes, done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante commented on a change in pull request #11333: KAFKA-13306: Null connector config value passes validation, but fails creation
Posted by GitBox <gi...@apache.org>.
C0urante commented on a change in pull request #11333:
URL: https://github.com/apache/kafka/pull/11333#discussion_r720508381
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/runtime/AbstractHerder.java
##########
@@ -576,8 +576,12 @@ private static ConfigInfos validateClientOverrides(String connName,
return new ConfigInfos(connectorClass.toString(), errorCount, new ArrayList<>(groups), configInfoList);
}
- // public for testing
public static ConfigInfos generateResult(String connType, Map<String, ConfigKey> configKeys, List<ConfigValue> configValues, List<String> groups) {
+ return generateResult(connType, configKeys, configValues, groups, Collections.emptyMap());
+ }
Review comment:
Since this is no longer used in the main code base, could we remove it from this class and, where necessary, replace it either with a call to the new variant created below, or a testing-only wrapper function that has the same body as this one?
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/entities/ConfigInfo.java
##########
@@ -60,6 +60,6 @@ public int hashCode() {
@Override
public String toString() {
- return "[" + configKey.toString() + "," + configValue.toString() + "]";
+ return "[" + configKey + "," + configValue + "]";
Review comment:
Nice cleanup 👍
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/runtime/AbstractHerder.java
##########
@@ -603,9 +607,39 @@ public static ConfigInfos generateResult(String connType, Map<String, ConfigKey>
}
configInfoList.add(new ConfigInfo(configKeyInfo, configValueInfo));
}
+
+ final List<ConfigValueInfo> nullConfigValues = buildConfigValueInfoWithNullValues(connectorProps);
+ nullConfigValues.forEach(configWithNullValue -> addError(configInfoList, configWithNullValue));
+ errorCount += nullConfigValues.size();
+
return new ConfigInfos(connType, errorCount, groups, configInfoList);
}
+ private static void addError(List<ConfigInfo> configInfoList, ConfigValueInfo configWithNullValue) {
+ final Optional<ConfigInfo> predefinedConfig = getConfigInfoForPredefinedConfig(configInfoList, configWithNullValue.name());
+ if (predefinedConfig.isPresent()) {
+ predefinedConfig.get()
+ .configValue()
+ .errors()
+ .addAll(configWithNullValue.errors());
+ } else {
+ configInfoList.add(new ConfigInfo(null, configWithNullValue));
+ }
+ }
+
+ private static Optional<ConfigInfo> getConfigInfoForPredefinedConfig(List<ConfigInfo> configInfoList, String nullConfigName) {
+ return configInfoList.stream()
+ .filter(configInfo -> configInfo.configValue().name().equals(nullConfigName))
+ .findAny();
+ }
+
+ private static List<ConfigValueInfo> buildConfigValueInfoWithNullValues(Map<String, String> connectorProps) {
+ return connectorProps.entrySet().stream()
+ .filter(configEntry -> configEntry.getValue() == null)
+ .map(configEntry -> new ConfigValueInfo(configEntry.getKey(), String.format("Null Value Not Allowed for key %s.", configEntry.getKey())))
Review comment:
I believe that including the name of the property in the error message is redundant as that information will be available already in the REST response.
I also think we may want to be clearer about the error message here. Users can't supply null values, but developers (by specifying `null` as the default value for a property in a `ConfigDef`, for example) definitely can, and we may want to make it clear which variety we're prohibiting.
What do you think about this?
```suggestion
.map(configEntry -> new ConfigValueInfo(configEntry.getKey(), "The JSON literal `null` may not be used in connector configurations"))
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org