NETOBSERV-974 Add SASL support #112
Conversation
|
linter fails due to cyclo: taking this opportunity to ask you, @msherif1234 @praveingk what's your thoughts on this. Personally I hate this linter metric, I think a long and dumb function is better and more readable than 10 small and dumb functions. Especially when it doesn't make code more reused. So if it was just me I would just raise the golangci-lint cyclop max value to make linter pass. But as you're more actively contributing to this repo, I'll follow what you can suggest :-) |
| // KafkaSASLClientIDPath is the path to the client ID (username) for SASL auth | ||
| KafkaSASLClientIDPath string `env:"KAFKA_SASL_CLIENT_ID_PATH"` | ||
| // KafkaSASLClientSecretPath is the path to the client secret (password) for SASL auth | ||
| KafkaSASLClientSecretPath string `env:"KAFKA_SASL_CLIENT_SECRET_PATH"` |
There was a problem hiding this comment.
is there a default value ?
There was a problem hiding this comment.
There's no default (or in fact, the default is SASL being disabled so these variable are unused). It's the same with the TLS cert path config: as soon as TLS / SASL is enabled, these variables must be configured - but the default still works as they're disabled by default.
| default: | ||
| return nil, fmt.Errorf("unknown SASL type: %s", cfg.KafkaSASLType) | ||
| } | ||
| if err != nil { |
There was a problem hiding this comment.
there is no need for this block since u already return on err
There was a problem hiding this comment.
actually, the line above was setting an error; I modified this part so it's more visible that an error might be produced
pkg/agent/tls.go
Outdated
| @@ -35,5 +35,5 @@ func buildTLSConfig(cfg *Config) (*tls.Config, error) { | |||
| } | |||
| return tlsConfig, nil | |||
There was a problem hiding this comment.
u won't need this return then ?
I would just creating 3 functions one for grpc , kafka and one for ipfix that take protocol as arg, in general even though the function is plain straight fwd but its not good practice having lengthy functions IMO but again this more of nit preference I will go with whatever the rest agree with |
|
@msherif1234 I did what you suggested about splitting the big function |
Codecov Report
@@ Coverage Diff @@
## main #112 +/- ##
==========================================
- Coverage 41.80% 41.24% -0.57%
==========================================
Files 30 31 +1
Lines 2050 2078 +28
==========================================
Hits 857 857
- Misses 1155 1183 +28
Partials 38 38
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
/lgtm |
|
pls add more context to the PR description so we can know what this PR did for reference Thanks!! |
jotak
added
no-doc
|
Thanks @msherif1234 /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jotak The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Just to clarify, I set no-doc/QE labels as it has no impact downstream unless it's configured from the operator - there's an epic for that which isn't planned yet. |
SASL is a common protocol used for authentication with Kafka, that strimzi supports.
This PR adds support to SASL configuration when the agent is used with Kafka. To enable it, set
KAFKA_ENABLE_SASLto true. The path to clientID (KAFKA_SASL_CLIENT_ID_PATH) and to a Secret (KAFKA_SASL_CLIENT_SECRET_PATH) must be configured.KAFKA_SASL_TYPEcan be either "plain" or "scramSHA512".