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.

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

Minor glitch regarding KSQL configuration #288

Closed
jeanlouisboudart opened this issue May 31, 2021 · 2 comments · Fixed by #308
Closed

Minor glitch regarding KSQL configuration #288

jeanlouisboudart opened this issue May 31, 2021 · 2 comments · Fixed by #308
Labels
bug Something isn't working

Comments

@jeanlouisboudart
Copy link

Describe the bug
The documentation (https://github.com/kafka-ops/julie/blob/master/docs/futures/what-ksql-management.rst#configuring-ksql-servers) suggest that we configure the KSQL endpoint as follows :

platform.server.ksql = "http://ksql:8088"

However, this doesn't work and KSQL Client seems to be trying use "http" as a hostname".

To Reproduce
Steps to reproduce the behavior:

  1. Setup an environment with Kafka + KSQL
  2. Configure platform.server.ksql = "http://ksql:8088" to point to your KSQL instance
  3. Create a dummy query
  4. Create a dummy topology
  5. Run julie ops

Expected behavior
Part of the problem comes from the fact that we parse the provided URL in two places :

  1. String server = ksqlAddress.substring(0, ksqlAddress.lastIndexOf(":"));
    Integer port = Integer.parseInt(ksqlAddress.substring(ksqlAddress.lastIndexOf(":") + 1));
    (at this point the host would be http://ksql and the port would be 8088)

  2. https://github.com/kafka-ops/julie/blob/master/src/main/java/com/purbon/kafka/topology/api/ksql/KsqlApiClient.java#L38-L39
    (at this point the host would be http and the port would be 8088)

We should either update the documentation or fix the behavior to be more robust.

Runtime (please complete the following information):

  • OS: Ubuntu
  • JVM version: openjdk 11
  • Version : Master (to test KSQL integration)
@jeanlouisboudart jeanlouisboudart added the bug Something isn't working label May 31, 2021
@schocco
Copy link
Contributor

schocco commented May 31, 2021

To make things more robust, switching to java.net.URL rather then passing Strings around could be an option. The KsqlApiClient constructor could then look like this:

    public KsqlApiClient(URL server, @Nullable BasicAuth basicAuth) {
        this.server = server;
        ClientOptions options = ClientOptions.create()
                .setHost(server.getHost())
                .setPort(server.getPort());
        if (server.getProtocol() != null && server.getProtocol().equals("https")) {
            options.setUseTls(true);
            options.setUseAlpn(true);
        }
        if (basicAuth != null) {
            options.setBasicAuthCredentials(basicAuth.getUser(), basicAuth.getPassword());
        }
        client = Client.create(options);
    }

And the Configuration class would need an additional getter, e.g.

public BasicAuth getKSQLBasicAuth() {
        return new BasicAuth(config.getString(PLATFORM_SERVER_KSQL_BASIC_AUTH_USER), config.getString(PLATFORM_SERVER_KSQL_BASIC_AUTH_PASSWORD));
    }

schocco pushed a commit to schocco/julie that referenced this issue Jun 15, 2021
* 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
purbon added a commit that referenced this issue Sep 11, 2021
* extend ksql support

* 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 #288 #287
* add missing toString method  #289

* refactor and add tests

* 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

* remove editorconfig

* ammend token variable initialization within the julie http base client

Co-authored-by: Rocco Schulz <rocco@is-gr8.com>
Co-authored-by: Pere Urbón <purbon@users.noreply.github.com>
Co-authored-by: Pere Urbon <pere@confluent.io>
@purbon
Copy link
Collaborator

purbon commented Sep 11, 2021

closed with the merge of #308

@purbon purbon closed this as completed Sep 11, 2021
@purbon purbon linked a pull request Sep 11, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants