Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ksql modifications: SSL support, fix URL handling #292

Closed
wants to merge 3 commits into from
Closed

ksql modifications: SSL support, fix URL handling #292

wants to merge 3 commits into from

Conversation

schocco
Copy link
Contributor

@schocco schocco commented Jun 2, 2021 •
edited
Loading

  • Please check if the PR fulfills these requirements
  • The commit messages are descriptive
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • An issue has been created for the pull requests. Some issues might require previous discussion.
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
    none

  • questions/discussion:

  • what property keys should be used for ksql settings (truststore, basic auth, etc)? prefixed with topology.* or platform.server.ksql?
  • should getMdsServer and getConfluentSchemaRegistryUrl be refactored to also return URL as a type for consistency?
  • any ideas for adding TLS tests in KsqlClientIT?

@schocco schocco marked this pull request as draft June 3, 2021 09:03
@purbon
Copy link
Collaborator

purbon commented Jun 3, 2021

Thanks a lot for your contribution @schocco I appreciate it a ton! I will let some comments in the next few minutes.

purbon
purbon reviewed Jun 3, 2021
View reviewed changes
.editorconfig Outdated Show resolved Hide resolved
purbon
purbon reviewed Jun 3, 2021
View reviewed changes
src/main/java/com/purbon/kafka/topology/Configuration.java Outdated
throw new ConfigurationException(config.getString(PLATFORM_SERVER_KSQL_URL) + " is not a valid KSQL URL");
}
}
return null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would probably suggest to return an optional, instead of null, right?

purbon
purbon reviewed Jun 3, 2021
View reviewed changes
src/main/java/com/purbon/kafka/topology/api/ksql/KsqlApiClient.java Outdated
.setPort(server.getPort());
if (server.getProtocol() != null && server.getProtocol().equals(HTTPS)) {
options.setUseTls(true);
options.setUseAlpn(true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder how this property will behave with old or less up to using old or less up to date TLS support. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I only tested this on CP 6.0.2 with JDK11 so far - it probably makes sense to provide sensible defaults and allow users to override them with the properties file.
Also an option for setting truststores/keystores might be needed as additional configuration option as mentioned in #287

These additional properties should then probably end up in platform.server.ksql.XXX

Copy link
Collaborator

Choose a reason for hiding this comment

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

I love the idea of adding it as config option 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

additional configuration options are now present and documented in docs/futures/what-ksql-management.rst

purbon
purbon reviewed Jun 3, 2021
View reviewed changes
src/main/java/com/purbon/kafka/topology/exceptions/ConfigurationException.java
import java.io.IOException;

public class ConfigurationException extends IOException {
public class ConfigurationException extends RuntimeException {
Copy link
Collaborator

@purbon purbon Jun 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

can you please elaborate a bit on this change to RuntimeException?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previously checked exception was never handled explicitly but just rethrown, so I changed it to an unchecked one (and I wanted to use the ConfigurationException when a URL value could not be parsed without adding a throws clause in the getter in Configuration#getKSQLServer)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder, if instead of making it runtime, we could keep it as an exception and properly handle it. Not sure about your experiences, but I kinda prefer always explicit exception handling as much as possible. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think in the end the question is: Can we properly handle or solve the issue in a catch block when the user provided an invalid configuration?

There is a little section for checked vs unchecked here:
https://docs.oracle.com/javase/tutorial/essential/exceptions/runtime.html

@purbon
Copy link
Collaborator

purbon commented Jun 3, 2021

what property keys should be used for ksql settings (truststore, basic auth, etc)? prefixed with topology.* or platform.server.ksql?

I prefer to go into platform.server.ksql, topology is more for the cluster description description.

@purbon
Copy link
Collaborator

purbon commented Jun 3, 2021

any ideas for adding TLS tests in KsqlClientIT?

yo should need to add support this in the ksql test containers, it would look something like https://github.com/confluentinc/cp-demo/blob/6.1.1-post/docker-compose.yml#L824-L831

Rocco Schulz added 2 commits June 14, 2021 15:05
extend ksql support
ad4df83 
* add documentation for new ksql properties and introduce KsqlClientConfig class to bundle ksql related options
    format code
* add wait strategy in to ksql container to get rid of Thread.sleep call in integration tests
* add basic support for TLS and fix handling ksql urls kafka-ops#288 kafka-ops#287
* add missing toString method  kafka-ops#289
refactor and add tests
ad83b4a 
* ensure verifyHost defaults to true
* testcontainers: wait for log message instead of polling ksql /info endpoint
* refactor to use BasicAuth class in JulieHttpClient
* add integration test for ksqldb with TLS
@schocco schocco force-pushed the ksql-modifications branch from 88240c4 to ad83b4a Compare June 15, 2021 09:48
@schocco schocco marked this pull request as ready for review June 15, 2021 09:51
@schocco schocco changed the title ksql modifications ksql modifications: SSL support, fix URL handling Jun 15, 2021
@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot closed this Jul 29, 2021
@purbon
Copy link
Collaborator

purbon commented Aug 11, 2021

feel free to reopen, I should really kick this github bot :-( I apologies. I will follow up soon, sorry this last months has been not very operational for my open source efforts.

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

@purbon purbon purbon left review comments

Assignees
No one assigned
Labels
no-activity
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@schocco @purbon