Add Kafka TLS support to the producer. #178
Conversation
Codecov Report
@@ Coverage Diff @@
## main #178 +/- ##
==========================================
- Coverage 79.36% 79.33% -0.03%
==========================================
Files 17 17
Lines 2118 2149 +31
==========================================
+ Hits 1681 1705 +24
+ Misses 300 298 -2
- Partials 137 146 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
|
srikartati
force-pushed
the
add_kafka_tls
branch
2 times, most recently
from
356c876
to
5a8c108
Compare
pkg/producer/kafka.go
Outdated
| if tlsConfig := setupTLSConfig(input.KafkaCAFile, input.KafkaTLSCertFile, input.KafkaTLSKeyFile); tlsConfig != nil { | ||
| kafkaConfig.Net.TLS.Config = tlsConfig | ||
| kafkaConfig.Net.TLS.Enable = true | ||
| if input.KafkaTLSSkipVerify { |
There was a problem hiding this comment.
Seems that input.KafkaTLSSkipVerify is not used other places other than here. Do we need to handle the case when input.KafkaTLSSkipVerify is set?
There was a problem hiding this comment.
This can be used in testing, where we can create a tlsConfig with TLSSkipVerify set as true.
I wanted to use this insetupTLSConfig function, but gosec, which is part of golint is not letting me add this as parameter because they do not want to set this as false.
There was a problem hiding this comment.
Got it. We can add code to ignore secure checking with proper reasoning description like:
// #nosec G402: only applicable for testing purpose
t.InsecureSkipVerify = input.KafkaTLSSkipVerify
There was a problem hiding this comment.
Why didn't we pass input.KafkaTLSSkipVerify to setupTLSConfig(), which returns a tls.Config object?
I thought that value can be directly assigned to the returning tls.Config, such as
tls.Config{InsecureSkipVerify: input.KafkaTLSSkipVerify} ?
There was a problem hiding this comment.
golang lint test fails if we directly the value like above. However, I moved the assignment to setupTLSConfig.
Sorry for late response. I had put this PR on hold. Please take a look again.
srikartati
force-pushed
the
add_kafka_tls
branch
2 times, most recently
from
28142d7
to
7ca4dec
Compare
I do not have tests for InitKafkaProducer function. I think TLS certificates warrant some unit tests. Will add those. |
srikartati
force-pushed
the
add_kafka_tls
branch
4 times, most recently
from
2fe4000
to
c61b1ed
Compare
Also, enhance the input consumption when initializing the Kafka producer. Unit tests added for TLS cert based kafka connection.
Also, enhance the input consumption when initializing the
Kafka producer.