add certificate options for all commands

[rwaweber]

Despite the branch name these changes would add certificate support to all available commands.

Happy to restructure them if this could be improved/simplified!

[fgeller]

Hey, thanks for the PR, give me a few days to get back into Wifi country and I'll take a look then 🙏

…

On 10. Oct 2018, at 20:27, Will Weber ***@***.***> wrote: Despite the branch name these changes would add certificate support to all available commands. Happy to restructure them if this could be improved/simplified! You can view, comment on, or merge this pull request online at: #86 Commit Summary add certificate options to all commands File Changes M README.md (7) M common.go (27) M consume.go (22) M group.go (22) M produce.go (22) M topic.go (22) Patch Links: https://github.com/fgeller/kt/pull/86.patch https://github.com/fgeller/kt/pull/86.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[fgeller]

@rwaweber finally back home, thanks for waiting and thanks a lot for your work and time!

the changes to look good to me, i have one suggestion for changing the setup per command and suggested some naming changes in common.go. have you tested the changes? would you mind writing a test for them?
let me know if you want to continue on this or if you want me to pull in your changes and continue working on it. in any case, thanks for pushing this forward! 🙏

[fgeller]

// It appends any certificates found to s and reports whether any certificates
// were successfully parsed.

I'd suggest we check that AppendCertsFromPEM returns true, or?

[fgeller]

certs might be a more concise name in this case?

[fgeller]

you might choose to drop a couple of certs from other names - the context of the function should be enough for a reader, what do you think? ie, for the func args cert, ca, key string

[fgeller]

let's pull most of this into common.go - then we don't need this differently for every command and could add a bit more ui-friendliness. e.g.:

func setupTLSConfig(cert, ca, key string) (tls.Config, error) {

then when we setup the commands we'd use it like this:

tls, err := setupTLSConfig(cmd.tlsCert, cmd.tlsCA, cmd.tlsKey)
if err != nil {
  failf("failed to setup TLS config err=%v", err)
}
if tls != nil {
  cfg.Net.TLS.Enable = true
  cfg.Net.TLS.Config = tls
}

so setupTLSConfig could return errors like TLS cert was supplied, but CA or key is missingand we'd keep the duplication in the command minimal. what do you think?
untested code btw 😇

[rwaweber]

Great suggestions all around! I'm not sure if I totally understood the third section but I tossed around two approaches:

1. Clean up the invocation of the setupCerts function in each of the command's, set the variables for the sarama config closer together(what I went with).
2. Passing in a sarama config object to the setupCerts function, or a function with 80% of the same body, that would also set the sarama config variables and return the, now modified, sarama config. I wasn't too much of a fan of this approach as I feel like it overcomplicated this function a bit and felt that most of the interaction with the sarama config should be handled with the rest of the functions that deal with it.

Totally down to revise my approach here as well!

In terms of testing, I've been using this branch at work without issue! Unfortunately, a tls-enabled kafka cluster isn't trivial to setup for CI/integration test purposes, at least the last time I took a look at it.

[fgeller]

@rwaweber awesome, thanks for that! i'll pull this in as is, and will apply a change so that we provide feedback if only one or two of ca, cert and key are provided. i'd ping you on the PR if you're interested.

re testing: depending on time, i'll try my hands at changing the system test - but if you're using this as is I don't see any reason not to pull this into master (ie, these changes should not interfere if someone isn't supplying the TLS specific args)