Add configuration to support broker SSL #493
Conversation
Possibly implements IBM#154, if my assumptions about the implementation are correct.
Conflicts: broker.go config.go
| dialer := net.Dialer{ | ||
| Timeout: conf.Net.DialTimeout, | ||
| KeepAlive: conf.Net.KeepAlive, | ||
| dialer := &net.Dialer{ |
There was a problem hiding this comment.
this struct should not have changed at all
There was a problem hiding this comment.
not sure why it did in the first place, changed it back to have the keepalive and use the reference to it in the tls.DialWithDialer call
|
We don't really have the guarantee that SSL will work with this once it lands in Kafka proper. I am OK with merging this, with the caveat that once proper SSL support lands in Kafka, and for some reason it doesn't work with this code, we will not maintain backwards compatibility for this code. |
Good point, a comment to that effect on the config values is definitely needed. |
|
i am busy with other pieces of the infra at the moment but i still intend to complete - or at least contribute to - this. i will address the various notes but please note that i did only rebase the old branch for the previous pull request to current master and opened the review . |
|
Most of the comments are re. changes to the example program, not the original rebased code. To keep it simple I've updated #156 appropriately and will merge that if it looks good. You are welcome to resubmit the example changes if you wish. |
fix help for the ca file
|
merged from master and addressed your comments, thanks again |
|
Looks mostly OK to me. @wvanbergen? You wrote the http server example. We'll have to re-add a comment re. compatibility guarantees but I think that can be done post-merge? Or would it be better to merge #156 and then rebase this to pull the http server changes in separately? |
| @@ -164,6 +195,11 @@ func newDataCollector(brokerList []string) sarama.SyncProducer { | |||
| config := sarama.NewConfig() | |||
| config.Producer.RequiredAcks = sarama.WaitForAll // Wait for all in-sync replicas to ack the message | |||
| config.Producer.Retry.Max = 10 // Retry up to 10 times to produce the message | |||
| tls_config := createTlsConfiguration() | |||
There was a problem hiding this comment.
Naming nitpick: tlsConfig
|
Couple of comments, mostly looks good 👍 |
Add flag for insecureNoVerify for the TLS config
|
@wvanbergen addressed your comments |
|
that is strange, we have failing tests but go test on my host succeeds |
|
yes i can confirm that the command
run on my host (as is from the travis CI test) completes successfully on my host |
|
Seems to have been a temporary fluke - I just did a rebuild and it passed now. @eapache merge? |
Add configuration to support broker SSL
as mentioned in #154 this has been rebased on current master and can be merged