Skip to content

KafkaStore SSL support.#319

Merged
Alex L (alexlod) merged 1 commit into
confluentinc:masterfrom
alexlod:kafakstore-ssl
May 12, 2016
Merged

KafkaStore SSL support.#319
Alex L (alexlod) merged 1 commit into
confluentinc:masterfrom
alexlod:kafakstore-ssl

Conversation

@alexlod

Copy link
Copy Markdown
Contributor

This diff adds support for an SSL KafkaStore. The implementation is pretty straightforward, mostly involving forwarding config parameters to the Kafka clients. The tests were a little tricky, though.

There were a few implementation choices I made which I'm not sure are ideal:

  • Some Kafka client configuration parameters have a default value of null. ConfigDef doesn't support optional parameters with a default value of null. My workaround is to make the default value in the schema registry "" and convert that to null when I initialize the Kafka clients.
  • I introduced two new test classes, one testing SSL with authentication, and the other testing SSL without authentication. I copy-pasted some code between these two, which feels bad. However, I couldn't figure out a better alternative.

Thanks ahead of time for taking a look!

# Memory options
if [ -z "$SCHEMA_REGISTRY_HEAP_OPTS" ]; then
SCHEMA_REGISTRY_HEAP_OPTS="-Xmx256M"
SCHEMA_REGISTRY_HEAP_OPTS="-Xmx512M"

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.

This seems quite large for what the schema registry is doing. Did you actually run into limits with the existing setting?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ewen Cheslack-Postava (@ewencp), yeah I was getting OOMEs. I didn't test whether a smaller value would work or not. Want me to test a smaller value, like 384?

@ewencp

Copy link
Copy Markdown
Contributor

Alex L (@alexlod) For the first ConfigDef issue, is this a problem with the Confluent version of those classes that's been fixed in Kafka trunk? I'm wondering if this can be addressed just by switching to the Apache classes? We're making ConfigDef public API in the next release since Connectors will use it to specify configuration info.

For the two new classes, can enough be saved by sharing a base class to make adding yet one more class into the mix?

@alexlod

Copy link
Copy Markdown
Contributor Author

Ewen Cheslack-Postava (@ewencp), great suggestions, thanks! I just pushed a new commit with trivial changes. I'll work on the less trivial suggestions now and report back when I have a new commit. Thanks again.

@alexlod

Copy link
Copy Markdown
Contributor Author

Ewen Cheslack-Postava (@ewencp) and I spoke in person, and I'm documenting the result here. ConfigDef in Kafka doesn't have defineOverride. Given this, there are two approaches to use Kafka's ConfigDef in the rest-utils repo: modify Kafka's ConfigDef to have defineOverride which would require a KIP; or remove RestConfig.baseConfigDef and instead define these defaults in each REST application manually. The latter feels annoying, so we decided the best solution is the former. But considering we'd need a KIP and an upstream Kafka change, in this patch, we'll keep the workarounds and change them later when ConfigDef has defineOverride.

@alexlod

Copy link
Copy Markdown
Contributor Author

Ewen Cheslack-Postava (@ewencp): new commit addressing all your suggestions (minus the ConfigDef ones). I also added one more test requiring client auth but not providing client certs should cause a TimeoutException.

Thanks, and let me know what other suggestions you have.

Comment thread core/pom.xml
</dependency>
<dependency>
<groupId>org.bouncycastle</groupId>
<artifactId>bcpkix-jdk15on</artifactId>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nitpick: should we add this as a property in the parent POM?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch.

@alexlod

Copy link
Copy Markdown
Contributor Author

New commit addressing all comments (exception the ConfigDef ones discussed above). Also fixing a merge conflict.

Comment thread core/pom.xml
</dependency>
<dependency>
<groupId>org.bouncycastle</groupId>
<artifactId>bcpkix-jdk15on</artifactId>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We are using bouncycastle outside of tests?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't believe so. I'll make the scope test and test on my local machine. Good catch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants