Feature/broker auth

[tonyfosterdev]

With this PR, kafka client configuration will not be stored with the context in .kcctl via kcctl config

The following is what the new config set-context usage would be:

Usage: kcctl config set-context [--bootstrap-servers=<bootstrapServers>]
                                [--cluster=<cluster>] [-f=<clientConfigFile>]
                                [--offset-topic=<offsetTopic>]
                                [--password=<password>] [--username=<username>]
                                [-o=<clientConfigs>]... <contextName>
Configures the specified context with the provided arguments
      <contextName>         Context name
      --bootstrap-servers=<bootstrapServers>
                            Comma-separated list of Kafka broker URLs
      --cluster=<cluster>   URL of the Kafka Connect cluster to connect to
  -f, --client-config-file=<clientConfigFile>
                            Configuration file for client
  -o, --client-config=<clientConfigs>
                            Configuration for client
      --offset-topic=<offsetTopic>
                            Name of the offset topic
      --password=<password> Password for basic authentication
      --username=<username> Username for basic authentication

Note the new options:

  -f, --client-config-file=<clientConfigFile>
                            Configuration file for client
  -o, --client-config=<clientConfigs>
                            Configuration for client

For this, you may pass in a -f/--client-config-file parameter to pass in a properties file, or the -o/--client-config which is a comma delimited list of configs (I chose -o as I figured -c and -s would likely be for cluster and bootstrap server at some point). The --client-config flag may be passed multiple times, i.e. --client-config="sasl.username=myuser" --client-config="sasl.password=mypassword".

--client-config values will override and values passed via --client-config-files, and --client-config values that appear later in the command will override prior values.

Note: I didn't put this in the README.md yet as I'd like some clarification on the best way to document all that behavior. Also, there's no feature that actually uses this yet, so documentation may just be confusing at this point. Thoughts? @gunnarmorling

[gunnarmorling]

Thx! Could you give an example for how set-context would be invoked with this new option? I'm wondering whether we should (in addition / alternatively) allow to read those options from a file?

In regards to the Maven dependency, can you add this Quarkus extension instead? This transitively pulls in the one you have above, but it also applies the configuration to use the Kafka client in a GraalVM native binary (reflection configuration, some substitutations, etc.)

[gunnarmorling]

I have also sent you an invite to join as a committer, if you are interested. The only we really have is that we don't push our own changes but always have hat least two pairs of eye balls on each change. I.e. this will let you merge pull requests by others to main. The exception are trivial typo fixes and such, feel free to push those straight away.

[tonyfosterdev]

Thx! Could you give an example for how set-context would be invoked with this new option? I'm wondering whether we should (in addition / alternatively) allow to read those options from a file?

In regards to the Maven dependency, can you add this Quarkus extension instead? This transitively pulls in the one you have above, but it also applies the configuration to use the Kafka client in a GraalVM native binary (reflection configuration, some substitutations, etc.)

The way it's currently setup is something like this:

kcctl config set-context my-context --admin-client-config=sasl.username=myuser,sasl.password=mypassword

The properties available would match with what the Admin client allows. Properties would be comma delimited.

The context configuration would look like this:

  "my-context" : {
    "cluster" : "http://localhost:8083",
    "bootstrapServers" : "localhost:9092",
    "adminClientConfig" : {
      "sasl.username": "myuser",
      "sasl.password" : "mypassword"
    }
  }

I'll take a look at that extension!

[tonyfosterdev]

I have also sent you an invite to join as a committer, if you are interested. The only we really have is that we don't push our own changes but always have hat least two pairs of eye balls on each change. I.e. this will let you merge pull requests by others to main. The exception are trivial typo fixes and such, feel free to push those straight away.

Thanks for this! I'm happy to be involved!

[gunnarmorling]

The properties available would match with what the Admin client allows. Properties would be comma delimited.

Gotcha; thx. How many props would one typically need? I'm wondering whether things become unwieldly if it's too many, hence the idea of supporting a file. Both works, I suppose.

[tonyfosterdev]

That's a good question. I suppose there are lots of combinations of things. If we used a props file, would you still want it stored in the context in .kcctl as well? I have a bad habit of just modifying kcctl directly for some of these settings, ha. Sent from ProtonMail mobile

…

-------- Original Message --------

On Nov 9, 2021, 12:59 PM, Gunnar Morling wrote: > The properties available would match with what the Admin client allows. Properties would be comma delimited. Gotcha; thx. How many props would one typically need? I'm wondering whether things become unwieldly if it's too many, hence the idea of supporting a file. Both works, I suppose. — You are receiving this because you authored the thread. Reply to this email directly, [view it on GitHub](#96 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AMHHZOUHKLNAMUUSFFNINGTULFOPZANCNFSM5HVUQQKQ). Triage notifications on the go with GitHub Mobile for [iOS](https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675) or [Android](https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub).

[tonyfosterdev]

OK, after reviewing a bit more, I'm going to rename things from admin-client-config to client-config and build in the properties file support like Gunnar suggested.

I went with admin originally as I thought the admin client was required... The admin client is only necessary for sink connectors, however not source. It's a bit of a misnomer and not necessary, and the credentials should work just the same.

[gunnarmorling]

If we used a props file, would you still want it stored in the context in .kcctl as well?

Yes, the props file would only serve as a source of the client properties when configuring a context. The thinking being, that you may share that file with other Kafka clients potentially. Once the context has been created, it should be self-contained for further usages. I'm not sure how realistic/helpful that ability would be.

Taking a step back, I can see these three options for sourcing the client props:

1. From a file: --client-config-file=my-client-props
2. From one option: --client-config=bootstrap-servers:localhost:9092;user=...;password=...
3. From multiple opaque options (as kafkacat does it): -c bootstrap-servers=localhost:9092 -c user=... -c password=...

After some more consideration, I am leaning towards 3., as it seems more convinient to work with than having to assemble one large string as in 2. And then perhaps 1. as an optional follow-up. WDYT?

[tonyfosterdev]

@gunnarmorling I've updated this PR ahnd description above!

[gunnarmorling]

Thanks, @tonyfosterdev, for the actual change itself and the comprehensive description!

I think the mechanics make lots of sense in general. When you say

The --client-config flag may be passed multiple times, i.e. --client-config="sasl.username" --client-config="sasl.password".

Where's the actual value for the options? Just to make sure I get things right. In terms of documentation, agreed to omit it for now, until we actually will benefit from this in the context of the offset display/reset feature. On the how, good question; I think the current way of describing all the options in the README is ok, but it might not scale indefinitely. Perhaps there should be some kind of man page or similar?

Other than that, there's one question inline, and could you also update the completion file (steps for doing so are in the README, too).

Thanks again!

[tonyfosterdev]

Thanks, @tonyfosterdev, for the actual change itself and the comprehensive description!

I think the mechanics make lots of sense in general. When you say

The --client-config flag may be passed multiple times, i.e. --client-config="sasl.username" --client-config="sasl.password".

Where's the actual value for the options? Just to make sure I get things right. In terms of documentation, agreed to omit it for now, until we actually will benefit from this in the context of the offset display/reset feature. On the how, good question; I think the current way of describing all the options in the README is ok, but it might not scale indefinitely. Perhaps there should be some kind of man page or similar?

Other than that, there's one question inline, and could you also update the completion file (steps for doing so are in the README, too).

Thanks again!

ahh I was typing a bit too quickly! The mechanics are:

--client-config="sasl.username=myuser" --client-config="sasl.password=mypassword"

I forgot to add the values!

[tonyfosterdev]

@gunnarmorling I think we're all set now!

[gunnarmorling]

Ah yes, thx. Btw. I goofed up when doing that package rename, this should be org.kcctl.* actually. ModiTect is another open-source effort I'm involved with, but it's unrelated to kcctl.

[gunnarmorling]

LGTM, merging. Very nice, looking forward to seeing this in action for the offset handling.