Skip to content

KAFKA-9570: Define SSL configs in all worker config classes, not just distributed#8135

Merged
rhauch merged 3 commits into
apache:trunkfrom
C0urante:kafka-9570
Jun 5, 2020
Merged

KAFKA-9570: Define SSL configs in all worker config classes, not just distributed#8135
rhauch merged 3 commits into
apache:trunkfrom
C0urante:kafka-9570

Conversation

@C0urante

@C0urante C0urante commented Feb 18, 2020

Copy link
Copy Markdown
Contributor

Jira

All SSL-related configs are currently defined only in the DistributedConfig class, even though they are applicable for standalone mode as well (since standalone mode also supports the Connect REST API). Because of how these configs are parsed by the framework, it's currently impossible to configure Connect in standalone mode to use SSL for the REST API with a password-protected keystore, key, or truststore, and even if no password protection is required, SSL configs will not be picked up correctly by the worker if any of the worker configs start with the listeners.https. prefix.

These changes define the relevant SSL-related configs in the parent WorkerConfig class, which should fix how they are picked up in standalone mode.

A new unit test is added to verify that the StandaloneConfig picks up these configs correctly.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

ncliang
ncliang reviewed Feb 19, 2020
View reviewed changes

@ncliang ncliang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a question about SASL support. Otherwise LGTM.

Comment thread ...ct/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/DistributedConfig.java Outdated
ncliang
ncliang approved these changes Feb 19, 2020
View reviewed changes

@ncliang ncliang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the fix @C0urante

rhauch
rhauch reviewed Feb 25, 2020
View reviewed changes

@rhauch rhauch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @C0urante. Looks good, though it seems like the test code could be simplified.

Comment thread .../runtime/src/test/java/org/apache/kafka/connect/runtime/standalone/StandaloneConfigTest.java Outdated
Comment thread .../runtime/src/test/java/org/apache/kafka/connect/runtime/standalone/StandaloneConfigTest.java Outdated
Comment thread .../runtime/src/test/java/org/apache/kafka/connect/runtime/standalone/StandaloneConfigTest.java Outdated
@C0urante

C0urante commented Mar 4, 2020

Copy link
Copy Markdown
Contributor Author

Thanks for the review, @rhauch. I've incorporated your feedback with some small tweaks that I hope are acceptable; could you give this another pass when you have a chance?

rhauch
rhauch approved these changes Mar 10, 2020
View reviewed changes

@rhauch rhauch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks, @C0urante.

I plan to merge this to trunk and backport when we're past the freeze on 2.5 and 2.4 branches.

@C0urante

C0urante commented Mar 10, 2020

Copy link
Copy Markdown
Contributor Author

Awesome, thanks Randall!

@kkonstantine

kkonstantine commented May 20, 2020

Copy link
Copy Markdown
Contributor

ok to test

@C0urante

C0urante commented Jun 4, 2020

Copy link
Copy Markdown
Contributor Author

@rhauch would it be possible to backport this to 2.6 before the upcoming release?

@rhauch

rhauch commented Jun 4, 2020

Copy link
Copy Markdown
Contributor

Yeah, it's on my list.

@rhauch

rhauch commented Jun 4, 2020

Copy link
Copy Markdown
Contributor

retest this please

@rhauch
rhauch merged commit 2d9376c into apache:trunk Jun 5, 2020
rhauch pushed a commit that referenced this pull request Jun 5, 2020
@C0urante @rhauch
KAFKA-9570: Define SSL configs in all worker config classes, not just…
0567334 
… distributed (#8135)

Define SSL configs in all worker config classes, not just distributed

Author: Chris Egerton <chrise@confluent.io>
Reviewers: Nigel Liang <nigel@nigelliang.com>, Randall Hauch <rhauch@gmail.com>
rhauch pushed a commit that referenced this pull request Jun 5, 2020
@C0urante @rhauch
KAFKA-9570: Define SSL configs in all worker config classes, not just…
2ee9eed 
… distributed (#8135)

Define SSL configs in all worker config classes, not just distributed

Author: Chris Egerton <chrise@confluent.io>
Reviewers: Nigel Liang <nigel@nigelliang.com>, Randall Hauch <rhauch@gmail.com>
rhauch pushed a commit that referenced this pull request Jun 5, 2020
@C0urante @rhauch
KAFKA-9570: Define SSL configs in all worker config classes, not just…
63fb0d5 
… distributed (#8135)

Define SSL configs in all worker config classes, not just distributed

Author: Chris Egerton <chrise@confluent.io>
Reviewers: Nigel Liang <nigel@nigelliang.com>, Randall Hauch <rhauch@gmail.com>
Kvicii pushed a commit to Kvicii/kafka that referenced this pull request Jun 6, 2020
Merge branch 'trunk' of github.com:apache/kafka into trunk-read
6f039a3 
* 'trunk' of github.com:apache/kafka: (46 commits)
  MINOR: improve code encapsulation between StreamThread and TaskManager (apache#8819)
  Fixing KAFKA-10094 (apache#8797)
  KAFKA-9851: Revoking Connect tasks due to connectivity issues should also clear the running assignment (apache#8804)
  KAFKA-9840; Skip End Offset validation when the leader epoch is not reliable (apache#8486)
  HOT_FIX: Update javadoc since imports added (apache#8817)
  KAFKA-8011: Fix flaky RegexSourceIntegrationTest (apache#8799)
  KAFKA-9570: Define SSL configs in all worker config classes, not just distributed (apache#8135)
  KAFKA-10111: Make SinkTaskContext.errantRecordReporter() a default method (apache#8814)
  KAFKA-10110: Corrected potential NPE when null label value added to KafkaMetricsContext (apache#8811)
  MINOR: Change the order that Connect calls `config()` and `validate()` to avoid validating if the required ConfigDef is null (apache#8810)
  MINOR: fix backwards incompatibility in JmxReporter introduced by KIP-606
  MINOR: Fix javadoc warnings (apache#8809)
  KAFKA-9441: Improve Kafka Streams task management (apache#8776)
  fix the broken links of streams javadoc (apache#8789)
  KAFKA-10040; Make computing the PreferredReplicaImbalanceCount metric more efficient (apache#8724)
  KAFKA-10066: TestOutputTopic should pass record headers into deserializers (apache#8759)
  MINOR: Add explanation for disabling forwarding from value transformers (apache#8771)
  KAFKA-10033: Throw UnknownTopicOrPartitionException if altering configs of non-existing topic
  KAFKA-9434: automated protocol for alterReplicaLogDirs (apache#8311)
  KAFKA-9313: Set `use_all_dns_ips` as the new default for `client.dns.lookup` (KIP-602) (apache#8644)
  ...
ijuma added a commit to confluentinc/kafka that referenced this pull request Jun 8, 2020
@ijuma
Merge remote-tracking branch 'apache-github/trunk' into ccs-master
a6f935e 
Conflicts:
* build.gradle: take upstream changes regarding heap memory
configuration for the build.

* apache-github/trunk: (33 commits)
  MINOR: fix HTML markup (apache#8823)
  KAFKA-10012; Reduce overhead of strings in SelectorMetrics (apache#8684)
KAFKA-9216: Enforce internal config topic settings for Connect workers
during startup (apache#8270)
  KAFKA-10097: Internalize checkpoint data (apache#8820)
KAFKA-10033: Throw UnknownTopicOrPartitionException when modifying a
non-existent topic's config
MINOR: improve code encapsulation between StreamThread and TaskManager
(apache#8819)
  Fixing KAFKA-10094 (apache#8797)
KAFKA-9851: Revoking Connect tasks due to connectivity issues should
also clear the running assignment (apache#8804)
KAFKA-9840; Skip End Offset validation when the leader epoch is not
reliable (apache#8486)
  HOT_FIX: Update javadoc since imports added (apache#8817)
  KAFKA-8011: Fix flaky RegexSourceIntegrationTest (apache#8799)
KAFKA-9570: Define SSL configs in all worker config classes, not just
distributed (apache#8135)
KAFKA-10111: Make SinkTaskContext.errantRecordReporter() a default
method (apache#8814)
KAFKA-10110: Corrected potential NPE when null label value added to
KafkaMetricsContext (apache#8811)
MINOR: Change the order that Connect calls `config()` and `validate()`
to avoid validating if the required ConfigDef is null (apache#8810)
MINOR: fix backwards incompatibility in JmxReporter introduced by
KIP-606
  MINOR: Fix javadoc warnings (apache#8809)
  KAFKA-9441: Improve Kafka Streams task management (apache#8776)
  fix the broken links of streams javadoc (apache#8789)
KAFKA-10040; Make computing the PreferredReplicaImbalanceCount metric
more efficient (apache#8724)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rhauch rhauch rhauch approved these changes
+2 more reviewers
@ncliang ncliang ncliang approved these changes
@andrewchoi5 andrewchoi5 andrewchoi5 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

connect

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@C0urante @kkonstantine @rhauch @ncliang @andrewchoi5