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

dttung2905 · 2022-12-18T06:14:09Z

Move sasl and tls config to metadata section of TriggerAuthentication

Checklist

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)

Relates to #3611

Relates to #

dttung2905 · 2022-12-18T07:52:23Z

Hi team,
I think this MR introduce breaking changes. Could you guide me on what should I do in this case?
Should we introduce grace period with deprecation messages or its fine to leave the MR like that with notes in the breaking changes section in the next release. The blast radius of this MR is just Kafka Scaler so the latter may be okay?

dttung2905 · 2022-12-19T09:09:31Z

Hi @JorTurFer, thanks for approving my other MR so quickly, even during weekend 🙏 Could you help to guide me on this one as well ?

zroubalik · 2022-12-19T09:45:47Z

@dttung2905 I think that we should not move this config, but support specifying thsi option in both places. This way we maintain backwards compatibility.

There should be a check when this option is specified in both places (with different values), in this case we should fail.

WDYT @kedacore/keda-contributors ?

dttung2905 · 2022-12-19T09:56:41Z

@dttung2905 I think that we should not move this config, but support specifying thsi option in both places. This way we maintain backwards compatibility.

There should be a check when this option is specified in both places (with different values), in this case we should fail.

WDYT @\kedacore/keda-contributors ?

Thanks for the feedback. Yes I can make it backward compatible too. Am I right to say that we will support having tls and sasl in both places ( but not both at the same time ) in the long run ? This is just for my understanding when updating the doc in the other MR

dttung2905 · 2022-12-20T04:08:15Z

Hi @zroubalik , I have make changes to accommodate backward compatibility. Also a few checks I have implemented

A check to flag out when the same value of tls or sasl are set in both places
A check to flag out when tls and sasl are set in different places at the same time
A few extra sets of unit test to test out the above 2 points

Could you help to review it ?

zroubalik

Thanks for the addition, the only nit is the changelog. Let see what other think.

zroubalik · 2023-01-02T10:26:00Z

CHANGELOG.md

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

 ### Breaking Changes

- TODO
+- Kafka: Move `sasl` and `tls` config to `TriggerAuthentication` metadata ([#4024](https://github.com/kedacore/keda/pull/4024))


Is this really a Brekaing Change? I am not sure, @tomkerkhove @JorTurFer wdyt?

Not necessarily though. Before we decided to make it backward compatible, I classified it as breaking changes. I can move it out of this section

I don't think so, because it's backward compatible

Based on the doc changes it looks like a breaking change as we are removing fields/parameters which we already support

I'd say that docs are outdated because based on the code, the values are read from both places

So instead of a secret, isn't the real fix that we need a plain text option in a CRD instead of moving this to metadata?

Something like:

plainText: - parameter: tls value: true

Because it feels better to me to keep everything part of the CRD "body" rather than some in metadata and other in the payload/body.

@tomkerkhove Thanks so much for the suggestion. I'm really curious to know how does it look like with your suggestion in ScaledObject. 🤔 My guess is below ( pls cmiiw )

apiVersion: keda.sh/v1alpha1 kind: ScaledObject metadata: name: pulsar-scaledobject namespace: default spec: scaleTargetRef: name: pulsar-consumer pollingInterval: 30 plainText: - parameter: tls value: true

Hi team,
What do you think about Tom's suggestion?
I'm totally fine with both approaches but I think parking it under metadata would be slightly better in terms of consistency across other scalers ( take pulsar for example )

My suggestion is not for ScaledObject, but for Trigger Authentication instead where we already have multiple auth providers and this could be another one

Sorry for the delay, I understand what @tomkerkhove proposes here, I have been thinking about the same approach, though I am not 100% sure we should provide a plaintext in TriggerAuth. Users might start using this not ideal approach for storing sensitive data. Or am I too sensitive and we should let them do this? :)

@kedacore/keda-core-contributors do you have an opinion on the plainText field in TA?

zroubalik · 2023-01-02T10:26:56Z

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

pkg/scalers/kafka_scaler.go

JorTurFer · 2023-01-02T12:23:23Z

CHANGELOG.md

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

 ### Breaking Changes

- TODO
+- Kafka: Move `sasl` and `tls` config to `TriggerAuthentication` metadata ([#4024](https://github.com/kedacore/keda/pull/4024))


I don't think so, because it's backward compatible

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

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

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

tomkerkhove · 2023-02-28T16:17:18Z

As agreed on standup, the best location for this should be:

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

dttung2905 · 2023-03-04T15:19:33Z

Closing this MR in favour of MR #4322

dttung2905 requested a review from a team as a code owner December 18, 2022 06:14

zroubalik reviewed Jan 2, 2023

View reviewed changes

JorTurFer reviewed Jan 2, 2023

View reviewed changes

dttung2905 requested a review from JorTurFer January 2, 2023 15:42

dttung2905 added 7 commits January 3, 2023 11:48

Add Changes + fix unittest

1224e67

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

Add Changelog and remove unused var

3e5cfde

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

Make changes backward compatible and add a few more set of test cases

50fc205

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

dummy commit to trigger CI build

2de17be

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

Update ChangeLog and use helper function GetFromAuthOrMeta() to simpl…

04aca15

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

Add extra args into GetFromAuthOrMeta() helper function

e711a45

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

dummy commit to re-trigger CI build

9e03d79

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

dttung2905 force-pushed the fix-kafka-tls-sasl-metadata branch from a7ff0e3 to 9e03d79 Compare January 3, 2023 04:01

dttung2905 added 2 commits January 3, 2023 12:28

Fix GetFromAuthOrMeta() method

eb0c6c0

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

dummy commit to re-trigger CI build

4066d02

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

dttung2905 closed this Mar 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dttung2905 commented Dec 18, 2022 •

edited by tomkerkhove

Loading

dttung2905 commented Dec 18, 2022

dttung2905 commented Dec 19, 2022

zroubalik commented Dec 19, 2022

dttung2905 commented Dec 19, 2022 •

edited

Loading

dttung2905 commented Dec 20, 2022

zroubalik left a comment

zroubalik Jan 2, 2023

dttung2905 Jan 2, 2023

JorTurFer Jan 2, 2023

tomkerkhove Jan 2, 2023

JorTurFer Jan 2, 2023

tomkerkhove Jan 3, 2023

dttung2905 Jan 4, 2023

tomkerkhove Jan 4, 2023

zroubalik Jan 16, 2023

zroubalik Feb 21, 2023

zroubalik commented Jan 2, 2023 •

edited by github-actions bot

Loading

JorTurFer Jan 2, 2023

tomkerkhove commented Feb 28, 2023

dttung2905 commented Mar 4, 2023

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

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

Conversation

dttung2905 commented Dec 18, 2022 • edited by tomkerkhove Loading

Checklist

dttung2905 commented Dec 18, 2022

dttung2905 commented Dec 19, 2022

zroubalik commented Dec 19, 2022

dttung2905 commented Dec 19, 2022 • edited Loading

dttung2905 commented Dec 20, 2022

zroubalik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zroubalik commented Jan 2, 2023 • edited by github-actions bot Loading

Choose a reason for hiding this comment

tomkerkhove commented Feb 28, 2023

dttung2905 commented Mar 4, 2023

dttung2905 commented Dec 18, 2022 •

edited by tomkerkhove

Loading

dttung2905 commented Dec 19, 2022 •

edited

Loading

zroubalik commented Jan 2, 2023 •

edited by github-actions bot

Loading