You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "gharris1727 (via GitHub)" <gi...@apache.org> on 2023/02/24 22:14:41 UTC
[GitHub] [kafka] gharris1727 opened a new pull request, #13302: KAFKA-14759: Move Mock, Schema, and Verifiable connectors to new test-plugins module
gharris1727 opened a new pull request, #13302:
URL: https://github.com/apache/kafka/pull/13302
These connectors do not belong in connect-runtime, and there is special code to hide them from the REST API.
Instead, these connectors should be excluded by the build process, and added to the classpath for testing.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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 #13302: KAFKA-14759: Move Mock, Schema, and Verifiable connectors to new test-plugins module
Posted by "C0urante (via GitHub)" <gi...@apache.org>.
C0urante commented on PR #13302:
URL: https://github.com/apache/kafka/pull/13302#issuecomment-1649944134
@gharris1727 I've filed [KAFKA-15249](https://issues.apache.org/jira/browse/KAFKA-15249) to make sure that we don't forget to check for the new `test-plugins` module in Maven Central when it's time to vote for 3.6.0 release candidates. I've self-assigned, but feel free to take over if you'd like.
--
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 #13302: KAFKA-14759: Move Mock, Schema, and Verifiable connectors to new test-plugins module
Posted by "C0urante (via GitHub)" <gi...@apache.org>.
C0urante commented on PR #13302:
URL: https://github.com/apache/kafka/pull/13302#issuecomment-1446859760
Ah yes, good point about KIP-898! Nice that we don't have to file a separate KIP for this change. Will revisit if/when KIP-898 passes.
--
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 #13302: KAFKA-14759: Move Mock, Schema, and Verifiable connectors to new test-plugins module
Posted by "C0urante (via GitHub)" <gi...@apache.org>.
C0urante commented on PR #13302:
URL: https://github.com/apache/kafka/pull/13302#issuecomment-1446393595
Hey @gharris1727, thanks for the PR. While I personally agree with the rationale here and would be happy to merge the changes, I think this may still require a KIP as it removes connectors from OOTB Connect.
Some users may be relying on the `SchemaSourceConnector` as a rudimentary data generation tool in their own testing or development environments, and might be annoyed if we make it more difficult to use that connector.
The description for KAFKA-14759 does mention a desire to "reduce the attack surface area of a default Connect installation", which may be the only reason we would want to merge this change without a KIP; if any of these connectors presents an exploit in Connect, then we'd be justified in removing them just like we did with the `FileStream` source and sink connectors. If you've identified anything like that, please let us know by emailing security@kafka.apache.org.
--
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] gharris1727 commented on pull request #13302: KAFKA-14759: Move Mock, Schema, and Verifiable connectors to new test-plugins module
Posted by "gharris1727 (via GitHub)" <gi...@apache.org>.
gharris1727 commented on PR #13302:
URL: https://github.com/apache/kafka/pull/13302#issuecomment-1446847150
> I think this may still require a KIP as it removes connectors from OOTB Connect.
This is included as part of KIP-898: https://cwiki.apache.org/confluence/display/KAFKA/KIP-898%3A+Modernize+Connect+plugin+discovery and I think we can block this PR from landing until the KIP-898 feature is approved.
> If you've identified anything like that, please let us know by emailing [security@kafka.apache.org](mailto:security@kafka.apache.org).
I am not currently aware of any exploits that leverage these connectors and would justify their removal like the File stream connectors. I'll make sure to properly report any exploitable flaws that I notice via the mailing list, thanks for pointing me in that direction!
--
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 #13302: KAFKA-14759: Move Mock, Schema, and Verifiable connectors to new test-plugins module
Posted by "C0urante (via GitHub)" <gi...@apache.org>.
C0urante commented on PR #13302:
URL: https://github.com/apache/kafka/pull/13302#issuecomment-1613519953
Verified locally by running the `ConnectDistributedTest::test_file_source_and_sink`, `ConnectDistributedTest::test_bounce`, and `ConnectStandaloneFileTest::test_file_source_and_sink` test cases, which cover the addition of the file connectors to both standalone and distributed mode, and the addition of testing-only connectors to distributed mode. There don't appear to be any standalone mode tests that require the testing-only connectors.
--
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] gharris1727 commented on pull request #13302: KAFKA-14759: Move Mock, Schema, and Verifiable connectors to new test-plugins module
Posted by "gharris1727 (via GitHub)" <gi...@apache.org>.
gharris1727 commented on PR #13302:
URL: https://github.com/apache/kafka/pull/13302#issuecomment-1681010261
Test failures in CI appear unrelated. I ran the Connect system tests for this change with #14011 reverted and they all passed.
--
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] gharris1727 merged pull request #13302: KAFKA-14759: Move Mock, Schema, and Verifiable connectors to new test-plugins module
Posted by "gharris1727 (via GitHub)" <gi...@apache.org>.
gharris1727 merged PR #13302:
URL: https://github.com/apache/kafka/pull/13302
--
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 diff in pull request #13302: KAFKA-14759: Move Mock, Schema, and Verifiable connectors to new test-plugins module
Posted by "C0urante (via GitHub)" <gi...@apache.org>.
C0urante commented on code in PR #13302:
URL: https://github.com/apache/kafka/pull/13302#discussion_r1246879168
##########
tests/kafkatest/services/connect.py:
##########
@@ -285,26 +285,31 @@ def append_to_environment_variable(self, envvar, value):
env_opts = "\"%s %s\"" % (env_opts.strip('\"'), value)
self.environment[envvar] = env_opts
- def append_filestream_connectors_to_classpath(self):
+ def maybe_append_filestream_connectors_to_classpath(self):
if self.include_filestream_connectors:
- cwd = os.getcwd()
- self.logger.info("Including filestream connectors when starting Connect. "
- "Looking for jar locally in: %s" % cwd)
- relative_path = "/connect/file/build/libs/"
- local_dir = cwd + relative_path
- lib_dir = self.path.home() + relative_path
- for pwd, dirs, files in os.walk(local_dir):
- for file in files:
- if file.startswith("connect-file") and file.endswith(".jar"):
- # Use the expected directory on the node instead of the path in the driver node
- file_path = lib_dir + file
- self.logger.debug("Appending %s to Connect worker's CLASSPATH" % file_path)
- return "export CLASSPATH=${CLASSPATH}:%s; " % file_path
- self.logger.info("Jar with filestream connectors was not found under %s" % lib_dir)
+ return self.append_module_to_classpath("file")
else:
self.logger.info("Starting Connect without filestream connectors in the CLASSPATH")
+ return ""
- return None
+ def append_test_plugins_to_classpath(self):
+ return self.append_module_to_classpath("test-plugins")
+
+ def append_module_to_classpath(self, module):
+ cwd = os.getcwd()
+ relative_path = "/connect/" + module + "/build/libs/"
+ local_dir = cwd + relative_path
+ lib_dir = self.path.home() + relative_path
+ for pwd, dirs, files in os.walk(local_dir):
+ for file in files:
+ if file.endswith(".jar"):
+ # Use the expected directory on the node instead of the path in the driver node
+ file_path = lib_dir + file
+ self.logger.debug("Appending %s to Connect worker's CLASSPATH" % file_path)
Review Comment:
One last nit: raise to INFO level to replace [this log message](https://github.com/apache/kafka/blob/30b087ead967b28d459945fe90c80545bf189d1f/tests/kafkatest/services/connect.py#L291-L292)?
```suggestion
self.logger.info("Appending %s to Connect worker's CLASSPATH" % file_path)
```
--
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 diff in pull request #13302: KAFKA-14759: Move Mock, Schema, and Verifiable connectors to new test-plugins module
Posted by "C0urante (via GitHub)" <gi...@apache.org>.
C0urante commented on code in PR #13302:
URL: https://github.com/apache/kafka/pull/13302#discussion_r1246819377
##########
build.gradle:
##########
@@ -3048,6 +3049,39 @@ project(':connect:mirror-client') {
}
}
+project(':connect:test-plugins') {
+ archivesBaseName = "connect-test-plugins"
+
+ dependencies {
+ api project(':connect:api')
+
+ implementation project(':tools')
+ implementation libs.slf4jApi
+ implementation libs.jacksonDatabind
+
+ testImplementation libs.junitJupiter
+
+ testRuntimeOnly libs.slf4jlog4j
Review Comment:
We don't have any tests for this module; is it premature to add these dependencies?
##########
build.gradle:
##########
@@ -3048,6 +3049,39 @@ project(':connect:mirror-client') {
}
}
+project(':connect:test-plugins') {
+ archivesBaseName = "connect-test-plugins"
+
+ dependencies {
+ api project(':connect:api')
+
+ implementation project(':tools')
+ implementation libs.slf4jApi
+ implementation libs.jacksonDatabind
+
+ testImplementation libs.junitJupiter
+
+ testRuntimeOnly libs.slf4jlog4j
+ }
+
+ tasks.create(name: "copyDependantLibs", type: Copy) {
+ from (configurations.testRuntimeClasspath) {
+ include('slf4j-log4j12*')
+ include('reload4j*jar')
+ }
+ from (configurations.runtimeClasspath) {
+ exclude('kafka-clients*')
+ exclude('connect-*')
+ }
+ into "$buildDir/dependant-libs"
+ duplicatesStrategy 'exclude'
+ }
Review Comment:
Do we need this? It doesn't look like we use it anywhere (not in `bin/kafka-run-class.sh` or in our system tests' `connect.py`).
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorPluginsResource.java:
##########
@@ -66,25 +58,14 @@ public class ConnectorPluginsResource implements ConnectResource {
private final List<PluginInfo> connectorPlugins;
private long requestTimeoutMs;
- static final List<Class<? extends SinkConnector>> SINK_CONNECTOR_EXCLUDES = Arrays.asList(
- VerifiableSinkConnector.class,
- MockSinkConnector.class
- );
-
- static final List<Class<? extends SourceConnector>> SOURCE_CONNECTOR_EXCLUDES = Arrays.asList(
- VerifiableSourceConnector.class,
- MockSourceConnector.class,
- SchemaSourceConnector.class
- );
-
public ConnectorPluginsResource(Herder herder) {
this.herder = herder;
this.connectorPlugins = new ArrayList<>();
this.requestTimeoutMs = DEFAULT_REST_REQUEST_TIMEOUT_MS;
// TODO: improve once plugins are allowed to be added/removed during runtime.
- addConnectorPlugins(herder.plugins().sinkConnectors(), SINK_CONNECTOR_EXCLUDES);
- addConnectorPlugins(herder.plugins().sourceConnectors(), SOURCE_CONNECTOR_EXCLUDES);
+ addConnectorPlugins(herder.plugins().sinkConnectors(), Collections.emptySet());
+ addConnectorPlugins(herder.plugins().sourceConnectors(), Collections.emptySet());
Review Comment:
Can we remove the `excludes` parameter from the `addConnectorPlugins` method now?
--
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