fix: move `sasl` and `tls` flag to ScaledObject instead of specifying in TriggerAuthentication #4322

dttung2905 · 2023-03-04T15:13:05Z

Hi team,
This MR will solve

Provide a description of what has been changed

Checklist

When introducing a new scaler, I agree with the scaling governance policy
I have verified that my change is according to the deprecations & breaking changes policy
Tests have been added
Changelog has been updated and is aligned with our changelog requirements
A PR is opened to update the documentation on (repo) Add Doc changes for enableTls and saslAuthType for Kafka Scaler ScaledObject keda-docs#1016
Commits are signed with Developer Certificate of Origin (DCO - learn more)

Example:

triggers:
- type: kafka
  metadata:
    bootstrapServers: kafka.svc:9092
    consumerGroup: my-group
    topic: test-topic
    lagThreshold: '5'
    activationLagThreshold: '3'
    offsetResetPolicy: latest
    allowIdleConsumers: false
    scaleToZeroOnInvalidOffset: false
    excludePersistentLag: false
    version: 1.0.0
+   saslAuthType: plaintext|scram_sha256|scram_sha512|oauthbearer|none
+   enableTls: true|false

Fixes #3611

Relates to #

dttung2905 · 2023-03-05T10:50:09Z

Also I have included a small fix for the static check failure for Azure Blob Scaler link ( Not quite relevant to this MR though but I try to make this pass the CI )

pkg/scalers/azure_blob_scaler.go:142:111: string `true` has 3 occurrences, but such constant `stringTrue` already exists (goconst)
	if val, ok := config.TriggerMetadata["useAAdPodIdentity"]; ok && config.PodIdentity.Provider == "" && val == "true" {
	                                                                                                             ^

Error: Process completed with exit code 1.

JorTurFer · 2023-03-06T19:49:50Z

/run-e2e kafka*
Update: You can check the progress here

JorTurFer

LGTM!

JorTurFer · 2023-03-06T20:14:42Z

Is this ready to review @dttung2905 ? I ask because it has WIP in the title

tomkerkhove · 2023-03-07T06:29:33Z

CHANGELOG.md

@@ -44,7 +44,7 @@ To learn more about active deprecations, we recommend checking [GitHub Discussio

 ### Breaking Changes

- TODO
+- **Kafka Scaler**: move `tls` and `sasl` in TriggerAuthentication to `enableTls` and `saslAuthType` in ScaledObject ([#4232](https://github.com/kedacore/keda/issues/4322))


This should not be a breaking change as we discussed but more a new approach next to the deprecated approach. Would you mind updating the changelog and ensure this complies with https://github.com/kedacore/governance/blob/main/DEPRECATIONS.md

Since we will go with supporting both methods, I will move this to Improvements section 🙏

dttung2905 · 2023-03-07T06:35:07Z

Is this ready to review @dttung2905 ? I ask because it has WIP in the title

Thanks for the super quick review on this! I put WIP because I still need to update the doc here , as @tomkerkhove pointed out (thanks Tom for that)
I will update the doc PR later and the CHANGELOG as well

tomkerkhove · 2023-03-07T06:47:40Z

Thanks a ton and no rush - Take your time!

zroubalik

Don't we want to support both approaches? (Instead of removing the old one?)

dttung2905 · 2023-03-07T13:59:35Z

Don't we want to support both approaches? (Instead of removing the old one?)

Personally, I would say we deprecate the old one and remove it in the next 2 releases. The reason is having multiple places to specify the same value just make it confusing to the user. Also tls: enable and sasl:scram_sha256 are not really secrets so it might be weird to park it under TriggerAuthentication. What do you think @JorTurFer @tomkerkhove ?

zroubalik · 2023-03-07T14:14:09Z

Don't we want to support both approaches? (Instead of removing the old one?)

Personally, I would say we deprecate the old one and remove it in the next 2 releases. The reason is having multiple places to specify the same value just make it confusing to the user. Also tls: enable and sasl:scram_sha256 are not really secrets so it might be weird to park it under TriggerAuthentication. What do you think @JorTurFer @tomkerkhove ?

Yeah, though the problem is, that in my opinion these settings use 90% of ScaledObjects with Kafka, which means a lot of users 😄

JorTurFer · 2023-03-07T16:00:15Z

What do you think @JorTurFer @tomkerkhove ?

We have a clear policy about that, we should keep both options during the deprecation period but AFAI see, you have kept both https://github.com/kedacore/keda/pull/4322/files#diff-03cf62956adcec86c0717723ab146472f0979221b0e5942a6e5460fa6d5e4f08R180-R196. Am I right?

dttung2905 · 2023-03-07T16:10:40Z

What do you think @JorTurFer @tomkerkhove ?

We have a clear policy about that, we should keep both options during the deprecation period but AFAI see, you have kept both https://github.com/kedacore/keda/pull/4322/files#diff-03cf62956adcec86c0717723ab146472f0979221b0e5942a6e5460fa6d5e4f08R180-R196. Am I right?

What @zroubalik proposed it that we keep the old method forever and not deprecate at all .

zroubalik · 2023-03-07T16:43:25Z

What do you think @JorTurFer @tomkerkhove ?

We have a clear policy about that, we should keep both options during the deprecation period but AFAI see, you have kept both https://github.com/kedacore/keda/pull/4322/files#diff-03cf62956adcec86c0717723ab146472f0979221b0e5942a6e5460fa6d5e4f08R180-R196. Am I right?

What @zroubalik proposed it that we keep the old method forever and not deprecate at all .

Yeah, I think that we should keep both options present. Some users might want to specify these settings in TriggerAuth.

I vote for not deprecating.

JorTurFer · 2023-03-07T16:53:22Z

What do you think @JorTurFer @tomkerkhove ?

We have a clear policy about that, we should keep both options during the deprecation period but AFAI see, you have kept both #4322 (files). Am I right?

What @zroubalik proposed it that we keep the old method forever and not deprecate at all .

I don't have any strong opinion, but it's true that it isn't a secret, so doesn't make sense treating it as a secret and maybe dropping the support is worth to make the things more clear

dttung2905 · 2023-03-08T02:37:53Z

@tomkerkhove what is your opinion on this :P ? I support smooth developer experience so there are two sides of the coin ( clarify vs disruption ). I can keep both and not deprecate the old method too if Tom is not strongly against it

In the future, if there are feedbacks from user about confusions of several methods, we can go back and deprecate the old one. For me its not fixed anyways 🙏

tomkerkhove · 2023-03-08T13:35:15Z

I don't have an opinion on this, I'm fine with either

zroubalik · 2023-03-08T13:39:42Z

Thanks, so let's not deprecate :)

tomkerkhove · 2023-03-08T14:00:45Z

Fine for me!

tomkerkhove · 2023-03-08T14:00:58Z

Let's make sure this is also removed from the docs

zroubalik · 2023-03-08T19:36:51Z

/run-e2e kafka*
Update: You can check the progress here

JorTurFer · 2023-03-09T12:30:14Z

pkg/scalers/kafka_scaler.go

+	case config.TriggerMetadata["saslAuthType"] != "":
+		saslAuthType = config.TriggerMetadata["saslAuthType"]


Why don't just use the same key? sasl should be better as it's the same key, recovered from both places

Good catch! Agree, we should use the same key for both TA and trigger metadata.

Agree with that as I cross commented on the doc PR. Also it means that for tls the value should be enable/disable instead of true/false right? Just want to confirm this

Let's reuse the very same apporach that already present wrt key/value naming please :)

We can think about deprecation/renaming later on. But let's be consistent.

yeah, the valid values shouldn't change from previous state

Got it. Thanks for the review. I will try do it later once im back to keyboard

JorTurFer · 2023-03-09T12:30:27Z

pkg/scalers/kafka_scaler.go

@@ -151,30 +165,48 @@ func parseKafkaAuthParams(config *ScalerConfig, meta *kafkaMetadata) error {
 	}

 	meta.enableTLS = false
+	enableTLS := false
+	if val, ok := config.TriggerMetadata["enableTls"]; ok {


Why don't just use the same key? tls should be better as it's the same key, recovered from both places

zroubalik

looking good, just rename the keys please

…erAuthentication Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>

Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>

zroubalik · 2023-03-09T14:57:27Z

/run-e2e kafka*
Update: You can check the progress here

… in TriggerAuthentication (kedacore#4322) Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>

dttung2905 requested a review from a team as a code owner March 4, 2023 15:13

dttung2905 mentioned this pull request Mar 4, 2023

feat: Kafka scaler move sasl and tls config to TriggerAuthentication metadata #4024

Closed

5 tasks

JorTurFer approved these changes Mar 6, 2023

View reviewed changes

dttung2905 changed the title ~~[WIP] fix: move sasl and tls flag to ScaledObject instead of specifying in TriggerAuthentication~~ fix: move sasl and tls flag to ScaledObject instead of specifying in TriggerAuthentication Mar 7, 2023

tomkerkhove reviewed Mar 7, 2023

View reviewed changes

zroubalik reviewed Mar 7, 2023

View reviewed changes

dttung2905 force-pushed the fix-kafka-scaler-sasl-tls-setting branch from 679028c to 082c171 Compare March 8, 2023 16:18

JorTurFer approved these changes Mar 9, 2023 •

edited

Loading

View reviewed changes

JorTurFer requested changes Mar 9, 2023

View reviewed changes

zroubalik requested changes Mar 9, 2023

View reviewed changes

dttung2905 added 3 commits March 9, 2023 22:32

move sasl and tls flag to ScaledObject instead of specifying in Trigg…

7052500

…erAuthentication Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>

Add CHANGELOG file

362145c

Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>

Fix static check in azure_blob_scaler

4510356

Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>

dttung2905 added 4 commits March 9, 2023 22:32

Do not deprecate the second auth method

b076f72

Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>

Fix CHANGELOG

a8ac929

Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>

both methods should be using the same keys name

1904c48

Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>

update CHANGELOG

fe03c6d

Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>

dttung2905 force-pushed the fix-kafka-scaler-sasl-tls-setting branch from 82f3f04 to fe03c6d Compare March 9, 2023 14:34

fix ci precommit webhook

3aa64b4

Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>

JorTurFer approved these changes Mar 9, 2023

View reviewed changes

zroubalik approved these changes Mar 9, 2023

View reviewed changes

zroubalik merged commit 846a4d0 into kedacore:main Mar 9, 2023

xoanmm pushed a commit to xoanmm/keda that referenced this pull request Mar 22, 2023

fix: move sasl and tls flag to ScaledObject instead of specifying…

e76fbd8

… in TriggerAuthentication (kedacore#4322) Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>

ianebot mentioned this pull request Jun 15, 2023

Kafka scaler with SASL plaintext auth mode fails with "error parsing kafka metadata: err SASL mode plaintext" #4689

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: move `sasl` and `tls` flag to ScaledObject instead of specifying in TriggerAuthentication #4322

fix: move `sasl` and `tls` flag to ScaledObject instead of specifying in TriggerAuthentication #4322

dttung2905 commented Mar 4, 2023 •

edited

Loading

dttung2905 commented Mar 5, 2023

JorTurFer commented Mar 6, 2023 •

edited by github-actions bot

Loading

JorTurFer left a comment

JorTurFer commented Mar 6, 2023 •

edited

Loading

tomkerkhove Mar 7, 2023

dttung2905 Mar 8, 2023

dttung2905 commented Mar 7, 2023

tomkerkhove commented Mar 7, 2023

zroubalik left a comment

dttung2905 commented Mar 7, 2023

zroubalik commented Mar 7, 2023

JorTurFer commented Mar 7, 2023

dttung2905 commented Mar 7, 2023

zroubalik commented Mar 7, 2023

JorTurFer commented Mar 7, 2023 •

edited

Loading

dttung2905 commented Mar 8, 2023 •

edited

Loading

tomkerkhove commented Mar 8, 2023

zroubalik commented Mar 8, 2023

tomkerkhove commented Mar 8, 2023

tomkerkhove commented Mar 8, 2023

zroubalik commented Mar 8, 2023 •

edited by github-actions bot

Loading

JorTurFer Mar 9, 2023 •

edited

Loading

zroubalik Mar 9, 2023

dttung2905 Mar 9, 2023

zroubalik Mar 9, 2023 •

edited

Loading

zroubalik Mar 9, 2023

JorTurFer Mar 9, 2023

dttung2905 Mar 9, 2023

JorTurFer Mar 9, 2023 •

edited

Loading

zroubalik left a comment

zroubalik commented Mar 9, 2023 •

edited by github-actions bot

Loading

		case config.TriggerMetadata["saslAuthType"] != "":
		saslAuthType = config.TriggerMetadata["saslAuthType"]

fix: move sasl and tls flag to ScaledObject instead of specifying in TriggerAuthentication #4322

fix: move sasl and tls flag to ScaledObject instead of specifying in TriggerAuthentication #4322

Conversation

dttung2905 commented Mar 4, 2023 • edited Loading

Checklist

dttung2905 commented Mar 5, 2023

JorTurFer commented Mar 6, 2023 • edited by github-actions bot Loading

JorTurFer left a comment

Choose a reason for hiding this comment

JorTurFer commented Mar 6, 2023 • edited Loading

tomkerkhove Mar 7, 2023

Choose a reason for hiding this comment

dttung2905 Mar 8, 2023

Choose a reason for hiding this comment

dttung2905 commented Mar 7, 2023

tomkerkhove commented Mar 7, 2023

zroubalik left a comment

Choose a reason for hiding this comment

dttung2905 commented Mar 7, 2023

zroubalik commented Mar 7, 2023

JorTurFer commented Mar 7, 2023

dttung2905 commented Mar 7, 2023

zroubalik commented Mar 7, 2023

JorTurFer commented Mar 7, 2023 • edited Loading

dttung2905 commented Mar 8, 2023 • edited Loading

tomkerkhove commented Mar 8, 2023

zroubalik commented Mar 8, 2023

tomkerkhove commented Mar 8, 2023

tomkerkhove commented Mar 8, 2023

zroubalik commented Mar 8, 2023 • edited by github-actions bot Loading

JorTurFer Mar 9, 2023 • edited Loading

Choose a reason for hiding this comment

zroubalik Mar 9, 2023

Choose a reason for hiding this comment

dttung2905 Mar 9, 2023

Choose a reason for hiding this comment

zroubalik Mar 9, 2023 • edited Loading

Choose a reason for hiding this comment

zroubalik Mar 9, 2023

Choose a reason for hiding this comment

JorTurFer Mar 9, 2023

Choose a reason for hiding this comment

dttung2905 Mar 9, 2023

Choose a reason for hiding this comment

JorTurFer Mar 9, 2023 • edited Loading

Choose a reason for hiding this comment

zroubalik left a comment

Choose a reason for hiding this comment

zroubalik commented Mar 9, 2023 • edited by github-actions bot Loading

fix: move `sasl` and `tls` flag to ScaledObject instead of specifying in TriggerAuthentication #4322

fix: move `sasl` and `tls` flag to ScaledObject instead of specifying in TriggerAuthentication #4322

dttung2905 commented Mar 4, 2023 •

edited

Loading

JorTurFer commented Mar 6, 2023 •

edited by github-actions bot

Loading

JorTurFer commented Mar 6, 2023 •

edited

Loading

JorTurFer commented Mar 7, 2023 •

edited

Loading

dttung2905 commented Mar 8, 2023 •

edited

Loading

zroubalik commented Mar 8, 2023 •

edited by github-actions bot

Loading

JorTurFer Mar 9, 2023 •

edited

Loading

zroubalik Mar 9, 2023 •

edited

Loading

JorTurFer Mar 9, 2023 •

edited

Loading

zroubalik commented Mar 9, 2023 •

edited by github-actions bot

Loading