accept 'disable' as a valid tls value for kafka scaler

[gunniwho]

Signed-off-by: Gunnar Palsson gunniwho@gmail.com

Provide a description of what has been changed

Accepts disable as a valid value for the tls auth parameter of the kafka scaler.

Checklist

• Commits are signed with Developer Certificate of Origin (DCO - learn more)
• Tests have been added
• A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
• A PR is opened to update the documentation on (repo) (if applicable)
• Changelog has been updated and is aligned with our changelog requirements

Fixes #2608

[zroubalik]

@gunniwho could you please add some tests for this parameter?

[gunniwho]

@zroubalik I could try but I don't have any experience writing go code. Should the test be added to the kafka scaler e2e tests?

[zroubalik]

@gunniwho thanks, unit tests are enough for this feature imho, they are located here: https://github.com/kedacore/keda/blob/main/pkg/scalers/kafka_scaler_test.go

it is just about adding validation for this field. Thanks a lot!

[JorTurFer]

I'm missing something, is not adding disable like having the default value?
I mean, in this case tls refers to the authentication method and not the usage of SSL/TLS protocol itself. Probably I'm wrong because I have never used Kafka before, but reading the docs that's what I understand

[nilayasiktoprak]

I'm missing something, is not adding disable like having the default value? I mean, in this case tls refers to the authentication method and not the usage of SSL/TLS protocol itself. Probably I'm wrong because I have never used Kafka before, but reading the docs that's what I understand

I agree with @JorTurFer. In the docs( https://keda.sh/docs/2.6/scalers/apache-kafka/#authentication-parameters ), it is indicated that if you don't pass the tls value, it means it's disabled which is the default. So that's the way to disable tls if you don't want to use it.
However, I think it's a better practice to pass the tls anyway whether it's enabled or disabled in the yaml so that it will be more clear.

[gunniwho]

I'm missing something, is not adding disable like having the default value? I mean, in this case tls refers to the authentication method and not the usage of SSL/TLS protocol itself. Probably I'm wrong because I have never used Kafka before, but reading the docs that's what I understand

@JorTurFer No, you're absolutely right. However, my motivation for this is that when creating the TokenAuthentication resource it was much harder for me (because reasons...) to not include the tls parameter than to include it as 'disable'. I just found it really weird that I couldn't do it.

[JorTurFer]

The point is that in this case it could be confusing because TLS sounds like transport encryption and not like client authentication method. Maybe we should rename them into more explicit keys.
Also, the documentation is wrong because true and false are invalid values:

IMHO we should rename the authentication method and honor TLS key if we want to allow secure/insecure traffic (if Kafka supports it, IDK) or keep it there for backward compatibility during some versions if not. But related with this, I don't see any necessity of adding extra code to explicitly set disable. Removing the else block sounds more correct to me (and it'll allow disable too).

WDYT @kedacore/keda-core-contributors ?

BTW: Thanks for exposing the problem and for the contribution ❤️ ❤️ ❤️

[gunniwho]

@JorTurFer I agree that it would be better to just remove the else block, effectively saying that passing anything other than 'enable' for this value just means that it's not enabled. It was annoying that it was throwing errors for seemingly no reason.

Regarding the docs, I also did PR to fix them (for 2.7.0 - was asked to also fix for previous versions).

Regarding the TLS naming, I don't have an opinion at all...

[tomkerkhove]

I think this change makes sense so that we only log errors for invalid configuration.

What is your question @JorTurFer? I'm not sure if I get the proposal.

[JorTurFer]

The point is that tls usually means Transport Layer Security but in Kafka scaler (AFAIK) means SSL authentication. You can use user/password (sasl) or certificate (tls). It's not related with the common meaning of tls and it could be confusing.
My proposal is renaming tls into certificate and decide if we want to support (if Kafka supports it) Transport Layer Security do it like we are doing in other scalers with something like unsafeSsl or similar.

Related with this PR in specific, my proposal is removing the else block directly

[gunniwho]

The tls option is not synonymous with certificate authentication for this scaler. It can be used to authenticate using certs (I think) but it is also the way to specify that you want to use the SASL authentication over a secured connection. If you specify sasl and not tls the credentials (or hashes in the case of scram) are sent over an insecure connection.

[JorTurFer]

The tls option is not synonymous with certificate authentication for this scaler. It can be used to authenticate using certs (I think) but it is also the way to specify that you want to use the SASL authentication over a secured connection. If you specify sasl and not tls the credentials (or hashes in the case of scram) are sent over an insecure connection.

That's why I propose to extract the real tls part from the certificate authentication. Not in this PR obviously, in fact, I'll create an issue and we can discuss it there. Sorry for the noise

[gunniwho]

@gunniwho thanks, unit tests are enough for this feature imho, they are located here: https://github.com/kedacore/keda/blob/main/pkg/scalers/kafka_scaler_test.go

it is just about adding validation for this field. Thanks a lot!

done

[zroubalik]

The tls option is not synonymous with certificate authentication for this scaler. It can be used to authenticate using certs (I think) but it is also the way to specify that you want to use the SASL authentication over a secured connection. If you specify sasl and not tls the credentials (or hashes in the case of scram) are sent over an insecure connection.

That's why I propose to extract the real tls part from the certificate authentication. Not in this PR obviously, in fact, I'll create an issue and we can discuss it there. Sorry for the noise

If I am not mistaken tls and sasl is being used for this specific purposes in Kafka world. I think the scaler should align with that.

[JorTurFer]

LGTM!

[JorTurFer]

hey @gunniwho ,
Your last 2 commits are not signed. Could you sign them?
https://github.com/kedacore/keda/blob/main/CONTRIBUTING.md#i-didnt-sign-my-commit-now-what

[JorTurFer]

Thanks for your contribution ❤️